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@xxxxxxxxxx>
> ---
> src/libvirt.c | 54 ++++++++++++++++++++++++++++--------------------------
> 1 file changed, 28 insertions(+), 26 deletions(-)
>
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 815dd64..33f7078 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -3588,11 +3588,15 @@ virDomainGetUUID(virDomainPtr domain, unsigned char *uuid)
> virDispatchError(NULL);
> return -1;
> }
> - virCheckNonNullArgReturn(uuid, -1);
> + virCheckNonNullArgGoto(uuid, error);
>
> memcpy(uuid, &domain->uuid[0], VIR_UUID_BUFLEN);
>
> return 0;
> +
> +error:
> + virDispatchError(domain->conn);
> + return -1;
> }
>
>
> @@ -9866,11 +9870,14 @@ virDomainSendKey(virDomainPtr domain,
>
> 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 ...
> + __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.
>
> if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> @@ -10477,10 +10484,8 @@ virDomainPinEmulator(virDomainPtr domain, unsigned char *cpumap,
> goto error;
> }
>
> - if ((cpumap == NULL) || (maplen < 1)) {
> - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> - goto error;
> - }
> + virCheckNonNullArgGoto(cpumap, error);
> + virCheckPositiveArgGoto(maplen, error);
>
> conn = domain->conn;
>
> @@ -10537,15 +10542,15 @@ virDomainGetEmulatorPinInfo(virDomainPtr domain, unsigned char *cpumap,
> return -1;
> }
>
> - if (!cpumap || maplen <= 0) {
> - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> - goto error;
> - }
> + virCheckNonNullArgGoto(cpumap, error);
> + virCheckPositiveArgGoto(maplen, error);
>
> /* At most one of these two flags should be set. */
> if ((flags & VIR_DOMAIN_AFFECT_LIVE) &&
> (flags & VIR_DOMAIN_AFFECT_CONFIG)) {
> - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> + 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.
ACK with nits.
John
> goto error;
> }
> conn = domain->conn;
> @@ -10759,10 +10764,7 @@ virDomainGetSecurityLabelList(virDomainPtr domain,
> return -1;
> }
>
> - if (seclabels == NULL) {
> - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> - goto error;
> - }
> + virCheckNonNullArgGoto(seclabels, error);
>
> conn = domain->conn;
>
> @@ -18815,7 +18817,7 @@ virConnectGetCPUModelNames(virConnectPtr conn, const char *arch, char ***models,
> virDispatchError(NULL);
> return -1;
> }
> - virCheckNonNullArgReturn(arch, -1);
> + virCheckNonNullArgGoto(arch, error);
>
> if (conn->driver->connectGetCPUModelNames) {
> int ret;
> @@ -21887,10 +21889,10 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
> return 0;
>
> error:
> + virDispatchError(conn);
> virObjectUnlock(conn->closeCallback);
> virMutexUnlock(&conn->lock);
> virObjectUnref(conn);
> - virDispatchError(NULL);
> return -1;
> }
>
> @@ -21945,9 +21947,9 @@ virConnectUnregisterCloseCallback(virConnectPtr conn,
> return 0;
>
> error:
> + virDispatchError(conn);
> virObjectUnlock(conn->closeCallback);
> virMutexUnlock(&conn->lock);
> - virDispatchError(NULL);
> return -1;
> }
>
> @@ -22305,10 +22307,10 @@ virDomainGetDiskErrors(virDomainPtr dom,
> return -1;
> }
>
> - if ((!errors && maxerrors) || (errors && !maxerrors)) {
> - virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> - goto error;
> - }
> + if (maxerrors)
> + virCheckNonNullArgGoto(errors, error);
> + else
> + virCheckNullArgGoto(errors, error);
>
> if (dom->conn->driver->domainGetDiskErrors) {
> int ret = dom->conn->driver->domainGetDiskErrors(dom, errors,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list