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