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

On 05/25/2012 11:44 AM, Daniel P. Berrange wrote: ... Looks useful. I'll review the macros first. Agreed, but this is a good start. 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. 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? function %s"), \ Good change - it might still print the function name twice, but now it won't be back-to-back 'func: func: unsupported...' 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 Yep, correctness change. That is indeed shorter. I'm liking it already. 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. Why'd we drop this one? But it looks okay. Sneaking in real bug fixes here. :) 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. 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); Again wrong message; this should call out dconnuri that we parsed to get to tempuri. "less than or equal to" or maybe: _("size in %s must not exceed %zu") Again. 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). Good thing you wrapped these macros in 'do{}while(0)'. The virUUIDParse() failure case is another frequent one that might be worth factoring into a one-liner. 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. Oops. Why the change from secret->conn to NULL? 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. 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". 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); } 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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org