[yast-devel] New libstorage: Returning NULL pointers vs. throwing exceptions

24 Nov 2015

      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.

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

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.

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".

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.

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!).

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).

Kind regards
-- 
Stefan Hundhammer  <shundhammer@suse.de>
YaST Developer

SUSE Linux GmbH
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Maxfeldstr. 5, 90409 Nürnberg, Germany
-- 
To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org
To contact the owner, e-mail: yast-devel+owner@opensuse.org

Stefan Hundhammer

Josef Reidinger

Ancor Gonzalez Sosa

Stefan Hundhammer

Josef Reidinger

Ladislav Slezak

Arvin Schnell

Josef Reidinger

Arvin Schnell

Ladislav Slezak

Arvin Schnell

Josef Reidinger

Arvin Schnell

Arvin Schnell

Josef Reidinger

Arvin Schnell

tags

participants (5)