Re: [PATCH] Implementation of pipe to pass GDB's command output to the shell.

[Abhijit Halder <abhijit dot k dot halder at gmail dot com>]

On Sun, Jan 8, 2012 at 12:46 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> Abhijit Halder <abhijit.k.halder@gmail.com> writes:
>
>>>> +#if defined (__MINGW32__)
>>>> +# define DEFAULT_SHELL "cmd.exe"
>>>> +# define OPTION_TO_SHELL "/c"
>>>> +#else
>>>> +# define DEFAULT_SHELL "/bin/sh"
>>>> +# define OPTION_TO_SHELL "-c"
>>>> +#endif
>>>
>>> As far as I have researched, all bash-compatible shells accept `-c' as a
>>> parameter, and all of them interpret this parameter in the same way
>>> (i.e., "execute this command"). ?However, and I am not sure this is
>>> something we should worry about or not, there might be other shells
>>> around which do not support `-c', or expect something else. ?I don't
>>> know if a check is worthwhile in this case.
>>
>> Actually I am not aware of this. But does GDB support such shell? If
>> yes, then ofcourse we should put a check. Please comment further.
>
> Yeah, I am not sure we should worry about this, it seems to be very
> specific indeed.
>
>>>
>>>> +/* Structure to encapsulate all entities associated with pipe. ?*/
>>>> +
>>>> +struct pipe_obj
>>>> +{
>>>> + ?/* The delimiter to separate out gdb-command and shell-command. ?This can be
>>>> + ? ? any arbitrary string without containing any whitespace. ?There should not
>>>> + ? ? be any leading '-' in the delimiter. ?*/
>>>> + ?char *dlim;
>>>
>>> I believe this can be declared as const, right? ?Same thing for
>>> `gdb_cmd' below. ?Unfortunately, `shell_cmd' cannot be declared const
>>> for now because is is using `skip_spaces', which does not accept a
>>> `const char *' as argument.
>>>
>>> Otherwise, the patch looks good to me.
>>
>> I was very much tempted for making this as const char * and that's why
>> submitted the patch in hurry!
>> Actually pex_run expects argv to be char * const *. I was about to ask
>> this question whether this is required? Can't we change the prototype
>> of pex_run? Regarding use of shell_cmd here, we can typecast that
>> thing. Only bottleneck is pex_run. There also typecast may work,
>> atleast for compilation, but this can give runtime error in case we
>> don't treat argv inside pex_run as an array of const string.
>
> Unfortunately GDB and others are full of those discrepancies. ?Thanks
> for letting me know that this const-correctness is actually harder than
> I thought. ?In my opinion, you should not worry about this for now
> otherwise your work will be compromised in a snowball of problems. ?I
> myself am facing such problems in another patch of mine. ?So I think I
> will just take back what I said here.
>
> Since I am not a maintainer, I cannot approve your patch, so I will take
> some time tomorrow and test it further while others review it.
>
> Thanks for working on this.

Thanks for your time in reviewing this.

Regards,
Abhijit Halder