[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 14/17] xen/hypfs: add support for id-based dynamic directories



On 04.12.2020 14:08, Jürgen Groß wrote:
> 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.

IOW just an optimization, as I was assuming. Whether you macroize the
name generation I'd like to leave up to you. But you could please add
comments on both sides as to parts which need to remain in sync?

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.