Gdbserver can listen on local domain sockets.

classic Classic list List threaded Threaded
40 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/4] GDB: Fix documentation for invoking GDBSERVER

John Darrington-4
On Thu, Oct 18, 2018 at 04:27:24PM -0400, Sergio Durigan Junior wrote:
     On Saturday, October 13 2018, John Darrington wrote:
     
     > The documentation did not mention the possibility of invoking gdbserver
     > with the new connection forms such as tcp6:host:port.  This change fixes
     > that.
     
     Thanks for the patch.
     
     As I mentioned while reviewing patch #1, there is a reason why gdbserver
     doesn't accept prefixes (which means that there isn't a mistake in the
     docs).  If you go with my suggestion and implement just the support for
     the "unix:" prefix, then this patch will need to be changed because it
     still won't be correct to mention the "tcp" prefixes.
     
Thanks for the reviews.

How about instead, we just add a test to print an error/warning if
someone tries to start gdbserver with a udp prefix?

J'

--
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

John Darrington-4
In reply to this post by Sergio Durigan Junior
On Thu, Oct 18, 2018 at 04:18:48PM -0400, Sergio Durigan Junior wrote:

     > +  bool is_unix = hint->ai_family == AF_UNIX;
     
     No need for a newline between the declarations of is_ipv6 and is_unix.
     
     Here, and everywhere else, AF_UNIX may be undefined if building GDB on a
     non-UNIX environment.  I'm afraid you may have to guard this code with
     "HAVE_SYS_UN_H".
     

This is true.  But a quick experiment showed me that there are quite a
few other places in gdb which has this problem.

I can prepare a separate patch to fix those.

J'
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

Sergio Durigan Junior
On Friday, October 19 2018, John Darrington wrote:

> On Thu, Oct 18, 2018 at 04:18:48PM -0400, Sergio Durigan Junior wrote:
>
>      > +  bool is_unix = hint->ai_family == AF_UNIX;
>      
>      No need for a newline between the declarations of is_ipv6 and is_unix.
>      
>      Here, and everywhere else, AF_UNIX may be undefined if building GDB on a
>      non-UNIX environment.  I'm afraid you may have to guard this code with
>      "HAVE_SYS_UN_H".
>      
>
> This is true.  But a quick experiment showed me that there are quite a
> few other places in gdb which has this problem.

Which places?  There are some files that are not compiled on certain
systems, so it's fine to have system-dependent code without the guards.
I don't use proprietary OSes, so the way I test here is to compile GDB
using a mingw compiler.  If it passes, then I assume things are OK.
Your patch, for example, broke the compilation (because of
AF_UNIX/AF_LOCAL).

Cheers,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/4] GDB: Fix documentation for invoking GDBSERVER

Sergio Durigan Junior
In reply to this post by John Darrington-4
On Friday, October 19 2018, John Darrington wrote:

> On Thu, Oct 18, 2018 at 04:27:24PM -0400, Sergio Durigan Junior wrote:
>      On Saturday, October 13 2018, John Darrington wrote:
>      
>      > The documentation did not mention the possibility of invoking gdbserver
>      > with the new connection forms such as tcp6:host:port.  This change fixes
>      > that.
>      
>      Thanks for the patch.
>      
>      As I mentioned while reviewing patch #1, there is a reason why gdbserver
>      doesn't accept prefixes (which means that there isn't a mistake in the
>      docs).  If you go with my suggestion and implement just the support for
>      the "unix:" prefix, then this patch will need to be changed because it
>      still won't be correct to mention the "tcp" prefixes.
>      
> Thanks for the reviews.
>
> How about instead, we just add a test to print an error/warning if
> someone tries to start gdbserver with a udp prefix?

Hm, I thought about that while writing the email, but at the time it
didn't seem like a good option.  However, thinking about it again, I
think it would be mostly fine.  I guess it's OK to support the
"tcp{,6}:" prefixes.

The check needs to implemented on gdbserver/remote-utils.c:remote_open,
and gdbserver needs to error out if "hint.ai_protocol == IPPROTO_UDP".
And "parse_connection_spec_without_prefix" could be merged with
"parse_connection_spec".

