Re: [PATCH v3 04/16] virtio-scsi-test: add missing feature negotiation

[Thomas Huth <thuth@redhat.com>]

On 19/10/2019 08.37, Stefan Hajnoczi wrote:
> VIRTIO Device Initialization requires feature negotiation.  Currently
> virtio-scsi-test.c is non-compliant.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/virtio-scsi-test.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
> index 7c8f9b27f8..0415e75876 100644
> --- a/tests/virtio-scsi-test.c
> +++ b/tests/virtio-scsi-test.c
> @@ -123,10 +123,16 @@ static QVirtioSCSIQueues *qvirtio_scsi_init(QVirtioDevice *dev)
>      QVirtioSCSIQueues *vs;
>      const uint8_t test_unit_ready_cdb[VIRTIO_SCSI_CDB_SIZE] = {};
>      struct virtio_scsi_cmd_resp resp;
> +    uint64_t features;
>      int i;
>  
>      vs = g_new0(QVirtioSCSIQueues, 1);
>      vs->dev = dev;
> +
> +    features = qvirtio_get_features(dev);
> +    features &= ~(QVIRTIO_F_BAD_FEATURE | (1ull << VIRTIO_RING_F_EVENT_IDX));
> +    qvirtio_set_features(dev, features);

I wonder whether this get_feat + "&=" + set_feat is really the right way
here? Maybe we should rather only set the feature bits that we care
about instead of setting all but BAD_FEATURE and RING_F_EVENT_IDX?

Otherwise, please mention in the commit message why you've chosen to
disable just these two bits.

 Thanks,
  Thomas