Hi!
Your patch looks much better, but I still see a maintenance problem in
CMD_LINE_* and opt_help[]: It's a bit tricky to keep them in sync.
I wonder if
#define CMD_LINE_OPTION_PORTAL 0
or maybe better
#define CLI_HELP_INDEX_PORTAL 0
and
#define CLI_HELP_CHAR_PORTAL 'p' /* maybe you could keep the string
type as well */
combined with
struct option_help
{
char option;
const char *help_str;
}opt_help[] = {
{CLI_HELP_CHAR_PORTAL,
"Portal in ip:port format. If only ip address is specified "
"then the value 3260 is assumed for the port."},
...
}
would make things easier to understand and maintain (In Perl I'd use a hash to
access the help texts by the option character).
Somehow it still looks a bit messy, and I'd put those (and alike) items on a
separate line each:
&opt_help[CMD_LINE_OPTION_PORTAL], &opt_help[CMD_LINE_OPTION_IFACE],
&opt_help[CMD_LINE_OPTION_OP], &opt_help[CMD_LINE_OPTION_TYPE],
&opt_help[CMD_LINE_OPTION_PRINT], &opt_help[CMD_LINE_OPTION_DSCVR],
&opt_help[CMD_LINE_OPTION_LOGIN], &opt_help[CMD_LINE_OPTION_DEBUG],
&opt_help[CMD_LINE_OPTION_HELP], NULL
I also wonder if get_tab_width(void) is really needed (for me a TAB is always 8
characters wide on a terminal). Also I wonder whether termlib is needed just to
get the COLUMNS from the environment into the program. Quoting the manual page:
Neither the XSI Curses standard nor the SVr4 man pages documented the
return
values of tgetent correctly, though all three were in fact returned
ever since
SVr1. In particular, an omission in the XSI Curses documentation has
been mis-
interpreted to mean that tgetent returns OK or ERR. Because the
purpose of
these functions is to provide compatibility with the termcap library,
that is a
defect in XCurses, Issue 4, Version 2 rather than in ncurses.
Also the comment "Calling tgetent again frees the buffer allocated by it."
seems to to apply to ncurses as it ignored the buffer altogether (according to
the manual).
Instead of "char *temp_str = (char *)str;" that removes "const", I'd use "const
char *temp_str = str;" (temp_str is never used for modifications, might apply
to "p" as well).
(Haven't looked to closely to the code, however)
Regards,
Ulrich
>>> Vivek S <[email protected]> schrieb am 28.08.2011 um 11:45 in Nachricht
<caapu5rpwqqswtnme7tyuy4j-zrpw-tvum-uvdb9nd2xdu1c...@mail.gmail.com>:
> Made the necessary changes. Kindly review.
>
> On Fri, Aug 12, 2011 at 2:17 AM, Mike Christie <[email protected]> wrote:
>
> > I agree with Ulrich.
> >
> > On 08/11/2011 01:53 AM, Ulrich Windl wrote:
> > >>>> Vivek S <[email protected]> schrieb am 10.08.2011 um 19:50 in
> > Nachricht
> > > <caapu5rp9mby1dednehsgrhhhygmvmnaku+dv4kjjkboe8uy...@mail.gmail.com>:
> > >> Hi Mike/Ulrich
> > >>
> > >> Attaching the updated patch based on your comments.
> > >
> > > Hi!
> > >
> > > Looks a lot better IMHO, but anyway a comment on style:
> > >
> > > + {"r", "Session ID to use. This is either a positive integer or a \
> > > +sysfs path like /sys/devices/platform/hostH/sessionS/targetH:B:I"},
> > >
> > > Since ANSI-C (many years now) you can concatenate strings using "string1"
> > "string2". In you case it may look nicer to avoid the backslash at the end
> > of the line and use double doublequotes instead. The other advantage is
> that
> > you can indent the continuation to align the first line quite nicely.
> > >
> > > Likewise, when printing a larger block of text that is one logical unit,
> > you might also use that. Consider the block starting with
> > >
> > > + printf("\nExample:\n\tTo login to a target\n");
> > > + print_str("\t\t",
> > > + "iscsiadm -m node -T
> > iqn.2005-06.com.mydomain.openiscsi:test1 "
> > > + "-p 192.168.1.2:3260 -l");
> > >
> > > You could use one printf()
> > >
> > > Side note: a better return value that "-1" for the error in get_cols()
> > might be "80", as you are not checking the error when using the return
> > value.
> > >
> > > + sprintf(fmt, "%s%c%c%d%c%c", tabs, '%', '.', idx, 's',
> > '\n');
> > > + printf(fmt, str);
> > >
> > > I wonder whether this is the same as
> > >
> > > printf("%s%.*s\n", tabs, idx, str);
> > >
> > > On "find_last_space(&str[cols-1], (cols-1), 0);":
> > >
> > > "&str[cols-1]" is simply "str + cols - 1". It's also confusing that your
> > start_idx is larger than the end_idx in find_last_space(). Maybe using
> > strchr(), strrchr() or strtok(str, " \t") could help you also.
> > >
> > > For texts like:
> > >
> > > + printf("discoverydb\ndiscovery [DEPRECATED]\nnode\n"
> > > + "session\niface\nfw\nhost\n\n");
> > >
> > > I'd prefer the formatting like this (if you like those newlines):
> > > printf("discoverydb\n"
> > > "discovery [DEPRECATED]\n"
> > > "node\n"
> > > "session\n"
> > > "iface\n"
> > > "fw\n"
> > > "host\n"
> > > "\n");
> > >
> > >
> > > For the many
> > > + print_str("\t\t",
> > >
> > > I'd probably either
> > > #define HELP_INDENT "\t\"
> > > and use print_str(HELP_INDENT,
> > >
> > > or define a ``const char *help_indent = "\t\t";'' and use that. Also, it
> > seems your print_str() doesn't count your "tabs" correctly when considering
> > line breaks. A possible improvement would be to automatically scan for
> > leading whitespace (SPC, TAB) at the beginning of a string, and use that as
> > indentation if lines are wrapped. I have the impression that your
> > print_str() only truncates long lines, but doesn't actually wrap them. Ah,
> > sorry, I overlooked the recursion!
> > >
> > > For the long block starting at
> > >
> > > + printf("\nExample:\n");
> > > + printf("\tTo discover a target\n");
> > > + print_str("\t\t",
> > > + "iscsiadm -m discoverydb -t st -p 192.168.1.2:3260 -D");
> > > + printf("\tTo discover and login\n");
> > > + print_str("\t\t",
> > > + "iscsiadm -m discoverydb -t st -p 192.168.1.2:3260 -Dl");
> > > + print_str("\t",
> > > + "To specify an interface during discovery and login");
> > > + print_str("\t\t",
> > > + "iscsiadm -m discoverydb -t st -p 192.168.1.2:3260 -I eth0
> > "
> > > + "-Dl");
> > >
> > > I'd probably separate code and data like this
> > >
> > > const char *help_text[] = {
> > > "Example:",
> > > "\tTo discover a target",
> > > "\t\tiscsiadm -m discoverydb -t st -p 192.168.1.2:3260 -Dl",
> > > ...,
> > > NULL
> > > };
> > >
> > > const char **txt;
> > > for (txt = help_text; *txt; ++txt)
> > > print_str(*txt); // new implementation!
> > >
> > >
> > > Regards,
> > > Ulrich
> > >
> > >>
> > >> I did not paste the patch contents as it was pretty long.
> > >>
> > >> On Tue, Aug 9, 2011 at 8:24 AM, Mike Christie <[email protected]>
> > wrote:
> > >>
> > >>> On 07/22/2011 12:09 PM, Vivek S wrote:
> > >>>> Changed the way iscsiadm displays usage help about its commands.
> > Rather
> > >>> than
> > >>>> simply displaying each possible mode along with its options on a
> > single
> > >>>> line,
> > >>>> the user can now ask help for each mode separately which describes the
> > >>>> various options and also provides some examples.
> > >>>>
> > >>>
> > >>>
> > >>>> + printf("\n");
> > >>>> + switch (mode)
> > >>>> + {
> > >>>> + case MODE_DISCOVERYDB:
> > >>>> + printf("iscsiadm -m discoverydb [options...]\n\n");
> > >>>> + options = CMD_LINE_OPTION_PORTAL |
> > CMD_LINE_OPTION_IFACE
> > >>> |
> > >>>> CMD_LINE_OPTION_OP \
> > >>>> +
> > >>>
> > >>> Not sure if it is the mailer but just try to keep things around 80
> > >>> columns in the code like the other code is doing unless it is a string
> > >>> and breaking it up makes it more difficult to read.
> > >>>
> > >>> For example if you hit 80 cols in the middle of a target name
> > >>> "iqn.2005-06.com.mydomain.openiscsi.test1" then I would not split that
> > >>> up into "iqn.2005-06.com.mydom" and "ain.openiscsi.test1" just to make
> > >>> it 80 cols.
> > >>>
> > >>> But, something like the above could be
> > >>>
> > >>> options = CMD_LINE_OPTION_PORTAL | CMD_LINE_OPTION_IFACE | \
> > >>> CMD_LINE_OPTION_OP \
> > >>>
> > >
> > >
> > >
> > >
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "open-iscsi" group.
> > To post to this group, send email to [email protected].
> > To unsubscribe from this group, send email to
> > [email protected].
> > For more options, visit this group at
> > http://groups.google.com/group/open-iscsi?hl=en.
> >
> >
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/open-iscsi?hl=en.
