|
[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
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.
>> 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.
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. 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
>>>>
>>>
>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |