Re: [patch] Parse quoted string for restore/dump commands.

Sergio Durigan Junior
2013-06-20T04:59:46Z

On Monday, June 17 2013, Wei-cheng Wang wrote:

> Hi Sergio,
>
> Thank you for you review.
> I've fixed them accordingly at the end of the mail.

Thanks, Wei.

> On Sun, Jun 16, 2013 at 2:37 AM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>> On Friday, June 07 2013, Wei-cheng Wang wrote:
>>
>> 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.
>
> We've signed the copryright assignment in GDB, on behalf of Andes Technology
> in Dec. 2009. Is that sufficent?

It is probably fine then, but I am not sure. Some companies have
copyright assignments for all of its employees without restrictions,
while others make copyright assignments that don't cover everything. A
maintainer is the right person to check this.

Can some maintainer take a look for Wei, please?

>>>
>>> - (*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.
>
> I was considering the trade-off between time and space.
> Assuming in most cases, there is no space in the path,
> the initial size of the buffer is the length to the first delimiter.
> If we expand the buffer only 1 more byte a time, xrealloc might be
> called much more times than double the size, so I use the same policy
> of buildargv.

Oh, sorry, I should have been clearer. I was not referring to the
xrealloc inside the for/if, but rather I was saying that you could call
xrealloc *again* after you set "*outp = '\0'", after the loop finishes.

When the loop finishes you might end up with "filename" being larger
than you need.

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 7ae0797..a061f27 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2013-06-16 Wei-cheng Wang <cole945@gmail.com>
> + Sergio Durigan Junior <sergiodj@redhat.com>
> +
> + * cli/cli-dump.c (scan_filename_with_cleanup): Parse quoted or
> + escaped string when scanning filename.
> +

The rest of the patch looks OK to me now. You will have to wait for a
maintainer to (a) say if you're clear to commit the patch (due to the
copyright assignment matter), and (b) actually approve the patch.

Also, when you commit the patch, please remember to include your info on
gdb/MAINTAINERS, since you will be able to commit after approval.

Thanks,

--
Sergio