8:16 a.m.
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(a)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