Aside from the documentation, you'd also probably have to update the
tests (both the selftests and the regular ones).

Thanks,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/4] GDB: Fix documentation for invoking GDBSERVER

John Darrington-4
On Fri, Oct 19, 2018 at 04:45:14PM -0400, Sergio Durigan Junior wrote:

     The check needs to implemented on gdbserver/remote-utils.c:remote_open,
     and gdbserver needs to error out if "hint.ai_protocol == IPPROTO_UDP".
     And "parse_connection_spec_without_prefix" could be merged with
     "parse_connection_spec".
     
A quick test shows that this is not necessary.  Any attempt to start
gdbserver with udp: will never get to remote_open because remote_prepare
will fail when it gets to listen:
 
  "Can't listen on socket: Operation no supported."

(listen is for connection orientated protocols only).


J'



--
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/4] GDB: Fix documentation for invoking GDBSERVER

Sergio Durigan Junior
On Sunday, October 21 2018, John Darrington wrote:

> On Fri, Oct 19, 2018 at 04:45:14PM -0400, Sergio Durigan Junior wrote:
>
>      The check needs to implemented on gdbserver/remote-utils.c:remote_open,
>      and gdbserver needs to error out if "hint.ai_protocol == IPPROTO_UDP".
>      And "parse_connection_spec_without_prefix" could be merged with
>      "parse_connection_spec".
>      
> A quick test shows that this is not necessary.  Any attempt to start
> gdbserver with udp: will never get to remote_open because remote_prepare
> will fail when it gets to listen:
>  
>   "Can't listen on socket: Operation no supported."
>
> (listen is for connection orientated protocols only).

Yes, but this error message is not entirely clear.  We should show a
more user-friendly message, and the best way to do that is to do an
early check for the UDP protocol.

Thanks,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/4] GDB: Fix documentation for invoking GDBSERVER

John Darrington-4
In reply to this post by Sergio Durigan Junior
On Fri, Oct 19, 2018 at 04:45:14PM -0400, Sergio Durigan Junior wrote:

     Aside from the documentation, you'd also probably have to update the
     tests (both the selftests and the regular ones).
     

When I run "make check" I get  a large number of failures, and I'm told
that this is normal.   How do I run just the tests which you think need
to be adjusted?

J'
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/4] GDB: Fix documentation for invoking GDBSERVER

Sergio Durigan Junior
On Tuesday, October 23 2018, John Darrington wrote:

> On Fri, Oct 19, 2018 at 04:45:14PM -0400, Sergio Durigan Junior wrote:
>
>      Aside from the documentation, you'd also probably have to update the
>      tests (both the selftests and the regular ones).
>      
>
> When I run "make check" I get  a large number of failures, and I'm told
> that this is normal.   How do I run just the tests which you think need
> to be adjusted?

You can take a look at the tests I touched when implementing IPv6:

commit c7ab0aef11d91b637bf091aa9176b8dc4aadee46
Author: Sergio Durigan Junior <[hidden email]>
Date:   Fri May 18 01:29:24 2018 -0400

    Implement IPv6 support for GDB/gdbserver


Specifically gdb.server/server-connect.exp.  Offhand I'm not sure if any
other test needs to be adjusted; probably not.  I'd advise taking a look
under the gdb.server/ directory just to double check.

Thanks,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

Simon Marchi-4
In reply to this post by Sergio Durigan Junior
On 2018-10-19 3:43 p.m., Sergio Durigan Junior wrote:

