[PATCH] disk: Move hardcoded max disk size literal to a GRUB_DISK_MAX_SECTORS in disk.h

[PATCH] disk: Move hardcoded max disk size literal to a GRUB_DISK_MAX_SECTORS in disk.h

[Glenn Washburn]

There is a hardcoded maximum disk size that can be read or written from,
currently set at 1EiB in grub_disk_adjust_range. Move the literal into a
macro in disk.h, so our assumptions are more visible. This hard coded limit
does not prevent using larger disks, just grub won't read/write past the
limit. The comment accompanying this restriction didn't quite make sense to
me, so its been modified.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/kern/disk_common.c | 13 +++++++------
 include/grub/disk.h          |  6 ++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/grub-core/kern/disk_common.c b/grub-core/kern/disk_common.c
index 5b92f670b..d20ed20d2 100644
--- a/grub-core/kern/disk_common.c
+++ b/grub-core/kern/disk_common.c
@@ -32,13 +32,14 @@ grub_disk_adjust_range (grub_disk_t disk, grub_disk_addr_t *sector,
   /* Transform total_sectors to number of 512B blocks.  */
   total_sectors = grub_disk_from_native_sector (disk, disk->total_sectors);
 
-  /* Some drivers have problems with disks above reasonable.
-     Treat unknown as 1EiB disk. While on it, clamp the size to 1EiB.
-     Just one condition is enough since GRUB_DISK_UNKNOWN_SIZE << ls is always
-     above 9EiB.
+  /*
+     Some drivers have problems with disks above reasonable sizes.
+     Clamp the size to GRUB_DISK_MAX_SECTORS. Just one condition is enough
+     since GRUB_DISK_UNKNOWN_SIZE is always above GRUB_DISK_MAX_SECTORS,
+     assuming a maximum 4KiB sector size.
   */
-  if (total_sectors > (1ULL << 51))
-    total_sectors = (1ULL << 51);
+  if (total_sectors > GRUB_DISK_MAX_SECTORS)
+    total_sectors = GRUB_DISK_MAX_SECTORS;
 
   if ((total_sectors <= *sector
        || ((*offset + size + GRUB_DISK_SECTOR_SIZE - 1)
diff --git a/include/grub/disk.h b/include/grub/disk.h
index 1a9e8fcf4..0bf08091f 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -157,6 +157,12 @@ struct grub_disk_memberlist
 typedef struct grub_disk_memberlist *grub_disk_memberlist_t;
 #endif
 
+/*
+   Some drivers have problems with disks above reasonable sizes.
+   Set max disk size at 1EiB.
+*/
+#define GRUB_DISK_MAX_SECTORS	(1ULL << (60 - 9))
+
 /* The sector size.  */
 #define GRUB_DISK_SECTOR_SIZE	0x200
 #define GRUB_DISK_SECTOR_BITS	9
-- 
2.27.0

[PATCH] loopback: Do not automaticaly replace existing loopback dev, error instead

[Glenn Washburn]

If there is a loopback device with the same name as the one to be created,
instead of closing the old one and replacing it with the new one, return an
error instead. If the loopback device was created, its probably being used
by something and just replacing it may cause grub to loop forever. This
fixes obvious problems like `loopback d (d)/somefile'. Its not too onerous
to force the user to delete the loopback first with the `-d' switch.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/loopback.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/grub-core/disk/loopback.c b/grub-core/disk/loopback.c
index 0f58d8622..d77584855 100644
--- a/grub-core/disk/loopback.c
+++ b/grub-core/disk/loopback.c
@@ -98,18 +98,13 @@ grub_cmd_loopback (grub_extcmd_context_t ctxt, int argc, char **args)
   if (! file)
     return grub_errno;
 
-  /* First try to replace the old device.  */
+  /* Check that a device with requested name does not already exist.  */
   for (newdev = loopback_list; newdev; newdev = newdev->next)
     if (grub_strcmp (newdev->devname, args[0]) == 0)
-      break;
-
-  if (newdev)
-    {
-      grub_file_close (newdev->file);
-      newdev->file = file;
-
-      return 0;
-    }
+      {
+	ret = grub_error(GRUB_ERR_BAD_ARGUMENT, "device name already exists");
+	goto fail;
+      }
 
   /* Unable to replace it, make a new entry.  */
   newdev = grub_malloc (sizeof (struct grub_loopback));
-- 
2.27.0

Re: [PATCH] loopback: Do not automaticaly replace existing loopback dev, error instead

[Daniel Kiper]

On Fri, Nov 27, 2020 at 02:36:24AM -0600, Glenn Washburn wrote:
> If there is a loopback device with the same name as the one to be created,
> instead of closing the old one and replacing it with the new one, return an
> error instead. If the loopback device was created, its probably being used
> by something and just replacing it may cause grub to loop forever. This
> fixes obvious problems like `loopback d (d)/somefile'. Its not too onerous
> to force the user to delete the loopback first with the `-d' switch.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/loopback.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/grub-core/disk/loopback.c b/grub-core/disk/loopback.c
> index 0f58d8622..d77584855 100644
> --- a/grub-core/disk/loopback.c
> +++ b/grub-core/disk/loopback.c
> @@ -98,18 +98,13 @@ grub_cmd_loopback (grub_extcmd_context_t ctxt, int argc, char **args)
>    if (! file)
>      return grub_errno;
>
> -  /* First try to replace the old device.  */
> +  /* Check that a device with requested name does not already exist.  */
>    for (newdev = loopback_list; newdev; newdev = newdev->next)
>      if (grub_strcmp (newdev->devname, args[0]) == 0)

Could you move this check before grub_file_open() call? Then you do not
need to open and close file for nothing.

> -      break;
> -
> -  if (newdev)
> -    {
> -      grub_file_close (newdev->file);
> -      newdev->file = file;
> -
> -      return 0;
> -    }
> +      {
> +	ret = grub_error(GRUB_ERR_BAD_ARGUMENT, "device name already exists");
> +	goto fail;
> +      }
>
>    /* Unable to replace it, make a new entry.  */
>    newdev = grub_malloc (sizeof (struct grub_loopback));

Daniel

Re: [PATCH] loopback: Do not automaticaly replace existing loopback dev, error instead

[Glenn Washburn]

On Wed, 2 Dec 2020 15:36:06 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Fri, Nov 27, 2020 at 02:36:24AM -0600, Glenn Washburn wrote:
> > If there is a loopback device with the same name as the one to be
> > created, instead of closing the old one and replacing it with the
> > new one, return an error instead. If the loopback device was
> > created, its probably being used by something and just replacing it
> > may cause grub to loop forever. This fixes obvious problems like
> > `loopback d (d)/somefile'. Its not too onerous to force the user to
> > delete the loopback first with the `-d' switch.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/loopback.c | 15 +++++----------
> >  1 file changed, 5 insertions(+), 10 deletions(-)
> >
> > diff --git a/grub-core/disk/loopback.c b/grub-core/disk/loopback.c
> > index 0f58d8622..d77584855 100644
> > --- a/grub-core/disk/loopback.c
> > +++ b/grub-core/disk/loopback.c
> > @@ -98,18 +98,13 @@ grub_cmd_loopback (grub_extcmd_context_t ctxt,
> > int argc, char **args) if (! file)
> >      return grub_errno;
> >
> > -  /* First try to replace the old device.  */
> > +  /* Check that a device with requested name does not already
> > exist.  */ for (newdev = loopback_list; newdev; newdev =
> > newdev->next) if (grub_strcmp (newdev->devname, args[0]) == 0)
> 
> Could you move this check before grub_file_open() call? Then you do
> not need to open and close file for nothing.

Good idea.

> > -      break;
> > -
> > -  if (newdev)
> > -    {
> > -      grub_file_close (newdev->file);
> > -      newdev->file = file;
> > -
> > -      return 0;
> > -    }
> > +      {
> > +	ret = grub_error(GRUB_ERR_BAD_ARGUMENT, "device name
> > already exists");
> > +	goto fail;
> > +      }
> >
> >    /* Unable to replace it, make a new entry.  */
> >    newdev = grub_malloc (sizeof (struct grub_loopback));
> 
> Daniel

Glenn

Re: [PATCH] disk: Move hardcoded max disk size literal to a GRUB_DISK_MAX_SECTORS in disk.h

[Daniel Kiper]

On Fri, Nov 27, 2020 at 02:36:23AM -0600, Glenn Washburn wrote:
> There is a hardcoded maximum disk size that can be read or written from,
> currently set at 1EiB in grub_disk_adjust_range. Move the literal into a
> macro in disk.h, so our assumptions are more visible. This hard coded limit
> does not prevent using larger disks, just grub won't read/write past the
> limit. The comment accompanying this restriction didn't quite make sense to
> me, so its been modified.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/kern/disk_common.c | 13 +++++++------
>  include/grub/disk.h          |  6 ++++++
>  2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/kern/disk_common.c b/grub-core/kern/disk_common.c
> index 5b92f670b..d20ed20d2 100644
> --- a/grub-core/kern/disk_common.c
> +++ b/grub-core/kern/disk_common.c
> @@ -32,13 +32,14 @@ grub_disk_adjust_range (grub_disk_t disk, grub_disk_addr_t *sector,
>    /* Transform total_sectors to number of 512B blocks.  */
>    total_sectors = grub_disk_from_native_sector (disk, disk->total_sectors);
>
> -  /* Some drivers have problems with disks above reasonable.
> -     Treat unknown as 1EiB disk. While on it, clamp the size to 1EiB.
> -     Just one condition is enough since GRUB_DISK_UNKNOWN_SIZE << ls is always
> -     above 9EiB.
> +  /*
> +     Some drivers have problems with disks above reasonable sizes.
> +     Clamp the size to GRUB_DISK_MAX_SECTORS. Just one condition is enough
> +     since GRUB_DISK_UNKNOWN_SIZE is always above GRUB_DISK_MAX_SECTORS,
> +     assuming a maximum 4KiB sector size.
>    */
> -  if (total_sectors > (1ULL << 51))
> -    total_sectors = (1ULL << 51);
> +  if (total_sectors > GRUB_DISK_MAX_SECTORS)
> +    total_sectors = GRUB_DISK_MAX_SECTORS;
>
>    if ((total_sectors <= *sector
>         || ((*offset + size + GRUB_DISK_SECTOR_SIZE - 1)
> diff --git a/include/grub/disk.h b/include/grub/disk.h
> index 1a9e8fcf4..0bf08091f 100644
> --- a/include/grub/disk.h
> +++ b/include/grub/disk.h
> @@ -157,6 +157,12 @@ struct grub_disk_memberlist
>  typedef struct grub_disk_memberlist *grub_disk_memberlist_t;
>  #endif
>
> +/*
> +   Some drivers have problems with disks above reasonable sizes.
> +   Set max disk size at 1EiB.

The comment is formatted incorrectly [1].

> +*/
> +#define GRUB_DISK_MAX_SECTORS	(1ULL << (60 - 9))
> +
>  /* The sector size.  */
>  #define GRUB_DISK_SECTOR_SIZE	0x200
>  #define GRUB_DISK_SECTOR_BITS	9

Please move the constant below these definitions and do this:
  #define GRUB_DISK_MAX_SECTORS		(1ULL << (60 - GRUB_DISK_SECTOR_BITS))

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments

Re: [PATCH] disk: Move hardcoded max disk size literal to a GRUB_DISK_MAX_SECTORS in disk.h

[Glenn Washburn]

On Mon, 30 Nov 2020 15:42:14 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Fri, Nov 27, 2020 at 02:36:23AM -0600, Glenn Washburn wrote:
> > There is a hardcoded maximum disk size that can be read or written
> > from, currently set at 1EiB in grub_disk_adjust_range. Move the
> > literal into a macro in disk.h, so our assumptions are more
> > visible. This hard coded limit does not prevent using larger disks,
> > just grub won't read/write past the limit. The comment accompanying
> > this restriction didn't quite make sense to me, so its been
> > modified.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/kern/disk_common.c | 13 +++++++------
> >  include/grub/disk.h          |  6 ++++++
> >  2 files changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/grub-core/kern/disk_common.c
> > b/grub-core/kern/disk_common.c index 5b92f670b..d20ed20d2 100644
> > --- a/grub-core/kern/disk_common.c
> > +++ b/grub-core/kern/disk_common.c
> > @@ -32,13 +32,14 @@ grub_disk_adjust_range (grub_disk_t disk,
> > grub_disk_addr_t *sector, /* Transform total_sectors to number of
> > 512B blocks.  */ total_sectors = grub_disk_from_native_sector
> > (disk, disk->total_sectors);
> >
> > -  /* Some drivers have problems with disks above reasonable.
> > -     Treat unknown as 1EiB disk. While on it, clamp the size to
> > 1EiB.
> > -     Just one condition is enough since GRUB_DISK_UNKNOWN_SIZE <<
> > ls is always
> > -     above 9EiB.
> > +  /*
> > +     Some drivers have problems with disks above reasonable sizes.
> > +     Clamp the size to GRUB_DISK_MAX_SECTORS. Just one condition
> > is enough
> > +     since GRUB_DISK_UNKNOWN_SIZE is always above
> > GRUB_DISK_MAX_SECTORS,
> > +     assuming a maximum 4KiB sector size.
> >    */
> > -  if (total_sectors > (1ULL << 51))
> > -    total_sectors = (1ULL << 51);
> > +  if (total_sectors > GRUB_DISK_MAX_SECTORS)
> > +    total_sectors = GRUB_DISK_MAX_SECTORS;
> >
> >    if ((total_sectors <= *sector
> >         || ((*offset + size + GRUB_DISK_SECTOR_SIZE - 1)
> > diff --git a/include/grub/disk.h b/include/grub/disk.h
> > index 1a9e8fcf4..0bf08091f 100644
> > --- a/include/grub/disk.h
> > +++ b/include/grub/disk.h
> > @@ -157,6 +157,12 @@ struct grub_disk_memberlist
> >  typedef struct grub_disk_memberlist *grub_disk_memberlist_t;
> >  #endif
> >
> > +/*
> > +   Some drivers have problems with disks above reasonable sizes.
> > +   Set max disk size at 1EiB.
> 
> The comment is formatted incorrectly [1].

Yep, copy/paste from the comment changed in previous diff which had an
incorrectly formatted comment and didn't catch it. Good catch. I'll
fix the comment in the previous diff as well.

> > +*/
> > +#define GRUB_DISK_MAX_SECTORS	(1ULL << (60 - 9))
> > +
> >  /* The sector size.  */
> >  #define GRUB_DISK_SECTOR_SIZE	0x200
> >  #define GRUB_DISK_SECTOR_BITS	9
> 
> Please move the constant below these definitions and do this:
>   #define GRUB_DISK_MAX_SECTORS		(1ULL << (60 -
> GRUB_DISK_SECTOR_BITS))

Great idea.

> [1]
> https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments

[PATCH v2] disk: Move hardcoded max disk size literal to a GRUB_DISK_MAX_SECTORS in disk.h

[Glenn Washburn]

There is a hardcoded maximum disk size that can be read or written from,
currently set at 1EiB in grub_disk_adjust_range. Move the literal into a
macro in disk.h, so our assumptions are more visible. This hard coded limit
does not prevent using larger disks, just grub won't read/write past the
limit. The comment accompanying this restriction didn't quite make sense to
me, so its been modified.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/kern/disk_common.c | 15 ++++++++-------
 include/grub/disk.h          |  6 ++++++
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/grub-core/kern/disk_common.c b/grub-core/kern/disk_common.c
index 2ca12b5f8..d43b927a4 100644
--- a/grub-core/kern/disk_common.c
+++ b/grub-core/kern/disk_common.c
@@ -32,13 +32,14 @@ grub_disk_adjust_range (grub_disk_t disk, grub_disk_addr_t *sector,
   /* Transform total_sectors to number of 512B blocks.  */
   total_sectors = disk->total_sectors << (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
 
-  /* Some drivers have problems with disks above reasonable.
-     Treat unknown as 1EiB disk. While on it, clamp the size to 1EiB.
-     Just one condition is enough since GRUB_DISK_UNKNOWN_SIZE << ls is always
-     above 9EiB.
-  */
-  if (total_sectors > (1ULL << 51))
-    total_sectors = (1ULL << 51);
+  /*
+   * Some drivers have problems with disks above reasonable sizes.
+   * Clamp the size to GRUB_DISK_MAX_SECTORS. Just one condition is enough
+   * since GRUB_DISK_UNKNOWN_SIZE is always above GRUB_DISK_MAX_SECTORS,
+   * assuming a maximum 4KiB sector size.
+   */
+  if (total_sectors > GRUB_DISK_MAX_SECTORS)
+    total_sectors = GRUB_DISK_MAX_SECTORS;
 
   if ((total_sectors <= *sector
        || ((*offset + size + GRUB_DISK_SECTOR_SIZE - 1)
diff --git a/include/grub/disk.h b/include/grub/disk.h
index 132a1bb75..93f4ae226 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -161,6 +161,12 @@ typedef struct grub_disk_memberlist *grub_disk_memberlist_t;
 #define GRUB_DISK_SECTOR_SIZE	0x200
 #define GRUB_DISK_SECTOR_BITS	9
 
+/*
+ * Some drivers have problems with disks above reasonable sizes.
+ * Set max disk size at 1EiB.
+ */
+#define GRUB_DISK_MAX_SECTORS	(1ULL << (60 - GRUB_DISK_MAX_SECTORS))
+
 /* The maximum number of disk caches.  */
 #define GRUB_DISK_CACHE_NUM	1021
 
-- 
2.27.0

Re: [PATCH v2] disk: Move hardcoded max disk size literal to a GRUB_DISK_MAX_SECTORS in disk.h

[Daniel Kiper]

On Mon, Nov 30, 2020 at 11:16:19PM -0600, Glenn Washburn wrote:
> There is a hardcoded maximum disk size that can be read or written from,
> currently set at 1EiB in grub_disk_adjust_range. Move the literal into a
> macro in disk.h, so our assumptions are more visible. This hard coded limit
> does not prevent using larger disks, just grub won't read/write past the
> limit. The comment accompanying this restriction didn't quite make sense to
> me, so its been modified.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Except one nit...

> ---
>  grub-core/kern/disk_common.c | 15 ++++++++-------
>  include/grub/disk.h          |  6 ++++++
>  2 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/kern/disk_common.c b/grub-core/kern/disk_common.c
> index 2ca12b5f8..d43b927a4 100644
> --- a/grub-core/kern/disk_common.c
> +++ b/grub-core/kern/disk_common.c
> @@ -32,13 +32,14 @@ grub_disk_adjust_range (grub_disk_t disk, grub_disk_addr_t *sector,
>    /* Transform total_sectors to number of 512B blocks.  */
>    total_sectors = disk->total_sectors << (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
>
> -  /* Some drivers have problems with disks above reasonable.
> -     Treat unknown as 1EiB disk. While on it, clamp the size to 1EiB.
> -     Just one condition is enough since GRUB_DISK_UNKNOWN_SIZE << ls is always
> -     above 9EiB.
> -  */
> -  if (total_sectors > (1ULL << 51))
> -    total_sectors = (1ULL << 51);
> +  /*
> +   * Some drivers have problems with disks above reasonable sizes.
> +   * Clamp the size to GRUB_DISK_MAX_SECTORS. Just one condition is enough
> +   * since GRUB_DISK_UNKNOWN_SIZE is always above GRUB_DISK_MAX_SECTORS,

s/GRUB_DISK_UNKNOWN_SIZE/GRUB_DISK_SIZE_UNKNOWN/

I can fix it before committing the patch.

Daniel

[PATCH v2] loopback: Do not automaticaly replace existing loopback dev, error instead

[Glenn Washburn]

If there is a loopback device with the same name as the one to be created,
instead of closing the old one and replacing it with the new one, return an
error instead. If the loopback device was created, its probably being used
by something and just replacing it may cause grub to crash unexpectedly.
This fixes obvious problems like `loopback d (d)/somefile'. Its not too
onerous to force the user to delete the loopback first with the `-d' switch.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/loopback.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/grub-core/disk/loopback.c b/grub-core/disk/loopback.c
index cdf9123fa..ef3a05fec 100644
--- a/grub-core/disk/loopback.c
+++ b/grub-core/disk/loopback.c
@@ -92,24 +92,16 @@ grub_cmd_loopback (grub_extcmd_context_t ctxt, int argc, char **args)
   if (argc < 2)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
 
+  /* Check that a device with requested name does not already exist.  */
+  for (newdev = loopback_list; newdev; newdev = newdev->next)
+    if (grub_strcmp (newdev->devname, args[0]) == 0)
+      return grub_error(GRUB_ERR_BAD_ARGUMENT, "device name already exists");
+
   file = grub_file_open (args[1], GRUB_FILE_TYPE_LOOPBACK
 			 | GRUB_FILE_TYPE_NO_DECOMPRESS);
   if (! file)
     return grub_errno;
 
-  /* First try to replace the old device.  */
-  for (newdev = loopback_list; newdev; newdev = newdev->next)
-    if (grub_strcmp (newdev->devname, args[0]) == 0)
-      break;
-
-  if (newdev)
-    {
-      grub_file_close (newdev->file);
-      newdev->file = file;
-
-      return 0;
-    }
-
   /* Unable to replace it, make a new entry.  */
   newdev = grub_malloc (sizeof (struct grub_loopback));
   if (! newdev)
-- 
2.27.0

Re: [PATCH v2] loopback: Do not automaticaly replace existing loopback dev, error instead

[Daniel Kiper]

On Thu, Dec 03, 2020 at 07:57:11PM -0600, Glenn Washburn wrote:
> If there is a loopback device with the same name as the one to be created,
> instead of closing the old one and replacing it with the new one, return an
> error instead. If the loopback device was created, its probably being used
> by something and just replacing it may cause grub to crash unexpectedly.
> This fixes obvious problems like `loopback d (d)/somefile'. Its not too
> onerous to force the user to delete the loopback first with the `-d' switch.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

PS May I ask you to create new thread for new version of the patches
   instead of attaching them to previous threads?

Re: [PATCH v2] loopback: Do not automaticaly replace existing loopback dev, error instead

[Glenn Washburn]

On Fri, 4 Dec 2020 13:34:44 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Thu, Dec 03, 2020 at 07:57:11PM -0600, Glenn Washburn wrote:
> > If there is a loopback device with the same name as the one to be
> > created, instead of closing the old one and replacing it with the
> > new one, return an error instead. If the loopback device was
> > created, its probably being used by something and just replacing it
> > may cause grub to crash unexpectedly. This fixes obvious problems
> > like `loopback d (d)/somefile'. Its not too onerous to force the
> > user to delete the loopback first with the `-d' switch.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> 
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> 
> Daniel
> 
> PS May I ask you to create new thread for new version of the patches
>    instead of attaching them to previous threads?

These two patches were not meant to be a thread together, ie separate
patches, which was why I was treating them separately.  I can see how
that might be confusing now.

Glenn

Re: [PATCH v2] loopback: Do not automaticaly replace existing loopback dev, error instead

[Daniel Kiper]

On Mon, Dec 07, 2020 at 10:52:16PM -0600, Glenn Washburn wrote:
> On Fri, 4 Dec 2020 13:34:44 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Thu, Dec 03, 2020 at 07:57:11PM -0600, Glenn Washburn wrote:
> > > If there is a loopback device with the same name as the one to be
> > > created, instead of closing the old one and replacing it with the
> > > new one, return an error instead. If the loopback device was
> > > created, its probably being used by something and just replacing it
> > > may cause grub to crash unexpectedly. This fixes obvious problems
> > > like `loopback d (d)/somefile'. Its not too onerous to force the
> > > user to delete the loopback first with the `-d' switch.
> > >
> > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> >
> > Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> >
> > Daniel
> >
> > PS May I ask you to create new thread for new version of the patches
> >    instead of attaching them to previous threads?
>
> These two patches were not meant to be a thread together, ie separate
> patches, which was why I was treating them separately.  I can see how
> that might be confusing now.

I was also referring to the LUKS2 patches, sorry if I was not precise,
which are always connected to the first thread created. This is confusing
too. Could you create separate thread for each version of patchset in
the future?

Daniel

Re: [PATCH v2] loopback: Do not automaticaly replace existing loopback dev, error instead

[Glenn Washburn]

On Thu, 10 Dec 2020 17:55:20 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Mon, Dec 07, 2020 at 10:52:16PM -0600, Glenn Washburn wrote:
> > On Fri, 4 Dec 2020 13:34:44 +0100
> > Daniel Kiper <dkiper@net-space.pl> wrote:
> >
> > > On Thu, Dec 03, 2020 at 07:57:11PM -0600, Glenn Washburn wrote:
> > > > If there is a loopback device with the same name as the one to
> > > > be created, instead of closing the old one and replacing it
> > > > with the new one, return an error instead. If the loopback
> > > > device was created, its probably being used by something and
> > > > just replacing it may cause grub to crash unexpectedly. This
> > > > fixes obvious problems like `loopback d (d)/somefile'. Its not
> > > > too onerous to force the user to delete the loopback first with
> > > > the `-d' switch.
> > > >
> > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > >
> > > Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> > >
> > > Daniel
> > >
> > > PS May I ask you to create new thread for new version of the
> > > patches instead of attaching them to previous threads?
> >
> > These two patches were not meant to be a thread together, ie
> > separate patches, which was why I was treating them separately.  I
> > can see how that might be confusing now.
> 
> I was also referring to the LUKS2 patches, sorry if I was not precise,
> which are always connected to the first thread created. This is
> confusing too. Could you create separate thread for each version of
> patchset in the future?

I was keeping them together because that was its easy to find the
comments from the previous version of the patch series to make sure
things weren't forgotten. Honestly, with more experience with
submitting patches this way, what I like best is to always send a new
version of a patch series in reply to the very first cover letter, so
each new version of a series is at the same thread depth. In my viewer,
I can collapse the thread to the patch series version when I don't need
to see anything in that version and the versions are ordered in
alphabetical order. I only started doing this with the LUKS patches
after v4, so its still confusing because there's a thread depth of 6 or
so before each version is at the same thread depth. Perhaps, its not a
big deal to have versions of patch series connected with previous
versions. I'll create a new thread for the next LUKS patch series. On
further series, I may consistently use the above method (unlike the
current LUKS thread) and you can tell me if you still don't like it.

Glenn