bhyve/virtio: Fix comparison of integer expressions of different signedness
Needs ReviewPublic
Actions

Authored by rosenfeld_grumpf.hope-2000.org on Tue, Mar 10, 2:39 PM.

Details

Reviewers

markj
emaste
siva
jhb

Group Reviewers

bhyve

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")

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 71317
Build 68200: arc lint + arc unit

Event Timeline

rosenfeld_grumpf.hope-2000.org created this revision.Tue, Mar 10, 2:39 PM

Herald added a reviewer: bhyve. · View Herald TranscriptTue, Mar 10, 2:39 PM

Herald added subscribers: bcran, rgrimes, imp. · View Herald Transcript

rosenfeld_grumpf.hope-2000.org requested review of this revision.Tue, Mar 10, 2:39 PM

Harbormaster completed remote builds in B71314: Diff 173509.Tue, Mar 10, 2:39 PM

rosenfeld_grumpf.hope-2000.org added reviewers: markj, emaste, siva.Tue, Mar 10, 2:40 PM

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.

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

Harbormaster completed remote builds in B71317: Diff 173522.Tue, Mar 10, 4:57 PM

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 accepted this revision.Tue, Mar 10, 6:58 PM

jhb added inline comments.

usr.sbin/bhyve/iov.c
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	I think this probably needs to be a separate fix/commit IMO.

This revision is now accepted and ready to land.Tue, Mar 10, 6:58 PM

Address review by jhb.

This revision now requires review to proceed.Tue, Mar 10, 7:17 PM

Harbormaster completed remote builds in B71320: Diff 173527.Tue, Mar 10, 7:17 PM

rosenfeld_grumpf.hope-2000.org added inline comments.Tue, Mar 10, 7:19 PM

usr.sbin/bhyve/iov.c
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.)