From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1767 invoked by alias); 15 Jun 2013 18:37:33 -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 1742 invoked by uid 89); 15 Jun 2013 18:37:28 -0000 X-Spam-SWARE-Status: No, score=-6.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Sat, 15 Jun 2013 18:37:27 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r5FIbPQB018382 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sat, 15 Jun 2013 14:37:25 -0400 Received: from psique (ovpn-113-133.phx2.redhat.com [10.3.113.133]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r5FIbMIL019081 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Sat, 15 Jun 2013 14:37:23 -0400 From: Sergio Durigan Junior To: Wei-cheng Wang Cc: gdb-patches@sourceware.org Subject: Re: [patch] Parse quoted string for restore/dump commands. References: <51B20120.8070909@gmail.com> X-URL: http://www.redhat.com Date: Sat, 15 Jun 2013 18:37:00 -0000 In-Reply-To: <51B20120.8070909@gmail.com> (Wei-cheng Wang's message of "Fri, 07 Jun 2013 23:49:52 +0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2013-06/txt/msg00345.txt.bz2 On Friday, June 07 2013, Wei-cheng Wang wrote: > This is a patch for parsing quoted string for restore/dump commands. > Test cases are also added. Thanks for the patch, Wei-cheng. I don't see your name on gdb/MAINTAINERS, do you have copyright assignment on file? You will need one in order to commit the changes. If you don't have it, please contact me off-list and I can send you the forms. > diff --git a/gdb/cli/cli-dump.c b/gdb/cli/cli-dump.c > index 208916c..1fc9f3f 100644 > --- a/gdb/cli/cli-dump.c > +++ b/gdb/cli/cli-dump.c > @@ -80,14 +80,54 @@ scan_filename_with_cleanup (char **cmd, const char *defname) > } > else > { > - /* FIXME: should parse a possibly quoted string. */ > - char *end; > + char *inp, *outp; > + char quote = '\0'; > + int escape = 0; > + int capacity; size_t capacity; > + > + inp = (*cmd) = skip_spaces (*cmd); I don't really like such multi-variables attributions, I think the code reads better if you write: (*cmd) = skip_spaces (*cmd); inp = (*cmd); I also understand that you used the same style as the old code, however. > + /* Allocate initial buffer for filename. > + If there is no space in quoted string, > + this should be the exactly size what we need. */ You could write this comment using larger lines. The soft limit is 72 chars, and the hard limit is 80 chars, so you could write: /* Allocate initial buffer for filename. If there is no space in quoted string, this should be the exactly size what we need. */ > + capacity = strcspn (*cmd, " \t") + 1; > + outp = filename = (char *) xmalloc (capacity); Same comment about the attribution as above. Also, you don't need to cast xmalloc's return. > + > + /* Quoting character. */ > + if (*inp == '\'' || *inp == '"') > + quote = *inp++; > + > + for (; *inp != '\0'; inp++) > + { > + /* Stop by whitespace, if not quoted or escaped. */ > + if (whitespace (*inp) && !escape && !quote) Hm, "whitespace" is readline-specific. Please use "isspace" instead. Also, since you are explicitly checking every char variable, I'd also do it with "quote", i.e., quote != '\0'. > + break; > + > + if (escape) > + { > + escape = 0; > + *outp++ = *inp; > + } > + else if (*inp == '\\') > + escape = 1; > + else if (*inp == quote) > + quote = 0; > + else > + *outp++ = *inp; > + > + /* Expand when needed. */ > + if (outp - filename >= capacity) > + { > + int len = outp - filename; size_t len; > > - (*cmd) = skip_spaces (*cmd); > - end = *cmd + strcspn (*cmd, " \t"); > - filename = savestring ((*cmd), end - (*cmd)); > + capacity *= 2; > + filename = (char *) xrealloc (filename, capacity); > + outp = filename + len; > + } > + } > + > + *outp = '\0'; Just for completeness, you could xrealloc (filename, outp - filename + 1) to save space. > make_cleanup (xfree, filename); > - (*cmd) = skip_spaces (end); > + (*cmd) = skip_spaces (inp); > } > gdb_assert (filename != NULL); > > diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog > index 698f116..e28d75e 100644 > --- a/gdb/testsuite/ChangeLog > +++ b/gdb/testsuite/ChangeLog > @@ -1,3 +1,7 @@ > +2013-06-07 Wei-cheng Wang > + > + * gdb.base/dump.exp: Test tests for filenames with spaces. > + > 2013-06-06 Doug Evans > > * gdb.cp/derivation.exp: Make tests have unique names. > diff --git a/gdb/testsuite/gdb.base/dump.exp b/gdb/testsuite/gdb.base/dump.exp > index b2d0b3a..ab0481c 100644 > --- a/gdb/testsuite/gdb.base/dump.exp > +++ b/gdb/testsuite/gdb.base/dump.exp > @@ -105,6 +105,18 @@ make_dump_file "dump bin val intarr1b.bin intarray" \ > make_dump_file "dump bin val intstr1b.bin intstruct" \ > "dump struct as value, binary" > > +make_dump_file "dump bin val \"intarr1c .bin\" intarray" \ > + "dump array as value, binary, quoted whitespace" > + > +make_dump_file "dump bin val \"intstr1c .bin\" intstruct" \ > + "dump struct as value, binary, quoted whitespace" > + > +make_dump_file "dump bin val intarr1d\\ .bin intarray" \ > + "dump array as value, binary, escaped whitespace" > + > +make_dump_file "dump bin val intstr1d\\ .bin intstruct" \ > + "dump struct as value, binary, escaped whitespace" > + > make_dump_file "dump srec val intarr1.srec intarray" \ > "dump array as value, srec" > > @@ -300,6 +312,26 @@ test_restore_saved_value "intstr1.bin binary $struct_start" \ > > gdb_test "print zero_all ()" ".*" > > +test_restore_saved_value "intarr1c\\ .bin binary $array_start" \ > + "array as value, binary, quoted whitespace" \ > + $array_val "intarray" > + > +test_restore_saved_value "intstr1c\\ .bin binary $struct_start" \ > + "struct as value, binary, quoted whitespace" \ > + $struct_val "intstruct" > + > +gdb_test "print zero_all ()" ".*" > + > +test_restore_saved_value "\"intarr1d .bin\" binary $array_start" \ > + "array as value, binary, escape whitespace" \ > + $array_val "intarray" > + > +test_restore_saved_value "\"intstr1d .bin\" binary $struct_start" \ > + "struct as value, binary, escape whitespace" \ > + $struct_val "intstruct" > + > +gdb_test "print zero_all ()" ".*" > + > test_restore_saved_value "intarr2.bin binary $array_start" \ > "array as memory, binary" \ > $array_val "intarray" > @@ -505,4 +537,4 @@ if ![string compare $is64bitonly "no"] then { > > # clean up files > > -remote_exec build "rm -f intarr1.bin intarr1b.bin intarr1.ihex > intarr1.srec intarr1.tekhex intarr2.bin intarr2b.bin intarr2.ihex > intarr2.srec intarr2.tekhex intstr1.bin intstr1b.bin intstr1.ihex > intstr1.srec intstr1.tekhex intstr2.bin intstr2b.bin intstr2.ihex > intstr2.srec intstr2.tekhex intarr3.srec" > +remote_exec build "rm -f intarr1.bin intarr1b.bin \"intarr1c .bin\" > \"intarr1d .bin\" intarr1.ihex intarr1.srec intarr1.tekhex intarr2.bin > intarr2b.bin intarr2.ihex intarr2.srec intarr2.tekhex intstr1.bin > intstr1b.bin \"intstr1c .bin\" \"intstr1d .bin\" intstr1.ihex > intstr1.srec intstr1.tekhex intstr2.bin intstr2b.bin intstr2.ihex > intstr2.srec intstr2.tekhex intarr3.srec" > -- The rest of the patch looks good to me. I am not a maintainer, you will need to wait for one to review the patch and give the OK. Thanks, -- Sergio