> On 7 May 2018, at 11:40, Frediano Ziglio <fziglio@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
>>>>
>>>>>
>>>>> Thank you for that. Looks good after two minor grammatical fixes.
>>>>>
>>>>>> On 22 Mar 2018, at 11:12, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>>>>>
>>>>>> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
>>>>>> ---
>>>>>> 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;
>>>>>> };
>>>>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel