libvmm: clean up vmmapi.h

[rew]

Summary

enum checkpoint_opcodes, struct checkpoint_op, and
MAX_SNAPSHOT_VMNAME are not vmm specific.

They are used for the save/restore functionality that bhyve(8) offers
and are better suited in usr.sbin/bhyve/snapshot.h.

Since bhyvectl(8) is a consumer of these variables, the Makefile for bhyvectl(8)
has been modified to include usr.sbin/bhyve/snapshot.h.

bhyve/snapshot.h requires definitions provided by ucl.h, so the bhyvectl(8)
Makefile has been modified to also include libucl. The libucl include dependency
will be removed in a later commit.

Test Plan

test build with and without BHYVE_SNAPSHOT defined.

[rgrimes]

usr.sbin/bhyve/snapshot.h

45

Why do we even have this constant?

sys/amd64/include/vmm.h:#define VM_MAX_NAMELEN 32

What is the future proofing of this constant? Why is it not the same as or based on the vmm.h paramter VM_MAX_NAMELEN?

69

Its the length of a filename? Shouldnt it be using a more global definition of filename length?
./sys/syslimits.h:#define NAME_MAX 255 /* max bytes in a file name */

[rew]

usr.sbin/bhyve/snapshot.h

45

Why do we even have this constant?

It's used by the save/restore code to cap the length of the filename. This constant determines the size of struct checkpoint_op, which is a struct that gets written across a socket. Why is it arbitrarily set at 100? I don't know. Should it be changed? probably.

What is the future proofing of this constant? Why is it not the same as or based on the vmm.h paramter VM_MAX_NAMELEN?

Because it has nothing to do with the length of a VM name. The naming semantics of MAX_SNAPSHOT_VMNAME is wrong - it should probably be MAX_SNAPSHOT_FILENAME or NAME_MAX, as mentioned below.

My intention is to address these issues in a separate review/commit. I can include the changes now, but my thinking was to keep this review as small as possible.

69

Yes. NAME_MAX is probably a better fit here.

maybe #define MAX_SNAPSHOT_FILENAME NAME_MAX?

still, I prefer to save the rename and size adjustment of this constant for a separate review, if possible.

[rew]

ping rgrimes@, does that sound reasonable to you?

[rgrimes]

In D28410#636597, @rew wrote:

ping rgrimes@, does that sound reasonable to you?

Either or, about the same number of commits. Ultimately not my decision, jhb should weigh in.

[grehan]

A separate review is fine.

[kevans]

usr.sbin/bhyvectl/Makefile

25

IMO we should prefer to spell this ${SRCTOP}/usr.sbin/bhyve -- my immediate reaction is that it's "better" when you're reaching into this level of organization (^/bin, ^/sbin, ^/usr.bin, ^/usr.sbin, ^/lib, etc.) so that we can more easily find where we've assumed the location of this kind of stuff. While it's not perhaps not likely for this particular binary to make a meaningful move that would benefit from it, it's a good habit.

28

No need for .PATH here -- we're not trying to build or install anything from ${BHYVE}

[kevans]

My requests are pretty minimal, so let's just go ahead and:

Approved by: kevans (mentor)

Probably no need to re-submit if you've build-tested your changes, they're build nits and thus not likely to change @grehan's disposition.

[jhb]

Hmm, so what I was hoping was to make the UNIX domain socket more generic and not specific to the save/restore code. That is, I think the control socket should be SOCK_DGRAM instead of SOCK_STREAM, and different control messages can then have different sizes (or even be variably-sized) where the "base message" type is something like struct control_msg { int op; }. The enum should also get renamed to not be checkpoint-specific (something like enum control_message_op) so it can support other message types. Once you do that though, you'll have to move the enum and at least the 'base' message structure back out of vmmapi.h. It seems more prudent to instead perhaps only move the struct for now but rename the enum and leave it in the existing location (and perhaps add a new structure for the "common" message header that includes the integer). Doing SOCK_DGRAM instead of SOCK_STREAM is probably the next step after that (and gets weird of loops around read in the current code in bhyve).

[rgrimes]

usr.sbin/bhyve/snapshot.h

45

Why do we even have this constant?

