Latest Time Machine Patch

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Latest Time Machine Patch

Samba - samba-technical mailing list
Hello everyone,

Attached is the latest time machine patch with the Avahi support
contributed by Omri and rebased against master. This has been
stable without any warnings of corrupted backup volumes.

The only thing that has come up lately is potentially supporting
setting the avahi hostname rather than the current method of
using the netbios name so that it would show as lowercase rather
than all capitals. I personally don't have a strong opinion either
way but am open to suggestions.

Depending on the result of the previous question, what remains
left to be done to have this merged?

Thanks,
Kevin Anderson

latest-timemachine.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list

On Wed, Sep 13, 2017 at 01:13:22AM +0000, Kevin Anderson wrote:

> Hello everyone,
>
> Attached is the latest time machine patch with the Avahi support
> contributed by Omri and rebased against master. This has been
> stable without any warnings of corrupted backup volumes.
>
> The only thing that has come up lately is potentially supporting
> setting the avahi hostname rather than the current method of
> using the netbios name so that it would show as lowercase rather
> than all capitals. I personally don't have a strong opinion either
> way but am open to suggestions.
>
> Depending on the result of the previous question, what remains
> left to be done to have this merged?

generally looks good. I'm currently at SDC, but I promise to provide feedback
and review in the next days!

-slow

Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi Kevin,

On Wed, Sep 13, 2017 at 01:13:22AM +0000, Kevin Anderson wrote:
> Attached is the latest time machine patch with the Avahi support
> contributed by Omri and rebased against master. This has been
> stable without any warnings of corrupted backup volumes.

attached is a patchset that splits your version into smaller commits with some
minor changes in the error checking in the Avahi code.

I'm happy with this version and verified that it actually works.

What's missing is proper signed-off tags in the individual commits added by the
author. I can add the tags, I just need to know the author and the author must
state that he's happy with the commit as it is.

> The only thing that has come up lately is potentially supporting
> setting the avahi hostname rather than the current method of
> using the netbios name so that it would show as lowercase rather
> than all capitals. I personally don't have a strong opinion either
> way but am open to suggestions.

The uppercase netbios name is used anyway for the _smb._tcp record, isn't it?
Looks like we're stuck with this cruft...

Cheerio!
-slow

latest-timemachine.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
> What's missing is proper signed-off tags in the individual commits added by the
> author. I can add the tags, I just need to know the author and the author must
> state that he's happy with the commit as it is.

I did the Avahi stuff (number 3 in the patchset).
The original commits did have a sign-off, but it looks like Kevin only attached the diff.

>> The only thing that has come up lately is potentially supporting
>> setting the avahi hostname rather than the current method of
>> using the netbios name so that it would show as lowercase rather
>> than all capitals. I personally don't have a strong opinion either
>> way but am open to suggestions.
>
> The uppercase netbios name is used anyway for the _smb._tcp record, isn't it?
> Looks like we're stuck with this cruft…

The discussion was about both _smb._tcp and _adisk._tcp (their names should obviously match).
That can be done in a separate patch, pending further discussion, after this goes through.
Note that the dns_sd code (which is, I must reiterate, broken and doesn’t compile) passes NULL to DNSServiceRegister() for the name parameter, which sets it to use the default name (i.e. the hostname), instead of using the NetBIOS name (like Avahi).

Instead of choosing a default that can’t be modified, we could add a global option, say `multicast dns name` with a sensible default of either the NetBIOS name or the hostname, whichever makes more sense.
Netatalk added a similar option `zeroconf name` about a year ago, see https://sourceforge.net/p/netatalk/feature-requests/99/.

Thanks!
Omri
Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi Ralph,

>
> What's missing is proper signed-off tags in the individual commits added by the
> author. I can add the tags, I just need to know the author and the author must
> state that he's happy with the commit as it is.
>

The author of the other commits (except for 3 which is Omri) should be
myself and I am OK the changes. Sorry for missing the signed off tags.

Thanks,
Kevin Anderson

Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
>>> The only thing that has come up lately is potentially supporting
>>> setting the avahi hostname rather than the current method of
>>> using the netbios name so that it would show as lowercase rather
>>> than all capitals. I personally don't have a strong opinion either
>>> way but am open to suggestions.
>>
>> The uppercase netbios name is used anyway for the _smb._tcp record, isn't it?
>> Looks like we're stuck with this cruft…
>
> The discussion was about both _smb._tcp and _adisk._tcp (their names should obviously match).
> That can be done in a separate patch, pending further discussion, after this goes through.
> Note that the dns_sd code (which is, I must reiterate, broken and doesn’t compile) passes NULL to DNSServiceRegister() for the name parameter, which sets it to use the default name (i.e. the hostname), instead of using the NetBIOS name (like Avahi).
>
> Instead of choosing a default that can’t be modified, we could add a global option, say `multicast dns name` with a sensible default of either the NetBIOS name or the hostname, whichever makes more sense.
> Netatalk added a similar option `zeroconf name` about a year ago, see https://sourceforge.net/p/netatalk/feature-requests/99/.
>

So personally I agree with Omri that I think that could go in a
separate patch afterwards.

I need to look more in to it but could a call to
avahi_client_get_host_name() [1] be used to get the hostname to
publish on? If that does in fact work it would default to the system
hostname (and could be modified in the Avahi configuration based on
what the user wishes).


[1] https://avahi.org/doxygen/html/client_8h.html#a89378618c3c592a255551c308ba300bf

Thanks,
Kevin Anderson

Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
On Fri, Sep 15, 2017 at 12:52:05PM +0000, Kevin Anderson wrote:

> >>> The only thing that has come up lately is potentially supporting
> >>> setting the avahi hostname rather than the current method of
> >>> using the netbios name so that it would show as lowercase rather
> >>> than all capitals. I personally don't have a strong opinion either
> >>> way but am open to suggestions.
> >>
> >> The uppercase netbios name is used anyway for the _smb._tcp record, isn't it?
> >> Looks like we're stuck with this cruft…
> >
> > The discussion was about both _smb._tcp and _adisk._tcp (their names should obviously match).
> > That can be done in a separate patch, pending further discussion, after this goes through.
> > Note that the dns_sd code (which is, I must reiterate, broken and doesn’t compile) passes NULL to DNSServiceRegister() for the name parameter, which sets it to use the default name (i.e. the hostname), instead of using the NetBIOS name (like Avahi).
> >
> > Instead of choosing a default that can’t be modified, we could add a global option, say `multicast dns name` with a sensible default of either the NetBIOS name or the hostname, whichever makes more sense.
> > Netatalk added a similar option `zeroconf name` about a year ago, see https://sourceforge.net/p/netatalk/feature-requests/99/.
> >
>
> So personally I agree with Omri that I think that could go in a
> separate patch afterwards.
alrighty. So here's an updated patchset that adds the signed-off tags for both
of you and fixes the author in the avahi patch.

If you're happy with the patchset I'll ask for a second team member review and
ove we get that we can finally push this upstream. Thanks for your patience and
persistence on this! :)

Cheerio!
-slow

latest-timemachine.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
Hi Ralph,

>
> alrighty. So here's an updated patchset that adds the signed-off tags for both
> of you and fixes the author in the avahi patch.
>
> If you're happy with the patchset I'll ask for a second team member review and
> ove we get that we can finally push this upstream. Thanks for your patience and
> persistence on this! :)
>

I am happy with it with one exception. There is an error in the first
patch of the patchset:

+ config->time_machine = lp_parm_bool(
+ SNUM(handle->conn), FRUIT_PARAM_TYPE_NAME, "time machine", false);
+
  config->model = lp_parm_const_string(
  -1, FRUIT_PARAM_TYPE_NAME, "model", "MacSamba");

