27/07/13 19:34, intrigeri wrote: > Hi, > > anonym wrote (26 Jul 2013 12:46:34 GMT) : >> branch: test/reorg
I'll push fixes into test/firewall-check-tag to avoid some unnecessary merge conflicts. > In the VMNet class, the clean_up and destroy methods do very similar > things. It's not obvious to me when I should use the one as opposed to > the other. May you please add comments or rdoc or something to explain > how their purpose is different? Done, and I marked `clean_up` as private (commit bdd57a0). `destroy` is the correct interface for this, to stick to libvirt/virsh terminology. > Also, it may be the Ruby way to design OO stuff, but having 2 methods > (clean_up and destroy) implicitly depend on a 3rd one having been > successfully run on the instance before looks fragile to me. > Perhaps instead use a read-only accessor (possibly already provided by > attr_reader?) that errors out if the attribute wasn't set yet? > Or simply force the `update' step, that apparently is cricitally > needed for the instance to do anything useful, to run at > construction time? I'm not sure what you're getting at here. Care to elaborate? Is the "3rd one" here 'update'? It probably should be marked private, but I first want to understand what you mean. > And while we're at it, once `clean_up' depends on `update' to work > properly, I don't get why we don't use the @net attribute that was > initialized in `update', and instead look it up again by name. > Or perhaps that `net' there isn't the same as the @net > instance attribute? It's not the same. clean_up removes any networks with the same name, and they can have been created in a previous run (so we don't have the Libvirt::Network object to refer to) that weren't properly cleaned up (e.g. aborted). > This: > >> net_xml = REXML::Document.new(@vmnet.net.xml_desc) >> @ip = net_xml.elements['network/ip/dhcp/host/'].attributes['ip'] >> @mac = net_xml.elements['network/ip/dhcp/host/'].attributes['mac'] > > in the VM class, looks like broken encapsulation to me: the VM class > shouldn't need to go that far into low-level implementation details of > the @vmnet instance. Perhaps provide helper `ip' and `mac' public > methods in VMNet (with some kind of caching or memoization to avoid > recomputing it every time)? This is just my bad, plain and simple. Fixed this in commit 942a030. Cheers! _______________________________________________ tails-dev mailing list [email protected] https://mailman.boum.org/listinfo/tails-dev
