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

Add support for newer floppy device after Qemu 5.1.0 #3719

Merged

Conversation

pevogam
Copy link
Contributor

@pevogam pevogam commented Jul 13, 2023

We keep compatibility with old floppy devices but add the new floppy support to re-enable unattended installs of older windows machines.

@pevogam
Copy link
Contributor Author

pevogam commented Jul 13, 2023

This pull request will close #3429 once reviewed, fully tested, and merged.

@pevogam
Copy link
Contributor Author

pevogam commented Jul 13, 2023

@luckyh @PaulYuuu This pull request hasn't been tested yet and the current is only an initial push where I would appreciate a generic superficial review that focuses on whether I use the correct QDevice instantiation. So far I mostly guessed all of this looking at the API but if this is a good direction and you don't have any big recommendation I will test this with both older and newer Qemu version next. Any feedback on my guesswork is highly appreciated.

Copy link
Contributor

@luckyh luckyh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @pevogam , thank you for fixing the issue! However, I have some concerns regarding the current approach.

virttest/qemu_devices/qcontainer.py Outdated Show resolved Hide resolved
virttest/qemu_devices/qcontainer.py Outdated Show resolved Hide resolved
virttest/qemu_devices/qcontainer.py Outdated Show resolved Hide resolved
@pevogam pevogam changed the title Add support for newer floppy device after Qemu 6.1.0 Add support for newer floppy device after Qemu 5.1.0 Jul 23, 2023
@pevogam
Copy link
Contributor Author

pevogam commented Jul 24, 2023

Alright, I tested this with Qemu 5.1.0 and so far it looks good, @luckyh let me know if it addresses your concerns.

@pevogam pevogam force-pushed the floppy-device-capability branch 2 times, most recently from 42ea6e2 to 0ba466e Compare August 4, 2023 19:26
@pevogam
Copy link
Contributor Author

pevogam commented Aug 4, 2023

Hi @luckyh I integrated your change and tested this functionally but the CI keeps failing even after rebase. The only problems I see there (other than numberous info messages) are of the types:

/home/runner/work/avocado-vt/avocado-vt/virttest/cartesian_config.py:1815:32: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
...
/home/runner/work/avocado-vt/avocado-vt/virttest/unittest_utils/mock.py:59:16: E721

Do you happen to know if the CI currently passes on the master branch? I don't see how we would have changed anything regarding the unittest_utils or the cartesian_config above.

@luckyh
Copy link
Contributor

luckyh commented Aug 7, 2023

Do you happen to know if the CI currently passes on the master branch? I don't see how we would have changed anything regarding the unittest_utils or the cartesian_config above.

@pevogam looks the issue has already been fixed by #3734 , so would you please try to rebase the code and push it one more time? thanks!

@pevogam
Copy link
Contributor Author

pevogam commented Aug 7, 2023

Do you happen to know if the CI currently passes on the master branch? I don't see how we would have changed anything regarding the unittest_utils or the cartesian_config above.

@pevogam looks the issue has already been fixed by #3734 , so would you please try to rebase the code and push it one more time? thanks!

Alright done and static checks are now passing.

Copy link
Contributor

@luckyh luckyh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@luckyh
Copy link
Contributor

luckyh commented Aug 8, 2023

@YongxueHong would you mind to review this PR? thanks!

Copy link
Contributor

@YongxueHong YongxueHong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@YongxueHong
Copy link
Contributor

@luckyh @PaulYuuu This pull request hasn't been tested yet and the current is only an initial push where I would appreciate a generic superficial review that focuses on whether I use the correct QDevice instantiation. So far I mostly guessed all of this looking at the API but if this is a good direction and you don't have any big recommendation I will test this with both older and newer Qemu version next. Any feedback on my guesswork is highly appreciated.

Hi @PaulYuuu
Would you mind reviewing this PR?
Thanks in advance.

We keep compatibility with old floppy devices but add the new floppy
support to re-enable unattended installs of older windows machines.

Signed-off-by: Plamen Dimitrov <[email protected]>
Co-authored-by: Xu Han <[email protected]>
@pevogam
Copy link
Contributor Author

pevogam commented Aug 9, 2023

@luckyh @PaulYuuu This pull request hasn't been tested yet and the current is only an initial push where I would appreciate a generic superficial review that focuses on whether I use the correct QDevice instantiation. So far I mostly guessed all of this looking at the API but if this is a good direction and you don't have any big recommendation I will test this with both older and newer Qemu version next. Any feedback on my guesswork is highly appreciated.

Hi @PaulYuuu Would you mind reviewing this PR? Thanks in advance.

@PaulYuuu I have adapted your changes and tested them - everything works fine.

@YongxueHong YongxueHong merged commit d001933 into avocado-framework:master Aug 10, 2023
49 checks passed
@pevogam pevogam deleted the floppy-device-capability branch August 22, 2023 18:04
@pevogam pevogam mentioned this pull request Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants