tools/xenstore: Log xenstored build ID on startup

[XEN PATCH] tools/xenstore: Log xenstored build ID on startup

Posted by Bjoern Doebel 3 years, 5 months ago

Right now we do not have a mechanism to determine the version of the
currently running xenstored at runtime. As xenstored runs throughout the
lifetime of a Xen host, this may lead to problems when newer user space
builds are staged. Then, the running xenstored will no longer match the
version of the installed xenstored.

To allow users to always identify the running version of xenstored, add
a linker-generated unique build ID to every xenstored build. Add
functionality to log this build ID into a file upon service startup.

Signed-off-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Martin Mazein <amazein@amazon.de>
Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk>
---
 tools/hotplug/Linux/launch-xenstore.in |  2 +-
 tools/xenstore/Makefile                |  4 +++
 tools/xenstore/buildid_symbols.ld      | 10 +++++++
 tools/xenstore/xenstored_core.c        |  8 ++++++
 tools/xenstore/xenstored_core.h        |  3 ++
 tools/xenstore/xenstored_minios.c      |  4 +++
 tools/xenstore/xenstored_posix.c       | 52 ++++++++++++++++++++++++++++++++++
 7 files changed, 82 insertions(+), 1 deletion(-)
 create mode 100644 tools/xenstore/buildid_symbols.ld

diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
index 991dec8d25..a6f2254030 100644
--- a/tools/hotplug/Linux/launch-xenstore.in
+++ b/tools/hotplug/Linux/launch-xenstore.in
@@ -62,7 +62,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 	}
 
 	echo -n Starting $XENSTORED...
-	$XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
+	$XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid --buildid-file @XEN_RUN_DIR@/xenstored.buildid $XENSTORED_ARGS
 
 	systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1
 
diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
index 9a0f0d012d..c63350980b 100644
--- a/tools/xenstore/Makefile
+++ b/tools/xenstore/Makefile
@@ -66,6 +66,10 @@ $(XENSTORED_OBJS): CFLAGS += $(SYSTEMD_CFLAGS)
 xenstored: LDFLAGS += $(SYSTEMD_LIBS)
 endif
 
+# xenstored: enforce creation of a buildID section and use a linker
+# script to add additional symbols around that section
+xenstored: LDFLAGS +=  -Wl,--build-id=sha1 -Wl,-T,buildid_symbols.ld
+
 $(XENSTORED_OBJS): CFLAGS += $(CFLAGS_libxengnttab)
 
 xenstored: $(XENSTORED_OBJS)
