termios patch

termios patch

[Jay Foster]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

The attached patch fixes tcflush() and tcdrain() in the termios 
package.  These functions would call the fo_getinfo() function of the 
file system code with a 0 length (the length value is unused).  The 
fo_getinfo() code in devfs.cxx would then convert this into a non-NULL 
pointer to a value of 0 when calling cyg_io_get_config().  This 
combination (non-NULL pointer to 0 value) is a special case in 
cyg_io_get_config() used to determine if the get_config method is 
available for the device.  It always returns success (ENOERR) when the 
method is defined and does nothing.

By changing the length value passed to the fo_getinfo() function to a 
non-zero value, this avoids this.

Jay


[-- Attachment #2: termios.pat --]
[-- Type: text/plain, Size: 2456 bytes --]

Index: ecos/packages/io/serial/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/io/serial/current/ChangeLog,v
retrieving revision 1.75
diff -u -5 -p -r1.75 ChangeLog
--- ecos/packages/io/serial/current/ChangeLog	29 Jan 2009 17:49:47 -0000	1.75
+++ ecos/packages/io/serial/current/ChangeLog	2 Jun 2010 18:45:57 -0000
@@ -1,5 +1,10 @@
+2010-06-02	Jay Foster	<jay@systech.com>
+
+	* src/common/termios.c : Fix tcflush() and tcdrain().  They were
+	effectively no-op functions.
+
 2008-09-02  Steven Clugston  <steven.clugston@ncl.ac.uk>
 
 	* tests/ser_test_protocol.inl : Add mpc555 generic platform
 	* include/serialio.h : Add noise error define to line status
 	
Index: ecos/packages/io/serial/current/src/common/termios.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/io/serial/current/src/common/termios.c,v
retrieving revision 1.4
diff -u -5 -p -r1.4 termios.c
--- ecos/packages/io/serial/current/src/common/termios.c	29 Jan 2009 17:49:48 -0000	1.4
+++ ecos/packages/io/serial/current/src/common/termios.c	2 Jun 2010 18:45:58 -0000
@@ -261,11 +261,11 @@ tcdrain( int fildes )
         return -1;
     }
 
     ret = fp->f_ops->fo_getinfo( fp,
                                  CYG_IO_GET_CONFIG_SERIAL_OUTPUT_DRAIN,
-                                 NULL, 0 );
+                                 NULL, 1 );
     cyg_fp_free( fp );
 
     if ( ret > 0 ) {
         errno = ret;
         CYG_REPORT_RETVAL( -1 );
@@ -300,21 +300,21 @@ tcflush( int fildes, int queue_sel )
 
     switch( queue_sel ) {
     case TCIOFLUSH:
         ret = fp->f_ops->fo_getinfo( fp,
                                      CYG_IO_GET_CONFIG_SERIAL_OUTPUT_FLUSH,
-                                     NULL, 0 );
+                                     NULL, 1 );
         // fallthrough
     case TCIFLUSH:
         ret = fp->f_ops->fo_getinfo( fp,
                                      CYG_IO_GET_CONFIG_SERIAL_INPUT_FLUSH,
-                                     NULL, 0 );
+                                     NULL, 1 );
         break;
     case TCOFLUSH:
         ret = fp->f_ops->fo_getinfo( fp,
                                      CYG_IO_GET_CONFIG_SERIAL_OUTPUT_FLUSH,
-                                     NULL, 0 );
+                                     NULL, 1 );
         break;
     default:
         ret = EINVAL;
         break;
     }

Re: termios patch

[Sergei Gavrikov]

On Wed, 2 Jun 2010, Jay Foster wrote:
> The attached patch fixes tcflush() and tcdrain() in the termios
> package.  These functions would call the fo_getinfo() function of the
> file system code with a 0 length (the length value is unused).  The
> fo_getinfo() code in devfs.cxx would then convert this into a non-NULL
> pointer to a value of 0 when calling cyg_io_get_config().  This
> combination (non-NULL pointer to 0 value) is a special case in
> cyg_io_get_config() used to determine if the get_config method is
> available for the device.  It always returns success (ENOERR) when the
> method is defined and does nothing.
>
> By changing the length value passed to the fo_getinfo() function to a
> non-zero value, this avoids this.

Hi Jay,

Thank you for good catch!

RFC: May be we need even in an one-line comment/note in termios.c to
notice user about that "strange" length? Or may be to use some local
macro there, e.g.

#define NOBUF_NOCHECK NULL,1

I need to have the best opinion. Jifl?

Sergei

Re: termios patch

