Latest Time Machine Patch

classic Classic list List threaded Threaded
24 messages Options
12
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
Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
> 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><0002-docs-vfs_fruit-Add-Time-Machine-support.patch><0003-s3-smbd-register-TimeMachine-shares-with-avahi.patch>

A user on GitHub brought up that on macOS High Sierra, which is due to be released quite soon, the new file system (APFS) doesn’t support Time Machine over AFP (https://support.apple.com/en-gb/HT208018).
It would thus be great to have the Time Machine over SMB support in Samba ready to go for the next release.

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
On Wed, Sep 20, 2017 at 12:20:57AM +0000, Kevin Anderson wrote:
> 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?

reviewed by me. Can I have a second team member review please? Thanks!

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
On Wed, Sep 27, 2017 at 11:28:23AM +0200, Ralph Böhme wrote:
> On Wed, Sep 20, 2017 at 12:20:57AM +0000, Kevin Anderson wrote:
> > 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?
>
> reviewed by me. Can I have a second team member review please? Thanks!

Sorry, NAK on the avahi patch.

+                               adisk = avahi_string_list_add_printf(
+                                           adisk, "dk%d=adVN=%s,adVF=0x82",
+                                           dk++, lp_const_servicename(snum));

and

+                       adisk = avahi_string_list_add(adisk, "sys=adVF=0x100");

have no return NULL check for allocation fail. Add those checks
and it looks better.

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
This was discussed previously. Did you read the warning I put in the top of the file?

/*
 * Avahi aborts on allocation failure (OOM),
 * unless a custom allocator which doesn't do so has been set.
 *
 * This is particularly important for the avahi_string_list_*() functions,
 * which return NULL on allocation failure.
 * Since it should abort on allocation failure (before returning NULL),
 * we don't check the result.
 */

Adding in a check for NULL doesn’t matter, as it aborts long before we would see that NULL.
In fact, the documentation doesn’t even state that it /can/ return NULL—I only found that out by reading the Avahi source.

Omri

> On Sep 29, 2017, at 19:09, Jeremy Allison <[hidden email]> wrote:
>
> On Wed, Sep 27, 2017 at 11:28:23AM +0200, Ralph Böhme wrote:
>> On Wed, Sep 20, 2017 at 12:20:57AM +0000, Kevin Anderson wrote:
>>> 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?
>>
>> reviewed by me. Can I have a second team member review please? Thanks!
>
> Sorry, NAK on the avahi patch.
>
> +                               adisk = avahi_string_list_add_printf(
> +                                           adisk, "dk%d=adVN=%s,adVF=0x82",
> +                                           dk++, lp_const_servicename(snum));
>
> and
>
> +                       adisk = avahi_string_list_add(adisk, "sys=adVF=0x100");
>
> have no return NULL check for allocation fail. Add those checks
> and it looks better.
>
> Jeremy.


Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
On Fri, Sep 29, 2017 at 07:21:46PM -0500, Omri Mor wrote:

> This was discussed previously. Did you read the warning I put in the top of the file?
>
> /*
>  * Avahi aborts on allocation failure (OOM),
>  * unless a custom allocator which doesn't do so has been set.
>  *
>  * This is particularly important for the avahi_string_list_*() functions,
>  * which return NULL on allocation failure.
>  * Since it should abort on allocation failure (before returning NULL),
>  * we don't check the result.
>  */
>
> Adding in a check for NULL doesn’t matter, as it aborts long before we would see that NULL.
> In fact, the documentation doesn’t even state that it /can/ return NULL—I only found that out by reading the Avahi source.

Yes, I read the source to determine it can return NULL,
that's why I wanted the checks here.

I must have missed the previous discussion on this, 'cos
I'd never have agreed to ignore NULL checks on allocation
return. Just because the current allocator aborts doesn't
mean the newest-shiny-updated one also will.

Please add the explicit NULL checks, otherwise
we're going to get coverity false positives.

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: Latest Time Machine Patch

Samba - samba-technical mailing list
Hi Jeremy/Ralph,
    I have attached a new patch
(0001-s3-smbd-register-Time-Machine-shares-with-Avahi.patch) from Omri
which adds the NULL checks and a TALLOC allocator. The remaining
patches have not been modified. I have tested the Avahi change and
things appear to function as expected. Can I have a review on these
changes?

Thanks,
Kevin Anderson

On Fri, Sep 29, 2017 at 8:27 PM, Jeremy Allison <[hidden email]> wrote:

> On Fri, Sep 29, 2017 at 07:21:46PM -0500, Omri Mor wrote:
>> This was discussed previously. Did you read the warning I put in the top of the file?
>>
>> /*
>>  * Avahi aborts on allocation failure (OOM),
>>  * unless a custom allocator which doesn't do so has been set.
>>  *
>>  * This is particularly important for the avahi_string_list_*() functions,
>>  * which return NULL on allocation failure.
>>  * Since it should abort on allocation failure (before returning NULL),
>>  * we don't check the result.
>>  */
>>
>> Adding in a check for NULL doesn’t matter, as it aborts long before we would see that NULL.
>> In fact, the documentation doesn’t even state that it /can/ return NULL—I only found that out by reading the Avahi source.
>
> Yes, I read the source to determine it can return NULL,
> that's why I wanted the checks here.
>
> I must have missed the previous discussion on this, 'cos
> I'd never have agreed to ignore NULL checks on allocation
> return. Just because the current allocator aborts doesn't
> mean the newest-shiny-updated one also will.
>
> Please add the explicit NULL checks, otherwise
> we're going to get coverity false positives.
>
> Jeremy.

0001-s3-smbd-register-Time-Machine-shares-with-Avahi.patch (6K) Download Attachment
0002-docs-vfs_fruit-Add-Time-Machine-support.patch (2K) Download Attachment
0001-vfs_fruit-Add-Time-Machine-support.patch (3K) Download Attachment
12