On Wed, Sep 12, 2018 at 11:33:11AM -0700, Ori Bernstein wrote: > On Wed, 12 Sep 2018 15:36:32 +0800 > Michael Mikonos <[email protected]> wrote: > > > On Wed, Sep 12, 2018 at 12:13:35AM -0700, Ori Bernstein wrote: > > > On Tue, 11 Sep 2018 23:29:53 -0700, Ori Bernstein <[email protected]> > > > wrote: > > > > > > > static ssize_t > > > > @@ -362,8 +377,9 @@ qc2_close(void *p) > > > > struct qcdisk *disk; > > > > > > > > disk = p; > > > > - pwrite(disk->fd, disk->l1, disk->l1sz, disk->l1off); > > > > - close(disk->fd); > > > > + if (disk->base) > > > > + qc2_close(disk->base); > > > > + free(disk->l1); > > > > free(disk); > > > > } > > > > > > > > > > Er, of course, the close should still exist here. > > > > I was curious about qc2_close() calling itself for disk->base. > > We open it recursively for multi-disk snapshots, where the disk > specifies the base to write deltas off of. We should close it > the same way. > > > Also, is it worth setting disk->fd = -1 after close(), and > > adding a check to prevent closing it again? > > The data is in a malloc'ed structure we're freeing on the next > line. I'm not sure that there's much point in setting the fd > to -1 and checking it: if we try closing the same fd again, we've > got bigger problems.
Thanks for the explanation. Overall it seems better having the clean-up code localised in qc2_close() so I'm happy to OK the patch with the close() call put back in (could you please forward the updated patch to the list in case someone else wants to look). > > -- > Ori Bernstein <[email protected]>
