On Tue, 2016-09-13 at 17:09 +0200, Christophe Fergeau wrote:
> 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
Ok, I will remove it. I guess the intention was to check if value is
in the allowed range (I think the agent uses enums this way)
>
> > -};
> > +} 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:
hmm, it is better to be safe. I will change it to explicit checks
Thanks,
Pavel
>
> Christophe
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel