bhyve/virtio: Fix comparison of integer expressions of different signedness

[rosenfeld_grumpf.hope-2000.org]

Summary

It's a bit silly to have iov_to_buf() and buf_to_iov() return a ssize_t
to begin with, just to be able to return -1 for error. Change this to
size_t and use 0 as an error indicator, which won't require any changes
to the code using these functions.

While here, switch iov_to_buf() to use reallocf() instead of realloc().

Fixes: 2a514d377b37 ("bhyve/virtio-scsi: Preallocate all I/O requests")

[jhb]

The error of having assert() invoke code that is required (such that the code will just flat out break if you build with NDEBUG) still needs fixing. Siva's version fixes that bigger bug. I think It's fine if buf_to_iov() returns size_t, but I think the -1 is still useful from iov_to_buf to avoid ambiguity for a return value of 0. reallocf() is also a good fix.

[rosenfeld_grumpf.hope-2000.org]

Don't invoke iov_to_buf() from an assert() expression.

[rosenfeld_grumpf.hope-2000.org]

In D55800#1276422, @jhb wrote:

The error of having assert() invoke code that is required (such that the code will just flat out break if you build with NDEBUG) still needs fixing. Siva's version fixes that bigger bug. I think It's fine if buf_to_iov() returns size_t, but I think the -1 is still useful from iov_to_buf to avoid ambiguity for a return value of 0. reallocf() is also a good fix.

I didn't realize NDEBUG was still a thing, but thanks for pointing it out.

Given that iov_to_buf() is only used in pci_virtio_scsi.c in two places, and neither code path even checks for -1 or < 0, I think all the extra hassle using a ssize_t isn't worth it. I could imagine it to make sense to just assert(total > 0) instead, though.

[jhb]

usr.sbin/bhyve/iov.c

119–120

You don't really need this special case I think as the existing code will handle a size of zero already.

usr.sbin/bhyve/pci_virtio_scsi.c

750 ↗

(On Diff #173522)

I think this probably needs to be a separate fix/commit IMO.

[rosenfeld_grumpf.hope-2000.org]

Address review by jhb.

[rosenfeld_grumpf.hope-2000.org]

usr.sbin/bhyve/iov.c

119–120

I couldn't find anything quickly in the realloc() manpage about how it handles a size of zero, that's why I added it. I now checked the POSIX documents and they clearly state that size of zero with ptr being non-NULL will mean ptr is freed, so I don't need to check total before reallocf().

But POSIX also states that in that case it may return NULL or any other valid pointer that may be passed to free(). I guess that means I can refactor this somewhat, but I can't assume realloc() to return NULL when a zero size is passed. (I generally assume reallocf() will work exactly the same.)

[rosenfeld_grumpf.hope-2000.org]

Fix commit message.

[des]

usr.sbin/bhyve/iov.c

121

The added check only serves to confuse the issue. The outcome will be exactly the same.

[rosenfeld_grumpf.hope-2000.org]

usr.sbin/bhyve/iov.c

121

As I mentioned in the comment above, POSIX says that it's up to the implementation whether realloc() returns NULL when given a zero size. It may also return a pointer which can be freed. I'd prefer not to make an assumption on this implementation detail here.

[des]

usr.sbin/bhyve/iov.c

121

I know. It makes no difference.

[rosenfeld_grumpf.hope-2000.org]

usr.sbin/bhyve/iov.c

121

Sure, but it's still clearer to check for this case explicitly.

[des]

usr.sbin/bhyve/iov.c

121

Clarity is in the eye of the beholder.

Right now you have the following possibilities:

• total > 0, allocation succeeds: iov_to_buf() successful, returns non-zero.
• total > 0, allocation fails: *buf is clobbered, original buffer is freed, iov_to_buf() returns zero.
• total == 0: *buf is clobbered with either NULL or a non-NULL pointer to a zero-length object, original buffer is freed, iov_to_buf() returns zero.

I don't find that clear at all.

In fact I think this entire change is a bad idea. The two functions are fine as they are. You are giving up the ability to tell an error apart from a zero-length iov for the sake of avoiding a single cast.

[rosenfeld_grumpf.hope-2000.org]

usr.sbin/bhyve/iov.c

121

The last two cases are actually the same: zero is returned, the original buffer is freed, and *buf contains a pointer that can be passed to free(). There exactly two code paths that use of this function, both of them in pci_virtio_scsi.c. One of them doesn't even check the returned value, and it would break in interesting ways if it was -1, but it does the right thing in case of 0. The other is the assertion you've seen in the build error.

That being said, being fast and loose with signedness on a value that is naturally unsigned just to be able indicate an error by returning -1, while at the same time instructing the compiler to error out on mismatched signedness comparisons, and then papering over it with an explicit cast isn't really all that great either, especially not when the error case returning -1 isn't all that interesting or necessary to begin with.

[jhb]

To be fair, I think the cast is required in the one place the function is used, so I think it is fine to make the function match the one use case.

usr.sbin/bhyve/iov.c

119–120

This is due to malloc(0) being special.

However, my point is that the rest of the function (including the for loop below that iterates over the iovec) will handle a size of zero just fine. There's no need to handle a size of 0 as a special case.