[Spice-devel] [PATCH v2] qxl: inline documentation for get_command and req_cmd_notification

[Uri Lublin]

On 01/26/2016 10:26 PM, Jonathon Jongsma wrote:
> From: Frediano Ziglio <fziglio at redhat.com>
>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
>
> Good idea. Here's a proposed re-wording of the documentation.
>
> Changes:
>   - for get_command(), I changed the verb from 'request' to 'retrieve' to make
>     it (I think) a bit more clear that it's getting the command and not simply
>     making a request. Also tweaked the wording a bit.
>   - for req_cmd_notification(), I dropped the part about "spice-server handled
>     all commands" since I think the documentation should just focus on the
>     function implementation, not about when it is called. I also added some more
>     details about exactly how the server gets notified after studying the code
>     for a while (thanks Uri!).

Hi,

Looks good to me with the some comments below.

>
>   server/spice-qxl.h | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/server/spice-qxl.h b/server/spice-qxl.h
> index e1f14e7..caa2693 100644
> --- a/server/spice-qxl.h
> +++ b/server/spice-qxl.h
> @@ -166,7 +166,15 @@ struct QXLInterface {
>       void (*set_mm_time)(QXLInstance *qin, uint32_t mm_time) SPICE_GNUC_DEPRECATED;
>
>       void (*get_init_info)(QXLInstance *qin, QXLDevInitInfo *info);

Consider adding an empty line here.

> +    /* Retrieve the next command to be processed
> +     * This call should be non-blocking. If no are commands available, it

If no commands are available                      ^^^

> +     * should return NULL */

This function returns int (boolean really) so
"If no commands are available it should return 0"

and maybe rewrite, adding -- or 1 (non-zero) if a command was retrieved.

>       int (*get_command)(QXLInstance *qin, struct QXLCommandExt *cmd);

Consider adding an empty line here


> +    /* Request notification when new commands are available
> +     * When a new command becomes available, the spice server should be
> +     * notified by calling spice_qxl_wakeup(). If commands are already
> +     * available, this function should return FALSE and no notification
> +     * triggered */
>       int (*req_cmd_notification)(QXLInstance *qin);
>       void (*release_resource)(QXLInstance *qin, struct QXLReleaseInfoExt release_info);
>       int (*get_cursor_command)(QXLInstance *qin, struct QXLCommandExt *cmd);
>