Re: [PATCH v7 04/12] xen: add basic hypervisor filesystem support

From: Jan Beulich <jbeulich@suse.com>
To: Juergen Gross <jgross@suse.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>, "Wei Liu" <wl@xen.org>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	xen-devel@lists.xenproject.org,
	"Daniel De Graaf" <dgdegra@tycho.nsa.gov>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v7 04/12] xen: add basic hypervisor filesystem support
Date: Fri, 3 Apr 2020 16:23:10 +0200	[thread overview]
Message-ID: <7dda5c2c-cb81-2cfa-2cf4-4440b49d401a@suse.com> (raw)
In-Reply-To: <20200402154616.16927-5-jgross@suse.com>

On 02.04.2020 17:46, Juergen Gross wrote:
> Add the infrastructure for the hypervisor filesystem.
> 
> This includes the hypercall interface and the base functions for
> entry creation, deletion and modification.
> 
> In order not to have to repeat the same pattern multiple times in case
> adding a new node should BUG_ON() failure, the helpers for adding a
> node (hypfs_add_dir() and hypfs_add_leaf()) get a nofault parameter
> causing the BUG() in case of a failure.
> 
> When supporting writable leafs the entry's write pointer will need to
> be set to the function performing the write to the variable holding the
> content. In case there are no special constraints this will be
> hypfs_write_bool() for type XEN_HYPFS_TYPE_BOOL and hypfs_write_leaf()
> for the other entry types.

Seeing your HYPFS_*_INIT_WRITABLE() macros I find this a pretty
strange requirement. Why can't the macros set the write hook right
away?

> +int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
> +                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> +{
> +    char *buf;
> +    int ret;
> +
> +    if ( leaf->e.type != XEN_HYPFS_TYPE_STRING &&
> +         leaf->e.type != XEN_HYPFS_TYPE_BLOB && ulen != leaf->e.size )
> +        return -EDOM;
> +
> +    buf = xmalloc_array(char, ulen);
> +    if ( !buf )
> +        return -ENOMEM;
> +
> +    ret = -EFAULT;
> +    if ( copy_from_guest(buf, uaddr, ulen) )
> +        goto out;
> +
> +    ret = -EINVAL;
> +    if ( leaf->e.type == XEN_HYPFS_TYPE_STRING &&
> +         memchr(buf, 0, ulen) != (buf + ulen - 1) )
> +        goto out;
> +
> +    ret = 0;
> +    memcpy(leaf->write_ptr, buf, ulen);
> +    leaf->e.size = ulen;
> +
> + out:
> +    xfree(buf);
> +    return ret;
> +}
> +
> +int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
> +                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> +{
> +    bool buf;
> +
> +    ASSERT(leaf->e.type == XEN_HYPFS_TYPE_BOOL && leaf->e.size == sizeof(bool));
> +
> +    if ( ulen != leaf->e.max_size )

Why max_size here when the ASSERT() checks size?

> +static int hypfs_write(struct hypfs_entry *entry,
> +                       XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
> +{
> +    struct hypfs_entry_leaf *l;
> +
> +    if ( !entry->write )
> +        return -EACCES;
> +
> +    if ( ulen > entry->max_size )
> +        return -ENOSPC;

max_size being zero for non-writable entries, perhaps use -EACCES
also for this special case? Together with the other comment above,
maybe the ->write check wants replacing this way?

Jan