Mailinglist Archive: yast-devel (211 mails)

< Previous Next >
Re: [yast-devel] High level notes about new storage API
On Wed, 21 Oct 2015 17:30:50 +0200
Arvin Schnell <aschnell@xxxxxxxx> wrote:

On Wed, Oct 21, 2015 at 04:36:19PM +0200, Josef Reidinger wrote:

Now lets move to examples. From my user POV there is some confusing
API calls and parameters. For example lets use find1.cc as example
which I comment (I can do this for all examples).

// creating some global storage is fine
Devicegraph* devicegraph = new Devicegraph();

// this looks strange for me
// 1) how disk can be created? I expect disk is detected or proposed

The probing code must also be able to create the object
representing a disk. With the Image/Kiwi mode you can create a
disk which is backed by a loop device.

What about using name "Disk::propose" for creating such disk? I think it
is just confusion caused by naming, as said I create in library disk a
bit confusing.


// 2) why saying that disk have "/dev/sda" what if I am on qemu
which // have "/dev/vda"?
Disk* sda = Disk::create(devicegraph, "/dev/sda");

The probing code uses the parameter. Currently you are basically
looking at functions to manipulate the device graph.

ok, if using propose, then it make sense to me as it propose disk with
given name, which is fine.


// this looks fine for me, creating partition table on disk
PartitionTable* gpt = sda->create_partition_table(PtType::GPT);

// here I do not get why I need to pass "/dev/sda1" ? Let me say it
// this way, if I have generic code that generate partition for
passed // partition table how I can know if it is hda, sda or vda?
and why // I need to know number of partition? cannot it by default
create next // available one and pass number as optional parameter?
gpt->create_partition("/dev/sda1", PRIMARY);
Partition* sda2 = gpt->create_partition("/dev/sda2", PRIMARY);

Use PartitionTable.get_unused_partition_slots() to get the name,
type and possible region. The interface will change to allow also
passing the region.

Great, so it already have such functionality. I just expect that create
partition can have reasonable defaults and use this method inside for
default parameters.
So you have

gpt->create_partition();

and it inside get available ones with above mentioned
get_unused_partition_slots. Of course it should support passing manual
names, region and so on. This way API will be simple for common usage
and powerfull if needed.


// here I create top level container, looks fine for me
LvmVg* system = LvmVg::create(devicegraph, "/dev/system");

// this is very confusing, why I need devicegraph here? Why
exposing it // to user? From doc I know that both sda2 and system
have reference to // it so why it need to be passed?
// And also why it need User::create? Why it is not simple
// `system.add(sda2)` call which is more intuitive for me?
User::create(devicegraph, sda2, system);

User::create is a low-level function to manipulate the device
graph. Functions like system.add_physcial_volume will be added.


Great.

// in general it looks fine for me, just is there reason to pass
// whole device name? who not having thing like
// `system->create_lvm_lv("root")`
LvmLv* system_root = system->create_lvm_lv("/dev/system/root");

This will change. For LVM there are only mostly empty classes
currently.

// creation of filesystem, intuitive and easy
Filesystem* filesystem = system_root->create_filesystem(EXT4);
// quite confusing, what is adding mountpoint to filesystem?
// filesystem do not know about mount points, I expect something
like // `devicegraph.mountpoint.add("/", filesystem)`
filesystem->add_mountpoint("/");

Only filesystems have mount-points so to me it looks natural to
handle them there. Alternatively we could add objects for
mount-points in the graph. BTW: E.g. ext4 knows its last mount
point (try dumpe2fs).

// some debug output, nothing to comment, not sure if it is needed
to // be public methods
cout << "num_devices: " << devicegraph->num_devices()<< endl;
cout << "num_holders: " << devicegraph->num_holders() << endl;
cout << endl;

// validation of storage, easy and intuitive... does it raise
exception // if failed?
devicegraph->check();

My idea are exceptions for fatal errors where the graph is so
broken that it cannot be used anymore, e.g. duplicate storage
id. Such an error indicates a libstorage internal problem. For
other things that the user can repair I think a list of issues
should be returned, e.g. the user created a raid with level 6 but
only added two drives. Here the user just has to add more drives,
change the level or remove the raid.

// printing of object, fine and intuitive C++ code
cout << devicegraph << endl;
// printing graphiz image, easy and intuitive API call
devicegraph->write_graphviz("test1.gv");

// looking for all filesystems that are mounted as root mountpoint
// is confusing. why it do not return single filesystem?
for (const Filesystem* filesystem :
Filesystem::find_by_mountpoint(devicegraph, "/")) {

You can mount a device at several mount-points. With btrfs this
is even standard, see
https://github.com/openSUSE/libstorage/blob/master/doc/status-current-code.md.

Yes, I know that device can have multiple mountpoints, but here is
logic reverse. It said that one mount point can have multiple devices
( or filesystem like used here ) which is at least for common usage
without overlapper layered mount points not possible. So on "/" is one
btrfs...of course you can get same btrfs also for /boot/grub2 etc.


// I am not sure if I can imagine what is ancestor of filesystem?
// I hope there is better name for it
// second note is boolean parameter, it is really hard to read it
and // hard to remember what such parameter mean. see e.g
//http://programmers.stackexchange.com/questions/147977/is-it-wrong-to-use-a-boolean-parameter-to-determine-behavior
for (const Device* device : filesystem->get_ancestors(false)) {

get_ancestors is the base function in Device and thus is not
specially named for filesystems. Otherwise the name is standard,
e.g. https://en.wikipedia.org/wiki/Tree_%28data_structure%29.

Yes, name make sense if you know that it is tree structure, but it is
inside POV, as user I try to read what api said to me and it said "get
me ancestors for given filesystem" which is a bit confusing and require
to know that filesystem is device and devices is stored in tree
structure and then you understand what code want you to say.
So more specialized call like
filesystem->get_device() or get_partition ( even if I know that disk
can be partitionless) can be more friendly for reader of code.


The bool parameter was already mentioned several times here and
will be replaced by an enum.

good


// this a bit break polymophysm, but it is explained in design
// document, so fine for me
if (dynamic_cast<const LvmLv*>(device))
cout << "mount point \"/\" somehow uses a logical
volume" << endl; }
}

delete devicegraph;

So in general, I think that we should now more focus on API and its
usability as it is hard to change it in future. When having good
API, some cleaning or refactoring of implementation later is easier
( current code in new libstorage is short and easy, but I worry in
future with more features, we need to be prepared to clean it also).

We are focused on the API at the moment.


Good, that brings me two ideas.

1) I can do such API review for all examples in example directory if it
make sense for you

2) maybe it make sense to write down expected use cases for library
usage and write example code for such use cases which can be discussed.

Josef

Regards,
Arvin


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

< Previous Next >
List Navigation