
Hi, let me do some high level user oriented review of new storage API. At first I think it is good that design decisions are documented, which is nice. Also having goals and requirements is documented which is fine to understand some decisions. What I miss is all requirements, I think it would be nice to write down all features old libstorage have and maybe discuss if it still make sense in new libstorage. Now first note. I do not see in decisions why C++ is used as language for this library. I think it would be nice to document it why it is needed as currently from document I see that planned users are machinery ( which use ruby ), Yast (ruby) and kiwi (perl). Is there any performance reasons, availability of bindings, libraries or any other reason to use it? As libstorage basically use CLI of other programs, so question is why not use more high level language. 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 // 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"); // 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); // 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); // 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"); // 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("/"); // 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(); // 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, "/")) { // 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)) { // 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). I think it make sense to write down an example ( or better test case in something like rspec) for each requirement and talk how its usage looks for target user and if it easy to use and intuitive. Also this examples can be something to show for review for potentional users of library and they can said if it fits their needs or if they missing something. Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org