Re: [libvirt] [PATCH 2/2] maint: improve debug of libvirt-{qemu, lxc} apis

On 12/19/2013 08:17 AM, Daniel P. Berrange wrote: > On Thu, Dec 19, 2013 at 08:13:36AM -0700, Eric Blake wrote: >> diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h >> index 115d8d1..b8c842d 100644 >> --- a/src/libvirt_internal.h >> +++ b/src/libvirt_internal.h >> @@ -27,6 +27,113 @@ >> >> # include "internal.h" >> >> +#define VIR_DOMAIN_DEBUG(...) \ >> + VIR_DOMAIN_DEBUG_EXPAND(VIR_DOMAIN_DEBUG_, \ >> + VIR_HAS_COMMA(__VA_ARGS__), \ >> + __VA_ARGS__) > > I'd suggest these go in datatypes.h Good idea. > > >> + >> +/** >> + * VIR_UUID_DEBUG: >> + * @conn: connection >> + * @uuid: possibly null UUID array >> + */ >> +#define VIR_UUID_DEBUG(conn, uuid) \ >> + do { \ >> + if (uuid) { \ >> + char _uuidstr[VIR_UUID_STRING_BUFLEN]; \ >> + virUUIDFormat(uuid, _uuidstr); \ >> + VIR_DEBUG("conn=%p, uuid=%s", conn, _uuidstr); \ >> + } else { \ >> + VIR_DEBUG("conn=%p, uuid=(null)", conn); \ >> + } \ >> + } while (0) > > > And this in viruuid.h Sure, although VIR_DOMAIN_DEBUG requires the use of viruuid.h in the first place. >> +#define virLibDomainSnapshotError(code, ...) \ >> + virReportErrorHelper(VIR_FROM_DOMAIN_SNAPSHOT, code, __FILE__, \ >> + __FUNCTION__, __LINE__, __VA_ARGS__) > > I'd venture to sugggest that these functions really have little > to no benefit / reason to exist. They aren't really simplifying > like, just making it different. They're historical baggage from > the old time when they were actual functions which did extra > sprintf formatting work. Could we just remove them ?? Perhaps, by using virReportError instead. I can give that a try; but just looking at it, virReportError() hardcodes VIR_FROM_THIS as its first argument, whereas virLib*Error() sets different VIR_FROM_* categories. Worse, we use multiple calls from within a single function (so continually redefining VIR_FROM_THIS would get painful) - for example, virDomainSnapshotCreateXML() uses both virLibDomainError() (VIR_FROM_DOMAIN) and virLibConnError() (VIR_FROM_NONE).