Re: [Qemu-devel] [PATCH v6 3/5] acpi: add build_append_namestring() helper

[Michael S. Tsirkin]

On Fri, Jan 30, 2015 at 12:46:41PM +0100, Igor Mammedov wrote:
> On Wed, 28 Jan 2015 17:16:17 +0200
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Wed, Jan 28, 2015 at 02:34:48PM +0000, Igor Mammedov wrote:
> > > Use build_append_namestring() instead of build_append_nameseg()
> > > So user won't have to care whether name is NameSeg, NamePath or
> > > NameString.
> > > 
> > > See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> > > 
> > 
> > typo
> > 
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > Reviewed-by: Claudio Fontana <address@hidden>
> > > Acked-by: Marcel Apfelbaum <address@hidden>
> > > ---
> > > v4:
> > >  * fix codding style of ACPI_DualNamePrefix & ACPI_MultiNamePrefix
> > >    it's imposible to use literals as suggested due to
> > >    g_array_append_val() requiring reference value
> > > 
> > > v3:
> > >  assert on wrong Segcount earlier and extend condition to
> > >   seg_count > 0 && seg_count <= 255
> > >  as suggested by Claudio Fontana <address@hidden>
> > > ---
> > >  hw/acpi/aml-build.c         | 92
> > > ++++++++++++++++++++++++++++++++++++++++++++-
> > > hw/i386/acpi-build.c        | 35 ++++++++---------
> > > include/hw/acpi/aml-build.h |  2 +- 3 files changed, 108
> > > insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index b28c1f4..ed4ab56 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.c
> > > @@ -52,7 +52,7 @@ void build_append_array(GArray *array, GArray
> > > *val) 
> > >  #define ACPI_NAMESEG_LEN 4
> > >  
> > > -void GCC_FMT_ATTR(2, 3)
> > > +static void GCC_FMT_ATTR(2, 3)
> > >  build_append_nameseg(GArray *array, const char *format, ...)
> > >  {
> > >      /* It would be nicer to use g_string_vprintf but it's only
> > > there in 2.22 */ @@ -71,6 +71,96 @@ build_append_nameseg(GArray
> > > *array, const char *format, ...) g_array_append_vals(array, "____",
> > > ACPI_NAMESEG_LEN - len); }
> > >  
> > > +static void
> > > +build_append_namestringv(GArray *array, const char *format,
> > > va_list ap) +{
> > > +    /* It would be nicer to use g_string_vprintf but it's only
> > > there in 2.22 */
> > > +    char *s;
> > > +    int len;
> > > +    va_list va_len;
> > > +    char **segs;
> > > +    char **segs_iter;
> > > +    int seg_count = 0;
> > > +
> > > +    va_copy(va_len, ap);
> > > +    len = vsnprintf(NULL, 0, format, va_len);
> > > +    va_end(va_len);
> > > +    len += 1;
> > > +    s = g_new(typeof(*s), len);
> > > +
> > > +    len = vsnprintf(s, len, format, ap);
> > > +
> > > +    segs = g_strsplit(s, ".", 0);
> > > +    g_free(s);
> > > +
> > > +    /* count segments */
> > > +    segs_iter = segs;
> > > +    while (*segs_iter) {
> > > +        ++segs_iter;
> > > +        ++seg_count;
> > >
> > How about we split out formatting?
> > Make +build_append_namestringv acceptconst char **segs, int nsegs,
> > put all code above to build_append_namestring.
> Can't do this, AML API calls which accept format string will
> use build_append_namestringv()

OK but I still think it's a good idea to split this out,
and have a static function that accepts an array of segments.