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=196678 --- 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 mean to criticize anyone, rather I criticized the state of the resulting code which was a mix of styles and had some errors that could be due to multiple authors and/or following bad examples elsewhere. So, I passed through it and cleaned 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. The outer loop iterates i with an early continue until the correct device is found 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 that intend to use i as an index into hw_type_path[]. This variable re-use is only safe if flags and xdriver are assigned before the inner loop, at the same time 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 new 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 building doesn't realloc, but it does a calloc() every time. If that succeeds, the new 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, one to hold the list we are building, and one to hold the return value while we check it before assigning to the list pointer. Without the use of a second pointer, 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 gotos, 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. -- 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>