Fwd: (from a long time ago) Shadow Copy Improvements

[James Peach]

Begin forwarded message:
> From: "Ed Plese" <ed at edplese.com>
> Date: 5 April 2008 12:04:05 AM
> To: jpeach at samba.org
> Subject: Re: (from a long time ago) Shadow Copy Improvements
>
> Hi James,
>
> Thanks for reviewing the patches many months ago.  I apologize for the
> huge delay in responding to this.  I noticed there's been some work
> done on a shadow_copy2 module so I don't know if there is even much of
> a need for this anymore.  There are some additional features in this
> module as well as backwards compatibility (except with sorting - see
> below) with the current shadow_copy module so it might still be
> useful.
>
> I made a git repository available with the patches at:
>
> http://www.edplese.com/git/samba.git
>
> I'm not too familiar with publishing git repositories but I think I
> have it setup properly.  If you'd prefer patches I can get you those
> instead.
>
>
>>> 1-dirent-fix.patch
>>> This fixes the module to work on systems that define struct
>>> dirent with
>>> d_name[1].  Solaris is an example of such a system and it causes the
>>> share to appear to be completely empty.
>>
>> This looks OK. I would prefer that you pad out the dirent structures
>> to guarantee a safe alignment, 8 or 16 bytes.
>>
>
> They are now aligned at 8 bytes and can be changed to 16 bytes in a  
> #define.
>
>
>>> 2-sort.patch
>>> With the existing shadow_copy module the shadow copies are
>>> displayed in
>>> the Windows GUI in the order the server obtains them from readdir
>>> (3).
>>> On some systems and filesystems readdir(3) does not return files
>>> in any
>>> particular order which leads to the list in the GUI being
>>> unsorted and
>>> difficult to use.  This patch allows the list to be sorted based on
>>> a "sort" parameter specified for the module.  Allowed values are
>>> "asc"
>>> or "desc".  When not specified the current unsorted behavior is
>>
>> This looks fine too, though I think we should sort by default.
>
> Now sorts in descending order by default.  This will show the most
> frequent changes at the top of the list which seems most logical.
>
>
>>> 3-paths.patch
>>
>> This needs a copyright attribution.
>
> Added.
>
>
>> In shadow_copy_opendir(), you are still using shadow_copy_match_name
>> to filter out shadow directories. It looks like this won't work
>> correctly with if you specify the format parameter.
>
> shadow_copy_match_name() as well as the last 75% of
> shadow_copy_opendir() are there only to preserve the existing behavior
> of the module.  It's there to filter out the @GMT directories that the
> administrator would have created that contain the snapshots.  The
> updates to this module are there to prevent the need to create these
> directories (or symlinks) in the first place so when using the new
> "format" parameter there probably won't be any of these directories to
> filter out anyways.
>
> All of the filtering code does almost the same thing as the "hide  
> files"
> smb.conf parameter.  The only difference is that the filtering code is
> automatic and the "@GMT" directories don't even show up as hidden  
> files
> as they would with "hide files".  If "hide files" is an acceptable
> replacement for the current functionality then  
> shadow_copy_match_name()
> can be eliminated along with large portions of shadow_copy_opendir().
>
>
>
>> You should be able to cache the prefix created in
>> shadow_copy_file_to_snapshot_path since it's quite expensive to
>> create. Attaching this to handle->data should be sufficient.
>
> The result of shadow_copy_file_to_snapshot_path() can't be cached
> because the client can request files from different snapshots.
> The MS Shadow Copy Client in particular it will stat the desired file
> in every shadow copy (snapshot) that is return by
> shadow_copy_get_shadow_copy_data().  This causes
> shadow_copy_file_to_snapshot_path() to return many different results
> throughout the lifetime of the connection.
>
> For normal (i.e. non-shadow copy) use, this funtion returns fairly
> quickly and the code path amounts to (only functions included):
>
> alloc_sub_basic(get_current_username(), .., lp_parm_const_string());
> loop over the string, replacing $ with %;
> memset();
> strptime();
> strncpy();
> SAFE_FREE();
>
> To speed this up further a check similar to shadow_copy_match_name()  
> can
> first be performed on the path.  If the path doesn't contain "@GMT-"
> anywhere in it then just strncpy() the path as-is and return.  If you
> think this would help then I can make the change.
>
>
>>> path - Specifies the path to the directory that contains the
>>> snapshots.
>>>
>>> format - This is the format of the snapshot names in str[fp]time
>>> notation
>>>     except that $ characters are used in place of % characters.
>>
>> Hmmm. We already use %$FOO to expand environment variables. Is it
>> possible for this to get unexpected results?
>
> I didn't realize those substitutions were available.  The "$"
> replacement is done after the normal substitution so the "$" in "% 
> $FOO"
> wouldn't be messed with.  I think it is possible to come up with  
> values
> that wouldn't be interpreted properly but I can't think of any that
> would come up in normal use.
>
> It may be clearer to use a different character ("#" possibly) which
> doesn't already have another meaning.  "%%" is another possibility  
> which
> Alison and I discussed but "%%" is currently broken (or was last year)
> and double escapes
> can be confusing.
>
> I'll switch it to "#" for now to avoid the double use of "$".
>
>
>> Can you elaborate on the need for both a "path" and a "subpath"
>> parameter? Why are they both necessary?
>>
>
> 'path' and 'subpath' are equivalent to 'snapdir' and 'basedir' (I
> think) in shadow_copy2.  'path' is the directory that contains the
> snapshots and 'subpath' is the relative path of the share within the
> filesystem.  'subpath' is necessary when multiple shares are contained
> in a single filesystem (e.g. /home).
>
> For example if /home is a filesystem and /home/ed is a share then the
> 'subpath' for this share 'ed'.
>
> It'd probably be beneficial to change the parameter names to match
> those in shadow_copy2.
>
>
>>> * shadow copy does not work correctly when mapping a drive to a
>>> subdirectory of a share
>>
>> Could you add a comment noting this. A future enhancement might be to
>> check that the share is the root of a mounted filesystem when the
>> module loads.
>
> Noted in the comments.
>
> To clarify the limitation, this is only affects situations where you
> would do from Windows:
>
> C:\>net use X: \\server\share\subdirectory
>
> In this case, shadow copy is broken for X: and all of it's
> subdirectories.
>
> If the drive is mapped directly to the share then the shadow copy
> functionality in the subdirectory works fine.  Likewise, if browsing  
> the
> share in Windows Explorer, shadow copy works fine in any  
> subdirectories
> of the share.
>
>
> Let me know if there is still any interest in this and if any changes
> are needed for it to be accepted.
>
> Thanks,
>
> Ed Plese