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 **)_compat, ARRAY_SIZE(clint_compat));
> > 
> > Signed-off-by: Ben Dooks 
> > ---
> > 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

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

[Ben Dooks]

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 **)_compat, ARRAY_SIZE(clint_compat));
> > 
> > Signed-off-by: Ben Dooks 
> > ---
> > 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

I think I did this due to the hw/ppc changes that /do/ check the
return code but are different enough to not be trivially changable.

I'll go back and make this the original do {} while() tongith and
post a v4 for people to look. The hw/ppc stuff can stay as is for
now.

Thank you for the review.

-- 
Ben Dooks, b...@fluff.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

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

[Andrew Jones]

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 **)_compat, ARRAY_SIZE(clint_compat));
> 
> Signed-off-by: Ben Dooks 
> ---
> 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__ };   \
   qemu_fdt_setprop_string_array(fdt, path, prop,  \
   (char **)&__strs, ARRAY_SIZE(__strs));  \
 } while(0)


Thanks,
drew

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

[Ben Dooks]

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 **)_compat, ARRAY_SIZE(clint_compat));

Signed-off-by: Ben Dooks 
---
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; })
+
+
 int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
  const char *property,
  const char *target_node_path);
-- 
2.35.1