bhyve nvme ramdisk backend cli option is broken after configuration management refactor.

[chuck]

Summary

The bhyve NVMe backend should support a device path of 'ram=<size_in_MiB>'
and this no longer works following the recent configuration management
changes.

Test Plan

I've tested that bhyve now correctly sets the ram configuration variable
when parsing legacy options:

# usr.sbin/bhyve/bhyve -o config.dump=1 -s 7,nvme,ram=16384 xx
pci.0.7.0.device=nvme
pci.0.7.0.ram=16384

# usr.sbin/bhyve/bhyve -o config.dump=1 -s 7,nvme,/dev/gpt/rootfs xx
pci.0.7.0.device=nvme
pci.0.7.0.path=/dev/gpt/rootfs

and with some other options, for completeness:

# usr.sbin/bhyve/bhyve -o config.dump=1 -s 7,nvme,ram=1234,direct,ro,sectorsize=512/512 xx
pci.0.7.0.device=nvme
pci.0.7.0.direct=true
pci.0.7.0.ro=true
pci.0.7.0.sectorsize=512/512
pci.0.7.0.ram=1234

[chuck]

Thank you for finding this bug!

What would you think of the following? It should work the same but avoids duplicating some of the logic found elsewhere:

diff --git a/usr.sbin/bhyve/pci_nvme.c b/usr.sbin/bhyve/pci_nvme.c
index 0abc0415a1d..d31a977c7bf 100644
--- a/usr.sbin/bhyve/pci_nvme.c
+++ b/usr.sbin/bhyve/pci_nvme.c
@@ -2800,11 +2800,30 @@ pci_nvme_init(struct vmctx *ctx, struct pci_devinst *pi, nvlist_t *nvl)        return (error);
 }

+static int
+pci_nvme_legacy_config(nvlist_t *nvl, const char *opts)
+{
+       char *cp;
+
+       if (opts == NULL)
+               return (0);
+
+       if (strncmp(opts, "ram=", 4) == 0) {
+               cp = strchr(opts, ',');
+               if (cp) {
+                       *cp = '\0';
+                       cp++;
+               }
+               set_config_value_node(nvl, "ram", opts + 4);
+               return (pci_parse_legacy_config(nvl, cp));
+       } else
+               return (blockif_legacy_config(nvl, opts));
+}

 struct pci_devemu pci_de_nvme = {
        .pe_emu =       "nvme",
        .pe_init =      pci_nvme_init,
-       .pe_legacy_config = blockif_legacy_config,
+       .pe_legacy_config = pci_nvme_legacy_config,
        .pe_barwrite =  pci_nvme_write,
        .pe_barread =   pci_nvme_read
 };

[jhb]

I think I like Chuck's shorter version a bit better, but Chuck's version needs to use strndup() instead of modifying the 'cosnt char *' input. (strchr's weird signature breaks the compiler noticing since it accepts a const char and silently "casts" away the const via the char * return type). Sorry for the regression though. The commit log should use a 'Fixes:' tag that references the original commit.

[andy_omniosce.org]

In D30717#690975, @jhb wrote:

I think I like Chuck's shorter version a bit better

Fine with me (modulo the *cp issue). Should I close this and let Chuck fix it?

[chuck]

In D30717#691487, @andy_omniosce.org wrote:

In D30717#690975, @jhb wrote:

I think I like Chuck's shorter version a bit better

Fine with me (modulo the *cp issue). Should I close this and let Chuck fix it?

Perhaps I'll commandeer this review to keep all the history in one place unless there are objections

[jhb]

Commandeering seems sensible to me.

[andy_omniosce.org]

Fine with me, thanks for doing this.

[chuck]

New version with change to strndup

[andy_omniosce.org]

I am used to contributing to illumos so please feel free to ignore these comments.

usr.sbin/bhyve/pci_nvme.c

2806

These could be scoped to the if block.

2817

No check for NULL from strndup()