[Xcb] [Bug 70956] New: passing lists with variadic sized types to requests looks complicated and error prone

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=70956

          Priority: medium
            Bug ID: 70956
          Assignee: xcb at lists.freedesktop.org
           Summary: passing lists with variadic sized types to requests
                    looks complicated and error prone
        QA Contact: xcb at lists.freedesktop.org
          Severity: normal
    Classification: Unclassified
                OS: All
          Reporter: consume.noise at gmail.com
          Hardware: Other
            Status: NEW
           Version: unspecified
         Component: Library
           Product: XCB

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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/xcb/attachments/20131028/e6142114/attachment.html>