[Libvir] [PATCH] output virsh log to file
Hi, I made the patch which output debug messages and error messages of virsh to the log file. I think that it is necessary to leave logs to the file for the investigation at the failure. Debug messages can take appointed optional information, and error messages will make it easy to specify a cause. Moreover, I'm thinking about outputting virterror information to the virsh log file by the next step. It is simple specifications as follows. * Destination -- <USER HOME>/.virsh/virsh.log * Log size -- 1MB(1024*1024) * Number of Rotations -- 5 * There is process exclusion. First, will you take in this patch ? And, I have a plan to enhance logging functionality. (for example, libvirt error logging) Signed-off-by: Nobuhiro Itou <fj0873gn@aa.jp.fujitsu.com> Thanks, Nobuhiro Itou.
Nobuhiro Itou wrote:
Hi,
I made the patch which output debug messages and error messages of virsh to the log file. I think that it is necessary to leave logs to the file for the investigation at the failure. Debug messages can take appointed optional information, and error messages will make it easy to specify a cause. Moreover, I'm thinking about outputting virterror information to the virsh log file by the next step.
It is simple specifications as follows. * Destination -- <USER HOME>/.virsh/virsh.log * Log size -- 1MB(1024*1024) * Number of Rotations -- 5 * There is process exclusion.
First, will you take in this patch ?
Well, this patch looks OK to me. What would be wrong with just virsh ... 2>/tmp/logfile?
And, I have a plan to enhance logging functionality. (for example, libvirt error logging)
Can you explain? Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
Hi,
I made the patch which output debug messages and error messages of virsh to the log file. I think that it is necessary to leave logs to the file for the investigation at the failure. Debug messages can take appointed optional information, and error messages will make it easy to specify a cause. Moreover, I'm thinking about outputting virterror information to the virsh log file by the next step.
It is simple specifications as follows. * Destination -- <USER HOME>/.virsh/virsh.log * Log size -- 1MB(1024*1024) * Number of Rotations -- 5 * There is process exclusion.
First, will you take in this patch ?
Well, this patch looks OK to me.
What would be wrong with just virsh ... 2>/tmp/logfile?
Our team intends to let users use virsh as a part of the our team product. Therefore, it is not kind of to let users use redirection. When users use virsh, and an error occurred, our team wants to investigate it without letting users take trouble.
And, I have a plan to enhance logging functionality. (for example, libvirt error logging)
Can you explain?
Because this patch cannot gather information of virterror yet, I plan to add the file output processing to error handler. Thanks, Nobuhiro Itou.
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 ", You should check the return value of write: + /* write log */ + write(logdef.log_fd, msg_buf, msg_len); 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) { What happens if this call fails? + rename(logdef.path_buff, bak_new_path); 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. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
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.
Nobuhiro Itou wrote:
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.
Technically I think you've fixed all the problems I raised. I really think we should be looking at either a logging library, or (better) syslog. These problems -- like logfile maxsize, logfile location, log rotation -- have all been solved before, and I don't believe we should reinvent this wheel. I'm keen to hear what others think though. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, May 22, 2007 at 03:15:03PM +0100, Richard W.M. Jones wrote:
Nobuhiro Itou wrote:
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.
Technically I think you've fixed all the problems I raised.
I really think we should be looking at either a logging library, or (better) syslog. These problems -- like logfile maxsize, logfile location, log rotation -- have all been solved before, and I don't believe we should reinvent this wheel.
I'm keen to hear what others think though.
I'm not convinced that virsh should have / needs persistent logging like this. The commands run via it really map straight onto individual APIs, and as such the error from any one command has no where near enough context to provide useful information. The virt-install/virt-manager logs are useful because they typically do have a good amount of context logged enabling easy error diagnosis fro the logs. I just don't see logs from virsh being anymore useful, than the stuff already printed on STDOUT/STDERR which a user will already cut+paste into bug repots quite happily. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Hi, Dan Thank you for your comment.
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.
Technically I think you've fixed all the problems I raised.
I really think we should be looking at either a logging library, or (better) syslog. These problems -- like logfile maxsize, logfile location, log rotation -- have all been solved before, and I don't believe we should reinvent this wheel.
I'm keen to hear what others think though.
I'm not convinced that virsh should have / needs persistent logging like this. The commands run via it really map straight onto individual APIs, and as such the error from any one command has no where near enough context to provide useful information. The virt-install/virt-manager logs are useful because they typically do have a good amount of context logged enabling easy error diagnosis fro the logs. I just don't see logs from virsh being anymore useful, than the stuff already printed on STDOUT/STDERR which a user will already cut+paste into bug repots quite happily.
Certainly, virsh does not provide enough information now. But I want virsh to provide useful information in the future. First, I want to implement this logging function. In addition, I think that we should not let customers take trouble. Cannot you apply this patch? Thanks, Nobuhiro Itou.
Nobuhiro Itou wrote:
Certainly, virsh does not provide enough information now. But I want virsh to provide useful information in the future. First, I want to implement this logging function.
In addition, I think that we should not let customers take trouble.
Cannot you apply this patch?
The trouble is - the maintainers of libvirt have to maintain and live with this patch forever, so they need to make sure it's right before it goes in. I wonder if a simpler solution can't be found which doesn't impact libvirt/virsh codebase at all, but still allows persistent logging. If you wrap virsh in a shell script which runs logger, something like this: ------------------------------ #!/bin/bash - real_virsh=/usr/bin/virsh $real_virsh "$@" 2> >(logger -t virsh) ------------------------------ (Well, that's not quite right because it _only_ sends stderr to syslog, but it's easy to fix that using something like 'tee'). This doesn't have any maintenance impact on libvirt code. It sends logs to where users will expect to find them. Log rotation is handled. Remote logging is handled. And it's probably a lot more secure. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
Hi, Rich Thank you for your suggestion. We also think this kind of solution. But from our view, the best solution is inclusion of this patch to libvirt. the second solution is to use your suggestion. So his mail is asking the logging policy of libvirt. Thanks Atsushi SAKAI "Richard W.M. Jones" <rjones@redhat.com> wrote:
Nobuhiro Itou wrote:
Certainly, virsh does not provide enough information now. But I want virsh to provide useful information in the future. First, I want to implement this logging function.
In addition, I think that we should not let customers take trouble.
Cannot you apply this patch?
The trouble is - the maintainers of libvirt have to maintain and live with this patch forever, so they need to make sure it's right before it goes in.
I wonder if a simpler solution can't be found which doesn't impact libvirt/virsh codebase at all, but still allows persistent logging. If you wrap virsh in a shell script which runs logger, something like this:
------------------------------ #!/bin/bash -
real_virsh=/usr/bin/virsh
$real_virsh "$@" 2> >(logger -t virsh) ------------------------------
(Well, that's not quite right because it _only_ sends stderr to syslog, but it's easy to fix that using something like 'tee').
This doesn't have any maintenance impact on libvirt code. It sends logs to where users will expect to find them. Log rotation is handled. Remote logging is handled. And it's probably a lot more secure.
Rich.
-- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, 2007-05-22 at 20:32 +0100, Daniel P. Berrange wrote:
On Tue, May 22, 2007 at 03:15:03PM +0100, Richard W.M. Jones wrote:
I really think we should be looking at either a logging library, or (better) syslog. These problems -- like logfile maxsize, logfile location, log rotation -- have all been solved before, and I don't believe we should reinvent this wheel.
I'm keen to hear what others think though.
I'm not convinced that virsh should have / needs persistent logging like this. The commands run via it really map straight onto individual APIs, and as such the error from any one command has no where near enough context to provide useful information.
I'm not convinced virsh needs this either: - What other similar shell logs its errors/debug to a file? - You generally want to see debug/error messages on stdout/stderr when debugging something, rather than having to go poking in a separate log file e.g. the log won't be much good if I'm doing: $> ssh markmc@foo virsh --debug 6 list - Logging stuff to ~/.virsh is not something people will expect - I'd imagine most people would be surprised to find a log file - Log rotation, log file size, remote logging, log filtering etc. are all solved problems ... let's not try and re-solve them Cheers, Mark.
On Tue, May 22, 2007 at 03:15:03PM +0100, Richard W.M. Jones wrote:
Nobuhiro Itou wrote:
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.
Technically I think you've fixed all the problems I raised.
I really think we should be looking at either a logging library, or (better) syslog. These problems -- like logfile maxsize, logfile location, log rotation -- have all been solved before, and I don't believe we should reinvent this wheel.
I'm keen to hear what others think though.
My experience with systematic logging at command line level is that you end up polluting your file system with (hidden) log files, and while it may be fine for a developper it's not something you want to inflict to your customers. Also the fixed name for the log file makes it easilly useless, a random run in the same directory will just wipe out the data you tried to collect in a separate process. I think having permanent systematic logging to a fixed file is not proper, I would rather not use that. But since the patch is relatively simple based on existing virsh logging code, I think this could go as a command line option for virsh, for example --log filename where the detailed logs can then be saved if needed when a problem occurs. I think this would avoid the main drawbacks of your proposed patch. thanks, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard wrote: [...] Morning Daniel,
My experience with systematic logging at command line level is that you end up polluting your file system with (hidden) log files, and while it may be fine for a developper it's not something you want to inflict to your customers. Also the fixed name for the log file makes it easilly useless, a random run in the same directory will just wipe out the data you tried to collect in a separate process. I think having permanent systematic logging to a fixed file is not proper, I would rather not use that.
Agreed.
But since the patch is relatively simple based on existing virsh logging code, I think this could go as a command line option for virsh, for example --log filename where the detailed logs can then be saved if needed when a problem occurs. I think this would avoid the main drawbacks of your proposed patch.
Did you see my other response to this? https://www.redhat.com/archives/libvir-list/2007-May/msg00256.html That script, modified a bit further (see attachment) does logging fairly well. Having it invoked as a wrapper/replacement for virsh is just a matter of packaging. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, May 29, 2007 at 11:52:08AM +0100, Richard W.M. Jones wrote:
Did you see my other response to this?
https://www.redhat.com/archives/libvir-list/2007-May/msg00256.html
That script, modified a bit further (see attachment) does logging fairly well. Having it invoked as a wrapper/replacement for virsh is just a matter of packaging.
yes, but it assumes all logging should go to stderr, which is viable sometimes but not always, the logfile opens the possibility to make really verbose debugging without cluttering what the user see normally (i.e. if stderr is not redirected), and if I understand well the goal is to actually increase debugging in the future. With nice GUI tools being developped on top of libvirt, I expect virsh role to gradually move more and more to debugging use cases (without neglecting the CLI aspect though). Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Hi, Daniel Thank you for your comment.
My experience with systematic logging at command line level is that you end up polluting your file system with (hidden) log files, and while it may be fine for a developper it's not something you want to inflict to your customers. Also the fixed name for the log file makes it easilly useless, a random run in the same directory will just wipe out the data you tried to collect in a separate process. I think having permanent systematic logging to a fixed file is not proper, I would rather not use that. But since the patch is relatively simple based on existing virsh logging code, I think this could go as a command line option for virsh, for example --log filename where the detailed logs can then be saved if needed when a problem occurs. I think this would avoid the main drawbacks of your proposed patch.
I agree about a command line option. So, I remaked the patch which --log option is added for virsh. How about this one? Signed-off-by: Nobuhiro Itou <fj0873gn@aa.jp.fujitsu.com> Thanks, Nobuhiro Itou.
On Fri, Jun 01, 2007 at 07:35:00PM +0900, Nobuhiro Itou wrote:
Hi, Daniel
Hi Nobuhiro,
But since the patch is relatively simple based on existing virsh logging code, I think this could go as a command line option for virsh, for example --log filename where the detailed logs can then be saved if needed when a problem occurs. I think this would avoid the main drawbacks of your proposed patch.
I agree about a command line option. So, I remaked the patch which --log option is added for virsh. How about this one?
I guess you still think of a single log file shared by multiple virsh run, and honnestly I think this adds way too much complexity and is not really useful. You have one log file per virsh run. It's the responsability of the user to pick a log file name. Trying to reinvent syslog at the level of virsh does not sounds right to me, or rather it makes what should be a small patch something really complex instead, I am not sure it is worth it.
Index: src/virsh.c [...] +#define LOCK_NAME ".log.lck" +#define LOCK_TIMER 100 /* mili seconds */
Honnestly I don't see the need for a lock. The filename is provided by the user, I would not add this extra layer there.
+ * Indicates the level of an log massage
typo, message :-)
+/* + * vshLogDef - log definition + */ +typedef struct { + int log_fd; /* log file descriptor */ + int lock_fd; /* lock file descriptor */ +} vshLogDef;
Drop the lock, then you just have the fd, then you don't need a structure and everything is way simpler. [...]
va_start(ap, format); - vfprintf(stdout, format, ap); + if (level <= ctl->debug) + vfprintf(stdout, format, ap); + vshOutputLogFile(ctl, VSH_ERR_DEBUG, format, ap); va_end(ap); }
I am not 100% sure you can actually reuse ap again without doing the va_end(ap) and va_start(ap, format) again. This may work in some case but break on some implemntations.
@@ -3274,6 +3321,7 @@ vshError(vshControl * ctl, int doexit, c
va_start(ap, format); vfprintf(stderr, format, ap); + vshOutputLogFile(ctl, VSH_ERR_ERROR, format, ap); va_end(ap);
same here [...]
+ if (ctl->logfile[0] != '/') { + vshError(ctl, TRUE, _("the log file name has to be full path")); + }
hum, I don't see why you would prevent relative paths for log files from a virsh run, except it makes simpler to compute the path to the lock file, but agains let's just get rid of that lock.
+ if (ctl->logfile[strlen(ctl->logfile) - 1] == '/') { + vshError(ctl, TRUE, _("the log path is not a file name")); + }
I would not do that kind of test. Open the file for writing, if it fails emit an error and return.
+ do { + /* check log directory */ + snprintf(file_buf, sizeof(file_buf), "%s", ctl->logfile); + snprintf(path_buf, sizeof(path_buf), "%s", dirname(file_buf)); + while ((ret = stat(path_buf, &st)) == -1) { + switch (errno) { + case ENOENT: + if (mkdir(path_buf, DIR_MODE) == -1) { + switch (errno) { + case ENOENT: + snprintf(file_buf, sizeof(file_buf), "%s", path_buf); + snprintf(path_buf, sizeof(path_buf), "%s", dirname(file_buf)); + continue; + default: + vshError(ctl, TRUE, _("failed to create the log directory")); + break; + } + } + break; + default: + vshError(ctl, TRUE, _("failed to get the log directory information")); + break; + } + break; + } + if (ret == 0 && !S_ISDIR(st.st_mode)) { + vshError(ctl, TRUE, _("no such the log directory")); + } + } while (ret == -1);
This is way too complex. I certainly do not want to create directories just because an error was made typing the path to the log file. This doesn't make much sense to me. Try to open the file for writing, if it fails, error out, that's all.
+ /* check log file */ + if (stat(ctl->logfile, &st) == -1) { + switch (errno) { + case ENOENT: + break; + default: + vshError(ctl, TRUE, _("failed to get the log file information")); + break; + } + } else { + if (!S_ISREG(st.st_mode)) { + vshError(ctl, TRUE, _("the log path is not a file")); + } + }
That could be kept but IMHO this is still more than needed.
+ /* lock file open */ + snprintf(file_buf, sizeof(file_buf), "%s/%s", path_buf, LOCK_NAME); + if ((logdef.lock_fd = open(file_buf, O_WRONLY | O_CREAT, LOCK_MODE)) < 0) { + vshError(ctl, TRUE, _("failed to open the log lock file")); + } + /* log file open */ + if ((logdef.log_fd = open(ctl->logfile, O_WRONLY | O_APPEND | O_CREAT | O_SYNC, FILE_MODE)) < 0) { + vshError(ctl, TRUE, _("failed to open the log file")); + } +}
Please drop the lock. Locking would have been needed if virsh required using the same file names. Now the user provides the file name, it's his responsability to get it right. [...]
+ + if (logdef.lock_fd == -1 || logdef.log_fd == -1) + return; + + /* lock */
drop locking. Anyway intermixed output from multiple run is unlikely to be very helpful, actually more a source of confusion than help in my optinion. [...]
+ /** + * create log format + * + * [YYYY.MM.DD HH:MM:SS SIGNATURE PID] LOG_LEVEL message + */ + gettimeofday(&stTimeval, NULL); + stTm = localtime(&stTimeval.tv_sec); + snprintf(msg_buf, sizeof(msg_buf), + "[%d.%02d.%02d %02d:%02d:%02d ", + (1900 + stTm->tm_year), + (1 + stTm->tm_mon), + (stTm->tm_mday), + (stTm->tm_hour), + (stTm->tm_min), + (stTm->tm_sec)); + snprintf(msg_buf + strlen(msg_buf), sizeof(msg_buf) - strlen(msg_buf), + "%s %d] ", SIGN_NAME, getpid());
saving the time is okay. I don't think sharing log files for multiple run helps in any way, so no need to put the pid in... [...]
+ /* Check log size */ + if (stat(ctl->logfile, &st) == 0) { + if (st.st_size >= MAX_LOGSIZE) { + /* rotate logs */ + for (i = ROTATE_NUM - 1; i >= 1; i--) { + snprintf(bak_old_path, sizeof(bak_old_path), "%s.%d", ctl->logfile, i); + snprintf(bak_new_path, sizeof(bak_new_path), "%s.%d", ctl->logfile, i+1); + if (rename(bak_old_path, bak_new_path) == -1) { + switch (errno) { + case ENOENT: + break; + default: + /* unlock file */ + flk.l_type = F_UNLCK; + fcntl(logdef.lock_fd, F_SETLK, &flk); + vshCloseLogFile(); + vshError(ctl, FALSE, _("failed to rename the log file")); + return; + } + } + } + snprintf(bak_new_path, sizeof(bak_new_path), "%s.%d", ctl->logfile, 1); + if (rename(ctl->logfile, bak_new_path) == -1) { + /* unlock file */ + flk.l_type = F_UNLCK; + fcntl(logdef.lock_fd, F_SETLK, &flk); + vshCloseLogFile(); + vshError(ctl, FALSE, _("failed to rename the log file")); + return; + } + } + }
I would not bother with the log size either. Just dump to the file. If the output is big well the user will probably need all of it to sort out the problem, but basically it shouldn't grow very large if it is not shared between virsh runs. I expect the use to be the following: - users uses virsh for virtualization operation - something suddenly does not work - then he re-runs the command with logging - then he can analyze the log or transmit it to a sysadmin who can have a look but I don't believe in reimplementing something like syslog within virsh to log all operations all the time, especially with a fixed size buffer. logs will be intermixed, hard to process, add a burden on the server, and makes the code way more complex than it needs to be. Maybe I didn't understood how you expected logging to work, but apparently we had different viewpoints, I would rather go for the simplest, does that still work for your use case ? Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Hi, Daniel Thank you for your comment and code review.
But since the patch is relatively simple based on existing virsh logging code, I think this could go as a command line option for virsh, for example --log filename where the detailed logs can then be saved if needed when a problem occurs. I think this would avoid the main drawbacks of your proposed patch.
I agree about a command line option. So, I remaked the patch which --log option is added for virsh. How about this one?
I guess you still think of a single log file shared by multiple virsh run, and honnestly I think this adds way too much complexity and is not really useful. You have one log file per virsh run. It's the responsability of the user to pick a log file name. Trying to reinvent syslog at the level of virsh does not sounds right to me, or rather it makes what should be a small patch something really complex instead, I am not sure it is worth it.
Okey, I corrected all review point. [...]
I expect the use to be the following: - users uses virsh for virtualization operation - something suddenly does not work - then he re-runs the command with logging - then he can analyze the log or transmit it to a sysadmin who can have a look but I don't believe in reimplementing something like syslog within virsh to log all operations all the time, especially with a fixed size buffer. logs will be intermixed, hard to process, add a burden on the server, and makes the code way more complex than it needs to be.
Maybe I didn't understood how you expected logging to work, but apparently we had different viewpoints, I would rather go for the simplest,
I agree.
does that still work for your use case ?
Yes. How about this attached patch? Signed-off-by: Nobuhiro Itou <fj0873gn@aa.jp.fujitsu.com> Thanks, Nobuhiro Itou.
On Wed, Jun 06, 2007 at 05:39:40PM +0900, Nobuhiro Itou wrote:
I expect the use to be the following: - users uses virsh for virtualization operation - something suddenly does not work - then he re-runs the command with logging - then he can analyze the log or transmit it to a sysadmin who can have a look but I don't believe in reimplementing something like syslog within virsh to log all operations all the time, especially with a fixed size buffer. logs will be intermixed, hard to process, add a burden on the server, and makes the code way more complex than it needs to be.
Maybe I didn't understood how you expected logging to work, but apparently we had different viewpoints, I would rather go for the simplest,
I agree.
excellent !
does that still work for your use case ?
Yes. How about this attached patch?
Way simpler, that works, I just fixed a couple of things: - removed defines for buffer size which were not needed anymore - if no log file name was given do not try to open it I commited this in CVS but I'm afraid there is something missing, look at the following: paphio:~/libvirt/src -> ./virsh --log logfile list libvir: error : no support for hypervisor (null) virsh: error: failed to connect to the hypervisor paphio:~/libvirt/src -> cat logfile [2007.06.06 14:26:30 virsh] ERROR failed to connect to the hypervisor paphio:~/libvirt/src -> the machine is not running Xen so the error is normal, the trouble is that the logfile only contains the virsh error not the libvirt one (which is the one the most useful), this need debugging I guess. Another question is that the log file is opened in append mode, while I guess this kind of option usually rewrite the file from scratch, but this is an open question, I can see how both behaviour could be useful or annoying depending on the circumstances. thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Hi, Daniel Thank you for committing virsh log patch.
Way simpler, that works, I just fixed a couple of things: - removed defines for buffer size which were not needed anymore
Oops, sorry. I had forgotten to erase it.
- if no log file name was given do not try to open it
Okey. I agree.
I commited this in CVS but I'm afraid there is something missing, look at the following:
paphio:~/libvirt/src -> ./virsh --log logfile list libvir: error : no support for hypervisor (null) virsh: error: failed to connect to the hypervisor paphio:~/libvirt/src -> cat logfile [2007.06.06 14:26:30 virsh] ERROR failed to connect to the hypervisor paphio:~/libvirt/src ->
the machine is not running Xen so the error is normal, the trouble is that the logfile only contains the virsh error not the libvirt one (which is the one the most useful), this need debugging I guess.
Yes. I also had recognized this problem. I think so the libvirt error is the most useful, too. I try to think about measures.
Another question is that the log file is opened in append mode, while I guess this kind of option usually rewrite the file from scratch, but this is an open question, I can see how both behaviour could be useful or annoying depending on the circumstances.
Hum, I understood that you said. But, I think that virsh must not overwrite the logfile without user's permission, because virsh doesn't know what the file user specified contains. If it is append mode, the old contents of a file do not disappear. User can delete it later if needless, and can keep it if need. Thanks, Nobuhiro Itou.
participants (6)
-
Atsushi SAKAI -
Daniel P. Berrange -
Daniel Veillard -
Mark McLoughlin -
Nobuhiro Itou -
Richard W.M. Jones