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@xxxxxxxxxx> > > --- > > 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list