Mailinglist Archive: yast-devel (211 mails)

< Previous Next >
Re: [yast-devel] Some notes about new storage API
On Wed, Oct 07, 2015 at 02:12:44PM +0200, Ancor Gonzalez Sosa wrote:

As part of the PBI titles "Evaluate design of future-proof storage
subsystem", I took a look to the content of

This mail can be considered to some extend a follow up of previous
evaluations and discussions in this mailing list. Most feedback received
from those threads has been already taken into account into the current
prototype. Thus, some topics discussed there are outdated but some
information is still useful.

First impression

First thing I noticed in that repository is the lack of information
about building and installing. In every github's README I expect to
find: (1) how to build the thing, (2) how to compile the inline docs or
a link to an up-to-date compiled version, (3) in (open)SUSE related
stuff I also expect instructions to download or build a rpm.

It's a prototype and development was halted abrupt eight months

Only some (maybe too Ruby-centric) comments follow.

Casting and Storage.some_method

Things like this look quite un-ruby:

tmp1 = devicegraph.find_device(42)
assert_raises(Storage.DeviceHasWrongType {

I would have expected things like


At least in C++ such a interface looks like a very bad idea since
for *every* new class you have to modify the base class. With
such a design ABI stability is not possible.

In fact, I wouldn't expect to have to perform this kind of

These cast should not be needed often as I already wrote in

at most I would expect to do more rubist things like


Query interface

The API contains methods like these:

Storage::BlkDevice.find(device_graph, "/dev/sda")
Storage::Filesystem.find_by_mountpoint(device_graph, "/")
Storage::Filesystem.find_by_label(device_graph, "someLabel")

Questions that come to my mind.

a) Is device_graph always an object representing the whole

The devicegraph always represents the whole graph.

Can I do things like this to restrict the search to a subtree?

disk1 = Storage::Disk.find(device_graph, "/dev/sda")
sub_graph = disk1.to_device_graph
# or maybe sub_graph = device_graph.sub_graph(disk1.sid)
# or any other thing that makes sense

In theory there are many ways to get a subgraph, look at the
functions in Device.h.

Storage::Filesystem.find_by_mountpoint(sub_graph, "/")

Do you have a real use-case for that? Adding subgraphs looks like
a lot of work so might not be worthwhile.

b) Adding find_by_xx at demand over time doesn't look very future-proof.
I would prefer to see something like Rails's #find_by, i.e.

Storage::Filesystem.find(device_graph, mountpoint: "/")
Storage::Filesystem.find(device_graph, mountpoint: "/", label: "blah")

Too ruby-centric and not easy to implement in a cross-language way, I
guess. But asking does not hurt. :-)

As I already wrote in if
someone takes care of bindings for a target language more things
are possible.

c) Just brainstorming. The approach in (b) could be pushed to the limit
to have things like this. But actually I'm not sure if it's a good idea,
just braindump.

device_graph.find(sid: numeric_id)
device_graph.find(type: :filesystem, mountpoint: "/")

Lack of inline documentation

Right now, the code is the documentation. I have to say that the code is
very nicely structured, so it was really easy to me to browse for the
methods I wanted to check (and they were all fairly small and
understandable as well). But it's time to start adding documentation to
the whole thing.

It's a prototype and development was halted abrupt eight months

Nitpicking: boolean as parameters

This is something that was already mentioned by Martin in one of the
mails referred at the top. Calls like this are not 100% obvious


It would be nicer to have something like sda.descendants(:direct) or
sda.descendants(direct: true)

Looks difficult as already mentioned in

Nickpicking: 42

I found the usage of 42 and 43 for the sids in examples and tests a
little bit confusing. I grepped the source code and found that we simply
use autoincremental integer to assign the sids starting with 42.

Nothing against it, but please make it more obvious (and robust, in case
we decide 42 is not cool anymore) in the testsuites. :-)

There is no technical reason to change the initial value for the
storage id so FMPOV spending time on changing the testsuites is
not justified.


Arvin Schnell, <aschnell@xxxxxxxx>
Senior Software Engineer, Research & Development
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284
(AG Nürnberg)
Maxfeldstraße 5
90409 Nürnberg
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >
Follow Ups