|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH v2 05/10] plat/xen: Add API for Xenstore messages
Costin Lupu <costin.lup@xxxxxxxxx> writes:
> On 09/04/2018 07:25 PM, Yuri Volchkov wrote:
>> Costin Lupu <costin.lup@xxxxxxxxx> writes:
>>
>>> Hi Yuri,
>>>
>>> Please see my comments inline.
>>>
>>> On 08/31/2018 06:27 PM, Yuri Volchkov wrote:
>>>> Hey Costin,
>>>>
>>>> see my comments inline.
>>>>
>>>> BR, Yuri.
>>>>
>>>> Costin Lupu <costin.lupu@xxxxxxxxx> writes:
>>>>
>>>>> Add the API needed for sending Xenstore messages. These functions
>>>>> are used by any client communicating with the Xenstore daemon.
>>>>>
>>>>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>>>>> ---
>>>>> plat/xen/Makefile.uk | 1 +
>>>>> plat/xen/include/xenbus/xs.h | 270 ++++++++++++++++++
>>>>> plat/xen/xenbus/xs.c | 650
>>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 921 insertions(+)
>>>>> create mode 100644 plat/xen/include/xenbus/xs.h
>>>>> create mode 100644 plat/xen/xenbus/xs.c
>>>>>
>>>>> diff --git a/plat/xen/Makefile.uk b/plat/xen/Makefile.uk
>>>>> index 2703a54..63cc42b 100644
>>>>> --- a/plat/xen/Makefile.uk
>>>>> +++ b/plat/xen/Makefile.uk
>>>>> @@ -80,4 +80,5 @@ LIBXENBUS_CFLAGS-y += $(LIBXENPLAT_CFLAGS-y)
>>>>> LIBXENBUS_CINCLUDES-y += $(LIBXENPLAT_CINCLUDES-y)
>>>>> LIBXENBUS_SRCS-y += $(LIBXENPLAT_BASE)/xenbus/xenbus.c
>>>>> LIBXENBUS_SRCS-y += $(LIBXENPLAT_BASE)/xenbus/xs_comms.c
>>>>> +LIBXENBUS_SRCS-y += $(LIBXENPLAT_BASE)/xenbus/xs.c
>>>>> endif
>>>>> diff --git a/plat/xen/include/xenbus/xs.h b/plat/xen/include/xenbus/xs.h
>>>>> new file mode 100644
>>>>> index 0000000..6ae761d
>>>>> --- /dev/null
>>>>> +++ b/plat/xen/include/xenbus/xs.h
>>>>> @@ -0,0 +1,270 @@
>>>>> +/* SPDX-License-Identifier: BSD-3-Clause */
>>>>> +/*
>>>>> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
>>>>> + *
>>>>> + * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. 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.
>>>>> + */
>>>>> +/* Xenstore API */
>>>>> +/*
>>>>> + * TODO The intention for this API is to be used by applications as well.
>>>>> + * Therefore, all data allocated by this API for external use must be
>>>>> free'd
>>>>> + * calling 'free' function (and not uk_xb_free). This is the reason why
>>>>> such
>>>>> + * data is allocated with 'malloc'/'calloc'.
>>>>> + */
>>>>> +
>>>>> +#ifndef __XS_H__
>>>>> +#define __XS_H__
>>>>> +
>>>>> +#include <xenbus/xenbus.h>
>>>>> +
>>>>> +
>>>>> +/*
>>>>> + * Read the value associated with a path.
>>>>> + *
>>>>> + * @param xbt Xenbus transaction id
>>>>> + * @param path Xenstore path
>>>>> + * @param node Xenstore subdirectory
>>>>> + * @return On success, returns a malloc'd copy of the value. On error,
>>>>> returns
>>>>> + * a negative error number which should be checked using PTRISERR.
>>>>> + */
>>>>> +char *xs_read(xenbus_transaction_t xbt, const char *path, const char
>>>>> *node);
>>>>> +
>>>>> +/*
>>>>> + * Associates a value with a path.
>>>>> + *
>>>>> + * @param xbt Xenbus transaction id
>>>>> + * @param path Xenstore path
>>>>> + * @param node Xenstore subdirectory (optional)
>>>>> + * @param value Xenstore value
>>>>> + * @return 0 on success, a negative errno value on error.
>>>>> + */
>>>>> +int xs_write(xenbus_transaction_t xbt, const char *path, const char
>>>>> *node,
>>>>> + const char *value);
>>>>> +
>>>>> +/*
>>>>> + * List the contents of a directory.
>>>>> + *
>>>>> + * @param xbt Xenbus transaction id
>>>>> + * @param path Xenstore directory path
>>>>> + * @param node Xenstore subdirectory (optional)
>>>>> + * @return On success, returns a malloc'd array of pointers to strings.
>>>>> The
>>>>> + * array is NULL terminated. The caller should free only the array. On
>>>>> error,
>>>>> + * returns a negative error number which should be checked using
>>>>> PTRISERR.
>>>>> + * May block.
>>>>> + */
>>>>> +char **xs_ls(xenbus_transaction_t xbt, const char *path);
>>>>> +
>>>>> +/*
>>>>> + * Removes the value associated with a path.
>>>>> + *
>>>>> + * @param xbt Xenbus transaction id
>>>>> + * @param path Xenstore path
>>>>> + * @return 0 on success, a negative errno value on error.
>>>>> + */
>>>>> +int xs_rm(xenbus_transaction_t xbt, const char *path);
>>>>> +
>>>>> +/*
>>>>> + * Xenstore permissions
>>>>> + */
>>>>> +enum xs_perm {
>>>>> + XS_PERM_NONE = 0x0,
>>>>> + XS_PERM_READ = 0x1,
>>>>> + XS_PERM_WRITE = 0x2,
>>>>> + XS_PERM_BOTH = XS_PERM_WRITE | XS_PERM_READ
>>>>> +};
>>>>> +
>>>>> +/*
>>>>> + * Converts a character to corresponding permission value.
>>>>> + *
>>>>> + * @param c Permission character
>>>>> + * @param perm Permission value
>>>>> + * @return 0 on success, a negative errno value on error.
>>>>> + */
>>>>> +int xs_char_to_perm(char c, enum xs_perm *perm);
>>>>> +
>>>>> +/*
>>>>> + * Converts a permission value to corresponding character.
>>>>> + *
>>>>> + * @param perm Permission value
>>>>> + * @param c Permission character
>>>>> + * @return 0 on success, a negative errno value on error.
>>>>> + */
>>>>> +int xs_perm_to_char(enum xs_perm perm, char *c);
>>>>> +
>>>>> +/*
>>>>> + * Extracts domid and permission value out of a permission string.
>>>>> + *
>>>>> + * @param str Permission string
>>>>> + * @param domid Domain ID
>>>>> + * @param perm Permission value
>>>>> + * @return 0 on success, a negative errno value on error.
>>>>> + */
>>>>> +int xs_str_to_perm(const char *str, domid_t *domid, enum xs_perm *perm);
>>>>> +
>>>>> +/*
>>>>> + * Returns a permission string from domid and permission value.
>>>>> + *
>>>>> + * @param domid Domain ID
>>>>> + * @param perm Permission value
>>>>> + * @return On success, returns a malloc'd string. On error, returns a
>>>>> negative
>>>>> + * error number which should be checked using PTRISERR.
>>>>> + */
>>>>> +char *xs_perm_to_str(domid_t domid, enum xs_perm perm);
>>>>> +
>>>>> +/*
>>>>> + * Xenstore ACL
>>>>> + */
>>>>> +struct xs_acl_entry {
>>>>> + domid_t domid;
>>>>> + enum xs_perm perm;
>>>>> +};
>>>>> +
>>>>> +struct xs_acl {
>>>>> + domid_t ownerid;
>>>>> + enum xs_perm others_perm;
>>>>> + int entries_num;
>>>>> + struct xs_acl_entry entries[];
>>>>> +};
>>>>> +
>>>>> +/*
>>>>> + * Returns the ACL for input path.
>>>>> + *
>>>>> + * @param xbt Xenbus transaction id
>>>>> + * @param path Xenstore path
>>>>> + * @return On success, returns a malloc'd ACL. On error, returns a
>>>>> + * negative error number which should be checked using PTRISERR.
>>>>> + */
>>>>> +struct xs_acl *xs_get_acl(xenbus_transaction_t xbt, const char *path);
>>>>> +
>>>>> +/*
>>>>> + * Sets ACL for input path.
>>>>> + *
>>>>> + * @param xbt Xenbus transaction id
>>>>> + * @param path Xenstore path
>>>>> + * @param acl New ACL
>>>>> + * @return 0 on success, a negative errno value on error.
>>>>> + */
>>>>> +int xs_set_acl(xenbus_transaction_t xbt, const char *path, struct xs_acl
>>>>> *acl);
>>>>> +
>>>>> +/*
>>>>> + * Reads permissions for input path and domid.
>>>>> + *
>>>>> + * @param xbt Xenbus transaction id
>>>>> + * @param path Xenstore path
>>>>> + * @param domid Domain ID
>>>>> + * @param perm Permission value
>>>>> + * @return 0 on success, a negative errno value on error.
>>>>> + */
>>>>> +int xs_get_perms(xenbus_transaction_t xbt, const char *path,
>>>>> + domid_t domid, enum xs_perm *perm);
>>>>> +
>>>>> +/*
>>>>> + * Sets permissions for input path and domid.
>>>>> + *
>>>>> + * @param xbt Xenbus transaction id
>>>>> + * @param path Xenstore path
>>>>> + * @param domid Domain ID
>>>>> + * @param perm Permission value
>>>>> + * @return 0 on success, a negative errno value on error.
>>>>> + */
>>>>> +int xs_set_perms(xenbus_transaction_t xbt, const char *path,
>>>>> + domid_t domid, enum xs_perm perm);
>>>>> +
>>>>> +/*
>>>>> + * Start a xenbus transaction. Returns the transaction in xbt on
>>>>> + * success or an error number otherwise.
>>>>> + *
>>>>> + * @param xbt Address for returning the Xenbus transaction id
>>>>> + * @return 0 on success, a negative errno value on error.
>>>>> + */
>>>>> +int xs_transaction_start(xenbus_transaction_t *xbt);
>>>>> +
>>>>> +/*
>>>>> + * End a xenbus transaction. Returns non-zero on failure.
>>>>> + * Parameter abort says whether the transaction should be aborted.
>>>>> + * Returns 1 in *retry iff the transaction should be retried.
>>>>> + *
>>>>> + * @param xbt Xenbus transaction id
>>>>> + * @param abort Non-zero if transaction should be aborted
>>>>> + * @return 0 on success, a negative errno value on error.
>>>>> + */
>>>>> +int xs_transaction_end(xenbus_transaction_t xbt, int abort);
>>>>> +
>>>>> +/*
>>>>> + * Sends a debug message to the Xenstore daemon for writing it in the
>>>>> debug log
>>>>> + *
>>>>> + * @param msg The logged message
>>>>> + * @return 0 on success, a negative errno value on error.
>>>>> + */
>>>>> +int xs_debug_msg(const char *msg);
>>>>> +
>>>>> +/*
>>>>> + * Read path and parse it as an integer.
>>>>> + *
>>>>> + * @param path Xenstore path
>>>>> + * @param value Returned int value
>>>>> + * @return 0 on success, a negative errno value on error.
>>>>> + */
>>>>> +int xs_read_integer(const char *path, int *value);
>>>>> +
>>>>> +/*
>>>>> + * Contraction of sscanf and xs_read(node/path).
>>>>> + *
>>>>> + * @param xbt Xenbus transaction id
>>>>> + * @param dir Xenstore directory
>>>>> + * @param node Xenstore directory entry
>>>>> + * @param fmt Path format string
>>>>> + * @return Just like sscanf, on success returns the number of input items
>>>>> + * successfully matched and assigned. On error returns a negative errno
>>>>> value.
>>>>> + */
>>>>> +int xs_scanf(xenbus_transaction_t xbt, const char *dir, const char *node,
>>>>> + const char *fmt, ...) __scanf(4, 5);
>>>>> +
>>>>> +/*
>>>>> + * Contraction of sprintf and xs_write(node/path).
>>>>> + *
>>>>> + * @param xbt Xenbus transaction id
>>>>> + * @param dir Xenstore directory
>>>>> + * @param node Xenstore directory entry
>>>>> + * @param fmt Path format string
>>>>> + * @return Just like sprintf, on success returns the number of the
>>>>> number of
>>>>> + * characters printed. On error returns a negative errno value.
>>>>> + */
>>>>> +int xs_printf(xenbus_transaction_t xbt, const char *dir, const char
>>>>> *node,
>>>>> + const char *fmt, ...) __printf(4, 5);
>>>>> +
>>>>> +/*
>>>>> + * Utility function to figure out our domain id
>>>>> + *
>>>>> + * @return Our domain id
>>>>> + */
>>>>> +domid_t xs_get_self_id(void);
>>>>> +
>>>>> +#endif /* __XS_H__ */
>>>>> diff --git a/plat/xen/xenbus/xs.c b/plat/xen/xenbus/xs.c
>>>>> new file mode 100644
>>>>> index 0000000..eb5131a
>>>>> --- /dev/null
>>>>> +++ b/plat/xen/xenbus/xs.c
>>>>> @@ -0,0 +1,650 @@
>>>>> +/* SPDX-License-Identifier: BSD-3-Clause */
>>>>> +/*
>>>>> + * Authors: Steven Smith (sos22@xxxxxxxxx)
>>>>> + * Grzegorz Milos (gm281@xxxxxxxxx)
>>>>> + * John D. Ramsdell
>>>>> + * Costin Lupu <costin.lupu@xxxxxxxxx>
>>>>> + *
>>>>> + * Copyright (c) 2006, Cambridge University
>>>>> + * 2018, NEC Europe Ltd., NEC Corporation. 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.
>>>>> + */
>>>>> +/*
>>>>> + * Ported from Mini-OS xenbus.c
>>>>> + */
>>>>> +
>>>>> +#include <stdlib.h>
>>>>> +#include <stdio.h>
>>>>> +#include <string.h>
>>>>> +#include <stdint.h>
>>>>> +#include <stdarg.h>
>>>>> +#include <uk/errptr.h>
>>>>> +#include <xen/io/xs_wire.h>
>>>>> +#include <xenbus/xs.h>
>>>>> +#include "xs_comms.h"
>>>>> +
>>>>> +
>>>>> +/* Helper macro for initializing xs requests from strings
>>>>> + * (w/ null terminator)
>>>>> + */
>>>>> +#define XS_IOVEC_STR(str) \
>>>>> + ((struct xs_iovec) { str, strlen(str) + 1 })
>>>>> +
>>>>> +
>>>>> +/* Common function used for sending requests when replies aren't handled
>>>>> */
>>>>> +static inline int xs_msg(enum xsd_sockmsg_type type,
>>>>> xenbus_transaction_t xbt,
>>>>> + struct xs_iovec *reqs, int reqs_num)
>>>>> +{
>>>>> + return xs_msg_reply(type, xbt, reqs, reqs_num, NULL);
>>>>> +}
>>>>> +
>>>>> +char *xs_read(xenbus_transaction_t xbt, const char *path, const char
>>>>> *node)
>>>>> +{
>>>>> + struct xs_iovec req, rep;
>>>>> + char *fullpath, *value;
>>>>> + int err;
>>>>> +
>>>>> + if (path == NULL)
>>>>> + return ERR2PTR(EINVAL);
>>>>> +
>>>>> + if (node != NULL) {
>>>>> + err = asprintf(&fullpath, "%s/%s", path, node);
>>>>> + if (err < 0) {
>>>>> + value = ERR2PTR(ENOMEM);
>>>>> + goto out;
>>>>> + }
>>>>> + } else
>>>>> + fullpath = (char *) path;
>>>>> +
>>>>> + req = XS_IOVEC_STR(fullpath);
>>>>> + err = xs_msg_reply(XS_READ, xbt, &req, 1, &rep);
>>>>> + if (err == 0)
>>>>> + value = rep.data;
>>>>> + else
>>>>> + value = ERR2PTR(-err);
>>>>> +
>>>>> + if (node != NULL)
>>>>> + free(fullpath);
>>>>> +out:
>>>>> + return value;
>>>>> +}
>>>>> +
>>>>> +int xs_write(xenbus_transaction_t xbt, const char *path, const char
>>>>> *node,
>>>>> + const char *value)
>>>>> +{
>>>>> + struct xs_iovec req[2];
>>>>> + char *fullpath;
>>>>> + int err;
>>>>> +
>>>>> + if (path == NULL || value == NULL)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + if (node != NULL) {
>>>>> + err = asprintf(&fullpath, "%s/%s", path, node);
>>>>> + if (err < 0) {
>>>>> + err = -ENOMEM;
>>>>> + goto out;
>>>>> + }
>>>>> + } else
>>>>> + fullpath = (char *) path;
>>>>> +
>>>>> + req[0] = XS_IOVEC_STR(fullpath);
>>>>> + req[1] = XS_IOVEC_STR((char *) value);
>>>>> +
>>>>> + err = xs_msg(XS_WRITE, xbt, req, ARRAY_SIZE(req));
>>>>> +
>>>>> + if (node != NULL)
>>>>> + free(fullpath);
>>>>> +out:
>>>>> + return err;
>>>>> +}
>>>>> +
>>>>> +/* Returns an array of strings out of the serialized reply */
>>>>> +static char **reply_to_string_array(struct xs_iovec *rep, int *size)
>>>>> +{
>>>>> + int strings_num, offs, i;
>>>>> + char *rep_strings, *strings, **res = NULL;
>>>>> +
>>>>> + rep_strings = rep->data;
>>>>> +
>>>>> + /* count the strings */
>>>>> + for (offs = strings_num = 0; offs < (int) rep->len; offs++)
>>>>> + strings_num += (rep_strings[offs] == 0);
>>>>> +
>>>>> + /* one alloc for both string addresses and contents */
>>>>> + res = malloc((strings_num + 1) * sizeof(char *) + rep->len);
>>>>> + if (!res)
>>>>> + return ERR2PTR(ENOMEM);
>>>>> +
>>>>> + /* copy the strings at the end of the array */
>>>> _to_ the end of the array
>>>
>>> Fixed.
>>>
>>>>> + strings = (char *) &res[strings_num + 1];
>>>>> + memcpy(strings, rep_strings, rep->len);
>>>>> +
>>>>> + /* fill the string array */
>>>>> + for (offs = i = 0; i < strings_num; i++) {
>>>>> + char *string = strings + offs;
>>>>> + int string_len = strlen(string);
>>>>> +
>>>>> + res[i] = string;
>>>>> +
>>>>> + offs += string_len + 1;
>>>>> + }
>>>>> + res[i] = NULL;
>>>>> +
>>>>> + if (size)
>>>>> + *size = strings_num;
>>>>> +
>>>>> + return res;
>>>>> +}
>>>>> +
>>>>> +char **xs_ls(xenbus_transaction_t xbt, const char *path)
>>>>> +{
>>>>> + struct xs_iovec req, rep;
>>>>> + char **res = NULL;
>>>>> + int err;
>>>>> +
>>>>> + if (path == NULL)
>>>>> + return ERR2PTR(EINVAL);
>>>>> +
>>>>> + req = XS_IOVEC_STR((char *) path);
>>>>> + err = xs_msg_reply(XS_DIRECTORY, xbt, &req, 1, &rep);
>>>>> + if (err)
>>>>> + return ERR2PTR(-err);
>>>>> +
>>>>> + res = reply_to_string_array(&rep, NULL);
>>>>> + free(rep.data);
>>>>> +
>>>>> + return res;
>>>>> +}
>>>>> +
>>>>> +int xs_rm(xenbus_transaction_t xbt, const char *path)
>>>>> +{
>>>>> + struct xs_iovec req;
>>>>> +
>>>>> + if (path == NULL)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + req = XS_IOVEC_STR((char *) path);
>>>>> +
>>>>> + return xs_msg(XS_RM, xbt, &req, 1);
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Permissions
>>>>> + */
>>>>> +
>>>>> +static const char xs_perm_tbl[] = {
>>>>> + [XS_PERM_NONE] = 'n',
>>>>> + [XS_PERM_READ] = 'r',
>>>>> + [XS_PERM_WRITE] = 'w',
>>>>> + [XS_PERM_BOTH] = 'b',
>>>>> +};
>>>>> +
>>>>> +int xs_char_to_perm(char c, enum xs_perm *perm)
>>>>> +{
>>>>> + int err = -EINVAL;
>>>>> +
>>>>> + if (perm == NULL)
>>>>> + goto out;
>>>>> +
>>>>> + for (int i = 0; i < (int) ARRAY_SIZE(xs_perm_tbl); i++) {
>>>>> + if (c == xs_perm_tbl[i]) {
>>>>> + *perm = i;
>>>>> + err = 0;
>>>>> + break;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> +out:
>>>>> + return err;
>>>>> +}
>>>>> +
>>>>> +int xs_perm_to_char(enum xs_perm perm, char *c)
>>>>> +{
>>>>> + int err = -EINVAL;
>>>>> +
>>>>> + if (c == NULL)
>>>>> + goto out;
>>>>> +
>>>>> + if (perm < ARRAY_SIZE(xs_perm_tbl)) {
>>>>> + *c = xs_perm_tbl[perm];
>>>>> + err = 0;
>>>>> + }
>>>>> +
>>>>> +out:
>>>>> + return err;
>>>>> +}
>>>> I don't mind this code, but consider this way:
>>>>
>>>> int xs_perm_to_char(enum xs_perm perm, char *c)
>>>> {
>>>> if (c == NULL || perm >= ARRAY_SIZE(xs_perm_tlb))
>>>> return -EINVAL;
>>>> *c = xs_perm_tbl[perm];
>>>> return 0;
>>>> }
>>>>
>>>
>>> Ack.
>>>
>>>>> +
>>>>> +int xs_str_to_perm(const char *str, domid_t *domid, enum xs_perm *perm)
>>>>> +{
>>>>> + int err = 0;
>>>>> +
>>>>> + if (str == NULL || domid == NULL || perm == NULL) {
>>>>> + err = -EINVAL;
>>>>> + goto out;
>>>>> + }
>>>>> +
>>>>> + err = xs_char_to_perm(str[0], perm);
>>>>> + if (err)
>>>>> + goto out;
>>>>> +
>>>>> + *domid = (domid_t) strtoul(&str[1], NULL, 10);
>>>>> +
>>>>> +out:
>>>>> + return err;
>>>>> +}
>>>>> +
>>>>> +#define PERM_MAX_SIZE 32
>>>>> +char *xs_perm_to_str(domid_t domid, enum xs_perm perm)
>>>>> +{
>>>>> + int err = 0;
>>>>> + char permc, value[PERM_MAX_SIZE];
>>>>> +
>>>>> + err = xs_perm_to_char(perm, &permc);
>>>>> + if (err)
>>>>> + return NULL;
>>>>> +
>>>>> + snprintf(value, PERM_MAX_SIZE, "%c%hu", permc, domid);
>>>>> +
>>>>> + return strdup(value);
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Returns the ACL for input path. An extra number of empty entries may
>>>>> be
>>>>> + * requested if caller intends to extend the list.
>>>>> + */
>>>>> +static struct xs_acl *__xs_get_acl(xenbus_transaction_t xbt, const char
>>>>> *path,
>>>>> + int extra)
>>>>> +{
>>>>> + struct xs_acl *acl = NULL;
>>>>> + struct xs_iovec req, rep;
>>>>> + char **values;
>>>>> + int values_num, err;
>>>>> +
>>>>> + if (path == NULL) {
>>>>> + err = EINVAL;
>>>>> + goto out;
>>>>> + }
>>>>> +
>>>>> + req = XS_IOVEC_STR((char *) path);
>>>>> + err = xs_msg_reply(XS_GET_PERMS, xbt, &req, 1, &rep);
>>>>> + if (err)
>>>>> + goto out;
>>>>> +
>>>>> + values = reply_to_string_array(&rep, &values_num);
>>>>> + free(rep.data);
>>>>> + if (PTRISERR(values)) {
>>>>> + err = PTR2ERR(values);
>>>>> + goto out;
>>>>> + }
>>>>> +
>>>>> + acl = malloc(sizeof(struct xs_acl) +
>>>>> + (values_num + extra) * sizeof(struct xs_acl_entry));
>>>>> + if (acl == NULL) {
>>>>> + err = ENOMEM;
>>>>> + goto out_values;
>>>>> + }
>>>>> +
>>>>> + /* set owner id and permissions for others */
>>>>> + err = xs_str_to_perm(values[0],
>>>>> + &acl->ownerid, &acl->others_perm);
>>>>> + if (err)
>>>>> + goto out_values;
>>>>> +
>>>>> + /* set ACL entries */
>>>>> + acl->entries_num = values_num - 1;
>>>>> + for (int i = 0; i < acl->entries_num; i++) {
>>>>> + err = xs_str_to_perm(values[i + 1],
>>>>> + &acl->entries[i].domid, &acl->entries[i].perm);
>>>>> + if (err)
>>>>> + goto out_values;
>>>>> + }
>>>>> +
>>>>> +out_values:
>>>>> + free(values);
>>>>> +out:
>>>>> + if (err) {
>>>>> + if (acl)
>>>>> + free(acl);
>>>>> + acl = ERR2PTR(-err);
>>>>> + }
>>>>> + return acl;
>>>>> +}
>>>>> +
>>>>> +struct xs_acl *xs_get_acl(xenbus_transaction_t xbt, const char *path)
>>>>> +{
>>>>> + return __xs_get_acl(xbt, path, 0);
>>>>> +}
>>>>> +
>>>>> +int xs_set_acl(xenbus_transaction_t xbt, const char *path, struct xs_acl
>>>>> *acl)
>>>>> +{
>>>>> + struct xs_iovec req[2 + acl->entries_num];
>>>>> + char *s;
>>>>> + int i, err;
>>>>> +
>>>>> + if (path == NULL || acl == NULL) {
>>>>> + err = -EINVAL;
>>>>> + goto out;
>>>>> + }
>>>>> +
>>>>> + req[0] = XS_IOVEC_STR((char *) path);
>>>>> +
>>>>> + s = xs_perm_to_str(acl->ownerid, acl->others_perm);
>>>>> + if (s == NULL) {
>>>>> + err = -EINVAL;
>>>>> + goto out;
>>>>> + }
>>>>> +
>>>>> + req[1].data = s;
>>>>> + req[1].len = strlen(s) + 1;
>>>>> +
>>>>> + for (i = 0; i < acl->entries_num; i++) {
>>>>> + struct xs_acl_entry *acle = &acl->entries[i];
>>>>> +
>>>>> + s = xs_perm_to_str(acle->domid, acle->perm);
>>>>> + if (s == NULL) {
>>>>> + err = -EINVAL;
>>>>> + goto out_req;
>>>>> + }
>>>>> +
>>>>> + req[i + 2].data = s;
>>>>> + req[i + 2].len = strlen(s) + 1;
>>>> I think you can use XS_IOVEC_STR macro here too. For the sake of
>>>> consistency.
>>>>
>>>
>>> Fixed.
>>>
>>>>> + }
>>>>> +
>>>>> + err = xs_msg(XS_SET_PERMS, xbt, req, ARRAY_SIZE(req));
>>>>> +
>>>>> +out_req:
>>>>> + for (i--; i > 0; i--)
>>>>> + free(req[i].data);
>>>>> +out:
>>>>> + return err;
>>>>> +}
>>>>> +
>>>>> +int xs_get_perms(xenbus_transaction_t xbt, const char *path,
>>>>> + domid_t domid, enum xs_perm *perm)
>>>> The functions gets only one permission. How about to rename it to
>>>> xs_get_perm. Same about xs_set_perms
>>>>
>>>
>>> Ack.
>>>
>>>>> +{
>>>>> + struct xs_acl *acl;
>>>>> + int err = 0;
>>>>> +
>>>>> + if (perm == NULL) {
>>>>> + err = -EINVAL;
>>>>> + goto out;
>>>>> + }
>>>>> +
>>>>> + acl = xs_get_acl(xbt, path);
>>>>> + if (PTRISERR(acl)) {
>>>>> + err = PTR2ERR(acl);
>>>>> + goto out;
>>>>> + }
>>>>> +
>>>>> + if (acl->ownerid == domid) {
>>>>> + *perm = XS_PERM_BOTH;
>>>>> + goto out_acl;
>>>>> + }
>>>>> +
>>>>> + for (int i = 0; i < acl->entries_num; i++) {
>>>>> + struct xs_acl_entry *acle = &acl->entries[i];
>>>>> +
>>>>> + if (acle->domid == domid) {
>>>>> + *perm = acle->perm;
>>>>> + goto out_acl;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + *perm = acl->others_perm;
>>>>> +
>>>>> +out_acl:
>>>>> + free(acl);
>>>>> +out:
>>>>> + return err;
>>>>> +}
>>>>> +
>>>>> +int xs_set_perms(xenbus_transaction_t xbt, const char *path,
>>>>> + domid_t domid, enum xs_perm perm)
>>>>> +{
>>>>> + struct xs_acl *acl;
>>>>> + struct xs_acl_entry *acle;
>>>>> + int i, err = 0;
>>>>> +
>>>>> + /* one extra entry in case a new one will be added */
>>>>> + acl = __xs_get_acl(xbt, path, 1);
>>>>> + if (PTRISERR(acl)) {
>>>>> + err = PTR2ERR(acl);
>>>>> + goto out;
>>>>> + }
>>>> If somebody else (e.g. dom0) will be changing acl of this path at the
>>>> same time, it might be that only one operation will be successful. I
>>>> would start a transaction if xbt==0 or add some check. Either return
>>>> with an error, or add debug assert, or at least print a warning. However
>>>> warnings need to be rate limited, which we do not have yet.
>>>>
>>>
>>> Starting a transaction here would defeat the purpose of this API. It's
>>> the client (caller) responsability to take care of the atomicity of her
>>> operations. Therefore, in that case one should start the transaction
>>> before calling this function.
>>>
>>> We might have paths for which only the caller domain would have
>>> permissions.
>> Isn't the dom0 always have a permission?
>>
>
> Please allow me to answer with a question here: what if Unikraft is
> dom0? :-) Joking aside, the intention behind this API is to allow a
> higher level of flexibility and to be as simple as possible.
If we are dom0, then the domain-owner of this path still can modify it.
That is error prone to do a multistep operation non-atomically. If you
think a higher level API should ensure transaction, then do an assert in
this function to check if transaction is registered. For more
flexibility one should use lower level functions. Especially since the
xs_set_perm(s) is pretty high level function and does not provide much
flexibility.
We can remove this check later if we have a good reason to. Currently I
don't see one..
>
>>> More than that, we would know for sure for a given path, that no one
>>> else would change it.
>> Of course, other domains can promise that they would never write to this
>> path. But if we could trust them, this whole permission thing would not
>> be needed. At the very least, a human might make this modification by
>> issuing xenstore-* commands.
>>
>
> I don't think we should make a decision here for the client whether he
> trusts the other domains or not. This is just an API for interacting
> with the Xenstore. The security policies should be enforced elsewhere.
>
>>> Another case would be if we are simply not interested in keeping the
>>> old permissions.
>> The function xs_set_perms does not provide a way to change all
>> permissions at once. It does modify/add only one permission at one
>> time. That means if I want to grand permission to 3 different domains, I
>> have to call this function 3 times. Every time it needs to read existing
>> permissions, make changes, and write them back.
>>
>
> I don't understand what this comment has to do with my observation.
From my understanding you observation was not completely valid (see
below).
> However, this is the exact purpose of xs_set_perms function: to set
> permissions for a single domain. If you want more domains at once, then
> you should use the ACL functions.
So, if you not interested in keeping the old permissions you should use
the ACL functions you just mentioned. Otherwise you operation might be
not successful:
step Dom0 Our Dom
───────────────────────────────────────────────────────────────
1 reads acl for the path
"r0 w124 r125"
2 reads the same acl
"r0 w124 r125"
3 adds one more entry disregard of our interest in
in memory old permissions, we can change
"r0 w124 r125 w126" only one permission at once, so
we change it (in memory) to
"r0 r124 r125"
4 writes new permissions
"n0 w555"
5 writes new permissions
"r0 w124 r125 w126
> Again, these xs_{set,get}_perm functions are there just to help the
> developer when she wants to set the permissions for a single domain
> since this is only a routine. The API and the implementation should be
> as simple and clear as possible.
>
> Since I don't endorse providing atomicity here, I suggest we should add
> a patch doing it after upstreaming this patch series.
>
>>>
>>> Long story short, this approach adds more flexbility. I think the best
>>> solution here would be to add some comments in the function description
>>> in order to clarify this behavior.
>>>
>>>>> +
>>>>> + if (acl->ownerid == domid) {
>>>>> + /*
>>>>> + * let's say the function isn't called correctly considering
>>>>> + * that the owner domain has all the rights, all the time
>>>>> + */
>>>>> + err = -EINVAL;
>>>>> + goto out_acl;
>>>>> + }
>>>>> +
>>>>> + for (i = 0, acle = &acl->entries[i]; i < acl->entries_num; i++) {
>>>>> + if (acle->domid == domid)
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + if (perm != XS_PERM_NONE) {
>>>>> + if (i == acl->entries_num) {
>>>>> + /* new entry */
>>>>> + acle->domid = domid;
>>>>> + acl->entries_num++;
>>>>> + }
>>>>> +
>>>>> + acle->perm = perm;
>>>>> +
>>>>> + } else {
>>>>> + if (i == acl->entries_num) {
>>>>> + /* no entry */
>>>>> + err = -ENOENT;
>>>>> + goto out_acl;
>>>>> + }
>>>>> + /* remove entry */
>>>>> + acl->entries_num--;
>>>>> + memmove(&acl->entries[i], &acl->entries[i + 1],
>>>>> + (acl->entries_num - i) * sizeof(struct xs_acl_entry));
>>>> From my shallow understanding, if acl->others_perm == XS_PERM_READ,
>>>> deleting an acl entry for the path will effectively grand reading
>>>> permission. Maybe the goal was to block this particular domain from
>>>> reading this path, while allowing everybody else.
>>>>
>>>> Anyways, this is a bit unexpected. I asked to change permission for
>>>> particular domain to XS_NONE, but the function deletes it instead. I
>>>> guess a separate function is needed for deletion
>>>>
>>>
>>> Right. In v3 I will add xs_del_perm function.
>>>
>>>>> + }
>>>>> +
>>>>> + err = xs_set_acl(xbt, path, acl);
>>>>> +
>>>>> +out_acl:
>>>>> + free(acl);
>>>>> +out:
>>>>> + return err;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Transactions
>>>>> + */
>>>>> +
>>>>> +int xs_transaction_start(xenbus_transaction_t *xbt)
>>>>> +{
>>>>> + /*
>>>>> + * xenstored becomes angry if you send a length 0 message,
>>>>> + * so just shove a nul terminator on the end
>>>>> + */
>>>>> + struct xs_iovec req, rep;
>>>>> + int err;
>>>>> +
>>>>> + if (xbt == NULL)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + req = XS_IOVEC_STR("");
>>>>> + err = xs_msg_reply(XS_TRANSACTION_START, 0, &req, 1, &rep);
>>>>> + if (err)
>>>>> + return err;
>>>>> +
>>>>> + *xbt = strtoul(rep.data, NULL, 10);
>>>>> + free(rep.data);
>>>>> +
>>>>> + return err;
>>>>> +}
>>>>> +
>>>>> +int xs_transaction_end(xenbus_transaction_t xbt, int abort)
>>>>> +{
>>>>> + struct xs_iovec req;
>>>>> +
>>>>> + req.data = abort ? "F" : "T";
>>>>> + req.len = 2;
>>>>> +
>>>>> + return xs_msg(XS_TRANSACTION_END, xbt, &req, 1);
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Misc
>>>>> + */
>>>>> +
>>>>> +/* Send a debug message to xenbus. Can block. */
>>>>> +int xs_debug_msg(const char *msg)
>>>>> +{
>>>>> + struct xs_iovec req[3], rep;
>>>>> + int err;
>>>>> +
>>>>> + if (msg == NULL)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + req[0] = XS_IOVEC_STR("print");
>>>>> + req[1] = XS_IOVEC_STR((char *) msg);
>>>>> + req[2] = XS_IOVEC_STR("");
>>>>> +
>>>>> + err = xs_msg_reply(XS_DEBUG, XBT_NIL, req, ARRAY_SIZE(req), &rep);
>>>>> + if (err)
>>>>> + goto out;
>>>>> +
>>>>> + uk_printd(DLVL_EXTRA,
>>>>> + "Got a debug reply %s\n", (char *) rep.data);
>>>>> + free(rep.data);
>>>>> +
>>>>> +out:
>>>>> + return err;
>>>>> +}
>>>>> +
>>>>> +int xs_read_integer(const char *path, int *value)
>>>>> +{
>>>>> + char *value_str;
>>>>> +
>>>>> + if (path == NULL || value == NULL)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + value_str = xs_read(XBT_NIL, path, NULL);
>>>>> + if (PTRISERR(value_str))
>>>>> + return PTR2ERR(value_str);
>>>>> +
>>>>> + *value = atoi(value_str);
>>>>> +
>>>>> + free(value_str);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +int xs_scanf(xenbus_transaction_t xbt, const char *dir, const char *node,
>>>>> + const char *fmt, ...)
>>>>> +{
>>>>> +#define FULLPATH_SIZE 256
>>>>> + char fullpath[FULLPATH_SIZE];
>>>>> + char *val;
>>>>> + va_list args;
>>>>> + int err = 0;
>>>>> +
>>>>> + if (dir == NULL || node == NULL || fmt == NULL)
>>>>> + return -EINVAL;
>>>> I guess this is not precisely mimicking scanf behavior. And this is
>>>> fine. Maybe worth to put a comment "returns the number of input items
>>>> successfully matched or (-err)". Same applies to xs_printf.
>>>>
>>>
>>> The function description (in xs.h) says exactly that: on success it
>>> behaves like sscanf, on error it returns a negative number. Same for
>>> xs_printf.
>> Oh, missed that. However, because the first sentence starts with "Just
>> like sprintf..", I would start the second sentence with "Contrary to
>> sprintf.."
>>
>
> I'll just remove the references to sscanf/sprintf in the text about the
> return values in order to avoid any further confusions.
>
>>>
>>>>> +
>>>>> + if (strlen(dir) + strlen(node) + 1 >= FULLPATH_SIZE)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + sprintf(fullpath, "%s/%s", dir, node);
>>>> But xs_read does path concatenation. I think we can offload this work to
>>>> xs_read. This also will make it possible to have node == Null. Not sure
>>>> if you need it, but you will get it for free.
>>>>
>>>> If it is done to save a memory allocation in xs_read, well, xs_read can
>>>> get a simple optimization - allocate fullpath only if more then 256
>>>> bytes are needed. Otherwise use on-stack buffer.
>>>>
>>>> Same applies to xs_printf
>>>>
>>>
>>> Fixed, using xs_read now.
>>>
>>>>> +
>>>>> + val = xs_read(xbt, fullpath, NULL);
>>>>> + if (PTRISERR(val)) {
>>>>> + err = PTR2ERR(val);
>>>> This will lead to positive value in err. But in the "out" section you
>>>> expect it be negative. I believe we have to invert it hire. Please
>>>> revise other places in the code for the same time of problems.
>>>>
>>>
>>> Right. I think we need to clarify this issue. With the current
>>> implementation of PTR*ERR macros, we can support only positive error
>>> numbers. More than that, when we need to convert the error number back
>>> from pointer with PTR2ERR() we must always invert it. So, we have 2 options:
>>> 1) keeping PTR*ERR macros like this and always reverting PTR2ERR value
>>> 2) changing the macros to the way Linux does it, which does not need to
>>> invert PTR2ERR value.
>>>
>>> Yuri, what do you think?
>> I actually prefer the linux way, it looks a bit more convenient. That is
>> probably not too late to change this.
>>
>
> Agreed.
>
>>>
>>>>> + goto out;
>>>>> + }
>>>>> +
>>>>> + va_start(args, fmt);
>>>>> + err = vsscanf(val, fmt, args);
>>>>> + va_end(args);
>>>>> +
>>>>> + free(val);
>>>>> +
>>>>> +out:
>>>>> + return err;
>>>>> +}
>>>>> +
>>>>> +int xs_printf(xenbus_transaction_t xbt, const char *dir, const char
>>>>> *node,
>>>>> + const char *fmt, ...)
>>>>> +{
>>>>> + char fullpath[FULLPATH_SIZE];
>>>>> + char val[FULLPATH_SIZE];
>>>>> + va_list args;
>>>>> + int err, _err;
>>>>> +
>>>>> + if (dir == NULL || node == NULL || fmt == NULL)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + if (strlen(dir) + strlen(node) + 1 >= FULLPATH_SIZE)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + sprintf(fullpath, "%s/%s", dir, node);
>>>>> +
>>>>> + va_start(args, fmt);
>>>>> + _err = vsprintf(val, fmt, args);
>>>> vsnprintf MUST be used here. The val is limited with FULLPATH_SIZE. Same
>>>> about printf-ing into fullpath (obviously in xs_scanf too), if you
>>>> decided not to rely on xs_write to concatenate path and node.
>>>>
>>>
>>> Fixed.
>>>
>>>>> + va_end(args);
>>>>> +
>>>>> + /* send to Xenstore iff vsprintf was successful */
>>>> Typo in iff
>>>>
>>>
>>> Actually iff stands for "if and only if". I set it to if now to avoid
>>> further confusion.
>>>
>>>>> + if (_err > 0)
>>>>> + err = xs_write(xbt, fullpath, NULL, val);
>>>>> +
>>>>> + /*
>>>>> + * if message sent to Xenstore was successful,
>>>>> + * return the number of characters
>>>>> + */
>>>>> + if (err == 0)
>>>>> + err = _err;
>>>>> +
>>>>> + return err;
>>>>> +}
>>>>> +> +domid_t xs_get_self_id(void)
>>>>> +{
>>>>> + char *domid_str;
>>>>> + domid_t domid;
>>>>> +
>>>>> + domid_str = xs_read(XBT_NIL, "domid", NULL);
>>>>> + if (PTRISERR(domid_str))
>>>>> + UK_CRASH("Error reading domain id.");
>>>>> +
>>>>> + domid = (domid_t) strtoul(domid_str, NULL, 10);
>>>>> +
>>>>> + free(domid_str);
>>>>> +
>>>>> + return domid;
>>>>> +}
>>>>> --
>>>>> 2.11.0
>>>>>
>>>>
>>
>
--
Yuri Volchkov
Software Specialist
NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |