[Qemu-devel] [for-1.7] hw/i386/acpi-build.c vs glib-2.12

[Qemu-devel] [for-1.7] hw/i386/acpi-build.c vs glib-2.12

[Richard Henderson]

hw/i386/acpi-build.c:294:5: error: implicit declaration of function
‘g_string_vprintf’ [-Werror=implicit-function-declaration]
     g_string_vprintf(s, format, args);

Introduced in 2.14.


hw/i386/acpi-build.c:427:5: error: implicit declaration of function
‘g_array_get_element_size’ [-Werror=implicit-function-declaration]
     return table->len * g_array_get_element_size(table);

Introduced in 2.22.


Our (self-)documented minimums are

if test "$mingw32" = yes; then
    # g_poll is required in order to integrate with the glib main loop.
    glib_req_ver=2.20
else
    glib_req_ver=2.12
fi


Within unix variants at least, vs(n)printf is likely to be much more portable
than the glib function.  I suspect MinGW has it as well, though I've not checked.

As for g_array_get_element_size, aren't all of your tables element size 1?
That's all I can see from acpi_build_tables_init, though I admit to not digging
deeper.



r~

Re: [Qemu-devel] [for-1.7] hw/i386/acpi-build.c vs glib-2.12

[Michael S. Tsirkin]

On Tue, Nov 19, 2013 at 06:49:40AM +1000, Richard Henderson wrote:
> hw/i386/acpi-build.c:294:5: error: implicit declaration of function
> ‘g_string_vprintf’ [-Werror=implicit-function-declaration]
>      g_string_vprintf(s, format, args);
> 
> Introduced in 2.14.
> 
> 
> hw/i386/acpi-build.c:427:5: error: implicit declaration of function
> ‘g_array_get_element_size’ [-Werror=implicit-function-declaration]
>      return table->len * g_array_get_element_size(table);
> 
> Introduced in 2.22.
> 
> 
> Our (self-)documented minimums are
> 
> if test "$mingw32" = yes; then
>     # g_poll is required in order to integrate with the glib main loop.
>     glib_req_ver=2.20
> else
>     glib_req_ver=2.12
> fi
> 
> 
> Within unix variants at least, vs(n)printf is likely to be much more portable
> than the glib function.

Hmm. The nice thing with the glib variant is that it allocated memory.
Our specific use is actually 4 bytes so yes, we can make do without.

>  I suspect MinGW has it as well, though I've not checked.
> 
> As for g_array_get_element_size, aren't all of your tables element size 1?
> That's all I can see from acpi_build_tables_init, though I admit to not digging
> deeper.
> 
> 
> 
> r~

There's one with size > 1, though that's not hard to change.

Re: [Qemu-devel] [for-1.7] hw/i386/acpi-build.c vs glib-2.12

[Michael S. Tsirkin]

On Tue, Nov 19, 2013 at 06:49:40AM +1000, Richard Henderson wrote:
> hw/i386/acpi-build.c:294:5: error: implicit declaration of function
> ‘g_string_vprintf’ [-Werror=implicit-function-declaration]
>      g_string_vprintf(s, format, args);
> 
> Introduced in 2.14.
> 
> 
> hw/i386/acpi-build.c:427:5: error: implicit declaration of function
> ‘g_array_get_element_size’ [-Werror=implicit-function-declaration]
>      return table->len * g_array_get_element_size(table);
> 
> Introduced in 2.22.
> 
> 
> Our (self-)documented minimums are
> 
> if test "$mingw32" = yes; then
>     # g_poll is required in order to integrate with the glib main loop.
>     glib_req_ver=2.20
> else
>     glib_req_ver=2.12
> fi
> 
> 
> Within unix variants at least, vs(n)printf is likely to be much more portable
> than the glib function.  I suspect MinGW has it as well, though I've not checked.
> 
> As for g_array_get_element_size, aren't all of your tables element size 1?
> That's all I can see from acpi_build_tables_init, though I admit to not digging
> deeper.
> 
> 
> 
> r~


Can you please confirm this fixes the issue for you?


diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 486e705..97e0fba 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -287,14 +287,17 @@ static inline void build_append_array(GArray *array, GArray *val)
 
 static void build_append_nameseg(GArray *array, const char *format, ...)
 {
-    GString *s = g_string_new("");
+    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
+    char s[] = "XXXX";
+    int len;
     va_list args;
 
     va_start(args, format);
     g_string_vprintf(s, format, args);
+    len = vsnprintf(s, sizeof s, format, args);
     va_end(args);
 
-    assert(s->len == 4);
+    assert(len == 4);
     g_array_append_vals(array, s->str, s->len);
     g_string_free(s, true);
 }
@@ -424,7 +427,10 @@ static inline void *acpi_data_push(GArray *table_data, unsigned size)
 
 static unsigned acpi_data_len(GArray *table)
 {
-    return table->len * g_array_get_element_size(table);
+#if GLIB_CHECK_VERSION(2, 14, 0)
+    assert(g_array_get_element_size(table) == 1);
+#endif
+    return table->len;
 }
 
 static void acpi_align_size(GArray *blob, unsigned align)

Re: [Qemu-devel] [for-1.7] hw/i386/acpi-build.c vs glib-2.12

[Peter Maydell]

On 21 November 2013 09:15, Michael S. Tsirkin <mst@redhat.com> wrote:
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 486e705..97e0fba 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -287,14 +287,17 @@ static inline void build_append_array(GArray *array, GArray *val)
>
>  static void build_append_nameseg(GArray *array, const char *format, ...)
>  {
> -    GString *s = g_string_new("");
> +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> +    char s[] = "XXXX";
> +    int len;
>      va_list args;
>
>      va_start(args, format);
>      g_string_vprintf(s, format, args);
> +    len = vsnprintf(s, sizeof s, format, args);
>      va_end(args);

...this patch doesn't seem to have removed the g_string_vprintf
call?

thanks
-- PMM

Re: [Qemu-devel] [for-1.7] hw/i386/acpi-build.c vs glib-2.12

[Michael S. Tsirkin]

On Thu, Nov 21, 2013 at 10:09:58AM +0000, Peter Maydell wrote:
> On 21 November 2013 09:15, Michael S. Tsirkin <mst@redhat.com> wrote:
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 486e705..97e0fba 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -287,14 +287,17 @@ static inline void build_append_array(GArray *array, GArray *val)
> >
> >  static void build_append_nameseg(GArray *array, const char *format, ...)
> >  {
> > -    GString *s = g_string_new("");
> > +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > +    char s[] = "XXXX";
> > +    int len;
> >      va_list args;
> >
> >      va_start(args, format);
> >      g_string_vprintf(s, format, args);
> > +    len = vsnprintf(s, sizeof s, format, args);
> >      va_end(args);
> 
> ...this patch doesn't seem to have removed the g_string_vprintf
> call?
> 
> thanks
> -- PMM

yes and I found more issues.
Pls don't test yet I'll post v2.