Re: [PATCH v2 2/2] kvm tools: Add '--rootfs' and '--binsh'

[Pekka Enberg <penberg@kernel.org>]

On 8/3/11 5:52 AM, Aneesh Kumar K.V wrote:
> On Tue,  2 Aug 2011 21:46:29 +0300, Sasha Levin<levinsasha928@gmail.com>  wrote:
>> This patch adds 2 new flags:
>>
>>   --rootfs [path] - Specifies a path to use as rootfs. The path will be mounted
>> using virtio-9p and booted from.
>> Easiest way to test it is to mount the sample image we recommend with kvm tool
>> (http://wiki.qemu.org/download/linux-0.2.img.bz2) and mounting it somewhere.
>> *PLEASE DO NOT TRY BOOTING FROM YOUR REAL ROOT AKA '/', EVEN WITH READ ONLY
>> SET* - Lets make sure the code is bulletproof before we delete your rootfs :)
>>
>>   --binsh - This flag will boot into simple /bin/sh shell instead of going
>> through /sbin/init. This is usefull for more complex distribution rootfs
>> where we have to deal with /etc/fstab and such.
>>
>> Signed-off-by: Sasha Levin<levinsasha928@gmail.com>
> Reviewed-by: Aneesh Kumar K.V<aneesh.kumar@linux.vnet.ibm.com>
>
>> ---
>>   tools/kvm/builtin-run.c |   18 +++++++++++++++++-
>>   1 files changed, 17 insertions(+), 1 deletions(-)
>>
>> diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
>> index 2e04265..78680ec 100644
>> --- a/tools/kvm/builtin-run.c
>> +++ b/tools/kvm/builtin-run.c
>> @@ -76,11 +76,13 @@ static const char *guest_mac;
>>   static const char *host_mac;
>>   static const char *script;
>>   static const char *guest_name;
>> +static const char *rootfs;
>>   static bool single_step;
>>   static bool readonly_image[MAX_DISK_IMAGES];
>>   static bool vnc;
>>   static bool sdl;
>>   static bool balloon;
>> +static bool bin_sh;
>>   extern bool ioport_debug;
>>   extern int  active_console;
>>   extern int  debug_iodelay;
>> @@ -156,6 +158,8 @@ static const struct option options[] = {
>>   	OPT_BOOLEAN('\0', "balloon",&balloon, "Enable virtio balloon"),
>>   	OPT_BOOLEAN('\0', "vnc",&vnc, "Enable VNC framebuffer"),
>>   	OPT_BOOLEAN('\0', "sdl",&sdl, "Enable SDL framebuffer"),
>> +	OPT_STRING('\0', "rootfs",&rootfs, "path", "Use path as rootfs"),
>> +	OPT_BOOLEAN('\0', "binsh",&bin_sh, "Boot rootfs to /bin/sh instead of /sbin/init"),
>>
>>   	OPT_GROUP("Kernel options:"),
>>   	OPT_STRING('k', "kernel",&kernel_filename, "kernel",
>> @@ -615,7 +619,7 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>>   		strlcat(real_cmdline, kernel_cmdline, sizeof(real_cmdline));
>>
>>   	hi = NULL;
>> -	if (!image_filename[0]) {
>> +	if (!rootfs&&  !image_filename[0]) {
>>   		hi = host_image(real_cmdline, sizeof(real_cmdline));
>>   		if (hi) {
>>   			image_filename[0] = hi;
>> @@ -627,6 +631,18 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>>   	if (!strstr(real_cmdline, "root="))
>>   		strlcat(real_cmdline, " root=/dev/vda rw ", sizeof(real_cmdline));
>>
>> +	if (rootfs) {
>> +		char tmp[PATH_MAX];
>> +
>> +		if (realpath(rootfs, tmp) == 0 ||
>> +		    virtio_9p__init(kvm, tmp, "/dev/root")<  0)
>> +			die("Unable to initialize virtio 9p");
>> +		strcat(real_cmdline, " root=/dev/root rootflags=rw,trans=virtio,version=9p2000.u rootfstype=9p");
>> +
>> +		if (bin_sh)
>> +			strcat(real_cmdline, " init=/bin/sh");
>> +	}
>>

Why don't we use the "-d" command line option for this? We can
use fstat() and S_ISDIR to detect that the user specified a directory
as a disk image?

I'm also not completely convinced we should introduce the 'binsh'
command line option. Instead, I'd simply default to 'init=/bin/sh'
unless the user uses -p to specify init.

                         Pekka