rust-vmm / vhost

Apache License 2.0
126 stars 64 forks source link

Remove non-standard BackedReq types #213

Closed germag closed 1 week ago

germag commented 7 months ago

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 commented 3 weeks ago

This can be closed with https://github.com/rust-vmm/vhost/pull/241

stefano-garzarella commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

@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 commented 1 week ago

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.

germag commented 1 week ago

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