Re: [libvirt] [PATCH v2 3/6] logical: Search for a segtype of "thin" and mark lv as sparse

On Thu, Jan 28, 2016 at 05:44:06PM -0500, John Ferlan wrote: > For any "thin" lv's, mark the volume as a sparse volume so that the > volume wipe algorithm doesn't work. Currently thin lv's are ignored > because the regex requires 1 or more 'devices' listed in order to > process. However, a future patch will be changing this. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/storage/storage_backend_logical.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c > index 3232c08..a9c6309 100644 > --- a/src/storage/storage_backend_logical.c > +++ b/src/storage/storage_backend_logical.c > @@ -64,6 +64,8 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, > } > > > +#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_THIN "thin" > + > struct virStorageBackendLogicalPoolVolData { > virStoragePoolObjPtr pool; > virStorageVolDefPtr vol; > @@ -201,12 +203,15 @@ virStorageBackendLogicalMakeVol(char **const groups, > } > > /* Mark the (s) sparse/snapshot lv, e.g. the lv created using > - * the --virtualsize/-V option. We've already ignored the (t)hin > + * the --virtualsize/-V option or a thin segtype as sparse. This > + * will make sure the volume wipe algorithm doesn't overwrite > + * a sparse/thin volumes. We've already ignored the (t)hin > * pool definition. In the manner libvirt defines these, the > * thin pool is hidden to the lvs output, except as the name > * in brackets [] described for the groups[1] (backingStore). > */ > - if (attrs[0] == 's') > + if (attrs[0] == 's' || > + STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_THIN)) > vol->target.sparse = true; Well, I'm not sure about this code. Based on my research, the 's' means snapshot volume and there is another flag 'V' which is thin volume. The comment doesn't seems to be correct. By using --virtualsize/-V it will create snapshot or thin volume based on 'sparse_segtype_default' configuration option from /etc/lvm/lvm.conf. It would be nice to clarify this in the comment for future review. One more thing, why don't use || attrs[0] == 'V' ?