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

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

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.

> 
>>> Anyway, this and ...
>>>
>>>> + out:
>>>> +    xfree(buf);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
>>>> +                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
>>>> +{
>>>
>>> ... this function aren't very helpful to review without there
>>> being a caller. Could these be introduced at the time a first
>>> caller appears?
>>
>> Of course. Question is where to stop. I wanted to have the basic hypfs
>> support in one patch. Are you fine with just those two functions being
>> moved to the runtime parameter patch?
> 
> Let me put it this way: For anything the patch adds but there's
> no usage at all (i.e. not even in a macro, where at least the
> usage intentions get sufficiently clarified), the description
> should cover for this lack of sufficient context. Therefore I'd
> also be fine with the two functions remaining here, as long as
> readers (reviewers) can understand the intentions. It might
> still be _easier_ for everyone to have them in a later patch.
> But then the same still goes for other functions that have no
> users here. (The helper macros HYPFS_*_INIT(), otoh, are clear
> enough the way they are imo, and hence are fine to remain, plus
> they serve as usage explanation for hypfs_read_{leaf,dir}(),
> which as it looks would otherwise too be orphaned.)

Fair enough.

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

> 
>>>> --- /dev/null
>>>> +++ b/xen/include/public/hypfs.h
>>>> @@ -0,0 +1,124 @@
>>>> +/******************************************************************************
>>>> + * Xen Hypervisor Filesystem
>>>> + *
>>>> + * Copyright (c) 2019, SUSE Software Solutions Germany GmbH
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>>> + * of this software and associated documentation files (the "Software"), to
>>>> + * deal in the Software without restriction, including without limitation the
>>>> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
>>>> + * sell copies of the Software, and to permit persons to whom the Software is
>>>> + * furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
>>>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>>>> + * DEALINGS IN THE SOFTWARE.
>>>> + *
>>>> + */
>>>> +
>>>> +#ifndef __XEN_PUBLIC_HYPFS_H__
>>>> +#define __XEN_PUBLIC_HYPFS_H__
>>>> +
>>>> +#include "xen.h"
>>>> +
>>>> +/*
>>>> + * Definitions for the __HYPERVISOR_hypfs_op hypercall.
>>>> + */
>>>> +
>>>> +/* Highest version number of the hypfs interface currently defined. */
>>>> +#define XEN_HYPFS_VERSION      1
>>>
>>> For this and the accompanying XEN_HYPFS_OP_get_version, at least
>>> the doc added by patch 3 could actually do with mentioning the
>>> intentions you have with this.
>>
>> Okay.
>>
>>>
>>>> +/* Maximum length of a path in the filesystem. */
>>>> +#define XEN_HYPFS_MAX_PATHLEN 1024
>>>> +
>>>> +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.

> 
>>>> +#define HYPFS_DIR_INIT(var, nam)                \
>>>> +    struct hypfs_entry_dir var = {              \
>>>> +        .e.type = XEN_HYPFS_TYPE_DIR,           \
>>>> +        .e.encoding = XEN_HYPFS_ENC_PLAIN,      \
>>>> +        .e.name = nam,                          \
>>>> +        .e.size = 0,                            \
>>>> +        .e.list = LIST_HEAD_INIT(var.e.list),   \
>>>> +        .e.read = hypfs_read_dir,               \
>>>> +        .dirlist = LIST_HEAD_INIT(var.dirlist), \
>>>> +    }
>>>> +
>>>> +/* Content and size need to be set via hypfs_string_set(). */
>>>> +#define HYPFS_STRING_INIT(var, nam)             \
>>>> +    struct hypfs_entry_leaf var = {             \
>>>> +        .e.type = XEN_HYPFS_TYPE_STRING,        \
>>>> +        .e.encoding = XEN_HYPFS_ENC_PLAIN,      \
>>>> +        .e.name = nam,                          \
>>>> +        .e.read = hypfs_read_leaf,              \
>>>> +    }
>>>> +
>>>> +static inline void hypfs_string_set(struct hypfs_entry_leaf *leaf,
>>>> +                                    const char *str)
>>>> +{
>>>> +    leaf->content = str;
>>>> +    leaf->e.size = strlen(str) + 1;
>>>> +}
>>>> +
>>>> +#define HYPFS_UINT_INIT(var, nam, uint)         \
>>>> +    struct hypfs_entry_leaf var = {             \
>>>> +        .e.type = XEN_HYPFS_TYPE_UINT,          \
>>>> +        .e.encoding = XEN_HYPFS_ENC_PLAIN,      \
>>>> +        .e.name = nam,                          \
>>>> +        .e.size = sizeof(uint),                 \
>>>> +        .e.read = hypfs_read_leaf,              \
>>>> +        .content = &uint,                       \
>>>> +    }
>>>
>>> So you've got such helper macros for dir, string, and uint. Why
>>> not e.g. int and bool?
>>
>> There are no users in my series yet.
> 
> Hmm, as per above strictly speaking it is this patch which
> matters, not the entire series. Hence I think you either want
> to supply a full set of helper macros here, or introduce the
> ones actually needed in the patches where they get first used.

Okay, I'll go with the full set of helpers.

> 
>>>> +struct hypfs_entry *hypfs_get_entry(const char *path);
>>>
>>> Does the only caller really need a non-const return type? Even
>>> hypfs_write() doesn't look to modify what its leaf parameter
>>> points at.
>>
>> This might change when support for dynamically allocated strings or
>> blobs is added (I have no plans to do this right now, but its easy
>> to think about the need).
>>
>>>
>>> And is there indeed an expectation for this to be used from
>>> outside of the source file it's defined in?
>>
>> Yes. As soon as support for e.g. per-domain or per-cpupool nodes is
>> added this will be needed.
> 
> Until then, make the function both static and return ptr-to-const?
> Such that when this changes, the need for either can actually be
> seen?

Fine with me.


Juergen

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