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.
Attachment:
virsh_log.patch
Description: Binary data