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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.


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

> +
>  # 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.

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

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



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