From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [PATCH kvmtool] Make static libc and guest-init functionality optional. Date: Fri, 11 Sep 2015 13:47:49 +0100 Message-ID: <55F2CD75.9050108@arm.com> References: <1441368249-23800-1-git-send-email-dimitri.j.ledkov@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Will Deacon To: Dimitri John Ledkov Return-path: Received: from cam-admin0.cambridge.arm.com ([217.140.96.50]:44145 "EHLO cam-admin0.cambridge.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751677AbbIKMqa (ORCPT ); Fri, 11 Sep 2015 08:46:30 -0400 In-Reply-To: <1441368249-23800-1-git-send-email-dimitri.j.ledkov@intel.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 > --- > 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 [] []", > @@ -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 > #include > > +#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); >