[libvirt] [PATCH 1/5] util: add API for writing to rotating files

[Peter Krempa]

On Tue, Nov 03, 2015 at 16:04:20 +0000, Daniel Berrange wrote:
> Add a virRotatingFile object which allows writing to a file
> with automation rotation to N backup files when a size limit
> is reached. This is useful for guest logging when a guaranteed
> finite size limit is required. Use of external tools like
> logrotate is inadequate since it leaves the possibility for
> guest to DOS the host in between invokations of logrotate.
> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  po/POTFILES.in              |   1 +
>  src/Makefile.am             |   1 +
>  src/libvirt_private.syms    |   6 +
>  src/util/virrotatingfile.c  | 237 +++++++++++++++++++++++++
>  src/util/virrotatingfile.h  |  44 +++++
>  tests/Makefile.am           |   6 +
>  tests/virrotatingfiletest.c | 415 ++++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 710 insertions(+)
>  create mode 100644 src/util/virrotatingfile.c
>  create mode 100644 src/util/virrotatingfile.h
>  create mode 100644 tests/virrotatingfiletest.c
> 

[...]

> +
> +
> +static int virRotatingFileDelete(virRotatingFilePtr file)
> +{
> +    size_t i;
> +
> +    if (unlink(file->path) < 0 &&
> +        errno != ENOENT) {
> +        virReportSystemError(errno,
> +                             _("Unable to delete file %s"),
> +                             file->path);
> +        return -1;
> +    }
> +
> +    for (i = 0; i < file->maxbackup; i++) {
> +        char *oldpath;
> +        if (virAsprintf(&oldpath, "%s.%zu", file->path, i) < 0)
> +            return -1;
> +
> +        if (unlink(oldpath) < 0 &&
> +            errno != ENOENT) {
> +            virReportSystemError(errno,
> +                                 _("Unable to delete file %s"),
> +                                 oldpath);
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
> +virRotatingFilePtr virRotatingFileNew(const char *path,
> +                                      off_t maxlen,
> +                                      size_t maxbackup,
> +                                      bool truncate,
> +                                      mode_t mode)

At least this function would deserve some documentation explaining at
least how maxbackup works.

> +{
> +    virRotatingFilePtr file;
> +
> +

One too many lines.

> +    if (VIR_ALLOC(file) < 0)
> +        goto error;
> +
> +    if (VIR_STRDUP(file->path, path) < 0)
> +        goto error;
> +
> +    file->mode = mode;
> +    file->maxbackup = maxbackup;

If maxbackup is set to 0 the rollover code below will become a no-op,
wich is fine, but in virRotatingFileWrite you close the fd, open the
same file in apend mode seek to the end and find out that is already
full.

> +    file->maxlen = maxlen;
> +
> +    if (truncate &&
> +        virRotatingFileDelete(file) < 0)
> +        goto error;
> +
> +    if (virRotatingFileOpen(file) < 0)
> +        goto error;
> +
> +    return file;
> +
> + error:
> +    virRotatingFileFree(file);
> +    return NULL;
> +}

[...]

> +
> +
> +static int virRotatingFileRollover(virRotatingFilePtr file)
> +{
> +    size_t i;
> +    char *nextpath;
> +    char *thispath;
> +    int ret = -1;
> +
> +    VIR_DEBUG("Rollover %s", file->path);
> +    if (virAsprintf(&nextpath, "%s.%zu", file->path, file->maxbackup - 1) < 0)
> +        return -1;
> +
> +    for (i = file->maxbackup; i > 0; i--) {
> +        if (i == 1) {
> +            if (VIR_STRDUP(thispath, file->path) < 0)
> +                goto cleanup;
> +        } else {
> +            if (virAsprintf(&thispath, "%s.%zu", file->path, i - 2) < 0)
> +                goto cleanup;
> +        }
> +        VIR_DEBUG("Rollover %s -> %s", thispath, nextpath);
> +
> +        if (rename(thispath, nextpath) < 0 &&
> +            errno != ENOENT) {
> +            virReportSystemError(errno,
> +                                 _("Unable to rename %s to %s"),
> +                                 thispath, nextpath);
> +            goto cleanup;
> +        }
> +
> +        VIR_FREE(nextpath);
> +        nextpath = thispath;
> +        thispath = NULL;
> +    }
> +
> +    VIR_DEBUG("Rollover done %s", file->path);
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(nextpath);
> +    VIR_FREE(thispath);
> +    return ret;
> +}

Inconsistent spacing between funcs.

> +
> +ssize_t virRotatingFileWrite(virRotatingFilePtr file,
> +                             const char *buf,
> +                             size_t len)
> +{
> +    ssize_t ret = 0;
> +    while (len) {
> +        size_t towrite = len;
> +
> +        if ((file->curlen + towrite) > file->maxlen)
> +            towrite = file->maxlen - file->curlen;

So the boundary on which the file will be broken is strictly decided on
the number of bytes. Generaly this is fine but for logging it might
break messages in half which isn't entirely nice.

I'd suggest that either this function (or via a flag or a different
function) will have logic that will peek into the last eg. 100
characters (perhaps from the end) of the logged string to see whether
there's a newline and if there is, it will rollover the log files
earlier.

This will slightly decrease the logging capacity but in turn we'll get
nicely broken error messages.

> +
> +        if (towrite) {
> +            if (safewrite(file->fd, buf, towrite) != towrite) {
> +                virReportSystemError(errno,
> +                                     _("Unable to write to file %s"),
> +                                     file->path);
> +                return -1;
> +            }
> +
> +            len -= towrite;
> +            buf += towrite;
> +            ret += towrite;
> +            file->curlen += towrite;
> +        }
> +
> +        if (file->curlen == file->maxlen && len) {
> +            VIR_DEBUG("Hit max size %zu on %s\n", file->maxlen, file->path);
> +            VIR_FORCE_CLOSE(file->fd);
> +
> +            if (virRotatingFileRollover(file) < 0)
> +                return -1;
> +
> +            if (virRotatingFileOpen(file) < 0)
> +                return -1;
> +        }
> +    }
> +
> +    return ret;
> +}

The rest looks okay.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151104/dd6be889/attachment-0001.sig>