Hi,
anonym wrote (06 Aug 2013 13:58:20 GMT) :
> I'll push fixes into test/firewall-check-tag to avoid some unnecessary
> merge conflicts.
Thanks, and sorry for the delay (I hoped I would be able to do so +
run the test suite + merge in one single pass, but let's give up and
just start with another pass of static review).
>> 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.
Cool.
> + # Used internally to clean up previous created networks same name.
Missing word?
> + private :clean_up
Is it customary in Ruby to declare this kind of method properties at
the end of the class (instead of close to the method definition as I'm
more used to with other languages)?
>> 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.
I was meaning that `destroy' depends on the @net attribute and
`clean_up' depends on @net_name, both being initialized by another
method (`update') with unclear responsibilities, that seems to act as
a builder for the @net and @net_name attributes, as well as doing
other internal state massaging.
I now realize `update' is unconditionally called by the constructor,
so it looks quite better than I initially thought. Let's forget it.
>> 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.
So please name it differently, else I might not be the only one to get
confused :)
>> 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.
Great. So, caching / memoization didn't look necessary?
Cheers!
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
