[patch] iscsiadm cli help update
Vivek S
the user can now ask help for each mode separately which describes the various options and also provides some examples.
Signed-off-by: Vivek Subbarao <vive...@gmail.com>
--- open-iscsi/usr/iscsiadm.c 2011-07-09 23:27:17.963424339 +0530
+++ open-iscsi-test/usr/iscsiadm.c 2011-07-22 22:19:16.452456354 +0530
@@ -103,24 +103,201 @@ static struct option const long_options[
};
static char *short_options = "RlDVhm:p:P:T:H:I:U:k:L:d:r:n:v:o:sSt:u";
-static void usage(int status)
-{
- if (status != 0)
- fprintf(stderr, "Try `%s --help' for more information.\n",
- program_name);
- else {
- printf("\
-iscsiadm -m discoverydb [ -hV ] [ -d debug_level ] [-P printlevel] [ -t type -p ip:port -I ifaceN ... [ -Dl ] ] | [ [ -p ip:port -t type] \
-[ -o operation ] [ -n name ] [ -v value ] [ -lD ] ] \n\
-iscsiadm -m discovery [ -hV ] [ -d debug_level ] [-P printlevel] [ -t type -p ip:port -I ifaceN ... [ -l ] ] | [ [ -p ip:port ] [ -l | -D ] ] \n\
-iiscsiadm -m node [ -hV ] [ -d debug_level ] [ -P printlevel ] [ -L all,manual,automatic ] [ -U all,manual,automatic ] [ -S ] [ [ -T targetname -p ip:port -I ifaceN ] [ -l | -u | -R | -s] ] \
-[ [ -o operation ] [ -n name ] [ -v value ] ]\n\
-iscsiadm -m session [ -hV ] [ -d debug_level ] [ -P printlevel] [ -r sessionid | sysfsdir [ -R | -u | -s ] [ -o operation ] [ -n name ] [ -v value ] ]\n\
-iscsiadm -m iface [ -hV ] [ -d debug_level ] [ -P printlevel ] [ -I ifacename ] [ [ -o operation ] [ -n name ] [ -v value ] ]\n\
-iscsiadm -m fw [ -l ]\n\
-iscsiadm -m host [ -P printlevel ] [ -H hostno ]\n\
-iscsiadm -k priority\n");
+/*
+ * Global defines for all iscsiadm command line options.
+ */
+#define CMD_LINE_OPTION_MODE (1<<0)
+#define CMD_LINE_OPTION_PORTAL (1<<1)
+#define CMD_LINE_OPTION_TGTNAME (1<<2)
+#define CMD_LINE_OPTION_IFACE (1<<3)
+#define CMD_LINE_OPTION_OP (1<<4)
+#define CMD_LINE_OPTION_TYPE (1<<5)
+#define CMD_LINE_OPTION_NAME (1<<6)
+#define CMD_LINE_OPTION_VALUE (1<<7)
+#define CMD_LINE_OPTION_HOST (1<<8)
+#define CMD_LINE_OPTION_SID (1<<9)
+#define CMD_LINE_OPTION_RESCAN (1<<10)
+#define CMD_LINE_OPTION_PRINT (1<<11)
+#define CMD_LINE_OPTION_DSCVR (1<<12)
+#define CMD_LINE_OPTION_LOGIN (1<<13)
+#define CMD_LINE_OPTION_LOGINALL (1<<14)
+#define CMD_LINE_OPTION_LOGOUT (1<<15)
+#define CMD_LINE_OPTION_LOGOUTALL (1<<16)
+#define CMD_LINE_OPTION_STATS (1<<17)
+#define CMD_LINE_OPTION_KILLISCSID (1<<18)
+#define CMD_LINE_OPTION_DEBUG (1<<19)
+#define CMD_LINE_OPTION_SHOW (1<<20)
+#define CMD_LINE_OPTION_VERSION (1<<21)
+#define CMD_LINE_OPTION_HELP (1<<22)
+
+static void print_option_help(unsigned int options)
+{
+ printf("Options\tDescription\n");
+ printf("-------\t-----------\n");
+
+ if (options & CMD_LINE_OPTION_PORTAL)
+ printf("-p\tPortal in ip:port format. If only ip address is specified\n\tthen the value 3260 is assumed for the port.\n");
+ if (options & CMD_LINE_OPTION_TGTNAME)
+ printf("-T\tIqn name of the target.\n");
+ if (options & CMD_LINE_OPTION_IFACE)
+ printf("-I\tInterface to use for the operation. This is the name of file\n\tin /etc/iscsi/ifaces/ directory. Multiple interfaces can be specified.\n");
+ if (options & CMD_LINE_OPTION_OP)
+ printf("-o\tOne of the new, delete, update, show or nonpersistent operations.\n");
+ if (options & CMD_LINE_OPTION_TYPE)
+ printf("-t\tType of discovery. Sendtargets, slp, isns or fw.\n");
+ if (options & CMD_LINE_OPTION_NAME)
+ printf("-n\tName of the field to use in the update operation.\n");
+ if (options & CMD_LINE_OPTION_VALUE)
+ printf("-v\tValue to use for the specified field in the update operation.\n");
+ if (options & CMD_LINE_OPTION_HOST)
+ printf("-H\tSCSI host to use for the operation.\n");
+ if (options & CMD_LINE_OPTION_SID)
+ printf("-r\tSession ID to use. This is either a positive integer or a sysfs\n\tpath like /sys/devices/platform/hostH/sessionS/targetH:B:I\n");
+ if (options & CMD_LINE_OPTION_RESCAN)
+ printf("-R\tRescan all sessions if no SID is specified, else scan only that session.\n");
+ if (options & CMD_LINE_OPTION_PRINT)
+ printf("-P\tLevel of information output. 0 to display a single line\n\tand 1 to display detailed information in tree format.\n");
+ if (options & CMD_LINE_OPTION_DSCVR)
+ printf("-D\tDiscover targets.\n");
+ if (options & CMD_LINE_OPTION_LOGIN)
+ printf("-l\tLogin to the discovered or specified target.\n");
+ if (options & CMD_LINE_OPTION_LOGINALL)
+ printf("-L\tLogin to all sessions with the specified node or connection startup value\n\tor all session if all is passed.\n");
+ if (options & CMD_LINE_OPTION_LOGOUT)
+ printf("-u\tLogout of the specified node or session.\n");
+ if (options & CMD_LINE_OPTION_LOGOUTALL)
+ printf("-U\tLogout of all sessions with the specified node or connection startup value\n\tor all sessions if all is passed.\n");
+ if (options & CMD_LINE_OPTION_STATS)
+ printf("-s\tDisplay session statistics\n");
+ if (options & CMD_LINE_OPTION_DEBUG)
+ printf("-d\tPrint debugging information. Valid values are 0 to 8.\n");
+ if (options & CMD_LINE_OPTION_KILLISCSID)
+ printf("-k\tKill iscsid.\n");
+ if (options & CMD_LINE_OPTION_SHOW)
+ printf("-S\tShow masked values during information display.\n");
+ if (options & CMD_LINE_OPTION_HELP)
+ printf("-h\tDisplay this help.\n");
+ if (options & CMD_LINE_OPTION_VERSION)
+ printf("-v\tDisplay iscsiadm version.\n");
+}
+
+
+static void usage(int status, int mode)
+{
+ unsigned int options = 0;
+
+ if ((status != 0) || (mode == -1))
+ {
+ printf("\nIscsiadm supports the following modes. The user should enter atleast one of them.\nFor help about a particular mode, enter 'iscsiadm \
+-m mode -h'.\n\n");
+ printf("discoverydb\ndiscovery [DEPRECATED]\nnode\nsession\niface\nfw\nhost\n\n");
+ printf("NOTE: To kill iscsid, use 'iscsiadm -k priority' with a priority of 0.\n\n");
+ }
+ else
+ {
+ 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 \
+ | CMD_LINE_OPTION_TYPE | CMD_LINE_OPTION_NAME | CMD_LINE_OPTION_VALUE \
+ | CMD_LINE_OPTION_PRINT | CMD_LINE_OPTION_DSCVR | CMD_LINE_OPTION_LOGIN \
+ | CMD_LINE_OPTION_DEBUG | CMD_LINE_OPTION_HELP;
+ print_option_help(options);
+ printf("\nExample:\n\tTo discover a target\n");
+ printf("\t\tiscsiadm -m discoverydb -t st -p 192.168.1.2:3260 -D\n");
+ printf("\tTo discover and login\n");
+ printf("\t\tiscsiadm -m discoverydb -t st -p 192.168.1.2:3260 -Dl\n");
+ printf("\tTo specify an interface during discovery and login\n");
+ printf("\t\tiscsiadm -m discoverydb -t st -p 192.168.1.2:3260 -I eth0 -Dl\n");
+ printf("\tTo add or delete a record\n");
+ printf("\t\tiscsiadm -m discoverydb -t st -p 192.168.1.2:3260 -o new/delete\n");
+ printf("\tTo update a record\n");
+ printf("\t\tiscsiadm -m discoverydb -t st -p 192.168.1.2:3260 -o update \n\t\t-n discovery.startup\
+ -v manual\n");
+ printf("\tTo display records from discovery database\n");
+ printf("\t\tiscsiadm -m discoverydb\n\n");
+ break;
+
+ case MODE_DISCOVERY:
+ printf("iscsiadm -m discovery [options...]\n\n");
+ options = CMD_LINE_OPTION_PORTAL | CMD_LINE_OPTION_IFACE | CMD_LINE_OPTION_OP \
+ | CMD_LINE_OPTION_TYPE | CMD_LINE_OPTION_PRINT | CMD_LINE_OPTION_DSCVR \
+ | CMD_LINE_OPTION_LOGIN | CMD_LINE_OPTION_DEBUG | CMD_LINE_OPTION_HELP;
+ print_option_help(options);
+ printf("\nExample:\n\tTo discover a target\n");
+ printf("\t\tiscsiadm -m discovery -t slp -p 192.168.1.1:3260 -D\n");
+ printf("\tTo discover and login to a target\n");
+ printf("\t\tiscsiadm -m discovery -t slp -p 192.168.1.1:3260 -Dl\n");
+ printf("\tTo specify an interface\n");
+ printf("\t\tiscsiadm -m discovery -t slp -p 192.168.1.1:3260 -I eth0\n\n");
+ break;
+
+ case MODE_NODE:
+ printf("iscsiadm -m node [options...]\n\n");
+ options = CMD_LINE_OPTION_PORTAL | CMD_LINE_OPTION_DEBUG | CMD_LINE_OPTION_PRINT \
+ | CMD_LINE_OPTION_LOGINALL | CMD_LINE_OPTION_LOGOUTALL | CMD_LINE_OPTION_SHOW \
+ | CMD_LINE_OPTION_TGTNAME | CMD_LINE_OPTION_IFACE | CMD_LINE_OPTION_LOGIN \
+ | CMD_LINE_OPTION_RESCAN | CMD_LINE_OPTION_STATS | CMD_LINE_OPTION_HELP \
+ | CMD_LINE_OPTION_LOGOUT | CMD_LINE_OPTION_OP | CMD_LINE_OPTION_NAME \
+ | CMD_LINE_OPTION_VALUE;
+ print_option_help(options);
+ printf("\nExample:\n\tTo login to a target\n");
+ printf("\t\tiscsiadm -m node -T iqn.2005-06.com.mydomain.openiscsi:test1 -p 192.168.1.2:3260 -l\n");
+ printf("\tTo login to all targets with node or connection startup type as manual\n");
+ printf("\t\tiscsiadm -m node -L manual\n");
+ printf("\tTo logout of all targets with node or connection startup type as automatic\n");
+ printf("\t\tiscsiadm -m node -U automatic\n");
+ printf("\tTo rescan the session passing through a target\n");
+ printf("\t\tiscsiadm -m node -T iqn.2005-06.com.mydomain.openiscsi.test1 -p 192.168.1.2:3260 -R\n");
+ printf("\tTo display statistics about a session passing through a target\n");
+ printf("\t\tiscsiadm -m node -T iqn.2005-06.com.mydomain.openiscsi:test1 -p 192.168.1.2:3260 -s\n");
+ printf("\tTo add a new node record\n");
+ printf("\t\tiscsiadm -m node -T iqn.2005-06.com.mydomain.openiscsi:test1 -p 192.168.1.2:3260 -o new\n\n");
+ break;
+
+ case MODE_SESSION:
+ printf("iscsiadm -m session [options...]\n\n");
+ options = CMD_LINE_OPTION_HELP | CMD_LINE_OPTION_DEBUG | CMD_LINE_OPTION_PRINT | CMD_LINE_OPTION_SID \
+ | CMD_LINE_OPTION_RESCAN | CMD_LINE_OPTION_LOGOUT | CMD_LINE_OPTION_STATS \
+ | CMD_LINE_OPTION_OP | CMD_LINE_OPTION_NAME | CMD_LINE_OPTION_VALUE;
+ print_option_help(options);
+ printf("\nExample:\n\tTo logout of a session\n");
+ printf("\t\tiscsiadm -m session -r 1 -u\n");
+ printf("\tTo delete a session record\n");
+ printf("\t\tiscsiadm -m session -r 2 -o delete\n\n");
+ break;
+
+ case MODE_IFACE:
+ printf("iscsiadm -m iface [options...]\n\n");
+ options = CMD_LINE_OPTION_HELP | CMD_LINE_OPTION_DEBUG | CMD_LINE_OPTION_PRINT \
+ | CMD_LINE_OPTION_IFACE | CMD_LINE_OPTION_OP | CMD_LINE_OPTION_NAME | CMD_LINE_OPTION_VALUE;
+ print_option_help(options);
+ printf("\nExample:\n\tTo add a iface record\n");
+ printf("\t\tiscsiadm -m iface -I eth1 -o new\n\n");
+ break;
+
+ case MODE_FW:
+ printf("iscsiadm -m fw [options...]\n\n");
+ options = CMD_LINE_OPTION_LOGIN;
+ print_option_help(options);
+ printf("\n");
+ break;
+
+ case MODE_HOST:
+ printf("iscsiadm -m host [options...]\n\n");
+ options = CMD_LINE_OPTION_PRINT | CMD_LINE_OPTION_HOST;
+ print_option_help(options);
+ printf("\n");
+ break;
+
+ deafult:
+ printf("Invalid mode\n\n");
+ }
+ printf("SEE MAN PAGE FOR MODE DETAILS\n\n");
}
+
exit(status);
}
@@ -370,7 +547,7 @@ logout_by_startup(char *mode)
if (!mode || !(!strcmp(mode, "automatic") || !strcmp(mode, "all") ||
!strcmp(mode,"manual"))) {
log_error("Invalid logoutall option %s.", mode);
- usage(ISCSI_ERR_INVAL);
+ usage(ISCSI_ERR_INVAL, 0);
return ISCSI_ERR_INVAL;
}
@@ -440,7 +617,7 @@ login_by_startup(char *mode)
if (!mode || !(!strcmp(mode, "automatic") || !strcmp(mode, "all") ||
!strcmp(mode,"manual") || !strcmp(mode, "onboot"))) {
log_error("Invalid loginall option %s.", mode);
- usage(ISCSI_ERR_INVAL);
+ usage(ISCSI_ERR_INVAL, 0);
return ISCSI_ERR_INVAL;
}
@@ -2071,7 +2248,7 @@ main(int argc, char **argv)
ISCSI_VERSION_STR);
return 0;
case 'h':
- usage(0);
+ usage(0, mode);
}
}
@@ -2087,7 +2264,7 @@ main(int argc, char **argv)
}
if (mode < 0)
- usage(ISCSI_ERR_INVAL);
+ usage(ISCSI_ERR_INVAL, 0);
if (mode == MODE_FW) {
if ((rc = verify_mode_params(argc, argv, "ml", 0))) {
Vivek S
Any update on this patch ? Is this fine or should I do some more changes ?
Thank you,
Vivek S
Ulrich Windl
The idea is good, but I'd use a different implementation, trying to put most text into one data structure, and then select the correct text. Roughly like
struct help {
const char *option;
const char *explanation;
};
struct help the_help[] = {
{"p", "Portal in ip:port format. If only ip address is specified then the value 3260 is assumed for the port."},
{"n", "Name of the field to use in the update operation."},
...
};
I'd do the line wrapping in the code, not in the texts (terminal sizes may vary anyway). I guess you get thge idea how to pick the correct help text from the array.
Regards,
Ulrich
>>> Vivek S <vive...@gmail.com> schrieb am 22.07.2011 um 19:09 in Nachricht
<8069269.2450.1311354555863.JavaMail.geo-discussion-forums@prcn22>:
Vivek S
--
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To post to this group, send email to open-...@googlegroups.com.
To unsubscribe from this group, send email to open-iscsi+...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/open-iscsi?hl=en.
Mike Christie
Sorry for not commenting sooner. I thought I did.
First, here is some useful git commands to make and send patches. The
patch that made it to the list got its formatting mangled by your mailer.
To make a patch for sending to the list that is already merged in your
copy of the tree do:
git format-patch -n -o dir-to-hold-your-patches-to-send
$GIT_COMMIT_ID_OF_THE_PATCH_BEFORE_YOURS
to then send those patches setup git to support your gmail account. See
here:
https://git.wiki.kernel.org/index.php/GitTips#Using_gmail_to_send_your_patches
then you can just run this command to send them to the list:
git send-email --to=open-...@googlegroups.com --smtp
dir-to-hold-your-patches-to-send
I am reviewing the patch today and should have some comments tomorrow.
Again, sorry about this.
On 08/03/2011 02:09 AM, Vivek S wrote:
> I can do as you say, but whats the advantage ?
>
> On Wed, Aug 3, 2011 at 11:39 AM, Ulrich Windl
> <Ulrich...@rz.uni-regensburg.de
> <mailto:Ulrich...@rz.uni-regensburg.de>> wrote:
>
> Hi!
>
> The idea is good, but I'd use a different implementation, trying to
> put most text into one data structure, and then select the correct
> text. Roughly like
>
> struct help {
> const char *option;
> const char *explanation;
> };
>
> struct help the_help[] = {
> {"p", "Portal in ip:port format. If only ip address is specified
> then the value 3260 is assumed for the port."},
> {"n", "Name of the field to use in the update operation."},
> ...
> };
>
> I'd do the line wrapping in the code, not in the texts (terminal
> sizes may vary anyway). I guess you get thge idea how to pick the
> correct help text from the array.
>
> Regards,
> Ulrich
>
>
>
> >>> Vivek S <vive...@gmail.com <mailto:vive...@gmail.com>> schrieb
> am 22.07.2011 um 19:09 in Nachricht
> <8069269.2450.1311354555863.JavaMail.geo-discussion-forums@prcn22>:
> > 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.
> >
> > Signed-off-by: Vivek Subbarao <vive...@gmail.com
> <mailto:vive...@gmail.com>>
> > 192.168.1.2:3260 <http://192.168.1.2:3260> -D\n");
> > + printf("\tTo discover and login\n");
> > + printf("\t\tiscsiadm -m discoverydb -t st -p
> > 192.168.1.2:3260 <http://192.168.1.2:3260> -Dl\n");
> > + printf("\tTo specify an interface during
> discovery and
> > login\n");
> > + printf("\t\tiscsiadm -m discoverydb -t st -p
> > 192.168.1.2:3260 <http://192.168.1.2:3260> -I eth0 -Dl\n");
> > + printf("\tTo add or delete a record\n");
> > + printf("\t\tiscsiadm -m discoverydb -t st -p
> > 192.168.1.2:3260 <http://192.168.1.2:3260> -o new/delete\n");
> > + printf("\tTo update a record\n");
> > + printf("\t\tiscsiadm -m discoverydb -t st -p
> > 192.168.1.2:3260 <http://192.168.1.2:3260> -o update \n\t\t-n
> discovery.startup\
> > + -v manual\n");
> > + printf("\tTo display records from discovery
> database\n");
> > + printf("\t\tiscsiadm -m discoverydb\n\n");
> > + break;
> > +
> > + case MODE_DISCOVERY:
> > + printf("iscsiadm -m discovery [options...]\n\n");
> > + options = CMD_LINE_OPTION_PORTAL |
> CMD_LINE_OPTION_IFACE |
> > CMD_LINE_OPTION_OP \
> > + | CMD_LINE_OPTION_TYPE |
> CMD_LINE_OPTION_PRINT |
> > CMD_LINE_OPTION_DSCVR \
> > + | CMD_LINE_OPTION_LOGIN |
> CMD_LINE_OPTION_DEBUG |
> > CMD_LINE_OPTION_HELP;
> > + print_option_help(options);
> > + printf("\nExample:\n\tTo discover a target\n");
> > + printf("\t\tiscsiadm -m discovery -t slp -p
> > 192.168.1.1:3260 <http://192.168.1.1:3260> -D\n");
> > + printf("\tTo discover and login to a target\n");
> > + printf("\t\tiscsiadm -m discovery -t slp -p
> > 192.168.1.1:3260 <http://192.168.1.1:3260> -Dl\n");
> > + printf("\tTo specify an interface\n");
> > + printf("\t\tiscsiadm -m discovery -t slp -p
> > 192.168.1.1:3260 <http://192.168.1.1:3260> -I eth0\n\n");
> > + break;
> > +
> > + case MODE_NODE:
> > + printf("iscsiadm -m node [options...]\n\n");
> > + options = CMD_LINE_OPTION_PORTAL |
> CMD_LINE_OPTION_DEBUG |
> > CMD_LINE_OPTION_PRINT \
> > + | CMD_LINE_OPTION_LOGINALL |
> > CMD_LINE_OPTION_LOGOUTALL | CMD_LINE_OPTION_SHOW \
> > + | CMD_LINE_OPTION_TGTNAME |
> CMD_LINE_OPTION_IFACE |
> > CMD_LINE_OPTION_LOGIN \
> > + | CMD_LINE_OPTION_RESCAN |
> CMD_LINE_OPTION_STATS |
> > CMD_LINE_OPTION_HELP \
> > + | CMD_LINE_OPTION_LOGOUT | CMD_LINE_OPTION_OP |
> > CMD_LINE_OPTION_NAME \
> > + | CMD_LINE_OPTION_VALUE;
> > + print_option_help(options);
> > + printf("\nExample:\n\tTo login to a target\n");
> > + printf("\t\tiscsiadm -m node -T
> > iqn.2005-06.com.mydomain.openiscsi:test1 -p 192.168.1.2:3260
> <http://192.168.1.2:3260> -l\n");
> > + printf("\tTo login to all targets with node or
> connection
> > startup type as manual\n");
> > + printf("\t\tiscsiadm -m node -L manual\n");
> > + printf("\tTo logout of all targets with node or
> connection
> > startup type as automatic\n");
> > + printf("\t\tiscsiadm -m node -U automatic\n");
> > + printf("\tTo rescan the session passing through a
> > target\n");
> > + printf("\t\tiscsiadm -m node -T
> > iqn.2005-06.com.mydomain.openiscsi.test1 -p 192.168.1.2:3260
> <http://192.168.1.2:3260> -R\n");
> > + printf("\tTo display statistics about a session
> passing
> > through a target\n");
> > + printf("\t\tiscsiadm -m node -T
> > iqn.2005-06.com.mydomain.openiscsi:test1 -p 192.168.1.2:3260
> <http://192.168.1.2:3260> -s\n");
> > + printf("\tTo add a new node record\n");
> > + printf("\t\tiscsiadm -m node -T
> > iqn.2005-06.com.mydomain.openiscsi:test1 -p 192.168.1.2:3260
> <http://192.168.1.2:3260> -o new\n\n");
> <mailto:open-...@googlegroups.com>.
> To unsubscribe from this group, send email to
> open-iscsi+...@googlegroups.com
> <mailto:open-iscsi%2Bunsu...@googlegroups.com>.
Ulrich Windl
<CAAPU5rPHFLAzwrxTg-VAfrL+...@mail.gmail.com>:
> I can do as you say, but whats the advantage ?
Hi!
I'd say: Easier to read, easier to change.
Ulrich
Vivek S
Vivek S
--
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
Ulrich Windl
> Nachricht
<CAAPU5rMhX0p3QN_EuShzhtNR...@mail.gmail
> .com>:
I'd do the line wrapping in the code, not in the texts (terminal sizes
> may
> vary anyway).
>
> How do you do line wrapping in code ?
Hi!
Wow would you do line-wrapping in non-code? ;-)
What I was thinkingg of was some thing like "length = atoi(getenv("COLUMNS"))" to get the length of a line, and then maybe get the next word to be output and check if that would exceed the length. If so add a newline before outputting the next word.
Would you consider that to be a very obscure algorithm?
Regards,
Ulrich
Vivek S
Mike Christie
> 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
Ulrich Windl
<CAAPU5rP9mBY1DeDnehsGRhHH...@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
Mike Christie
Ulrich Windl
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 <vive...@gmail.com> schrieb am 28.08.2011 um 11:45 in Nachricht
<CAAPU5rPwQQSWTnme7tYUy4j-...@mail.gmail.com>:
Ulrich Windl
10:29 in Nachricht <4E5B69F10200...@gwsmtp1.uni-regensburg.de>:
[...]
> 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
Oops: Meant to write "seems not to apply"...
> 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
[...]
Ulrich Windl
<CAAPU5rPY1F3bkYYyASAFbOtb6JrfGw7JSb6m=--KKUEZVmc=w...@mail.gmail.com>:
> Removed ordering constrains between #defines and structure elements.
> Removed calculating tab width and defaulting to 8.
>
> Hope this will do :-)
Hi!
It looks like converging ;-)
You could (I know it's non-hacker-like) add some comments on the struct def:
int option;
char *option_str;
const char *help_str;
BTW: shouldn't "option_str" be "const char" also?
On
+/*
+ * Global defines for all iscsiadm command line options.
+ */
+#define CMD_LINE_OPTION_PORTAL (1 << 0)
Improve the comment by saying why you use bitmasks there, or say what those defines are going to be used for.
If you feel like a masochist today, you could temporarily add some of the gcc options that I used myself years ago:
-Wall -Wshadow -Wpointer-arith -Wcast-qual -Wcast-align -Wwrite-strings -Wtraditional -Wstrict-prototypes -Wnested-externs -Wredundant-decls -Wconversion
Maybe this helps you to clean out a few edges.
Regards,
Ulrich
Vivek S
Mike Christie
> Any update ?
>
Looks ok. Thanks for all your work on it. And thanks to Ulrich for the
review.
Just one stye comment though. The coding style we follow has us put the
leading "{" on the first line for structs, for, and if/else blocks. So
+struct option_help
+{
would be
+struct option_help {
+ if ( (str_len + (num_tabs * TAB_WIDTH) - num_tabs) > max_cols )
+ {
would be
if ( (str_len + (num_tabs * TAB_WIDTH) - num_tabs) > max_cols ) {
Also in here you do not need the extra spaces between the ((s and at the
end after max_cols and )
And
+ }
+ else
+ {
would be
} else {
then here
+ for (i=0 ; i<str_len ; i++)
+ {
you want spaces between i and = and 0 and i and < and str_len but not
after str_len so it should be
for (i = 0 ; i < str_len; i++)
If there is only one line and so {} are not needed then we do not add them
+ if (ioctl(0,TIOCGWINSZ,&ws)!=0)
+ {
+ return 80;
+ }
should be
if (ioctl(0,TIOCGWINSZ, &ws) != 0)
return 80;
I will fix these up when I merge it. I am having troubles logging into
kernel.org to update the git tree due to the breakin.
Vivek S
Will follow the coding style more rigourously the next time I submit a patch.