Re: Some iotests are failing with -luks

[Kevin Wolf <kwolf@redhat.com>]

Am 11.09.2025 um 12:38 hat Daniel P. Berrangé geschrieben:
> On Thu, Sep 11, 2025 at 12:04:24PM +0200, Kevin Wolf wrote:
> > Am 11.09.2025 um 04:33 hat Yong Huang geschrieben:
> > > On Thu, Sep 11, 2025 at 2:38 AM Kevin Wolf <kwolf@redhat.com> wrote:
> > > 
> > > > Am 10.09.2025 um 18:08 hat Kevin Wolf geschrieben:
> > > > > Am 10.09.2025 um 17:16 hat Thomas Huth geschrieben:
> > > > > > luks-detached-header   fail       [17:15:26] [17:15:38]   12.2s
> > > >         failed, exit status 1
> > > > > > ---
> > > > /home/thuth/devel/qemu/tests/qemu-iotests/tests/luks-detached-header.out
> > > > > > +++
> > > > /home/thuth/tmp/qemu-build/tests/qemu-iotests/scratch/luks-file-luks-detached-header/luks-detached-header.out.bad
> > > > > > @@ -1,5 +1,55 @@
> > > > > > -..
> > > > > > +EE
> > > > > > +======================================================================
> > > > > > +ERROR: test_detached_luks_header
> > > > (__main__.TestDetachedLUKSHeader.test_detached_luks_header)
> > > > > > +----------------------------------------------------------------------
> > > > > > +Traceback (most recent call last):
> > > > > > +  File
> > > > "/home/thuth/devel/qemu/tests/qemu-iotests/tests/luks-detached-header",
> > > > line 139, in setUp
> > > > > > +    res = qemu_img_create(
> > > > > > +          ^^^^^^^^^^^^^^^^
> > > > > > +  File "/home/thuth/devel/qemu/tests/qemu-iotests/iotests.py", line
> > > > 278, in qemu_img_create
> > > > > > +    return qemu_img('create', *args)
> > > > > > +           ^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > > +  File "/home/thuth/devel/qemu/tests/qemu-iotests/iotests.py", line
> > > > 261, in qemu_img
> > > > > > +    return qemu_tool(*full_args, check=check,
> > > > combine_stdio=combine_stdio)
> > > > > > +
> > > >  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > > +  File "/home/thuth/devel/qemu/tests/qemu-iotests/iotests.py", line
> > > > 241, in qemu_tool
> > > > > > +    raise VerboseProcessError(
> > > > > > +qemu.utils.VerboseProcessError: Command
> > > > '('/home/thuth/tmp/qemu-build/qemu-img', 'create', '-f', 'luks', '-o',
> > > > 'iter-time=10', '-o', 'key-secret=sec0', '-o', 'detached-header=true',
> > > > '--object', 'secret,id=sec0,data=foo',
> > > > '/home/thuth/tmp/qemu-build/tests/qemu-iotests/scratch/luks-file-luks-detached-header/detached_header.img2')'
> > > > returned non-zero exit status 1.
> > > > > > +  ┏━ output
> > > > ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
> > > > > > +  ┃ Formatting
> > > > '/home/thuth/tmp/qemu-build/tests/qemu-iotests/scratch/l
> > > > > > +  ┃ uks-file-luks-detached-header/detached_header.img2', fmt=luks
> > > > > > +  ┃ size=-1 key-secret=sec0 iter-time=10 detached-header=true
> > > > > > +  ┃ qemu-img:
> > > > /home/thuth/tmp/qemu-build/tests/qemu-iotests/scratch/luk
> > > > > > +  ┃ s-file-luks-detached-header/detached_header.img2: Parameter
> > > > > > +  ┃ 'detached-header' is unexpected
> > > > > > +
> > > > ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
> > > > >
> > > > > This one is surprising. I don't think anything relevant in the luks
> > > > > driver has changed since the test was introduced. At the same time, the
> > > > > code clearly has a problem when it tries to convert a QemuOpts
> > > > > containing a "detached-header" option into a QAPI object when the schema
> > > > > doesn't even have this option. Was this broken from the beginning? Would
> > > > > have been for a year and half.
> > > >
> > > > I bisected this one because I was curious how this could happen, and it
> > > > was broken quite explicitly by commit e818c01a:
> > > >
> > > > commit e818c01ae6e7c54c7019baaf307be59d99ce80b9 (HEAD)
> > > > Author: Daniel P. Berrangé <berrange@redhat.com>
> > > > Date:   Mon Feb 19 15:12:59 2024 +0000
> > > >
> > > >     qapi: drop unused QCryptoBlockCreateOptionsLUKS.detached-header
> > > >
> > > >     The 'detached-header' field in QCryptoBlockCreateOptionsLUKS
> > > >     was left over from earlier patch iterations.
> > > >
> > > >     Acked-by: Markus Armbruster <armbru@redhat.com>
> > > >     Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > >
> > > > The test case demonstrates that it actually wasn't unused.
> > > >
> > > > If we don't want to reintroduce the field in QAPI, we need to explicitly
> > > >
> > > 
> > > Keeping the detached-header option is more convenient for users when
> > > creating a detached-header image.
> > > 
> > > My inclination is to bring this optionback.  Any suggestions? cc @Daniel P.
> > > Berrangé <berrange@redhat.com>
> > 
> > Having it available for users in qemu-img is different from having it in
> > QAPI. Arguably there is no use for it in QAPI, as long as you make sure
> > that it's taken out of the QemuOpts before going to QAPI.
> 
> Ah so what we actually need to fix is
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 4eed3ffa6a..8b60510c61 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -792,7 +792,7 @@ block_crypto_co_create_opts_luks(BlockDriver *drv, const char *filename,
>      char *buf = NULL;
>      int64_t size;
>      bool detached_hdr =
> -        qemu_opt_get_bool(opts, "detached-header", false);
> +        qemu_opt_get_bool_del(opts, "detached-header", false);
>      unsigned int cflags = 0;
>      int ret;
>      Error *local_err = NULL;

That's what I had in mind, but we need to verify that we don't get a
scenario like what you hinted at where converting the options back to
some non-QAPI data structure (QemuOpts or QDict) would still use it
later.

Kevin