[LEDE-DEV] [PATCH] base-files: do not backup unchanged files

[Jonas Gorski]

On 17 November 2017 at 11:35, Matthias Schiffer
<mschiffer at universe-factory.net> wrote:
> On 11/17/2017 10:14 AM, Jonas Gorski wrote:
>> On 17 November 2017 at 01:41,  <luizluca at gmail.com> wrote:
>>> From: Luiz Angelo Daros de Luca <luizluca at gmail.com>
>>>
>>> Only backup /aaa/bbb/ccc if /rom/aaa/bbb/ccc does not exist
>>> or /aaa/bbb/ccc is different from /rom/aaa/bbb/ccc.
>>>
>>> Signed-off-by: Luiz Angelo Daros de Luca <luizluca at gmail.com>
>>> ---
>>>  package/base-files/files/sbin/sysupgrade | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/package/base-files/files/sbin/sysupgrade b/package/base-files/files/sbin/sysupgrade
>>> index 359f21f51c..0085dbe07e 100755
>>> --- a/package/base-files/files/sbin/sysupgrade
>>> +++ b/package/base-files/files/sbin/sysupgrade
>>> @@ -101,8 +101,10 @@ add_uci_conffiles() {
>>>         local file="$1"
>>>         ( find $(sed -ne '/^[[:space:]]*$/d; /^#/d; p' \
>>>                 /etc/sysupgrade.conf /lib/upgrade/keep.d/* 2>/dev/null) \
>>> -               -type f -o -type l 2>/dev/null;
>>> -         opkg list-changed-conffiles ) | sort -u > "$file"
>>> +               $(opkg list-changed-conffiles) \
>>> +               \( -type f -o -type l \) \
>>> +               \( \( -exec test -e /rom{} \; -exec cmp -s {} /rom{} \; \) -o -print \) 2>/dev/null;
>>> +       ) | sort -u > "$file"
>>
>> "opkg list-changed-conffiles" should have already filtered by that
>> (but obviously didn't), so the issue should be fixed at the source
>> instead of being worked around.
>
> `opkg list-changed-conffiles` does filter for changed files

The issue seems to be a bit more complex than at first sight.

There are at least two issues causing all /rom files treated as modified:

1) The hashing of opkg seems to be broken:

# opkg -V2 list-changed-conffiles
...
conffile_has_been_modified: Conffile /etc/ppp/options:
        old chk=851bf20b58373edaba24255bb5c8abf86288379f6f0d99c72c01b76cee56a7b7
        new chk=a0509db666fc831a3f9332ca6e911ba9a32e7f1ce13733e539a719f0413794b9
# sha256sum /etc/ppp/options
a0509db666fc831a3f9332ca6e911ba9a32e7f1ce13733e539a719f0413794b9
/etc/ppp/options
# grep "/etc/ppp/options" /usr/lib/opkg/status
 /etc/ppp/options
a0509db666fc831a3f9332ca6e911ba9a32e7f1ce13733e539a719f0413794b9

(old chk is the calculated checksum of the existing file, new chk is
the one according to status)

So while the one from status is properly read, the generated one
doesn't match the existing file at all.

2.) We *do* modify some files during offline root generation after the
base package had been installed.

E.g. we modify the /etc/passwd and /etc/groups for all users and
groups required by packages. So these being treated as modified
shouldn't come as a surprise.


The first issue is an obvious bug, and needs to be fixed in opkg.

The second issue is a bit more complicated.

One option to fix would be to regenerate the checksums at the end of
the rootfs-creation, one option would be to the manual check like this
patch does.

The question for implications is if there are parts that rely on files
being implicitly backed up, and can there be issues when only doing
partial backups. On a first glance it seems that adding a user to an
existing group will still modify the /etc/groups, so /etc/passwd and
/etc/users will always be backed up at the same time, thus group ids
cannot go out of sync (in case they are dynamically allocated).

> - but the files
> listed in /etc/sysupgrade.conf or /lib/upgrade/keep.d are backed up
> unconditionally. I'm not sure which behaviour the average admin would
> expect - but I'm not happy about the different handling of config files
> managed by opkg and the other file lists.
>
> I don't like adding overlay-specific logic here, sysupgrade should work
> "the same" on all targets, with or without overlay. Maybe we can rather
> filter out unchanged files *after* the sysupgrade (after a major config
> backup rework I guess - but I heard people are thinking about a generic
> tar-based backup approach for sysupgrades).

This is IMHO a bit of a philosophical question - if one specifies
additional files to backup, does this mean we should back them up in
any case, or should we second guess the user and check if they were
present before and were modified since then? Maybe we need a new flag
for that (meaning "full config backup" or "modified-only").

I have to admit, I always assumed that if I tell sysupgrade to backup
the config, it backs up the full config, and not only the modified
parts. But that might be only me ;-)

Although, the help says:

        -b | --create-backup <file>
                     create .tar.gz of files specified in sysupgrade.conf
                     then exit. Does not flash an image. If file is '-',
                     i.e. stdout, verbosity is set to 0 (i.e. quiet).

and the sysupgrade.conf says:

  ## This file contains files and directories that should
  ## be preserved during an upgrade.

Which sort of implies to me that it creates a full backup - it doesn't
mention anywhere that it will skip unmodified files. Not that I'm
against doing the diff-version, just that the texts should be updated
as well in that case.


Regards
Jonas