| Message ID | 1f320bb0c1e11dbe441dd44eac006873de5f267c.1698674332.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs/219 cloned-device mount capability update | expand |
On Mon, Oct 30, 2023 at 2:15 PM Anand Jain <anand.jain@oracle.com> wrote: > > Check if the dev arg1 is set before calling losetup -d on it. Why? Do we have any callers that call the function without an argument? More comments below. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > common/rc | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/common/rc b/common/rc > index 18d2ddcf8e35..e7d6801b20e8 100644 > --- a/common/rc > +++ b/common/rc > @@ -4150,7 +4150,10 @@ _create_loop_device() > _destroy_loop_device() > { > local dev=$1 > - losetup -d $dev || _fail "Cannot destroy loop device $dev" > + > + if [ ! -z $dev ]; then > + losetup -d $dev || _fail "Cannot destroy loop device $dev" > + fi So this is just ignoring if no argument is given and the function does nothing. This is quite the opposite of everywhere else, where we error out if a necessary argument is missing. btrfs/219 never calls this function without the argument, both before and after this patchset, so I don't see why this patch is needed. If we have any callers not passing the argument, I'd rather fix them and make the function error out if no argument is given. Thanks. > } > > _scale_fsstress_args() > -- > 2.39.3 >
On 10/31/23 00:21, Filipe Manana wrote: > On Mon, Oct 30, 2023 at 2:15 PM Anand Jain <anand.jain@oracle.com> wrote: >> >> Check if the dev arg1 is set before calling losetup -d on it. > > Why? > > Do we have any callers that call the function without an argument? > More comments below. > >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> common/rc | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/common/rc b/common/rc >> index 18d2ddcf8e35..e7d6801b20e8 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -4150,7 +4150,10 @@ _create_loop_device() >> _destroy_loop_device() >> { >> local dev=$1 >> - losetup -d $dev || _fail "Cannot destroy loop device $dev" >> + >> + if [ ! -z $dev ]; then >> + losetup -d $dev || _fail "Cannot destroy loop device $dev" >> + fi > > So this is just ignoring if no argument is given and the function does nothing. > This is quite the opposite of everywhere else, where we error out if a > necessary argument is missing. > > btrfs/219 never calls this function without the argument, > both before > and after this patchset, so I don't see why this patch is needed. You are right. We no longer need this patch. In version 2, we had the last mount called with or without temp-fsid. That's being removed, and initialization is taken care of, so we can drop this patch. I'll drop this patch. > If we have any callers not passing the argument, I'd rather fix them > and make the function error out if no argument is given. Thanks, Anand
diff --git a/common/rc b/common/rc index 18d2ddcf8e35..e7d6801b20e8 100644 --- a/common/rc +++ b/common/rc @@ -4150,7 +4150,10 @@ _create_loop_device() _destroy_loop_device() { local dev=$1 - losetup -d $dev || _fail "Cannot destroy loop device $dev" + + if [ ! -z $dev ]; then + losetup -d $dev || _fail "Cannot destroy loop device $dev" + fi } _scale_fsstress_args()
Check if the dev arg1 is set before calling losetup -d on it. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- common/rc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)