From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 65097 invoked by alias); 17 Apr 2015 15:30:08 -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 65083 invoked by uid 89); 17 Apr 2015 15:30:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 17 Apr 2015 15:30:06 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t3HFU4WU008593 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 17 Apr 2015 11:30:04 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3HFU3kX018027; Fri, 17 Apr 2015 11:30:03 -0400 Message-ID: <553126FA.8030402@redhat.com> Date: Fri, 17 Apr 2015 15:30:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Gary Benson , gdb-patches@sourceware.org Subject: Re: [PATCH 7/7] Implement vFile:setfs in gdbserver References: <1429186791-6867-1-git-send-email-gbenson@redhat.com> <1429186791-6867-8-git-send-email-gbenson@redhat.com> In-Reply-To: <1429186791-6867-8-git-send-email-gbenson@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-04/txt/msg00685.txt.bz2 On 04/16/2015 01:19 PM, Gary Benson wrote: > +/* Handle a "vFile:setfs:" packet. */ > + > +static void > +handle_setfs (char *own_buf) > +{ > + char *p; > + int pid; > + > + p = own_buf + strlen ("vFile:setfs:"); > + > + if (require_int (&p, &pid) > + || pid < 0 > + || require_end (p)) > + { > + hostio_packet_error (own_buf); > + return; > + } > + > + hostio_fs_pid = pid; > + > + hostio_reply (own_buf, 0); > +} This should probably return empty if the_target->call_with_fs_of is NULL, to disable the packet in GDB? > int > handle_vFile (char *own_buf, int packet_len, int *new_packet_len) > { > - if (startswith (own_buf, "vFile:open:")) > + if (the_target->call_with_fs_of != NULL > + && startswith (own_buf, "vFile:setfs:")) > + handle_setfs (own_buf); > + else if (startswith (own_buf, "vFile:open:")) Ah, I see now that it's handled here. I'd prefer moving that check inside the handle_setfs function instead so that the "setfs" specifics handling is all localized. > +struct open_closure > +{ > + char filename[HOSTIO_PATH_MAX]; > + int flags; > + int mode; mode_t mode. (please check for the the same issue on the native side.) > + int return_value; > +}; > +/* Implementation of target_ops->call_with_fs_of. */ > + > +static int > +linux_low_call_with_fs_of (int pid, void (*func) (void *), void *arg) > +{ > + return linux_ns_enter (pid, LINUX_NS_MNT, func, arg); > +} > + So back on linux_ns_enter's naming --- I find the abstraction here a bit odd. It seems to be me that a function that does: #1 - select filesystem/namespace #2 - call foo #3 - restore filesystem/namespace should be a function in common code, and that the only target-specific part is selecting a filesystem/namespace. That is, simplifying, something like: /* Cause the filesystem to appear as it does to process PID and call FUNC with argument ARG, restoring the filesystem to its original state afterwards. Return nonzero if FUNC was called, zero otherwise (and set ERRNO). */ int call_with_fs_of (int pid, void (*func) (void *), void *arg) { int ret; target->select_fs (pid); ret = func (); target->select_fs (0); return ret; } Was there a reason this wasn't done that way? (maybe make target->select_fs() return the previous namespace, instead of passing 0 on the second call.) Thanks, Pedro Alves