[Spice-devel] [RFC/POC PATCH vd_agent 13/16] vdagent: Log the diddly doo X11 error

[Christophe de Dinechin]

> On 7 Jun 2018, at 10:55, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> 
> On Wed, 2018-06-06 at 12:56 -0400, Frediano Ziglio wrote:
>>> 
>>> Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
>>> ---
>>> src/vdagent/x11-randr.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>> 
>>> diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
>>> index 803cf73..84c75f2 100644
>>> --- a/src/vdagent/x11-randr.c
>>> +++ b/src/vdagent/x11-randr.c
>>> @@ -38,6 +38,10 @@
>>> 
>>> static int error_handler(Display *display, XErrorEvent *error)
>>> {
>>> +    char buf[1024];
>>> +    XGetErrorText(display, error->error_code, buf, 1024);
>>> +    syslog(LOG_ERR, "X11 Error: %s", buf);
>>> +
>>>     vdagent_x11_caught_error = 1;
>>>     return 0;
>>> }
>> 
>> Beside the title too slang patch looks good...
> 
> Yeah there was a different word before this :) I can remove it, but...
> 
>> ... but I suppose that this
>> function is here to catch error and ignore them so why logging as
>> error something already expected?
> 
> Is that the purpose? I haven't realized that and it's not documented in
> any way. Now that you tell me, I see it in the code, but it's
> confusing.
> 
> The concrete case I had was at x11-randr.c:840 (on current master),
> calling XRRSetScreenSize (can't seem to find any documentation of this
> one). So I was getting a rather unhelpful "XRRSetScreenSize failed, not
> enough mem?" from our log, figured I'm getting some X error and got
> even slighly more unhlepful "BadMatch" from that. Luckily I noticed the
> comment about that a bit above the call.
> 
> So, if the error from the XRRSetScreenSize is always "BadMatch", I it
> can be ignored but would be more clear about it in the comments.
> 
> Shall I add some comments instead then?

Yes. Given how long your description is, they might be at least as funn as “diddly doo”.

But I think that this also means we expect to ignore a specific error in a specific context, maybe be a bit more restrictive instead of hiding everything?

Christophe
> 
> Cheers,
> Lukas
> 
>> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel