Xen devel - Apr 2013 - [PATCH v2] xenstore: create pidfile in init-xenstore-domain

Since libxl checks for the existance of /var/run/xenstored.pid in order
to ensure xenstore is running, create this file when starting the
xenstore stub domain. This also changes the Makefile to enable the
creation of the init-xenstore-domain tool during tools compilation,
since the existing Makefile incorrectly added to the ALL_TARGETS list
when compiling the stubdom, when this variable is not used.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 tools/xenstore/Makefile               |  5 ++++-
 tools/xenstore/init-xenstore-domain.c | 12 +++++++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
index 9172d3a..1bb6e58 100644
--- a/tools/xenstore/Makefile
+++ b/tools/xenstore/Makefile
@@ -29,9 +29,12 @@ endif
 
 ALL_TARGETS = libxenstore.so libxenstore.a clients xs_tdb_dump xenstored
 
+ifeq ($(CONFIG_Linux),y)
+ALL_TARGETS += init-xenstore-domain
+endif
+
 ifdef CONFIG_STUBDOM
 CFLAGS += -DNO_SOCKETS=1
-ALL_TARGETS += init-xenstore-domain
 endif
 
 .PHONY: all
diff --git a/tools/xenstore/init-xenstore-domain.c
b/tools/xenstore/init-xenstore-domain.c
index 18c075b..35f1aa3 100644
--- a/tools/xenstore/init-xenstore-domain.c
+++ b/tools/xenstore/init-xenstore-domain.c
@@ -1,4 +1,5 @@
 #include <fcntl.h>
+#include <unistd.h>
 #include <stdio.h>
 #include <string.h>
 #include <stdint.h>
@@ -69,7 +70,7 @@ int main(int argc, char** argv)
 	xc_interface *xch;
 	struct xs_handle *xsh;
 	char buf[16];
