[PATCH] kvmtool: don't rely on $HOME

[PATCH] kvmtool: don't rely on $HOME

[Alban Crequy]

kvm__set_dir() called in main() and kvm__get_dir() rely on $HOME. But in
some environments (such as starting lkvm through systemd-run), $HOME is
undefined. This causes bind() to use a socket path containing "(null)"
and this fails. The current code does not check errors returned by
realpath().

Symptoms:

| bind: No such file or directory
|   Error: Failed adding socket to epoll
|  Warning: Failed init: kvm_ipc__init
|
|  Fatal: Initialisation failed

This bug was first reported on https://github.com/coreos/rkt/issues/1393

Instead of using "$HOME/.lkvm/" (i.e. "/root/.lkvm/"), this patch uses
"/var/lib/lkvm/". This also improve the error reporting by printing the
socket filename.

Signed-off-by: Alban Crequy <alban.crequy@gmail.com>
---
 include/kvm/kvm.h |  5 ++---
 kvm-ipc.c         |  1 +
 kvm.c             | 26 ++++++++------------------
 main.c            |  6 +++++-
 4 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 37155db..368f256 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -16,8 +16,7 @@
 #define SIGKVMEXIT		(SIGRTMIN + 0)
 #define SIGKVMPAUSE		(SIGRTMIN + 1)
 
-#define KVM_PID_FILE_PATH	"/.lkvm/"
-#define HOME_DIR		getenv("HOME")
+#define KVM_PID_FILE_PATH	"/var/lib/lkvm/"
 #define KVM_BINARY_NAME		"lkvm"
 
 #ifndef PAGE_SIZE
@@ -70,7 +69,7 @@ struct kvm {
 	int			vm_state;
 };
 
-void kvm__set_dir(const char *fmt, ...);
+int kvm__set_dir(void);
 const char *kvm__get_dir(void);
 
 int kvm__init(struct kvm *kvm);
diff --git a/kvm-ipc.c b/kvm-ipc.c
index 857b0dc..d94456c 100644
--- a/kvm-ipc.c
+++ b/kvm-ipc.c
@@ -60,6 +60,7 @@ static int kvm__create_socket(struct kvm *kvm)
 	r = bind(s, (struct sockaddr *)&local, len);
 	if (r < 0) {
 		perror("bind");
+		pr_err("Cannot bind on %s", full_name);
 		goto fail;
 	}
 
diff --git a/kvm.c b/kvm.c
index 10ed230..482f47b 100644
--- a/kvm.c
+++ b/kvm.c
@@ -63,31 +63,21 @@ extern struct kvm_ext kvm_req_ext[];
 
 static char kvm_dir[PATH_MAX];
 
-static int set_dir(const char *fmt, va_list args)
+int kvm__set_dir(void)
 {
-	char tmp[PATH_MAX];
+	int err;
 
-	vsnprintf(tmp, sizeof(tmp), fmt, args);
-
-	mkdir(tmp, 0777);
-
-	if (!realpath(tmp, kvm_dir))
-		return -errno;
+	err = mkdir(KVM_PID_FILE_PATH, 0700);
+	if (err != 0 && errno != EEXIST) {
+		perror("mkdir " KVM_PID_FILE_PATH);
+		return 1;
+	}
 
-	strcat(kvm_dir, "/");
+	snprintf(kvm_dir, sizeof(kvm_dir), KVM_PID_FILE_PATH);
 
 	return 0;
 }
 
