Date: Mon, 12 Apr 2010 20:56:11 -0700 From: Tim Kientzle <kientzle@freebsd.org> To: Garrett Cooper <gcooper@freebsd.org> Cc: Perforce Change Reviews <perforce@freebsd.org> Subject: Re: PERFORCE change 176831 for review Message-ID: <4BC3EB5B.5070801@freebsd.org> In-Reply-To: <201004121230.o3CCUsIX029146@repoman.freebsd.org> References: <201004121230.o3CCUsIX029146@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Garrett Cooper wrote:
> +FILE*
> +unpack_file_to_fd(const char *pkg, const char *file)
> +{
....
> + r = archive_read_extract(archive, archive_entry,
> + EXTRACT_ARCHIVE_FLAGS);
> + if (r == ARCHIVE_OK) {
> + if (Verbose)
> + printf("X - %s\n",
> + entry_pathname);
> + fd = fopen(entry_pathname, "r");
This is potentially race-prone. I would suggest instead that
you:
int fd = open(entry_pathname, O_RDWR | O_CREAT, ...)
archive_read_data_into_fd(a, fd);
Then rewind the fd and return it. archive_read_extract()
is handy but it's way overkill for this kind of simple
operation. I would also put in a simple verification
that the entry you found is a regular entry:
archive_entry_filetype(entry) == AE_IFREG
> + * NOTE: the exit code is 0 / 1 so that this can be fed directly into exit
> + * when doing piped tar commands for copying hierarchies *hint*, *hint*.
Why do you want to copy hierarchies?
Seems a waste of disk bandwidth. *hint* *hint* ;-)
> int
> unpack(const char *pkg, const char *file_expr)
> {
Since this is only called with file_expr=="*" and file_expr=="+*",
I don't think you need fnmatch(). You can get away with
a simple
int unpack(const char *pkg, int metadata_only)
and then use
if (!metadata_only || entry_pathname[0] == '+')
> + (fnmatch(file_expr, entry_pathname,
> + FNM_PATHNAME)) == 0) {
>
You might also consider passing in an fd to unpack and
using archive_read_open_fd() to attach an archive
handle to it instead of archive_read_open_filename().
(It doesn't make any performance difference, but it
does avoid races and opens the door to other interesting
games in the future.)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4BC3EB5B.5070801>