assert(3) vq_getchain() on pci_vtcon_notify_tx() spotted by gcc.

[araujo]

Summary

vq_getchain() can return -1 if some descriptor(s) are invalid and
prints a diagnostic message.

We should assert the return on variable n.

[rgrimes]

usr.sbin/bhyve/pci_virtio_console.c

582 ↗

(On Diff #42397)

assert() can be optimized away, this should be a full and proper error return check on n.

[araujo]

usr.sbin/bhyve/pci_virtio_console.c

582 ↗

(On Diff #42397)

Did you read the summary of this review?

[rgrimes]

usr.sbin/bhyve/pci_virtio_console.c

582 ↗

(On Diff #42397)

Yes.
If assert() goes away you are failing to check the return value from vq_getchain, and n becomes an unused variable.
Understand that assert() should never be used to test for an error condition from a function,
as that code can go away if #define NODEBUG is done.

Though vg_getchain may print a diagnostic, the above code compiled #NODEBUG shall continue on past
the point at which we should of checked the error and taken appropriate action (probably exit() in this case.

[araujo]

usr.sbin/bhyve/pci_virtio_console.c

582 ↗

(On Diff #42397)

In this case, the issue is all over the place! Do you have a better approach to solve it? I'm really wondering!

[avg]

usr.sbin/bhyve/pci_virtio_console.c

582 ↗

(On Diff #42397)

I read the summary too, but it is very terse.
Could you please explain what you want to achieve with assert?
I think that explanation is a missing link in the summary.

[araujo]

usr.sbin/bhyve/pci_virtio_console.c

582 ↗

(On Diff #42397)

Fail as soon as possible to be able to identify a possible bug if that exists, or otherwise we don't need to have that n variable. That is my main approach!

[avg]

usr.sbin/bhyve/pci_virtio_console.c

582 ↗

(On Diff #42397)

Thank you for clarifying. So, you want to silence the warning about n being write-only and you use assert to do that and to get an additional benefit of sanity checking.
This looks fine to me.

However, the warning _may_ come back if you do a non-debug build, the one with NDEBUG, see assert(3).
I am not sure if compilers are smart enough to not warn about a use in a macro that may or may not be compiled out.
That's what @rgrimes warned about.

Additionally, the assert should probably check for n being zero as that's another possible but impossible (because vq_has_descs is true) return value. In both cases, -1 and 0, idx would not be set and, thus, a bogus value would get passed to vq_relchain.

[araujo]

Address @avg concern about idx becomes 0 and be passed to vq_relchain and perhaps
end up on vq_endchains with a bogus values.

[avg]

usr.sbin/bhyve/pci_virtio_console.c

582 ↗

(On Diff #42397)

I think that 'n > 0' would look more "idiomatic", but that's just nitpicking.

[jhb]

Normally if you weren't planning to do a runtime check you would just not have 'n' at all. What you really kind of want is a working version of 'assert(vq_getchain(...) >=1)' but you can't do that safely. Andriy is right that the warning will be back with a NDEBUG build (but we don't define NDEBUG in FreeBSD builds, even for releases). The current version is perhaps the least evil. Other ideas I considered:

if (vq_getchain(...) < 1)
    abort();

if (vq_getchain(...) < 1)
   assert(false);

[araujo]

In D15388#325307, @jhb wrote:

Normally if you weren't planning to do a runtime check you would just not have 'n' at all. What you really kind of want is a working version of 'assert(vq_getchain(...) >=1)' but you can't do that safely. Andriy is right that the warning will be back with a NDEBUG build (but we don't define NDEBUG in FreeBSD builds, even for releases). The current version is perhaps the least evil. Other ideas I considered:

if (vq_getchain(...) < 1)
    abort();

if (vq_getchain(...) < 1)
   assert(false);

Hi @jhb , thanks for the explanation as well as the other 2 ideas.