> On Friday, October 19 2018, John Darrington wrote:
>
>> On Thu, Oct 18, 2018 at 04:18:48PM -0400, Sergio Durigan Junior wrote:
>>
>>      > +  bool is_unix = hint->ai_family == AF_UNIX;
>>      
>>      No need for a newline between the declarations of is_ipv6 and is_unix.
>>      
>>      Here, and everywhere else, AF_UNIX may be undefined if building GDB on a
>>      non-UNIX environment.  I'm afraid you may have to guard this code with
>>      "HAVE_SYS_UN_H".
>>      
>>
>> This is true.  But a quick experiment showed me that there are quite a
>> few other places in gdb which has this problem.
>
> Which places?  There are some files that are not compiled on certain
> systems, so it's fine to have system-dependent code without the guards.
> I don't use proprietary OSes, so the way I test here is to compile GDB
> using a mingw compiler.  If it passes, then I assume things are OK.
> Your patch, for example, broke the compilation (because of
> AF_UNIX/AF_LOCAL).
>
> Cheers,
>

I just tried compiling with mingw and stumbled on this:

  CXX    common/netstuff.o
/home/simark/src/binutils-gdb/gdb/common/netstuff.c: In function ‘parsed_connection_spec parse_connection_spec(const char*, addrinfo*)’:
/home/simark/src/binutils-gdb/gdb/common/netstuff.c:148:18: error: ‘AF_LOCAL’ was not declared in this scope
       { "unix:", AF_LOCAL,  SOCK_STREAM },
                  ^~~~~~~~

What is the status on this?

Simon

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

John Darrington-4
Mea Culpa.

I missed that one.  I'll try to get a mingw environment set up and
check in a fix.

J'


On Sun, Oct 28, 2018 at 12:20:28PM -0400, Simon Marchi wrote:
     On 2018-10-19 3:43 p.m., Sergio Durigan Junior wrote:
     > On Friday, October 19 2018, John Darrington wrote:
     >
     >> On Thu, Oct 18, 2018 at 04:18:48PM -0400, Sergio Durigan Junior wrote:
     >>
     >>      > +  bool is_unix = hint->ai_family == AF_UNIX;
     >>      
     >>      No need for a newline between the declarations of is_ipv6 and is_unix.
     >>      
     >>      Here, and everywhere else, AF_UNIX may be undefined if building GDB on a
     >>      non-UNIX environment.  I'm afraid you may have to guard this code with
     >>      "HAVE_SYS_UN_H".
     >>      
     >>
     >> This is true.  But a quick experiment showed me that there are quite a
     >> few other places in gdb which has this problem.
     >
     > Which places?  There are some files that are not compiled on certain
     > systems, so it's fine to have system-dependent code without the guards.
     > I don't use proprietary OSes, so the way I test here is to compile GDB
     > using a mingw compiler.  If it passes, then I assume things are OK.
     > Your patch, for example, broke the compilation (because of
     > AF_UNIX/AF_LOCAL).
     >
     > Cheers,
     >
     
     I just tried compiling with mingw and stumbled on this:
     
       CXX    common/netstuff.o
     /home/simark/src/binutils-gdb/gdb/common/netstuff.c: In function ???parsed_connection_spec parse_connection_spec(const char*, addrinfo*)???:
     /home/simark/src/binutils-gdb/gdb/common/netstuff.c:148:18: error: ???AF_LOCAL??? was not declared in this scope
            { "unix:", AF_LOCAL,  SOCK_STREAM },
                       ^~~~~~~~
     
     What is the status on this?
     
     Simon

--
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

Simon Marchi-4
On 2018-10-28 14:10, John Darrington wrote:
> Mea Culpa.
>
> I missed that one.  I'll try to get a mingw environment set up and
> check in a fix.

Ok.  If you are on Linux, it shouldn't be too hard.  For example, I use
these packages (on two different machines):

https://aur.archlinux.org/packages/mingw-w64-gcc/
https://packages.ubuntu.com/en/bionic/gcc-mingw-w64

and configure with --host=x86_64-w64-mingw32.  I think that's all.

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

John Darrington-4
On Sun, Oct 28, 2018 at 02:51:50PM -0400, Simon Marchi wrote:
     On 2018-10-28 14:10, John Darrington wrote:
     > Mea Culpa.
     >
     > I missed that one.  I'll try to get a mingw environment set up and
     > check in a fix.
     
     Ok.  If you are on Linux, it shouldn't be too hard.  For example, I use
     these packages (on two different machines):
     
     https://aur.archlinux.org/packages/mingw-w64-gcc/
     https://packages.ubuntu.com/en/bionic/gcc-mingw-w64
     
     and configure with --host=x86_64-w64-mingw32.  I think that's all.
     
