Re: [Xen-devel] [PATCH v1 04/11] xen-xsplice: Tool to manipulate xsplice payloads.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

..snip..

Probably need this:

/* This value was choosen adhoc. It could be 42 too. */

> +#define MAX_LEN 11
> +static int list_func(int argc, char *argv[])
> +{

..snip..
> +

May be worth having an comment:
/* These MUST match to the 'action_options[]' array. 
> +enum {
> +    ACTION_APPLY = 0,
> +    ACTION_REVERT = 1,
> +    ACTION_UNLOAD = 2,
> +    ACTION_CHECK = 3

And:
        ACTION_REPLACE = 4,

> +};
> +
> +struct {
> +    int allow; /* State it must be in to call function. */
> +    int expected; /* The state to be in after the function. */
> +    const char *name;
> +    int (*function)(xc_interface *xch, char *id);
> +    unsigned int executed; /* Has the function been called?. */
> +} action_options[] = {
> +    {   .allow = XSPLICE_STATE_CHECKED,
> +        .expected = XSPLICE_STATE_APPLIED,
> +        .name = "apply",
> +        .function = xc_xsplice_apply,
> +    },
> +    {   .allow = XSPLICE_STATE_APPLIED,
> +        .expected = XSPLICE_STATE_CHECKED,
> +        .name = "revert",
> +        .function = xc_xsplice_revert,
> +    },
> +    {   .allow = XSPLICE_STATE_CHECKED | XSPLICE_STATE_LOADED,
> +        .expected = -ENOENT,
> +        .name = "unload",
> +        .function = xc_xsplice_unload,
> +    },
> +    {   .allow = XSPLICE_STATE_CHECKED | XSPLICE_STATE_LOADED,
> +        .expected = XSPLICE_STATE_CHECKED,
> +        .name = "check",
> +        .function = xc_xsplice_check
> +    },
> +    {   .allow = XSPLICE_STATE_CHECKED,
> +        .expected = XSPLICE_STATE_APPLIED,
> +        .name = "replace",
> +        .function = xc_xsplice_replace,
> +    },
> +};
> +

May want to have a comment saying what the delay is in human
values. Minutes? Seconds? Days?

> +#define RETRIES 300
> +#define DELAY 100000
> +

We may want to add a comment:

/* There are functions in action_options that are called in case
 * none of these match. */

> +struct {
> +    const char *name;
> +    int (*function)(int argc, char *argv[]);
> +} main_options[] = {
> +    { "help", help_func },
> +    { "list", list_func },
> +    { "upload", upload_func },
> +    { "all", all_func },

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel