Hi!
On Sun, 2020-04-19 at 07:34:18 +0200, Helmut Grohne wrote:
> you asked me to review your next/d-m-h-root branch.
Thanks for the review!
> This is what I found:
>
> * Diagnostic messages tend to include DPKG_ROOT. Is that a useful thing
> to do?
I think not including DPKG_ROOT would be too weird and confusing, as
then the user might end up acting on the host filesystem instead of
the chrooted one after some diagnostic error.
> My intuition was that I'd not include DPKG_ROOT there.
>
> For example:
>
> echo "Obsolete conffile ${DPKG_ROOT}$CONFFILE has been modified by you."
>
> Counter examples:
>
> echo "Restoring backup of $SYMLINK ..."
> echo "Restoring backup of $PATHNAME ..."
>
> At the very least, consistency seems in order here.
Right, fixed this now locally.
> * In dir_to_symlink, in the case for postrm, in the second (long)
> condition, I think there is a line that misses a trailing backslash.
>
> [ \( ! -h "${DPKG_ROOT}$PATHNAME" -a
Done.
> * In readlink_canon, in the final realpath the $dst argument likely
> should be quoted.
Done.
> * In readlink_canon I was wondering whether passing DPKG_ROOT inside a
> sed expression is a good idea. How about reworking the final line:
>
> -realpath $dst | sed -e "s:^$DPKG_ROOT::"
> +dst=$(realpath $dst)
> +echo ${dst#"$DPKG_ROOT"}
>
> The quoting of the variable removes any pattern meaning from its
> contents.
Done, much better this way indeed.
> * I observe that readlink -f and readlink_canon significantly differ in
> their behaviour. While readlink -f canonicalizes symbolic links in
> every component of the path, readlink_canon only canonicalizes
> symbolic links in the final component. It then uses realpath saying
> that no symlinks are involved, which may be untrue.
>
> I do wonder whether implementing this feature here is a useful thing
> to do. It seems that we need a chroot-aware readlink and/or realpath
> in more occasions. Possibly extending coreutils is a better solution?
Yes, I think also checked around at the time whether there was a
"chroot"-aware canonicalizer around that could simply be used but
found nothing. ISTR pondering about requesting this, but it seems I
didn't.
I'm also thinking that because we might indeed need it elswhere while
adding DPKG_ROOT support, perhaps a better idea for now is to provide a
very simple dpkg-readlink command or similar which would take DPKG_ROOT
into account so that other packages can use it.
> In any case, I'm attaching a version of readlink_canon that deals
> with this.
Thanks! I notice this is susceptible to directory traversals. I've
amended it and added comments in the attached version. I'm thinking
I'll need to add unit tests to cover for this among other similar
issues.
> * I observe that the coding style is inconsistent about using test
> expressions using -a/-o and &&/||. There is a slight reason to prefer
> the latter, see:
>
> https://github.com/koalaman/shellcheck/wiki/SC2166
Yeah, while several could be easily switched (I'll do that in a
different commit), some I don't think can easily be done, as they are
within parentheticals within test ([]). If this was bash we could use
[[ ]] f.ex. If you have an alternative I'd be happy to use that though.
> That's all. Looks like we can go ahead with this with minor changes.
Great, thanks for this!
Regards,
Guillem
Attachment:
readlink_f.sh
Description: Bourne shell script