From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32398 invoked by alias); 8 Jan 2012 07:17:04 -0000 Received: (qmail 32389 invoked by uid 22791); 8 Jan 2012 07:17:03 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_RG,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 08 Jan 2012 07:16:46 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q087GiR4020508 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sun, 8 Jan 2012 02:16:45 -0500 Received: from psique ([10.3.112.15]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q087GeLS012625; Sun, 8 Jan 2012 02:16:42 -0500 From: Sergio Durigan Junior To: Abhijit Halder Cc: "gdb-patches\@sourceware.org ml" Subject: Re: [PATCH] Implementation of pipe to pass GDB's command output to the shell. References: Date: Sun, 08 Jan 2012 07:17:00 -0000 In-Reply-To: (Abhijit Halder's message of "Sun, 8 Jan 2012 12:27:04 +0530") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes 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 X-SW-Source: 2012-01/txt/msg00258.txt.bz2 Abhijit Halder 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"). =C2=A0However, 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. =C2=A0I 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. =C2=A0*/ >>> + >>> +struct pipe_obj >>> +{ >>> + =C2=A0/* The delimiter to separate out gdb-command and shell-command.= =C2=A0This can be >>> + =C2=A0 =C2=A0 any arbitrary string without containing any whitespace.= =C2=A0There should not >>> + =C2=A0 =C2=A0 be any leading '-' in the delimiter. =C2=A0*/ >>> + =C2=A0char *dlim; >> >> I believe this can be declared as const, right? =C2=A0Same thing for >> `gdb_cmd' below. =C2=A0Unfortunately, `shell_cmd' cannot be declared con= st >> 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.