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

[Alexander Graf]

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.

> 
>> 
>>> +    }
>>> +
>>> +    /*
>>> +     * 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);
>>> +    if (rc)  {
>>> +        fprintf(stderr, "vscsi_send_capabilities: DMA write failure !\n");
>>> +    }
>>> +error_out:
>> 
>> This is just "out" rather than "error_out", no? We also get here when we 
>> don't hit an error.
> 
> Yes, the label is more readable at the goto, where we set the error
> return code. In case of no error return code is H_SUCCESS. So that we
> send a response back to the guest.

... which means we're not jumping into an error-only label.


Alex
>