[Nut-upsdev] [PATCH] al175: updated driver, please restore it

[Arjen de Korte]

Citeren Kirill Smelkov <kirr at mns.spb.ru>:

> So what would we do about this patch?

It fails to address the concerns raised, so we have no other option  
than to reject it.

> I've tried to address the issues you've raised, and to describe my
> rationale behind alarm().

The use of alarm() here is inappropriate (and possibly incorrect as  
well, see below). As stated before, none of the other drivers require  
the use of alarm(), you don't have demonstrated a reason why the AL175  
does so it shouldn't use it either. The alarm() function has many side  
effects on other functions, most particularly with timer functions on  
some systems. Therefor, the use of this should be kept to a bare  
minimum in drivers. We intend to use the timer functions in the  
future, to prevent drivers from stalling the communication with the  
server and timing out. This is a frequent problem and we want to solve  
this in a generic way for all drivers and not on a driver-by-driver  
basis (like you did in the AL175 driver now).

Your misconception is that you need to calculate the time spent during  
the communication. This is not the case. The ser_get_*() functions  
will return as soon they have read the number of bytes requested or  
the end character was found, no matter how much time is remaining. The  
timeout is only there to make sure that if characters are lost or the  
UPS is disconnected, the driver can break from (usually)  
upsdrv_updateinfo() and retry the next time it goes there. If you find  
that during normal communication with the UPS you're already getting  
close to the magic 5 seconds, using alarm() to bail out is incorrect.  
You might never reach the last entries in the list of queries to the  
UPS you would like to do. The right solution would be to break up the  
queries to the UPS in smaller pieces and perform only a limited set  
each time upsdrv_updateinfo() is called (see documentation). You could  
check for the OL/OB status on each poll and round robin select a  
couple of queries for other variables.

> If it can't go in -- that's ok with me, but could you please at least
> tell me what you objections (if any) are now.

My main objection is still the alarm() function (which I have told you  
a couple of times already), although I also don't like what you did to  
replace the alloca() functions. Dynamic memory allocation should be  
avoided if possible (due to possible leaking/fragmentation of memory)  
as it is hard to diagnose in a backgrounded driver. I absolutely fail  
to understand why you want to use structures to deal with data going  
in and out of the UPS. There is absolutely no reason/need to attempt  
to read all data in one large chunk. It's really beyond me why you  
don't just read the (fixed length) data header, determine how many  
data characters will follow based on information in this header, read  
exactly that number of characters and then read the closing characters:

     ser_get_buf(<header>)
     parse header data ('n' bytes)
     ser_get_buf(<read 'n' bytes>)
     calculate checksum
     ser_get_buf(<checksum and closing characters>)
     compare checksum

The reason for being so picky on the above issues, is that we already  
have far too many drivers in the tree that are impossible to  
understand after the original author has lost interest in maintaining  
it.

Also, use library calls as much as possible. Don't add new functions  
with almost the same functionality. Pretty printing ASCII control  
characters in addition to the existing upsdebug_hex may be useful for  
someone very familiar with the protocol used, but usually it is much  
more informative if you decode the meaning of a message. So instead of  
pretty printing the header, break up the elements and list them. The  
upsdebug_hex function should be used only to display raw data for  
diagnosing protocol errors. You've received something, but are unable  
to parse it. In that case, the raw data may help diagnosing the problem.

When looking at the sources, it should be immediately clear in which  
debug level messages will be printed (in numbers, we don't want to  
have to lookup the value of a macro first, to see that it is printed  
in debug level 3). Therefor, although macro's may reduce the amount of  
typing involved, they are generally a bad idea to make code more  
readable. A special problematic case here is macro's that expand into  
multiple lines. This makes debugging harder, since it is no longer  
possible to pinpoint a problem to a specific line in the source code.  
Don't do this, use a function call instead.

Last but not least, it would be helpful if you first discuss possible  
changes/solutions/countering, before actually doing them. Now you've  
spent considerable effort in making changes, which I feel are a waste  
of your effort and I really feel bad about having to reject them now,  
after spending all this effor. I don't want you to waste considerable  
time on changing things, if I already know that we'll be forced to  
reject them when done because they don't fix the concerns there are.

Best regards, Arjen
-- 
Please keep list traffic on the list