Arjen de Korte
2009-Jan-27 18:38 UTC
[Nut-upsdev] [PATCH] al175: updated driver, please restore it
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