[Qemu-devel] [RESEND][PATCH] gdbstub: listen on ipv4 address

[Qemu-devel] [RESEND][PATCH] gdbstub: listen on ipv4 address

[Sebastian Herbszt]

Make gdbstub listen on ipv4 address like before r5697.

- Sebastian

--- qemu-r6133/gdbstub.c.orig Sat Dec 27 15:57:16 2008
+++ qemu-r6133/gdbstub.c Sat Dec 27 15:57:29 2008
@@ -2276,7 +2276,7 @@
     if (*p == 0) {
         /* A numeric value is interpreted as a port number.  */
         snprintf(gdbstub_port_name, sizeof(gdbstub_port_name),
-                 "tcp::%d,nowait,nodelay,server", port_num);
+                 "tcp::%d,nowait,nodelay,server,ipv4", port_num);
         port = gdbstub_port_name;
     }
 

Re: [Qemu-devel] [RESEND][PATCH] gdbstub: listen on ipv4 address

[Anthony Liguori]

Sebastian Herbszt wrote:
> Make gdbstub listen on ipv4 address like before r5697.

Any reason to make it explicit?

BTW, the patch needs a signed-off-by.

Regards,

Anthony Liguori

> - Sebastian
>
> --- qemu-r6133/gdbstub.c.orig Sat Dec 27 15:57:16 2008
> +++ qemu-r6133/gdbstub.c Sat Dec 27 15:57:29 2008
> @@ -2276,7 +2276,7 @@
>     if (*p == 0) {
>         /* A numeric value is interpreted as a port number.  */
>         snprintf(gdbstub_port_name, sizeof(gdbstub_port_name),
> -                 "tcp::%d,nowait,nodelay,server", port_num);
> +                 "tcp::%d,nowait,nodelay,server,ipv4", port_num);
>         port = gdbstub_port_name;
>     }
>
>
>
>

[Qemu-devel] Re: [RESEND][PATCH] gdbstub: listen on ipv4 address

[Sebastian Herbszt]

Anthony Liguori wrote:
> Sebastian Herbszt wrote:
>> Make gdbstub listen on ipv4 address like before r5697.
> 
> Any reason to make it explicit?

There seems to be no support for IPv6 in gdb.
 
> BTW, the patch needs a signed-off-by.

Signed-off-by: Sebastian Herbszt <herbszt@gmx.de>

- Sebastian

> Regards,
> 
> Anthony Liguori
> 
>> - Sebastian
>>
>> --- qemu-r6133/gdbstub.c.orig Sat Dec 27 15:57:16 2008
>> +++ qemu-r6133/gdbstub.c Sat Dec 27 15:57:29 2008
>> @@ -2276,7 +2276,7 @@
>>     if (*p == 0) {
>>         /* A numeric value is interpreted as a port number.  */
>>         snprintf(gdbstub_port_name, sizeof(gdbstub_port_name),
>> -                 "tcp::%d,nowait,nodelay,server", port_num);
>> +                 "tcp::%d,nowait,nodelay,server,ipv4", port_num);
>>         port = gdbstub_port_name;
>>     }
>>
>>
>>
>>
> 
> 
> 
>

Re: [Qemu-devel] Re: [RESEND][PATCH] gdbstub: listen on ipv4 address

[Anthony Liguori]

Sebastian Herbszt wrote:
> Anthony Liguori wrote:
>> Sebastian Herbszt wrote:
>>> Make gdbstub listen on ipv4 address like before r5697.
>>
>> Any reason to make it explicit?
>
> There seems to be no support for IPv6 in gdb.

Are you unable to connect without the ipv4 option?  My understanding was 
that we shouldn't explicitly need this option.

Regards,

Anthony Liguori

>
>> BTW, the patch needs a signed-off-by.
>
> Signed-off-by: Sebastian Herbszt <herbszt@gmx.de>
>
> - Sebastian
>
>> Regards,
>>
>> Anthony Liguori
>>
>>> - Sebastian
>>>
>>> --- qemu-r6133/gdbstub.c.orig Sat Dec 27 15:57:16 2008
>>> +++ qemu-r6133/gdbstub.c Sat Dec 27 15:57:29 2008
>>> @@ -2276,7 +2276,7 @@
>>>     if (*p == 0) {
>>>         /* A numeric value is interpreted as a port number.  */
>>>         snprintf(gdbstub_port_name, sizeof(gdbstub_port_name),
>>> -                 "tcp::%d,nowait,nodelay,server", port_num);
>>> +                 "tcp::%d,nowait,nodelay,server,ipv4", port_num);
>>>         port = gdbstub_port_name;
>>>     }
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>
>

