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.
> // 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.
> // 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.
> // 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.
> // 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.
> // 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.
The bool parameter was already mentioned several times here and
will be replaced by an enum.
> // 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.
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]
