[libvirt] [PATCH 3/6] conf: report error on chain lookup failure

[John Ferlan]

On 04/11/2014 12:21 AM, Eric Blake wrote:
> The chain lookup function was inconsistent on whether it left
> a message in the log when looking up a name that is not found
> on the chain (leaving a message for OOM or if name was
> relative but not part of the chain), and could litter the log
> even when successful (when name was relative but deep in the
> chain, use of virFindBackingFile early in the chain would complain
> about a file not found).  It's easier to make the function
> consistently emit a message exactly once on failure, and to let
> all callers rely on the clean semantics.
> 
> * src/util/virstoragefile.c (virStorageFileChainLookup): Always
> report error on failure.  Simplify relative lookups.
> * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Avoid
> overwriting error.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/qemu/qemu_driver.c    | 10 +---------
>  src/util/virstoragefile.c | 17 +++++++++--------
>  2 files changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b0c6a74..b8cfe27 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15346,9 +15346,6 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base,
>                                                         disk->src.path,
>                                                         top, &top_meta,
>                                                         &top_parent))) {
> -        virReportError(VIR_ERR_INVALID_ARG,
> -                       _("could not find top '%s' in chain for '%s'"),
> -                       top, path);
>          goto endjob;
>      }
>      if (!top_meta || !top_meta->backingStore) {
> @@ -15360,13 +15357,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base,
>      if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) {
>          base_canon = top_meta->backingStore;
>      } else if (!(base_canon = virStorageFileChainLookup(top_meta, top_canon,
> -                                                        base, NULL, NULL))) {
> -        virReportError(VIR_ERR_INVALID_ARG,
> -                       _("could not find base '%s' below '%s' in chain "
> -                         "for '%s'"),
> -                       base ? base : "(default)", top_canon, path);
> +                                                        base, NULL, NULL)))
>          goto endjob;
> -    }

Does removal of {} violate the one line "rule of thumb"... In either
case the "if" portion should be made consistent as well as this now has

if (xxx) {
    oneline
} else
    onelonglinebecauseofarguments

Probably would be nice to have an extra line after endjob; and before
following comment - just a readability thing for me at least.

>      /* Note that this code exploits the fact that
>       * virStorageFileChainLookup guarantees a simple pointer
>       * comparison will work, rather than needing full-blown STREQ.  */
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 2013914..66a6ab1 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1537,7 +1537,8 @@ int virStorageFileGetSCSIKey(const char *path,
>   * backing element is not a file).  If PARENT is not NULL, set *PARENT
>   * to the preferred name of the parent (or to NULL if NAME matches
>   * START).  Since the results point within CHAIN, they must not be
> - * independently freed.  */
> + * independently freed.  Reports an error and returns NULL if NAME is
> + * not found.  */
>  const char *
>  virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start,
>                            const char *name, virStorageFileMetadataPtr *meta,
> @@ -1570,15 +1571,12 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start,
>                     STREQ(name, owner->backingStore)) {
>              break;
>          } else if (virStorageIsFile(owner->backingStore)) {
> -            char *absName = NULL;
> -            if (virFindBackingFile(owner->directory, name,
> -                                   NULL, &absName) < 0)
> +            int result = virFileRelLinkPointsTo(owner->directory, name,
> +                                                owner->backingStore);
> +            if (result < 0)
>                  goto error;
> -            if (absName && STREQ(absName, owner->backingStore)) {
> -                VIR_FREE(absName);
> +            if (result > 0)
>                  break;
> -            }
> -            VIR_FREE(absName);
>          }
>          *parent = owner->backingStore;
>          owner = owner->backingMeta;
> @@ -1590,6 +1588,9 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start,
>      return owner->backingStore;
> 
>   error:
> +    virReportError(VIR_ERR_INVALID_ARG,
> +                   _("could not find image '%s' in chain for '%s'"),
> +                   name, start);

Looking at the context of the code expanded out a bit... there's a few
checks for !name = can we get here with name == NULL?  Looking ahead to
patch 4 this will be more important...

>      *parent = NULL;
>      if (meta)
>          *meta = NULL;
> 

ACK - seems reasonable, just address the possible NULL name...

John