tree: fix mem leaks in multiple nvme ctrl routines

[martin-gpy]

Valgrind revealed memory leaks in multiple nvme ctrl routines including nvme_configure_ctrl(), nvme_ctrl_alloc() & nvme_scan_ctrl(). This was because previously allocated memory was not freed before assigning new attr values to the respective nvme_ctrl struct members in these routines. Fix the same.

[martin-belanger]

free() is NULL-safe. So you could just call it w/o first cheking that the pointer in non-NULL.

https://man7.org/linux/man-pages/man3/free.3p.html

[martin-gpy]

Ok. Guess this should be cleaned up in other parts of the code too.

[igaw]

I don't think these function should be exposed at all.

[martin-gpy]

Are you saying these pointers should be freed within nvme_configure_ctrl() & nvme_ctrl_alloc() itself? That would make the code ugly to read though...

[igaw]

I am still reading through the code. Not sure what is best, but I really don't want these setters in the public API.

[igaw]

nvme_configure_ctrl is called either in nvme_init_ctrl or nvme_ctrl_alloc. Hmm, I thought these function should only initialize the ctrl object once.

[martin-gpy]

nvme_configure_ctrl is called either in nvme_init_ctrl or nvme_ctrl_alloc. Hmm, I thought these function should only initialize the ctrl object once.

So how should we proceed here? Do you want me to resubmit after addressing @martin-belanger's comment above? Or avoid exposing these new ctrl functions altogether and free the individual ctrl poiners within nvme_configure_ctrl() & nvme_ctrl_alloc() itself. Please clarify.

[igaw]

So how should we proceed here? Do you want me to resubmit after addressing @martin-belanger's comment above? Or avoid exposing these new ctrl functions altogether and free the individual ctrl poiners within nvme_configure_ctrl() & nvme_ctrl_alloc() itself. Please clarify.

I haven't had enough time to look into it yet. The issue I have is that all those function have been written with the assumption that we only initialize a ctrl object once. But doesn't seem the case. Thus I want to figure out if there is a fundamental problem which we need to address. I don't want to paper over design issues.

[igaw]

unrelated.

[martin-gpy]

Why unrelated? path already gets freed and set to NULL in the helper function in my patch. So there was no need to separately set it to NULL here to avoid getting it freed through the cleanup macro. Otherwise this would have resulted in a double free.

[igaw]

Ah didn't realize there is the cleanup hook. Hmm I think we should avoid passing owner ship like this around, it leads to more problems in the long run. If nvme_ctrl_alloc needs the path variable it should duplicate it. Let me look into it.

[igaw]

We already have nvme_deconfigure_ctrl which does all the freeing. Let's just use this one in nvme_configure_ctrl with a comment explaining why it is necessary to free stuff first.

In nvme_ctrl_alloc use the FREE_CTRL_ATTR to stay consistent.

[igaw]

urgh, this doesn't work either, because nvme_alloc_ctrl is doing a lookup... this is so an antipattern. we should have never done it this way. let me see how to resolve this.