On 05/17/2010 08:54 AM, Daniel P. Berrange wrote:
int ret; - DEBUG("mon=%p, fd=%d", mon, mon->fd); + DEBUG("mon=%p", mon); + + if (!mon) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + }
Wouldn't it be better to move the DEBUG() to be after the (!mon) check, so that we can still print mon->fd? (Throughout the patch).
Yes & no. In practice I've never found a need to look at the 'fd' parameter being logged. So I think it is more important to log every call, so that we can see cases there 'mon=(null)' instead of them being skipped.
Fair enough; besides, the fact that mon is logged, a gdb session can easily get at mon->fd from the logged pointer.
Likewise. And while it was nice to add password=%p,...
if (!password) password = "";
...you may have just dereferenced another NULL pointer (at least DEBUG tends to only be used with glibc, where you get a sane "(null)" instead of a crash).
Where is the NULL de-reference ? We're using %p, not %s for logging the password because we don't want to actually expose the password string in the logs & %p doesn't de-reference the pointer.
Chalk it up to an early morning on my part. You're right, that %p is safe on NULL. So, given your rebuttals, none of my points remain, and you now have my: ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org