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

[Jürgen Groß <jgross@xxxxxxxx>]

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