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

Re: [libvirt] [PATCH 04/24] maint: improve error condition style in public API



On 01/02/2014 11:16 AM, John Ferlan wrote:
> 
> 
> On 12/28/2013 11:11 AM, Eric Blake wrote:
>> While auditing error messages in libvirt.c, I found a couple
>> instances that had not been converted to modern error styles,
>> and a few places that failed to dispatch the error through
>> the known-good connection.
>>
>> * src/libvirt.c (virDomainPinEmulator, virDomainGetDiskErrors)
>> (virDomainSendKey, virDomainGetSecurityLabelList)
>> (virDomainGetEmulatorPinInfo): Use typical error reporting.
>> (virConnectGetCPUModelNames, virConnectRegisterCloseCallback)
>> (virConnectUnregisterCloseCallback, virDomainGetUUID): Report
>> error through connection.
>>
>> Signed-off-by: Eric Blake <eblake redhat com>
>> ---
>>  src/libvirt.c | 54 ++++++++++++++++++++++++++++--------------------------
>>  1 file changed, 28 insertions(+), 26 deletions(-)
>>

>>      virResetLastError();
>>
>> -    if (keycodes == NULL ||
>> -        nkeycodes <= 0 || nkeycodes > VIR_DOMAIN_SEND_KEY_MAX_KEYS) {
>> -        virLibDomainError(VIR_ERR_OPERATION_INVALID, __FUNCTION__);
>> -        virDispatchError(NULL);
>> -        return -1;
>> +    virCheckNonNullArgGoto(keycodes, error);
>> +    virCheckPositiveArgGoto(nkeycodes, error);
>> +
>> +    if (nkeycodes > VIR_DOMAIN_SEND_KEY_MAX_KEYS) {
>> +        virReportInvalidArg(nkeycodes,
>> +                            _("nkeycodes in %s must be less than %d"),
> 
> technically...
> 
> less than or equal to ...

Indeed.

> 
> 
>> +                            __FUNCTION__, VIR_DOMAIN_SEND_KEY_MAX_KEYS);
>> +        goto error;
>>      }
> 
> I think the above code may need to move after the next two domain checks
> (although looking forward, I realize it may be irrelevant)...
> 
> If domain is NULL at the goto error above, then the domain->conn deref
> in error: will cause more havoc.

Yes, I ended up fixing it later in the series; but you have a point
about bisectability not introducing known temporary bugs.  I'll hoist
that fix into this patch.


>> +        virReportInvalidArg(flags,
>> +                            _("flags 'affect live' and 'affect config' in %s are mutually exclusive"),
>> +                            __FUNCTION__);
> 
> The new error message is longer than 80 columns.. split across multiple
> lines.

Copied and pasted from elsewhere, but as that is a style issue, fixing
all such instances is still within the bounds of this patch.

> 
> ACK with nits.

Here's what I squashed in before pushing...

On second thought, it makes the content changes harder to find, so I
resplit this patch and did the line wrapping in its own patch, then
pushed both patches (line wrapping under the trivial rule).  Pushed.

diff --git i/src/libvirt.c w/src/libvirt.c
index 33538f4..c259d2f 100644
--- i/src/libvirt.c
+++ w/src/libvirt.c
@@ -2831,7 +2831,8 @@ virDomainSaveFlags(virDomainPtr domain, const char
*to,

     if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags &
VIR_DOMAIN_SAVE_PAUSED)) {
         virReportInvalidArg(flags, "%s",
-                            _("running and paused flags are mutually
exclusive"));
+                            _("running and paused flags are mutually "
+                              "exclusive"));
         goto error;
     }

@@ -2971,7 +2972,8 @@ virDomainRestoreFlags(virConnectPtr conn, const
char *from, const char *dxml,

     if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags &
VIR_DOMAIN_SAVE_PAUSED)) {
         virReportInvalidArg(flags, "%s",
-                            _("running and paused flags are mutually
exclusive"));
+                            _("running and paused flags are mutually "
+                              "exclusive"));
         goto error;
     }

@@ -3122,7 +3124,8 @@ virDomainSaveImageDefineXML(virConnectPtr conn,
const char *file,

     if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags &
VIR_DOMAIN_SAVE_PAUSED)) {
         virReportInvalidArg(flags, "%s",
-                            _("running and paused flags are mutually
exclusive"));
+                            _("running and paused flags are mutually "
+                              "exclusive"));
         goto error;
     }

@@ -4163,7 +4166,8 @@ virDomainGetMemoryParameters(virDomainPtr domain,
     if ((flags & VIR_DOMAIN_AFFECT_LIVE) &&
         (flags & VIR_DOMAIN_AFFECT_CONFIG)) {
         virReportInvalidArg(flags,
-                            _("flags 'affect live' and 'affect config'
in %s are mutually exclusive"),
+                            _("flags 'affect live' and 'affect config'
in %s "
+                              "are mutually exclusive"),
                             __FUNCTION__);
         goto error;
     }
@@ -4428,7 +4432,8 @@ virDomainGetBlkioParameters(virDomainPtr domain,
     if ((flags & VIR_DOMAIN_AFFECT_LIVE) &&
         (flags & VIR_DOMAIN_AFFECT_CONFIG)) {
         virReportInvalidArg(flags,
-                            _("flags 'affect live' and 'affect config'
in %s are mutually exclusive"),
+                            _("flags 'affect live' and 'affect config'
in %s "
+                              "are mutually exclusive"),
                             __FUNCTION__);
         goto error;
     }
@@ -7738,7 +7743,8 @@ virNodeGetCPUStats(virConnectPtr conn,
     virCheckNonNegativeArgGoto(*nparams, error);
     if (cpuNum < 0 && cpuNum != VIR_NODE_CPU_STATS_ALL_CPUS) {
         virReportInvalidArg(cpuNum,
-                            _("cpuNum in %s only accepts %d as a
negative value"),
+                            _("cpuNum in %s only accepts %d as a negative "
+                              "value"),
                             __FUNCTION__, VIR_NODE_CPU_STATS_ALL_CPUS);
         goto error;
     }
@@ -7829,7 +7835,8 @@ virNodeGetMemoryStats(virConnectPtr conn,
     virCheckNonNegativeArgGoto(*nparams, error);
     if (cellNum < 0 && cellNum != VIR_NODE_MEMORY_STATS_ALL_CELLS) {
         virReportInvalidArg(cpuNum,
-                            _("cellNum in %s only accepts %d as a
negative value"),
+                            _("cellNum in %s only accepts %d as a
negative "
+                              "value"),
                             __FUNCTION__, VIR_NODE_MEMORY_STATS_ALL_CELLS);
         goto error;
     }
@@ -8241,7 +8248,8 @@ virDomainGetSchedulerParametersFlags(virDomainPtr
domain,
     if ((flags & VIR_DOMAIN_AFFECT_LIVE) &&
         (flags & VIR_DOMAIN_AFFECT_CONFIG)) {
         virReportInvalidArg(flags,
-                            _("flags 'affect live' and 'affect config'
in %s are mutually exclusive"),
+                            _("flags 'affect live' and 'affect config'
in %s "
+                              "are mutually exclusive"),
                             __FUNCTION__);
         goto error;
     }
@@ -9070,7 +9078,8 @@ virDomainMemoryPeek(virDomainPtr dom,
     /* Exactly one of these two flags must be set.  */
     if (!(flags & VIR_MEMORY_VIRTUAL) == !(flags & VIR_MEMORY_PHYSICAL)) {
         virReportInvalidArg(flags,
-                            _("flags in %s must include
VIR_MEMORY_VIRTUAL or VIR_MEMORY_PHYSICAL"),
+                            _("flags in %s must include
VIR_MEMORY_VIRTUAL or "
+                              "VIR_MEMORY_PHYSICAL"),
                             __FUNCTION__);
         goto error;
     }
@@ -9877,21 +9886,22 @@ virDomainSendKey(virDomainPtr domain,

     virResetLastError();

+    if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
+        virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
+        virDispatchError(NULL);
+        return -1;
+    }
+
     virCheckNonNullArgGoto(keycodes, error);
     virCheckPositiveArgGoto(nkeycodes, error);

     if (nkeycodes > VIR_DOMAIN_SEND_KEY_MAX_KEYS) {
         virReportInvalidArg(nkeycodes,
-                            _("nkeycodes in %s must be less than %d"),
+                            _("nkeycodes in %s must be <= %d"),
                             __FUNCTION__, VIR_DOMAIN_SEND_KEY_MAX_KEYS);
         goto error;
     }

-    if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
-        virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
-        virDispatchError(NULL);
-        return -1;
-    }
     if (domain->conn->flags & VIR_CONNECT_RO) {
         virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
         goto error;
@@ -10414,7 +10424,8 @@ virDomainGetVcpuPinInfo(virDomainPtr domain, int
ncpumaps,
     if ((flags & VIR_DOMAIN_AFFECT_LIVE) &&
         (flags & VIR_DOMAIN_AFFECT_CONFIG)) {
         virReportInvalidArg(flags,
-                            _("flags 'affect live' and 'affect config'
in %s are mutually exclusive"),
+                            _("flags 'affect live' and 'affect config'
in %s "
+                              "are mutually exclusive"),
                             __FUNCTION__);
         goto error;
     }
@@ -10556,7 +10567,8 @@ virDomainGetEmulatorPinInfo(virDomainPtr domain,
unsigned char *cpumap,
     if ((flags & VIR_DOMAIN_AFFECT_LIVE) &&
         (flags & VIR_DOMAIN_AFFECT_CONFIG)) {
         virReportInvalidArg(flags,
-                            _("flags 'affect live' and 'affect config'
in %s are mutually exclusive"),
+                            _("flags 'affect live' and 'affect config'
in %s "
+                              "are mutually exclusive"),
                             __FUNCTION__);
         goto error;
     }
@@ -10850,7 +10862,8 @@ virDomainSetMetadata(virDomainPtr domain,
     case VIR_DOMAIN_METADATA_TITLE:
         if (metadata && strchr(metadata, '\n')) {
             virReportInvalidArg(metadata,
-                                _("metadata title in %s can't contain
newlines"),
+                                _("metadata title in %s can't contain "
+                                  "newlines"),
                                 __FUNCTION__);
             goto error;
         }
@@ -10928,7 +10941,8 @@ virDomainGetMetadata(virDomainPtr domain,
     if ((flags & VIR_DOMAIN_AFFECT_LIVE) &&
         (flags & VIR_DOMAIN_AFFECT_CONFIG)) {
         virReportInvalidArg(flags,
-                            _("flags 'affect live' and 'affect config'
in %s are mutually exclusive"),
+                            _("flags 'affect live' and 'affect config'
in %s "
+                              "are mutually exclusive"),
                             __FUNCTION__);
         goto error;
     }
@@ -15464,7 +15478,8 @@ virStorageVolResize(virStorageVolPtr vol,
     if (capacity == 0 && !((flags & VIR_STORAGE_VOL_RESIZE_DELTA) ||
                            (flags & VIR_STORAGE_VOL_RESIZE_SHRINK))) {
         virReportInvalidArg(capacity,
-                            _("capacity in %s cannot be zero without
'delta' or 'shrink' flags set"),
+                            _("capacity in %s cannot be zero without
'delta' "
+                              "or 'shrink' flags set"),
                             __FUNCTION__);
         goto error;
     }
@@ -19610,7 +19625,8 @@ virDomainManagedSave(virDomainPtr dom, unsigned
int flags)

     if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags &
VIR_DOMAIN_SAVE_PAUSED)) {
         virReportInvalidArg(flags,
-                            _("running and paused flags in %s are
mutually exclusive"),
+                            _("running and paused flags in %s are
mutually "
+                              "exclusive"),
                             __FUNCTION__);
         goto error;
     }
@@ -19937,21 +19953,24 @@ 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;
     }
@@ -20868,7 +20887,8 @@ virDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
     if ((flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) &&
         (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
         virReportInvalidArg(flags,
-                            _("running and paused flags in %s are
mutually exclusive"),
+                            _("running and paused flags in %s are
mutually "
+                              "exclusive"),
                             __FUNCTION__);
         goto error;
     }
@@ -22110,7 +22130,8 @@ virDomainGetBlockIoTune(virDomainPtr dom,
     if ((flags & VIR_DOMAIN_AFFECT_LIVE) &&
         (flags & VIR_DOMAIN_AFFECT_CONFIG)) {
         virReportInvalidArg(flags,
-                            _("flags 'affect live' and 'affect config'
in %s are mutually exclusive"),
+                            _("flags 'affect live' and 'affect config'
in %s "
+                              "are mutually exclusive"),
                             __FUNCTION__);
         goto error;
     }


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

Attachment: signature.asc
Description: OpenPGP digital signature


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