Re: [PATCH v7 6/8] xencommons: move module list into a generic place

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

On Thu, Jul 24, 2014 at 04:35:31PM +0100, Ian Campbell wrote:
> On Thu, 2014-07-17 at 16:28 -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > This will allows us to share the same module list with
> > sysemd, and lets us upkeep it in one place. Document this
> 
> "systemd"

Fixed.

> > while at it on the top level README and expand on the wiki:
> > 
> > http://wiki.xen.org/wiki/Category:Host_Configuration#Linux_kernel_modules
> > 
> > In order to upkeep parallelism builds be explicit about the
> > requirement to complete all actions before any installation
> > targets.
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > ---
> >  README                                             | 12 ++++++++
> >  config/Linux.modules                               | 20 ++++++++++++
> >  tools/hotplug/Linux/Makefile                       | 36 +++++++++++++++++++---
> >  .../Linux/init.d/{xencommons => xencommons.in}     | 16 +---------
> >  4 files changed, 65 insertions(+), 19 deletions(-)
> >  create mode 100644 config/Linux.modules
> >  rename tools/hotplug/Linux/init.d/{xencommons => xencommons.in} (89%)
> > 
> > diff --git a/README b/README
> > index 9bbe734..c6cc09b 100644
> > --- a/README
> > +++ b/README
> > @@ -183,3 +183,15 @@ There are optional targets as part of Xen's top-level makefile that will
> >  download and build tboot: install-tboot, build-tboot, dist-tboot, clean-tboot.
> >  These will download the latest tar file from the SourceForge site using wget,
> >  then build/install/dist according to Xen's settings.
> > +
> > +Required Linux modules
> > +======================
> > +
> > +Xen has a set of Linux modules which the init scripts ensure to load before
> > +before starting Xen guests. The list of modules are maintained in one place:
> > +
> > +  * config/modules
> 
> It's Linux.modules now.

Fixed.

> We try to avoid being to Linux specific, since we do support other OSes
> as dom0 and are generally pretty agnostic.
> 
> Perhaps we could reword this slightly with s/Linux/kernel/ and
> referencing config/$(XEN_OS).modules?

Sure.

> > +
> > +For more details refer to:
> > +
> > +http://wiki.xen.org/wiki/Category:Host_Configuration#Linux_kernel_modules
> 
> The text in this is also out of date wrt the filename now.

Updated.

> Also, like previous iterations of the README, the text in that wiki page
> also suffers from being a bit dev focused in what is supposed to be an
> end user focused page.

Its a wiki so help with editorial stuff is welcomed. I just figured its best
to add more information than less. People tend to complain about documentation.

> > diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
> > index d5de9e6..1805746 100644
> > --- a/tools/hotplug/Linux/Makefile
> > +++ b/tools/hotplug/Linux/Makefile
> > @@ -33,17 +33,45 @@ UDEV_RULES_DIR = $(CONFIG_DIR)/udev
> >  UDEV_RULES = xen-backend.rules $(UDEV_RULES-y)
> >  
> >  .PHONY: all
> > -all:
> > +all: $(XENCOMMONS_INITD)
> > +
> > +$(XENCOMMONS_INITD): $(XEN_ROOT)/config/$(XEN_OS).modules $(XENCOMMONS_INITD).in
> > +	@set -e ;							\
> > +	IFS=''								;\
> > +	cat  $(XEN_ROOT)/config/$(XEN_OS).modules	| (		\
> > +		while read l ; do					\
> > +			if echo $${l} | egrep -q "^#" ; then		\
> > +				continue				;\
> > +			fi						;\
> > +			if echo "$${l}" | egrep -q "\|" ; then		\
> > +				m1=$${l%%|*}				;\
> > +				m2=$${l#*|} 				;\
> > +				echo "        modprobe $$m1 2>/dev/null || modprobe $$m2 2>/dev/null" ;\
> > +			else						\
> > +				echo "        modprobe $$l 2>/dev/null"		;\
> > +			fi						;\
> > +		done							\
> > +	) > $(XENCOMMONS_INITD).modules					;\
> > +	cat  $(XENCOMMONS_INITD).in	| (				\
> > +		while read l ; do					\
> > +			if echo "$${l}" | egrep -q "@LOAD_MODULES@" ; then \
> > +				cat $(XENCOMMONS_INITD).modules 	;\
> > +			else						\
> > +				echo $$l				;\
> > +			fi						;\
> > +		done							\
> > +	) > $@
> > +	@rm -f $(XENCOMMONS_INITD).modules
> 
> Perhaps consider putting this into a mkinitscript shell script and
> calling it instead?

OK.

 Luis