[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Santize the reporting of VIR_ERR_INVALID_ERROR



On Fri, May 25, 2012 at 05:35:20PM -0600, Eric Blake wrote:
> On 05/25/2012 11:44 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange redhat com>
> > 
> > To ensure consistent error reporting of invalid arguments,
> > provide a number of predefined helper methods & macros.
> > 
> >  - An arg which must not be NULL:
> > 
> >    virCheckNonNullArgReturn(argname, retvalue)
> >    virCheckNonNullArgGoto(argname, label)
> 
> ... Looks useful.  I'll review the macros first.

[snip]

> ACK with problems fixed.  If you post a v2, post an interdiff (I don't
> want to scroll through the whole thing again :)

Here is what I propose to amend before pushing

diff --git a/src/internal.h b/src/internal.h
index 825b0fe..1b1598b 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -232,7 +232,7 @@
     do {                                                                \
         unsigned long __unsuppflags = flags & ~(supported);             \
         if (__unsuppflags) {                                            \
-            virReportInvalidArg("flags",                                \
+            virReportInvalidArg(flags,                                  \
                                 _("unsupported flags (0x%lx) in function %s"), \
                                 __unsuppflags, __FUNCTION__);           \
             return retval;                                              \
@@ -242,51 +242,51 @@
 # define virCheckNonNullArgReturn(argname, retval)  \
     do {                                            \
         if (argname == NULL) {                      \
-            virReportInvalidNullArg(#argname);      \
+            virReportInvalidNullArg(argname);       \
             return retval;                          \
         }                                           \
     } while (0)
 # define virCheckNullArgGoto(argname, label)        \
     do {                                            \
         if (argname == NULL) {                      \
-            virReportInvalidNullArg(#argname);      \
+            virReportInvalidNullArg(argname);       \
             goto label;                             \
         }                                           \
     } while (0)
 # define virCheckNonNullArgGoto(argname, label)     \
     do {                                            \
         if (argname == NULL) {                      \
-            virReportInvalidNonNullArg(#argname);   \
+            virReportInvalidNonNullArg(argname);    \
             goto label;                             \
         }                                           \
     } while (0)
 # define virCheckPositiveArgGoto(argname, label)    \
     do {                                            \
         if (argname <= 0) {                         \
-            virReportInvalidPositiveArg(#argname);  \
+            virReportInvalidPositiveArg(argname);   \
             goto label;                             \
         }                                           \
     } while (0)
 # define virCheckNonZeroArgGoto(argname, label)     \
     do {                                            \
         if (argname == 0) {                         \
-            virReportInvalidNonZeroArg(#argname);   \
+            virReportInvalidNonZeroArg(argname);    \
             goto label;                             \
         }                                           \
     } while (0)
 # define virCheckZeroArgGoto(argname, label)        \
     do {                                            \
         if (argname != 0) {                         \
-            virReportInvalidNonZeroArg(#argname);   \
+            virReportInvalidNonZeroArg(argname);    \
             goto label;                             \
         }                                           \
     } while (0)
-# define virCheckNonNegativeArgGoto(argname, label) \
-    do {                                            \
-        if (argname < 0) {                          \
-            virReportInvalidNonNegativeArg(#argname);   \
-            goto label;                             \
-        }                                           \
+# define virCheckNonNegativeArgGoto(argname, label)     \
+    do {                                                \
+        if (argname < 0) {                              \
+            virReportInvalidNonNegativeArg(argname);    \
+            goto label;                                 \
+        }                                               \
     } while (0)
 
 
diff --git a/src/libvirt.c b/src/libvirt.c
index d52557e..2fc85f0 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -4918,16 +4918,16 @@ virDomainMigratePeer2Peer (virDomainPtr domain,
     }
 
     if (!tempuri->server) {
-        virReportInvalidArg(tempuri,
-                            _("server field in tempuri in %s must not be NULL"),
+        virReportInvalidArg(dconnuri,
+                            _("unable to parser server from dconnuri in %s"),
                             __FUNCTION__);
         virDispatchError(domain->conn);
         virURIFree(tempuri);
         return -1;
     }
     if (STRPREFIX(tempuri->server, "localhost")) {
-        virReportInvalidArg(tempuri,
-                            _("server field in tempuri in %s must not be 'localhost'"),
+        virReportInvalidArg(dconnuri,
+                            _("unable to parser server from dconnuri in %s"),
                             __FUNCTION__);
         virDispatchError(domain->conn);
         virURIFree(tempuri);
@@ -7041,7 +7041,7 @@ virDomainBlockStats(virDomainPtr dom, const char *disk,
     virCheckNonNullArgGoto(stats, error);
     if (size > sizeof(stats2)) {
         virReportInvalidArg(size,
-                            _("size in %s must be less than %zu"),
+                            _("size in %s must not exceed %zu"),
                             __FUNCTION__, sizeof(stats2));
         goto error;
     }
@@ -7186,7 +7186,7 @@ virDomainInterfaceStats (virDomainPtr dom, const char *path,
     virCheckNonNullArgGoto(stats, error);
     if (size > sizeof(stats2)) {
         virReportInvalidArg(size,
-                            _("size in %s must be less than %zu"),
+                            _("size in %s must not exceed %zu"),
                             __FUNCTION__, sizeof(stats2));
         goto error;
     }
@@ -14150,7 +14150,7 @@ virSecretGetUUID(virSecretPtr secret, unsigned char *uuid)
     return 0;
 
 error:
-    virDispatchError(NULL);
+    virDispatchError(secret->conn);
     return -1;
 }
 
@@ -16844,21 +16844,21 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
     if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) &&
         !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) {
         virReportInvalidArg(flags,
-                            _("use of current flag in %s requires redefine flag"),
+                            _("use of 'current' flag in %s requires 'redefine' flag"),
                             __FUNCTION__);
         goto error;
     }
     if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) &&
         (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
         virReportInvalidArg(flags,
-                            _("redefine and no metadata flags in %s are mutually exclusive"),
+                            _("'redefine' and 'no metadata' flags in %s are mutually exclusive"),
                             __FUNCTION__);
         goto error;
     }
     if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) &&
         (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) {
         virReportInvalidArg(flags,
-                            _("redefine and halt flags in %s are mutually exclusive"),
+                            _("'redefine' and 'halt' flags in %s are mutually exclusive"),
                             __FUNCTION__);
         goto error;
     }
@@ -18406,11 +18406,15 @@ int virDomainGetCPUStats(virDomainPtr domain,
      * ncpus must be non-zero unless params == NULL
      * nparams * ncpus must not overflow (RPC may restrict it even more)
      */
-    if (start_cpu < -1 && ncpus != -1) {
-        virReportInvalidArg(start_cpu,
-                            _("start_cpu in %s must be -1 or greater if ncpus is not -1"),
-                            __FUNCTION__);
-        goto error;
+    if (start_cpu == -1) {
+        if (ncpus != 1) {
+            virReportInvalidArg(start_cpu,
+                                _("ncpus in %s must be 1 when start_cpu is -1"),
+                                __FUNCTION__);
+            goto error;
+        }
+    } else {
+        virCheckNonNegativeArgGoto(start_cpu, error);
     }
     if (nparams)
         virCheckNonNullArgGoto(params, error);


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]