Re: [Xen-devel] [PATCH] tools/xenconsoled: Add syslog

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-05-10 at 10:58 +0100, Jean Guyader wrote:
> @@ -158,7 +160,7 @@ static void buffer_append(struct domain *dom)
>         if ((size == 0) || (size > sizeof(intf->out)))
>                 return;
>  
> -       if ((buffer->capacity - buffer->size) < size) {
> +       if ((buffer->capacity - buffer->size) < size + 1) {

What's this?

>                 buffer->capacity += (size + 1024);
>                 buffer->data = realloc(buffer->data,
> buffer->capacity);
>                 if (buffer->data == NULL) {
> @@ -166,10 +168,31 @@ static void buffer_append(struct domain *dom)
>                         exit(ENOMEM);
>                 }
>         }
> +       slbuf.capacity = buffer->capacity;
> +       slbuf.max_capacity = buffer->max_capacity;
> +       slbuf.data = malloc(slbuf.capacity);
> +       if (!slbuf.data) {
> +               dolog(LOG_ERR, "Memory allocation failed");
> +               exit(ENOMEM);
> +       }
>  
> -       while (cons != prod)
> -               buffer->data[buffer->size++] = intf->out[
> -                       MASK_XENCONS_IDX(cons++, intf->out)];
> +       for (; cons != prod; ++cons) {
> +               char ch = intf->out[MASK_XENCONS_IDX(cons,
> intf->out)];
> +               slbuf.data[slbuf.size++] = ch;
> +               buffer->data[buffer->size++] = ch;
> +               if  (ch == '\r' || ch == '\n') {
> +                       slbuf.data[slbuf.size - 1] = '\0';
> +                       ++cons;
> +                       if (cons != prod) {
> +                               ch = intf->out[MASK_XENCONS_IDX(cons
> ++, intf->out)];
> +                               buffer->data[buffer->size++] = ch;
> +                               if (ch != '\r' && ch != '\n') {
> +                                       slbuf.data[slbuf.size++] = ch;
> +                               }
> +                       }

This is chomping \r\n or \n\r into a \0 for the syslog buffer only?

It's a bit fiddly, worth a comment I would say..

> +                       break;
> +               }
> +       }
>  
>         xen_mb();
>         intf->out_cons = cons;
> @@ -196,6 +219,9 @@ static void buffer_append(struct domain *dom)
>                         dolog(LOG_ERR, "Write to log failed "
>                               "on domain %d: %d (%s)\n",
>                               dom->domid, errno, strerror(errno));
> +       } else {
> +               /* Falls back to syslog is the domain doesn't have a
> log file */

Isn't that the same as logging to syslog by default? Shouldn't there be
a check for if (use_syslog) in here somewhere?

> +               syslog(LOG_INFO, "%s (%i): %s", dom->name, dom->domid,
> slbuf.data);
>         }
>  
>         if (discard_overflowed_data && buffer->max_capacity &&
> @@ -219,6 +245,7 @@ static void buffer_append(struct domain *dom)
>                         buffer->size = buffer->max_capacity / 2 +
> over;
>                 }
>         }
> +       free(slbuf.data);
>  }
>  
>  static bool buffer_empty(struct buffer *buffer)
> @@ -255,6 +282,10 @@ static int create_hv_log(void)
>  {
>         char logfile[PATH_MAX];
>         int fd;
> +
> +       if (!log_dir)
> +               return -1;
> +
>         snprintf(logfile, PATH_MAX-1, "%s/hypervisor.log", log_dir);
>         logfile[PATH_MAX-1] = '\0';
>  
> @@ -275,32 +306,56 @@ static int create_hv_log(void)
>         return fd;
>  }
>  
> +static char *safe_xs_read(const char *key, int tries)
> +{
> +       char *data = NULL;
> +       unsigned int len;
> +       struct timespec req = { .tv_sec = 0, .tv_nsec =
> 100000000 }; /* 100 ms */
> +       int i;
> +
> +       for (i = 0; i < tries; i++) {
> +               data = xs_read(xs, XBT_NULL, key, &len);
> +               if (data && len > 0)
> +                       break;
> +               free(data);
> +               nanosleep(&req, NULL);
> +       }
> +       return data;
> +}
> +
> +static char *name_from_dompath(const char *path)
> +{
> +       char namepath[64] = { 0 }, *name;

I suppose 64 is enough, but you could use strlen + N here...

> +       strncat( namepath, path , sizeof(namepath) );
> +       strncat( namepath, "/name", sizeof(namepath) -
> strlen(namepath) );
> +
> +       name = safe_xs_read(namepath, 1);
> +       /* without any name after 100 tries, just default to unnamed
> */

Does this ever happen in practice? What sort of failure mode does it
indicate?

You do this for both the logfile and the sysfs name, so I guess this is
strictly speaking an unrelated fix?

> +       if (!name)
> +               name = strdup("unnamed");
> +       return name;
> +}
> +
>  static int create_domain_log(struct domain *dom)
>  {
>         char logfile[PATH_MAX];
> -       char *namepath, *data, *s;
> +       char *namepath;
>         int fd;
> -       unsigned int len;
>  
>         namepath = xs_get_domain_path(xs, dom->domid);
> -       s = realloc(namepath, strlen(namepath) + 6);
> -       if (s == NULL) {
> +       if (namepath == NULL) {
>                 free(namepath);
>                 return -1;
>         }
> -       namepath = s;
> -       strcat(namepath, "/name");
> -       data = xs_read(xs, XBT_NULL, namepath, &len);
> +       dom->name = name_from_dompath(namepath);
>         free(namepath);
> -       if (!data)
> +       if (!dom->name) {
>                 return -1;
> -       if (!len) {
> -               free(data);
> +       }
> +       if (!log_dir) {
>                 return -1;
>         }
> -
> -       snprintf(logfile, PATH_MAX-1, "%s/guest-%s.log", log_dir,
> data);
> -       free(data);
> +       snprintf(logfile, PATH_MAX - 1, "%s/%s.log", log_dir,
> dom->name);

Have you just changed the name of the logfile or does the "guest-"
prefix come from somewhere else?

I think the guest namespace is important to avoid clobbering the
hypervisor.log (ok, so naming a guest "hypervisor" is dumb, but still).

More plausibly someone could name a domain "smtp" which would be
confusing when seen in syslog next to the dom0 MTA logging, perhaps.

>         logfile[PATH_MAX-1] = '\0';
>  
>         fd = open(logfile, O_WRONLY|O_CREAT|O_APPEND, 0644);
> @@ -878,7 +939,10 @@ static void handle_xs(void)
>  
>  static void handle_hv_logs(void)
>  {
> -       char buffer[1024*16];
> +       char buffer[1024*16 + 1];
> +       static char lbuf[1024*16 + 1];
> +       static int loff = 0;
> +       int i = 0;
>         char *bufptr = buffer;
>         unsigned int size = sizeof(buffer);
>         static uint32_t index = 0;
> @@ -888,17 +952,36 @@ static void handle_hv_logs(void)
>                 return;
>  
>         if (xc_readconsolering(xch, bufptr, &size, 0, 1, &index) == 0
> && size > 0) {
> -               int logret;
> -               if (log_time_hv)
> -                       logret = write_with_timestamp(log_hv_fd,
> buffer, size,
> -
> &log_time_hv_needts);
> -               else
> -                       logret = write_all(log_hv_fd, buffer, size);
> -
> -               if (logret < 0)
> -                       dolog(LOG_ERR, "Failed to write hypervisor
> log: "
> -                                      "%d (%s)", errno,
> strerror(errno));
> -       }
> +               if (log_hv_fd != -1) {
> +                       int logret;
> +                       if (log_time_hv)
> +                               logret =
> write_with_timestamp(log_hv_fd, buffer, size,
> +
> &log_time_hv_needts);
> +                       else
> +                               logret = write_all(log_hv_fd, buffer,
> size);
> +
> +                       if (logret < 0)
> +                               dolog(LOG_ERR, "Failed to write
> hypervisor log: "
> +                                              "%d (%s)", errno,
> strerror(errno));
> +               } else {
> +                       while (i < size) {
> +                               while ((i < size) &&
> +                                      (buffer[i] != '\n') &&
> (buffer[i] != '\r')) {
> +                                       lbuf[loff++] = buffer[i++];
> +                               }
> +                               if ((buffer[i] == '\n') || (buffer[i]
> == '\r')) {

Is this now the second time we have done this \n\r conversion?

FWIW this seems like the better location for it to me, since it made the
code at the other location hard to follow whereas it looks more natural
to skip char here.

Also, can't you do this inplace in buffer[] and therefore avoid copying
all the bytes plus the overhead of a second buffer?

And could you also avoid the whole separate "struct buffer slbuf"? The
need for that did make me pause earlier...

> +                                       lbuf[loff] = '\0';
> +                                       ++i;
> +                                       if ((i < size) &&
> +                                           ((buffer[i] == '\n') ||
> (buffer[i] == '\r'))) {
> +                                               ++i;
> +                                       }
> +                                       syslog(LOG_INFO, "hypervisor:
> %s", lbuf);
> +                                       loff = 0;
> +                               }
> +                       }
> +               }
> +        }
>  
>         (void)xc_evtchn_unmask(xce_handle, port);
>  }
> @@ -940,8 +1023,6 @@ void handle_io(void)
>                         goto out;
>                 }
>                 log_hv_fd = create_hv_log();
> -               if (log_hv_fd == -1)
> -                       goto out;
>                 log_hv_evtchn = xc_evtchn_bind_virq(xce_handle,
> VIRQ_CON_RING);
>                 if (log_hv_evtchn == -1) {
>                         dolog(LOG_ERR, "Failed to bind to
> VIRQ_CON_RING: "
> diff --git a/tools/console/daemon/main.c b/tools/console/daemon/main.c



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel