Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

[Kevin Wolf]

Am 28.02.2024 um 07:43 hat Markus Armbruster geschrieben:
> Yong Huang <yong.huang@smartx.com> writes:
> 
> > On Wed, Feb 21, 2024 at 4:26 PM Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Yong Huang <yong.huang@smartx.com> writes:
> >>
> >> > On Wed, Feb 21, 2024 at 2:43 PM Markus Armbruster <armbru@redhat.com>
> >> wrote:
> >> >
> >> >> Hyman Huang <yong.huang@smartx.com> writes:
> >> >>
> >> >> > Add comment in detail for commit 433957bb7f (qapi:
> >> >> > Make parameter 'file' optional for
> >> >> > BlockdevCreateOptionsLUKS).
> >> >> >
> >> >> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> >> >> > ---
> >> >> >  qapi/block-core.json | 20 +++++++++++++++++++-
> >> >> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >> >> >
> >> >> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> >> > index ab5a93a966..42b0840d43 100644
> >> >> > --- a/qapi/block-core.json
> >> >> > +++ b/qapi/block-core.json
> >> >> > @@ -4973,7 +4973,25 @@
> >> >> >  ##
> >> >> >  # @BlockdevCreateOptionsLUKS:
> >> >> >  #
> >> >> > -# Driver specific image creation options for LUKS.
> >> >> > +# Driver specific image creation options for LUKS. Note that
> >> >> > +# @file is required if @preallocation is specified and equals
> >> >> > +# PREALLOC_MODE_ON. The following three scenarios determine how
> >> >> > +# creation logic behaves when @preallocation is either equal to
> >> >> > +# PREALLOC_MODE_OFF or is not given:
> >> >> > +#
> >> >> > +#  1) When @file is given only, format the block device referenced
> >> >> > +#     by @file as the LUKS specification and trunk it to the @size.
> >> >>
> >> >> Do you mean "truncate it to @size"?
> >> >>
> >> > Yes, :( sorry for the spelling mistake.
> >>
> >> Writing good documentation in a second language is *hard*.  All we can
> >> reasonably expect from contributors to try their best.  And then we
> >> improve the text together in review.  Just like we do for code :)
> >>
> >> >> > +#     In this case, the @size should reflect amount of space made
> >> >> > +#     available to the guest, so the trunk size must take account
> >> >> > +#     of that which will be used by the crypto header.
> >> >> > +#
> >> >> > +#  2) When @header is given only, just format the block device
> >> >> > +#     referenced by @header as the LUKS specification.
> >> >> > +#
> >> >> > +#  3) When both @file and @header are given, block device
> >> >> > +#     referenced by @file should be trunked to @size, and block
> >> >> > +#     device referenced by @header should be formatted as the LUKS
> >> >> > +#     specification.
> >> >> >  #
> >> >> >  # @file: Node to create the image format on, mandatory except when
> >> >> >  #        'preallocation' is not requested
> >> >>
> >> >> Let's see whether I understand.
> >> >>
> >> >> blockdev-create with "driver": "luks" can work in three different ways:
> >> >>
> >> >> 1. Create an image with a LUKS header
> >> >>
> >> >> 2. Create just a detached LUKS header
> >> >>
> >> >> 3. Create an image and a detached LUKS header
> >> >>
> >> >> Correct?
> >> >>
> >> >
> >> > Yes
> >> >
> >> >
> >> >> @file and @header are BlockdevRef, which means they refer to existing
> >> >> images with arbitrary driver.  Could be "file", "qcow2", or anything.
> >> >>
> >> >> Correct?
> >> >>
> >> > Yes
> >> >
> >> >
> >> >>
> >> >> To get 1., specify @file, but not @header.
> >> >>
> >> >> To get 2., specify @header, but not @file.
> >> >>
> >> >> To get 3., specify both.
> >> >>
> >> >> Specifying neither is an error.
> >> >>
> >> >> Correct?
> >> >>
> >> >
> >> > Yes
> >> >
> >> >
> >> >> In any case, @size is the logical size of the image (how much data it
> >> >> can hold).
> >> >>
> >> >
> >> > Yes
> >> >
> >> >
> >> >>
> >> >> With 1., the actual image size is a bit larger due to the LUKS header.
> >> >> The @file image is resized to that size: if it's shorter, it's grown, if
> >> >> it's longer, it's truncated.
> >> >>
> >> >
> >> > Yes
> >> >
> >> >
> >> >> With 2., @size is merely recorded in the detached LUKS header.
> >> >>
> >> >
> >> > In LUKS1 specification, payload data size is not contained in the header,
> >> > so in this case, @size is not recorded in the detached LUKS header.
> >> > The creation logic just does the LUKS header formatting only.
> >>
> >> Is @size unused then?
> >>
> >
> > IIUC, yes. Creation logic will ignore the @size. See the following code
> > in function block_crypto_co_create_luks:
> >
> >     if (luks_opts->header) {
> >         /* LUKS volume with detached header */
> >         hdr_bs = bdrv_co_open_blockdev_ref(luks_opts->header, errp);
> >         if (hdr_bs == NULL) {
> >             return -EIO;
> >         }
> >
> >         cflags |= QCRYPTO_BLOCK_CREATE_DETACHED;
> >
> >         /* Format the LUKS header node, here just ignore the size
> >           * and passed zero to block_crypto_co_create_generic */
> >         ret = block_crypto_co_create_generic(hdr_bs, 0, &create_opts,
> >                                              PREALLOC_MODE_OFF, cflags, 
> > errp);
> >         if (ret < 0) {
> >             goto fail;
> >         }
> >
> >         /* Format the LUKS payload node */
> >         if (luks_opts->file) {
> >             ret = block_crypto_co_format_luks_payload(luks_opts, errp);
> >             if (ret < 0) {
> >                 goto fail;
> >             }
> >         }
> 
> @size is a required argument, but silently ignored when @header is
> present and @file is absent (2. Create just a detached LUKS header).
> Feels awkward.
> 
> Should @size be optional, absent when and only when @header is present
> and @file is absent?
> 
> Kevin or Hanna, got an opinion?

What is the use case for creating a header without a corresponding
image?

Until now, @size has been mandatory for creating images with every
driver. Maybe we should even have put it into BlockdevCreateOptions's
base, because without a size, you're not really creating an image.

So before making it optional here and breaking the consistency that we
have so far, I'd like to understand what reason we have to use
blockdev-create for something that is not creating a full image, but
only a part of it.

(Of course, I also don't exactly _like_ marking things optional when
they are not really optional, but their presence depends on some other
field. But I'm afraid the QAPI schema language isn't expressive enough
to avoid it if we do have this requirement.)

Kevin