[libvirt] [PATCH v2 01/13] util: add APIs for reading/writing from/to rotating files

[Daniel P. Berrange]

On Wed, Nov 18, 2015 at 08:46:43AM -0500, John Ferlan wrote:
> 
> 
> On 11/12/2015 12:18 PM, Daniel P. Berrange wrote:
> > Add virRotatingFileReader and virRotatingFileWriter objects
> > which allow reading & writing from/to files 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    |  13 +
> >  src/util/virrotatingfile.c  | 608 ++++++++++++++++++++++++++++++++++++++
> >  src/util/virrotatingfile.h  |  62 ++++
> >  tests/Makefile.am           |   6 +
> >  tests/virrotatingfiletest.c | 698 ++++++++++++++++++++++++++++++++++++++++++++
> >  7 files changed, 1389 insertions(+)
> >  create mode 100644 src/util/virrotatingfile.c
> >  create mode 100644 src/util/virrotatingfile.h
> >  create mode 100644 tests/virrotatingfiletest.c
> > 
> 
> [...]
> 
> > --- /dev/null
> > +++ b/src/util/virrotatingfile.c
> > @@ -0,0 +1,608 @@
> 
> In the grand scheme of things a nit - lately there seems to be an effort
> to use two lines for functions, such as:
> 
> [static] int
> virFunctionName(param1Type param1,
>                 param2Type param2)
> 
> This module has some that are and some that aren't.  I don't have heart
> break one way or another, but someone may ;-)
> 
> > +/*
> > + * virrotatingfile.c: file I/O with size rotation
> > + *
> > + * Copyright (C) 2015 Red Hat, Inc.
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library.  If not, see
> > + * <http://www.gnu.org/licenses/>.
> > + *
> > + */
> > +
> 
> [...]
> 
> > +
> > +
> > +static virRotatingFileReaderEntryPtr
> > +virRotatingFileReaderEntryNew(const char *path)
> > +{
> > +    virRotatingFileReaderEntryPtr entry;
> > +    struct stat sb;
> > +
> > +    VIR_DEBUG("Opening %s", path);
> > +
> > +    if (VIR_ALLOC(entry) < 0)
> > +        return NULL;
> > +
> > +    if (VIR_STRDUP(entry->path, path) < 0)
> > +        goto error;
> 
> Should the open occur first; otherwise, entry->fd == 0 and the *Free
> routine will VIR_FORCE_CLOSE(0);

Yep, correct.


> 
> Although I don't see it being documented/used later in/for some config
> file (and the max value I see used in code is 2)... Should there be some
> sort of "sanity check" that maxbackup doesn't exceed some number - only
> so many files can be open per process, right?  Then of course there's
> the silly test of someone passing -1...

Well it is defined a size_t so you "cant" pass -1, but yeah we should
refuse greater than say 20 backup files.

> Also, as a config value it seems if someone changes the maxbackup value
> (higher to lower), then some algorithms may miss files... If then going
> from lower to higher, then files that may not have been deleted might be
> found.

IMHO if changing from higher to lower it is the admins responsibility
to kill off obsolete files.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|