For some reason, when I try to crossbuild in this way, there are many
failures (unrelated to this issue).

However I've checked in a fix for this issue, and tested it by building
natively with a hacked set of standard include headers.

J'

--
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

Rainer Orth-2
Hi John,

> However I've checked in a fix for this issue, and tested it by building
> natively with a hacked set of standard include headers.

you always need to post patches here, if only for reference.

Besides, we're currently very inconsistent here (haven't checked which
part of that is due to your code): most places use AF_UNIX, only two use
AF_LOCAL instead (common/netstuff.c, gdbserver/remote-utils.c), and your
configure check only checks for AF_LOCAL.  I believe we should
canonicalize for one of the two and allow for systems that define only
one or the other.

        Rainer

--
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

Rainer Orth-2
Hi John,

>> However I've checked in a fix for this issue, and tested it by building
>> natively with a hacked set of standard include headers.
>
> you always need to post patches here, if only for reference.

I also noticed that your ChangeLog entry had a couple of formatting
issues.  I've fixed them with my last commit.

        Rainer

--
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

Simon Marchi-2
In reply to this post by Rainer Orth-2
On 2018-10-29 5:11 a.m., Rainer Orth wrote:
> Hi John,
>
>> However I've checked in a fix for this issue, and tested it by building
>> natively with a hacked set of standard include headers.
>
> you always need to post patches here, if only for reference.

Not only should you post here the patches you push as obvious, but I don't
think that this:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=98a17ece013cb94cd602496b9efb92b8816b3953

falls under the obvious rule:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/MAINTAINERS;h=d5154d394206861ef5471b4984a4762589c34fc4;hb=HEAD#l82

I can't judge whether the patch is right or not with a quick glance, but it
certainly is complex enough to warrant a discussion (as Rainer's reply below
shows).

Additionally, it seems like the initial 4-patch series was pushed without
explicit approval from a maintainer (at least I can't find any).  Next time,
please wait to have an approval before pushing.  If you are not sure whether
a reply constitutes an a approval, it's better to ask the maintainer to
clarify.

> Besides, we're currently very inconsistent here (haven't checked which
> part of that is due to your code): most places use AF_UNIX, only two use
> AF_LOCAL instead (common/netstuff.c, gdbserver/remote-utils.c), and your
> configure check only checks for AF_LOCAL.  I believe we should
> canonicalize for one of the two and allow for systems that define only
> one or the other.

mingw defines AF_UNIX, so I would tend to go towards that route.  Any of you
knows what happens at runtime when you try to bind a AF_UNIX socket on mingw/Windows?

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

John Darrington-4
On Mon, Oct 29, 2018 at 03:51:55PM +0000, Simon Marchi wrote:
     On 2018-10-29 5:11 a.m., Rainer Orth wrote:
     > Hi John,
     >
     >> However I've checked in a fix for this issue, and tested it by building
     >> natively with a hacked set of standard include headers.
     >
     > you always need to post patches here, if only for reference.
     
Doesn't that make the gdb-cvs list completely redundant?

     Not only should you post here the patches you push as obvious, but I don't
     think that this:
     
     https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=98a17ece013cb94cd602496b9efb92b8816b3953
     
     falls under the obvious rule:

