Re: [PATCH] Fix assignment of devid value returned from libxl__device_nextid

[Jim Fehlig <jfehlig@suse.com>]

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