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

[Lukáš Hrázký]

On Tue, 2018-06-19 at 17:29 +0200, Christophe de Dinechin wrote:
> > 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?

You mean about the specific error? As I said, I haven't found any
documentation and I faitly recall I tried quite hard. Next up dig in
the source what errors it can actually raise, but I suspect it's dumb
and just gives the BadMatch on everything.

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