But a number of people had complained that their build was broken, and
this was a fix for that.   I judged that in consideration of those
people fixding their problem was more important than strict observance
of protocol.
     
     I can't judge whether the patch is right or not with a quick glance, but it
     certainly is complex enough to warrant a discussion (as Rainer's reply below
     shows).

     
     Additionally, it seems like the initial 4-patch series was pushed without
     explicit approval from a maintainer (at least I can't find any).  Next time,
     please wait to have an approval before pushing.  If you are not sure whether
     a reply constitutes an a approval, it's better to ask the maintainer to
     clarify.

All of those patches were certainly discussed.   In the past, when I've
followed up a person who has commented on a patch, but been vague about
approval, I have had either a piqued response; or no response at all.

If you think it necesary however I can revert anything you think hasn't
had enough discussion.

     
     > Besides, we're currently very inconsistent here (haven't checked which
     > part of that is due to your code): most places use AF_UNIX, only two use
     > AF_LOCAL instead (common/netstuff.c, gdbserver/remote-utils.c), and your
     > configure check only checks for AF_LOCAL.  I believe we should
     > canonicalize for one of the two and allow for systems that define only
     > one or the other.
     
     mingw defines AF_UNIX, so I would tend to go towards that route.  Any of you
     knows what happens at runtime when you try to bind a AF_UNIX socket on mingw/Windows?
     
AS I understand it, it depends upon which version of windows.
Apparently recent versions have local ssockets, but older ones do not.

See https://blogs.msdn.microsoft.com/commandline/2017/12/19/af_unix-comes-to-windows/

J'

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

Sergio Durigan Junior
On Monday, October 29 2018, John Darrington wrote:

> On Mon, Oct 29, 2018 at 03:51:55PM +0000, Simon Marchi wrote:
>      On 2018-10-29 5:11 a.m., Rainer Orth wrote:
>      > Hi John,
>      >
>      >> However I've checked in a fix for this issue, and tested it by building
>      >> natively with a hacked set of standard include headers.
>      >
>      > you always need to post patches here, if only for reference.
>      
> Doesn't that make the gdb-cvs list completely redundant?
>
>      Not only should you post here the patches you push as obvious, but I don't
>      think that this:
>      
>      https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=98a17ece013cb94cd602496b9efb92b8816b3953
>      
>      falls under the obvious rule:
>
> But a number of people had complained that their build was broken, and
> this was a fix for that.   I judged that in consideration of those
> people fixding their problem was more important than strict observance
> of protocol.
>      
>      I can't judge whether the patch is right or not with a quick glance, but it
>      certainly is complex enough to warrant a discussion (as Rainer's reply below
>      shows).
>
>      
>      Additionally, it seems like the initial 4-patch series was pushed without
>      explicit approval from a maintainer (at least I can't find any).  Next time,
>      please wait to have an approval before pushing.  If you are not sure whether
>      a reply constitutes an a approval, it's better to ask the maintainer to
>      clarify.
>
> All of those patches were certainly discussed.   In the past, when I've
> followed up a person who has commented on a patch, but been vague about
> approval, I have had either a piqued response; or no response at all.

We were certainly discussing the patches, but they were not approved by
anyone.  It's also worth mentioning that I raised various points that
were not addressed (even though we discussed them).  It is still a
requirement that the patches need to be approved by at least one
maintainer before it is pushed to the repository.

> If you think it necesary however I can revert anything you think hasn't
> had enough discussion.

Yes, please.  The patch series is not ready to be pushed yet; there are
a bunch of points I raised that were not addressed (and are now causing
the failures).  I am not a global maintainer, but it is my opinion that
the patch series should be reverted for now.  We can continue discussing
and fixing things with it here in the list.

Thanks,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

John Darrington-4
On Mon, Oct 29, 2018 at 12:42:43PM -0400, Sergio Durigan Junior wrote:

     > If you think it necesary however I can revert anything you think hasn't
     > had enough discussion.
     
     Yes, please.  The patch series is not ready to be pushed yet; there are
     a bunch of points I raised that were not addressed (and are now causing
     the failures).  I am not a global maintainer, but it is my opinion that
     the patch series should be reverted for now.  We can continue discussing
     and fixing things with it here in the list.
     
Done.

Start discussing.

J'
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

Simon Marchi-2
In reply to this post by Sergio Durigan Junior
On 2018-10-29 12:42 p.m., Sergio Durigan Junior wrote:

> On Monday, October 29 2018, John Darrington wrote:
>
>> On Mon, Oct 29, 2018 at 03:51:55PM +0000, Simon Marchi wrote:
>>      On 2018-10-29 5:11 a.m., Rainer Orth wrote:
>>      > Hi John,
>>      >
>>      >> However I've checked in a fix for this issue, and tested it by building
>>      >> natively with a hacked set of standard include headers.
>>      >
>>      > you always need to post patches here, if only for reference.
>>      
>> Doesn't that make the gdb-cvs list completely redundant?

Perhaps, I don't know.  Personally, I am not subscribed to gdb-cvs.  And generally,
when posting on gdb-patches, you'll mention that the patch was pushed as "obvious",
which the commit message won't necessarily contain.

It's also sometimes necessary to discuss a patch that was initially committed as
obvious.  The post on gdb-patches would be the place for that.

>>      Not only should you post here the patches you push as obvious, but I don't
>>      think that this:
>>      
>>      https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=98a17ece013cb94cd602496b9efb92b8816b3953
>>      
>>      falls under the obvious rule:
>>
>> But a number of people had complained that their build was broken, and
>> this was a fix for that.   I judged that in consideration of those
>> people fixding their problem was more important than strict observance
>> of protocol.

Let me reassure you, I completely understand that the intention was good.  I don't
want you to feel bad in any way for trying to fix things up!  Next time, post the
patch to the mailing list for review.  If the patch title contains "Fix build for...",
it is likely that it will be reviewed quite quickly.

>>      
>>      I can't judge whether the patch is right or not with a quick glance, but it
>>      certainly is complex enough to warrant a discussion (as Rainer's reply below
>>      shows).
>>
>>      
>>      Additionally, it seems like the initial 4-patch series was pushed without
>>      explicit approval from a maintainer (at least I can't find any).  Next time,
>>      please wait to have an approval before pushing.  If you are not sure whether
>>      a reply constitutes an a approval, it's better to ask the maintainer to
>>      clarify.
>>
>> All of those patches were certainly discussed.   In the past, when I've
>> followed up a person who has commented on a patch, but been vague about
>> approval, I have had either a piqued response; or no response at all.
>
> We were certainly discussing the patches, but they were not approved by
> anyone.  It's also worth mentioning that I raised various points that
> were not addressed (even though we discussed them).  It is still a
> requirement that the patches need to be approved by at least one
> maintainer before it is pushed to the repository.

Yep.

>> If you think it necesary however I can revert anything you think hasn't
>> had enough discussion.
>
> Yes, please.  The patch series is not ready to be pushed yet; there are
> a bunch of points I raised that were not addressed (and are now causing
> the failures).  I am not a global maintainer, but it is my opinion that
> the patch series should be reverted for now.  We can continue discussing
> and fixing things with it here in the list.

I have reverted the initial series as well as the fixup.  Sergio has mentioned on IRC
that it causes some test failures on the buildbot that cause the test time very long.
So this needs more testing before we push it again.

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

Simon Marchi-2
In reply to this post by John Darrington-4
On 2018-10-29 1:34 p.m., John Darrington wrote:

> On Mon, Oct 29, 2018 at 12:42:43PM -0400, Sergio Durigan Junior wrote:
>
>      > If you think it necesary however I can revert anything you think hasn't
>      > had enough discussion.
>      
>      Yes, please.  The patch series is not ready to be pushed yet; there are
>      a bunch of points I raised that were not addressed (and are now causing
>      the failures).  I am not a global maintainer, but it is my opinion that
>      the patch series should be reverted for now.  We can continue discussing
>      and fixing things with it here in the list.
>      
> Done.
>
> Start discussing.

Well, there are these two loose ends.

https://sourceware.org/ml/gdb-patches/2018-10/msg00438.html
https://sourceware.org/ml/gdb-patches/2018-10/msg00465.html

Also, please check why this series made the buildbot go crazy in
the native-extended-gdbserver configuration:

  https://gdb-build.sergiodj.net/builders/Fedora-x86_64-native-extended-gdbserver-m64/builds/11314

It was suggested on IRC that the scoped_free_addrinfo in remote-utils.c is
at a wrong place, causing something to be used after free.

Simon
12