|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH 2/8] lib/uk9p: Add 9P transport registration
On Sat, Jun 29, 2019 at 10:10 AM Simon Kuenzer <simon.kuenzer@xxxxxxxxx> wrote:
>
> On 19.06.19 09:03, Cristian Banu wrote:
> > This patch introduces the 9P transport registration and lookup API.
>
> Maybe you could also explain the concept of having multiple of them and
> describe what a default transport is and what it is for. In case you
> keep it.
Fixed in the next patch version.
>
> >
> > Signed-off-by: Cristian Banu <cristb@xxxxxxxxx>
> > ---
> > lib/uk9p/9pdev_trans.c | 82 +++++++++++++++++++++++++++++++
> > lib/uk9p/Makefile.uk | 2 +
> > lib/uk9p/exportsyms.uk | 3 ++
> > lib/uk9p/include/uk/9pdev_trans.h | 101
> > ++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 188 insertions(+)
> > create mode 100644 lib/uk9p/9pdev_trans.c
> > create mode 100644 lib/uk9p/exportsyms.uk
> > create mode 100644 lib/uk9p/include/uk/9pdev_trans.h
> >
> > diff --git a/lib/uk9p/9pdev_trans.c b/lib/uk9p/9pdev_trans.c
> > new file mode 100644
> > index 000000000000..60b8a230cbf3
> > --- /dev/null
> > +++ b/lib/uk9p/9pdev_trans.c
> > @@ -0,0 +1,82 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause */
> > +/*
> > + * Authors: Cristian Banu <cristb@xxxxxxxxx>
> > + *
> > + * Copyright (c) 2019, University Politehnica of Bucharest. All rights
> > reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + *
> > + * 1. Redistributions of source code must retain the above copyright
> > + * notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + * notice, this list of conditions and the following disclaimer in the
> > + * documentation and/or other materials provided with the distribution.
> > + * 3. Neither the name of the copyright holder nor the names of its
> > + * contributors may be used to endorse or promote products derived from
> > + * this software without specific prior written permission.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
> > IS"
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> > THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> > PURPOSE
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS
> > BE
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
> > THE
> > + * POSSIBILITY OF SUCH DAMAGE.
> > + *
> > + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> > + */
> > +
> > +#include <errno.h>
> > +#include <string.h>
> > +#include <uk/config.h>
> > +#include <uk/list.h>
> > +#include <uk/assert.h>
> > +#include <uk/9pdev_trans.h>
> > +
> > +static UK_LIST_HEAD(uk_9pdev_trans_list);
> > +
> > +int uk_9pdev_trans_register(struct uk_9pdev_trans *trans)
> > +{
> > + UK_ASSERT(trans);
> > + UK_ASSERT(trans->name);
> > + UK_ASSERT(trans->a);
> > +
> > + uk_list_add_tail(&trans->_list, &uk_9pdev_trans_list);
> > + uk_pr_info("Registered transport %s\n", trans->name);
> > +
> > + return 0;
> > +}
> > +
> > +int uk_9pdev_trans_by_name(const char *name, struct uk_9pdev_trans **trans)
> > +{
> > + struct uk_9pdev_trans *t;
> > +
> > + uk_list_for_each_entry(t, &uk_9pdev_trans_list, _list) {
> > + if (!strcmp(t->name, name)) {
> > + *trans = t;
> > + return 0;
> > + }
> > + }
> > +
> > + return -ENODEV;
> > +}
> > +
> > +int uk_9pdev_trans_default(struct uk_9pdev_trans **trans)
> > +{
> > + struct uk_9pdev_trans *t;
> > +
> > + uk_list_for_each_entry(t, &uk_9pdev_trans_list, _list) {
> > + if (t->is_default) {
> > + *trans = t;
> > + return 0;
> > + }
> > + }
>
> Maybe you want to store internally to the library an extra pointer that
> points to the current default transport. Then you would not need this
> boolean scheme and to traverse the list.
Good idea, fixed in the next patch version.
>
> > +
> > + return -ENODEV;
> > +}
> > diff --git a/lib/uk9p/Makefile.uk b/lib/uk9p/Makefile.uk
> > index fa754440598c..b1071a0e7d3c 100644
> > --- a/lib/uk9p/Makefile.uk
> > +++ b/lib/uk9p/Makefile.uk
> > @@ -2,3 +2,5 @@ $(eval $(call addlib_s,libuk9p,$(CONFIG_LIBUK9P)))
> >
> > CINCLUDES-$(CONFIG_LIBUK9P) += -I$(LIBUK9P_BASE)/include
> > CXXINCLUDES-$(CONFIG_LIBUK9P) += -I$(LIBUK9P_BASE)/include
> > +
> > +LIBUK9P_SRCS-y += $(LIBUK9P_BASE)/9pdev_trans.c
> > diff --git a/lib/uk9p/exportsyms.uk b/lib/uk9p/exportsyms.uk
> > new file mode 100644
> > index 000000000000..45f487da2fff
> > --- /dev/null
> > +++ b/lib/uk9p/exportsyms.uk
> > @@ -0,0 +1,3 @@
> > +uk_9pdev_trans_register
> > +uk_9pdev_trans_by_name
> > +uk_9pdev_trans_default
> > diff --git a/lib/uk9p/include/uk/9pdev_trans.h
> > b/lib/uk9p/include/uk/9pdev_trans.h
> > new file mode 100644
> > index 000000000000..a0c6d252361c
> > --- /dev/null
> > +++ b/lib/uk9p/include/uk/9pdev_trans.h
> > @@ -0,0 +1,101 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause */
> > +/*
> > + * Authors: Cristian Banu <cristb@xxxxxxxxx>
> > + *
> > + * Copyright (c) 2019, University Politehnica of Bucharest. All rights
> > reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + *
> > + * 1. Redistributions of source code must retain the above copyright
> > + * notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + * notice, this list of conditions and the following disclaimer in the
> > + * documentation and/or other materials provided with the distribution.
> > + * 3. Neither the name of the copyright holder nor the names of its
> > + * contributors may be used to endorse or promote products derived from
> > + * this software without specific prior written permission.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
> > IS"
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> > THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> > PURPOSE
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS
> > BE
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
> > THE
> > + * POSSIBILITY OF SUCH DAMAGE.
> > + *
> > + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> > + */
> > +
> > +#ifndef __UK_9PDEV_TRANS__
> > +#define __UK_9PDEV_TRANS__
> > +
> > +#include <stdbool.h>
> > +#include <uk/config.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/** A structure used to describe a transport. */
> > +struct uk_9pdev_trans {
> > + /*
> > + * Transport name (e.g. "virtio", "xen"). This field is reserved for
> > + * future use, when multiple transport options are available on the
> > + * same platform.
> > + */
>
> What is a common case where you have multiple transports? Is it like
> multiple shared mount points?
> I am wondering if we need the abstraction of multiple transports.
There are 9pfs transports for RDMA and TCP, which do not necessarily
depend on the platform, as is the case right now. Will add the explanation
in the next patch version.
>
> > + const char *name;
> > + /* Can the transport be used as a default one? */
> > + bool is_default;
> > + /* Allocator used for devices which use this transport layer. */
> > + struct uk_alloc *a;
> > + /* @internal Entry in the list of available transports. */
> > + struct uk_list_head _list;
> > +};
> > +
> > +/**
> > + * Adds a transport to the available transports list for Unikraft 9P
> > Devices.
> > + * This should be called once per driver (once for virtio, once for xen,
> > etc.).
> > + *
> > + * @param trans
> > + * Pointer to the transport structure.
> > + * @return
> > + * - (0): Successful.
> > + * - (< 0): Failed to register the transport layer.
> > + */
> > +int uk_9pdev_trans_register(struct uk_9pdev_trans *trans);
> > +
> > +/**
> > + * Looks up a transport layer by its name.
> > + *
> > + * @param name
> > + * The transport layer name.
> > + * @param trans
> > + * Where to store the pointer to the found transport structure.
> > + * @return
> > + * - (-ENOENT): No transport with the given name.
> > + * - (0): Successful.
> > + */
> > +int uk_9pdev_trans_by_name(const char *name, struct uk_9pdev_trans
> > **trans);
> > +
> > +/**
> > + * Gets the default transport layer.
> > + *
> > + * @param trans
> > + * Where to store the pointer to the found transport structure.
> > + * @return
> > + * - (-ENOENT): No default transport found.
>
> I saw that your function returns -ENODEV. This description is
> mismatching. ;-)
Oops. :)
>
> > + * - (0): Successful.
> > + */
> > +int uk_9pdev_trans_default(struct uk_9pdev_trans **trans);
>
> Do you ever expect to return a different error code? What about
> returning the pointer (instead of going through an argument) and NULL in
> error case? We had a similar API decision in ukalloc
> (uk_alloc_get_default()) and uksched, so it would fit here.
>
Fixed.
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* __UK_9PDEV_TRANS__ */
> >
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |