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

- Cole