| Message ID | 1450444471-6454-9-git-send-email-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote: > init-xenstore-domain takes only positional parameters today. Change > this to a more flexible parameter syntax allowing to specify additional > options or to omit some. > > Today the supported usage is: > > init-xenstore-domain <xenstore-kernel> <memory_mb> <flask-label> > [<ramdisk-file>] > > Modify this to: > > init-xenstore-domain --kernel <xenstore-kernel> > --memory <memory_mb> > [--flask <flask-label>] > [--ramdisk <ramdisk-file>] > > The flask label is made optional in order to support xenstore domains > without the need of a flask enabled hypervisor. > > Signed-off-by: Juergen Gross <jgross@suse.com> I think Daniel and I both acked this patch in v1 (as #5/9). Is this version different? If so please include an intra-version changelog after the "---" to help us out.
On 06/01/16 17:21, Ian Campbell wrote: > On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote: >> init-xenstore-domain takes only positional parameters today. Change >> this to a more flexible parameter syntax allowing to specify additional >> options or to omit some. >> >> Today the supported usage is: >> >> init-xenstore-domain <xenstore-kernel> <memory_mb> <flask-label> >> [<ramdisk-file>] >> >> Modify this to: >> >> init-xenstore-domain --kernel <xenstore-kernel> >> --memory <memory_mb> >> [--flask <flask-label>] >> [--ramdisk <ramdisk-file>] >> >> The flask label is made optional in order to support xenstore domains >> without the need of a flask enabled hypervisor. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > I think Daniel and I both acked this patch in v1 (as #5/9). Is this version > different? If so please include an intra-version changelog after the "---" > to help us out. There was an error in parameter parsing (omitting kernel or memory wasn't detected). It was mentioned in the changelog in the cover letter. I'll add a note next time when I drop an Ack. Juergen
On Thu, 2016-01-07 at 07:34 +0100, Juergen Gross wrote: > On 06/01/16 17:21, Ian Campbell wrote: > > On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote: > > > init-xenstore-domain takes only positional parameters today. Change > > > this to a more flexible parameter syntax allowing to specify > > > additional > > > options or to omit some. > > > > > > Today the supported usage is: > > > > > > init-xenstore-domain <xenstore-kernel> <memory_mb> <flask-label> > > > [<ramdisk-file>] > > > > > > Modify this to: > > > > > > init-xenstore-domain --kernel <xenstore-kernel> > > > --memory <memory_mb> > > > [--flask <flask-label>] > > > [--ramdisk <ramdisk-file>] > > > > > > The flask label is made optional in order to support xenstore domains > > > without the need of a flask enabled hypervisor. > > > > > > Signed-off-by: Juergen Gross <jgross@suse.com> > > > > I think Daniel and I both acked this patch in v1 (as #5/9). Is this > > version > > different? If so please include an intra-version changelog after the "- > > --" > > to help us out. > > There was an error in parameter parsing (omitting kernel or memory > wasn't detected). It was mentioned in the changelog in the cover letter. In general we prefer these sorts of details to be mentioned in the patch themselves, after a "---" marker such that they don't get included in the actual commit. The cover letter should be more of a summary. > I'll add a note next time when I drop an Ack. Thanks. WRT that change and: > + if (optind != argc || !kernel || !memory) { > + usage(); > return 2; > } Under what circumstances can optind != argc? AFAICT it should never happen because the switch cover all valid options and returns otherwise. If it can never happen it should be an assert. Oh, is it to catch things like "--foo bar baz" i.e. additional non-option arguments (of which there should be none)? If that is the case: Acked-by: Ian Campbell <ian.campbell@citrix.com> Ian.
On 07/01/16 11:23, Ian Campbell wrote: > On Thu, 2016-01-07 at 07:34 +0100, Juergen Gross wrote: >> On 06/01/16 17:21, Ian Campbell wrote: >>> On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote: >>>> init-xenstore-domain takes only positional parameters today. Change >>>> this to a more flexible parameter syntax allowing to specify >>>> additional >>>> options or to omit some. >>>> >>>> Today the supported usage is: >>>> >>>> init-xenstore-domain <xenstore-kernel> <memory_mb> <flask-label> >>>> [<ramdisk-file>] >>>> >>>> Modify this to: >>>> >>>> init-xenstore-domain --kernel <xenstore-kernel> >>>> --memory <memory_mb> >>>> [--flask <flask-label>] >>>> [--ramdisk <ramdisk-file>] >>>> >>>> The flask label is made optional in order to support xenstore domains >>>> without the need of a flask enabled hypervisor. >>>> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> >>> I think Daniel and I both acked this patch in v1 (as #5/9). Is this >>> version >>> different? If so please include an intra-version changelog after the "- >>> --" >>> to help us out. >> >> There was an error in parameter parsing (omitting kernel or memory >> wasn't detected). It was mentioned in the changelog in the cover letter. > > In general we prefer these sorts of details to be mentioned in the patch > themselves, after a "---" marker such that they don't get included in the > actual commit. The cover letter should be more of a summary. Okay, I'll add a patch specific changelog in the future. > >> I'll add a note next time when I drop an Ack. > > Thanks. > > WRT that change and: > >> + if (optind != argc || !kernel || !memory) { >> + usage(); >> return 2; >> } > > Under what circumstances can optind != argc? AFAICT it should never happen > because the switch cover all valid options and returns otherwise. If it can > never happen it should be an assert. > > Oh, is it to catch things like "--foo bar baz" i.e. additional non-option > arguments (of which there should be none)? Exactly. > > If that is the case: Acked-by: Ian Campbell <ian.campbell@citrix.com> Thanks, Juergen
diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c index d3abec0..17f0151 100644 --- a/tools/helpers/init-xenstore-domain.c +++ b/tools/helpers/init-xenstore-domain.c @@ -4,6 +4,7 @@ #include <string.h> #include <stdint.h> #include <stdlib.h> +#include <getopt.h> #include <sys/ioctl.h> #include <sys/mman.h> #include <xenctrl.h> @@ -13,16 +14,42 @@ #include <xen-xsm/flask/flask.h> static uint32_t domid = ~0; +static char *kernel; +static char *ramdisk; +static char *flask; +static int memory; + +static struct option options[] = { + { "kernel", 1, NULL, 'k' }, + { "memory", 1, NULL, 'm' }, + { "flask", 1, NULL, 'f' }, + { "ramdisk", 1, NULL, 'r' }, + { NULL, 0, NULL, 0 } +}; + +static void usage(void) +{ + fprintf(stderr, +"Usage:\n" +"\n" +"init-xenstore-domain <options>\n" +"\n" +"where options may include:\n" +"\n" +" --kernel <xenstore-kernel> kernel file of the xenstore domain, mandatory\n" +" --memory <memory size> size of the domain in MB, mandatory\n" +" --flask <flask-label> optional flask label of the domain\n" +" --ramdisk <ramdisk-file> optional ramdisk file for the domain\n"); +} -static int build(xc_interface *xch, int argc, char** argv) +static int build(xc_interface *xch) { char cmdline[512]; uint32_t ssid; xen_domain_handle_t handle = { 0 }; int rv, xs_fd; struct xc_dom_image *dom = NULL; - int maxmem = atoi(argv[2]); - int limit_kb = (maxmem + 1)*1024; + int limit_kb = (memory + 1)*1024; xs_fd = open("/dev/xen/xenbus_backend", O_RDWR); if (xs_fd == -1) { @@ -30,10 +57,14 @@ static int build(xc_interface *xch, int argc, char** argv) return -1; } - rv = xc_flask_context_to_sid(xch, argv[3], strlen(argv[3]), &ssid); - if (rv) { - fprintf(stderr, "xc_flask_context_to_sid failed\n"); - goto err; + if (flask) { + rv = xc_flask_context_to_sid(xch, flask, strlen(flask), &ssid); + if (rv) { + fprintf(stderr, "xc_flask_context_to_sid failed\n"); + goto err; + } + } else { + ssid = SECINITSID_DOMU; } rv = xc_domain_create(xch, ssid, handle, 0, &domid, NULL); if (rv) { @@ -64,14 +95,14 @@ static int build(xc_interface *xch, int argc, char** argv) snprintf(cmdline, 512, "--event %d --internal-db", rv); dom = xc_dom_allocate(xch, cmdline, NULL); - rv = xc_dom_kernel_file(dom, argv[1]); + rv = xc_dom_kernel_file(dom, kernel); if (rv) { fprintf(stderr, "xc_dom_kernel_file failed\n"); goto err; } - if (argc > 4) { - rv = xc_dom_ramdisk_file(dom, argv[4]); + if (ramdisk) { + rv = xc_dom_ramdisk_file(dom, ramdisk); if (rv) { fprintf(stderr, "xc_dom_ramdisk_file failed\n"); goto err; @@ -88,7 +119,7 @@ static int build(xc_interface *xch, int argc, char** argv) fprintf(stderr, "xc_dom_parse_image failed\n"); goto err; } - rv = xc_dom_mem_init(dom, maxmem); + rv = xc_dom_mem_init(dom, memory); if (rv) { fprintf(stderr, "xc_dom_mem_init failed\n"); goto err; @@ -136,15 +167,34 @@ err: int main(int argc, char** argv) { + int opt; xc_interface *xch; struct xs_handle *xsh; char buf[16]; int rv, fd; - if (argc < 4 || argc > 5) { - fprintf(stderr, - "Use: %s <xenstore-kernel> <memory_mb> <flask-label> [<ramdisk-file>]\n", - argv[0]); + while ((opt = getopt_long(argc, argv, "", options, NULL)) != -1) { + switch (opt) { + case 'k': + kernel = optarg; + break; + case 'm': + memory = strtol(optarg, NULL, 10); + break; + case 'f': + flask = optarg; + break; + case 'r': + ramdisk = optarg; + break; + default: + usage(); + return 2; + } + } + + if (optind != argc || !kernel || !memory) { + usage(); return 2; } @@ -154,7 +204,7 @@ int main(int argc, char** argv) return 1; } - rv = build(xch, argc, argv); + rv = build(xch); xc_interface_close(xch);
init-xenstore-domain takes only positional parameters today. Change this to a more flexible parameter syntax allowing to specify additional options or to omit some. Today the supported usage is: init-xenstore-domain <xenstore-kernel> <memory_mb> <flask-label> [<ramdisk-file>] Modify this to: init-xenstore-domain --kernel <xenstore-kernel> --memory <memory_mb> [--flask <flask-label>] [--ramdisk <ramdisk-file>] The flask label is made optional in order to support xenstore domains without the need of a flask enabled hypervisor. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/helpers/init-xenstore-domain.c | 82 +++++++++++++++++++++++++++++------- 1 file changed, 66 insertions(+), 16 deletions(-)