Page MenuHomeFreeBSD
Differential D33433 Diff 99988 usr.sbin/bhyve/bootrom.c

Changeset View

Standalone View

View Options

usr.sbin/bhyve/bootrom.c

Show First 20 Lines • Show All 185 Lines • ▼ Show 20 Linesbootrom_alloc(struct vmctx *ctx, size_t len, int prot, int flags,
*region_out = romptr + segoff; *region_out = romptr + segoff;
if (gpa_out != NULL) if (gpa_out != NULL)
*gpa_out = gpa; *gpa_out = gpa;
return (0); return (0);
} }
int int
bootrom_loadrom(struct vmctx *ctx, const char *romfile) bootrom_loadrom(struct vmctx *ctx, const nvlist_t *const nvl)
jhbUnsubmitted
Done
Inline Actions

A general note that '*const' seems a bit odd style-wise (vs * const). But it's also true that nowhere else in bhyve (and rarely in FreeBSD) uses the const on the pointer (vs the const on what the pointer points to). I would probably leave it out to be consistent with the rest of usr.sbin/bhyve.

jhb: A general note that '*const' seems a bit odd style-wise (vs `* const`). But it's also true…
{ {
struct stat sbuf; struct stat sbuf;
ssize_t rlen; ssize_t rlen;
off_t rom_size, var_size, total_size; off_t rom_size, var_size, total_size;
char *ptr, *varfile; char *ptr;
int fd, varfd, i, rv; int fd, varfd, i, rv;
rv = -1; rv = -1;
varfd = -1; varfd = -1;
varfile = strdup(romfile); const char *romfile = get_config_value_node(nvl, "romfile");
jhbUnsubmitted
Done
Inline Actions

So this won't work as well as you might hope as if a variable has any expansions, then it uses a thread-local buffer (so you can really only query one variable at a time). Perhaps structure this as:

romfile = get_config_value_node(nvl, "romfile");
if (romfile == NULL)
    return (-1);

fd = open(romfile, O_RDONLY);
...

varfile = get_config_value_node(nvl, "varfile");
if (varfile != NULL) {
    ...
jhb: So this won't work as well as you might hope as if a variable has any expansions, then it uses…
corvinkAuthorUnsubmitted
Done
Inline Actions

romfile is used by some debug logs after varfile is opened. Additionally, if someone adds new changes, this behaviour might be overlooked. For that reasons, I prefer to strdup the variables. If you don't like it, I will revert it and use your solution.

corvink: `romfile` is used by some debug logs after `varfile` is opened. Additionally, if someone adds…
jhbUnsubmitted
Done
Inline Actions

strdup() of romfile makes sense if it used multiple times. If you are only going to use varfile once, I'd rather just get it the one place it is used. That is the style more commonly used in other places in bhyve (fetching config items as-needed vs fetching them all at once up front).

jhb: strdup() of romfile makes sense if it used multiple times. If you are only going to use…
romfile = strsep(&varfile, ","); const char *varfile = get_config_value_node(nvl, "varfile");
if (romfile == NULL) {
return (-1);
}
jhbUnsubmitted
Done
Inline Actions

I would drop the const to avoid the cast (which really needs __DECONST()) for free.

jhb: I would drop the `const` to avoid the cast (which really needs __DECONST()) for `free`.
fd = open(romfile, O_RDONLY); fd = open(romfile, O_RDONLY);
andy_omniosce.orgUnsubmitted
Done
Inline Actions

get_config_value_node() can return NULL, particularly since you're now calling bootrom_load() even if lpc.bootrom is not present. How about something like this?

char *romfile = get_config_value_node(nvl, "bootrom");
if (romfile != NULL)
    romfile = strdup(romfile);
if (romfile == NULL)
    return (-1);
andy_omniosce.org: get_config_value_node() can return NULL, particularly since you're now calling bootrom_load()…
corvinkAuthorUnsubmitted
Done
Inline Actions

Thanks for catching that.

corvink: Thanks for catching that.
if (fd < 0) { if (fd < 0) {
EPRINTLN("Error opening bootrom \"%s\": %s", EPRINTLN("Error opening bootrom \"%s\": %s",
romfile, strerror(errno)); romfile, strerror(errno));
goto done; goto done;
} }
if (varfile != NULL) { if (varfile != NULL) {
varfd = open(varfile, O_RDWR); varfd = open(varfile, O_RDWR);
▲ Show 20 Lines • Show All 71 Lines • ▼ Show 20 Lines if (varfd >= 0) {
if (rv != 0) if (rv != 0)
goto done; goto done;
} }
rv = 0; rv = 0;
done: done:
if (fd >= 0) if (fd >= 0)
close(fd); close(fd);
return (rv); return (rv);
markjUnsubmitted
Done
Inline Actions

Isn't varfd being leaked here? Not a new bug in any case.

markj: Isn't varfd being leaked here? Not a new bug in any case.
corvinkAuthorUnsubmitted
Done
Inline Actions

I'm not sure. Bhyve has to keep the file open. Therefore, it uses var.mmap. I'm not sure if bhyve could close varfd while keeping var.mmap open.

corvink: I'm not sure. Bhyve has to keep the file open. Therefore, it uses var.mmap. I'm not sure if…
corvinkAuthorUnsubmitted
Done
Inline Actions

Man page of mmap states

The close(2) system call does not unmap pages, see munmap(2) for further information.

corvink: Man page of mmap states > The close(2) system call does not unmap pages, see munmap(2) for…
} }