libnv: Fix handling of nvlist_dump() and nvlist_send() for child nvlists

[markj]

Summary

Suppose an nvlist nvl belongs to a parent nvlist or nvlist array. In
this case, nvl contains a pointer to its container. This trips up
nvlist_send(nvl) and nvlist_dump(nvl), which intuitively should only
operate on nvl and its nvpairs. In particular, both of these functions
will traverse to nvl's parent and start sending/dumping the parent's
nvpairs, which results in assertion failures or nonsensical output,
respectively.

[oshogbo]

The goal, which obviously was not met, was to prevent sending an nvlist that is part of another list (array or list).
I think we should return an error from send/dump. What do you think?

[markj]

In D52360#1195553, @oshogbo wrote:

The goal, which obviously was not met, was to prevent sending an nvlist that is part of another list (array or list).

Why is that the goal? What's the problem with sending an nvlist that's part of another list? It's a natural and useful thing to be able to do. The only thing preventing it seems to be an oversight in the nvlist traversal code.

I think we should return an error from send/dump. What do you think?

nvlist_dump() has no good way to signal an error.

[oshogbo]

Why is that the goal? What's the problem with sending an nvlist that's part of another list? It's a natural and useful thing to be able to do. The only thing preventing it seems to be an oversight in the nvlist traversal code.

The main idea was to have something like ownership of a nvlist. When you use nvlist_move to transfer an object into nvlist, you are moving ownership of that object to that list. The assumption is that if you continue to reference the original list afterward, you’re probably doing something wrong. Like having a dangling reference. Otherwise one can use for example nvlist_xfer or nvlist_destroy as you forget that this nvlist is a part of bigger one.

However, if you see an advantage in sending a part of the nvlist, we can change this assumption.

nvlist_dump() has no good way to signal an error.

nvlist_dump() is actually a good point where you might want to investigate only part of the list.
So, there is no objections here.

[markj]

In D52360#1195919, @oshogbo wrote:

Why is that the goal? What's the problem with sending an nvlist that's part of another list? It's a natural and useful thing to be able to do. The only thing preventing it seems to be an oversight in the nvlist traversal code.

The main idea was to have something like ownership of a nvlist. When you use nvlist_move to transfer an object into nvlist, you are moving ownership of that object to that list. The assumption is that if you continue to reference the original list afterward, you’re probably doing something wrong. Like having a dangling reference. Otherwise one can use for example nvlist_xfer or nvlist_destroy as you forget that this nvlist is a part of bigger one.

But nvlist_send() and nvlist_dump() do not modify the input nvlist. They take a const nvlist_t *. So with this patch one can write nvl = nvlist_get_nvlist(parent, "foo"); nvlist_dump(nvl);. Without this patch, I can work around the problem: nvl = nvlist_get_nvlist(parent, "foo"); nvl = nvlist_clone(nvl); nvlist_dump(nvl);. But that's silly.

However, if you see an advantage in sending a part of the nvlist, we can change this assumption.

It is useful in bhyve. Various device models take a nvlist as input to an initialization routine, and this nvlist belongs to a global config nvlist which describes all of the device models for the VM. In my case I want to send that child nvlist to a different process with nvlist_send().

[oshogbo]

Ok :)