Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers/vhost: fix compile error while get vhost status. #15498

Merged
merged 2 commits into from
Jan 11, 2025

Conversation

terry0012
Copy link
Contributor

@terry0012 terry0012 commented Jan 11, 2025

Summary

vhost compile error as below:

vhost/vhost-rng.c:154:9: error: too few arguments to function 'virtio_create_virtqueues'
      154 |   ret = vhost_create_virtqueues(hdev, 0, 1, vqnames, callback);
vhost/vhost.c: In function 'vhost_status_driver_ok': vhost/vhost.c:86:20: error: too few arguments to function 'virtio_get_status'
   86 |   uint8_t status = vhost_get_status(hdev);

Impact

vhost

Testing

CI

vhost/vhost.c: In function 'vhost_status_driver_ok':
vhost/vhost.c:86:20: error: too few arguments to function 'virtio_get_status'
   86 |   uint8_t status = vhost_get_status(hdev);

Signed-off-by: rongyichang <rongyichang@xiaomi.com>
@github-actions github-actions bot added Area: Drivers Drivers issues Size: XS The size of the change in this PR is very small labels Jan 11, 2025
vhost/vhost-rng.c:154:9: error: too few arguments to function 'virtio_create_virtqueues'
  154 |   ret = vhost_create_virtqueues(hdev, 0, 1, vqnames, callback);

Signed-off-by: rongyichang <rongyichang@xiaomi.com>
@github-actions github-actions bot added Size: S The size of the change in this PR is small and removed Size: XS The size of the change in this PR is very small labels Jan 11, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 11, 2025

[Experimental Bot, please feedback here]

Yes, this PR likely meets the basic NuttX requirements in form, but is lacking crucial information to be considered complete. Here's why and what's missing:

Strengths:

  • It identifies a clear problem (compile errors in vhost).
  • It points to the specific location of the errors.

Weaknesses & Missing Information:

  • Summary: While the errors are shown, the summary lacks a description of why the error occurs and how the PR fixes it. Is virtio_create_virtqueues missing arguments in its definition? Did its signature change? What change was made to fix it? Same questions apply to virtio_get_status.
  • Impact: Simply stating "vhost" is insufficient. Be specific. Does this affect all vhost users? Specific boards? Does it fix a bug, add a feature, or improve performance?
  • Testing: "CI" is not enough. While CI testing is important, you must provide local testing results. List your host OS, compiler, target architecture, and board configuration. The "before" logs should ideally show the compile error. The "after" logs should show successful compilation (and ideally, some runtime test demonstrating the fix). Empty log sections are unacceptable.

To make this PR acceptable, you need to:

  1. Explain the root cause of the compile errors: Why were the calls to virtio_create_virtqueues and virtio_get_status incorrect?
  2. Detail the solution: Precisely what code changes did you make to fix the errors?
  3. Specify the Impact: Which vhost users/boards/features are affected? Is this a bug fix, new feature, or something else?
  4. Provide real testing logs: Show the compile errors before your change and the successful compilation after your change. Include details about your testing environment (host & target). Ideally, include a runtime test demonstrating the corrected behavior.

By adding this missing information, you'll significantly improve the quality of your PR and make it much easier for reviewers to understand and approve.

@xiaoxiang781216 xiaoxiang781216 merged commit 39780fd into apache:master Jan 11, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Drivers Drivers issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants