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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> +    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)

> +    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.)

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.

> +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?

> --- 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?

> @@ -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?

> +                               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?

Jan