[libvirt] [PATCH 8/9] util: storagefile: Add helpers to check presence of backing store

[Eric Blake]

On 10/12/2017 02:07 PM, Peter Krempa wrote:
> Add a helper that will retrun true if a virStorageSource has backing

s/retrun/return/

> store. This will also aid in refactoring of the code further down.
> ---

The commit message mentions adding only one helper, but doesn't name it.
 I like to mention new interfaces in commit messages, if only to make
grepping 'git log' for a particular interface more likely to hit the
relevant commits.

> +++ b/src/conf/domain_conf.c
> @@ -26607,7 +26607,7 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
>          }
>      }
> 
> -    for (tmp = disk->src; tmp; tmp = tmp->backingStore) {
> +    for (tmp = disk->src; virStorageSourceIsBacking(tmp); tmp = tmp->backingStore) {

Here, you are adding a use of virStorageSourceIsBacking,

>          /* execute the callback only for local storage */
>          if (virStorageSourceIsLocalStorage(tmp) &&
>              tmp->path) {
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 7c373e781..f808cd291 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -1169,7 +1169,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>          if (virStorageSize(unit, capacity, &ret->target.capacity) < 0)
>              goto error;
>      } else if (!(flags & VIR_VOL_XML_PARSE_NO_CAPACITY) &&
> -               !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) && ret->target.backingStore)) {
> +               !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) &&
> +                 virStorageSourceHasBacking(&ret->target))) {

and here, virStorageSourceHasBacking.  So my immediate thought was
whether the 'Is' vs. 'Has' naming intentional?  If so, you added two
interfaces, so the commit message needs a tweak.

> +++ b/src/libvirt_private.syms
> @@ -2692,7 +2692,9 @@ virStorageSourceFindByNodeName;
>  virStorageSourceFree;
>  virStorageSourceGetActualType;
>  virStorageSourceGetSecurityLabelDef;
> +virStorageSourceHasBacking;
>  virStorageSourceInitChainElement;
> +virStorageSourceIsBacking;

So you indeed are adding two interfaces, but it took me this far into
the review to know it.

One trick with git is that you can tell it to produce patches in a
particular order (see qemu's scripts/git.orderfile, for example); this
can be useful to hoist headers and build instructions first, showing the
new interfaces, prior to the .c files that change to use the new
interfaces, for a slightly faster review.  But even with a preference of
.syms before .h before .c, I still find it helpful to do one-off
orderfile overrides; in this case, listing virstoragefile.c before other
.c files would also aid review.

> +++ b/src/qemu/qemu_cgroup.c
> @@ -160,7 +160,7 @@ qemuTeardownDiskCgroup(virDomainObjPtr vm,
>  {
>      virStorageSourcePtr next;
> 
> -    for (next = disk->src; next; next = next->backingStore) {
> +    for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) {

Line longer than 80 columns; do we care?

> +++ b/src/util/virstoragefile.c
> @@ -1292,7 +1292,7 @@ virStorageFileChainGetBroken(virStorageSourcePtr chain,
>      if (!chain)
>          return 0;
> 
> -    for (tmp = chain; tmp; tmp = tmp->backingStore) {
> +    for (tmp = chain; virStorageSourceIsBacking(tmp); tmp = tmp->backingStore) {
>          /* Break when we hit end of chain; report error if we detected
>           * a missing backing file, infinite loop, or other error */
>          if (!tmp->backingStore && tmp->backingStoreRaw) {
> @@ -1566,6 +1566,33 @@ virStorageFileParseChainIndex(const char *diskTarget,
>      return ret;
>  }
> 
> +
> +/**
> + * virStorageSourceIsBacking:
> + * @src: storage source
> + *
> + * Returns true if @src is a eligible backing store structure. Useful
> + * for iterators.
> + */
> +bool
> +virStorageSourceIsBacking(const virStorageSource *src)
> +{
> +    return !!src;
> +}
> +
> +/**
> + * virStorageSourceHasBacking:
> + * @src: storage source
> + *
> + * Returns true if @src has backing store/chain.
> + */
> +bool
> +virStorageSourceHasBacking(const virStorageSource *src)
> +{
> +    return virStorageSourceIsBacking(src) && src->backingStore;
> +}

The conversions look sane, and now you have an entry point for future
patches to tweak what constitutes a valid reference, without having to
hunt down all the iterations changed in this patch.

Looks like a reasonable refactoring, so in spite of my comments, I'm
fine with the code, and it is just the commit message that can be
improved, and/or the use of an order file to present things in an
altered order for ease of review.

Reviewed-by: Eric Blake <eblake at redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171012/f038742b/attachment-0001.sig>