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.
