Application Server 7
  1. Application Server 7
  2. AS7-5008

host-controller module does parse command line args too late

    Details

    • Similar Issues:
      Show 10 results 

      Description

      When starting the host controller module to use the -v or -h command line switches (which are supported in the code) there will be no result. The reason for this is, that the host-controller reads the authKey before it parses the command line for the manual invok. Also it looks like the redirected input/output leads to the case that no usage() can be writte anyway.

      org.jboss.as.process.protocol.StreamUtils.readFully(StreamUtils.java:124)
      org.jboss.as.host.controller.Main.main(Main.java:82)
      sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
      sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
      java.lang.reflect.Method.invoke(Unknown Source)
      org.jboss.modules.Module.run(Module.java:260)
      org.jboss.modules.Main.main(Main.java:291)

        Issue Links

          Activity

          Hide
          Bernd Eckenfels
          added a comment -

          I started it this way and entered 16 keys:

          JB>java -jar jboss-modules.jar -mp modules\ org.jboss.as.host-controller -v
          1234567890123456

          Show
          Bernd Eckenfels
          added a comment - I started it this way and entered 16 keys: JB>java -jar jboss-modules.jar -mp modules\ org.jboss.as.host-controller -v 1234567890123456
          Hide
          Brian Stansberry
          added a comment -

          That's not supposed to work – the org.jboss.as.host-controller module is not meant to be the launched directly by the end user.

          Show
          Brian Stansberry
          added a comment - That's not supposed to work – the org.jboss.as.host-controller module is not meant to be the launched directly by the end user.
          Hide
          Bernd Eckenfels
          added a comment -

          Then you should remove the parsing code. Should I provide a patch?

          Show
          Bernd Eckenfels
          added a comment - Then you should remove the parsing code. Should I provide a patch?
          Hide
          Brian Stansberry
          added a comment -

          You mean -v and -h parsing code? OK, sure, a patch removing that is fine. Thanks!

          Show
          Brian Stansberry
          added a comment - You mean -v and -h parsing code? OK, sure, a patch removing that is fine. Thanks!
          Hide
          Bernd Eckenfels
          added a comment -

          I sent a Pull request which removes the -v/-h parsing code. It also removes the usage() output and fixed one occurence where command line errors are written to .out instead of .err:

          https://github.com/jbossas/jboss-as/pull/3412

          However while working on this I noticed that there are still some inconsitencies:

          • not all parsing errors write helpfull messages to stderr - is it needed nowhere or everywhere?
          • the abort() function seems to have inconsistent usage, what error code should be returned on parsing errors? (even in some places System.exit() is used instead of abort())

          Let me know in what direction you want the code to be reworked, glad to help here.

          Show
          Bernd Eckenfels
          added a comment - I sent a Pull request which removes the -v/-h parsing code. It also removes the usage() output and fixed one occurence where command line errors are written to .out instead of .err: https://github.com/jbossas/jboss-as/pull/3412 However while working on this I noticed that there are still some inconsitencies: not all parsing errors write helpfull messages to stderr - is it needed nowhere or everywhere? the abort() function seems to have inconsistent usage, what error code should be returned on parsing errors? (even in some places System.exit() is used instead of abort()) Let me know in what direction you want the code to be reworked, glad to help here.
          Hide
          Brian Stansberry
          added a comment -

          Sorry I've been so slow looking at this.

          The error handling certainly needs some rework.

          AFAICT, the calling of usage() was consistent in the parsing. The way it all works isn't very good though.

          Main.parseValue() isn't outputting an error message; I think that is what you are referring to in your last comment. That needs to be fixed.

          The use of System.err in HostController Main is an issue. The ProcessController consumes the HC's syserr and pipes it to its own syserr, and the output doesn't appear on the PC's console. Currently the usage() output from the HC shows up as log output, so that at least gives some clue to the user that something was wrong with the command line arguments. But the more descriptive error message doesn't appear in the console.

          To decide what to do about that I need to talk to David Lloyd to try and refresh my memory as to why the ProcessController handles its output streams the way it does. I don't want to inadvertently break something. Intuitively, I think output to System.err from a ManagedProcess should result in an ERROR log message in the PC's console.

          Having HC's Main dump the full usage() output to the log isn't nice since it looks bad, but I think following the error message with something like "INFO Use domain.sh --help for information on valid command line arguments" would be good. That could also be appended to the error messages.

          As for System.exit() vs. abort(), sure, having everything go through a consistent routine is better. The key thing is following any refactor, for any situation the exit codes that are currently sent must still be sent.

          Show
          Brian Stansberry
          added a comment - Sorry I've been so slow looking at this. The error handling certainly needs some rework. AFAICT, the calling of usage() was consistent in the parsing. The way it all works isn't very good though. Main.parseValue() isn't outputting an error message; I think that is what you are referring to in your last comment. That needs to be fixed. The use of System.err in HostController Main is an issue. The ProcessController consumes the HC's syserr and pipes it to its own syserr, and the output doesn't appear on the PC's console. Currently the usage() output from the HC shows up as log output, so that at least gives some clue to the user that something was wrong with the command line arguments. But the more descriptive error message doesn't appear in the console. To decide what to do about that I need to talk to David Lloyd to try and refresh my memory as to why the ProcessController handles its output streams the way it does. I don't want to inadvertently break something. Intuitively, I think output to System.err from a ManagedProcess should result in an ERROR log message in the PC's console. Having HC's Main dump the full usage() output to the log isn't nice since it looks bad, but I think following the error message with something like "INFO Use domain.sh --help for information on valid command line arguments" would be good. That could also be appended to the error messages. As for System.exit() vs. abort(), sure, having everything go through a consistent routine is better. The key thing is following any refactor, for any situation the exit codes that are currently sent must still be sent.
          Hide
          Brian Stansberry
          added a comment -

          Oops; much of my last comment was based on me being an idiot. The error I introduced to look into what the output looked like lead into the parseValue() method that produces no error message. So, of course I didn't see one. When I do something that actually results in an error message to System.err, it appears in the console as an ERROR log, as desired.

          So, I'll process your patch now (BTW, thanks!), but with an added commit that does the following:

          1) Add a System.err.println call to parseValue()
          2) Appends "Use domain.sh --help for information on valid command line arguments." to the parsing error messages.

          If there are other parsing errors that aren't writing error messages, a patch fixing that would be welcome, as would be a patch cleaning up the System.exit/abort stuff.

          Show
          Brian Stansberry
          added a comment - Oops; much of my last comment was based on me being an idiot. The error I introduced to look into what the output looked like lead into the parseValue() method that produces no error message. So, of course I didn't see one. When I do something that actually results in an error message to System.err, it appears in the console as an ERROR log, as desired. So, I'll process your patch now (BTW, thanks!), but with an added commit that does the following: 1) Add a System.err.println call to parseValue() 2) Appends "Use domain.sh --help for information on valid command line arguments." to the parsing error messages. If there are other parsing errors that aren't writing error messages, a patch fixing that would be welcome, as would be a patch cleaning up the System.exit/abort stuff.
          Hide
          Brian Stansberry
          added a comment -

          Thanks for the patch!

          I'll open a separate JIRA for the System.exit/abort issue.

          Show
          Brian Stansberry
          added a comment - Thanks for the patch! I'll open a separate JIRA for the System.exit/abort issue.
          Hide
          Brian Stansberry
          added a comment -

          See AS7-6037 for the System.exit/abort cleanup task.

          Show
          Brian Stansberry
          added a comment - See AS7-6037 for the System.exit/abort cleanup task.

            People

            • Assignee:
              Bernd Eckenfels
              Reporter:
              Bernd Eckenfels
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: