Jan Zelený wrote:
> Hi,
> I'm sending a patch for fcoeadm, which fixes broken support of long options.
> Only some long options are supported currently. See:
> https://bugzilla.redhat.com/show_bug.cgi?id=498551
>
> After aplying this patch, all types of options supported by getopt_long will
> be also supported by fcoeadm.
>
> The patch has been made againts 1.0.7 of fcoe-utils, but it should apply fine
> to current code. Please let me know if there is anything wrong with it.
The patch mail seems to be formatted by something that padded lines and
split some. Some lines are longer than 80 chars. I can't apply it so
it's hard to see exactly what it does. It seems the coding style varies
from the one used in the rest of the code, which should be Linux kernel
style, more or less. Indentation by tabs.
I'm glad you're addressing this problem. I think the arg handling needed
improvement, but still might. It might be better to do all the argument
handling before calling the function which does the operation, such as
fcoeadm_create() for the -c option. That way, if some adds a meaningful
option (like --verbose) we don't restrict it to being before the -c option.
For example, if there were such an option, you could say:
fcoeadm --verbose --create eth1 eth2 eth3
or equivalently:
fcoeadm --create --verbose eth1 eth2 ...
currently it wouldn't work to say
fcoeadm --create eth1 --verbose
if we added that option, since the create would be performed as soon
as it is parsed. As soon as we add any options like that, we would
have to restructure this code.
What I propose is something like: handle all options, setting a variable op
that says what the main action is to 'c', 'd', or 'r'. Check to make sure
that only one of those mutually-exclusive options are given (don't set op if
it is already set).
The commands options (e.g., -c or -r) wouldn't take an argument.
In otherwords, I wouldn't allow --create=eth0, just --create or -c.
Once all the getopt options have been handled, go through the remaining args
(eth0 eth1) etc., and perform the op on each of them. For some operations,
it's possible only a single arg would be handled.
Some operations should be handled on all interfaces by default. For
example 'fcoeadm -i' could give a list of all HBAs if none were specified.
These are a lot of issues to change, and they could be done in separate patches,
and not all are urgent. Also, others might not agree, since these would
change the man page, but the changes I suggest are in the spirit of how
getopt was designed to be used.
See a few more comments below.
Joe
> Signed-off-by: Jan Zeleny <[email protected]>
> ---
> --- fcoe-utils-1.0.7/fcoeadm.c 2009-05-04 11:29:00.000000000 +0200
>
> +++ fcoe-utils-1.0.7/fcoeadm.c.longOptions 2009-05-05 17:34:37.000000000
> +0200
> @@ -44,7 +44,7 @@ static struct option fcoeadm_opts[] = {
>
> {"create", 1, 0, 'c'},
>
> {"destroy", 1, 0, 'd'},
>
> {"reset", 1, 0, 'r'},
>
> - {"interface", 1, 0, 'a'},
>
> + {"interface", 1, 0, 'i'},
>
> {"target", 1, 0, 't'},
>
> {"lun", 1, 0, 'l'},
>
> {"stats", 1, 0, 's'},
>
> @@ -449,44 +449,45 @@ int main(int argc, char *argv[])
>
> case 'c':
>
> if ((argc < 2 || argc > 3) ||
>
> strnlen(optarg, MAX_ARG_LEN) > (IFNAMSIZ - 1) ||
>
> - ((argc == 3) && strnlen(argv[1], MAX_ARG_LEN) >
> 2))
> + ((argc == 3) && strnlen(argv[1], MAX_ARG_LEN) > 2
> && argv[1][1] != '-'))
> goto error;
>
> rc = fcoeadm_create(optarg);
>
> goto done;
>
> case 'd':
>
> if ((argc < 2 || argc > 3) ||
>
> strnlen(optarg, MAX_ARG_LEN) > (IFNAMSIZ - 1) ||
>
> - ((argc == 3) && strnlen(argv[1], MAX_ARG_LEN) >
> 2))
> + ((argc == 3) && strnlen(argv[1], MAX_ARG_LEN) > 2
> && argv[1][1] != '-'))
> goto error;
>
> rc = fcoeadm_destroy(optarg);
>
> goto done;
>
> case 'r':
>
> if ((argc < 2 || argc > 3) ||
>
> strnlen(optarg, MAX_ARG_LEN) > (IFNAMSIZ - 1) ||
>
> - ((argc == 3) && strnlen(argv[1], MAX_ARG_LEN) >
> 2))
> + ((argc == 3) && strnlen(argv[1], MAX_ARG_LEN) > 2
> && argv[1][1] != '-'))
> goto error;
>
> rc = fcoeadm_reset(optarg);
>
> goto done;
>
> case 'i':
>
> - if (argc < 2 || argc > 3)
>
> + if (argc < 2 || argc > 3 ||
>
> + (argc == 3 && strnlen(argv[1], MAX_ARG_LEN) > 2
> &&
> + (argv[1][1] != '-' || strchr(argv[1],'=') != NULL)))
>
> goto error;
>
> s = NULL;
>
> - if (argc == 2 && argv[optind]) {
>
> - if (strnlen(argv[optind], MAX_ARG_LEN) >
>
> - (IFNAMSIZ - 1))
>
> - goto error;
>
> - if (strnlen(argv[optind], MAX_ARG_LEN) > 2)
>
> - s = argv[optind] + 2;
>
> - }
>
> - if (argc == 3) {
>
> - if ((optind == 1) &&
>
> - strnlen(argv[1], MAX_ARG_LEN) > 2)
>
> - goto error;
>
> - s = argv[optind];
>
> - }
>
> - if (s)
>
> + if (argc == 2) {
>
> + if (argv[1][1] == '-')
>
> + s = strchr(argv[1],'=')+1;
Style: there should be spaces after the comma and around the +.
I'm not sure why this is having to look for - and =, getopt should
do that for you with optarg. I don't think this is the way to do it.
> + else
>
> + s = argv[1]+2;
>
> + }
>
> + else {
>
> + s = argv[2];
>
> + }
>
> + if (s) {
>
> + if (strnlen(s,MAX_ARG_LEN) > (IFNAMSIZ - 1))
>
> + goto error;
>
> strncpy(opt_info->ifname, s,
>
> sizeof(opt_info->ifname));
>
> + }
>
> if (strnlen(opt_info->ifname, IFNAMSIZ - 1)) {
>
> if (fcoeadm_validate_interface(
>
> opt_info->ifname,
>
> @@ -497,21 +498,26 @@ int main(int argc, char *argv[])
>
> rc = fcoeadm_display_adapter_info(opt_info);
>
> goto done;
>
> case 't':
>
> - if ((argc < 2 || argc > 3) ||
>
> - (argv[1] &&
>
> - strnlen(argv[1], MAX_ARG_LEN) > (IFNAMSIZ - 1))
> ||
> - (argv[2] &&
> - strnlen(argv[2], MAX_ARG_LEN) > (IFNAMSIZ - 1)))
> + if (argc < 2 || argc > 3 ||
> + (argc == 3 && strnlen(argv[1], MAX_ARG_LEN) > 2 &&
> + (argv[1][1] != '-' || strchr(argv[1],'=') != NULL)))
> goto error;
> - if (strnlen(argv[1], MAX_ARG_LEN) > 2) {
> - if (argc >= 3)
> - goto error;
> - strncpy(opt_info->ifname, argv[1] + 2,
> - sizeof(opt_info->ifname));
> - }
> - if (argv[2])
> - strncpy(opt_info->ifname, argv[2],
> + s = NULL;
> + if (argc == 2) {
> + if (argv[1][1] == '-')
> + s = strchr(argv[1],'=')+1;
> + else
> + s = argv[1]+2;
> + }
> + else {
> + s = argv[2];
> + }
> + if (s) {
> + if (strnlen(s,MAX_ARG_LEN) > (IFNAMSIZ - 1))
> + goto error;
> + strncpy(opt_info->ifname, s,
> sizeof(opt_info->ifname));
> + }
> if (strnlen(opt_info->ifname, IFNAMSIZ - 1)) {
> if (fcoeadm_validate_interface(
> opt_info->ifname,
> _______________________________________________
> devel mailing list
> [email protected]
> http://www.open-fcoe.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
