[Spice-devel] [PATCH spice v2 1/2] agent-filter: Differentiate xfer messages

[Pavel Grunt]

On Tue, 2017-01-10 at 06:47 -0500, Frediano Ziglio wrote:
> > 
> > To be able to inform the sender about the cancelled file transfer
> > 
> > Related:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1373725
> > ---
> >  server/agent-msg-filter.c            | 12 ++++++++++--
> >  server/agent-msg-filter.h            |  1 +
> >  server/reds.c                        |  6 ++++++
> >  server/tests/test-agent-msg-filter.c | 10 ++++++++--
> >  4 files changed, 25 insertions(+), 4 deletions(-)
> > 
> > diff --git a/server/agent-msg-filter.c b/server/agent-msg-filter.c
> > index 17f8e889..a83dee97 100644
> > --- a/server/agent-msg-filter.c
> > +++ b/server/agent-msg-filter.c
> > @@ -80,7 +80,15 @@ data_to_read:
> >      }
> >  
> >      if (filter->discard_all) {
> > -        filter->result = AGENT_MSG_FILTER_DISCARD;
> > +        switch (msg_header.type) {
> > +        case VD_AGENT_FILE_XFER_START:
> > +        case VD_AGENT_FILE_XFER_STATUS:
> > +        case VD_AGENT_FILE_XFER_DATA:
> > +            filter->result = AGENT_MSG_FILTER_DISCARD_XFER;
> > +            break;
> > +        default:
> > +            filter->result = AGENT_MSG_FILTER_DISCARD;
> > +        }
> >      } else {
> >          switch (msg_header.type) {
> >          case VD_AGENT_CLIPBOARD:
> > @@ -99,7 +107,7 @@ data_to_read:
> >              if (filter->file_xfer_enabled) {
> >                  filter->result = AGENT_MSG_FILTER_OK;
> >              } else {
> > -                filter->result = AGENT_MSG_FILTER_DISCARD;
> > +                filter->result = AGENT_MSG_FILTER_DISCARD_XFER;
> >              }
> >              break;
> >          case VD_AGENT_MONITORS_CONFIG:
> > diff --git a/server/agent-msg-filter.h b/server/agent-msg-filter.h
> > index b4d8e720..acbeaaaf 100644
> > --- a/server/agent-msg-filter.h
> > +++ b/server/agent-msg-filter.h
> > @@ -28,6 +28,7 @@
> >  typedef enum {
> >      AGENT_MSG_FILTER_OK,
> >      AGENT_MSG_FILTER_DISCARD,
> > +    AGENT_MSG_FILTER_DISCARD_XFER,
> >      AGENT_MSG_FILTER_PROTO_ERROR,
> >      AGENT_MSG_FILTER_MONITORS_CONFIG,
> >  } AgentMsgFilterResult;
> 
> I don't know why but I feel that the filter should just
> filter but looks like it starts classifying the messages
> too. It's not clear the responsibility of this filter code but
> if you ask me I would attempt to remove
> AGENT_MSG_FILTER_MONITORS_CONFIG
> instead of adding other classifications.
> 
I did it to avoid checking twice for the message type as you suggested
in v1

It is true that it is more a classifier than a filter.

Pavel

> > diff --git a/server/reds.c b/server/reds.c
> > index 3b30928a..b9f13e2e 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -803,6 +803,8 @@ static RedPipeItem
> > *vdi_port_read_one_msg_from_device(RedCharDevice *self,
> >              case AGENT_MSG_FILTER_MONITORS_CONFIG:
> >                  /* fall through */
> >              case AGENT_MSG_FILTER_DISCARD:
> > +                /* fall through */
> > +            case AGENT_MSG_FILTER_DISCARD_XFER:
> >                  red_pipe_item_unref(&dispatch_buf->base);
> >              }
> >          }
> > @@ -1111,6 +1113,8 @@ void reds_on_main_agent_data(RedsState
> > *reds,
> > MainChannelClient *mcc, void *mess
> >      case AGENT_MSG_FILTER_OK:
> >          break;
> >      case AGENT_MSG_FILTER_DISCARD:
> > +        /* fall through */
> > +    case AGENT_MSG_FILTER_DISCARD_XFER:
> >          return;
> >      case AGENT_MSG_FILTER_MONITORS_CONFIG:
> >          reds_on_main_agent_monitors_config(reds, mcc, message,
> > size);
> > @@ -1198,6 +1202,8 @@ void reds_on_main_channel_migrate(RedsState
> > *reds,
> > MainChannelClient *mcc)
> >          case AGENT_MSG_FILTER_MONITORS_CONFIG:
> >              /* fall through */
> >          case AGENT_MSG_FILTER_DISCARD:
> > +            /* fall through */
> > +        case AGENT_MSG_FILTER_DISCARD_XFER:
> >              red_pipe_item_unref(&read_buf->base);
> >          }
> >  
> > diff --git a/server/tests/test-agent-msg-filter.c
> > b/server/tests/test-agent-msg-filter.c
> > index 2f5568a6..ea8b9363 100644
> > --- a/server/tests/test-agent-msg-filter.c
> > +++ b/server/tests/test-agent-msg-filter.c
> > @@ -83,8 +83,12 @@ static void test_agent_msg_filter_run(void)
> >      msg.msg_header.protocol = VD_AGENT_PROTOCOL;
> >      for (type = VD_AGENT_MOUSE_STATE; type <
> > VD_AGENT_END_MESSAGE; type++) {
> >          msg.msg_header.type = type;
> > +        AgentMsgFilterResult filter_result =
> > AGENT_MSG_FILTER_DISCARD;
> > +        if (type >= VD_AGENT_FILE_XFER_START && type <=
> > VD_AGENT_FILE_XFER_DATA) {
> > +            filter_result = AGENT_MSG_FILTER_DISCARD_XFER;
> > +        }
> >          g_assert_cmpint(agent_msg_filter_process_data(&filter,
> > msg.data,
> >          len), ==,
> > -                        AGENT_MSG_FILTER_DISCARD);
> > +                        filter_result);
> >      }
> >  
> >      /* data exceeds size from header */
> > @@ -113,10 +117,12 @@ static void test_agent_msg_filter_run(void)
> >          case VD_AGENT_CLIPBOARD_GRAB:
> >          case VD_AGENT_CLIPBOARD_REQUEST:
> >          case VD_AGENT_CLIPBOARD_RELEASE:
> > +            result = AGENT_MSG_FILTER_DISCARD;
> > +            break;
> >          case VD_AGENT_FILE_XFER_START:
> >          case VD_AGENT_FILE_XFER_STATUS:
> >          case VD_AGENT_FILE_XFER_DATA:
> > -            result = AGENT_MSG_FILTER_DISCARD;
> > +            result = AGENT_MSG_FILTER_DISCARD_XFER;
> >              break;
> >          case VD_AGENT_MONITORS_CONFIG:
> >              result = AGENT_MSG_FILTER_MONITORS_CONFIG;
> 
> Frediano