[pacman-dev] [PATCHv2] extract_single_file: consolidate symlink cases

[Allan McRae]

On 11/05/13 12:21, Andrew Gregory wrote:
> Signed-off-by: Andrew Gregory <andrew.gregory.8 at gmail.com>
> ---
> 
> Note the newly modified test at the bottom.
> 
>  lib/libalpm/add.c               | 56 +++++++++++++----------------------------
>  test/pacman/tests/upgrade045.py |  2 +-
>  2 files changed, 18 insertions(+), 40 deletions(-)
> 
> diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c
> index 3ef81e3..49f36ce 100644
> --- a/lib/libalpm/add.c
> +++ b/lib/libalpm/add.c
> @@ -197,30 +197,25 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive,
>  	 * to get 'right'. Here are the possibilities, with the filesystem
>  	 * on the left and the package on the top:
>  	 * (F=file, N=node, S=symlink, D=dir)
> -	 *               |  F/N  |  S  |  D
> -	 *  non-existent |   1   |  2  |  3
> -	 *  F/N          |   4   |  5  |  6
> -	 *  S            |   7   |  8  |  9
> -	 *  D            |   10  |  11 |  12
> +	 *               |  F/N  |   D
> +	 *  non-existent |   1   |   2
> +	 *  F/N          |   3   |   4
> +	 *  D            |   5   |   6
>  	 *
> -	 *  1,2,3- extract, no magic necessary. lstat (_alpm_lstat) will fail here.
> -	 *  4,5,6,7,8- conflict checks should have caught this. either overwrite
> +	 *  1,2- extract, no magic necessary. lstat (_alpm_lstat) will fail here.
> +	 *  3,4- conflict checks should have caught this. either overwrite
>  	 *      or backup the file.
> -	 *  9- follow the symlink, hopefully it is a directory, check it.
> -	 *  10- file replacing directory- don't allow it.
> -	 *  11- don't extract symlink- a dir exists here. we don't want links to
> -	 *      links, etc.
> -	 *  12- skip extraction, dir already exists.
> +	 *  5- file replacing directory- don't allow it.
> +	 *  6- skip extraction, dir already exists.
>  	 */
>  
> -	/* do both a lstat and a stat, so we can see what symlinks point to */
> -	struct stat lsbuf, sbuf;
> -	if(_alpm_lstat(filename, &lsbuf) != 0 || stat(filename, &sbuf) != 0) {
> -		/* cases 1,2,3: couldn't stat an existing file, skip all backup checks */
> +	struct stat lsbuf;
> +	if(_alpm_lstat(filename, &lsbuf) != 0) {
> +		/* cases 1,2: file doesn't exist, skip all backup checks */
>  	} else {
>  		if(S_ISDIR(lsbuf.st_mode)) {
>  			if(S_ISDIR(entrymode)) {
> -				/* case 12: existing dir, ignore it */
> +				/* case 6: existing dir, ignore it */
>  				if(lsbuf.st_mode != entrymode) {
>  					/* if filesystem perms are different than pkg perms, warn user */
>  					mode_t mask = 07777;
> @@ -237,33 +232,18 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive,
>  				archive_read_data_skip(archive);
>  				return 0;
>  			} else {
> -				/* case 10/11: trying to overwrite dir with file/symlink, don't allow it */
> +				/* case 5: trying to overwrite dir with file, don't allow it */
>  				_alpm_log(handle, ALPM_LOG_ERROR, _("extract: not overwriting dir with file %s\n"),
>  						filename);
>  				archive_read_data_skip(archive);
>  				return 1;
>  			}
> -		} else if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(entrymode)) {
> -			/* case 9: existing symlink, dir in package */
> -			if(S_ISDIR(sbuf.st_mode)) {
> -				/* the symlink on FS is to a directory, so we'll use it */
> -				_alpm_log(handle, ALPM_LOG_DEBUG, "extract: skipping symlink overwrite of %s\n",
> -						filename);
> -				archive_read_data_skip(archive);
> -				return 0;
> -			} else {
> -				/* this is BAD. symlink was not to a directory */
> -				_alpm_log(handle, ALPM_LOG_ERROR, _("extract: symlink %s does not point to dir\n"),
> -						filename);
> -				archive_read_data_skip(archive);
> -				return 1;
> -			}
> -		} else if(S_ISREG(lsbuf.st_mode) && S_ISDIR(entrymode)) {
> -			/* case 6: trying to overwrite file with dir */
> +		} else if(S_ISDIR(entrymode)) {
> +			/* case 4: trying to overwrite file with dir */
>  			_alpm_log(handle, ALPM_LOG_DEBUG, "extract: overwriting file with dir %s\n",
>  					filename);
> -		} else if(S_ISREG(entrymode)) {
> -			/* case 4,7: */
> +		} else {
> +			/* case 3: */
>  			/* if file is in NoUpgrade, don't touch it */
>  			if(alpm_list_find(handle->noupgrade, entryname, _alpm_fnmatch)) {
>  				notouch = 1;
> @@ -288,8 +268,6 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive,
>  				}
>  			}
>  		}
> -		/* else if(S_ISLNK(entrymode)) */
> -		/* case 5,8: don't need to do anything special */
>  	}
>  
>  	/* we need access to the original entryname later after calls to
> diff --git a/test/pacman/tests/upgrade045.py b/test/pacman/tests/upgrade045.py
> index 3b7f476..e165ba3 100644
> --- a/test/pacman/tests/upgrade045.py
> +++ b/test/pacman/tests/upgrade045.py
> @@ -14,4 +14,4 @@
>  
>  self.addrule("PACMAN_RETCODE=0")
>  self.addrule("PKG_VERSION=foo|1.0-2")
> -self.addrule("FILE_EXIST=etc/foo.cfg")
> +self.addrule("LINK_EXIST=etc/foo.cfg")
> 

Looks good.  For that pactest, add a FILE_EXISTS=/etc/foo.cfg.pacnew too.

Also, in that pactest:

p1.files = ["etc/foo.cfg*"]

what is the "*" for there?