[Qemu-devel] Re: [RESEND][PATCH] gdbstub: listen on ipv4 address

[Jan Kiszka]

Anthony Liguori wrote:
> Sebastian Herbszt wrote:
>> Anthony Liguori wrote:
>>> Sebastian Herbszt wrote:
>>>> Make gdbstub listen on ipv4 address like before r5697.
>>>
>>> Any reason to make it explicit?
>>
>> There seems to be no support for IPv6 in gdb.
> 
> Are you unable to connect without the ipv4 option?  My understanding was
> that we shouldn't explicitly need this option.

inet_listen (which is finally called for this) falls back to PF_UNSPEC
when no protocol is given. I'm not sure if we can expect (according to
specs) that all our host OSes pick the protocol we want (normally IPv4).
But at least it worked so far, not only for gdb.

However, before hard-coding any protocol here, I would suggest to rework
the command line parameters. I mean: drop "-p port" and extend "-s" to
"-s [dev]" where "dev" falls back to
"tcp::1234,nowait,nodelay,server,ipv4" when omitted.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [RESEND][PATCH] gdbstub: listen on ipv4 address

[Daniel P. Berrange]

On Wed, Jan 28, 2009 at 10:14:22AM +0100, Jan Kiszka wrote:
> Anthony Liguori wrote:
> > Sebastian Herbszt wrote:
> >> Anthony Liguori wrote:
> >>> Sebastian Herbszt wrote:
> >>>> Make gdbstub listen on ipv4 address like before r5697.
> >>>
> >>> Any reason to make it explicit?
> >>
> >> There seems to be no support for IPv6 in gdb.
> > 
> > Are you unable to connect without the ipv4 option?  My understanding was
> > that we shouldn't explicitly need this option.
> 
> inet_listen (which is finally called for this) falls back to PF_UNSPEC
> when no protocol is given. I'm not sure if we can expect (according to
> specs) that all our host OSes pick the protocol we want (normally IPv4).

There are three scenarios you need to consider wrt command line
args for TCP

 - IP address - the parser determines whether it IPv4 or V6 
   address and asets PF_INET / PF_INET6 as needed

 - Hostname - with PF_UNSPEC, and AI_ADDRCONFIG, getaddrinfo()
   will resolve the name, and return zero or more IP addresses.
   An IPv6 address is only returned if a NIC has IPv6 enabled.
   An IPv4 address is only returned if a NIC has IPv4 enabled.
   QEMU listens on the first address it gets

   If an IPv6 address is returned, QEMU *disables* IPV6_V6ONLY
   socket option, so the kernel  also accepts IPv4 connections
   on the IPv6 socket.

 - No hostname - with PF_UNSPEC, and AI_ADDRCONFIG, getaddrinf()
   will return the 0.0.0.0 or :: depending on whether the host
   has IPv6 enabled on any NICs. Again QEMU disables IPV6_V6ONLY
   to ensure IPv4 connections can be accepted over the IPv6 socket

Finally the ipv4/v6 command line flags in QEMU can override
the normal getaddrinfo() config. 

The GDB stub current code should be hitting scenario 3, and thus QEMU
should be listening on both IPv4 and IPv6 ports.

If everything is working as I describe above, we should not need any 
special case to force disable IPv6 for GDB stub, even if GDB itself is
IPv4 only - we always listen on IPv4 address even if we're also listening
on IPv6, unless the command line explicitly has the ',ipv6' flag provided.

If this isn't working, then perhaps our call to disable the IPV6_V6ONLY 
socket option is not working correctly on some OS ?

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[Qemu-devel] Re: [RESEND][PATCH] gdbstub: listen on ipv4 address

[Jan Kiszka]

Daniel P. Berrange wrote:
> On Wed, Jan 28, 2009 at 10:14:22AM +0100, Jan Kiszka wrote:
>> Anthony Liguori wrote:
>>> Sebastian Herbszt wrote:
>>>> Anthony Liguori wrote:
>>>>> Sebastian Herbszt wrote:
>>>>>> Make gdbstub listen on ipv4 address like before r5697.
>>>>> Any reason to make it explicit?
>>>> There seems to be no support for IPv6 in gdb.
>>> Are you unable to connect without the ipv4 option?  My understanding was
>>> that we shouldn't explicitly need this option.
>> inet_listen (which is finally called for this) falls back to PF_UNSPEC
>> when no protocol is given. I'm not sure if we can expect (according to
>> specs) that all our host OSes pick the protocol we want (normally IPv4).
> 
> There are three scenarios you need to consider wrt command line
> args for TCP
> 
>  - IP address - the parser determines whether it IPv4 or V6 
>    address and asets PF_INET / PF_INET6 as needed
> 
>  - Hostname - with PF_UNSPEC, and AI_ADDRCONFIG, getaddrinfo()
>    will resolve the name, and return zero or more IP addresses.
>    An IPv6 address is only returned if a NIC has IPv6 enabled.
>    An IPv4 address is only returned if a NIC has IPv4 enabled.
>    QEMU listens on the first address it gets
> 
>    If an IPv6 address is returned, QEMU *disables* IPV6_V6ONLY
>    socket option, so the kernel  also accepts IPv4 connections
>    on the IPv6 socket.
> 
>  - No hostname - with PF_UNSPEC, and AI_ADDRCONFIG, getaddrinf()
>    will return the 0.0.0.0 or :: depending on whether the host
>    has IPv6 enabled on any NICs. Again QEMU disables IPV6_V6ONLY
>    to ensure IPv4 connections can be accepted over the IPv6 socket
> 
> Finally the ipv4/v6 command line flags in QEMU can override
> the normal getaddrinfo() config. 
> 
> The GDB stub current code should be hitting scenario 3, and thus QEMU
> should be listening on both IPv4 and IPv6 ports.

That's true. gdb stumbled over optimizations and showed me the state
before the getaddrinf call. In fact, AF_INET6 is passed to socket() as
IPv6 is enabled for my host.

> 
> If everything is working as I describe above, we should not need any 
> special case to force disable IPv6 for GDB stub, even if GDB itself is
> IPv4 only - we always listen on IPv4 address even if we're also listening
> on IPv6, unless the command line explicitly has the ',ipv6' flag provided.
> 
> If this isn't working, then perhaps our call to disable the IPV6_V6ONLY 
> socket option is not working correctly on some OS ?

Independent of that, having a canonical command line switch for the
gdbstub has some value of its own. The current -s/-p duo is unfortunate.
Find a proposal below. Quick-tested only, breaks user mode (which is
hard-coded to IPv4, BTW).

Jan

--------->

diff --git a/gdbstub.c b/gdbstub.c
index c99c080..58e55f5 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2411,26 +2411,20 @@ static void gdb_chr_event(void *opaque, int event)
     }
 }
 
-int gdbserver_start(const char *port)
+int gdbserver_start(const char *device)
 {
     GDBState *s;
-    char gdbstub_port_name[128];
-    int port_num;
-    char *p;
+    char gdbstub_device_name[128];
     CharDriverState *chr;
 
-    if (!port || !*port)
+    if (!device)
       return -1;
 
-    port_num = strtol(port, &p, 10);
-    if (*p == 0) {
-        /* A numeric value is interpreted as a port number.  */
-        snprintf(gdbstub_port_name, sizeof(gdbstub_port_name),
-                 "tcp::%d,nowait,nodelay,server", port_num);
-        port = gdbstub_port_name;
-    }
+    /* enforce required attributes */
+    snprintf(gdbstub_device_name, sizeof(gdbstub_device_name),
+             "%s,nowait,nodelay,server", device);
 
-    chr = qemu_chr_open("gdb", port, NULL);
+    chr = qemu_chr_open("gdb", gdbstub_device_name, NULL);
     if (!chr)
         return -1;
 
diff --git a/vl.c b/vl.c
index 3676537..a2424ed 100644
--- a/vl.c
+++ b/vl.c
@@ -3985,8 +3985,7 @@ static void help(int exitcode)
            "-monitor dev    redirect the monitor to char device 'dev'\n"
            "-pidfile file   write PID to 'file'\n"
            "-S              freeze CPU at startup (use 'c' to start execution)\n"
-           "-s              wait gdb connection to port\n"
-           "-p port         set gdb connection port [default=%s]\n"
+           "-s [dev]        wait for gdb connection on 'dev' [default=tcp::%s]\n"
            "-d item1,...    output log to %s (use -d ? for a list of log items)\n"
            "-hdachs c,h,s[,t]\n"
            "                force hard disk 0 physical geometry and the optional BIOS\n"
@@ -4050,6 +4049,7 @@ static void help(int exitcode)
 }
 
 #define HAS_ARG 0x0001
