Do not hide libzypp exceptions in Pkg::ResolvableProperties()

[lslezak]

See bnc#895418

• it makes debugging more difficult, return nil and log details in that case
• 3.1.18

[jreidinger]

LGTM

[mvidner]

No.

[mvidner]

I agree that silently swallowing exceptions with catch(...) {} must be fixed. But are you sure you want to change the return value too, at this stage? Before, the returned value would be an empty list, IIUC. There may well be a code path where we rely on ignoring a zypp exception and work with the empty list. Nil would break that.

Also, the fix is incomplete. ResolvablePropertiesEx calls Resolvable2YCPMap which still contains a black hole for exceptions.

[lslezak]

Before, the returned value would be an empty list

Not really, the empty list was returned because the exception was raised at the first item, but it can happen later and in that case the returned list would be incomplete (containing just the already processed items).

And in that case debugging would be even more difficult, with empty list it's obvious that something went wrong, but incomplete list is really nasty...

There may well be a code path where we rely on ignoring a zypp exception and work with the empty list. Nil would break that.

No, actually nil could be already returned in the existing code, see https://github.com/yast/yast-pkg-bindings/blob/master/src/Resolvable_Properties.cc#L641.

I'm not sure if there is such code which relies on ignoring libzypp errors (I'm not aware of any case), the pkg-bindings convention is use nil as the error value. If it suddenly fails because of nil then IHMO it's better then hiding a problem and use a wrong result value.

Also, the fix is incomplete. ResolvablePropertiesEx calls Resolvable2YCPMap which still contains a black hole for exceptions

No, if an exception is raised there it will caught here as well.

[lslezak]

In normal conditions nil should be never returned, only in some bad situations, like in the referenced bug where there is a broken product contained in the repository.

So yes, I'd like to change it even in this stage.

[mvidner]

actually nil could be already returned in the existing code

OK, but that is only for kind == "language" which is an uncommon case I think. Anyway, your voice has more weight, I've just wanted to make sure you have considered the consequences carefully.

if an exception is raised there it will caught here as well.

I don't think so. This code eats it:

yast-pkg-bindings/src/Resolvable_Properties.cc

Line 575 in 95a3ec2

catch (...)

[lslezak]

Oh, I overlooked that part, thanks!

[mvidner]

LGTM.

[lslezak]

Thanks for review!