On Tue, Jul 07, 2009 at 12:52:48PM -0400, Cole Robinson wrote:
> Kind of a tangent, but...
>
> Can we go back to using macros for all this duplicate validation? For
> example, the idiom:
>
> if (!vm) {
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> virUUIDFormat(dom->uuid, uuidstr);
> qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN,
> _("no domain with matching uuid '%s'"),
> uuidstr);
> goto cleanup;
> }
>
> Takes up over 200 lines in qemu_driver.c. Sure, having a goto in a macro
> seems disgusting, but label names are used consistently so it hopefully
> wouldn't cause any problems.
I don't really like the use of macros when flow control is
involved because I think its important for people reading
the code to clearly see code paths.
I agree the amount of code involved here is getting a bit
cumbersome though, particularly for UUID based errors.
Part of the problem here is the tedious conversion from
the raw UUID to printable UUID. This is causing us pain
elsewhere - we hash virDomainPtr objects based on name
since you can't use a raw UUID as a hash key. I think
we should do a couple of things:
* Change virDomainPtr internal struct to store the
printable UUID in canonical format. This would
allow us to remove all the
char uuidstr[VIR_UUID_STRING_BUFLEN];
virUUIDFormat(dom->uuid, uuidstr);
* Add a macro around the qemuReportError()
call, specifically for the VIR_ERR_NO_DOMAIN
case, in same way we did for VIR_ERR_NO_MEMORY
and VIR_ERR_SYSTEM_ERROR.
#define virReportNoDomain(dom) \
virReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN,
_("no domain with matching name '%s' uuid '%s'"),
dom->name, dom->uuid)
This would make code to report an error look like
if (!vm) {
virReportNoDomain(dom);
goto error;
}
* Change virConnectPtr/virGetDomain to do hashing
based on dom->uuid, for better uniqueness which
would avoid some problems we've seen in virt-manager
with name based hashing.
Further along, I'd also like to see us try to get some more generic
helper methods for certain API operations. For example, LXC, QEMU
and UML drivers all have very similar code for certain methods which
could likely be pulled into a shared module
> Would there be any objections to reintroducing macros for this stuff?
> Can CIL handle macros when doing the lock checking?
CIL works on the intermediate files between the cpp and compile phase,
so it doesn't care about macros from a functional POV. The only problem
is human interpretation of the error location info it produces, since
you'll get the line number of the macro call, not th eexact line within
the macro - same problem you see in GDB when debugging code with macros.
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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list