Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

[Michael McMahon]

Ulf,

It seems the NameComparator class uses exactly the same algorithm as the
CASE_INSENSITIVE_ORDER comparator provided by the String class. So,
it probably makes sense to replace NameComparator with the one from String
(one less class in rt.jar :) )
On "SystemRoot" vs "systemroot", I had thought that Windows required the 
sort
to be case-sensitive. But, I double-checked and it doesn't. So we can 
use "SystemRoot".

I think your original comment about not needing the check for sb being 
empty was correct.
On Windows, we are ensuring that SystemRoot will at a minimum always be set.

I'll send another webrev later reflecting these and David's comments.

- Michael.

Ulf Zibis wrote:
You can have the code much shorter (and faster):

    // Only for use by ProcessImpl.start()
    String toEnvironmentBlock() {
        // Sort Unicode-case-insensitively by name
        Map.Entry<String,String>[] list = entrySet().toArray(new 
Map.Entry<>[size()]);
        Arrays.sort(list, entryComparator);

        StringBuilder sb = new StringBuilder(list.length*30);
        for (int i = 0, cmp = -1; ; i++) {
            Map.Entry<String,String> e = i < list.length ? list[i] : 
null;
            final String ROOT = "SystemRoot";
            if (cmp < 0 && (e == null || (cmp = 
nameComparator.compare(e.getKey(), ROOT)) > 0)) {
                String value = getenv(ROOT);
                if (value != null)
                    addEnv(sb, ROOT, value);
                // Ensure double NUL termination, even if environment 
is empty.
                else if (list.isEmpty())
                    sb.append('\u0000');
            }
            if (e == null)  break;
            addEnv(sb, e.getKey(), e.getValue());
        }
        sb.append('\u0000');
        return sb.toString();
    }

    // add the environment variable to the child
    private static void addEnv(StringBuilder sb, String key, String 
value) {
        sb.append(key+"="+value+'\u0000'); // under the hood, it 
compiles to 4 sb.append()
    }

Do you like it?

Note: I think, if we use NameComparator, we can use "SystemRoot".

-Ulf


Am 14.04.2011 16:06, schrieb Michael McMahon:
An updated webrev is available for this fix. I'll probably go ahead
with the CCC request for the spec. change anyway.

http://cr.openjdk.java.net/~michaelm/7034570/webrev.2/


Thanks,
Michael.