Jim Fehlig
2013-Jun-25 22:02 UTC
[PATCH] Fix assignment of devid value returned from libxl__device_nextid
Commit 5420f265 has some misplaced parenthesis that caused devid
to be assigned 1 or 0 based on checking return value of
libxl__device_nextid < 0, e.g.
devid = libxl__device_nextid(...) < 0
This works when only one instance of a given device type exists, but
subsequent devices of the same type will also have a devid = 1 if
libxl__device_nextid succeeds. Fix by checking the value assigned to
devid, e.g.
(devid = libxl__device_nextid(...)) < 0
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
tools/libxl/libxl.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 0612d85..1bbb990 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1781,7 +1781,7 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t
domid,
back = flexarray_make(gc, 16, 1);
if (vtpm->devid == -1) {
- if ((vtpm->devid = libxl__device_nextid(gc, domid, "vtpm")
< 0)) {
+ if ((vtpm->devid = libxl__device_nextid(gc, domid,
"vtpm")) < 0) {
rc = ERROR_FAIL;
goto out;
}
@@ -2877,7 +2877,7 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t
domid,
back = flexarray_make(gc, 18, 1);
if (nic->devid == -1) {
- if ((nic->devid = libxl__device_nextid(gc, domid, "vif")
< 0)) {
+ if ((nic->devid = libxl__device_nextid(gc, domid, "vif"))
< 0) {
rc = ERROR_FAIL;
goto out;
}
@@ -3258,7 +3258,7 @@ int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
back = flexarray_make(gc, 16, 1);
if (vkb->devid == -1) {
- if ((vkb->devid = libxl__device_nextid(gc, domid, "vkb")
< 0)) {
+ if ((vkb->devid = libxl__device_nextid(gc, domid, "vkb"))
< 0) {
rc = ERROR_FAIL;
goto out;
}
@@ -3359,7 +3359,7 @@ int libxl__device_vfb_add(libxl__gc *gc, uint32_t domid,
libxl_device_vfb *vfb)
back = flexarray_make(gc, 16, 1);
if (vfb->devid == -1) {
- if ((vfb->devid = libxl__device_nextid(gc, domid, "vfb")
< 0)) {
+ if ((vfb->devid = libxl__device_nextid(gc, domid, "vfb"))
< 0) {
rc = ERROR_FAIL;
goto out;
}
--
1.8.0.1
George Dunlap
2013-Jun-26 09:41 UTC
Re: [PATCH] Fix assignment of devid value returned from libxl__device_nextid
On Tue, Jun 25, 2013 at 11:02 PM, Jim Fehlig <jfehlig@suse.com> wrote:> Commit 5420f265 has some misplaced parenthesis that caused devid > to be assigned 1 or 0 based on checking return value of > libxl__device_nextid < 0, e.g. > > devid = libxl__device_nextid(...) < 0 > > This works when only one instance of a given device type exists, but > subsequent devices of the same type will also have a devid = 1 if > libxl__device_nextid succeeds. Fix by checking the value assigned to > devid, e.g. > > (devid = libxl__device_nextid(...)) < 0 > > Signed-off-by: Jim Fehlig <jfehlig@suse.com>Both for 4.3, and as a patch in general: Acked-by: George Dunlap <george.dunlap@eu.citrix.com>> --- > tools/libxl/libxl.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 0612d85..1bbb990 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -1781,7 +1781,7 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid, > back = flexarray_make(gc, 16, 1); > > if (vtpm->devid == -1) { > - if ((vtpm->devid = libxl__device_nextid(gc, domid, "vtpm") < 0)) { > + if ((vtpm->devid = libxl__device_nextid(gc, domid, "vtpm")) < 0) { > rc = ERROR_FAIL; > goto out; > } > @@ -2877,7 +2877,7 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid, > back = flexarray_make(gc, 18, 1); > > if (nic->devid == -1) { > - if ((nic->devid = libxl__device_nextid(gc, domid, "vif") < 0)) { > + if ((nic->devid = libxl__device_nextid(gc, domid, "vif")) < 0) { > rc = ERROR_FAIL; > goto out; > } > @@ -3258,7 +3258,7 @@ int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid, > back = flexarray_make(gc, 16, 1); > > if (vkb->devid == -1) { > - if ((vkb->devid = libxl__device_nextid(gc, domid, "vkb") < 0)) { > + if ((vkb->devid = libxl__device_nextid(gc, domid, "vkb")) < 0) { > rc = ERROR_FAIL; > goto out; > } > @@ -3359,7 +3359,7 @@ int libxl__device_vfb_add(libxl__gc *gc, uint32_t domid, libxl_device_vfb *vfb) > back = flexarray_make(gc, 16, 1); > > if (vfb->devid == -1) { > - if ((vfb->devid = libxl__device_nextid(gc, domid, "vfb") < 0)) { > + if ((vfb->devid = libxl__device_nextid(gc, domid, "vfb")) < 0) { > rc = ERROR_FAIL; > goto out; > } > -- > 1.8.0.1 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Ian Campbell
2013-Jun-26 16:24 UTC
Re: [PATCH] Fix assignment of devid value returned from libxl__device_nextid
On Wed, 2013-06-26 at 10:41 +0100, George Dunlap wrote:> On Tue, Jun 25, 2013 at 11:02 PM, Jim Fehlig <jfehlig@suse.com> wrote: > > Commit 5420f265 has some misplaced parenthesis that caused devid > > to be assigned 1 or 0 based on checking return value of > > libxl__device_nextid < 0, e.g. > > > > devid = libxl__device_nextid(...) < 0 > > > > This works when only one instance of a given device type exists, but > > subsequent devices of the same type will also have a devid = 1 if > > libxl__device_nextid succeeds. Fix by checking the value assigned to > > devid, e.g. > > > > (devid = libxl__device_nextid(...)) < 0This is why I hate combined assignment & logic!> > Signed-off-by: Jim Fehlig <jfehlig@suse.com> > > Both for 4.3, and as a patch in general: > > Acked-by: George Dunlap <george.dunlap@eu.citrix.com>Acked + applied, thanks.
Jim Fehlig
2013-Jun-26 16:42 UTC
Re: [PATCH] Fix assignment of devid value returned from libxl__device_nextid
George Dunlap wrote:> On Tue, Jun 25, 2013 at 11:02 PM, Jim Fehlig <jfehlig@suse.com> wrote: > >> Commit 5420f265 has some misplaced parenthesis that caused devid >> to be assigned 1 or 0 based on checking return value of >> libxl__device_nextid < 0, e.g. >> >> devid = libxl__device_nextid(...) < 0 >> >> This works when only one instance of a given device type exists, but >> subsequent devices of the same type will also have a devid = 1 if >> libxl__device_nextid succeeds. Fix by checking the value assigned to >> devid, e.g. >> >> (devid = libxl__device_nextid(...)) < 0 >> >> Signed-off-by: Jim Fehlig <jfehlig@suse.com> >> > > Both for 4.3, and as a patch in general: >Noticed I have the commit msg a tad bit wrong. It should read "...devices of the same type will also have a devid = 0 if..." since libxl__device_nextid(...) < 0 is false when libxl__device_nextid() succeeds. Also, perhaps I should add that xl is not affected since it supplies devid, but apps such as libvirt that allow libxl to fill in devid are certainly affected when there is more than 1 of the same device type. Should I submit a V2 with an improved commit msg? Regards, Jim> Acked-by: George Dunlap <george.dunlap@eu.citrix.com> > > >> --- >> tools/libxl/libxl.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index 0612d85..1bbb990 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -1781,7 +1781,7 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid, >> back = flexarray_make(gc, 16, 1); >> >> if (vtpm->devid == -1) { >> - if ((vtpm->devid = libxl__device_nextid(gc, domid, "vtpm") < 0)) { >> + if ((vtpm->devid = libxl__device_nextid(gc, domid, "vtpm")) < 0) { >> rc = ERROR_FAIL; >> goto out; >> } >> @@ -2877,7 +2877,7 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid, >> back = flexarray_make(gc, 18, 1); >> >> if (nic->devid == -1) { >> - if ((nic->devid = libxl__device_nextid(gc, domid, "vif") < 0)) { >> + if ((nic->devid = libxl__device_nextid(gc, domid, "vif")) < 0) { >> rc = ERROR_FAIL; >> goto out; >> } >> @@ -3258,7 +3258,7 @@ int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid, >> back = flexarray_make(gc, 16, 1); >> >> if (vkb->devid == -1) { >> - if ((vkb->devid = libxl__device_nextid(gc, domid, "vkb") < 0)) { >> + if ((vkb->devid = libxl__device_nextid(gc, domid, "vkb")) < 0) { >> rc = ERROR_FAIL; >> goto out; >> } >> @@ -3359,7 +3359,7 @@ int libxl__device_vfb_add(libxl__gc *gc, uint32_t domid, libxl_device_vfb *vfb) >> back = flexarray_make(gc, 16, 1); >> >> if (vfb->devid == -1) { >> - if ((vfb->devid = libxl__device_nextid(gc, domid, "vfb") < 0)) { >> + if ((vfb->devid = libxl__device_nextid(gc, domid, "vfb")) < 0) { >> rc = ERROR_FAIL; >> goto out; >> } >> -- >> 1.8.0.1 >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel >> > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > >
Ian Campbell
2013-Jun-26 16:49 UTC
Re: [PATCH] Fix assignment of devid value returned from libxl__device_nextid
On Wed, 2013-06-26 at 10:42 -0600, Jim Fehlig wrote:> George Dunlap wrote: > > On Tue, Jun 25, 2013 at 11:02 PM, Jim Fehlig <jfehlig@suse.com> wrote: > > > >> Commit 5420f265 has some misplaced parenthesis that caused devid > >> to be assigned 1 or 0 based on checking return value of > >> libxl__device_nextid < 0, e.g. > >> > >> devid = libxl__device_nextid(...) < 0 > >> > >> This works when only one instance of a given device type exists, but > >> subsequent devices of the same type will also have a devid = 1 if > >> libxl__device_nextid succeeds. Fix by checking the value assigned to > >> devid, e.g. > >> > >> (devid = libxl__device_nextid(...)) < 0 > >> > >> Signed-off-by: Jim Fehlig <jfehlig@suse.com> > >> > > > > Both for 4.3, and as a patch in general: > > > > Noticed I have the commit msg a tad bit wrong. It should read > "...devices of the same type will also have a devid = 0 if..." since > libxl__device_nextid(...) < 0 is false when libxl__device_nextid() > succeeds. Also, perhaps I should add that xl is not affected since it > supplies devid, but apps such as libvirt that allow libxl to fill in > devid are certainly affected when there is more than 1 of the same > device type. Should I submit a V2 with an improved commit msg?I''m afraid I have already committed. Not to worry. Ian.
Jim Fehlig
2013-Jun-26 17:05 UTC
Re: [PATCH] Fix assignment of devid value returned from libxl__device_nextid
Ian Campbell wrote:> On Wed, 2013-06-26 at 10:42 -0600, Jim Fehlig wrote: > >> George Dunlap wrote: >> >>> On Tue, Jun 25, 2013 at 11:02 PM, Jim Fehlig <jfehlig@suse.com> wrote: >>> >>> >>>> Commit 5420f265 has some misplaced parenthesis that caused devid >>>> to be assigned 1 or 0 based on checking return value of >>>> libxl__device_nextid < 0, e.g. >>>> >>>> devid = libxl__device_nextid(...) < 0 >>>> >>>> This works when only one instance of a given device type exists, but >>>> subsequent devices of the same type will also have a devid = 1 if >>>> libxl__device_nextid succeeds. Fix by checking the value assigned to >>>> devid, e.g. >>>> >>>> (devid = libxl__device_nextid(...)) < 0 >>>> >>>> Signed-off-by: Jim Fehlig <jfehlig@suse.com> >>>> >>>> >>> Both for 4.3, and as a patch in general: >>> >>> >> Noticed I have the commit msg a tad bit wrong. It should read >> "...devices of the same type will also have a devid = 0 if..." since >> libxl__device_nextid(...) < 0 is false when libxl__device_nextid() >> succeeds. Also, perhaps I should add that xl is not affected since it >> supplies devid, but apps such as libvirt that allow libxl to fill in >> devid are certainly affected when there is more than 1 of the same >> device type. Should I submit a V2 with an improved commit msg? >> > > I''m afraid I have already committed. >Yep, saw that just after I sent this message :).> Not to worry. >Right. The existing msg at least adequately describes the problem. Regards, Jim