Re: [Qemu-devel] [PATCH] spapr-vscsi: Adding VSCSI capabilities

[Alexander Graf]

On 27.08.2013, at 07:43, Nikunj A Dadhania wrote:

> Alexander Graf <address@hidden> writes:
> 
>> On 26.08.2013, at 13:46, Nikunj A Dadhania wrote:
>> 
>>> Alexander Graf <address@hidden> writes:
>>> 
>>>> On 26.08.2013, at 12:58, Nikunj A Dadhania wrote:
>>>> 
>>>>> This implements capabilities exchange between host and client.
>>>>> As at the moment no capability is supported, put zero flags everywhere
>>>>> and return.
>>>>> 
>>>>> Signed-off-by: Nikunj A Dadhania <address@hidden>
>>>>> ---
>>>>> +    rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len);
>>>>> +    if (rc)  {
>>>>> +        fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");
>>>> 
>>>> At this point cap contains random host data, no?
>>> 
>>> Yes, and we zero it out in this case.
>> 
>> Then please make this obvious to the reader. Either memset(0) it or do
>> cap = { };. But do something that doesn't make me look up the header
>> file to check whether you really did catch all the fields there are in
>> the struct.
> 
> Here is the patch incorporating the comments, moreover goto is not there
> anymore, we are being more accomodative and copying only the size(cap)
> structure even if guest is requesting for more.
> 
> From: Nikunj A Dadhania <address@hidden>
> 
> This implements capabilities exchange between vscsi host and client.  As
> at the moment no capability is supported, put zero flags everywhere and
> return.
> 
> Signed-off-by: Nikunj A Dadhania <address@hidden>
> ---
> hw/scsi/spapr_vscsi.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 54 insertions(+)
> 
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> index e9090e5..030b775 100644
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
> @@ -858,6 +858,57 @@ static int vscsi_send_adapter_info(VSCSIState *s, 
> vscsi_req *req)
>     return vscsi_send_iu(s, req, sizeof(*sinfo), VIOSRP_MAD_FORMAT);
> }
> 
> +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.

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.


Alex