<html>
<head>
<base href="https://bugs.freedesktop.org/" />
</head>
<body><table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>Priority</th>
<td>medium
</td>
</tr>
<tr>
<th>Bug ID</th>
<td><a class="bz_bug_link
bz_status_NEW "
title="NEW --- - passing lists with variadic sized types to requests looks complicated and error prone"
href="https://bugs.freedesktop.org/show_bug.cgi?id=70956">70956</a>
</td>
</tr>
<tr>
<th>Assignee</th>
<td>xcb@lists.freedesktop.org
</td>
</tr>
<tr>
<th>Summary</th>
<td>passing lists with variadic sized types to requests looks complicated and error prone
</td>
</tr>
<tr>
<th>QA Contact</th>
<td>xcb@lists.freedesktop.org
</td>
</tr>
<tr>
<th>Severity</th>
<td>normal
</td>
</tr>
<tr>
<th>Classification</th>
<td>Unclassified
</td>
</tr>
<tr>
<th>OS</th>
<td>All
</td>
</tr>
<tr>
<th>Reporter</th>
<td>consume.noise@gmail.com
</td>
</tr>
<tr>
<th>Hardware</th>
<td>Other
</td>
</tr>
<tr>
<th>Status</th>
<td>NEW
</td>
</tr>
<tr>
<th>Version</th>
<td>unspecified
</td>
</tr>
<tr>
<th>Component</th>
<td>Library
</td>
</tr>
<tr>
<th>Product</th>
<td>XCB
</td>
</tr></table>
<p>
<div>
<pre>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 : <a href="http://cgit.freedesktop.org/xcb/proto/tree/src/xproto.xml#n3556">http://cgit.freedesktop.org/xcb/proto/tree/src/xproto.xml#n3556</a>
var.type: <a href="http://cgit.freedesktop.org/xcb/proto/tree/src/xproto.xml#n3416">http://cgit.freedesktop.org/xcb/proto/tree/src/xproto.xml#n3416</a>
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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the QA Contact for the bug.</li>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>