Alexander Graf <ag...@suse.de> writes:
>> +static int vscsi_send_capabilities(VSCSIState *s, vscsi_req *req)
>> +{
>> + struct viosrp_capabilities *vcap;
>> + struct capabilities cap = { };
>> + uint16_t len, req_len;
>> + uint64_t buffer;
>> + int rc;
>> +
>> + vcap = &req->iu.mad.capabilities;
>> + req_len = len = be16_to_cpu(vcap->common.length);
>> + buffer = be64_to_cpu(vcap->buffer);
>> + if (len > sizeof(cap)) {
>> + fprintf(stderr, "vscsi_send_capabilities: capabilities size
>> mismatch !\n");
>> +
>> + /*
>> + * Just read and populate the structure that is known.
>> + * Zero rest of the structure.
>> + */
>> + len = sizeof(cap);
>> + }
>> + rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len);
>> + if (rc) {
>> + fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");
>
> ... if we fail reading
>
>> + }
>> +
>> + /*
>> + * Current implementation does not suppport any migration or
>> + * reservation capabilities. Construct the response telling the
>> + * guest not to use them.
>> + */
>> + cap.flags = 0;
>> + cap.migration.ecl = 0;
>> + cap.reserve.type = 0;
>> + cap.migration.common.server_support = 0;
>> + cap.reserve.common.server_support = 0;
>> +
>> + rc = spapr_vio_dma_write(&s->vdev, buffer, &cap, len);
>
> ... but then succeed writing back the empty structure we will report
> "success". Is this correct? It's fine with me if it's intended, just
> want to make sure we're making a conscious decision about it.
Yes, that should be fine if it succeeds writing it. If DMA read/write is
an issue, it should fail writing as well and we report back failure.
>
> The rest looks good to me. But please post new patch versions as new
> top level emails, not as inline replies. My scripts use patchwork and
> that can't really handle this case overly well (it works, but is
> additional work). It also makes it hard for people who just follow the
> thread loosely to realize that a new version is there.
Sure, will take care next time.
Regards
Nikunj
