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

Ralph Böhme slow at samba.org
Sun Jul 23 16:26:01 UTC 2017


On Sat, Jul 22, 2017 at 09:59:43PM -0700, Omri Mor wrote:
> > On Jul 22, 2017, at 01:46, Ralph Böhme <slow at samba.org> wrote:
> >> The sys record can be static though.
> >> 
> >> Samba has, apparently, a concept of service name and volume label; I’m not
> >> sure which to use.  lp_servicename(), lp_volume(), or volume_label() (which
> >> uses the volume if it exists, else the service name).
> > 
> > lp_servicename should do it.
> 
> I’ve used lp_const_servicename(), which seems to do the trick.
> 
> The Avahi portion is below—please tell me if there are any glaring errors or
> coding guideline violations.

Generally looks good, some nitpicks, see below.

> dns_register_smbd_fde_handler() appears to be bugged (badly) as well.  When
> the DNSServiceProcessResult() call succeeds, talloc_free() is called on the
> state struct, which in turn calls the destructor, which deregisters the
> service.  If the call fails, then the entire service is reinitialized.  I
> suspect the intent was: on success, use tevent_add_fd() to wait for the next
> response on failure, deregister and free the resources

If you really want to open this can of worms, I suggest you do this in a
seperate thread.

> If my understanding is correct, then dns_sd has been broken since 2009, when
> it was changed to be event-driven.  I suspect that the only platform using the
> dns_sd interface is macOS; since Apple ships its own SMB server, there isn’t
> much of a Samba userbase on the platform.  Samba isn’t even available in
> Homebrew, while the versions in MacPorts are ancient and unmaintained, as are
> those in Fink.  In short: is there even anyone using the dns_sd interface that
> would suffer from its removal?

FreeBSD ports has a bunch of patches:
<https://svnweb.freebsd.org/ports/head/net/samba46/files/>

The Samba 4.6 package in FreeBSD 11 doesn't seem to use neither Avahi nor
dns-sd.

I'm for keeping it as is and just adding the TM registration to Avahi and dns-sd
if possible.

> 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.

>  	switch (status) {
>  	case AVAHI_CLIENT_S_RUNNING:
> @@ -97,6 +102,29 @@ static void avahi_client_callback(AvahiClient *c, AvahiClientState status,
>  			state->entry_group = NULL;
>  			break;
>  		}
> +		adisk = avahi_string_list_add(adisk, "sys=waMa=0,adVF=0x100");

Can avahi_string_list_add() fail and return NULL?

> +		num_services = lp_numservices();
> +		dk = 0;

Please add an empty line here.

> +		for (snum = 0; snum < num_services; snum++) {
> +			if (lp_snum_ok(snum) &&
> +			    lp_parm_bool(snum, "fruit", "time machine", false))
> +			{
> +				adisk = avahi_string_list_add_printf(adisk,
> +					    "dk%d=adVN=%s,adVF=0x82",
> +					    dk++, lp_const_servicename(snum));

Can avahi_string_list_add_printf() fail and return NULL?

> +			}
> +		}
> +		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...
        }
}

> +			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.

> +			avahi_entry_group_free(state->entry_group);
> +			state->entry_group = NULL;
> +			break;
> +		}
>  		if (avahi_entry_group_commit(state->entry_group) < 0) {
>  			error = avahi_client_errno(c);
>  			DEBUG(10, ("avahi_entry_group_commit failed: "
> @@ -138,6 +166,7 @@ static void avahi_client_callback(AvahiClient *c, AvahiClientState status,
>  			   "AVAHI_CLIENT_CONNECTING\n"));

Same here: please split function call from if error check.

>  		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() ?

-slow



More information about the samba-technical mailing list