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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list