Skip to content

passing lists with variadic sized types to requests looks complicated and error prone

Submitted by Daniel Martin

Assigned to Daniel Martin

Link to original bug (#70956)

Description

While working on the serialization of requests in my rewrite - yeah,
finally - I've found a problem in the currently generated code. I
haven't tested it, but the code doesn't look right. It should be possible to workaround it, but imho this shouldn't be the way those lists are handled.

Requests (with the affected lists in brackets) are:
xinput:XIChangeHierarchy - ['changes']
xinput:XISelectEvents - ['masks']
xkb:SetDeviceInfo - ['leds']
xkb:SetGeometry - ['properties', 'colors', 'shapes', 'outlines']
xkb:SetMap - ['types', 'syms']
xproto:SetFontPath - ['font']

Let's pick xproto:SetFontPath. One shouldn't need to use a font
server, but it's an easy example:
request : http://cgit.freedesktop.org/xcb/proto/tree/src/xproto.xml#n3556
var.type: http://cgit.freedesktop.org/xcb/proto/tree/src/xproto.xml#n3416

The variadic type becomes the structure:

    typedef struct xcb_str_t {
        uint8_t name_len;
    }

There's no note on the list of characters 'name', which would need to
be used as font path in this case and there's no way to attach such a
string to the structure.

The request takes the font paths as: const xcb_str_t *font then within the request 'xcb_set_font_path()' the list of font paths becomes part of the 'struct iovec xcb_parts[]' for serialization: xcb_parts[4].iov_base = (char *) font; and the length gets calculated:

    for(i=0; i<font_qty; i++) {
        xcb_tmp_len = xcb_str_sizeof(xcb_tmp);
        xcb_parts[4].iov_len += xcb_tmp_len;
        xcb_tmp += xcb_tmp_len;
    }

The length calculation (including xcb_str_sizeof()) looks good. But, it's just one xcb_parts[] to serialize the font paths, so the xcb_str_t list (the font parameter) already has to be seriallized as there's only one iov_base pointing to it. That means one has to setup and pass something like this as *font: (Note: Those are just examples, not tested, just to make the problem obvious.)

    char path0[] = "/foo/bar0";
    char path1[] = "/foo/bar1";
    char *font = malloc(2*sizeof(xcb_str_t) + strlen(path0) + strlen(path1));
    font[0] = strlen(path0);
    memcpy(font[1], path0, strlen(path0));
    font[strlen(path0)] = strlen(path1);
    memcpy(font[strlen(path0)+1], path1, strlen(path1));
    
    xcb_set_font_path (conn, 2, (xcb_str_t*)font);

or like this (possible, because 'name_len' is an uint8_t):

    char font[] =
        {/* name_len */ 9, /* name */ '/', 'f', 'o', 'o', '/', 'b', 'a', 'r', '0',
         /* name_len */ 9, /* name */'/', 'f', 'o', 'o', '/', 'b', 'a', 'r', '1'};

    xcb_set_font_path (conn, 2, (xcb_str_t*)font[0]);

The last example may not look that bad. But, imagine a more complex structure having more fields, different types, even fields behind lists. They'd be a mess to setup.

My solution would be an additional type/structure for such variadic types if they show up in a request, which don't hide the variadic parts:

    typedef struct xcb_str_request_t {
        uint8_t name_len;
        char *name;
    }

With that one could setup the font paths much more easy:

    char path0[] = "/foo/bar0";
    char path1[] = "/foo/bar1";
    xcb_str_request_t font[2];
    font[0].name_len = strlen(path0);
    font[0].name = path0;
    font[1].name_len = strlen(path1);
    font[1].name = path1;

    xcb_set_font_path(conn, 2, &font);

To serialize this list, it would be necessary to iterate over the list (as it's done atm.) and to adjust the 'struct iovec xcb_parts[]' and the index within dynamically, so we end up having an xcb_parts[] for every xcb_str_request_t and the 'name' list within. But, from my pov that's easy to do when generating the code - much more easy and less error prone than setting up a memory block and filling it with data on the application side.

I hope I didn't missed something important and I hope that I could describe the problem properly.

Edited by Alan Coopersmith