vfs_fruit: Time Machine/FULLSYNC: add mDNS/DNS-SD advertisement

[Omri Mor]

>> diff --git a/source3/smbd/avahi_register.c b/source3/smbd/avahi_register.c
>> index 368168d..3e8d639 100644
>> --- a/source3/smbd/avahi_register.c
>> +++ b/source3/smbd/avahi_register.c
>> @@ -24,6 +24,7 @@
>> #include <avahi-client/client.h>
>> #include <avahi-client/publish.h>
>> #include <avahi-common/error.h>
>> +#include <avahi-common/strlst.h>
>> 
>> struct avahi_state_struct {
>> 	struct AvahiPoll *poll;
>> @@ -73,6 +74,10 @@ static void avahi_client_callback(AvahiClient *c, AvahiClientState status,
>> 	struct avahi_state_struct *state = talloc_get_type_abort(
>> 		userdata, struct avahi_state_struct);
>> 	int error;
>> +	int num_services;
>> +	int snum;
>> +	int dk;
>> +	AvahiStringList *adisk = NULL;
> 
> Please move the variables down into the switch block where they're used.

Where should they go? At the beginning of 'case AVAHI_CLIENT_S_RUNNING’?
It *can’t* be the first line of the case, as it *must* be a statement (a declaration is not a statement).
Currently that is DEBUG(10, ("avahi_client_callback: AVAHI_CLIENT_S_RUNNING\n"));
They can be declared under that, but it seemed like Samba generally uses C90-like declarations, where declarations needed to come before any code.

The variables could be declared under the switch, and then assigned in the case—is that what you meant?
However, any assignment done there is skipped (switch jumps directly into the case statement).

switch (status) {
	int num_services;
	int snum;
	int dk;
	AvahiStringList *adisk = NULL; /* assignment gets skipped, which can be dangerous */
case AVAHI_CLIENT_S_RUNNING:
	/* do stuff */
	break;
case AVAHI_CLIENT_FAILURE:
	/* do stuff */
	break;
…
}

If the entire case is enclosed in a new scope, then the variables can be declared without any problems.

switch (status) {
case AVAHI_CLIENT_S_RUNNING: {
	int num_services;
	int snum;
	int dk;
	AvahiStringList *adisk = NULL;

	/* do stuff */
	break;
}
case AVAHI_CLIENT_FAILURE:
	/* do stuff */
	break;
…
}

> Can avahi_string_list_add() fail and return NULL?
> Can avahi_string_list_add_printf() fail and return NULL?

The documentation doesn’t say; however, the Avahi source code reveals that it can.
I’ll add in checks.

>> +			}
>> +		}
>> +		if (dk > 0 && avahi_entry_group_add_service_strlst(
>> +			    state->entry_group, AVAHI_IF_UNSPEC,
>> +			    AVAHI_PROTO_UNSPEC, 0, lp_netbios_name(),
>> +			    "_adisk._tcp", NULL, NULL, 0, adisk) < 0) {
> 
> Please split this into:
> 
> int ret;
> 
> ...
> 
> if (dk > 0) {
>        ret = avahi_entry_group_add_service_strlst(...);
>        if (ret < 0) {
> 	        ...error handling...
>        }
> }

I was following the same style as the code above used:

if (avahi_entry_group_add_service(
	    state->entry_group, AVAHI_IF_UNSPEC,
	    AVAHI_PROTO_UNSPEC, 0, lp_netbios_name(),
	    "_smb._tcp", NULL, NULL, state->port, NULL) < 0) {
	error = avahi_client_errno(c);
	DEBUG(10, ("avahi_entry_group_add_service failed: "
		   "%s\n", avahi_strerror(error)));
	avahi_entry_group_free(state->entry_group);
	state->entry_group = NULL;
	break;
}

Should I change that as well?

> 
>> +			error = avahi_client_errno(c);
>> +			DEBUG(10, ("avahi_entry_group_add_service failed: "
>> +				   "%s\n", avahi_strerror(error)));
> 
> Please use the modern DEBUG macros, DBG_DEBUG in this case.

Same as above; the (older) code above used DEBUG, so I just copied it.
Easy fix :).

>> 		break;
>> 	}
>> +	avahi_string_list_free(adisk);
> 
> If possible, please move this to closest place where the resource can be freed,
> probably after avahi_entry_group_add_service_strlst(). Or is it needed in
> avahi_entry_group_commit() ?

I wanted to have only one place for the cleanup code (otherwise it would need to be replicated for every error path).
With a bit of refactoring it shouldn’t be difficult to move it closer though.

Thanks for the input!
Omri