PERFORCE change 176831 for review
Tim Kientzle
kientzle at freebsd.org
Tue Apr 13 04:36:13 UTC 2010
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.)
More information about the p4-projects
mailing list