Date: Thu, 09 Mar 2017 15:21:33 +0000 From: bugzilla-noreply@freebsd.org To: freebsd-x11@FreeBSD.org Subject: [Bug 196678] x11-servers/xorg-server: make config/devd recognize /dev/input/eventX from multimedia/webcamd Message-ID: <bug-196678-8047-PYmiW2wsed@https.bugs.freebsd.org/bugzilla/> In-Reply-To: <bug-196678-8047@https.bugs.freebsd.org/bugzilla/> References: <bug-196678-8047@https.bugs.freebsd.org/bugzilla/>
next in thread | previous in thread | raw e-mail | index | archive | help
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=3D196678 --- Comment #56 from Matthew Rezny <rezny@freebsd.org> --- (In reply to rozhuk.im from comment #55) I know there are multiple authors of this revision, I did not look through = the different versions to attribute changes to anyone, and I certainly do not m= ean to criticize anyone, rather I criticized the state of the resulting code wh= ich was a mix of styles and had some errors that could be due to multiple autho= rs and/or following bad examples elsewhere. So, I passed through it and cleane= d it in such a way to try to minimize errors. I certainly must thank everyone who has contributed to the devd config code and I hope everyone has added their name to the list of copyrights. The double loop with a re-used index variable is in get_dev_type_by_path. T= he outer loop iterates i with an early continue until the correct device is fo= und in the table. Once it finds the right entry, two assignments are made, one = of which, path_offset, depends on the current value of i as its an index into hw_type_path. Then, the inner loop re-uses i to compute the value to assign= to dev_name_size. Since we eventually return from within the outer loop, it doesn't matter to the overall flow that the value of i has changed to an unrelated meaning, but there are two more assignments before that return th= at intend to use i as an index into hw_type_path[]. This variable re-use is on= ly safe if flags and xdriver are assigned before the inner loop, at the same t= ime path_offset was assigned. Perhaps the variable re-use once was safe and it evolved over time such that it no longer is. A safer approach is to use a n= ew variable for the inner loop to keep the meaning clear so that future devd hackers won't get tripped up on the ordering there. Comparing input_option_new() to realloc() is a good analogy. The list build= ing doesn't realloc, but it does a calloc() every time. If that succeeds, the n= ew list entry is created with the old pointer stored in it. If that fails, it returns NULL. Thus, safely building a list requires use of two pointers, on= e to hold the list we are building, and one to hold the return value while we ch= eck it before assigning to the list pointer. Without the use of a second pointe= r, the goto circus to call input_option_free_list() was comical. There is of course a much cleaner way to structure the code using two pointers, no goto= s, and far fewer lines of code. At least there was an attempt to do the right thing here; the hald and udev config code on the other hand never checks the return value of a call to input_option_new(), so they might call NewInputDeviceRequest() with either NULL, if they lost the whole list, or a partial list, if they lost the initial list after a large calloc failed but then got a new list when a smaller calloc succeeded in a later call to input_option_new(). The upstream code isn't exactly a good example in many ways. --=20 You are receiving this mail because: You are the assignee for the bug.=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?bug-196678-8047-PYmiW2wsed>