Re: [Lcd4linux-devel] small patch for plugin_mkfifo

[Mattia Jona-L. <mat...@gm...>]

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
>