[PATCH xf86-video-nested 09/10] Introduce a new XCB client backend, and make it the default one.

[Uli Schlachter]

Hi,

Am 06.11.2015 um 14:10 schrieb Laércio de Sousa:
[...]
>>> +static void
>>> +_NestedClientSetWMClass(NestedClientPrivatePtr pPriv,
>>> +                        const char* wm_class)
>>> +{
>>> +    size_t class_len = strlen(wm_class) + 1;
>>> +    char *class_hint = malloc(class_len);
>>> +
>>> +    if (class_hint)
>>> +    {
>>> +        strcpy(class_hint, wm_class);
>>> +        xcb_change_property(pPriv->conn,
>>> +                            XCB_PROP_MODE_REPLACE,
>>> +                            pPriv->window,
>>> +                            XCB_ATOM_WM_CLASS,
>>> +                            XCB_ATOM_STRING,
>>> +                            8,
>>> +                            class_len,
>>> +                            class_hint);
>>> +        free(class_hint);
>>> +    }
>>
>> Why is this strcpy needed?
>>
> I've copied over this piece of code from Xephyr. In that context,
> class_hint stores a concatenation bewteen two strings, so that strcpy is
> needed, but here the WM_CLASS string is much more simple, so that strcpy is
> not needed. I'll remove it. Thanks!

Uhm, right. This wants to set WM_CLASS and WM_CLASS should contain "two" strings
(the class and the instance), separated by a null byte.

Sorry for not noticing this earlier, but isn't this code wrong then?

https://tronche.com/gui/x/icccm/sec-4.html#s-4.1.2.5

[...]
>>> +        if (!ev)
>>> +        {
>>> +            if (_NestedClientConnectionHasError(pPriv->scrnIndex,
>>> +                                                pPriv->conn))
>>> +            {
>>> +                /* XXX: Is there a better way to do this? */
>>> +                xf86DrvMsg(pPriv->scrnIndex,
>>> +                           X_ERROR,
>>> +                           "Connection with host X server lost.\n");
>>> +                free(ev);
>>> +                NestedClientCloseScreen(pPriv);
>>> +                exit(1);
>>> +            }
>>> +
>>> +            break;
>>> +        }
>>> +
>>> +        switch (ev->response_type & ~0x80)
>>> +        {
>>> +        case XCB_EXPOSE:
>>> +            _NestedClientProcessExpose(pPriv, ev);
>>> +            break;
>>> +        case XCB_CLIENT_MESSAGE:
>>> +            _NestedClientProcessClientMessage(pPriv, ev);
>>> +            break;
>>> +        case XCB_MOTION_NOTIFY:
>>> +            _NestedClientProcessMotionNotify(pPriv, ev);
>>> +            break;
>>> +        case XCB_KEY_PRESS:
>>> +            _NestedClientProcessKeyPress(pPriv, ev);
>>> +            break;
>>> +        case XCB_KEY_RELEASE:
>>> +            _NestedClientProcessKeyRelease(pPriv, ev);
>>> +            break;
>>> +        case XCB_BUTTON_PRESS:
>>> +            _NestedClientProcessButtonPress(pPriv, ev);
>>> +            break;
>>> +        case XCB_BUTTON_RELEASE:
>>> +            _NestedClientProcessButtonRelease(pPriv, ev);
>>> +            break;
>>> +        }
>>> +
>>> +        free(ev);
>>> +        xcb_flush(pPriv->conn);
>>
>> Why is this flushing inside of the event loop? Wouldn't a flush after the
>> loop
>> be enough?
>>
> Well... Comparing it with other XCB snippets I've found in the web, it
> seems this xcb_flush() call is not needed at all. Calling it right atfer
> window creation or redrawing (XCB_EXPOSE event only) should be enough,
> right?
[...]

Well, it depends. Something should flush in the end in case there are any
requests still in XCB's output buffer (and nothing implicitly flushes them by
waiting for a reply). So I think that having a call to xcb_flush() before
returning to this function should be added. If it does something, it just
prevented a bug and if it doesn't do anything, it's not expensive. ;-)

Everything else seems fine, thanks.

Uli
-- 
"For saving the Earth.. and eating cheesecake!"