Re: [Xen-devel] [PATCH v2 5/6] xen: add /buildinfo/config entry to hypervisor filesystem

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel