[mythtv] [mythtv-commits] Ticket #1945: DVB-S/diseqc patch

Yeasah Pell yeasah at schwide.com
Sun Jul 2 17:19:10 UTC 2006

Daniel Kristjansson wrote:

>I have not even attempted to run this code, this is just 3-4
>passes of fixing obvious mistakes with respect to MythTV coding
>standards and conformity with the rest of the code, and adding
>error checking where system libraries are likely to return errors.
>There may have also been some thread safety fixes, but I know
>the code is not really thread safe yet.
>One way for you go about this is to start with your patch and 
>apply the fixes in the branch to your code one by one. Testing
>along the way. When anything breaks try to fix it or go back
>to the previous version and start applying other fixes. Repeat
>this process until you have something that looks like the code
>in SVN but doesn't have any of the problems the code in SVN has.
>Then just send me a patch with the differences. I'll apply any
>bits that are ok, and either fix other problems another way or
>punt them back to you with an explanation.
>-- Daniel
I can't easily do what you're suggesting here, because the branch has so 
many changes all in one changeset, it would be more difficult to tease 
apart the individually coherent changes than it is worth. I'll enumerate 
the changes I know about here.

Things I agree with, or at least don't have a major problem with:

1) Changes to the names of many files, classes, types and variables -- I 
certainly don't have a problem with that.
2) Changes to the method of adhering to USING_DVB -- I just copied the 
method used in dvbchannel, but I thought that was extremely hokey and 
I'm glad to see it changed.
3) Movement of the db upgrade code into dbcheck.cpp and the assignment 
of a new db schema number -- great, I've been asking for that for as 
long as there's been a patch.
4) Movement of some stuff which is technically private from the diseqc 
headers to the implementation files -- sure, I agree with that. I don't 
much care for interface documentation in the implementation file, but I 
understand the desire to reduce the impact of changes on recompliations, 
and thus keeping the headers as simple as possible.
5) Random syntax changes, no problem with that.
6) Changing from STL containers to Qt containers -- they are similar 
enough that it's not a big deal, and there's plenty of STL in the 
application already, so I figured it was ok. But I can understand the 
desire to not use two different implementations of the same thing.
7) Adding the new GUI file to the translation qmake file -- great, I 
didn't even know about that.
8) Additional db error checking everywhere, which is very good.
9) Changing the default source id on the tune methods to be zero instead 
of -1 -- fine, I figured it would be better to use a value that was 
entirely out of the range of the database field, but autoincrement won't 
ever cough up a zero normally so I suppose it's just as good (and then 
it can use a uint for the ID like it should) -- OTOH if somebody ever 
makes myth multi-database something like that could be a problem.
10) Locking added to the signal monitor stuff. Great, I wasn't sure 
exactly what was required in the signal monitor for thread safety.
11) In general checking if a pointer is NULL before deleting it. I 
dislike that, just because it perpetuates the common misconception that 
it is unsafe to delete a null pointer, and explicitly refuses to use a 
feature of the language that was put there for that exact purpose. But 
it's harmless, so I don't really have a problem with it.

Things that I don't really agree with -- I'm not particularly happy with 
some of these changes, but I'm not about to argue them either, as it's 
not my project:

