On 05/25/2012 11:44 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list