GNOME Bugzilla – Bug 750700
use ip neigh instead of arp command for getting IP
Last modified: 2016-09-20 08:15:55 UTC
Arp command is obsolete in favour of ip neigh. Add logic to avoid multiple IP addressed for single mac reporting. If such situation occurs only the newer (higher) IP is reported. This happens during express installations.
Created attachment 304962 [details] [review] tests: Exchange arp command for ip neigh Arp command is obsolete in favour of ip neigh. Add a logic to avoid multiple IP addressed for single mac reporting. If such situation occurs only the newer (higher) IP is reported. This happens during express installations.
Created attachment 304975 [details] [review] tests: Exchange arp command for ip neigh Arp command is obsolete in favour of ip neigh. Add a logic to avoid multiple IP addressed for single mac reporting. If such situation occurs only the newer (higher) IP is reported. This happens during express installations.
Created attachment 304976 [details] [review] tests: Exchange arp command for ip neigh Arp command is obsolete in favour of ip neigh. Add a logic to avoid multiple IP addresses for single mac reporting. It may happen during express installation and in such case only the newer (higher) IP is reported.
Review of attachment 304976 [details] [review]: * "Exchange arp command for ip neigh" -> "Replace `arp` cmd w/ `ip neigh`". Surrounding "ip neigh" by either `` or "" would improve readability slightly. * "is obsolete in" -> "has been obsoleted in". * Second para in description makes me thing these are two logical changes or is this change simply a side-affect? In case its a side-affect, please update description to reflect so. ::: tests/steps/general.py @@ +187,3 @@ ret = cmd.wait() + + #this happens from time to time after express install * What happens? This comment makes it sound like this "if" is looking for an erroneous condition while the block under it is the only code that actually returns an IP address. * Space after # and capitalize first letter please.
Created attachment 305583 [details] [review] tests: Obtain IP from libvirt's log Exhange getting IP via arp command with reading libvirt's log.
Review of attachment 305583 [details] [review]: Interesting so the patch has completely changed now? I don't think I like this approach since log formats can easily change, while output format of unix system utilities rarely change.
as I've filed https://bugzilla.gnome.org/show_bug.cgi?id=752955 this is very likely not needed anymore.. I thought that this would fix the issue but it's very likely somewhere else.
Created attachment 308301 [details] [review] tests: Exchange arp command for ip neigh Arp command has been obsoleted in favour of ip neigh.
ok, I've changed arp command to ip neighbour which seems to be the best solution. Please review.
(In reply to vladimir benes from comment #9) > ok, I've changed arp command to ip neighbour which seems to be the best > solution. Please review. Judging solely from shortlog, I'm not sure you addressed my comments from previous review to this patch in comment#4.
Created attachment 308327 [details] [review] tests: Exchange arp command for ip neigh Arp command has been obsoleted in favour of ip neigh.
(In reply to Zeeshan Ali (Khattak) from comment #10) > (In reply to vladimir benes from comment #9) > > ok, I've changed arp command to ip neighbour which seems to be the best > > solution. Please review. > > Judging solely from shortlog, I'm not sure you addressed my comments from > previous review to this patch in comment#4. no, the second if is gone as I do use ip neigh nud reachable so there shouldn't be any duplicate ip to one mac. I've updated function names too. so now really ready for review. #comment is gone as well.
Review of attachment 308327 [details] [review]: Well this first comment from previous review was definitely missed: * "Exchange arp command for ip neigh" -> "Replace `arp` cmd w/ `ip neigh`". Surrounding "ip neigh" by either `` or "" would improve readability slightly. ::: tests/steps/general.py @@ -185,3 @@ return mac -def get_ip_from_libvirts_log(mac): I think this patch is based on the previous (not merged) patch on this bug cause I never merged your "get ip from libvirt log" patch.
Created attachment 308403 [details] [review] tests: Replace "arp" cmd w/ "ip neigh" "Arp" command has been obsoleted in favour of "ip neigh".
Ah yes, now based on master. Sorry for confusion.
Attachment 308403 [details] pushed as 22a6992 - tests: Replace "arp" cmd w/ "ip neigh"