Re: [Bug-tar] tar -T option
Sergey Poznyakoff ha escrit: > > 14:16:55 /tmp/tar$ ( ./bootstrap && ./configure && make -j5 ) &> /dev/null > Thanks, I see the problem now. Fixed in repository. Please pull. Regards, Sergey
Re: [Bug-tar] tar -T option
Pavel Raiskup ha escrit: > 14:16:20 /tmp$ rm -rf tar > 14:16:35 /tmp$ git clone git://git.savannah.gnu.org/tar.git &>/dev/null > 14:16:53 /tmp$ cd tar > 14:16:55 /tmp/tar$ ( ./bootstrap && ./configure && make -j5 ) &> /dev/null Thanks, I see the problem now. Regards, Sergey
Re: [Bug-tar] tar -T option
> ../src/tar -cf test.tar -T FILE > ../src/tar: configure: Cannot stat: No such file or directory > ../src/tar: Exiting with failure status due to previous errors > > which is kind of what I expected... Are you sure you tried it > with the latest tar from git head? Hmm, pretty sure: 14:16:20 /tmp$ rm -rf tar 14:16:35 /tmp$ git clone git://git.savannah.gnu.org/tar.git &>/dev/null 14:16:53 /tmp$ cd tar 14:16:55 /tmp/tar$ ( ./bootstrap && ./configure && make -j5 ) &> /dev/null 14:19:07 /tmp/tar$ echo "-Cpo" >> FILE 14:19:13 /tmp/tar$ echo "configure" >> FILE 14:19:18 /tmp/tar$ ./src/tar -cf /tmp/test.tar -T FILE 14:19:30 /tmp/tar$ ./src/tar -tf /tmp/test.tar configure 14:19:39 /tmp/tar$ cat FILE -Cpo configure 14:19:44 /tmp/tar$
Re: [Bug-tar] tar -T option
Hi Pavel, > .. && do we really need to parse all options? Wouldn't it be better to > artificially bound the -T FILE parsing power? I mean, this is starting to > have Turing machine power :) Well, basically, it provides a convenient way to store tar command lines into a kind of "configuration files": e.g., supposing that foo.cmd contains all the necessary options and arguments to do a particular task, just running `tar -T foo.cmd' should be enough to do it. A bit unexpected, perhaps, but in general I think it is a useful feature. > Trying again, wait .. I can observe that the lazy opt_parsing is broken > even for the -C option: Hmm... My tries give quite different results: ../src/tar -cf test.tar -T FILE ../src/tar: configure: Cannot stat: No such file or directory ../src/tar: Exiting with failure status due to previous errors which is kind of what I expected... Are you sure you tried it with the latest tar from git head? Regards, Sergey
Re: [Bug-tar] tar -T option
> So perhaps tar should allow subsequent (as opposed to recursive) reads of > the same file? Ahh, I see now what happens there :), thanks. Yes. That would make sense. .. && do we really need to parse all options? Wouldn't it be better to artificially bound the -T FILE parsing power? I mean, this is starting to have Turing machine power :) and such lazy option evaluation probably hides a lot of surprises... and users could potentially think everything is supported. Trying again, wait .. I can observe that the lazy opt_parsing is broken even for the -C option: $ ls po/ configure $ ls po/ # (empty) $ cat FILE -Cpo configure $ tar -cf test.tar -T FILE (this successes!) $ tar -tf test.tar configure Pavel
Re: [Bug-tar] tar -T option
Pavel Raiskup ha escrit: > tar -cvvf test.tar -T LIST1 -T LIST1 -T LIST2 > > Before these changes, tar failed immediately (before even tried to store > files defined in LIST1). Now it firstly stores everything from FILE1 and > then fails (it may take hours to process FILE1 to reach this failure). Ah, that's important indeed. Thanks for spotting that! Actually, the main reason why tar checks for the same file being read twice is to avoid recursive inclusion (e.g. "-T LIST1" appearing in LIST1 itself). So perhaps tar should allow subsequent (as opposed to recursive) reads of the same file? Regards, Sergey
Re: [Bug-tar] tar -T option
On Thursday, August 15, 2013 14:30:27 Christian Wetzel wrote: > What do you mean by 're-definition' ? Mentioning the same file twice ? I should rather say 'definition' by -T. Example: tar -cvvf test.tar -T LIST1 -T LIST1 -T LIST2 Before these changes, tar failed immediately (before even tried to store files defined in LIST1). Now it firstly stores everything from FILE1 and then fails (it may take hours to process FILE1 to reach this failure). Pavel
Re: [Bug-tar] tar -T option
What do you mean by 're-definition' ? Mentioning the same file twice ?
Re: [Bug-tar] tar -T option
> > - the semantics of -T option changed in relation to -C option. I am > > unable to find any note in changelog that this is expected - so I'm just > > not sure. Do you really want that? > > Actually, yes. I think that -C should affect all options that follow > it. I indeed failed to list that in the news file, something which I > will fix soon. Thanks. > I doubt that this change will have a big impact on > backward-compatibility: it took almost 6 years for *one* user to notice > that the behavior of -T option has changed since 1.15.1, so we can > reasonably assume that it does not belong to most widely used options. You are most probably right. One other note worth to document (considering that this decision is stable) is the checking for re-definition of the same file. Previously, the -T option failed very early — before creation of archive started.. But now it fails in the middle of tar's hard work (though, don't think there is much to do with that, except to just ignore the that situation istead of hard fail). Pavel
Re: [Bug-tar] tar -T option
Hi Pavel, > - there is one uninitialized variable, patch attached Thanks a lot! > - the semantics of -T option changed in relation to -C option. I am > unable to find any note in changelog that this is expected - so I'm just > not sure. Do you really want that? Actually, yes. I think that -C should affect all options that follow it. I indeed failed to list that in the news file, something which I will fix soon. Thanks a lot for noticing. I doubt that this change will have a big impact on backward-compatibility: it took almost 6 years for *one* user to notice that the behavior of -T option has changed since 1.15.1, so we can reasonably assume that it does not belong to most widely used options. Regards, Sergey
Re: [Bug-tar] tar -T option
Hi Sergey, > Just treat warnings as warnings :) Or apply the following patch, which > I have just pushed to the repository. thanks, I was able to recompile it - but I wanted exactly that ^^^ push from you. Thanks a lot for your work. I'm looking at the patch once again. Basically, I like the idea you implemented. But I'm unable to backport to Fedora such big change without a risk :(. (Ahm, the release cycle of GNU tar is so long.. and distributions need non-risky back-patching). - Something from my review: I see at least two problems there. - there is one uninitialized variable, patch attached - the semantics of -T option changed in relation to -C option. I am unable to find any note in changelog that this is expected - so I'm just not sure. Do you really want that? I know that the -T FILE's content is handled relatively to "current" directory. Thus it is probably more consistent to try to find FILE also relatively, but... if I'm thinking as user, I don't like specifying the files relatively to current cwd. (I can cooperate on fix if you don't have enough time to look at it) Pavel >From c4d5c14d5cc247db9016000f7832c81c4f0707c7 Mon Sep 17 00:00:00 2001 From: Pavel Raiskup Date: Thu, 15 Aug 2013 11:43:17 +0200 Subject: [PATCH] tar: fix use of uninitialized memory This complements 26538c9bf. * src/names.c (name_add_file): Initialize the ep->v.file.fp pointer after structure allocation. * tests/testsuite.at: Export MALLOC_PERTURB_ environmental variables for whole testsuite. This should avoid bugs like this. For platforms not supporting that it should be innocent, but lets see what will happen. --- src/names.c| 1 + tests/testsuite.at | 1 + 2 files changed, 2 insertions(+) diff --git a/src/names.c b/src/names.c index f3a3536..c8e2f03 100644 --- a/src/names.c +++ b/src/names.c @@ -291,6 +291,7 @@ name_add_file (const char *name, int term) ep->type = NELT_FILE; ep->v.file.name = name; ep->v.file.term = term; + ep->v.file.fp = NULL; /* not opened yet */ } /* Names from external name file. */ diff --git a/tests/testsuite.at b/tests/testsuite.at index 605cca3..7721b5b 100644 --- a/tests/testsuite.at +++ b/tests/testsuite.at @@ -33,6 +33,7 @@ TEST_TAR_FORMAT=FMT export TEST_TAR_FORMAT TAR_OPTIONS="-H FMT" export TAR_OPTIONS +export MALLOC_PERTURB_=170 rm -rf * $1)],$2,$3,$4,$5,$6) AT_TAR_CHECK_HOOK]) -- 1.8.3.1
Re: [Bug-tar] tar -T option
Hi Pavel,
> Thanks, I'm observing build fails now:
Just treat warnings as warnings :) Or apply the following patch, which
I have just pushed to the repository.
Regards,
Sergey
>From 95d7b37a34c9519b7ccc1063216527ccd2c2f833 Mon Sep 17 00:00:00 2001
From: Sergey Poznyakoff
Date: Mon, 5 Aug 2013 15:14:08 +0300
Subject: [PATCH] Minor changes
* src/names.c (handle_option): Use program_invocation_short_name
instead of the constant string.
(read_next_name): Remove unused variable.
---
src/names.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/names.c b/src/names.c
index 4710848..f3a3536 100644
--- a/src/names.c
+++ b/src/names.c
@@ -402,7 +402,7 @@ handle_option (const char *str)
if (wordsplit (str, &ws, WRDSF_DEFFLAGS|WRDSF_DOOFFS))
FATAL_ERROR ((0, 0, _("cannot split string '%s': %s"),
str, wordsplit_strerror (&ws)));
- ws.ws_wordv[0] = "tar";
+ ws.ws_wordv[0] = program_invocation_short_name;
more_options (ws.ws_wordc+ws.ws_offs, ws.ws_wordv);
for (i = 0; i < ws.ws_wordc+ws.ws_offs; i++)
ws.ws_wordv[i] = NULL;
@@ -414,8 +414,6 @@ handle_option (const char *str)
static int
read_next_name (struct name_elt *ent, struct name_elt *ret)
{
- enum read_file_list_state read_state;
-
if (!ent->v.file.fp)
{
if (!strcmp (ent->v.file.name, "-"))
--
1.7.12.1
Re: [Bug-tar] tar -T option
> I have installed the following patch, which fixes both the bugs in > -T handling discovered by Michal and reduces the memory consuption to > an acceptable level (the bug was reported by Christian). Thanks, I'm observing build fails now: ... CC incremen.o CC list.o CC misc.o CC names.o names.c: In function ‘handle_option’: names.c:405:18: error: assignment discards ‘const’ qualifier from pointer target type [-Werror] ws.ws_wordv[0] = "tar"; ^ names.c: In function ‘read_next_name’: names.c:417:29: error: unused variable ‘read_state’ [-Werror=unused-variable] enum read_file_list_state read_state; ^ CC sparse.o cc1: all warnings being treated as errors make[2]: *** [names.o] Error 1 make[2]: *** Waiting for unfinished jobs make[2]: Leaving directory `/tmp/tar/src' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/tmp/tar' make: *** [all] Error 2
