[Spice-devel] [PATCH spice-server] spice-char: Add some documentation to SpiceCharDeviceInterface

[Christophe de Dinechin]

> On 7 May 2018, at 11:40, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
> ping

Several minor enhancements were suggested. Could you share the updated text for review?


Thanks
Christophe

> 
>> 
>>> 
>>>> On 23 Mar 2018, at 17:20, Frediano Ziglio <fziglio at redhat.com> wrote:
>>>> 
>>>>> 
>>>>> Thank you for that. Looks good after two minor grammatical fixes.
>>>>> 
>>>>>> On 22 Mar 2018, at 11:12, Frediano Ziglio <fziglio at redhat.com> wrote:
>>>>>> 
>>>>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>>>>>> ---
>>>>>> server/spice-char.h | 21 +++++++++++++++++++++
>>>>>> 1 file changed, 21 insertions(+)
>>>>>> 
>>>>>> diff --git a/server/spice-char.h b/server/spice-char.h
>>>>>> index 1a8a031d..4d780eb7 100644
>>>>>> --- a/server/spice-char.h
>>>>>> +++ b/server/spice-char.h
>>>>>> @@ -40,9 +40,30 @@ typedef enum {
>>>>>> struct SpiceCharDeviceInterface {
>>>>>>   SpiceBaseInterface base;
>>>>>> 
>>>>>> +    /* Set the state of the device.
>>>>>> +     * connected should be 0 or 1.
>>>>>> +     * Setting state to 0 cause the device to be disabled.
>>>>> 
>>>>> Maybe document uses cases for that function?
>>>>> 
>>>>> Why does the function return a void? Can’t fail? Can you always change
>>>>> the
>>>>> state? (Does not look like an open/close to me)
>>>>> 
>>>> 
>>>> Not sure but I suppose the function cannot fail.
>>>> Is not a open/close, the open/close is more a guest aspect.
>>>> I cannot find an easy analogy, we (spice server code) are kind of
>>>> implementing
>>>> a device (like a kernel module) and we basically are calling a kernel
>>>> function
>>>> to tell that this device is not available.
>>>> The streaming device for instance uses it to enable/disable the guest
>>>> device.
>>>> 
>>>>>> +     */
>>>>>>   void (*state)(SpiceCharDeviceInstance *sin, int connected);
>>>>>> +
>>>>>> +    /* Write some bytes to the character device.
>>>>>> +     * Returns bytes copied from buf or a value < 0 on errors.
>>>>>> +     * Function can return a value < len, even 0.
>>>>>> +     * errno is not determined after calling this function.
>>>>>> +     * Function should be implemented as no-blocking.
>>>>>> +     * A len < 0 cause indeterminate results.
>>>>> 
>>>>> cause -> causes
>>>>> 
>>>> 
>>>> updated here and below.
>>>> 
>>>>> I suspect this is not an atomic operation, so you may have written some
>>>>> stuff
>>>>> even if the result is < 0?
>>>>> 
>>>> 
>>>> By atomic you mean "Function can return a value < len, even 0." or
>>>> something related
>>>> to threads?
>>> 
>>> The former. I was thinking of partial writes followed by an error, making
>>> the
>>> write non-atomic from the point of view of the caller.
>>> 
>>> In other words, say I write “HelloWorld”, and it fails at the W, I am
>>> wondering if there is a guarantee that write will return 5, and then that
>>> the retry will return < 0, or if it’s possible that it returns < 0 right
>>> away.
>>> 
>>> Probably the best way to know is to dig in the code ;-)
>>> 
>> 
>> I don't think we should document the actual implementation but more the
>> contract,
>> is supposed to be an interface.
>> I think both returning a 0 <= value < len or < 0 should be fine in case of
>> partial
>> read/write.
>> 
>>>> Or maybe a comment that say that if you get an error after handling some
>>>> bytes the
>>>> function should return that amount of bytes not returning an error?
>>>> No idea what we expect in this case, the only reason I can see this would
>>>> happen is when the guest closes the device but I'm not sure what happens
>>>> with current Qemu code and what we should expect. I personally would
>>>> expect
>>>> that we could receive last bytes sent by the guest.
>>> 
>>>> 
>>>>> 
>>>>>> +     */
>>>>>>   int (*write)(SpiceCharDeviceInstance *sin, const uint8_t *buf, int
>>>>>>   len);
>>>>>> +
>>>>>> +    /* Read some bytes from the character device.
>>>>>> +     * Returns bytes copied into buf or a value < 0 on errors.
>>>>>> +     * Function can return 0 if no data is available or len is 0.
>>>>>> +     * errno is not determined after calling this function.
>>>>>> +     * Function should be implemented as no-blocking.
>>>>>> +     * A len < 0 cause indeterminate results.
>>>>> 
>>>>> cause -> causes
>>>>> 
>>>>>> +     */
>>>>>>   int (*read)(SpiceCharDeviceInstance *sin, uint8_t *buf, int len);
>>>>>> +
>>>>>>   void (*event)(SpiceCharDeviceInstance *sin, uint8_t event);
>>>>>>   spice_char_device_flags flags;
>>>>>> };
>>>>