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!
