Re: [Xen-devel] [PATCH 2/6] xenconsoled: switch hypervisor log to use logfile abstraction

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("[PATCH 2/6] xenconsoled: switch hypervisor log to use logfile 
abstraction"):
> To minimise code churn, copy and paste a some existing functions and
> adapt them to write to logfile. The functions to deal with fd directly
> will go away eventually.
...
> +static int write_logfile(struct logfile *logfile, const char *buf,
> +                      size_t len)
> +{
> +     ssize_t r = logfile_append(logfile, buf, len);
> +
> +     if (r < 0) return -1;
> +     return 0;
> +}

That this is necessary suggests that logfile_append has an
inconvenient calling convention.

> +static int write_logfile_with_timestamp(struct logfile *logfile,
> +                                     const char *data, size_t sz,
> +                                     int *needts)
> +{

This should be in logfile.c.  Furthermore, this is at the wrong level
with respect to log rotation:

> +             if ((*needts && logfile_append(logfile, ts, tslen))

This could result in a timestamp being split across two different
logfiles which is undesirable.  So I think this needs to be done at
the layer below.

> +             data = nl + 1;
> +             if (found_nl) {
> +                     // If we printed a newline, strip all \r following it
> +                     while (data <= last_byte && *data == '\r')
> +                             data++;
> +             }

I would prefer that we only apply a lossless transformation to
logfiles, even if that means that our lotfiles contain CRs.  Also,
this is rather odd.  Surely it is more usual for the CR to precede the
LF.

I suggest the transformation `after every LF, preface the next byte
with the timestamp of that next byte'.

> -     if (fd != -1 && log_time_hv) {
> -             if (write_with_timestamp(fd, "Logfile Opened\n",
> -                                      strlen("Logfile Opened\n"),
> -                                      &log_time_hv_needts) < 0) {
> +
> +     if (tmp && log_time_hv) {
> +             if (write_logfile_with_timestamp(tmp, "Logfile Opened\n",
> +                                              strlen("Logfile Opened\n"),
> +                                              &log_time_hv_needts) < 0) {

I think that if timestamps are turned off, writing the "Logfile
Opened" (with no preceding timestamp) is probably a good idea.  So
this can be made simpler.

>       if (log_hv) {
> -             if (log_hv_fd != -1)
> -                     close(log_hv_fd);
> -             log_hv_fd = create_hv_log();
> +             if (log_hv_file)
> +                     logfile_free(log_hv_file);
> +             log_hv_file = create_hv_log();

You could separate out the nonfunctional changes if you did it in
stages:

 * provide logfile_valid as a dummy macro that tests against -1

 * replace `log_hv_fd' with `log_hv' throughout; replace
   comparisons with -1 with logfile_valid(log_hv); likewise
   client logfiles - no functional change

 * replace log opening code with calls to logfile_open; change
   types; change implementation of logfile_valid

Ian.

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