[PATCH v2 0/3]: Snapper support

[Christof Schmitt]

On Fri, Jun 27, 2014 at 01:54:49PM -0700, Jeremy Allison wrote:
> On Fri, Jun 27, 2014 at 03:31:34PM +0200, David Disseldorp wrote:
> > Snapper[1] is a snapshot management tool for Linux, that offers support
> > for the creation and deletion of snapshots on btrfs and ext4
> > thin-provisioned LVM volumes.
> > 
> > This patch-set adds and documents a new vfs_snapper module, which
> > exposes snapshots managed by snapper to Windows clients, via the
> > FSCTL_SRV_ENUM_SNAPS/FSCTL_GET_SHADOW_COPY_DATA ioctls and the
> > @GMT/TWrp path tokens.
> > 
> > Demonstration video: http://youtu.be/RZq84ONWu1I
> > 
> > Changes since v1 (Thanks Jeremy):
> > - Added missing previous file versions patch
> > - Removed unused snapper_snap_path_to_id()
> 
> LGTM, pushed.
> 
> Things to clean up in a future patch (i.e.
> these things bugged me, but not enough to
> ask for another go around :-):
> 
> +static void snapper_conf_array_free(int32_t num_confs,
> +                                   struct snapper_conf *confs)
> +{
> +       int i;
> +
> +       for (i = 0; i < num_confs; i++) {
> +               talloc_free(confs[i].attrs);
> +       }
> +       talloc_free(confs);
> +}
> 
> confs[i].attrs should be a talloc child of confs.
> 
> and
> 
> +static void snapper_snap_array_free(int32_t num_snaps,
> +                                   struct snapper_snap *snaps)
> +{
> +       int i;
> +
> +       for (i = 0; i < num_snaps; i++) {
> +               talloc_free(snaps[i].user_data);
> +       }
> +       talloc_free(snaps);
> +}
> 
> snaps[i].user_data should be a talloc child of snaps.
> 
> That way, the free functions just become talloc_free(parent).
> 
> Plus I think you should look at the changes just
> pushed to master by Volker and Christof, as I think
> they may also be relevent in this module too.

We should try sharing the common code between the vfs_snapper and
vfs_shadow_copy2 module. 

Another minor comment would be that source3/include/smb.h already
defines GMT_FORMAT, no need to define it again in the vfs module.

Christof