On Thu, Mar 16, 2017 at 10:18:21AM +0100, Maxime Coquelin wrote:

On 03/16/2017 08:08 AM, Yuanhan Liu wrote:

quoted

On Tue, Mar 14, 2017 at 10:53:23AM +0100, Maxime Coquelin wrote:

quoted

On 03/14/2017 10:46 AM, Maxime Coquelin wrote:

quoted

On 03/03/2017 10:51 AM, Yuanhan Liu wrote:

quoted

Introduce few APIs to set/get/enable/disable driver features.

Signed-off-by: Yuanhan Liu <redacted>
---
lib/librte_vhost/rte_vhost_version.map | 10 ++++
lib/librte_vhost/rte_virtio_net.h      |  9 ++++
lib/librte_vhost/socket.c              | 90
++++++++++++++++++++++++++++++++++
3 files changed, 109 insertions(+)

Nice!
Reviewed-by: Maxime Coquelin <redacted>

I wonder whether we could protect from setting/enabling/disabling
features once negotiation is done?

Those APIs won't be able to change the negotiated features. They are
just some interfaces before the vhost-user connection is established.

Right.
I meant it could return an error is the connection is already
established. Else, the caller might think the feature has been
successfully enabled/disabled, whereas it is not the case.
But this is maybe over-engineering to handle this case.

quoted

Ideally, we could/should get rid of the enabling/disabling functions:
let the vhost-user driver to handle the features directly (say, for
vhost-user pmd, we could use vdev options to disable/enable few specific
features). Once that is done, use the rte_vhost_driver_set_features()
set the features once.

Ok, but what about vhost lib users and TSO (I'm thinking of OVS).

Yes, that's why I kept them :)

	--yliu

quoted

The reason I introduced the enable/disable_features() is to keep the
compatability with the builtin vhost-user net driver (virtio_net.c).
If there is a plan to move it into vhost-pmd, they won't be needed.
And I don't think that will happen soon.

I agree with you that would be ideal, but unlikely to happen soon.

quoted

quoted

Oh, I forgot one comment on this patch.
Since these new functions are part to the API, shouldn't them be
documented?

Yes, indeed, it's also noted in my cover letter.

Oops, I missed that!

Thanks,
Maxime

quoted

--yliu