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

[Igor Mammedov <imammedo@redhat.com>]

On Sat, 31 Jan 2015 19:05:47 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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" <mst@redhat.com> 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 <imammedo@redhat.com>
> > > > Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> > > > Acked-by: Marcel Apfelbaum <marcel@redhat.com>
> > > > ---
> > > > 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 <claudio.fontana@huawei.com>
> > > > ---
> > > >  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.
It would be a static function with a single call site from another
static function. I see no point is doing it, on contrary it would be
confusing why do we have function if we do not reuse it.
I'd split out the moment we actually need it.



> 
>