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

[Cole Robinson]

On 05/07/2015 11:27 AM, Giuseppe Scrivano wrote:
> 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
> 

ACK

- Cole