[Spice-devel] [PATCH spice-streaming-agent v3 1/4] Use exception handling data from streaming device

[Christophe de Dinechin]

Lukáš Hrázký writes:

> On Tue, 2018-02-20 at 20:48 +0000, Frediano Ziglio wrote:
>> In all paths errors from this function are treated like fatal
>> error, there's no need to handle all manually potentially
>> forgetting to handle errors.
>> Also avoid to deal directly with logging moving the responsibility
>> to other layers.
>>
>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>> ---
>>  src/spice-streaming-agent.cpp | 35 ++++++++++++++---------------------
>>  1 file changed, 14 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 1f41a6f..5613934 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -77,7 +77,7 @@ static int have_something_to_read(int timeout)
>>      return 0;
>>  }
>>
>> -static int read_command_from_device(void)
>> +static void read_command_from_device(void)
>>  {
>>      StreamDevHeader hdr;
>>      uint8_t msg[64];
>> @@ -86,40 +86,32 @@ static int read_command_from_device(void)
>>      std::lock_guard<std::mutex> stream_guard(stream_mtx);
>>      n = read(streamfd, &hdr, sizeof(hdr));
>>      if (n != sizeof(hdr)) {
>> -        syslog(LOG_WARNING,
>> -               "read command from device FAILED -- read %d expected %lu\n",
>> -               n, sizeof(hdr));
>> -        return -1;
>> +        throw std::runtime_error("read command from device FAILED -- read " + std::to_string(n) +
>> +                                 " expected " + std::to_string(sizeof(hdr)));
>>      }
>>      if (hdr.protocol_version != STREAM_DEVICE_PROTOCOL) {
>> -        syslog(LOG_WARNING, "BAD VERSION %d (expected is %d)\n", hdr.protocol_version,
>> -               STREAM_DEVICE_PROTOCOL);
>> -        return -1;
>> +        throw std::runtime_error("BAD VERSION " + std::to_string(hdr.protocol_version) +
>> +                                 " (expected is " + std::to_string(STREAM_DEVICE_PROTOCOL) + ")");
>>      }
>>      if (hdr.type != STREAM_TYPE_START_STOP) {
>> -        syslog(LOG_WARNING, "UNKNOWN msg of type %d\n", hdr.type);
>> -        return -1;
>> +        throw std::runtime_error("UNKNOWN msg of type " + std::to_string(hdr.type));
>>      }
>>      if (hdr.size >= sizeof(msg)) {
>> -        syslog(LOG_WARNING,
>> -               "msg size (%d) is too long (longer than %lu)\n",
>> -               hdr.size, sizeof(msg));
>> -        return -1;
>> +        throw std::runtime_error("msg size (" + std::to_string(hdr.size) + ") is too long "
>> +                                 "(longer than " + std::to_string(sizeof(msg)) + ")");
>>      }
>>      n = read(streamfd, &msg, hdr.size);
>>      if (n != hdr.size) {
>> -        syslog(LOG_WARNING,
>> -               "read command from device FAILED -- read %d expected %d\n",
>> -               n, hdr.size);
>> -        return -1;
>> +        throw std::runtime_error("read command from device FAILED -- read " + std::to_string(n) +
>> +                                 " expected " + std::to_string(hdr.size));
>>      }
>>      streaming_requested = (msg[0] != 0); /* num_codecs */
>>      syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
>>             streaming_requested ? "START" : "STOP");
>>      client_codecs.clear();
>> -    for (int i = 1; i <= msg[0]; ++i)
>> +    for (int i = 1; i <= msg[0]; ++i) {
>>          client_codecs.insert((SpiceVideoCodecType) msg[i]);
>> -    return 1;
>> +    }
>
> Adding brackets, unrelated :) Just mentioning, I don't mind much.
>
>>  }
>>
>>  static int read_command(bool blocking)
>> @@ -133,7 +125,8 @@ static int read_command(bool blocking)
>>              sleep(1);
>>              continue;
>>          }
>> -        return read_command_from_device();
>> +        read_command_from_device();
>> +        break;
>>      }
>>
>>      return 1;
>
> I'm not entirely sure, if Christophe D still has anything to add to
> this style of constructing exceptions. I'll give him the chance to say
> it, for me:

Well, it's not different that before. But at the moment, I don't mind.
In order to show what I had in mind, I added it to the next iteration of
the series to deal with write errors. You'll tell me if you think it
makes the code shorter and cleaner. And if that's the case, then we
can generalize it.

In the present state of things

Acked-by: Christophe de Dinechin <dinechin at redhat.com>


>
> Reviewed-by: Lukáš Hrázký <lhrazky at redhat.com>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


--
Cheers,
Christophe de Dinechin (IRC c3d)