Re: [libvirt] [PATCH] Change logrotate to be per-hypervisor logs

According to Daniel Veillard on 3/4/2010 7:36 AM: > Original bug is: > > https://bugzilla.redhat.com/show_bug.cgi?id=547514 > > maybe this could be done in slightly different way, possibly more > generic, but I think doing a simple split is good enough for now. Seems easy enough. > +++ b/daemon/Makefile.am > @@ -25,7 +25,9 @@ EXTRA_DIST = \ > libvirtd.sasl \ > libvirtd.sysconf \ > libvirtd.aug \ > - libvirtd.logrotate.in \ > + libvirtd.qemu.logrotate.in \ > + libvirtd.lxc.logrotate.in \ > + libvirtd.uml.logrotate.in \ > test_libvirtd.aug \ > $(AVAHI_SOURCES) \ > $(DAEMON_SOURCES) > @@ -177,20 +179,37 @@ remote_dispatch_args.h: $(srcdir)/remote_generate_stubs.pl $(REMOTE_PROTOCOL) > remote_dispatch_ret.h: $(srcdir)/remote_generate_stubs.pl $(REMOTE_PROTOCOL) > perl -w $(srcdir)/remote_generate_stubs.pl -r $(REMOTE_PROTOCOL) > $@ > > -BUILT_SOURCES += libvirtd.logrotate > +LOGROTATE_CONFS = libvirtd.qemu.logrotate libvirtd.lxc.logrotate \ > + libvirtd.uml.logrotate > > -libvirtd.logrotate: libvirtd.logrotate.in > +BUILT_SOURCES += $(LOGROTATE_CONFS) > + > +libvirtd.qemu.logrotate: libvirtd.qemu.logrotate.in > +libvirtd.uml.logrotate: libvirtd.uml.logrotate.in > sed \ > -e s!\@localstatedir\@!@localstatedir@!g \ > < $< > $@-t > mv $@-t $@ Not introduced by your patch, but copied and pasted two more times. Unfortunately, POSIX does not define \@ in sed (GNU sed treats it like @, but some others treat it like \\@). Can you guarantee that @localstatedir@ will not expand to any shell metacharacters? Also, automake guarantees that $(localstatedir) will have the same value as @localstatedir@, but without triggering a 'make syntax-check' warning (although we must already be ignoring that particular check). It would probably be better to write: -e 's![@]localstatedir[@]!$(localstatedir)!g' \ as the substitution. Other than that, you have my ACK