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?
signature.asc
Description: PGP signature
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
