bhyve/virtio: rework iovec handling functions for efficiency and clarity

[rosenfeld_grumpf.hope-2000.org]

Summary

Add check_iov_len() to check whether an iovec array covers a certain
length without the need to call count_iov() on the whole array first.

Garbage-collect the 'seek' argument to buf_to_iov(), used only by
virtio-scsi control request handling. The apparent benefit of using it
to copy only the final status byte instead of the whole TMF or AN
request (25 and 21 bytes, respectively) is dubious at best, given that
the extra code to handle this in buf_to_iov() allocates and frees a new
iovec array and uses seek_iov(), which traverses the whole array and
copies iovecs around.

Replace seek_iov() and truncate_iov(), used only by virtio-scsi, with
a single function split_iov() which combines the functionality of both
in a more efficient way:
While seek_iov() always copies all iovecs past the seek offset into a
new iovec array, split_iov() works in place and doesn't copy iovecs
unless actually necessary. By using split_iov(), we can avoid almost
all copying of iovecs in I/O handling code paths of virtio-scsi.

[rosenfeld_grumpf.hope-2000.org]

Keep in line with original code style.

[corvink]

Thanks for splitting your initial commit! Note that this can also be split into smaller commits to make it easier to read: add check_iov_len, update buf_to_iov and replace seek + truncate by split. While I'm personally preferring splitting the commits, I'd be fine keeping it as is too.

usr.sbin/bhyve/iov.c

35

It's already included by iov.h.

usr.sbin/bhyve/iov.h

40

Why removing the parameter names from existing functions?

usr.sbin/bhyve/pci_virtio_scsi.c

492–520

IMO, it's a bit confusing, that we're passing two pointers here which are related to each other. Wouldn't it make things more clear to pass a single iov pointer and then passing an niov_in and niov_out length to this function?

[rosenfeld_grumpf.hope-2000.org]

Rework split_iov() as requested by Corvin.

[rosenfeld_grumpf.hope-2000.org]

usr.sbin/bhyve/iov.c

35

I think that to prevent header dependency issues, every source or header file should always include every header file whose definitions and declarations it uses directly. This file uses now uses the standard bool type and thus needs to include stdbool.h.

usr.sbin/bhyve/iov.h

40

Given that these parameter names in function prototypes are ignored by the compiler, not even causing a warning if the name is different from what the actual function implementation is using, they serve very little purpose if any purpose at all. I generally consider them noise that adds no value but clutters up the code and lengthens lines needlessly.

For this reason I don't add them for new functions or functions that I change, and in a case like this where it's relatively unintrusive I change the others, too, to make it all nice and consistent.

usr.sbin/bhyve/pci_virtio_scsi.c

492–520

I reworked it to return the data_iov pointer instead and streamlined the split_iov() code somewhat. Please let me know what you think?