[PATCH] libxl: do not try to detach pci device when none attached

[PATCH] libxl: do not try to detach pci device when none attached

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1307662302 -7200
# Node ID 88f67b423c89f4bd604837b9eae2483dad5ebb0d
# Parent  4b392511ae0840fba66c40aa2788dc1ff402b6e8
libxl: do not try to detach pci device when none attached

Fixes SEGV on domain destroy

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1039,6 +1039,8 @@ int libxl_device_pci_shutdown(libxl_ctx 
     rc = libxl_device_pci_list_assigned(ctx, &pcidevs, domid, &num);
     if ( rc )
         return rc;
+    if ( num == 0 )
+        return 0;
     for (i = 0; i < num; i++) {
         /* Force remove on shutdown since, on HVM, qemu will not always
          * respond to SCI interrupt because the guest kernel has shut down the

Re: [PATCH] libxl: do not try to detach pci device when none attached

[Stefano Stabellini]

On Fri, 10 Jun 2011, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1307662302 -7200
> # Node ID 88f67b423c89f4bd604837b9eae2483dad5ebb0d
> # Parent  4b392511ae0840fba66c40aa2788dc1ff402b6e8
> libxl: do not try to detach pci device when none attached
> 
> Fixes SEGV on domain destroy
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
> 
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -1039,6 +1039,8 @@ int libxl_device_pci_shutdown(libxl_ctx 
>      rc = libxl_device_pci_list_assigned(ctx, &pcidevs, domid, &num);
>      if ( rc )
>          return rc;
> +    if ( num == 0 )
> +        return 0;
>      for (i = 0; i < num; i++) {
>          /* Force remove on shutdown since, on HVM, qemu will not always
>           * respond to SCI interrupt because the guest kernel has shut down the

Is the problem here that num is 0 and pcidevs is uninitialized?
If so, would this patch solve the problem you are seeing?


diff -r 37c77bacb52a tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Mon May 23 17:38:28 2011 +0100
+++ b/tools/libxl/libxl_pci.c	Fri Jun 10 10:39:05 2011 +0000
@@ -1022,8 +1022,7 @@ int libxl_device_pci_list_assigned(libxl
             } while ((p = strtok_r(NULL, ",=", &saveptr)) != NULL);
         }
     }
-    if ( *num )
-        *list = pcidevs;
+    *list = pcidevs;
     libxl__free_all(&gc);
     return 0;
 }

Re: [PATCH] libxl: do not try to detach pci device when none attached

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 1913 bytes --]

On 10.06.2011 12:44, Stefano Stabellini wrote:
> On Fri, 10 Jun 2011, Marek Marczykowski wrote:
>> # HG changeset patch
>> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
>> # Date 1307662302 -7200
>> # Node ID 88f67b423c89f4bd604837b9eae2483dad5ebb0d
>> # Parent  4b392511ae0840fba66c40aa2788dc1ff402b6e8
>> libxl: do not try to detach pci device when none attached
>>
>> Fixes SEGV on domain destroy
>>
>> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
>>
>> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
>> --- a/tools/libxl/libxl_pci.c
>> +++ b/tools/libxl/libxl_pci.c
>> @@ -1039,6 +1039,8 @@ int libxl_device_pci_shutdown(libxl_ctx 
>>      rc = libxl_device_pci_list_assigned(ctx, &pcidevs, domid, &num);
>>      if ( rc )
>>          return rc;
>> +    if ( num == 0 )
>> +        return 0;
>>      for (i = 0; i < num; i++) {
>>          /* Force remove on shutdown since, on HVM, qemu will not always
>>           * respond to SCI interrupt because the guest kernel has shut down the
> 
> Is the problem here that num is 0 and pcidevs is uninitialized?
> If so, would this patch solve the problem you are seeing?

No, when there is no PCI devs, libxl_device_pci_list_assigned exists
earlier - before allocating memory for pcidevs.

> diff -r 37c77bacb52a tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c	Mon May 23 17:38:28 2011 +0100
> +++ b/tools/libxl/libxl_pci.c	Fri Jun 10 10:39:05 2011 +0000
> @@ -1022,8 +1022,7 @@ int libxl_device_pci_list_assigned(libxl
>              } while ((p = strtok_r(NULL, ",=", &saveptr)) != NULL);
>          }
>      }
> -    if ( *num )
> -        *list = pcidevs;
> +    *list = pcidevs;
>      libxl__free_all(&gc);
>      return 0;
>  }


-- 
Pozdrawiam / Best Regards,
Marek Marczykowski         | RLU #390519
marmarek at mimuw edu pl   | xmpp:marmarek at staszic waw pl


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5842 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] libxl: do not try to detach pci device when none attached

[Stefano Stabellini]

On Fri, 10 Jun 2011, Marek Marczykowski wrote:
> On 10.06.2011 12:44, Stefano Stabellini wrote:
> > On Fri, 10 Jun 2011, Marek Marczykowski wrote:
> >> # HG changeset patch
> >> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> >> # Date 1307662302 -7200
> >> # Node ID 88f67b423c89f4bd604837b9eae2483dad5ebb0d
> >> # Parent  4b392511ae0840fba66c40aa2788dc1ff402b6e8
> >> libxl: do not try to detach pci device when none attached
> >>
> >> Fixes SEGV on domain destroy
> >>
> >> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
> >>
> >> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> >> --- a/tools/libxl/libxl_pci.c
> >> +++ b/tools/libxl/libxl_pci.c
> >> @@ -1039,6 +1039,8 @@ int libxl_device_pci_shutdown(libxl_ctx 
> >>      rc = libxl_device_pci_list_assigned(ctx, &pcidevs, domid, &num);
> >>      if ( rc )
> >>          return rc;
> >> +    if ( num == 0 )
> >> +        return 0;
> >>      for (i = 0; i < num; i++) {
> >>          /* Force remove on shutdown since, on HVM, qemu will not always
> >>           * respond to SCI interrupt because the guest kernel has shut down the
> > 
> > Is the problem here that num is 0 and pcidevs is uninitialized?
> > If so, would this patch solve the problem you are seeing?
> 
> No, when there is no PCI devs, libxl_device_pci_list_assigned exists
> earlier - before allocating memory for pcidevs.

Sorry, but I still don't understand.

If num is 0, it won't enter the following loop anyway, the only
other operation in libxl_device_pci_shutdown is free(pcidevs) and
pcidevs should be NULL so nothing bad should happen.

Re: [PATCH] libxl: do not try to detach pci device when none attached

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 1889 bytes --]

On 10.06.2011 13:03, Stefano Stabellini wrote:
> On Fri, 10 Jun 2011, Marek Marczykowski wrote:
>> On 10.06.2011 12:44, Stefano Stabellini wrote:
>>> On Fri, 10 Jun 2011, Marek Marczykowski wrote:
>>>> # HG changeset patch
>>>> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
>>>> # Date 1307662302 -7200
>>>> # Node ID 88f67b423c89f4bd604837b9eae2483dad5ebb0d
>>>> # Parent  4b392511ae0840fba66c40aa2788dc1ff402b6e8
>>>> libxl: do not try to detach pci device when none attached
>>>>
>>>> Fixes SEGV on domain destroy
>>>>
>>>> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
>>>>
>>>> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
>>>> --- a/tools/libxl/libxl_pci.c
>>>> +++ b/tools/libxl/libxl_pci.c
>>>> @@ -1039,6 +1039,8 @@ int libxl_device_pci_shutdown(libxl_ctx 
>>>>      rc = libxl_device_pci_list_assigned(ctx, &pcidevs, domid, &num);
>>>>      if ( rc )
>>>>          return rc;
>>>> +    if ( num == 0 )
>>>> +        return 0;
>>>>      for (i = 0; i < num; i++) {
>>>>          /* Force remove on shutdown since, on HVM, qemu will not always
>>>>           * respond to SCI interrupt because the guest kernel has shut down the
>>>
>>> Is the problem here that num is 0 and pcidevs is uninitialized?
>>> If so, would this patch solve the problem you are seeing?
>>
>> No, when there is no PCI devs, libxl_device_pci_list_assigned exists
>> earlier - before allocating memory for pcidevs.
> 
> Sorry, but I still don't understand.
> 
> If num is 0, it won't enter the following loop anyway, the only
> other operation in libxl_device_pci_shutdown is free(pcidevs) and
> pcidevs should be NULL so nothing bad should happen.

Ah, ok. So your patch should also works.

-- 
Pozdrawiam / Best Regards,
Marek Marczykowski         | RLU #390519
marmarek at mimuw edu pl   | xmpp:marmarek at staszic waw pl


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5842 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] libxl: do not try to detach pci device when none attached

[Ian Jackson]

Marek Marczykowski writes ("Re: [Xen-devel] [PATCH] libxl: do not try to detach pci device when none attached"):
> On 10.06.2011 13:03, Stefano Stabellini wrote:
> > If num is 0, it won't enter the following loop anyway, the only
> > other operation in libxl_device_pci_shutdown is free(pcidevs) and
> > pcidevs should be NULL so nothing bad should happen.
> 
> Ah, ok. So your patch should also works.

I have applied Stefano's patch.

Ian.

Re: [PATCH] libxl: do not try to detach pci device when none attached

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] libxl: do not try to detach pci device when none attached"):
> diff -r 37c77bacb52a tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c	Mon May 23 17:38:28 2011 +0100
> +++ b/tools/libxl/libxl_pci.c	Fri Jun 10 10:39:05 2011 +0000
> @@ -1022,8 +1022,7 @@ int libxl_device_pci_list_assigned(libxl
>              } while ((p = strtok_r(NULL, ",=", &saveptr)) != NULL);
>          }
>      }
> -    if ( *num )
> -        *list = pcidevs;
> +    *list = pcidevs;

This patch looks correct to me, as far as it goes.

If n==0 then calloc is passed 0 and may return 0 willy-nilly but since
there's no error check this doesn't cause other trouble :-/.

Ian.