[PATCH] shadow_copy2: Allow configurable prefix for snapshots
Ira Cooper
ira at wakeful.net
Mon May 9 06:46:49 UTC 2016
Uri Simchoni <uri at samba.org> writes:
> I haven't gone through the patch line-by-line yet but if Jeremy's
> commenting, I have comments too:
>
> 1. Most of the config->x --> priv->config->x changes can be avoided by
> adding a "config" local variable set to priv->config. This both makes
> the patch smaller (and easier to review!), and in most if not all of the
> cases represents (IMHO) better coding style.
I had him do that due to bugs I hit in similar code. Jeremy warned me,
he ok'd it.. and then we both missed a crash bug ;). That's why he did
it that way. Blame me. The original code was you suggest.
> 2. The patch seems to require that the client first call
> get_shadow_copy_data_fn, because that's where the snapentries get
> populated. I think there's no such assumption in the code prior to the
> patch. The unit tests go directly to the snapshots without listing them
> first. Not sure what to make of this, perhaps we can start that way and
> add lazy-population of the list if need arises.
Maybe we should just lazy populate? :) One code path beats two!
Cheers,
-Ira
More information about the samba-technical
mailing list