12) Changing the database identifier lookup functions to use arrays of 
QStrings instead of const ascii strings. That one baffles me -- it's 
more complex and has more overhead with QStrings, and I don't see any 
benefit there -- it's not like i18n'd or unicode db identifiers makes 
any sense, and the underlying strings are still stored as const char* 
entities in the library's constant data section in either case. Also, 
the arrays are now initialized with an explicit size now instead of 
allowing the compiler to determine the array count -- there is just no 
benefit to that, and it can only cause problems.
13) Generally changing things to favor compactness. I can't say I 
generally agree with all of these changes. For example, adding multiple 
explicit return statements to a function can be problematic from a 
maintenance perspective when done to even moderately complex functions, 
as the chance that somebody will decide to add some code to that 
function after an earlier conditional return that they didn't spot is 
too great, and if that conditional is a rare case such a fault may not 
be detected for a long time.
14) Similarly, eliminating interim temporary values is IMO generally a 
loss, since the output of any half-assed optimizing compiler will be 
indentical in virtually all cases (in fact, breaking subexpressions out 
explicitly can help some crappy optimizers in some cases), but you lose 
a piece of self-documentation in that the code no longer assigns a 
meaningful name to a subexpression -- plus it just adds more work later 
if/when the function becomes more complicated and needs to reference the 
subexpression multiple times.
15) A comment indicating that the QTime object that is used to determine 
the elapsed time since the last tuning request is broken at midnight 
because of a wrap -- I don't think this is so. The QTime documentation 
only states that the elapsed() method wraps after 24 hours of elapsed 
time, which means it would only be a problem if 24 hours had passed 
after an initial tune and then a retune was somehow required, which 
sounds pretty absurd to me -- and even then, the only negative effect 
would be that it would take an extra retune interval of time (since the 
counter resets to zero) -- even if this can actually happen at all, 
after 24 hours of watching one thing, I doubt waiting an extra retune 
interval is going to be a problem.
16) Moving two of the diseqc ioctl methods that previously resided in 
the main tree class into static module functions. I definitely don't 
agree with that. Currently it is certainly possible to do that, since 
those methods didn't currently use any state from the tree object 
(actually they did, but you changed it to be passed in), but I can 
easily expect that they might use more significant state in the future 
as people report that their particular gear needs more tweaks, and one 
of the methods had to stay anyway (so now two out of three of a 
symmetric set of methods have been randomly moved to static non-class 
functions) -- I can't see any good rationale here.

Things that are definitely bugs:

17) Deleting the diseqc tree object in dvbchannel. Not right, needs to 
be removed.
18) Changes to the device IDs to use unsigned ints. Ah, I see why some 
of the code is broken now. In order to assign unique IDs to nodes that 
haven't been created yet, the device tree temporarily assigns nodes with 
negative number IDs as placeholders until the final store command, at 
which point the nodes are inserted into the database and proper positive 
number IDs are assigned for all the new nodes. By changing the IDs to be 
unsigned, that totally breaks that. It's true that the original code 
couldn't properly represent device IDs greater than ~2 billion, but 
changing them all to be unsigned is not a solution to that. Avoiding the 
negative number thing would result in a more complex solution (maybe 
storing a unique id plus a bool that indicates if it's "real" or not, 
and generating strings for the UI to index everything off of based on a 
formatted combination of the bool and int), but it would work -- another 
much more simple option would simply be to alter the table IDs to match 
the range of a signed int.
19) The switch SetChild() method no longer checks the supplied ordinal 
to see if it's in the range of the array, but instead tries to retrieve 
a child at that ordinal and fails if there is no child object. Of course 
this breaks the switch, since it means that you can't add child nodes 
unless there are already child nodes present. I imagine this is the 
source of the no-switch-children problem I noticed.

... That's about all I have the stamina for so far, which is just what I 
can see by manual comparison about halfway through the main diseqc file, 
but I hope you can see why I don't have a lot of confidence in the 
changeset as a whole right now -- since some of the functional changes 
are problematic in a way that indicates a failure to fully understand 
the new code, coupled with the fact that it's not straightforward at all 
for me to locate the actual functional changes, and it's virtually 
impossible to know if I've reviewed them all. I expect there could 
easily be more subtle problems that have been introduced that might not 
show up in testing.

Regarding thread safety, I think the most important thing is that the 
tree be locked for the lifetime of a retrieved rotor object (when the 
signal monitor requests the rotor from the channel class and uses it to 
update its values), which it currently isn't -- beyond that, the tree 
itself could easily be locked when it is in use -- I think that's not 
strictly necessary given the current usage patterns (if you have two 
dvbchannel objects trying to use the same device at the same time you've 
got other problems, and I assume that should never happen), but it would 
be a trivial addition so it's probably a good idea whether it's strictly 
needed or not -- to avoid failure cascades. As far as I remember, the 
other objects are either definitively single-threaded or already locked.

More information about the mythtv-dev mailing list