[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: Report cmdline output if VM dies early



On 05/04/2010 04:58 PM, Eric Blake wrote:
> On 04/30/2010 09:44 AM, Cole Robinson wrote:
>> qemuReadLogOutput early VM death detection is racy and won't always work.
>> Startup then errors when connecting to the VM monitor. This won't report
>> the emulator cmdline output which is typically the most useful diagnostic.
>>
>> Check if the VM has died at the very end of the monitor connection step,
>> and if so, report the cmdline output.
> 
>> +static void
>> +qemuReadLogFD(int logfd, char *buf, int maxlen, int off)
>> +{
>> +    int ret;
>> +    char *tmpbuf = buf+off;
> 
> Isn't the style to separate operators by space?  'buf + off'
> 
>> +
>> +    ret = saferead(logfd, tmpbuf, maxlen-off-1);
> 
> Likewise, 'maxlen - off - 1'.
> 
>> +    if (ret < 0) {
>> +        ret = 0;
>> +    }
>> +
>> +    *(tmpbuf+ret) = '\0';
> 
> Stylistically, I like 'tmpbuf[ret]' better than '*(tmpbuf+ret)'.
> 

Doh, I was overthinking here :)

>> +}
>> +
>>  static int
>>  qemudWaitForMonitor(struct qemud_driver* driver,
>>                      virDomainObjPtr vm, off_t pos)
>>  {
>> -    char buf[4096]; /* Plenty of space to get startup greeting */
>> +    char buf[4096] = "\0"; /* Plenty of space to get startup greeting */
> 
> "" is sufficient; you don't have to use "\0" to zero-initialize a
> statically sized char[].
> 
> But overall, the patch looks sane; I didn't see any logic errors.
> ACK with the style nits addressed.
> 

Thanks for the review, I made these style adjustments and pushed.

- Cole


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]