FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Debian > Debian dpkg

 
 
LinkBack Thread Tools
 
Old 07-03-2010, 11:30 PM
Jonathan Nieder
 
Default leverage -x/-X tar call to pass more tar options

Hi,

Pierre Habouzit wrote:

> The point is that doing chained pipes is pretty annoying from C
> (especially if you want to use parallelism in the process), plus it kind
> of make sense for many kind of operations where you don't need
> --fsys-tarfile anymore: e.g. extracting a single file can be done with:
>
> dpkg-deb -x file.deb some-dir ./path/to/file/to/extract

I have wished for something like this before.

> +++ b/dpkg-deb/extract.c
> @@ -325,7 +329,17 @@ void extracthalf(const char *debar, const char *directory,
>
> unsetenv("TAR_OPTIONS");
>
> - execlp(TAR, "tar", buffer, "-", NULL);
> + for (i = 0; taropts && taropts[i]; i++);

Produces a warning from gcc.

> + arg = targv = m_malloc((5 + i + 1) * sizeof(targv[0]));

Why not (4 + i) * sizeof(targv[0])?

> + *arg++ = "tar";
> + *arg++ = buffer;
> + *arg++ = "-";
> + if (i) {

Needed for the !taropts case. Good.

> + mempcpy(arg, taropts, i * sizeof(taropts[0]));
> + arg += i;
> + }
> + *arg++ = NULL;
> + execvp(TAR, (char *const *)targv);
> ohshite(_("failed to exec tar"));
> }
> close(p2[0]);

I couldn’t find any other uses of mempcpy in dpkg, so it might be better
to use memcpy() or use a wrapper like git does.

> diff --git a/dpkg-deb/main.c b/dpkg-deb/main.c
> index 319e715..8238720 100644
> --- a/dpkg-deb/main.c
> +++ b/dpkg-deb/main.c
> @@ -78,8 +78,10 @@ usage(const struct cmdinfo *cip, const char *value)
> " -W|--show <deb> Show information on package(s)
"
> " -f|--field <deb> [<cfield> ...] Show field(s) to stdout.
"
> " -e|--control <deb> [<directory>] Extract control info.
"
> -" -x|--extract <deb> <directory> Extract files.
"
> -" -X|--vextract <deb> <directory> Extract & list files.
"
> +" -x|--extract <deb> <directory> [tar-options...]
"
> +" Extract files.
"
> +" -X|--vextract <deb> <directory> [tar-options...]
"
> +" Extract & list files.
"
> " --fsys-tarfile <deb> Output filesystem tarfile.
"
> "
"));
>

Might be worth hinting that myopt will try to consume any tar option
flags itself. Something like:

" -x|--extract <deb> <directory> [[--] tar-options...]
"
" Extract files.
"
" -X|--vextract <deb> <directory> [[--] tar-options...]
"
" Extract & list files.
"

> diff --git a/man/dpkg-deb.1 b/man/dpkg-deb.1
> index ec451ef..c5db6e7 100644
> --- a/man/dpkg-deb.1
> +++ b/man/dpkg-deb.1
> @@ -123,9 +123,11 @@ package archive. It is currently produced in the format generated by
> .BR tar 's
> verbose listing.
> .TP
> -.BR -x ", " --extract " fIarchive directoryfP"
> +.BR -x ", " --extract " fIarchive directoryfP [fItar-optionsfP...]"
> Extracts the filesystem tree from a package archive into the specified
> -directory.
> +directory. Additionnal arguments are passed straight to
> +.BR tar (1)
> + .

Likewise.

>
> Note that extracting a package to the root directory will
> .I not
> @@ -137,7 +139,7 @@ to install packages.
> (but not its parents) will be created if necessary, and its permissions
> modified to match the contents of the package.
> .TP
> -.BR -X ", " --vextract " fIarchive directoryfP"
> +.BR -X ", " --vextract " fIarchive directoryfP [fItar-optionsfP...]"
> Is like
> .BR --extract " (" -x ")"
> but prints a listing of the files extracted as it goes.

Likewise.

With whatever subset of the changes suggested seems suitable,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

diff --git a/dpkg-deb/extract.c b/dpkg-deb/extract.c
index 914224b..2f6ac57 100644
--- a/dpkg-deb/extract.c
+++ b/dpkg-deb/extract.c
@@ -329,13 +329,18 @@ void extracthalf(const char *debar, const char *directory,

unsetenv("TAR_OPTIONS");

- for (i = 0; taropts && taropts[i]; i++);
- arg = targv = m_malloc((5 + i + 1) * sizeof(targv[0]));
+ for (i = 0; taropts && taropts[i]; i++)
+ ; /* just counting */
+ arg = targv = m_malloc((
+ 3 + /* tar tvf - */
+ i + /* taropts */
+ 1 /* terminating NULL */
+ ) * sizeof(targv[0]));
*arg++ = "tar";
*arg++ = buffer;
*arg++ = "-";
if (i) {
- mempcpy(arg, taropts, i * sizeof(taropts[0]));
+ memcpy(arg, taropts, i * sizeof(taropts[0]));
arg += i;
}
*arg++ = NULL;
diff --git a/dpkg-deb/main.c b/dpkg-deb/main.c
index 8238720..3ad20e6 100644
--- a/dpkg-deb/main.c
+++ b/dpkg-deb/main.c
@@ -78,9 +78,9 @@ usage(const struct cmdinfo *cip, const char *value)
" -W|--show <deb> Show information on package(s)
"
" -f|--field <deb> [<cfield> ...] Show field(s) to stdout.
"
" -e|--control <deb> [<directory>] Extract control info.
"
-" -x|--extract <deb> <directory> [tar-options...]
"
+" -x|--extract <deb> <directory> [[--] tar-options...]
"
" Extract files.
"
-" -X|--vextract <deb> <directory> [tar-options...]
"
+" -X|--vextract <deb> <directory> [[--] tar-options...]
"
" Extract & list files.
"
" --fsys-tarfile <deb> Output filesystem tarfile.
"
"
"));
diff --git a/man/dpkg-deb.1 b/man/dpkg-deb.1
index c5db6e7..c267734 100644
--- a/man/dpkg-deb.1
+++ b/man/dpkg-deb.1
@@ -123,11 +123,13 @@ package archive. It is currently produced in the format generated by
.BR tar 's
verbose listing.
.TP
-.BR -x ", " --extract " fIarchive directoryfP [fItar-optionsfP...]"
+.BR -x ", " --extract " fIarchive directoryfP [[--] fItar-optionsfP...]"
Extracts the filesystem tree from a package archive into the specified
-directory. Additionnal arguments are passed straight to
-.BR tar (1)
- .
+directory. Any arguments left over after dpkg-deb processes its own are
+passed to
+.BR tar "(1)."
+Option flags (e.g., --exclude) are only passed to tar if preceded by
+-- on the command line.

Note that extracting a package to the root directory will
.I not
@@ -139,7 +141,7 @@ to install packages.
(but not its parents) will be created if necessary, and its permissions
modified to match the contents of the package.
.TP
-.BR -X ", " --vextract " fIarchive directoryfP [fItar-optionsfP...]"
+.BR -X ", " --vextract " fIarchive directoryfP [[--] fItar-optionsfP...]"
Is like
.BR --extract " (" -x ")"
but prints a listing of the files extracted as it goes.
--


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100703233058.GA25140@burratino">http://lists.debian.org/20100703233058.GA25140@burratino
 
Old 07-03-2010, 11:34 PM
Pierre Habouzit
 
Default leverage -x/-X tar call to pass more tar options

On Sat, Jul 03, 2010 at 06:30:58PM -0500, Jonathan Nieder wrote:
> Hi,
>
> Pierre Habouzit wrote:
>
> > The point is that doing chained pipes is pretty annoying from C
> > (especially if you want to use parallelism in the process), plus it kind
> > of make sense for many kind of operations where you don't need
> > --fsys-tarfile anymore: e.g. extracting a single file can be done with:
> >
> > dpkg-deb -x file.deb some-dir ./path/to/file/to/extract
>
> I have wished for something like this before.
>
> > +++ b/dpkg-deb/extract.c
> > @@ -325,7 +329,17 @@ void extracthalf(const char *debar, const char *directory,
> >
> > unsetenv("TAR_OPTIONS");
> >
> > - execlp(TAR, "tar", buffer, "-", NULL);
> > + for (i = 0; taropts && taropts[i]; i++);
>
> Produces a warning from gcc.
>
> > + arg = targv = m_malloc((5 + i + 1) * sizeof(targv[0]));
>
> Why not (4 + i) * sizeof(targv[0])?
>
> > + *arg++ = "tar";
> > + *arg++ = buffer;
> > + *arg++ = "-";
> > + if (i) {
>
> Needed for the !taropts case. Good.
>
> > + mempcpy(arg, taropts, i * sizeof(taropts[0]));
> > + arg += i;
> > + }
> > + *arg++ = NULL;
> > + execvp(TAR, (char *const *)targv);
> > ohshite(_("failed to exec tar"));
> > }
> > close(p2[0]);
>
> I couldn’t find any other uses of mempcpy in dpkg, so it might be better
> to use memcpy() or use a wrapper like git does.
err that was a typo actually sorry, the previous version was arg =
mempcpy(...), I fixed it to avoid the mempcpy and forgot to remove the
'p'.

I agree with the rest of your mail of course.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100703233425.GB4586@madism.org">http://lists.debian.org/20100703233425.GB4586@madism.org
 
Old 07-05-2010, 09:12 AM
Pierre Habouzit
 
Default leverage -x/-X tar call to pass more tar options

On Sun, Jul 04, 2010 at 01:34:25AM +0200, Pierre Habouzit wrote:
> On Sat, Jul 03, 2010 at 06:30:58PM -0500, Jonathan Nieder wrote:
> > Hi,
> >
> > Pierre Habouzit wrote:
> >
> > > The point is that doing chained pipes is pretty annoying from C
> > > (especially if you want to use parallelism in the process), plus it kind
> > > of make sense for many kind of operations where you don't need
> > > --fsys-tarfile anymore: e.g. extracting a single file can be done with:
> > >
> > > dpkg-deb -x file.deb some-dir ./path/to/file/to/extract
> >
> > I have wished for something like this before.
> >
> > > +++ b/dpkg-deb/extract.c
> > > @@ -325,7 +329,17 @@ void extracthalf(const char *debar, const char *directory,
> > >
> > > unsetenv("TAR_OPTIONS");
> > >
> > > - execlp(TAR, "tar", buffer, "-", NULL);
> > > + for (i = 0; taropts && taropts[i]; i++);
> >
> > Produces a warning from gcc.
> >
> > > + arg = targv = m_malloc((5 + i + 1) * sizeof(targv[0]));
> >
> > Why not (4 + i) * sizeof(targv[0])?

actually I missed that question, it's because in a previous iteration of
the patch, there was another argument needed, which it doesn't anymore.

> > > + *arg++ = "tar";
> > > + *arg++ = buffer;
> > > + *arg++ = "-";
> > > + if (i) {
> >
> > Needed for the !taropts case. Good.

actually this isn't really needed anymore either (before when !taropts,
I had kept the old code, and I didn't used the malloced argv).
memcpy(dst, src, 0) will do what it should, even when src is NULL.

But it's only cosmetics of course.

> > > + mempcpy(arg, taropts, i * sizeof(taropts[0]));
> > > + arg += i;
> > > + }
> > > + *arg++ = NULL;
> > > + execvp(TAR, (char *const *)targv);
> > > ohshite(_("failed to exec tar"));
> > > }
> > > close(p2[0]);
> >
> > I couldn’t find any other uses of mempcpy in dpkg, so it might be better
> > to use memcpy() or use a wrapper like git does.
> err that was a typo actually sorry, the previous version was arg =
> mempcpy(...), I fixed it to avoid the mempcpy and forgot to remove the
> 'p'.
>
> I agree with the rest of your mail of course.
> --
> ·O· Pierre Habouzit
> ··O madcoder@debian.org
> OOO http://www.madism.org

--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100705091231.GA3645@madism.org">http://lists.debian.org/20100705091231.GA3645@madism.org
 
Old 07-05-2010, 01:47 PM
Jonathan Nieder
 
Default leverage -x/-X tar call to pass more tar options

Hi again,

Pierre Habouzit wrote:
>> On Sat, Jul 03, 2010 at 06:30:58PM -0500, Jonathan Nieder wrote:

>>> Why not (4 + i) * sizeof(targv[0])?
>
> actually I missed that question, it's because in a previous iteration of
> the patch

Thanks for the explanation. That’s a comfort.

>>> Needed for the !taropts case. Good.
>
> actually this isn't really needed anymore either (before when !taropts,
> I had kept the old code, and I didn't used the malloced argv).
> memcpy(dst, src, 0) will do what it should, even when src is NULL.

What you say makes sense, so I just checked Posix to see if there were
any gotchas. It is silent on the issue and defers to ISO C. The C
standard itself (at least C99 TC2 from
http://www.open-std.org/jtc1/sc22/wg14/www/standards.html#9899 )
stubbornly refuses to standardize[1] on the sane behavior:

Where an argument declared as size_t n specifies the
length of the array for a function, n can have the value
zero on a call to that function. Unless explicitly stated
otherwise in the description of a particular function in
this subclause, pointer arguments on such a call shall
still have valid values, as described in 7.1.4.

Sadly the description of memcpy() has nothing to add.

> But it's only cosmetics of course.

Right, it’s only theoretical. If the if is missing and people run
into problems dpkg could learn to deal with it then.

[1] Library → String handling <string.h> → String function conventions,
paragraph 2, currently numbered 7.21.1.2. Thanks to Larry Jones from
c.l.c.m, 1996 for a pointer.


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100705134745.GA10291@burratino">http://lists.debian.org/20100705134745.GA10291@burratino
 
Old 07-05-2010, 04:49 PM
Pierre Habouzit
 
Default leverage -x/-X tar call to pass more tar options

On Mon, Jul 05, 2010 at 08:47:45AM -0500, Jonathan Nieder wrote:
> Hi again,
>
> Pierre Habouzit wrote:
> >> On Sat, Jul 03, 2010 at 06:30:58PM -0500, Jonathan Nieder wrote:
>
> >>> Why not (4 + i) * sizeof(targv[0])?
> >
> > actually I missed that question, it's because in a previous iteration of
> > the patch
>
> Thanks for the explanation. That’s a comfort.
>
> >>> Needed for the !taropts case. Good.
> >
> > actually this isn't really needed anymore either (before when !taropts,
> > I had kept the old code, and I didn't used the malloced argv).
> > memcpy(dst, src, 0) will do what it should, even when src is NULL.
>
> What you say makes sense, so I just checked Posix to see if there were
> any gotchas. It is silent on the issue and defers to ISO C. The C
> standard itself (at least C99 TC2 from
> http://www.open-std.org/jtc1/sc22/wg14/www/standards.html#9899 )
> stubbornly refuses to standardize[1] on the sane behavior:
>
> Where an argument declared as size_t n specifies the
> length of the array for a function, n can have the value
> zero on a call to that function. Unless explicitly stated
> otherwise in the description of a particular function in
> this subclause, pointer arguments on such a call shall
> still have valid values, as described in 7.1.4.
>
> Sadly the description of memcpy() has nothing to add.

Yeah right, OTOH I see absolutely no-one causing a segfault is the
address insn't valid.

> > But it's only cosmetics of course.
>
> Right, it’s only theoretical. If the if is missing and people run
> into problems dpkg could learn to deal with it then.

Well I don't care if the if(i) is there or not after all, it's not even
worth the discussion
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100705164910.GH3645@madism.org">http://lists.debian.org/20100705164910.GH3645@madism.org
 
Old 07-08-2010, 12:03 PM
Guillem Jover
 
Default leverage -x/-X tar call to pass more tar options

Hi!

On Sun, 2010-07-04 at 00:20:02 +0200, Pierre Habouzit wrote:
> I'm in the process of moving apt-listchanges to C, slowly, and the
> attached patch would make it really easier for me to work with dpkg-deb.

Just OOC, why C and not C++ being it supposedly part of the apt
namespace?

Ok, I've checked now the source and I guess the answer might be,
because it does not really depend on anything apt related (except
for the invokation hook protocol to retrieve the package and version
list). If we extended the dpkg invoke hooks, apt-listchanges could
become something lower level, that could be hooked directly into dpkg.

> The point is that doing chained pipes is pretty annoying from C
> (especially if you want to use parallelism in the process), plus it kind
> of make sense for many kind of operations where you don't need
> --fsys-tarfile anymore: e.g. extracting a single file can be done with:

Well, as surely it might be slightly annoying, I don't think it implies
that much code. It's oubviously easier and more comfortable to be able
to extract specific files to extract, it's also more performant than
filtering them out after the fact. So I agree it's a nice feature to
have, but ...

> dpkg-deb -x file.deb some-dir ./path/to/file/to/extract
>
> What I do is that additionnal arguments passed after the "directory"
> argument of -x and -X are passed straight to tar.

... definitely not by allowing any option to be forwarded to tar,
which would make the fact that we are internally using the tar binary
an exported interface. And in case we wanted to switch to a pure C
implementation we'd have to either break or support parsing random tar
options!

What I can see as acceptable would be to only allow additional
optional filenames after the directory argument. But then I guess that
might not be enough, and we might need to support wildcards too. So
the tar call would end up being something like:

if $tar_files
tar xpf - --wildcards -- $tar_files
else
tar xpf -

> >From ba0dd1312e16e5a9ad266549b0caa8d6fa9186fd Mon Sep 17 00:00:00 2001
> From: Pierre Habouzit <madcoder@debian.org>
> Date: Sat, 3 Jul 2010 09:04:26 +0200
> Subject: [PATCH] dpkg-deb: --[v]extract can take tar options.
>
> Allow --extract/vextract to take additionnal arguments as arguments to the
> underlying tar call. This leverages the underlying tar invocation.
> Avoiding the need for a pipe of `dpkg-deb --fsys-tarfile` into tar(1).
>
> This eases the call of dpkg-deb from e.g. C.

> diff --git a/dpkg-deb/extract.c b/dpkg-deb/extract.c
> index e5e1f9c..914224b 100644
> --- a/dpkg-deb/extract.c
> +++ b/dpkg-deb/extract.c
> @@ -315,7 +316,10 @@ void extracthalf(const char *debar, const char *directory,
> if (taroption) {
> c3 = subproc_fork();
> if (!c3) {
> + const char **targv, **arg;
> char buffer[30+2];
> + int i;
> +
> if (strlen(taroption) > 30)
> internerr("taroption is too long '%s'", taroption);
> strcpy(buffer, taroption);
> @@ -325,7 +329,17 @@ void extracthalf(const char *debar, const char *directory,
>
> unsetenv("TAR_OPTIONS");
>
> - execlp(TAR, "tar", buffer, "-", NULL);
> + for (i = 0; taropts && taropts[i]; i++);
> + arg = targv = m_malloc((5 + i + 1) * sizeof(targv[0]));
> + *arg++ = "tar";
> + *arg++ = buffer;
> + *arg++ = "-";
> + if (i) {
> + mempcpy(arg, taropts, i * sizeof(taropts[0]));
> + arg += i;
> + }
> + *arg++ = NULL;
> + execvp(TAR, (char *const *)targv);
> ohshite(_("failed to exec tar"));
> }
> close(p2[0]);

Here you should just use the command module.

thanks,
guillem


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100708120321.GB20823@gaara.hadrons.org">http://lists.debian.org/20100708120321.GB20823@gaara.hadrons.org
 
Old 07-08-2010, 12:13 PM
Pierre Habouzit
 
Default leverage -x/-X tar call to pass more tar options

On Thu, Jul 08, 2010 at 02:03:21PM +0200, Guillem Jover wrote:
> Hi!
>
> On Sun, 2010-07-04 at 00:20:02 +0200, Pierre Habouzit wrote:
> > I'm in the process of moving apt-listchanges to C, slowly, and the
> > attached patch would make it really easier for me to work with dpkg-deb.
>
> Just OOC, why C and not C++ being it supposedly part of the apt
> namespace?
>
> Ok, I've checked now the source and I guess the answer might be,
> because it does not really depend on anything apt related (except
> for the invokation hook protocol to retrieve the package and version
> list). If we extended the dpkg invoke hooks, apt-listchanges could
> become something lower level, that could be hooked directly into dpkg.
>
> > The point is that doing chained pipes is pretty annoying from C
> > (especially if you want to use parallelism in the process), plus it kind
> > of make sense for many kind of operations where you don't need
> > --fsys-tarfile anymore: e.g. extracting a single file can be done with:
>
> Well, as surely it might be slightly annoying, I don't think it implies
> that much code. It's oubviously easier and more comfortable to be able
> to extract specific files to extract, it's also more performant than
> filtering them out after the fact. So I agree it's a nice feature to
> have, but ...
>
> > dpkg-deb -x file.deb some-dir ./path/to/file/to/extract
> >
> > What I do is that additionnal arguments passed after the "directory"
> > argument of -x and -X are passed straight to tar.
>
> .... definitely not by allowing any option to be forwarded to tar,
> which would make the fact that we are internally using the tar binary
> an exported interface. And in case we wanted to switch to a pure C
> implementation we'd have to either break or support parsing random tar
> options!
>
> What I can see as acceptable would be to only allow additional
> optional filenames after the directory argument. But then I guess that
> might not be enough, and we might need to support wildcards too. So
> the tar call would end up being something like:
>
> if $tar_files
> tar xpf - --wildcards -- $tar_files
> else
> tar xpf -

That was my first version of the patch indeed, though I found it too
restrictive at first.

I'll try to reroll it to that then
--
O Pierre Habouzit
O madcoder@debian.org
OOO http://www.madism.org


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100708121327.GD12789@madism.org">http://lists.debian.org/20100708121327.GD12789@madism.org
 

Thread Tools




All times are GMT. The time now is 02:56 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org