+#define OPT_ARG 0x0002
 
 enum {
     /* Please keep in synch with help, qemu_options[] and
@@ -4121,7 +4121,6 @@ enum {
     QEMU_OPTION_pidfile,
     QEMU_OPTION_S,
     QEMU_OPTION_s,
-    QEMU_OPTION_p,
     QEMU_OPTION_d,
     QEMU_OPTION_hdachs,
     QEMU_OPTION_L,
@@ -4239,8 +4238,7 @@ static const QEMUOption qemu_options[] = {
     { "monitor", HAS_ARG, QEMU_OPTION_monitor },
     { "pidfile", HAS_ARG, QEMU_OPTION_pidfile },
     { "S", 0, QEMU_OPTION_S },
-    { "s", 0, QEMU_OPTION_s },
-    { "p", HAS_ARG, QEMU_OPTION_p },
+    { "s", OPT_ARG, QEMU_OPTION_s },
     { "d", HAS_ARG, QEMU_OPTION_d },
     { "hdachs", HAS_ARG, QEMU_OPTION_hdachs },
     { "L", HAS_ARG, QEMU_OPTION_L },
@@ -4548,8 +4546,8 @@ static void termsig_setup(void)
 int main(int argc, char **argv, char **envp)
 {
 #ifdef CONFIG_GDBSTUB
-    int use_gdbstub;
-    const char *gdbstub_port;
+    int use_gdbstub = 0;
+    const char *gdbstub_dev = "tcp::" DEFAULT_GDBSTUB_PORT;
 #endif
     uint32_t boot_devices_bitmap = 0;
     int i;
@@ -4625,10 +4623,6 @@ int main(int argc, char **argv, char **envp)
     initrd_filename = NULL;
     ram_size = 0;
     vga_ram_size = VGA_RAM_SIZE;
-#ifdef CONFIG_GDBSTUB
-    use_gdbstub = 0;
-    gdbstub_port = DEFAULT_GDBSTUB_PORT;
-#endif
     snapshot = 0;
     nographic = 0;
     curses = 0;
@@ -4698,6 +4692,9 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 optarg = argv[optind++];
+            } else if ((popt->flags & OPT_ARG) && optind < argc &&
+                       argv[optind][0] != '-') {
+                optarg = argv[optind++];
             } else {
                 optarg = NULL;
             }
@@ -4963,9 +4960,8 @@ int main(int argc, char **argv, char **envp)
 #ifdef CONFIG_GDBSTUB
             case QEMU_OPTION_s:
                 use_gdbstub = 1;
-                break;
-            case QEMU_OPTION_p:
-                gdbstub_port = optarg;
+                if (optarg)
+                    gdbstub_dev = optarg;
                 break;
 #endif
             case QEMU_OPTION_L:
@@ -5660,9 +5656,9 @@ int main(int argc, char **argv, char **envp)
     if (use_gdbstub) {
         /* XXX: use standard host:port notation and modify options
            accordingly. */
-        if (gdbserver_start(gdbstub_port) < 0) {
-            fprintf(stderr, "qemu: could not open gdbstub device on port '%s'\n",
-                    gdbstub_port);
+        if (gdbserver_start(gdbstub_dev) < 0) {
+            fprintf(stderr, "qemu: could not open gdbstub device on '%s'\n",
+                    gdbstub_dev);
             exit(1);
         }
     }

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [RESEND][PATCH] gdbstub: listen on ipv4 address

[Jamie Lokier]

Anthony Liguori wrote:
> Sebastian Herbszt wrote:
> >There seems to be no support for IPv6 in gdb.

Even though GDB itself cannot connect over IPv6, it can be run over a
tunnel which may route to IPv6.

> Are you unable to connect without the ipv4 option?  My understanding was 
> that we shouldn't explicitly need this option.

It should.

(I gather some OSes (possibly old versions) don't listen to IPv4 and
IPv6 on the same socket; you need two sockets.  I don't know any of
the hosts QEMU works with need that.)

-- Jamie