On 04.12.20 10:16, Jan Beulich wrote:
> On 04.12.2020 09:52, Jürgen Groß wrote:
>> On 03.12.20 16:44, Jan Beulich wrote:
>>> On 01.12.2020 09:21, Juergen Gross wrote:
>>>> --- a/xen/common/hypfs.c
>>>> +++ b/xen/common/hypfs.c
>>>> @@ -355,6 +355,81 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry)
>>>>        return entry->size;
>>>>    }
>>>>    
>>>> +int hypfs_read_dyndir_id_entry(const struct hypfs_entry_dir *template,
>>>> +                               unsigned int id, bool is_last,
>>>> +                               XEN_GUEST_HANDLE_PARAM(void) *uaddr)
>>>> +{
>>>> +    struct xen_hypfs_dirlistentry direntry;
>>>> +    char name[HYPFS_DYNDIR_ID_NAMELEN];
>>>> +    unsigned int e_namelen, e_len;
>>>> +
>>>> +    e_namelen = snprintf(name, sizeof(name), template->e.name, id);
>>>> +    e_len = 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, 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(
>>>> +    const struct hypfs_entry_dir *dir, const char *name, unsigned int name_len)
>>>> +{
>>>> +    const struct hypfs_dyndir_id *data;
>>>> +
>>>> +    data = hypfs_get_dyndata();
>>>> +
>>>> +    /* Use template with original findentry function. */
>>>> +    return data->template->e.funcs->findentry(data->template, name, name_len);
>>>> +}
>>>> +
>>>> +static int hypfs_read_dyndir(const struct hypfs_entry *entry,
>>>> +                             XEN_GUEST_HANDLE_PARAM(void) uaddr)
>>>> +{
>>>> +    const struct hypfs_dyndir_id *data;
>>>> +
>>>> +    data = hypfs_get_dyndata();
>>>> +
>>>> +    /* Use template with original read function. */
>>>> +    return data->template->e.funcs->read(&data->template->e, uaddr);
>>>> +}
>>>> +
>>>> +struct hypfs_entry *hypfs_gen_dyndir_entry_id(
>>>> +    const struct hypfs_entry_dir *template, unsigned int id)
>>>> +{
>>>> +    struct hypfs_dyndir_id *data;
>>>> +
>>>> +    data = hypfs_get_dyndata();
>>>> +
>>>> +    data->template = template;
>>>> +    data->id = id;
>>>> +    snprintf(data->name, sizeof(data->name), template->e.name, id);
>>>> +    data->dir = *template;
>>>> +    data->dir.e.name = data->name;
>>>
>>> I'm somewhat puzzled, if not confused, by the apparent redundancy
>>> of this name generation with that in hypfs_read_dyndir_id_entry().
>>> Wasn't the idea to be able to use generic functions on these
>>> generated entries?
>>
>> I can add a macro replacing the double snprintf().
> 
> That wasn't my point. I'm concerned of there being two name generation
> sites in the first place. Is this perhaps simply some form of
> optimization, avoiding hypfs_read_dyndir_id_entry() to call
> hypfs_gen_dyndir_entry_id() (but risking the two going out of sync)?

Be aware that hypfs_read_dyndir_id_entry() is generating a struct
xen_hypfs_dirlistentry, which is different from the internal
representation of the data produced by hypfs_gen_dyndir_entry_id().

So the main common part is the name generation. What else would you
want apart from making it common via e.g. a macro? Letting
hypfs_read_dyndir_id_entry() call hypfs_gen_dyndir_entry_id() would
just be a more general approach with all the data but the generated
name of hypfs_gen_dyndir_entry_id() dropped or just copied around
a second time.


Juergen