Re: [PATCHv2 2/5] qemu_capabilities: Add support for nvme-ns bus capabilities
On Fri, May 09, 2025 at 08:34:42 -0000, ray wang wrote:
>
> Generally patches adding a capability flag should only add the
> capability flag. Thus you'll need to split this patch.
>
> 1) capability addition
> - move it ahead to the beginning of the series (doing that will
> allow you to do capability checks when adding the code instead
> when adding the capability
> - note the full flag name (QEMU_CAPS...) in the summary line of the
> commit message
> 2) domain caps addition
> - that ought to be done after the code addition is compelete, thus
> after you add the XML schema bits and implementation
>
Okay, that's a very reasonable suggestion.
That is actually a requirement. Patch adding capability flags is kept
clean and minimal in cases when it needs to be backported as it greatly
simplifies that work.
> All of the above is suspicious. The test files were forgotten
after we
> dropped support for the qemu versions mentioned in them. How did you
> modify these?
>
> Interestingly there are more of the files for unsupported versions but
> you didnt modify all of them.
>
> Anyways I'll send a patch deleting those.
>
Modifying these test files is necessary to ensure the correctness of the unit tests.
In the context you trimmed I was refering to test files which were not
actually used by unit tests.
Thus while your changes were most likely correct (we can't know because
the the files were not used) it was suspicious to me that you've
modified them because they would not cause a test failure.
The changes I made were based on the feedback from the unit tests :)
I've seen your patch that removes the deprecated XML—great job! I’ll rebase my patch
on top of it, but the other XML test files still need to be updated to include the nvme-ns
related content.
Sure but those are in fact checked against the existing test data so it
is needed and I didn't object to any of that.
Rebasing on top of the current tree and thus dropping the modifications
to the unused files will satisfy my comment.