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

Sun Aug 29 15:08:52 UTC 2021

On Sun, Aug 29, 2021 at 8:58 AM Arnout Vandecappelle <arnout at mind.be> wrote:
>
>
>
> 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.

Well I split it up a bit more:
https://patchwork.ozlabs.org/project/buildroot/list/?series=260043

>
> [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...

Oh, it's fixed in my v2.

>
> >>
> >>> 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.

Yeah, I didn't see another way to make the website install depend on it.

>
> >
> >>
> >>>       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.

I think a number of these config files don't even have a good spot for
that, but I did add a note in the makefile regarding the license for them.

>
>
> >>> 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.

Yeah, I left the udev rule out of my v2 for now.

>
>
> >>  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
>