[PATCH] shadow_copy2: Allow configurable prefix for snapshots

Michael Adam obnox at samba.org
Fri Jul 8 10:55:11 UTC 2016 

On 2016-06-28 at 21:01 +0530, Rajesh Joseph wrote:
> Hey guys,
> 
> Sorry for this late update on this thread. Due to vacation and other
> priorities this left unattended.
> 
> As Uri recommended I added test cases to cover shadow:format. Also
> addressed review comments from others.
> 
> Please go through the patch and let me know if it looks OK or not.
> 
> Thanks & Regards,
> Rajesh

Hi Rajesh,

I am finally finding the time to go through this carefully.

First off: Great work, thanks!

No first a few general comments / requests:

- I am a big fan of as-atomic-as possible patches.
  So I will propose to split this into several patches,
  which will make each single change much more obvious
  and easier to understand.

- It is great that you added tests! If I got it right,
  the tests partly test existing code (the format option)
  and partly test new code (the prefix / delim options).

  So I porpose to add the tests that test the old code
  first, as a seprarate patch. Your addition of the prefix
  option then just amounts in adding more share defs and test
  calls to this.

- There are a few preparatory changes to the shadow-copy
  module that I could see as preparatory patches:
  The introduction of the indirection of the private
  struct which contains the config struct. I'd suggest
  to add that in a prep patch and add content to and use
  of the other parts of the priv struct in later patches.

- Also I don't agree with the request to NOT use a config
  helper variable. Like Uri, I request you to use a config
  helper variable to keep the diff as small as possible.
  I think the risk that Ira is pointing out here is
  extremely low, because it is only in the connect method
  that this is created. If creation of private and
  private->config fails, other code paths will not be taken.

- I don't tink that shadow_copy2_posix_gmt_string() should be
  changed from returning size_t to ssize_t. While returning 0
  from strftime does not necessarily mean an error, all callers
  treat ret <= 0 as the exit condition and don't treat < 0
  separately. So there is no extra value here.

  If you do insist on changing the return type, I suggest
  you do it in a separate preparatory patch before doing the
  actual changes.

- I see that you have addressed Jeremy's comments by
  adding a mem_ctx member to the private struct.
  I am with Jeremy here in that I find this a little
  artifical and would probably prefer a free-function
  that walks the list. But this is a minor comment.

So, these were my more superficial comments.
I will follow up with more detailed comments after
looking at the changes more.

Inline below find a few comments on typos and wordings
in the manpage change:


> From f5eac520150a7408cf0de5f1052954df2faeb3b2 Mon Sep 17 00:00:00 2001
> From: Rajesh Joseph <rjoseph at redhat.com>
> Date: Tue, 28 Jun 2016 14:56:44 +0000
> Subject: [PATCH] vfs/shadow_copy2: allow configurable prefix for snapshot name
> 
> shadow_copy2 module provides configurable format for time via
> shadow:format option in smb.conf. Currently there is no way
> to specify a variable name along with time.
> 
> As part of this change added a new option (shadow:snapprefix) in
> shadow_copy2 config. This option will accept regular expression as input.
> e.g.
> shadow:snapprefix = [a-z]*[0-9]
> 
> When this option is provided then shadow:format option should always
> start with <delimiter> string. This delimiter is configurable via a new option,
> i.e. shadow:delimiter. Default value for this is "_GMT",
> e.g. _GMT-%Y.%m.%d-%H.%M.%S
> 
> Signed-off-by: Rajesh Joseph <rjoseph at redhat.com>
> ---
>  docs-xml/manpages/vfs_shadow_copy2.8.xml |  26 ++
>  selftest/target/Samba3.pm                |  57 ++++
>  source3/modules/vfs_shadow_copy2.c       | 488 ++++++++++++++++++++++++-------
>  source3/script/tests/test_shadow_copy.sh |  41 +++
>  4 files changed, 510 insertions(+), 102 deletions(-)
> 
> diff --git a/docs-xml/manpages/vfs_shadow_copy2.8.xml b/docs-xml/manpages/vfs_shadow_copy2.8.xml
> index fbc0651..b960636 100644
> --- a/docs-xml/manpages/vfs_shadow_copy2.8.xml
> +++ b/docs-xml/manpages/vfs_shadow_copy2.8.xml
> @@ -406,6 +406,32 @@
>                  </listitem>
>                  </varlistentry>
>  
> +		<varlistentry>
> +		<term>shadow:snapprefix
> +		</term>
> +		<listitem>
> +		<para>
> +		With this optional parameter, one can specify a prefix to
> +		snapshot name. This parameter works in combination with

            + the


> +		<command>shadow:format</command> parameter. Both these

           +the

> +		paramters together specifies the snapshot name in the file

                                   specify

> +		system. The option only supports Basic Regular Expression (BRE).
> +		</para>
> +		</listitem>
> +		</varlistentry>

Maybe be a little more elaborate here, e.g.:

  With this optional parameter, one can specify a variable prefix
  component for names of the snapshot directories in the file system.
  If this parameter is set, together with the shadow:format and
  shadow:delimiter parameters it determines the possible names of
  snapshot directories in the file system.


> +		<varlistentry>
> +		<term>shadow:delimiter
> +		</term>
> +		<listitem>
> +		<para>
> +		This optional parameter is used as a delimiter between
> +		<command>shadow:snapprefix</command> and <command>shadow:format
> +		</command>.

           ^^ remove the space? i.e.  <command>shadow:format</command>.


Mention that this is no used when snapprefix is not set?

It seems that the intention is that the delimiter
value is always a leading (fixed) part of the snapdir:format
string. Is that required? Or can I put a delim in between
the prefix and the format string? ... This needs clarification.

> +		</para>
> +		<para>Default: shadow:delimiter = "_GMT"</para>
> +		</listitem>
> +		</varlistentry>
>  		</variablelist>
>  </refsect1>


More later...

Cheers - Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160708/e204b94e/signature.sig>

More information about the samba-technical mailing list