From takeda@chinatsu.takeda.tk Sat Mar 8 20:03:28 2008 Return-Path: Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8B9B41065670 for ; Sat, 8 Mar 2008 20:03:28 +0000 (UTC) (envelope-from takeda@chinatsu.takeda.tk) Received: from chinatsu.takeda.tk (h-74-0-89-210.lsanca54.covad.net [74.0.89.210]) by mx1.freebsd.org (Postfix) with ESMTP id 48B558FC1B for ; Sat, 8 Mar 2008 20:03:28 +0000 (UTC) (envelope-from takeda@chinatsu.takeda.tk) Received: from chinatsu.takeda.tk (takeda@localhost.takeda.tk [127.0.0.1]) by chinatsu.takeda.tk (8.14.2/8.13.8) with ESMTP id m28JV3DO059993 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Sat, 8 Mar 2008 11:31:03 -0800 (PST) (envelope-from takeda@chinatsu.takeda.tk) Received: (from takeda@localhost) by chinatsu.takeda.tk (8.14.2/8.13.8/Submit) id m28JV3C9059992; Sat, 8 Mar 2008 11:31:03 -0800 (PST) (envelope-from takeda) Message-Id: <200803081931.m28JV3C9059992@chinatsu.takeda.tk> Date: Sat, 8 Mar 2008 11:31:03 -0800 (PST) From: Derek Kulinski Reply-To: Derek Kulinski To: FreeBSD-gnats-submit@freebsd.org Cc: Subject: option -P appears to be broken in "restore" since FreeBSD 6.3 X-Send-Pr-Version: 3.113 X-GNATS-Notify: >Number: 121502 >Category: bin >Synopsis: [patch] option -P appears to be broken in restore(8) since FreeBSD 6.3 >Confidential: no >Severity: non-critical >Priority: medium >Responsible: jh >State: closed >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Sat Mar 08 20:10:01 UTC 2008 >Closed-Date: Mon May 17 14:27:14 UTC 2010 >Last-Modified: Mon May 17 14:27:14 UTC 2010 >Originator: Derek Kulinski >Release: FreeBSD 6.3-RELEASE-p1 i386 >Organization: none >Environment: System: FreeBSD chinatsu.takeda.tk 6.3-RELEASE-p1 FreeBSD 6.3-RELEASE-p1 #1: Sun Feb 17 18:52:39 PST 2008 root@chinatsu.takeda.tk:/usr/obj/usr/src/sys/CHINATSU i386 >Description: I wrote a simple shell script in the past that mounts remote filesystem, does the backup, and then checks if filesystem was dumped correctly. After upgrade to FreeBSD 6.3 (from 6.2) the script started to warn about errors in the backup files. According to fsck the filesystem appears to be fine. After futher investigation it turns out that the option -P is a problem: [chinatsu]:/root# gzcat /backup/dump.chinatsu.3.5.2008-03-08.var.gz | restore -xNf - set owner/mode for '.'? [yn] n [chinatsu]:/root# restore -xNP "gzcat /backup/dump.chinatsu.3.5.2008-03-08.var.gz" unknown tape header type 0 abort? [yn] n You have not read any tapes yet. If you are extracting just a few files, start with the last volume and work towards the first; restore can quickly skip tapes that have no further files to extract. Otherwise, begin with volume 1. Specify next volume #: 1 not at beginning of a file abort? [yn] n Segmentation fault Exit 139 >How-To-Repeat: The way my backup is done (I don't think this actually matters) is as follows: mksnap_ffs /usr /usr/.snap/backup dump -0 -auh 0 -f - /usr/.snap/backup | gzip -6 -c > /backup/backup.dump rm /usr/.snap/backup >Fix: My original command with option -P was as follows: (echo 1; echo n) | restore -xNP "gzcat /backup/backup.dump" Using pipe gets rid of the problem, unfortunately, for each backup the script will halt and wait for user input: set owner/mode for '.'? [yn] I don't know about any workaround for that (option -y doesn't seem to apply here) >Release-Note: >Audit-Trail: From: Jaakko Heinonen To: bug-followup@FreeBSD.org, takeda@takeda.tk Cc: Subject: Re: bin/121502: option -P appears to be broken in restore(8) since FreeBSD 6.3 (regression) Date: Sun, 23 Mar 2008 11:14:28 +0200 Hi, This seems to be somehow related to gzip(1) change in 6.3. Can you reproduce the problem with GNU gzip from ports? $ restore -xNP '/usr/bin/gzip -dc test.dump.gz' Header with wrong dumpdate. unknown tape header type 16843009 abort? [yn] y dump core? [yn] n $ restore -xNP '/usr/local/bin/gzip -dc test.dump.gz' Header with wrong dumpdate. You have not read any tapes yet. If you are extracting just a few files, start with the last volume and work towards the first; restore can quickly skip tapes that have no further files to extract. Otherwise, begin with volume 1. Specify next volume #: 1 set owner/mode for '.'? [yn] n -- Jaakko From: =?windows-1250?Q?Derek_Kuli=F1ski?= To: Jaakko Heinonen Cc: bug-followup@FreeBSD.org Subject: Re: bin/121502: option -P appears to be broken in restore(8) since FreeBSD 6.3 (regression) Date: Sun, 23 Mar 2008 02:49:45 -0700 Hello Jaakko, Sunday, March 23, 2008, 2:14:28 AM, you wrote: > This seems to be somehow related to gzip(1) change in 6.3. Can you > reproduce the problem with GNU gzip from ports? I was actually really puzzled by this, after I submitted the PR, I was looking at changes to restore and dump commands and found that there was no significant changes, I even downgraded those commands to those from 6.2 and they still behaved the same. Now I tried to your suggestion and it seems to work as it supposed to with the gzip from ports. Well.. I see the "Header with wrong dumpdate." but other than that it works fine. I would never thought that gzip was to blame for that... P.S. I upgraded to 7.0 recently, but it didn't affect that bug anyway. -- Derek From: Jaakko Heinonen To: Derek =?utf-8?B?S3VsacWEc2tp?= Cc: bug-followup@FreeBSD.org Subject: Re: bin/121502: option -P appears to be broken in restore(8) since FreeBSD 6.3 (regression) Date: Mon, 24 Mar 2008 17:35:23 +0200 --Q68bSM7Ycu6FN28Q Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit On 2008-03-23, Derek Kuliński wrote: > I would never thought that gzip was to blame for that... Although this not a regression in restore(8) I think it's a bug in it. restore(8) has a special block reading code for pipes. This code handles short reads from pipes. However the code is only enabled if your source file name is "-" (stdin) not when you use the -P switch. Could you try the attached patch which enables the special handling for pipes when using the -P switch? -- Jaakko --Q68bSM7Ycu6FN28Q Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="restore-pipeincmd-as-pipe.diff" Index: tape.c =================================================================== RCS file: /home/ncvs/src/sbin/restore/tape.c,v retrieving revision 1.49 diff -p -u -r1.49 tape.c --- tape.c 6 Mar 2007 08:13:20 -0000 1.49 +++ tape.c 24 Mar 2008 10:51:49 -0000 @@ -134,9 +134,10 @@ setinput(char *source, int ispipecommand newtapebuf(NTREC > HIGHDENSITYTREC ? NTREC : HIGHDENSITYTREC); terminal = stdin; - if (ispipecommand) + if (ispipecommand) { pipecmdin++; - else + pipein++; + } else #ifdef RRESTORE if (strchr(source, ':')) { host = source; --Q68bSM7Ycu6FN28Q-- From: =?utf-8?Q?Derek_Kuli=C5=84ski?= To: Jaakko Heinonen Cc: bug-followup@FreeBSD.org Subject: Re: bin/121502: option -P appears to be broken in restore(8) since FreeBSD 6.3 (regression) Date: Mon, 24 Mar 2008 14:50:21 -0700 Monday, March 24, 2008, 8:35:23 AM, Jaakko Heinonen wrote: > Could you try the attached patch which enables the special handling for > pipes when using the -P switch? It seems to work fine with both, the standard gzip and the one from ports. -- Derek From: =?utf-8?Q?Derek_Kuli=C5=84ski?= To: Jaakko Heinonen Cc: bug-followup@FreeBSD.org Subject: Re: bin/121502: option -P appears to be broken in restore(8) since FreeBSD 6.3 (regression) Date: Mon, 24 Mar 2008 14:57:24 -0700 One more thing, before patching, there's a different behavior: [chinatsu]:/usr/src/sbin/restore# restore -xNP "/usr/local/bin/gzip -dc /backup/dump.chinatsu.1.1.2008-03-20.var.gz" Header with wrong dumpdate. You have not read any tapes yet. If you are extracting just a few files, start with the last volume and work towards the first; restore can quickly skip tapes that have no further files to extract. Otherwise, begin with volume 1. Specify next volume #: 1 set owner/mode for '.'? [yn] n [chinatsu]:/usr/src/sbin/restore# restore -xNP "/usr/bin/gzip -dc /backup/dump.chinatsu.1.1.2008-03-20.var.gz" Header with wrong dumpdate. You have not read any tapes yet. If you are extracting just a few files, start with the last volume and work towards the first; restore can quickly skip tapes that have no further files to extract. Otherwise, begin with volume 1. Specify next volume #: 1 unknown tape header type 16843009 abort? [yn] n Mount tape volume 2 Enter ``none'' if there are no more tapes otherwise enter tape name (default: /usr/bin/gzip -dc /backup/dump.chinatsu.1.1.2008-03-20.var.gz) When I use it with gzip from ports, it behaves pretty much like it did in the past (asking for the volume number) I'm not sure if that was correct or not. According to manpage, the RESTORE_VOLUME variable supposed to be defined each time restore is called. -- Derek From: Jaakko Heinonen To: Derek =?utf-8?B?S3VsacWEc2tp?= Cc: bug-followup@FreeBSD.org, green@FreeBSD.org Subject: Re: bin/121502: option -P appears to be broken in restore(8) since FreeBSD 6.3 (regression) Date: Tue, 25 Mar 2008 11:10:20 +0200 Hi, On 2008-03-24, Derek Kuliński wrote: > One more thing, before patching, there's a different behavior: > When I use it with gzip from ports, it behaves pretty much like it did > in the past (asking for the volume number) Looking at the source code the behavior after patching might be correct: (tape.c) 308 if (pipein) { 309 if (nextvol != 1) { 310 panic("Changing volumes on pipe input?\n"); 311 /* Avoid looping if we couldn't ask the user. */ 312 if (yflag || ferror(terminal) || feof(terminal)) 313 done(1); 314 } 315 if (volno == 1) 316 return; 317 if (pipecmdin) { 318 closemt(); 319 goto getpipecmdhdr; 320 } 321 goto gethdr; 322 } In unpatched restore(8) pipein and pipecmdin are never set at the same time. Thus the code never reaches line 319 which causes restore(8) to skip "Specify next volume #: " prompt. (When volno == 1 it's already skipped at line 316.) However looking at it more carefully the goto on line 319 can't be correct either because then newvol will be uninitialized at line 409. Seems that -P has been broken since it's introduction to tape.c revision 1.39. I have cc'd green@ who committed the revision 1.39. He should know what the intended behavior is. (Please see http://www.freebsd.org/cgi/query-pr.cgi?pr=121502 what we have found out so far.) -- Jaakko From: Jaakko Heinonen To: Derek =?utf-8?B?S3VsacWEc2tp?= Cc: bug-followup@FreeBSD.org, green@FreeBSD.org Subject: Re: bin/121502: option -P appears to be broken in restore(8) since FreeBSD 6.3 (regression) Date: Sun, 30 Mar 2008 22:28:55 +0300 --huq684BweRXVnRxX Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit Hi, On 2008-03-23, Derek Kuliński wrote: > Now I tried to your suggestion and it seems to work as it supposed to > with the gzip from ports. Well.. I see the "Header with wrong > dumpdate." but other than that it works fine. This is a separate bug. See the PR bin/118087 (http://www.freebsd.org/cgi/query-pr.cgi?pr=118087). On 2008-03-24, Derek Kuliński wrote: > One more thing, before patching, there's a different behavior: Here's a patch that keeps the behavior same. The patch just enables the short read code in readtape() (tape.c) when -P is used. There's still obviously wrong code related to -P option in getvol(). I have marked it with XXX comment. -- Jaakko --huq684BweRXVnRxX Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="restore-pipecmdin.diff" Index: tape.c =================================================================== RCS file: /home/ncvs/src/sbin/restore/tape.c,v retrieving revision 1.49 diff -p -u -r1.49 tape.c --- tape.c 6 Mar 2007 08:13:20 -0000 1.49 +++ tape.c 30 Mar 2008 17:17:11 -0000 @@ -333,6 +333,12 @@ getvol(long nextvol) } if (volno == 1) return; + /* + * XXX: Following if branch is never reached because pipein + * XXX: and pipecmdin are never set at the same time. + * XXX: Secondly the goto to getpipecmdhdr can't be correct + * XXX: because it skips the code where newvol is initialized. + */ if (pipecmdin) { closemt(); goto getpipecmdhdr; @@ -1202,17 +1208,17 @@ getmore: * Check for mid-tape short read error. * If found, skip rest of buffer and start with the next. */ - if (!pipein && numtrec < ntrec && i > 0) { + if ((!pipein && !pipecmdin) && numtrec < ntrec && i > 0) { dprintf(stdout, "mid-media short read error.\n"); numtrec = ntrec; } /* * Handle partial block read. */ - if (pipein && i == 0 && rd > 0) + if ((pipein || pipecmdin) && i == 0 && rd > 0) i = rd; else if (i > 0 && i != ntrec * TP_BSIZE) { - if (pipein) { + if (pipein || pipecmdin) { rd += i; cnt -= i; if (cnt > 0) --huq684BweRXVnRxX-- From: Gavin Atkinson To: bug-followup@FreeBSD.org Cc: Subject: Re: bin/121502: [patch] option -P appears to be broken in restore(8) since FreeBSD 6.3 [regression] Date: Fri, 29 Aug 2008 12:19:34 +0100 Submitter of duplicate PR bin/126882 confirms that the most recent patch in this PR fixes things for him. Responsible-Changed-From-To: freebsd-bugs->jh Responsible-Changed-By: jh Responsible-Changed-When: Thu Jan 28 15:55:34 UTC 2010 Responsible-Changed-Why: Take. http://www.freebsd.org/cgi/query-pr.cgi?pr=121502 From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: bin/121502: commit references a PR Date: Fri, 29 Jan 2010 10:04:09 +0000 (UTC) Author: jh Date: Fri Jan 29 10:04:00 2010 New Revision: 203157 URL: http://svn.freebsd.org/changeset/base/203157 Log: - Handle short reads when the -P option is used. Short reads must be handled when reading from pipes. - Remove dead code related to the -P option from getvol(). pipein and pipecmdin are never set at the same time. PR: bin/121502 Approved by: trasz (mentor) MFC after: 2 weeks Modified: head/sbin/restore/tape.c Modified: head/sbin/restore/tape.c ============================================================================== --- head/sbin/restore/tape.c Fri Jan 29 10:02:50 2010 (r203156) +++ head/sbin/restore/tape.c Fri Jan 29 10:04:00 2010 (r203157) @@ -333,10 +333,6 @@ getvol(long nextvol) } if (volno == 1) return; - if (pipecmdin) { - closemt(); - goto getpipecmdhdr; - } goto gethdr; } again: @@ -400,7 +396,6 @@ again: if (pipecmdin) { char volno[sizeof("2147483647")]; -getpipecmdhdr: (void)sprintf(volno, "%ld", newvol); if (setenv("RESTORE_VOLUME", volno, 1) == -1) { fprintf(stderr, "Cannot set $RESTORE_VOLUME: %s\n", @@ -1205,17 +1200,17 @@ getmore: * Check for mid-tape short read error. * If found, skip rest of buffer and start with the next. */ - if (!pipein && numtrec < ntrec && i > 0) { + if (!pipein && !pipecmdin && numtrec < ntrec && i > 0) { dprintf(stdout, "mid-media short read error.\n"); numtrec = ntrec; } /* * Handle partial block read. */ - if (pipein && i == 0 && rd > 0) + if ((pipein || pipecmdin) && i == 0 && rd > 0) i = rd; else if (i > 0 && i != ntrec * TP_BSIZE) { - if (pipein) { + if (pipein || pipecmdin) { rd += i; cnt -= i; if (cnt > 0) _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" State-Changed-From-To: open->patched State-Changed-By: jh State-Changed-When: Fri Jan 29 10:22:55 UTC 2010 State-Changed-Why: Patched in head (r203157). http://www.freebsd.org/cgi/query-pr.cgi?pr=121502 From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: bin/121502: commit references a PR Date: Sat, 13 Feb 2010 10:22:21 +0000 (UTC) Author: jh Date: Sat Feb 13 10:22:07 2010 New Revision: 203816 URL: http://svn.freebsd.org/changeset/base/203816 Log: Don't try to determine tape block size when the -P option is used. This was missed in r203157. PR: bin/121502 Modified: head/sbin/restore/tape.c Modified: head/sbin/restore/tape.c ============================================================================== --- head/sbin/restore/tape.c Sat Feb 13 09:45:50 2010 (r203815) +++ head/sbin/restore/tape.c Sat Feb 13 10:22:07 2010 (r203816) @@ -227,7 +227,7 @@ setup(void) volno = 1; setdumpnum(); FLUSHTAPEBUF(); - if (!pipein && !bflag) + if (!pipein && !pipecmdin && !bflag) findtapeblksize(); if (gethead(&spcl) == FAIL) { fprintf(stderr, "Tape is not a dump tape\n"); _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: bin/121502: commit references a PR Date: Sat, 20 Feb 2010 13:35:24 +0000 (UTC) Author: jh Date: Sat Feb 20 13:35:05 2010 New Revision: 204119 URL: http://svn.freebsd.org/changeset/base/204119 Log: MFC r203157, r203816: Handle short reads when the -P option is used and remove some dead code. PR: bin/121502 Modified: stable/8/sbin/restore/tape.c Directory Properties: stable/8/sbin/restore/ (props changed) Modified: stable/8/sbin/restore/tape.c ============================================================================== --- stable/8/sbin/restore/tape.c Sat Feb 20 13:33:50 2010 (r204118) +++ stable/8/sbin/restore/tape.c Sat Feb 20 13:35:05 2010 (r204119) @@ -227,7 +227,7 @@ setup(void) volno = 1; setdumpnum(); FLUSHTAPEBUF(); - if (!pipein && !bflag) + if (!pipein && !pipecmdin && !bflag) findtapeblksize(); if (gethead(&spcl) == FAIL) { fprintf(stderr, "Tape is not a dump tape\n"); @@ -333,10 +333,6 @@ getvol(long nextvol) } if (volno == 1) return; - if (pipecmdin) { - closemt(); - goto getpipecmdhdr; - } goto gethdr; } again: @@ -400,7 +396,6 @@ again: if (pipecmdin) { char volno[sizeof("2147483647")]; -getpipecmdhdr: (void)sprintf(volno, "%d", newvol); if (setenv("RESTORE_VOLUME", volno, 1) == -1) { fprintf(stderr, "Cannot set $RESTORE_VOLUME: %s\n", @@ -1204,17 +1199,17 @@ getmore: * Check for mid-tape short read error. * If found, skip rest of buffer and start with the next. */ - if (!pipein && numtrec < ntrec && i > 0) { + if (!pipein && !pipecmdin && numtrec < ntrec && i > 0) { dprintf(stdout, "mid-media short read error.\n"); numtrec = ntrec; } /* * Handle partial block read. */ - if (pipein && i == 0 && rd > 0) + if ((pipein || pipecmdin) && i == 0 && rd > 0) i = rd; else if (i > 0 && i != ntrec * TP_BSIZE) { - if (pipein) { + if (pipein || pipecmdin) { rd += i; cnt -= i; if (cnt > 0) _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: bin/121502: commit references a PR Date: Sun, 16 May 2010 16:33:56 +0000 (UTC) Author: jh Date: Sun May 16 16:33:38 2010 New Revision: 208153 URL: http://svn.freebsd.org/changeset/base/208153 Log: MFC r203157, r203816: Handle short reads when the -P option is used and remove some dead code. PR: bin/121502 Modified: stable/7/sbin/restore/tape.c Directory Properties: stable/7/sbin/restore/ (props changed) Modified: stable/7/sbin/restore/tape.c ============================================================================== --- stable/7/sbin/restore/tape.c Sun May 16 15:56:59 2010 (r208152) +++ stable/7/sbin/restore/tape.c Sun May 16 16:33:38 2010 (r208153) @@ -227,7 +227,7 @@ setup(void) volno = 1; setdumpnum(); FLUSHTAPEBUF(); - if (!pipein && !bflag) + if (!pipein && !pipecmdin && !bflag) findtapeblksize(); if (gethead(&spcl) == FAIL) { fprintf(stderr, "Tape is not a dump tape\n"); @@ -333,10 +333,6 @@ getvol(long nextvol) } if (volno == 1) return; - if (pipecmdin) { - closemt(); - goto getpipecmdhdr; - } goto gethdr; } again: @@ -400,7 +396,6 @@ again: if (pipecmdin) { char volno[sizeof("2147483647")]; -getpipecmdhdr: (void)sprintf(volno, "%d", newvol); if (setenv("RESTORE_VOLUME", volno, 1) == -1) { fprintf(stderr, "Cannot set $RESTORE_VOLUME: %s\n", @@ -1204,17 +1199,17 @@ getmore: * Check for mid-tape short read error. * If found, skip rest of buffer and start with the next. */ - if (!pipein && numtrec < ntrec && i > 0) { + if (!pipein && !pipecmdin && numtrec < ntrec && i > 0) { dprintf(stdout, "mid-media short read error.\n"); numtrec = ntrec; } /* * Handle partial block read. */ - if (pipein && i == 0 && rd > 0) + if ((pipein || pipecmdin) && i == 0 && rd > 0) i = rd; else if (i > 0 && i != ntrec * TP_BSIZE) { - if (pipein) { + if (pipein || pipecmdin) { rd += i; cnt -= i; if (cnt > 0) _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" State-Changed-From-To: patched->closed State-Changed-By: jh State-Changed-When: Mon May 17 14:27:12 UTC 2010 State-Changed-Why: Fixed in head, stable/8 and stable/7. http://www.freebsd.org/cgi/query-pr.cgi?pr=121502 >Unformatted: