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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/