On Wed, Jun 07, 2023 at 10:15:03AM +0200, David Disseldorp wrote:
> Thanks for the feedback, Alan...
>
> On Tue, 6 Jun 2023 18:33:22 -0400, Alan Stern wrote:
>
> > On Wed, Jun 07, 2023 at 12:15:18AM +0200, David Disseldorp wrote:
> > > fsg_lun_is_open() will always and only be true after a successful
> > > fsg_lun_open() call, so drop the unnecessary conditional.
> >
> > I don't follow the reasoning. The relevant code is:
> >
> > if (cfg->filename) {
> > rc = fsg_lun_open(lun, cfg->filename);
> > if (rc)
> > goto error_lun;
> > }
> > ...
> > if (fsg_lun_is_open(lun)) {
> >
> > So at this point, the fsg_lun_is_open() condition is true iff
> > cfg->filename was set upon entry. What makes this test unnecessary?
>
> The fsg_lun_is_open() test is unnecessary as it will always be true
> following a successful fsg_lun_open() call and will always be false if
> cfg->filename is unset. This means that the logic from the
> fsg_lun_is_open() conditional block can be appended directly after the
> error_lun goto. pathbuf allocation ('...' above) is only needed for
> the open case, so can also be wrapped into the conditional block.
>
> I'd be happy to update the commit message if the explanation above
> makes things clearer. I should probably also mention that I've tested
> this using dummy-hcd.
Yes, please do that. All you have to say is that the fsg_lun_is_open()
test can be eliminated and the code merged with the preceding
conditional, because the LUN won't be open if cfg->filename wasn't set.
Alan Stern
