Re: [PATCH kvmtool] Make static libc and guest-init functionality optional.

[Andre Przywara <andre.przywara@arm.com>]

Hi Dimitri,

thanks for sharing this patch and sorry for the delay.

(CC:ing Will)

On 04/09/15 13:04, Dimitri John Ledkov wrote:
> If one typically only boots full disk-images, one wouldn't necessaraly
> want to statically link glibc, for the guest-init feature of the
> kvmtool. As statically linked glibc triggers haevy security
> maintainance.

I like the idea of making guest-init optional, and actually was bitten
by this annoying static libc requirement once before.
Some comments below:

> 
> Signed-off-by: Dimitri John Ledkov <dimitri.j.ledkov@intel.com>
> ---
>  Makefile        | 11 ++++++-----
>  builtin-run.c   |  7 +++++++
>  builtin-setup.c |  7 +++++++
>  3 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 1534e6f..42a629a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -34,8 +34,6 @@ bindir_SQ = $(subst ','\'',$(bindir))
>  PROGRAM	:= lkvm
>  PROGRAM_ALIAS := vm
>  
> -GUEST_INIT := guest/init
> -
>  OBJS	+= builtin-balloon.o
>  OBJS	+= builtin-debug.o
>  OBJS	+= builtin-help.o
> @@ -279,8 +277,12 @@ ifeq ($(LTO),1)
>  	endif
>  endif
>  
> -ifneq ($(call try-build,$(SOURCE_STATIC),,-static),y)
> -        $(error No static libc found. Please install glibc-static package.)
> +ifeq ($(call try-build,$(SOURCE_STATIC),,-static),y)
> +	CFLAGS        	+= -DCONFIG_HAS_LIBC

The name CONFIG_HAS_LIBC seems a bit misleading to me, so at least this
symbol should read CONFIG_HAS_STATIC_LIBC. But I'd prefer to have it
named after it's user instead: CONFIG_GUEST_INIT (or the like), since
this is what it protects in the code.

> +	GUEST_INIT := guest/init
> +	GUEST_OBJS = guest/guest_init.o
> +else
> +	NOTFOUND        += static-libc
>  endif
>  
>  ifeq (y,$(ARCH_WANT_LIBFDT))
> @@ -356,7 +358,6 @@ c_flags	= -Wp,-MD,$(depfile) $(CFLAGS)
>  # $(OTHEROBJS) are things that do not get substituted like this.
>  #
>  STATIC_OBJS = $(patsubst %.o,%.static.o,$(OBJS) $(OBJS_STATOPT))
> -GUEST_OBJS = guest/guest_init.o
>  
>  $(PROGRAM)-static:  $(STATIC_OBJS) $(OTHEROBJS) $(GUEST_INIT)
>  	$(E) "  LINK    " $@
> diff --git a/builtin-run.c b/builtin-run.c
> index 1ee75ad..0f67471 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -59,8 +59,13 @@ static int  kvm_run_wrapper;
>  
>  bool do_debug_print = false;
>  
> +#ifdef CONFIG_HAS_LIBC
>  extern char _binary_guest_init_start;
>  extern char _binary_guest_init_size;
> +#else
> +static char _binary_guest_init_start=0;
> +static char _binary_guest_init_size=0;
> +#endif
>  
>  static const char * const run_usage[] = {
>  	"lkvm run [<options>] [<kernel image>]",
> @@ -354,6 +359,8 @@ static int kvm_setup_guest_init(struct kvm *kvm)
>  	char *data;
>  
>  	/* Setup /virt/init */
> +	if (!_binary_guest_init_size)
> +		die("Guest init not compiled");

I wonder if comparing with 0 is safe in every case. I appreciate not
spoiling the code with #ifdefs, but putting one around here seems
cleaner to me (especially if you look at the error message).

>  	size = (size_t)&_binary_guest_init_size;
>  	data = (char *)&_binary_guest_init_start;
>  	snprintf(tmp, PATH_MAX, "%s%s/virt/init", kvm__get_dir(), rootfs);
> diff --git a/builtin-setup.c b/builtin-setup.c
> index 8b45c56..d77e5e0 100644
> --- a/builtin-setup.c
> +++ b/builtin-setup.c
> @@ -16,8 +16,13 @@
>  #include <sys/mman.h>
>  #include <fcntl.h>
>  
> +#ifdef CONFIG_HAS_LIBC
>  extern char _binary_guest_init_start;
>  extern char _binary_guest_init_size;
> +#else
> +static char _binary_guest_init_start=0;
> +static char _binary_guest_init_size=0;
> +#endif
>  
>  static const char *instance_name;
>  
> @@ -131,6 +136,8 @@ static int copy_init(const char *guestfs_name)
>  	int fd, ret;
>  	char *data;
>  
> +	if (!_binary_guest_init_size)
> +		die("Guest init not compiled");

Same as above.

Cheers,
Andre.

>  	size = (size_t)&_binary_guest_init_size;
>  	data = (char *)&_binary_guest_init_start;
>  	snprintf(path, PATH_MAX, "%s%s/virt/init", kvm__get_dir(), guestfs_name);
>