Separate command line tokens parsing from processing of the option arguments
With this it's easier to add an alternative option set getopt loop because the code for processing the arguments isn't contained in the loop so it's not duplicated.
Differential D21434
ping6: separate command line tokens parsing from processing of the option arguments asomers on Aug 27 2019, 12:48 PM. Authored by Tags None Referenced Files
Subscribers
Details
Separate command line tokens parsing from processing of the option arguments With this it's easier to add an alternative option set getopt loop because the code for processing the arguments isn't contained in the loop so it's not duplicated.
Diff Detail
Event TimelineComment Actions I would also like to ask for help with copyright. What should I use if some code is not mine (mainly the #define directives) and some is? Comment Actions If you're moving it from another file, then move or copy that file's copyright header. If you're also substantially adding to it, you can add another copyright in your name.
Comment Actions Add copyright information to options.c and options.h.
Comment Actions Sort SRCS in the Makefile alphabetically.
Comment Actions Yes, that's about what I had in mind. But did you intend to set f_sock_buff_size in the getopt loop? Comment Actions Yes, it's intended. When a pointer to an argument was an output from the getopt loop, it contained information about the value and whether the user provided the argument (there were basically two variables: the pointer itself and data which it was pointing to). When a number is the output, the information about providing an argument is contained in the boolean flag. Another solution would be to restrict range of values returned by strtoul (or use a wider data type) so it's possible to differentiate between return value of strtoul and a value (outside that range) to which the variable is initialized by default. Comment Actions Rename struct options_processed to struct options. Comment Actions So what's the plan for ping -6 vs ping? Are you going to have two different copies of options_parse? Comment Actions Well, I'm not sure yet. I see two solutions:
However, there is a small issue that affects both of the solutions. Some of the error messages include option names. An error message containing incorrect option name could be produced. In the first solution it would be a semantic error message in ping6.c (both of the getopt loops would have the information about the actual names so they would produce correct error messages). In the second solution an incorrect error message would be produced even earlier; in the main getopt loop.
From a user point of view, I would vote for the first solution. Having an option name included in an error message is a great help when identifying what went wrong. I would like to know your opinion. Comment Actions Since only a few options have changed, what about defining constants like #DEFINE ADDRTYPE_OPT 'k' before including options.h, and composing the getopt string like we already do with ADDOPTS. Then in options.c, for those options whose flags differ between the two programs, compose the error message at runtime. Comment Actions I need to be sure I understand you correctly. Does it mean that there could be only one getopt loop with a variable getopt string for solving the compatibility problem? In order to construct the message at runtime information about which name for the option was used on the command line will be needed. The name could be saved in a char variable and used in the error message. Comment Actions Yes, exactly.
It's already saved in the variable ch. Comment Actions
In this case there would have to be some syntactic check duplication in the getopt's switch-case. Some options were renamed according to the pattern (A ->B, B->C) so one case would contain syntactic check for option A and the other for both B and A. If only the case labels could have been non-constant.
Yes, I forgot that. However, this information would be only for currently processed option. It would be needed also for semantic check error messages in ping6.c after the while(getopt()) loop would have been exited. I will try to think about how to solve these two issues. Maybe results from this discussion could also be beneficial to some other utilities (e.g., merging traceroute6 to traceroute). Comment Actions True, the case labels must be compile-time constants, but they needn't be literals. If you #define ADDRTYPE_OPT 'k' somewhere, then you can case ADDRTYPE_OPT. Comment Actions The single getopt loop would have to be parametrized at runtime. How do the preprocessor directives fit into this? Comment Actions You can move all of the getopt stuff into options.h, and then define the variable flags _before_ including options.h, like this: ping6.c #define ADDRTYPE_FLAG 'a' #include "options.h" ping.c #define ADDRTYPE_FLAG 'k' #include "options.h" Comment Actions Aha, to me it's similar to C++ templates. So there would be only one getopt loop at a source level, and two getopt loops at an object level. The two getopt loops (for the ping6's new option set and for the old one) would be called from a single source file like this: if (strcmp(argv[0], "ping6") == 0) parse_old_options(); else parse_new_options(); so the name of the parse function would also need to be parametrized to avoid conflicts of symbols at the object level. Comment Actions Would it be easier if you used an actual C++ template? That's an option. Utilities in the base system are allowed to use C++98. Comment Actions I would like to stick with C, but after some basic experiments it seems that it's not possible to use a C preprocessor to parametrize function names. I'm going to do some research on this. Comment Actions The parametrization using macros is possible. However, it probably means having the loop defined in a header file and not using an include guard in it so it can be included twice in a single source file. The single parametrized loop would contain cases for options of the two loops (new and compat). If some new options are added in the future, it would be more clear to a reader to have two separate loops at a source level. Should I use the parametrization? I'm asking just to be completely sure that it's the way to go and to minimize the diff rewriting. Comment Actions Prepare the option parsing function for parsing the compatibility Comment Actions I implemented the parametrization by macros. The primary purpose of this change is to initiate discussion about the compatibility options for ping6 and to get feedback. |