[PATCH 1/1] sandbox: host bind must close file descriptor

[Simon Glass]

Hi Heinrich,

On Mon, 1 Feb 2021 at 14:08, Heinrich Schuchardt <xypron.glpk at 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 at 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 at 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