Subject: Re: [PATCH 4/4] services/qemu-binfmt: Use the F flag and the static output of QEMU.

From: Maxim Cournoyer <[email-masked]>
Date: Thu, 25 Feb 2021 16:47:55 -0500

Hello again,

Ludovic Courtès <[email-masked]> writes:

[...]

>>  ;; Platforms that QEMU can emulate.
>> -(define-record-type <qemu-platform>
>> -  (qemu-platform name family magic mask)
>> +(define-record-type* <qemu-platform>
>
> Since this is for internal consumption, I’m in favor of keeping plain
> ‘define-record-type’.  Also, I don’t think the “F” flag belongs here,
> it’s mostly orthogonal.

Even though it's still internal for now, the intent here was to allow
the advanced users (those who would use (@@ ...) to access what they
need) to configure the qemu-platform objects should they have special
needs for it.

I also don't think the flags belong to a qemu-platform object, but the
same can be said for the magic field.  The most elegant thing would be
to have a binfmt_misc service which we would then extend for QEMU, but
that's more work and the use cases appear to be rare outside of QEMU (I
can only think of WINE).  If you feel strongly about it I can revert
those hunks and hard-coded the F flag.

>>  (define qemu-binfmt-service-type
>>    ;; TODO: Make a separate binfmt_misc service out of this?
>>    (service-type (name 'qemu-binfmt)
>> @@ -800,9 +833,7 @@ given QEMU package."
>>                                            (const
>>                                             (list %binary-format-file-system)))
>>                         (service-extension shepherd-root-service-type
>> -                                          qemu-binfmt-shepherd-services)
>> -                       (service-extension guix-service-type
>> -                                          qemu-binfmt-guix-chroot)))
>> +                                          qemu-binfmt-shepherd-services)))
>
> As discussed on IRC, the downside of this approach is increased disk and
> memory footprint (those big binaries have to be loaded in memory).

The 'big' binaries are not that bigger than what we already have.  For a
typical one:

$ du -h {/gnu/store/7w04gv6m92n40dainn4s6xr3l20r90xw-qemu-5.1.0,\
  /gnu/store/wqh2dyskzkl4vjn6harclyl317h4vfaf-qemu-5.1.0-static}/bin/qemu-arm

4.6M    /gnu/store/7w04gv6m92n40dainn4s6xr3l20r90xw-qemu-5.1.0/bin/qemu-arm
6.4M    /gnu/store/wqh2dyskzkl4vjn6harclyl317h4vfaf-qemu-5.1.0-static/bin/qemu-arm

Only the registered QEMU architectures would be preloaded; so if you
enable 5, the increase it at worst ~ 30 MiB.  Nothing too worrying on
machines that should be quite capable for transparent emulation
purposes, in my opinion.

> One possibility would be to add an option to choose between this and the
> current approach, but maybe it’s not worth the maintenance trouble.

I would rather K.I.S.S., with this only case that works everywhere and
thus doesn't surprise anyone.

> Thanks for fixing this issue!

Thanks for the review!

Maxim