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
> https://github.com/aschnell/libstorage-bgl-eval/
>
> 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.
> http://lists.opensuse.org/yast-devel/2014-10/msg00045.html
> http://lists.opensuse.org/yast-devel/2014-12/msg00016.html
>
> 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
ago.
> 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.
> 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)
>
> 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.
> 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
http://lists.opensuse.org/yast-devel/2014-12/msg00028.html 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
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
> 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.
Regards,
Arvin
--
Arvin Schnell, <[email protected]>
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
Germany
--
To unsubscribe, e-mail: [email protected]
To contact the owner, e-mail: [email protected]
