Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix show-xml filtering by interface name (issue #735,bsc#954758) #744

Merged
merged 3 commits into from
Mar 23, 2018
Merged

Fix show-xml filtering by interface name (issue #735,bsc#954758) #744

merged 3 commits into from
Mar 23, 2018

Conversation

rtorrero
Copy link
Contributor

This PR adds filtering functionality to the show-xml command, with the following behaviour:

  • wicked show-xml <ifname1> <ifname2>: show xml only for ifname1 and ifname2
  • wicked show-xml all: show xml for all available network interfaces
  • wicked show-xml: same as above, show all interfaces

This should address issue #735

@rtorrero
Copy link
Contributor Author

I'm not entirely happy with the double for loop, it looks messy. I'm open to suggestions if that can be improved.
If there is no better way to do it using any of the ni_dbus_variant or ni_dbus_dict functions, variable naming could use some love too :p.

@mtomaschewski mtomaschewski self-requested a review March 19, 2018 14:55
@mtomaschewski
Copy link
Member

Please improve & fix as commented via pm mail :-)

@rtorrero
Copy link
Contributor Author

Thanks for your comments. Hope those commits address all the issues. Awaiting your feedback :).

Copy link
Member

@mtomaschewski mtomaschewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please cleanup the (leading) space changes and adjust also the usage message? I'd also prefer to squash it into 1..3 clean & final commits instead of full evolution history adding 2nd loop & removing it again.

client/main.c Outdated
object_node = xml_node_new("object", parent);
xml_node_add_attr(object_node, "path", object_path);
object_node = xml_node_new("object", NULL);
xml_node_add_attr(object_node, "path", object_path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove the leading spaces. you may configure .gitconfig to show (and or fix) them, e.g. core.whitespace = fix,indent-with-non-tab,trailing-space

@rtorrero
Copy link
Contributor Author

I've added your additional suggestions (usage info and trailing whitespaces), but I did it before squashing the commits. Should I have done those afterwards?

@mtomaschewski
Copy link
Member

This looks very good now... There is still a bit odd usage inside of the show-xml, could you adjust it too please?

@rtorrero
Copy link
Contributor Author

I'm not sure how to improve that one, I'm looking at how other subcommands have their usage section and doing the < --raw | [ <ifname ...>|all ] > thing would mean that we have to remove the 'Supported options' part (as --raw is one of the options). Could you provide more details?

@mtomaschewski
Copy link
Member

Yes, your note should be sufficient to cover --raw case (until we refactor it to apply the filter to --raw too), but we should adjust at least:

- "wicked [options] show-xml <ifname|all>\n"
- "\nSupported options:\n"
+ "wicked show-xml [options] [ifname ... |all]\n"
+ "\n"
+ "Supported options:\n"

@rtorrero
Copy link
Contributor Author

I got you, should be done :). Thanks for the feedback!

@mtomaschewski mtomaschewski changed the title Fix show-xml filtering by interface name (issue #735) Fix show-xml filtering by interface name (issue #735,bsc#954758) Mar 22, 2018
@mtomaschewski
Copy link
Member

mtomaschewski commented Mar 23, 2018

Could you please also remove the obsolete variable? :

main.c: In function ‘do_show_xml’:
main.c:613:14: warning: unused variable ‘ifname’ [-Wunused-variable]
  const char *ifname = NULL;
              ^

@mtomaschewski
Copy link
Member

mtomaschewski commented Mar 23, 2018

OK, fixed in a separate commit 54a8504c4

@mtomaschewski mtomaschewski merged commit 0f06a51 into openSUSE:master Mar 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants