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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list