Re: [Lcd4linux-devel] small patch for plugin_mkfifo

[Michael R. <mi...@re...>]

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