Re: [Xen-devel] [PATCH v3 4/9] xen: add basic hypervisor filesystem support

["Jürgen Groß" <jgross@suse.com>]

On 04.02.20 10:58, Jan Beulich wrote:
> On 04.02.2020 10:21, Jürgen Groß wrote:
>> On 04.02.20 09:48, Jan Beulich wrote:
>>> On 04.02.2020 07:43, Jürgen Groß wrote:
>>>> On 03.02.20 16:07, Jan Beulich wrote:
>>>>> On 21.01.2020 09:43, Juergen Gross wrote:
>>>>>> +static int hypfs_read(const struct hypfs_entry *entry,
>>>>>> +                      XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
>>>>>> +{
>>>>>> +    struct xen_hypfs_direntry e;
>>>>>> +    long ret = -EINVAL;
>>>>>> +
>>>>>> +    if ( ulen < sizeof(e) )
>>>>>> +        goto out;
>>>>>> +
>>>>>> +    e.flags = entry->write ? XEN_HYPFS_WRITEABLE : 0;
>>>>>> +    e.type = entry->type;
>>>>>> +    e.encoding = entry->encoding;
>>>>>> +    e.content_len = entry->size;
>>>>>> +
>>>>>> +    ret = -EFAULT;
>>>>>> +    if ( copy_to_guest(uaddr, &e, 1) )
>>>>>> +        goto out;
>>>>>> +
>>>>>> +    ret = 0;
>>>>>> +    if ( ulen < entry->size + sizeof(e) )
>>>>>> +        goto out;
>>>>>
>>>>> So you return "success" even if the operation didn't complete
>>>>> successfully. This isn't very nice, plus ...
>>>>
>>>> The direntry contains the needed size. The caller should know the
>>>> size he passed to Xen.
>>>>
>>>>>
>>>>>> +    guest_handle_add_offset(uaddr, sizeof(e));
>>>>>> +
>>>>>> +    ret = entry->read(entry, uaddr);
>>>>>
>>>>> ... how is the caller to know whether direntry was at least
>>>>> copied if this then fails?
>>>>
>>>> Is this really important? Normally -EFAULT should just not happen. In
>>>> case it does I don't think the caller can make real use of the direntry.
>>>
>>> "Important" has various possible meanings. The success/failure
>>> indication to the caller should at least be rational. "If the
>>> data buffer was not large enough for all the data no entry data
>>> is returned, but the direntry will contain the needed size for
>>> the returned data" is fine to be stated in the public header,
>>> but I think this wants to be -ENOBUFS then, not 0 (success).
>>
>> I would be fine with this, but this contradicts your previous demand
>> not to enumerate the possible failure cases, which would be essential
>> for this case.
> 
> Slightly re-writing the part of the comment I did quote would be
> all that's needed afaict: "If the data buffer was not large enough
> for all the data -ENOBUFS and no entry data is returned, but the
> direntry will contain the needed size for the returned data."

Okay. Fine with me.

> 
>>>>>> +    union {
>>>>>> +        char buf[8];
>>>>>> +        uint8_t u8;
>>>>>> +        uint16_t u16;
>>>>>> +        uint32_t u32;
>>>>>> +        uint64_t u64;
>>>>>> +    } u;
>>>>>> +
>>>>>> +    ASSERT(leaf->e.type == XEN_HYPFS_TYPE_UINT && leaf->e.size <= 8);
>>>>>> +
>>>>>> +    if ( ulen != leaf->e.size )
>>>>>> +        return -EDOM;
>>>>>
>>>>> Is this restriction really necessary? Setting e.g. a 4-byte
>>>>> field from 1-byte input is no problem at all. This being for
>>>>> booleans I anyway wonder why input might be helpful to have
>>>>> larger than a single byte. But maybe all of this is again a
>>>>> result of not seeing what a user of the function would look
>>>>> like.
>>>>
>>>> I wanted to have as little functionality as possible in the hypervisor.
>>>> It is no problem for the library to pass a properly sized buffer.
>>>>
>>>> Allowing larger variables for booleans is just a consequence of the
>>>> hypervisor parameters allowing that.
>>>
>>> But the caller shouldn't be concerned of the hypervisor
>>> implementation detail of what the chose width is. Over time we
>>> e.g. convert int (along with bool_t) to bool when it's used in
>>> a boolean way. This should not result in the caller needing to
>>> change, despite the width change of the variable.
>>
>> This is basically a consequence of now passing binary values to and from
>> the hypervisor.
>>
>> The normal way of handling this (as can be seen in libxenhypfs) is to
>> query the hypervisor for the size of the value (no matter whether its
>> int, uint or bool) and then to do the conversion between ASCII and the
>> binary value at the caller's side.
> 
> I can see why this is needed for e.g. integer values, but I don't
> see the need for booleans.

At some level the conversion needs to be done. I'd rather do it for all
types at the same level.

> 
>>>>>> +struct xen_hypfs_direntry {
>>>>>> +    uint16_t flags;
>>>>>> +#define XEN_HYPFS_WRITEABLE    0x0001
>>>>>> +    uint8_t type;
>>>>>> +#define XEN_HYPFS_TYPE_DIR     0x0000
>>>>>> +#define XEN_HYPFS_TYPE_BLOB    0x0001
>>>>>> +#define XEN_HYPFS_TYPE_STRING  0x0002
>>>>>> +#define XEN_HYPFS_TYPE_UINT    0x0003
>>>>>> +#define XEN_HYPFS_TYPE_INT     0x0004
>>>>>> +#define XEN_HYPFS_TYPE_BOOL    0x0005
>>>>>> +    uint8_t encoding;
>>>>>> +#define XEN_HYPFS_ENC_PLAIN    0x0000
>>>>>> +#define XEN_HYPFS_ENC_GZIP     0x0001
>>>>>
>>>>> Meaning I can e.g. have a gzip-ed string or bool (or even dir)?
>>>>> If this is just for "blob", why have separate fields instead of
>>>>> e.g. BLOB_RAW and BLOB_GZIP or some such?
>>>>
>>>> gzip-ed string or blob are the primary targets.
>>>>
>>>> Maybe we want to have other encoding s later (Andrew asked for that
>>>> possibility when I posted the patch for retrieving the .config file
>>>> contents early last year).
>>>
>>> To me it would seem preferable if the contents of a blob
>>> identified itself as to its format. But since this leaves
>>> room for ambiguities I accept that the format needs
>>> specifying. However, to me a gzip-ed string is as good as a
>>> gzip-ed blob, and hence I still think sub-dividing "blob" is
>>> the way to go, with no separate "encoding". Otherwise at the
>>> very least a comment here would need adding to clarify what
>>> combinations are valid / to be expected by callers.
>>
>> libxenhypfs is able to handle all possible combinations. I just don't
>> think some of the combinations are making sense (gzip-ing a binary
>> value of 4 bytes e.g. is nonsense).
>>
>> OTOH in case we'll add large arrays of longs in the future it might be
>> beneficial to compress them in some way. So I'd like to keep type and
>> encoding as separate information.
> 
> Okay, I'm not entirely opposed. But I'd be curious if anyone
> else has an opinion here.

I think content type and transport encoding should not be mixed up. They
are orthogonal to each other and so they should be handled.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel