After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 747776 - tests: Use xml dump and arp to get IP
tests: Use xml dump and arp to get IP
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: tests
3.16.x
Other Linux
: Normal normal
: --
Assigned To: GNOME Boxes maintainer(s)
Vladimir Benes
Depends on:
Blocks:
 
 
Reported: 2015-04-13 13:23 UTC by vladimir benes
Modified: 2016-09-20 08:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tests: get vm IP from libvirt (6.13 KB, patch)
2015-04-13 13:24 UTC, vladimir benes
needs-work Details | Review
tests: get vm IP from libvirt directly (5.38 KB, patch)
2015-04-17 14:16 UTC, vladimir benes
needs-work Details | Review
tests: get VM IP from libvirt directly (5.37 KB, patch)
2015-05-27 08:14 UTC, vladimir benes
none Details | Review
tests: get VM IP from libvirt directly (4.78 KB, patch)
2015-05-28 11:47 UTC, vladimir benes
committed Details | Review

Description vladimir benes 2015-04-13 13:23:57 UTC
New style vm IP storing. Obtaining mac address from libvirt xml dump and using arp to get IP from it.
Comment 1 vladimir benes 2015-04-13 13:24:55 UTC
Created attachment 301457 [details] [review]
tests: get vm IP from libvirt

Obtain mac via xml dump and use arp to get IP from it.
Comment 2 Zeeshan Ali 2015-04-14 12:23:50 UTC
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. :)
Comment 3 vladimir benes 2015-04-17 14:16:08 UTC
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.
Comment 4 Zeeshan Ali 2015-04-24 14:59:02 UTC
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"
Comment 5 vladimir benes 2015-05-27 08:04:49 UTC
(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
Comment 6 vladimir benes 2015-05-27 08:14:04 UTC
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.
Comment 7 vladimir benes 2015-05-28 11:47:18 UTC
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.
Comment 8 vladimir benes 2015-05-28 11:47:58 UTC
there was one step that shouldn't go in this patch.
Comment 9 Zeeshan Ali 2015-05-28 17:24:53 UTC
(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?
Comment 10 vladimir benes 2015-05-28 21:26:28 UTC
(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
Comment 11 Zeeshan Ali 2015-05-29 12:40:34 UTC
Review of attachment 304147 [details] [review]:

ack
Comment 12 Zeeshan Ali 2015-05-29 12:43:40 UTC
Attachment 304147 [details] pushed as 606c801 - tests: get VM IP from libvirt directly