[PATCH 7/9] Patching STTY command for use in RTEMS
Christian Mauderer
christian.mauderer at embedded-brains.de
Mon Feb 13 09:00:51 UTC 2017
Am 10.02.2017 um 15:06 schrieb Kirspel, Kevin:
> I can include the libbsd.txt updates in my updated patches.
>
> If you want getopt_r() to match FREEBSD, it needs to be updated so that it works correctly when optind is initialized to 1 or 0.
I think it might be worth to raise an issue at the newlib but it is
quite possible that patching this would break a number of other existing
applications (not only RTEMS but others that use newlib) so I'm not sure
whether the newlib developers are really found of this idea.
Kind regards
Christian Mauderer
>
> Kevin Kirspel
> Electrical Engineer - Sr. Staff
> Idexx Roswell
> 235 Hembree Park Drive
> Roswell GA 30076
> Tel: (770)-510-4444 ext. 81642
> Direct: (770)-688-1642
> Fax: (770)-510-4445
>
> -----Original Message-----
> From: Christian Mauderer [mailto:christian.mauderer at embedded-brains.de]
> Sent: Friday, February 10, 2017 6:04 AM
> To: Kirspel, Kevin <Kevin-Kirspel at idexx.com>
> Cc: RTEMS Devel <devel at rtems.org>
> Subject: Re: [PATCH 7/9] Patching STTY command for use in RTEMS
>
> Hello Kevin,
>
> Am 09.02.2017 um 22:51 schrieb Kirspel, Kevin:
>> Before or after the "Build the libbsd without optimization." step, I would add a statement about fixing up any compilation errors. Maybe most commands will compile after making the changes listed previously but I had to fixup errors related to termios differences between RTEMS and FREEBSD. Besides that, the instructions were fine and I got the suspected output.
>
> OK. So in general it worked. That is good to know. Do you want to add this information to the documentation or should I create a patch?
>
>>
>> I didn't realize the files were being changed to executables. I edit the files from a windows application running over a Samba share. I'll check and see if samba is making the change on save.
>
> I assumed such a problem. Also it's just a detail.
>
>>
>> I will fix the tabs. Sometimes I forget to change my editor settings for the different files I am working on.
>
> Thanks.
>
>>
>> There is an issue with the getopt_r() function. Even if you initialize the optind value to 1 the getopt_r() function returns '?'. So you get past the strspn() check but fail the getopt_r() call. If you initialize the optind value to 0, you get the right return value but the optind is wrong for the strspn() call. If you change the original code to the following, then it will also work:
>>
>> if (strspn(argv[optind == 0 ? 1 : optind], "-aefg") ==
>> strlen(argv[optind == 0 ? 1 : optind])
>
> OK. So if I understand you correctly, only the first value is really wrong but the optind will have the correct value for the further processing farther below. That was my main concern.
>
>>
>> Kevin Kirspel
>> Electrical Engineer - Sr. Staff
>> Idexx Roswell
>> 235 Hembree Park Drive
>> Roswell GA 30076
>> Tel: (770)-510-4444 ext. 81642
>> Direct: (770)-688-1642
>> Fax: (770)-510-4445
>>
>> -----Original Message-----
>> From: Christian Mauderer
>> [mailto:christian.mauderer at embedded-brains.de]
>> Sent: Thursday, February 09, 2017 3:36 PM
>> To: Kirspel, Kevin <Kevin-Kirspel at idexx.com>
>> Cc: RTEMS Devel <devel at rtems.org>
>> Subject: Re: [PATCH 7/9] Patching STTY command for use in RTEMS
>>
>> Hello Kevin,
>>
>> thanks for your work.
>>
>> Out of curiosity: It seems that you are one of the first users of the new porting process for user space applications (beneath Sebastian and me). Did you encounter any problems that might be worth adding to the guide in libbsd.txt?
>>
>> Beneath that please see my remarks below.
>>
>> Kind regards
>>
>> Christian
>>
>> ----- Ursprüngliche Mail -----
>>> Von: "Kevin Kirspel" <kevin-kirspel at idexx.com>
>>> An: "RTEMS Devel" <devel at rtems.org>
>>> Gesendet: Donnerstag, 9. Februar 2017 04:21:38
>>> Betreff: [PATCH 7/9] Patching STTY command for use in RTEMS
>>
>> [...]
>>> diff --git a/freebsd/bin/stty/stty.c b/freebsd/bin/stty/stty.c old
>>> mode 100644 new mode 100755
>>
>> You made the files executable. It would be better, if you could avoid that. Please note that this is also true for a lot of other sources in your patches.
>>
>>> index 54c63f6..3999a7f
>>> --- a/freebsd/bin/stty/stty.c
>>> +++ b/freebsd/bin/stty/stty.c
>>> @@ -1,5 +1,8 @@
>>> #include <machine/rtems-bsd-user-space.h>
>>>
>>> +#ifdef __rtems__
>>> +#include "rtems-bsd-stty-namespace.h"
>>> +#endif /* __rtems__ */
>>> /*-
>>> * Copyright (c) 1989, 1991, 1993, 1994
>>> * The Regents of the University of California. All rights reserved.
>>> @@ -43,6 +46,12 @@ static char sccsid[] = "@(#)stty.c 8.3 (Berkeley) 4/2/94";
>>> #include <sys/cdefs.h>
>>> __FBSDID("$FreeBSD$");
>>>
>>> +#ifdef __rtems__
>>> +#define __need_getopt_newlib
>>> +#include <getopt.h>
>>> +#include <machine/rtems-bsd-program.h> #include
>>> +<machine/rtems-bsd-commands.h> #endif /* __rtems__ */
>>> #include <sys/types.h>
>>>
>>> #include <ctype.h>
>>> @@ -57,20 +66,56 @@ __FBSDID("$FreeBSD$");
>>>
>>> #include "stty.h"
>>> #include "extern.h"
>>> +#ifdef __rtems__
>>> +#include "rtems-bsd-stty-stty-data.h"
>>> +#endif /* __rtems__ */
>>> +
>>> +#ifdef __rtems__
>>> +static int main(int argc, char *argv[]);
>>> +
>>> +RTEMS_LINKER_RWSET(bsd_prog_stty, char);
>>>
>>> int
>>> +rtems_bsd_command_stty(int argc, char *argv[]) {
>>> + int exit_code;
>>> + void *data_begin;
>>> + size_t data_size;
>>> +
>>> + data_begin = RTEMS_LINKER_SET_BEGIN(bsd_prog_stty);
>>> + data_size = RTEMS_LINKER_SET_SIZE(bsd_prog_stty);
>>> +
>>> + rtems_bsd_program_lock();
>>> + exit_code = rtems_bsd_program_call_main_with_data_restore("stty",
>>> + main, argc, argv, data_begin, data_size);
>>> + rtems_bsd_program_unlock();
>>> +
>>> + return exit_code;
>>> +}
>>
>> In most (all?) places the libbsd uses BSD coding style. This is especially true for code that is inserted into existing BSD code. In this case that means one tab instead of two spaces. I also noted that on some of the other patches in the patch set.
>>
>>> +#endif /* __rtems__ */
>>> +int
>>> main(int argc, char *argv[])
>>> {
>>> struct info i;
>>> enum FMT fmt;
>>> int ch;
>>> const char *file, *errstr = NULL;
>>> +#ifdef __rtems__
>>> + struct getopt_data getopt_data;
>>> + memset(&getopt_data, 0, sizeof(getopt_data)); #define optind
>>> +getopt_data.optind #define optarg getopt_data.optarg #define opterr
>>> +getopt_data.opterr #define optopt getopt_data.optopt #define
>>> +getopt(argc, argv, opt) getopt_r(argc, argv, "+" opt, &getopt_data)
>>> +#endif /* __rtems__ */
>>>
>>> fmt = NOTSET;
>>> i.fd = STDIN_FILENO;
>>> file = "stdin";
>>>
>>> opterr = 0;
>>> +#ifndef __rtems__
>>> while (optind < argc &&
>>> strspn(argv[optind], "-aefg") == strlen(argv[optind]) &&
>>> (ch = getopt(argc, argv, "aef:g")) != -1) @@ -93,6 +138,34 @@
>>> main(int argc, char *argv[])
>>> default:
>>> goto args;
>>> }
>>> +#else /* __rtems__ */
>>> + while (optind < argc && (ch = getopt(argc, argv, "aef:g")) != -1) {
>>> + int optidx = optind - ((optarg == 0) ? 1 : 2);
>>> + if(strspn(argv[optidx], "-aefg") == strlen(argv[optidx])) {
>>
>> I'm really not sure about that one. Eventually you could help me understand it. Why was the change necessary?
>>
>> I assume that it has to do something with the problem that optind should normally be initialized with 1 (according to https://urldefense.proofpoint.com/v2/url?u=http-3A__pubs.opengroup.org_onlinepubs_9699919799_functions_getopt.html&d=DwIFaQ&c=2do6VJGs3LvEOe4OFFM1bA&r=HDiJ93ANMEQ32G5JGdpyUxbdebuwKHBbeiHMr3RbR74&m=GP0OdJY1ZudYsz_Sa37k-2CMMp1GBHHp5R-7pDdgqzI&s=7caspKinrmx0H4JqdzPYRp3--HEIMqctvGpTQ0XmDGo&e= ) but in our case it is 0 due to the memset. Is that correct? Would the value for the further loop cycles be correct or is there a general problem that the newlib optind is one off?
>>
>> A few lines further down the original code has the following two lines:
>>
>> args: argc -= optind;
>> argv += optind;
>>
>> If you had a problem with optind in the query: Has it the correct value here?
>>
>>> + switch(ch) {
>>> + case 'a': /* undocumented: POSIX compatibility */
>>> + fmt = POSIX;
>>> + break;
>>> + case 'e':
>>> + fmt = BSD;
>>> + break;
>>> + case 'f':
>>> + if ((i.fd = open(optarg, O_RDONLY | O_NONBLOCK)) < 0)
>>> + err(1, "%s", optarg);
>>> + file = optarg;
>>> + break;
>>> + case 'g':
>>> + fmt = GFLAG;
>>> + break;
>>> + case '?':
>>> + default:
>>> + goto args;
>>> + }
>>> + } else {
>>> + break;
>>> + }
>>> + }
>>> +#endif /* __rtems__ */
>>>
>>> args: argc -= optind;
>>> argv += optind;
>>> @@ -136,8 +209,13 @@ args: argc -= optind;
>>> speed = strtonum(*argv, 0, UINT_MAX, &errstr);
>>> if (errstr)
>>> err(1, "speed");
>>> +#ifndef __rtems__
>>> cfsetospeed(&i.t, speed);
>>> cfsetispeed(&i.t, speed);
>>> +#else /* __rtems__ */
>>> + cfsetospeed(&i.t, rtems_bsd_bsd_speed_to_rtems_speed(speed));
>>> + cfsetispeed(&i.t, rtems_bsd_bsd_speed_to_rtems_speed(speed));
>>> +#endif /* __rtems__ */
>>> i.set = 1;
>>> continue;
>>> }
>> [...]
>>
>> --
>> --------------------------------------------
>> embedded brains GmbH
>> Christian Mauderer
>> Dornierstr. 4
>> D-82178 Puchheim
>> Germany
>> email: christian.mauderer at embedded-brains.de
>> Phone: +49-89-18 94 741 - 18
>> Fax: +49-89-18 94 741 - 08
>> PGP: Public key available on request.
>>
>> Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
>>
>
> --
> --------------------------------------------
> embedded brains GmbH
> Christian Mauderer
> Dornierstr. 4
> D-82178 Puchheim
> Germany
> email: christian.mauderer at embedded-brains.de
> Phone: +49-89-18 94 741 - 18
> Fax: +49-89-18 94 741 - 08
> PGP: Public key available on request.
>
> Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
>
--
--------------------------------------------
embedded brains GmbH
Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian.mauderer at embedded-brains.de
Phone: +49-89-18 94 741 - 18
Fax: +49-89-18 94 741 - 08
PGP: Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
More information about the devel
mailing list