Re: [PATCH v3 1/5] device_tree: add qemu_fdt_setprop_strings() helper

[Andrew Jones]

On Thu, Jul 28, 2022 at 11:22:27AM +0200, Andrew Jones wrote:
> On Wed, Jul 27, 2022 at 11:39:01PM +0100, Ben Dooks wrote:
> > Add a helper to set a property from a set of strings
> > to reduce the following code:
> > 
> >     static const char * const clint_compat[2] = {
> >         "sifive,clint0", "riscv,clint0"
> >     };
> > 
> >     qemu_fdt_setprop_string_array(fdt, nodename, "compatible",
> >         (char **)&clint_compat, ARRAY_SIZE(clint_compat));
> > 
> > Signed-off-by: Ben Dooks <qemu@ben.fluff.org>
> > ---
> > v3;
> >  - fix return value for the call
> >  - add better help text
> > v2:
> >  - fix node/path in comment
> > ---
> >  include/sysemu/device_tree.h | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> > index ef060a9759..83bdfe390e 100644
> > --- a/include/sysemu/device_tree.h
> > +++ b/include/sysemu/device_tree.h
> > @@ -87,6 +87,25 @@ int qemu_fdt_setprop_string(void *fdt, const char 
> > *node_path,
> >  int qemu_fdt_setprop_string_array(void *fdt, const char *node_path,
> >                                    const char *prop, char **array, int len);
> >  
> > +/**
> > + * qemu_fdt_setprop_strings: set a property from a set of strings
> > + *
> > + * @fdt: pointer to the dt blob
> > + * @path: node name
> > + * @prop: property array
> > + *
> > + * This is a helper for the qemu_fdt_setprop_string_array() function
> > + * which takes a va-arg set of strings instead of having to setup a
> > + * single use string array.
> > + */
> > +#define qemu_fdt_setprop_strings(fdt, path, prop, ...)          \
> > +    ({ int __ret; do {                                          \
> > +        static const char * const __strs[] = { __VA_ARGS__ };   \
> > +        __ret = qemu_fdt_setprop_string_array(fdt, path, prop,  \
> > +                (char **)&__strs, ARRAY_SIZE(__strs));          \
> > +     } while(0); __ret; })
> 
> The do { } while (0) shouldn't be necessary inside the ({}), but I
> think we should change the places that are checking the return value
> of qemu_fdt_setprop_string_array() to not check the return value,
> because it will always be zero. qemu_fdt_setprop_string_array() calls
> qemu_fdt_setprop() which exits QEMU on error. Then, after there are
> no callers checking the return value we can change this macro to
> 
> #define qemu_fdt_setprop_strings(fdt, path, prop, ...)          \
>     do {                                                       \
>        static const char * const __strs[] = { __VA_ARGS__ };   \

Eh, I just realized I didn't notice the static the first time I read this
and so I copy+pasted here in my suggestion. Sorry about that.

Thanks,
drew


>        qemu_fdt_setprop_string_array(fdt, path, prop,          \
>                (char **)&__strs, ARRAY_SIZE(__strs));          \
>      } while(0)
> 
> 
> Thanks,
> drew