Re: [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem
On 13.11.19 15:12, Jan Beulich wrote:
On 12.11.2019 17:45, Jürgen Groß wrote:
On 12.11.19 15:22, Jan Beulich wrote:
On 02.10.2019 13:20, Juergen Gross wrote:
@@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan
subdir-$(CONFIG_NEEDS_LIBELF) += libelf
subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
+
+config_data.c: ../.config
+ ( echo "char xen_config_data[] ="; \
+ ../tools/bin2c <$<; \
+ echo ";" ) > $@
Furthermore is there a reason to expose this as plain text, when
Linux exposes a gzip-ed version in /proc? The file isn't very
large now, but this was also the case for Linux many years ago.
gzip data may contain bytes with 0x00. Supporting that would require a
different interface at all levels.
Then perhaps better do so now, when the code is still in flux, than
after the fact, especially if "at all levels" is meant to also
include the public interface?
I'll have a look into that.
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = {
.dir = &hypfs_root,
};
+static struct hypfs_dir hypfs_buildinfo = {
+.list = LIST_HEAD_INIT(hypfs_buildinfo.list),
+};
+
static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
{
int ret = -ENOENT;
@@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd,
return ret;
}
+
+static int __init hypfs_init(void)
+{
+int ret;
+
+ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo);
+BUG_ON(ret);
+ret = hypfs_new_entry_string(&hypfs_buildinfo, "config", xen_config_data);
+BUG_ON(ret);
+
+return 0;
+}
+__initcall(hypfs_init);
Hmm, do you really want to centralize population of the file system
here, rather than having the individual components take care of it?
I can add a new source, e.g. common/buildinfo.c if you like that better.
I was rather thinking of moving this into common/kernel.c, next to the
version hypercall handling, and together with exposing the suggested
values here ahead of exposing .config.
Okay.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem
On 12.11.2019 17:45, Jürgen Groß wrote:
> On 12.11.19 15:22, Jan Beulich wrote:
>> On 02.10.2019 13:20, Juergen Gross wrote:
>>> @@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan
>>>
>>> subdir-$(CONFIG_NEEDS_LIBELF) += libelf
>>> subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
>>> +
>>> +config_data.c: ../.config
>>> + ( echo "char xen_config_data[] ="; \
>>> + ../tools/bin2c <$<; \
>>> + echo ";" ) > $@
>>
>> Furthermore is there a reason to expose this as plain text, when
>> Linux exposes a gzip-ed version in /proc? The file isn't very
>> large now, but this was also the case for Linux many years ago.
>
> gzip data may contain bytes with 0x00. Supporting that would require a
> different interface at all levels.
Then perhaps better do so now, when the code is still in flux, than
after the fact, especially if "at all levels" is meant to also
include the public interface?
>>> --- a/xen/common/hypfs.c
>>> +++ b/xen/common/hypfs.c
>>> @@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = {
>>> .dir = &hypfs_root,
>>> };
>>>
>>> +static struct hypfs_dir hypfs_buildinfo = {
>>> +.list = LIST_HEAD_INIT(hypfs_buildinfo.list),
>>> +};
>>> +
>>> static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry
>>> *new)
>>> {
>>> int ret = -ENOENT;
>>> @@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd,
>>>
>>> return ret;
>>> }
>>> +
>>> +static int __init hypfs_init(void)
>>> +{
>>> +int ret;
>>> +
>>> +ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo);
>>> +BUG_ON(ret);
>>> +ret = hypfs_new_entry_string(&hypfs_buildinfo, "config",
>>> xen_config_data);
>>> +BUG_ON(ret);
>>> +
>>> +return 0;
>>> +}
>>> +__initcall(hypfs_init);
>>
>> Hmm, do you really want to centralize population of the file system
>> here, rather than having the individual components take care of it?
>
> I can add a new source, e.g. common/buildinfo.c if you like that better.
I was rather thinking of moving this into common/kernel.c, next to the
version hypercall handling, and together with exposing the suggested
values here ahead of exposing .config.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem
On 12.11.19 15:22, Jan Beulich wrote:
On 02.10.2019 13:20, Juergen Gross wrote:
Add the /buildinfo/config entry to the hypervisor filesystem. This
entry contains the .config file used to build the hypervisor.
I think this is the 2nd step ahead of the 1st: Much of the stuff
exposed as XENVER_* sub-ops should manifest itself here ahead of
exposing xen/.config.
Yes and no. This is meant as a replacement for my previous patch series
adding .config read support.
It is no problem to add other data as well, but the need for being able
to read .config contents was already agreed on.
@@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan
subdir-$(CONFIG_NEEDS_LIBELF) += libelf
subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
+
+config_data.c: ../.config
+ ( echo "char xen_config_data[] ="; \
+ ../tools/bin2c <$<; \
+ echo ";" ) > $@
This is the typical kind of construct that may break (a subsequent
build attempt) when interrupted in the middle. This pretty clearly
is a move-if-changed candidate, at the same time also avoiding a
(cheap, but anyway) pointless re-build in case .config was touched
without actually changing.
Okay.
Furthermore is there a reason to expose this as plain text, when
Linux exposes a gzip-ed version in /proc? The file isn't very
large now, but this was also the case for Linux many years ago.
gzip data may contain bytes with 0x00. Supporting that would require a
different interface at all levels.
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = {
.dir = &hypfs_root,
};
+static struct hypfs_dir hypfs_buildinfo = {
+.list = LIST_HEAD_INIT(hypfs_buildinfo.list),
+};
+
static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
{
int ret = -ENOENT;
@@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd,
return ret;
}
+
+static int __init hypfs_init(void)
+{
+int ret;
+
+ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo);
+BUG_ON(ret);
+ret = hypfs_new_entry_string(&hypfs_buildinfo, "config", xen_config_data);
+BUG_ON(ret);
+
+return 0;
+}
+__initcall(hypfs_init);
Hmm, do you really want to centralize population of the file system
here, rather than having the individual components take care of it?
I can add a new source, e.g. common/buildinfo.c if you like that better.
--- a/xen/tools/Makefile
+++ b/xen/tools/Makefile
@@ -1,13 +1,18 @@
include $(XEN_ROOT)/Config.mk
+PROGS = symbols bin2c
+
.PHONY: default
default:
- $(MAKE) symbols
+ $(MAKE) $(PROGS)
Could I ask you to take the opportunity and do away with the
unnecessary (as it seems to me) make recursion? $(PROGS) could
easily become a dependency of "default" afaict.
Fine with me.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem
On 02.10.2019 13:20, Juergen Gross wrote:
> Add the /buildinfo/config entry to the hypervisor filesystem. This
> entry contains the .config file used to build the hypervisor.
I think this is the 2nd step ahead of the 1st: Much of the stuff
exposed as XENVER_* sub-ops should manifest itself here ahead of
exposing xen/.config.
> @@ -79,3 +80,11 @@ subdir-$(CONFIG_UBSAN) += ubsan
>
> subdir-$(CONFIG_NEEDS_LIBELF) += libelf
> subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
> +
> +config_data.c: ../.config
> + ( echo "char xen_config_data[] ="; \
> + ../tools/bin2c <$<; \
> + echo ";" ) > $@
This is the typical kind of construct that may break (a subsequent
build attempt) when interrupted in the middle. This pretty clearly
is a move-if-changed candidate, at the same time also avoiding a
(cheap, but anyway) pointless re-build in case .config was touched
without actually changing.
Furthermore is there a reason to expose this as plain text, when
Linux exposes a gzip-ed version in /proc? The file isn't very
large now, but this was also the case for Linux many years ago.
> --- a/xen/common/hypfs.c
> +++ b/xen/common/hypfs.c
> @@ -25,6 +25,10 @@ static struct hypfs_entry hypfs_root_entry = {
> .dir = &hypfs_root,
> };
>
> +static struct hypfs_dir hypfs_buildinfo = {
> +.list = LIST_HEAD_INIT(hypfs_buildinfo.list),
> +};
> +
> static int hypfs_add_entry(struct hypfs_dir *parent, struct hypfs_entry *new)
> {
> int ret = -ENOENT;
> @@ -316,3 +320,16 @@ long do_hypfs_op(unsigned int cmd,
>
> return ret;
> }
> +
> +static int __init hypfs_init(void)
> +{
> +int ret;
> +
> +ret = hypfs_new_dir(&hypfs_root, "buildinfo", &hypfs_buildinfo);
> +BUG_ON(ret);
> +ret = hypfs_new_entry_string(&hypfs_buildinfo, "config",
> xen_config_data);
> +BUG_ON(ret);
> +
> +return 0;
> +}
> +__initcall(hypfs_init);
Hmm, do you really want to centralize population of the file system
here, rather than having the individual components take care of it?
> --- a/xen/tools/Makefile
> +++ b/xen/tools/Makefile
> @@ -1,13 +1,18 @@
>
> include $(XEN_ROOT)/Config.mk
>
> +PROGS = symbols bin2c
> +
> .PHONY: default
> default:
> - $(MAKE) symbols
> + $(MAKE) $(PROGS)
Could I ask you to take the opportunity and do away with the
unnecessary (as it seems to me) make recursion? $(PROGS) could
easily become a dependency of "default" afaict.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
