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

[Nikunj A Dadhania]

Alexander Graf <address@hidden> writes:
>>>> +    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.

Sure

>
>> 
>>> 
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * 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.

True.

Regards
Nikunj