Re: [Xen-devel] [PATCH 1/6] xenconsoled: introduce log file abstraction

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("[PATCH 1/6] xenconsoled: introduce log file abstraction"):
> It will be used to handle hypervsior log and guest console log.

Thanks.

> diff --git a/tools/console/daemon/logfile.h b/tools/console/daemon/logfile.h
> new file mode 100644
> index 0000000..87f3c51
...
> +#define LOGFILE_MAX_BACKUP_NUM  5
> +#define LOGFILE_MAX_SIZE        (1024*128)

These should surely be configurable by the end-user.

> +/* Append only abstraction for log file */
> +struct logfile_entry {
> +     int fd;
> +     off_t pos;
> +     off_t len;
> +};

AFAICT these types could be defined inside logfile.c, and `struct
logfile' declared (but not defined) here in the header file.

That would make the abstraction clearer.

> diff --git a/tools/console/daemon/logfile.c b/tools/console/daemon/logfile.c
> new file mode 100644
> index 0000000..ad73f8e
> --- /dev/null
> +++ b/tools/console/daemon/logfile.c
...
> +static void logfile_entry_free(struct logfile_entry *entry)
> +{
> +     if (!entry) return;
> +
> +     if (entry->fd >= 0) close(entry->fd);
> +     free(entry);
> +}

I do wonder whether it's actually worth having a whole separate struct
for an `entry'.  Each `struct logfile' has zero or one `struct
logfile_entry's but most of the time exactly one.

Eliminating the separate struct logfile_entry and folding it into
struct logfile would save some memory allocation (and associated error
handling).

> +     entry = calloc(1, sizeof(*entry));
> +     if (!entry) goto err;

This pattern calls close(0) on error!  Either declare that fd==0 means
"none here" (which would be sane IMO) or initialise fd=-1.

> +     entry->fd = open(path, O_CREAT|O_APPEND|O_WRONLY|O_CLOEXEC, mode);

I think we had concluded that O_CLOEXEC wasn't sufficiently portable ?
It's not needed here because xenconsoled does not exec.

> +     entry->pos = lseek(entry->fd, 0, SEEK_END);
> +     if (entry->pos == (off_t)-1) {
> +             dolog(LOG_ERR, "Unable to determine current file offset in %s",
> +                   path);
> +             goto err;
> +     }
> +
> +     if (fstat(entry->fd, &sb) < 0) {
> +             dolog(LOG_ERR, "Unable to fstat current file %s", path);
> +             goto err;
> +     }

I'm not sure why you need to keep track of `len' and `pos'
separately.  AFAICT len is never used.

> +struct logfile *logfile_new(const char *path, mode_t mode)
> +{

Why does logfile_new take a mode for which all callers pass 644 ?
Do we anticipate logs with different permissions ?

> +             if (rename(this, next) < 0 && errno != ENOENT) {
> +                     dolog(LOG_ERR, "Failed to rename %s to %s",
> +                           this, next);
> +                     goto err;
> +             }

If LOGFILE_MAX_BACKUP_NUM becomes configurable, this loop will have to
become more sophisticated.  I think it may have to count up and then
down again.

> +static ssize_t write_all(int fd, const char* buf, size_t len)
> +{
...

This is a copy of an identical function in io.c.

> +ssize_t logfile_append(struct logfile *file, const char *buf,
> +                    size_t len)
> +{
> +     ssize_t ret = 0;
> +
> +     while (len) {
> +             struct logfile_entry *entry = file->entry;
> +             size_t towrite = len;
> +             bool rollover = false;
> +
> +             if (entry->pos > file->maxlen) {
> +                     rollover = true;
> +                     towrite = 0;
> +             } else if (entry->pos + towrite > file->maxlen) {
> +                     towrite = file->maxlen - entry->pos;
> +             }
...
[and later]
> +             if ((entry->pos == file->maxlen && len) || rollover) {

This logic is rather tangled.  Why not

  maxwrite = file->maxlen - file->pos;
  if (towrite > maxwrite) {
      rollover = 1;
      towrite = maxwrite;
      if (towrite < 0)
          towrite = 0;
  }

and then later simply

    if (rollover) {

?

> +
> +             if (towrite) {
> +                     if (write_all(entry->fd, buf, towrite) != towrite) {
> +                             dolog(LOG_ERR, "Unable to write file %s",
> +                                   file->basepath);
> +                             return -1;
> +                     }
> +
> +                     len -= towrite;
> +                     buf += towrite;
> +                     ret += towrite;

I'm not a huge fan of this approach where several control variables
need to be kept in step.  You could save the beginning and end of the
buffer, and then you could do away with ret and no longer need to
modify len.

Ian.

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