bhyvectl: support querying VM pid

[novel]

Summary

Extend bhyvectl(8) to support querying VM pid using the
--get-vm-pid flag.

This is useful in monitor mode when the VM pid differs
from the main bhyve(8) process run by the user. Knowing the VM pid
is necessary, for example, to query process resource usage or
trigger ACPI shutdown with SIGTERM.

Of course, it could be obtained by matching the monitor process
children by the process title, but it's a little more complex
and fragile than it could be.

Implement that by adding the "get_vm_pid" IPC command,
and using it to implement bhyvectl --get-vm-pid, which
prints the VM PID.

When the VM PID is not known, ESRCH is returned.

[bnovkov]

This is a good feature, but I don't think that this is the correct approach design-wise. Adding an ioctl handler to just to fetch a VM's PID is overkill IMO, new ioctl commands should only be added when there's no other way to obtain the needed information (e.g., fetching memsegs and other metadata that is stored in the vmm module).

I strongly suggest that you implement this functionality using bhyve's IPC facilities. These were only reserved for the snapshot subsystem up until very recently, but I had to split these out into a separate subsystem as a part of my hotplugging work.
This should make the implementation way simpler since you just need to register an IPC handler instead of adding a new ioctl.
I just committed the refactor so you can build on top of it.

[novel]

In D59108#1355951, @bnovkov wrote:

This is a good feature, but I don't think that this is the correct approach design-wise. Adding an ioctl handler to just to fetch a VM's PID is overkill IMO, new ioctl commands should only be added when there's no other way to obtain the needed information (e.g., fetching memsegs and other metadata that is stored in the vmm module).

I strongly suggest that you implement this functionality using bhyve's IPC facilities. These were only reserved for the snapshot subsystem up until very recently, but I had to split these out into a separate subsystem as a part of my hotplugging work.
This should make the implementation way simpler since you just need to register an IPC handler instead of adding a new ioctl.
I just committed the refactor so you can build on top of it.

Thanks, I'll take a look at the new IPC code.

[novel]

Re-implement using IPC.

[bnovkov]

usr.sbin/bhyvectl/bhyvectl.c

270

Are we allowing the error to be suppressed so we can handle get_all gracefully?

313

If we pass a non-NULL replyp we'll end up passing NULL to this nvlist_destroy call. We should only be destroying it if the caller isn't interested in the value.

577–581

Why not just handle the error in vm_pid_request?

[novel]

• Do not call nvlist_destroy() on NULL.
• Update vm_pid_request() to properly set report_error depending on whether it's --get-all or --get-vm-pid option.
• bhyvectl.8: rundir: s/checkpoint socket/bhyve IPC socket/.

[novel]

usr.sbin/bhyvectl/bhyvectl.c

270

Correct. As described in summary, my idea is that --get-all should print the PID if it knows it, but do not fail if it does not (and should not print errors coming from attempting to get it). And when explicitly requested with --get-vm-pid, it should fail if it cannot get it, and report errors related to that.

313

nvlist_destroy() allows passing NULL and does nothing in this case, so it should be safe. I'll update the code to be more explicit though.

577–581

As mentioned in the other comment, I'd like treat errors differently based on the mode we're currently in (--get-all or --get-vm-pid). I noticed though that I don't set report_error to true for the --get-vm-pid, so I'll update vm_pid_request(). Anyway, it feels like this error handling logic fits here better than in the vm_pid_request().

[bnovkov]

usr.sbin/bhyvectl/bhyvectl.c

270

hm, I am a bit conflicted about having --get-vm-pid included in --get-all since it is implicitly dependent on the rundir option. I think that we shouldn't mix ioctl and IPC operations in --get-all, so I'd personally leave this out of --get-all . I'm not sure what the best solution here would be, so I won't insist.

[novel]

usr.sbin/bhyvectl/bhyvectl.c

270

I don't have practical reasons to keep it in --get-all. The only motivation was consistency in sense that "vm pid" falls under "all". On the other hand, the --get-all seems to be a sort of a service feature that we do not document in the manual page and do not promise anything about it . I personally do not use --get-all, at least not in the libvirt driver. I think I'll remove this part from the patch for now, it seems to create too many complications for a feature that probably nobody needs.

[novel]

Do not report VM PID in --get-all.

[bnovkov]

Looks good now, thanks! I only have one minor inline comment.

usr.sbin/bhyve/ipc.c

65

We should check the return value here since this can fail.

[novel]

usr.sbin/bhyve/ipc.c

65

Hm, according to the manual page,. getpid() is always successful, and nvlist_add_number() is void.

[bnovkov]

usr.sbin/bhyve/ipc.c

65

ah sorry, I misinterpreted the nvlist manpage here.

[novel]

Update the final manual pages bits.