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

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

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.

> 
>>  extras/mini-os/files.mk       |   28 ++++++++++++++++++++++++++++
>>  extras/mini-os/main.c         |   16 ++++++++--------
>>  extras/mini-os/minios.mk      |    4 ++--
>>  stubdom/Makefile              |    8 ++++----
>>  8 files changed, 65 insertions(+), 20 deletions(-)
>>  create mode 100644 extras/mini-os/apps/common.mk
>>  create mode 100644 extras/mini-os/apps/grub.mk
>>  create mode 100644 extras/mini-os/apps/ioemu.mk
>>  create mode 100644 extras/mini-os/files.mk
>>
>> diff --git a/extras/mini-os/Makefile b/extras/mini-os/Makefile
>> index c2ee062..af7d0d4 100644
>> --- a/extras/mini-os/Makefile
>> +++ b/extras/mini-os/Makefile
>> @@ -8,7 +8,12 @@ export XEN_ROOT = $(CURDIR)/../..
>>  include $(XEN_ROOT)/Config.mk
>>  OBJ_DIR ?= $(CURDIR)
>>  
>> -ifneq ($(stubdom),y)
>> +ifeq ($(stubdom),y)
>> +-include apps/$(MINIOS_APP).mk
> 
> If you do as I suggest above this can become an unconditional include.
> 
>> +include apps/common.mk
> 
> Probably the app-specific mk should include this if it wants it, or just
> inline in each app config since I think the contents being common is
> more a coincidence than anything else.
> 
>> +EXTRA_DEPS += $(wildcard $(CURDIR)/apps/$(MINIOS_APP).mk)
>> +EXTRA_DEPS += $(CURDIR)/apps/common.mk
>> +else
>>  include Config.mk
>>  endif
>>  
>> @@ -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).
 
>> +
>>  # The common mini-os objects to build.
>>  APP_OBJS :=
>> -OBJS := $(patsubst %.c,$(OBJ_DIR)/%.o,$(wildcard *.c))
>> -OBJS += $(patsubst %.c,$(OBJ_DIR)/%.o,$(wildcard lib/*.c))
>> -OBJS += $(patsubst %.c,$(OBJ_DIR)/%.o,$(wildcard xenbus/*.c))
>> -OBJS += $(patsubst %.c,$(OBJ_DIR)/%.o,$(wildcard console/*.c))
>> -
>> +OBJS := $(patsubst %.c,$(OBJ_DIR)/%.o,$(src-y))
>>  
>>  .PHONY: default
>>  default: $(OBJ_DIR)/$(TARGET)
>> diff --git a/extras/mini-os/apps/common.mk b/extras/mini-os/apps/common.mk
>> new file mode 100644
>> index 0000000..12b686d
>> --- /dev/null
>> +++ b/extras/mini-os/apps/common.mk
>> @@ -0,0 +1,11 @@
>> +# Defaults
>> +CONFIG_START_NETWORK ?= y
>> +CONFIG_SPARSE_BSS ?= y
>> +
>> +# Export items as compiler directives
>> +flags-$(CONFIG_START_NETWORK) += -DCONFIG_START_NETWORK
>> +flags-$(CONFIG_SPARSE_BSS) += -DCONFIG_SPARSE_BSS
>> +flags-$(CONFIG_QEMU_XS_ARGS) += -DCONFIG_QEMU_XS_ARGS
>> +
>> +DEF_CFLAGS += $(flags-y)
> 
> I'd be inclined to put the CFLAGS stuff in the main makefile. It's not
> really "config" as such but part of the config system scaffolding.

Doing this would mostly eliminate common.mk - which sounds fine.

> [...]
>> 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.

>>      char *vm;
>>      char path[128];
>>      int domid;
>> @@ -60,15 +60,15 @@ static void call_main(void *p)
>>       * crashing. */
>>      //sleep(1);
>>  
>> -#ifndef CONFIG_GRUB
>> +#ifdef CONFIG_SPARSE_BSS
>>      sparse((unsigned long) &__app_bss_start, &__app_bss_end - 
>> &__app_bss_start);
>> -#if defined(HAVE_LWIP) && !defined(CONFIG_QEMU)
>> -    start_networking();
>>  #endif
>> +#if defined(HAVE_LWIP) && defined(CONFIG_START_NETWORK)
> 
> In grub.mk (which I've already trimmed, oops) you have
> CONFIG_START_NETWORK=n
> which will pass that half of the test, which isn't what I think you
> wanted.
> 
> I've just noticed the same with the SPARSE_BSS option. Oh, and common.mk
> actually ends up unconditionally setting some vars too (using ?=).
> 
> I think a Linux style "# CONFIG_FOO is not set" would be better if you
> think it is necessary to explicitly list options we are not enabling.
> 
> Ian.
 
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.

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.

-- 
Daniel De Graaf
National Security Agency

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