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

[Frediano Ziglio]

ping

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