From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 115138 invoked by alias); 13 Dec 2019 23:13:32 -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 115130 invoked by uid 89); 13 Dec 2019 23:13:32 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=stan, Stan, stone, Hopefully X-HELO: gateway20.websitewelcome.com Received: from gateway20.websitewelcome.com (HELO gateway20.websitewelcome.com) (192.185.59.4) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 13 Dec 2019 23:13:30 +0000 Received: from cm17.websitewelcome.com (cm17.websitewelcome.com [100.42.49.20]) by gateway20.websitewelcome.com (Postfix) with ESMTP id D1466400D4018 for ; Fri, 13 Dec 2019 16:02:58 -0600 (CST) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id fu7liGvLAqNtvfu7lizCEb; Fri, 13 Dec 2019 17:13:29 -0600 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=OUzyMsrkYl38+LU8kv+WhfQ44ZivrDhSWIwBneatq9Y=; b=lsU25E5qoxQVZuHtuwLlVLolHO Lyr4k6yd4GCWGWoFro/cdtIcUcaUJs6j1iWB1F41l5hcDee1UCj7IkQk0kp8SUQl+t0O4TRZBfFBA g4utnW4UcitGO3ZrvXgVtIofL; Received: from 75-166-123-50.hlrn.qwest.net ([75.166.123.50]:55792 helo=murgatroyd) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.92) (envelope-from ) id 1ifu7k-003MWA-T0; Fri, 13 Dec 2019 16:13:28 -0700 From: Tom Tromey To: Stan Cox Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] add file desc to gdbserver client_state References: <20191122233003.211567-1-cbiesinger@google.com> <8448b37f-ebdf-d2d9-829a-87f7f6ea102c@redhat.com> Date: Fri, 13 Dec 2019 23:13:00 -0000 In-Reply-To: <8448b37f-ebdf-d2d9-829a-87f7f6ea102c@redhat.com> (Stan Cox's message of "Tue, 26 Nov 2019 13:01:23 -0500") Message-ID: <87fthnvmxz.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2019-12/txt/msg00650.txt.bz2 >>>>> "Stan" == Stan Cox writes: Stan> This patch adds gdb_fildes_t support to gdbserver for later use by the Stan> gdbserver multi-client patch. That patch enables multiple clients Stan> with each client associated with its corresponding file desc. This Stan> patch adds the corresponding file desc to the client_state and adds a Stan> file desc parameter to the gdbserver I/O ops. It also adds a minimal Stan> multi_client_states which currently just manages a single client_state Stan> but later will be expanded to be a collection of multiple Stan> client_states where each client_state is the state for an individual Stan> client. The gdbserver multi-client patch is mentioned in: Stan> https://sourceware.org/ml/gdb-patches/2019-08/msg00577.html Thanks. This looks like a reasonable first step. Hopefully a few things will change in coming patches? I'll note some below. Also, I think in general it needs a few more comments and some formatting cleanup. Stan> + Stan> +int Stan> +get_remote_desc (void) Stan> +{ In gdb new functions need a comment -- usually a description in the header and then something like "see mumble.h" at the implementation. Will remote_desc be going away? And hopefully get_remote_desc is also just a stepping stone? Stan> + struct multi_client_states & client_states = get_client_states(); Should be "&client_states", without a space. Stan> +multi_client_states& "multi_client_states &". Stan> +get_client_states (void) No more "(void)" now that we're using C++. I think there's a few spots for this. Stan> +client_state& "client_state &". Stan> +multi_client_states::set_client_state (gdb_fildes_t fd) Stan> +{ Stan> + client_state &cs = get_client_state (); Can get_client_state be made static now? If not, why not? Stan> + int remote_desc = get_remote_desc(); "get_remote_desc ()". But why does this use int while other spots use gdb_fildes_t? Stan> --- a/gdb/gdbserver/server.h Stan> +++ b/gdb/gdbserver/server.h Stan> @@ -142,6 +142,8 @@ struct client_state Stan> own_buf ((char *) xmalloc (PBUFSIZ + 1)) Stan> {} Stan> + gdb_fildes_t file_desc; Needs a comment. Stan> -client_state &get_client_state (); Stan> +struct multi_client_states Stan> +{ Stan> +public: Needs a comment. Probably should say "class" and not "struct", particularly since it's using "public:". Stan> + client_state & set_client_state (gdb_fildes_t); "&set_client_state". Needs a comment. Stan> +client_state & get_client_state (); Stan> +struct multi_client_states & get_client_states (void); Extra spaces and lack of comments here too. Tom