[Sergei Gavrikov]

On Sat, 5 Jun 2010, Sergei Gavrikov wrote:
> On Wed, 2 Jun 2010, Jay Foster wrote:
>> The attached patch fixes tcflush() and tcdrain() in the termios
>> package.  These functions would call the fo_getinfo() function of the
>> file system code with a 0 length (the length value is unused).  The
>> fo_getinfo() code in devfs.cxx would then convert this into a non-NULL
>> pointer to a value of 0 when calling cyg_io_get_config().  This
>> combination (non-NULL pointer to 0 value) is a special case in
>> cyg_io_get_config() used to determine if the get_config method is
>> available for the device.  It always returns success (ENOERR) when the
>> method is defined and does nothing.
>> 
>> By changing the length value passed to the fo_getinfo() function to a
>> non-zero value, this avoids this.
>
> Hi Jay,
>
> Thank you for good catch!
>
> RFC: May be we need even in an one-line comment/note in termios.c to
> notice user about that "strange" length? Or may be to use some local
> macro there, e.g.
>
> #define NOBUF_NOCHECK NULL,1
>
> I need to have the best opinion. Jifl?

To save time on testing.

I forgot to mention that was not only 'a backtrace/code view in mind'
from my side. Yesterday, I did a gDebuging a real termios example on
synthetic target. So, I confirm the Jay's report and confirm that his
workaround fixes the issue.

Sergei

Re: termios patch

[Jonathan Larmour]

On 05/06/10 15:28, Sergei Gavrikov wrote:
> On Sat, 5 Jun 2010, Sergei Gavrikov wrote:
>> On Wed, 2 Jun 2010, Jay Foster wrote:
>>> The attached patch fixes tcflush() and tcdrain() in the termios
>>> package.  These functions would call the fo_getinfo() function of the
>>> file system code with a 0 length (the length value is unused).[snip] This
>>> combination (non-NULL pointer to 0 value) is a special case in
>>> cyg_io_get_config() used to determine if the get_config method is
>>> available for the device.  It always returns success (ENOERR) when the
>>> method is defined and does nothing.
[snip]
>> RFC: May be we need even in an one-line comment/note in termios.c to
>> notice user about that "strange" length? Or may be to use some local
>> macro there, e.g.
>>
>> #define NOBUF_NOCHECK NULL,1
>>
>> I need to have the best opinion. Jifl?
> 
> To save time on testing.
> 
> I forgot to mention that was not only 'a backtrace/code view in mind'
> from my side. Yesterday, I did a gDebuging a real termios example on
> synthetic target. So, I confirm the Jay's report and confirm that his
> workaround fixes the issue.

That sounds good, and I think a comment would be valuable. A #define is
probably unnecessary. Thanks!

Jifl

Re: termios patch

[Sergei Gavrikov]

On Sat, 5 Jun 2010, Jonathan Larmour wrote:
> On 05/06/10 15:28, Sergei Gavrikov wrote:
>> On Sat, 5 Jun 2010, Sergei Gavrikov wrote:
>>> On Wed, 2 Jun 2010, Jay Foster wrote:
>>>> The attached patch fixes tcflush() and tcdrain() in the termios
>>>> package.  These functions would call the fo_getinfo() function of the
>>>> file system code with a 0 length (the length value is unused).[snip] This
>>>> combination (non-NULL pointer to 0 value) is a special case in
>>>> cyg_io_get_config() used to determine if the get_config method is
>>>> available for the device.  It always returns success (ENOERR) when the
>>>> method is defined and does nothing.
> [snip]
>>> RFC: May be we need even in an one-line comment/note in termios.c to
>>> notice user about that "strange" length? Or may be to use some local
>>> macro there, e.g.
>>>
>>> #define NOBUF_NOCHECK NULL,1
>>>
>>> I need to have the best opinion. Jifl?
>>
>> To save time on testing.
>>
>> I forgot to mention that was not only 'a backtrace/code view in mind'
>> from my side. Yesterday, I did a gDebuging a real termios example on
>> synthetic target. So, I confirm the Jay's report and confirm that his
>> workaround fixes the issue.
>
> That sounds good, and I think a comment would be valuable. A #define is
> probably unnecessary. Thanks!

Hi Jay,

Could you, please, submit new patch with an included comment (just a few
sentences)? I confess that I could not believe that tcflush() does not
work  :-) and only GDB passed me through out that 'll' and the check.
So, I think a using those "ones" won't stick in the mind without your
short notice. Thanks!

Sergei

Re: termios patch

[Sergei Gavrikov]

