On 05/07/2015 11:27 AM, Giuseppe Scrivano wrote: > 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 > ACK - Cole _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
