(In reply to Thomas Haller from comment #0)
> $ nmcli --terse -f all connection show $NAME | grep ipv4.dad-timeout
> ipv4.dad-timeout:-1 (default)
> 
> at least in "terse" mode, the output should be a value that can be fed back
> into nmcli.
> 
> $ nmcli connection modify $NAME \
>      ipv4.dad-timeout \
>      "$(nmcli --terse -f all connection show $NAME |
>         grep ipv4.dad-timeout |
>         sed 's/^[^:]*://')"
> Error: failed to modify ipv4.dad-timeout: '-1 (default)' is not a valid
> number (or out of range).
> 

pushed upstream branch fg/nmcli_parsing_rh1391170
I am a little worried that this could break scripts that already manage the "extra" strings in terse mode that are currently displayed.
But I agree that we have to output parsable values when the terse option is specified.


> 
> Maybe we can also consider:
> 
> 
> 1)
> 
>   nmcli -m tabular --terse -f ipv4.dad-timeout connection show $NAME 
> 
> prints a trailing newline. Not sure if that is great... ok, command
> expansion $(), `` will eat the newline, but maybe it would be better to
> suppress it(?). 
> 
> 2) --escape and "--mode" seem over-designed. Somebody either wants it pretty
> (to look at it) or parsable... requiring the user to understand that he
> needs "-m tabular --terse -f $ONE_PROPERTY" simply to get a parsable values
> seems too much.

1) Not sure about the newline. I would live it there... but no strong opinion on this.
2) Here I am much more worried to break existing scripts but I agree that having a plain, parsable output deserve a quicker and easier option.
What about adding another option to group "-m tabular --terse -f $ONE_PROPERTY" ? Something like "--raw-value / -r" (yeah, I know, this option name is terrible, I was looking for a name starting with a letter unused in current options...) ?

The branch is nice, but maybe we shouldn't ~fix~ existing format -- as you say.

How about deprecating existing options
    -t[erse]
    -m[ode] tabular|multiline
    -e[scape] yes|no
and add a new --raw output? (Or --verbatim, but then -v conflicts with the usual meaning --verbose)?

But I don't even understand how the existing options interact with each other. I think it's a bit of a mess.





Regarding the trailing '\n'. I changed my opinion. Maybe it's fine to *always* add one additional '\n'. Then a sufficiently sophisticated shell user can do:

  A="$(nmcli -t -m tabular -f proxy.pac-script connection show $C
       && printf 'x')"
  A="${A%$'\n'x}"

ugly, but at least correct.

(In reply to Thomas Haller from comment #3)
> The branch is nice, but maybe we shouldn't ~fix~ existing format -- as you
> say.

Well, regarding "--", I think we should ~fix~ (change) even more: some empty/unset values are printed as "--" while other just leave blank when unset.
I think we have to keep the blank output in terse mode and print "--" in other modes (pretty and normal).
This could potentially break something but output should be coherent.

> 
> How about deprecating existing options
>     -t[erse]
>     -m[ode] tabular|multiline
>     -e[scape] yes|no
> and add a new --raw output? (Or --verbatim, but then -v conflicts with the
> usual meaning --verbose)?
> 
> But I don't even understand how the existing options interact with each
> other. I think it's a bit of a mess.
> 

Well, I reviewed the option which basically maps to:
1. (-t / -p)   NMCPrintOutput print_output (NORMAL, TERSE, PRETTY)
               a. NORMAL --> output is aligned
               b. TERSE  --> remove output alignment + ':' separator (in tabular mode) wrt NORMAL
               c. PRETTY --> add extra headings wrt NORMAL

2. (-m <mode>) gboolean multiline_output
               gboolean multiline_mode_specified
               a. one property per line or on same line tab/space separated?
               b. default is FALSE, but for:
                  1. "connection show <ID>"
                  2. "device show"
                  3. "device lldp"
                  4. connection editor

3. (-e)        gboolean escape_values
               a. escape values but JUST FOR TERSE print_output and multiline_output FALSE (TABULAR)
               b. defaults to true

4. (-c)        NmcColorOption use_colors (YES, NO, AUTO)
               a. defaults to AUTO, generally do the right thing (color only if term specified)


I think that they allow fine tuning of the output. Not that bad. After thinking to them a while I think is good to allow such fine-grained control... I would not deprecate them. But for sure we should allow a better default for terse mode and a quick option for retrieving easily one "parsable" val.
So, I think we should do:
I)   1.b should suppress all "--" (DONE)
II)  1.a and 1.c should always return "--" when no value is set/empty
III) 2.b should be ALWAYS FALSE when 1.b, no exceptions
IV)  2.b.2 is incoherent output than with "connection show", shouldn't we align this?
V)   avoid enforcing -f for 1.b (-t) everywhere (or enforce it always)
VI)  add a new option (-g[et-var] <property>?) that is equivalent to "-t -f <property>"

> 
> Regarding the trailing '\n'. I changed my opinion. Maybe it's fine to
> *always* add one additional '\n'. Then a sufficiently sophisticated shell
> user can do:
> 
>   A="$(nmcli -t -m tabular -f proxy.pac-script connection show $C
>        && printf 'x')"
>   A="${A%$'\n'x}"
> 
> ugly, but at least correct.

Agreed.

Well, after working on the code for a while and playing a bit with nmcli I have slightly changed my mind and I would do:

1) In terse mode (-t) avoid printing "--" and strings that cannot be feed back in nmcli
2) In normal/pretty mode always print "--" for null/empty values
3) Avoid enforcing -f(ields) when -t(erse) is specified (do it everywhere)
4) Add a new -g(et-var) option equivalent to "-t -f <property>" to quickly retrieve a single specified property

I split the work in two branches, for easier management/review.

Please, review branch: fg/nmcli_parsing_part_I_rh1391170 which addresses points 1) and 2).

Thanks

Second part completed too, put on top of fg/nmcli_parsing_part_I_rh1391170.

Final, complete branch is:
fg/nmcli_parsing_rh1391170

Summing up the main changes:

1) Terse mode (-t) no more prints value strings that cannot be feed back in nmcli
2) normal/pretty modes always print "--" for null/empty values
3) -f(ields) is no more required for terse (-t) mode
4) New -g(et-values) option equivalent to "-m tabular -t -f <property>": its main usage should be to quickly retrieve one or more clean values, e.g.:

# nmcli -g ipv4.dad-timeout connection show $NAME
# -1

output will be just a value (no headers) that can be feed back in nmcli


You can also get more values separated by comma and you will get clean values, one per line. E.g.:

# nmcli -g ipv4.dad-timeout,ipv4.method connection  show
# -1
# auto
-----------------

Please review:
fg/nmcli_parsing_rh1391170

+                               (*((char *) field->value) ? (char *) field->value :
+                                                          (char *) not_set_str)) :


could you refine the line-wrapping so that it is clearer? Like

                  condition
                      ? expr1
                      : expr2

btw, get_value_to_print() is horrible. It's totally unclear how ownership is passed. Like, casting "const char *" to "char *", and then
  free_value = field->value && is_array && !field_name;



