Bug 30893

Summary: [PATCH] burncd(8) progress meter
Product: Base System Reporter: Gerhard Sittig <Gerhard.Sittig>
Component: binAssignee: Søren Schmidt <sos>
Status: Closed FIXED    
Severity: Affects Only Me    
Priority: Normal    
Version: 4.3-STABLE   
Hardware: Any   
OS: Any   

Description Gerhard Sittig 2001-09-28 18:20:00 UTC 
burncd(8)'s progress meter is kept in absolute (KB) and
relative (%) measure when writing CD media.  Should the input
come from stdin (usually a pipe) or should the source file's
size be indetermined(id?) (seen with /dev/acd0c) the output
will show one percent for *every* KB written -- which obviously
is bogus.  The comment in burncd.c ack's this fact. :)

Fix: Apply the patch cited below.  This one was done in June and went
unanswered since (I take it it's my fault trying to contact
sos@FreeBSD.org via PM while he's busy doing other things, so I
understand this PR as being a more formal way of feedback for
better record or for the option of somebody else to pick it up).

The patch should apply cleanly to today's -STABLE since burncd.c
didn't move since rev 1.10.2.1 (2001/02/25 21:39:13).




virtually yours   82D1 9B9C 01DC 4FB4 D7B4  61BE 3F49 4F77 72DE DA76
Gerhard Sittig   true | mail -s "get gpg key" Gerhard.Sittig@gmx.net
-- 
     If you don't understand or are scared by any of the above
             ask your parents or an adult to help you.--PECm4OuxpcIguq3kEcBtzkLdrRRUk8UlbKJuDArX6KOHRbZc
Content-Type: text/plain; name="file.shar"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="file.shar"

