wrappers should make access to [has]node_id fields more readable

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 follow up patches will use this wrappers
 v2:
    - add wrappers (Drew)
---
 include/sysemu/numa.h | 10 ++++++++++
 numa.c                |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 46ea6c7..98d01e6 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -35,4 +35,14 @@ uint32_t numa_get_node(ram_addr_t addr, Error **errp);
 /* on success returns node index in numa_info,
  * on failure returns nb_numa_nodes */
 int numa_get_node_for_cpu(int idx);
+
+static inline bool numa_has_node_id(const CPUArchIdList *possible_cpus, int idx)
+{
+    return possible_cpus->cpus[idx].props.has_node_id;
+}
+
+static inline int numa_node_id(const CPUArchIdList *possible_cpus, int idx)
+{
+    return possible_cpus->cpus[idx].props.node_id;
+}
 #endif
diff --git a/numa.c b/numa.c
index c7e3e0a..872ee0d 100644
--- a/numa.c
+++ b/numa.c
@@ -394,7 +394,7 @@ void parse_numa_opts(MachineState *ms)
 
         possible_cpus = mc->possible_cpu_arch_ids(ms);
         for (i = 0; i < possible_cpus->len; i++) {
-            if (possible_cpus->cpus[i].props.has_node_id) {
+            if (numa_has_node_id(possible_cpus, i)) {
                 break;
             }
         }
-- 
2.7.4

On Wed, May 03, 2017 at 02:57:06PM +0200, Igor Mammedov wrote:
> wrappers should make access to [has]node_id fields more readable
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  follow up patches will use this wrappers
>  v2:
>     - add wrappers (Drew)
> ---
>  include/sysemu/numa.h | 10 ++++++++++
>  numa.c                |  2 +-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 46ea6c7..98d01e6 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -35,4 +35,14 @@ uint32_t numa_get_node(ram_addr_t addr, Error **errp);
>  /* on success returns node index in numa_info,
>   * on failure returns nb_numa_nodes */
>  int numa_get_node_for_cpu(int idx);
> +
> +static inline bool numa_has_node_id(const CPUArchIdList *possible_cpus, int idx)
> +{
> +    return possible_cpus->cpus[idx].props.has_node_id;
> +}
> +
> +static inline int numa_node_id(const CPUArchIdList *possible_cpus, int idx)
> +{
> +    return possible_cpus->cpus[idx].props.node_id;
> +}
>  #endif
> diff --git a/numa.c b/numa.c
> index c7e3e0a..872ee0d 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -394,7 +394,7 @@ void parse_numa_opts(MachineState *ms)
>  
>          possible_cpus = mc->possible_cpu_arch_ids(ms);
>          for (i = 0; i < possible_cpus->len; i++) {
> -            if (possible_cpus->cpus[i].props.has_node_id) {
> +            if (numa_has_node_id(possible_cpus, i)) {
>                  break;
>              }
>          }
> -- 
> 2.7.4
> 
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

On Wed, May 03, 2017 at 02:57:06PM +0200, Igor Mammedov wrote:
> wrappers should make access to [has]node_id fields more readable
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Correct, though I'm not sure it actually simplifies things that much.
Maybe more in future patches, though.

> ---
>  follow up patches will use this wrappers
>  v2:
>     - add wrappers (Drew)
> ---
>  include/sysemu/numa.h | 10 ++++++++++
>  numa.c                |  2 +-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 46ea6c7..98d01e6 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -35,4 +35,14 @@ uint32_t numa_get_node(ram_addr_t addr, Error **errp);
>  /* on success returns node index in numa_info,
>   * on failure returns nb_numa_nodes */
>  int numa_get_node_for_cpu(int idx);
> +
> +static inline bool numa_has_node_id(const CPUArchIdList *possible_cpus, int idx)
> +{
> +    return possible_cpus->cpus[idx].props.has_node_id;
> +}
> +
> +static inline int numa_node_id(const CPUArchIdList *possible_cpus, int idx)
> +{
> +    return possible_cpus->cpus[idx].props.node_id;
> +}
>  #endif
> diff --git a/numa.c b/numa.c
> index c7e3e0a..872ee0d 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -394,7 +394,7 @@ void parse_numa_opts(MachineState *ms)
>  
>          possible_cpus = mc->possible_cpu_arch_ids(ms);
>          for (i = 0; i < possible_cpus->len; i++) {
> -            if (possible_cpus->cpus[i].props.has_node_id) {
> +            if (numa_has_node_id(possible_cpus, i)) {
>                  break;
>              }
>          }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

On Fri, 5 May 2017 11:45:22 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, May 03, 2017 at 02:57:06PM +0200, Igor Mammedov wrote:
> > wrappers should make access to [has]node_id fields more readable
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Correct, though I'm not sure it actually simplifies things that much.
> Maybe more in future patches, though.
that's what Drew insisted on, and even though I prefer other way around
I won't stall series arguing about styling issues,
so here this patch goes.

> 
> > ---
> >  follow up patches will use this wrappers
> >  v2:
> >     - add wrappers (Drew)
> > ---
> >  include/sysemu/numa.h | 10 ++++++++++
> >  numa.c                |  2 +-
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > index 46ea6c7..98d01e6 100644
> > --- a/include/sysemu/numa.h
> > +++ b/include/sysemu/numa.h
> > @@ -35,4 +35,14 @@ uint32_t numa_get_node(ram_addr_t addr, Error **errp);
> >  /* on success returns node index in numa_info,
> >   * on failure returns nb_numa_nodes */
> >  int numa_get_node_for_cpu(int idx);
> > +
> > +static inline bool numa_has_node_id(const CPUArchIdList *possible_cpus, int idx)
> > +{
> > +    return possible_cpus->cpus[idx].props.has_node_id;
> > +}
> > +
> > +static inline int numa_node_id(const CPUArchIdList *possible_cpus, int idx)
> > +{
> > +    return possible_cpus->cpus[idx].props.node_id;
> > +}
> >  #endif
> > diff --git a/numa.c b/numa.c
> > index c7e3e0a..872ee0d 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -394,7 +394,7 @@ void parse_numa_opts(MachineState *ms)
> >  
> >          possible_cpus = mc->possible_cpu_arch_ids(ms);
> >          for (i = 0; i < possible_cpus->len; i++) {
> > -            if (possible_cpus->cpus[i].props.has_node_id) {
> > +            if (numa_has_node_id(possible_cpus, i)) {
> >                  break;
> >              }
> >          }  
> 

On Fri, May 05, 2017 at 10:09:18AM +0200, Igor Mammedov wrote:
> On Fri, 5 May 2017 11:45:22 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, May 03, 2017 at 02:57:06PM +0200, Igor Mammedov wrote:
> > > wrappers should make access to [has]node_id fields more readable
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > Correct, though I'm not sure it actually simplifies things that much.
> > Maybe more in future patches, though.
> that's what Drew insisted on, and even though I prefer other way around
> I won't stall series arguing about styling issues,
> so here this patch goes.

My argument in the last review of this series was that references like

 machine->possible_cpus->cpus[cs->cpu_index].props.has_node_id
and
 machine->possible_cpus->cpus[cs->cpu_index].props.node_id

are quite long, and only differ by 'has_', making it tough to
easily recognize. But, if nobody, but me, sees value in changing
them to

 numa_has_node_id(machine->possible_cpus, cs->cpu_index)
and
 numa_node_id(machine->possible_cpus, cs->cpu_index)

then I won't insist.

Thanks,
drew

> 
> > 
> > > ---
> > >  follow up patches will use this wrappers
> > >  v2:
> > >     - add wrappers (Drew)
> > > ---
> > >  include/sysemu/numa.h | 10 ++++++++++
> > >  numa.c                |  2 +-
> > >  2 files changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > > index 46ea6c7..98d01e6 100644
> > > --- a/include/sysemu/numa.h
> > > +++ b/include/sysemu/numa.h
> > > @@ -35,4 +35,14 @@ uint32_t numa_get_node(ram_addr_t addr, Error **errp);
> > >  /* on success returns node index in numa_info,
> > >   * on failure returns nb_numa_nodes */
> > >  int numa_get_node_for_cpu(int idx);
> > > +
> > > +static inline bool numa_has_node_id(const CPUArchIdList *possible_cpus, int idx)
> > > +{
> > > +    return possible_cpus->cpus[idx].props.has_node_id;
> > > +}
> > > +
> > > +static inline int numa_node_id(const CPUArchIdList *possible_cpus, int idx)
> > > +{
> > > +    return possible_cpus->cpus[idx].props.node_id;
> > > +}
> > >  #endif
> > > diff --git a/numa.c b/numa.c
> > > index c7e3e0a..872ee0d 100644
> > > --- a/numa.c
> > > +++ b/numa.c
> > > @@ -394,7 +394,7 @@ void parse_numa_opts(MachineState *ms)
> > >  
> > >          possible_cpus = mc->possible_cpu_arch_ids(ms);
> > >          for (i = 0; i < possible_cpus->len; i++) {
> > > -            if (possible_cpus->cpus[i].props.has_node_id) {
> > > +            if (numa_has_node_id(possible_cpus, i)) {
> > >                  break;
> > >              }
> > >          }  
> > 
> 
> 

On Fri, May 05, 2017 at 11:06:02AM +0200, Andrew Jones wrote:
> On Fri, May 05, 2017 at 10:09:18AM +0200, Igor Mammedov wrote:
> > On Fri, 5 May 2017 11:45:22 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > On Wed, May 03, 2017 at 02:57:06PM +0200, Igor Mammedov wrote:
> > > > wrappers should make access to [has]node_id fields more readable
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > > 
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > 
> > > Correct, though I'm not sure it actually simplifies things that much.
> > > Maybe more in future patches, though.
> > that's what Drew insisted on, and even though I prefer other way around
> > I won't stall series arguing about styling issues,
> > so here this patch goes.
> 
> My argument in the last review of this series was that references like
> 
>  machine->possible_cpus->cpus[cs->cpu_index].props.has_node_id
> and
>  machine->possible_cpus->cpus[cs->cpu_index].props.node_id
> 
> are quite long, and only differ by 'has_', making it tough to
> easily recognize.

If expression length is a problem, we can just use an extra
variable:

 CPUArchId *slot = &machine->possible_cpus->cpus[cs->cpu_index];
 if (slot->props.has_node_id && slot->props.node_id == FOO) ...


>                   But, if nobody, but me, sees value in changing
> them to
> 
>  numa_has_node_id(machine->possible_cpus, cs->cpu_index)
> and
>  numa_node_id(machine->possible_cpus, cs->cpu_index)
> 
> then I won't insist.

I don't mind that much, but I still prefer the version without
the wrappers.

-- 
Eduardo

On Fri, 5 May 2017 14:12:26 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, May 05, 2017 at 11:06:02AM +0200, Andrew Jones wrote:
> > On Fri, May 05, 2017 at 10:09:18AM +0200, Igor Mammedov wrote:
> > > On Fri, 5 May 2017 11:45:22 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > 
> > > > On Wed, May 03, 2017 at 02:57:06PM +0200, Igor Mammedov wrote:
> > > > > wrappers should make access to [has]node_id fields more readable
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > > > 
> > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > > 
> > > > Correct, though I'm not sure it actually simplifies things that much.
> > > > Maybe more in future patches, though.
> > > that's what Drew insisted on, and even though I prefer other way around
> > > I won't stall series arguing about styling issues,
> > > so here this patch goes.
> > 
> > My argument in the last review of this series was that references like
> > 
> >  machine->possible_cpus->cpus[cs->cpu_index].props.has_node_id
> > and
> >  machine->possible_cpus->cpus[cs->cpu_index].props.node_id
> > 
> > are quite long, and only differ by 'has_', making it tough to
> > easily recognize.
> 
> If expression length is a problem, we can just use an extra
> variable:
> 
>  CPUArchId *slot = &machine->possible_cpus->cpus[cs->cpu_index];
>  if (slot->props.has_node_id && slot->props.node_id == FOO) ...
> 
> 
> >                   But, if nobody, but me, sees value in changing
> > them to
> > 
> >  numa_has_node_id(machine->possible_cpus, cs->cpu_index)
> > and
> >  numa_node_id(machine->possible_cpus, cs->cpu_index)
> > 
> > then I won't insist.
> 
> I don't mind that much, but I still prefer the version without
> the wrappers.
considering 3vs1 for wrapper less variant, I'll return it back.
but respin will have to wait till Tuesday due to holiday over here.