Message ID | 20190916105945.93632-3-wipawel@amazon.de (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | livepatch: new features and fixes | expand |
On 9/16/19 11:59 AM, Pawel Wieczorkiewicz wrote: snip > +/* > + * Parse user provided action flags. > + * This function expects to only receive an array of input parameters being flags. > + * Expected action is specified via idx paramater (index of flag_options[]). > + */ > +static int get_flags(int argc, char *argv[], unsigned int idx, uint64_t *flags) > +{ > + int i, j; > + > + if ( !flags || idx >= ARRAY_SIZE(flag_options) ) > + return -1; > + > + *flags = 0; > + for ( i = 0; i < argc; i++ ) > + { > + for ( j = 0; j < ARRAY_SIZE(flag_options[idx]); j++ ) > + { > + if ( !flag_options[idx][j].name ) > + goto error; > + > + if ( !strcmp(flag_options[idx][j].name, argv[i]) ) > + { > + *flags |= flag_options[idx][j].flag; > + break; > + } > + } > + > + if ( j == ARRAY_SIZE(flag_options[idx]) ) > + goto error; > + } > + > + return 0; > +error: > + fprintf(stderr, "Unsupported flag: %s.\n", argv[i]); > + errno = EINVAL; > + return errno; > +} You return -1 above but +ve errno here. Please make it consistent. Also, you don't need to set errno if returning the actual error. (The error handling in this file looks fairly inconsistent anyway but let's not make it worse.) > + > /* The hypervisor timeout for the live patching operation is 30 msec, > * but it could take some time for the operation to start, so wait twice > * that period. */ > @@ -291,8 +357,9 @@ int action_func(int argc, char *argv[], unsigned int idx) > char name[XEN_LIVEPATCH_NAME_SIZE]; > int rc; > xen_livepatch_status_t status; > + uint64_t flags; > > - if ( argc != 1 ) > + if ( argc < 1 ) > { > show_help(); > return -1; > @@ -301,7 +368,10 @@ int action_func(int argc, char *argv[], unsigned int idx) > if ( idx >= ARRAY_SIZE(action_options) ) > return -1; > > - if ( get_name(argc, argv, name) ) > + if ( get_name(argc--, argv++, name) ) > + return EINVAL; > + > + if ( get_flags(argc, argv, idx, &flags) ) > return EINVAL; > > /* Check initial status. */ > @@ -332,7 +402,7 @@ int action_func(int argc, char *argv[], unsigned int idx) > if ( action_options[idx].allow & status.state ) > { > printf("%s %s... ", action_options[idx].verb, name); > - rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS); > + rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS, flags); > if ( rc ) > { > int saved_errno = errno; > @@ -394,17 +464,23 @@ int action_func(int argc, char *argv[], unsigned int idx) > > static int load_func(int argc, char *argv[]) > { > - int rc; > - char *new_argv[2]; > - char *path, *name, *lastdot; > + int i, rc = ENOMEM; > + char *upload_argv[2]; > + char **apply_argv, *path, *name, *lastdot; > > - if ( argc != 1 ) > + if ( argc < 1 ) > { > show_help(); > return -1; > } > + > + /* apply action has <id> [flags] input requirement, which must be constructed */ > + apply_argv = (char **) malloc(argc * sizeof(*apply_argv)); > + if ( !apply_argv ) > + return rc; > + > /* <file> */ > - new_argv[1] = argv[0]; > + upload_argv[1] = argv[0]; > > /* Synthesize the <id> */ > path = strdup(argv[0]); > @@ -413,16 +489,23 @@ static int load_func(int argc, char *argv[]) > lastdot = strrchr(name, '.'); > if ( lastdot != NULL ) > *lastdot = '\0'; > - new_argv[0] = name; > + upload_argv[0] = name; > + apply_argv[0] = name; > > - rc = upload_func(2 /* <id> <file> */, new_argv); > + /* Fill in all user provided flags */ > + for ( i = 0; i < argc - 1; i++ ) > + apply_argv[i + 1] = argv[i + 1]; Wouldn't this make the loop body simpler? i = 1; i < argc; Or alternatively, just a straight memcpy().
On 16. Sep 2019, at 19:01, Ross Lagerwall <ross.lagerwall@citrix.com<mailto:ross.lagerwall@citrix.com>> wrote:
On 9/16/19 11:59 AM, Pawel Wieczorkiewicz wrote:
snip
+/*
+ * Parse user provided action flags.
+ * This function expects to only receive an array of input parameters being flags.
+ * Expected action is specified via idx paramater (index of flag_options[]).
+ */
+static int get_flags(int argc, char *argv[], unsigned int idx, uint64_t *flags)
+{
+ int i, j;
+
+ if ( !flags || idx >= ARRAY_SIZE(flag_options) )
+ return -1;
+
+ *flags = 0;
+ for ( i = 0; i < argc; i++ )
+ {
+ for ( j = 0; j < ARRAY_SIZE(flag_options[idx]); j++ )
+ {
+ if ( !flag_options[idx][j].name )
+ goto error;
+
+ if ( !strcmp(flag_options[idx][j].name, argv[i]) )
+ {
+ *flags |= flag_options[idx][j].flag;
+ break;
+ }
+ }
+
+ if ( j == ARRAY_SIZE(flag_options[idx]) )
+ goto error;
+ }
+
+ return 0;
+error:
+ fprintf(stderr, "Unsupported flag: %s.\n", argv[i]);
+ errno = EINVAL;
+ return errno;
+}
You return -1 above but +ve errno here. Please make it consistent.
Well, I understood from the code of the file (e.g. action_func()) that the -1 value indicates a unexpected runtime error (negative val).
Whereas, positive errno values are expected error to be dealt with.
So:
<0 - fatal errors
0 - ok
>0 - errors to be handled
Could you confirm please that I should make get_flags() return only positive errors?
Also, you don't need to set errno if returning the actual error.
Honestly, I just copied the code from get_name() and wanted to the get_flags() to follow similar pattern.
(The error handling in this file looks fairly inconsistent anyway but let's not make it worse.)
+
/* The hypervisor timeout for the live patching operation is 30 msec,
* but it could take some time for the operation to start, so wait twice
* that period. */
@@ -291,8 +357,9 @@ int action_func(int argc, char *argv[], unsigned int idx)
char name[XEN_LIVEPATCH_NAME_SIZE];
int rc;
xen_livepatch_status_t status;
+ uint64_t flags;
- if ( argc != 1 )
+ if ( argc < 1 )
{
show_help();
return -1;
@@ -301,7 +368,10 @@ int action_func(int argc, char *argv[], unsigned int idx)
if ( idx >= ARRAY_SIZE(action_options) )
return -1;
- if ( get_name(argc, argv, name) )
+ if ( get_name(argc--, argv++, name) )
+ return EINVAL;
+
+ if ( get_flags(argc, argv, idx, &flags) )
return EINVAL;
/* Check initial status. */
@@ -332,7 +402,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
if ( action_options[idx].allow & status.state )
{
printf("%s %s... ", action_options[idx].verb, name);
- rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS);
+ rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS, flags);
if ( rc )
{
int saved_errno = errno;
@@ -394,17 +464,23 @@ int action_func(int argc, char *argv[], unsigned int idx)
static int load_func(int argc, char *argv[])
{
- int rc;
- char *new_argv[2];
- char *path, *name, *lastdot;
+ int i, rc = ENOMEM;
+ char *upload_argv[2];
+ char **apply_argv, *path, *name, *lastdot;
- if ( argc != 1 )
+ if ( argc < 1 )
{
show_help();
return -1;
}
+
+ /* apply action has <id> [flags] input requirement, which must be constructed */
+ apply_argv = (char **) malloc(argc * sizeof(*apply_argv));
+ if ( !apply_argv )
+ return rc;
+
/* <file> */
- new_argv[1] = argv[0];
+ upload_argv[1] = argv[0];
/* Synthesize the <id> */
path = strdup(argv[0]);
@@ -413,16 +489,23 @@ static int load_func(int argc, char *argv[])
lastdot = strrchr(name, '.');
if ( lastdot != NULL )
*lastdot = '\0';
- new_argv[0] = name;
+ upload_argv[0] = name;
+ apply_argv[0] = name;
- rc = upload_func(2 /* <id> <file> */, new_argv);
+ /* Fill in all user provided flags */
+ for ( i = 0; i < argc - 1; i++ )
+ apply_argv[i + 1] = argv[i + 1];
Wouldn't this make the loop body simpler? i = 1; i < argc;
ACK. It would indeed.
Or alternatively, just a straight memcpy().
--
Ross Lagerwall
Best Regards,
Pawel Wieczorkiewicz
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
On 9/17/19 9:27 AM, Wieczorkiewicz, Pawel wrote: > > >> On 16. Sep 2019, at 19:01, Ross Lagerwall <ross.lagerwall@citrix.com >> <mailto:ross.lagerwall@citrix.com>> wrote: >> >> On 9/16/19 11:59 AM, Pawel Wieczorkiewicz wrote: >> snip >>> +/* >>> + * Parse user provided action flags. >>> + * This function expects to only receive an array of input >>> parameters being flags. >>> + * Expected action is specified via idx paramater (index of >>> flag_options[]). >>> + */ >>> +static int get_flags(int argc, char *argv[], unsigned int idx, >>> uint64_t *flags) >>> +{ >>> + int i, j; >>> + >>> + if ( !flags || idx >= ARRAY_SIZE(flag_options) ) >>> + return -1; >>> + >>> + *flags = 0; >>> + for ( i = 0; i < argc; i++ ) >>> + { >>> + for ( j = 0; j < ARRAY_SIZE(flag_options[idx]); j++ ) >>> + { >>> + if ( !flag_options[idx][j].name ) >>> + goto error; >>> + >>> + if ( !strcmp(flag_options[idx][j].name, argv[i]) ) >>> + { >>> + *flags |= flag_options[idx][j].flag; >>> + break; >>> + } >>> + } >>> + >>> + if ( j == ARRAY_SIZE(flag_options[idx]) ) >>> + goto error; >>> + } >>> + >>> + return 0; >>> +error: >>> + fprintf(stderr, "Unsupported flag: %s.\n", argv[i]); >>> + errno = EINVAL; >>> + return errno; >>> +} >> >> You return -1 above but +ve errno here. Please make it consistent. > > Well, I understood from the code of the file (e.g. action_func()) that > the -1 value indicates a unexpected runtime error (negative val). > Whereas, positive errno values are expected error to be dealt with. > > So: > <0 - fatal errors > 0 - ok > >0 - errors to be handled > > Could you confirm please that I should make get_flags() return only > positive errors? From what I can see, the only positive errors that are "handled" are EAGAIN and EBUSY to report EXIT_TIMEOUT and the mixture of returning -1 and positive errno is a bug. But it's fine to leave it as is for now so it. > >> Also, you don't need to set errno if returning the actual error. >> > > Honestly, I just copied the code from get_name() and wanted to the > get_flags() to follow similar pattern. Sure.
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index c92386aab8..2fc62422f5 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -2598,11 +2598,12 @@ int xc_livepatch_list(xc_interface *xch, unsigned int max, unsigned int start, * to complete them. The `timeout` offers an option to expire the * operation if it could not be completed within the specified time * (in ns). Value of 0 means let hypervisor decide the best timeout. + * The `flags` allows to pass extra parameters to the actions. */ -int xc_livepatch_apply(xc_interface *xch, char *name, uint32_t timeout); -int xc_livepatch_revert(xc_interface *xch, char *name, uint32_t timeout); -int xc_livepatch_unload(xc_interface *xch, char *name, uint32_t timeout); -int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout); +int xc_livepatch_apply(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags); +int xc_livepatch_revert(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags); +int xc_livepatch_unload(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags); +int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags); /* * Ensure cache coherency after memory modifications. A call to this function diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c index 8e60b6e9f0..a8e9e7d1e2 100644 --- a/tools/libxc/xc_misc.c +++ b/tools/libxc/xc_misc.c @@ -854,7 +854,8 @@ int xc_livepatch_list(xc_interface *xch, unsigned int max, unsigned int start, static int _xc_livepatch_action(xc_interface *xch, char *name, unsigned int action, - uint32_t timeout) + uint32_t timeout, + uint64_t flags) { int rc; DECLARE_SYSCTL; @@ -880,6 +881,7 @@ static int _xc_livepatch_action(xc_interface *xch, sysctl.u.livepatch.pad = 0; sysctl.u.livepatch.u.action.cmd = action; sysctl.u.livepatch.u.action.timeout = timeout; + sysctl.u.livepatch.u.action.flags = flags; sysctl.u.livepatch.u.action.name = def_name; set_xen_guest_handle(sysctl.u.livepatch.u.action.name.name, name); @@ -891,24 +893,24 @@ static int _xc_livepatch_action(xc_interface *xch, return rc; } -int xc_livepatch_apply(xc_interface *xch, char *name, uint32_t timeout) +int xc_livepatch_apply(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags) { - return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_APPLY, timeout); + return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_APPLY, timeout, flags); } -int xc_livepatch_revert(xc_interface *xch, char *name, uint32_t timeout) +int xc_livepatch_revert(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags) { - return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REVERT, timeout); + return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REVERT, timeout, flags); } -int xc_livepatch_unload(xc_interface *xch, char *name, uint32_t timeout) +int xc_livepatch_unload(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags) { - return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_UNLOAD, timeout); + return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_UNLOAD, timeout, flags); } -int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout) +int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags) { - return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REPLACE, timeout); + return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REPLACE, timeout, flags); } /* diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c index 3233472157..a37b2457ff 100644 --- a/tools/misc/xen-livepatch.c +++ b/tools/misc/xen-livepatch.c @@ -23,18 +23,23 @@ void show_help(void) { fprintf(stderr, "xen-livepatch: live patching tool\n" - "Usage: xen-livepatch <command> [args]\n" + "Usage: xen-livepatch <command> [args] [command-flags]\n" " <name> An unique name of payload. Up to %d characters.\n" "Commands:\n" " help display this help\n" " upload <name> <file> upload file <file> with <name> name\n" " list list payloads uploaded.\n" - " apply <name> apply <name> patch.\n" + " apply <name> [flags] apply <name> patch.\n" + " Supported flags:\n" + " --nodeps Disable inter-module buildid dependency check.\n" + " Check only against hypervisor buildid.\n" " revert <name> revert name <name> patch.\n" " replace <name> apply <name> patch and revert all others.\n" " unload <name> unload name <name> patch.\n" - " load <file> upload and apply <file>.\n" - " name is the <file> name\n", + " load <file> [flags] upload and apply <file> with name as the <file> name\n" + " Supported flags:\n" + " --nodeps Disable inter-module buildid dependency check.\n" + " Check only against hypervisor buildid.\n", XEN_LIVEPATCH_NAME_SIZE); } @@ -225,12 +230,13 @@ static int upload_func(int argc, char *argv[]) return rc; } -/* These MUST match to the 'action_options[]' array slots. */ +/* These MUST match to the 'action_options[]' and 'flag_options[]' array slots. */ enum { ACTION_APPLY = 0, ACTION_REVERT = 1, ACTION_UNLOAD = 2, ACTION_REPLACE = 3, + ACTION_NUM }; struct { @@ -238,7 +244,7 @@ struct { int expected; /* The state to be in after the function. */ const char *name; const char *verb; - int (*function)(xc_interface *xch, char *name, uint32_t timeout); + int (*function)(xc_interface *xch, char *name, uint32_t timeout, uint64_t flags); } action_options[] = { { .allow = LIVEPATCH_STATE_CHECKED, .expected = LIVEPATCH_STATE_APPLIED, @@ -266,6 +272,66 @@ struct { }, }; +/* + * This structure defines supported flag options for actions. + * It defines entries for each action and supports up to 64 + * flags per action. + */ +struct { + const char *name; + const uint64_t flag; +} flag_options[ACTION_NUM][8 * sizeof(uint64_t)] = { + { /* ACTION_APPLY */ + { .name = "--nodeps", + .flag = LIVEPATCH_ACTION_APPLY_NODEPS, + }, + }, + { /* ACTION_REVERT */ + }, + { /* ACTION_UNLOAD */ + }, + { /* ACTION_REPLACE */ + } +}; + +/* + * Parse user provided action flags. + * This function expects to only receive an array of input parameters being flags. + * Expected action is specified via idx paramater (index of flag_options[]). + */ +static int get_flags(int argc, char *argv[], unsigned int idx, uint64_t *flags) +{ + int i, j; + + if ( !flags || idx >= ARRAY_SIZE(flag_options) ) + return -1; + + *flags = 0; + for ( i = 0; i < argc; i++ ) + { + for ( j = 0; j < ARRAY_SIZE(flag_options[idx]); j++ ) + { + if ( !flag_options[idx][j].name ) + goto error; + + if ( !strcmp(flag_options[idx][j].name, argv[i]) ) + { + *flags |= flag_options[idx][j].flag; + break; + } + } + + if ( j == ARRAY_SIZE(flag_options[idx]) ) + goto error; + } + + return 0; +error: + fprintf(stderr, "Unsupported flag: %s.\n", argv[i]); + errno = EINVAL; + return errno; +} + /* The hypervisor timeout for the live patching operation is 30 msec, * but it could take some time for the operation to start, so wait twice * that period. */ @@ -291,8 +357,9 @@ int action_func(int argc, char *argv[], unsigned int idx) char name[XEN_LIVEPATCH_NAME_SIZE]; int rc; xen_livepatch_status_t status; + uint64_t flags; - if ( argc != 1 ) + if ( argc < 1 ) { show_help(); return -1; @@ -301,7 +368,10 @@ int action_func(int argc, char *argv[], unsigned int idx) if ( idx >= ARRAY_SIZE(action_options) ) return -1; - if ( get_name(argc, argv, name) ) + if ( get_name(argc--, argv++, name) ) + return EINVAL; + + if ( get_flags(argc, argv, idx, &flags) ) return EINVAL; /* Check initial status. */ @@ -332,7 +402,7 @@ int action_func(int argc, char *argv[], unsigned int idx) if ( action_options[idx].allow & status.state ) { printf("%s %s... ", action_options[idx].verb, name); - rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS); + rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS, flags); if ( rc ) { int saved_errno = errno; @@ -394,17 +464,23 @@ int action_func(int argc, char *argv[], unsigned int idx) static int load_func(int argc, char *argv[]) { - int rc; - char *new_argv[2]; - char *path, *name, *lastdot; + int i, rc = ENOMEM; + char *upload_argv[2]; + char **apply_argv, *path, *name, *lastdot; - if ( argc != 1 ) + if ( argc < 1 ) { show_help(); return -1; } + + /* apply action has <id> [flags] input requirement, which must be constructed */ + apply_argv = (char **) malloc(argc * sizeof(*apply_argv)); + if ( !apply_argv ) + return rc; + /* <file> */ - new_argv[1] = argv[0]; + upload_argv[1] = argv[0]; /* Synthesize the <id> */ path = strdup(argv[0]); @@ -413,16 +489,23 @@ static int load_func(int argc, char *argv[]) lastdot = strrchr(name, '.'); if ( lastdot != NULL ) *lastdot = '\0'; - new_argv[0] = name; + upload_argv[0] = name; + apply_argv[0] = name; - rc = upload_func(2 /* <id> <file> */, new_argv); + /* Fill in all user provided flags */ + for ( i = 0; i < argc - 1; i++ ) + apply_argv[i + 1] = argv[i + 1]; + + rc = upload_func(2 /* <id> <file> */, upload_argv); if ( rc ) - return rc; + goto error; - rc = action_func(1 /* only <id> */, new_argv, ACTION_APPLY); + rc = action_func(argc, apply_argv, ACTION_APPLY); if ( rc ) - action_func(1, new_argv, ACTION_UNLOAD); + action_func(1 /* only <id> */, upload_argv, ACTION_UNLOAD); +error: + free(apply_argv); free(path); return rc; } diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index ef081f112c..c5655a43d2 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -1575,9 +1575,17 @@ static int livepatch_action(struct xen_sysctl_livepatch_action *action) break; } - rc = build_id_dep(data, !!list_empty(&applied_list)); - if ( rc ) - break; + /* + * Check if action is issued with nodeps flags to ignore module + * stack dependencies. + */ + if ( !(action->flags & LIVEPATCH_ACTION_APPLY_NODEPS) ) + { + rc = build_id_dep(data, !!list_empty(&applied_list)); + if ( rc ) + break; + } + data->rc = -EAGAIN; rc = schedule_work(data, action->cmd, action->timeout); } diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index 91c48dcae0..1b2b165a6d 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -35,7 +35,7 @@ #include "domctl.h" #include "physdev.h" -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000012 +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013 /* * Read console content from Xen buffer ring. @@ -956,6 +956,15 @@ struct xen_sysctl_livepatch_action { /* hypervisor default. */ /* Or upper bound of time (ns) */ /* for operation to take. */ + +/* + * Overwrite default inter-module buildid dependency chain enforcement. + * Check only if module is built for given hypervisor by comparing buildid. + */ +#define LIVEPATCH_ACTION_APPLY_NODEPS (1 << 0) + uint64_t flags; /* IN: action flags. */ + /* Provide additional parameters */ + /* for an action. */ }; struct xen_sysctl_livepatch_op {