| Message ID | 20210131103856.80651-1-xypron.glpk@gmx.de |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [1/1] sandbox: host bind must close file descriptor | expand |
Hi Heinrich, On Sun, 31 Jan 2021 at 03:39, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > Each invocation of the 'host bind' command with a file name argument opens > a file descriptor. The next invocation of the 'host bind' command destroys > the block device but the file descriptor remains open. The same holds true > for the 'unbind blk' command. > > Close the file descriptor when unbinding the host block device. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > drivers/block/sandbox.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/block/sandbox.c b/drivers/block/sandbox.c > index f57f690d3c..02992ac34f 100644 > --- a/drivers/block/sandbox.c > +++ b/drivers/block/sandbox.c > @@ -230,6 +230,25 @@ int host_get_dev_err(int devnum, struct blk_desc **blk_devp) > } > > #ifdef CONFIG_BLK > + > +int sandbox_host_unbind(struct udevice *dev) > +{ > + struct host_block_dev *host_dev; > + > + host_dev = dev_get_plat(dev); > + if (host_dev) { > + if (host_dev->fd) { > + os_close(host_dev->fd); > + host_dev->fd = 0; > + } else { > + log_err("missing file descriptor\n"); How does this happen? > + } > + } else { > + log_err("missing platform data\n"); I don't think this case can happen. See the .plat_auto below. > + } > + return 0; > +} > + > static const struct blk_ops sandbox_host_blk_ops = { > .read = host_block_read, > .write = host_block_write, > @@ -239,6 +258,7 @@ U_BOOT_DRIVER(sandbox_host_blk) = { > .name = "sandbox_host_blk", > .id = UCLASS_BLK, > .ops = &sandbox_host_blk_ops, > + .unbind = sandbox_host_unbind, > .plat_auto = sizeof(struct host_block_dev), > }; > #else > -- > 2.29.2 > Regards, Simon
On 2/1/21 9:44 PM, Simon Glass wrote: > Hi Heinrich, > > On Sun, 31 Jan 2021 at 03:39, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> Each invocation of the 'host bind' command with a file name argument opens >> a file descriptor. The next invocation of the 'host bind' command destroys >> the block device but the file descriptor remains open. The same holds true >> for the 'unbind blk' command. >> >> Close the file descriptor when unbinding the host block device. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> drivers/block/sandbox.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/drivers/block/sandbox.c b/drivers/block/sandbox.c >> index f57f690d3c..02992ac34f 100644 >> --- a/drivers/block/sandbox.c >> +++ b/drivers/block/sandbox.c >> @@ -230,6 +230,25 @@ int host_get_dev_err(int devnum, struct blk_desc **blk_devp) >> } >> >> #ifdef CONFIG_BLK >> + >> +int sandbox_host_unbind(struct udevice *dev) >> +{ >> + struct host_block_dev *host_dev; >> + >> + host_dev = dev_get_plat(dev); >> + if (host_dev) { >> + if (host_dev->fd) { >> + os_close(host_dev->fd); >> + host_dev->fd = 0; >> + } else { >> + log_err("missing file descriptor\n"); > > How does this happen? > >> + } >> + } else { >> + log_err("missing platform data\n"); > > I don't think this case can happen. See the .plat_auto below. Both would only happen if there were a bug in the rest of the code. Do you prefer assert_noisy() or should the messages be removed? Best regards Heinrich > >> + } >> + return 0; >> +} >> + >> static const struct blk_ops sandbox_host_blk_ops = { >> .read = host_block_read, >> .write = host_block_write, >> @@ -239,6 +258,7 @@ U_BOOT_DRIVER(sandbox_host_blk) = { >> .name = "sandbox_host_blk", >> .id = UCLASS_BLK, >> .ops = &sandbox_host_blk_ops, >> + .unbind = sandbox_host_unbind, >> .plat_auto = sizeof(struct host_block_dev), >> }; >> #else >> -- >> 2.29.2 >> > > Regards, > Simon >
Hi Heinrich, On Mon, 1 Feb 2021 at 14:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 2/1/21 9:44 PM, Simon Glass wrote: > > Hi Heinrich, > > > > On Sun, 31 Jan 2021 at 03:39, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> Each invocation of the 'host bind' command with a file name argument opens > >> a file descriptor. The next invocation of the 'host bind' command destroys > >> the block device but the file descriptor remains open. The same holds true > >> for the 'unbind blk' command. > >> > >> Close the file descriptor when unbinding the host block device. > >> > >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > >> --- > >> drivers/block/sandbox.c | 20 ++++++++++++++++++++ > >> 1 file changed, 20 insertions(+) > >> > >> diff --git a/drivers/block/sandbox.c b/drivers/block/sandbox.c > >> index f57f690d3c..02992ac34f 100644 > >> --- a/drivers/block/sandbox.c > >> +++ b/drivers/block/sandbox.c > >> @@ -230,6 +230,25 @@ int host_get_dev_err(int devnum, struct blk_desc **blk_devp) > >> } > >> > >> #ifdef CONFIG_BLK > >> + > >> +int sandbox_host_unbind(struct udevice *dev) > >> +{ > >> + struct host_block_dev *host_dev; > >> + > >> + host_dev = dev_get_plat(dev); > >> + if (host_dev) { > >> + if (host_dev->fd) { > >> + os_close(host_dev->fd); > >> + host_dev->fd = 0; > >> + } else { > >> + log_err("missing file descriptor\n"); > > > > How does this happen? > > > >> + } > >> + } else { > >> + log_err("missing platform data\n"); > > > > I don't think this case can happen. See the .plat_auto below. > > Both would only happen if there were a bug in the rest of the code. > > Do you prefer assert_noisy() or should the messages be removed? Well a bug in the core driver model is the only way the platform data can be missing. We have tests to protect against that. So I think that should be removed. For the file descriptor, I don't think we want a check against a bug in host_dev_bind(). Let's just make them consistent as you have (one sets it, one clears it), perhaps adding a comment if you like. Regards, Simon
diff --git a/drivers/block/sandbox.c b/drivers/block/sandbox.c index f57f690d3c..02992ac34f 100644 --- a/drivers/block/sandbox.c +++ b/drivers/block/sandbox.c @@ -230,6 +230,25 @@ int host_get_dev_err(int devnum, struct blk_desc **blk_devp) } #ifdef CONFIG_BLK + +int sandbox_host_unbind(struct udevice *dev) +{ + struct host_block_dev *host_dev; + + host_dev = dev_get_plat(dev); + if (host_dev) { + if (host_dev->fd) { + os_close(host_dev->fd); + host_dev->fd = 0; + } else { + log_err("missing file descriptor\n"); + } + } else { + log_err("missing platform data\n"); + } + return 0; +} + static const struct blk_ops sandbox_host_blk_ops = { .read = host_block_read, .write = host_block_write, @@ -239,6 +258,7 @@ U_BOOT_DRIVER(sandbox_host_blk) = { .name = "sandbox_host_blk", .id = UCLASS_BLK, .ops = &sandbox_host_blk_ops, + .unbind = sandbox_host_unbind, .plat_auto = sizeof(struct host_block_dev), }; #else
Each invocation of the 'host bind' command with a file name argument opens a file descriptor. The next invocation of the 'host bind' command destroys the block device but the file descriptor remains open. The same holds true for the 'unbind blk' command. Close the file descriptor when unbinding the host block device. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- drivers/block/sandbox.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) -- 2.29.2