I cannot resist to commit (No tails to FWC :-) I hope CL will say all
about issue. Checked-in.

Thank you!

Sergei

On Sun, 6 Jun 2010, Sergei Gavrikov wrote:
> On Sat, 5 Jun 2010, Jonathan Larmour wrote:
>> On 05/06/10 15:28, Sergei Gavrikov wrote:
>>> On Sat, 5 Jun 2010, Sergei Gavrikov wrote:
>>>> On Wed, 2 Jun 2010, Jay Foster wrote:
>>>>> The attached patch fixes tcflush() and tcdrain() in the termios
>>>>> package.  These functions would call the fo_getinfo() function of the
>>>>> file system code with a 0 length (the length value is unused).[snip] 
>>>>> This
>>>>> combination (non-NULL pointer to 0 value) is a special case in
>>>>> cyg_io_get_config() used to determine if the get_config method is
>>>>> available for the device.  It always returns success (ENOERR) when the
>>>>> method is defined and does nothing.
>> [snip]
>>>> RFC: May be we need even in an one-line comment/note in termios.c to
>>>> notice user about that "strange" length? Or may be to use some local
>>>> macro there, e.g.
>>>> 
>>>> #define NOBUF_NOCHECK NULL,1
>>>> 
>>>> I need to have the best opinion. Jifl?
>>> 
>>> To save time on testing.
>>> 
>>> I forgot to mention that was not only 'a backtrace/code view in mind'
>>> from my side. Yesterday, I did a gDebuging a real termios example on
>>> synthetic target. So, I confirm the Jay's report and confirm that his
>>> workaround fixes the issue.
>> 
>> That sounds good, and I think a comment would be valuable. A #define is
>> probably unnecessary. Thanks!
>
> Hi Jay,
>
> Could you, please, submit new patch with an included comment (just a few
> sentences)? I confess that I could not believe that tcflush() does not
> work  :-) and only GDB passed me through out that 'll' and the check.
> So, I think a using those "ones" won't stick in the mind without your
> short notice. Thanks!
>
> Sergei
>

Re: termios patch

[Jay Foster]

I have been out on vacation the last couple of weeks, so have not been 
able to respond.  I see that you have committed the change which is 
fine.  You may add an additional comment if you wish.  That will be fine.
Jay

On 6/10/2010 2:00 PM, Sergei Gavrikov wrote:
> I cannot resist to commit (No tails to FWC :-) I hope CL will say all
> about issue. Checked-in.
>
> Thank you!
>
> Sergei
>
> On Sun, 6 Jun 2010, Sergei Gavrikov wrote:
>> On Sat, 5 Jun 2010, Jonathan Larmour wrote:
>>> On 05/06/10 15:28, Sergei Gavrikov wrote:
>>>> On Sat, 5 Jun 2010, Sergei Gavrikov wrote:
>>>>> On Wed, 2 Jun 2010, Jay Foster wrote:
>>>>>> The attached patch fixes tcflush() and tcdrain() in the termios
>>>>>> package.  These functions would call the fo_getinfo() function of 
>>>>>> the
>>>>>> file system code with a 0 length (the length value is 
>>>>>> unused).[snip] This
>>>>>> combination (non-NULL pointer to 0 value) is a special case in
>>>>>> cyg_io_get_config() used to determine if the get_config method is
>>>>>> available for the device.  It always returns success (ENOERR) 
>>>>>> when the
>>>>>> method is defined and does nothing.
>>> [snip]
>>>>> RFC: May be we need even in an one-line comment/note in termios.c to
>>>>> notice user about that "strange" length? Or may be to use some local
>>>>> macro there, e.g.
>>>>>
>>>>> #define NOBUF_NOCHECK NULL,1
>>>>>
>>>>> I need to have the best opinion. Jifl?
>>>>
>>>> To save time on testing.
>>>>
>>>> I forgot to mention that was not only 'a backtrace/code view in mind'
>>>> from my side. Yesterday, I did a gDebuging a real termios example on
>>>> synthetic target. So, I confirm the Jay's report and confirm that his
>>>> workaround fixes the issue.
>>>
>>> That sounds good, and I think a comment would be valuable. A #define is
>>> probably unnecessary. Thanks!
>>
>> Hi Jay,
>>
>> Could you, please, submit new patch with an included comment (just a few
>> sentences)? I confess that I could not believe that tcflush() does not
>> work  :-) and only GDB passed me through out that 'll' and the check.
>> So, I think a using those "ones" won't stick in the mind without your
>> short notice. Thanks!
>>
>> Sergei
>>
>