Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove non-standard BackedReq types #213

Closed
germag opened this issue Nov 20, 2023 · 8 comments · Fixed by #246
Closed

Remove non-standard BackedReq types #213

germag opened this issue Nov 20, 2023 · 8 comments · Fixed by #246

Comments

@germag
Copy link
Collaborator

germag commented Nov 20, 2023

The following back-end request types are not part of the vhost-user protocol and should be removed:

pub enum BackendReq {
    ...
    /// Virtio-fs draft: map file content into the window.
    FS_MAP = 6,
    /// Virtio-fs draft: unmap file content from the window.
    FS_UNMAP = 7,
    /// Virtio-fs draft: sync file content.
    FS_SYNC = 8,
    /// Virtio-fs draft: perform a read/write from an fd directly to GPA.
    FS_IO = 9,
    ...
}

These are not in the vhost-user standard, which in itself is confusing, but even conflict with values that are defined in the standard (see Back-end message types), such as: VHOST_USER_BACKEND_SHARED_OBJECT_ADD, VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE and VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP.

This issue is to continue the discussion started on #205

@aesteve-rh
Copy link
Contributor

This can be closed with #241

@stefano-garzarella
Copy link
Member

@aesteve-rh I don't know if we can do even more by removing those types completely, or putting them behind a feature (e.g. unstable)

@aesteve-rh
Copy link
Contributor

aesteve-rh commented Jun 19, 2024

Ah, fair point. It is true that the issue states removing them, not only "moving them out of the way".

So I'd be fine either way, but I see some value in having some support for non-standard features/messages (as long as they are backed up by a patch or an effort to standardize, or something like that). So I guess I'd vote for the feature option.

Given that we have staging devices in vhost-device, and these may require non-standard messages, having these devices upstream would be supported just by enabling the feature in the crate settings.

Not sure if it makes much sense to keep the FS_* messages specifically, though...

@stefano-garzarella
Copy link
Member

Given that we have staging devices in vhost-device, and these may require non-standard messages, having these devices upstream would be supported just by enabling the feature in the crate settings.

Yep, I agree.

Not sure if it makes much sense to keep the FS_* messages specifically, though...

Good point, if they are not used by anyone, we could remove them and add them back with a feature when they are needed.

@germag
Copy link
Collaborator Author

germag commented Jun 27, 2024

Not sure if it makes much sense to keep the FS_* messages specifically, though...

I think it is unlikely that those will be used any time soon (or ever), the qemu community prefer a more general approach, so I think is safe to remove them

@stefano-garzarella wrote: putting them behind a feature (e.g. unstable)
@aesteve-rh wrote: I see some value in having some support for non-standard features/messages (as long as they are backed up by a patch or an effort to standardize, or something like that). So I guess I'd vote for the feature option.

I'm more in line to remove them, because even if there is an effort to standardize them, it doesn't mean that it will succeed or how long it will take. IMO this adds many possible mantainership problems, like should we get the burden of maintaining non-standard code?, how to test it if it requires patches in qemu/C-H/etc, no available upstream. Do we need to accept any non-standard PR?, if not, how to decide which one is accepted and which one don't?, what happens if in the meantime an incompatible change is added to the standard, we should have more ugly xor features like the xen one?, if it's incompatible with another non-standard feature?

I think if there is an effort to standardize a Draft is appropriate

@stefano-garzarella
Copy link
Member

@germag I think both @aesteve-rh and I are in favor to remove them, but we considered adding a feature if it would help any ongoing development. If you, the maintainer of virtiofsd, tell us that they will not be used, I would say remove them without any problems ;-)

BTW the title of this issue was already explanatory, so my fault for coming up with the idea of the feature.

Any volunteers to do this? I can do eventually.

@aesteve-rh
Copy link
Contributor

I can post a patch to remove them.

The feature thingy, we can just take it into account for the next non-standard set of messages that gets posted. No need to preemptively add it if there is no usecase atm.

aesteve-rh added a commit to aesteve-rh/vhost that referenced this issue Jun 27, 2024
Since these non-standard backend request
message types are confirmed not to be used
anymore, let's just remove them.

```
pub enum BackendReq {
    ...
    /// Virtio-fs draft: map file content into the window.
    FS_MAP = 100,
    /// Virtio-fs draft: unmap file content from the window.
    FS_UNMAP = 1001,
    /// Virtio-fs draft: sync file content.
    FS_SYNC = 1002,
    /// Virtio-fs draft: perform a read/write from an fd directly to GPA.
    FS_IO = 1003,
    ...
}
```

Closes: rust-vmm#213
Signed-off-by: Albert Esteve <[email protected]>
aesteve-rh added a commit to aesteve-rh/vhost that referenced this issue Jun 27, 2024
Since these non-standard backend request
message types are confirmed not to be used
anymore, let's just remove them.

```
pub enum BackendReq {
    ...
    /// Virtio-fs draft: map file content into the window.
    FS_MAP = 100,
    /// Virtio-fs draft: unmap file content from the window.
    FS_UNMAP = 1001,
    /// Virtio-fs draft: sync file content.
    FS_SYNC = 1002,
    /// Virtio-fs draft: perform a read/write from an fd directly to GPA.
    FS_IO = 1003,
    ...
}
```

Closes: rust-vmm#213
Signed-off-by: Albert Esteve <[email protected]>
@germag
Copy link
Collaborator Author

germag commented Jun 27, 2024

@germag I think both @aesteve-rh and I are in favor to remove them, but we considered adding a feature if it would help any ongoing development. If you, the maintainer of virtiofsd, tell us that they will not be used, I would say remove them without any problems ;-)

BTW the title of this issue was already explanatory, so my fault for coming up with the idea of the feature.

Sorry, I misinterpreted it as allowing any non-standard and experimental code to be accepted in the future under the "experimental" feature... you know Mondays :) (yes, let's remove them)

Any volunteers to do this? I can do eventually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants