Re: [Xen-devel] [PATCH 10/21] mini-os: create app-specific configuration

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-01-23 at 16:05 +0000, Daniel De Graaf wrote:
> On 01/23/2012 07:41 AM, Ian Campbell wrote:
> > On Fri, 2012-01-20 at 20:47 +0000, Daniel De Graaf wrote:
> >> Instead of using CONFIG_QEMU and CONFIG_GRUB to enable or disable minios
> >> code, create CONFIG_ items for features and use application-specific
> >> configuration files to enable or disable the features.
> >>
> >> The configuration flags are currently added to the compiler command
> >> line; as the number of flags grows this may need to move to a header.
> >>
> >> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> >> ---
> >>  extras/mini-os/Makefile       |   15 +++++++++------
> >>  extras/mini-os/apps/common.mk |   11 +++++++++++
> >>  extras/mini-os/apps/grub.mk   |    2 ++
> >>  extras/mini-os/apps/ioemu.mk  |    1 +
> > 
> > I think these should go under stubdom/xxx. You can simply pass in
> > MINIOS_CONFIG as an absolute path and included it
> > ifneq($(MINIOS_CONFIG),) instead of the ifeq($(stubdom),y) change you
> > made.
> > 
> 
> That location also looks nicer, but it will make it more likely that some
> config file will be missed if the defaults are updated. Shouldn't be too
> much of a problem, though - we only have 5 stubdom configs at the moment.

Perhaps a comment directing people to look in stubdoms/... too?

> >> @@ -34,13 +39,11 @@ TARGET := mini-os
> >>  # Subdirectories common to mini-os
> >>  SUBDIRS := lib xenbus console
> >>  
> >> +include files.mk
> > 
> > I don't think moving this out of line is necessary, the pattern in moast
> > of our makefiles is to have the obj-(YN) stuff inline in the Makefiles.
> 
> OK, I wasn't sure how this Makefile was intended split up (it has some logic
> in minios.mk that seemed related).

minios.mk looks like general rules for building bits of mini-os itself,
it's included from submakefiles too (we'd normally call that Rules.mk
these days).

> > [...]
> >> diff --git a/extras/mini-os/main.c b/extras/mini-os/main.c
> >> index b95b889..aeda548 100644
> >> --- a/extras/mini-os/main.c
> >> +++ b/extras/mini-os/main.c
> >> @@ -43,13 +43,13 @@ extern char __app_bss_start, __app_bss_end;
> >>  static void call_main(void *p)
> >>  {
> >>      char *c, quote;
> >> -#ifdef CONFIG_QEMU
> >> +#ifdef CONFIG_QEMU_XS_ARGS
> >>      char *domargs, *msg;
> >>  #endif
> >>      int argc;
> >>      char **argv;
> >>      char *envp[] = { NULL };
> >> -#ifdef CONFIG_QEMU
> >> +#ifdef CONFIG_QEMU_XS_ARGS
> > 
> > If you allow for the "%s/image/dmargs" (not shown in the patch context)
> > to come from a CONFIG_MUMBLE then this is no longer QEMU specific.
> 
> It's still mostly ioemu-specific, since we start by looking up a target
> domain ID, convert it to a VM path, and then look for a path under there.
> Making only the final path of /vm/UUID/image/dmargs configurable doesn't
> sound as useful for a general case, and making the intermediate steps
> configurable would be a mess.

Oh, I hadn't realised it was reading from the target VM and not the stub
VM -- as you say that does make it somewhat more convoluted.

Maybe just refactoring into a function of it's own would help. I was
going to suggest CONFIG_HAS_ARGS_PARSER surrounding a call to
parse_the_args which was supplied from under stubdoms but I don't see
anywhere convenient to put it.

[...] 
> Actually, =n will result in the C symbol being undefined; the Makefile symbol
> can't be undefined or the ?= used to set defaults will override it.

Oh right, this is the CPP symbol not the make one which is done with
CFLAGS-$(XXX) += -D$(XXX) -- so that does indeed work.

> The other way I thought of doing it is to discard the defaults and add them to
> each stubdom's configuration, but this seemed more prone to getting out of 
> sync
> when adding new configuration items.

Sure.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel