Re: [libvirt] [PATCH 08/24] maint: improve VIR_ERR_NO_SUPPORT usage
On 01/02/2014 03:54 PM, John Ferlan wrote:
>> Note that in libvirt.c, we were inconsistent on whether
virDomain*
>> API used virLibConnError() (with VIR_FROM_NONE) or virLibDomainError()
>> (with VIR_FROM_DOMAIN); this patch unifies these errors to all use
>> VIR_FROM_NONE, on the grounds that it is unlikely that a caller
>> learning that a call is unimplemented can do anything in particular
>> with extra knowledge of which error domain it belongs to.
>>
> Thus all error messages of this type will start with
"libvirt: Unknown"
> rather than "some" starting with "libvirt: <domain-name>".
Not quite. VIR_FROM_DOMAIN does not magically inject <domain-name> into
the error message, it is only a meta-category that the end client can
use to get an idea of where the error was issued (VIR_FROM_NONE in a
generic file, VIR_FROM_DOMAIN in a file related mainly to common domain
operations, VIR_FROM_QEMU in the qemu backend, and so forth). virsh
uses this information in a couple of places, but it generally doesn't
provide quite as much information as the actual error message (and the
error message is unchanged).
hmm.. after re-reading virRaiseErrorFull(), virDefaultErrorFunc(), and
friends - seems my eyes weren't functioning with my brain completely
with regard to error domain and domain domain... I'm less concerned now
with my original thoughts. Guess some of the work recently in virt-test
has made me a bit more sensitive to changing error messages as believe
it or not, they are used to make decisions in some tests to FAIL or SKIP
based on whether some functionality exists. For example, if hotplug
isn't supported an error is returned "cannot change vcpu count of this
domain" - and that's a decision point for returning SKIP or FAIL.
Yep, glad to see that you figured this out in the meantime.
> BlockCommit
> virDomainOpenGraphics <- Different than rest in the way it's checked
> SetBlockIoTune
> GetBlockIoTune
> GetCPUStats
>
> The only one that I'd say is different is virDomainOpenGraphics(). It
> checks VIR_DRV_SUPPORTS_FEATURE on one of its calls to
> virLibDomainError(). Thus perhaps it'd be better to generate a "real"
> error so as to differentiate between the function not being available as
> a general rule of thumb as opposed to it not being available to a
> specific domain because the domain doesn't support a specific feature.
> In this case VIR_DRV_FEATURE_FD_PASSING supported in the driver.
Okay, I'll revisit that function, and possibly just defer that hunk or a
modified version of it to my v2 series so I can push the rest of the patch.
>
> I'm OK with an ACK - I just wanted to provide the "counter point"
though
> to why a caller may want to know the domain name of an unimplemented
> function. Python makes it all too easy to snag error messages and make
> decisions based on "known" fields.
Again, it's not the domain name, but the category of which "type" of
error is being issued, but knowing whether an unimplemented function is
of type VIR_FROM_NONE or VIR_FROM_DOMAIN doesn't really help you use the
function - either way, it still isn't implemented :) But I do
appreciate the caution of making me justify why it is okay to make
user-visible changes.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)