[PATCH] shadow_copy2: Allow configurable prefix for snapshots

[Rajesh Joseph]

My bad, sent the wrong patch in my previous mail.

On Fri, May 6, 2016 at 7:33 PM, Rajesh Joseph <rjoseph at redhat.com> wrote:

>
>
> On Thu, May 5, 2016 at 11:16 PM, Jeremy Allison <jra at samba.org> wrote:
>
>> On Mon, Apr 18, 2016 at 07:48:12PM +0530, Rajesh Joseph wrote:
>> > Hi all,
>> >
>> > Current shadow_copy2 module provide a configurable time format
>> > (shadow:format) to specify snapshot names. But this seems little
>> > restrictive in nature where your snapshot name can only be a time
>> string.
>> >
>> > I updated shadow_copy2 module to take a new prefix option. With this now
>> > user can also provide a configurable prefix along with time format.
>> >
>> > Implementation detail:
>> > Let's say the underlying file-system creates snapshots in the following
>> > format
>> > <snap_name>_GMT-%Y.%m.%d-%H.%M.%S
>> >
>> > Windows expect snapshot names to be in the following format
>> > @GMT-%Y.%m.%d-%H.%M.%S
>> >
>> > VFS using shadow_copy2 module converts underlying file-system format  to
>> > the Windows format and vice-versa.
>> >
>> > Earlier with just a time and the shadow:format you can create a unique
>> > file-name and access that file. With the addition of prefix this is no
>> more
>> > valid. Now with given time we also need to find a unique file which will
>> > match the prefix.
>> >
>> > One way to address this is to scan the directory every time we access
>> > snapshot. Another way, which I have chosen, is to store the list of
>> > snapshot on the first scan and use this list for subsequent access.
>> >
>> > The implementation currently uses a global list to save the list of
>> > snapshot names so that we
>> > can get actual snapshot name with a given time-stamp. Currently I am
>> using
>> > VFS handle's private
>> > variable to store this list.
>> >
>> > This approach was already discussed with Michael Adam (obnox) and Ira
>> > Cooper. I got some internal review comments from Ira, which I addressed.
>> >
>> > Ira also raised a question on this approach and how well it can work
>> > durable handles and persistent handles. I am saving or creating the
>> > snapshot list in shadow_copy2_get_shadow_copy_data function. Ira
>> suspected
>> > that there is no guarantee shadow_copy2_get_shadow_copy_data function
>> will
>> > be called before shadow_copy2_get_real_filename, and other routines like
>> > it. Especially in case of a reconnect.
>> >
>> > Since I am not very well versed with this part of code, I like to have
>> > opinion of others here.
>> >
>> > Also review the patch and let me know if you have any comments.
>>
>> This:
>>
>> +static shadow_copy2_snapentry *shadow_copy2_create_snapentry(
>> +                                       struct shadow_copy2_private *priv)
>> +{
>> +       shadow_copy2_snapentry *tmpentry = NULL;
>> +
>> +       if (priv->snaplist == NULL) {
>> +               tmpentry = talloc_zero(priv, shadow_copy2_snapentry);
>> +               priv->snaplist = tmpentry;
>> +       } else {
>> +               tmpentry = talloc_zero(priv->snaplist,
>> +                                       shadow_copy2_snapentry);
>> +               DLIST_ADD(priv->snaplist, tmpentry);
>> +       }
>> +
>> +       return tmpentry;
>> +}
>> +
>> +/* This function sets a new snaplist and destroys the
>> + * previous list
>> + *
>> + * @param priv                 shadow_copy2 specific data strucuter
>> + * @param entry                Head of new snaplist
>> + */
>> +static void shadow_copy2_set_snaplist(struct shadow_copy2_private *priv,
>> +                                     shadow_copy2_snapentry *entry)
>> +{
>> +       if (priv->snaplist != NULL) {
>> +               TALLOC_FREE(priv->snaplist);
>> +       }
>> +
>> +       priv->snaplist = entry;
>> +}
>>
>> is going to leak memory. DLIST_ADD(head, entry) changes
>> the head pointer to entry. You are talloc'ing off the
>> previous head pointer (priv->snaplist) and then changing
>> it, so TALLOC_FREE(priv->snaplist) will only free the
>> last entry in the list, not the whole list.
>>
>> This is just a first glance, more as I look at it,
>> but you do need to rethink the memory allocation
>> here.
>>
>
>> The code should be written so that an empty priv->snaplist
>> pointer isn't treated any differently from an existing
>> one.
>>
>
> Thanks Jeremy for your valuable comments. I have fixed the memory
> issue and the snaplist pointer issue.
>
>
>>
>> Jeremy.
>>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-vfs-shadow_copy2-allow-configurable-prefix-for-snaps.patch
Type: text/x-patch
Size: 26697 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160506/e9981525/0001-vfs-shadow_copy2-allow-configurable-prefix-for-snaps.bin>