[pacman-dev] [PATCH] Readd some symlink checking in file conflict code

[Xavier]

On Wed, Jul 23, 2008 at 4:14 AM, Dan McGee <dan at archlinux.org> wrote:
> This patch makes both fileconflict003 and fileconflict004 pactests
> successfully pass. It is a rehash of the check was removed in commit
> 584ffa6aef13d0933ad4930ab9cb70d3af2977ff, but done slightly differently. I'm
> still not very fond of this code, and it would be much better if we knew
> more about the filelist than a listing of paths- we have no real concept of
> whether a to-be-installed file is a symlink, etc.
>
> Issue:
> http://archlinux.org/pipermail/pacman-dev/2008-July/012465.html
>
> Signed-off-by: Dan McGee <dan at archlinux.org>
> ---
>  lib/libalpm/conflict.c |   30 ++++++++++++++++++++++--------
>  1 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c
> index a8bcdd5..f0e1116 100644
> --- a/lib/libalpm/conflict.c
> +++ b/lib/libalpm/conflict.c
> @@ -434,14 +434,28 @@ alpm_list_t *_alpm_db_find_fileconflicts(pmdb_t *db, pmtrans_t *trans, char *roo
>                        }
>                        stat(path, &sbuf);
>
> -                       if(path[strlen(path)-1] == '/') {
> -                               if(S_ISDIR(lsbuf.st_mode)) {
> -                                       _alpm_log(PM_LOG_DEBUG, "%s is a directory, not a conflict\n", path);
> -                                       skip_conflict = 1;
> -                               } else if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(sbuf.st_mode)) {
> -                                       _alpm_log(PM_LOG_DEBUG,
> -                                                       "%s is a symlink to a dir, hopefully not a conflict\n", path);
> -                                       skip_conflict = 1;
> +                       if(path[strlen(path)-1] == '/' && S_ISDIR(lsbuf.st_mode)) {
> +                               _alpm_log(PM_LOG_DEBUG, "%s is a directory, not a conflict\n", path);
> +                               skip_conflict = 1;
> +                       } else if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(sbuf.st_mode)) {
> +                               _alpm_log(PM_LOG_DEBUG,
> +                                               "%s is a symlink to a dir, hopefully not a conflict\n", path);
> +                               skip_conflict = 1;
> +                       } else if(S_ISDIR(sbuf.st_mode)) {
> +                               if(dbpkg) {
> +                                       /* Make sure the possible conflict is not a symlink that points
> +                                        * to a path in the old package. */
> +                                       struct stat pkgbuf;
> +                                       char str[PATH_MAX+1];
> +                                       for(k = alpm_pkg_get_files(dbpkg); k; k = k->next) {
> +                                               snprintf(str, PATH_MAX, "%s%s", root, (char*)k->data);
> +                                               if(!_alpm_lstat(str, &pkgbuf) && lsbuf.st_ino == pkgbuf.st_ino) {
> +                                                       skip_conflict = 1;
> +                                                       _alpm_log(PM_LOG_DEBUG,
> +                                                                       "%s is a symlink to a dir in previous package\n", path);
> +                                                       break;
> +                                               }
> +                                       }
>                                }
>                        }
>                        if(!skip_conflict) {
> --
> 1.5.6.4
>
>

Well, I am unable to understand that code, but maybe Nagy did, since
he was able to provide a (fileconflict005 ?) pactest which fails after
this patch :
http://archlinux.org/pipermail/pacman-dev/2008-July/012522.html
That pactest also triggers this "%s is a symlink to a dir in previous
package\n" message which looks totally bogus.

"/path/to/pacman/root/test is a symlink to a dir in previous package"

I don't see how this reflects the situation of "fileconflict005" at all.
In the new package, test is a symlink to a new directory test2. and
test2 did not exist in any previous package.
However, test existed in previous packages, as a directory.

Anyway, here are the different ways I propose to deal with this issue :
1) choose that fileconflict003 is more important than 004, and only
supports that situation (current 3.2 code)
2) choose that 004 is more important than 003, so slightly edit the
code to get back to this old behavior (of 3.0?)
3) we decide that we need to support both, in that case we go the way
that Nagy suggested in the beginning :
http://archlinux.org/pipermail/pacman-dev/2008-July/012470.html

In the first 2 cases, the code will remain (relatively) simple and
understandable.
In the third one, it will get more complex, but it will still be
logical and handle all the cases correctly.