# This is a shell archive.  Save it in a file, remove anything before
# this line, and then unpack it by entering "sh file".  Note, it may
# create directories; files and directories will be owned by you and
# have default permissions.
#
# This archive contains:
#
#	burncd.diff
#
echo x - burncd.diff
sed 's/^X//' >burncd.diff << 'END-of-burncd.diff'
XIndex: burncd.c
X===================================================================
XRCS file: /CVSREPO/FreeBSD/src/usr.sbin/burncd/burncd.c,v
Xretrieving revision 1.10.2.1
Xdiff -u -r1.10.2.1 burncd.c
X--- burncd.c	2001/02/25 21:39:13	1.10.2.1
X+++ burncd.c	2001/06/01 19:06:10
X@@ -234,11 +234,12 @@
X void
X write_file(const char *name, int block_size)
X {
X-	int addr, count, file, filesize, size;
X+	int addr, count, file, filesize, hasfilesize, size;
X 	char buf[2352*BLOCKS];
X 	struct stat stat;
X 	static int cdopen, done_stdin, tot_size = 0;
X 
X+	hasfilesize = 1;	/* assumption, to be corrected */
X 	if (!strcmp(name, "-")) {
X 		if (done_stdin) {
X 			warn("skipping multiple usages of stdin");
X@@ -246,6 +247,7 @@
X 		}
X 		file = STDIN_FILENO;
X 		done_stdin = 1;
X+		hasfilesize = 0;
X 	}
X 	else if ((file = open(name, O_RDONLY, 0)) < 0)
X 		err(EX_NOINPUT, "open(%s)", name);
X@@ -265,6 +267,8 @@
X 	if (fstat(file, &stat) < 0)
X 		err(EX_IOERR, "fstat(%s)", name);
X 	filesize = stat.st_size / 1024;
X+	if (filesize == 0)
X+		hasfilesize = 0;
X 
X 	if (!quiet) {
X 		fprintf(stderr, "next writeable LBA %d\n", addr);
X@@ -278,8 +282,6 @@
X 
X 	lseek(fd, addr * block_size, SEEK_SET);
X 	size = 0;
X-	if (filesize == 0)
X-		filesize++;	/* cheat, avoid divide by zero */
X 
X 	while ((count = read(file, buf, block_size * BLOCKS)) > 0) {	
X 		int res;
X@@ -299,7 +301,7 @@
X 			int pct;
X 
X 			fprintf(stderr, "written this track %d KB", size/1024);
X-			if (file != STDIN_FILENO) {
X+			if (hasfilesize) {
X 				pct = (size / 1024) * 100 / filesize;
X 				fprintf(stderr, " (%d%%)", pct);
X 			}
END-of-burncd.diff
exit
How-To-Repeat: 
$ SOURCE=/dev/acd0c
$ burncd -e -f /dev/acd1c -s 8 data $SOURCE fixate

$ $MKIMAGE | burncd -e -f /dev/acd1c -s 8 data - fixate
Comment 1 Peter Pentchev freebsd_committer freebsd_triage 2001-09-29 11:12:01 UTC 
Responsible Changed
From-To: freebsd-bugs->sos

Over to the author/maintainer.
Comment 2 Søren Schmidt freebsd_committer freebsd_triage 2001-12-12 14:12:07 UTC 
State Changed
From-To: open->closed

Fixed, but done differently than this patch.
Comment 3 Gerhard Sittig 2002-01-03 21:08:07 UTC 
On Wed, Dec 12, 2001 at 06:14 -0800, sos@FreeBSD.org wrote:
> 
> Synopsis: [PATCH] burncd(8) progress meter
> 
> State-Changed-From-To: open->closed
> State-Changed-By: sos
> State-Changed-When: Wed Dec 12 06:12:07 PST 2001
> State-Changed-Why: 
> Fixed, but done differently than this patch.

Excuse me, but now I'm a little confused. :)

Kind of following the cvs-all mailing list I didn't notice any
commit in the burncd area relevant to the bug since I submitted
the PR.  Your closing the PR with the "fixed" explanation three
weeks ago made me assume that even if you didn't commit the
provided fix yet you might have one in your local tree which
soon will make it into the repo.  But this didn't happen since
then.  Checking the -CURRENT sandbox I cvsupped this morning
and cvs up'ed right now I still found the "cheat, avoid divide
by zero" comment in burncd.c next to the code which leads to
bogus percentage values.  And still the criterion for calculating
the relative completion is whether the source is stdin or not.
I'm talking about
  $FreeBSD: src/usr.sbin/burncd/burncd.c,v 1.20 2001/12/27 10:10:56 sos Exp $
here.

> http://www.FreeBSD.org/cgi/query-pr.cgi?pr=30893

Please have another look at the patch I submitted and consider
to reopen the PR.  I still feel the problem exists and needs
attention (although it's a cosmetic problem and not esential
to the functionality of the program yet it is a bug).  The
above mentioned patch fixes the bogus calculation of one percent
per KB written and supresses percentage messages for any input
stream you don't have a total size for (the non stdin example
I gave was when burning from a CD in a neighbour drive and I
understand this holds for any non regular file like FIFOs and
the like).


virtually yours   82D1 9B9C 01DC 4FB4 D7B4  61BE 3F49 4F77 72DE DA76
Gerhard Sittig   true | mail -s "get gpg key" Gerhard.Sittig@gmx.net
-- 
     If you don't understand or are scared by any of the above
             ask your parents or an adult to help you.
Comment 4 Gerhard Sittig 2002-01-06 19:09:35 UTC 
On Fri, Jan 04, 2002 at 09:25 +0100, S=F8ren Schmidt wrote:
>=20
> It seems Gerhard Sittig wrote:
> >=20
> >  On Wed, Dec 12, 2001 at 06:14 -0800, sos@FreeBSD.org wrote:
> >  >=20
> >  > Synopsis: [PATCH] burncd(8) progress meter
> >  >=20
> >  > State-Changed-From-To: open->closed
> >  > State-Changed-By: sos
> >  > State-Changed-When: Wed Dec 12 06:12:07 PST 2001
> >  > State-Changed-Why:=20
> >  > Fixed, but done differently than this patch.
> > =20
> >  Excuse me, but now I'm a little confused. :)
> > =20
> >  [ ... haven't seen a fix for the situation getting
> >  committed, still feel the bug to be there and the
> >  patch to fix it;  thus asked to reopen the PR ]
>=20
> Hmm, you can rip out the "cheat" code, its actually not needed anymore.

No, it's still needed.  Well, sort of.  Applying my patch or
a different solution makes it obsolete. :)  But the current code
definitely is defective in repect of the relative progress meter.

> If you look at the code where the pct is printed you will see that it
> is ONLY printed when the input file is !=3D stdin.

Yes, I see this.  And I saw it back then.  It's simply wrong
since "it's not stdin" is *not* the criterion for "I can
calculate percentage values".  If it was, you wouldn't have
to cheat.  Please look at the
  $ burncd -e -f /dev/acd1c -s 8 -t data /dev/acd0c fixate
example and you see some output like this
  written this track 19552 KB (1955200%) total 19552 KB
  written this track 40928 KB (4092800%) total 40928 KB
  ...
  written this track 132768 KB (13276800%) total 132768 KB
I believe that any progress above 100% is bogus. :)

Please have another look at the patch.  The only criterion for
"percentage is available" can be "I know how much I have to do
(plus I know how much I did up to now, of course)".  The above
mentioned patch introduces this clear variable "hasfilesize".
Reading from regular files will set it (leave it set).  Reading
from stdin will drop it.  Plus (that's new and the case you did
not cover yet) non stdin input with indetermined(id?) sizes won't
show bogus percentage values.  You can even extend this approach
to "how should the percentage value be presented" if you like to.
But *please* reopen the PR or attack the bug in a different way.
Just don't state that the current behaviour is correct when it
isn't. :>


virtually yours   82D1 9B9C 01DC 4FB4 D7B4  61BE 3F49 4F77 72DE DA76
Gerhard Sittig   true | mail -s "get gpg key" Gerhard.Sittig@gmx.net
--=20
     If you don't understand or are scared by any of the above
             ask your parents or an adult to help you.