[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Santize the reporting of VIR_ERR_INVALID_ERROR



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]