bhyve: Use directory `fd` with checkpoint

[gusev.vitaliy_gmail.com]

Summary

Part of Capsicum integration for snapshots.

Snapshots are placed to a directory obtained from --checkpoint/--snapshot argument.
If only name is described, snapshot is placed to the bhyvectl's current directory.

Example:

bhyvectl --suspend=/disk/dump/x1--vm=ubuntu

Snapshot 'x1' will be stored to the '/disk/dump' directory.

Sponsored by: vStack

Test Plan

Compile, Run VM, Suspend, Resume.

[rew]

I still don't understand why you haven't addressed my comments that I raised in the original review. Do you feel it's not worth addressing or is that I'm out of line?

I'd suggest creating a dependency stack with phabricator now that you've split the original review into multiple ones.

usr.sbin/bhyvectl/bhyvectl.c

1754

see basename(3)

1756

what's going to happen if open_directory() fails?

[gusev.vitaliy_gmail.com]

In D38858#884827, @rew wrote:

I still don't understand why you haven't addressed my comments that I raised in the original review. Do you feel it's not worth addressing or is that I'm out of line?

Did you mean about format of the image files or something another? If format, I am going to resolve it with coming nvlist changes.

I'd suggest creating a dependency stack with phabricator now that you've split the original review into multiple ones.

Could you provide how to do it? Thanks.

usr.sbin/bhyvectl/bhyvectl.c

1754

basename() is not case because it needs not constant pointer and therefore needs allocation and free-ing. Current implementation doesn't need any allocation.

1756

Negative 'fd' will be added to nvl, and nvlist_send() will fail. If you think it needs additional log message, please describe which.

[rew]

In D38858#884878, @gusev.vitaliy_gmail.com wrote:

Did you mean about format of the image files or something another? If format, I am going to resolve it with coming nvlist changes.

Have you sought out jhb's advice on this yet? I know he has expressed his opinion on it in the past.

Could you provide how to do it? Thanks.

https://llvm.org/docs/Phabricator.html#creating-a-patch-series

usr.sbin/bhyvectl/bhyvectl.c

1754

I prefer to see basename() used as opposed to using a hand-rolled function - it's more idiomatic.

its probably fine to drop the const qualifier or do the allocation...either way.

1756

why try to send the nvlist if the file descriptor is known to be bad?

[gusev.vitaliy_gmail.com]

In D38858#884892, @rew wrote:

In D38858#884878, @gusev.vitaliy_gmail.com wrote:

Did you mean about format of the image files or something another? If format, I am going to resolve it with coming nvlist changes.

Have you sought out jhb's advice on this yet? I know he has expressed his opinion on it in the past.

Sorry, which advice did you mean? I don't see any comment from jhb in https://reviews.freebsd.org/D35454

Could you provide how to do it? Thanks.

https://llvm.org/docs/Phabricator.html#creating-a-patch-series

Done. Thanks.

usr.sbin/bhyvectl/bhyvectl.c

1754

Will update.

1756

I wanted to make a code short. Understood your complaints and will update. Thanks.

[gusev.vitaliy_gmail.com]

Corrected according with @rew notes.

[gusev.vitaliy_gmail.com]

usr.sbin/bhyvectl/bhyvectl.c

1754

Done.

1756

Done.

[rew]

usr.sbin/bhyvectl/bhyvectl.c

1741

what happens when just a filename is passed to open_directory?

1746

what happens if basename() fails?

1748

where is this file descriptor being closed?

[rew]

! In D38858#884878, @gusev.vitaliy_gmail.com wrote:
Did you mean about format of the image files or something another? If format, I am going to resolve it with coming nvlist changes.

you plan on converting the snapshot file format to an nvlist?

Sorry, which advice did you mean? I don't see any comment from jhb in https://reviews.freebsd.org/D35454

see the original commit message that brought the snapshot code in

[gusev.vitaliy_gmail.com]

usr.sbin/bhyvectl/bhyvectl.c

1741

If only name is described, snapshot is placed to the bhyvectl's current directory.

I.e. fd will point to "."

From man dirname(3):

RETURN VALUES
     If path is a null pointer, the empty string, or contains no ‘/’
     characters, dirname() returns a pointer to the string ".", signifying the
     current directory.  Otherwise, it returns a pointer to the parent
     directory of path.

1746

base name never fails.

From man:

RETURN VALUES
     If path consists entirely of ‘/’ characters, a pointer to the string "/"
     is returned.  If path is a null pointer or the empty string, a pointer to
     the string "." is returned.  Otherwise, it returns a pointer to the last
     component of path.

1748

nvlist_move_descriptor() "owns" passed descriptor and it will be closed at nvlist_destroy().

