Re: [Xen-devel] [PATCH] python: Adjust xc_physinfo wrapper for updated virt_caps bits

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Apr 29, 2019 at 11:16:26AM +0100, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("[PATCH] python: Adjust xc_physinfo 
> wrapper for updated virt_caps bits"):
> > Commit f089fddd94 "xen: report PV capability in sysctl and use it in
> > toolstack" changed meaning of virt_caps bit 1 - previously it was
> > "directio", but was changed to "pv" and "directio" was moved to bit 2.
> > Adjust python wrapper, and add reporting of both "pv_directio" and
> > "hvm_directio".
> 
> Thanks for your attention to this...
> 
> But:
> 
> > index cc8175a11e..0a8d8f407e 100644
> > --- a/tools/python/xen/lowlevel/xc/xc.c
> > +++ b/tools/python/xen/lowlevel/xc/xc.c
> > @@ -973,7 +973,8 @@ static PyObject *pyxc_physinfo(XcObject *self)
> >      xc_physinfo_t pinfo;
> >      char cpu_cap[128], virt_caps[128], *p;
> >      int i;
> > -    const char *virtcap_names[] = { "hvm", "hvm_directio" };
> > +    const char *virtcap_names[] = { "hvm", "pv",
> > +                                    "hvm_directio", "pv_directio" };
> 
> It seems quite wrong that we have no way to keep this in sync - and
> not even comments in both places!  (This is not your fault...)

I'll add a comment...

> > @@ -989,6 +990,10 @@ static PyObject *pyxc_physinfo(XcObject *self)
> >      for ( i = 0; i < 2; i++ )
> >          if ( (pinfo.capabilities >> i) & 1 )
> >            p += sprintf(p, "%s ", virtcap_names[i]);
> > +    if (pinfo.capabilities & XEN_SYSCTL_PHYSCAP_directio)
> > +        for ( i = 0; i < 2; i++ )
> > +            if ( (pinfo.capabilities >> i) & 1 )
> > +              p += sprintf(p, "%s ", virtcap_names[i+2]);
> >      if ( p != virt_caps )
> >        *(p-1) = '\0';
> 
> I'm not sure I like this.  AFAICT the +2 is magic, and you in fact
> treat the two halves of this array together as a single array.  So
> this should either be two arrays, or, more likely, something like this
> maybe:
> 
>   +              p += sprintf(p, "%s_directio ", virtcap_names[i]);
> 
> What do you think ?

Makes sense.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel