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

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(a)redhat.com&gt; > --- > 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);

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...

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.