From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68231 invoked by alias); 17 Mar 2015 14:56:25 -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 68219 invoked by uid 89); 17 Mar 2015 14:56:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.4 required=5.0 tests=AWL,BAYES_50 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Tue, 17 Mar 2015 14:56:23 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 37AB51164BA; Tue, 17 Mar 2015 10:56:21 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id oZ7FdwvK3ons; Tue, 17 Mar 2015 10:56:21 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 1CF4D1164B3; Tue, 17 Mar 2015 10:56:21 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id AFB3D40EAD; Tue, 17 Mar 2015 10:56:20 -0400 (EDT) Date: Tue, 17 Mar 2015 14:56:00 -0000 From: Joel Brobecker To: Yurij Grechishhev Cc: gdb-patches@sourceware.org Subject: Re: Setting parity for remote serial Message-ID: <20150317145620.GB7494@adacore.com> References: <20130926165939.GA5440@adacore.com> <20131004073447.GB3326@adacore.com> <20150225151646.GA30181@adacore.com> <20150227081642.GE31815@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2015-03/txt/msg00491.txt.bz2 Yurij, Below are my commends regarding your patch. You'll see that I have a lot of comments, but they are very minor and the patch essentially looks fine to me. Please forgive me if it feels like I am nitpicking - GDB has a fairly complex coding style, which is mostly dictated by the fact that it is part of the GNU project and therefore following the GNU Coding Style (and then some). Also, it would be useful for you to confirm that this patch is on top of master, and on which platform you tested this patch (I assume that you tested it by running the testsuite before and after your change). Thank you! > +2013-07-16 Yurij Grechishhev > + > + * NEWS: Mention set/show serial parity command. > + * monitor.c (monitor_open): Add serial_setparity. "Add" was a little confusing to me. How about "Call serial_setparity"? > + * remote.c (remote_open_1): Likewise. > + * ser-base.c (ser_base_serparity): New function. > + * ser-base.h: Add ser_base_setparity declaration. * ser-base.h (ser_base_setparity): Add declaration. > + * serail.c: Add serial_parity declaration and definitions of > + parity_enums and parity. > + (serial_parity): Definition > + (set_parity): New function. > + (serial_setparity): New function. > + (_initialize_serial): Add set/show serial parity command description typo: "serail.c" -> "serial.c" Same remark as above in terms of how the entry should be formatted. It also seems like the contents is a bit out of order with respect to the diff, making it hard to double-check it. I suggest: * serial.c (serial_parity): New function. (serial_parity): New global. (parity_none, parity_odd, parity_even, parity_enums, parity): New static globals. (set_parity): New function. (_initialize_serial): Add set/show serial parity commands. Also, the last comment on the ChangeLog entry is that sentences should start with a capital letter, and end with a period. Can you adjust the rest of the ChangeLog entry using the above guidelines, please? > + * serial.h: Add GDBPARITY_NONE, GDBPARITY_ODD, GDBPARITY_EVEN > + definitions. Add serial_setparity declaration. > + (serial_ops): Add setparity entry. > + * ser-mingw.c (ser_windows_raw): Remove state.fParity and > + state.Parity definitions. > + (ser_windows_setparity): New function. > + (hardwire_ops): add ser_windows_setparity. > + (tty_ops): add NULL for setparity field > + (pipe_ops): add ser_base_setparity. > + (tcp_ops): add ser_base_setparity. > + * ser-pipe.c (pipe_ops): Likewise. > + * ser-tcp.c (tcp_ops): Likewise. > + * ser-unix.c: Add hardwire_setparity declaration. > + (hardwire_setparity): New function. > + (hardwire_ops): add hardwire_setparity. > + * ser-go32.c (dos_ops): add dos_noop > + * target.h: serial_parity declaration. > diff --git a/gdb/monitor.c b/gdb/monitor.c > index b040ec4..548dae3 100644 > --- a/gdb/monitor.c > +++ b/gdb/monitor.c > @@ -767,6 +767,7 @@ monitor_open (const char *args, struct monitor_ops > *mon_ops, int from_tty) > } > } > > + serial_setparity (monitor_desc, serial_parity); > serial_raw (monitor_desc); > > serial_flush_input (monitor_desc); > diff --git a/gdb/remote.c b/gdb/remote.c > index 9aaee13..1554259 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -4310,6 +4310,7 @@ remote_open_1 (const char *name, int from_tty, > } > } > > + serial_setparity(rs->remote_desc, serial_parity); Missing space before the '('. > serial_raw (rs->remote_desc); > > /* If there is something sitting in the buffer we might take it as a > diff --git a/gdb/ser-base.c b/gdb/ser-base.c > index 87817c4..93aad40 100644 > --- a/gdb/ser-base.c > +++ b/gdb/ser-base.c > @@ -541,6 +541,12 @@ ser_base_setstopbits (struct serial *scb, int num) > return 0; /* Never fails! */ > } > > +int > +ser_base_setparity (struct serial *scb, int parity) We have a policy, now, that all new functions should be documented. For functions like these, that simply implement a callback, a one-line introductory comment is sufficient. For instance: /* Implement the "setparity" serial_ops callback. */ That's sufficient to show that the function's documentation is the same as the callback's documentation. > extern int ser_base_setbaudrate (struct serial *scb, int rate); > -extern int ser_base_setstopbits (struct serial *scb, int rate); > +extern int ser_base_setstopbits (struct serial *scb, int num); This change is unrelated, and obvious, so I pushed it on its own: https://www.sourceware.org/ml/gdb-patches/2015-03/msg00488.html > diff --git a/gdb/ser-mingw.c b/gdb/ser-mingw.c > index 7f335e9..be0f990 100644 > --- a/gdb/ser-mingw.c > +++ b/gdb/ser-mingw.c > @@ -153,7 +153,6 @@ ser_windows_raw (struct serial *scb) > if (GetCommState (h, &state) == 0) > return; > > - state.fParity = FALSE; > state.fOutxCtsFlow = FALSE; > state.fOutxDsrFlow = FALSE; > state.fDtrControl = DTR_CONTROL_ENABLE; > @@ -163,7 +162,6 @@ ser_windows_raw (struct serial *scb) > state.fNull = FALSE; > state.fAbortOnError = FALSE; > state.ByteSize = 8; > - state.Parity = NOPARITY; > > scb->current_timeout = 0; > > @@ -199,6 +197,36 @@ ser_windows_setstopbits (struct serial *scb, int num) > } > > static int > +ser_windows_setparity (struct serial *scb, int parity) Same as above - missing function introductory comment. > +{ > + HANDLE h = (HANDLE) _get_osfhandle (scb->fd); > + DCB state; > + > + if (GetCommState (h, &state) == 0) > + return -1; > + > + switch (parity) > + { > + case GDBPARITY_NONE: > + state.Parity = NOPARITY; > + state.fParity = FALSE; > + break; > + case GDBPARITY_ODD: > + state.Parity = ODDPARITY; > + state.fParity = TRUE; > + break; > + case GDBPARITY_EVEN: > + state.Parity = EVENPARITY; > + state.fParity = TRUE; > + break; > + default: > + return 1; Let's emit an internal_warning if parity has an unknown value. PARITY should always be one of those 3, and getting anything else is an internal_error. But the reason why I suggested the use of an internal_warning instead of internal_error is that internal_error is pretty-much a fatal error, but maybe we don't absolutely need it to be fatal. Communication is probably going to be screwed up, but anything else should still be working. We just want to warn the user of the issue he's likely going to face. > + } > +static int > +hardwire_setparity (struct serial *scb, int parity) > +{ Missing function documentation. > + struct hardwire_ttystate state; > + int newparity = 0; > + > + if (get_tty_state (scb, &state)) > + return -1; > + > + switch (parity) > + { > + case GDBPARITY_NONE: > + newparity = 0; > + break; > + case GDBPARITY_ODD: > + newparity = PARENB | PARODD; > + break; > + case GDBPARITY_EVEN: > + newparity = PARENB; > + break; > + default: > + return 1; Same here - internal_warning to warn the user of likely communication issues. > --- a/gdb/serial.c > +++ b/gdb/serial.c > @@ -525,6 +525,12 @@ serial_setstopbits (struct serial *scb, int num) > } > > int > +serial_setparity (struct serial*scb, int parity) > +{ > + return scb->ops->setparity (scb, parity); > +} Missing function documentation. > +/* Parity for serial port */ > +int serial_parity = GDBPARITY_NONE; > + > +static const char parity_none[] = "none"; > +static const char parity_odd[] = "odd"; > +static const char parity_even[] = "even"; > +static const char *const parity_enums[] = > +{parity_none, parity_odd, parity_even, NULL}; Two spaces before '{' (indentation). > +static const char *parity = parity_none; > +static void > +set_parity (char *ignore_args, int from_tty, struct cmd_list_element *c) When the function's documentation is in the .h file, can you add a comment that says, Eg: /* See serial.h. */ ? > +/* Set parity for serial port. Return 0 for success, -1 for failure */ > + > +#define GDBPARITY_NONE 0 > +#define GDBPARITY_ODD 1 > +#define GDBPARITY_EVEN 2 > + > +extern int serial_setparity (struct serial *scb, int parity); Can you move the function's documentation after the #define's? > /* Can the serial device support asynchronous mode? */ > @@ -271,6 +279,7 @@ struct serial_ops > serial_ttystate); > int (*setbaudrate) (struct serial *, int rate); > int (*setstopbits) (struct serial *, int num); > + int (*setparity) (struct serial *, int parity); Please document here what this hook should be doing. Please name all the arguments as well, so you can use those names in the hook's description. Below is an example from target.h which shows how these kinds of "methods" are now documented in GDB: /* Return the thread-local address at OFFSET in the thread-local storage for the thread PTID and the shared library or executable file given by OBJFILE. If that block of thread-local storage hasn't been allocated yet, this function may return an error. LOAD_MODULE_ADDR may be zero for statically linked multithreaded inferiors. */ CORE_ADDR (*to_get_thread_local_address) (struct target_ops *ops, ptid_t ptid, CORE_ADDR load_module_addr, CORE_ADDR offset) > index c95e1a4..685a37f 100644 > --- a/gdb/target.h > +++ b/gdb/target.h > @@ -2236,6 +2236,10 @@ extern int remote_debug; > > /* Speed in bits per second, or -1 which means don't mess with the speed. */ > extern int baud_rate; > + > +/* Parity for serial port */ > +extern int serial_parity; For the global's documentation, can you add a period followed by two spaces? Thus: /* Parity for serial port. */ -- Joel