On Mon, 2020-08-10 at 14:07 -0500, Benjamin Marzinski wrote: > On Mon, Aug 10, 2020 at 02:20:27PM +0200, Martin Wilck wrote: > > Hello Liu, > > > > On Fri, 2020-07-24 at 09:40 +0800, Zhiqiang Liu wrote: > > > In disassemble_map func, one pp will be allocated and stored in > > > pgp->paths. However, if store_path fails, pp will not be freed, > > > then memory leak problem occurs. > > > > > > Here, we will call free_path to free pp when store_path fails. > > > > > > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@xxxxxxxxxx> > > > Signed-off-by: lixiaokeng <lixiaokeng@xxxxxxxxxx> > > > --- > > > V1->V2: update based on ups/submit-2007 branch. > > > > > > libmultipath/dmparser.c | 9 +++++++-- > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > thanks for the patch. I'd suggest to do this without the > > pp_alloc_flag > > variable, by pulling the store_path() call into the if (!pp) > > conditional and treating failure differently in both branches of > > the > > conditional. (Side note: if we introduce a boolean like this, we > > should > > use "bool" as the variable type). > > > > Unless you object, I'll add that change on top of my submit-2007 > > series, giving you appropriate credits. > > > > @Ben, @Christophe: I've been considering for some time to start > > handling cases like this with __attribute__((cleanup(f)))) ( > > https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html) > > . > > That could reduce the amount of goto clauses for error handling > > significantly. It would be a major change in coding style though. > > What > > do you think? > > So, I haven't really looked into the cleanup attribute. How would it > work here? We only want to free the path if we didn't store it. We > can't > free it if it got stored. Can you disable the cleanup? If we have to > make the cleanup function check if the path got stored, that seems > like > more work than simply using the multiple goto labels like we do now. I need to think it through in more detail for this particular use case. The general pattern makes use of the steal_ptr() macro, and the fact that free(NULL) is a noop: #define steal_ptr(x) ({ void *__p = x; x = NULL; __p; }) struct xyz; void cleanup_xyz(struct xyz **p) { free(*p); } struct xyz *func() { struct xyz *xy __attribute__((cleanup(cleanup_xyz))) = NULL; xy = alloc_xyz(); if (!xy) return; if (do_something_with(xy) == SUCCESS) /* avoid freeing xy by setting it to NULL */ return steal_ptr(xy); else /* xy remains non-NULL and will be freed on return */ return NULL; } More often than not, with this technique, gotos can be avoided completely, and the readability is IMO improved. systemd uses this pattern a lot. Martin -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
