Review: Review: branches/vince/netsurf-file-fetcher

Vincent Sanders vince at kyllikki.org
Sat Sep 11 19:55:45 BST 2010


Sigh, ok, ok I give in, commited a less sophisticated version, does
not assume it can use open() on anything except plain files. Only uses
opendir() on directories and specifically avoids using any expected
clib behaviour introduced in the last couple of decades.

I updated the url_to_path of some frontends in the vauge hope using
the url lib is a bit more correct than their previously implementations.

Should fix all your points from below.

On Sat, Sep 11, 2010 at 06:44:27PM +0100, Chris Young wrote:
> Firstly, this has the same problem as the old libcurl file fetcher, in
> that it insists on opening every file prepended with a slash.  The fix
> for that particular issue is this:
> 
> --8<--
> Index: content/fetchers/fetch_file.c
> ===================================================================
> --- content/fetchers/fetch_file.c	(revision 10754)
> +++ content/fetchers/fetch_file.c	(working copy)
> @@ -135,8 +135,8 @@
>  	if (ctx == NULL)
>  	return NULL;
>  
> -	res = url_path(url, &path);
> -	if (res != URL_FUNC_OK) {
> +	path = url_to_path(url);
> +	if (path == NULL) {
>  	free(ctx);
>  	return NULL;
>  	}
> 
> --8<--
> 
> I've not committed that change myself as I'm not in a position to test
> on other platforms, but it should work as it asks the platform code
> what the path is instead of assuming.
> 
> Secondly, the following doesn't work as expected on AmigaOS and
> probably doesn't work on anything that isn't UNIX-derived:
> 
> > +/* process a file fetch */
> > +static void fetch_file_process(struct fetch_file_context *ctx)
> > +{
> > +	ctx->fd = open(ctx->path, O_RDONLY);
> > +	if (ctx->fd < 0) {
> > +		/* process errors as appropriate */
> [...]
> > +	if (S_ISDIR(ctx->fstat.st_mode)) {
> > +		/* directory listing */
> > +		fetch_file_process_dir(ctx);
> > +		return;
> > +	}
> 
> The issue is that if you open() a directory, it branches off to report
> an error (you can't open() directories) before getting to S_ISDIR() -
> so that code never executes.  S_ISDIR would fail anyway as there would
> be no file descriptor to pass to it.
> 
> I would guess that anything which doesn't have fdopendir() would also
> not be able to open() a directory.
> 
> If open() fails, opendir() should be called.  Only if that reports an
> error should the error branch be taken.
> 
> This should also resolve this comment:
> >	/* this poses the possibility of a race where the directory
> >	 * has been removed from the namespace or resources for more
> >	 * fd are now unavailable between the previous open() and this
> >	 * call.
> >	 */
> >	close(fd);
> >	scandir = opendir(ctx->path);
> 
> opendir() has already been called if fdopendir() is not available,
> just needs the pointer stored somewhere where the directory parser can
> get to it (being passed into the function would probably suffice)
> 
> Chris
> 

-- 
Regards Vincent
http://www.kyllikki.org/



More information about the netsurf-dev mailing list