GNOME Bugzilla – Bug 747776
tests: Use xml dump and arp to get IP
Last modified: 2016-09-20 08:15:55 UTC
New style vm IP storing. Obtaining mac address from libvirt xml dump and using arp to get IP from it.
Created attachment 301457 [details] [review] tests: get vm IP from libvirt Obtain mac via xml dump and use arp to get IP from it.
Review of attachment 301457 [details] [review]: Good work in general. * short log seems wrong. We were already getting it from libvirt. Adding 'directly' at the end would make a whole lot of difference. :) * You should mention in the details how its obtained right now. * While i'm at it, i might get into real nitpicks too :) * mac -> MAC address * xml -> XML * arp -> arp command (to differentiate from ARP protocol). * 'from it' -> 'through it'. ::: tests/steps/general.py @@ +10,2 @@ import re +import libvirt While its good that you are using libvirt's API directly in this patch, I feel that now things a bit inconsistent in steps with some code using libvirt directly and rest using virsh commands. Best approach would have been to first convert the existing code to use libvirt directly but if you have plans to do that (not so) later, that is good too. @@ +131,3 @@ sleep(0.5) +def get_context(dom): We have dogtail's context involved in this code too so please add some prefix (e.g 'libvirt_domain') to this and get_val functions. @@ +144,3 @@ + return value + +def get_mac(title): I think some prefix to function and argument name would keep the code a lot more readable. Python being ducked type language means programmer has to do more efforts to write readable code. @@ +241,3 @@ +@step('Start showkey recording') +def start_showkey(context): Aren't you adding these as part of another patch in another bug? Hence they obviously do no belong here. :)
Created attachment 301848 [details] [review] tests: get vm IP from libvirt directly Obtain MAC address via XML dump and use arp command to get IP through it.
Review of attachment 301848 [details] [review]: almost there. While you fix it, please capitalize the abbreviation "vm" as "VM" also in the log. ::: tests/steps/general.py @@ +10,2 @@ import re +import libvirt This comment from previous review seems to have been ignored: "While its good that you are using libvirt's API directly in this patch, I feel that now things a bit inconsistent in steps with some code using libvirt directly and rest using virsh commands. Best approach would have been to first convert the existing code to use libvirt directly but if you have plans to do that (not so) later, that is good too." If the latter is true, I need your word on that in written here. :) @@ +131,3 @@ sleep(0.5) +def libvirt_domain_get_domain_context(dom): much better but i think we can do with 1 less 'domain' in name. @@ +172,3 @@ + return mac + +def arp_command_get_ip(mac): sounds weird, I'd name it "get_ip_through_arp_cmd"
(In reply to Zeeshan Ali (Khattak) from comment #4) snip.. > "While its good that you are using libvirt's API directly in this patch, I > feel that now things a bit inconsistent in steps with some code using > libvirt directly and rest using virsh commands. Best approach would have > been to first convert the existing code to use libvirt directly but if you > have plans to do that (not so) later, that is good too." > > If the latter is true, I need your word on that in written here. :) yes, I plan to do so! :-) in written now
Created attachment 304053 [details] [review] tests: get VM IP from libvirt directly Obtain MAC address via XML dump and use arp command to get IP through it.
Created attachment 304147 [details] [review] tests: get VM IP from libvirt directly Obtain MAC address via XML dump and use arp command to get IP through it.
there was one step that shouldn't go in this patch.
(In reply to vladimir benes from comment #8) > there was one step that shouldn't go in this patch. ah ok, so i should hold up on review and you provide new patch?
(In reply to Zeeshan Ali (Khattak) from comment #9) > (In reply to vladimir benes from comment #8) > > there was one step that shouldn't go in this patch. > > ah ok, so i should hold up on review and you provide new patch? no the patch from comment 7 is the final one
Review of attachment 304147 [details] [review]: ack
Attachment 304147 [details] pushed as 606c801 - tests: get VM IP from libvirt directly