Page MenuHomeFreeBSD

bhyve: abstract the configuration managment internal implementation
Needs ReviewPublic

Authored by andy_omniosce.org on Sun, Apr 4, 11:53 AM.

Details

Reviewers
None
Group Reviewers
bhyve
Summary

Abstract the configuration management implementation so that modules do not
have to manipulate nvlists directly and instead exclusively use the exposed
APIs from config.[ch].

The initial rationale for this was porting the new configuration management
framework to illumos, where nvlists exist but are somewhat different, but
centralising the nvlist implementation detail in one place seems generally
good.

This change uses a new config_node_t type for passing configuration nodes
around, and implements a config node iterator in config.c. config_node_t
is just a typedef to nvlist_t but could be an opaque pointer in the future.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 38312
Build 35201: arc lint + arc unit

Event Timeline

yuripv added inline comments.
usr.sbin/bhyve/config.c
46

you probably meant to check nvl?

412

parentheses around return value

usr.sbin/bhyve/net_backends.c
209

indentation?

403

indentation?

636

indentation?

usr.sbin/bhyve/pci_emul.h
57–58

indentation?

andy_omniosce.org marked 5 inline comments as done.

Addressed feedback.

usr.sbin/bhyve/pci_emul.h
57–58

This one is consistent with the rest of the files, I'll keep it matching.

I'm fine with the overall approach and am happy to have wrappers for the missing bits so that config_* is self-contained. I'm not sure some of the API changes in terms of adding 'const' really make sense though.

usr.sbin/bhyve/config.c
52

You could drop this 'const' if you do from the public APIs, then you don't need any of the casts there since the compiler will see they are correct. My understanding is that illumos will use a completely separate config.c anyway, so this one can assume that config_node_t == nvlist_t in the implementation.

usr.sbin/bhyve/config.h
96

I would not add the 'const' here. 'create' is definitely modifying *parent. Perhaps 'find' could use 'const', but it requires you to add -Wcast-qual breaking casts, so it seems simpler to just leave it off as the current API does.

106

Similarly, I would not add a 'const' that has to be casted away.