Cole Robinson <crobinso@xxxxxxxxxx> writes: > On 05/07/2015 08:33 AM, Giuseppe Scrivano wrote: >> Giuseppe Scrivano <gscrivan@xxxxxxxxxx> writes: >> >>> Giuseppe Scrivano <gscrivan@xxxxxxxxxx> writes: >>> >>>> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1219427 >>>> >>>> Signed-off-by: Giuseppe Scrivano <gscrivan@xxxxxxxxxx> >>>> --- >>>> 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 _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
