search

rlaager / docsis

This program encodes a DOCSIS binary configuration file from a human-readable text configuration file.
http://docsis.sourceforge.net
GNU General Public License v2.0
115 stars 70 forks source link

Fix cli parsing #37

Closed nickhilliard closed 8 years ago

nickhilliard commented 8 years ago

The current CLI parsing code is spaghetti. This fixes it up so that it's now clear how it works and so that it is possible to add in new CLI commands and modifiers.

rlaager commented 8 years ago

It looks like this needs some git rebase. For example, isn't "fix a spacing issue" fixing something in the commit just before it?

nickhilliard commented 8 years ago

i've collapsed this into 3 commits. is this ok or would you prefer a single commit?

AdrianSimionov commented 8 years ago

I totally agree this change is needed, it took me 50 hours to add the last cli options.

Did this change break any old features?

I also see a goto statement, can it be avoided in any way?

Again, I do not have time to test it but I will come to this commit later on as it is something I was interested in by myself.

Thanks for the code. Adrian.

nickhilliard commented 8 years ago

I tested all the usage modes as outlined in usage() and they all worked, i.e. no regressions. The parsing mechanism is a good deal more flexible than before, so you can specify arguments in any order, as long as the modifiers are first and the commands are last. It's a pity this wasn't done using getopt() or getopts() in the first place because it's no longer possible to back-port the existing syntax without breaking things for people :-(

goto could be avoided by putting in a couple more nests of if-then-else. Mostly it's better to avoid goto when coding but from time to time, goto is a better tool to use. In this case, goto is used as an ad-hoc means of implementing "break".

nickhilliard commented 8 years ago

this patch has now been refactored into two different commits, which is a bit cleaner than the previous pull requests. I'll submit the changes to src/docsis_decode.c in a separate pull request because that is a different bug.

rlaager commented 8 years ago

You seem to have a far better handle on this code than I do, so I'll assume this is good. I added a couple formatting comments, and one about the FIXME. Other than that, it's probably safe to merge.

nickhilliard commented 8 years ago

That spacing was copied from the original code. It's now fixed and I've rebased the commit into one of the existing commits.

The FIXME statement refers to a potential problem with someone else's code that needs to be looked at more closely. Probably the snmp statement shouldn't be there, but that doesn't belong to this commit. I can remove the FIXME statement if you want, but it would be a good idea to flag this as being a potential problem.

AdrianSimionov commented 8 years ago

Nick, for FIXME statement please open a new bug and assign it to me, that code was introduced by myself. It should be documented in Bug #19. I can take a look again if you think might be a problem.

nickhilliard commented 8 years ago

new issue opened for that FIXME.

The FIXME text has been removed from this commit.

rlaager commented 8 years ago

I merged this.

rlaager commented 8 years ago

Thanks for looking at this!