search

CyberShadow / dhcptest

Cross-platform DHCP test client
https://blog.cy.md/2013/01/10/dhcp-test-client
363 stars 57 forks source link

Allow formatted output for --print-only option #5

Closed DarrenWhite99 closed 9 years ago

DarrenWhite99 commented 9 years ago

Modified to allow --print-only the same formatted output as normal output. Use of print-only would treat all options as a string value, which would not display an IP address, timestamp, or other number correctly.

CyberShadow commented 9 years ago

We shouldn't change behavior of existing switches, as it would break existing scripts that rely on this.

CyberShadow commented 9 years ago

I was thinking of adopting the same syntax used by --option for --print-only, e.g. --print-only 6[IP]. That would put the formatting in the users' hands, which is useful for DHCP option types that dhcptest doesn't know about.

DarrenWhite99 commented 9 years ago

I don't see it as changing behavior of an existing switch since the switch as documented only supported strings. IPAddresses are particularly poorly supported, as the output is not even an ascii representation of the numeric value, there is no ascii equivalent to the string conversion of the longint value. Having the formatting dictated by the option make sense, is extensible and backwards compatible (no format specified can just follow the old behavior)

CyberShadow commented 9 years ago

Hypothetically someone might have been piping binary data through xxd for example, but fair point about the documentation. However, if I'm reading this right, this patch changes the output of string options that DHCP dhcptest doesn't know about (before they were written as a string, now they are written through the maybeAscii function).

DarrenWhite99 commented 9 years ago

I have modified the --print-only option to restore the prior logic and support output modifiers [ip] and [hex]. This should be ready to pull if you don't see any problems with my implementation. FWIW, even though I documented the examples as lowercase, the modifier is not case sensitive (HeX==hex).

CyberShadow commented 9 years ago

Can you rebase pull request on master to get rid of the merge commit?

DarrenWhite99 commented 9 years ago

Sorry, I'm not sure what you mean by "rebasing the pull request on master". My master does not have the print-only branch updates. The print-only branch on my project has the code as I mentioned. Feel free to view the raw file and copy/paste into your branch to create clean commit on your side if trying to pull from my branch is problematic. I also am learning about GitHub/cvs/rcs systems this week. :) I get patch/diff stuff, but I have no background in the basics for managing forks/branches/pull requests/etc. Just trying to make it easy to contribute, but like I said, feel free to grab a finished file from my branch and drop if into yours to generate a clean diff on your side, as if you created the code changes. Whatever is simplest. :)

CyberShadow commented 9 years ago

Alright, I rebased, squashed and pushed 62b3a68cd4b34f4b87d56ab2b7a28a3030dc96d0.