It's used by the save/restore code to cap the length of the filename. This constant determines the size of struct checkpoint_op, which is a struct that gets written across a socket. Why is it arbitrarily set at 100? I don't know. Should it be changed? probably.

What is the future proofing of this constant? Why is it not the same as or based on the vmm.h paramter VM_MAX_NAMELEN?

Because it has nothing to do with the length of a VM name. The naming semantics of MAX_SNAPSHOT_VMNAME is wrong - it should probably be MAX_SNAPSHOT_FILENAME or NAME_MAX, as mentioned below.

My intention is to address these issues in a separate review/commit. I can include the changes now, but my thinking was to keep this review as small as possible.

45

Why do we even have this constant?

It's used by the save/restore code to cap the length of the filename. This constant determines the size of struct checkpoint_op, which is a struct that gets written across a socket. Why is it arbitrarily set at 100? I don't know. Should it be changed? probably.

What is the future proofing of this constant? Why is it not the same as or based on the vmm.h paramter VM_MAX_NAMELEN?

Because it has nothing to do with the length of a VM name. The naming semantics of MAX_SNAPSHOT_VMNAME is wrong - it should probably be MAX_SNAPSHOT_FILENAME or NAME_MAX, as mentioned below.

My intention is to address these issues in a separate review/commit. I can include the changes now, but my thinking was to keep this review as small as possible.

[rew]

In D28410#636635, @jhb wrote:

Hmm, so what I was hoping was to make the UNIX domain socket more generic and not specific to the save/restore code.

Indeed, that is also my goal here. I was going to rebase https://reviews.freebsd.org/D28199 off this review. The linked review separates the socket from the save/restore code, but still keeps the socket behind '#ifdef` guards.

That is, I think the control socket should be SOCK_DGRAM instead of SOCK_STREAM,

The linked review above uses SOCK_DGRAM.

It seems more prudent to instead perhaps only move the struct for now but rename the enum and leave it in the existing location (and perhaps add a new structure for the "common" message header that includes the integer).

My initial thought process was that these variables seemed more specific to bhyve and less to do with libvmm (wrong reasoning here?). Other than for bhyvectl, is there a reason why these variables would need to be shared through libvmm as opposed to a header file that lives at usr.sbin/bhyve? Are you thinking that messages could/should go to bhyve through a libvmm api?

[jhb]

In D28410#636710, @rew wrote:

In D28410#636635, @jhb wrote:

It seems more prudent to instead perhaps only move the struct for now but rename the enum and leave it in the existing location (and perhaps add a new structure for the "common" message header that includes the integer).

My initial thought process was that these variables seemed more specific to bhyve and less to do with libvmm (wrong reasoning here?). Other than for bhyvectl, is there a reason why these variables would need to be shared through libvmm as opposed to a header file that lives at usr.sbin/bhyve? Are you thinking that messages could/should go to bhyve through a libvmm api?

Oh, that's true. I would actually be quite happy to have a separate "bhyve_ipc.h" or some such header that just defined the opcodes and structures for the messages (and maybe a macro for /var/run/bhyve). I wasn't really sure if we wanted a separate header vs (ab)using vmmapi.h to serve as that.

[rew]

The review I linked earlier introduces ipc.h (and a macro for /var/run/bhyve), this link should show that file:

https://reviews.freebsd.org/D28199#change-cqS8HrGQfGJO

My main goal with this review was to hook up the makefile bits for bhyvectl to use headers from bhyve, instead of piling it into the linked review.

Perhaps I should have uploaded these reviews to phabricator with dependencies on one another - apologies for the confusion. If there’s any suggestions how I could have done this better, would appreciate hearing it.

[jhb]

Ok, @kevans notes about .PATH and SRCTOP should still be fixed, but I'm happy to defer other changes to the next review. I do think we might eventually want to make the ipc message definitions in a public header, e.g. perhaps vmm_ipc.h or some such that gets installed to /usr/include, but that is something we can change in the future if we want to go that route.

usr.sbin/bhyvectl/Makefile

28

Also, assuming we don't need the .PATH in the future, we can just use a direct path for bhyve in CFLAGS without needing the helper variable, i.e.

CFLAGS+= -I${SRCTOP}/usr.sbin/bhyve