Re: [Lcd4linux-devel] small patch for plugin_mkfifo

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 422-6466

Hi

The new plugin fifo is now in the repository with some bugfixes and
improvements.

The main differences are:
- all static memory allocations are now dynamical
- the plugin tries to match the internal buffer size with the number
of display columns; if it fails
 then it can use a parameter supplied in the configuration file; if
this parameter is absent then it uses a built in
 constant value
- some off-by-one memory overflows have been fixed or avoided

Bye!

Mattia

On Mon, Feb 8, 2010 at 12:32 PM, Mattia Jona-Lasinio
<mat...@gm...> wrote:
> Hi List!
>
> Ok, it seems that concerning the read(n) function, n being the number
> of bytes to read, a lot of things can happen. This is what I found in
> the docs.
>
> errno is a variable that specifies the type of error occured. This
> means that for errno to be meaningful there MUST be an error. If
> everything is fine then errno is undefined.
>
> read(n) is a function returning -1 in case of error or returning the
> number of bytes that were read, if any (this number can be zero). It
> is NOT considered an error if this number is less than n. Maybe we
> reached the end of file or we were interrupted by the system.
>
> What you can count on is the following.
>
> If read(n) returns -1 then errno is set accordingly, to specify the error.
> If read(n) returns n then everything went fine and errno is undefined.
>
> Here comes the "funny" part.
> If errno == EINTR then we were interrupted by the system BUT it is
> unspecified (POSIX allows for both behaviors) whether read must return
> the number of bytes actually read or -1. And in the latter case it is
> unspecified whether the file position is incremented or not so we
> don't know whether we will reread the same data twice or we will read
> new data. This is a killer!!! Anyway. As far as I know the Linux
> implementation in case of an interruption by the system is to return
> the bytes that were read and to set errno to EINTR, so we don't have
> to worry about the file position but we cannot rely on read(n) < n to
> assume errno is valid because, for example, if we are not interrupted
> but simply we have a short read then read(n) < n but errno is not set
> (undefined).
>
> So, to make a long story short we have three possible cases:
> if read(n) == -1 then errno is valid.
> if 0 <= read(n) < n then errno MIGHT be valid and set to EINTR. But in
> the end who cares? Even if it were the case we also have the number of
> bytes actually read and we have to treat them. So the errno == EINTR
> case does not deserve a special treatment in the end. We might spot
> this case by explicitely clearing errno (horrible! :) ) before calling
> read, but then what? In any case we will be called again by the main
> routine of lcd4linux.
> if read(n) == n then errno is invalid.
>
> Since I heavily rewrote most of the code in a (to me) more rational
> and better way, I send the complete plugin_fifo.c file instead of a
> diff file.
> And before committing it I'd like to have some feedback on it, to be
> sure that the plugin has the intended behavior. Of course I'm open to
> suggestions, improvements, criticisms, etc etc ;)
>
> Bye!
>
> Mattia
>
> On Thu, Feb 4, 2010 at 3:42 PM, Mattia Jona-Lasinio
> <mat...@gm...> wrote:
>> Hi Michael,
>>
>> I tried my solution but it does not solve completely the problem. I'm
>> working on it and when I find an elegant solution, I will commit it.
>> For the moment let's keep the errno = 0 patch.
>>
>> Bye,
>>
>> Mattia
>>
>> On Thu, Feb 4, 2010 at 3:31 PM, Michael Reinelt <mi...@re...> wrote:
>>> Hi Mattia,
>>>
>>> thanks a bunch for pointing this out! I didn't read the code that careful (fortunately you did!), but I'm afraid even if
>>> I did I would have missed this...
>>>
>>> as you already provided a clean solution, could you please check it in?
>>>
>>>
>>>
>>> TIA, Michael
>>>
>>> Mattia Jona-Lasinio schrieb:
>>>> Hi Claas, Michael and The List!
>>>>
>>>> IMHO explicitely clearing the errno variable is generally a _VEEEEERY_
>>>> bad idea and should never be done.
>>>> errno should be treated like a read only variable that one must assume
>>>> to be completely undefined before a function that is allowed to modify
>>>> it, is actually called. This means that its value must not be checked
>>>> before calling the function that might change it, is actually called.
>>>> And in any case the errno value must be checked only after checking
>>>> the return value of the called function.
>>>> Unfortunately in plugin_fifo.c, line 191
>>>>
>>>> while (bytes > 0 && errno != EINTR) {
>>>>             bytes = read(fd.input, buf, FIFO_BUFFER_SIZE);
>>>> }
>>>>
>>>> the value of errno is checked BEFORE the very first call to read. Then
>>>> a previous function call might had set its value to EINTR and we are
>>>> not going to read anything just because errno was spuriously set. In
>>>> any case this piece of code is buggy because errno is set by read only
>>>> if bytes < 0 OR 0 <= bytes < FIFO_BUFFER_SIZE, but is untouched
>>>> (therefore still undefined) if bytes == FIFO_BUFFER_SIZE.
>>>>
>>>> But the solution is not clearing errno, but rather changing the code
>>>> above to fix this bug. Something like
>>>>
>>>> while ((bytes = read(fd.input, buf, FIFO_BUFFER_SIZE)) > 0) {
>>>>               if (bytes < FIFO_BUFFER_SIZE && errno == EINTR)
>>>>                             break;
>>>> }
>>>>
>>>> so that the bytes = 1 initialization is not necessary.
>>>>
>>>> At this point the subsequent test
>>>>
>>>> if (bytes < 0 || (errno > 0 && errno != EAGAIN)) {
>>>>             error("[FIFO] Error %i: %s", errno, strerror(errno));
>>>> }
>>>>
>>>> becomes meaningful but one has to distinguish the cases "bytes < 0"
>>>> (read returned an error) from "bytes < FIFO_BUFFER_SIZE" (there was
>>>> just a "short" read), so that it should be something like
>>>>
>>>> if (bytes < 0 || (bytes < FIFO_BUFFER_SIZE && errno > 0 && errno != EAGAIN)) {
>>>>             error("[FIFO] Error %i: %s", errno, strerror(errno));
>>>> }
>>>>
>>>>
>>>> What do you think about all this?
>>>>
>>>>
>>>> Bye!
>>>>
>>>>
>>>> Mattia
>>>>
>>>>
>>>>
>>>> On Thu, Feb 4, 2010 at 10:38 AM, Michael Reinelt <mi...@re...> wrote:
>>>>> Applied! Thanks!
>>>>>
>>>>> Claas Hilbrecht schrieb:
>>>>>> Hello,
>>>>>>
>>>>>> here is a very small fix (against current svn) for the plugin_fifo. You
>>>>>> need to clear errno after creating the FIFO. Without clearing the errno
>>>>>> variable you will get an error in fiforead() like this:
>>>>>>
>>>>>> [FIFO] Error 2: No such file or directory
>>>>>>
>>>>>>
>>>>>> babel@eisler:~/tmp/lcd4linux$ svn diff plugin_fifo.c
>>>>>> Index: plugin_fifo.c
>>>>>> ===================================================================
>>>>>> --- plugin_fifo.c       (Revision 1097)
>>>>>> +++ plugin_fifo.c       (Arbeitskopie)
>>>>>> @@ -110,6 +110,8 @@
>>>>>>         error("Couldn't create FIFO \"%s\": %s\n", fd.path,
>>>>>> strerror(errno));
>>>>>>         return -1;
>>>>>>      }
>>>>>> +    /* clear errno */
>>>>>> +    errno = 0;
>>>>>>      fd.created = 1;
>>>>>>      return 0;
>>>>>>  }
>>>>>>
>>>>>> Mit freundlichem Gruss
>>>>>> Claas Hilbrecht
>>>>>>
>>>>>>
>>>>>> ------------------------------------------------------------------------------
>>>>>> The Planet: dedicated and managed hosting, cloud storage, colocation
>>>>>> Stay online with enterprise data centers and the best network in the business
>>>>>> Choose flexible plans and management services without long-term contracts
>>>>>> Personal 24x7 support from experience hosting pros just a phone call away.
>>>>>> http://p.sf.net/sfu/theplanet-com
>>>>>> _______________________________________________
>>>>>> Lcd4linux-devel mailing list
>>>>>> Lcd...@li...
>>>>>> https://lists.sourceforge.net/lists/listinfo/lcd4linux-devel
>>>>>>
>>>>>>
>>>>> --
>>>>> Michael Reinelt <mi...@re...>
>>>>> http://home.pages.at/reinelt
>>>>> GPG-Key 0xDF13BA50
>>>>> ICQ #288386781
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>> The Planet: dedicated and managed hosting, cloud storage, colocation
>>>>> Stay online with enterprise data centers and the best network in the business
>>>>> Choose flexible plans and management services without long-term contracts
>>>>> Personal 24x7 support from experience hosting pros just a phone call away.
>>>>> http://p.sf.net/sfu/theplanet-com
>>>>> _______________________________________________
>>>>> Lcd4linux-devel mailing list
>>>>> Lcd...@li...
>>>>> https://lists.sourceforge.net/lists/listinfo/lcd4linux-devel
>>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>> The Planet: dedicated and managed hosting, cloud storage, colocation
>>>> Stay online with enterprise data centers and the best network in the business
>>>> Choose flexible plans and management services without long-term contracts
>>>> Personal 24x7 support from experience hosting pros just a phone call away.
>>>> http://p.sf.net/sfu/theplanet-com
>>>> _______________________________________________
>>>> Lcd4linux-devel mailing list
>>>> Lcd...@li...
>>>> https://lists.sourceforge.net/lists/listinfo/lcd4linux-devel
>>>>
>>>>
>>>
>>> --
>>> Michael Reinelt <mi...@re...>
>>> http://home.pages.at/reinelt
>>> GPG-Key 0xDF13BA50
>>> ICQ #288386781
>>>
>>
>