bhyve nvlist constness bug
- Reply: Brooks Davis : "Re: bhyve nvlist constness bug"
- Reply: John Baldwin : "Re: bhyve nvlist constness bug"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Sun, 23 Oct 2022 18:18:54 UTC
I'm going through compiler warnings in the bhyve code with the aim of bumping WARNS, since the current setting hides bugs. There's one in the config code that looks a bit tricky to resolve and I was hoping for some guidance on the right way to do that. The basic problem is _lookup_config_node() may return an nvlist via nvlist_get_nvlist(), but nvlist_get_nvlist() returns a const nvlist_t *, so _lookup_config_node() is discarding the const qualifier. And indeed, some callers will modify the returned node. The use of nvlist_move_nvlist() in _lookup_config_node() has a similar problem: nvlist_move_* "consumes" the passed value and is not supposed to be mutated after. I'm pretty sure this is actually a non-issue with our nvlist implementation; when adding an nvlist value to an nvlist, it doesn't store anything except for the pointer itself, so you can mutate it safely. Note, this is not true for all value types, specifically arrays. However, there are multiple nvlist implementations out there, and ours might change such that bhyve's config code becomes incorrect. The bug seems annoying to fix because consumers can do this: nvlist_t *nvl = find_config_node(path); set_config_value_node(nvl, "foo", "bar"); So, if we naively changed find_config_node() to return a copy of the nvlist at "path", set_config_value_node() would have to somehow figure out where in the config tree to insert the updated nvlist, but it doesn't have the path anymore. To fix it properly could change find_config_node() to return some opaque type which contains the source path of the node so that we can DTRT when mutating the node. IMO it's better if the config node type is opaque to consumers. Or, make each nvlist node store its source path by storing it in the nvlist itself. e.g., the nvlist node describing the device model at "pci.0.1.2" would have a "__path" key with value "pci.0.1.2", so that set_config_value_node() can figure out where in the tree to replace an existing copy of "pci.0.1.2". Or is there a simpler way to fix this that I missed? Any thoughts? I'm happy to implement the suggestions above but didn't want to do that work without some buy-in.