Re: URLClassLoader update

[Mark Wielaard <mark at klomp dot org>]

Hi,

On Tue, 2002-12-03 at 10:41, Tom Tromey wrote:
> >>>>> "Mark" == Mark Wielaard <mark@klomp.org> writes:
> 
> Mark> The last couple of weeks I have rewritten the URLClassLoader
> Mark> implementation based on the old GNU Classpath version and the
> Mark> changes that the Orp developers made. This version has different
> Mark> Loaders for the different types of URLs that can be given to the
> Mark> URLClassLoader (URLClassLoader now works for remote locations! -
> Mark> now if only we had a complete verifier and security permissions
> Mark> framework...).
> 
> Wow, this is a complicated patch.  Overall it looks good to me.

Maybe I should just have included the new URLClassLoader.java file since
it was based on the GNU Classpath version not on the old libgcj version
(even though some of the methods were already merged). Hopefully the
design with the different URLLoader classes that are optimized for the
different casses (local/remote locations, dir/jar based) is clear.

> I have a few questions.
> 
> Mark> [ ClassLoader ]
> Mark>    /**
> Mark> +   * Normalizes a resource path name. This removes all ./ from the start and
> Mark> +   * all further '//', "/../" and "/./" from the given name.
> Mark> +   */
> 
> I don't understand why this is a valid operation.  For instance, if
> the resource comes from the file system, then `..' removal is
> incorrect in the presence of symlinks.  Maybe it doesn't matter
> though -- a plausible argument.

I first wrote some Mauve tests with different params for getResource()
that should load or not load a particular resource. This was based on
the directory based loader at first, I then extended it to use different
URLs to load from. Since the .. and . things didn't work in all cases I
added this code. But it might not be appropriate when the underlaying
loader/protocol doesn't support it (like when reading from a jar file).
I will check how other implementations handle this issue by running
Mauve against kaffe or Sun implementations. Maybe this should only be
done in directory based loaders or just let the URL connection handler
handle it. Will check.

> Mark> [ URLClassLoader ]
> Mark> +	    // Check if it's a jar url
> Mark> +	    if (!(file.endsWith("/") || file.endsWith(File.separator)))
> Mark> +	      loader = new JarURLLoader(this, newUrl);
> 
> I missed that `!' the first few times through this...
> speaking of which there are various formatting buglets in this code.

OK, I will fix it up. BTW what is the correct way to format this?
It isn't cover in http://www.gnu.org/prep/standards_23.html#SEC23

> Mark> +  protected Class findClass(final String className)
> Mark> +    throws ClassNotFoundException
> Mark> +  {
> Mark> +    // Just try to find the resource by the (almost) same name
> Mark> +    String resourceName = className.replace('.', '/') + ".class";
> Mark> +    Resource resource = findURLResource(resourceName);
> 
> I have a patch here with this hunk:
> 
> @@ -266,7 +274,7 @@
>  
>      try 
>        {
> -	URL url = getResource (name.replace ('.', '/') + ".class");
> +	URL url = findResource (name.replace ('.', '/') + ".class");
>  
>  	if (url == null)
>  	  throw new ClassNotFoundException (name);
> 
> Should findClass be calling findResource instead?  The rationale is
> that a subclass might override just findResource.  Eclipse does this.

Yes it should. Thanks for spotting that.
(getResource would eventually call findResource, but only if none of the
parent classloaders have the named resource but in that case we
shouldn't have been called in the first place.)

Cheers,

Mark