Changeset View
Standalone View
usr.sbin/bhyve/gdb.c
Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Lines | |||||
#include <netinet/in.h> | #include <netinet/in.h> | ||||
#include <assert.h> | #include <assert.h> | ||||
#ifndef WITHOUT_CAPSICUM | #ifndef WITHOUT_CAPSICUM | ||||
#include <capsicum_helpers.h> | #include <capsicum_helpers.h> | ||||
#endif | #endif | ||||
#include <err.h> | #include <err.h> | ||||
#include <errno.h> | #include <errno.h> | ||||
#include <fcntl.h> | #include <fcntl.h> | ||||
#include <netdb.h> | |||||
#include <pthread.h> | #include <pthread.h> | ||||
#include <pthread_np.h> | #include <pthread_np.h> | ||||
#include <stdbool.h> | #include <stdbool.h> | ||||
#include <stdio.h> | #include <stdio.h> | ||||
#include <stdlib.h> | #include <stdlib.h> | ||||
#include <string.h> | #include <string.h> | ||||
#include <sysexits.h> | #include <sysexits.h> | ||||
#include <unistd.h> | #include <unistd.h> | ||||
▲ Show 20 Lines • Show All 1,753 Lines • ▼ Show 20 Lines | cap_rights_init(&rights, CAP_ACCEPT, CAP_EVENT, CAP_READ, CAP_WRITE, | ||||
CAP_SETSOCKOPT, CAP_IOCTL); | CAP_SETSOCKOPT, CAP_IOCTL); | ||||
if (caph_rights_limit(s, &rights) == -1) | if (caph_rights_limit(s, &rights) == -1) | ||||
errx(EX_OSERR, "Unable to apply rights for sandbox"); | errx(EX_OSERR, "Unable to apply rights for sandbox"); | ||||
if (caph_ioctls_limit(s, ioctls, nitems(ioctls)) == -1) | if (caph_ioctls_limit(s, ioctls, nitems(ioctls)) == -1) | ||||
errx(EX_OSERR, "Unable to apply rights for sandbox"); | errx(EX_OSERR, "Unable to apply rights for sandbox"); | ||||
} | } | ||||
#endif | #endif | ||||
static void | |||||
parse_gdb_options(char *optarg, const char **saddr, const char **sport, bool *wait) | |||||
jhb: You can't do this parsing here as it breaks config files. You would need to parse the command… | |||||
{ | |||||
char *colon; | |||||
if (optarg[0] == 'w') { | |||||
*wait = true; | |||||
optarg++; | |||||
} else { | |||||
*wait = false; | |||||
} | |||||
colon = strrchr(optarg, ':'); | |||||
if (colon == NULL) { | |||||
*saddr = NULL; | |||||
*sport = optarg; | |||||
return; | |||||
} | |||||
*colon = '\0'; | |||||
colon++; | |||||
*saddr = optarg; | |||||
*sport = colon; | |||||
} | |||||
void | void | ||||
init_gdb(struct vmctx *_ctx, int sport, bool wait) | init_gdb(struct vmctx *_ctx, const char *optarg) | ||||
{ | { | ||||
struct sockaddr_in sin; | |||||
int error, flags, s; | int error, flags, s; | ||||
struct addrinfo hints; | |||||
struct addrinfo *gdbaddr; | |||||
const char *saddr, *sport; | |||||
bool wait; | |||||
char *str; | |||||
Not Done Inline ActionsI'd prefer the default was the loopback address (rfb defaults to that). grehan: I'd prefer the default was the loopback address (rfb defaults to that). | |||||
Done Inline ActionsI agree, I just wasn't sure if we want to change the default. Currently we are listening to ANY. oshogbo: I agree, I just wasn't sure if we want to change the default. Currently we are listening to ANY. | |||||
Not Done Inline ActionsI'll say it's fine, and can be changed later if there's disagreement. grehan: I'll say it's fine, and can be changed later if there's disagreement. | |||||
Not Done Inline Actions
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()? jhb: > I'll say it's fine, and can be changed later if there's disagreement.
I think qemu defaults… | |||||
Done Inline ActionsI will change the default in separate commit. oshogbo: I will change the default in separate commit. | |||||
debug("==> starting on %d, %swaiting\n", sport, wait ? "" : "not "); | if (optarg == NULL) | ||||
return; | |||||
str = strdup(optarg); | |||||
if (str == NULL) | |||||
errx(4, "Failed to allocate memory"); | |||||
parse_gdb_options(str, &saddr, &sport, &wait); | |||||
if (!saddr) { | |||||
#if defined(INET) | |||||
saddr = "0.0.0.0"; | |||||
#elif defined(INET6) | |||||
saddr = "[::]"; | |||||
#endif | |||||
} | |||||
debug("==> starting on %s:%s, %swaiting\n", | |||||
saddr, sport, wait ? "" : "not "); | |||||
error = pthread_mutex_init(&gdb_lock, NULL); | error = pthread_mutex_init(&gdb_lock, NULL); | ||||
if (error != 0) | if (error != 0) | ||||
errc(1, error, "gdb mutex init"); | errc(1, error, "gdb mutex init"); | ||||
error = pthread_cond_init(&idle_vcpus, NULL); | error = pthread_cond_init(&idle_vcpus, NULL); | ||||
if (error != 0) | if (error != 0) | ||||
errc(1, error, "gdb cv init"); | errc(1, error, "gdb cv init"); | ||||
memset(&hints, 0, sizeof(hints)); | |||||
hints.ai_family = AF_UNSPEC; | |||||
hints.ai_socktype = SOCK_STREAM; | |||||
hints.ai_flags = AI_NUMERICHOST | AI_NUMERICSERV | AI_PASSIVE; | |||||
Not Done Inline ActionsAre 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. jhb: Are we certain we want AI_NUMERICHOST vs allowing hostnames? If we don't give AI_NUMERICHOST… | |||||
Not Done Inline ActionsOne 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. markj: One problem with allowing hostnames here (which the committed diff does) is that they can't… | |||||
Not Done Inline Actions'-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. jhb: '-o gdb.address=<hostname>' should work for hostnames that start with a 'w'. We should perhaps… | |||||
if (getaddrinfo(saddr, sport, &hints, &gdbaddr) != 0) | |||||
err(1, "gdb address resolve"); | |||||
ctx = _ctx; | ctx = _ctx; | ||||
s = socket(PF_INET, SOCK_STREAM, 0); | s = socket(gdbaddr->ai_family, gdbaddr->ai_socktype, 0); | ||||
if (s < 0) | if (s < 0) | ||||
err(1, "gdb socket create"); | err(1, "gdb socket create"); | ||||
sin.sin_len = sizeof(sin); | if (bind(s, gdbaddr->ai_addr, gdbaddr->ai_addrlen) < 0) | ||||
sin.sin_family = AF_INET; | |||||
sin.sin_addr.s_addr = htonl(INADDR_ANY); | |||||
sin.sin_port = htons(sport); | |||||
if (bind(s, (struct sockaddr *)&sin, sizeof(sin)) < 0) | |||||
err(1, "gdb socket bind"); | err(1, "gdb socket bind"); | ||||
if (listen(s, 1) < 0) | if (listen(s, 1) < 0) | ||||
err(1, "gdb socket listen"); | err(1, "gdb socket listen"); | ||||
stopped_vcpu = -1; | stopped_vcpu = -1; | ||||
TAILQ_INIT(&breakpoints); | TAILQ_INIT(&breakpoints); | ||||
vcpu_state = calloc(guest_ncpus, sizeof(*vcpu_state)); | vcpu_state = calloc(guest_ncpus, sizeof(*vcpu_state)); | ||||
Show All 12 Lines | #endif | ||||
if (fcntl(s, F_SETFL, flags | O_NONBLOCK) == -1) | if (fcntl(s, F_SETFL, flags | O_NONBLOCK) == -1) | ||||
err(1, "Failed to mark gdb socket non-blocking"); | err(1, "Failed to mark gdb socket non-blocking"); | ||||
#ifndef WITHOUT_CAPSICUM | #ifndef WITHOUT_CAPSICUM | ||||
limit_gdb_socket(s); | limit_gdb_socket(s); | ||||
#endif | #endif | ||||
mevent_add(s, EVF_READ, new_connection, NULL); | mevent_add(s, EVF_READ, new_connection, NULL); | ||||
gdb_active = true; | gdb_active = true; | ||||
freeaddrinfo(gdbaddr); | |||||
free(str); | |||||
} | } |
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.