[virt-tools-list] [virt-manager PATCH] delete: do not throw exception if the volume or the pool don't exist

[Giuseppe Scrivano]

Cole Robinson <crobinso at redhat.com> writes:

> On 05/07/2015 08:33 AM, Giuseppe Scrivano wrote:
>> Giuseppe Scrivano <gscrivan at redhat.com> writes:
>> 
>>> Giuseppe Scrivano <gscrivan at redhat.com> writes:
>>>
>>>> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1219427
>>>>
>>>> Signed-off-by: Giuseppe Scrivano <gscrivan at redhat.com>
>>>> ---
>>>>  virtManager/delete.py | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/virtManager/delete.py b/virtManager/delete.py
>>>> index 2addcfa..a7d4ec2 100644
>>>> --- a/virtManager/delete.py
>>>> +++ b/virtManager/delete.py
>>>> @@ -236,7 +236,11 @@ def populate_storage_list(storage_list, vm, conn):
>>>>          if disk.source_pool:
>>>>              try:
>>>>                  pool = conn.get_pool(disk.source_pool)
>>>> +                if pool is None:
>>>> +                    return disk.path
>>>>                  vol = pool.get_volume(disk.path)
>>>> +                if vol is None:
>>>> +                    return disk.path
>>>>                  return vol.get_target_path()
>>>>              except KeyError:
>>>>                  return disk.path
>>>
>>> looking into the root cause of it..
>>>
>>> commit 5357b91402fb7a8a73921216926908c08f6ad99d changed the semantic of
>>> conn.get_(vm|pool|interface|nodedev|net), to return None instead of
>>> raising KeyError.  Should we perhaps restore the previous behavior?
>> 
>> I am more convinced about this version now (additionally, the try/except
>> code in the first patch is not useful):
>> 
>> diff --git a/virtManager/connection.py b/virtManager/connection.py
>> index 41403f6..47204e9 100644
>> --- a/virtManager/connection.py
>> +++ b/virtManager/connection.py
>> @@ -133,7 +133,7 @@ class _ObjectList(vmmGObject):
>>          for obj in self.get_objects_for_class(classobj):
>>              if obj.get_connkey() == connkey:
>>                  return obj
>> -        return None
>> +        raise KeyError("Key %s does not exist" % connkey)
>> 
>> Regards,
>> Giuseppe
>> 
>
> I'm slightly afraid that new code might be depending on that return None
> behavior now. I changed a lot of stuff in this area...
>
> Looking at all places that catch KeyError, most have the exact same behavior
> with or without this change. The only other two places that likely have
> different behavior with the current code are:
>
> host.py:net_selected (fairly minor)
> details.py:refresh_disk_page (another instance of pool/vol bug)
>
> Though some sites may be catching Exception and not KeyError
>
> How about a patch to fix the delete.py and details.py issues that you can
> backport, I'll audit the remaining callers to look for any regressions

ok sure, then we can go with the first version of the patch, I'll leave
the exception handling in case the old behavior is restored.

OK to push this?

diff --git a/virtManager/delete.py b/virtManager/delete.py
index 2addcfa..a7d4ec2 100644
--- a/virtManager/delete.py
+++ b/virtManager/delete.py
@@ -236,7 +236,11 @@ def populate_storage_list(storage_list, vm, conn):
         if disk.source_pool:
             try:
                 pool = conn.get_pool(disk.source_pool)
+                if pool is None:
+                    return disk.path
                 vol = pool.get_volume(disk.path)
+                if vol is None:
+                    return disk.path
                 return vol.get_target_path()
             except KeyError:
                 return disk.path
diff --git a/virtManager/details.py b/virtManager/details.py
index 7690e46..03184d8 100644
--- a/virtManager/details.py
+++ b/virtManager/details.py
@@ -2700,12 +2700,14 @@ class vmmDetails(vmmGObjectUI):
         if not path:
             size = "-"
         else:
+            vol = None
             if source_pool:
                 try:
                     pool = self.conn.get_pool(source_pool)
-                    vol = pool.get_volume(path)
+                    if pool is not None:
+                        vol = pool.get_volume(path)
                 except KeyError:
-                    vol = None
+                    pass
             else:
                 vol = self.conn.get_vol_by_path(path)

Thanks,
Giuseppe