@@ -2122,6 +2126,7 @@ static NTSTATUS check_aapl(vfs_handle_struct *handle,
  DATA_BLOB blob = data_blob_talloc(req, NULL, 0);
  uint64_t req_bitmap, client_caps;
  uint64_t server_caps = SMB2_CRTCTX_AAPL_UNIX_BASED;
+ uint64_t volume_caps = 0;
  smb_ucs2_t *model;
  size_t modellen;

@@ -2206,6 +2211,10 @@ static NTSTATUS check_aapl(vfs_handle_struct *handle,
  break;
  }

+ if (config->time_machine) {
+ volume_caps |= SMB2_CRTCTX_AAPL_FULL_SYNC;
+ }
+
  SBVAL(p, 0, caps);



The "volume_caps" variable should be "caps". I double checked and it
was correct in my first email but I missed the error on the first
modification to split the patches in to smaller chunks so I apologize
for that. Besides that though it looks good to me.

I appreciate all of your help and guidance on this!

Thanks,
Kevin Anderson

Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
Hi Kevin,

On Fri, Sep 15, 2017 at 06:47:43PM +0000, Kevin Anderson wrote:
> The "volume_caps" variable should be "caps".

oops, my fault, sorry! Fixed in the attached patchset.

Fwiw, I removed forcing "strict sync = yes" as that will be the default setting
in 4.7.

-slow

latest-timemachine.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
Hi Ralph,

>
> oops, my fault, sorry! Fixed in the attached patchset.
>
> Fwiw, I removed forcing "strict sync = yes" as that will be the default setting
> in 4.7.
>
> -slow

That is acceptable to me.

So I reviewed the patch set completely and the final change I have is
to correct the documentation. For the time machine yes documentation,
it should read:

+ <listitem><para><command>yes</command> - Enables advertising
+ Time Machine support and the FULLSYNC volume capability to
+ clients. This is necessary for supporting Time Machine backups
+ from Mac OSX clients. This value advertises the capability
+ and with the options below, fsync's a file at the request of
+ the client.
+ </para></listitem>

Additionally I have attached a working patch that sets the avahi
service name to from the avahi client hostname. This shows the
hostname of the system in all lowercase rather than the uppercased
NETBIOS name. The patch has the signed off tag as well.

Thanks,
Kevin Anderson

0001-Set-the-Avahi-service-name-based-on-the-hostname.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
Hi folks,

On Sat, Sep 16, 2017 at 01:56:51AM +0000, Kevin Anderson wrote:

> So I reviewed the patch set completely and the final change I have is
> to correct the documentation. For the time machine yes documentation,
> it should read:
>
> + <listitem><para><command>yes</command> - Enables advertising
> + Time Machine support and the FULLSYNC volume capability to
> + clients. This is necessary for supporting Time Machine backups
> + from Mac OSX clients. This value advertises the capability
> + and with the options below, fsync's a file at the request of
> + the client.
> + </para></listitem>
I think we should simplify this and keep it to the minimum that describes the
feature, without going into technical and implementation details. So just
something like:

    Enables Time Machine support for this share. Also registers the share with
    mDNS in case Samba is built with mDNS support.

> Additionally I have attached a working patch that sets the avahi
> service name to from the avahi client hostname. This shows the
> hostname of the system in all lowercase rather than the uppercased
> NETBIOS name. The patch has the signed off tag as well.

We can't change the default here, we could add this as an option as suggested
before, but we should keep that discussion seperate from this thread.

Cheerio!
-slow

latest-timemachine-2017-09-17.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
Hi Ralph,

> I think we should simplify this and keep it to the minimum that describes the
> feature, without going into technical and implementation details. So just
> something like:
>
>     Enables Time Machine support for this share. Also registers the share with
>     mDNS in case Samba is built with mDNS support.

That is acceptable to me.


>> Additionally I have attached a working patch that sets the avahi
>> service name to from the avahi client hostname. This shows the
>> hostname of the system in all lowercase rather than the uppercased
>> NETBIOS name. The patch has the signed off tag as well.
>
> We can't change the default here, we could add this as an option as suggested
> before, but we should keep that discussion seperate from this thread.
>

OK. I can start a new thread for that.

The attached patch looks good to me.

Thanks,
Kevin Anderson

Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
>> The "volume_caps" variable should be "caps".
>
> oops, my fault, sorry! Fixed in the attached patchset.
>
> Fwiw, I removed forcing "strict sync = yes" as that will be the default setting
> in 4.7.

The F_FULLFSYNC extension "ensures that all data is flushed to stable storage”, according to the Apple documentation.
This means, to me, that it ought to be mutually exclusive with `strict sync = no`.
While `strict sync = yes` is the default, currently nothing stops someone from manually setting `strict sync = no` and `fruit: time machine` simultaneously.
I’d recommend adding a check to ensure that is the case, if we don’t want to force it.

Other than that, everything looks good.

Omri
Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
Hi everyone,

>
> The F_FULLFSYNC extension "ensures that all data is flushed to stable storage”, according to the Apple documentation.
> This means, to me, that it ought to be mutually exclusive with `strict sync = no`.
> While `strict sync = yes` is the default, currently nothing stops someone from manually setting `strict sync = no` and `fruit: time machine` simultaneously.
> I’d recommend adding a check to ensure that is the case, if we don’t want to force it.
>

I personally agree with a warning in case a user sets strict sync to
no. I have added the check and reattached the patch series. Can I have
a review for these?

(Reordering/breaking up the commits was the most "interesting" time I
have had with Git to date. I have verified that they match in terms of
authors, dates, and content but if I messed something up please let me
know)

Thanks,
Kevin Anderson

0001-vfs_fruit-Add-Time-Machine-support.patch (3K) Download Attachment
0002-docs-vfs_fruit-Add-Time-Machine-support.patch (2K) Download Attachment
0003-s3-smbd-register-TimeMachine-shares-with-avahi.patch (4K) Download Attachment