[Qemu-devel] [PATCH] bios-tables-test: delete a assert about block name

[Qemu-devel] [PATCH] bios-tables-test: delete a assert about block name

[wei.yang]

From: Wei Yang <wei.yang@ucloud.cn>

The assert would be touched when the version of acpica is greater than or
equal to 20160318. its reason is that "Disasembler: Update a couple
output items(commit id: 1ecbb3d)" is introduced by Robert, the patch
emits the AML filename as a zero-length string, and allows the compiler
to create the name later.

Signed-off-by: Yang, Wei <wei.yang@ucloud.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Igor Mammedov <imammedo@redhat.com>
CC: qemu-devel@nongnu.org
---
 tests/bios-tables-test.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 88dbf97853..d2ab073848 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -350,7 +350,6 @@ static GString *normalize_asl(gchar *asl_code)
     /* strip def block name (it has file path in it) */
     if (g_str_has_prefix(asl->str, DEF_BLOCK)) {
         block_name = g_strstr_len(asl->str, asl->len, BLOCK_NAME_END);
-        g_assert(block_name);
         asl = g_string_erase(asl, 0,
                              block_name + sizeof(BLOCK_NAME_END) - asl->str);
     }
-- 
2.11.0

Re: [Qemu-devel] [PATCH] bios-tables-test: delete a assert about block name

[Michael S. Tsirkin]

On Mon, Aug 14, 2017 at 04:48:38PM +0800, wei.yang@ucloud.cn wrote:
> From: Wei Yang <wei.yang@ucloud.cn>
> 
> The assert would be touched when the version of acpica is greater than or
> equal to 20160318. its reason is that "Disasembler: Update a couple
> output items(commit id: 1ecbb3d)" is introduced by Robert, the patch
> emits the AML filename as a zero-length string, and allows the compiler
> to create the name later.

So how does the output look then? Do you mean there's no comma then?


> 
> Signed-off-by: Yang, Wei <wei.yang@ucloud.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Igor Mammedov <imammedo@redhat.com>
> CC: qemu-devel@nongnu.org
> ---
>  tests/bios-tables-test.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 88dbf97853..d2ab073848 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -350,7 +350,6 @@ static GString *normalize_asl(gchar *asl_code)
>      /* strip def block name (it has file path in it) */
>      if (g_str_has_prefix(asl->str, DEF_BLOCK)) {
>          block_name = g_strstr_len(asl->str, asl->len, BLOCK_NAME_END);
> -        g_assert(block_name);
>          asl = g_string_erase(asl, 0,
>                               block_name + sizeof(BLOCK_NAME_END) - asl->str);

The pointer math here won't do the right thing then, will it?

>      }
> -- 
> 2.11.0
> 

Re: [Qemu-devel] [PATCH] bios-tables-test: delete a assert about block name

[wyang]

On 2017年08月14日 23:58, Michael S. Tsirkin wrote:
> On Mon, Aug 14, 2017 at 04:48:38PM +0800, wei.yang@ucloud.cn wrote:
>> From: Wei Yang <wei.yang@ucloud.cn>
>>
>> The assert would be touched when the version of acpica is greater than or
>> equal to 20160318. its reason is that "Disasembler: Update a couple
>> output items(commit id: 1ecbb3d)" is introduced by Robert, the patch
>> emits the AML filename as a zero-length string, and allows the compiler
>> to create the name later.
> So how does the output look then? Do you mean there's no comma then?

sorry for confusion, more clearly,  since the AML filename is a 
zero-length string, so the output of dsl file looks like this after
executing iasl command to disassemble AML file to DSL file


DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)

the block name is NULL string.

If the version of iasl command is less than 20160318, the output looks like

DefinitionBlock ("/tmp/xxxx.aml", "DSDT", 1, "BOCHS ", "BXPCDSDT", 
0x00000001)

>
>
>> Signed-off-by: Yang, Wei <wei.yang@ucloud.com>
>> CC: Michael S. Tsirkin <mst@redhat.com>
>> CC: Igor Mammedov <imammedo@redhat.com>
>> CC: qemu-devel@nongnu.org
>> ---
>>   tests/bios-tables-test.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
>> index 88dbf97853..d2ab073848 100644
>> --- a/tests/bios-tables-test.c
>> +++ b/tests/bios-tables-test.c
>> @@ -350,7 +350,6 @@ static GString *normalize_asl(gchar *asl_code)
>>       /* strip def block name (it has file path in it) */
>>       if (g_str_has_prefix(asl->str, DEF_BLOCK)) {
>>           block_name = g_strstr_len(asl->str, asl->len, BLOCK_NAME_END);
>> -        g_assert(block_name);
>>           asl = g_string_erase(asl, 0,
>>                                block_name + sizeof(BLOCK_NAME_END) - asl->str);
> The pointer math here won't do the right thing then, will it?

Yes, I will fix it.

Thanks
Wei
>
>>       }
>> -- 
>> 2.11.0
>>

-- 
Wei Yang(杨伟)
Mobile: +86-13436682065
Email: wei.yang@ucloud.cn

Re: [Qemu-devel] [PATCH] bios-tables-test: delete a assert about block name

[Michael S. Tsirkin]

On Tue, Aug 15, 2017 at 10:21:08AM +0800, wyang wrote:
> On 2017年08月14日 23:58, Michael S. Tsirkin wrote:
> > On Mon, Aug 14, 2017 at 04:48:38PM +0800, wei.yang@ucloud.cn wrote:
> > > From: Wei Yang <wei.yang@ucloud.cn>
> > > 
> > > The assert would be touched when the version of acpica is greater than or
> > > equal to 20160318. its reason is that "Disasembler: Update a couple
> > > output items(commit id: 1ecbb3d)" is introduced by Robert, the patch
> > > emits the AML filename as a zero-length string, and allows the compiler
> > > to create the name later.
> > So how does the output look then? Do you mean there's no comma then?
> 
> sorry for confusion, more clearly,  since the AML filename is a zero-length
> string, so the output of dsl file looks like this after
> executing iasl command to disassemble AML file to DSL file
> 
> 
> DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)
> 
> the block name is NULL string.

It's an empty string, sure, but is it NULL?
I see:
block_name = g_strstr_len(asl->str, asl->len, BLOCK_NAME_END);

which looks for "," and assigns a pointer to it to block_name.

Is it actually NULL in your testing?

> If the version of iasl command is less than 20160318, the output looks like
> 
> DefinitionBlock ("/tmp/xxxx.aml", "DSDT", 1, "BOCHS ", "BXPCDSDT",
> 0x00000001)
> 
> > 
> > 
> > > Signed-off-by: Yang, Wei <wei.yang@ucloud.com>
> > > CC: Michael S. Tsirkin <mst@redhat.com>
> > > CC: Igor Mammedov <imammedo@redhat.com>
> > > CC: qemu-devel@nongnu.org
> > > ---
> > >   tests/bios-tables-test.c | 1 -
> > >   1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> > > index 88dbf97853..d2ab073848 100644
> > > --- a/tests/bios-tables-test.c
> > > +++ b/tests/bios-tables-test.c
> > > @@ -350,7 +350,6 @@ static GString *normalize_asl(gchar *asl_code)
> > >       /* strip def block name (it has file path in it) */
> > >       if (g_str_has_prefix(asl->str, DEF_BLOCK)) {
> > >           block_name = g_strstr_len(asl->str, asl->len, BLOCK_NAME_END);
> > > -        g_assert(block_name);
> > >           asl = g_string_erase(asl, 0,
> > >                                block_name + sizeof(BLOCK_NAME_END) - asl->str);
> > The pointer math here won't do the right thing then, will it?
> 
> Yes, I will fix it.
> 
> Thanks
> Wei
> > 
> > >       }
> > > -- 
> > > 2.11.0
> > > 
> 
> -- 
> Wei Yang(杨伟)
> Mobile: +86-13436682065
> Email: wei.yang@ucloud.cn
> 

Re: [Qemu-devel] [PATCH] bios-tables-test: delete a assert about block name

[wyang]

