On wed, 06 Feb 2013 13:14:23 +0100, Arne Jansen wrote: > Hi Alex, > > On 02/06/13 12:18, Alex Lyakas wrote: >> Hi Jan, Arne, >> I see this code in create_snapshot: >> >> if (inherit) { >> pending_snapshot->inherit = *inherit; >> *inherit = NULL; /* take responsibility to free it */ >> } >> >> So, first thing I think it should be: >> if (*inherit) >> because in btrfs_ioctl_snap_create_v2() we have: >> struct btrfs_qgroup_inherit *inherit = NULL; >> ... >> btrfs_ioctl_snap_create_transid(..., &inherit) >> >> so the current check is very unlikely to be NULL. > > But in btrfs_ioctl_snap_create it is called with NULL, so *inherit would > dereference a NULL pointer. > >> >> Second, I don't see anybody freeing pending_snapshot->inherit. I guess >> it should be freed after callin btrfs_qgroup_inherit() and also in >> btrfs_destroy_pending_snapshots(). > > You're right. In our original version (6f72c7e20dbaea5) it was still > there, in transaction.c. It has been removed in 6fa9700e734: > > commit 6fa9700e734275de2acbcb0e99414bd7ddfc60f1 > Author: Miao Xie <mi...@cn.fujitsu.com> > Date: Thu Sep 6 04:00:32 2012 -0600 > > Btrfs: fix error path in create_pending_snapshot() > > This patch fixes the following problem: > - If we failed to deal with the delayed dir items, we should abort > transaction, > just as its comment said. Fix it. > - If root reference or root back reference insertion failed, we should > abort transaction. Fix it. > - Fix the double free problem of pending->inherit. > - Do not restore the trans->rsv if we doesn't change it. > - make the error path more clearly. > > Signed-off-by: Miao Xie <mi...@cn.fujitsu.com> > > Miao, can you please explain where you see a double free?
Sorry, I misread the code,I didn't notice that the pointer had been assigned to NULL. But I think we can make the code more readable and be easy to maintain, we can free the memory in the caller(btrfs_ioctl_snap_create_v2()) since we are sure the snapshot creation is done after btrfs_ioctl_snap_create_transid() completes. Thanks Miao -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html