[pacman-dev] difficult symlink "puzzle"

[Andrew Fyfe]

Xavier wrote:
<snip>
>> When extracting a symlink:
>> - If a directory already exists on the filesystem then the symlink is
>>   not extracted.
>>
> 
> That should be used without the NO_OVERWRITE flag ?
Oops I knew I forgot something, this patch should remove the need for 
NO_OVERWRITE.

> As I said in the bug report at the beginning, I was hesitating
> between doing the checks manually, or trying to configure libarchive
> like we want.
> I guess the advantage of the first way is that it gives us more control.
This is more a pacman problem than a libarchive problem.

> I'm just realizing there are many odd cases I didn't consider, eg
> extracting a directory when a file already exists on the filesystem.
> Using only NO_OVERWRITE, the directory will never be extracted,
> while with the behavior above, it would be extracted.
I forgot about this as well, I need to go do a little more testing :)

<snip>
>> +				if(stat(filename, &buf) == 0) {
>> +					if ( S_ISDIR(entrytype) && (S_ISDIR(buf.st_mode) || S_ISLNK(buf.st_mode)) ) {
>> +						/* If the package entry is a directory and there's a directory or
>> +						 * symlink with the same name on the filesystem, skip this entry. */
>> +						_alpm_log(PM_LOG_DEBUG, _("%s (directory) won't be extracted over existing directory or symlink"), entryname);
>> +						archive_read_data_skip(archive);
>> +						continue;
>>  					}
> 
> Is S_ISLNK ever true when using stat? I think you want to use lstat there, if
> you want to check for symlinks :)
Ahhh I didn't know that, I'll give lstat a try.

> Also, we could check for permission differences there, as suggested in the
> BR.
Permissions are the next problem I need to tackle.

> There is also still the problem with the tmp/ directory, which is first created by
> pacman with the wrong permissions, and which will keep these wrong
> permissions if we don't update it there when installing the filesystem package.
> But that problem could be fixed at the places where pacman creates tmp/
This is a pacman problem, it should set the correct permissions when 
creating tmp.

<snip>
>> +
>> +					if ( (S_ISFIFO(entrytype) || S_ISBLK(entrytype) || S_ISCHR(entrytype)) &&
>> +						(S_ISFIFO(buf.st_mode) || S_ISBLK(buf.st_mode) || S_ISCHR(buf.st_mode)) ) {
>> +						/* If the package entry is a FIFO, block device or character device
>> +						 * and the filesystem entry is a FIFO, block devicce or character device
>> +						 * unlink the fs entry and extract the package entry. */
>> +						_alpm_log(PM_LOG_DEBUG, _("%s is a FIFO/block/character device, extracting as normal."), entryname);
>> +					}
> 
> As I said above, won't pacman find a conflict in this case ?
> Maybe when using the --force flag then, that condition could be true.
Yes this probably is caught by the conflict check, it's mainly to avoid 
doing pointless backup checks on special files. But it still does the 
backup checks if a special file is replacing a regular file.

> And does the code below apply to every cases except the one above ?
Yes, This could probably still be refined a little.

>> +					else {
>> +						/* it does, is it a backup=() file?
>> +						 * always check the newpkg first, so when we do add a backup=() file,
>> +						 * we don't have to wait a full upgrade cycle */
>> +						needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname);
>> +
>> +						if(is_upgrade) {
>> +							hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg));
>> +							if(hash_orig) {
>> +								needbackup = 1;
>> +							}
>> +						}
>> +
>> +						/* this is kind of gross.  if we force hash_orig to be non-NULL we can
>> +						 * catch the pro-active backup=() case (when the backup entry is in
>> +						 * the new package, and not the old */
>> +						if(needbackup && !hash_orig) {
>> +							hash_orig = strdup("");
>> +						}
>> +
>> +						/* NoUpgrade skips all this backup stuff, because it's just never
>> +						 * touched */
>> +						if(alpm_list_find_str(handle->noupgrade, entryname)) {
>> +							notouch = 1;
>> +							needbackup = 0;
>> +						}
>>  					}
>>  				}
>>  
> 
> _______________________________________________
> pacman-dev mailing list
> pacman-dev at archlinux.org
> http://archlinux.org/mailman/listinfo/pacman-dev