[PATCH] minor size optimization for dd.c

[Rob Landley]

On Saturday 18 March 2006 6:20 pm, Bernhard Fischer wrote:
> On Wed, Mar 15, 2006 at 09:21:51AM +0200, Denis Vlasenko wrote:
> >On Tuesday 14 March 2006 15:41, Tito wrote:
> >> Hi,
> >> this patch reduces size of dd.c.
> >> Size reduction is:
> >>
> >>    text    data     bss     dec     hex filename
> >>   1346       0       0    1346     542 coreutils/dd.o.orig
> >>   1302       0       0    1302     516 coreutils/dd.o
> >>
> >> Changes are trivial.
> >>
> >> Please apply.
> >
> >                if ((ofd = open(outfile, oflag, 0666)) < 0) {
> >-                       bb_perror_msg_and_die("%s", outfile);
> >+                       goto OUTFILE_ERROR;
> >                }
> >
> >I think it is better to use bb_xopen(), which will be doing
> >the bb_perror_msg_and_die thing to you. Pity current bb_xopen()
> >has only two arguments, but it's trivial to have three-argument one.
> >See at http://195.66.192.167/linux/bbox/bb_xopen3.patch
>
> landley disliked this the bb_xopen3 idea, iirc.
>
> Rob, care to elaborate on why you don't want it, please?

More I decided it wasn't something to worry about for 1.1.1 and I put it in my 
"to revisit" pile.

Having two different functions that close is slightly uncomfortable, but then 
we have error and perror variants already, so it's not like there isn't 
precedent.

We need to document libbb...

> Tito, I'd propose we revisit this after rob has branched a
> busybox_1_1_stable branch, so we have clear sight for 1.2 assuming that
> 1.1 will go into bug-fix only mode, ok?

I'm not branching.  I'm releasing. :)

We branched the tree between 1.0 and 1.1 and it didn't really help matters.  
It's easier to just have one line of development and say "release X is svn 
xxxx".  (Can't really say that for 1.0.1.)

I expect to have a month or two of new development going into the three, 
followed by a couple weeks of bug fixing, and a release.  A good regression 
test should help this theory immensely. :)

But yeah, right after 1.1.1 go for it.

> Thank you,

Rob

> >diff -urpN busybox.org/libbb/xfuncs.c busybox.unlzma2/libbb/xfuncs.c
> >--- busybox.org/libbb/xfuncs.c	Tue Mar  7 07:58:22 2006
> >+++ busybox.unlzma2/libbb/xfuncs.c	Mon Mar 13 09:59:22 2006
> >@@ -108,6 +108,19 @@ int bb_xopen(const char *pathname, int f
> > }
> > #endif
> >
> >+#ifdef L_xopen3
> >+int bb_xopen3(const char *pathname, int flags, int mode)
> >+{
> >+	int ret;
> >+
> >+	ret = open(pathname, flags, mode);
> >+	if (ret == -1) {
> >+		bb_perror_msg_and_die("%s", pathname);
> >+	}
> >+	return ret;
> >+}
> >+#endif

If we do have this, shouldn't bb_xopen() call bb_xopen3?  In theory it's 
smaller, plus the extra function call is going to get _swamped_ by the I/O 
request, and the extra stack depth shouldn't be too bad either.

> >Or else you may change bb_xopen() definition so that
> >it can take 3 params. FYI: libc defines open() as
> >int open(const char *file, int oflag, ...);
> >
> >Unfortunately, it's not trivial to do the same to lseek etc:
> >
> >	bb_xlseek(ifd, skip * bs, SEEK_CUR);
> >
> >does not have any way to report _filename_ on error :(
> >Maybe
> >
> >	bb_xlseek(ifd, skip * bs, SEEK_CUR, infile);

No thanks.

What's the failure case for lseek, anyway?  The file wasn't that long?  Is 
this a real problem someone actually saw?

I don't particularly care about errors that only occur on NFS, which is broken 
by design in a bunch of ways.  (Yes, it's a common embedded development 
environment, but how common is it as an embedded _deployment_ environment?)

> >where "infile" is a filename to use in bb_perror_msg_and_die
> >if lseek will fail.

What's a use case for this?  Where are we currently checking the return value 
of lseek?

Rob
-- 
Never bet against the cheap plastic solution.