Page MenuHomeFreeBSD

byhve: add option to specify IP address for gdb
Needs ReviewPublic

Authored by oshogbo on Apr 6 2021, 3:36 PM.

Details

Reviewers
jhb
rgrimes
Group Reviewers
manpages
bhyve
Summary

Allow user to specify the IP address available for gdb
debugger.

I even wonder if we should't change the default from ANY to
localhost.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 38466
Build 35355: arc lint + arc unit

Event Timeline

bcr added a subscriber: bcr.

OK from manpages.

grehan added inline comments.
usr.sbin/bhyve/gdb.c
1854

I'd prefer the default was the loopback address (rfb defaults to that).

usr.sbin/bhyve/gdb.c
1854

I agree, I just wasn't sure if we want to change the default. Currently we are listening to ANY.

usr.sbin/bhyve/gdb.c
1854

I'll say it's fine, and can be changed later if there's disagreement.

Though this is not exactly a normal service, to me listening on any by default is the correct thing to do here. One thing I would like to note, this code appears to only listens on one of IPv4 or IPv6, which is probably a short coming.

This revision is now accepted and ready to land.Apr 7 2021, 12:07 PM

Rod, why is your opinion so important ?

usr.sbin/bhyve/bhyverun.c
489

I would go ahead and use "gdb.address" as the name.

1445

This won't actually work due to the way get_config_value works (it uses a single static buffer in case it has to do any variable expansion, so the next call to get_config_* invalidates any previously-returned char *. The old code ran atoi() before calling get_config_bool_default() which was ok, but the new code can't do that. Probably should just move the parsing into init_gdb() instead and call init_gdb() unconditionally.

usr.sbin/bhyve/gdb.c
1854

I'll say it's fine, and can be changed later if there's disagreement.

I think qemu defaults to localhost and that's probably better to do (as well as generally safer to not expose the debug port on a network by default). I wonder if it would be better though to use "localhost" rather than the numbers? Then you can get a dual-stack socket from getaddrinfo()?

This revision now requires review to proceed.Apr 9 2021, 5:57 PM
oshogbo added inline comments.
usr.sbin/bhyve/bhyverun.c
1445

Oh, I didn't notice that. I have look into the function and seen that you are using nvlist under hood and assume that you are just returning the value. As I test it only with the static texts I got two different pointers and everything look good. Your suggestion makes sense for me.

usr.sbin/bhyve/gdb.c
1854

I will change the default in separate commit.

usr.sbin/bhyve/gdb.c
1822

You can't do this parsing here as it breaks config files. You would need to parse the command line option and set 'gdb.wait', 'gdb.address', and 'gdb.port' over in main() in the getopt() loop. However, init_gdb() would just use get_config_value() directly instead of taking the port and wait bool as arguments.