[libvirt] [PATCH] Santize the reporting of VIR_ERR_INVALID_ERROR

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) - An arg which must be NULL virCheckNullArgGoto(argname, label) - An arg which must be positive (ie 1 or greater) virCheckPositiveArgGoto(argname, label) - An arg which must not be 0 virCheckNonZeroArgGoto(argname, label) - An arg which must be zero virCheckZeroArgGoto(argname, label) - An arg which must not be negative (ie 0 or greater) virCheckNonNegativeArgGoto(argname, label) TBD: many more source files to fixup --- src/datatypes.c | 119 ++--- src/internal.h | 61 ++- src/libvirt-qemu.c | 14 +- src/libvirt.c | 1160 +++++++++++++++-------------------------- src/nodeinfo.c | 19 +- src/util/virterror_internal.h | 77 +++ 6 files changed, 616 insertions(+), 834 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 0857f9a..d718170 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -138,7 +138,7 @@ virUnrefConnect(virConnectPtr conn) { int refs; if ((!VIR_IS_CONNECT(conn))) { - virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); + virLibConnError(VIR_ERR_INVALID_CONN, _("no connection")); return -1; } virMutexLock(&conn->lock); @@ -173,17 +173,12 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { char uuidstr[VIR_UUID_STRING_BUFLEN]; if (!VIR_IS_CONNECT(conn)) { - virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); - return NULL; - } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); - return NULL; - } - if (uuid == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing uuid")); + virLibConnError(VIR_ERR_INVALID_CONN, _("no connection")); return NULL; } + virCheckNonNullArgReturn(name, NULL); + virCheckNonNullArgReturn(uuid, NULL); + virMutexLock(&conn->lock); virUUIDFormat(uuid, uuidstr); @@ -269,7 +264,7 @@ virUnrefDomain(virDomainPtr domain) { int refs; if (!VIR_IS_CONNECTED_DOMAIN(domain)) { - virLibConnError(VIR_ERR_INVALID_ARG, _("bad domain or no connection")); + virLibConnError(VIR_ERR_INVALID_DOMAIN, _("bad domain or no connection")); return -1; } virMutexLock(&domain->conn->lock); @@ -305,17 +300,12 @@ virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) { char uuidstr[VIR_UUID_STRING_BUFLEN]; if (!VIR_IS_CONNECT(conn)) { - virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); - return NULL; - } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); - return NULL; - } - if (uuid == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing uuid")); + virLibConnError(VIR_ERR_INVALID_CONN, _("no connection")); return NULL; } + virCheckNonNullArgReturn(name, NULL); + virCheckNonNullArgReturn(uuid, NULL); + virMutexLock(&conn->lock); virUUIDFormat(uuid, uuidstr); @@ -399,7 +389,7 @@ virUnrefNetwork(virNetworkPtr network) { int refs; if (!VIR_IS_CONNECTED_NETWORK(network)) { - virLibConnError(VIR_ERR_INVALID_ARG, + virLibConnError(VIR_ERR_INVALID_NETWORK, _("bad network or no connection")); return -1; } @@ -437,13 +427,10 @@ virGetInterface(virConnectPtr conn, const char *name, const char *mac) { virInterfacePtr ret = NULL; if (!VIR_IS_CONNECT(conn)) { - virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); - return NULL; - } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); + virLibConnError(VIR_ERR_INVALID_CONN, _("no connection")); return NULL; } + virCheckNonNullArgReturn(name, NULL); /* a NULL mac from caller is okay. Treat it as blank */ if (mac == NULL) @@ -535,7 +522,7 @@ virUnrefInterface(virInterfacePtr iface) { int refs; if (!VIR_IS_CONNECTED_INTERFACE(iface)) { - virLibConnError(VIR_ERR_INVALID_ARG, + virLibConnError(VIR_ERR_INVALID_INTERFACE, _("bad interface or no connection")); return -1; } @@ -574,17 +561,12 @@ virGetStoragePool(virConnectPtr conn, const char *name, char uuidstr[VIR_UUID_STRING_BUFLEN]; if (!VIR_IS_CONNECT(conn)) { - virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); - return NULL; - } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); - return NULL; - } - if (uuid == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing uuid")); + virLibConnError(VIR_ERR_INVALID_CONN, _("no connection")); return NULL; } + virCheckNonNullArgReturn(name, NULL); + virCheckNonNullArgReturn(uuid, NULL); + virMutexLock(&conn->lock); virUUIDFormat(uuid, uuidstr); @@ -669,7 +651,7 @@ virUnrefStoragePool(virStoragePoolPtr pool) { int refs; if (!VIR_IS_CONNECTED_STORAGE_POOL(pool)) { - virLibConnError(VIR_ERR_INVALID_ARG, + virLibConnError(VIR_ERR_INVALID_STORAGE_POOL, _("bad storage pool or no connection")); return -1; } @@ -708,17 +690,12 @@ virGetStorageVol(virConnectPtr conn, const char *pool, const char *name, virStorageVolPtr ret = NULL; if (!VIR_IS_CONNECT(conn)) { - virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); - return NULL; - } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); - return NULL; - } - if (key == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing key")); + virLibConnError(VIR_ERR_INVALID_CONN, _("no connection")); return NULL; } + virCheckNonNullArgReturn(name, NULL); + virCheckNonNullArgReturn(key, NULL); + virMutexLock(&conn->lock); if (VIR_ALLOC(ret) < 0) { @@ -813,7 +790,7 @@ virUnrefStorageVol(virStorageVolPtr vol) { int refs; if (!VIR_IS_CONNECTED_STORAGE_VOL(vol)) { - virLibConnError(VIR_ERR_INVALID_ARG, + virLibConnError(VIR_ERR_INVALID_STORAGE_VOL, _("bad storage volume or no connection")); return -1; } @@ -850,13 +827,11 @@ virGetNodeDevice(virConnectPtr conn, const char *name) virNodeDevicePtr ret = NULL; if (!VIR_IS_CONNECT(conn)) { - virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); - return NULL; - } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); + virLibConnError(VIR_ERR_INVALID_CONN, _("no connection")); return NULL; } + virCheckNonNullArgReturn(name, NULL); + virMutexLock(&conn->lock); if (VIR_ALLOC(ret) < 0) { @@ -970,17 +945,12 @@ virGetSecret(virConnectPtr conn, const unsigned char *uuid, char uuidstr[VIR_UUID_STRING_BUFLEN]; if (!VIR_IS_CONNECT(conn)) { - virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); - return NULL; - } - if (uuid == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing uuid")); - return NULL; - } - if (usageID == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing usageID")); + virLibConnError(VIR_ERR_INVALID_CONN, _("no connection")); return NULL; } + virCheckNonNullArgReturn(uuid, NULL); + virCheckNonNullArgReturn(usageID, NULL); + virMutexLock(&conn->lock); virUUIDFormat(uuid, uuidstr); @@ -1061,7 +1031,7 @@ virUnrefSecret(virSecretPtr secret) { int refs; if (!VIR_IS_CONNECTED_SECRET(secret)) { - virLibConnError(VIR_ERR_INVALID_ARG, _("bad secret or no connection")); + virLibConnError(VIR_ERR_INVALID_SECRET, _("bad secret or no connection")); return -1; } virMutexLock(&secret->conn->lock); @@ -1156,17 +1126,12 @@ virGetNWFilter(virConnectPtr conn, const char *name, const unsigned char *uuid) char uuidstr[VIR_UUID_STRING_BUFLEN]; if (!VIR_IS_CONNECT(conn)) { - virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); - return NULL; - } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); - return NULL; - } - if (uuid == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing uuid")); + virLibConnError(VIR_ERR_INVALID_CONN, _("no connection")); return NULL; } + virCheckNonNullArgReturn(name, NULL); + virCheckNonNullArgReturn(uuid, NULL); + virMutexLock(&conn->lock); virUUIDFormat(uuid, uuidstr); @@ -1253,7 +1218,7 @@ virUnrefNWFilter(virNWFilterPtr nwfilter) int refs; if (!VIR_IS_CONNECTED_NWFILTER(nwfilter)) { - virLibConnError(VIR_ERR_INVALID_ARG, + virLibConnError(VIR_ERR_INVALID_NWFILTER, _("bad nwfilter or no connection")); return -1; } @@ -1279,13 +1244,11 @@ virGetDomainSnapshot(virDomainPtr domain, const char *name) virDomainSnapshotPtr ret = NULL; if (!VIR_IS_DOMAIN(domain)) { - virLibConnError(VIR_ERR_INVALID_ARG, _("bad domain")); - return NULL; - } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); + virLibConnError(VIR_ERR_INVALID_DOMAIN, _("bad domain")); return NULL; } + virCheckNonNullArgReturn(name, NULL); + virMutexLock(&domain->conn->lock); if (VIR_ALLOC(ret) < 0) { @@ -1344,7 +1307,7 @@ virUnrefDomainSnapshot(virDomainSnapshotPtr snapshot) int refs; if (!VIR_IS_DOMAIN_SNAPSHOT(snapshot)) { - virLibConnError(VIR_ERR_INVALID_ARG, _("not a snapshot")); + virLibConnError(VIR_ERR_INVALID_DOMAIN_SNAPSHOT, _("not a snapshot")); return -1; } diff --git a/src/internal.h b/src/internal.h index d13847a..825b0fe 100644 --- a/src/internal.h +++ b/src/internal.h @@ -232,17 +232,64 @@ do { \ unsigned long __unsuppflags = flags & ~(supported); \ if (__unsuppflags) { \ - virReportErrorHelper(VIR_FROM_THIS, \ - VIR_ERR_INVALID_ARG, \ - __FILE__, \ - __FUNCTION__, \ - __LINE__, \ - _("%s: unsupported flags (0x%lx)"), \ - __FUNCTION__, __unsuppflags); \ + virReportInvalidArg("flags", \ + _("unsupported flags (0x%lx) in function %s"), \ + __unsuppflags, __FUNCTION__); \ return retval; \ } \ } while (0) +# define virCheckNonNullArgReturn(argname, retval) \ + do { \ + if (argname == NULL) { \ + virReportInvalidNullArg(#argname); \ + return retval; \ + } \ + } while (0) +# define virCheckNullArgGoto(argname, label) \ + do { \ + if (argname == NULL) { \ + virReportInvalidNullArg(#argname); \ + goto label; \ + } \ + } while (0) +# define virCheckNonNullArgGoto(argname, label) \ + do { \ + if (argname == NULL) { \ + virReportInvalidNonNullArg(#argname); \ + goto label; \ + } \ + } while (0) +# define virCheckPositiveArgGoto(argname, label) \ + do { \ + if (argname <= 0) { \ + virReportInvalidPositiveArg(#argname); \ + goto label; \ + } \ + } while (0) +# define virCheckNonZeroArgGoto(argname, label) \ + do { \ + if (argname == 0) { \ + virReportInvalidNonZeroArg(#argname); \ + goto label; \ + } \ + } while (0) +# define virCheckZeroArgGoto(argname, label) \ + do { \ + if (argname != 0) { \ + virReportInvalidNonZeroArg(#argname); \ + goto label; \ + } \ + } while (0) +# define virCheckNonNegativeArgGoto(argname, label) \ + do { \ + if (argname < 0) { \ + virReportInvalidNonNegativeArg(#argname); \ + goto label; \ + } \ + } while (0) + + /* divide value by size, rounding up */ # define VIR_DIV_UP(value, size) (((value) + (size) - 1) / (size)) diff --git a/src/libvirt-qemu.c b/src/libvirt-qemu.c index 5267bba..4b2f8c2 100644 --- a/src/libvirt-qemu.c +++ b/src/libvirt-qemu.c @@ -28,6 +28,8 @@ #include "datatypes.h" #include "libvirt/libvirt-qemu.h" +#define VIR_FROM_THIS VIR_FROM_NONE + #define virLibConnError(conn, error, info) \ virReportErrorHelper(VIR_FROM_NONE, error, NULL, __FUNCTION__, \ __LINE__, info) @@ -87,10 +89,7 @@ virDomainQemuMonitorCommand(virDomainPtr domain, const char *cmd, conn = domain->conn; - if (result == NULL) { - virLibDomainError(domain, VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(result, error); if (conn->flags & VIR_CONNECT_RO) { virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__); @@ -159,8 +158,11 @@ virDomainQemuAttach(virConnectPtr conn, return NULL; } - if (pid != pid_value || pid <= 1) { - virLibDomainError(domain, VIR_ERR_INVALID_ARG, __FUNCTION__); + virCheckPositiveArgGoto(pid_value, error); + if (pid != pid_value) { + virReportInvalidArg(pid_value, + _("pid_value in %s is too large"), + __FUNCTION__); goto error; } diff --git a/src/libvirt.c b/src/libvirt.c index 8900011..542660c 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -533,10 +533,7 @@ virRegisterNetworkDriver(virNetworkDriverPtr driver) if (virInitialize() < 0) return -1; - if (driver == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - return -1; - } + virCheckNonNullArgReturn(driver, -1); if (virNetworkDriverTabCount >= MAX_DRIVERS) { virLibConnError(VIR_ERR_INTERNAL_ERROR, @@ -566,10 +563,7 @@ virRegisterInterfaceDriver(virInterfaceDriverPtr driver) if (virInitialize() < 0) return -1; - if (driver == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - return -1; - } + virCheckNonNullArgReturn(driver, -1); if (virInterfaceDriverTabCount >= MAX_DRIVERS) { virLibConnError(VIR_ERR_INTERNAL_ERROR, @@ -599,10 +593,7 @@ virRegisterStorageDriver(virStorageDriverPtr driver) if (virInitialize() < 0) return -1; - if (driver == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - return -1; - } + virCheckNonNullArgReturn(driver, -1); if (virStorageDriverTabCount >= MAX_DRIVERS) { virLibConnError(VIR_ERR_INTERNAL_ERROR, @@ -632,10 +623,7 @@ virRegisterDeviceMonitor(virDeviceMonitorPtr driver) if (virInitialize() < 0) return -1; - if (driver == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - return -1; - } + virCheckNonNullArgReturn(driver, -1); if (virDeviceMonitorTabCount >= MAX_DRIVERS) { virLibConnError(VIR_ERR_INTERNAL_ERROR, @@ -665,10 +653,7 @@ virRegisterSecretDriver(virSecretDriverPtr driver) if (virInitialize() < 0) return -1; - if (driver == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - return -1; - } + virCheckNonNullArgReturn(driver, -1); if (virSecretDriverTabCount >= MAX_DRIVERS) { virLibConnError(VIR_ERR_INTERNAL_ERROR, @@ -698,10 +683,7 @@ virRegisterNWFilterDriver(virNWFilterDriverPtr driver) if (virInitialize() < 0) return -1; - if (driver == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - return -1; - } + virCheckNonNullArgReturn(driver, -1); if (virNWFilterDriverTabCount >= MAX_DRIVERS) { virLibConnError(VIR_ERR_INTERNAL_ERROR, @@ -734,10 +716,7 @@ virRegisterDriver(virDriverPtr driver) if (virInitialize() < 0) return -1; - if (driver == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - return -1; - } + virCheckNonNullArgReturn(driver, -1); if (virDriverTabCount >= MAX_DRIVERS) { virLibConnError(VIR_ERR_INTERNAL_ERROR, @@ -746,12 +725,6 @@ virRegisterDriver(virDriverPtr driver) return -1; } - if (driver->no < 0) { - virLibConnError(VIR_ERR_INVALID_ARG, - _("Tried to register an internal Xen driver")); - return -1; - } - VIR_DEBUG ("registering %s as driver %d", driver->name, virDriverTabCount); @@ -774,10 +747,7 @@ virRegisterStateDriver(virStateDriverPtr driver) if (virInitialize() < 0) return -1; - if (driver == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - return -1; - } + virCheckNonNullArgReturn(driver, -1); if (virStateDriverTabCount >= MAX_DRIVERS) { virLibConnError(VIR_ERR_INTERNAL_ERROR, @@ -1172,7 +1142,7 @@ do_open (const char *name, STRCASEEQ(ret->uri->scheme, "xenapi") || #endif false)) { - virReportErrorHelper(VIR_FROM_NONE, VIR_ERR_INVALID_ARG, + virReportErrorHelper(VIR_FROM_NONE, VIR_ERR_CONFIG_UNSUPPORTED, __FILE__, __FUNCTION__, __LINE__, _("libvirt was built without the '%s' driver"), ret->uri->scheme); @@ -1588,10 +1558,7 @@ virConnectGetVersion(virConnectPtr conn, unsigned long *hvVer) return -1; } - if (hvVer == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(hvVer, error); if (conn->driver->version) { int ret = conn->driver->version (conn, hvVer); @@ -1632,10 +1599,7 @@ virConnectGetLibVersion(virConnectPtr conn, unsigned long *libVer) return -1; } - if (libVer == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(libVer, error); if (conn->driver->libvirtVersion) { ret = conn->driver->libvirtVersion(conn, libVer); @@ -1831,10 +1795,8 @@ virConnectListDomains(virConnectPtr conn, int *ids, int maxids) return -1; } - if ((ids == NULL) || (maxids < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(ids, error); + virCheckNonNegativeArgGoto(maxids, error); if (conn->driver->listDomains) { int ret = conn->driver->listDomains (conn, ids, maxids); @@ -1951,10 +1913,7 @@ virDomainCreateXML(virConnectPtr conn, const char *xmlDesc, virDispatchError(NULL); return NULL; } - if (xmlDesc == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xmlDesc, error); if (conn->flags & VIR_CONNECT_RO) { virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; @@ -2017,10 +1976,7 @@ virDomainLookupByID(virConnectPtr conn, int id) virDispatchError(NULL); return NULL; } - if (id < 0) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNegativeArgGoto(id, error); if (conn->driver->domainLookupByID) { virDomainPtr ret; @@ -2059,10 +2015,7 @@ virDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDispatchError(NULL); return NULL; } - if (uuid == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuid, error); if (conn->driver->domainLookupByUUID) { virDomainPtr ret; @@ -2102,13 +2055,12 @@ virDomainLookupByUUIDString(virConnectPtr conn, const char *uuidstr) virDispatchError(NULL); return NULL; } - if (uuidstr == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuidstr, error); if (virUUIDParse(uuidstr, uuid) < 0) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virReportInvalidArg(uuidstr, + _("uuidstr in %s must be a valid UUID"), + __FUNCTION__); goto error; } @@ -2141,10 +2093,7 @@ virDomainLookupByName(virConnectPtr conn, const char *name) virDispatchError(NULL); return NULL; } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(name, error); if (conn->driver->domainLookupByName) { virDomainPtr dom; @@ -2336,7 +2285,7 @@ int virDomainRef(virDomainPtr domain) { if ((!VIR_IS_CONNECTED_DOMAIN(domain))) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); return -1; } @@ -2588,10 +2537,7 @@ virDomainSave(virDomainPtr domain, const char *to) goto error; } conn = domain->conn; - if (to == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(to, error); if (conn->driver->domainSave) { int ret; @@ -2680,14 +2626,11 @@ virDomainSaveFlags(virDomainPtr domain, const char *to, goto error; } conn = domain->conn; - if (to == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(to, error); if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags & VIR_DOMAIN_SAVE_PAUSED)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("running and paused flags are mutually exclusive")); + virReportInvalidArg(flags, "%s", + _("running and paused flags are mutually exclusive")); goto error; } @@ -2745,10 +2688,7 @@ virDomainRestore(virConnectPtr conn, const char *from) virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (from == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(from, error); if (conn->driver->domainRestore) { int ret; @@ -2824,14 +2764,11 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml, virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (from == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(from, error); if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags & VIR_DOMAIN_SAVE_PAUSED)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("running and paused flags are mutually exclusive")); + virReportInvalidArg(flags, "%s", + _("running and paused flags are mutually exclusive")); goto error; } @@ -2895,10 +2832,7 @@ virDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *file, virDispatchError(NULL); return NULL; } - if (file == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(file, error); if ((conn->flags & VIR_CONNECT_RO) && (flags & VIR_DOMAIN_XML_SECURE)) { virLibConnError(VIR_ERR_OPERATION_DENIED, @@ -2978,14 +2912,12 @@ virDomainSaveImageDefineXML(virConnectPtr conn, const char *file, virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (!file || !dxml) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(file, error); + virCheckNonNullArgGoto(dxml, error); if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags & VIR_DOMAIN_SAVE_PAUSED)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("running and paused flags are mutually exclusive")); + virReportInvalidArg(flags, "%s", + _("running and paused flags are mutually exclusive")); goto error; } @@ -3061,26 +2993,23 @@ virDomainCoreDump(virDomainPtr domain, const char *to, unsigned int flags) goto error; } conn = domain->conn; - if (to == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(to, error); if ((flags & VIR_DUMP_CRASH) && (flags & VIR_DUMP_LIVE)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("crash and live flags are mutually exclusive")); + virReportInvalidArg(flags, "%s", + _("crash and live flags are mutually exclusive")); goto error; } if ((flags & VIR_DUMP_CRASH) && (flags & VIR_DUMP_RESET)) { - virLibDomainError(VIR_ERR_INVALID_ARG, + virReportInvalidArg(flags, "%s", _("crash and reset flags are mutually exclusive")); goto error; } if ((flags & VIR_DUMP_LIVE) && (flags & VIR_DUMP_RESET)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("live and reset flags are mutually exclusive")); + virReportInvalidArg(flags, "%s", + _("live and reset flags are mutually exclusive")); goto error; } @@ -3275,7 +3204,8 @@ virDomainShutdownFlags(virDomainPtr domain, unsigned int flags) /* At most one of these two flags should be set. */ if ((flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) && (flags & VIR_DOMAIN_SHUTDOWN_GUEST_AGENT)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virReportInvalidArg(flags, "%s", + _("flags for acpi power button and guest agent are mutually exclusive")); goto error; } @@ -3336,7 +3266,8 @@ virDomainReboot(virDomainPtr domain, unsigned int flags) /* At most one of these two flags should be set. */ if ((flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) && (flags & VIR_DOMAIN_SHUTDOWN_GUEST_AGENT)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virReportInvalidArg(flags, "%s", + _("flags for acpi power button and guest agent are mutually exclusive")); goto error; } @@ -3452,11 +3383,7 @@ virDomainGetUUID(virDomainPtr domain, unsigned char *uuid) virDispatchError(NULL); return -1; } - if (uuid == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - virDispatchError(domain->conn); - return -1; - } + virCheckNonNullArgReturn(uuid, -1); memcpy(uuid, &domain->uuid[0], VIR_UUID_BUFLEN); @@ -3487,10 +3414,7 @@ virDomainGetUUIDString(virDomainPtr domain, char *buf) virDispatchError(NULL); return -1; } - if (buf == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(buf, error); if (virDomainGetUUID(domain, &uuid[0])) goto error; @@ -3649,10 +3573,8 @@ virDomainSetMaxMemory(virDomainPtr domain, unsigned long memory) virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (!memory) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonZeroArgGoto(memory, error); + conn = domain->conn; if (conn->driver->domainSetMaxMemory) { @@ -3703,10 +3625,7 @@ virDomainSetMemory(virDomainPtr domain, unsigned long memory) virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (!memory) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonZeroArgGoto(memory, error); conn = domain->conn; @@ -3771,11 +3690,7 @@ virDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory, virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - - if (!memory) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonZeroArgGoto(memory, error); conn = domain->conn; @@ -3810,27 +3725,24 @@ virTypedParameterValidateSet(virDomainPtr domain, for (i = 0; i < nparams; i++) { if (strnlen(params[i].field, VIR_TYPED_PARAM_FIELD_LENGTH) == VIR_TYPED_PARAM_FIELD_LENGTH) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("string parameter name '%.*s' too long"), - VIR_TYPED_PARAM_FIELD_LENGTH, - params[i].field); - virDispatchError(NULL); + virReportInvalidArg(params, + _("string parameter name '%.*s' too long"), + VIR_TYPED_PARAM_FIELD_LENGTH, + params[i].field); return -1; } if (params[i].type == VIR_TYPED_PARAM_STRING) { if (string_okay) { if (!params[i].value.s) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("NULL string parameter '%s'"), - params[i].field); - virDispatchError(NULL); + virReportInvalidArg(params, + _("NULL string parameter '%s'"), + params[i].field); return -1; } } else { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("string parameter '%s' unsupported"), - params[i].field); - virDispatchError(NULL); + virReportInvalidArg(params, + _("string parameter '%s' unsupported"), + params[i].field); return -1; } } @@ -3872,12 +3784,11 @@ virDomainSetMemoryParameters(virDomainPtr domain, virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if ((nparams <= 0) || (params == NULL)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(params, error); + virCheckPositiveArgGoto(nparams, error); + if (virTypedParameterValidateSet(domain, params, nparams) < 0) - return -1; + goto error; conn = domain->conn; @@ -3948,11 +3859,11 @@ virDomainGetMemoryParameters(virDomainPtr domain, virDispatchError(NULL); return -1; } - if ((nparams == NULL) || (*nparams < 0) || - (params == NULL && *nparams != 0)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(nparams, error); + virCheckNonNegativeArgGoto(*nparams, error); + if (*nparams != 0) + virCheckNonNullArgGoto(params, error); + if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_TYPED_PARAM_STRING)) flags |= VIR_TYPED_PARAM_STRING_OKAY; @@ -3960,7 +3871,9 @@ virDomainGetMemoryParameters(virDomainPtr domain, /* 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__); goto error; } conn = domain->conn; @@ -4013,12 +3926,10 @@ virDomainSetNumaParameters(virDomainPtr domain, virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if ((nparams <= 0) || (params == NULL)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(params, error); + virCheckPositiveArgGoto(nparams, error); if (virTypedParameterValidateSet(domain, params, nparams) < 0) - return -1; + goto error; conn = domain->conn; @@ -4082,11 +3993,11 @@ virDomainGetNumaParameters(virDomainPtr domain, virDispatchError(NULL); return -1; } - if ((nparams == NULL) || (*nparams < 0) || - (params == NULL && *nparams != 0)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(nparams, error); + virCheckNonNegativeArgGoto(*nparams, error); + if (*nparams != 0) + virCheckNonNullArgGoto(params, error); + if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_TYPED_PARAM_STRING)) flags |= VIR_TYPED_PARAM_STRING_OKAY; @@ -4142,12 +4053,11 @@ virDomainSetBlkioParameters(virDomainPtr domain, virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if ((nparams <= 0) || (params == NULL)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(params, error); + virCheckNonNegativeArgGoto(nparams, error); + if (virTypedParameterValidateSet(domain, params, nparams) < 0) - return -1; + goto error; conn = domain->conn; @@ -4209,11 +4119,11 @@ virDomainGetBlkioParameters(virDomainPtr domain, virDispatchError(NULL); return -1; } - if ((nparams == NULL) || (*nparams < 0) || - (params == NULL && *nparams != 0)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(nparams, error); + virCheckNonNegativeArgGoto(*nparams, error); + if (*nparams != 0) + virCheckNonNullArgGoto(params, error); + if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_TYPED_PARAM_STRING)) flags |= VIR_TYPED_PARAM_STRING_OKAY; @@ -4221,7 +4131,9 @@ virDomainGetBlkioParameters(virDomainPtr domain, /* 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__); goto error; } conn = domain->conn; @@ -4265,10 +4177,7 @@ virDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) virDispatchError(NULL); return -1; } - if (info == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(info, error); memset(info, 0, sizeof(virDomainInfo)); @@ -4320,10 +4229,7 @@ virDomainGetState(virDomainPtr domain, virDispatchError(NULL); return -1; } - if (!state) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(state, error); conn = domain->conn; if (conn->driver->domainGetState) { @@ -4368,10 +4274,7 @@ virDomainGetControlInfo(virDomainPtr domain, return -1; } - if (!info) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(info, error); conn = domain->conn; if (conn->driver->domainGetControlInfo) { @@ -4478,10 +4381,8 @@ char *virConnectDomainXMLFromNative(virConnectPtr conn, return NULL; } - if (nativeFormat == NULL || nativeConfig == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(nativeFormat, error); + virCheckNonNullArgGoto(nativeConfig, error); if (conn->driver->domainXMLFromNative) { char *ret; @@ -4535,10 +4436,8 @@ char *virConnectDomainXMLToNative(virConnectPtr conn, goto error; } - if (nativeFormat == NULL || domainXml == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(nativeFormat, error); + virCheckNonNullArgGoto(domainXml, error); if (conn->driver->domainXMLToNative) { char *ret; @@ -5018,8 +4917,18 @@ virDomainMigratePeer2Peer (virDomainPtr domain, return -1; } - if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!tempuri->server) { + virReportInvalidArg(tempuri, + _("server field in tempuri in %s must not be NULL"), + __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'"), + __FUNCTION__); virDispatchError(domain->conn); virURIFree(tempuri); return -1; @@ -5620,10 +5529,7 @@ virDomainMigrateToURI (virDomainPtr domain, goto error; } - if (duri == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(duri, error); if (flags & VIR_MIGRATE_PEER2PEER) { if (VIR_DRV_SUPPORTS_FEATURE (domain->conn->driver, domain->conn, @@ -6068,7 +5974,9 @@ virDomainMigratePrepareTunnel(virConnectPtr conn, } if (conn != st->conn) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virReportInvalidArg(conn, + _("conn in %s must match stream connection"), + __FUNCTION__); goto error; } @@ -6234,7 +6142,9 @@ virDomainMigratePrepareTunnel3(virConnectPtr conn, } if (conn != st->conn) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virReportInvalidArg(conn, + _("conn in %s must match stream connection"), + __FUNCTION__); goto error; } @@ -6441,10 +6351,7 @@ virNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info) virDispatchError(NULL); return -1; } - if (info == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(info, error); if (conn->driver->nodeGetInfo) { int ret; @@ -6570,9 +6477,12 @@ int virNodeGetCPUStats (virConnectPtr conn, return -1; } - if ((nparams == NULL) || (*nparams < 0) || - ((cpuNum < 0) && (cpuNum != VIR_NODE_CPU_STATS_ALL_CPUS))) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virCheckNonNullArgGoto(nparams, error); + 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"), + __FUNCTION__, VIR_NODE_CPU_STATS_ALL_CPUS); goto error; } @@ -6656,9 +6566,12 @@ int virNodeGetMemoryStats (virConnectPtr conn, return -1; } - if ((nparams == NULL) || (*nparams < 0) || - ((cellNum < 0) && (cellNum != VIR_NODE_MEMORY_STATS_ALL_CELLS))) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virCheckNonNullArgGoto(nparams, error); + 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"), + __FUNCTION__, VIR_NODE_MEMORY_STATS_ALL_CELLS); goto error; } @@ -6852,10 +6765,9 @@ virDomainGetSchedulerParameters(virDomainPtr domain, return -1; } - if (params == NULL || nparams == NULL || *nparams <= 0) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(params, error); + virCheckNonNullArgGoto(nparams, error); + virCheckPositiveArgGoto(*nparams, error); conn = domain->conn; @@ -6923,10 +6835,9 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain, return -1; } - if (params == NULL || nparams == NULL || *nparams <= 0) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(params, error); + virCheckNonNullArgGoto(nparams, error); + virCheckPositiveArgGoto(*nparams, error); if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_TYPED_PARAM_STRING)) @@ -6935,7 +6846,9 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain, /* 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__); goto error; } conn = domain->conn; @@ -6991,12 +6904,11 @@ virDomainSetSchedulerParameters(virDomainPtr domain, virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (params == NULL || nparams < 0) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(params, error); + virCheckNonNegativeArgGoto(nparams, error); + if (virTypedParameterValidateSet(domain, params, nparams) < 0) - return -1; + goto error; conn = domain->conn; @@ -7056,12 +6968,11 @@ virDomainSetSchedulerParametersFlags(virDomainPtr domain, virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (params == NULL || nparams < 0) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(params, error); + virCheckNonNegativeArgGoto(nparams, error); + if (virTypedParameterValidateSet(domain, params, nparams) < 0) - return -1; + goto error; conn = domain->conn; @@ -7126,8 +7037,12 @@ virDomainBlockStats(virDomainPtr dom, const char *disk, virDispatchError(NULL); return -1; } - if (!disk || !stats || size > sizeof(stats2)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virCheckNonNullArgGoto(disk, error); + virCheckNonNullArgGoto(stats, error); + if (size > sizeof(stats2)) { + virReportInvalidArg(size, + _("size in %s must be less than %zu"), + __FUNCTION__, sizeof(stats2)); goto error; } conn = dom->conn; @@ -7202,11 +7117,12 @@ int virDomainBlockStatsFlags(virDomainPtr dom, virDispatchError(NULL); return -1; } - if (!disk || (nparams == NULL) || (*nparams < 0) || - (params == NULL && *nparams != 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(disk, error); + virCheckNonNullArgGoto(nparams, error); + virCheckNonNegativeArgGoto(*nparams, error); + if (*nparams != 0) + virCheckNonNullArgGoto(params, error); + if (VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, VIR_DRV_FEATURE_TYPED_PARAM_STRING)) flags |= VIR_TYPED_PARAM_STRING_OKAY; @@ -7266,10 +7182,15 @@ virDomainInterfaceStats (virDomainPtr dom, const char *path, virDispatchError(NULL); return -1; } - if (!path || !stats || size > sizeof(stats2)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virCheckNonNullArgGoto(path, error); + virCheckNonNullArgGoto(stats, error); + if (size > sizeof(stats2)) { + virReportInvalidArg(size, + _("size in %s must be less than %zu"), + __FUNCTION__, sizeof(stats2)); goto error; } + conn = dom->conn; if (conn->driver->domainInterfaceStats) { @@ -7328,12 +7249,11 @@ virDomainSetInterfaceParameters(virDomainPtr domain, virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if ((nparams <= 0) || (params == NULL)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(params, error); + virCheckPositiveArgGoto(nparams, error); + if (virTypedParameterValidateSet(domain, params, nparams) < 0) - return -1; + goto error; conn = domain->conn; @@ -7399,11 +7319,11 @@ virDomainGetInterfaceParameters(virDomainPtr domain, virDispatchError(NULL); return -1; } - if ((nparams == NULL) || (*nparams < 0) || - (params == NULL && *nparams != 0)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(nparams, error); + virCheckNonNegativeArgGoto(*nparams, error); + if (*nparams != 0) + virCheckNonNullArgGoto(params, error); + if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_TYPED_PARAM_STRING)) flags |= VIR_TYPED_PARAM_STRING_OKAY; @@ -7565,18 +7485,11 @@ virDomainBlockPeek (virDomainPtr dom, goto error; } - if (!disk) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("disk is NULL")); - goto error; - } + virCheckNonNullArgGoto(disk, error); /* Allow size == 0 as an access test. */ - if (size > 0 && !buffer) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("buffer is NULL")); - goto error; - } + if (size > 0) + virCheckNonNullArgGoto(buffer, error); if (conn->driver->domainBlockPeek) { int ret; @@ -7646,11 +7559,7 @@ virDomainBlockResize (virDomainPtr dom, goto error; } - if (!disk) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("disk is NULL")); - goto error; - } + virCheckNonNullArgGoto(disk, error); if (conn->driver->domainBlockResize) { int ret; @@ -7749,17 +7658,15 @@ virDomainMemoryPeek (virDomainPtr dom, /* Exactly one of these two flags must be set. */ if (!(flags & VIR_MEMORY_VIRTUAL) == !(flags & VIR_MEMORY_PHYSICAL)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("flags parameter must include VIR_MEMORY_VIRTUAL or VIR_MEMORY_PHYSICAL")); + virReportInvalidArg(flags, + _("flags in %s must include VIR_MEMORY_VIRTUAL or VIR_MEMORY_PHYSICAL"), + __FUNCTION__); goto error; } /* Allow size == 0 as an access test. */ - if (size > 0 && !buffer) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("buffer is NULL but size is non-zero")); - goto error; - } + if (size > 0) + virCheckNonNullArgGoto(buffer, error); if (conn->driver->domainMemoryPeek) { int ret; @@ -7811,10 +7718,8 @@ virDomainGetBlockInfo(virDomainPtr domain, const char *disk, virDispatchError(NULL); return -1; } - if (disk == NULL || info == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(disk, error); + virCheckNonNullArgGoto(info, error); memset(info, 0, sizeof(virDomainBlockInfo)); @@ -7874,10 +7779,7 @@ virDomainDefineXML(virConnectPtr conn, const char *xml) { virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (xml == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xml, error); if (conn->driver->domainDefineXML) { virDomainPtr ret; @@ -8064,10 +7966,8 @@ virConnectListDefinedDomains(virConnectPtr conn, char **const names, return -1; } - if ((names == NULL) || (maxnames < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(names, error); + virCheckNonNegativeArgGoto(maxnames, error); if (conn->driver->listDefinedDomains) { int ret; @@ -8222,10 +8122,7 @@ virDomainGetAutostart(virDomainPtr domain, virDispatchError(NULL); return -1; } - if (!autostart) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(autostart, error); conn = domain->conn; @@ -8435,10 +8332,8 @@ virDomainSetVcpus(virDomainPtr domain, unsigned int nvcpus) goto error; } - if (nvcpus < 1) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonZeroArgGoto(nvcpus, error); + conn = domain->conn; if (conn->driver->domainSetVcpus) { @@ -8507,11 +8402,8 @@ virDomainSetVcpusFlags(virDomainPtr domain, unsigned int nvcpus, goto error; } - /* Perform some argument validation common to all implementations. */ - if (nvcpus < 1) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonZeroArgGoto(nvcpus, error); + if ((unsigned short) nvcpus != nvcpus) { virLibDomainError(VIR_ERR_OVERFLOW, _("input too large: %u"), nvcpus); goto error; @@ -8576,7 +8468,9 @@ virDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) /* 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__); goto error; } conn = domain->conn; @@ -8638,9 +8532,12 @@ virDomainPinVcpu(virDomainPtr domain, unsigned int vcpu, goto error; } - if ((vcpu > 32000) || (cpumap == NULL) || (maplen < 1)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; + virCheckNonNullArgGoto(cpumap, error); + virCheckPositiveArgGoto(maplen, error); + + if ((unsigned short) vcpu != vcpu) { + virLibDomainError(VIR_ERR_OVERFLOW, _("input too large: %u"), vcpu); + goto error; } conn = domain->conn; @@ -8715,8 +8612,11 @@ virDomainPinVcpuFlags(virDomainPtr domain, unsigned int vcpu, goto error; } - if ((vcpu > 32000) || (cpumap == NULL) || (maplen < 1)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virCheckNonNullArgGoto(cpumap, error); + virCheckPositiveArgGoto(maplen, error); + + if ((unsigned short) vcpu != vcpu) { + virLibDomainError(VIR_ERR_OVERFLOW, _("input too large: %u"), vcpu); goto error; } @@ -8779,10 +8679,10 @@ virDomainGetVcpuPinInfo(virDomainPtr domain, int ncpumaps, return -1; } - if (ncpumaps < 1 || !cpumaps || maplen <= 0) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(cpumaps, error); + virCheckPositiveArgGoto(ncpumaps, error); + virCheckPositiveArgGoto(maplen, error); + if (INT_MULTIPLY_OVERFLOW(ncpumaps, maplen)) { virLibDomainError(VIR_ERR_OVERFLOW, _("input too large: %d * %d"), ncpumaps, maplen); @@ -8792,7 +8692,9 @@ virDomainGetVcpuPinInfo(virDomainPtr domain, int ncpumaps, /* 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__); goto error; } conn = domain->conn; @@ -8855,17 +8757,16 @@ virDomainGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo, virDispatchError(NULL); return -1; } - if ((info == NULL) || (maxinfo < 1)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(info, error); + virCheckPositiveArgGoto(maxinfo, error); /* Ensure that domainGetVcpus (aka remoteDomainGetVcpus) does not try to memcpy anything into a NULL pointer. */ - if (!cpumaps ? maplen != 0 : maplen <= 0) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + if (cpumaps) + virCheckPositiveArgGoto(maplen, error); + else + virCheckZeroArgGoto(maplen, error); + if (cpumaps && INT_MULTIPLY_OVERFLOW(maxinfo, maplen)) { virLibDomainError(VIR_ERR_OVERFLOW, _("input too large: %d * %d"), maxinfo, maplen); @@ -8958,10 +8859,7 @@ virDomainGetSecurityLabel(virDomainPtr domain, virSecurityLabelPtr seclabel) return -1; } - if (seclabel == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(seclabel, error); conn = domain->conn; @@ -9040,22 +8938,19 @@ virDomainSetMetadata(virDomainPtr domain, switch (type) { case VIR_DOMAIN_METADATA_TITLE: if (metadata && strchr(metadata, '\n')) { - virLibDomainError(VIR_ERR_INVALID_ARG, "%s", - _("Domain title can't contain newlines")); - goto error; + virReportInvalidArg(metadata, + _("metadata title in %s can't contain newlines"), + __FUNCTION__); + goto error; } /* fallthrough */ case VIR_DOMAIN_METADATA_DESCRIPTION: - if (uri || key) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNullArgGoto(uri, error); + virCheckNullArgGoto(key, error); break; case VIR_DOMAIN_METADATA_ELEMENT: - if (!uri || !key) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uri, error); + virCheckNonNullArgGoto(key, error); break; default: /* For future expansion */ @@ -9119,23 +9014,19 @@ virDomainGetMetadata(virDomainPtr domain, 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__); goto error; } switch (type) { case VIR_DOMAIN_METADATA_TITLE: case VIR_DOMAIN_METADATA_DESCRIPTION: - if (uri) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNullArgGoto(uri, error); break; case VIR_DOMAIN_METADATA_ELEMENT: - if (!uri) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uri, error); break; default: /* For future expansion */ @@ -9180,10 +9071,7 @@ virNodeGetSecurityModel(virConnectPtr conn, virSecurityModelPtr secmodel) return -1; } - if (secmodel == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(secmodel, error); if (conn->driver->nodeGetSecurityModel) { int ret; @@ -9229,10 +9117,7 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml) return -1; } - if (xml == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xml, error); if (domain->conn->flags & VIR_CONNECT_RO) { virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); @@ -9295,10 +9180,7 @@ virDomainAttachDeviceFlags(virDomainPtr domain, return -1; } - if (xml == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xml, error); if (domain->conn->flags & VIR_CONNECT_RO) { virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); @@ -9346,10 +9228,7 @@ virDomainDetachDevice(virDomainPtr domain, const char *xml) return -1; } - if (xml == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xml, error); if (domain->conn->flags & VIR_CONNECT_RO) { virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); @@ -9412,10 +9291,7 @@ virDomainDetachDeviceFlags(virDomainPtr domain, return -1; } - if (xml == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xml, error); if (domain->conn->flags & VIR_CONNECT_RO) { virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); @@ -9478,10 +9354,7 @@ virDomainUpdateDeviceFlags(virDomainPtr domain, return -1; } - if (xml == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xml, error); if (domain->conn->flags & VIR_CONNECT_RO) { virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); @@ -9537,10 +9410,9 @@ virNodeGetCellsFreeMemory(virConnectPtr conn, unsigned long long *freeMems, return -1; } - if ((freeMems == NULL) || (maxCells <= 0) || (startCell < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(freeMems, error); + virCheckPositiveArgGoto(maxCells, error); + virCheckNonNegativeArgGoto(startCell, error); if (conn->driver->nodeGetCellsFreeMemory) { int ret; @@ -9645,10 +9517,8 @@ virConnectListNetworks(virConnectPtr conn, char **const names, int maxnames) return -1; } - if ((names == NULL) || (maxnames < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(names, error); + virCheckNonNegativeArgGoto(maxnames, error); if (conn->networkDriver && conn->networkDriver->listNetworks) { int ret; @@ -9725,10 +9595,8 @@ virConnectListDefinedNetworks(virConnectPtr conn, char **const names, return -1; } - if ((names == NULL) || (maxnames < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(names, error); + virCheckNonNegativeArgGoto(maxnames, error); if (conn->networkDriver && conn->networkDriver->listDefinedNetworks) { int ret; @@ -9768,10 +9636,7 @@ virNetworkLookupByName(virConnectPtr conn, const char *name) virDispatchError(NULL); return NULL; } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(name, error); if (conn->networkDriver && conn->networkDriver->networkLookupByName) { virNetworkPtr ret; @@ -9810,10 +9675,8 @@ virNetworkLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDispatchError(NULL); return NULL; } - if (uuid == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + + virCheckNonNullArgGoto(uuid, error); if (conn->networkDriver && conn->networkDriver->networkLookupByUUID){ virNetworkPtr ret; @@ -9853,13 +9716,13 @@ virNetworkLookupByUUIDString(virConnectPtr conn, const char *uuidstr) virDispatchError(NULL); return NULL; } - if (uuidstr == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + + virCheckNonNullArgGoto(uuidstr, error); if (virUUIDParse(uuidstr, uuid) < 0) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virReportInvalidArg(uuidstr, + _("uuidstr in %s must be a valid UUID"), + __FUNCTION__); goto error; } @@ -9892,10 +9755,8 @@ virNetworkCreateXML(virConnectPtr conn, const char *xmlDesc) virDispatchError(NULL); return NULL; } - if (xmlDesc == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xmlDesc, error); + if (conn->flags & VIR_CONNECT_RO) { virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; @@ -9941,10 +9802,7 @@ virNetworkDefineXML(virConnectPtr conn, const char *xml) virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (xml == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xml, error); if (conn->networkDriver && conn->networkDriver->networkDefineXML) { virNetworkPtr ret; @@ -10196,10 +10054,7 @@ virNetworkGetUUID(virNetworkPtr network, unsigned char *uuid) virDispatchError(NULL); return -1; } - if (uuid == NULL) { - virLibNetworkError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuid, error); memcpy(uuid, &network->uuid[0], VIR_UUID_BUFLEN); @@ -10233,10 +10088,7 @@ virNetworkGetUUIDString(virNetworkPtr network, char *buf) virDispatchError(NULL); return -1; } - if (buf == NULL) { - virLibNetworkError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(buf, error); if (virNetworkGetUUID(network, &uuid[0])) goto error; @@ -10362,10 +10214,7 @@ virNetworkGetAutostart(virNetworkPtr network, virDispatchError(NULL); return -1; } - if (!autostart) { - virLibNetworkError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(autostart, error); conn = network->conn; @@ -10520,10 +10369,8 @@ virConnectListInterfaces(virConnectPtr conn, char **const names, int maxnames) return -1; } - if ((names == NULL) || (maxnames < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(names, error); + virCheckNonNegativeArgGoto(maxnames, error); if (conn->interfaceDriver && conn->interfaceDriver->listInterfaces) { int ret; @@ -10602,10 +10449,8 @@ virConnectListDefinedInterfaces(virConnectPtr conn, return -1; } - if ((names == NULL) || (maxnames < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(names, error); + virCheckNonNegativeArgGoto(maxnames, error); if (conn->interfaceDriver && conn->interfaceDriver->listDefinedInterfaces) { int ret; @@ -10644,10 +10489,7 @@ virInterfaceLookupByName(virConnectPtr conn, const char *name) virDispatchError(NULL); return NULL; } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(name, error); if (conn->interfaceDriver && conn->interfaceDriver->interfaceLookupByName) { virInterfacePtr ret; @@ -10686,10 +10528,7 @@ virInterfaceLookupByMACString(virConnectPtr conn, const char *macstr) virDispatchError(NULL); return NULL; } - if (macstr == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(macstr, error); if (conn->interfaceDriver && conn->interfaceDriver->interfaceLookupByMACString) { virInterfacePtr ret; @@ -10841,10 +10680,7 @@ virInterfaceDefineXML(virConnectPtr conn, const char *xml, unsigned int flags) virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (xml == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xml, error); if (conn->interfaceDriver && conn->interfaceDriver->interfaceDefineXML) { virInterfacePtr ret; @@ -11312,10 +11148,8 @@ virConnectListStoragePools(virConnectPtr conn, return -1; } - if ((names == NULL) || (maxnames < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(names, error); + virCheckNonNegativeArgGoto(maxnames, error); if (conn->storageDriver && conn->storageDriver->listPools) { int ret; @@ -11397,10 +11231,8 @@ virConnectListDefinedStoragePools(virConnectPtr conn, return -1; } - if ((names == NULL) || (maxnames < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(names, error); + virCheckNonNegativeArgGoto(maxnames, error); if (conn->storageDriver && conn->storageDriver->listDefinedPools) { int ret; @@ -11455,10 +11287,7 @@ virConnectFindStoragePoolSources(virConnectPtr conn, virDispatchError(NULL); return NULL; } - if (type == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(type, error); if (conn->flags & VIR_CONNECT_RO) { virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); @@ -11503,10 +11332,7 @@ virStoragePoolLookupByName(virConnectPtr conn, virDispatchError(NULL); return NULL; } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(name, error); if (conn->storageDriver && conn->storageDriver->poolLookupByName) { virStoragePoolPtr ret; @@ -11546,10 +11372,7 @@ virStoragePoolLookupByUUID(virConnectPtr conn, virDispatchError(NULL); return NULL; } - if (uuid == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuid, error); if (conn->storageDriver && conn->storageDriver->poolLookupByUUID) { virStoragePoolPtr ret; @@ -11590,13 +11413,12 @@ virStoragePoolLookupByUUIDString(virConnectPtr conn, virDispatchError(NULL); return NULL; } - if (uuidstr == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuidstr, error); if (virUUIDParse(uuidstr, uuid) < 0) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virReportInvalidArg(uuidstr, + _("uuidstr in %s must be a valid UUID"), + __FUNCTION__); goto error; } @@ -11670,10 +11492,8 @@ virStoragePoolCreateXML(virConnectPtr conn, virDispatchError(NULL); return NULL; } - if (xmlDesc == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xmlDesc, error); + if (conn->flags & VIR_CONNECT_RO) { virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; @@ -11723,10 +11543,7 @@ virStoragePoolDefineXML(virConnectPtr conn, virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (xml == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xml, error); if (conn->storageDriver && conn->storageDriver->poolDefineXML) { virStoragePoolPtr ret; @@ -12025,7 +11842,7 @@ int virStoragePoolRef(virStoragePoolPtr pool) { if ((!VIR_IS_CONNECTED_STORAGE_POOL(pool))) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_STORAGE_POOL, __FUNCTION__); virDispatchError(NULL); return -1; } @@ -12130,10 +11947,7 @@ virStoragePoolGetUUID(virStoragePoolPtr pool, virDispatchError(NULL); return -1; } - if (uuid == NULL) { - virLibStoragePoolError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuid, error); memcpy(uuid, &pool->uuid[0], VIR_UUID_BUFLEN); @@ -12167,10 +11981,7 @@ virStoragePoolGetUUIDString(virStoragePoolPtr pool, virDispatchError(NULL); return -1; } - if (buf == NULL) { - virLibStoragePoolError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(buf, error); if (virStoragePoolGetUUID(pool, &uuid[0])) goto error; @@ -12208,10 +12019,7 @@ virStoragePoolGetInfo(virStoragePoolPtr pool, virDispatchError(NULL); return -1; } - if (info == NULL) { - virLibStoragePoolError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(info, error); memset(info, 0, sizeof(virStoragePoolInfo)); @@ -12301,10 +12109,7 @@ virStoragePoolGetAutostart(virStoragePoolPtr pool, virDispatchError(NULL); return -1; } - if (!autostart) { - virLibStoragePoolError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(autostart, error); conn = pool->conn; @@ -12434,10 +12239,8 @@ virStoragePoolListVolumes(virStoragePoolPtr pool, return -1; } - if ((names == NULL) || (maxnames < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(names, error); + virCheckNonNegativeArgGoto(maxnames, error); if (pool->conn->storageDriver && pool->conn->storageDriver->poolListVolumes) { int ret; @@ -12508,10 +12311,8 @@ virStorageVolLookupByName(virStoragePoolPtr pool, virDispatchError(NULL); return NULL; } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + + virCheckNonNullArgGoto(name, error); if (pool->conn->storageDriver && pool->conn->storageDriver->volLookupByName) { virStorageVolPtr ret; @@ -12553,10 +12354,8 @@ virStorageVolLookupByKey(virConnectPtr conn, virDispatchError(NULL); return NULL; } - if (key == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + + virCheckNonNullArgGoto(key, error); if (conn->storageDriver && conn->storageDriver->volLookupByKey) { virStorageVolPtr ret; @@ -12596,10 +12395,7 @@ virStorageVolLookupByPath(virConnectPtr conn, virDispatchError(NULL); return NULL; } - if (path == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(path, error); if (conn->storageDriver && conn->storageDriver->volLookupByPath) { virStorageVolPtr ret; @@ -12695,10 +12491,7 @@ virStorageVolCreateXML(virStoragePoolPtr pool, return NULL; } - if (xmldesc == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xmldesc, error); if (pool->conn->flags & VIR_CONNECT_RO) { virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); @@ -12756,10 +12549,7 @@ virStorageVolCreateXMLFrom(virStoragePoolPtr pool, goto error; } - if (xmldesc == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xmldesc, error); if (pool->conn->flags & VIR_CONNECT_RO || clonevol->conn->flags & VIR_CONNECT_RO) { @@ -13117,7 +12907,7 @@ int virStorageVolRef(virStorageVolPtr vol) { if ((!VIR_IS_CONNECTED_STORAGE_VOL(vol))) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__); virDispatchError(NULL); return -1; } @@ -13152,10 +12942,7 @@ virStorageVolGetInfo(virStorageVolPtr vol, virDispatchError(NULL); return -1; } - if (info == NULL) { - virLibStorageVolError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(info, error); memset(info, 0, sizeof(virStorageVolInfo)); @@ -13323,7 +13110,9 @@ virStorageVolResize(virStorageVolPtr vol, /* Zero capacity is only valid with either delta or shrink. */ if (capacity == 0 && !((flags & VIR_STORAGE_VOL_RESIZE_DELTA) || (flags & VIR_STORAGE_VOL_RESIZE_SHRINK))) { - virLibStorageVolError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virReportInvalidArg(capacity, + _("capacity in %s cannot be zero without 'delta' or 'shrink' flags set"), + __FUNCTION__); goto error; } @@ -13415,10 +13204,8 @@ virNodeListDevices(virConnectPtr conn, virDispatchError(NULL); return -1; } - if ((names == NULL) || (maxnames < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(names, error); + virCheckNonNegativeArgGoto(maxnames, error); if (conn->deviceMonitor && conn->deviceMonitor->listDevices) { int ret; @@ -13457,10 +13244,7 @@ virNodeDevicePtr virNodeDeviceLookupByName(virConnectPtr conn, const char *name) return NULL; } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(name, error); if (conn->deviceMonitor && conn->deviceMonitor->deviceLookupByName) { virNodeDevicePtr ret; @@ -13630,10 +13414,8 @@ int virNodeDeviceListCaps(virNodeDevicePtr dev, return -1; } - if (names == NULL || maxnames < 0) { - virLibNodeDeviceError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(names, error); + virCheckNonNegativeArgGoto(maxnames, error); if (dev->conn->deviceMonitor && dev->conn->deviceMonitor->deviceListCaps) { int ret; @@ -13700,7 +13482,7 @@ int virNodeDeviceRef(virNodeDevicePtr dev) { if ((!VIR_IS_CONNECTED_NODE_DEVICE(dev))) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_NODE_DEVICE, __FUNCTION__); virDispatchError(NULL); return -1; } @@ -13893,10 +13675,7 @@ virNodeDeviceCreateXML(virConnectPtr conn, goto error; } - if (xmlDesc == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xmlDesc, error); if (conn->deviceMonitor && conn->deviceMonitor->deviceCreateXML) { @@ -14000,10 +13779,7 @@ virConnectDomainEventRegister(virConnectPtr conn, virDispatchError(NULL); return -1; } - if (cb == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(cb, error); if ((conn->driver) && (conn->driver->domainEventRegister)) { int ret; @@ -14046,10 +13822,8 @@ virConnectDomainEventDeregister(virConnectPtr conn, virDispatchError(NULL); return -1; } - if (cb == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(cb, error); + if ((conn->driver) && (conn->driver->domainEventDeregister)) { int ret; ret = conn->driver->domainEventDeregister (conn, cb); @@ -14151,10 +13925,8 @@ virConnectListSecrets(virConnectPtr conn, char **uuids, int maxuuids) virDispatchError(NULL); return -1; } - if (uuids == NULL || maxuuids < 0) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuids, error); + virCheckNonNegativeArgGoto(maxuuids, error); if (conn->secretDriver != NULL && conn->secretDriver->listSecrets != NULL) { int ret; @@ -14195,10 +13967,7 @@ virSecretLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDispatchError(NULL); return NULL; } - if (uuid == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuid, error); if (conn->secretDriver && conn->secretDriver->lookupByUUID) { @@ -14240,13 +14009,12 @@ virSecretLookupByUUIDString(virConnectPtr conn, const char *uuidstr) virDispatchError(NULL); return NULL; } - if (uuidstr == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuidstr, error); if (virUUIDParse(uuidstr, uuid) < 0) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virReportInvalidArg(uuidstr, + _("uuidstr in %s must be a valid UUID"), + __FUNCTION__); goto error; } @@ -14285,10 +14053,7 @@ virSecretLookupByUsage(virConnectPtr conn, virDispatchError(NULL); return NULL; } - if (usageID == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(usageID, error); if (conn->secretDriver && conn->secretDriver->lookupByUsage) { @@ -14338,10 +14103,7 @@ virSecretDefineXML(virConnectPtr conn, const char *xml, unsigned int flags) virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (xml == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xml, error); if (conn->secretDriver != NULL && conn->secretDriver->defineXML != NULL) { virSecretPtr ret; @@ -14381,15 +14143,15 @@ virSecretGetUUID(virSecretPtr secret, unsigned char *uuid) virDispatchError(NULL); return -1; } - if (uuid == NULL) { - virLibSecretError(VIR_ERR_INVALID_ARG, __FUNCTION__); - virDispatchError(secret->conn); - return -1; - } + virCheckNonNullArgGoto(uuid, error); memcpy(uuid, &secret->uuid[0], VIR_UUID_BUFLEN); return 0; + +error: + virDispatchError(NULL); + return -1; } /** @@ -14415,10 +14177,7 @@ virSecretGetUUIDString(virSecretPtr secret, char *buf) virDispatchError(NULL); return -1; } - if (buf == NULL) { - virLibSecretError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(buf, error); if (virSecretGetUUID(secret, &uuid[0])) goto error; @@ -14565,10 +14324,7 @@ virSecretSetValue(virSecretPtr secret, const unsigned char *value, virLibSecretError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (value == NULL) { - virLibSecretError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(value, error); if (conn->secretDriver != NULL && conn->secretDriver->setValue != NULL) { int ret; @@ -14616,10 +14372,7 @@ virSecretGetValue(virSecretPtr secret, size_t *value_size, unsigned int flags) virLibSecretError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if (value_size == NULL) { - virLibSecretError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(value_size, error); if (conn->secretDriver != NULL && conn->secretDriver->getValue != NULL) { unsigned char *ret; @@ -14801,7 +14554,7 @@ int virStreamRef(virStreamPtr stream) { if ((!VIR_IS_CONNECTED_STREAM(stream))) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_STREAM, __FUNCTION__); virDispatchError(NULL); return -1; } @@ -14891,10 +14644,7 @@ int virStreamSend(virStreamPtr stream, return -1; } - if (data == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(data, error); if (stream->driver && stream->driver->streamSend) { @@ -14991,10 +14741,7 @@ int virStreamRecv(virStreamPtr stream, return -1; } - if (data == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(data, error); if (stream->driver && stream->driver->streamRecv) { @@ -15072,10 +14819,7 @@ int virStreamSendAll(virStreamPtr stream, return -1; } - if (handler == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + virCheckNonNullArgGoto(handler, cleanup); if (stream->flags & VIR_STREAM_NONBLOCK) { virLibConnError(VIR_ERR_OPERATION_INVALID, @@ -15174,10 +14918,7 @@ int virStreamRecvAll(virStreamPtr stream, return -1; } - if (handler == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + virCheckNonNullArgGoto(handler, cleanup); if (stream->flags & VIR_STREAM_NONBLOCK) { virLibConnError(VIR_ERR_OPERATION_INVALID, @@ -15768,10 +15509,8 @@ virConnectListNWFilters(virConnectPtr conn, char **const names, int maxnames) return -1; } - if ((names == NULL) || (maxnames < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(names, error); + virCheckNonNegativeArgGoto(maxnames, error); if (conn->nwfilterDriver && conn->nwfilterDriver->listNWFilters) { int ret; @@ -15811,10 +15550,7 @@ virNWFilterLookupByName(virConnectPtr conn, const char *name) virDispatchError(NULL); return NULL; } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(name, error); if (conn->nwfilterDriver && conn->nwfilterDriver->nwfilterLookupByName) { virNWFilterPtr ret; @@ -15853,10 +15589,7 @@ virNWFilterLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDispatchError(NULL); return NULL; } - if (uuid == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuid, error); if (conn->nwfilterDriver && conn->nwfilterDriver->nwfilterLookupByUUID){ virNWFilterPtr ret; @@ -15896,13 +15629,12 @@ virNWFilterLookupByUUIDString(virConnectPtr conn, const char *uuidstr) virDispatchError(NULL); return NULL; } - if (uuidstr == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuidstr, error); if (virUUIDParse(uuidstr, uuid) < 0) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virReportInvalidArg(uuidstr, + _("uuidstr in %s must be a valid UUID"), + __FUNCTION__); goto error; } @@ -15986,10 +15718,7 @@ virNWFilterGetUUID(virNWFilterPtr nwfilter, unsigned char *uuid) virDispatchError(NULL); return -1; } - if (uuid == NULL) { - virLibNWFilterError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuid, error); memcpy(uuid, &nwfilter->uuid[0], VIR_UUID_BUFLEN); @@ -16023,10 +15752,7 @@ virNWFilterGetUUIDString(virNWFilterPtr nwfilter, char *buf) virDispatchError(NULL); return -1; } - if (buf == NULL) { - virLibNWFilterError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(buf, error); if (virNWFilterGetUUID(nwfilter, &uuid[0])) goto error; @@ -16062,10 +15788,8 @@ virNWFilterDefineXML(virConnectPtr conn, const char *xmlDesc) virDispatchError(NULL); return NULL; } - if (xmlDesc == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xmlDesc, error); + if (conn->flags & VIR_CONNECT_RO) { virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; @@ -16197,7 +15921,7 @@ int virNWFilterRef(virNWFilterPtr nwfilter) { if ((!VIR_IS_CONNECTED_NWFILTER(nwfilter))) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virLibConnError(VIR_ERR_INVALID_NWFILTER, __FUNCTION__); virDispatchError(NULL); return -1; } @@ -16338,10 +16062,7 @@ virConnectCompareCPU(virConnectPtr conn, virDispatchError(NULL); return VIR_CPU_COMPARE_ERROR; } - if (xmlDesc == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xmlDesc, error); if (conn->driver->cpuCompare) { int ret; @@ -16395,10 +16116,7 @@ virConnectBaselineCPU(virConnectPtr conn, virDispatchError(NULL); return NULL; } - if (xmlCPUs == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xmlCPUs, error); if (conn->driver->cpuBaseline) { char *cpu; @@ -16441,10 +16159,7 @@ virDomainGetJobInfo(virDomainPtr domain, virDomainJobInfoPtr info) virDispatchError(NULL); return -1; } - if (info == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(info, error); memset(info, 0, sizeof(virDomainJobInfo)); @@ -16637,10 +16352,7 @@ virDomainMigrateGetMaxSpeed(virDomainPtr domain, conn = domain->conn; - if (!bandwidth) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(bandwidth, error); if (conn->flags & VIR_CONNECT_RO) { virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); @@ -16716,8 +16428,12 @@ virConnectDomainEventRegisterAny(virConnectPtr conn, virDispatchError(conn); return -1; } - if (eventID < 0 || eventID >= VIR_DOMAIN_EVENT_ID_LAST || cb == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virCheckNonNullArgGoto(cb, error); + virCheckNonNegativeArgGoto(eventID, error); + if (eventID >= VIR_DOMAIN_EVENT_ID_LAST) { + virReportInvalidArg(eventID, + _("eventID in %s must be less than %d"), + __FUNCTION__, VIR_DOMAIN_EVENT_ID_LAST); goto error; } @@ -16758,10 +16474,8 @@ virConnectDomainEventDeregisterAny(virConnectPtr conn, virDispatchError(NULL); return -1; } - if (callbackID < 0) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNegativeArgGoto(callbackID, error); + if ((conn->driver) && (conn->driver->domainEventDeregisterAny)) { int ret; ret = conn->driver->domainEventDeregisterAny(conn, callbackID); @@ -16826,8 +16540,9 @@ int virDomainManagedSave(virDomainPtr dom, unsigned int flags) } if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags & VIR_DOMAIN_SAVE_PAUSED)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("running and paused flags are mutually exclusive")); + virReportInvalidArg(flags, + _("running and paused flags in %s are mutually exclusive"), + __FUNCTION__); goto error; } @@ -17119,10 +16834,7 @@ virDomainSnapshotCreateXML(virDomainPtr domain, conn = domain->conn; - if (xmlDesc == NULL) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(xmlDesc, error); if (conn->flags & VIR_CONNECT_RO) { virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); @@ -17131,20 +16843,23 @@ virDomainSnapshotCreateXML(virDomainPtr domain, if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("use of current flag requires redefine flag")); + virReportInvalidArg(flags, + _("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)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("redefine and no metadata flags are mutually exclusive")); + virReportInvalidArg(flags, + _("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)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("redefine and halt flags are mutually exclusive")); + virReportInvalidArg(flags, + _("redefine and halt flags in %s are mutually exclusive"), + __FUNCTION__); goto error; } @@ -17308,10 +17023,8 @@ virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, conn = domain->conn; - if ((names == NULL) || (nameslen < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(names, error); + virCheckNonNegativeArgGoto(nameslen, error); if (conn->driver->domainSnapshotListNames) { int ret = conn->driver->domainSnapshotListNames(domain, names, @@ -17423,10 +17136,8 @@ virDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, conn = snapshot->domain->conn; - if ((names == NULL) || (nameslen < 0)) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(names, error); + virCheckNonNegativeArgGoto(nameslen, error); if (conn->driver->domainSnapshotListChildrenNames) { int ret = conn->driver->domainSnapshotListChildrenNames(snapshot, @@ -17475,10 +17186,7 @@ virDomainSnapshotLookupByName(virDomainPtr domain, conn = domain->conn; - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(name, error); if (conn->driver->domainSnapshotLookupByName) { virDomainSnapshotPtr dom; @@ -17688,8 +17396,9 @@ virDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if ((flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("running and paused flags are mutually exclusive")); + virReportInvalidArg(flags, + _("running and paused flags in %s are mutually exclusive"), + __FUNCTION__); goto error; } @@ -17755,9 +17464,10 @@ virDomainSnapshotDelete(virDomainSnapshotPtr snapshot, if ((flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) && (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("children and children_only flags are " - "mutually exclusive")); + virReportInvalidArg(flags, + _("children and children_only flags in %s are " + "mutually exclusive"), + __FUNCTION__); goto error; } @@ -17933,11 +17643,7 @@ int virDomainBlockJobAbort(virDomainPtr dom, const char *disk, goto error; } - if (!disk) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("disk is NULL")); - goto error; - } + virCheckNonNullArgGoto(disk, error); if (conn->driver->domainBlockJobAbort) { int ret; @@ -17989,17 +17695,8 @@ int virDomainGetBlockJobInfo(virDomainPtr dom, const char *disk, } conn = dom->conn; - if (!disk) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("disk is NULL")); - goto error; - } - - if (!info) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("info is NULL")); - goto error; - } + virCheckNonNullArgGoto(disk, error); + virCheckNonNullArgGoto(info, error); if (conn->driver->domainGetBlockJobInfo) { int ret; @@ -18057,11 +17754,7 @@ int virDomainBlockJobSetSpeed(virDomainPtr dom, const char *disk, goto error; } - if (!disk) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("disk is NULL")); - goto error; - } + virCheckNonNullArgGoto(disk, error); if (conn->driver->domainBlockJobSetSpeed) { int ret; @@ -18132,11 +17825,7 @@ int virDomainBlockPull(virDomainPtr dom, const char *disk, goto error; } - if (!disk) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("disk is NULL")); - goto error; - } + virCheckNonNullArgGoto(disk, error); if (conn->driver->domainBlockPull) { int ret; @@ -18254,23 +17943,16 @@ int virDomainBlockRebase(virDomainPtr dom, const char *disk, goto error; } - if (!disk) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("disk is NULL")); - goto error; - } + virCheckNonNullArgGoto(disk, error); if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY) { - if (!base) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("base is required when starting a copy")); - goto error; - } + virCheckNonNullArgGoto(base, error); } else if (flags & (VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | VIR_DOMAIN_BLOCK_REBASE_COPY_RAW)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("use of flags requires a copy job")); + virReportInvalidArg(flags, + _("use of flags in %s requires a copy job"), + __FUNCTION__); goto error; } @@ -18332,10 +18014,7 @@ int virDomainOpenGraphics(virDomainPtr dom, return -1; } - if (fd < 0) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNegativeArgGoto(fd, error); if (fstat(fd, &sb) < 0) { virReportSystemError(errno, @@ -18344,8 +18023,9 @@ int virDomainOpenGraphics(virDomainPtr dom, } if (!S_ISSOCK(sb.st_mode)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("File descriptor %d must be a socket"), fd); + virReportInvalidArg(fd, + _("fd %d in %s must be a socket"), + fd, __FUNCTION__); goto error; } @@ -18512,13 +18192,12 @@ int virDomainSetBlockIoTune(virDomainPtr dom, goto error; } - if (!disk || (nparams <= 0) || (params == NULL)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(disk, error); + virCheckPositiveArgGoto(nparams, error); + virCheckNonNullArgGoto(params, error); if (virTypedParameterValidateSet(dom, params, nparams) < 0) - return -1; + goto error; conn = dom->conn; @@ -18589,10 +18268,11 @@ int virDomainGetBlockIoTune(virDomainPtr dom, return -1; } - if (nparams == NULL || *nparams < 0 || - ((params == NULL || disk == NULL) && *nparams != 0)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; + virCheckNonNullArgGoto(nparams, error); + virCheckNonNegativeArgGoto(*nparams, error); + if (*nparams != 0) { + virCheckNonNullArgGoto(params, error); + virCheckNonNullArgGoto(disk, error); } if (VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, @@ -18602,7 +18282,9 @@ int virDomainGetBlockIoTune(virDomainPtr dom, /* 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__); goto error; } conn = dom->conn; @@ -18724,13 +18406,19 @@ 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 || - (start_cpu == -1 && ncpus != 1) || - ((params == NULL) != (nparams == 0)) || - (ncpus == 0 && params != NULL)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + 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 (nparams) + virCheckNonNullArgGoto(params, error); + else + virCheckNullArgGoto(params, error); + if (ncpus == 0) + virCheckNullArgGoto(params, error); + if (nparams && ncpus > UINT_MAX / nparams) { virLibDomainError(VIR_ERR_OVERFLOW, _("input too large: %u * %u"), nparams, ncpus); diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 1c4272b..f7d0cc6 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -451,8 +451,9 @@ int linuxNodeGetCPUStats(FILE *procstat, } if ((*nparams) != LINUX_NB_CPU_STATS) { - nodeReportError(VIR_ERR_INVALID_ARG, - "%s", _("Invalid parameter count")); + virReportInvalidArg(*nparams, + _("nparams in %s must be equal to %d"), + __FUNCTION__, LINUX_NB_CPU_STATS); goto cleanup; } @@ -526,7 +527,9 @@ int linuxNodeGetCPUStats(FILE *procstat, } } - nodeReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid cpu number")); + virReportInvalidArg(cpuNum, + _("Invalid cpuNum in %s"), + __FUNCTION__); cleanup: return ret; @@ -569,8 +572,9 @@ int linuxNodeGetMemoryStats(FILE *meminfo, } if ((*nparams) != nr_param) { - nodeReportError(VIR_ERR_INVALID_ARG, - "%s", _("Invalid stats count")); + virReportInvalidArg(nparams, + _("nparams in %s must be %d"), + __FUNCTION__, nr_param); goto cleanup; } @@ -779,8 +783,9 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED, # if HAVE_NUMACTL if (cellNum > numa_max_node()) { - nodeReportError(VIR_ERR_INVALID_ARG, "%s", - _("Invalid cell number")); + virReportInvalidArg(cellNum, + _("cellNum in %s must be less than or equal to %d"), + __FUNCTION__, numa_max_node()); return -1; } # endif diff --git a/src/util/virterror_internal.h b/src/util/virterror_internal.h index b8cb279..cb7df03 100644 --- a/src/util/virterror_internal.h +++ b/src/util/virterror_internal.h @@ -68,6 +68,83 @@ void virReportSystemErrorFull(int domcode, __FILE__, __FUNCTION__, __LINE__, \ (fmt), __VA_ARGS__) +# define virReportInvalidNullArg(argname) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + _("%s in %s must be NULL"), \ + #argname, __FUNCTION__) +# define virReportInvalidNonNullArg(argname) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + _("%s in %s must not be NULL"), \ + #argname, __FUNCTION__) +# define virReportInvalidPositiveArg(argname) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + _("%s in %s must greater than zero"), \ + #argname, __FUNCTION__) +# define virReportInvalidNonZeroArg(argname) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + _("%s in %s must not be zero"), \ + #argname, __FUNCTION__) +# define virReportInvalidZeroArg(argname) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + _("%s in %s must be zero"), \ + #argname, __FUNCTION__) +# define virReportInvalidNonNegativeArg(argname) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + _("%s in %s must be zero or greater"), \ + #argname, __FUNCTION__) +# define virReportInvalidArg(argname, fmt, ...) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + (fmt), __VA_ARGS__) + void virReportOOMErrorFull(int domcode, const char *filename, const char *funcname, -- 1.7.10.1

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.
TBD: many more source files to fixup
Agreed, but this is a good start.
--- src/datatypes.c | 119 ++--- src/internal.h | 61 ++- src/libvirt-qemu.c | 14 +- src/libvirt.c | 1160 +++++++++++++++-------------------------- src/nodeinfo.c | 19 +- src/util/virterror_internal.h | 77 +++ 6 files changed, 616 insertions(+), 834 deletions(-)
+++ b/src/util/virterror_internal.h @@ -68,6 +68,83 @@ void virReportSystemErrorFull(int domcode, __FILE__, __FUNCTION__, __LINE__, \ (fmt), __VA_ARGS__)
+# define virReportInvalidNullArg(argname) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + _("%s in %s must be NULL"), \ + #argname, __FUNCTION__)
Still feels a little redundant to report __FUNCTION__ in both the location and in the message, but at least the message is sane and by funnelling all clients through this one point you can change the message in the future with a lot less effort. I can live with it.
+# define virReportInvalidArg(argname, fmt, ...) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + (fmt), __VA_ARGS__)
This requires a fmt argument and subsequent arguments, probably a good thing since you aren't using it for a canned message. Is the idea that down the road you will add a syntax check that forbids raw use of VIR_ERR_INVALID_ARG, and must instead use virReportInvalidArg() or a better error category?
+++ b/src/internal.h @@ -232,17 +232,64 @@ do { \ unsigned long __unsuppflags = flags & ~(supported); \ if (__unsuppflags) { \ - virReportErrorHelper(VIR_FROM_THIS, \ - VIR_ERR_INVALID_ARG, \ - __FILE__, \ - __FUNCTION__, \ - __LINE__, \ - _("%s: unsupported flags (0x%lx)"), \ - __FUNCTION__, __unsuppflags); \ + virReportInvalidArg("flags", \ + _("unsupported flags (0x%lx) in function %s"), \ + __unsuppflags, __FUNCTION__); \
Good change - it might still print the function name twice, but now it won't be back-to-back 'func: func: unsupported...'
return retval; \ } \ } while (0)
+# define virCheckNonNullArgReturn(argname, retval) \ + do { \ + if (argname == NULL) { \ + virReportInvalidNullArg(#argname); \
Hmm. This stringizes 'argname', but then virReportInvalidNullArg() also stringizes its argument. Does that mean we are actually generating messages with literal quotes injected due to the double stringize? func: "arg" in func must not be NULL
diff --git a/src/datatypes.c b/src/datatypes.c index 0857f9a..d718170 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -138,7 +138,7 @@ virUnrefConnect(virConnectPtr conn) { int refs;
if ((!VIR_IS_CONNECT(conn))) { - virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); + virLibConnError(VIR_ERR_INVALID_CONN, _("no connection"));
Yep, correctness change.
return -1; } virMutexLock(&conn->lock); @@ -173,17 +173,12 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { char uuidstr[VIR_UUID_STRING_BUFLEN];
if (!VIR_IS_CONNECT(conn)) { - virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); - return NULL; - } - if (name == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing name")); - return NULL; - } - if (uuid == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, _("missing uuid")); + virLibConnError(VIR_ERR_INVALID_CONN, _("no connection")); return NULL; } + virCheckNonNullArgReturn(name, NULL); + virCheckNonNullArgReturn(uuid, NULL);
That is indeed shorter. I'm liking it already.
@@ -813,7 +790,7 @@ virUnrefStorageVol(virStorageVolPtr vol) { int refs;
if (!VIR_IS_CONNECTED_STORAGE_VOL(vol)) { - virLibConnError(VIR_ERR_INVALID_ARG, + virLibConnError(VIR_ERR_INVALID_STORAGE_VOL, _("bad storage volume or no connection")); return -1; }
I'm wondering if a future patch should factor out these checks for valid object into a one-liner as well, such as virCheckStorageVolReturn(vol, NULL). But doesn't need to be done for the current patch.
+++ b/src/libvirt.c
@@ -746,12 +725,6 @@ virRegisterDriver(virDriverPtr driver) return -1; }
- if (driver->no < 0) { - virLibConnError(VIR_ERR_INVALID_ARG, - _("Tried to register an internal Xen driver")); - return -1; - } -
Why'd we drop this one? But it looks okay.
@@ -4013,12 +3926,10 @@ virDomainSetNumaParameters(virDomainPtr domain, virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if ((nparams <= 0) || (params == NULL)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(params, error); + virCheckPositiveArgGoto(nparams, error); if (virTypedParameterValidateSet(domain, params, nparams) < 0) - return -1; + goto error;
Sneaking in real bug fixes here. :)
@@ -4142,12 +4053,11 @@ virDomainSetBlkioParameters(virDomainPtr domain, virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } - if ((nparams <= 0) || (params == NULL)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(params, error); + virCheckNonNegativeArgGoto(nparams, error); + if (virTypedParameterValidateSet(domain, params, nparams) < 0) - return -1; + goto error;
And again. Looks like copy-and-paste introduced them, and probably my fault. Thanks for catching this. But maybe the bug fix should be split into a separate patch for all instances like this? I won't insist, though, since splitting a huge patch is hard.
@@ -5018,8 +4917,18 @@ virDomainMigratePeer2Peer (virDomainPtr domain, return -1; }
- if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!tempuri->server) { + virReportInvalidArg(tempuri, + _("server field in tempuri in %s must not be NULL"), + __FUNCTION__);
Wrong message. tempuri is something we constructed ourselves (virURIParse), and not something the user passed in. Rather, we should say something like: virReportInvalidArg(dconnuri, _("Unable to parse 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'"),
Again wrong message; this should call out dconnuri that we parsed to get to tempuri.
@@ -7126,8 +7037,12 @@ virDomainBlockStats(virDomainPtr dom, const char *disk, virDispatchError(NULL); return -1; } - if (!disk || !stats || size > sizeof(stats2)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virCheckNonNullArgGoto(disk, error); + virCheckNonNullArgGoto(stats, error); + if (size > sizeof(stats2)) { + virReportInvalidArg(size, + _("size in %s must be less than %zu"),
"less than or equal to" or maybe: _("size in %s must not exceed %zu")
@@ -7266,10 +7182,15 @@ virDomainInterfaceStats (virDomainPtr dom, const char *path, virDispatchError(NULL); return -1; } - if (!path || !stats || size > sizeof(stats2)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virCheckNonNullArgGoto(path, error); + virCheckNonNullArgGoto(stats, error); + if (size > sizeof(stats2)) { + virReportInvalidArg(size, + _("size in %s must be less than %zu"),
Again.
@@ -8638,9 +8532,12 @@ virDomainPinVcpu(virDomainPtr domain, unsigned int vcpu, goto error; }
- if ((vcpu > 32000) || (cpumap == NULL) || (maplen < 1)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; + virCheckNonNullArgGoto(cpumap, error); + virCheckPositiveArgGoto(maplen, error); + + if ((unsigned short) vcpu != vcpu) {
Not strictly equivalent to '(vcpu > 32000)', but does a similar job (but only because 'vcpu' is unsigned; if it were signed, then you get into subtleties where it might not work).
@@ -8855,17 +8757,16 @@ virDomainGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo, virDispatchError(NULL); return -1; } - if ((info == NULL) || (maxinfo < 1)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(info, error); + virCheckPositiveArgGoto(maxinfo, error);
/* Ensure that domainGetVcpus (aka remoteDomainGetVcpus) does not try to memcpy anything into a NULL pointer. */ - if (!cpumaps ? maplen != 0 : maplen <= 0) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + if (cpumaps) + virCheckPositiveArgGoto(maplen, error); + else + virCheckZeroArgGoto(maplen, error);
Good thing you wrapped these macros in 'do{}while(0)'.
@@ -11590,13 +11413,12 @@ virStoragePoolLookupByUUIDString(virConnectPtr conn, virDispatchError(NULL); return NULL; } - if (uuidstr == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuidstr, error);
if (virUUIDParse(uuidstr, uuid) < 0) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virReportInvalidArg(uuidstr, + _("uuidstr in %s must be a valid UUID"), + __FUNCTION__); goto error;
The virUUIDParse() failure case is another frequent one that might be worth factoring into a one-liner.
@@ -14046,10 +13822,8 @@ virConnectDomainEventDeregister(virConnectPtr conn, virDispatchError(NULL); return -1; } - if (cb == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(cb, error);
Another nice side effect of all this cleanup: clang complains about using a printf-style format string with no % (which is true of all possible __FUNCTION__ value), the new macros still use __FUNCTION__, but as a proper argument to a format string, so we get fewer false positives of clang output to sift through.
@@ -14381,15 +14143,15 @@ virSecretGetUUID(virSecretPtr secret, unsigned char *uuid) virDispatchError(NULL); return -1; } - if (uuid == NULL) { - virLibSecretError(VIR_ERR_INVALID_ARG, __FUNCTION__); - virDispatchError(secret->conn); - return -1; - } + virCheckNonNullArgGoto(uuid, error);
memcpy(uuid, &secret->uuid[0], VIR_UUID_BUFLEN);
return 0; + +error: + virDispatchError(NULL);
Oops. Why the change from secret->conn to NULL?
@@ -16716,8 +16428,12 @@ virConnectDomainEventRegisterAny(virConnectPtr conn, virDispatchError(conn); return -1; } - if (eventID < 0 || eventID >= VIR_DOMAIN_EVENT_ID_LAST || cb == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virCheckNonNullArgGoto(cb, error); + virCheckNonNegativeArgGoto(eventID, error); + if (eventID >= VIR_DOMAIN_EVENT_ID_LAST) { + virReportInvalidArg(eventID, + _("eventID in %s must be less than %d"), + __FUNCTION__, VIR_DOMAIN_EVENT_ID_LAST);
No change in functionality by this hunk (so if you fix it, make it a separate patch), but I think we have an independent bug here. Suppose that I have a client compiled against 0.9.13 headers, but running against the 0.9.12 libvirt.so, and it is talking to an 0.9.13 server. This check prevents me from registering for an event new to 0.9.13, even though both client and server know about it, all because the intermediary RPC call is rejecting values. I think the check for eventID too large has to be pushed down into the drivers, not here in libvirt.c.
@@ -17131,20 +16843,23 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("use of current flag requires redefine flag")); + virReportInvalidArg(flags, + _("use of current flag in %s requires redefine flag"),
Elsewhere, you used quoting to make it obvious which flags were being talked about, as in: "use of 'current' flag in %s requires 'redefine' flag".
@@ -18724,13 +18406,19 @@ 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 || - (start_cpu == -1 && ncpus != 1) || - ((params == NULL) != (nparams == 0)) || - (ncpus == 0 && params != NULL)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (start_cpu < -1 && ncpus != -1) { + virReportInvalidArg(start_cpu, + _("start_cpu in %s must be -1 or greater if ncpus is not -1"), + __FUNCTION__);
Huh? You botched that conversion. Consider: start_cpu == -2, ncpus == 0 # old code rejected, new code lets through start_cpu == -1, ncpus == -1 # old code rejected, new code lets through I think you want: if (start_cpu == -1) { if (ncpus != 1) { virReportInvalidArg(ncpus, _("ncpus in %s must be 1 when start_cpu is -1"), __FUNCTION); goto error; } } else { virCheckNonNegativeArgGoto(start_cpu, error); }
+++ b/src/nodeinfo.c @@ -451,8 +451,9 @@ int linuxNodeGetCPUStats(FILE *procstat, }
if ((*nparams) != LINUX_NB_CPU_STATS) { - nodeReportError(VIR_ERR_INVALID_ARG, - "%s", _("Invalid parameter count")); + virReportInvalidArg(*nparams, + _("nparams in %s must be equal to %d"), + __FUNCTION__, LINUX_NB_CPU_STATS);
Faithful conversion, but latent bug that we should fix. I remember changing lots of similar situations to specifically allow *nparams to be smaller (truncate the result, good for new server talking to old library that only cares about the first parameter) or larger (ignore the extras, good for old server talking to new library that knows about more results, but can still behave when those results are not present). ACK with problems fixed. If you post a v2, post an interdiff (I don't want to scroll through the whole thing again :) -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Fri, May 25, 2012 at 05:35:20PM -0600, Eric Blake wrote:
+# define virReportInvalidNullArg(argname) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + _("%s in %s must be NULL"), \ + #argname, __FUNCTION__)
Still feels a little redundant to report __FUNCTION__ in both the location and in the message, but at least the message is sane and by funnelling all clients through this one point you can change the message in the future with a lot less effort. I can live with it.
Although when we send the error message onto virLogMessage(), the function name will be prepended to the error string, if the application has installed a custom handler for virErrorPtr they may well only be printing out the error message. Thus I want to make sure that the error message is "self contained" and not rely on people printing out other data fields from the virErrorPtr.
+# define virReportInvalidArg(argname, fmt, ...) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + (fmt), __VA_ARGS__)
This requires a fmt argument and subsequent arguments, probably a good thing since you aren't using it for a canned message.
Is the idea that down the road you will add a syntax check that forbids raw use of VIR_ERR_INVALID_ARG, and must instead use virReportInvalidArg() or a better error category?
Yes, I think we should deny direct use of VIR_ERR_INVALID_ARG once this conversion is complete.
return retval; \ } \ } while (0)
+# define virCheckNonNullArgReturn(argname, retval) \ + do { \ + if (argname == NULL) { \ + virReportInvalidNullArg(#argname); \
Hmm. This stringizes 'argname', but then virReportInvalidNullArg() also stringizes its argument. Does that mean we are actually generating messages with literal quotes injected due to the double stringize? func: "arg" in func must not be NULL
That double stringizing is a mistake.
@@ -813,7 +790,7 @@ virUnrefStorageVol(virStorageVolPtr vol) { int refs;
if (!VIR_IS_CONNECTED_STORAGE_VOL(vol)) { - virLibConnError(VIR_ERR_INVALID_ARG, + virLibConnError(VIR_ERR_INVALID_STORAGE_VOL, _("bad storage volume or no connection")); return -1; }
I'm wondering if a future patch should factor out these checks for valid object into a one-liner as well, such as virCheckStorageVolReturn(vol, NULL). But doesn't need to be done for the current patch.
Yes, shortening this is definitely my intention.
+++ b/src/libvirt.c
@@ -746,12 +725,6 @@ virRegisterDriver(virDriverPtr driver) return -1; }
- if (driver->no < 0) { - virLibConnError(VIR_ERR_INVALID_ARG, - _("Tried to register an internal Xen driver")); - return -1; - } -
Why'd we drop this one? But it looks okay.
Originally the Xen sub-drivers uses the same virDriverPtr struct, but now they have a dedicated xenUnifiedDriver struct instead.
@@ -5018,8 +4917,18 @@ virDomainMigratePeer2Peer (virDomainPtr domain, return -1; }
- if (!tempuri->server || STRPREFIX(tempuri->server, "localhost")) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!tempuri->server) { + virReportInvalidArg(tempuri, + _("server field in tempuri in %s must not be NULL"), + __FUNCTION__);
Wrong message. tempuri is something we constructed ourselves (virURIParse), and not something the user passed in. Rather, we should say something like:
virReportInvalidArg(dconnuri, _("Unable to parse server from dconnuri in %s"), __FUNCTION);
Ah, ok I missed that
@@ -8638,9 +8532,12 @@ virDomainPinVcpu(virDomainPtr domain, unsigned int vcpu, goto error; }
- if ((vcpu > 32000) || (cpumap == NULL) || (maplen < 1)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; + virCheckNonNullArgGoto(cpumap, error); + virCheckPositiveArgGoto(maplen, error); + + if ((unsigned short) vcpu != vcpu) {
Not strictly equivalent to '(vcpu > 32000)', but does a similar job (but only because 'vcpu' is unsigned; if it were signed, then you get into subtleties where it might not work).
I did this change, because several other places related to vCPUs were already doing this cast+compare, so it is better to be consistent about the precise point at which we raise errors.
@@ -11590,13 +11413,12 @@ virStoragePoolLookupByUUIDString(virConnectPtr conn, virDispatchError(NULL); return NULL; } - if (uuidstr == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + virCheckNonNullArgGoto(uuidstr, error);
if (virUUIDParse(uuidstr, uuid) < 0) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virReportInvalidArg(uuidstr, + _("uuidstr in %s must be a valid UUID"), + __FUNCTION__); goto error;
The virUUIDParse() failure case is another frequent one that might be worth factoring into a one-liner.
Yes, definitely. It is also my intent to simplify the checks for the read-only flag.
@@ -14381,15 +14143,15 @@ virSecretGetUUID(virSecretPtr secret, unsigned char *uuid) virDispatchError(NULL); return -1; } - if (uuid == NULL) { - virLibSecretError(VIR_ERR_INVALID_ARG, __FUNCTION__); - virDispatchError(secret->conn); - return -1; - } + virCheckNonNullArgGoto(uuid, error);
memcpy(uuid, &secret->uuid[0], VIR_UUID_BUFLEN);
return 0; + +error: + virDispatchError(NULL);
Oops. Why the change from secret->conn to NULL?
Bugtastic.
@@ -16716,8 +16428,12 @@ virConnectDomainEventRegisterAny(virConnectPtr conn, virDispatchError(conn); return -1; } - if (eventID < 0 || eventID >= VIR_DOMAIN_EVENT_ID_LAST || cb == NULL) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virCheckNonNullArgGoto(cb, error); + virCheckNonNegativeArgGoto(eventID, error); + if (eventID >= VIR_DOMAIN_EVENT_ID_LAST) { + virReportInvalidArg(eventID, + _("eventID in %s must be less than %d"), + __FUNCTION__, VIR_DOMAIN_EVENT_ID_LAST);
No change in functionality by this hunk (so if you fix it, make it a separate patch), but I think we have an independent bug here. Suppose that I have a client compiled against 0.9.13 headers, but running against the 0.9.12 libvirt.so, and it is talking to an 0.9.13 server. This check prevents me from registering for an event new to 0.9.13, even though both client and server know about it, all because the intermediary RPC call is rejecting values. I think the check for eventID too large has to be pushed down into the drivers, not here in libvirt.c.
Yes that is probably right.
@@ -17131,20 +16843,23 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("use of current flag requires redefine flag")); + virReportInvalidArg(flags, + _("use of current flag in %s requires redefine flag"),
Elsewhere, you used quoting to make it obvious which flags were being talked about, as in: "use of 'current' flag in %s requires 'redefine' flag".
Good point.
@@ -18724,13 +18406,19 @@ 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 || - (start_cpu == -1 && ncpus != 1) || - ((params == NULL) != (nparams == 0)) || - (ncpus == 0 && params != NULL)) { - virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (start_cpu < -1 && ncpus != -1) { + virReportInvalidArg(start_cpu, + _("start_cpu in %s must be -1 or greater if ncpus is not -1"), + __FUNCTION__);
Huh? You botched that conversion. Consider:
start_cpu == -2, ncpus == 0 # old code rejected, new code lets through
start_cpu == -1, ncpus == -1 # old code rejected, new code lets through
I think you want:
if (start_cpu == -1) { if (ncpus != 1) { virReportInvalidArg(ncpus, _("ncpus in %s must be 1 when start_cpu is -1"), __FUNCTION); goto error; } } else { virCheckNonNegativeArgGoto(start_cpu, error); }
Hmm, don't know what I was thinking there !
+++ b/src/nodeinfo.c @@ -451,8 +451,9 @@ int linuxNodeGetCPUStats(FILE *procstat, }
if ((*nparams) != LINUX_NB_CPU_STATS) { - nodeReportError(VIR_ERR_INVALID_ARG, - "%s", _("Invalid parameter count")); + virReportInvalidArg(*nparams, + _("nparams in %s must be equal to %d"), + __FUNCTION__, LINUX_NB_CPU_STATS);
Faithful conversion, but latent bug that we should fix. I remember changing lots of similar situations to specifically allow *nparams to be smaller (truncate the result, good for new server talking to old library that only cares about the first parameter) or larger (ignore the extras, good for old server talking to new library that knows about more results, but can still behave when those results are not present).
Yes, it is a little odd, but before changing this, I'd have to examine the code in more detail 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 :|

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 :|

On 05/28/2012 07:34 AM, Daniel P. Berrange wrote:
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
+++ 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"),
s/parser/parse/
__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"),
and again. ACK with those changes. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange
-
Eric Blake