[PATCH 4/19] bhyve: add basl support for int values

[corvink]

Summary

In upcoming commits, bhyve will build some ACPI tables by it's own.
Therefore, it should be capable of appending int values to ACPI tables.

This is the fourth patch required to merge D36983

[markj]

usr.sbin/bhyve/basl.c

163

Can this be an assertion instead of a runtime check?

[corvink]

usr.sbin/bhyve/basl.c

163

IMHO, bhyve makes too much use of assertions. You should assume that an assertion won't be compiled for release builds.

In this case, ACPI tables aren't very dynamic yet. So, an assertion could make sense. Nevertheless, ACPI table creation could be more dynamic in the future (e.g. with tpm emulation). So, it's harder to check all code pathes on debug builds.

[markj]

usr.sbin/bhyve/basl.c

163

bhyve uses assert() too much, yes. In practice it works because assert() is always enabled, in release or debug builds of FreeBSD. I believe it should start using a custom assert() which is guaranteed to be enabled.

So, it's harder to check all code pathes on debug builds.

I don't quite follow what you mean. To me, the reasons assertions are better here are:

• They document the expectations of the interface. When these are runtime checks, I can't easily tell if they may legitimately arise at runtime.
• They make debugging easier since bhyve fails closed.
• We do not need to rely on all callers to correctly check for errors and pass error state up the stack.

[corvink]

• address feedback of markj

[markj]

usr.sbin/bhyve/basl.c

163

Thanks, but really the change should be consistent. e.g., basl_table_append_bytes() should use asserts as well. I'm not sure if you applied the change elsewhere or not.

I don't mean to insist on this approach; if you really prefer runtime checks, then please do it your way. Using assert() (or some hypothetical bhyve_assert()) just helps distinguish between true runtime errors and defensive checks, and helps reduce code complexity, so I find it easier to read. Anyway, I will leave it up to you.

[corvink]

usr.sbin/bhyve/basl.c

163

I'm going to update all of my patches to use assertions.

[jhb]

usr.sbin/bhyve/basl.c

166

I would perhaps have done this:

char buf[8];

le64enc(buf, val);
return (basl_table_append_bytes(table, buf, size));

I think using a byte buffer explicitly is clearer intent I think.

[corvink]

• add basl_le_enc helper