thr3ads.net - Libguestfs - Re: [Libguestfs] [PATCH common v2 4/4] options: Ignore errors from guestfs_luks

If this information is useful, please help other people find it:
Share via:

Martin Kletzander

2020-Oct-06 13:25 UTC

Re: [Libguestfs] [PATCH common v2 4/4] options: Ignore errors from guestfs_luks_uuid.

On Mon, Sep 07, 2020 at 10:41:20AM +0100, Richard W.M. Jones
wrote:>For BitLocker disks cryptsetup does not (yet? ever?) support reading
>UUIDs and this function will fail.  This does not matter here so just
>ignore the error.
>
>Note there is no error message, cryptsetup simply returns with a bad
>exit code:
>
>><rescue> cryptsetup luksUUID /dev/sda2
>><rescue> echo $?
>1
>
>Updates commit bb4a2dc17a78b53437896d4215ae82df8e11b788.
>---
> options/decrypt.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
>diff --git a/options/decrypt.c b/options/decrypt.c
>index 8eb24bc..6b1c0a8 100644
>--- a/options/decrypt.c
>+++ b/options/decrypt.c
>@@ -25,6 +25,7 @@
>
> #include <stdio.h>
> #include <stdlib.h>
>+#include <stdbool.h>
> #include <string.h>
> #include <libintl.h>
> #include <error.h>
>@@ -82,11 +83,23 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks)
>     CLEANUP_FREE char *type = guestfs_vfs_type (g, partitions[i]);
>     if (type &&
>         (STREQ (type, "crypto_LUKS") || STREQ (type,
"BitLocker"))) {
>+      bool is_bitlocker = STREQ (type, "BitLocker");
>       char mapname[32];
>       make_mapname (partitions[i], mapname, sizeof mapname);
>
> #ifdef GUESTFS_HAVE_LUKS_UUID
>-      CLEANUP_FREE char *uuid = guestfs_luks_uuid (g, partitions[i]);
>+      CLEANUP_FREE char *uuid;
>+      if (!is_bitlocker)
>+        uuid = guestfs_luks_uuid (g, partitions[i]);
>+      else {
>+        /* This may fail for Windows BitLocker disks because
>+         * cryptsetup luksUUID cannot read a UUID (unclear if
>+         * this is a limitation of the format or cryptsetup).
>+         */
>+        guestfs_push_error_handler (g, NULL, NULL);
>+        uuid = guestfs_luks_uuid (g, partitions[i]);
I have yet to look at the libguestfs patches, but I do not completely understand
what is the reason for calling "guestfs_luks_uuid" when you know it
will fail.
Or is there a case when it might be useful to get the result?

Other than that the series looks fine to me.
>+        guestfs_pop_error_handler (g);
>+      }
> #else
>       const char *uuid = NULL;
> #endif
>-- 
>2.27.0
>
>_______________________________________________
>Libguestfs mailing list
>Libguestfs@redhat.com
>https://www.redhat.com/mailman/listinfo/libguestfs

Richard W.M. Jones

2020-Oct-06 14:06 UTC

head link

Re: [Libguestfs] [PATCH common v2 4/4] options: Ignore errors from guestfs_luks_uuid.

On Tue, Oct 06, 2020 at 03:25:20PM +0200, Martin Kletzander
wrote:> On Mon, Sep 07, 2020 at 10:41:20AM +0100, Richard W.M. Jones wrote:
> >For BitLocker disks cryptsetup does not (yet? ever?) support reading
> >UUIDs and this function will fail.  This does not matter here so just
> >ignore the error.
> >
> >Note there is no error message, cryptsetup simply returns with a bad
> >exit code:
> >
> >><rescue> cryptsetup luksUUID /dev/sda2
> >><rescue> echo $?
> >1
> >
> >Updates commit bb4a2dc17a78b53437896d4215ae82df8e11b788.
> >---
> >options/decrypt.c | 15 ++++++++++++++-
> >1 file changed, 14 insertions(+), 1 deletion(-)
> >
> >diff --git a/options/decrypt.c b/options/decrypt.c
> >index 8eb24bc..6b1c0a8 100644
> >--- a/options/decrypt.c
> >+++ b/options/decrypt.c
> >@@ -25,6 +25,7 @@
> >
> >#include <stdio.h>
> >#include <stdlib.h>
> >+#include <stdbool.h>
> >#include <string.h>
> >#include <libintl.h>
> >#include <error.h>
> >@@ -82,11 +83,23 @@ inspect_do_decrypt (guestfs_h *g, struct key_store
*ks)
> >    CLEANUP_FREE char *type = guestfs_vfs_type (g, partitions[i]);
> >    if (type &&
> >        (STREQ (type, "crypto_LUKS") || STREQ (type,
"BitLocker"))) {
> >+      bool is_bitlocker = STREQ (type, "BitLocker");
> >      char mapname[32];
> >      make_mapname (partitions[i], mapname, sizeof mapname);
> >
> >#ifdef GUESTFS_HAVE_LUKS_UUID
> >-      CLEANUP_FREE char *uuid = guestfs_luks_uuid (g, partitions[i]);
> >+      CLEANUP_FREE char *uuid;
> >+      if (!is_bitlocker)
> >+        uuid = guestfs_luks_uuid (g, partitions[i]);
> >+      else {
> >+        /* This may fail for Windows BitLocker disks because
> >+         * cryptsetup luksUUID cannot read a UUID (unclear if
> >+         * this is a limitation of the format or cryptsetup).
> >+         */
> >+        guestfs_push_error_handler (g, NULL, NULL);
> >+        uuid = guestfs_luks_uuid (g, partitions[i]);
> 
> I have yet to look at the libguestfs patches, but I do not completely
understand
> what is the reason for calling "guestfs_luks_uuid" when you know
it will fail.
> Or is there a case when it might be useful to get the result?
Yes I don't know what I was thinking there.  It's difficult to revisit
and revise patches months later.  It would be better as simply:

diff --git a/options/decrypt.c b/options/decrypt.c
index 8eb24bc..ca2a0bb 100644
--- a/options/decrypt.c
+++ b/options/decrypt.c
@@ -25,6 +25,7 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <stdbool.h>
 #include <string.h>
 #include <libintl.h>
 #include <error.h>
@@ -82,11 +83,19 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks)
     CLEANUP_FREE char *type = guestfs_vfs_type (g, partitions[i]);
     if (type &&
         (STREQ (type, "crypto_LUKS") || STREQ (type,
"BitLocker"))) {
+      bool is_bitlocker = STREQ (type, "BitLocker");
       char mapname[32];
       make_mapname (partitions[i], mapname, sizeof mapname);
 
 #ifdef GUESTFS_HAVE_LUKS_UUID
-      CLEANUP_FREE char *uuid = guestfs_luks_uuid (g, partitions[i]);
+      CLEANUP_FREE char *uuid = NULL;
+
+      /* This fails for Windows BitLocker disks because cryptsetup
+       * luksUUID cannot read a UUID (unclear if this is a limitation
+       * of the format or cryptsetup).
+       */
+      if (!is_bitlocker)
+        uuid = guestfs_luks_uuid (g, partitions[i]);
 #else
       const char *uuid = NULL;
 #endif

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Martin Kletzander

2020-Oct-07 07:25 UTC

head link

Re: [Libguestfs] [PATCH common v2 4/4] options: Ignore errors from guestfs_luks_uuid.

On Tue, Oct 06, 2020 at 03:06:54PM +0100, Richard W.M. Jones
wrote:>On Tue, Oct 06, 2020 at 03:25:20PM +0200, Martin Kletzander wrote:
>> On Mon, Sep 07, 2020 at 10:41:20AM +0100, Richard W.M. Jones wrote:
>> >For BitLocker disks cryptsetup does not (yet? ever?) support
reading
>> >UUIDs and this function will fail.  This does not matter here so
just
>> >ignore the error.
>> >
>> >Note there is no error message, cryptsetup simply returns with a
bad
>> >exit code:
>> >
>> >><rescue> cryptsetup luksUUID /dev/sda2
>> >><rescue> echo $?
>> >1
>> >
>> >Updates commit bb4a2dc17a78b53437896d4215ae82df8e11b788.
>> >---
>> >options/decrypt.c | 15 ++++++++++++++-
>> >1 file changed, 14 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/options/decrypt.c b/options/decrypt.c
>> >index 8eb24bc..6b1c0a8 100644
>> >--- a/options/decrypt.c
>> >+++ b/options/decrypt.c
>> >@@ -25,6 +25,7 @@
>> >
>> >#include <stdio.h>
>> >#include <stdlib.h>
>> >+#include <stdbool.h>
>> >#include <string.h>
>> >#include <libintl.h>
>> >#include <error.h>
>> >@@ -82,11 +83,23 @@ inspect_do_decrypt (guestfs_h *g, struct
key_store *ks)
>> >    CLEANUP_FREE char *type = guestfs_vfs_type (g, partitions[i]);
>> >    if (type &&
>> >        (STREQ (type, "crypto_LUKS") || STREQ (type,
"BitLocker"))) {
>> >+      bool is_bitlocker = STREQ (type, "BitLocker");
>> >      char mapname[32];
>> >      make_mapname (partitions[i], mapname, sizeof mapname);
>> >
>> >#ifdef GUESTFS_HAVE_LUKS_UUID
>> >-      CLEANUP_FREE char *uuid = guestfs_luks_uuid (g,
partitions[i]);
>> >+      CLEANUP_FREE char *uuid;
>> >+      if (!is_bitlocker)
>> >+        uuid = guestfs_luks_uuid (g, partitions[i]);
>> >+      else {
>> >+        /* This may fail for Windows BitLocker disks because
>> >+         * cryptsetup luksUUID cannot read a UUID (unclear if
>> >+         * this is a limitation of the format or cryptsetup).
>> >+         */
>> >+        guestfs_push_error_handler (g, NULL, NULL);
>> >+        uuid = guestfs_luks_uuid (g, partitions[i]);
>>
>> I have yet to look at the libguestfs patches, but I do not completely
understand
>> what is the reason for calling "guestfs_luks_uuid" when you
know it will fail.
>> Or is there a case when it might be useful to get the result?
>
>Yes I don't know what I was thinking there.  It's difficult to
revisit
>and revise patches months later.  It would be better as simply:
>
>diff --git a/options/decrypt.c b/options/decrypt.c
>index 8eb24bc..ca2a0bb 100644
>--- a/options/decrypt.c
>+++ b/options/decrypt.c
>@@ -25,6 +25,7 @@
>
> #include <stdio.h>
> #include <stdlib.h>
>+#include <stdbool.h>
> #include <string.h>
> #include <libintl.h>
> #include <error.h>
>@@ -82,11 +83,19 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks)
>     CLEANUP_FREE char *type = guestfs_vfs_type (g, partitions[i]);
>     if (type &&
>         (STREQ (type, "crypto_LUKS") || STREQ (type,
"BitLocker"))) {
>+      bool is_bitlocker = STREQ (type, "BitLocker");
>       char mapname[32];
>       make_mapname (partitions[i], mapname, sizeof mapname);
>
> #ifdef GUESTFS_HAVE_LUKS_UUID
>-      CLEANUP_FREE char *uuid = guestfs_luks_uuid (g, partitions[i]);
>+      CLEANUP_FREE char *uuid = NULL;
>+
>+      /* This fails for Windows BitLocker disks because cryptsetup
>+       * luksUUID cannot read a UUID (unclear if this is a limitation
>+       * of the format or cryptsetup).
>+       */
>+      if (!is_bitlocker)
>+        uuid = guestfs_luks_uuid (g, partitions[i]);
> #else
>       const char *uuid = NULL;
> #endif
>
>Rich.
>
Glad I understood it then, ACK to that.
>--
>Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
>Read my programming and virtualization blog: http://rwmj.wordpress.com
>virt-builder quickly builds VMs from scratch
>http://libguestfs.org/virt-builder.1.html

Apparently Analagous Threads

Search for more possibly parallel threads

Libguestfs - Oct 2020 - Re: [PATCH common v2 4/4] options: Ignore errors from guestfs_luks_uuid.

Re: [Libguestfs] [PATCH common v2 4/4] options: Ignore errors from guestfs_luks_uuid.

Re: [Libguestfs] [PATCH common v2 4/4] options: Ignore errors from guestfs_luks_uuid.

Re: [Libguestfs] [PATCH common v2 4/4] options: Ignore errors from guestfs_luks_uuid.

Apparently Analagous Threads