[Spice-devel] [RFC v4 20/62] server/main_channel: support multiple clients

Alon Levy alevy at redhat.com
Tue May 3 00:21:28 PDT 2011


On Tue, May 03, 2011 at 01:52:31AM +0200, Marc-André Lureau wrote:
> The ref system is not clear, why is it heap allocated? Some
> documentation about that Refs type would help.
> 
> Having explicit _ref() and _unref() methods would make things more
> clear. Can it be pushed higher in the object hierarchy instead of
> having new Refs types?

But the _ref part here is done in one go - I set the refs to the number
of clients (which is known at pipe item creation time).

The RefsPipeItem is badly named - I assumed I'd use that in more places,
but it ends up just being a SwitchHostPipeItem. The purpose of the refs
is to have a "item sent to all clients" event, as you can see in the free
function (--ref, and only if 0 then do something). I should probably just
make this a SwitchHostPipeItem and comment on the refs.

I think using glib / gobject is a good idea, just don't see how it helps
this particular case (because of the assymetry in reference set once and
reference remove multiple times).

> 
> I am a bit skeptical reading:
> 
> switch (base_item->type)
> ....
> case SPICE_MSG_MAIN_AGENT_DATA:
> AgentDataPipeItem *data = (AgentDataPipeItem*)base;
> if (!--data->refs->refs)
> ....
> case SPICE_MSG_MAIN_MIGRATE_SWITCH_HOST:
> RefsPipeItem *data = (RefsPipeItem*)base;
> if (!--*(data->refs)) {
> 
> Passing item creation arguments into Info structs doesn't make things
> easier to read, and duplicate code (more assignments, more type
> declaration with similar fields, more things to instanciate...). It
> should be possible to simplify PipeItem creation, for example, I don't
> see why we are passing RedChannelClient or RedChannel.

Ok, I thought the structs were easier, but I can remove those.

Regarding the RCC / RC argument, I think it's there as a "just in case", I
don't remember if it's actually used.

> 
> There is many unused "num" arguments.
> 


> On Tue, Apr 26, 2011 at 12:54 PM, Alon Levy <alevy at redhat.com> wrote:
> > Notable TODOs:
> >  * migration testing
> 
> Simple migration seems to work.
> 
> >  * agent tokens are wrongly sent (or did I fix that? check)
> 
> In which case? I have been doing basic tests with a single client, it
> seems to work fine.
> 
> > ---
> >  server/main_channel.c |  392 ++++++++++++++++++++++++++++---------------------
> >  server/main_channel.h |    8 +-
> >  server/reds.c         |    7 +-
> >  server/reds.h         |    2 +-
> >  4 files changed, 231 insertions(+), 178 deletions(-)
> >
> > diff --git a/server/main_channel.c b/server/main_channel.c
> > index 8ba0978..74c7f40 100644
> > --- a/server/main_channel.c
> > +++ b/server/main_channel.c
> > @@ -61,6 +61,11 @@ struct RedsOutItem {
> >     PipeItem base;
> >  };
> >
> > +typedef struct RefsPipeItem {
> > +    PipeItem base;
> > +    int *refs;
> > +} RefsPipeItem;
> > +
> >  typedef struct PingPipeItem {
> >     PipeItem base;
> >     int size;
> > @@ -77,8 +82,13 @@ typedef struct TokensPipeItem {
> >     int tokens;
> >  } TokensPipeItem;
> >
> > +typedef struct AgentDataPipeItemRefs {
> > +    int refs;
> > +} AgentDataPipeItemRefs;
> > +
> >  typedef struct AgentDataPipeItem {
> >     PipeItem base;
> > +    AgentDataPipeItemRefs *refs;
> >     uint8_t* data;
> >     size_t len;
> >     spice_marshaller_item_free_func free_data;
> > @@ -152,25 +162,27 @@ static void main_disconnect(MainChannel *main_chan)
> >     red_channel_destroy(&main_chan->base);
> >  }
> >
> > +#define MAIN_FOREACH(_link, _main, _mcc) \
> > +    if ((_main) && ((_mcc) = \
> > +        SPICE_CONTAINEROF((_main)->base.rcc, MainChannelClient, base)))
> 
> Used only once, perhaps we don't need a macro for a single usage.
> 
> _main != NULL should be checked before,
> _link is unused
> 
> >  RedClient *main_channel_get_client_by_link_id(MainChannel *main_chan, uint32_t link_id)
> >  {
> >     MainChannelClient *mcc;
> >
> > -    if (!main_chan || !main_chan->base.rcc) {
> > -        return NULL;
> > -    }
> > -    mcc = SPICE_CONTAINEROF(main_chan->base.rcc, MainChannelClient, base);
> > -    if (mcc->link_id == link_id) {
> > -        return mcc->base.client;
> > +    MAIN_FOREACH(link, main_chan, mcc) {
> > +        if (mcc->link_id == link_id) {
> > +            return mcc->base.client;
> > +        }
> >     }
> >     return NULL;
> >  }
> >
> > -static int main_channel_client_push_ping(MainChannelClient *rcc, int size);
> > +static int main_channel_client_push_ping(MainChannelClient *mcc, int size);
> >
> > -void main_channel_start_net_test(MainChannelClient *mcc)
> > +void main_channel_client_start_net_test(MainChannelClient *mcc)
> >  {
> > -    if (!mcc) {
> > +    if (!mcc || mcc->net_test_id) {
> >         return;
> >     }
> >     if (main_channel_client_push_ping(mcc, NET_TEST_WARMUP_BYTES)
> > @@ -181,67 +193,89 @@ void main_channel_start_net_test(MainChannelClient *mcc)
> >     }
> >  }
> >
> > -static RedsOutItem *main_pipe_item_new(RedChannelClient *rcc, int type)
> > +static PipeItem *main_pipe_item_new_broadcast(RedChannelClient *rcc, void *data, int num)
> > +{
> > +    RedsOutItem *item = spice_malloc(sizeof(RedsOutItem));
> > +
> > +    red_channel_pipe_item_init(rcc->channel, &item->base, (uint64_t)data);
> > +    return &item->base;
> > +}
> > +
> 
> so what is num for?
> 
> Why do we have
> 
> main_pipe_item_new() with: int type
> main_pipe_item_new_broadcast() with: void *data that is cast to (uint64_t)
> 

It used to be used through red_channel_pipes_new_add_push but I see now
that it isn't any more, so I'll drop those num's where I can.

> Could we have just one fonction with a "bool broadcast" instead?

Or I could do this.

> 
> > +static PipeItem *main_pipe_item_new(MainChannelClient *mcc, int type)
> >  {
> >     RedsOutItem *item = spice_malloc(sizeof(RedsOutItem));
> >
> > -    red_channel_pipe_item_init(rcc->channel, &item->base, type);
> > -    return item;
> > +    red_channel_pipe_item_init(mcc->base.channel, &item->base, type);
> > +    return &item->base;
> >  }
> >
> > -static MouseModePipeItem *main_mouse_mode_item_new(RedChannelClient *rcc,
> > -    int current_mode, int is_client_mouse_allowed)
> > +typedef struct MainMouseModeItemInfo {
> > +    int current_mode;
> > +    int is_client_mouse_allowed;
> > +} MainMouseModeItemInfo;
> > +
> > +static PipeItem *main_mouse_mode_item_new(RedChannelClient *rcc, void *data, int num)
> >  {
> >     MouseModePipeItem *item = spice_malloc(sizeof(MouseModePipeItem));
> > +    MainMouseModeItemInfo *info = data;
> >
> >     red_channel_pipe_item_init(rcc->channel, &item->base,
> >                                SPICE_MSG_MAIN_MOUSE_MODE);
> > -    item->current_mode = current_mode;
> > -    item->is_client_mouse_allowed = is_client_mouse_allowed;
> > -    return item;
> > +    item->current_mode = info->current_mode;
> > +    item->is_client_mouse_allowed = info->is_client_mouse_allowed;
> > +    return &item->base;
> >  }
> >
> > -static PingPipeItem *main_ping_item_new(RedChannelClient *rcc, int size)
> > +static PipeItem *main_ping_item_new(MainChannelClient *mcc, int size)
> >  {
> >     PingPipeItem *item = spice_malloc(sizeof(PingPipeItem));
> >
> > -    red_channel_pipe_item_init(rcc->channel, &item->base, SPICE_MSG_PING);
> > +    red_channel_pipe_item_init(mcc->base.channel, &item->base, SPICE_MSG_PING);
> >     item->size = size;
> > -    return item;
> > +    return &item->base;
> >  }
> >
> > -static TokensPipeItem *main_tokens_item_new(RedChannelClient *rcc, int tokens)
> > +static PipeItem *main_tokens_item_new(RedChannelClient *rcc, void *data, int num)
> >  {
> >     TokensPipeItem *item = spice_malloc(sizeof(TokensPipeItem));
> >
> >     red_channel_pipe_item_init(rcc->channel, &item->base,
> >                                SPICE_MSG_MAIN_AGENT_TOKEN);
> > -    item->tokens = tokens;
> > -    return item;
> > +    item->tokens = (uint64_t)data;
> > +    return &item->base;
> >  }
> >
> > -static AgentDataPipeItem *main_agent_data_item_new(RedChannelClient *rcc,
> > -           uint8_t* data, size_t len,
> > -           spice_marshaller_item_free_func free_data, void *opaque)
> > +typedef struct MainAgentDataItemInfo {
> > +    uint8_t* data;
> > +    size_t len;
> > +    spice_marshaller_item_free_func free_data;
> > +    void *opaque;
> > +    AgentDataPipeItemRefs *refs;
> > +} MainAgentDataItemInfo;
> > +
> > +static PipeItem *main_agent_data_item_new(RedChannelClient *rcc, void *data, int num)
> >  {
> > +    MainAgentDataItemInfo *info = data;
> >     AgentDataPipeItem *item = spice_malloc(sizeof(AgentDataPipeItem));
> >
> > -    red_channel_pipe_item_init(rcc->channel, &item->base, SPICE_MSG_MAIN_AGENT_DATA);
> > -    item->data = data;
> > -    item->len = len;
> > -    item->free_data = free_data;
> > -    item->opaque = opaque;
> > -    return item;
> > +    red_channel_pipe_item_init(rcc->channel, &item->base,
> > +                               SPICE_MSG_MAIN_AGENT_DATA);
> > +    item->refs = info->refs;
> > +    item->data = info->data;
> > +    item->len = info->len;
> > +    item->free_data = info->free_data;
> > +    item->opaque = info->opaque;
> > +    return &item->base;
> >  }
> >
> > -static InitPipeItem *main_init_item_new(RedChannelClient *rcc,
> > +static PipeItem *main_init_item_new(MainChannelClient *mcc,
> >     int connection_id, int display_channels_hint, int current_mouse_mode,
> >     int is_client_mouse_allowed, int multi_media_time,
> >     int ram_hint)
> >  {
> >     InitPipeItem *item = spice_malloc(sizeof(InitPipeItem));
> >
> > -    red_channel_pipe_item_init(rcc->channel, &item->base,
> > +    red_channel_pipe_item_init(mcc->base.channel, &item->base,
> >                                SPICE_MSG_MAIN_INIT);
> >     item->connection_id = connection_id;
> >     item->display_channels_hint = display_channels_hint;
> > @@ -249,57 +283,70 @@ static InitPipeItem *main_init_item_new(RedChannelClient *rcc,
> >     item->is_client_mouse_allowed = is_client_mouse_allowed;
> >     item->multi_media_time = multi_media_time;
> >     item->ram_hint = ram_hint;
> > -    return item;
> > +    return &item->base;
> >  }
> >
> > -static NotifyPipeItem *main_notify_item_new(RedChannelClient *rcc,
> > -                                        uint8_t *mess, const int mess_len)
> > +typedef struct NotifyPipeInfo {
> > +    uint8_t *mess;
> > +    int mess_len;
> > +} NotifyPipeInfo;
> > +
> > +static PipeItem *main_notify_item_new(RedChannelClient *rcc, void *data, int num)
> >  {
> >     NotifyPipeItem *item = spice_malloc(sizeof(NotifyPipeItem));
> > +    NotifyPipeInfo *info = data;
> >
> >     red_channel_pipe_item_init(rcc->channel, &item->base,
> >                                SPICE_MSG_NOTIFY);
> > -    item->mess = mess;
> > -    item->mess_len = mess_len;
> > -    return item;
> > +    item->mess = info->mess;
> > +    item->mess_len = info->mess_len;
> > +    return &item->base;
> >  }
> >
> > -static MigrateBeginPipeItem *main_migrate_begin_item_new(
> > -    RedChannelClient *rcc, int port, int sport,
> > -    char *host, uint16_t cert_pub_key_type, uint32_t cert_pub_key_len,
> > -    uint8_t *cert_pub_key)
> > +typedef struct MigrateBeginItemInfo {
> > +    int port;
> > +    int sport;
> > +    char *host;
> > +    uint16_t cert_pub_key_type;
> > +    uint32_t cert_pub_key_len;
> > +    uint8_t *cert_pub_key;
> > +} MigrateBeginItemInfo;
> > +
> > +// TODO: MC: migration is not tested at all with multiclient.
> > +static PipeItem *main_migrate_begin_item_new(
> > +    RedChannelClient *rcc, void *data, int num)
> >  {
> >     MigrateBeginPipeItem *item = spice_malloc(sizeof(MigrateBeginPipeItem));
> > +    MigrateBeginItemInfo *info = data;
> >
> >     red_channel_pipe_item_init(rcc->channel, &item->base,
> >                                SPICE_MSG_MAIN_MIGRATE_BEGIN);
> > -    item->port = port;
> > -    item->sport = sport;
> > -    item->host = host;
> > -    item->cert_pub_key_type = cert_pub_key_type;
> > -    item->cert_pub_key_len = cert_pub_key_len;
> > -    item->cert_pub_key = cert_pub_key;
> > -    return item;
> > +    item->port = info->port;
> > +    item->sport = info->sport;
> > +    item->host = info->host;
> > +    item->cert_pub_key_type = info->cert_pub_key_type;
> > +    item->cert_pub_key_len = info->cert_pub_key_len;
> > +    item->cert_pub_key = info->cert_pub_key;
> > +    return &item->base;
> >  }
> >
> > -static MultiMediaTimePipeItem *main_multi_media_time_item_new(
> > -    RedChannelClient *rcc, int time)
> > +static PipeItem *main_multi_media_time_item_new(
> > +    RedChannelClient *rcc, void *data, int num)
> >  {
> > -    MultiMediaTimePipeItem *item;
> > +    MultiMediaTimePipeItem *item, *info = data;
> >
> >     item = spice_malloc(sizeof(MultiMediaTimePipeItem));
> >     red_channel_pipe_item_init(rcc->channel, &item->base,
> >                                SPICE_MSG_MAIN_MULTI_MEDIA_TIME);
> > -    item->time = time;
> > -    return item;
> > +    item->time = info->time;
> > +    return &item->base;
> >  }
> >
> > -static void main_channel_push_channels(RedChannelClient *rcc)
> > +static void main_channel_push_channels(MainChannelClient *mcc)
> >  {
> > -    RedsOutItem *item;
> > +    PipeItem *item = main_pipe_item_new(mcc, SPICE_MSG_MAIN_CHANNELS_LIST);
> >
> > -    item = main_pipe_item_new(rcc, SPICE_MSG_MAIN_CHANNELS_LIST);
> > -    red_channel_client_pipe_add_push(rcc, &item->base);
> > +    red_channel_client_pipe_add_push(&mcc->base, item);
> >  }
> >
> >  static void main_channel_marshall_channels(SpiceMarshaller *m)
> > @@ -315,20 +362,14 @@ static void main_channel_marshall_channels(SpiceMarshaller *m)
> >
> >  int main_channel_client_push_ping(MainChannelClient *mcc, int size)
> >  {
> > -    PingPipeItem *item;
> > +    PipeItem *item;
> >
> > -    item = main_ping_item_new(&mcc->base, size);
> > -    red_channel_client_pipe_add_push(&mcc->base, &item->base);
> > -    return TRUE;
> > -}
> > -
> > -int main_channel_push_ping(MainChannel *main_chan, int size)
> > -{
> > -    if (main_chan->base.rcc == NULL) {
> > +    if (mcc == NULL) {
> >         return FALSE;
> >     }
> > -    return main_channel_client_push_ping(
> > -        SPICE_CONTAINEROF(main_chan->base.rcc, MainChannelClient, base), size);
> > +    item = main_ping_item_new(mcc, size);
> > +    red_channel_client_pipe_add_push(&mcc->base, item);
> > +    return TRUE;
> >  }
> >
> >  static void main_channel_marshall_ping(SpiceMarshaller *m, int size, int ping_id)
> > @@ -348,26 +389,16 @@ static void main_channel_marshall_ping(SpiceMarshaller *m, int size, int ping_id
> >     }
> >  }
> >
> > -static void main_channel_client_push_mouse_mode(RedChannelClient *rcc, int current_mode,
> > -                                         int is_client_mouse_allowed);
> > -
> >  void main_channel_push_mouse_mode(MainChannel *main_chan, int current_mode,
> >                                   int is_client_mouse_allowed)
> >  {
> > -    if (main_chan && main_chan->base.rcc != NULL) {
> > -        main_channel_client_push_mouse_mode(main_chan->base.rcc, current_mode,
> > -                                            is_client_mouse_allowed);
> > -    }
> > -}
> > -
> > -static void main_channel_client_push_mouse_mode(RedChannelClient *rcc, int current_mode,
> > -                                         int is_client_mouse_allowed)
> > -{
> > -    MouseModePipeItem *item;
> > +    MainMouseModeItemInfo info = {
> > +        .current_mode=current_mode,
> > +        .is_client_mouse_allowed=is_client_mouse_allowed,
> > +    };
> >
> > -    item = main_mouse_mode_item_new(rcc, current_mode,
> > -                                    is_client_mouse_allowed);
> > -    red_channel_client_pipe_add_push(rcc, &item->base);
> > +    red_channel_pipes_new_add_push(&main_chan->base,
> > +        main_mouse_mode_item_new, &info);
> >  }
> >
> >  static void main_channel_marshall_mouse_mode(SpiceMarshaller *m, int current_mode, int is_client_mouse_allowed)
> > @@ -383,23 +414,14 @@ static void main_channel_marshall_mouse_mode(SpiceMarshaller *m, int current_mod
> >
> >  void main_channel_push_agent_connected(MainChannel *main_chan)
> >  {
> > -    RedsOutItem *item;
> > -    RedChannelClient *rcc = main_chan->base.rcc;
> > -
> > -    item = main_pipe_item_new(rcc, SPICE_MSG_MAIN_AGENT_CONNECTED);
> > -    red_channel_client_pipe_add_push(rcc, &item->base);
> > +    red_channel_pipes_new_add_push(&main_chan->base,
> > +        main_pipe_item_new_broadcast, (void*)SPICE_MSG_MAIN_AGENT_CONNECTED);
> >  }
> >
> >  void main_channel_push_agent_disconnected(MainChannel *main_chan)
> >  {
> > -    RedsOutItem *item;
> > -    RedChannelClient *rcc = main_chan->base.rcc;
> > -
> > -    if (!rcc) {
> > -        return;
> > -    }
> > -    item = main_pipe_item_new(rcc, SPICE_MSG_MAIN_AGENT_DISCONNECTED);
> > -    red_channel_client_pipe_add_push(rcc, &item->base);
> > +    red_channel_pipes_new_add_push(&main_chan->base,
> > +        main_pipe_item_new_broadcast, (void*)SPICE_MSG_MAIN_AGENT_DISCONNECTED);
> >  }
> >
> >  static void main_channel_marshall_agent_disconnected(SpiceMarshaller *m)
> > @@ -410,16 +432,11 @@ static void main_channel_marshall_agent_disconnected(SpiceMarshaller *m)
> >     spice_marshall_msg_main_agent_disconnected(m, &disconnect);
> >  }
> >
> > +// TODO: make this targeted (requires change to agent token accounting)
> >  void main_channel_push_tokens(MainChannel *main_chan, uint32_t num_tokens)
> >  {
> > -    TokensPipeItem *item;
> > -    RedChannelClient *rcc = main_chan->base.rcc;
> > -
> > -    if (!rcc) {
> > -        return;
> > -    }
> > -    item = main_tokens_item_new(rcc, num_tokens);
> > -    red_channel_client_pipe_add_push(rcc, &item->base);
> > +    red_channel_pipes_new_add_push(&main_chan->base,
> > +        main_tokens_item_new, (void*)(uint64_t)num_tokens);
> >  }
> 
> 
> (void*)(uint64_t)num_tokens
> 
> Can we avoid writing 64bits specific code?

Yes, I'll make that a struct.

> 
> >  static void main_channel_marshall_tokens(SpiceMarshaller *m, uint32_t num_tokens)
> > @@ -433,30 +450,29 @@ static void main_channel_marshall_tokens(SpiceMarshaller *m, uint32_t num_tokens
> >  void main_channel_push_agent_data(MainChannel *main_chan, uint8_t* data, size_t len,
> >            spice_marshaller_item_free_func free_data, void *opaque)
> >  {
> > -    RedChannelClient *rcc = main_chan->base.rcc;
> > -    AgentDataPipeItem *item;
> > +    MainAgentDataItemInfo info = {
> > +        .data = data,
> > +        .len = len,
> > +        .free_data = free_data,
> > +        .opaque = opaque,
> > +        .refs = spice_malloc(sizeof(AgentDataPipeItemRefs)),
> > +    };
> >
> > -    item = main_agent_data_item_new(rcc, data, len, free_data, opaque);
> > -    red_channel_client_pipe_add_push(rcc, &item->base);
> > +    info.refs->refs = main_chan->base.clients_num;
> > +    red_channel_pipes_new_add_push(&main_chan->base,
> > +        main_agent_data_item_new, &info);
> >  }
> >
> >  static void main_channel_marshall_agent_data(SpiceMarshaller *m,
> >                                   AgentDataPipeItem *item)
> >  {
> > -    spice_marshaller_add_ref_full(m,
> > -        item->data, item->len, item->free_data, item->opaque);
> > +    spice_marshaller_add_ref(m, item->data, item->len);
> >  }
> >
> >  static void main_channel_push_migrate_data_item(MainChannel *main_chan)
> >  {
> > -    RedsOutItem *item;
> > -    RedChannelClient *rcc = main_chan->base.rcc;
> > -
> > -    if (!rcc) {
> > -        return;
> > -    }
> > -    item = main_pipe_item_new(rcc, SPICE_MSG_MIGRATE_DATA);
> > -    red_channel_client_pipe_add_push(rcc, &item->base);
> > +    red_channel_pipes_new_add_push(&main_chan->base,
> > +        main_pipe_item_new_broadcast, (void*)(uint64_t)SPICE_MSG_MIGRATE_DATA);
> >  }
> >
> >  static void main_channel_marshall_migrate_data_item(SpiceMarshaller *m, int serial, int ping_id)
> > @@ -468,8 +484,7 @@ static void main_channel_marshall_migrate_data_item(SpiceMarshaller *m, int seri
> >     data->ping_id = ping_id;
> >  }
> >
> > -static uint64_t main_channel_handle_migrate_data_get_serial_proc(
> > -    RedChannelClient *rcc,
> > +static uint64_t main_channel_handle_migrate_data_get_serial(RedChannelClient *base,
> >     uint32_t size, void *message)
> >  {
> >     MainMigrateData *data = message;
> > @@ -481,10 +496,10 @@ static uint64_t main_channel_handle_migrate_data_get_serial_proc(
> >     return data->serial;
> >  }
> >
> > -static uint64_t main_channel_handle_migrate_data(RedChannelClient *rcc,
> > +static uint64_t main_channel_handle_migrate_data(RedChannelClient *base,
> >     uint32_t size, void *message)
> >  {
> > -    MainChannelClient *mcc = SPICE_CONTAINEROF(rcc, MainChannelClient, base);
> > +    MainChannelClient *mcc = SPICE_CONTAINEROF(base, MainChannelClient, base);
> >     MainMigrateData *data = message;
> >
> >     if (size < sizeof(*data)) {
> > @@ -501,18 +516,18 @@ void main_channel_push_init(MainChannelClient *mcc, int connection_id,
> >     int is_client_mouse_allowed, int multi_media_time,
> >     int ram_hint)
> >  {
> > -    InitPipeItem *item;
> > +    PipeItem *item;
> >
> > -    item = main_init_item_new(&mcc->base,
> > +    item = main_init_item_new(mcc,
> >              connection_id, display_channels_hint, current_mouse_mode,
> >              is_client_mouse_allowed, multi_media_time, ram_hint);
> > -    red_channel_client_pipe_add_push(&mcc->base, &item->base);
> > +    red_channel_client_pipe_add_push(&mcc->base, item);
> >  }
> >
> >  static void main_channel_marshall_init(SpiceMarshaller *m,
> >                                        InitPipeItem *item)
> >  {
> > -    SpiceMsgMainInit init;
> > +    SpiceMsgMainInit init; // TODO - remove this copy, make InitPipeItem reuse SpiceMsgMainInit
> >
> >     init.session_id = item->connection_id;
> >     init.display_channels_hint = item->display_channels_hint;
> > @@ -528,13 +543,16 @@ static void main_channel_marshall_init(SpiceMarshaller *m,
> >     spice_marshall_msg_main_init(m, &init);
> >  }
> >
> > +// TODO - some notifications are new client only (like "keyboard is insecure" on startup)
> >  void main_channel_push_notify(MainChannel *main_chan, uint8_t *mess, const int mess_len)
> >  {
> > -    RedChannelClient *rcc = main_chan->base.rcc;
> > -    NotifyPipeItem *item;
> > -
> > -    item = main_notify_item_new(rcc, mess, mess_len);
> > -    red_channel_client_pipe_add_push(rcc, &item->base);
> > +    NotifyPipeInfo info = {
> > +        .mess = mess,
> > +        .mess_len = mess_len,
> > +    };
> > +
> > +    red_channel_pipes_new_add_push(&main_chan->base,
> > +        main_notify_item_new, &info);
> >  }
> >
> >  static void main_channel_marshall_notify(SpiceMarshaller *m, NotifyPipeItem *item)
> > @@ -554,12 +572,17 @@ void main_channel_push_migrate_begin(MainChannel *main_chan, int port, int sport
> >     char *host, uint16_t cert_pub_key_type, uint32_t cert_pub_key_len,
> >     uint8_t *cert_pub_key)
> >  {
> > -    RedChannelClient *rcc = main_chan->base.rcc;
> > -    MigrateBeginPipeItem *item;
> > -
> > -    item = main_migrate_begin_item_new(rcc, port,
> > -        sport, host, cert_pub_key_type, cert_pub_key_len, cert_pub_key);
> > -    red_channel_client_pipe_add_push(rcc, &item->base);
> > +    MigrateBeginItemInfo info = {
> > +        .port =port,
> > +        .sport = sport,
> > +        .host = host,
> > +        .cert_pub_key_type = cert_pub_key_type,
> > +        .cert_pub_key_len = cert_pub_key_len,
> > +        .cert_pub_key = cert_pub_key,
> > +    };
> > +
> > +    red_channel_pipes_new_add_push(&main_chan->base,
> > +        main_migrate_begin_item_new, &info);
> >  }
> >
> >  static void main_channel_marshall_migrate_begin(SpiceMarshaller *m,
> > @@ -579,11 +602,8 @@ static void main_channel_marshall_migrate_begin(SpiceMarshaller *m,
> >
> >  void main_channel_push_migrate(MainChannel *main_chan)
> >  {
> > -    RedChannelClient *rcc = main_chan->base.rcc;
> > -    RedsOutItem *item;
> > -
> > -    item = main_pipe_item_new(rcc, SPICE_MSG_MIGRATE);
> > -    red_channel_client_pipe_add_push(rcc, &item->base);
> > +    red_channel_pipes_new_add_push(&main_chan->base,
> > +        main_pipe_item_new_broadcast, (void*)(uint64_t)SPICE_MSG_MIGRATE);
> >  }
> >
> >  static void main_channel_marshall_migrate(SpiceMarshaller *m)
> > @@ -596,37 +616,38 @@ static void main_channel_marshall_migrate(SpiceMarshaller *m)
> >
> >  void main_channel_push_migrate_cancel(MainChannel *main_chan)
> >  {
> > -    RedChannelClient *rcc = main_chan->base.rcc;
> > -    RedsOutItem *item;
> > -
> > -    item = main_pipe_item_new(rcc, SPICE_MSG_MAIN_MIGRATE_CANCEL);
> > -    red_channel_client_pipe_add_push(rcc, &item->base);
> > +    red_channel_pipes_new_add_push(&main_chan->base,
> > +        main_pipe_item_new_broadcast, (void*)SPICE_MSG_MAIN_MIGRATE_CANCEL);
> >  }
> >
> >  void main_channel_push_multi_media_time(MainChannel *main_chan, int time)
> >  {
> > -    RedChannelClient *rcc = main_chan->base.rcc;
> > -    MultiMediaTimePipeItem *item;
> > +    MultiMediaTimePipeItem info = {
> > +        .time = time,
> > +    };
> >
> > -    item =main_multi_media_time_item_new(rcc, time);
> > -    red_channel_client_pipe_add_push(rcc, &item->base);
> > +    red_channel_pipes_new_add_push(&main_chan->base,
> > +        main_multi_media_time_item_new, &info);
> >  }
> >
> > -static PipeItem *main_migrate_switch_item_new(MainChannel *main_chan)
> > +static PipeItem *main_migrate_switch_item_new(RedChannelClient *rcc,
> > +                                              void *data, int num)
> >  {
> > -    PipeItem *item = spice_malloc(sizeof(*item));
> > +    RefsPipeItem *item = spice_malloc(sizeof(*item));
> >
> > -    red_channel_pipe_item_init(&main_chan->base, item,
> > +    item->refs = data;
> > +    red_channel_pipe_item_init(rcc->channel, &item->base,
> >                                SPICE_MSG_MAIN_MIGRATE_SWITCH_HOST);
> > -    return item;
> > +    return &item->base;
> >  }
> 
> What is num for?
> 
> >  void main_channel_push_migrate_switch(MainChannel *main_chan)
> >  {
> > -    RedChannelClient *rcc = main_chan->base.rcc;
> > +    int *refs = spice_malloc0(sizeof(int));
> >
> > -    red_channel_client_pipe_add_push(rcc,
> > -        main_migrate_switch_item_new(main_chan));
> > +    *refs = main_chan->base.clients_num;
> > +    red_channel_pipes_new_add_push(&main_chan->base,
> > +        main_migrate_switch_item_new, (void*)refs);
> >  }
> >
> >  static void main_channel_marshall_migrate_switch(SpiceMarshaller *m)
> > @@ -637,8 +658,6 @@ static void main_channel_marshall_migrate_switch(SpiceMarshaller *m)
> >
> >     reds_fill_mig_switch(&migrate);
> >     spice_marshall_msg_main_migrate_switch_host(m, &migrate);
> > -
> > -    reds_mig_release();
> >  }
> >
> >  static void main_channel_marshall_multi_media_time(SpiceMarshaller *m,
> > @@ -717,6 +736,27 @@ static void main_channel_send_item(RedChannelClient *rcc, PipeItem *base)
> >  static void main_channel_release_pipe_item(RedChannelClient *rcc,
> >     PipeItem *base, int item_pushed)
> >  {
> > +    switch (base->type) {
> > +        case SPICE_MSG_MAIN_AGENT_DATA: {
> > +            AgentDataPipeItem *data = (AgentDataPipeItem*)base;
> 
> s/data/item
> 
> > +            if (!--data->refs->refs) {
> > +                red_printf("SPICE_MSG_MAIN_AGENT_DATA %p %p, %d", data, data->refs, data->refs->refs);
> > +                free(data->refs);
> > +                data->data, data->opaque);
> > +            }
> > +            break;
> > +        }
> > +        case SPICE_MSG_MAIN_MIGRATE_SWITCH_HOST: {
> > +            RefsPipeItem *data = (RefsPipeItem*)base;
> > +            if (!--*(data->refs)) {
> > +                free(data->refs);
> 
> s/data/item
> 
> but we don't call data->free_data() here?
> 
> > +                reds_mig_release();
> > +            }
> > +            break;
> > +        }
> > +        default:
> > +            break;
> > +    }
> >     free(base);
> >  }
> >
> > @@ -731,16 +771,16 @@ static int main_channel_handle_parsed(RedChannelClient *rcc, uint32_t size, uint
> >         if (!main_chan) {
> >             return FALSE;
> >         }
> > -        reds_on_main_agent_start(main_chan);
> > +        reds_on_main_agent_start(mcc, main_chan);
> >         break;
> >     case SPICE_MSGC_MAIN_AGENT_DATA: {
> > -        reds_on_main_agent_data(message, size);
> > +        reds_on_main_agent_data(mcc, message, size);
> >         break;
> >     }
> >     case SPICE_MSGC_MAIN_AGENT_TOKEN:
> >         break;
> >     case SPICE_MSGC_MAIN_ATTACH_CHANNELS:
> > -        main_channel_push_channels(rcc);
> > +        main_channel_push_channels(mcc);
> >         break;
> >     case SPICE_MSGC_MAIN_MIGRATE_CONNECTED:
> >         red_printf("connected");
> > @@ -781,7 +821,8 @@ static int main_channel_handle_parsed(RedChannelClient *rcc, uint32_t size, uint
> >                                "bandwidth", mcc->latency, roundtrip);
> >                     break;
> >                 }
> > -                mcc->bitrate_per_sec = (uint64_t)(NET_TEST_BYTES * 8) * 1000000 / (roundtrip - mcc->latency);
> > +                mcc->bitrate_per_sec = (uint64_t)(NET_TEST_BYTES * 8) * 1000000
> > +                                        / (roundtrip - mcc->latency);
> >                 red_printf("net test: latency %f ms, bitrate %lu bps (%f Mbps)%s",
> >                            (double)mcc->latency / 1000,
> >                            mcc->bitrate_per_sec,
> > @@ -816,6 +857,7 @@ static int main_channel_handle_parsed(RedChannelClient *rcc, uint32_t size, uint
> >
> >  static void main_channel_on_error(RedChannelClient *rcc)
> >  {
> > +    red_printf("");
> >     reds_client_disconnect(rcc->client);
> >  }
> >
> > @@ -840,7 +882,7 @@ static void main_channel_hold_pipe_item(RedChannelClient *rcc, PipeItem *item)
> >  {
> >  }
> >
> > -static int main_channel_handle_migrate_flush_mark_proc(RedChannelClient *rcc)
> > +static int main_channel_handle_migrate_flush_mark(RedChannelClient *rcc)
> >  {
> >     main_channel_push_migrate_data_item(SPICE_CONTAINEROF(rcc->channel,
> >                                         MainChannel, base));
> > @@ -880,6 +922,11 @@ static void ping_timer_cb(void *opaque)
> >  }
> >  #endif /* RED_STATISTICS */
> >
> > +uint32_t main_channel_client_get_link_id(MainChannelClient *mcc)
> > +{
> > +    return mcc->link_id;
> > +}
> > +
> >  MainChannelClient *main_channel_client_create(MainChannel *main_chan,
> >     RedClient *client, RedsStream *stream, uint32_t link_id)
> >  {
> > @@ -919,11 +966,14 @@ MainChannelClient *main_channel_link(Channel *channel, RedClient *client,
> >             ,main_channel_release_pipe_item
> >             ,main_channel_on_error
> >             ,main_channel_on_error
> > -            ,main_channel_handle_migrate_flush_mark_proc
> > +            ,main_channel_handle_migrate_flush_mark
> >             ,main_channel_handle_migrate_data
> > -            ,main_channel_handle_migrate_data_get_serial_proc);
> > +            ,main_channel_handle_migrate_data_get_serial);
> >         ASSERT(channel->data);
> >     }
> > +    // TODO - migration - I removed it from channel creation, now put it
> > +    // into usage somewhere (not an issue until we return migration to it's
> > +    // former glory)
> >     red_printf("add main channel client");
> >     mcc = main_channel_client_create(channel->data, client, stream, link_id);
> >     return mcc;
> > diff --git a/server/main_channel.h b/server/main_channel.h
> > index eb1ee04..a294a2f 100644
> > --- a/server/main_channel.h
> > +++ b/server/main_channel.h
> > @@ -21,6 +21,7 @@
> >  #include <stdint.h>
> >  #include <spice/vd_agent.h>
> >  #include "common/marshaller.h"
> > +#include "reds.h"
> >  #include "red_channel.h"
> >
> >  /* This is a temporary measure for reds/main split - should not be in a header,
> > @@ -45,23 +46,21 @@ struct MainMigrateData {
> >  };
> >
> >  typedef struct MainChannel MainChannel;
> > -typedef struct MainChannelClient MainChannelClient;
> >
> >  Channel *main_channel_init();
> >  RedClient *main_channel_get_client_by_link_id(MainChannel *main_chan, uint32_t link_id);
> > -/* This is a 'clone' from the reds.h Channel.link callback */
> > +/* This is a 'clone' from the reds.h Channel.link callback to allow passing link_id */
> >  MainChannelClient *main_channel_link(struct Channel *, RedClient *client,
> >      RedsStream *stream, uint32_t link_id, int migration, int num_common_caps,
> >      uint32_t *common_caps, int num_caps, uint32_t *caps);
> >  void main_channel_close(MainChannel *main_chan); // not destroy, just socket close
> > -int main_channel_push_ping(MainChannel *main_chan, int size);
> >  void main_channel_push_mouse_mode(MainChannel *main_chan, int current_mode, int is_client_mouse_allowed);
> >  void main_channel_push_agent_connected(MainChannel *main_chan);
> >  void main_channel_push_agent_disconnected(MainChannel *main_chan);
> >  void main_channel_push_tokens(MainChannel *main_chan, uint32_t num_tokens);
> >  void main_channel_push_agent_data(MainChannel *main_chan, uint8_t* data, size_t len,
> >            spice_marshaller_item_free_func free_data, void *opaque);
> > -void main_channel_start_net_test(MainChannelClient *mcc);
> > +void main_channel_client_start_net_test(MainChannelClient *mcc);
> >  // TODO: huge. Consider making a reds_* interface for these functions
> >  // and calling from main.
> >  void main_channel_push_init(MainChannelClient *mcc, int connection_id, int display_channels_hint,
> > @@ -77,6 +76,7 @@ void main_channel_push_migrate_cancel(MainChannel *main_chan);
> >  void main_channel_push_multi_media_time(MainChannel *main_chan, int time);
> >  int main_channel_getsockname(MainChannel *main_chan, struct sockaddr *sa, socklen_t *salen);
> >  int main_channel_getpeername(MainChannel *main_chan, struct sockaddr *sa, socklen_t *salen);
> > +uint32_t main_channel_client_get_link_id(MainChannelClient *mcc);
> >
> >  int main_channel_client_is_low_bandwidth(MainChannelClient *mcc);
> >  uint64_t main_channel_client_get_bitrate_per_sec(MainChannelClient *mcc);
> > diff --git a/server/reds.c b/server/reds.c
> > index 413b348..0ce6f1c 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -982,8 +982,10 @@ void reds_on_main_agent_start()
> >     reds->agent_state.write_filter.discard_all = FALSE;
> >  }
> >
> > -void reds_on_main_agent_data(void *message, size_t size)
> > +void reds_on_main_agent_data(MainChannelClient *mcc, void *message, size_t size)
> >  {
> > +    // TODO - use mcc (and start tracking agent data per channel. probably just move the whole
> > +    // tokens accounting to mainchannel.
> >     RingItem *ring_item;
> >     VDAgentExtBuf *buf;
> >     int res;
> > @@ -1560,7 +1562,7 @@ static void reds_handle_main_link(RedLinkInfo *link)
> >             reds_get_mm_time() - MM_TIME_DELTA,
> >             red_dispatcher_qxl_ram_size());
> >
> > -        main_channel_start_net_test(mcc);
> > +        main_channel_client_start_net_test(mcc);
> >         /* Now that we have a client, forward any pending agent data */
> >         while (read_from_vdi_port());
> >     }
> > @@ -3099,6 +3101,7 @@ void reds_mig_switch(void)
> >  void reds_fill_mig_switch(SpiceMsgMainMigrationSwitchHost *migrate)
> >  {
> >     RedsMigSpice *s = reds->mig_spice;
> > +
> >     migrate->port = s->port;
> >     migrate->sport = s->sport;
> >     migrate->host_size = strlen(s->host) + 1;
> > diff --git a/server/reds.h b/server/reds.h
> > index 8eea164..5698c22 100644
> > --- a/server/reds.h
> > +++ b/server/reds.h
> > @@ -152,7 +152,7 @@ void reds_update_stat_value(uint32_t value);
> >
> >  // callbacks from main channel messages
> >  void reds_on_main_agent_start();
> > -void reds_on_main_agent_data(void *message, size_t size);
> > +void reds_on_main_agent_data(MainChannelClient *mcc, void *message, size_t size);
> >  void reds_on_main_migrate_connected();
> >  void reds_on_main_migrate_connect_error();
> >  void reds_on_main_receive_migrate_data(MainMigrateData *data, uint8_t *end);
> > --
> > 1.7.4.4
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> 
> 
> 
> -- 
> Marc-André Lureau


More information about the Spice-devel mailing list