diff --git a/tools/xenstore/buildid_symbols.ld b/tools/xenstore/buildid_symbols.ld
new file mode 100644
index 0000000000..d74024c4e9
--- /dev/null
+++ b/tools/xenstore/buildid_symbols.ld
@@ -0,0 +1,10 @@
+SECTIONS
+{
+       __buildid_note_section = . ;
+       .note.gnu.build-id :
+       {
+               *(.note.gnu.build-id)
+       }
+       __buildid_end = . ;
+}
+INSERT AFTER .data
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index b4be374d3f..c6f107bdd9 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1844,6 +1844,7 @@ static void usage(void)
 
 
 static struct option options[] = {
+	{ "buildid-file", 1, NULL, 'B' },
 	{ "no-domain-init", 0, NULL, 'D' },
 	{ "entry-nb", 1, NULL, 'E' },
 	{ "pid-file", 1, NULL, 'F' },
@@ -1875,12 +1876,16 @@ int main(int argc, char *argv[])
 	bool outputpid = false;
 	bool no_domain_init = false;
 	const char *pidfile = NULL;
+	const char *buildid_file = NULL;
 	int timeout;
 
 
 	while ((opt = getopt_long(argc, argv, "DE:F:HNPS:t:T:RVW:", options,
 				  NULL)) != -1) {
 		switch (opt) {
+		case 'B':
+			buildid_file = optarg;
+			break;
 		case 'D':
 			no_domain_init = true;
 			break;
@@ -1948,6 +1953,9 @@ int main(int argc, char *argv[])
 	if (pidfile)
 		write_pidfile(pidfile);
 
+	if (buildid_file)
+		write_buildid_file(buildid_file);
+
 	/* Talloc leak reports go to stderr, which is closed if we fork. */
 	if (!dofork)
 		talloc_enable_leak_report_full();
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 1df6ad94ab..712280626c 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -193,6 +193,9 @@ void xenbus_notify_running(void);
 /* Write out the pidfile */
 void write_pidfile(const char *pidfile);
 
+/* Write the buildid file */
+void write_buildid_file(const char *buildidfile);
+
 /* Fork but do not close terminal FDs */
 void daemonize(void);
 /* Close stdin/stdout/stderr to complete daemonize */
diff --git a/tools/xenstore/xenstored_minios.c b/tools/xenstore/xenstored_minios.c
index c94493e52a..ef1151aee4 100644
--- a/tools/xenstore/xenstored_minios.c
+++ b/tools/xenstore/xenstored_minios.c
@@ -24,6 +24,10 @@ void write_pidfile(const char *pidfile)
 {
 }
 
+void write_buildid_file(const char *buildid_file)
+{
+}
+
 void daemonize(void)
 {
 }
diff --git a/tools/xenstore/xenstored_posix.c b/tools/xenstore/xenstored_posix.c
index 1f9603fea2..ec017611d6 100644
--- a/tools/xenstore/xenstored_posix.c
+++ b/tools/xenstore/xenstored_posix.c
@@ -20,6 +20,7 @@
 #include <sys/stat.h>
 #include <unistd.h>
 #include <fcntl.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <sys/mman.h>
 
@@ -48,6 +49,57 @@ void write_pidfile(const char *pidfile)
 	close(fd);
 }
 
+/*
+ * We don't have a working elf.h available here, so let's define our very own
+ * data structs and accessor macros for ELF notes.
+ *
+ * https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter6-18048.html:
+ * For 64–bit objects and 32–bit objects, each entry is an array of 4-byte
+ * words in the format of the target processor.
+ */
+typedef struct
+{
+	uint32_t namesz;
+	uint32_t descsz;
+	uint32_t type;
+} elf_note_hdr;
+
+/* ELF Note accessors, copied from Xen's elf.h */
+#define ELFNOTE_ALIGN(_n_) (((_n_)+3)&~3)
+#define ELFNOTE_NAME(_n_) ((char*)(_n_) + sizeof(*(_n_)))
+#define ELFNOTE_DESC(_n_) (ELFNOTE_NAME(_n_) + ELFNOTE_ALIGN((_n_)->namesz))
+/* GNU LD: type == note (NT_GNU_BUILD_ID as in
+ * https://sourceware.org/ml/binutils/2007-07/msg00012.html)*/
+#define NT_GNU_BUILD_ID 3
+
+
+void write_buildid_file(const char *buildid_file)
+{
+	unsigned int i = 0;
+	FILE *fdesc;
+	extern elf_note_hdr __buildid_note_section;
+	unsigned int id_length = __buildid_note_section.descsz;
+	char* desc = ELFNOTE_DESC(&__buildid_note_section);
+
+	if (__buildid_note_section.type != NT_GNU_BUILD_ID)
+		barf("Expected GNU_BUILDID note, but found type '%d'",
+		     __buildid_note_section.type);
+
+	fdesc = fopen(buildid_file, "w+");
+	if (!fdesc)
+		barf_perror("Error opening buildid file %s", buildid_file);
+
+	/* We exit silently if daemon already running. */
+	if (lockf(fileno(fdesc), F_TLOCK, 0) == -1)
+		exit(0);
+
+	for (i = 0; i < id_length; ++i)
+		fprintf(fdesc, "%02x", (unsigned char)desc[i]);
+	fprintf(fdesc, "\n");
+
+	fclose(fdesc);
+}
+
 /* Stevens. */
 void daemonize(void)
 {
-- 
2.16.6




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [XEN PATCH] tools/xenstore: Log xenstored build ID on startup

Posted by Jan Beulich 3 years, 5 months ago

On 13.11.2020 15:18, Bjoern Doebel wrote:
> --- a/tools/xenstore/Makefile
> +++ b/tools/xenstore/Makefile
> @@ -66,6 +66,10 @@ $(XENSTORED_OBJS): CFLAGS += $(SYSTEMD_CFLAGS)
>  xenstored: LDFLAGS += $(SYSTEMD_LIBS)
>  endif
>  
> +# xenstored: enforce creation of a buildID section and use a linker
> +# script to add additional symbols around that section
> +xenstored: LDFLAGS +=  -Wl,--build-id=sha1 -Wl,-T,buildid_symbols.ld

Since in the hypervisor build we're careful to not use --build-id
when the linker doesn't support it, I suppose the same care needs
applying here. See the setting of build_id_linker in ./Config.mk.

Jan

Re: [XEN PATCH] tools/xenstore: Log xenstored build ID on startup

Posted by Bjoern Doebel 3 years, 5 months ago

On 13.11.20 15:27, Jan Beulich wrote:
> On 13.11.2020 15:18, Bjoern Doebel wrote:
>> --- a/tools/xenstore/Makefile
>> +++ b/tools/xenstore/Makefile
>> @@ -66,6 +66,10 @@ $(XENSTORED_OBJS): CFLAGS += $(SYSTEMD_CFLAGS)
>>   xenstored: LDFLAGS += $(SYSTEMD_LIBS)
>>   endif
>>
>> +# xenstored: enforce creation of a buildID section and use a linker
>> +# script to add additional symbols around that section
>> +xenstored: LDFLAGS +=  -Wl,--build-id=sha1 -Wl,-T,buildid_symbols.ld
> Since in the hypervisor build we're careful to not use --build-id
> when the linker doesn't support it, I suppose the same care needs
> applying here. See the setting of build_id_linker in ./Config.mk.

Ok, I will make this conditional on the setting of $(build_id_linker).

>
> Jan

Bjoern




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [XEN PATCH] tools/xenstore: Log xenstored build ID on startup

Posted by Jürgen Groß 3 years, 5 months ago

On 13.11.20 15:18, Bjoern Doebel wrote:
> Right now we do not have a mechanism to determine the version of the
> currently running xenstored at runtime. As xenstored runs throughout the
> lifetime of a Xen host, this may lead to problems when newer user space
> builds are staged. Then, the running xenstored will no longer match the
> version of the installed xenstored.
> 
> To allow users to always identify the running version of xenstored, add
> a linker-generated unique build ID to every xenstored build. Add
> functionality to log this build ID into a file upon service startup.
> 
> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Martin Mazein <amazein@amazon.de>
> Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk>

No support for oxenstored or xenstore-stubdom?

> ---
>   tools/hotplug/Linux/launch-xenstore.in |  2 +-
>   tools/xenstore/Makefile                |  4 +++
>   tools/xenstore/buildid_symbols.ld      | 10 +++++++
>   tools/xenstore/xenstored_core.c        |  8 ++++++
>   tools/xenstore/xenstored_core.h        |  3 ++
>   tools/xenstore/xenstored_minios.c      |  4 +++
>   tools/xenstore/xenstored_posix.c       | 52 ++++++++++++++++++++++++++++++++++
>   7 files changed, 82 insertions(+), 1 deletion(-)
>   create mode 100644 tools/xenstore/buildid_symbols.ld
> 
> diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
> index 991dec8d25..a6f2254030 100644
> --- a/tools/hotplug/Linux/launch-xenstore.in
> +++ b/tools/hotplug/Linux/launch-xenstore.in
> @@ -62,7 +62,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
>   	}
>   
>   	echo -n Starting $XENSTORED...
> -	$XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
> +	$XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid --buildid-file @XEN_RUN_DIR@/xenstored.buildid $XENSTORED_ARGS
>   
>   	systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1
>   
> diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
> index 9a0f0d012d..c63350980b 100644
> --- a/tools/xenstore/Makefile
> +++ b/tools/xenstore/Makefile
> @@ -66,6 +66,10 @@ $(XENSTORED_OBJS): CFLAGS += $(SYSTEMD_CFLAGS)
>   xenstored: LDFLAGS += $(SYSTEMD_LIBS)
>   endif
>   
> +# xenstored: enforce creation of a buildID section and use a linker
> +# script to add additional symbols around that section
> +xenstored: LDFLAGS +=  -Wl,--build-id=sha1 -Wl,-T,buildid_symbols.ld
> +
>   $(XENSTORED_OBJS): CFLAGS += $(CFLAGS_libxengnttab)
>   
>   xenstored: $(XENSTORED_OBJS)
> diff --git a/tools/xenstore/buildid_symbols.ld b/tools/xenstore/buildid_symbols.ld
> new file mode 100644
> index 0000000000..d74024c4e9
> --- /dev/null
> +++ b/tools/xenstore/buildid_symbols.ld
> @@ -0,0 +1,10 @@
> +SECTIONS
> +{
> +       __buildid_note_section = . ;
> +       .note.gnu.build-id :
> +       {
> +               *(.note.gnu.build-id)
> +       }
> +       __buildid_end = . ;
> +}
> +INSERT AFTER .data
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index b4be374d3f..c6f107bdd9 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1844,6 +1844,7 @@ static void usage(void)
>   
>   
>   static struct option options[] = {
> +	{ "buildid-file", 1, NULL, 'B' },
>   	{ "no-domain-init", 0, NULL, 'D' },
>   	{ "entry-nb", 1, NULL, 'E' },
>   	{ "pid-file", 1, NULL, 'F' },
> @@ -1875,12 +1876,16 @@ int main(int argc, char *argv[])
>   	bool outputpid = false;
>   	bool no_domain_init = false;
>   	const char *pidfile = NULL;
> +	const char *buildid_file = NULL;
>   	int timeout;
>   
>   
>   	while ((opt = getopt_long(argc, argv, "DE:F:HNPS:t:T:RVW:", options,

You are missing "B:" in the short options.

>   				  NULL)) != -1) {
>   		switch (opt) {
> +		case 'B':
> +			buildid_file = optarg;
> +			break;
>   		case 'D':
>   			no_domain_init = true;
>   			break;
> @@ -1948,6 +1953,9 @@ int main(int argc, char *argv[])
>   	if (pidfile)
>   		write_pidfile(pidfile);
>   
> +	if (buildid_file)
> +		write_buildid_file(buildid_file);
> +
>   	/* Talloc leak reports go to stderr, which is closed if we fork. */
>   	if (!dofork)
>   		talloc_enable_leak_report_full();

You should update the usage printout, too.

> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
> index 1df6ad94ab..712280626c 100644
> --- a/tools/xenstore/xenstored_core.h
> +++ b/tools/xenstore/xenstored_core.h
> @@ -193,6 +193,9 @@ void xenbus_notify_running(void);
>   /* Write out the pidfile */
>   void write_pidfile(const char *pidfile);
>   
> +/* Write the buildid file */
> +void write_buildid_file(const char *buildidfile);
> +
>   /* Fork but do not close terminal FDs */
>   void daemonize(void);
>   /* Close stdin/stdout/stderr to complete daemonize */
> diff --git a/tools/xenstore/xenstored_minios.c b/tools/xenstore/xenstored_minios.c
> index c94493e52a..ef1151aee4 100644
> --- a/tools/xenstore/xenstored_minios.c
> +++ b/tools/xenstore/xenstored_minios.c
> @@ -24,6 +24,10 @@ void write_pidfile(const char *pidfile)
>   {
>   }
>   
> +void write_buildid_file(const char *buildid_file)
> +{
> +}
> +
>   void daemonize(void)
>   {
>   }
> diff --git a/tools/xenstore/xenstored_posix.c b/tools/xenstore/xenstored_posix.c
> index 1f9603fea2..ec017611d6 100644
> --- a/tools/xenstore/xenstored_posix.c
> +++ b/tools/xenstore/xenstored_posix.c
> @@ -20,6 +20,7 @@
>   #include <sys/stat.h>
>   #include <unistd.h>
>   #include <fcntl.h>
> +#include <stdint.h>
>   #include <stdlib.h>
>   #include <sys/mman.h>
>   
> @@ -48,6 +49,57 @@ void write_pidfile(const char *pidfile)
>   	close(fd);
>   }
>   
> +/*
> + * We don't have a working elf.h available here, so let's define our very own
> + * data structs and accessor macros for ELF notes.
> + *
> + * https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter6-18048.html:
> + * For 64–bit objects and 32–bit objects, each entry is an array of 4-byte
> + * words in the format of the target processor.
> + */
> +typedef struct
> +{
> +	uint32_t namesz;
> +	uint32_t descsz;
> +	uint32_t type;
> +} elf_note_hdr;
> +
> +/* ELF Note accessors, copied from Xen's elf.h */
> +#define ELFNOTE_ALIGN(_n_) (((_n_)+3)&~3)
> +#define ELFNOTE_NAME(_n_) ((char*)(_n_) + sizeof(*(_n_)))
> +#define ELFNOTE_DESC(_n_) (ELFNOTE_NAME(_n_) + ELFNOTE_ALIGN((_n_)->namesz))
> +/* GNU LD: type == note (NT_GNU_BUILD_ID as in
> + * https://sourceware.org/ml/binutils/2007-07/msg00012.html)*/
> +#define NT_GNU_BUILD_ID 3
> +
> +
> +void write_buildid_file(const char *buildid_file)
> +{
> +	unsigned int i = 0;
> +	FILE *fdesc;
> +	extern elf_note_hdr __buildid_note_section;
> +	unsigned int id_length = __buildid_note_section.descsz;
> +	char* desc = ELFNOTE_DESC(&__buildid_note_section);
> +
> +	if (__buildid_note_section.type != NT_GNU_BUILD_ID)
> +		barf("Expected GNU_BUILDID note, but found type '%d'",
> +		     __buildid_note_section.type);
> +
> +	fdesc = fopen(buildid_file, "w+");
> +	if (!fdesc)
> +		barf_perror("Error opening buildid file %s", buildid_file);
> +
> +	/* We exit silently if daemon already running. */
> +	if (lockf(fileno(fdesc), F_TLOCK, 0) == -1)
> +		exit(0);
> +
> +	for (i = 0; i < id_length; ++i)
> +		fprintf(fdesc, "%02x", (unsigned char)desc[i]);
> +	fprintf(fdesc, "\n");
> +
> +	fclose(fdesc);
> +}
> +
>   /* Stevens. */
>   void daemonize(void)
>   {
> 

In general I don't like the approach using a file for this purpose.
In case there is no generic solution possible, I'd rather have a
XS_CONTROL sub command to get the current version from Xenstore. This
will then at once cover xenstore-stubdom, too.


Juergen

Re: [XEN PATCH] tools/xenstore: Log xenstored build ID on startup

Posted by Bjoern Doebel 3 years, 5 months ago

On 13.11.20 16:36, Jürgen Groß wrote:
> On 13.11.20 15:18, Bjoern Doebel wrote:
>> Right now we do not have a mechanism to determine the version of the
>> currently running xenstored at runtime. As xenstored runs throughout the
>> lifetime of a Xen host, this may lead to problems when newer user space
>> builds are staged. Then, the running xenstored will no longer match the
>> version of the installed xenstored.
>>
>> To allow users to always identify the running version of xenstored, add
>> a linker-generated unique build ID to every xenstored build. Add
>> functionality to log this build ID into a file upon service startup.
>>
>> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
>> Reviewed-by: Martin Mazein <amazein@amazon.de>
>> Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk>
>
> No support for oxenstored or xenstore-stubdom?

Your suggestion further down will apparently help for stubdom. I do not 
speak ocaml at all - how do we address this?

>
>> ---
>>   tools/hotplug/Linux/launch-xenstore.in |  2 +-
>>   tools/xenstore/Makefile                |  4 +++
>>   tools/xenstore/buildid_symbols.ld      | 10 +++++++
>>   tools/xenstore/xenstored_core.c        |  8 ++++++
>>   tools/xenstore/xenstored_core.h        |  3 ++
>>   tools/xenstore/xenstored_minios.c      |  4 +++
>>   tools/xenstore/xenstored_posix.c       | 52 
>> ++++++++++++++++++++++++++++++++++
>>   7 files changed, 82 insertions(+), 1 deletion(-)
>>   create mode 100644 tools/xenstore/buildid_symbols.ld
>>
>> diff --git a/tools/hotplug/Linux/launch-xenstore.in 
>> b/tools/hotplug/Linux/launch-xenstore.in
>> index 991dec8d25..a6f2254030 100644
>> --- a/tools/hotplug/Linux/launch-xenstore.in
>> +++ b/tools/hotplug/Linux/launch-xenstore.in
>> @@ -62,7 +62,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons 
>> && . @CONFIG_DIR@/@CONFIG_LEAF
>>       }
>>         echo -n Starting $XENSTORED...
>> -    $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
>> +    $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid --buildid-file 
>> @XEN_RUN_DIR@/xenstored.buildid $XENSTORED_ARGS
>>         systemd-notify --booted 2>/dev/null || timeout_xenstore 
>> $XENSTORED || exit 1
>>   diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
>> index 9a0f0d012d..c63350980b 100644
>> --- a/tools/xenstore/Makefile
>> +++ b/tools/xenstore/Makefile
>> @@ -66,6 +66,10 @@ $(XENSTORED_OBJS): CFLAGS += $(SYSTEMD_CFLAGS)
>>   xenstored: LDFLAGS += $(SYSTEMD_LIBS)
>>   endif
>>   +# xenstored: enforce creation of a buildID section and use a linker
>> +# script to add additional symbols around that section
>> +xenstored: LDFLAGS +=  -Wl,--build-id=sha1 -Wl,-T,buildid_symbols.ld
>> +
>>   $(XENSTORED_OBJS): CFLAGS += $(CFLAGS_libxengnttab)
>>     xenstored: $(XENSTORED_OBJS)
>> diff --git a/tools/xenstore/buildid_symbols.ld 
>> b/tools/xenstore/buildid_symbols.ld
>> new file mode 100644
>> index 0000000000..d74024c4e9
>> --- /dev/null
>> +++ b/tools/xenstore/buildid_symbols.ld
>> @@ -0,0 +1,10 @@
>> +SECTIONS
>> +{
>> +       __buildid_note_section = . ;
>> +       .note.gnu.build-id :
>> +       {
>> +               *(.note.gnu.build-id)
>> +       }
>> +       __buildid_end = . ;
>> +}
>> +INSERT AFTER .data
>> diff --git a/tools/xenstore/xenstored_core.c 
>> b/tools/xenstore/xenstored_core.c
>> index b4be374d3f..c6f107bdd9 100644
>> --- a/tools/xenstore/xenstored_core.c
>> +++ b/tools/xenstore/xenstored_core.c
>> @@ -1844,6 +1844,7 @@ static void usage(void)
>>       static struct option options[] = {
>> +    { "buildid-file", 1, NULL, 'B' },
>>       { "no-domain-init", 0, NULL, 'D' },
>>       { "entry-nb", 1, NULL, 'E' },
>>       { "pid-file", 1, NULL, 'F' },
>> @@ -1875,12 +1876,16 @@ int main(int argc, char *argv[])
>>       bool outputpid = false;
>>       bool no_domain_init = false;
>>       const char *pidfile = NULL;
>> +    const char *buildid_file = NULL;
>>       int timeout;
>>           while ((opt = getopt_long(argc, argv, "DE:F:HNPS:t:T:RVW:", 
>> options,
>
> You are missing "B:" in the short options.
Ack. Fixing.
>
>>                     NULL)) != -1) {
>>           switch (opt) {
>> +        case 'B':
>> +            buildid_file = optarg;
>> +            break;
>>           case 'D':
>>               no_domain_init = true;
>>               break;
>> @@ -1948,6 +1953,9 @@ int main(int argc, char *argv[])
>>       if (pidfile)
>>           write_pidfile(pidfile);
>>   +    if (buildid_file)
>> +        write_buildid_file(buildid_file);
>> +
>>       /* Talloc leak reports go to stderr, which is closed if we 
>> fork. */
>>       if (!dofork)
>>           talloc_enable_leak_report_full();
>
> You should update the usage printout, too.
Ok.
>
>> diff --git a/tools/xenstore/xenstored_core.h 
>> b/tools/xenstore/xenstored_core.h
>> index 1df6ad94ab..712280626c 100644
>> --- a/tools/xenstore/xenstored_core.h
>> +++ b/tools/xenstore/xenstored_core.h
>> @@ -193,6 +193,9 @@ void xenbus_notify_running(void);
>>   /* Write out the pidfile */
>>   void write_pidfile(const char *pidfile);
>>   +/* Write the buildid file */
>> +void write_buildid_file(const char *buildidfile);
>> +
>>   /* Fork but do not close terminal FDs */
>>   void daemonize(void);
>>   /* Close stdin/stdout/stderr to complete daemonize */
>> diff --git a/tools/xenstore/xenstored_minios.c 
>> b/tools/xenstore/xenstored_minios.c
>> index c94493e52a..ef1151aee4 100644
>> --- a/tools/xenstore/xenstored_minios.c
>> +++ b/tools/xenstore/xenstored_minios.c
>> @@ -24,6 +24,10 @@ void write_pidfile(const char *pidfile)
>>   {
>>   }
>>   +void write_buildid_file(const char *buildid_file)
>> +{
>> +}
>> +
>>   void daemonize(void)
>>   {
>>   }
>> diff --git a/tools/xenstore/xenstored_posix.c 
>> b/tools/xenstore/xenstored_posix.c
>> index 1f9603fea2..ec017611d6 100644
>> --- a/tools/xenstore/xenstored_posix.c
>> +++ b/tools/xenstore/xenstored_posix.c
>> @@ -20,6 +20,7 @@
>>   #include <sys/stat.h>
>>   #include <unistd.h>
>>   #include <fcntl.h>
>> +#include <stdint.h>
>>   #include <stdlib.h>
>>   #include <sys/mman.h>
>>   @@ -48,6 +49,57 @@ void write_pidfile(const char *pidfile)
>>       close(fd);
>>   }
>>   +/*
>> + * We don't have a working elf.h available here, so let's define our 
>> very own
>> + * data structs and accessor macros for ELF notes.
>> + *
>> + * 
>> https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter6-18048.html:
>> + * For 64–bit objects and 32–bit objects, each entry is an array of 
>> 4-byte
>> + * words in the format of the target processor.
>> + */
>> +typedef struct
>> +{
>> +    uint32_t namesz;
>> +    uint32_t descsz;
>> +    uint32_t type;
>> +} elf_note_hdr;
>> +
>> +/* ELF Note accessors, copied from Xen's elf.h */
>> +#define ELFNOTE_ALIGN(_n_) (((_n_)+3)&~3)
>> +#define ELFNOTE_NAME(_n_) ((char*)(_n_) + sizeof(*(_n_)))
>> +#define ELFNOTE_DESC(_n_) (ELFNOTE_NAME(_n_) + 
>> ELFNOTE_ALIGN((_n_)->namesz))
>> +/* GNU LD: type == note (NT_GNU_BUILD_ID as in
>> + * https://sourceware.org/ml/binutils/2007-07/msg00012.html)*/
>> +#define NT_GNU_BUILD_ID 3
>> +
>> +
>> +void write_buildid_file(const char *buildid_file)
>> +{
>> +    unsigned int i = 0;
>> +    FILE *fdesc;
>> +    extern elf_note_hdr __buildid_note_section;
>> +    unsigned int id_length = __buildid_note_section.descsz;
>> +    char* desc = ELFNOTE_DESC(&__buildid_note_section);
>> +
>> +    if (__buildid_note_section.type != NT_GNU_BUILD_ID)
>> +        barf("Expected GNU_BUILDID note, but found type '%d'",
>> +             __buildid_note_section.type);
>> +
>> +    fdesc = fopen(buildid_file, "w+");
>> +    if (!fdesc)
>> +        barf_perror("Error opening buildid file %s", buildid_file);
>> +
>> +    /* We exit silently if daemon already running. */
>> +    if (lockf(fileno(fdesc), F_TLOCK, 0) == -1)
>> +        exit(0);
>> +
>> +    for (i = 0; i < id_length; ++i)
>> +        fprintf(fdesc, "%02x", (unsigned char)desc[i]);
>> +    fprintf(fdesc, "\n");
>> +
>> +    fclose(fdesc);
>> +}
>> +
>>   /* Stevens. */
>>   void daemonize(void)
>>   {
>>
>
> In general I don't like the approach using a file for this purpose.
> In case there is no generic solution possible, I'd rather have a
> XS_CONTROL sub command to get the current version from Xenstore. This
> will then at once cover xenstore-stubdom, too.
>
I will have a look into that.
>
> Juergen
Bjoern



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [XEN PATCH] tools/xenstore: Log xenstored build ID on startup

Posted by Andrew Cooper 3 years, 5 months ago

On 13/11/2020 16:56, Bjoern Doebel wrote:
> On 13.11.20 16:36, Jürgen Groß wrote:
>> On 13.11.20 15:18, Bjoern Doebel wrote:
>>> Right now we do not have a mechanism to determine the version of the
>>> currently running xenstored at runtime. As xenstored runs throughout
>>> the
>>> lifetime of a Xen host, this may lead to problems when newer user space
>>> builds are staged. Then, the running xenstored will no longer match the
>>> version of the installed xenstored.
>>>
>>> To allow users to always identify the running version of xenstored, add
>>> a linker-generated unique build ID to every xenstored build. Add
>>> functionality to log this build ID into a file upon service startup.
>>>
>>> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
>>> Reviewed-by: Martin Mazein <amazein@amazon.de>
>>> Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk>
>>
>> No support for oxenstored or xenstore-stubdom?
>
> Your suggestion further down will apparently help for stubdom. I do
> not speak ocaml at all - how do we address this?

CC'ing Edwin and Christian who have done the bulk of the oxenstored
recently.

It sounds like it might not be possible right now, but would be possible
with a future plan to switch the Ocaml build system over to dune (the
new/preferred Ocaml upstream toolchain).

If it does end up being an XS_CONTROL sub-op, we can implement it at a
future point when we can usefully answer the question.

~Andrew

Re: [XEN PATCH] tools/xenstore: Log xenstored build ID on startup

Posted by Edwin Torok 3 years, 5 months ago

On Fri, 2020-11-13 at 17:13 +0000, Andrew Cooper wrote:
> On 13/11/2020 16:56, Bjoern Doebel wrote:
> > On 13.11.20 16:36, Jürgen Groß wrote:
> > > On 13.11.20 15:18, Bjoern Doebel wrote:
> > > > Right now we do not have a mechanism to determine the version
> > > > of the
> > > > currently running xenstored at runtime. As xenstored runs
> > > > throughout
> > > > the
> > > > lifetime of a Xen host, this may lead to problems when newer
> > > > user space
> > > > builds are staged. Then, the running xenstored will no longer
> > > > match the
> > > > version of the installed xenstored.
> > > > 
> > > > To allow users to always identify the running version of
> > > > xenstored, add
> > > > a linker-generated unique build ID to every xenstored build.
> > > > Add
> > > > functionality to log this build ID into a file upon service
> > > > startup.
> > > > 
> > > > Signed-off-by: Bjoern Doebel <doebel@amazon.de>
> > > > Reviewed-by: Martin Mazein <amazein@amazon.de>
> > > > Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk>
> > > 
> > > No support for oxenstored or xenstore-stubdom?
> > 
> > Your suggestion further down will apparently help for stubdom. I do
> > not speak ocaml at all - how do we address this?
> 
> CC'ing Edwin and Christian who have done the bulk of the oxenstored
> recently.
> 
> It sounds like it might not be possible right now, but would be
> possible
> with a future plan to switch the Ocaml build system over to dune (the
> new/preferred Ocaml upstream toolchain).

See here what is possible with Dune:
https://dune.readthedocs.io/en/stable/dune-libs.html#build-info

Would the output of 'git describe --always --dirty' (perhaps combined
with a build date) serve as a useful build ID?

> 
> If it does end up being an XS_CONTROL sub-op, we can implement it at
> a
> future point when we can usefully answer the question.

Wouldn't using readelf on /proc/<pid>/exe give you the running buildid?

readelf -a /usr/sbin/oxenstored /proc/$(pidof oxenstored)/exe | grep
'Build ID'
    Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae
    Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae

readelf -a /usr/sbin/oxenstored /proc/$(pidof oxenstored)/exe | grep
'Build ID'
    Build ID: b44ff99b216db7526e3ee7841068d584cc9c2b95
    Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae


When you're inside a stubdom it is probably not so easy though.

Best regards,
--Edwin

> 
> ~Andrew

Re: [XEN PATCH] tools/xenstore: Log xenstored build ID on startup

Posted by Bjoern Doebel 3 years, 5 months ago

On 13.11.20 18:23, Edwin Torok wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Fri, 2020-11-13 at 17:13 +0000, Andrew Cooper wrote:
>> On 13/11/2020 16:56, Bjoern Doebel wrote:
>>> On 13.11.20 16:36, Jürgen Groß wrote:
>>>> On 13.11.20 15:18, Bjoern Doebel wrote:
>>>>> Right now we do not have a mechanism to determine the version
>>>>> of the
>>>>> currently running xenstored at runtime. As xenstored runs
>>>>> throughout
>>>>> the
>>>>> lifetime of a Xen host, this may lead to problems when newer
>>>>> user space
>>>>> builds are staged. Then, the running xenstored will no longer
>>>>> match the
>>>>> version of the installed xenstored.
>>>>>
>>>>> To allow users to always identify the running version of
>>>>> xenstored, add
>>>>> a linker-generated unique build ID to every xenstored build.
>>>>> Add
>>>>> functionality to log this build ID into a file upon service
>>>>> startup.
>>>>>
>>>>> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
>>>>> Reviewed-by: Martin Mazein <amazein@amazon.de>
>>>>> Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk>
>>>> No support for oxenstored or xenstore-stubdom?
>>> Your suggestion further down will apparently help for stubdom. I do
>>> not speak ocaml at all - how do we address this?
>> CC'ing Edwin and Christian who have done the bulk of the oxenstored
>> recently.
>>
>> It sounds like it might not be possible right now, but would be
>> possible
>> with a future plan to switch the Ocaml build system over to dune (the
>> new/preferred Ocaml upstream toolchain).
> See here what is possible with Dune:
> https://dune.readthedocs.io/en/stable/dune-libs.html#build-info
>
> Would the output of 'git describe --always --dirty' (perhaps combined
> with a build date) serve as a useful build ID?

The point of the build ID is to verify something like 
"binary-equivalence" of two builds.

* a git hash is not sufficient because different git hashes may result 
in the same binary to be created (i.e., if there is no code change in 
the target binary in between those two builds)

* a time stamp is counter-productive, because then you'd have to 
recreate this timestamp every time you want to re-create a build

GNU ld's --build-id claims to perform a checksumming of the "normative 
parts of the output contents". Whatever that means. ;)

>
>> If it does end up being an XS_CONTROL sub-op, we can implement it at
>> a
>> future point when we can usefully answer the question.
> Wouldn't using readelf on /proc/<pid>/exe give you the running buildid?
>
> readelf -a /usr/sbin/oxenstored /proc/$(pidof oxenstored)/exe | grep
> 'Build ID'
>      Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae
>      Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae
>
> readelf -a /usr/sbin/oxenstored /proc/$(pidof oxenstored)/exe | grep
> 'Build ID'
>      Build ID: b44ff99b216db7526e3ee7841068d584cc9c2b95
>      Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae
>
>
> When you're inside a stubdom it is probably not so easy though.

Interesting. I had not considered that because after upgrading xenstored 
to a different version, the running xenstored's /proc/$PID/exe shows as

# ls -l /proc/$(pgrep xenstored)/exe
lrwxrwxrwx 1 root root 0 Nov  9 14:06 /proc/3528/exe -> 
/usr/sbin/xenstored (deleted)

But you are right, one can still read that procfs file. Nice!


Bjoern





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [XEN PATCH] tools/xenstore: Log xenstored build ID on startup

Posted by Christian Lindig 3 years, 5 months ago

How about keeping such an ID in xenstore itself in some kind of /meta hierarchy where xenstore could also keep stats? As long xenstore is running this information is easily accessible for outside tools.

-- C

________________________________________
From: Bjoern Doebel <doebel@amazon.de>
Sent: 16 November 2020 07:53
To: Edwin Torok; xen-devel@lists.xenproject.org; jgross@suse.com; Andrew Cooper
Cc: wl@xen.org; Christian Lindig; jgrall@amazon.co.uk; elnikety@amazon.de; iwj@xenproject.org
Subject: Re: [XEN PATCH] tools/xenstore: Log xenstored build ID on startup


On 13.11.20 18:23, Edwin Torok wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Fri, 2020-11-13 at 17:13 +0000, Andrew Cooper wrote:
>> On 13/11/2020 16:56, Bjoern Doebel wrote:
>>> On 13.11.20 16:36, Jürgen Groß wrote:
>>>> On 13.11.20 15:18, Bjoern Doebel wrote:
>>>>> Right now we do not have a mechanism to determine the version
>>>>> of the
>>>>> currently running xenstored at runtime. As xenstored runs
>>>>> throughout
>>>>> the
>>>>> lifetime of a Xen host, this may lead to problems when newer
>>>>> user space
>>>>> builds are staged. Then, the running xenstored will no longer
>>>>> match the
>>>>> version of the installed xenstored.
>>>>>
>>>>> To allow users to always identify the running version of
>>>>> xenstored, add
>>>>> a linker-generated unique build ID to every xenstored build.
>>>>> Add
>>>>> functionality to log this build ID into a file upon service
>>>>> startup.
>>>>>
>>>>> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
>>>>> Reviewed-by: Martin Mazein <amazein@amazon.de>
>>>>> Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk>
>>>> No support for oxenstored or xenstore-stubdom?
>>> Your suggestion further down will apparently help for stubdom. I do
>>> not speak ocaml at all - how do we address this?
>> CC'ing Edwin and Christian who have done the bulk of the oxenstored
>> recently.
>>
>> It sounds like it might not be possible right now, but would be
>> possible
>> with a future plan to switch the Ocaml build system over to dune (the
>> new/preferred Ocaml upstream toolchain).
> See here what is possible with Dune:
> https://dune.readthedocs.io/en/stable/dune-libs.html#build-info
>
> Would the output of 'git describe --always --dirty' (perhaps combined
> with a build date) serve as a useful build ID?

The point of the build ID is to verify something like
"binary-equivalence" of two builds.

* a git hash is not sufficient because different git hashes may result
in the same binary to be created (i.e., if there is no code change in
the target binary in between those two builds)

* a time stamp is counter-productive, because then you'd have to
recreate this timestamp every time you want to re-create a build

GNU ld's --build-id claims to perform a checksumming of the "normative
parts of the output contents". Whatever that means. ;)

>
>> If it does end up being an XS_CONTROL sub-op, we can implement it at
>> a
>> future point when we can usefully answer the question.
> Wouldn't using readelf on /proc/<pid>/exe give you the running buildid?
>
> readelf -a /usr/sbin/oxenstored /proc/$(pidof oxenstored)/exe | grep
> 'Build ID'
>      Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae
>      Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae
>
> readelf -a /usr/sbin/oxenstored /proc/$(pidof oxenstored)/exe | grep
> 'Build ID'
>      Build ID: b44ff99b216db7526e3ee7841068d584cc9c2b95
>      Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae
>
>
> When you're inside a stubdom it is probably not so easy though.

Interesting. I had not considered that because after upgrading xenstored
to a different version, the running xenstored's /proc/$PID/exe shows as

# ls -l /proc/$(pgrep xenstored)/exe
lrwxrwxrwx 1 root root 0 Nov  9 14:06 /proc/3528/exe ->
/usr/sbin/xenstored (deleted)

But you are right, one can still read that procfs file. Nice!


Bjoern





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [XEN PATCH] tools/xenstore: Log xenstored build ID on startup

Posted by Andrew Cooper 3 years, 5 months ago

On 13/11/2020 14:18, Bjoern Doebel wrote:
> Right now we do not have a mechanism to determine the version of the
> currently running xenstored at runtime. As xenstored runs throughout the
> lifetime of a Xen host, this may lead to problems when newer user space
> builds are staged. Then, the running xenstored will no longer match the
> version of the installed xenstored.
>
> To allow users to always identify the running version of xenstored, add
> a linker-generated unique build ID to every xenstored build. Add
> functionality to log this build ID into a file upon service startup.
>
> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Martin Mazein <amazein@amazon.de>
> Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk>

I understand the problem you're trying to solve, but why is this
anything more than just enabling build-id's by default across tools/ ?

There are already standard ways of interacting with the build id of
running executables on the system.  I'd strongly discourage doing
anything custom in xenstored specifically.

~Andrew

Re: [XEN PATCH] tools/xenstore: Log xenstored build ID on startup

Posted by Bjoern Doebel 3 years, 5 months ago

On 13.11.20 15:30, Andrew Cooper wrote:
> On 13/11/2020 14:18, Bjoern Doebel wrote:
>> Right now we do not have a mechanism to determine the version of the
>> currently running xenstored at runtime. As xenstored runs throughout the
>> lifetime of a Xen host, this may lead to problems when newer user space
>> builds are staged. Then, the running xenstored will no longer match the
>> version of the installed xenstored.
>>
>> To allow users to always identify the running version of xenstored, add
>> a linker-generated unique build ID to every xenstored build. Add
>> functionality to log this build ID into a file upon service startup.
>>
>> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
>> Reviewed-by: Martin Mazein <amazein@amazon.de>
>> Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk>
> I understand the problem you're trying to solve, but why is this
> anything more than just enabling build-id's by default across tools/ ?
>
> There are already standard ways of interacting with the build id of
> running executables on the system.  I'd strongly discourage doing
> anything custom in xenstored specifically.
May I ask what tooling you would use to interact with a running process' 
buildid?
> ~Andrew

Bjoern




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [XEN PATCH] tools/xenstore: Log xenstored build ID on startup

Posted by Andrew Cooper 3 years, 5 months ago

On 13/11/2020 16:55, Bjoern Doebel wrote:
>
> On 13.11.20 15:30, Andrew Cooper wrote:
>> On 13/11/2020 14:18, Bjoern Doebel wrote:
>>> Right now we do not have a mechanism to determine the version of the
>>> currently running xenstored at runtime. As xenstored runs throughout
>>> the
>>> lifetime of a Xen host, this may lead to problems when newer user space
>>> builds are staged. Then, the running xenstored will no longer match the
>>> version of the installed xenstored.
>>>
>>> To allow users to always identify the running version of xenstored, add
>>> a linker-generated unique build ID to every xenstored build. Add
>>> functionality to log this build ID into a file upon service startup.
>>>
>>> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
>>> Reviewed-by: Martin Mazein <amazein@amazon.de>
>>> Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk>
>> I understand the problem you're trying to solve, but why is this
>> anything more than just enabling build-id's by default across tools/ ?
>>
>> There are already standard ways of interacting with the build id of
>> running executables on the system.  I'd strongly discourage doing
>> anything custom in xenstored specifically.
> May I ask what tooling you would use to interact with a running
> process' buildid?

Amongst other things, yes.  Although as Juergen points out, we want
something which works with stub domains as well, and "normal userspace
tools" won't cut it there.

I still think a first patch in this series should be to turn build-id's
on by default if supported by the toolchain, generally.

~Andrew