Re: ldomctl: Move code into hv_config(), defer to commands needing it
> Date: Tue, 31 Dec 2019 03:26:13 +0100
> From: Klemens Nanni
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
>
> This moves setup code from main() into its own function so instead of
> upfront it can be used only when and where needed.
>
> With the exception of `create-vdisk' all currently open /dev/hvctl; for
> that command I added a rather quirky goto to avoid this unneeded step,
> but `list-io' for example does not need /dev/hvctl at all either.
>
> So instead of adding more quirks, split as per above and clearly call
> hv_config() from the commands that *do* require it.
>
> This also effectively defers such privileged operations after all argv[]
> parsing is done, that is the code fails earlier on invalid input without
> file I/O for nothing.
>
> With that in, I can easily add more commands not requiring hvctl access,
> e.g. a dry-run configuration check.
>
> OK?
ok kettenis@
> Index: ldomctl.c
> ===
> RCS file: /cvs/src/usr.sbin/ldomctl/ldomctl.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 ldomctl.c
> --- ldomctl.c 28 Dec 2019 18:36:02 - 1.31
> +++ ldomctl.c 31 Dec 2019 02:15:14 -
> @@ -83,6 +83,7 @@ struct command commands[] = {
>
> void hv_open(void);
> void hv_close(void);
> +void hv_config(void);
> void hv_read(uint64_t, void *, size_t);
> void hv_write(uint64_t, void *, size_t);
>
> @@ -103,11 +104,6 @@ int
> main(int argc, char **argv)
> {
> struct command *cmdp;
> - struct hvctl_msg msg;
> - ssize_t nbytes;
> - struct md_header hdr;
> - struct md_node *node;
> - struct md_prop *prop;
>
> if (argc < 2)
> usage();
> @@ -122,46 +118,6 @@ main(int argc, char **argv)
> if (cmdp->cmd_name == NULL)
> usage();
>
> - if (strcmp(argv[0], "create-vdisk") == 0)
> - goto skip_hv;
> -
> - hv_open();
> -
> - /*
> - * Request config.
> - */
> - bzero(&msg, sizeof(msg));
> - msg.hdr.op = HVCTL_OP_GET_HVCONFIG;
> - msg.hdr.seq = hvctl_seq++;
> - nbytes = write(hvctl_fd, &msg, sizeof(msg));
> - if (nbytes != sizeof(msg))
> - err(1, "write");
> -
> - bzero(&msg, sizeof(msg));
> - nbytes = read(hvctl_fd, &msg, sizeof(msg));
> - if (nbytes != sizeof(msg))
> - err(1, "read");
> -
> - hv_membase = msg.msg.hvcnf.hv_membase;
> - hv_memsize = msg.msg.hvcnf.hv_memsize;
> -
> - hv_mdpa = msg.msg.hvcnf.hvmdp;
> - hv_read(hv_mdpa, &hdr, sizeof(hdr));
> - hvmd_len = sizeof(hdr) + hdr.node_blk_sz + hdr.name_blk_sz +
> - hdr.data_blk_sz;
> - hvmd_buf = xmalloc(hvmd_len);
> - hv_read(hv_mdpa, hvmd_buf, hvmd_len);
> -
> - hvmd = md_ingest(hvmd_buf, hvmd_len);
> - node = md_find_node(hvmd, "guests");
> - TAILQ_INIT(&guest_list);
> - TAILQ_FOREACH(prop, &node->prop_list, link) {
> - if (prop->tag == MD_PROP_ARC &&
> - strcmp(prop->name->str, "fwd") == 0)
> - add_guest(prop->d.arc.node);
> - }
> -
> -skip_hv:
> (cmdp->cmd_func)(argc, argv);
>
> exit(EXIT_SUCCESS);
> @@ -288,6 +244,8 @@ init_system(int argc, char **argv)
> if (argc != 2)
> usage();
>
> + hv_config();
> +
> build_config(argv[1]);
> }
>
> @@ -300,6 +258,8 @@ list(int argc, char **argv)
> if (argc != 1)
> usage();
>
> + hv_config();
> +
> dc = ds_conn_open("/dev/spds", NULL);
> mdstore_register(dc);
> while (TAILQ_EMPTY(&mdstore_sets))
> @@ -332,6 +292,8 @@ xselect(int argc, char **argv)
> if (argc != 2)
> usage();
>
> + hv_config();
> +
> dc = ds_conn_open("/dev/spds", NULL);
> mdstore_register(dc);
> while (TAILQ_EMPTY(&mdstore_sets))
> @@ -351,6 +313,8 @@ delete(int argc, char **argv)
> if (strcmp(argv[1], "factory-default") == 0)
> errx(1, "\"%s\" should not be deleted", argv[1]);
>
> + hv_config();
> +
> dc = ds_conn_open("/dev/spds", NULL);
> mdstore_register(dc);
> while (TAILQ_EMPTY(&mdstore_sets))
> @@ -409,6 +373,8 @@ download(int argc, char **argv)
> if (argc != 2)
> usage();
>
> + hv_config();
> +
> dc = ds_conn_open("/dev/spds", NULL);
> mdstore_register(dc);
> while (TAILQ_EMPTY(&mdstore_sets))
> @@ -426,6 +392,8 @@ guest_start(int argc, char **argv)
> if (argc != 2)
> usage();
>
> + hv_config();
> +
> /*
>* Start guest domain.
>*/
> @@ -452,6 +420,8 @@ guest_stop(int argc, char **argv)
> if (argc != 2)
> usage();
>
> + hv_config();
> +
> /*
>* Stop guest domain.
>*/
> @@ -478,6 +448,8 @@ guest_panic(int argc, char **argv)
> if (argc != 2)
> usage();
>
> + hv_config();
> +
> /*
>* Stop guest domain.
>*/
> @@
ldomctl: Move code into hv_config(), defer to commands needing it
This moves setup code from main() into its own function so instead of
upfront it can be used only when and where needed.
With the exception of `create-vdisk' all currently open /dev/hvctl; for
that command I added a rather quirky goto to avoid this unneeded step,
but `list-io' for example does not need /dev/hvctl at all either.
So instead of adding more quirks, split as per above and clearly call
hv_config() from the commands that *do* require it.
This also effectively defers such privileged operations after all argv[]
parsing is done, that is the code fails earlier on invalid input without
file I/O for nothing.
With that in, I can easily add more commands not requiring hvctl access,
e.g. a dry-run configuration check.
OK?
Index: ldomctl.c
===
RCS file: /cvs/src/usr.sbin/ldomctl/ldomctl.c,v
retrieving revision 1.31
diff -u -p -r1.31 ldomctl.c
--- ldomctl.c 28 Dec 2019 18:36:02 - 1.31
+++ ldomctl.c 31 Dec 2019 02:15:14 -
@@ -83,6 +83,7 @@ struct command commands[] = {
void hv_open(void);
void hv_close(void);
+void hv_config(void);
void hv_read(uint64_t, void *, size_t);
void hv_write(uint64_t, void *, size_t);
@@ -103,11 +104,6 @@ int
main(int argc, char **argv)
{
struct command *cmdp;
- struct hvctl_msg msg;
- ssize_t nbytes;
- struct md_header hdr;
- struct md_node *node;
- struct md_prop *prop;
if (argc < 2)
usage();
@@ -122,46 +118,6 @@ main(int argc, char **argv)
if (cmdp->cmd_name == NULL)
usage();
- if (strcmp(argv[0], "create-vdisk") == 0)
- goto skip_hv;
-
- hv_open();
-
- /*
-* Request config.
-*/
- bzero(&msg, sizeof(msg));
- msg.hdr.op = HVCTL_OP_GET_HVCONFIG;
- msg.hdr.seq = hvctl_seq++;
- nbytes = write(hvctl_fd, &msg, sizeof(msg));
- if (nbytes != sizeof(msg))
- err(1, "write");
-
- bzero(&msg, sizeof(msg));
- nbytes = read(hvctl_fd, &msg, sizeof(msg));
- if (nbytes != sizeof(msg))
- err(1, "read");
-
- hv_membase = msg.msg.hvcnf.hv_membase;
- hv_memsize = msg.msg.hvcnf.hv_memsize;
-
- hv_mdpa = msg.msg.hvcnf.hvmdp;
- hv_read(hv_mdpa, &hdr, sizeof(hdr));
- hvmd_len = sizeof(hdr) + hdr.node_blk_sz + hdr.name_blk_sz +
- hdr.data_blk_sz;
- hvmd_buf = xmalloc(hvmd_len);
- hv_read(hv_mdpa, hvmd_buf, hvmd_len);
-
- hvmd = md_ingest(hvmd_buf, hvmd_len);
- node = md_find_node(hvmd, "guests");
- TAILQ_INIT(&guest_list);
- TAILQ_FOREACH(prop, &node->prop_list, link) {
- if (prop->tag == MD_PROP_ARC &&
- strcmp(prop->name->str, "fwd") == 0)
- add_guest(prop->d.arc.node);
- }
-
-skip_hv:
(cmdp->cmd_func)(argc, argv);
exit(EXIT_SUCCESS);
@@ -288,6 +244,8 @@ init_system(int argc, char **argv)
if (argc != 2)
usage();
+ hv_config();
+
build_config(argv[1]);
}
@@ -300,6 +258,8 @@ list(int argc, char **argv)
if (argc != 1)
usage();
+ hv_config();
+
dc = ds_conn_open("/dev/spds", NULL);
mdstore_register(dc);
while (TAILQ_EMPTY(&mdstore_sets))
@@ -332,6 +292,8 @@ xselect(int argc, char **argv)
if (argc != 2)
usage();
+ hv_config();
+
dc = ds_conn_open("/dev/spds", NULL);
mdstore_register(dc);
while (TAILQ_EMPTY(&mdstore_sets))
@@ -351,6 +313,8 @@ delete(int argc, char **argv)
if (strcmp(argv[1], "factory-default") == 0)
errx(1, "\"%s\" should not be deleted", argv[1]);
+ hv_config();
+
dc = ds_conn_open("/dev/spds", NULL);
mdstore_register(dc);
while (TAILQ_EMPTY(&mdstore_sets))
@@ -409,6 +373,8 @@ download(int argc, char **argv)
if (argc != 2)
usage();
+ hv_config();
+
dc = ds_conn_open("/dev/spds", NULL);
mdstore_register(dc);
while (TAILQ_EMPTY(&mdstore_sets))
@@ -426,6 +392,8 @@ guest_start(int argc, char **argv)
if (argc != 2)
usage();
+ hv_config();
+
/*
* Start guest domain.
*/
@@ -452,6 +420,8 @@ guest_stop(int argc, char **argv)
if (argc != 2)
usage();
+ hv_config();
+
/*
* Stop guest domain.
*/
@@ -478,6 +448,8 @@ guest_panic(int argc, char **argv)
if (argc != 2)
usage();
+ hv_config();
+
/*
* Stop guest domain.
*/
@@ -513,6 +485,9 @@ guest_status(int argc, char **argv)
if (argc < 1 || argc > 2)
usage();
+
+ hv_config();
+
if (argc == 2)
gid = find_guest(argv[1]);
@@ -632,6 +607,8 @@ guest_console(int argc, char **argv)
if (argc != 2)
