On Thu, Jun 10, 2021 at 09:19:45AM -0400, Dave Voutila wrote:
>
> Still looking for an OK or feedback on the below. This is finishing work
> to fixes made previously to vmd(8)/vmctl(8) regarding vm
> stopping/running state corruption when using vmctl(8) to wait for a vm
> to stop.
>
Sorry for the delay. ok mlarkin@ with one comment below.
-ml
> Dave Voutila writes:
>
> > ping
> >
> > Dave Voutila writes:
> >
> >> Dave Voutila writes:
> >>
> >>> The conclusion of my previous fixes to vmd(8) [1] changes the event
> >>> handling in vmctl(8) to support receiving IMSG_VMDOP_TERMINATE_VM_EVENTs
> >>> from the control process. (This removes a XXX comment from vmd.)
> >>>
> >>> For clarity, the messaging logic was changed previously:
> >>>
> >>> - ...TERMINATE_VM_RESPONSE conveying success/failure of the request to
> >>> terminate a guest regardless of waiting for termination
> >>> - ...TERMINATE_VM_EVENT conveying the actual termination of a guest
> >>>
> >>> This diff finishes bringing that logic from vmd(8) to vmctl(8).
> >>>
> >>> OK?
> >>
> >> Ping. Looking to close this gap.
> >>
> >> Note: this diff does preserve some errno abuse in vmd & vmctl that I'm
> >> working on separately.
> >>
> >>>
> >>> -dv
> >>>
> >>>
> >>> Index: usr.sbin/vmd/control.c
> >>> ===================================================================
> >>> RCS file: /cvs/src/usr.sbin/vmd/control.c,v
> >>> retrieving revision 1.35
> >>> diff -u -p -r1.35 control.c
> >>> --- usr.sbin/vmd/control.c 26 Apr 2021 22:58:27 -0000 1.35
> >>> +++ usr.sbin/vmd/control.c 30 Apr 2021 12:31:22 -0000
> >>> @@ -154,9 +154,8 @@ control_dispatch_vmd(int fd, struct priv
> >>> if (notify->ctl_vmid != vmr.vmr_id)
> >>> continue;
> >>> if ((c = control_connbyfd(notify->ctl_fd)) != NULL) {
> >>> - /* XXX vmctl expects *_RESPONSE, not *_EVENT */
> >>> - imsg_compose_event(&c->iev,
> >>> - IMSG_VMDOP_TERMINATE_VM_RESPONSE,
> >>> + /* Forward to the vmctl(8) client */
> >>> + imsg_compose_event(&c->iev, imsg->hdr.type,
> >>> 0, 0, -1, imsg->data, IMSG_DATA_SIZE(imsg));
> >>> TAILQ_REMOVE(&ctl_notify_q, notify, entry);
> >>> free(notify);
> >>> Index: usr.sbin/vmctl/vmctl.c
> >>> ===================================================================
> >>> RCS file: /cvs/src/usr.sbin/vmctl/vmctl.c,v
> >>> retrieving revision 1.77
> >>> diff -u -p -r1.77 vmctl.c
> >>> --- usr.sbin/vmctl/vmctl.c 22 Mar 2021 18:50:11 -0000 1.77
> >>> +++ usr.sbin/vmctl/vmctl.c 30 Apr 2021 12:31:22 -0000
> >>> @@ -461,7 +461,7 @@ terminate_vm(uint32_t terminate_id, cons
> >>> * terminate_vm_complete
> >>> *
> >>> * Callback function invoked when we are expecting an
> >>> - * IMSG_VMDOP_TERMINATE_VM_RESPONSE message indicating the completion of
> >>> + * IMSG_VMDOP_TERMINATE_VM_EVENT message indicating the completion of
It looks like this function has cases for both IMSG_VMDOP_TERMINATE_VM_RESPONSE
*and* _EVENT. Should the comment be phrased accordingly? If I read this
correctly, the comment would only state this function handles _EVENT messages.
> >>> * a terminate vm operation.
> >>> *
> >>> * Parameters:
> >>> @@ -484,41 +484,50 @@ terminate_vm_complete(struct imsg *imsg,
> >>> struct vmop_result *vmr;
> >>> int res;
> >>>
> >>> - if (imsg->hdr.type == IMSG_VMDOP_TERMINATE_VM_RESPONSE) {
> >>> + switch (imsg->hdr.type) {
> >>> + case IMSG_VMDOP_TERMINATE_VM_RESPONSE:
> >>> + IMSG_SIZE_CHECK(imsg, &vmr);
> >>> vmr = (struct vmop_result *)imsg->data;
> >>> res = vmr->vmr_result;
> >>> - if (res) {
> >>> - switch (res) {
> >>> - case VMD_VM_STOP_INVALID:
> >>> - fprintf(stderr,
> >>> - "cannot stop vm that is not running\n");
> >>> - *ret = EINVAL;
> >>> - break;
> >>> - case ENOENT:
> >>> - fprintf(stderr, "vm not found\n");
> >>> - *ret = EIO;
> >>> - break;
> >>> - case EINTR:
> >>> - fprintf(stderr, "interrupted call\n");
> >>> - *ret = EIO;
> >>> - break;
> >>> - default:
> >>> - errno = res;
> >>> - fprintf(stderr, "failed: %s\n",
> >>> - strerror(res));
> >>> - *ret = EIO;
> >>> - }
> >>> - } else if (flags & VMOP_WAIT) {
> >>> +
> >>> + switch (res) {
> >>> + case 0:
> >>> + fprintf(stderr, "requested to shutdown vm %d\n",
> >>> + vmr->vmr_id);
> >>> + *ret = 0;
> >>> + break;
> >>> + case VMD_VM_STOP_INVALID:
> >>> + fprintf(stderr,
> >>> + "cannot stop vm that is not running\n");
> >>> + *ret = EINVAL;
> >>> + break;
> >>> + case ENOENT:
> >>> + fprintf(stderr, "vm not found\n");
> >>> + *ret = EIO;
> >>> + break;
> >>> + case EINTR:
> >>> + fprintf(stderr, "interrupted call\n");
> >>> + *ret = EIO;
> >>> + break;
> >>> + default:
> >>> + errno = res;
> >>> + fprintf(stderr, "failed: %s\n",
> >>> + strerror(res));
> >>> + *ret = EIO;
> >>> + }
> >>> + break;
> >>> + case IMSG_VMDOP_TERMINATE_VM_EVENT:
> >>> + IMSG_SIZE_CHECK(imsg, &vmr);
> >>> + vmr = (struct vmop_result *)imsg->data;
> >>> + if (flags & VMOP_WAIT) {
> >>> 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);
> >>> - } else {
> >>> - fprintf(stderr, "requested to shutdown vm %d\n",
> >>> - vmr->vmr_id);
> >>> - *ret = 0;
> >>> }
> >>> - } else {
> >>> + *ret = 0;
> >>> + break;
> >>> + default:
> >>> fprintf(stderr, "unexpected response received from vmd\n");
> >>> *ret = EINVAL;
> >>> }
> >>> @@ -951,4 +960,3 @@ create_imagefile(int type, const char *i
> >>>
> >>> return (ret);
> >>> }
> >>> -
>
>
> --
> -Dave Voutila
>
