byhve: add option to specify IP address for gdb
ClosedPublic
Actions

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 ?

jhb added inline comments.Apr 8 2021, 6:59 PM

usr.sbin/bhyve/bhyverun.c
491	I would go ahead and use "gdb.address" as the name.
1471	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
1831	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()?

Address @jhb suggestions.

This revision now requires review to proceed.Apr 9 2021, 5:57 PM

Harbormaster completed remote builds in B38466: Diff 87131.Apr 9 2021, 5:57 PM

oshogbo marked 2 inline comments as done.Apr 9 2021, 5:59 PM

oshogbo added inline comments.

usr.sbin/bhyve/bhyverun.c
1471	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
1831	I will change the default in separate commit.

jhb added inline comments.Apr 27 2021, 3:20 PM

usr.sbin/bhyve/gdb.c
1823	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.

Address @jhb comments (I hope).

Harbormaster completed remote builds in B39877: Diff 90815.Jun 13 2021, 6:18 PM

Codewise this looks fine. I had one comment about permitting hostnames. Also, the new gdb.address variable should be documented in bhyve_config.5.

usr.sbin/bhyve/gdb.c
1863	Are we certain we want AI_NUMERICHOST vs allowing hostnames? If we don't give AI_NUMERICHOST, then the followup change to default to localhost can remove the #ifdef's above and just use "localhost" as the default address.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 21 2021, 5:43 PM

Closed by commit rG2cdff9918e79: byhve: add option to specify IP address for gdb (authored by oshogbo). · Explain Why

This revision was automatically updated to reflect the committed changes.

oshogbo added a commit: rG2cdff9918e79: byhve: add option to specify IP address for gdb.

markj added a subscriber: markj.Oct 21 2021, 5:05 PM

markj added inline comments.

usr.sbin/bhyve/gdb.c
1863	One problem with allowing hostnames here (which the committed diff does) is that they can't begin with "w" since we use that character to decide whether or not to set gdb.wait. I think we need an alternate syntax.

jhb added inline comments.Oct 22 2021, 4:23 PM

usr.sbin/bhyve/gdb.c
1863	'-o gdb.address=<hostname>' should work for hostnames that start with a 'w'. We should perhaps document this limitation though? In general I'd like to move towards using the config vars via -o. For example, I think it's fine to add new config options that can only be set via '-o' without adding a corresponding "short" option going forward.