-void kvm__set_dir(const char *fmt, ...)
-{
-	va_list args;
-
-	va_start(args, fmt);
-	set_dir(fmt, args);
-	va_end(args);
-}
-
 const char *kvm__get_dir(void)
 {
 	return kvm_dir;
diff --git a/main.c b/main.c
index 05bc82c..22cc4e2 100644
--- a/main.c
+++ b/main.c
@@ -13,7 +13,11 @@ static int handle_kvm_command(int argc, char **argv)
 
 int main(int argc, char *argv[])
 {
-	kvm__set_dir("%s/%s", HOME_DIR, KVM_PID_FILE_PATH);
+	int ret;
+
+	ret = kvm__set_dir();
+	if (ret != 0)
+		return ret;
 
 	return handle_kvm_command(argc - 1, &argv[1]);
 }
-- 
2.4.3

Re: [PATCH] kvmtool: don't rely on $HOME

[Will Deacon]

On Thu, Sep 17, 2015 at 03:03:15PM +0100, Alban Crequy wrote:
> kvm__set_dir() called in main() and kvm__get_dir() rely on $HOME. But in
> some environments (such as starting lkvm through systemd-run), $HOME is
> undefined. This causes bind() to use a socket path containing "(null)"
> and this fails. The current code does not check errors returned by
> realpath().
> 
> Symptoms:
> 
> | bind: No such file or directory
> |   Error: Failed adding socket to epoll
> |  Warning: Failed init: kvm_ipc__init
> |
> |  Fatal: Initialisation failed
> 
> This bug was first reported on https://github.com/coreos/rkt/issues/1393
> 
> Instead of using "$HOME/.lkvm/" (i.e. "/root/.lkvm/"), this patch uses
> "/var/lib/lkvm/". This also improve the error reporting by printing the
> socket filename.

Hmm, but that requires lkvm to be run with sufficient privileges to
write to /var/lib, which I don't think is generally the case. I think we
have a few options:

  (1) Try /var/lib/lkvm if $HOME is NULL
  (2) Use an alternative environment variable for the pid prefix
  (3) Add a --pid command line option for the pidfile
  (4) ???

Any preferences? What do other projects do?

Will

Re: [PATCH] kvmtool: don't rely on $HOME

[Vasiliy Tolstov]

2015-09-17 18:53 GMT+03:00 Will Deacon <will.deacon@arm.com>:
>
> On Thu, Sep 17, 2015 at 03:03:15PM +0100, Alban Crequy wrote:
> > kvm__set_dir() called in main() and kvm__get_dir() rely on $HOME. But in
> > some environments (such as starting lkvm through systemd-run), $HOME is
> > undefined. This causes bind() to use a socket path containing "(null)"
> > and this fails. The current code does not check errors returned by
> > realpath().
> >
> > Symptoms:
> >
> > | bind: No such file or directory
> > |   Error: Failed adding socket to epoll
> > |  Warning: Failed init: kvm_ipc__init
> > |
> > |  Fatal: Initialisation failed
> >
> > This bug was first reported on https://github.com/coreos/rkt/issues/1393
> >


Why not pass HOME via systemd unit for example to
Environment=HOME=/var/lib/lkvm ?


-- 
Vasiliy Tolstov,
e-mail: v.tolstov@selfip.ru

Re: [PATCH] kvmtool: don't rely on $HOME

[Dimitri John Ledkov]

Hello,

On 17 September 2015 at 15:03, Alban Crequy <alban.crequy@gmail.com> wrote:
> kvm__set_dir() called in main() and kvm__get_dir() rely on $HOME. But in
> some environments (such as starting lkvm through systemd-run), $HOME is
> undefined. This causes bind() to use a socket path containing "(null)"
> and this fails. The current code does not check errors returned by
> realpath().
>
> Symptoms:
>
> | bind: No such file or directory
> |   Error: Failed adding socket to epoll
> |  Warning: Failed init: kvm_ipc__init
> |
> |  Fatal: Initialisation failed
>
> This bug was first reported on https://github.com/coreos/rkt/issues/1393
>
> Instead of using "$HOME/.lkvm/" (i.e. "/root/.lkvm/"), this patch uses
> "/var/lib/lkvm/". This also improve the error reporting by printing the
> socket filename.
>

kvmtool is used as the hypervisor in Clear Containers, at clearlinux.org.

I have seen similar issue, and e.g. I would welcome a --pid or
--socket options in lkvm, however I wouldn't want to remove the
current HOME handling as it is good enough.

What we have done, is essentially pass a HOME variable to the location
we want lkvm to use. Sure, a subfolder will be created. Can you simply
set appropriate HOME variable?

Failing that, lkvm could migrate to XDG spec and use XDG_CONFIG_HOME &
XDG_CONFIG_DIRS to store things, which should work for both regular
users and restricted environments (ending up with /etc/xdg/lkvm for
home-less case).

Regards,

Dimitri.

> Signed-off-by: Alban Crequy <alban.crequy@gmail.com>
> ---
>  include/kvm/kvm.h |  5 ++---
>  kvm-ipc.c         |  1 +
>  kvm.c             | 26 ++++++++------------------
>  main.c            |  6 +++++-
>  4 files changed, 16 insertions(+), 22 deletions(-)
>
> diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
> index 37155db..368f256 100644
> --- a/include/kvm/kvm.h
> +++ b/include/kvm/kvm.h
> @@ -16,8 +16,7 @@
>  #define SIGKVMEXIT             (SIGRTMIN + 0)
>  #define SIGKVMPAUSE            (SIGRTMIN + 1)
>
> -#define KVM_PID_FILE_PATH      "/.lkvm/"
> -#define HOME_DIR               getenv("HOME")
> +#define KVM_PID_FILE_PATH      "/var/lib/lkvm/"
>  #define KVM_BINARY_NAME                "lkvm"
>
>  #ifndef PAGE_SIZE
> @@ -70,7 +69,7 @@ struct kvm {
>         int                     vm_state;
>  };
>
> -void kvm__set_dir(const char *fmt, ...);
> +int kvm__set_dir(void);
>  const char *kvm__get_dir(void);
>
>  int kvm__init(struct kvm *kvm);
> diff --git a/kvm-ipc.c b/kvm-ipc.c
> index 857b0dc..d94456c 100644
> --- a/kvm-ipc.c
> +++ b/kvm-ipc.c
> @@ -60,6 +60,7 @@ static int kvm__create_socket(struct kvm *kvm)
>         r = bind(s, (struct sockaddr *)&local, len);
>         if (r < 0) {
>                 perror("bind");
> +               pr_err("Cannot bind on %s", full_name);
>                 goto fail;
>         }
>
> diff --git a/kvm.c b/kvm.c
> index 10ed230..482f47b 100644
> --- a/kvm.c
> +++ b/kvm.c
> @@ -63,31 +63,21 @@ extern struct kvm_ext kvm_req_ext[];
>
>  static char kvm_dir[PATH_MAX];
>
> -static int set_dir(const char *fmt, va_list args)
> +int kvm__set_dir(void)
>  {
> -       char tmp[PATH_MAX];
> +       int err;
>
> -       vsnprintf(tmp, sizeof(tmp), fmt, args);
> -
> -       mkdir(tmp, 0777);
> -
> -       if (!realpath(tmp, kvm_dir))
> -               return -errno;
> +       err = mkdir(KVM_PID_FILE_PATH, 0700);
> +       if (err != 0 && errno != EEXIST) {
> +               perror("mkdir " KVM_PID_FILE_PATH);
> +               return 1;
> +       }
>
> -       strcat(kvm_dir, "/");
> +       snprintf(kvm_dir, sizeof(kvm_dir), KVM_PID_FILE_PATH);
>
>         return 0;
>  }
>
> -void kvm__set_dir(const char *fmt, ...)
> -{
> -       va_list args;
> -
> -       va_start(args, fmt);
> -       set_dir(fmt, args);
> -       va_end(args);
> -}
> -
>  const char *kvm__get_dir(void)
>  {
>         return kvm_dir;
> diff --git a/main.c b/main.c
> index 05bc82c..22cc4e2 100644
> --- a/main.c
> +++ b/main.c
> @@ -13,7 +13,11 @@ static int handle_kvm_command(int argc, char **argv)
>
>  int main(int argc, char *argv[])
>  {
> -       kvm__set_dir("%s/%s", HOME_DIR, KVM_PID_FILE_PATH);
> +       int ret;
> +
> +       ret = kvm__set_dir();
> +       if (ret != 0)
> +               return ret;
>
>         return handle_kvm_command(argc - 1, &argv[1]);
>  }
> --
> 2.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Regards,

Dimitri.
98 sleeps till Christmas

https://clearlinux.org
Open Source Technology Center
Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ.

Re: [PATCH] kvmtool: don't rely on $HOME

[Alban Crequy]

On 18 September 2015 at 09:59, Vasiliy Tolstov <v.tolstov@selfip.ru> wrote:
> 2015-09-17 18:53 GMT+03:00 Will Deacon <will.deacon@arm.com>:
>>
>> On Thu, Sep 17, 2015 at 03:03:15PM +0100, Alban Crequy wrote:
>> > kvm__set_dir() called in main() and kvm__get_dir() rely on $HOME. But in
>> > some environments (such as starting lkvm through systemd-run), $HOME is
>> > undefined. This causes bind() to use a socket path containing "(null)"
>> > and this fails. The current code does not check errors returned by
>> > realpath().
>> >
>> > Symptoms:
>> >
>> > | bind: No such file or directory
>> > |   Error: Failed adding socket to epoll
>> > |  Warning: Failed init: kvm_ipc__init
>> > |
>> > |  Fatal: Initialisation failed
>> >
>> > This bug was first reported on https://github.com/coreos/rkt/issues/1393
>> >
>
>
> Why not pass HOME via systemd unit for example to
> Environment=HOME=/var/lib/lkvm ?

We can also use "systemd-run --uid=0" (or "User=root" in the .service
file) and systemd will automatically set the $HOME variable.

But if $HOME is mandatory for kvmtool, the error message should be
improved: instead of "bind: No such file or directory", it should say
"error: $HOME is undefined" or something.

On 17 September 2015 at 17:53, Will Deacon <will.deacon@arm.com> wrote:
> Hmm, but that requires lkvm to be run with sufficient privileges to
> write to /var/lib, which I don't think is generally the case. I think we
> have a few options:
>
>   (1) Try /var/lib/lkvm if $HOME is NULL
>   (2) Use an alternative environment variable for the pid prefix
>   (3) Add a --pid command line option for the pidfile
>   (4) ???
>
> Any preferences? What do other projects do?

I don't know what is better.

Re: [PATCH] kvmtool: don't rely on $HOME

[Riku Voipio]

On 17 September 2015 at 18:53, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Sep 17, 2015 at 03:03:15PM +0100, Alban Crequy wrote:
>> kvm__set_dir() called in main() and kvm__get_dir() rely on $HOME. But in
>> some environments (such as starting lkvm through systemd-run), $HOME is
>> undefined. This causes bind() to use a socket path containing "(null)"
>> and this fails. The current code does not check errors returned by
>> realpath().
>>
>> Symptoms:
>>
>> | bind: No such file or directory
>> |   Error: Failed adding socket to epoll
>> |  Warning: Failed init: kvm_ipc__init
>> |
>> |  Fatal: Initialisation failed
>>
>> This bug was first reported on https://github.com/coreos/rkt/issues/1393
>>
>> Instead of using "$HOME/.lkvm/" (i.e. "/root/.lkvm/"), this patch uses
>> "/var/lib/lkvm/". This also improve the error reporting by printing the
>> socket filename.
>
> Hmm, but that requires lkvm to be run with sufficient privileges to
> write to /var/lib, which I don't think is generally the case. I think we
> have a few options:
>
>   (1) Try /var/lib/lkvm if $HOME is NULL
>   (2) Use an alternative environment variable for the pid prefix
>   (3) Add a --pid command line option for the pidfile
>   (4) ???
>
> Any preferences? What do other projects do?

The right place to put a pid file would be $XDG_RUNTIME_DIR aka
/run/user/$UID/. System services write their pidfiles and sockets to
plain /run

The place where to put the rootfs structure created in builtin-setup.c
is more complicated. Is the  created rootfs epheremal? Then sticking
under /run(user/UID) is fine.

Riku

Re: [PATCH] kvmtool: don't rely on $HOME

[Will Deacon]

On Fri, Sep 18, 2015 at 11:51:37AM +0100, Riku Voipio wrote:
> On 17 September 2015 at 18:53, Will Deacon <will.deacon@arm.com> wrote:
> > On Thu, Sep 17, 2015 at 03:03:15PM +0100, Alban Crequy wrote:
> >> kvm__set_dir() called in main() and kvm__get_dir() rely on $HOME. But in
> >> some environments (such as starting lkvm through systemd-run), $HOME is
> >> undefined. This causes bind() to use a socket path containing "(null)"
> >> and this fails. The current code does not check errors returned by
> >> realpath().
> >>
> >> Symptoms:
> >>
> >> | bind: No such file or directory
> >> |   Error: Failed adding socket to epoll
> >> |  Warning: Failed init: kvm_ipc__init
> >> |
> >> |  Fatal: Initialisation failed
> >>
> >> This bug was first reported on https://github.com/coreos/rkt/issues/1393
> >>
> >> Instead of using "$HOME/.lkvm/" (i.e. "/root/.lkvm/"), this patch uses
> >> "/var/lib/lkvm/". This also improve the error reporting by printing the
> >> socket filename.
> >
> > Hmm, but that requires lkvm to be run with sufficient privileges to
> > write to /var/lib, which I don't think is generally the case. I think we
> > have a few options:
> >
> >   (1) Try /var/lib/lkvm if $HOME is NULL
> >   (2) Use an alternative environment variable for the pid prefix
> >   (3) Add a --pid command line option for the pidfile
> >   (4) ???
> >
> > Any preferences? What do other projects do?
> 
> The right place to put a pid file would be $XDG_RUNTIME_DIR aka
> /run/user/$UID/. System services write their pidfiles and sockets to
> plain /run

Ok, that certainly sounds like the right things to do for temporary
structures then. Thanks.

> The place where to put the rootfs structure created in builtin-setup.c
> is more complicated. Is the  created rootfs epheremal? Then sticking
> under /run(user/UID) is fine.

Currently, the filesystem persists across multiple invocations of lkvm,
so I can imagine somebody being surprised if what they thought was
persistent was lost over something like a reboot.

Will

Re: [PATCH] kvmtool: don't rely on $HOME

[Dimitri John Ledkov]

On 18 September 2015 at 13:56, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Sep 18, 2015 at 11:51:37AM +0100, Riku Voipio wrote:
>> On 17 September 2015 at 18:53, Will Deacon <will.deacon@arm.com> wrote:
>> > On Thu, Sep 17, 2015 at 03:03:15PM +0100, Alban Crequy wrote:
>> >> kvm__set_dir() called in main() and kvm__get_dir() rely on $HOME. But in
>> >> some environments (such as starting lkvm through systemd-run), $HOME is
>> >> undefined. This causes bind() to use a socket path containing "(null)"
>> >> and this fails. The current code does not check errors returned by
>> >> realpath().
>> >>
>> >> Symptoms:
>> >>
>> >> | bind: No such file or directory
>> >> |   Error: Failed adding socket to epoll
>> >> |  Warning: Failed init: kvm_ipc__init
>> >> |
>> >> |  Fatal: Initialisation failed
>> >>
>> >> This bug was first reported on https://github.com/coreos/rkt/issues/1393
>> >>
>> >> Instead of using "$HOME/.lkvm/" (i.e. "/root/.lkvm/"), this patch uses
>> >> "/var/lib/lkvm/". This also improve the error reporting by printing the
>> >> socket filename.
>> >
>> > Hmm, but that requires lkvm to be run with sufficient privileges to
>> > write to /var/lib, which I don't think is generally the case. I think we
>> > have a few options:
>> >
>> >   (1) Try /var/lib/lkvm if $HOME is NULL
>> >   (2) Use an alternative environment variable for the pid prefix
>> >   (3) Add a --pid command line option for the pidfile
>> >   (4) ???ow
>> >
>> > Any preferences? What do other projects do?
>>
>> The right place to put a pid file would be $XDG_RUNTIME_DIR aka
>> /run/user/$UID/. System services write their pidfiles and sockets to
>> plain /run
>
> Ok, that certainly sounds like the right things to do for temporary
> structures then. Thanks.
>
>> The place where to put the rootfs structure created in builtin-setup.c
>> is more complicated. Is the  created rootfs epheremal? Then sticking
>> under /run(user/UID) is fine.
>
> Currently, the filesystem persists across multiple invocations of lkvm,
> so I can imagine somebody being surprised if what they thought was
> persistent was lost over something like a reboot.

http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html

In that case that data could go into XDG_DATA_HOME, XDG_CONFIG_HOME,
or XDG_CACHE_HOME depending on what semantic meaning it has.

-- 
Regards,

Dimitri.
98 sleeps till Christmas

https://clearlinux.org
Open Source Technology Center
Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ.