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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux