From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29892 invoked by alias); 21 Apr 2014 14:53:05 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 29862 invoked by uid 89); 21 Apr 2014 14:53:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 21 Apr 2014 14:53:04 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 3CE50116133; Mon, 21 Apr 2014 10:53:02 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id fMh7I0Q3Tk8C; Mon, 21 Apr 2014 10:53:02 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id F25E5116111; Mon, 21 Apr 2014 10:53:01 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 54B6CE087E; Mon, 21 Apr 2014 07:53:01 -0700 (PDT) Date: Mon, 21 Apr 2014 14:53:00 -0000 From: Joel Brobecker To: Kai Tietz Cc: Ray Donnelly , gcc-patches@gcc.gnu.org, ktietz70@gmail.com, "binutils@sourceware.org Development" , gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] Windows libibery: Don't quote args unnecessarily Message-ID: <20140421145301.GA4477@adacore.com> References: <1397936424-2290-1-git-send-email-mingw.android@gmail.com> <1397936424-2290-2-git-send-email-mingw.android@gmail.com> <1826621422.9537772.1397939013891.JavaMail.zimbra@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1826621422.9537772.1397939013891.JavaMail.zimbra@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-04/txt/msg00402.txt.bz2 > Changelog libiberty/ > * pex-win32.c (argv_to_cmdline): Don't quote > args unnecessarily Some minor comments... > > diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c > > index eae72c5..775b53c 100644 > > --- a/libiberty/pex-win32.c > > +++ b/libiberty/pex-win32.c > > @@ -340,17 +340,26 @@ argv_to_cmdline (char *const *argv) > > char *p; > > size_t cmdline_len; > > int i, j, k; > > + int needs_quotes; > > > > cmdline_len = 0; > > for (i = 0; argv[i]; i++) > > { > > - /* We quote every last argument. This simplifies the problem; > > - we need only escape embedded double-quotes and immediately > > + /* We only quote arguments that contain spaces, \n \t \v or " > > characters > > + to prevent wasting 2 chars per argument of the CreateProcess 32k char > > + limit We need only escape embedded double-quotes and immediately Period missing after "limit". JIC, remember that we ask that periods be followed by 2 spaces. > > preceeding backslash characters. A sequence of backslach characters > > that is not follwed by a double quote character will not be > > escaped. */ > > + needs_quotes = 0; > > for (j = 0; argv[i][j]; j++) > > { > > + if (argv[i][j] == ' ' || argv[i][j] == '\n' || > > + argv[i][j] == '\t' || argv[i][j] == '"' ) GNU Coding Style requirement: Binary operators should be at the start of the next line, not at the end. For instance: if (argv[i][j] == ' ' || argv[i][j] == '\n' || argv[i][j] == '\t' || argv[i][j] == '"') Also, watch out for the extra space before ')' - please remove it. > > + { > Here seems to be an intend issue. > > + needs_quotes = 1; > > + } > > + > > if (argv[i][j] == '"') > > { > > /* Escape preceeding backslashes. */ > > @@ -362,16 +371,34 @@ argv_to_cmdline (char *const *argv) > > } > > /* Trailing backslashes also need to be escaped because they will be > > followed by the terminating quote. */ > > - for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) > > - cmdline_len++; > > + if (needs_quotes) > > + { > > + for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) > > + cmdline_len++; > > + } > > cmdline_len += j; > > - cmdline_len += 3; /* for leading and trailing quotes and space */ > > + /* for leading and trailing quotes and space */ > > + cmdline_len += needs_quotes * 2 + 1; > > } > > cmdline = XNEWVEC (char, cmdline_len); > > p = cmdline; > > for (i = 0; argv[i]; i++) > > { > > - *p++ = '"'; > > + needs_quotes = 0; > > + for (j = 0; argv[i][j]; j++) > > + { > > + if (argv[i][j] == ' ' || argv[i][j] == '\n' || > > + argv[i][j] == '\t' || argv[i][j] == '"' ) Same as above. > > + { > > + needs_quotes = 1; > > + break; > > + } > > + } > > + > > + if (needs_quotes) > > + { > > + *p++ = '"'; > > + } > > for (j = 0; argv[i][j]; j++) > > { > > if (argv[i][j] == '"') > > @@ -382,9 +409,12 @@ argv_to_cmdline (char *const *argv) > > } > > *p++ = argv[i][j]; > > } > > - for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) > > - *p++ = '\\'; > > - *p++ = '"'; > > + if (needs_quotes) > > + { > > + for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) > > + *p++ = '\\'; > > + *p++ = '"'; > > + } > > *p++ = ' '; > > } > > p[-1] = '\0'; > > -- > > 1.9.2 > > Patch itself makes sense. Let see if there are additional comments. > > Regards, > Kai -- Joel