| Message ID | CAOj9pe9d_4m15q2Gb7DCSXD8FJH+QzRDNx8wRV89EV90dfpDpA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 09/22/2016 11:26 PM, Tadek Kijkowski wrote: > The fixincl executable uses system function to call applyfix or to > direcly patch a header file, with parameters enclosed in single > quotes. This problem is that MinGW system function just calls cmd.exe, > which doesn't strip quotes from parameters and completely ignores > quotes for embedding spaces in parameters. The MinGW system function > also doesn't allow for newlines in executed command parameters. As a > result fixincludes doesn't wotk at on when trying to build a cross > compiler with mingw as host. > > On MinGW host, this patch adds system_with_shell function, which > transforms command passed to system function is following way: > - Enclose entire command in double quotes > - Prepend shell executable name, taken from environment variable SHELL > - Replace each occurence of newline with '$'\n'' sequence which is > understood by bash and ksh (it is assumed that all newlines are > embedded in command parameters enclosed in single quotes) > > 2016-09-23 Tadek Kijkowski (tkijkowski@gmail.com) > > * fixincl.c: Added system_with_shell for MinGW host. > > > Index: fixincludes/fixincl.c > =================================================================== > --- fixincludes/fixincl.c (revision 240386) > +++ fixincludes/fixincl.c (working copy) > @@ -170,7 +170,111 @@ > exit (EXIT_SUCCESS); > } > > +#ifdef __MINGW32__ > > +/* Count number of \c needle character instances in string */ > +static int > +count_chars ( char* str, char needle ) Formatting it. This should be: count_chars (char* str, char needle) Note the lack of horizontal whitespace after the open paren or before the close paren. Similarly for system_with_shell declaration below. Wouldn't this be better named "count_occurrences_of_char" or "count_instances_of_char"? > + > + /* Allocate enough memory to fit newly created command string */ > + cmd_size = strlen (env_shell) + (sizeof (z_shell_start_args) - 1) > + + strlen (s) + newline_cnt * (sizeof(z_shell_newline) - 1 - 1) > + + (sizeof (z_shell_end_args) - 1) + 1; Why use sizeof (foo) - 1 rather than just strlen (foo) here? Note that GCC can compute the string length as a compile time constant, so you're not gaining any performance by using sizeof here and strlen seems clearer. > + > + long_cmd = XNEWVEC (char, cmd_size); > + > + /* Start with ${SHELL} -c " */ > + strcpy (long_cmd, env_shell); > + strcat (long_cmd, z_shell_start_args); > + > + /* End pointer for appending pieces of command while replacing newlines */ > + cmd_endp = long_cmd + strlen(long_cmd); Formatting nit. Space between function name and its argument list. > + nl_scan = s; > + while (nl_scan != NULL) > + { > + char* next_nl = strchr (nl_scan, '\n'); Formatting nit. char *next_nl. > + if (next_nl) > + { > + /* Append part of command between newlines */ > + size_t part_len = next_nl - nl_scan; > + memcpy(cmd_endp, nl_scan, part_len); Formatting nit, space between function name and its argument list. > + cmd_endp += part_len; > + > + /* Append newline replacement */ > + memcpy(cmd_endp, z_shell_newline, sizeof(z_shell_newline)); Smilarly, space between functio nname and its argument list. > + cmd_endp += sizeof(z_shell_newline) - 1; Here too. > + > + free ( (void*) long_cmd); free (long_cmd); Can you fix those various (minor) issue and resubmit. I think it'll be OK for the trunk with those changes. jeff
I usually try to catch emails with "fixincludes" in the title. Can I please get a copy of the original patch? Thanks. On 09/29/16 11:44, Jeff Law wrote: > On 09/22/2016 11:26 PM, Tadek Kijkowski wrote: >> The fixincl executable uses system function to call applyfix or to >> direcly patch a header file, with parameters enclosed in single >> quotes. This problem is that MinGW system function just calls cmd.exe, >> which doesn't strip quotes from parameters and completely ignores >> quotes for embedding spaces in parameters. The MinGW system function >> also doesn't allow for newlines in executed command parameters. As a >> result fixincludes doesn't wotk at on when trying to build a cross >> compiler with mingw as host. >> >> On MinGW host, this patch adds system_with_shell function, which >> transforms command passed to system function is following way: >> - Enclose entire command in double quotes >> - Prepend shell executable name, taken from environment variable SHELL >> - Replace each occurence of newline with '$'\n'' sequence which is >> understood by bash and ksh (it is assumed that all newlines are >> embedded in command parameters enclosed in single quotes) >> >> 2016-09-23 Tadek Kijkowski (tkijkowski@gmail.com) >> >> * fixincl.c: Added system_with_shell for MinGW host. >> >> >> Index: fixincludes/fixincl.c >> =================================================================== >> --- fixincludes/fixincl.c (revision 240386) >> +++ fixincludes/fixincl.c (working copy) >> @@ -170,7 +170,111 @@ >> exit (EXIT_SUCCESS); >> } >> >> +#ifdef __MINGW32__ >> >> +/* Count number of \c needle character instances in string */ >> +static int >> +count_chars ( char* str, char needle ) > Formatting it. This should be: > > count_chars (char* str, char needle) > > Note the lack of horizontal whitespace after the open paren or before > the close paren. Similarly for system_with_shell declaration below. > > Wouldn't this be better named "count_occurrences_of_char" or > "count_instances_of_char"? > > > > >> + >> + /* Allocate enough memory to fit newly created command string */ >> + cmd_size = strlen (env_shell) + (sizeof (z_shell_start_args) - 1) >> + + strlen (s) + newline_cnt * (sizeof(z_shell_newline) - 1 >> - 1) >> + + (sizeof (z_shell_end_args) - 1) + 1; > Why use sizeof (foo) - 1 rather than just strlen (foo) here? Note that > GCC can compute the string length as a compile time constant, so you're > not gaining any performance by using sizeof here and strlen seems clearer. > > > >> + >> + long_cmd = XNEWVEC (char, cmd_size); >> + >> + /* Start with ${SHELL} -c " */ >> + strcpy (long_cmd, env_shell); >> + strcat (long_cmd, z_shell_start_args); >> + >> + /* End pointer for appending pieces of command while replacing >> newlines */ >> + cmd_endp = long_cmd + strlen(long_cmd); > Formatting nit. Space between function name and its argument list. > > >> + nl_scan = s; >> + while (nl_scan != NULL) >> + { >> + char* next_nl = strchr (nl_scan, '\n'); > Formatting nit. char *next_nl. > > >> + if (next_nl) >> + { >> + /* Append part of command between newlines */ >> + size_t part_len = next_nl - nl_scan; >> + memcpy(cmd_endp, nl_scan, part_len); > Formatting nit, space between function name and its argument list. > >> + cmd_endp += part_len; >> + >> + /* Append newline replacement */ >> + memcpy(cmd_endp, z_shell_newline, sizeof(z_shell_newline)); > Smilarly, space between functio nname and its argument list. > >> + cmd_endp += sizeof(z_shell_newline) - 1; > Here too. > >> + >> + free ( (void*) long_cmd); > free (long_cmd); > > Can you fix those various (minor) issue and resubmit. I think it'll be > OK for the trunk with those changes. > > jeff > >
OK, I found it. Looks like my MUA is getting too aggressive with its filtering. What Jeff said, plus I would prefer the tail end looking like: + +#else + +#define system_with_shell system // normal call + +#endif /* defined(__MINGW32__) */ and modifying the call to use "system_with_shell". The point being that obvious clues are better than subtle switcheroos. On Thu, Sep 29, 2016 at 11:44 AM, Jeff Law <law@redhat.com> wrote:
Hold on. I have much improved version almost ready. It passes all the tests now. 2016-09-30 0:15 GMT+02:00 Bruce Korb <bruce.korb@gmail.com>: > I usually try to catch emails with "fixincludes" in the title. > Can I please get a copy of the original patch? Thanks. > > > On 09/29/16 11:44, Jeff Law wrote: >> >> On 09/22/2016 11:26 PM, Tadek Kijkowski wrote: >>> >>> The fixincl executable uses system function to call applyfix or to >>> direcly patch a header file, with parameters enclosed in single >>> quotes. This problem is that MinGW system function just calls cmd.exe, >>> which doesn't strip quotes from parameters and completely ignores >>> quotes for embedding spaces in parameters. The MinGW system function >>> also doesn't allow for newlines in executed command parameters. As a >>> result fixincludes doesn't wotk at on when trying to build a cross >>> compiler with mingw as host. >>> >>> On MinGW host, this patch adds system_with_shell function, which >>> transforms command passed to system function is following way: >>> - Enclose entire command in double quotes >>> - Prepend shell executable name, taken from environment variable SHELL >>> - Replace each occurence of newline with '$'\n'' sequence which is >>> understood by bash and ksh (it is assumed that all newlines are >>> embedded in command parameters enclosed in single quotes) >>> >>> 2016-09-23 Tadek Kijkowski (tkijkowski@gmail.com) >>> >>> * fixincl.c: Added system_with_shell for MinGW host. >>> >>> >>> Index: fixincludes/fixincl.c >>> =================================================================== >>> --- fixincludes/fixincl.c (revision 240386) >>> +++ fixincludes/fixincl.c (working copy) >>> @@ -170,7 +170,111 @@ >>> exit (EXIT_SUCCESS); >>> } >>> >>> +#ifdef __MINGW32__ >>> >>> +/* Count number of \c needle character instances in string */ >>> +static int >>> +count_chars ( char* str, char needle ) >> >> Formatting it. This should be: >> >> count_chars (char* str, char needle) >> >> Note the lack of horizontal whitespace after the open paren or before >> the close paren. Similarly for system_with_shell declaration below. >> >> Wouldn't this be better named "count_occurrences_of_char" or >> "count_instances_of_char"? >> >> >> >> >>> + >>> + /* Allocate enough memory to fit newly created command string */ >>> + cmd_size = strlen (env_shell) + (sizeof (z_shell_start_args) - 1) >>> + + strlen (s) + newline_cnt * (sizeof(z_shell_newline) - 1 >>> - 1) >>> + + (sizeof (z_shell_end_args) - 1) + 1; >> >> Why use sizeof (foo) - 1 rather than just strlen (foo) here? Note that >> GCC can compute the string length as a compile time constant, so you're >> not gaining any performance by using sizeof here and strlen seems clearer. >> >> >> >>> + >>> + long_cmd = XNEWVEC (char, cmd_size); >>> + >>> + /* Start with ${SHELL} -c " */ >>> + strcpy (long_cmd, env_shell); >>> + strcat (long_cmd, z_shell_start_args); >>> + >>> + /* End pointer for appending pieces of command while replacing >>> newlines */ >>> + cmd_endp = long_cmd + strlen(long_cmd); >> >> Formatting nit. Space between function name and its argument list. >> >> >>> + nl_scan = s; >>> + while (nl_scan != NULL) >>> + { >>> + char* next_nl = strchr (nl_scan, '\n'); >> >> Formatting nit. char *next_nl. >> >> >>> + if (next_nl) >>> + { >>> + /* Append part of command between newlines */ >>> + size_t part_len = next_nl - nl_scan; >>> + memcpy(cmd_endp, nl_scan, part_len); >> >> Formatting nit, space between function name and its argument list. >> >>> + cmd_endp += part_len; >>> + >>> + /* Append newline replacement */ >>> + memcpy(cmd_endp, z_shell_newline, sizeof(z_shell_newline)); >> >> Smilarly, space between functio nname and its argument list. >> >>> + cmd_endp += sizeof(z_shell_newline) - 1; >> >> Here too. >> >>> + >>> + free ( (void*) long_cmd); >> >> free (long_cmd); >> >> Can you fix those various (minor) issue and resubmit. I think it'll be >> OK for the trunk with those changes. >> >> jeff >> >> >
Index: fixincludes/fixincl.c =================================================================== --- fixincludes/fixincl.c (revision 240386) +++ fixincludes/fixincl.c (working copy) @@ -170,7 +170,111 @@ exit (EXIT_SUCCESS); } +#ifdef __MINGW32__ +/* Count number of \c needle character instances in string */ +static int +count_chars ( char* str, char needle ) +{ + int instances = 0; + + while (str) + { + str = strchr (str, needle); + if (str) + { + ++str; + ++instances; + } + } + + return instances; +} + +/* On Mingw32 system(3) will just start cmd by default. + Try using unix style shell passed via SHELL env. variable. + */ + +/* Call system(3) function, but prepend ${SHELL} -c to the command, + replace newlines with '$'\n'' and enclose command with double quotes. + */ +static int +system_with_shell ( char* s ) +{ + static const char z_shell_start_args[] = " -c \""; + static const char z_shell_end_args[] = "\""; + static const char z_shell_newline[] = "'$'\\n''"; + + char* env_shell; + char* long_cmd; + char* cmd_endp; + size_t cmd_size; + int sys_result; + int newline_cnt; + char* nl_scan; + + /* If SHELL variable is not defined just call standard shell function */ + env_shell = getenv ("SHELL"); + if (env_shell == NULL) + return system (s); + + /* Count number of newlines in command */ + newline_cnt = count_chars(s, '\n'); + + /* Allocate enough memory to fit newly created command string */ + cmd_size = strlen (env_shell) + (sizeof (z_shell_start_args) - 1) + + strlen (s) + newline_cnt * (sizeof(z_shell_newline) - 1 - 1) + + (sizeof (z_shell_end_args) - 1) + 1; + + long_cmd = XNEWVEC (char, cmd_size); + + /* Start with ${SHELL} -c " */ + strcpy (long_cmd, env_shell); + strcat (long_cmd, z_shell_start_args); + + /* End pointer for appending pieces of command while replacing newlines */ + cmd_endp = long_cmd + strlen(long_cmd); + nl_scan = s; + while (nl_scan != NULL) + { + char* next_nl = strchr (nl_scan, '\n'); + if (next_nl) + { + /* Append part of command between newlines */ + size_t part_len = next_nl - nl_scan; + memcpy(cmd_endp, nl_scan, part_len); + cmd_endp += part_len; + + /* Append newline replacement */ + memcpy(cmd_endp, z_shell_newline, sizeof(z_shell_newline)); + cmd_endp += sizeof(z_shell_newline) - 1; + + /* Skip newline in src */ + ++next_nl; + } + else + { + /* Last portion */ + strcpy (cmd_endp, nl_scan); + } + nl_scan = next_nl; + } + + /* Closing quote */ + strcat (long_cmd, z_shell_end_args); + + sys_result = system (long_cmd); + + free ( (void*) long_cmd); + + return sys_result; +} + +#define system system_with_shell + +#endif /* defined(__MINGW32__) */ + + static void do_version (void) {
The fixincl executable uses system function to call applyfix or to direcly patch a header file, with parameters enclosed in single quotes. This problem is that MinGW system function just calls cmd.exe, which doesn't strip quotes from parameters and completely ignores quotes for embedding spaces in parameters. The MinGW system function also doesn't allow for newlines in executed command parameters. As a result fixincludes doesn't wotk at on when trying to build a cross compiler with mingw as host. On MinGW host, this patch adds system_with_shell function, which transforms command passed to system function is following way: - Enclose entire command in double quotes - Prepend shell executable name, taken from environment variable SHELL - Replace each occurence of newline with '$'\n'' sequence which is understood by bash and ksh (it is assumed that all newlines are embedded in command parameters enclosed in single quotes) 2016-09-23 Tadek Kijkowski (tkijkowski@gmail.com) * fixincl.c: Added system_with_shell for MinGW host.