Re: [Lcd4linux-devel] small patch for plugin_mkfifo

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

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
>