Bug 70956

Summary: passing lists with variadic sized types to requests looks complicated and error prone
Product: XCB Reporter: Daniel Martin <consume.noise>
Component: LibraryAssignee: Daniel Martin <consume.noise>
Status: RESOLVED MOVED QA Contact: xcb mailing list dummy <xcb>
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description Daniel Martin 2013-10-28 12:21:50 UTC
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.
Comment 1 Ran Benita 2013-10-30 22:03:15 UTC
Hi Daniel,
First thanks for making the bug easy to follow :)

I can see the problem, the current interface is clearly not usable, and certainly unexpected. As for the solution, it's the only reasonable approach I can come up with as well.

Instead of changing the function signature/abi, you might consider generating a '_serialize' function for this argument, which takes the array-of-structs as you proposed and puts out the serialized buffer, which you can then pass to e.g. xcb_set_font_path(). There's some precedent to these '_serialize' functions as you know (though only for entire requests AFAIK). A shortcoming of this is that you have to malloc() the returned buffer inside the serialize function, but xcb isn't repelled by malloc() anyway. However since no one's using these functions, I would personally prefer the direct-function-argument approach. But just something to consider.
Comment 2 GitLab Migration User 2019-02-16 19:40:37 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/lib/libxcb/issues/12.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.