On Mon, May 04, 2015 at 05:21:29AM -0400, Martin Polednik wrote: > > > ----- Original Message ----- > > From: "Dan Kenigsberg" <dan...@redhat.com> > > To: "Martin Polednik" <mpoled...@redhat.com> > > Cc: devel@ovirt.org > > Sent: Friday, May 1, 2015 5:06:04 PM > > Subject: Re: [ovirt-devel] [vdsm] VmDevices rework > > > > On Tue, Apr 28, 2015 at 04:28:12AM -0400, Martin Polednik wrote: > > > Hello everyone, > > > > > > I have started working on line of patches that deal with current state of > > > VM devices > > > in VDSM. There is a wiki page at [1] describing the issues, phases and > > > final goals > > > for this work. Additionally, there is formal naming system for code > > > related > > > to > > > devices. I would love to hear your opinions and comments about this > > > effort! > > > (and please note that this is very long term work) > > > > > > [1] http://www.ovirt.org/Feature/VmDevices_rework > > > > Hi Martin, > > > > It's a great and just initiative. Renaming is good, but I'd like to hear > > more about your final goal - besides the hope that it the code is > > shorter. > > > > In my opion you should state your high-level goal explicitly: > > device_object should be self-sufficient regarding: initiation, state > > change, stat extraction and stat reporting. > > > > Comments: > > 1. We want to drop the legacy conf support. In > > https://gerrit.ovirt.org/40104 we have removed support of engine<3.3 > > so production environment does not need it any more. > > > > There might be an issue with command-line test scripts, though that > > might be abusing the legacy args. At the very least it should be > > marked as deprecated. > > Therefore, the move still makes sense, as for the deprecation - I fully > agree. Will try to figure a nice way to mention it (logging and docs I guess).
At the very least, you should mark any use of legacy with a log ERROR. > > > 2. dev_map is an unfortunate historical mistake. The fact that it is a > > map of device type to device_object lists helps no one. It > > should be hidden behind one or two lookup functions. Each object > > class should be able to iterate its instances, and add filtering on > > top of that. > > Interesting point, but you still need to somehow store the device > instances. Having the implementation hidden is fine (and the _device name > already suggests that) but it still needs to exist. Of course. And you may keep the current mapping for the mean while. But it should be layered away by a set of accessors. > > > 3. A definition like "Type of the device is dev_type" does not help for > > someone who knows what is dev_type. Please give examples. > > The "formal naming system" could use some wiki styling. > > Will work on that! > > > 4. I did not understand "Libvirt XML parsing and processing is dumb - > > missing orchestration object to delegate XML chunks to devices themself" > > Too terse indeed, will try to rephrase. The issue is the fact that each > dev_type > parsing iterates over all device elements in XML, leading to O(n^2) complexity > just for the parsing. The it's not XML parsing per se, but the convesion of the data into objects. Please explain this issue on wiki. > > > 5. It seems that you have further "phases" planned, but not written. > > Could you provide at least the title, or "charter" of each phase? 1.1 > > is about renaming. 1.2 is about legacy. Do you have phases 2, 3..? > > Still in process of figuring out the naming of the phases and their order, > I'll try to add them when it makes sense (I just can't spit 10 names out of > my head right now that would make sense in a long term). > > > Death (or at least diet) to vm.py! > > +1 :) Thanks for the review! _______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel