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