4:53 a.m.
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 :|