Hi Helge, Sorry for not reviewing this patch earlier.
On Wed, 2023-01-25 at 06:38 +0100, Helmut Grohne wrote:
> Hi Helge,
>
> On Tue, Jan 24, 2023 at 10:30:37PM +0100, Helge Deller wrote:
> > On 1/24/23 06:27, Helmut Grohne wrote:
> > > On Mon, Jan 23, 2023 at 10:48:27PM +0100, Helge Deller wrote:
> > > > --- ./init.org 2023-01-23 21:40:33.079738389 +0000
> > > > +++ ./init 2023-01-23 21:40:45.983861851 +0000
> > > > @@ -205,6 +205,15 @@ else
> > > > resume=${RESUME:-}
> > > > fi
> > > >
> > > > +if [ -z "${RUNSIZE}" ] || [[ "${RUNSIZE}" \< "20" ]]; then
This test is extremely dodgy. RUNSIZE could have been specified as a
percentage, a raw number, or a number with a letter suffix ('k', 'M',
'G', etc.).
> > >
> > > This is as bashism and init runs with dash as far as I can see.
> >
> > Hmm... I did tested it, at it seemed to work...
> > Which part of that line exactly do you think is problematic?
> > I'm open for any other idea how to code it.
>
> The lexicographic comparison is outside the realm of POSIX shell, but to
> my surprise this actually is supported by dash. So fixing this would be
> academic.
>
> > Both will work, because I assume that on such systems you probably have
> > more than 200MB RAM
> > and thus my patch won't touch the user-provided value at all.
>
> Fair enough.
>
> > > > + read MemTotal mem_kb rest < /proc/meminfo
> > > > + # systemd requires at minumum 16MB for /run, so reserve
> > > > + # 20MB for machines which have less than 200MB RAM
This is being very sloppy with units. systemd wants 16 MiB (16384 kiB)
and this bumps to a minimum of 20000 kiB (which is neither 20 MiB nor 20
MB).
> > > > + if [ "$mem_kb" -lt "200000" ]; then
> > > > + RUNSIZE=20M # for machines <= 200MB RAM
Suppose the system has 192 MiB RAM and initramfs.conf (or the kernel
command line) sets RUNSIZE=15%. That should result in a maximum size of
29491 kiB but this *reduces* it to 20480 kiB.
> else
> : "${RUNSIZE:=10%}"
>
> > >
> > > Given that you initialize a default here, I think it would make the code
> > > more obvious if you pulled the 10% default 4 lines later into an else
> > > branch.
> >
> > Not sure I understand this...?
> >
> > > > + fi
> > > > +fi
> > > > +
> > > > mount -t tmpfs -o
> > > > "nodev,noexec,nosuid,size=${RUNSIZE:-10%},mode=0755" tmpfs /run
>
> This is the other line that contains a default. I suggested moving this
> default up to make it more obvious, but this is really only a cosmetic
> improvement.
>
> As such LGTM, but I am not an initramfs maintainer.
I think that a correct fix for this would add a lot of complication to
handle what is now an extremely rare case. And the alternative
configuration change on those rare systems is not hard to do (though
users might not know which configuration file to edit).
So I'm inclined to mark this wontfix. I could be persuaded otherwise by
a patch that's simple and correct, but I don't think that's possible.
Ben.
--
Ben Hutchings
Any smoothly functioning technology is indistinguishable
from a rigged demo.
signature.asc
Description: This is a digitally signed message part
