Page MenuHomeFreeBSD

bhyve: Use directory `fd` with checkpoint
ClosedPublic

Authored by gusev.vitaliy_gmail.com on Mar 2 2023, 6:20 PM.
Tags
Referenced Files
Unknown Object (File)
Feb 21 2024, 2:46 PM
Unknown Object (File)
Feb 21 2024, 2:46 PM
Unknown Object (File)
Dec 23 2023, 12:43 AM
Unknown Object (File)
Dec 22 2023, 9:29 AM
Unknown Object (File)
Dec 22 2023, 9:29 AM
Unknown Object (File)
Dec 10 2023, 7:27 PM
Unknown Object (File)
Sep 9 2023, 7:44 PM
Unknown Object (File)
Aug 14 2023, 3:10 PM

Details

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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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?

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.

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?

In D38858#884892, @rew 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 added inline comments.
usr.sbin/bhyvectl/bhyvectl.c
1754

Done.

1756

Done.

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?

! 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 added inline comments.
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().

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.

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?

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 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.

In D38858#885635, @rew 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 requested changes to this revision.Mar 3 2023, 11:15 PM
In D38858#885635, @rew 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.

This revision now requires changes to proceed.Mar 3 2023, 11:15 PM
In D38858#885643, @rew 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.

I withdraw my request for changes.

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

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?

usr.sbin/bhyve/snapshot.c
1357

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

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 = ...?

Corrected fd_meta usage in case of error.

gusev.vitaliy_gmail.com added inline comments.
usr.sbin/bhyve/snapshot.c
1357

Good point. Corrected.

@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 Can it be committed or I should improve/correct it ?

This revision was not accepted when it landed; it landed in state Needs Review.Apr 28 2023, 7:07 AM
This revision was automatically updated to reflect the committed changes.