From: Dave Voutila Subject: Re: make vmctl stop mucking with imsg innards To: tech@openbsd.org Cc: claudio@openbsd.org Date: Thu, 29 May 2025 07:13:17 -0400 Dave Voutila writes: ping > 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 */