Mailinglist Archive: yast-devel (127 mails)

< Previous Next >
Re: [yast-devel] New libstorage: Returning NULL pointers vs. throwing exceptions
On Tue, 24 Nov 2015 14:45:45 +0100
Stefan Hundhammer <shundhammer@xxxxxxx> wrote:

When we put the new libstorage to the test in our tech previews, we
had one typical situation occuring again and again: I get a pointer
(or object reference in Ruby) to a base class object (typically
BlkDevice), and I need to know what it actually is, or even just if
it exists.

Typical example:

I need to check each filesystem on a disk for certain things.
Since the disk might or might not have a partition table, first I
need to check if it has one, and if it does, iterate over all
partitions.

If there is no partition table, there might be a filesystem directly
on the disk (on, say, /dev/sdc rather than /dev/sdc1 - this might
easily happen with USB sticks), so I need to check that, too.

Now there are two approaches with an API like libstorage:

(a) I query the disk for its partition table. If it doesn't have one,
I get a NULL pointer (or nil object reference in Ruby).

(b) I query the disk for its partition table. If it doesn't have one,
an exception is thrown.

At first thought, getting an exception for a quite common situation
sounds like a bad idea. Worse, I might be tempted to introduce a new
functionality:

(c) Check if the disk has a partition table and only fetch that
pointer/object reference if it does. If I fetch it when there is
none, an exception is thrown.

Basically, (c) is combining the worst of (a) and (b): I have to do a
lot of checks and I might still get an exception. Worse, what happens
behind the scenes when I call that has_partition_table() (C++) or
partition_table? (Ruby) function is exactly the same as in (a): The
lib fetches the pointer and checks if the object it points to has the
desired type.

Well, in OOP is also one more approach called NullObject pattern, which
is create kind if partition table, that in represent no partition
table. Let me below show how it will behave. ( and yes, I know it is a
bit controversial pattern -
https://en.wikipedia.org/wiki/Null_Object_pattern


In scenario (a), the corresponding Ruby code looks like this:


# Check if a disk is our installation disk - the medium we just booted
# and started the installation from. This will check all filesystems
on # that disk.
#
def installation_disk?(disk)
log.info("Checking if #{disk} is an installation disk")
partition_table = disk.partition_table
if partition_table
partition_table.partitions.each do |partition|
next if [::Storage::ID_SWAP,
::Storage::ID_EXTENDED].include?(partition.id)
return true if installation_volume?(partition)
end
return false # No installation partition in the partition table
end

# Check if there is a filesystem directly on the disk
# (without partition table). This is very common for installation
# media such as USB sticks.

installation_volume?(disk)
end


so if NoPartitionTable is used and its partitions return single
partition, that is in fact filesystem on disk.

def installation_disk?(disk)
log.info("Checking if #{disk} is an installation disk")
disk.partition_table.partitions.each do |partition|
next if [::Storage::ID_SWAP,
::Storage::ID_EXTENDED].include?(partition.id)
return true if installation_volume?(partition)
end
return false # No installation partition in the partition table
end



Here, just one check was necessary: 'if partition_table'. We got
lucky here. If that expression was any longer, we might have to do a
cascade of such tests:

if disk && disk.partition_table && disk.partition_table.gpt?

This gets annoying pretty quickly, and it doesn't exactly make the
code better readable.

JFYI new ruby 2.3 will have &. operator for such cases, so you can then
write it like:

if disk&.partition_table&.gpt?

which looks a bit better, but still I prefer NullObject (for expected
situations ) and exceptions ( for bugs or errors )



Scenario (b) looks like this:

# Check if a disk is our installation disk - the medium we just booted
# and started the installation from. This will check all filesystems
on # that disk.
#
def installation_disk?(disk)
log.info("Checking if #{disk} is an installation disk")
begin
disk.partition_table.partitions.each do |partition|
next if [::Storage::ID_SWAP,
::Storage::ID_EXTENDED].include?(partition.id)
return true if installation_volume?(partition)
end
return false # if we get here, there is a partition table.
rescue Exception => ex # No partition table on this disk
log.info("CAUGHT exception: #{ex} for #{disk}")
end

# Check if there is a filesystem directly on the disk (without
# partition table). This is very common for installation media such
# as USB sticks.
return installation_volume?(disk)
end

Notice that there is no check if the disk has a partition table - we
just use it. Or, rather, we try to use it: That's why this is wrapped
into that begin..rescue block. If there is no partition table, the
lib will throw an exception, and code flow will continue in the
'rescue' clause.

Coming from C++ and a world where exceptions are evil and should not
occur within normal control flow, this sounds bad: We get an
exception in a situation that might easily happen without anything
being really broken. At least not broken in the sense that "uh-oh, we
have a real problem here, we can't reasonable continue".

It is very similar in ruby. Do not use exceptions for common situations.


OTOH if there would be more checks that might fail (such as the
'if disk && disk.partition_table && disk.partition_table.gpt?' check
above), that one 'begin..rescue' block would handle them all, and the
control flow remains clear and straightforward.

And that's the beauty of it.

Well, I am not sure as you should use more specialized exception
catcher, otherwise you can catch also other problem, that should go
more top level, so in the end, you might want to rescue from three
kinds of exception like

rescue NoPartitionTable, NoDisk, NotGpt => e

which is not so beauty.


Using exceptions and not checks all over the place can help to
clarify the algorithm without clobbering the code with that checking
code - code that is very rarely executed and thus mostly untested
(try to create test cases for all those if/else branches!).

on other hand testing all potential raiser of exception is also quite
tricky.


So, even though right now libstorage writes each exception to the log
and thus the log gets less pretty, we prefered to use the approach
with exceptions rather than having to check each pointer/object
reference.

We also strongly believe in making life easier for the developers
using the lib, even if that comes at the cost of making life harder
for the lib developers. If we forced the lib users to check every
piece of information they get from the lib because it might be
NULL/nil, we'd bury land mines all over the place: Every
pointer/object reference that might be NULL/nil is an accident
waiting to happen. Even though uncaught exceptions are a similar
class of errors, at least the exceptions carry enough information to
track down the problem - and you get a backtrace that tells you
what's going on at exactly the place where something goes wrong
(unless a nil reference that might be used much later in the code).


Still I think for common situation that can easily happen like disk
without partitions we should consider NullPattern approach, which *for
me* is in this situation better approach.

Just my 2c.

Josef



Kind regards

--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >
References