[Libvir] [PATCH] output virsh log to file

[Nobuhiro Itou]

Hi,

Thank you for your review.
How about this correction?

> Actually I had a closer look at this patch, and there are some problems 
> (thanks to Jim Meyering who pointed these out).  For example:
> 
> Using sprintf into a fixed size message buffer with no other checks may 
> cause a buffer overflow:
> 
> +    sprintf(msg_buf, "[%d.%02d.%02d %02d:%02d:%02d ",

I corrected to use snprintf.

> You should check the return value of write:
> 
> +    /* write log */
> +    write(logdef.log_fd, msg_buf, msg_len);

I added the return value check and error handling.

> You don't need to zero out the stat structure before calling stat:
> 
> +    memset(&st, 0x00, sizeof(struct stat));
> +    if (stat(logdef.path_buff, &st) == 0) {

Okey, I removed memset.

> What happens if this call fails?
> 
> +            rename(logdef.path_buff, bak_new_path);

I added error handling.

> But my broader point is: What use would this feature be, since you can 
> capture the output of virsh easily using shell redirection?  The Xen 
> 'xm' command doesn't have this feature and I don't know if anyone has 
> asked for it.

If I use it, the redirection is no problem.
However, when our customers are made to use virsh, it is necessary to 
explain the redirection.
Mostly, a lot of customers do not seem to use the redirection usually. 
And, executing the command applying the redirection to customers again 
when the error occurs is troublesome of customers. 

Therefore, the purpose is to make it immediately correspond to 
the customer's trouble report.


Thanks,
Nobuhiro Itou.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: virsh_log.patch
Type: application/octet-stream
Size: 9514 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20070517/97a521bc/attachment-0001.obj>