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

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

On Tue, Apr 30, 2019 at 12:08:38AM +0200, Marek Marczykowski-Górecki wrote:
> 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...

Actually, this would work much better if the loop below would use
#defines from sysctl.h, instead of hardcoded values. I'll update it this
way.

> > > @@ -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