refactor(virtio): replace per-device switch-case with ops table dispatch - #112
Merged
Merged
Conversation
agicy
force-pushed
the
ops-table
branch
2 times, most recently
from
August 12, 2026 05:55
915e393 to
b83c563
Compare
agicy
marked this pull request as ready for review
August 12, 2026 06:01
li041
reviewed
Aug 19, 2026
li041
reviewed
Aug 19, 2026
li041
reviewed
Aug 19, 2026
Contributor
|
Thank you for this contribution. The table-driven device operations make the VirtIO framework easier to maintain and extend, while the per-device parsing and lifecycle hooks provide a much cleaner separation of responsibilities. I also appreciate the effort put into improving initialization failure handling and resource cleanup. Overall, this is a valuable refactor and a solid foundation for adding future VirtIO device types. |
Add struct virtio_device_ops (type/features/num_queues/queue_max_size/ init/close/reset/status_changed/notify_handlers) in virtio.h, and one const ops instance per device defined in its own .c, together with the typed init params structs and do_init wrappers that unpack them. Purely additive — the table is consumed in the following commit.
Replace the switch-cases in create_virtio_device and init_virtio_queue with table lookups: features, queue count/size and per-queue notify handlers now come from the device's ops entry, init goes through ops->init with params, and the error path tears down via ops->close instead of a bare free(). The queue setup loop is table-driven too (ops->num_queues / queue_max_size / notify_handlers). Drop the now-redundant manual vdev->virtio_close / status_changed assignments in device inits — the table wires them up centrally.
Add struct virtio_config_ops (parse/free) and move JSON parsing out of
create_virtio_device_from_json into each device file: blk's img, net's
tap + mac, gpu's width/height, scmi's clock/reset/power id arrays (via
scmi_dev_parse_* helpers). parse allocates a typed params struct that
create_virtio_device consumes and config_ops->free releases right after.
Also fix net MAC parsing to use parse_json_u8: the mac field is an
array of hex strings ("0x00", ...) and cJSON_IsNumber() is false for
strings.
NULL-guard every device close(): bail on NULL vdev, guard the per-dev state (dev->mtx/cond/img_fd), and null out freed pointers. Make all device init/notify/close/reset functions static and drop their declarations from the device headers — only the *_ops externs stay exported.
const-qualify the read-only data flow across the whole boundary: - config_ops.parse and every device parse_params take const cJSON * (scmi's void *json_array becomes const cJSON *, dropping the casts) - ops->init, create_virtio_device and create_virtio_device_from_json take const void *params / const cJSON *device_json - virtio-net: init_net_dev/open_tap/virtio_net_init take const inputs, so the (uint8_t *)/(char *) casts in net do_init disappear gpu: init_gpu_dev takes const GPURequestedState * and its stale non-static declaration is dropped from virtio_gpu.h — the definition was static since the encapsulation cleanup, so the header decl was both dead and (with VIRTIO_GPU=y) a static-after-extern compile error.
Unify the device-init failure contract across all devices: init attaches resources to dev as they are created and returns on any failure — ops->close on the create error path then cleans up whatever is attached (each close guards its fds and frees NULL-safe). - virtio-net: replace the byte-wise MAC copy in init_net_dev with a single memcpy. Drop the manual close/tapfd=-1 and free/in_iov=NULL bookkeeping on the set_nonblocking/add_event/iov failure paths — close() owns all of it. set_nonblocking failure now returns directly instead of proceeding to a pointless add_event(-1). - virtio-blk: attach img_fd to dev->img_fd right after open() so any later failure is cleaned up by ops->close; drop the redundant close(-1) on open failure and the manual close on fstat failure. - virtio-console: drop the manual close/state-reset bookkeeping on the slave_fd/set_nonblocking/add_event failure paths — close() owns master_fd, slave_keepalive_fd and event. - virtio-scmi: adopt the device-init contract — init attaches the (possibly partially built) dev to vdev->dev before any failure point, and the create error path runs ops->close, which is safe on any partial state (scmi_dev_free tolerates NULL id arrays, close skips a NULL dev). This removes the err_copy label and the manual frees from virtio_scmi_do_init; cleanup logic now lives in exactly one place. The three id arrays (clock/reset/power) were copied with the same calloc-check-memcpy block; extract scmi_copy_id_array() so the body shrinks to a 3-way helper call. Failure contract unchanged.
Contributor
|
LGTM. Thanks for your contribution! |
li041
approved these changes
Aug 19, 2026
agicy
added a commit
that referenced
this pull request
Aug 24, 2026
refactor(virtio): replace per-device switch-case with ops table dispatch
agicy
added a commit
that referenced
this pull request
Aug 24, 2026
refactor(virtio): replace per-device switch-case with ops table dispatch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace per-device switch-case across
create_virtio_device,init_virtio_queue,destroy_unpublished_virtio_device, andcreate_virtio_device_from_jsonwith two table-driven ops structs.This matches the VirtIO v1.4 device model. Each device type is a self-contained entity with its own feature bits (§2.2), virtqueue layout (§2.6), and lifecycle hooks covering driver status transitions (§2.1: DRIVER_OK, FEATURES_OK, FAILED), device reset (§2.4), and teardown. The framework no longer knows that blk needs an
img_path, net has amac, or scmi hasclock_ids- it just callsops->init(params).Adding a new device type (e.g. virtio-rng, virtio-balloon, virtio-fs) is now a matter of defining two ops structs in a new
.cfile and adding two lines to the lookup table invirtio.c. Zero framework changes.After this lands, #103 can rebase on top and replace its per-device switch-cases with
ops->init(vdev, params)/ops->close(vdev).