Re: [PATCH v6 11/13] xencommons: move module list into a generic place

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

On Wed, Jul 02, 2014 at 02:44:03PM +0100, Ian Campbell wrote:
> On Thu, 2014-06-12 at 18:18 -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
> > 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                                             | 18 +++++++++++
> >  config/modules                                     | 16 ++++++++++
> 
> This list is Linux specific. If tools/hotplug/Linux/config or something
> doesn't work for the upcoming system patches then perhaps
> config/Linux.modules, where Linux is chosen to match $(XEN_OS) aka
> "uname -s".

Moved to config/Linux.modules, and made the Makefile use $(XEN_OS).

> >  tools/hotplug/Linux/Makefile                       | 36 +++++++++++++++++++---
> >  .../Linux/init.d/{xencommons => xencommons.in}     | 16 +---------
> >  4 files changed, 67 insertions(+), 19 deletions(-)
> >  create mode 100644 config/modules
> >  rename tools/hotplug/Linux/init.d/{xencommons => xencommons.in} (89%)
> > 
> > diff --git a/README b/README
> > index 9bbe734..34d7c0b 100644
> > --- a/README
> > +++ b/README
> > @@ -129,6 +129,24 @@ performed with root privileges.]
> >     versions of those scripts, so that you can copy the dist directory
> >     to another machine and install from that distribution.
> >  
> > +Required Linux modules
> > +======================
> > +
> > +Xen has a set of Linux modules which the init scripts ensure to load before
> > +enablement of xen guests. The list of modules are upkept in one place:
> 
> enablement isn't a word. I think "before starting Xen guests" is OK.
> 
> upkept also isn't, "maintained" perhaps.

Amended.

> 
> Also, end users don't care a jot about any of this. Move it right to the
> end of the README? At the moment it comes before the Python stuff which
> is end user focused.

Moved to the bottom.

> TBH if it were me I'd just put the "The file supports a simple.." as a
> comment in config/modules (wherever it ends up) and leave it at that.

Put as a comment on the config OS Linux file.

> > +  * config/modules
> > +
> > +The file supports a simple language, comments are ignored, and if you there
> > +are module replacements this can be listed by using a pipe to show preference
> > +for the first module, followed by the older module.
> > +
> > +We should strive to not require statically loading modules but it seems some
> > +systems have had issues with automatically loading some Linux kernel modules.
> > +For more details refer to:
> 
> This last paragraph is just meta commentary, IMHO it is commit log
> material (at best) rather than README material.

Nuked.

> > +http://wiki.xen.org/wiki/Category:Host_Configuration#Linux_kernel_modules
> > +
> >  Python Runtime Libraries
> >  ========================
> >  
> > diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
> > index d5de9e6..90b0b0c 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/modules $(XENCOMMONS_INITD).in
> > +	@set -e ;							\
> > +	IFS=''								;\
> > +	cat  $(XEN_ROOT)/config/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						;\
> 
> Sed with something like "/@LOAD_MODULES@/r$(...).modules" can replace
> this second loop.

I gave this a few minues minutes but couldn't figure it out, surely this can be
an evolution after ? Below is what I tried.

  Luis

diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
index 90b0b0c..f652451 100644
--- a/tools/hotplug/Linux/Makefile
+++ b/tools/hotplug/Linux/Makefile
@@ -35,10 +35,10 @@ UDEV_RULES = xen-backend.rules $(UDEV_RULES-y)
 .PHONY: all
 all: $(XENCOMMONS_INITD)
 
-$(XENCOMMONS_INITD): $(XEN_ROOT)/config/modules $(XENCOMMONS_INITD).in
+$(XENCOMMONS_INITD): $(XEN_ROOT)/config/$(XEN_OS).modules $(XENCOMMONS_INITD).in
 	@set -e ;							\
 	IFS=''								;\
-	cat  $(XEN_ROOT)/config/modules	| (				\
+	cat  $(XEN_ROOT)/config/$(XEN_OS).modules	| (		\
 		while read l ; do					\
 			if echo $${l} | egrep -q "^#" ; then		\
 				continue				;\
@@ -52,15 +52,12 @@ $(XENCOMMONS_INITD): $(XEN_ROOT)/config/modules $(XENCOMMONS_INITD).in
 			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							\
-	) > $@
+	cat $(XENCOMMONS_INITD).in | ( 					\
+			sed -e '/@LOAD_MODULES@/ {			\
+				r $(XENCOMMONS_INITD).modules		\
+				d					\
+			}' 						\
+	) > $@								;\
 	@rm -f $(XENCOMMONS_INITD).modules
 
 .PHONY: build