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