[Spice-devel] [PATCH spice-server] reds: Check we don't register a channel twice in reds_register_channel

[Snir Sheriber]

Hi,

On 3/18/19 12:54 PM, Frediano Ziglio wrote:
>> On 3/18/19 12:12 PM, Frediano Ziglio wrote:
>>>> Hi,
>>>>
>>>> On 3/15/19 11:27 AM, Frediano Ziglio wrote:
>>>>> To avoid possibly regression check it only if extra checks are
>>>>> enabled.
>>>> Currently there's already a "channel duplication" warning upon connection
>>>> but won't hurt to have this extra check and emit an error.
>>>>
>>> I cannot find it. This patch is for spice-server.
>>
>> Oh, sorry, "duplicate channel" on red_client_add_channel
>>
> These functions are checking RedChannelClient, not RedChannel.
> Basically they check if client is connecting twice to the same
> channel.


ok

>
>>>>> This allowed to check previous "Move channel registration to constructed
>>>> This allow/s? i don't get this sentence, it just avoids duplicate call.
>>>> I think
>>>> i'd not even mention the previous commit
>>>>
>>> No, it will also exit the program if this condition is met.
>>
>> Yes, i got it, i meant that such patch would have make sense also before
>> the "Move channel reg.." commit,
>> so not necessarily need to mention it.
>>
> Yes, I suppose would have made sense even before.
> But that's the reason why I added it and how I used it.


Just the sentence not clear to me, if I'm not mistaken "allows" should 
be used

Fine with me other than that


>
>>
>>>> Snir.
>>>>
>>>>> vfunc" commit.
>>>>>
>>>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>>>>> ---
>>>>>     server/reds.c | 5 +++++
>>>>>     1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/server/reds.c b/server/reds.c
>>>>> index 429f8142..e182eba7 100644
>>>>> --- a/server/reds.c
>>>>> +++ b/server/reds.c
>>>>> @@ -380,6 +380,11 @@ void stat_remove_counter(SpiceServer *reds,
>>>>> RedStatCounter *counter)
>>>>>     void reds_register_channel(RedsState *reds, RedChannel *channel)
>>>>>     {
>>>>>         spice_assert(reds);
>>>>> +    if (spice_extra_checks) {
>>>>> +        uint32_t this_type, this_id;
>>>>> +        g_object_get(channel, "channel-type", &this_type, "id",
>>>>> &this_id,
>>>>> NULL);
>>>>> +        spice_assert(reds_find_channel(reds, this_type, this_id) ==
>>>>> NULL);
>>>>> +    }
>>>>>         reds->channels = g_list_prepend(reds->channels, channel);
>>>>>         // create new channel in the client if possible
>>>>>         main_channel_registered_new_channel(reds->main_channel, channel);
>>> Frediano