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