<html> <head> <meta http-equiv="content-type" content="text/html; charset=windows-1252"> </head> <body bgcolor="#FFFFFF" text="#000000"> Good afternoon all, <div class="moz-forward-container"> Yesterday was the last official day of my internship and I am here to report about work I've done and also to respond about fixes according to previous letter from Peter Hutterer. I worked and tested mainly on xproto and shape extensions, but almost all the extensions are covered with existing code, excluding such complex ones, as xinput and xkb. The main goal of internship was to implement and size checking and byteswapping code to xserver. The generator of code is in proto/ directory and is called gen_swap_check.py. It takes as an arguments path to directory where the generated c-files will be stored and an xml-file to generate from. So far, the generator supports: 1. Requests and <blockquote> <ul> <li>lists (of both variable size and fixed size) </li> <li>simple fields (as CARD32)</li> <li>pads (not completely)</li> <li>switches</li> <li>structs</li> </ul> within requests </blockquote> 2. Enums 3. Structs valueparams and unions are deprecated and, thanks to Jaya Tiwari and Christian Linhart, are no more used in xproto. Errors and events handlers are not yet implemented. Integration into xserver consisted in refactoring the Xext/shape.c so it used the generated code. We tested the generated code with SPARC machine and checking works OK, but there is some bug with byte-swapping, I am working on. I attached the diff with the newest changes to the reply to this letter because these two take more than 100 KB together and sending such a big letter requires moderator approval. You can also check the <a moz-do-not-send="true" href="https://bitbucket.org/AsalleKim/gen_swap_check">bitbucket</a> repo on branch master. I inserted some comments below: <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">Here is the link tobitbucket <bitbucket.org/AsalleKim/gen_swap_check>. I didn't create patch, because in my opinion it's pretty inconvenient to send patches with code written from scratch, though it's a powerful tool for maintaining already existing code. The repo contains forked xserver from main xserver repo <a moz-do-not-send="true" class="moz-txt-link-rfc2396E" href="http://cgit.freedesktop.org/xorg/xserver/"><http://cgit.freedesktop.org/xorg/xserver/></a>. It uses some changes in xcb made by Christian Linhart and Jaya Tiwari, the changes are in pending patches and will be soon applied (if they are not already). This mainly touches the valueparam to switch replacement in xproto.xml. As valueparam is deprecated, the generator, I am working on, does not support it. </pre> </blockquote> <pre wrap="">I mentioned this in private email already, but for the archives: next time please submit a git-formatted patch. If you're not quite ready yet submit it as [RFC PATCH] ... so people know immediately that this is still work in progress.</pre> </blockquote> OK. Sorry for inconvenience. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">Generator's code can process almost all extensions, but I am now mainly working on Shape and Xproto. For now it creates source file and header file in the directory proto/gen and then they are built with automake (proto/Makefile.am). There are several things I was not sure about. 1. The working name of the swapping and size-checking functions generator is gen_swap_check, which is rather awkward. But I couldn't invent anything better. 2. To test the code I need to integrate it to the xserver. I already made some changes into Xext/shape.c so it uses the generated code now. The Xext/shape.h must include the swapcheck_shape.h (generated one). Would it be okay if I copy all generated headers to {installdir}/include/xcb, mixing the genswap-generated code and the regular code, generated by c_client.py? And copy the compiled swapcheck_* files to {installdir}/lib. </pre> </blockquote> <pre wrap="">I don't think you'd want to install it to $installdir, rather it should be written to $builddir (which you already do) and then used from there, i.e. add the directory to CORE_INCS in configure.ac </pre> <blockquote type="cite"> <pre wrap="">3. proto/Makefile.am is written in such way that no make targets are declared. How can I add cp commad to it? </pre> </blockquote> <pre wrap="">depends, it's not clear to me what you want to copy, maybe you can comment on this?</pre> </blockquote> To two previous comments: I understood that there should be no copying, I just added generated-headers-and-libs-path to Xext_INCS and Xext_LIBS respectively. I guess this is how it should look like. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">4. When building several warnings in included file os.h appeared. How can I get rid of them? Or should I ignore them? /home/asalle/xorg/Debug2/test-install/include/xorg/os.h:541:1: warning: redundant redeclaration of 'strndup' [-Wredundant-decls] 1055 strndup(const char *str, size_t n); </pre> </blockquote> <pre wrap="">don't worry about these. unless warnings are added by your code you can ignore them (well, or fix them if you have the time and motivation) always helps to switch back to the master branch to check if a warning is there. </pre> </blockquote> OK. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">This is the short overview of swapping and size-checking functions generator, I would be glad to hear your comments and messages about found bugs, Asalle </pre> </blockquote> <pre wrap="">ok, I ran a diff to make the review a bit easier. Most of the diff is in the parser code anyway so that won't change. see my comments in-line </pre> <blockquote type="cite"> <pre wrap="">diff --git a/.gitignore b/.gitignore index dc56b46..b45e8cb 100644 --- a/.gitignore +++ b/.gitignore @@ -80,3 +80,6 @@ core doltcompile doltlibtool xserver.ent + +# vim swap files +*.swp </pre> </blockquote> <pre wrap="">please submit this as separate commit.</pre> </blockquote> As far as I understand, it's already done (<a moz-do-not-send="true" href="https://bitbucket.org/AsalleKim/gen_swap_check/commits/77bb0d891e391d1af02837d88c030af097ed65e8">link</a>). <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">diff --git a/Makefile.am b/Makefile.am index f0fa2d8..1c214b4 100644 --- a/Makefile.am +++ b/Makefile.am @@ -42,6 +42,7 @@ SUBDIRS = \ dix \ fb \ mi \ + proto \ Xext \ miext \ os \ @@ -62,7 +63,7 @@ SUBDIRS = \ $(GLAMOR_DIR) \ config \ hw \ - test + test </pre> </blockquote> <pre wrap="">please drop this hunk. There's a couple of whitespace issues in the various hunks, I'll point some of them out but double check the whole diff please. trailing whitespaces can be removed with s/[ ]*$// in vim. you should also configure your vimrc to highlight such issues, it makes life a bit easier: let c_space_errors=1 </pre> </blockquote> Thanks for vim-hint. I fixed it on all hunks. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap=""> if XORG aclocaldir = $(datadir)/aclocal @@ -116,7 +117,8 @@ DIST_SUBDIRS = \ dri3 \ present \ hw \ - test + test \ + proto # gross hack relink: all diff --git a/Xext/Makefile.am b/Xext/Makefile.am index a9a4468..92a3b72 100644 --- a/Xext/Makefile.am +++ b/Xext/Makefile.am @@ -1,6 +1,7 @@ noinst_LTLIBRARIES = libXext.la libXextdpmsstubs.la -AM_CFLAGS = $(DIX_CFLAGS) + +AM_CFLAGS = $(DIX_CFLAGS) $(PROTO_CFLAGS) </pre> </blockquote> <pre wrap="">see my XCBPROTO_CFLAGS comment in the configure.ac hunk</pre> </blockquote> I guess, everything is OK here, because we cannot simply replace PROTO_CFLAGS by XCBPROTO_CFLAGS, because PROTO_CFLAGS contains also XSERVER_CFLAGS beside the XCBPROTO_CFLAGS. Or had I misunderstood anything? <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap=""> if XORG sdk_HEADERS = xvdix.h xvmcext.h geext.h geint.h shmint.h syncsdk.h diff --git a/Xext/shape.c b/Xext/shape.c index bb479b1..54e71d6 100644 --- a/Xext/shape.c +++ b/Xext/shape.c @@ -1025,9 +1025,25 @@ ProcShapeGetRectangles(ClientPtr client) return Success; } +#include "xcb/swapcheck_shape.h" + static int ProcShapeDispatch(ClientPtr client) { + REQUEST(xReq); + if(xcb_shape_Check_dispatch(client) == Success) + { + return ProcShapeDispatch_Unchecked(client); + } + else + { + return BadLength; + } +} </pre> </blockquote> <pre wrap="">couple of coding style issues: { on same line as the condition space after if, before the actual condition. { on same line as else please (but not }) xserver coding style is indent of 4 spaces, no tabs. Please fix this in all hunks.</pre> </blockquote> Fixed <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+ +static int +ProcShapeDispatch_Unchecked(ClientPtr client) +{ REQUEST(xReq); switch (stuff->data) { case X_ShapeQueryVersion: @@ -1073,6 +1089,22 @@ ProcShapeDispatch(ClientPtr client) } } +static int +SProcShapeDispatch(ClientPtr client) +{ + REQUEST(xReq); + + if(xcb_shape_SwapFromClient_dispatch(client)) + { + return ProcShapeDispatch_Unchecked(client); + } + else + { + return BadLength; + } +} + + static void SShapeNotifyEvent(xShapeNotifyEvent * from, xShapeNotifyEvent * to) { diff --git a/configure.ac b/configure.ac index 96524c5..ef9f0bc 100644 --- a/configure.ac +++ b/configure.ac @@ -33,6 +33,10 @@ AC_CONFIG_SRCDIR([Makefile.am]) AC_CONFIG_MACRO_DIR([m4]) AM_INIT_AUTOMAKE([foreign dist-bzip2]) AC_USE_SYSTEM_EXTENSIONS +AM_PATH_PYTHON([2.7]) + + + </pre> </blockquote> <pre wrap="">no spare empty lines please (in hunk above too) </pre> </blockquote> Fixed. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap=""> # Require xorg-macros minimum of 1.14 for XORG_COMPILER_BRAND in XORG_DEFAULT_OPTIONS m4_ifndef([XORG_MACROS_VERSION], @@ -1779,6 +1783,31 @@ AC_MSG_RESULT([yes])], AC_MSG_RESULT([no])) XSERVER_CFLAGS="$XSERVER_CFLAGS $CORE_INCS $XEXT_INC $COMPOSITE_INC $DAMAGE_INC $FIXES_INC $XI_INC $MI_INC $MIEXT_SYNC_INC $MIEXT_SHADOW_INC $MIEXT_LAYER_INC $MIEXT_DAMAGE_INC $RENDER_INC $RANDR_INC $FB_INC $DBE_INC $PRESENT_INC" dnl --------------------------------------------------------------------------- +dnl proto section. +dnl --------------------------------------------------------------------------- + </pre> </blockquote> <pre wrap="">run PKG_CHECK_MODULES for xcb-proto here so you can be sure it exists before you access the various variables.</pre> </blockquote> Fixed. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+# Find the xcb-proto protocol descriptions +AC_MSG_CHECKING(XCBPROTO_XCBINCLUDEDIR) +XCBPROTO_XCBINCLUDEDIR=`$PKG_CONFIG --variable=xcbincludedir xcb-proto` +AC_MSG_RESULT($XCBPROTO_XCBINCLUDEDIR) +AC_SUBST(XCBPROTO_XCBINCLUDEDIR) </pre> </blockquote> <pre wrap="">quick check shows that xcbincludedir was added in 2006, you can just grab it directly and skip the AC_MSG_CHECKING/AC_MSG_RESULT </pre> </blockquote> <meta http-equiv="content-type" content="text/html; charset=windows-1252"> <meta http-equiv="content-type" content="text/html; charset=windows-1252"> <big><big><big><big><big><big><big><big><big><big><font style="font-family: 'Times New Roman'; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">I haven't found any. There are some random code in configure.ac, line 2006. I also searched the whole dir with grep and found no other 'xcbincludedir' entries. Could you please point out what 2006 do you mean? Anyways, I removed the re-checking lines. </big><span style="color: rgb(0, 0, 0); font-family: 'Times New Roman'; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; display: inline !important; floa t: none; background-color: rgb(255 , 255, 255);"><big><big> </big></big></big></big></big></big></big></big></big></big></big> <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+ +# Find the xcb-proto version +XCBPROTO_VERSION=`$PKG_CONFIG --modversion xcb-proto` +AC_SUBST(XCBPROTO_VERSION) </pre> </blockquote> <pre wrap="">you don't seem to use this one anywhere, is this still needed?</pre> </blockquote> I guess, we don't. So I Removed them. <span style="color: rgb(0, 102, 0); font-family: 'Times New Roman'; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; display: inline !important; float: none; background-color: rgb(255, 255, 255);"> <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+ +# Find the xcbgen Python package +AC_MSG_CHECKING(XCBPROTO_XCBPYTHONDIR) +XCBPROTO_XCBPYTHONDIR=`$PKG_CONFIG --variable=pythondir xcb-proto` +AC_MSG_RESULT($XCBPROTO_XCBPYTHONDIR) +AC_SUBST(XCBPROTO_XCBPYTHONDIR) </pre> </blockquote> <pre wrap="">likewise, pythondir was added in 2008, just require a new-ish version and skip the CHECKING/RESULTS</pre> </blockquote> <meta http-equiv="content-type" content="text/html; charset=windows-1252"> <span style="font-family: 'Times New Roman'; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; display: inline ! important; float: none; background-color: rgb(255, 255, 255);">Couldn't find other mentions of variable 'pythondir' in line 2008 of configure.ac or anywhere else, as in previous hunk. <meta http-equiv="content-type" content="text/html; charset=windows-1252"> <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+ +# CFLAGS +libxcb_inc=`$PKG_CONFIG --cflags xcb` +PROTO_CFLAGS="$XSERVER_CFLAGS $libxcb_inc" +AC_SUBST([PROTO_CFLAGS]) </pre> </blockquote> <pre wrap="">if you use PKG_CHECK_MODULES([XCBPROTO], [xcb-proto]) it will automatically supply XCBPROTO_CFLAGS and XCBPROTO_LIBS. no extra work needed.</pre> </blockquote> Fixed. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+ +dnl --------------------------------------------------------------------------- dnl DDX section. dnl --------------------------------------------------------------------------- @@ -2546,6 +2575,7 @@ xfixes/Makefile exa/Makefile dri3/Makefile present/Makefile +proto/Makefile hw/Makefile hw/xfree86/Makefile hw/xfree86/Xorg.sh diff --git a/proto/.gitignore b/proto/.gitignore new file mode 100644 index 0000000..cc4611b --- /dev/null +++ b/proto/.gitignore @@ -0,0 +1,5 @@ +# Python "compiled" files +*.pyc + +# Autogenerated by gen_swap_check.py in proto files +gen/* diff --git a/proto/Makefile.am b/proto/Makefile.am new file mode 100644 index 0000000..b2d5768 --- /dev/null +++ b/proto/Makefile.am @@ -0,0 +1,19 @@ +noinst_LTLIBRARIES = libproto.la + +AM_CFLAGS = $(PROTO_CFLAGS) + +prefix='gen/swapcheck_' </pre> </blockquote> <pre wrap="">I think "generated" instead of "gen" is better here. It's not like we need to type this often so spelling it out makes it quicker to understand.</pre> </blockquote> Fixed. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ +EXTSOURCES = \ + gen/swapcheck_xproto.c \ + gen/swapcheck_shape.c </pre> </blockquote> <pre wrap="">huh, I had to google to check if EXTSOURCES was a special automake variable. I'd recommend writing it lowercase or even "extension_sources" to make this more obvious. </pre> </blockquote> Fixed. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+ +libproto_la_SOURCES = $(EXTSOURCES) + +ech = $(subst gen/swapcheck_, ,$(@)) </pre> </blockquote> <pre wrap="">"ech"? I wonder what that stand for... :)</pre> </blockquote> Silly mistake, I changed it to 'sources' <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+ +$(EXTSOURCES): $(XCBPROTO_XCBINCLUDEDIR)/$(ech:.c=.xml) $(srcdir)/gen_swap_check.py + + $(AM_V_GEN)$(PYTHON) $(srcdir)/gen_swap_check.py \ + -p $(XCBPROTO_XCBPYTHONDIR) \ + $(XCBPROTO_XCBINCLUDEDIR)/$(ech:.c=.xml) diff --git a/proto/gen_swap_check.py b/proto/gen_swap_check.py new file mode 100644 index 0000000..b1937c2 --- /dev/null +++ b/proto/gen_swap_check.py @@ -0,0 +1,753 @@ +#!/usr/bin/env python +import getopt +import os +import sys +import errno +import re + +def _i(): + ''' + Indent function + ''' + return ''.join(_indent) + +#Helper functions and definitions copy-pasted from c_client.py, to replace with xcbutils.py </pre> </blockquote> <pre wrap="">I'm struggling to find any reference to xcbutils.py in google. is this something proposed on the xcb list? or a TODO for this patch? </pre> </blockquote> It was an idea of file, that stores common for c_client.py and gen_swap_check.py proposed by me, as Christian Linhart has already clarified. We are still disscussing if it's really necessary to create new dependencies in order to enhance code utilization. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+ +def _h(fmt, *args): + ''' + Writes the given line to the header file. + ''' + _hlines[_hlevel].append(fmt % args) + +def _c(fmt, *args): + ''' + Writes the given line to the source file. + ''' + _clines[_clevel].append(fmt % args) + +def _hc(fmt, *args): + ''' + Writes the given line to both the header and source files. + ''' + _h(fmt, *args) + _c(fmt, *args) + +def _c_wr_stringlist(indent, strlist): + ''' + Writes the given list of strings to the source file. + Each line is prepended by the indent string + ''' + for str in strlist: + _c("%s%s", indent, str) </pre> </blockquote> <pre wrap="">can you pop a comment in where the copy/pasted helper functions stop? I suspect here, but a comment makes this more obvious</pre> </blockquote> There is a comment, I made it even more obvious, as it turned out to be misunderstood. <pre><pre wrap=""><blockquote type="cite"><pre><pre wrap=""># Copy-pasted from c_client.py code ends here+</pre></pre></blockquote> <blockquote type="cite"><pre><pre wrap="">+# Some hacks to make the API more readable and to keep backwards compability +_cname_special_cases = {'DECnet':'decnet'} +_extension_special_cases = ['XPrint', 'XCMisc', 'BigRequests'] +_cname_re = re.compile('([A-Z0-9][a-z]+|[A-Z0-9]+(?![a-z])|[a-z]+)') </pre></pre></blockquote></pre></pre> <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> </blockquote> <pre wrap="">please add a comment to describe what the regex should do (roughly anyway). I can probably figure this out with a bit of time, but then I likely still won't know whether it's what you intended to do :) </pre> <blockquote type="cite"> <pre wrap="">+ +_hlines = [] +_hlevel = 0 +_clines = [] +_clevel = 0 +_ns = None +_filename = '' +requests = {} +_indent = [] </pre> </blockquote> <pre wrap="">huh, I know that python uses _ to try to hide things, it's not clear here why requests doesn't do this. Also, afaict this is just a standalone program, not a library. I don't think we need to worry about visibility anyway.</pre> </blockquote> I used underscore to hide global variables, because I read that it's a good tone and to follow c_client.py's naming traditions (with 'requests' I just forgot to name it properly). But, when thinking about separating reused code to a different file, the main reason I've not done it yet is that under-scored named vars cannot be imported via import command, they are visible only within their file. Still don't know if it's a feature or a bug. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+ +def _increase_indent(): + global _indent + _indent.append(' ') + +def _decrease_indent(): + global _indent + if _indent: # if not empty + _indent.pop() + +# XXX See if this level thing is really necessary. +def _h_setlevel(idx): + ''' + Changes the array that header lines are written to. + Supports writing different sections of the header file. + ''' + global _hlevel + while len(_hlines) <= idx: + _hlines.append([]) + _hlevel = idx + +def _c_setlevel(idx): + ''' + Changes the array that source lines are written to. + Supports writing to different sections of the source file. + ''' + global _clevel + while len(_clines) <= idx: + _clines.append([]) + _clevel = idx + +def _n_item(str): </pre> </blockquote> <pre wrap="">"n_item" usually refers to "number of items". can we rename this into something more expressive? same goes for _ext, _n and _t, no idea what they do based on the function name alone. </pre> <blockquote type="cite"> <pre wrap="">+ ''' + Does C-name conversion on a single string fragment. </pre> </blockquote> <pre wrap="">an example of the conversion would be nice here (same below)</pre> </blockquote> This issue and the one above are about c_client.py code that I use, as C. Linhart said before. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+ Uses a regexp with some hard-coded special cases. + ''' + if str in _cname_special_cases: + return _cname_special_cases[str] + else: + split = _cname_re.finditer(str) + name_parts = [match.group(0) for match in split] + return '_'.join(name_parts) + +def _ext(str): + ''' + Does C-name conversion on an extension name. + Has some additional special cases on top of _n_item. + ''' + if str in _extension_special_cases: + return _n_item(str).lower() + else: + return str.lower() + +def _n(list): + ''' + Does C-name conversion on a tuple of strings. + Different behavior depending on length of tuple, extension/not extension, etc. + Basically C-name converts the individual pieces, then joins with underscores. + ''' + if len(list) == 1: + parts = list + elif len(list) == 2: + parts = [list[0], _n_item(list[1])] + elif _ns.is_ext: + parts = [list[0], _ext(list[1])] + [_n_item(i) for i in list[2:]] + else: + parts = [list[0]] + [_n_item(i) for i in list[1:]] + return '_'.join(parts).lower() + +def _t(list): + ''' + Does C-name conversion on a tuple of strings representing a type. + Same as _n but adds a "_t" on the end. + ''' + if len(list) == 1: + parts = list + elif len(list) == 2: + parts = [list[0], _n_item(list[1]), 't'] + elif _ns.is_ext: + parts = [list[0], _ext(list[1])] + [_n_item(i) for i in list[2:]] + ['t'] + else: + parts = [list[0]] + [_n_item(i) for i in list[1:]] + ['t'] + return '_'.join(parts).lower() + +#to replace part ends here + + +# Fuctions required by output </pre> </blockquote> <pre wrap="">typo, Functions </pre> <blockquote type="cite"> <pre wrap="">+ +def c_open(self): + ''' + Exported function that handles module open. + Opens the files and writes out the auto-generated comment, + header file includes, etc. + ''' </pre> </blockquote> <pre wrap="">tab/space indentation mixup </pre> <blockquote type="cite"> <pre wrap="">+ global _ns + _ns = self.namespace + _ns.c_ext_global_name = _n(_ns.prefix + ('id',)) + + global _filename + _filename = ''.join(('swapcheck_',_ns.header)) + + _h_setlevel(0) + _c_setlevel(0) + + _hc('/*') + _hc(' * This file generated automatically from %s by gen_swap_check.py.', _ns.file) + _hc(' * Edit at your peril.') + _hc(' */') + _hc('') + + _h('/**') + _h(' * @defgroup XCB_%s_API XCB %s API', _ns.ext_name, _ns.ext_name) + _h(' * @brief %s XCB Protocol Implementation.', _ns.ext_name) + _h(' * @{') + _h(' **/') + _h('') + _h('#ifndef __%s_H', _ns.header.upper()) + _h('#define __%s_H', _ns.header.upper()) + _h('') + + # swapcheck_xproto.h is include in all the others' extensions header files, so + # it's very convenient to include the common libs into this header + if _ns.header == 'xproto': + _h('#include "xorg/misc.h"') + _h('#include "X11/X.h"') + _h('#include "X11/Xproto.h"') + else: + _hc('#include "swapcheck_xproto.h"') + + _c('#include "%s.h"', _filename) + _c('#include <stdlib.h>') + _c('#include <assert.h>') + _c('#include <stddef.h> /* for offsetof() */') + _c('#include <errno.h>') + + _c('') + _c('#define ALIGNOF(type) offsetof(struct { char dummy; type member; }, member)') + + _h('') + _h('#ifdef __cplusplus') + _h('extern "C" {') + _h('#endif') + + if _ns.is_ext: + _h('') + _h('#define XCB_%s_MAJOR_VERSION %s', _ns.ext_name.upper(), _ns.major_version) + _h('#define XCB_%s_MINOR_VERSION %s', _ns.ext_name.upper(), _ns.minor_version) + _h('') #XXX + #_h('extern xcb_extension_t %s;', _ns.c_ext_global_name) + + _c('') + #_c('xcb_extension_t %s = { "%s", 0 };', _ns.c_ext_global_name, _ns.ext_xname) + + _hc('') + +def c_close(self): + ''' + Exported function that handles module close. + Writes out all the stored content lines, then closes the files. + ''' + _c_setlevel(0) + for reqnum in requests: + _c('#define %s %s', 'GEN_' + _n(requests[reqnum]).upper(), reqnum) + _c('') + + _h_setlevel(2) + _c_setlevel(2) + _hc('') + + for kind in swapCheckDescendants: + generate_dispatch(kind(RequestHandler()), self.namespace.header) + + _h('') + _h('#ifdef __cplusplus') + _h('}') + _h('#endif') + + _h('') + _h('#endif') + _h('') + _h('/**') + _h(' * @}') + _h(' */') + + # Ensure the gen subdirectory exists + _gendir = 'gen' </pre> </blockquote> <pre wrap="">shouldn't this come from the Makefile? or at last be defined at the top somewhere so it's easy to find.</pre> </blockquote> <span style="color: rgb(51, 204, 0); font-family: 'Times New Roman'; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; display: inline !important; float: none; background-color: rgb(255, 255, 255);">Moved it to proto/Makefile.am. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ try: + os.mkdir(_gendir) + except OSError as e: + if e.errno != errno.EEXIST: + raise + + # Write header file + hfile = open('%s/%s.h' % (_gendir, _filename), 'w') + for list in _hlines: + for line in list: + hfile.write(line) + hfile.write('\n') + hfile.close() + + # Write source file + cfile = open('%s/%s.c' % (_gendir, _filename), 'w') + for list in _clines: + for line in list: + cfile.write(line) + cfile.write('\n') + + cfile.close() + +def c_simple(self, name): + ''' + Exported function that handles cardinal type declarations. + These are types which are typedef'd to one of the CARDx's, char, float, etc. + ''' + #print(name) + #print(self) </pre> </blockquote> <pre wrap="">can we drop the functions that don't do anything? Or at least comment why they don't do anything?</pre> </blockquote> They still need an implementation, I will be working on them in couple of days. Commented as 'needs a future implementation'. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ + +def c_enum(self, name): + ''' + Exported function that handles enum declarations. + + Private fields: + * fields dictonary contains (enum_number -> enum_name) pair, which + represents enum entry + ''' + _fields = {} + + if(self.values): + for entry in self.values: + _fields[entry[1]] = entry[0] </pre> </blockquote> <blockquote type="cite"> <pre wrap="">+ else: #if self.bits + for entry in self.bits: + _fields[2**entry[1]] = entry[0] # dunno if it's right + + _c('%stypedef enum %s {', _i(), _n(name)) + _increase_indent() + for enum_num in _fields: + _c('%s%s = %s,', _i(), (_n(name)+'_'+_fields[enum_num]).upper(), enum_num) + + _decrease_indent() + _c('%s} %s_t;', _i(), _n(name)) + _c('') + +def c_struct(self, name): + ''' + Exported function that handles struct declarations. + ''' + _h_setlevel(1) + _c_setlevel(1) + + for kind in swapCheckDescendants: + _swapcheck = kind(StructHandler()) + _swapcheck.generate(self, name) + +def c_union(self, name): + ''' + Exported function that handles union declarations (union is deprecated) + ''' + #print(name) + +class SwapCheck: + ''' + Represent abstract swapper/checker, that generates appropriate c-code + + Created combined because these classes share a lot functions + _name is hardcoded literal that represents name of it's class, is used to concatenate it with other stuff to generate functions + _docheck is bool that determines accurate position to define afterEnd by calling determine_afterEnd + ''' + _name = None + _docheck = False + _reusedvars = [] + _typeHandler = None + _has_struct = False + + def __init__(self, typeHandler): + self._typeHandler = typeHandler + + def access_check( self, size ): + if self._docheck: + _c('%sif( p + %u > (uint8_t*)afterEnd)', _i(), size) + _c('%s{', _i()) + _increase_indent() + _c('%sreturn BadLength;', _i()) + _decrease_indent() + _c('%s}', _i()) + + def process_fieldvalue(self, fname, ftype): + self._docheck = self._typeHandler.determine_afterEnd(self._docheck, fname) + if(fname in self._reusedvars): + _varname = 'fieldvalue' + '_' + ''.join(fname) + _datatype = _n(ftype.name) + _c('%s%s %s;', _i(), _datatype, _varname) + _c('%s%s = *(%s*)p;', _i(), _varname, _datatype) + + def funcName(self, name): + return '_'.join(name) + '_' + self.nameToString() + + def printHeader(self, type): + _hc('int') + self._typeHandler.printSwapCheckFuncSignature(self.funcName(type.name)) + _increase_indent() + _c('%s//type, field_type, field_name, visible, wire, auto, enum, isfd', _i()) + _c('') + _c('%suint8_t* p = (uint8_t*)data;', _i()) + + if self._has_struct: + self._typeHandler.initAfterStruct() + self._typeHandler.init_afterEnd() + + _c('') + + def printFieldHeader(self, field): + _c('%s//%s, %s, %s, %s, %s, %s, %s', _i(), + field.field_type, + field.field_name, + field.visible, + field.wire, + field.auto, + field.enum, + field.isfd) </pre> </blockquote> <pre wrap="">whoah, what happened here? that's a bit excessive on the indenting</pre> </blockquote> I hope, it is more good-looking now :). <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+ + def process_simple(self, fname, ftype): + pass + + def process_list(self, ftype, it, llen): + _c('%s{', _i()) + _increase_indent() + _c('%sunsigned int %s;', _i(), it) + + _c('%sfor(%s = 0; %s < %s; %s++)', _i(), it, it, llen, it) + _c('%s{', _i()) + + _increase_indent() + self.check(ftype.member, None) + _decrease_indent() + + _c('%s}', _i()) + _decrease_indent() + _c('%s}', _i()) + + def process_struct(self, tname): + self._typeHandler.process_struct(self.funcName(tname)) + + def process_switch(self, ftype): + _casevarn = 'fieldvalue_' + ftype.expr.lenfield_name # case variable name + _c('\n%s//switch begins\n', _i()) + + for case in ftype.bitcases: + _eq_sign = '==' if case.type.is_case else '&' + + if case.type.expr: # if bitcase/case has enumref + _enumn = case.type.expr[0].lenfield_type.name # enum name + _enument = case.type.expr[0].lenfield_name # enum entry name #TODO WHAT IF NOT 0? + _c('%sif (%s %s %s)', _i(), + _casevarn, _eq_sign, + (_n(_enumn)+'_'+_enument).upper()) + _c('%s{', _i()) + _increase_indent() + for field in case.type.fields: + _c('%s//%s %s', _i(), field.type.name, field.field_name) + self.check(field.type, field.field_name) + _decrease_indent() + _c('%s}', _i()) + _c('') + + _c('%s//switch ends\n', _i()) + + def check(self, ftype, fname): + _size = ftype.size + if ftype.is_simple or ftype.is_expr: + self.process_simple(fname, ftype) + _c('%sp += %u;', _i(), _size) + elif ftype.is_pad and ftype.fixed_size(): + byts = ftype.nmemb + _c('%sp += %u;', _i(), byts) + elif ftype.is_pad and not ftype.fixed_size(): + al = ftype.align + _c('%sp += %u;', _i(), al) + elif ftype.is_list and ftype.fixed_size(): + self.process_list(ftype, 'i_'+fname, ftype.nmemb) + elif ftype.is_list and not ftype.fixed_size(): + self.process_list(ftype, + 'i_'+ftype.expr.lenfield_name, + 'fieldvalue_' + ftype.expr.lenfield_name) + elif ftype.is_switch: + self.process_switch(ftype) + elif ftype.is_container: + self.process_struct(ftype.name) + else: + _c( '%s#error yet not implemented', _i()) + + def generate(self, item, name): + self._docheck = self._typeHandler.init_docheck(self._docheck) + _afterEnd = None + + self._reusedvars = self.checkReusedVariables(item, []) + for field in item.fields: + if(self.isAfterStructNedeed(field.type)): + break + self.printHeader(item) + + for field in item.fields: + self.printFieldHeader(field) + self.check(field.type, field.field_name) + self.printEmpty() + self._typeHandler.printFooter() + self._typeHandler.printReturn() + + def nameToString(self): + return self._name + + def isAfterStructNedeed(self, ftype): + ''' + recurce over every field in request to find out whether it contains structs + + to avoid afterEnd is declared but not used warning we iterate over all + fields in the request and if it contains a single struct entry we must + declare it, otherwise we must not. + ''' + if ftype.is_list: + self._has_struct = self.isAfterStructNedeed(ftype.member) # check if members of list are structs + elif ftype.is_switch: + for sfield in ftype.fields: # check if any field of switch is a struct + if self.isAfterStructNedeed(sfield.type): + self._has_struct = True + elif ftype.is_container: + self._has_struct = True + else: + self._has_struct = False + return self._has_struct + + def printEmpty(self): + _c('') + + def checkReusedVariables(self, _container, other): + appendvars = [] + listvars = [] + othervars = other + for field in _container.fields: + if field.type.is_list or field.type.is_switch: + listvars.append(field.type.expr.lenfield_name) + if field.type.is_switch: + appendvars.extend(self.checkReusedVariables(field.type, othervars)) + else: + othervars.append(field.field_name) + listvars = list(set(listvars) & set(othervars)) + listvars.extend(appendvars) + return listvars + +class SwapParent(SwapCheck): + ''' + Represents abstract class to generate toClient and fromClient swapping functions + ''' + def process_simple(self, fname, ftype): + self.before_swap_simplefield(fname, ftype) #before swap hook + self.swap_simplefield(ftype.size) + self.after_swap_simplefield(fname, ftype) #after swap hook + + def swap_simplefield(self, _size ): + if _size == 1: + pass + elif _size == 2: + self.access_check(_size ) + self.swap_with_swapper('swaps', 'uint16_t') + elif _size ==4 : + self.access_check( _size ) + self.swap_with_swapper('swapl', 'uint32_t') + else: + _c( ' #error swap of size %d not implemented', _size ) + + def swap_with_swapper( self, swapper, size ): + _c('%s%s((%s*)p);', _i(), swapper, size) + + def before_swap_simplefield( self, fname, ftype ): + pass + + def after_swap_simplefield( self, fname, ftype ): + pass + +class SwapFromClient(SwapParent): + _name = 'SwapFromClient' + + def after_swap_simplefield( self, fname, ftype): + self.process_fieldvalue( fname, ftype ) + +class SwapToClient(SwapParent): + _name = 'SwapToClient' + + def before_swap_simplefield( self, fname, ftype): + self.process_fieldvalue( fname, ftype) + +class Check(SwapCheck): + _name = 'Check' + + def process_simple(self, fname, _size): + self.process_fieldvalue(fname, _size) + +class TypeHandler: + def printFooter(self): + pass + + def determine_afterEnd(self, _docheck, fname): + pass + + def printReturn(self): + _c(' return Success;') + _c(' else') + _c(' return BadLength;') + _c('}') + _decrease_indent() + _hc('') + + def printSwapCheckFuncSignature(self, name): + pass + + def init_afterEnd(self): + pass + + def init_docheck(self, docheck): + pass + + def initAfterStruct(self): + pass + + def process_struct(self, name): + pass + +class RequestHandler(TypeHandler): + def printFooter(self): + _c(' if (p == afterEnd)') + + def determine_afterEnd(self, _docheck, fname): + """ + Overloaded function to return the appropriate value of _docheck, + + unlike the StructHandler does + """ + if fname == 'length': + _c(' afterEnd = ((uint8_t*)data) + 4 * ( *(uint16_t*)p );') + return True + else: + return _docheck + def printSwapCheckFuncSignature(self, name): + _h('%s(void *data);', name) + _c('%s(void *data)\n{', name) + + def init_afterEnd(self): + _c(' uint8_t* afterEnd = NULL;') + + def init_docheck(self, docheck): + return False + + def initAfterStruct(self): + _c(' uint8_t* afterStruct = NULL;') + + def process_struct(self, name): + _c('%sif(%s(p, afterEnd, &afterStruct) != Success)\n\t\t\treturn BadLength;', _i(), name) + _c('%sp = afterStruct;', _i()) + +class StructHandler(TypeHandler): + def printFooter(self): + _c(' *afterStruct = p;') + _c(' if (p <= (uint8_t*)afterEnd)') + + def printSwapCheckFuncSignature(self, name): + _h('%s(void *data, void *afterEnd, uint8_t **afterStruct);', name) + _c('%s(void *data, void *afterEnd, uint8_t **afterStruct)\n{', name) + + def init_docheck(self, docheck): + return True + + def process_struct(self, name): + _c('%sif(%s(p, afterEnd, afterStruct) != Success)\n\t\t\treturn BadLength;', _i(), name) + _c('%sp = (uint8_t*)(*afterStruct);', _i()) + + def determine_afterEnd(self, _docheck, fname): + """ + Overloaded function to return the same value as _docheck had, + + unlike the RequestHandler's one + """ + return _docheck + +def generate_dispatch(kind, name): + """ + Function to generate dispatch function + + kind is an object of class SwapCheck + name is the name of extension + """ + #print function header-signature + _hc('%sint', _i()) + _h('%sxcb_%s_dispatch(void *req);', _i(), name+'_'+kind._name) + _c('%sxcb_%s_dispatch(void *req)\n{', _i(), name+'_'+kind._name) + _increase_indent() + + # init variables according to c90 std + _c('%slong switch_item;', _i()) + _c('%sint return_val = 0;', _i()) + _c('%sxReq* request = (xReq*)req;', _i()) + + # define varible to switch, depends on extension + _switch_item = 'request->' + _switch_item = _switch_item + 'reqType' if name == 'xproto' else _switch_item + 'data' + _c('%sswitch_item = %s;', _i(), _switch_item) + + # print switch header + _c('%sswitch(switch_item)', _i()) + _c('%s{', _i()) + _increase_indent() + + # for every opcode in generated (opcode -> name) dict "requests" + # print case + for reqnum in requests: + _c('%scase %s:', _i(), 'GEN_'+_n(requests[reqnum]).upper()) + _increase_indent() + _c('%sreturn_val = %s((void*)request);', _i(), + kind.funcName(requests[reqnum])) + _c('%sbreak;', _i()) + _decrease_indent() + + _c('%s}', _i()) + _decrease_indent() + _c('%sreturn return_val;', _i()) + _decrease_indent() + _c('%s}', _i()) + +def c_request(self, name): + ''' + Exported function that handles request declarations. + ''' + _h_setlevel(1) + _c_setlevel(1) + global requests + requests[self.opcode] = name + for kind in swapCheckDescendants: + swapcheck = kind(RequestHandler()) + swapcheck.generate(self, name) + +swapCheckDescendants = {SwapFromClient, SwapToClient, Check} </pre> </blockquote> <pre wrap="">move this to the top of the file somewhere pls</pre> </blockquote> Christian already commented on this; I can add that, as the list declaration should proceed the classes declarations, I can move the whole declaration bunch of code to the top but this way or another the list will be declared after a huge piece of code. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+ +def c_event(self, name): + ''' + Exported function that handles event declarations. + ''' + #print(name) + +def c_error(self, name): + ''' + Exported function that handles error declarations. + ''' + #print(self) + #print(name) </pre> </blockquote> <pre wrap="">hmm, so gut feeling tells me that you probably want some error message here so you don't accidentally create swapping code that skips events or errors.</pre> </blockquote> Christian already commented about the functions we cannot drop, because they are required by output dictionary (declared at the end). <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+ +# Must create an "output" dictionary before any xcbgen imports. +output = {'open' : c_open, + 'close' : c_close, + 'simple' : c_simple, + 'enum' : c_enum, + 'struct' : c_struct, + 'union' : c_union, + 'request' : c_request, + 'event' : c_event, + 'error' : c_error, + } + +# Check for the argument that specifies path to the xcbgen python package. +try: + opts, args = getopt.getopt(sys.argv[1:], 'p:') +except getopt.GetoptError as err: + print(err) + print('Usage: gen_swap_check.py [-p path] file.xml') + sys.exit(1) + +for (opt, arg) in opts: + if opt == '-p': + sys.path.insert(1, arg) + +# Import the module class +try: + from xcbgen.state import Module + from xcbgen.xtypes import * +except ImportError: + print(''' +Failed to load the xcbgen Python package! +Make sure that xcb/proto installed it on your Python path. +If not, you will need to create a .pth file or define $PYTHONPATH +to extend the path. +Refer to the README file in xcb/proto for more info. +''') + raise + +module = Module(args[0], output) +module.register() +module.resolve() +module.generate() </pre> </blockquote> <pre wrap="">this is the C programmer in me, but I've really started liking the python approach of: def main(argv): # do stuff if __name__ == "__main__": main(sys.argv) IMO it nicely groups the code to run when the file is executed. That's about all I have for now. I admit the python bits made my head spin, they're a bit hard to decypher (not your fault). I haven't run this yet, so I need to look at the output to comment any further. Meanwhile, most of what I pointed out above is easy to fix and a trivial change, please incorporate those, it'll make review a lot easier. Cheers, Peter</pre> </blockquote> Lots and lots lines in the patch changed only because I removed trailing whitespaces and spare new lines. I would like to work on project during this week informally. Thanks, best regards, Asalle P. S. Sorry for the delay, I needed to handle with some health problems. <blockquote cite="mid:20150217073637.GA15359@jelly.redhat.com" type="cite"> </blockquote> </div> </body> </html>