| Message ID | 20190428190824.28029-1-marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | python: Adjust xc_physinfo wrapper for updated virt_caps bits | expand |
On Sun, Apr 28, 2019 at 09:08:23PM +0200, Marek Marczykowski-Górecki wrote: > 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". > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Acked-by: Wei Liu <wei.liu2@citrix.com>
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...)
> @@ -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 ?
Ian.
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.
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. >
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c 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" }; if ( xc_physinfo(self->xc_handle, &pinfo) != 0 ) return pyxc_error_to_exception(self->xc_handle); @@ -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';
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". Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- This should be backported to 4.12 Cc: Ian Jackson <ian.jackson@eu.citrix.com> --- tools/python/xen/lowlevel/xc/xc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)