[PATCH v5] Add a comment in bios-tables-test.c to clarify the reason behind approach

Ani Sinha posted 1 patch 3 years, 7 months ago
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test checkpatch passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200929142501.1057-1-ani@anisinha.ca
Maintainers: Igor Mammedov <imammedo@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>
tests/qtest/bios-tables-test.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
[PATCH v5] Add a comment in bios-tables-test.c to clarify the reason behind approach
Posted by Ani Sinha 3 years, 7 months ago
A comment is added in bios-tables-test.c that explains the reasoning
behind the process of updating the ACPI table blobs when new tests are added
or old tests are modified or code is committed that affect tests. The
explanation would help future contributors follow the correct process when
making code changes that affect ACPI tables.

Signed-off-by: Ani Sinha <ani@anisinha.ca>
Acked-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/qtest/bios-tables-test.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

changelog:
v5: incorporated suggestion from eric.
v4: more updates as per Igor's suggestion. Dropped some comment lines. added
    ack'd by line.
v3: review feedback incorporared and actual comment in the code reworded.
v2: cosmetic - commit log reworded.

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 3c09b844f9..fc7aaaf82c 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -11,7 +11,7 @@
  */
 
 /*
- * How to add or update the tests:
+ * How to add or update the tests or commit changes that affect ACPI tables:
  * Contributor:
  * 1. add empty files for new tables, if any, under tests/data/acpi
  * 2. list any changed files in tests/qtest/bios-tables-test-allowed-diff.h
@@ -38,6 +38,11 @@
  *      $(SRC_PATH)/tests/data/acpi/rebuild-expected-aml.sh
  * 6. Now commit any changes to the expected binary, include diff from step 4
  *    in commit log.
+ *    Expected binary updates needs to be a separate patch from the code that
+ *    introduces changes to ACPI tables. It lets the maintainer drop
+ *    and regenerate binary updates in case of merge conflicts. Further, a code
+ *    change is easily reviewable but a binary blob is not (without doing a
+ *    disassembly).
  * 7. Before sending patches to the list (Contributor)
  *    or before doing a pull request (Maintainer), make sure
  *    tests/qtest/bios-tables-test-allowed-diff.h is empty - this will ensure
-- 
2.17.1


Re: [PATCH v5] Add a comment in bios-tables-test.c to clarify the reason behind approach
Posted by Ani Sinha 3 years, 6 months ago
Request to queue this patch with the next pull. 

> On Sep 29, 2020, at 7:55 PM, Ani Sinha <ani@anisinha.ca> wrote:
> 
> A comment is added in bios-tables-test.c that explains the reasoning
> behind the process of updating the ACPI table blobs when new tests are added
> or old tests are modified or code is committed that affect tests. The
> explanation would help future contributors follow the correct process when
> making code changes that affect ACPI tables.
> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> Acked-by: Igor Mammedov <imammedo@redhat.com>
> ---
> tests/qtest/bios-tables-test.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
> 
> changelog:
> v5: incorporated suggestion from eric.
> v4: more updates as per Igor's suggestion. Dropped some comment lines. added
>    ack'd by line.
> v3: review feedback incorporared and actual comment in the code reworded.
> v2: cosmetic - commit log reworded.
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 3c09b844f9..fc7aaaf82c 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -11,7 +11,7 @@
>  */
> 
> /*
> - * How to add or update the tests:
> + * How to add or update the tests or commit changes that affect ACPI tables:
>  * Contributor:
>  * 1. add empty files for new tables, if any, under tests/data/acpi
>  * 2. list any changed files in tests/qtest/bios-tables-test-allowed-diff.h
> @@ -38,6 +38,11 @@
>  *      $(SRC_PATH)/tests/data/acpi/rebuild-expected-aml.sh
>  * 6. Now commit any changes to the expected binary, include diff from step 4
>  *    in commit log.
> + *    Expected binary updates needs to be a separate patch from the code that
> + *    introduces changes to ACPI tables. It lets the maintainer drop
> + *    and regenerate binary updates in case of merge conflicts. Further, a code
> + *    change is easily reviewable but a binary blob is not (without doing a
> + *    disassembly).
>  * 7. Before sending patches to the list (Contributor)
>  *    or before doing a pull request (Maintainer), make sure
>  *    tests/qtest/bios-tables-test-allowed-diff.h is empty - this will ensure
> -- 
> 2.17.1
> 

Re: [PATCH v5] Add a comment in bios-tables-test.c to clarify the reason behind approach
Posted by Thomas Huth 3 years, 6 months ago
On 12/10/2020 16.07, Ani Sinha wrote:
> Request to queue this patch with the next pull. 

I can take it via qtest-next in case Michael / Igor do not plan to send a
pull request any time soon.

 Thomas


Re: [PATCH v5] Add a comment in bios-tables-test.c to clarify the reason behind approach
Posted by Ani Sinha 3 years, 6 months ago
On Mon, Oct 12, 2020 at 19:43 Thomas Huth <thuth@redhat.com> wrote:

> On 12/10/2020 16.07, Ani Sinha wrote:
> > Request to queue this patch with the next pull.
>
> I can take it via qtest-next in case Michael / Igor do not plan to send a
> pull request any time soon.


Michael or Igor, care to comment?


>
>  Thomas
>
>
Re: [PATCH v5] Add a comment in bios-tables-test.c to clarify the reason behind approach
Posted by Thomas Huth 3 years, 6 months ago
On 15/10/2020 13.28, Ani Sinha wrote:
> 
> 
> On Mon, Oct 12, 2020 at 19:43 Thomas Huth <thuth@redhat.com
> <mailto:thuth@redhat.com>> wrote:
> 
>     On 12/10/2020 16.07, Ani Sinha wrote:
>     > Request to queue this patch with the next pull.
> 
>     I can take it via qtest-next in case Michael / Igor do not plan to send a
>     pull request any time soon.
> 
> 
> Michael or Igor, care to comment?

FYI, patch got merged:

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=07f5903c852de10fa7

 Thomas


Re: [PATCH v5] Add a comment in bios-tables-test.c to clarify the reason behind approach
Posted by Ani Sinha 3 years, 6 months ago
On Thu, Oct 15, 2020 at 7:29 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 15/10/2020 13.28, Ani Sinha wrote:
> >
> >
> > On Mon, Oct 12, 2020 at 19:43 Thomas Huth <thuth@redhat.com
> > <mailto:thuth@redhat.com>> wrote:
> >
> >     On 12/10/2020 16.07, Ani Sinha wrote:
> >     > Request to queue this patch with the next pull.
> >
> >     I can take it via qtest-next in case Michael / Igor do not plan to send a
> >     pull request any time soon.
> >
> >
> > Michael or Igor, care to comment?
>
> FYI, patch got merged:
>
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=07f5903c852de10fa7

Thanks!

Re: [PATCH v5] Add a comment in bios-tables-test.c to clarify the reason behind approach
Posted by Michael S. Tsirkin 3 years, 6 months ago
On Thu, Oct 15, 2020 at 04:58:37PM +0530, Ani Sinha wrote:
> 
> 
> On Mon, Oct 12, 2020 at 19:43 Thomas Huth <thuth@redhat.com> wrote:
> 
>     On 12/10/2020 16.07, Ani Sinha wrote:
>     > Request to queue this patch with the next pull.
> 
>     I can take it via qtest-next in case Michael / Igor do not plan to send a
>     pull request any time soon.
> 
> 
> Michael or Igor, care to comment?

Go ahead

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> 
> 
> 
>      Thomas
> 
>