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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list