qapi: Craft the BlockdevCreateOptionsLUKS comment

[PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

Posted by Hyman Huang 9 months, 1 week ago

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.
+#     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
-- 
2.39.3

Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

Posted by Markus Armbruster 9 months, 1 week ago

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

> +#     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?

@file and @header are BlockdevRef, which means they refer to existing
images with arbitrary driver.  Could be "file", "qcow2", or anything.

Correct?

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?

In any case, @size is the logical size of the image (how much data it
can hold).

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.

With 2., @size is merely recorded in the detached LUKS header.

With 3., @size is recorded in the detached LUKS header, and the @file
image is resized as with 1.

Correct?

Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

Posted by Yong Huang 9 months, 1 week ago

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.


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


>
> With 3., @size is recorded in the detached LUKS header, and the @file
> image is resized as with 1.
>

Same reason as above, @size is not recorded and @file image is
resized to @size exactly.


>
> Correct?
>
>
Thanks for the comment,

Yong

-- 
Best regards

Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

Posted by Markus Armbruster 9 months, 1 week ago

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?

>> With 3., @size is recorded in the detached LUKS header, and the @file
>> image is resized as with 1.
>>
>
> Same reason as above, @size is not recorded and @file image is
> resized to @size exactly.
>
>
>>
>> Correct?
>>
>>
> Thanks for the comment,
>
> Yong

Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

Posted by Yong Huang 9 months, 1 week ago

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


>
> >> With 3., @size is recorded in the detached LUKS header, and the @file
> >> image is resized as with 1.
> >>
> >
> > Same reason as above, @size is not recorded and @file image is
> > resized to @size exactly.
> >
> >
> >>
> >> Correct?
> >>
> >>
> > Thanks for the comment,
> >
> > Yong
>
>

-- 
Best regards

Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

Posted by Markus Armbruster 9 months ago

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?

>> >> With 3., @size is recorded in the detached LUKS header, and the @file
>> >> image is resized as with 1.
>> >>
>> >
>> > Same reason as above, @size is not recorded and @file image is
>> > resized to @size exactly.
>> >
>> >
>> >>
>> >> Correct?
>> >>
>> >>
>> > Thanks for the comment,
>> >
>> > Yong
>>
>>

Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

Posted by Kevin Wolf 9 months ago

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

Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

Posted by Daniel P. Berrangé 9 months ago

On Wed, Feb 28, 2024 at 11:17:37AM +0100, Kevin Wolf wrote:
> 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.

NB, @size isn't mandatory for creating images. It isn't required
when creating qcow2 files with backing stores, as the size is
acquired from the backing file instead.

$ qemu-img create demo.raw 1g
Formatting 'demo.raw', fmt=raw size=1073741824
$ qemu-img create -o backing_file=demo.raw -o backing_fmt=raw -f qcow2 demo.qcow2
Formatting 'demo.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off compression_type=zlib size=1073741824 backing_file=demo.raw backing_fmt=raw lazy_refcounts=off refcount_bits=16


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

Posted by Kevin Wolf 9 months ago

Am 28.02.2024 um 12:30 hat Daniel P. Berrangé geschrieben:
> On Wed, Feb 28, 2024 at 11:17:37AM +0100, Kevin Wolf wrote:
> > 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.
> 
> NB, @size isn't mandatory for creating images. It isn't required
> when creating qcow2 files with backing stores, as the size is
> acquired from the backing file instead.
> 
> $ qemu-img create demo.raw 1g
> Formatting 'demo.raw', fmt=raw size=1073741824
> $ qemu-img create -o backing_file=demo.raw -o backing_fmt=raw -f qcow2 demo.qcow2
> Formatting 'demo.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off compression_type=zlib size=1073741824 backing_file=demo.raw backing_fmt=raw lazy_refcounts=off refcount_bits=16

Yes, 'qemu-img create' is different, it adds some convenience magic like
this. But for 'blockdev-create', it's mandatory for every driver. I
double checked the QAPI schema.

Kevin

Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

Posted by Yong Huang 9 months ago

On Wed, Feb 28, 2024 at 7:58 PM Kevin Wolf <kwolf@redhat.com> wrote:

> Am 28.02.2024 um 12:30 hat Daniel P. Berrangé geschrieben:
> > On Wed, Feb 28, 2024 at 11:17:37AM +0100, Kevin Wolf wrote:
> > > 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.
> >
> > NB, @size isn't mandatory for creating images. It isn't required
> > when creating qcow2 files with backing stores, as the size is
> > acquired from the backing file instead.
> >
> > $ qemu-img create demo.raw 1g
> > Formatting 'demo.raw', fmt=raw size=1073741824
> > $ qemu-img create -o backing_file=demo.raw -o backing_fmt=raw -f qcow2
> demo.qcow2
> > Formatting 'demo.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off
> compression_type=zlib size=1073741824 backing_file=demo.raw backing_fmt=raw
> lazy_refcounts=off refcount_bits=16
>
> Yes, 'qemu-img create' is different, it adds some convenience magic like
> this. But for 'blockdev-create', it's mandatory for every driver. I
> double checked the QAPI schema.
>
> Kevin
>
>
IMHO, LUKS detached header creation is not the case as qcow2 backing store
creation in
aspect of allowing @size absent, since qemu-img creation process fetch
the @size from
backing file as Daniel said above actually, while contrastively LUKS
detached header
creation with no need for @size. Making @size optional feels better for me.

Yong

-- 
Best regards

Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

Posted by Daniel P. Berrangé 9 months ago

On Wed, Feb 28, 2024 at 11:17:37AM +0100, Kevin Wolf wrote:
> 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?

eg you're creating a detached LUKS header that will be used in
combination with a block device, or an NBD export, or some other
storage that was pre-created in some manner, and onto which you
now want to store LUKS encrypted data.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

Posted by Kevin Wolf 9 months ago

Am 28.02.2024 um 11:23 hat Daniel P. Berrangé geschrieben:
> On Wed, Feb 28, 2024 at 11:17:37AM +0100, Kevin Wolf wrote:
> > 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?
> 
> eg you're creating a detached LUKS header that will be used in
> combination with a block device, or an NBD export, or some other
> storage that was pre-created in some manner, and onto which you
> now want to store LUKS encrypted data.

Why don't you pass a host_device/nbd/... node as @file then?

Kevin

Re: [PATCH] qapi: Craft the BlockdevCreateOptionsLUKS comment

Posted by Daniel P. Berrangé 9 months ago

On Wed, Feb 28, 2024 at 12:21:02PM +0100, Kevin Wolf wrote:
> Am 28.02.2024 um 11:23 hat Daniel P. Berrangé geschrieben:
> > On Wed, Feb 28, 2024 at 11:17:37AM +0100, Kevin Wolf wrote:
> > > 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?
> > 
> > eg you're creating a detached LUKS header that will be used in
> > combination with a block device, or an NBD export, or some other
> > storage that was pre-created in some manner, and onto which you
> > now want to store LUKS encrypted data.
> 
> Why don't you pass a host_device/nbd/... node as @file then?

It is possible of course, but it serves no functional purpose. The
act of formatting the LUKS volume would never touch that data payload
storage, unless preallocation is requested. IOW, aside from QEMU
opening the @file (which proves it exists) it would be ignored.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|