From: Dave Voutila Subject: make vmctl stop mucking with imsg innards To: tech@openbsd.org Cc: claudio@openbsd.org Date: Thu, 22 May 2025 11:48:06 -0400 Similar to fixing up vmd, this stops vmctl from touching the internals of imsg objects. Took the same approach (and functions) from vmd to unmarshall imsg payloads. ok? diffstat refs/heads/master refs/heads/vmctl-imsg M usr.sbin/vmctl/main.c | 5+ 3- M usr.sbin/vmctl/vmctl.c | 71+ 29- M usr.sbin/vmctl/vmctl.h | 3+ 6- 3 files changed, 79 insertions(+), 38 deletions(-) diff refs/heads/master refs/heads/vmctl-imsg commit - 38645cd8d9647e941cedb9bed41c2480381552cc commit + 862de7e80b180fec8a7fcf1a6f1f457b84a29b2a blob - d44d385df8adf5142071e6cfed566ded1864e7d6 blob + a807ed83670116195dd7c0bdbbed9168d03ab421 --- usr.sbin/vmctl/main.c +++ usr.sbin/vmctl/main.c @@ -191,6 +191,7 @@ vmmaction(struct parse_result *res) int n; int ret, action; unsigned int flags; + uint32_t type; if (ctl_sock == -1) { if (unveil(SOCKET_NAME, "w") == -1) @@ -290,9 +291,10 @@ vmmaction(struct parse_result *res) if (n == 0) break; - if (imsg.hdr.type == IMSG_CTL_FAIL) { - if (IMSG_DATA_SIZE(&imsg) == sizeof(ret)) - memcpy(&ret, imsg.data, sizeof(ret)); + type = imsg_get_type(&imsg); + if (type == IMSG_CTL_FAIL) { + if (imsg_get_len(&imsg) == sizeof(ret)) + ret = imsg_int_read(&imsg); else ret = 0; if (ret != 0) { blob - 07ac51fe57278f96ff20d15d9e86286578d899c2 blob + 27f4a5783b6c1c4c0e9035e354cdf0a2b392d7f8 --- usr.sbin/vmctl/vmctl.c +++ usr.sbin/vmctl/vmctl.c @@ -238,12 +238,14 @@ vm_start(uint32_t start_id, const char *name, size_t m int vm_start_complete(struct imsg *imsg, int *ret, int autoconnect) { - struct vmop_result *vmr; + struct vmop_result vmr; + uint32_t type; int res; - if (imsg->hdr.type == IMSG_VMDOP_START_VM_RESPONSE) { - vmr = (struct vmop_result *)imsg->data; - res = vmr->vmr_result; + type = imsg_get_type(imsg); + if (type == IMSG_VMDOP_START_VM_RESPONSE) { + vmop_result_read(imsg, &vmr); + res = vmr.vmr_result; if (res) { switch (res) { case VMD_BIOS_MISSING: @@ -274,10 +276,10 @@ vm_start_complete(struct imsg *imsg, int *ret, int aut } } else if (autoconnect) { /* does not return */ - ctl_openconsole(vmr->vmr_ttyname); + ctl_openconsole(vmr.vmr_ttyname); } else { warnx("started vm %d successfully, tty %s", - vmr->vmr_id, vmr->vmr_ttyname); + vmr.vmr_id, vmr.vmr_ttyname); *ret = 0; } } else { @@ -383,18 +385,20 @@ pause_vm(uint32_t pause_id, const char *name) int pause_vm_complete(struct imsg *imsg, int *ret) { - struct vmop_result *vmr; + struct vmop_result vmr; + uint32_t type; int res; - if (imsg->hdr.type == IMSG_VMDOP_PAUSE_VM_RESPONSE) { - vmr = (struct vmop_result *)imsg->data; - res = vmr->vmr_result; + type = imsg_get_type(imsg); + if (type == IMSG_VMDOP_PAUSE_VM_RESPONSE) { + vmop_result_read(imsg, &vmr); + res = vmr.vmr_result; if (res) { errno = res; warn("pause vm command failed"); *ret = EIO; } else { - warnx("paused vm %d successfully", vmr->vmr_id); + warnx("paused vm %d successfully", vmr.vmr_id); *ret = 0; } } else { @@ -422,18 +426,20 @@ unpause_vm(uint32_t pause_id, const char *name) int unpause_vm_complete(struct imsg *imsg, int *ret) { - struct vmop_result *vmr; + struct vmop_result vmr; + uint32_t type; int res; - if (imsg->hdr.type == IMSG_VMDOP_UNPAUSE_VM_RESPONSE) { - vmr = (struct vmop_result *)imsg->data; - res = vmr->vmr_result; + type = imsg_get_type(imsg); + if (type == IMSG_VMDOP_UNPAUSE_VM_RESPONSE) { + vmop_result_read(imsg, &vmr); + res = vmr.vmr_result; if (res) { errno = res; warn("unpause vm command failed"); *ret = EIO; } else { - warnx("unpaused vm %d successfully", vmr->vmr_id); + warnx("unpaused vm %d successfully", vmr.vmr_id); *ret = 0; } } else { @@ -499,19 +505,20 @@ terminate_vm(uint32_t terminate_id, const char *name, int terminate_vm_complete(struct imsg *imsg, int *ret, unsigned int flags) { - struct vmop_result *vmr; + struct vmop_result vmr; + uint32_t type; int res; - switch (imsg->hdr.type) { + type = imsg_get_type(imsg); + switch (type) { case IMSG_VMDOP_TERMINATE_VM_RESPONSE: - IMSG_SIZE_CHECK(imsg, &vmr); - vmr = (struct vmop_result *)imsg->data; - res = vmr->vmr_result; + vmop_result_read(imsg, &vmr); + res = vmr.vmr_result; switch (res) { case 0: fprintf(stderr, "requested to shutdown vm %d\n", - vmr->vmr_id); + vmr.vmr_id); *ret = 0; break; case VMD_VM_STOP_INVALID: @@ -535,13 +542,12 @@ terminate_vm_complete(struct imsg *imsg, int *ret, uns } break; case IMSG_VMDOP_TERMINATE_VM_EVENT: - IMSG_SIZE_CHECK(imsg, &vmr); - vmr = (struct vmop_result *)imsg->data; + vmop_result_read(imsg, &vmr); if (flags & VMOP_WAIT) { - fprintf(stderr, "terminated vm %d\n", vmr->vmr_id); + fprintf(stderr, "terminated vm %d\n", vmr.vmr_id); } else if (flags & VMOP_FORCE) { fprintf(stderr, "forced to terminate vm %d\n", - vmr->vmr_id); + vmr.vmr_id); } *ret = 0; break; @@ -698,20 +704,22 @@ add_info(struct imsg *imsg, int *ret) { static size_t ct = 0; static struct vmop_info_result *vir = NULL; + uint32_t type; *ret = 0; - if (imsg->hdr.type == IMSG_VMDOP_GET_INFO_VM_DATA) { + type = imsg_get_type(imsg); + if (type == IMSG_VMDOP_GET_INFO_VM_DATA) { vir = reallocarray(vir, ct + 1, sizeof(struct vmop_info_result)); if (vir == NULL) { *ret = ENOMEM; return (1); } - memcpy(&vir[ct], imsg->data, sizeof(struct vmop_info_result)); + vmop_info_result_read(imsg, &vir[ct]); ct++; return (0); - } else if (imsg->hdr.type == IMSG_VMDOP_GET_INFO_VM_END_DATA) { + } else if (type == IMSG_VMDOP_GET_INFO_VM_END_DATA) { switch (info_action) { case CMD_CONSOLE: vm_console(vir, ct); @@ -987,3 +995,37 @@ create_imagefile(int type, const char *imgfile_path, c return (ret); } + +void +vmop_result_read(struct imsg *imsg, struct vmop_result *vmr) +{ + if (imsg_get_data(imsg, vmr, sizeof(*vmr))) + fatal("%s", __func__); + + vmr->vmr_ttyname[sizeof(vmr->vmr_ttyname) - 1] = '\0'; +} + +void +vmop_info_result_read(struct imsg *imsg, struct vmop_info_result *vir) +{ + struct vm_info_result *r; + + if (imsg_get_data(imsg, vir, sizeof(*vir))) + fatal("%s", __func__); + + r = &vir->vir_info; + r->vir_name[sizeof(r->vir_name) - 1] = '\0'; + + vir->vir_ttyname[sizeof(vir->vir_ttyname) - 1] = '\0'; +} + +int +imsg_int_read(struct imsg *imsg) +{ + int val; + + if (imsg_get_data(imsg, &val, sizeof(val))) + fatal("%s", __func__); + + return (val); +} blob - 13fe75bb60f561e1760892ff0f81db9aa0cbbc21 blob + 0cb06652f14c57852ad127a1f13942d69f624694 --- usr.sbin/vmctl/vmctl.h +++ usr.sbin/vmctl/vmctl.h @@ -113,11 +113,8 @@ void terminate_all(struct vmop_info_result *, size_t, __dead void vm_console(struct vmop_info_result *, size_t); -/* XXX remove once vmctl is cleaned up of imsg shenanigans. */ -#define IMSG_SIZE_CHECK(imsg, p) do { \ - if (IMSG_DATA_SIZE(imsg) < sizeof(*p)) \ - fatalx("bad length imsg received (%s)", #p); \ -} while (0) -#define IMSG_DATA_SIZE(imsg) ((imsg)->hdr.len - IMSG_HEADER_SIZE) +int imsg_int_read(struct imsg *); +void vmop_result_read(struct imsg *, struct vmop_result *); +void vmop_info_result_read(struct imsg *, struct vmop_info_result *); #endif /* VMCTL_PARSER_H */