>>> Vivek S <vivek...@gmail.com> 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 <micha...@cs.wisc.edu> 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 open-iscsi@googlegroups.com.
To unsubscribe from this group, send email to
open-iscsi+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/open-iscsi?hl=en.
