On 11 September 2015 at 13:47, Andre Przywara <andre.przywara@xxxxxxx> wrote: > Hi Dimitri, > > thanks for sharing this patch and sorry for the delay. No worries, I have a few more patches to send, polishing them for release. > > (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: > \o/ >> >> Signed-off-by: Dimitri John Ledkov <dimitri.j.ledkov@xxxxxxxxx> >> --- >> 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. > OK, sounds good. I am bad at naming things =) this looks good. >> + 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). Ok, I can put the #ifdef here as well. Note that the non-extern declaration will still be needed in the code above, as otherwise the build fails to link without static-libc. > >> 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. Ack. > > 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); >> -- Regards, Dimitri. Pura Vida! https://clearlinux.org Open Source Technology Center Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html