Re: [libvirt] [PATCH] Unify most error reporting (ver 2)
On Fri, Oct 10, 2008 at 11:59:34AM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> On Fri, Oct 10, 2008 at 11:47:49AM +0200, Jim Meyering wrote:
>> Daniel Veillard <veillard(a)redhat.com> wrote:
>>
>> > On Thu, Oct 09, 2008 at 11:44:02AM -0400, Cole Robinson wrote:
>> >> Daniel Veillard wrote:
>> >> > On Wed, Oct 08, 2008 at 08:35:38PM +0100, Daniel P. Berrange
wrote:
>> >
>> > Hum, this breaks "make check"
>> >
>> > PASS: read-non-seekable
>> > --- out 2008-10-10 10:23:57.000000000 +0200
>> > +++ exp 2008-10-10 10:23:57.000000000 +0200
>> > @@ -1,2 +1,2 @@
>> > -libvir: Test error : internal error Domain is still running
>> > +libvir: Test error test: internal error Domain is still running
>> > error: Failed to undefine domain test
>> > FAIL: undefine
>> >
>> > Seems we loose the domain name in the error message when trying to
>> > undefine a running domain:
>> >
>> > paphio:~/libvirt/tests -> /usr/bin/virsh -q -c test:///default undefine
>> > test
>> > libvir: Test error test: internal error Domain is still running
>> > error: Failed to undefine domain test
>> > paphio:~/libvirt/tests -> ../src/virsh -q -c test:///default undefine
>> > test
>> > libvir: Test error : internal error Domain is still running
>> > error: Failed to undefine domain test
>> > paphio:~/libvirt/tests ->
>> >
>> > I should be possible to restore the old behaviour of reporting the domain
>> > name there
>>
>> The documentation suggests that the new interface was initially
>> supposed to have dom and net parameters:
>
> We removed them because they're deprecated and 90% of the driver code
> never provides them - the test driver was the exception to the rule here
Oh well.
FYI, the other exceptions:
lxc_conf.h: lxcError (dom-only)
qemu_conf.h: qemudReportError
Some parts of qemu supply it, many other parts do not since they
have no access to the virDomainPtr object. This is one of the
reasons for deprecating this field - it was impossible to reliably
provide it when raising errors.
In this particular test case error, we are better off supplying the
domain name in the format string - it'll improve the error message
to have it placed in context of the description
eg, instead of
libvir: Test error test: internal error Domain is still running
We'd have
libvir: Test error: internal error Domain 'test' is still running
Which could be done by changing
if (privdom->state != VIR_DOMAIN_SHUTOFF) {
testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR,
_("Domain is still running"));
to be
if (privdom->state != VIR_DOMAIN_SHUTOFF) {
testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
_("Domain '%s' is still running"), domain->name);
We already follow this pattern throughout the QEMU driver
eg
vm = virDomainFindByName(&driver->domains, def->name);
if (vm) {
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
_("domain '%s' is already defined"),
def->name);
goto cleanup;
}
eg
virUUIDFormat(def->uuid, uuidstr);
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
_("domain with uuid '%s' is already defined"),
uuidstr);
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|