[libvirt] [PATCH] Unify most error reporting (ver 2)

[Daniel P. Berrange]

On Fri, Oct 10, 2008 at 11:59:34AM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange at redhat.com> wrote:
> 
> > On Fri, Oct 10, 2008 at 11:47:49AM +0200, Jim Meyering wrote:
> >> Daniel Veillard <veillard at 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 :|