[Buildroot] [PATCH 1/1] package/swupdate: update service/init configs

Sun Aug 29 14:58:51 UTC 2021

On 29/08/2021 03:54, James Hilliard wrote:
> On Sat, Aug 28, 2021 at 6:44 AM Arnout Vandecappelle <arnout at mind.be> wrote:
>>
>>  Hi James,
>>
>>  "update" in the subject is not really helpful.
> 
> I had assumed these configs were mostly just re-syncing with meta-swupdate.
> 
> I'll split this patch up.
> 
>>
>> On 28/08/2021 06:46, James Hilliard wrote:
>>> Set SKIP_STRIP=y so that the swupdate binary isn't stripped.
>>
>>  We can see that from the patch - instead you should explain why we want to skip
>> strip.
>>
>>  Also, this has nothing to do with systemd configs, so should be a separate patch.
>>
>>  In this case for sure, because I think it might be something for master.
> 
> Ok, will send separately.

 Splitting off just this bit is probably sufficient - I don't want to push
useless work on you.

[snip]
>>> diff --git a/package/swupdate/10-mongoose-args b/package/swupdate/10-mongoose-args
>>> new file mode 100644
>>> index 0000000000..896e66ba86
>>> --- /dev/null
>>> +++ b/package/swupdate/10-mongoose-args
>>> @@ -0,0 +1 @@
>>> +SWUPDATE_WEBSERVER_ARGS="-r /www ${SWUPDATE_MONGOOSE_EXTRA_ARGS:--p 8080}"
>>
>>  Is this correct? The webserver is installed in /var/www, not /www...

 You didn't reply to this...

>>
>>> diff --git a/package/swupdate/90-start-progress b/package/swupdate/90-start-progress
>>> new file mode 100644
>>> index 0000000000..3b0eb5e7d7
>>> --- /dev/null
>>> +++ b/package/swupdate/90-start-progress
>>> @@ -0,0 +1 @@
>>> +exec /usr/bin/swupdate-progress -w -r &
>>> diff --git a/package/swupdate/Config.in b/package/swupdate/Config.in
>>> index f601e45fcd..6b832cfcbf 100644
>>> --- a/package/swupdate/Config.in
>>> +++ b/package/swupdate/Config.in
>>> @@ -60,9 +60,16 @@ config BR2_PACKAGE_SWUPDATE_CONFIG
>>>         I you wish to use your own modified swupdate configuration
>>>         file specify the config file location with this option.
>>>
>>> +config BR2_PACKAGE_SWUPDATE_WEBSERVER
>>> +     bool "swupdate webserver"
>>> +     default y
>>> +     help
>>> +       Enable update from remote using a web server on the target.
>>> +
>>>  config BR2_PACKAGE_SWUPDATE_INSTALL_WEBSITE
>>>       bool "install default website"
>>>       default y
>>> +     depends on BR2_PACKAGE_SWUPDATE_WEBSERVER
>>
>>  I could be mistaken, but isn't the website perfectly usable with an external
>> webserver as well? You just have to provide the upload endpoint IIRC.
> 
> The webserver is also the upload endpoint and is thus required, sure you can
> proxy to it via nginx even but it's not optional for the update
> website to function.

 So I was indeed mistaken :-)

 I thought I made a comment somewhere that maybe the
BR2_PACKAGE_SWUPDATE_WEBSERVER isn't needed (but now I can't find that comment
so maybe in the end I removed it). In case I did make that comment: ignore it
:-) The explanation above makes it clear that it really *is* needed as a config
option.

> 
>>
>>>       help
>>>         Install the provided website to /var/www/swupdate.
>>>
>>> diff --git a/package/swupdate/swupdate-progress.service b/package/swupdate/swupdate-progress.service
>>> new file mode 100644
>>> index 0000000000..0d464bb420
>>> --- /dev/null
>>> +++ b/package/swupdate/swupdate-progress.service
>>> @@ -0,0 +1,9 @@
>>> +[Unit]
>>> +Description=swupdate progress service
>>> +After=swupdate.service
>>> +
>>> +[Service]
>>> +ExecStart=/usr/bin/swupdate-progress -r -w
>>> +
>>> +[Install]
>>> +WantedBy=swupdate.service
>>
>>  I suspect this is copied verbatim from meta-swupdate... It's good form to then
>> add a comment in the beginning of the file that specifies the upstream copyright
>> and license (in SPDX) and an upstream link.
>>
>>  Same for all other files you copied, of course.
> 
> Hmm, I'm thinking the best option would just be to add a comment to the makefile
> indicating the source of them, they are MIT licensed which is already a license
> listed for the swupdate package.

 An SPDX-License-Identifier comment costs nothing and can be very helpful for
people who have to analyse licenses. We haven't this discussed it at all so it's
purely my own opinion, but I believe we should require that on all files.

>>> diff --git a/package/swupdate/swupdate-usb.rules b/package/swupdate/swupdate-usb.rules
>>> new file mode 100644
>>> index 0000000000..b4efd0b733
>>> --- /dev/null
>>> +++ b/package/swupdate/swupdate-usb.rules
>>> @@ -0,0 +1,2 @@
>>> +ACTION=="add", KERNEL=="sd*", SUBSYSTEM=="block", ENV{ID_BUS}=="usb", ENV{ID_FS_USAGE}=="filesystem", TAG+="systemd", ENV{SYSTEMD_WANTS}+="swupdate-usb@%k.service"
>>
>>  Hm, I wonder if it's a good idea to install this unconditionally... It feels to
>> me that this is a possible attack vector that the user should be aware of - and
>> the best way to do that is to make the whole thing optional.
> 
> Maybe, although swu files should typically be signed.

 With the default installation, there is no signature check - you need to change
the swupdate.conf and install a public key for it to work. So, if you're going
to that trouble, it's no effort at all to also install the udev rules for
automounting USB. That is, of course, assuming that it's documented - which
currently it is not. Which is why I made the suggestion to instead contribute it
as upstream documentation.

>>  But really, I kind of wonder if this is something that Buildroot should
>> provide. Ideally, this should be part of the swupdate documentation, and if you
>> actually want automatic updates when a USB storage device is plugged in, you can
>> put that stuff in an overlay.
> 
> Well it's included by default in the meta-swupdate layer, but yeah I
> should probably
> add a config option for it.
[snip]
>>> diff --git a/package/swupdate/tmpfiles-swupdate.conf b/package/swupdate/tmpfiles-swupdate.conf
>>> new file mode 100644
>>> index 0000000000..4743672fcc
>>> --- /dev/null
>>> +++ b/package/swupdate/tmpfiles-swupdate.conf
>>> @@ -0,0 +1,2 @@
>>> +X /tmp/datadst
>>> +X /tmp/scripts
>>
>>  Why are these needed? Are they only needed for systemd? If not, the S80swupdate
>> init script should also be updated to create those directories.
> 
> These directives prevent systemd from deleting those folders, they are
> not needed
> for non-systemd setups.
> 
> See:
> https://www.freedesktop.org/software/systemd/man/tmpfiles.d.html
> 
> Specifically:
> X
> Ignore a path during cleaning. Use this type to exclude paths from clean-up as
> controlled with the Age parameter. Unlike x, this parameter will not exclude the
> content if path is a directory, but only directory itself. Note that
> lines of this type do
> not influence the effect of r or R lines. Lines of this type accept
> shell-style globs in
> place of normal path names.

 Thanks - TIL.

 Regards,
 Arnout