On Thu, Jun 04, 2015 at 07:02:00 -0400, John Ferlan wrote:
On 05/29/2015 09:33 AM, Peter Krempa wrote:
virDomainLiveConfigHelperMethod that is used for this job now does modify the flags but still requires the callers to extract the correct definition objects.
In addition coverity and other static analyzers are usually unhappy as they don't grasp the fact that @flags are upadted according to the correct def to be present.
To work this issue around and simplify the calling chain let's add a new helper that will work only on drivers that always copy the persistent def to a transient at start of a vm. This will allow to drop a few arguments. The new function syntax will also fill two definition pointers rather than modifying the @flags parameter. --- src/conf/domain_conf.c | 113 ++++++++++++++++++++++++++++++++++++++--------- src/conf/domain_conf.h | 8 ++++ src/libvirt_private.syms | 2 + 3 files changed, 102 insertions(+), 21 deletions(-)
Well Coverity is most unhappy with these changes - causes 14 new issues - all related... I didn't get a chance to run your series through my checker because not all of the patches made it through even though they were in the archive (strange).
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4a8c1b5..e8bda73 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
...
+ +/** + * virDomainObjGetDefs: + * + * @vm: domain object + * @flags: for virDomainModificationImpact + * @liveDef: Set to the pointer to the live definition of @vm. + * @persDef: Set to the pointer to the config definition of @vm. + * + * Helper function to resolve @flags and retrieve correct domain pointer + * objects. This function should be used only when the hypervisor driver always + * creates vm->newDef once the vm is started. (qemu driver does that) + * + * If @liveDef or @persDef are set it implies that @flags request modification + * of thereof. + * + * Returns 0 on success and sets @liveDef and @persDef; -1 if @flags are + * inappropriate. + */ +int +virDomainObjGetDefs(virDomainObjPtr vm, + unsigned int flags, + virDomainDefPtr *liveDef, + virDomainDefPtr *persDef) +{ + if (liveDef) + *liveDef = NULL; + + if (*persDef) + *persDef = NULL;
You dereference "*persDef" here without checking and furthermore this causes Coverity to complain about UNINIT usage in each of the callers since non initialized their def to NULL.a
[1]
+ + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + return -1; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (liveDef) + *liveDef = vm->def; + + if (persDef) + *liveDef = vm->newDef;
here you check for persDef, but adjust *liveDef - so if liveDef == NULL, then there's going to be a failure...
+ } else { + if (persDef) + *persDef = vm->def;
Also persDef is checked again...
+ } + + return 0; }
+ /* * The caller must hold a lock on the driver owning 'doms', * and must also have locked 'dom', to ensure no one else diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 34b669d..262d267 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2545,6 +2545,14 @@ virDomainObjGetPersistentDef(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, virDomainObjPtr domain);
+int virDomainObjUpdateModificationImpact(virDomainObjPtr vm, + unsigned int *flags); + +int virDomainObjGetDefs(virDomainObjPtr vm, + unsigned int flags, + virDomainDefPtr *liveDef, + virDomainDefPtr *persDef);
How about a "ATTRIBUTE_NONNULL(4)" here?
Actually, the first check has an extra dereference that should not be there. The function expects that NULL is passed here.
John
I'd paste a diff here that worked for me, except that I know you prefer not to have inlined diffs like that in email... So I'll wait to see how you fix this...
The change is to remove the deref at [1]. Peter