[PATCH] shadow_copy2: Allow configurable prefix for snapshots

Uri Simchoni uri at samba.org
Wed Jul 20 05:31:28 UTC 2016


Regarding 6/6 (the doc) - can you add a default for snapprefix, and an
example how the whole thing works together (a set of snapshot folders,
the RE, delimiter, and format, and which ones get picked up).

So to summarize:
1-3,5 - RB+ me
4 - all those pecky little comments from previous email
6 - asking for some more details.

Thanks,
Uri.

On 07/20/2016 01:31 AM, Uri Simchoni wrote:
> On 07/14/2016 10:12 AM, Rajesh Joseph wrote:
>> On Tue, Jul 12, 2016 at 7:56 PM, Michael Adam <obnox at samba.org> wrote:
>>
>>> Update:
>>>
>>> Had a longer discussion with Rajesh.
>>> The overall logic looks good to me.
>>> Rajesh will follow up with an updated
>>> patchset for (final?) review.
>>>
>>> Cheers- Michael
>>>
>>
>> I have split the patch into smaller ones and addressed Michael's comments.
>>
>> Thanks & Regards,
>> Rajesh
>>
> 
> Sorry for taking so long to respond, been busy with stuff but that's no
> excuse.
> 
> I've gone through the first 4 patches (still need to review the tests
> and the manpage). The splitting of the patch set into refactoring and
> truly new code made reviewing much simpler. Good work! Some comments follow.
> 
> General comment 1:
> It would help if the commit message (and possibly the man page) would
> explain why this is useful - why are we adding non-trivial functionality
> that we have to maintain later. What snapshot system would benefit from
> that.
> 
> General comment 2:
> This module already has a mix of old-style and new-style debug
> statements (DBG_XXX vs DEBUG(xx, ()) - My fault I guess... But since
> there are already new-style statements, let's use them in new code, esp.
> in new functions.
> 
> In [4/6]:
> 
> Indentation of struct shadow_copy2_snapentry and struct
> shadow_copy2_snaplist_info, struct shadow_copy2_private - haven't seen
> any reference to it in a style guide so I'm going to be careful, but I
> think we use a single space between the type and the variable.
> 
> 
>> +static ssize_t shadow_copy2_saved_snapname(struct shadow_copy2_private *priv,
>> +                                         struct tm *timestamp,
>> +                                         char *snap_str, size_t len)
>> +{
>> +       ssize_t snaptime_len = -1;
>> +       struct shadow_copy2_snapentry *entry = NULL;
>> +
>> +       snaptime_len = strftime(snap_str, len, GMT_FORMAT, timestamp);
>> +       if (snaptime_len == 0) {
>> +               DEBUG(10, ("strftime failed\n"));
>> +               return -1;
>> +       }
> Should be DBG_ERR, we don't expect this to happen.
> 
>> +static bool shadow_copy2_update_snaplist(struct vfs_handle_struct *handle,
>> +               time_t snap_time)
>> +{
>> +       int ret = -1;
>> +       bool snaplist_updated = false;
>> +       struct files_struct fsp = {0};
>> +       struct smb_filename smb_fname = {0};
>> +       double seconds = 0.0;
>> +       struct shadow_copy2_private *priv = NULL;
>> +
>> +       SMB_VFS_HANDLE_GET_DATA(handle, priv, struct shadow_copy2_private,
>> +                               return false);
>> +
>> +       seconds = difftime(snap_time, priv->snaps->fetch_time);
> I'm suspicious towards use of double in networking/storage-related code.
> In essence time_t can be trivially compared. We have some more opaque
> stuff in lib/util/time.h, but that would require changing to timeval or
> timespec, or converting on-the fly. difftime() is hardly used in the
> project.
> 
> 
>> +       if (seconds > 0 || (priv->snaps->snaplist == NULL)) {
>> +               smb_fname.base_name = ".";
>> +               fsp.fsp_name = &smb_fname;
>> +
>> +               ret = shadow_copy2_get_shadow_copy_data(handle, &fsp,
>> +                                                       NULL, false);
>> +               if (ret == 0) {
>> +                       snaplist_updated = true;
>> +               } else {
>> +                       DEBUG(0,("Failed to get shadow copy data\n"));
>> +               }
>> +
>> +       }
> The assumption that "." is the share's root made me a little edgy.
> That's not always the case. After thinking long and hard, at least in
> current code base, for this code path, I think it's safe to assume so
> (because the pattern where we're not in the share's root is where we CD
> into a directory and do some operation on "." or on a file within the
> dir. In that case the pathname does not include the @GMT marker and
> passes through this module, even if we're now inside a snapshot dir).
> 
> We really need a more generic reliable way to retrieve the original
> connect path, but until we do, can you at least put in a comment of why
> it's safe, so that other module writers don't willy-nilly use this as a
> reference to make that assumption?
> 
>> @@ -1360,12 +1528,38 @@ static bool shadow_copy2_snapshot_to_gmt(vfs_handle_struct *handle,
>>  
>>         fmt = config->gmt_format;
>>  
>> +       /* If regex is provided then we will have to parse the
>> +        * filename which will contain both the prefix and the time format.
>> +        * e.g. <prefix><delimiter><time_format>*/
>> +       if (priv->snaps->regex != NULL) {
>> +               tmpstr = talloc_strdup(NULL, name);
> We prefer talloc_tos() as talloc context, even if we take care to free
> the memory.
> 
>>         if (config->use_sscanf) {
>>                 if (sscanf(name, fmt, &timestamp_long) != 1) {
>>                         DEBUG(10, ("shadow_copy2_snapshot_to_gmt: "
>>                                    "no sscanf match %s: %s\n",
>>                                    fmt, name));
>> +                       talloc_free(tmpstr);
>>                         return false;
>>                 }
>>                 timestamp_t = timestamp_long;
>> @@ -1375,6 +1569,7 @@ static bool shadow_copy2_snapshot_to_gmt(vfs_handle_struct *handle,
>>                         DEBUG(10, ("shadow_copy2_snapshot_to_gmt: "
>>                                    "no match %s: %s\n",
>>                                    fmt, name));
>> +                       talloc_free(tmpstr);
>>                         return false;
>>                 }
>>                 DEBUG(10, ("shadow_copy2_snapshot_to_gmt: match %s: %s\n",
>> @@ -1388,6 +1583,7 @@ static bool shadow_copy2_snapshot_to_gmt(vfs_handle_struct *handle,
>>         }
>>  
>>         strftime(gmt, gmt_len, GMT_FORMAT, &timestamp);
>> +       talloc_free(tmpstr);
> Instead of all those talloc_free(), goto the end and do one
> TALLOC_FREE() - that's the common style. There's much more TALLOC_FREE()
> than talloc_free().
> 
>> @@ -1485,8 +1684,22 @@ static int shadow_copy2_get_shadow_copy_data(
>>                 return -1;
>>         }
>>  
>> -       shadow_copy2_data->num_volumes = 0;
>> -       shadow_copy2_data->labels      = NULL;
>> +       if (shadow_copy2_data != NULL) {
>> +               shadow_copy2_data->num_volumes = 0;
>> +               shadow_copy2_data->labels      = NULL;
>> +       }
>> +
>> +       SMB_VFS_HANDLE_GET_DATA(handle, priv, struct shadow_copy2_private,
>> +                               return -1);
>> +
>> +       if ((priv->snaps->regex != NULL) && (labels || shadow_copy2_data == NULL)) {
> Why does "labels" affect the decision to refresh the snapshot list?
> Why isn't it sufficient to look at regex as a condition for refreshing
> the snapshot list? The shadow_copy2_data == NULL condition is confusing
> - the meaning of this parameter is "this is a buffer for shadow copy
> data". Adding meaning of "if it's NULL then refresh snapshot list
> regardless of labels" is confusing.
> 
>> @@ -1507,6 +1720,21 @@ static int shadow_copy2_get_shadow_copy_data(
>>                 DEBUG(6,("shadow_copy2_get_shadow_copy_data: %s -> %s\n",
>>                          d->d_name, snapshot));
>>  
>> +               if (get_snaplist) {
>> +                       /* Create a snap entry for each successful
>> +                        * pattern match */
>> +                       tmpentry = shadow_copy2_create_snapentry(priv);
>> +                       if (tmpentry == NULL) {
>> +                               DEBUG(0, ("talloc_zero() failed\n"));
>> +                               return -1;
>> +                       }
> tmp_ctx needs to be freed here (refactor with goto or add it).
> 
>> @@ -2117,6 +2354,37 @@ static int shadow_copy2_connect(struct vfs_handle_struct *handle,
>>                 return -1;
>>         }
>>  
>> +       snapprefix = lp_parm_const_string(SNUM(handle->conn),
>> +                                      "shadow", "snapprefix",
>> +                                      NULL);
>> +       if (snapprefix != NULL) {
>> +               priv->snaps->regex = talloc_zero(priv->snaps, regex_t);
>> +               if (priv->snaps->regex == NULL) {
>> +                       DEBUG(0, ("talloc_zero() failed\n"));
>> +                       errno = ENOMEM;
>> +                       return -1;
>> +               }
>> +
>> +               /* pre-compute regex rule for matching pattern later */
>> +               ret = regcomp(priv->snaps->regex, snapprefix, 0);
>> +               if (ret) {
>> +                       DEBUG(0, ("Failed to create regex object\n"));
>> +                       return -1;
>> +               }
>> +       }
>> +
>> +       delimiter = lp_parm_const_string(SNUM(handle->conn),
>> +                                      "shadow", "delimiter",
>> +                                      "_GMT");
>> +       if (delimiter != NULL) {
> What if delimiter *is* NULL? this is an error condition.
> 
> Thanks,
> Uri.
> 
> 




More information about the samba-technical mailing list