Re: [Xen-devel] [PATCH 11/21] mini-os: make frontends and xenbus optional

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-01-20 at 20:47 +0000, Daniel De Graaf wrote:
> This adds compile-time logic to disable certain frontends in mini-os:
>  - pcifront is disabled by default, enabled for ioemu
>  - blkfront, netfront, fbfront, and kbdfront are enabled by default
>  - xenbus is required for any frontend, and is enabled by default
> 
> If all frontends and xenbus are disabled, mini-os will run without
> needing to communicate with xenstore, making it suitable to run the
> xenstore daemon.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> ---
>  extras/mini-os/Makefile               |    5 +++-
>  extras/mini-os/apps/common.mk         |   11 +++++++++
>  extras/mini-os/apps/ioemu.mk          |    1 +
>  extras/mini-os/console/xencons_ring.c |   15 ++++++++++--
>  extras/mini-os/files.mk               |   12 +++++-----
>  extras/mini-os/include/lib.h          |    2 +
>  extras/mini-os/kernel.c               |   40 +++++++++++++++++++++++++++++++-
>  extras/mini-os/lib/sys.c              |   28 +++++++++++++++++++++++
>  extras/mini-os/main.c                 |    6 +++-
>  9 files changed, 106 insertions(+), 14 deletions(-)
> 
> diff --git a/extras/mini-os/Makefile b/extras/mini-os/Makefile
> index af7d0d4..7419211 100644
> --- a/extras/mini-os/Makefile
> +++ b/extras/mini-os/Makefile
> @@ -70,7 +70,10 @@ ifeq ($(lwip),y)
>  LWC    := $(shell find $(LWIPDIR)/ -type f -name '*.c')
>  LWC    := $(filter-out %6.c %ip6_addr.c %ethernetif.c, $(LWC))
>  LWO    := $(patsubst %.c,%.o,$(LWC))
> -LWO    += $(addprefix $(OBJ_DIR)/,lwip-arch.o lwip-net.o)
> +LWO    += $(OBJ_DIR)/lwip-arch.o
> +ifeq ($(CONFIG_NETFRONT),y)
> +LWO += $(OBJ_DIR)/lwip-net.o
> +endif

Without lwip-net.o is there any point in having the rest of LWO? Or does
the linker optimise it all away anyway?

[...]

> diff --git a/extras/mini-os/console/xencons_ring.c 
> b/extras/mini-os/console/xencons_ring.c
> index af0afed..c3eba35 100644
> --- a/extras/mini-os/console/xencons_ring.c
> +++ b/extras/mini-os/console/xencons_ring.c
> @@ -189,6 +189,7 @@ struct consfront_dev *xencons_ring_init(void)
> 
>  void free_consfront(struct consfront_dev *dev)
>  {
> +#ifdef CONFIG_XENBUS
>      char* err = NULL;
>      XenbusState state;
> 
> @@ -217,6 +218,7 @@ void free_consfront(struct consfront_dev *dev)
>  close:
>      if (err) free(err);
>      xenbus_unwatch_path_token(XBT_NIL, path, path);
> +#endif
> 
>      mask_evtchn(dev->evtchn);
>      unbind_evtchn(dev->evtchn);
> @@ -231,16 +233,18 @@ close:
> 
>  struct consfront_dev *init_consfront(char *_nodename)
>  {
> +    struct consfront_dev *dev;
> +    char nodename[256];
> +    static int consfrontends = 3;
> +#ifdef CONFIG_XENBUS
>      xenbus_transaction_t xbt;
>      char* err;
>      char* message=NULL;
>      int retry=0;
>      char* msg = NULL;
> -    char nodename[256];
>      char path[256];
> -    static int consfrontends = 3;
> -    struct consfront_dev *dev;
>      int res;
> +#endif
> 
>      if (!_nodename)
>          snprintf(nodename, sizeof(nodename), "device/console/%d", 
> consfrontends);
> @@ -257,6 +261,7 @@ struct consfront_dev *init_consfront(char *_nodename)
>      dev->fd = -1;
>  #endif
> 
> +#ifdef CONFIG_XENBUS
>      snprintf(path, sizeof(path), "%s/backend-id", nodename);
>      if ((res = xenbus_read_integer(path)) < 0)
>          return NULL;
> @@ -351,17 +356,21 @@ done:
>              goto error;
>          }
>      }
> +#endif

Haven't you ifdef'd out everything which would have set dev->evtchn?

I'm not sure that the CONFIG_XENBUS is worthwhile, at least at the
moment, and it seems to add an awful lot of ifdefery.

[...]
> diff --git a/extras/mini-os/kernel.c b/extras/mini-os/kernel.c
> index 2875bf1..9e490d5 100644
> --- a/extras/mini-os/kernel.c
> +++ b/extras/mini-os/kernel.c
> [...]
> @@ -462,11 +474,21 @@ __attribute__((weak)) int app_main(start_info_t *si)
>      printk("Dummy main: start_info=%p\n", si);
>      create_thread("xenbus_tester", xenbus_tester, si);
>      create_thread("periodic_thread", periodic_thread, si);
> +#ifdef CONFIG_NETFRONT
>      create_thread("netfront", netfront_thread, si);
> +#endif

Better to define init_FOOfront for each of these and have it be a nop in
the ifndef case and avoid the ifdefs in the code itself.

Likewise the ifdef's in the teardown. Ideally the actual meat in the
ifdef cases would be moved into the files you aren't compiling (e.g.
netfront_thread goes into netfront.c) and only the stubs remain in some
header somewhere.

Ian.




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