[Spice-devel] [PATCH spice-streaming-agent 4/6] Separate handling start/stop message from server

[Uri Lublin]

On 02/19/2018 06:44 PM, Frediano Ziglio wrote:
>>
>> On Mon, 2018-02-19 at 15:52 +0000, Frediano Ziglio wrote:
>>> Prepare to add support for other messages.
>>>
>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>>> ---
>>>   src/spice-streaming-agent.cpp | 26 ++++++++++++++++++--------
>>>   1 file changed, 18 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>> index 08f4b48..41b4d3d 100644
>>> --- a/src/spice-streaming-agent.cpp
>>> +++ b/src/spice-streaming-agent.cpp
>>> @@ -78,10 +78,11 @@ static int have_something_to_read(int timeout)
>>>       return 0;
>>>   }
>>>   
>>> +static int read_stream_start_stop(uint32_t len);
>>> +
>>>   static int read_command_from_device(void)
>>>   {
>>>       StreamDevHeader hdr;
>>> -    uint8_t msg[64];
>>>       int n;
>>>   
>>>       std::lock_guard<std::mutex> stream_guard(stream_mtx);
>>> @@ -93,15 +94,24 @@ static int read_command_from_device(void)
>>>           ERROR("BAD VERSION " << hdr.protocol_version <<
>>>                 " (expected is " << STREAM_DEVICE_PROTOCOL << ")");
>>>       }
>>> -    if (hdr.type != STREAM_TYPE_START_STOP) {
>>> -        ERROR("UNKNOWN msg of type " << hdr.type);
>>> +
>>> +    switch (hdr.type) {
>>> +    case STREAM_TYPE_START_STOP:
>>> +        return read_stream_start_stop(hdr.size);
>>>       }
>>> -    if (hdr.size >= sizeof(msg)) {
>>> -        ERROR("msg size (" << hdr.size << ") is too long (longer than " <<
>>> sizeof(msg));
>>> +    ERROR("UNKNOWN msg of type " << hdr.type);
>>> +}
>>> +
>>> +static int read_stream_start_stop(uint32_t len)
>>> +{
>>> +    uint8_t msg[256];
>>> +
>>> +    if (len >= sizeof(msg)) {
>>> +        ERROR("msg size (" << len << ") is too long (longer than " <<
>>> sizeof(msg));
>>>       }
>>> -    n = read(streamfd, &msg, hdr.size);
>>> -    if (n != hdr.size) {
>>> -        ERROR("read command from device FAILED -- read " << n << "
>>> expected " << hdr.size);
>>> +    int n = read(streamfd, &msg, len);
>>> +    if (n != len) {
>>> +        ERROR("read command from device FAILED -- read " << n << "
>>> expected " << len);
>>>       }
>>>       streaming_requested = (msg[0] != 0); /* num_codecs */
>>>       syslog(LOG_INFO, "GOT START_STOP message -- request to %s
>>>       streaming\n",
>>
>> So, you started working on the same code as well :P I actually have
>> this part rewritten in a C++ way. I was surprised to not find it in
>> Christophe's patches. My head starts going crazy from all the different
>> patches to the same code...
>>
>> I would suggest leaving this off until we switch to an encapsulated C++
>> implementation?
>>
>> Lukas
>>
> 
> Sorry, I thought you were working on the notify error message only,
> I'm trying to add mainly capability support for start fixing the
> ID mismatch issue, I left the error handling (notify error) as stub.
> 
> Yes, unfortunately the code is small and there's quite a lot of clash.

I too wrote some very similar patches :)
(but did not yet send them and now I will not send them)

That's what happens when there are problems in the code and people
are trying to fix them.

Regards,
     Uri.