From: Sergio Durigan Junior <sergiodj@redhat.com>
To: Pedro Alves <palves@redhat.com>
Cc: GDB Patches <gdb-patches@sourceware.org>,
Eli Zaretskii <eliz@gnu.org>,
Jan Kratochvil <jan.kratochvil@redhat.com>,
Paul Fertser <fercerpav@gmail.com>,
Tsutomu Seki <sekiriki@gmail.com>,
Armand Scholtes <armandsmailings@home.nl>
Subject: Re: [PATCH v3] Implement IPv6 support for GDB/gdbserver
Date: Wed, 11 Jul 2018 19:13:00 -0000 [thread overview]
Message-ID: <87k1q1twlj.fsf@redhat.com> (raw)
In-Reply-To: <bde67173-a04f-81a8-3956-8a6cd9af5f2d@redhat.com> (Pedro Alves's message of "Wed, 11 Jul 2018 13:55:11 +0100")
On Wednesday, July 11 2018, Pedro Alves wrote:
> Hi,
>
> This version looks mostly good to me. Only one thing raised
> my eyebrow. I also noticed a few typos here and there.
> See below.
>
> On 07/07/2018 09:47 PM, Sergio Durigan Junior wrote:
>
>> -/* Open a tcp socket. */
>> +/* Try to connect to the host represented by AINFO. If the connection
>> + succeeds, return its socket. If we get a ECONNREFUSED error,
>> + return -1. Otherwise, return nothing. POLLS is used when
>> + 'connect' returns EINPROGRESS, and we need to invoke
>> + 'wait_for_connect' to obtain the status. */
>>
>> -int
>> -net_open (struct serial *scb, const char *name)
>> +static gdb::optional<int>
>> +try_connect (const struct addrinfo *ainfo, unsigned int *polls)
>> {
>
> I find this use of gdb::optional<int> more confusing than
> helpful, since it creates multiple "levels" of places
> to check error. You have:
>
> #1 - empty optional, check errno for error [1]
> #2 - non-empty optional, value == -1, means ECONNREFUSED.
> #3 - non-empty optional, value is socket.
>
> ([1] the errno part is missing in the function's description)
>
> Above, #1 and #2 seem redundant?
>
> You can instead make the return type plain int, return -1 on
> all errors, and the caller can check errno for ECONNREFUSED.
>
> (If we were going to avoid errno, I think something like
> std::expected would be better than std/gdb::optional. std::optional
> for a type that already has a nullable state (in this case -1)
> tends to be confusing, IMO. See here for example:
> https://www.boost.org/doc/libs/1_61_0/libs/optional/doc/html/boost_optional/tutorial/when_to_use_optional.html
> )
>
> Notice how your patch left this case incorrectly returning -1:
>
> if (n < 0)
> {
> /* A negative value here means that we either timed out or
> got interrupted by the user. Just return. */
> close (sock);
> return -1;
> }
No problem. I've changed the code to use plain int now.
>> - if [target_info exists sockethost] {
>> + if { [info exists GDB_TEST_SOCKETHOST] } {
>> + # The user is not supposed to provide a port number, just a
>> + # hostname/address, therefore we add the trailing ":" here.
>> + set debughost "${GDB_TEST_SOCKETHOST}:"
>> + # Espace open and close square brackets.
>
> Typo: "Espace" -> "Escape"
Fixed.
>> +/* Initialized an unprefixed entry. In this case, we don't expect
>> + nothing on the 'struct addrinfo' HINT. */
>> +#define INIT_UNPREFIXED_ENTRY(ADDR, EXP_HOST, EXP_PORT) \
>> + INIT_ENTRY (ADDR, EXP_HOST, EXP_PORT, false, 0, 0, 0)
>
> Typo: "Initialized" -> "Initialize".
Fixed.
> Also, double negatives "don't expect nothing" don't work well in English.
>
> Do you mean "don't expect anything", or "expect nothing" ?
> (There's a difference: the former doesn't expect it but
> tolerates, the latter really requires nothing.)
We don't really care about HINT, so "don't expect anything" is the
proper one to use here. Fixed.
>> +
>> +/* Initialized an unprefixed IPv6 entry. In this case, we don't
>> + expect nothing on the 'struct addrinfo' HINT. */
>> +#define INIT_UNPREFIXED_IPV6_ENTRY(ADDR, EXP_HOST, EXP_PORT) \
>> + INIT_ENTRY (ADDR, EXP_HOST, EXP_PORT, false, AF_INET6, 0, 0)
>
> Same comments apply here.
Fixed.
>> +
>> + /* IPv6, host and port present, no brackets. */
>> + INIT_UNPREFIXED_ENTRY ("::1:1234", "::1", "1234"),
>> + /* IPv6, only host, no brackets. */
>> + // INIT_UNPREFIXED_ENTRY ("::1", "::1", ""),
>
> Leftover, remove?
Indeed, removed.
>> + /* IPv6, missing port, no brackets. */
>> + INIT_UNPREFIXED_ENTRY ("::1:", "::1", ""),
>
>> + /* Prefixed "tcp4:" IPv4, host and port presents. */
>
> Typo: "presents" -> "present". This appears in several places.
Fixed all of them.
I'll send a v4 soon.
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
next prev parent reply other threads:[~2018-07-11 19:13 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-23 21:48 [PATCH] " Sergio Durigan Junior
2018-05-23 23:40 ` Eli Zaretskii
2018-05-24 0:41 ` Sergio Durigan Junior
2018-05-24 16:54 ` Eli Zaretskii
2018-05-25 1:57 ` Sergio Durigan Junior
2018-05-31 20:10 ` Sergio Durigan Junior
2018-06-06 12:26 ` Pedro Alves
2018-06-08 1:13 ` Sergio Durigan Junior
2018-06-08 13:53 ` Pedro Alves
2018-06-08 17:47 ` Sergio Durigan Junior
2018-06-08 18:44 ` Pedro Alves
2018-06-08 19:28 ` Pedro Alves
2018-06-08 19:51 ` Pedro Alves
2018-06-08 20:43 ` Sergio Durigan Junior
2018-06-08 21:21 ` Sergio Durigan Junior
2018-06-08 21:51 ` Pedro Alves
2018-06-08 22:01 ` Sergio Durigan Junior
2018-06-15 0:25 ` [PATCH v2] " Sergio Durigan Junior
2018-06-15 7:12 ` Eli Zaretskii
2018-06-20 15:24 ` Pedro Alves
2018-06-21 4:54 ` Sergio Durigan Junior
2018-07-07 20:47 ` [PATCH v3] " Sergio Durigan Junior
2018-07-11 12:55 ` Pedro Alves
2018-07-11 19:13 ` Sergio Durigan Junior [this message]
2018-07-11 19:16 ` [PATCH v4] " Sergio Durigan Junior
2018-07-11 21:48 ` Pedro Alves
2018-07-11 23:43 ` Sergio Durigan Junior
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87k1q1twlj.fsf@redhat.com \
--to=sergiodj@redhat.com \
--cc=armandsmailings@home.nl \
--cc=eliz@gnu.org \
--cc=fercerpav@gmail.com \
--cc=gdb-patches@sourceware.org \
--cc=jan.kratochvil@redhat.com \
--cc=palves@redhat.com \
--cc=sekiriki@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).