On Thu, Apr 20, 2017 at 10:00 AM, Guillem Jover <guil...@debian.org> wrote:
> Control: forcemerge 813454 -1
>
> Hi!
>
> On Thu, 2017-04-13 at 12:02:07 +0200, Bastien ROUCARIÈS wrote:
>> Package: dpkg
>> Version: 1.18.23
>> Severity: important
>> affects: piuparts.debian.org
>> affects: src:imagemagick
>> control: tags -1 + patch
>
>> I know it is late on release but it will really help to add this patch.
>>
>> Could you consider for release ?
>
> When you first filed this, I took a quick look, but producing a proper
> fix seemed involved, given the contstrains I set for myself:
>
> Thanks for the patch, although I think this is not enough.
>
>> diff --git a/scripts/dpkg-maintscript-helper.sh 
>> b/scripts/dpkg-maintscript-helper.sh
>> index b4b3ac1b3..0b867d805 100755
>> --- a/scripts/dpkg-maintscript-helper.sh
>> +++ b/scripts/dpkg-maintscript-helper.sh
>> @@ -412,14 +412,8 @@ prepare_dir_to_symlink()
>>
>>       # If there are locally created files or files owned by another package
>>       # we should not perform the switch.
>> -     find "$PATHNAME" -print0 | xargs -0 -n1 sh -c '
>> -             package="$1"
>> -             file="$2"
>> -             if ! dpkg-query -L "$package" | grep -F -q -x "$file"; then
>> -                     exit 1
>> -             fi
>> -             exit 0
>> -     ' check-files-ownership "$PACKAGE" || \
>> +     find "$PATHNAME" -print0 | xargs -0 -n1 \
>> +             dpkg-maintscript-helper package_owns_file_or_error $PACKAGE || 
>> \
>
> The problem with this is that it aborts on first error, so any other
> remaining problematic files are missed, which might make debugging
> difficult, miss files, or require several iterations.

Thanks for this remark but it I think you are wrong: xargs will
execute next command if error code is < 125 (and it is POSIX and
portable).
Try for instance
echo '/ /tmp' | xargs -n1 chmod +x
So this implementation will print the name of all not owned files.

>
> I also considered using a command like this, but it seemed wrong, as
> that is exposing a private implementation detail as part of the
> interface, so I'd rather not do this.
>
>>               error "directory '$PATHNAME' contains files not owned by" \
>>                     "package $PACKAGE, cannot switch to symlink"
>>
>> @@ -515,6 +509,18 @@ ensure_package_owns_file() {
>>       return 0
>>  }

Does adding a private suffix and passing a magic number as arg1 will
solve this ?

I means:

MAGICK_PRIVATE="This is private interface do not use"
package_owns_file_or_error() {
    local MAGICK="$1"
    local PACKAGE="$2"
    local FILE="$3"
    if test x$MAGICKL != x$MAGICK_PRIVATE; then
        error "$MAGICK_PRIVATE";
    fi
    if ! ensure_package_owns_file $PACKAGE $FILE ; then
        error "File '$FILE' not owned by package " \
             "'$PACKAGE'"
        return 1
....

> error() already «exits 1». :)

Ok
>> +       fi
>> +       return 0

}
>> +
>> +package_owns_file_or_error() {
>> +       local PACKAGE="$1"
>> +       local FILE="$2"
>> +       if ! ensure_package_owns_file $PACKAGE $FILE ; then
>> +        error "File '$FILE' not owned by package " \
>> +              "'$PACKAGE'"
>> +        return 1
>
> error() already «exits 1». :)

Yes but for clarification it is better.

>> +       fi
>> +       return 0
>> +}

Thanks for the review

Bastien

> Thanks,
> Guillem

Reply via email to