[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [UNIKRAFT PATCH 1/8] lib/uklibparam: Introduce the library parameter
On 4/11/19 4:13 PM, Florian Schmidt wrote:
Hi Sharan,
thanks for the patch series. I have a few comments on this rather hefty
first patch:
Thanks for the review. There are still some bugs in the codes. I will
fix them and send out a V2.
First of all, you need to create an entry in MAINTAINERS.md for the
library. Also, I get several checkpatch warnings and errors for this
patch, most importantly related to enclosing statements in complex
macros in do-while loops.
I will add it to the MAINTAINERS.md
I did not add the do-while loops for the static global fields. Anyways I
will check if I have missed out others.
On 3/15/19 6:06 PM, Sharan Santhanam wrote:
This patch provides the header necessary to register a variable as an
boot argument with Unikraft that may depend on user input. The patch
provides an implementation for parsing scalar arguments.
Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
---
lib/Config.uk | 1 +
lib/Makefile.uk | 1 +
lib/uklibparam/Config.uk | 5 +
lib/uklibparam/Makefile.uk | 7 +
lib/uklibparam/exportsyms.uk | 2 +
lib/uklibparam/include/uk/libparam.h | 402
+++++++++++++++++++++++++++++
lib/uklibparam/param.c | 480
+++++++++++++++++++++++++++++++++++
7 files changed, 898 insertions(+)
create mode 100644 lib/uklibparam/Config.uk
create mode 100644 lib/uklibparam/Makefile.uk
create mode 100644 lib/uklibparam/exportsyms.uk
create mode 100644 lib/uklibparam/include/uk/libparam.h
create mode 100644 lib/uklibparam/param.c
diff --git a/lib/Config.uk b/lib/Config.uk
index 7d86f76..a630d15 100644
--- a/lib/Config.uk
+++ b/lib/Config.uk
@@ -45,3 +45,4 @@ source "lib/ukswrand/Config.uk"
source "lib/ukbus/Config.uk"
source "lib/uksglist/Config.uk"
source "lib/uknetdev/Config.uk"
+source "lib/uklibparam/Config.uk"
diff --git a/lib/Makefile.uk b/lib/Makefile.uk
index d66c7ee..c1a4544 100644
--- a/lib/Makefile.uk
+++ b/lib/Makefile.uk
@@ -22,3 +22,4 @@ $(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/ukmpi))
$(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/ukbus))
$(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/uksglist))
$(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/uknetdev))
+$(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/uklibparam))
diff --git a/lib/uklibparam/Config.uk b/lib/uklibparam/Config.uk
new file mode 100644
index 0000000..18bb43d
--- /dev/null
+++ b/lib/uklibparam/Config.uk
@@ -0,0 +1,5 @@
+config LIBUKLIBPARAM
+ bool "uk library parameter: Pass arguments to a unikraft library"
+ default n
+ select LIBUKDEBUG
+ select LIBNOLIBC if !HAVE_LIBC
diff --git a/lib/uklibparam/Makefile.uk b/lib/uklibparam/Makefile.uk
new file mode 100644
index 0000000..d354152
--- /dev/null
+++ b/lib/uklibparam/Makefile.uk
@@ -0,0 +1,7 @@
+$(eval $(call addlib_s,libuklibparam,$(CONFIG_LIBUKLIBPARAM)))
+
+ASINCLUDES-$(CONFIG_LIBUKLIBPARAM) += -I$(LIBUKLIBPARAM_BASE)/include
+CINCLUDES-$(CONFIG_LIBUKLIBPARAM) += -I$(LIBUKLIBPARAM_BASE)/include
+CXXINCLUDES-$(CONFIG_LIBUKLIBPARAM) +=
-I$(LIBUKLIBPARAM_BASE)/include
+
+LIBUKLIBPARAM_SRCS-y += $(LIBUKLIBPARAM_BASE)/param.c
diff --git a/lib/uklibparam/exportsyms.uk b/lib/uklibparam/exportsyms.uk
new file mode 100644
index 0000000..94b6ca7
--- /dev/null
+++ b/lib/uklibparam/exportsyms.uk
@@ -0,0 +1,2 @@
+uk_libparam_parse
+_uk_libparam_lib_add
diff --git a/lib/uklibparam/include/uk/libparam.h
b/lib/uklibparam/include/uk/libparam.h
new file mode 100644
index 0000000..0855d2d
--- /dev/null
+++ b/lib/uklibparam/include/uk/libparam.h
@@ -0,0 +1,402 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
+ *
+ *
+ * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights
reserved.
We're in 2019 by now :)
Agreed
+ *
+ * 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_LIBPARAM_H
+#define __UK_LIBPARAM_H
+
+#include <uk/config.h>
+#ifndef __ASSEMBLY__
+#include <uk/arch/types.h>
+#include <uk/essentials.h>
+#include <uk/list.h>
+#include <uk/print.h>
+
+#ifdef __cplusplus
+extern C {
+#endif /* __cplusplus */
+#endif /* !__ASSEMBLY__ */
+
+/**
+ * Variable name prefix/suffix
+ */
+#define UK_LIBPARAM_SECTION uk_lib_arg
+/**
+ * Library: section suffix for the name and the
+ * parameter.
+ */
+#define LIB_PARAM_SUFFIX __lib_param
+#define LIB_NAME_SUFFIX __lib_str
+/**
+ * Library variable names for the name and the
+ * parameter.
+ */
+#define LIB_PARAMVAR_PREFIX _lib_param_
+#define LIB_NAMEVAR_PREFIX _lib_name_
+/**
+ * Parameter within a library: section suffix for the name and the
+ * parameter.
+ */
+#define PARAM_SECTION_SUFFIX __param_arg
+#define PARAM_NAME_SUFFIX __param_str
+/**
+ * Parameter within a library: variable name prefix for the name and the
+ * parameter.
+ */
+#define PARAM_PARAMVAR_PREFIX _param_param_
+#define PARAM_NAMEVAR_PREFIX _param_name_
+
+#define __STRINGCONCAT(x, y) x##y
+
+/**
+ * Create a section name.
+ * @param libname
+ * The library name
+ * @param section
+ * The section suffix for the library
+ */
+#define _LIB_PARAM_SECTION_NAME(libname, section_name) \
+ __STRINGCONCAT(libname, section_name)
+
+/**
+ * Macros to denote the start / stop of a section.
+ */
+#define _SECTION_START(name) __STRINGCONCAT(__start_, name)
+#define _SECTION_STOP(name) __STRINGCONCAT(__stop_, name)
+
+#ifndef __ASSEMBLY__
+/**
+ * Each parameter is bit-mapped as follows:
+ * ---------------------------------------
+ * | sign | copy | size of the parameter |
+ * ---------------------------------------
+ * 7 6 5 0
+ */
+/**
+ * Sign bit: Shift & Mask
+ */
+#define PARAM_SIGN_SHIFT (7)
+#define PARAM_SIGN_MASK (0x1)
+/**
+ * Shallow copy: Shift & Mask
+ */
+#define PARAM_SCOPY_SHIFT (6)
+#define PARAM_SCOPY_MASK (0x1)
+/**
+ * Size of the param: Shift & Mask
+ */
+#define PARAM_SIZE_SHIFT (0x0)
+#define PARAM_SIZE_MASK (0x3F)
The ASCII figure and the actual implementation don't seem to match, I
would have expected your figure to look like this:
---------------------------------------
| sign | copy | size of the parameter |
---------------------------------------
7 6 5 0
Also, is there a reason the order in memory of these items is size,
copy, sign; but the macro uses the order size, sign, copy? It just goes
against my sense of order. ;-)
I will change the order in the macro.
+
+/**
+ * Get the parameter type.
+ * @param size
+ * The size of the parameter.
+ * @param sign
+ * The sign of the data type.
+ * @param scopy
+ * Flag to indicate shallow copy.
+ * 1 - shallow copy.
+ * 0 - data copy.
+ */
+#define PARAM_TYPE(size, sign, scopy) \
+ ( \
+ ((((__u8) (sign & PARAM_SIGN_MASK)) << \
+ PARAM_SIGN_SHIFT) | \
+ (((__u8) (scopy & PARAM_SCOPY_MASK)) << \
+ PARAM_SCOPY_SHIFT) | \
+ (((__u8) (size & PARAM_SIZE_MASK)) << \
+ PARAM_SIZE_SHIFT)) \
+ )
+
+
+/**
+ * Support data types as parameters
+ */
+#define _LIB_PARAM___s8 PARAM_TYPE(sizeof(__s8), 1, 0)
+#define _LIB_PARAM_char _LIB_PARAM___s8
+#define _LIB_PARAM___u8 PARAM_TYPE(sizeof(__u8), 0, 0)
+#define _LIB_PARAM___s16 PARAM_TYPE(sizeof(__s16), 1, 0)
+#define _LIB_PARAM___u16 PARAM_TYPE(sizeof(__u16), 0, 0)
+#define _LIB_PARAM___s32 PARAM_TYPE(sizeof(__s32), 1, 0)
+#define _LIB_PARAM_int _LIB_PARAM___s32
+#define _LIB_PARAM___u32 PARAM_TYPE(sizeof(__u32), 0, 0)
+#define _LIB_PARAM___s64 PARAM_TYPE(sizeof(__s64), 1, 0)
+#define _LIB_PARAM___u64 PARAM_TYPE(sizeof(__u64), 0, 0)
+
+struct uk_param {
+ /* The name of the param */
+ const char *name;
+ /* Type information for the param */
+ const __u8 param_type;
+ /* Type information for the variable size param */
+ const __u8 param_size;
+ /* Define a reference to location of the parameter */
+ __uptr addr;
+};
+
+struct uk_lib_section {
+ /* Library name */
+ const char *lib_name;
+ /* Section header of the uk_param args */
+ struct uk_param *sec_addr_start;
+ /* Length of the section */
+ __u32 len;
+ /* Next section entry */
+ struct uk_list_head next;
+};
+
+/**
+ * Parse through the kernel parameter
+ * @param progname
+ * The application name
+ * @param argc
+ * The number of arguments
+ * @param argv
+ * Reference to the command line arguments
+ * @return
+ * On success, return the number of argument parsed.
+ * On Failure, return the error code.
+ */
+int uk_libparam_parse(const char *progname, int argc, char **argv);
+
+/**
+ * Register the library containing kernel parameter.
+ *
+ * @param lib_sec
+ * A reference to the uk_lib_section.
+ */
+void _uk_libparam_lib_add(struct uk_lib_section *lib_sec);
+
+/**
+ * Add a variable to a specific section.
+ * @param section_name
+ * The name of the section.
+ * @param align_type
+ * The alignment requirements for the variable definitions.
+ */
+#define _LIB_PARAM_SECTION_ADD(section_name, align_type) \
+ __attribute__ ((used, \
+ section( \
+ __STRINGIFY(section_name)), \
+ aligned(sizeof(align_type)) \
+ ))
+/**
+ * Create a constructor name.
+ * @param libname
+ * The library name.
+ * @param suffix
+ * The suffix appended to the library name.
+ */
+#define _LIB_UK_CONSTRUCT_NAME(libname, suffix) \
+ libname##_##suffix
+
+/**
+ * Create a variable name
+ * @param prefix
+ * The prefix to the variable name.
+ * @param name
+ * The name of the variable
+ */
+#define _LIB_VARNAME_SET(prefix, name) \
+ __STRINGCONCAT(prefix, name)
+
+/**
+ * Import the section header.
+ * @param libname
+ * The library name.
+ * @param suffix
+ * The suffix
+ */
+#define UK_LIB_IMPORT_SECTION_PARAMS(libname, section_suffix) \
+ extern char *_SECTION_START( \
+ _LIB_PARAM_SECTION_NAME(libname, \
+ section_suffix)); \
+ extern char *_SECTION_STOP( \
+ _LIB_PARAM_SECTION_NAME(libname, \
+ section_suffix)) \
+
+/**
+ * Create a library name variable and uk_lib_section for each library.
+ * @param libname
+ * The library name.
+ */
+#define UK_LIB_SECTION_CREATE(section, libname) \
+ static const _LIB_PARAM_SECTION_ADD( \
+ _LIB_PARAM_SECTION_NAME(section, \
+ LIB_NAME_SUFFIX),\
+ char) \
+ char _LIB_VARNAME_SET(LIB_NAMEVAR_PREFIX, libname)[] = \
+ __STRINGIFY(libname); \
+ static _LIB_PARAM_SECTION_ADD( \
+ _LIB_PARAM_SECTION_NAME(section, \
+ LIB_PARAM_SUFFIX), \
+ void *) \
+ struct uk_lib_section \
+ _LIB_VARNAME_SET(LIB_PARAMVAR_PREFIX, libname) = \
+ { .lib_name = __NULL, \
+ .sec_addr_start = __NULL, .len = 0 \
+ }
+
+/**
+ * Create a constructor to initialize the parameters in the library.
+ */
+#define UK_LIB_CONSTRUCTOR_CREATE(libname) \
+ static void __constructor_prio(101) \
+ _LIB_UK_CONSTRUCT_NAME(libname, process_arg)(void) \
+ { \
+ int len = (__uptr) &_SECTION_STOP( \
+ _LIB_PARAM_SECTION_NAME( \
+ libname, PARAM_SECTION_SUFFIX) \
+ ) - \
+ (__uptr) &_SECTION_START( \
+ _LIB_PARAM_SECTION_NAME( \
+ libname, PARAM_SECTION_SUFFIX) \
+ ); \
+ if (len > 0) { \
+ _LIB_VARNAME_SET(LIB_PARAMVAR_PREFIX, libname). \
+ sec_addr_start = \
+ (struct uk_param *) \
+ ALIGN_UP((__uptr) \
+ &_SECTION_START( \
+ _LIB_PARAM_SECTION_NAME(\
+ libname, \
+ PARAM_SECTION_SUFFIX)), \
+ sizeof(void *)); \
+ _LIB_VARNAME_SET(LIB_PARAMVAR_PREFIX, libname). \
+ len = len; \
+ _LIB_VARNAME_SET(LIB_PARAMVAR_PREFIX, libname). \
+ lib_name = \
+ &_LIB_VARNAME_SET( \
+ LIB_NAMEVAR_PREFIX, \
+ libname)[0]; \
+ _uk_libparam_lib_add( \
+ &_LIB_VARNAME_SET( \
+ LIB_PARAMVAR_PREFIX, \
+ libname) \
+ ); \
+ } \
+ }
+
+#define UK_LIB_CONSTRUCTOR_INIT(libname) \
+ UK_LIB_IMPORT_SECTION_PARAMS(libname, \
+ PARAM_SECTION_SUFFIX); \
+ UK_LIB_SECTION_CREATE(UK_LIBPARAM_SECTION, libname); \
+ UK_LIB_CONSTRUCTOR_CREATE(libname)
+
+
+/**
+ * Create a constructor to fill in the parameter.
+ */
+#ifdef UK_LIBPARAM_PREFIX
+ UK_LIB_CONSTRUCTOR_INIT(UK_LIBPARAM_PREFIX);
+#endif /* UK_LIBPARAM_PREFIX */
+
+/**
+ * Create the fully qualified name of a parameter.
+ *
+ * @param libname
+ * The name of the library
+ * @param name
+ * The name of the parameter
+ */
+#define _LIB_PARAM_STRING(libname, name) \
+ libname.name
+
+/**
+ * Initialize the parameter string in a variable. The name of the
+ * parameter is stored in a separate linker section.
+ *
+ * @param name
+ * The name of the variable
+ * @param value
+ * The string representation of the parameter.
+ */
+#define _LIB_PARAM_NAME_SET(name, value) \
+ static const \
+ _LIB_PARAM_SECTION_ADD( \
+ _LIB_PARAM_SECTION_NAME( \
+ UK_LIBPARAM_PREFIX, \
+ PARAM_NAME_SUFFIX), \
+ char \
+ ) \
+ char _LIB_VARNAME_SET(PARAM_NAMEVAR_PREFIX, name)[] = \
+ __STRINGIFY(value)
+
+
+/**
+ * Initialize the parameter structure.
+ *
+ * @param param_name
+ * The name of the parameter
+ * @param type
+ * The type of the parameter
+ * @param cnt
+ * The number of the elements of that type.
+ */
+#define _LIB_UK_PARAM_SET(param_name, type, cnt) \
+ static const \
+ _LIB_PARAM_SECTION_ADD( \
+ _LIB_PARAM_SECTION_NAME( \
+ UK_LIBPARAM_PREFIX, \
+ PARAM_SECTION_SUFFIX), \
+ void * \
+ ) \
+ struct uk_param _LIB_VARNAME_SET(PARAM_SECTION_SUFFIX, \
+ param_name) = { \
+ .name = _LIB_VARNAME_SET(PARAM_NAMEVAR_PREFIX, \
+ param_name), \
+ .param_type = _LIB_PARAM_##type, \
+ .param_size = cnt, \
+ .addr = (__uptr) ¶m_name, \
+ }
+
+/**
+ * Declare a library param.
+ * @param name
+ * The name of the library param.
+ * @param type
+ * The type of the param.
+ */
+#define UK_LIB_PARAM(name, type) \
+ _LIB_PARAM_NAME_SET(name, _LIB_PARAM_STRING(UK_LIBPARAM_PREFIX, \
+ name)); \
+ _LIB_UK_PARAM_SET(name, type, 1) \
+
+
+#ifdef __cplusplus
+}
+#endif /* __cplusplus */
+#endif /* !__ASSEMBLY__ */
+
+#endif /* __UK_LIBPARAM_H */
diff --git a/lib/uklibparam/param.c b/lib/uklibparam/param.c
new file mode 100644
index 0000000..eb263be
--- /dev/null
+++ b/lib/uklibparam/param.c
@@ -0,0 +1,480 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
+ *
+ * Copyright (c) 2018, NEC Europe Ltd., NEC Corporation. All rights
reserved.
^^^^
2019
Agreed
+ *
+ * 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 <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <errno.h>
+#include <uk/list.h>
+#include <uk/print.h>
+#include <uk/assert.h>
+#include <uk/libparam.h>
+
+#define LIB_ARG_SEP "--"
+
+struct param_args {
+ /* Reference to the start of the library */
+ char *lib;
+ /* Reference to the start of the parameter */
+ char *param;
+ /* Reference to the start of the value */
+ char *value;
+ /* length of the library name */
+ __u8 lib_len;
+ /* length of the parameter */
+ __u8 param_len;
+ /* length of the value */
+ __u8 value_len;
+};
Is there a reason you make these variables a __u8? I understand that
parameters longer than 255 characters would be unusual, but I don't see
an advantage in limiting the values like that. Why not make them an
unsigned int or something and make use of the memory that's gonna get
used for the struct anyway (due do padding)?
The reason for __u8 was 255 is sufficient. I could make them __u32.
+
+static UK_LIST_HEAD(uk_libsections);
+
+/**
+ * Local functions
+ */
+static int kernel_arg_range_fetch(int argc, char **argv);
+static void uk_version(void);
+static void uk_usage(const char *progname);
+static int kernel_arg_fetch(char **args, int nr_args,
+ struct param_args *pargs, int *rewind);
+static int kernel_lib_fetch(struct param_args *pargs,
+ struct uk_lib_section **section);
+static int kernel_parse_arg(struct param_args *pargs,
+ struct uk_lib_section *section,
+ struct uk_param **param);
+static int kernel_arg_set(void *addr, char *value, int size, int sign);
+static int kernel_args_set(struct param_args *pargs,
+ struct uk_param *param);
+static int kernel_value_sanitize(struct param_args *pargs);
+
+void _uk_libparam_lib_add(struct uk_lib_section *lib_sec)
+{
+ uk_pr_info("libname: %s, %d\n", lib_sec->lib_name, lib_sec->len);
+ uk_list_add_tail(&lib_sec->next, &uk_libsections);
+}
+
+static void uk_version(void)
+{
+ printf("Unikraft "
+ STRINGIFY(UK_CODENAME) " "
+ STRINGIFY(UK_FULLVERSION) "\n");
+}
That function feels out of place in this library. I think it might be
better placed (maybe with a different name like uk_print_version) into
ukboot.
+
+static void uk_usage(const char *progname)
+{
+ printf("Usage: %s\n", progname);
+ printf(" [[UNIKRAFT KERNEL ARGUMENT]].. -- [[APPLICATION
ARGUMENT]]..\n\n");
+ printf("Unikraft library arguments:\n");
+ printf("The library arguments are represented as
[LIBPARAM_PREFIX].[PARAMNAME]\n\n");
+ printf(" -h, --help display this help and exit\n");
+ printf(" -V, --version display Unikraft version and
exit\n");
+}
+
+static int kernel_arg_range_fetch(int argc, char **argv)
+{
+ int i = 0;
+
+ while (i < argc) {
+ /* Separate the kernel param from the application parameters */
+ if (strcmp(LIB_ARG_SEP, argv[i]) == 0)
+ return i;
+ i++;
+ }
+
+ return -1;
+}
+
+static int kernel_arg_fetch(char **args, int nr_args,
+ struct param_args *pargs, int *rewind)
+{
+ int i = 0;
+ int rc = 0;
+ char *equals_idx;
+ int len;
+ int cnt = 0;
+
+ UK_ASSERT(rewind && pargs);
+
+ pargs->param = NULL;
+ pargs->value = NULL;
+ pargs->param_len = 0;
+ pargs->value_len = 0;
+
+ for (i = 0; (!pargs->value_len ||
+ !pargs->param_len) && i < nr_args; i++) {
+ uk_pr_err("Iteration :%d %s\n", i, args[i]);
I think this print is some leftover debug output.
Will change it.
+ len = strlen(args[i]);
+ /* Identify if there equal character is available */
I think there's a typo in this comment, because I can't understand it.
Should it say "if the equal character is present" or something like that?
agreed
+ equals_idx = strchr(args[i], '=');
+ cnt++;
+
+ if (equals_idx && (len > 1) &&
+ (equals_idx - args[i]) == (len - 1)) {
+ /* [libname_prefix].[parameter]= value */
+ if (!pargs->param) {
+ pargs->param = args[i];
+ pargs->param_len = len - 1;
+ } else {
+ uk_pr_err("Invalid char (=) in value: %s\n",
+ args[i]);
+ rc = -EINVAL;
+ goto error_exit;
+ }
+ } else if (equals_idx && len == 1)
+ /* Contains only equals */
+ continue;
+ else if (equals_idx &&
+ (equals_idx - args[i]) < (len - 1)) {
+ if (!pargs->param) {
+ uk_pr_debug("Expecting Entire Argument%s\n",
+ args[i]);
+ /* [libname_prefix].[parameter]=value */
+ pargs->param = args[i];
+ pargs->param_len = equals_idx - args[i];
+
+ pargs->value = equals_idx + 1;
+ pargs->value_len = len - (pargs->param_len + 1);
+ } else {
+ /* [libname_prefix].[parameter] =value */
+ pargs->value = equals_idx + 1;
+ pargs->value_len = len - 1;
+ }
+ } else if (!equals_idx) {
+ /* [libname_prefix].[parameter] = value */
+ if (!pargs->param) {
+ uk_pr_debug("Expecting Parameter Only %s\n",
+ args[i]);
+ pargs->param = args[i];
+ pargs->param_len = len;
+ } else {
+ uk_pr_debug("Expecting Value Only %s\n",
+ args[i]);
+ pargs->value = args[i];
+ pargs->value_len = len;
+ }
+ } else {
+ uk_pr_err("Failed to parse the argument:%s\n", args[i]);
+ rc = -EINVAL;
+ goto error_exit;
+ }
+ }
+
+ uk_pr_err("pargs->param: %p, pargs->value %p\n", pargs->param,
+ pargs->value);
This is also leftover debug output, I think.
Will fix it.
+ if (pargs->param_len != 0 && pargs->value_len == 0) {
+ uk_pr_err("No value provided for arg: %s\n", pargs->param);
+ rc = -EINVAL;
+ goto error_exit;
+ }
+
+error_exit:
+ *rewind = cnt;
+ return rc;
+}
In general, kernel_arg_fetch needs a bit of reworking. Some of the debug
messages are misformatted (like the spacing in "Iteration :%d"), and
there is a mix of sub-branches that have debug output and branches that
don't making the debug output surprisingly hard to read, because it's
easy to misjudge with branch you're in.
I will fix it.
You also invest a lot of effort into parsing differently-spaced versions
of arguments: "lib.param=value", "lib.param = value", "lib.param=
value", etc... I would've just thrown a bit fat error to the user and
told him to stop being stupid with spaces in arguments. ;-)
I would suggest keeping it, as it increase usablity
This complicated parsing also trips into a bug: if I have two parameters
in a library, and I provide the following command line, "lib.param1=
libparam2=23", then I would've expected a big fat error message or even
a crash saying "that's not a valid command line argument, you can't just
specify a parameter and then refuse to give a value". Instead, what
happens is that your code tries to recover from it, but gets confused in
the process: param1 gets the value of param2 (in this case, 23), and
param2 stays at the default value.
I will fix it. I think crashing it is a overkill for the command line
parser. I would just report warning or an error and move on.
+
+/**
+ * Kernel Parameter are passed in this format
+ * [libname_prefix].[parameter]
+ */
+static int kernel_lib_fetch(struct param_args *pargs,
+ struct uk_lib_section **section)
+{
+ char *libparam = NULL;
That initialization is unnecessary.
+ int rc = -EINVAL;
And that variable isn't needed, I think. It's only ever used to return
-EINVAL.
+ struct uk_lib_section *iter;
+
+ UK_ASSERT(section);
You might as well also ASSERT pargs here, right?I left it as it pargs is file local and it known in this context. But I
could add it.
+ pargs->lib_len = 0;
+ libparam = memchr(pargs->param, '.', pargs->param_len);
+ if (!libparam) {
+ uk_pr_err("Failed to identify the library\n");
+ rc = -EINVAL;
+ goto error_exit;
+ }
+
+ uk_list_for_each_entry(iter, &uk_libsections, next) {
+ uk_pr_debug("Lib: %s, libname: %s %ld\n", iter->lib_name,
+ pargs->param, libparam - pargs->param);
+ if ((strlen(iter->lib_name) ==
+ (size_t) (libparam - pargs->param)) &&
+ memcmp(pargs->param, iter->lib_name,
+ (libparam - pargs->param)) == 0) {
For this if clause, maybe write a short comment why both strlen and
memcmp are necessary here.
Sure.
+ *section = iter;
+ pargs->lib_len = libparam - pargs->param;
+ return 0;
+ }
+ }
+ uk_pr_err("Failed to fetch the library\n");
+
+error_exit:
+ *section = NULL;
+ pargs->lib_len = 0;
+ return rc;
+}
+
+static int kernel_parse_arg(struct param_args *pargs,
+ struct uk_lib_section *section,
+ struct uk_param **param)
+{
+ int rc = -EINVAL;
+ int i = 0;
+ struct uk_param *iter = NULL;
+ int len = 0;
There's also a bunch of unnecessary initializations here.
+
+ UK_ASSERT(section && param);
As above: you can also ASSERT pargs?
+
+ len = section->len / sizeof(struct uk_param);
+ iter = section->sec_addr_start;
+ uk_pr_debug("Section length %d section: %p, uk_param: %lu\n",
len, iter,
+ sizeof(*iter));
+
+ for (i = 0; i < len; i++, iter++) {
+ UK_ASSERT(iter->name);
+ uk_pr_debug("%s -- %p\n", &pargs->param[0], iter->name);
Again, I appreciate your dedication to provide lots of debug output, but
this could benefit from some explanation what those numbers are.
Sure
+ if ((strlen(iter->name) == pargs->param_len) &&
+ memcmp(iter->name, pargs->param, pargs->param_len) == 0) {
+ *param = iter;
+ return 0;
+ }
+ }
+
+ uk_pr_err("Failed to identify the parameter\n");
+ *param = NULL;
+ return rc;
+}
+
+static int kernel_arg_set(void *addr, char *value, int size, int sign)
+{
+ switch (size) {
+ case 1:
+ if (sign)
+ *((__s8 *)addr) = *value;
+ else
+ *((__u8 *)addr) = (__u8) strtoul(value, NULL, 10);
+
+ break;
+ case 2:
+ if (sign)
+ *((__s16 *)addr) = (__s16) strtol(value, NULL, 10);
+ else
+ *((__u16 *)addr) = (__u16) strtoul(value, NULL, 10);
+ break;
+ case 4:
+ if (sign)
+ *((__s32 *)addr) = (__s32) strtol(value, NULL, 10);
+ else
+ *((__u32 *)addr) = (__u32) strtoul(value, NULL, 10);
+ break;
+ case 8:
+ if (sign)
+ *((__s64 *)addr) = (__s64) strtoll(value, NULL, 10);
+ else
+ *((__u64 *)addr) = (__u64) strtoull(value, NULL, 10);
+ break;
+ default:
+ uk_pr_err("Cannot understand type of size %d\n", size);
+ return -EINVAL;
+
+ }
+ return 0;
+}
Maybe kernel_arg_set could benefit from overflow/underflow checking?
Check if the command line parameter fits into the data type, and if not,
spew out a warning. basically, checking the parsed parameter against
INT8_MIN, INT8_MAX, or appropriately.
I will add it.
+
+static int kernel_args_set(struct param_args *pargs,
+ struct uk_param *param)
+{
+ int rc = 0;
+ int sign = (param->param_type >> PARAM_SIGN_SHIFT) &
PARAM_SIGN_MASK;
+ int scopy = (param->param_type >> PARAM_SCOPY_SHIFT) &
PARAM_SCOPY_MASK;
+ int param_type = (param->param_type >> PARAM_SIZE_SHIFT)
+ & PARAM_SIZE_MASK;
+
+ if (scopy == 1)
+ /* Reference the pointer instead of copying the value */
+ *((__uptr *)param->addr) = (__uptr) pargs->value;
+ else {
+ if (param->param_size == 1) {
+ rc = kernel_arg_set((void *)param->addr,
+ pargs->value, param_type, sign);
+ } else {
+ uk_pr_err("Error: Cannot find the parameter\n");
+ rc = -EINVAL;
+ }
+ }
+
+ return rc;
+}
+
+/**
+ * The function removes parse for quotes around the value.
+ */
+static int kernel_value_sanitize(struct param_args *pargs)
+{
+ int rc = 0;
+ char *ptr;
+ char *start_idx = NULL;
+ char *end_idx = NULL;
+ int remove_space = 1;
+
+ UK_ASSERT(pargs && pargs->value);
+ ptr = pargs->value;
+ uk_pr_info("Cleaning value%s--%d\n", pargs->value,
pargs->value_len);
This might be clearer if it says "Sanitizing parameter string \"%s\"
(length %d)\n".
Sure I will add it.
+
+ while (*ptr != '\0') {
+ switch (*ptr) {
+ case '\0':
+ goto exit;
This case can't happen, can it?
Yeah. I will fix it.
+ case ' ':
+ case '\r':
+ case '\n':
+ case '\t':
+ case '\v':
+ if (remove_space) {
+ *ptr = '\0';
+ pargs->value_len--;
+ }
+ ptr++;
+ break;
+ case'\'':
+ case '"':
+ if (start_idx) {
+ remove_space = 1;
+ end_idx = ptr;
+ } else if (!(start_idx)) {
+ remove_space = 0;
+ start_idx = ptr;
+ }
+ ptr++;
+ break;
+ default:
+ remove_space = 0;
+ ptr++;
+ break;
+
+ }
+ }
+
+ uk_pr_debug("Value Start: %p Value End: %p\n", start_idx, end_idx);
+
+ if (start_idx && !end_idx) {
+ uk_pr_err("Malformed value string: %s\n", pargs->value);
+ rc = -EINVAL;
+ } else if (start_idx && end_idx &&
+ ((pargs->value_len - 2) != (end_idx - (start_idx + 1)))) {
+ /* Removing the open and close quote from the value string */
+ uk_pr_err("Value string(%s) not quoted properly %d: %ld\n",
+ pargs->value, pargs->value_len, ((end_idx -
+ start_idx)));
Explaining what %d and %ld are in this error message would help a user
understand the issue.
Sure
+ rc = -EINVAL;
+ } else if (start_idx && end_idx) {
+ pargs->value = start_idx + 1;
+ uk_pr_debug("Removing quotes from value %s", pargs->value);
+ *start_idx = '\0';
+ *end_idx = '\0';
+ pargs->value_len = end_idx - pargs->value;
+ uk_pr_debug("to: %s\n", pargs->value);
This output looks strange. For my test of lib.param='"3 4 1 2"', it gave
me the following output: 'Removing quotes from value 3 4 1 2"to: 3 4 1
2'. Note the single occurrence of a double quote and the missing space.
You might need to set pargs->value to start_idx first and then increment
it after the debug output.
I will fix it.
+ }
+
+exit:
+ return rc;
+}
+
+int uk_libparam_parse(const char *progname, int argc, char **argv)
+{
+ int keindex = 0;
+ int rc = 0, cnt = 0, args_read;
+ struct param_args pargs = {0};
+ struct uk_lib_section *section = NULL;
+ struct uk_param *param = NULL;
+
+ keindex = kernel_arg_range_fetch(argc, argv);
+ if (keindex < 0) {
+ uk_pr_info("No kernel argument found\n");
+ return 0;
+ }
+
+ uk_pr_debug("kernel argument end %d\n", keindex);
+
+ while (cnt < keindex) {
+
+ /* Parsing the help and version */
+ if (strcmp(argv[cnt], "-h") == 0 ||
+ strcmp(argv[cnt], "--help") == 0) {
+ uk_usage(progname);
+ ukplat_halt();
+ } else if (strcmp(argv[cnt], "-V") == 0 ||
+ strcmp(argv[cnt], "--version") == 0) {
+ uk_version();
+ ukplat_halt();
+ }
+
+ args_read = 0;
+ /* Fetch the argument from the input */
+ rc = kernel_arg_fetch(&argv[cnt], keindex, &pargs, &args_read);
+ if (rc < 0) {
+ uk_pr_err("Failed to fetch arg between index %d and %d\n",
+ cnt, (cnt + args_read));
+ cnt += args_read;
+ continue;
+ }
+ uk_pr_debug("kernel argument %s\n", pargs.param);
+ cnt += args_read;
+
+ /* Fetch library for the argument */
+ rc = kernel_lib_fetch(&pargs, §ion);
+ if (rc < 0 || !section) {
+ uk_pr_err("Failed to identify the library\n");
+ continue;
+ }
+
+ /* Fetch the parameter for the argument */
+ rc = kernel_parse_arg(&pargs, section, ¶m);
+ if (rc < 0 || !param) {
+ uk_pr_err("Failed to parse arg\n");
+ continue;
+ }
+
+ rc = kernel_value_sanitize(&pargs);
+ if (rc < 0) {
+ uk_pr_err("Failed to sanitize %s param\n", pargs.param);
+ continue;
+ }
+
+ rc = kernel_args_set(&pargs, param);
+ uk_pr_info("Parsed %d args\n", cnt);
+ }
+
+ /* Replacing the -- with progname */
+ argv[keindex] = DECONST(char *, progname);
+
+ return keindex + 1;
+}
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
