Mailinglist Archive: yast-devel (211 mails)

< Previous Next >
Re: [yast-devel] Some notes about new storage API
On Tue, 13 Oct 2015 08:55:48 +0200
Ancor Gonzalez Sosa <ancor@xxxxxxx> wrote:

On 10/09/2015 02:54 PM, Arvin Schnell wrote:
On Wed, Oct 07, 2015 at 02:12:44PM +0200, Ancor Gonzalez Sosa wrote:

[,,,]

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(Storage.disk?(tmp1))
assert(Storage.to_disk(tmp1))
assert(!Storage.partition_table?(tmp1))
assert_raises(Storage.DeviceHasWrongType {
Storage.to_partition_table(tmp1)
}

I would have expected things like

tmp1.disk?
tmp1.to_partition_table

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.

Ok.

In general in object desing you do not need to have such questioning.
And if so, then maybe questioning like `MDRaid.can_use?(obj)` so it
questioning ability and not type.


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

These cast should not be needed often as I already wrote in
http://lists.opensuse.org/yast-devel/2014-12/msg00025.html.

at most I would expect to do more rubist things like

tmp1.is_a?(Storage::Disk)
tmp1.respond_to?(:whatever_method)

I'll do more experiments with this as soon as the prototype is easily
installable. But looks pretty clear that it would be fairly easy to
fix the un-rubism by adding the corresponding methods in the Ruby
side in case we decide we are so purist that we cannot live with the
current API. :-) So nothing to fix in the libstorage (C++) side.

I agree, that it make sense to have some helpers to have ruby
bindings more "ruby".


Query interface
===============

The API contains methods like these:

device_graph.find_device(numeric_id)
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
graph?

The devicegraph always represents the whole graph.


Well, in general I do not like this API before and I also do not like
it now :)
It is not object oriented and whats more device_graph is god like
object. What I expect is object like API that looks like

device_graph.find_device(id)
device_graph.find_device("/dev/sda", block: true)
device_graph.mount_points.get("/")
device_graph.labels.get("someLabel", filesystem: true)


this way it is more separated responsibility. In general now
device_graph looks like target map, which I would like to avoid. I
prefer to have graph of objects that points to each other and it is
easy to say what object it returns. It also allows easy introspection
and better documentation that allows easier usability.


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.

No concrete example, just wondering. But the idea of restricting the
search to a subtree somehow came naturally to my mind while reading.
Like "look for a filesystem with this property, but only in this
volume group".

Probably not worth the effort, I was just curious.

For bootloader it make sense to e.g. restrict search for boot flag for
one disk, as it is disk specific flag. Same way it can be interesting
to e.g. find intersections for known problem cases like
`device_graph.find(fs:
"nfs).include?(device_graph.mount_points.get("/boot")`


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
http://lists.opensuse.org/yast-devel/2014-12/msg00028.html if
someone takes care of bindings for a target language more things
are possible.

Yes. And it should be fairly easy to implement. But I still wonder
about how flexible and future-proof is to keep adding find_by_xx on
demand to the API.

I agree, almost all languages have maps that can be used to pass such
kind of options. In C it is usually done by flags. In C++ it is usually
done by functor. So maybe another option is to have generic find which
can take functor or lambda in way like

device_graph.mountpoints.find { |mp| mp.path == "/" || mp.label ==
"blah" }


More flexible solutions would also be more complex to use for sure. I
just wonder if they are worth exploring. One obvious solution (that
would need refinement, of course) if having a search object that
accepts any number of key-pair filters. The most primitive form just
to expose the idea would be something like (in pseudocode):

s = Storage.search(device_graph, "filesystem")
s.add_filter("mountpoint", "/")
s.first()

Or any other idea that can be implemented with a fixed set of classes
and methods instead of extending the existing classes with new custom
methods over time.

Agreed. number for methods for given object should be kept quite low,
otherwise it is hard to remember it and also learn it.


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.

I think that code is the best documentation if API is small and have
good names.

Josef


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

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

sda.descendants(false)

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

Looks difficult as already mentioned in
http://lists.opensuse.org/yast-devel/2014-12/msg00025.html

Ok.


[...]


Cheers.

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

< Previous Next >
Follow Ups