[gusev.vitaliy_gmail.com]

In D38858#885508, @rew wrote:

! In D38858#884878, @gusev.vitaliy_gmail.com wrote:
Did you mean about format of the image files or something another? If format, I am going to resolve it with coming nvlist changes.

you plan on converting the snapshot file format to an nvlist?

Plain is:

1. Capsicum (has review)
2. Multiple devices support (has review)
3. nvlist review. Not yet created and waits for 1-2 finish. I will describe and propose file format with nvlist implementation.

Sorry, which advice did you mean? I don't see any comment from jhb in https://reviews.freebsd.org/D35454

see the original commit message that brought the snapshot code in

The file format also does not currently support
    versioning of individual chunks of state.  As a result, the current
    file format is not a fixed binary format and future revisions to save
    and restore will break binary compatiblity of snapshot files.  The
    goal is to move to a more flexible format that adds versioning,
    etc. and at that point to commit to providing a reasonable level of
    compatibility.

If I understand right, all things could be achieved with nvlist implementation. Thanks.

[rew]

In D38858#885557, @gusev.vitaliy_gmail.com wrote:

In D38858#885508, @rew wrote:

! In D38858#884878, @gusev.vitaliy_gmail.com wrote:
Did you mean about format of the image files or something another? If format, I am going to resolve it with coming nvlist changes.

you plan on converting the snapshot file format to an nvlist?

Plain is:

1. Capsicum (has review)
2. Multiple devices support (has review)
3. nvlist review. Not yet created and waits for 1-2 finish. I will describe and propose file format with nvlist implementation.

Sorry, which advice did you mean? I don't see any comment from jhb in https://reviews.freebsd.org/D35454

see the original commit message that brought the snapshot code in

The file format also does not currently support
    versioning of individual chunks of state.  As a result, the current
    file format is not a fixed binary format and future revisions to save
    and restore will break binary compatiblity of snapshot files.  The
    goal is to move to a more flexible format that adds versioning,
    etc. and at that point to commit to providing a reasonable level of
    compatibility.

If I understand right, all things could be achieved with nvlist implementation. Thanks.

Why does there need to be multiple files for a single snapshot?

[gusev.vitaliy_gmail.com]

In D38858#885604, @rew wrote:

..

see the original commit message that brought the snapshot code in

The file format also does not currently support
    versioning of individual chunks of state.  As a result, the current
    file format is not a fixed binary format and future revisions to save
    and restore will break binary compatiblity of snapshot files.  The
    goal is to move to a more flexible format that adds versioning,
    etc. and at that point to commit to providing a reasonable level of
    compatibility.

If I understand right, all things could be achieved with nvlist implementation. Thanks.

Why does there need to be multiple files for a single snapshot?

nvlist implementation will use single file for all:

• config
• vram
• kernel data
• devices data

So you are right, using multiple files is not reasonable and is hard to operate.

[rew]

In D38858#885606, @gusev.vitaliy_gmail.com wrote:

In D38858#885604, @rew wrote:

..

see the original commit message that brought the snapshot code in

The file format also does not currently support
    versioning of individual chunks of state.  As a result, the current
    file format is not a fixed binary format and future revisions to save
    and restore will break binary compatiblity of snapshot files.  The
    goal is to move to a more flexible format that adds versioning,
    etc. and at that point to commit to providing a reasonable level of
    compatibility.

If I understand right, all things could be achieved with nvlist implementation. Thanks.

Why does there need to be multiple files for a single snapshot?

nvlist implementation will use single file for all:

• config
• vram
• kernel data
• devices data

So you are right, using multiple files is not reasonable and is hard to operate.

In which case, the file descriptor change needs to happen after bhyve starts using a single file for snapshots.

So this review and https://reviews.freebsd.org/D38860 will have to wait till the above work is merged.

[gusev.vitaliy_gmail.com]

In D38858#885635, @rew wrote:

In D38858#885606, @gusev.vitaliy_gmail.com wrote:

nvlist implementation will use single file for all:

• config
• vram
• kernel data
• devices data

So you are right, using multiple files is not reasonable and is hard to operate.

In which case, the file descriptor change needs to happen after bhyve starts using a single file for snapshots.

I don't think so. It is not dependent thing. Current state of snapshot/resume is using multiple files. And this work is about adding support Capsicum. It is not related to the single file format at all.

So this review and https://reviews.freebsd.org/D38860 will have to wait till the above work is merged.

Why do you think single format work (nvlist) should be *before* enabling Capsicum? nvlist work will have a lot of changes and we can stuck if discussing any useful thing eats a lot of time and risk is not to move forward for the months. Just note, multiple device review is 7-8 months old. It is crazy for the code that is under #ifdef BHYVE_SNAPSHOT. We need speedup this work. For now it is about 2 years when Snapshot/Resume was added and there no significant progress.

If you have questions or notes about the code, please write here. Otherwise please accept the reviews.

[rew]

In D38858#885637, @gusev.vitaliy_gmail.com wrote:

In D38858#885635, @rew wrote:

In D38858#885606, @gusev.vitaliy_gmail.com wrote:

nvlist implementation will use single file for all:

• config
• vram
• kernel data
• devices data

So you are right, using multiple files is not reasonable and is hard to operate.

In which case, the file descriptor change needs to happen after bhyve starts using a single file for snapshots.

I don't think so. It is not dependent thing. Current state of snapshot/resume is using multiple files. And this work is about adding support Capsicum. It is not related to the single file format at all.

So this review and https://reviews.freebsd.org/D38860 will have to wait till the above work is merged.

Why do you think single format work (nvlist) should be *before* enabling Capsicum? nvlist work will have a lot of changes and we can stuck if discussing any useful thing eats a lot of time and risk is not to move forward for the months. Just note, multiple device review is 7-8 months old. It is crazy for the code that is under #ifdef BHYVE_SNAPSHOT. We need speedup this work. For now it is about 2 years when Snapshot/Resume was added and there no significant progress.

If you have questions or notes about the code, please write here. Otherwise please accept the reviews.

The only file descriptor that needs to be passed to bhyve is the snapshot file that is being written to, which won't require opening a file descriptor to a directory.

Rebase this review after the file format work has been merged into main.

Thanks.

[gusev.vitaliy_gmail.com]

In D38858#885643, @rew wrote:

In D38858#885637, @gusev.vitaliy_gmail.com wrote:

Why do you think single format work (nvlist) should be *before* enabling Capsicum? nvlist work will have a lot of changes and we can stuck if discussing any useful thing eats a lot of time and risk is not to move forward for the months. Just note, multiple device review is 7-8 months old. It is crazy for the code that is under #ifdef BHYVE_SNAPSHOT. We need speedup this work. For now it is about 2 years when Snapshot/Resume was added and there no significant progress.

If you have questions or notes about the code, please write here. Otherwise please accept the reviews.

The only file descriptor that needs to be passed to bhyve is the snapshot file that is being written to, which won't require opening a file descriptor to a directory.

Rebase this review after the file format work has been merged into main.

Thanks.

Ok. I was wrong. It is about 3 years when Suspend/Resume was added (May 2020) and there is no any progress in that - almost 3 years! No new features. No fixing issues. Nothing.

Multiple Devices Review (https://reviews.freebsd.org/D35590) is stuck for 7 months.

Do we need any progress in Suspend/Resume functionality? If yes, we need moving forward without any "Please do something another thing and maybe later we can consider to accept".

Just note, nvlist work is huge and will require a lot of efforts and discussion. And there is no any guarantee that I will create nvlist patchwork and it will not stuck like Capsicum work or Multiple Device work or others.

Again, do we need progress in Suspend/Resume area? If yes, please move this work forward. This patchwork improves security and there is no any wrong thing in opening directory.

[rew]

I withdraw my request for changes.

Having capsicum support is more important than using a single file for snapshots.

[emaste]

Given that this review is relatively small, self-contained, and the work is already done I agree with getting this in and having Capsicum support enabled.

We really do need to get the single-fd and mutliple-devices changes done as well (and the work in this review will be effectively reverted afterwards), but we will at least have Capsicum enabled in the interim.

usr.sbin/bhyve/snapshot.c

1357

Is it OK to pass -1 to fdopen?

[gusev.vitaliy_gmail.com]

usr.sbin/bhyve/snapshot.c

1357

@emaste fdopen() will fail with EBADF. If you think it needs additional check, I can add it.

[emaste]

usr.sbin/bhyve/snapshot.c

1357

I think so, otherwise the EBADF could be used by perror below and result in a confusing message.

meta_file is initialized to NULL already so it should be enough to just do if (fd_meta != -1) meta_file = ...?

[gusev.vitaliy_gmail.com]

Corrected fd_meta usage in case of error.

[gusev.vitaliy_gmail.com]

usr.sbin/bhyve/snapshot.c

1357

Good point. Corrected.

[gusev.vitaliy_gmail.com]

@rew @emaste Can we go further with this review ? Note, that some of code will be re-written in "one file per snapshot" review. However it will be later, because format of image file requires discussion.

[corvink]

LGTM

[gusev.vitaliy_gmail.com]

@corvink Can it be committed or I should improve/correct it ?