Re: [PATCH 3/9] kvm tools: Allow giving instance names

[Sasha Levin <levinsasha928@gmail.com>]

On Thu, 2011-06-30 at 10:53 +0300, Pekka Enberg wrote:
> On Wed, 29 Jun 2011, Sasha Levin wrote:
> > This will allow tracking instance names and sending commands
> > to specific instances if multiple instances are running.
> >
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> 
> I skipped this and related patches because I really would like to hear 
> what Ingo thinks about the '/var/run/kvm-tools/' pidfile mechanism.
> 
> > ---
> > tools/kvm/include/kvm/kvm.h |    5 +++-
> > tools/kvm/kvm-run.c         |    5 +++-
> > tools/kvm/kvm.c             |   55 ++++++++++++++++++++++++++++++++++++++++++-
> > tools/kvm/term.c            |    3 ++
> > 4 files changed, 65 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/kvm/include/kvm/kvm.h b/tools/kvm/include/kvm/kvm.h
> > index 7d90d35..5ad3236 100644
> > --- a/tools/kvm/include/kvm/kvm.h
> > +++ b/tools/kvm/include/kvm/kvm.h
> > @@ -41,9 +41,11 @@ struct kvm {
> > 	const char		*vmlinux;
> > 	struct disk_image       **disks;
> > 	int                     nr_disks;
> > +
> > +	const char		*name;
> > };
> >
> > -struct kvm *kvm__init(const char *kvm_dev, u64 ram_size);
> > +struct kvm *kvm__init(const char *kvm_dev, u64 ram_size, const char *name);
> > int kvm__max_cpus(struct kvm *kvm);
> > void kvm__init_ram(struct kvm *kvm);
> > void kvm__delete(struct kvm *kvm);
> > @@ -61,6 +63,7 @@ bool kvm__deregister_mmio(struct kvm *kvm, u64 phys_addr);
> > void kvm__pause(void);
> > void kvm__continue(void);
> > void kvm__notify_paused(void);
> > +int kvm__get_pid_by_instance(const char *name);
> >
> > /*
> >  * Debugging
> > diff --git a/tools/kvm/kvm-run.c b/tools/kvm/kvm-run.c
> > index 0dece2d..a4abf76 100644
> > --- a/tools/kvm/kvm-run.c
> > +++ b/tools/kvm/kvm-run.c
> > @@ -69,6 +69,7 @@ static const char *network;
> > static const char *host_ip_addr;
> > static const char *guest_mac;
> > static const char *script;
> > +static const char *guest_name;
> > static bool single_step;
> > static bool readonly_image[MAX_DISK_IMAGES];
> > static bool vnc;
> > @@ -132,6 +133,8 @@ static int virtio_9p_rootdir_parser(const struct option *opt, const char *arg, i
> >
> > static const struct option options[] = {
> > 	OPT_GROUP("Basic options:"),
> > +	OPT_STRING('\0', "name", &guest_name, "guest name",
> > +			"A name for the guest"),
> > 	OPT_INTEGER('c', "cpus", &nrcpus, "Number of CPUs"),
> > 	OPT_U64('m', "mem", &ram_size, "Virtual machine memory size in MiB."),
> > 	OPT_CALLBACK('d', "disk", NULL, "image", "Disk image", img_name_parser),
> > @@ -546,7 +549,7 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
> >
> > 	term_init();
> >
> > -	kvm = kvm__init(kvm_dev, ram_size);
> > +	kvm = kvm__init(kvm_dev, ram_size, guest_name);
> >
> > 	ioeventfd__init();
> >
> > diff --git a/tools/kvm/kvm.c b/tools/kvm/kvm.c
> > index c400c70..4f723a6 100644
> > --- a/tools/kvm/kvm.c
> > +++ b/tools/kvm/kvm.c
> > @@ -113,11 +113,60 @@ static struct kvm *kvm__new(void)
> > 	return kvm;
> > }
> >
> > +static void kvm__create_pidfile(struct kvm *kvm)
> > +{
> > +	int fd;
> > +	char full_name[PATH_MAX], pid[10];
> > +
> > +	if (!kvm->name)
> > +		return;
> > +
> > +	mkdir("/var/run/kvm-tools", 0777);
> 
> mkdir() can fail

My assumption here is that it would indeed fail most of the time, My
idea here was to try to create the directory anyway - even if it's
already there.

> 
> > +	sprintf(full_name, "/var/run/kvm-tools/%s.pid", kvm->name);
> > +	fd = open(full_name, O_CREAT | O_WRONLY, 0666);
> > +	sprintf(pid, "%u\n", getpid());
> > +	if (write(fd, pid, strlen(pid)) <= 0)
> > +		die("Failed creating PID file");
> > +	close(fd);
> > +}
> > +
> > +static void kvm__remove_pidfile(struct kvm *kvm)
> > +{
> > +	char full_name[PATH_MAX];
> > +
> > +	if (!kvm->name)
> > +		return;
> > +
> > +	sprintf(full_name, "/var/run/kvm-tools/%s.pid", kvm->name);
> > +	unlink(full_name);
> 
> unlink() can fail too - dunno if it matters in practice but there 
> shouldn't be harm in checking for it?

I can check for it, but what should I do if it indeed fails? The code is
located on the exit patch, so die() here would do more harm than good.

> 
> > +}
> > +
> > +int kvm__get_pid_by_instance(const char *name)
> > +{
> > +	int fd, pid;
> > +	char pid_str[10], pid_file[PATH_MAX];
> > +
> > +	sprintf(pid_file, "/var/run/kvm-tools/%s.pid", name);
> > +	fd = open(pid_file, O_RDONLY);
> > +	if (fd < 0)
> > +		return -1;
> > +
> > +	if (read(fd, pid_str, 10) == 0)
> > +		return -1;
> > +
> > +	pid = atoi(pid_str);
> > +	if (pid < 0)
> > +		return -1;
> > +
> > +	return pid;
> > +}
> > +
> > void kvm__delete(struct kvm *kvm)
> > {
> > 	kvm__stop_timer(kvm);
> >
> > 	munmap(kvm->ram_start, kvm->ram_size);
> > +	kvm__remove_pidfile(kvm);
> > 	free(kvm);
> > }
> >
> > @@ -237,7 +286,7 @@ int kvm__max_cpus(struct kvm *kvm)
> > 	return ret;
> > }
> >
> > -struct kvm *kvm__init(const char *kvm_dev, u64 ram_size)
> > +struct kvm *kvm__init(const char *kvm_dev, u64 ram_size, const char *name)
> > {
> > 	struct kvm_pit_config pit_config = { .flags = 0, };
> > 	struct kvm *kvm;
> > @@ -300,6 +349,10 @@ struct kvm *kvm__init(const char *kvm_dev, u64 ram_size)
> > 	if (ret < 0)
> > 		die_perror("KVM_CREATE_IRQCHIP ioctl");
> >
> > +	kvm->name = name;
> > +
> > +	kvm__create_pidfile(kvm);
> > +
> > 	return kvm;
> > }
> >
> > diff --git a/tools/kvm/term.c b/tools/kvm/term.c
> > index 9947223..a0cb03f 100644
> > --- a/tools/kvm/term.c
> > +++ b/tools/kvm/term.c
> > @@ -9,7 +9,9 @@
> > #include "kvm/read-write.h"
> > #include "kvm/term.h"
> > #include "kvm/util.h"
> > +#include "kvm/kvm.h"
> >
> > +extern struct kvm *kvm;
> > static struct termios	orig_term;
> >
> > int term_escape_char	= 0x01; /* ctrl-a is used for escape */
> > @@ -32,6 +34,7 @@ int term_getc(int who)
> > 	if (term_got_escape) {
> > 		term_got_escape = false;
> > 		if (c == 'x') {
> > +			kvm__delete(kvm);
> > 			printf("\n  # KVM session terminated.\n");
> > 			exit(1);
> > 		}
> > -- 
> > 1.7.6
> >
> >

-- 

Sasha.