RFR: JDK-8013736: [launcher] cleanup code for correctness

[Martin Buchholz]

This looks good.

There's one check I found confusing.

175     if (alen <= 0 || alen > INT_MAX / sizeof(char *)) {

Not that it matters much, but I'm not sure exactly what you're trying
to check here.
If you're trying to check that argc+2 doesn't overflow INT_MAX, I
would do that directly in the original argc check.
If you're trying to check that alen won't overflow, I would check something like
(argc+2) < SIZE_MAX/sizeof(const char *)

On 5/6/13, Kumar Srinivasan <kumar.x.srinivasan at oracle.com> wrote:
> Here is the modified webrev:
> http://cr.openjdk.java.net/~ksrini/8013736/webrev.1/
>
> delta webrev to the last webrev:
> http://cr.openjdk.java.net/~ksrini/8013736/webrev.1/webrev.delta/index.html
>
> I added the do { ..... } while (0) pattern to all the launcher's macro
> defs,
> I also addressed Alan's comments.
>
> I know that the native unpacker uses macros which will need this pattern
> I will fix that separately.
>
> Alan, do you want me to search and file bugs on other jdk components ?
>
> Thanks
> Kumar
>
>
>
>> Oops.  Of course, that shouldn't have the trailing semicolon
>>
>> #define NULL_CHECK_OR_RETURN(ncor_pointer_to_check,     \
>>      ncor_failure_return_value) \
>>   do {                                  \
>>     if ((ncor_pointer_to_check) == NULL) {              \
>> JLI_ReportErrorMessage(JNI_ERROR);                \
>>       return ncor_failure_return_value;                 \
>>     }                                 \
>>   } while(0)
>>
>>
>>
>> On Thu, May 2, 2013 at 11:16 AM, Martin Buchholz <martinrb at google.com
>> <mailto:martinrb at google.com>> wrote:
>>
>>     What would Martin actually do?
>>
>>     #define NULL_CHECK_OR_RETURN(ncor_pointer_to_check,     \
>>                ncor_failure_return_value) \
>>       do {                                        \
>>         if ((ncor_pointer_to_check) == NULL) {              \
>>     JLI_ReportErrorMessage(JNI_ERROR);                \
>>           return ncor_failure_return_value;                 \
>>         }                                       \
>>       } while(0);
>>
>>
>>
>>
>>     On Thu, May 2, 2013 at 10:45 AM, Kumar Srinivasan
>>     <kumar.x.srinivasan at oracle.com
>>     <mailto:kumar.x.srinivasan at oracle.com>> wrote:
>>
>>         On 5/2/2013 10:17 AM, Martin Buchholz wrote:
>>>         This is global fix creep, but ...
>>
>>         :(
>>
>>
>>>
>>>         these macros are also found in the hotspot sources.
>>>         I would rewrite all the macros in the jdk to adopt the
>>>         blessed style
>>>         do { ... } while(0)
>>>         and remove all comments in the jdk of the form
>>>          /* next token must be ; */
>>>         If you want a macro that does nothing at all, you should
>>>         define it
>>>         do {} while (0)
>>>         instead of defining it to the empty string.
>>         I am not following, could you be more explicit on how this
>>         applies to
>>
>>         -#define NULL_CHECK0(e) if ((e) == 0) { \
>>         +#define NULL_CHECK_RV(e, rv) if ((e) == 0) { \
>>               JLI_ReportErrorMessage(JNI_ERROR); \
>>         -    return 0; \
>>         +    return rv; \
>>             }
>>
>>         -#define NULL_CHECK(e) if ((e) == 0) { \
>>         -    JLI_ReportErrorMessage(JNI_ERROR); \
>>         -    return; \
>>         -  }
>>         +#define NULL_CHECK0(e) NULL_CHECK_RV(e, 0)
>>
>>         +#define NULL_CHECK(e) NULL_CHECK_RV(e, )
>>         +
>>
>>
>>>
>>>
>>>             I would also be inclined to change
>>>             == 0
>>>             to
>>>             == NULL
>>>
>>
>>         Yes will take care of this, as well as Alan suggestion added a
>>         check for malloc return.
>>
>>
>>>             This seems like another occasion to use the weird
>>>
>>>             do { ... } while(0)
>>>
>>>             trick to make the macro behave more like a statement.
>>>
>>>             I might obfuscate the macro parameters to make collisions
>>>             less likely, e.g. e => N_C_RV_e
>>>
>>
>>         You want me to rename the macro parameter e to N_C_RV_e ? or
>>         something else
>>         say ncrve to avoid collision ?
>>
>>         Kumar
>>
>>
>>>
>>>
>>>
>>>
>>>             On Wed, May 1, 2013 at 12:33 PM, Kumar Srinivasan
>>>             <kumar.x.srinivasan at oracle.com
>>>             <mailto:kumar.x.srinivasan at oracle.com>> wrote:
>>>
>>>                 Hi,
>>>
>>>                 Please review simple fixes for code correctness in
>>>                 the launcher.
>>>
>>>                 http://cr.openjdk.java.net/~ksrini/8013736/webrev.0/
>>>                 <http://cr.openjdk.java.net/%7Eksrini/8013736/webrev.0/>
>>>
>>>                 Thanks
>>>                 Kumar
>>>
>>>
>>>
>>
>>
>>
>
>