On 2017年08月15日 10:39, Michael S. Tsirkin wrote:
> On Tue, Aug 15, 2017 at 10:21:08AM +0800, wyang wrote:
>> On 2017年08月14日 23:58, Michael S. Tsirkin wrote:
>>> On Mon, Aug 14, 2017 at 04:48:38PM +0800, wei.yang@ucloud.cn wrote:
>>>> From: Wei Yang <wei.yang@ucloud.cn>
>>>>
>>>> The assert would be touched when the version of acpica is greater than or
>>>> equal to 20160318. its reason is that "Disasembler: Update a couple
>>>> output items(commit id: 1ecbb3d)" is introduced by Robert, the patch
>>>> emits the AML filename as a zero-length string, and allows the compiler
>>>> to create the name later.
>>> So how does the output look then? Do you mean there's no comma then?
>> sorry for confusion, more clearly,  since the AML filename is a zero-length
>> string, so the output of dsl file looks like this after
>> executing iasl command to disassemble AML file to DSL file
>>
>>
>> DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)
>>
>> the block name is NULL string.
> It's an empty string, sure, but is it NULL?
> I see:
> block_name = g_strstr_len(asl->str, asl->len, BLOCK_NAME_END);
>
> which looks for "," and assigns a pointer to it to block_name.
>
> Is it actually NULL in your testing?
Hmm, as I touched the assert, so I thought it is NULL, seems I SHOULD go 
on deeply digging into it. Thank MST  for your review.

Wei
>> If the version of iasl command is less than 20160318, the output looks like
>>
>> DefinitionBlock ("/tmp/xxxx.aml", "DSDT", 1, "BOCHS ", "BXPCDSDT",
>> 0x00000001)
>>
>>>
>>>> Signed-off-by: Yang, Wei <wei.yang@ucloud.com>
>>>> CC: Michael S. Tsirkin <mst@redhat.com>
>>>> CC: Igor Mammedov <imammedo@redhat.com>
>>>> CC: qemu-devel@nongnu.org
>>>> ---
>>>>    tests/bios-tables-test.c | 1 -
>>>>    1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
>>>> index 88dbf97853..d2ab073848 100644
>>>> --- a/tests/bios-tables-test.c
>>>> +++ b/tests/bios-tables-test.c
>>>> @@ -350,7 +350,6 @@ static GString *normalize_asl(gchar *asl_code)
>>>>        /* strip def block name (it has file path in it) */
>>>>        if (g_str_has_prefix(asl->str, DEF_BLOCK)) {
>>>>            block_name = g_strstr_len(asl->str, asl->len, BLOCK_NAME_END);
>>>> -        g_assert(block_name);
>>>>            asl = g_string_erase(asl, 0,
>>>>                                 block_name + sizeof(BLOCK_NAME_END) - asl->str);
>>> The pointer math here won't do the right thing then, will it?
>> Yes, I will fix it.
>>
>> Thanks
>> Wei
>>>>        }
>>>> -- 
>>>> 2.11.0
>>>>
>> -- 
>> Wei Yang(杨伟)
>> Mobile: +86-13436682065
>> Email: wei.yang@ucloud.cn
>>

-- 
Wei Yang(杨伟)
Mobile: +86-13436682065
Email: wei.yang@ucloud.cn

Re: [Qemu-devel] [PATCH] bios-tables-test: delete a assert about block name

[Michael S. Tsirkin]

On Tue, Aug 15, 2017 at 10:46:27AM +0800, wyang wrote:
> 
> 
> On 2017年08月15日 10:39, Michael S. Tsirkin wrote:
> > On Tue, Aug 15, 2017 at 10:21:08AM +0800, wyang wrote:
> > > On 2017年08月14日 23:58, Michael S. Tsirkin wrote:
> > > > On Mon, Aug 14, 2017 at 04:48:38PM +0800, wei.yang@ucloud.cn wrote:
> > > > > From: Wei Yang <wei.yang@ucloud.cn>
> > > > > 
> > > > > The assert would be touched when the version of acpica is greater than or
> > > > > equal to 20160318. its reason is that "Disasembler: Update a couple
> > > > > output items(commit id: 1ecbb3d)" is introduced by Robert, the patch
> > > > > emits the AML filename as a zero-length string, and allows the compiler
> > > > > to create the name later.
> > > > So how does the output look then? Do you mean there's no comma then?
> > > sorry for confusion, more clearly,  since the AML filename is a zero-length
> > > string, so the output of dsl file looks like this after
> > > executing iasl command to disassemble AML file to DSL file
> > > 
> > > 
> > > DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)
> > > 
> > > the block name is NULL string.
> > It's an empty string, sure, but is it NULL?
> > I see:
> > block_name = g_strstr_len(asl->str, asl->len, BLOCK_NAME_END);
> > 
> > which looks for "," and assigns a pointer to it to block_name.
> > 
> > Is it actually NULL in your testing?
> Hmm, as I touched the assert, so I thought it is NULL, seems I SHOULD go on
> deeply digging into it. Thank MST  for your review.
> 
> Wei
> > > If the version of iasl command is less than 20160318, the output looks like
> > > 
> > > DefinitionBlock ("/tmp/xxxx.aml", "DSDT", 1, "BOCHS ", "BXPCDSDT",
> > > 0x00000001)
> > > 
> > > > 
> > > > > Signed-off-by: Yang, Wei <wei.yang@ucloud.com>
> > > > > CC: Michael S. Tsirkin <mst@redhat.com>
> > > > > CC: Igor Mammedov <imammedo@redhat.com>
> > > > > CC: qemu-devel@nongnu.org
> > > > > ---
> > > > >    tests/bios-tables-test.c | 1 -
> > > > >    1 file changed, 1 deletion(-)
> > > > > 
> > > > > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> > > > > index 88dbf97853..d2ab073848 100644
> > > > > --- a/tests/bios-tables-test.c
> > > > > +++ b/tests/bios-tables-test.c
> > > > > @@ -350,7 +350,6 @@ static GString *normalize_asl(gchar *asl_code)
> > > > >        /* strip def block name (it has file path in it) */
> > > > >        if (g_str_has_prefix(asl->str, DEF_BLOCK)) {
> > > > >            block_name = g_strstr_len(asl->str, asl->len, BLOCK_NAME_END);
> > > > > -        g_assert(block_name);
> > > > >            asl = g_string_erase(asl, 0,
> > > > >                                 block_name + sizeof(BLOCK_NAME_END) - asl->str);
> > > > The pointer math here won't do the right thing then, will it?
> > > Yes, I will fix it.
> > > 
> > > Thanks
> > > Wei

BTW sizeof(BLOCK_NAME_END) is actually 2. I think it works
because there's a space after ,. It is better to replace it with
strlen so it will work without a space too.

> > > > >        }
> > > > > -- 
> > > > > 2.11.0
> > > > > 
> > > -- 
> > > Wei Yang(杨伟)
> > > Mobile: +86-13436682065
> > > Email: wei.yang@ucloud.cn
> > > 
> 
> -- 
> Wei Yang(杨伟)
> Mobile: +86-13436682065
> Email: wei.yang@ucloud.cn
> 

Re: [Qemu-devel] [PATCH] bios-tables-test: delete a assert about block name

[wyang]

On 2017年08月15日 10:49, Michael S. Tsirkin wrote:
> BTW sizeof(BLOCK_NAME_END) is actually 2. I think it works
> because there's a space after ,. It is better to replace it with
> strlen so it will work without a space too.

MST,

I double check the failure of assert, it has already been fixed by the 
following commit,

commit a3973f551dbee91f1f6f2c78e9942fb113b5d30b
Author: Marcel Apfelbaum <marcel@redhat.com>
Date:   Mon Apr 4 20:00:57 2016 +0300

     tests/bios-tables-test: fix assert

     Newer iasl does not add the aml file name to the Definition Block.
     See acpica tools commit  1ecbb3d5:
       "Emit the AMLFilename as a zero-length string. Allows the 
compiler to create
        the name later -- making it easier to rename the parent ASL 
(DSL) file."

     That causes an assert in acpi tests:
        tests/bios-tables-test.c:455:normalize_asl: assertion failed: 
(block_name)

     Fix it by striping the start of the definition block line until the 
first comma.
     The block name is always the first parameter and
     the grammar does not allow comma in between, so it is safe.

     Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
     Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
     Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


I am sorry for my fault,  my qemu version does not contain the patch.:-(

Seems the only place, which needs to be fixed, is to replace sizeof with 
strlen.  Just like what you said, it is not fault but just refinement.
If you need it, I am glad to send patch to fix it.

Wei

-- 
Wei Yang(杨伟)
Mobile: +86-13436682065
Email: wei.yang@ucloud.cn

Re: [Qemu-devel] [PATCH] bios-tables-test: delete a assert about block name

[Michael S. Tsirkin]

On Tue, Aug 15, 2017 at 11:53:29AM +0800, wyang wrote:
> Seems the only place, which needs to be fixed, is to replace sizeof with
> strlen.  Just like what you said, it is not fault but just refinement.
> If you need it, I am glad to send patch to fix it.
> 
> Wei

Sure.

> -- 
> Wei Yang(杨伟)
> Mobile: +86-13436682065
> Email: wei.yang@ucloud.cn
>