[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