develooper Front page | perl.perl5.porters | Postings from February 2003

Re: Smoke [5.9.0] 18609 FAIL(F) MSWin32 5.1 (x86/1 cpu)

Thread Previous | Thread Next
From:
Enache Adrian
Date:
February 2, 2003 10:00
Subject:
Re: Smoke [5.9.0] 18609 FAIL(F) MSWin32 5.1 (x86/1 cpu)
Message ID:
20030202180732.GB931@ratsnest.hole
On Sun, Feb 02, 2003 at 11:37:20AM +0000, Nick Ing-Simmons wrote:
> >
> >That' still kludgy somehow.
> 
> Yes - but it is pragmatic.

I was meaning by 'kludge' my hack, not your modifications to it. Its
kludginess became me obvious when I start thinking about various
stream-like i/o layers.

> What I most dislike about the read_record scheme is that can give wrong 
> return data if the "guess" is too small - it does not re-try looking 
> for more. Also if you look at the the patch you will notice that the 
> read-record block also has VMS specific code which bypasses layer stack
> to get special file-system record behaviour. So 'goto' that for $/ = undef
> breaks :encoding() on VMS.

I think it's fine not to use the record mode code.

> >My suggestion is to add a new layer-level function 'PerlIO_size()' wired
> >to a 'Size' callback from the _PerlIO_funcs structure.
> 
> I am not convinced that would be better than the "kludge" as modified 
> by my patch. The only layers which "know" the size are non-translating
> binary ones associated with disk files (fstat works) or in-memory data
> (see below).

Three reasons why I find this nicer:

1) A simple function call is always _much_ cheaper than a (failed)
system call.  As this function can be very simple -

SSize_t
Perl_PerlIO_size(pTHX_ PerlIO *f)
{
    if (PerlIOBase(f)->tab->Size)
	return (*PerlIOBase(f)->tab->Size) (aTHX_ f);
    else
	return -1;
}

- it'll be most probably inlined by the compiler.

When using the non-layered i/o model we'll use a macro:

#define PerlIO_size(f) \
    (fstat(fileno(f), &PL_statbuf) ? -1 : PL_statbuf.st_size)

Exactly what this macro does will do also the 'Size' callback in
the 'normal' (:perlio, :stdio, :crlf, etc) layers.

In transforming layers 'Size' will be a NULL pointer. So it's a
test-for-NULL instead of a system call here.

2) 'Size' could be implemented differently by random i/o layers.

3) It's generally bad to call low-level functions when using a layer.

> The in-memory data case is going to 
> be fast anyway. With the patch such streams will presumably fail
> the fstat as they will not have a fileno, so we fallback to reading 
> in stream-buffer sized chunks. An in-memory stream (e.g. PerlIO_scalar)
> will normally own up to whole memory chunk as the buffer, so it 
> will in simple case (where it is top of layer stack) read in one go. 
> If there are layers on top then incremental is safer anyway.

Maybe someone will try to use a weird in-memory stream which do not
have the classical cnt, ptr.

I'll definitely stop here insisting on something you probably dislike.

However, an issue remains since my first patch - st_size is 64 bits.
If the user tries to slurp a file > 2G SvGROW() may grow the variable
to something negative.
If the file is void, the code will gratuitously realloc the variable
growing it by one (and then try to read when it's already known that
this isn't possible).

Maybe this will be more correct ?
-----------------------------------------------------------
--- /tmp/sv.c	2003-02-02 19:36:04.000000000 +0200
+++ sv.c	2003-02-02 19:32:17.000000000 +0200
@@ -6180,10 +6180,9 @@
 	 */
 	Stat_t st;
 	if (!PerlLIO_fstat(PerlIO_fileno(fp), &st) && S_ISREG(st.st_mode))  {
-	    Off_t offset = PerlIO_tell(fp);
-	    if (offset != (Off_t) -1) {
-	     	(void) SvGROW(sv, (STRLEN)((st.st_size - offset) + append + 1));
-	    }
+	    recsize = st.st_size - PerlIO_tell(fp);
+	    if (recsize > 0)
+	     	(void) SvGROW(sv, (STRLEN)(recsize + append + 1));
 	}
 	rsptr = NULL;
 	rslen = 0;

Regards
Adi

Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About