[Spice-devel] [PATCH spice] agent-filter: Use enum as return value

[Christophe Fergeau]

Hey,

Looks good, couple of comments:

On Mon, Sep 12, 2016 at 07:25:12PM +0200, Pavel Grunt wrote:
> ---
>  server/agent-msg-filter.c |  4 ++--
>  server/agent-msg-filter.h | 10 +++++-----
>  server/reds.c             |  8 +++++---
>  3 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/server/agent-msg-filter.c b/server/agent-msg-filter.c
> index a11f624..7921fe7 100644
> --- a/server/agent-msg-filter.c
> +++ b/server/agent-msg-filter.c
> @@ -48,8 +48,8 @@ void agent_msg_filter_init(AgentMsgFilter *filter,
>      filter->discard_all = discard_all;
>  }
>  
> -int agent_msg_filter_process_data(AgentMsgFilter *filter,
> -                                  const uint8_t *data, uint32_t len)
> +AgentMsgFilterResult agent_msg_filter_process_data(AgentMsgFilter *filter,
> +                                                   const uint8_t *data, uint32_t len)
>  {
>      struct VDAgentMessage msg_header;
>  
> diff --git a/server/agent-msg-filter.h b/server/agent-msg-filter.h
> index d61f8d4..ddcf0ae 100644
> --- a/server/agent-msg-filter.h
> +++ b/server/agent-msg-filter.h
> @@ -25,17 +25,17 @@
>  #include <glib.h>
>  
>  /* Possible return values for agent_msg_filter_process_data */
> -enum {
> +typedef enum {
>      AGENT_MSG_FILTER_OK,
>      AGENT_MSG_FILTER_DISCARD,
>      AGENT_MSG_FILTER_PROTO_ERROR,
>      AGENT_MSG_FILTER_MONITORS_CONFIG,
>      AGENT_MSG_FILTER_END

AGENT_MSG_FILTER_END does not seem to be used

> -};
> +} AgentMsgFilterResult;
>  
>  typedef struct AgentMsgFilter {
>      int msg_data_to_read;
> -    int result;
> +    AgentMsgFilterResult result;
>      gboolean copy_paste_enabled;
>      gboolean file_xfer_enabled;
>      gboolean use_client_monitors_config;
> @@ -49,7 +49,7 @@ void agent_msg_filter_init(AgentMsgFilter *filter,
>  void agent_msg_filter_config(AgentMsgFilter *filter,
>                               gboolean copy_paste, gboolean file_xfer,
>                               gboolean use_client_monitors_config);
> -int agent_msg_filter_process_data(AgentMsgFilter *filter,
> -                                  const uint8_t *data, uint32_t len);
> +AgentMsgFilterResult agent_msg_filter_process_data(AgentMsgFilter *filter,
> +                                                   const uint8_t *data, uint32_t len);
>  
>  #endif
> diff --git a/server/reds.c b/server/reds.c
> index 800107b..61f03c8 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -767,7 +767,7 @@ static void vdi_port_read_buf_release(uint8_t *data, void *opaque)
>  static gboolean vdi_port_read_buf_process(RedCharDeviceVDIPort *dev,
>                                            RedVDIReadBuf *buf, gboolean *error)
>  {
> -    int res;
> +    AgentMsgFilterResult res;
>  
>      *error = FALSE;
>  
> @@ -780,7 +780,7 @@ static gboolean vdi_port_read_buf_process(RedCharDeviceVDIPort *dev,
>              return TRUE;
>          case AGENT_MSG_FILTER_DISCARD:
>              return FALSE;
> -        case AGENT_MSG_FILTER_PROTO_ERROR:
> +        default:


I'm always a bit torn on whether to use default: or not. On one hand
it's convenient, on the other hand this means if we ever add a new enum
member that we should be handling there, we'll never get a warning
because of the use of default:
I think I'd go with explicitly listing the current enum members though.
Fine with me if you prefer to stick with default:

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160913/50ca931d/attachment.sig>