[PATCH] shadow_copy2: Allow configurable prefix for snapshots

Jeremy Allison jra at samba.org
Thu May 5 17:46:24 UTC 2016


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.

Jeremy.



More information about the samba-technical mailing list