+    if (multiline) {
+    /* --- Multiline mode --- */

wrong intention. But let's avoid such redundant comments that don't explain anything that the code already clearly states.


The signature of colorize_string() is wrong.
Can you change it from
   char *colorize_string (..., gboolean &dealloc)
to
   const char *colorize_string (..., char **out_to_free);
and then:

   gs_free char *val_to_free;
   const char *val;

   val = colorize_string (..., &val_to_free);

let's avoid casting away the constness.






+    /* Lazy way to retrieve sorted array from 0 to the number of dev fields */
     nmc->print_fields.indices = parse_output_fields (NMC_FIELDS_DEV_SHOW_GENERAL_ALL,
                                                      nmc_fields_dev_show_general, FALSE, NULL, NULL);

NmcOutputField/parse_output_fields() is hard to follow (and very wrong because it merges formatting-options, setting-meta-data, what-to-output-options, and the output *sigh*).
If code cannot be self-explanatory, that is not going to fixed by a comment (which I don't understand). Anyway, unless it's improved, the comment is fine.



> nmcli: fix missing CONNECTION section name from nmcli -f all -m tab dev show <dev>


can you include an example in the comment message of "before"/"after". Or only "before" and pointing out what's wrong.
Or at least, it's unclear to me why the patch adds "NAME" but the subject talks about "CONNECTION section". Maybe just the subject is wrong?


+                  "  -g[et-values] <field1,field2,...>|all|common shortcut for -m tabular -t -f <args>\n"

The separation between "common shortcut" is not clear. Maybe more spaces or line-wrap.



> man: add new nmcli section for the --get-values option

merge in previous commit.



Rest lgtm

Pushed a fixup.

I'm a bit worried about the change in --terse output. What are the chances anyone's scripts are going to break?

Please consider adding an example of --get-values to nmcli-examples manual.

The rest looks good to me.

(In reply to Lubomir Rintel from comment #8)
> Pushed a fixup.

Thanks! Merged in the branch

> 
> I'm a bit worried about the change in --terse output. What are the chances
> anyone's scripts are going to break?

Yeah, they will break probably. This worries a bit me too, but terse mode output is buggy: consider the output of empty values.
Sometimes was "--", sometimes was just empty. Now we are coherent and we leave it empty every time. This is going to break scripts too, but is something that in my opinion we needed to address sooner or later (better sooner).
And as we start fixing terse mode, let's do it once for all... I think having output that could be feed back is smart for terse mode. Moreover, single values without parenthesis are easier to manage. I think is a slightly painful change but is something we should do.

> 
> Please consider adding an example of --get-values to nmcli-examples manual.

sure, done thanks

> 
> The rest looks good to me.

(In reply to Thomas Haller from comment #7)
> +                               (*((char *) field->value) ? (char *)
> field->value :
> +                                                          (char *)
> not_set_str)) :
> 
> 
> could you refine the line-wrapping so that it is clearer? Like
> 
>                   condition
>                       ? expr1
>                       : expr2
> 

Yeah, much better...  done thanks.


> btw, get_value_to_print() is horrible. It's totally unclear how ownership is
> passed. Like, casting "const char *" to "char *", and then
>   free_value = field->value && is_array && !field_name;

I don't like it also, nor the "print_required_fields".
I think a major, radical rework would be beneficial...

> 
> 
> 
> +    if (multiline) {
> +    /* --- Multiline mode --- */
> 
> wrong intention. But let's avoid such redundant comments that don't explain
> anything that the code already clearly states.

Removed

> 
> 
> The signature of colorize_string() is wrong.
> Can you change it from
>    char *colorize_string (..., gboolean &dealloc)
> to
>    const char *colorize_string (..., char **out_to_free);
> and then:
> 
>    gs_free char *val_to_free;
>    const char *val;
> 
>    val = colorize_string (..., &val_to_free);
> 
> let's avoid casting away the constness.
> 

Well, colorize_string() is called also by get_value_to_print() which passes over the "dealloc" pointer to let the caller do the free.
Fixed both function signatures.
New commit added on top of the branch.

> 
> 
> 
> 
> 
> +    /* Lazy way to retrieve sorted array from 0 to the number of dev fields
> */
>      nmc->print_fields.indices = parse_output_fields
> (NMC_FIELDS_DEV_SHOW_GENERAL_ALL,
>                                                      
> nmc_fields_dev_show_general, FALSE, NULL, NULL);
> 
> NmcOutputField/parse_output_fields() is hard to follow (and very wrong
> because it merges formatting-options, setting-meta-data,
> what-to-output-options, and the output *sigh*).
> If code cannot be self-explanatory, that is not going to fixed by a comment
> (which I don't understand). Anyway, unless it's improved, the comment is
> fine.
> 

Yeah, it has been a little bit hard to follow the code.
I think this part deserves a major rework... in the meanwhile I put just that comment to remember that parse_output_fields is used there just to generate an array populated with field labels...
If don't mind I would like to let it there till that part will be reworked... not a very helpful comment, I agree, just an harmless mark.

> 
> 
> > nmcli: fix missing CONNECTION section name from nmcli -f all -m tab dev show <dev>
> 
> 
> can you include an example in the comment message of "before"/"after". Or
> only "before" and pointing out what's wrong.
> Or at least, it's unclear to me why the patch adds "NAME" but the subject
> talks about "CONNECTION section". Maybe just the subject is wrong?

Err, was not CONNECTION but CONNECTIONS. Explained a bit more in the message, added output sample too. Thanks.

> 
> 
> +                  "  -g[et-values] <field1,field2,...>|all|common shortcut
> for -m tabular -t -f <args>\n"
> 
> The separation between "common shortcut" is not clear. Maybe more spaces or
> line-wrap.

added a couple of spaces and removed "<args>", thanks

> 
> 
> 
> > man: add new nmcli section for the --get-values option
> 
> merge in previous commit.

Done

> 
> 
> 
> Rest lgtm

Thanks

Repushed with all the fixes and changes suggested (also from Lubomir):
fg/nmcli_parsing_rh1391170

Merged upstream:
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=5526769950c81cf0f410a852b1fcfb70c9e9b158

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2017:2299