-	int rv;
+	int rv, fd;
 
 	if (argc != 4) {
 		printf("Use: %s <xenstore-kernel> <memory_mb>
<flask-label>\n", argv[0]);
@@ -90,5 +91,14 @@ int main(int argc, char** argv)
 	xs_write(xsh, XBT_NULL, "/tool/xenstored/domid", buf, rv);
 	xs_daemon_close(xsh);
 
+	fd = creat("/var/run/xenstored.pid", 0666);
+	if (fd < 0)
+		return 3;
+	rv = snprintf(buf, 16, "domid:%d\n", domid);
+	rv = write(fd, buf, rv);
+	close(fd);
+	if (rv < 0)
+		return 3;
+
 	return 0;
 }
-- 
1.8.1.4

On 24.04.13 18:44, Daniel De Graaf wrote:
> Since libxl checks for the existance of /var/run/xenstored.pid in order
> to ensure xenstore is running, create this file when starting the
> xenstore stub domain. This also changes the Makefile to enable the
> creation of the init-xenstore-domain tool during tools compilation,
> since the existing Makefile incorrectly added to the ALL_TARGETS list
> when compiling the stubdom, when this variable is not used.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  tools/xenstore/Makefile               |  5 ++++-
>  tools/xenstore/init-xenstore-domain.c | 12 +++++++++++-
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
> index 9172d3a..1bb6e58 100644
> --- a/tools/xenstore/Makefile
> +++ b/tools/xenstore/Makefile
> @@ -29,9 +29,12 @@ endif
>  
>  ALL_TARGETS = libxenstore.so libxenstore.a clients xs_tdb_dump xenstored
>  
> +ifeq ($(CONFIG_Linux),y)
> +ALL_TARGETS += init-xenstore-domain
> +endif
> +
Please explain what is Linux-specific?
>  ifdef CONFIG_STUBDOM
>  CFLAGS += -DNO_SOCKETS=1
> -ALL_TARGETS += init-xenstore-domain
>  endif
>  
>  .PHONY: all
> diff --git a/tools/xenstore/init-xenstore-domain.c
b/tools/xenstore/init-xenstore-domain.c
> index 18c075b..35f1aa3 100644
> --- a/tools/xenstore/init-xenstore-domain.c
> +++ b/tools/xenstore/init-xenstore-domain.c
> @@ -1,4 +1,5 @@
>  #include <fcntl.h>
> +#include <unistd.h>
>  #include <stdio.h>
>  #include <string.h>
>  #include <stdint.h>
> @@ -69,7 +70,7 @@ int main(int argc, char** argv)
>  	xc_interface *xch;
>  	struct xs_handle *xsh;
>  	char buf[16];
> -	int rv;
> +	int rv, fd;
>  
>  	if (argc != 4) {
>  		printf("Use: %s <xenstore-kernel> <memory_mb>
<flask-label>\n", argv[0]);
> @@ -90,5 +91,14 @@ int main(int argc, char** argv)
>  	xs_write(xsh, XBT_NULL, "/tool/xenstored/domid", buf, rv);
>  	xs_daemon_close(xsh);
>  
> +	fd = creat("/var/run/xenstored.pid", 0666);
> +	if (fd < 0)
> +		return 3;
> +	rv = snprintf(buf, 16, "domid:%d\n", domid);
Use sizeof(buf). That''s less error-prone whenever the size
of buf changes.
> +	rv = write(fd, buf, rv);
> +	close(fd);
> +	if (rv < 0)
> +		return 3;
> +
>  	return 0;
>  }
>

On Thu, 2013-04-25 at 09:19 +0100, Christoph Egger
wrote:
> @@ -29,9 +29,12 @@ endif
> >  
> >  ALL_TARGETS = libxenstore.so libxenstore.a clients xs_tdb_dump
xenstored
> >  
> > +ifeq ($(CONFIG_Linux),y)
> > +ALL_TARGETS += init-xenstore-domain
> > +endif
> > +
> 
> Please explain what is Linux-specific?
Can you confirm that this file at least builds as is on NetBSD? Was it
built there previously?

Ian.

On 25.04.13 10:51, Ian Campbell wrote:
> On Thu, 2013-04-25 at 09:19 +0100, Christoph Egger wrote:
>> @@ -29,9 +29,12 @@ endif
>>>  
>>>  ALL_TARGETS = libxenstore.so libxenstore.a clients xs_tdb_dump
xenstored
>>>  
>>> +ifeq ($(CONFIG_Linux),y)
>>> +ALL_TARGETS += init-xenstore-domain
>>> +endif
>>> +
>>
>> Please explain what is Linux-specific?
> 
> Can you confirm that this file at least builds as is on NetBSD? Was it
> built there previously?
No. It was only enabled for stubdom.
Building it manually says:
<xen/sys/xenbus_dev.h>: No such file or directory.

Christoph

On Thu, 2013-04-25 at 10:05 +0100, Egger, Christoph
wrote:
> On 25.04.13 10:51, Ian Campbell wrote:
> > On Thu, 2013-04-25 at 09:19 +0100, Christoph Egger wrote:
> >> @@ -29,9 +29,12 @@ endif
> >>>  
> >>>  ALL_TARGETS = libxenstore.so libxenstore.a clients
xs_tdb_dump xenstored
> >>>  
> >>> +ifeq ($(CONFIG_Linux),y)
> >>> +ALL_TARGETS += init-xenstore-domain
> >>> +endif
> >>> +
> >>
> >> Please explain what is Linux-specific?
> > 
> > Can you confirm that this file at least builds as is on NetBSD? Was it
> > built there previously?
> 
> No. It was only enabled for stubdom.
> Building it manually says:
> <xen/sys/xenbus_dev.h>: No such file or directory.
OK, so this should be Linux only until that interface is implemented for
BSD (should probably be in libxc in any case)

Ian/

Putting back the original CC list which Christoph dropped a few mails
back. Please be careful of this.

On Thu, 2013-04-25 at 10:56 +0100, Ian Campbell wrote:
> On Thu, 2013-04-25 at 10:05 +0100, Egger, Christoph wrote:
> > On 25.04.13 10:51, Ian Campbell wrote:
> > > On Thu, 2013-04-25 at 09:19 +0100, Christoph Egger wrote:
> > >> @@ -29,9 +29,12 @@ endif
> > >>>  
> > >>>  ALL_TARGETS = libxenstore.so libxenstore.a clients
xs_tdb_dump xenstored
> > >>>  
> > >>> +ifeq ($(CONFIG_Linux),y)
> > >>> +ALL_TARGETS += init-xenstore-domain
> > >>> +endif
> > >>> +
> > >>
> > >> Please explain what is Linux-specific?
> > > 
> > > Can you confirm that this file at least builds as is on NetBSD?
Was it
> > > built there previously?
> > 
> > No. It was only enabled for stubdom.
> > Building it manually says:
> > <xen/sys/xenbus_dev.h>: No such file or directory.
> 
> OK, so this should be Linux only until that interface is implemented for
> BSD (should probably be in libxc in any case)
With this in mind I have acked + applied this. Christoph''s comment
about
using sizeof(buf) is a good one but not worth delaying this patch for.

Ian.