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

[Frediano Ziglio]

> 
> > 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;
> >>> };
> > 

Frediano