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.m....
// 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-b... 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
(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,