Re: [PATCH v7 8/8] systemd: add xen systemd service and module files

["Luis R. Rodriguez" <mcgrof@suse.com>]

On Thu, Jul 24, 2014 at 04:47:49PM +0100, Ian Campbell wrote:
> On Thu, 2014-07-17 at 16:28 -0700, Luis R. Rodriguez wrote:
> > diff --git a/Makefile b/Makefile
> > index 41dabbf..104e39d 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -216,8 +216,12 @@ uninstall:
> >  	rm -f  $(D)$(CONFIG_DIR)/udev/rules.d/xen-backend.rules
> >  	rm -f  $(D)$(CONFIG_DIR)/udev/rules.d/xend.rules
> >  	rm -f  $(D)$(SYSCONFIG_DIR)/xendomains
> > +	rm -f  $(D)$(SBINDIR)/xendomains
> >  	rm -f  $(D)$(SYSCONFIG_DIR)/xencommons
> > -	rm -rf $(D)/var/run/xen* $(D)/var/lib/xen*
> > +	rm -f  $(D)$(XEN_SYSTEMD_DIR)/*.service
> > +	rm -f  $(D)$(XEN_SYSTEMD_DIR)/*.mount
> > +	rm -f  $(D)$(XEN_SYSTEMD_MODULES_LOAD)/*.conf
> > +	rm -rf $(D)${XEN_RUN_DIR}* $(D)/var/lib/xen*
> 
> ${} rather than $()? I'm a bit concerned that might expand to nothing
> and nuke $(D)...

Adjusted.

> > +Systemd and legacy init support
> 
> s/legacy/SysVinit/ please, no need to be rude about our old friend.

Asdf.

> TBH, you don't mention sysvinit in this para, so the section could
> equally be called "Systemd support".

OK.

> > +===============================
> > +
> > +If you have systemd development packages installed you can build binaries
> > +with systemd support. Systemd support is enabled by default if you have
> > +systemd development libraries present. If you want to force enable systemd to
> > +ensure you build binaries with systemd support you can use the --enable-systemd
> > +flag. Likewise if you want to force disable systemd you can use either of
> > +these two options:
> > +
> > +	./configure --disable-systemd
> > +	./configure --enable-systemd=no
> 
> It's nice that both work but users only need to know about one of them,
> else we are just bamboozling them with needlessly multiple ways to do
> things.

Removed the second one.

> > diff --git a/tools/hotplug/Linux/systemd/Makefile b/tools/hotplug/Linux/systemd/Makefile
> > new file mode 100644
> > index 0000000..dc98b67
> > --- /dev/null
> > +++ b/tools/hotplug/Linux/systemd/Makefile
> 
> I've not reviewed these files since I don't really know much about
> systemd specifics. I assume that people who do know them have looked at
> some point and/or they are derived from existing ones which work and/or
> they will get fixed eventually if they happen to be not quite right.

These were derived from Fedora's systemd files, I've modified them
quite a bit and annotated all changes made on the commit log. All folks
who should review this are on the Cc and have been since v1. Some of
their own pointers have helped to evolve these as well.

  Luis