On Wed, Jan 05, 2011 at 09:34:20AM -0700, Eric Blake wrote:
> On 01/05/2011 03:40 AM, Osier Yang wrote:
> >> Then maybe the patch should be altered to output both name and UUID
> >> (probably by introducing a helper function, which when given a
> >> virDomainPtr outputs all three pieces of debug information rather than
> >> the current %p). I like the idea behind the patch, but only if we can
> >> come up with the correct way of getting the extra information when
> >> debugging is enabled, and without adding extra time when debugging is
> >> off. I'm not even sure what the right internal APIs for the job would
> >> be, or if they even exist yet.
> >>
> >
> > Hi, Eric,
> >
> > I implemented a helper function like so in src/libvirt.c:
> >
> > static const char *
> > virDomainNameUUID(virDomainPtr domain) {
> > char *entry = NULL;
> > const char *name = NULL;
> > char uuidstr[VIR_UUID_STRING_BUFLEN];
> >
> > if (!VIR_IS_DOMAIN(domain)) {
> > entry = strdup("(VM: invalid domain)");
>
> malloc'd return...
>
> > return entry;
> > }
> >
> > name = domain->name;
> > virUUIDFormat(domain->uuid, uuidstr);
> >
> > if(virAsprintf(&entry, "(VM: name=%s, uuid=%s)", name,
> > uuidstr) < 0)
> > virReportOOMError();
> >
> > return NULLSTR(entry);
>
> and static return. Ouch - that means the caller doesn't know whether to
> call free or not. You have to be consistent (the result is either NULL
> or malloc'd; or the result is always static).
>
> > }
> >
> > But how to avoid extra time when debugging is off? Following
> > is not correct way definitely, as malloc'ed "entry" need to be
> > freed.
> >
> > virConnectPtr
> > virDomainGetConnect (virDomainPtr dom)
> > {
> > DEBUG("dom=%p, %s", dom, virDomainNameUUID(dom));
>
> When I envisioned a helper function, I'm thinking more like callers
> using a simpler:
>
> DEBUG_DOMAIN(dom)
>
> and a helper like:
>
> void
> DEBUG_DOMAIN (virDomainPtr dom)
> {
> if !DEBUG then early exit
> gather data
> DEBUG("dom=%p, %s", dom, virDomainNameUUID(dom));
> free any gathered data if needed
> }
>
> That would mean that your new helper function would need to look at the
> guts of DEBUG to mirror the same decision of whether DEBUG will result
> in any output.
Ideally it'd be a macro that accepts extra args too eg
perhaps
#define DEBUG_DOMAIN(dom, fmt, ...) \
do {
char uuidstr[VIR_UUID_STRING_BUFLEN];
char *name;
if (VIR_IS_DOMAIN(dom) {
virUUIDFormat(uuidstr, dom->uuid);
name=dom->name;
} else {
memset(uuidstr, 0, sizeof(uuidstr));
}
DEBUG("dom=%p (name=%s, uuid=%s) " fmt,
NULLSTR(name), uuidstr, __VA_ARGS__)
} while(0)
NB, this relies on 'fmt' being a literal string in the
caller, to avoid malloc'ing, but that's already true.
This should be low enough overhead that we don't need
to have "if ! DEBUG then return" - at least not seriously
worse than our existing overhead - its only adding a
single virUUIDFormat call
So
int
virDomainSetMaxMemory(virDomainPtr domain, unsigned long memory)
{
virConnectPtr conn;
DEBUG("domain=%p, memory=%lu", domain, memory);
would thus become
int
virDomainSetMaxMemory(virDomainPtr domain, unsigned long memory)
{
virConnectPtr conn;
DEBUG_DOMAIN(domain, "memory=%lu", memory);
Daniel
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list