Re: [PATCH 09/12] xen/hypfs: add support for id-based dynamic directories

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

On 17.11.20 14:33, Jan Beulich wrote:
> On 26.10.2020 10:13, Juergen Gross wrote:
> > --- a/xen/common/hypfs.c
> > +++ b/xen/common/hypfs.c
> > @@ -257,6 +257,82 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry)
> >       return entry->size;
> >   }
> >   
> > +int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template,
> > +                               unsigned int id, bool is_last,
> > +                               XEN_GUEST_HANDLE_PARAM(void) *uaddr)
> > +{
> > +    struct xen_hypfs_dirlistentry direntry;
> > +    char name[12];
> Perhaps better tie this literal 12 to the one used for declaring
> struct hypfs_dyndir_id's name[] field, such that an eventual
> change will need making in exactly one place?
Yes.

> > +    unsigned int e_namelen, e_len;
> > +
> > +    e_namelen = snprintf(name, sizeof(name), "%u", id);
> > +    e_len = HYPFS_DIRENTRY_SIZE(e_namelen);
> > +    direntry.e.pad = 0;
> > +    direntry.e.type = template->e.type;
> > +    direntry.e.encoding = template->e.encoding;
> > +    direntry.e.content_len = template->e.funcs->getsize(&template->e);
> > +    direntry.e.max_write_len = template->e.max_size;
> > +    direntry.off_next = is_last ? 0 : e_len;
> > +    if ( copy_to_guest(*uaddr, &direntry, 1) )
> > +        return -EFAULT;
> > +    if ( copy_to_guest_offset(*uaddr, HYPFS_DIRENTRY_NAME_OFF, name,
> > +                              e_namelen + 1) )
> > +        return -EFAULT;
> > +
> > +    guest_handle_add_offset(*uaddr, e_len);
> > +
> > +    return 0;
> > +}
> > +
> > +static struct hypfs_entry *hypfs_dyndir_findentry(struct hypfs_entry_dir *dir,
> > +                                                  const char *name,
> > +                                                  unsigned int name_len)
> > +{
> > +    struct hypfs_dyndir_id *data;
> const? (also in read_dyndir below)
Okay.

> > +    data = hypfs_get_dyndata();
> > +    if ( !data )
> > +        return ERR_PTR(-ENOENT);
> > +
> > +    /* Use template with original findentry function. */
> > +    return data->template->e.funcs->findentry(data->template, name, name_len);
> Why does this pass the address of the template? If it truly is
> (just) a template, then its dirlist ought to be empty at all
> times? If otoh the "template" indeed gets used as a node in the
> tree then perhaps it wants naming differently? "Stem" would come
> to mind, but likely there are better alternatives. I've also
> considered the German "Statthalter", but its English translations
> don't seem reasonable to me here. And "placeholder" has kind of a
> negative touch. (Also in this case some of my "const?" remarks
> may be irrelevant.)
It is basically a template tree.

In the current use case (cpupool/<id>/sched-gran) the template is
<id> with the leaf "sched-gran" which is the template for the per
cpupool incarnation.

If you like it better I can use stem.

> Further this and ...
>
> > +static int hypfs_read_dyndir(const struct hypfs_entry *entry,
> > +                             XEN_GUEST_HANDLE_PARAM(void) uaddr)
> > +{
> > +    struct hypfs_dyndir_id *data;
> > +
> > +    data = hypfs_get_dyndata();
> > +    if ( !data )
> > +        return -ENOENT;
> > +
> > +    /* Use template with original read function. */
> > +    return data->template->e.funcs->read(&data->template->e, uaddr);
> ... this using the template's funcs is somewhat unexpected, but
> with the functions acting as the entry's .findentry() / .read()
> hooks is obviously the right thing (and if the template is more
> that what the word says, the consideration may become
> inapplicable anyway). The implication is that the hooks
> themselves can't be replaced, if need be down the road.
Correct. In case this is needed the related node must be really
completely dynamical instead.

> > +struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template,
> > +                                              unsigned int id)
> > +{
> > +    struct hypfs_dyndir_id *data;
> > +
> > +    data = hypfs_alloc_dyndata(sizeof(*data), alignof(*data));
> > +    if ( !data )
> > +        return ERR_PTR(-ENOMEM);
> > +
> > +    data->template = template;
> > +    data->id = id;
> I can't seem to spot any consumer of this field: Is it really
> needed?
Yes. It will be used by the specific read/write functions, e.g.
cpupool_gran_read().

> > --- a/xen/include/xen/hypfs.h
> > +++ b/xen/include/xen/hypfs.h
> > @@ -50,6 +50,15 @@ struct hypfs_entry_dir {
> >       struct list_head dirlist;
> >   };
> >   
> > +struct hypfs_dyndir_id {
> > +    struct hypfs_entry_dir dir;       /* Modified copy of template. */
> > +    struct hypfs_funcs funcs;         /* Dynamic functions. */
> > +    struct hypfs_entry_dir *template; /* Template used. */
> const?
Yes.

> > @@ -150,6 +159,11 @@ struct hypfs_entry *hypfs_dir_findentry(struct 
> > hypfs_entry_dir *dir,
> >                                           unsigned int name_len);
> >   void *hypfs_alloc_dyndata(unsigned long size, unsigned long align);
> >   void *hypfs_get_dyndata(void);
> > +int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template,
> const?
Yes.

> > +                               unsigned int id, bool is_last,
> > +                               XEN_GUEST_HANDLE_PARAM(void) *uaddr);
> > +struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template,
> const?
Yes.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature