Re: [PATCHv3 2/2] daemon: allow more than one host address given via --listen
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 30, 2010, 07:28 UTC
- Message-ID
- <7vwrr8ftjj.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1283094796-27984-3-git-send-email-alexander@sulfrian.net>
Alexander Sulfrian <alexander@sulfrian.net> writes:
Show 23 quoted lines
> @@ -861,11 +862,20 @@ static int setup_named_sock(char *listen_addr, int listen_port, int **socklist_p
>
> #endif
>
> -static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
> +static int socksetup(struct string_list *listen_addr, int listen_port, int **socklist_p)
> {
> int socknum = 0, *socklist = NULL;
>
> - socknum = setup_named_sock(listen_addr, listen_port, &socklist, socknum);
> + if (!listen_addr->nr)
> + socknum = setup_named_sock(NULL, listen_port, &socklist,
> + socknum);
> + else {
> + int i;
> + for (i = 0; i < listen_addr->nr; i++)
> + socknum = setup_named_sock(listen_addr->items[i].string,
> + listen_port, &socklist,
> + socknum);
> + }
>
> *socklist_p = socklist;
> return socknum;Giving an old number and returning a new number feels a bit awkward as an API. If you create a structure that consists of a <pointer, nr, alloc> tuple that is suitable for ALLOC_GROW() API and pass that around instead of <&socklist, socknum> pair, then the helper can return how many sockets it prepared, and signal a failure with a negative value, no?
Show 10 quoted lines
> +static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
> {
> int socknum, *socklist;
>
> socknum = socksetup(listen_addr, listen_port, &socklist);
> if (socknum == 0)
> - die("unable to allocate any listen sockets on host %s port %u",
> - listen_addr, listen_port);
> + die("unable to allocate any listen sockets on port %u",
> + listen_port);The old code accepted only one --listen, so it was clear when no socket can be prepared for a given name (which may expand to multiple addresses), it is clear what failed (i.e. the failing input could have been only _one_ from the user's point of view). This check diagnoses the case where socket preparation failed for all names. Don't we want to have a new check inside the iteration over names in socksetup() to warn when socket preparation for all addresses for a name fails?
Also doesn't setup_named_sock() as refactored die() when one of the names given to --listen results no socket under ipv6 build but keeps going under noipv6 build? Without multi-listen, the distinction did not matter exactly because it took only one name, but your multi-listen addition exposes this inconsistency. I think the helper should be modified not to die but just return "I didn't prepare any socket for the given name", to allow the outer loop to continue on to the next name (perhaps while issuing a warning e.g. "No listening socket resulted for '%s'".