Bug 1350201

Summary: nmcli failed to modify IPv4 routes metric on i686 system
Product: [Fedora] Fedora Reporter: Vladislav Grigoryev <vg.aetera>
Component: NetworkManagerAssignee: Beniamino Galvani <bgalvani>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 24CC: bgalvani, dcbw, fgiudici, lkundrak, psimerda, thaller, vg.aetera
Target Milestone: ---   
Target Release: ---   
Hardware: i686   
OS: Linux   
Whiteboard:
Fixed In Version: NetworkManager-1.2.4-2.fc24 Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-08-19 19:49:56 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
[PATCH] cli: fix parsing of route metric on 32-bit archs
none
[PATCH v2] cli: fix parsing of route metric on 32-bit archs none

Description Vladislav Grigoryev 2016-06-26 12:15:53 UTC 
Description of problem:
$ nmcli connection add type ethernet con-name test ifname "*"
Connection 'test' (4029438a-eedd-4606-be32-6f9d2da6bea7) successfully added.
$ nmcli connection modify test ipv4.routes "10.10.10.0/24 192.168.1.254 10"
Error: failed to modify ipv4.routes: invalid metric '10'.

Version-Release number of selected component (if applicable):
NetworkManager-1.2.2-2.fc24.i686

How reproducible:
Always.

Steps to Reproduce:
1. Use i686.
2. nmcli connection add type ethernet con-name test ifname "*"
3. nmcli connection modify test ipv4.routes "10.10.10.0/24 192.168.1.254 10"

Actual results:
nmcli failed to modify IPv4 routes metric on i686 system.
Comment 1 Beniamino Galvani 2016-06-27 13:51:23 UTC 
Created attachment 1172885 [details]
[PATCH] cli: fix parsing of route metric on 32-bit archs

Untested patch.
Comment 2 Thomas Haller 2016-06-30 08:08:45 UTC 
(In reply to Beniamino Galvani from comment #1)
> Created attachment 1172885 [details]
> [PATCH] cli: fix parsing of route metric on 32-bit archs
> 
> Untested patch.

I think you have to initialize @metric, otherwise (depending on compiler optimiation flags) it may emit a warning about uninitialized variable.


and in "(gint64) (metric_valid ? metric : -1)" inside the ternary operator we have types "? unsigned long : signed". This will be promoted to signed long (I guess). Which is actually fine, but it still makes me hesitate for 5 seconds to reason whether it is correct. I'd avoid that by doing the cast inside the ternary operator.


Can you not just avoid that by doing:

  gint64 metric = -1;
  ...
      if (!nmc_string_to_uint (third, TRUE, 0, G_MAXUINT32, &tmp_ulong)) {
      }
      metric = tmp_ulong;





(btw. how hard can it be to get a string-to-int function right :) ? We should eventually replace this with _nm_utils_ascii_str_to_int64()).


rest, lgtm
Comment 3 Beniamino Galvani 2016-06-30 10:01:41 UTC 
Created attachment 1174454 [details]
[PATCH v2] cli: fix parsing of route metric on 32-bit archs

(In reply to Thomas Haller from comment #2)

> Can you not just avoid that by doing:

Changed, thanks.
Comment 4 Francesco Giudici 2016-06-30 10:29:55 UTC 
lgtm
Comment 5 Thomas Haller 2016-06-30 11:25:48 UTC 
v2 lgtm
Comment 6 Beniamino Galvani 2016-06-30 11:30:38 UTC 
Patch applied to master:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=938a4b35c64027a2a337fa8ac123fe97d230f785

and nm-1-2:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=nm-1-2&id=751e8b89144bfc1a61cad45e3626edf2dfc86180
Comment 7 Fedora Update System 2016-08-05 07:52:47 UTC 
NetworkManager-1.2.4-1.fc24 network-manager-applet-1.2.4-1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-f739ece3cf
Comment 8 Fedora Update System 2016-08-05 21:21:33 UTC 
NetworkManager-1.2.4-1.fc24, network-manager-applet-1.2.4-1.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-f739ece3cf
Comment 9 Fedora Update System 2016-08-18 16:38:52 UTC 
NetworkManager-1.2.4-2.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-fade485364
Comment 10 Fedora Update System 2016-08-19 00:56:46 UTC 
NetworkManager-1.2.4-2.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-fade485364
Comment 11 Fedora Update System 2016-08-19 10:29:01 UTC 
network-manager-applet-1.2.4-1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-f739ece3cf
Comment 12 Fedora Update System 2016-08-19 19:49:31 UTC 
NetworkManager-1.2.4-2.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.