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

[Eric Blake]

On 04/11/2014 01:12 PM, John Ferlan wrote:
> 
> 
> 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.
>>

>>          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"...

Eww, I did indeed break the double {} rule.


>>   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...

Indeed, callers can pass NULL; I'll improve it.

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

Pushing with this added:


diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index b8cfe27..fed7d1c 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -15354,11 +15354,12 @@ qemuDomainBlockCommit(virDomainPtr dom, const
char *path, const char *base,
                        top_canon, path);
         goto endjob;
     }
-    if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) {
+    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)))
+    else if (!(base_canon = virStorageFileChainLookup(top_meta, top_canon,
+                                                      base, NULL, NULL)))
         goto endjob;
+
     /* Note that this code exploits the fact that
      * virStorageFileChainLookup guarantees a simple pointer
      * comparison will work, rather than needing full-blown STREQ.  */
diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c
index 66a6ab1..77cc878 100644
--- i/src/util/virstoragefile.c
+++ w/src/util/virstoragefile.c
@@ -1588,9 +1588,14 @@
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);
+    if (name)
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("could not find image '%s' in chain for '%s'"),
+                       name, start);
+    else
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("could not find base image in chain for '%s'"),
+                       start);
     *parent = NULL;
     if (meta)
         *meta = NULL;


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

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