On Thu, 2009-05-07 at 10:36 -0700, Joe Eykholt wrote: > 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. > Just a thought-
Hopefully my debug_logging code will make it into 2.6.31 which would allow tunable logging levels. I had previously envisioned having a debug/debuglog.sh script to tune the logging. Maybe we could have a verbose option with fcoeadm that turns on some of the logging instead. Most of our logging is for developer debugging though, so I'm not sure it deserves to be integrated with the administration tool. > 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. > How would your suggestion change the '--lun' option? It's not a primary operation like '-c, -r and -d', but it needs arguments. Would the LUN identifiers also just become the last arguments to the application? Generally I support this suggestion, it would make the argument parsing much cleaner and I don't think it restricts the functionality. _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
