lcd4linux-devel

Re: [Lcd4linux-devel] small patch for plugin_mkfifo

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

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

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

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
>

Re: [Lcd4linux-devel] small patch for plugin_mkfifo

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

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
>>
>

Re: [Lcd4linux-devel] small patch for plugin_mkfifo

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

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
>>>
>>
>

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
>