nvme: output nvme_alloc() error message

[ikegami-t]

Currently only the value -ENOMEM returned for the error. Also fixed some nvme_alloc() error handling.

[ikegami-t]

This related to the fix #2850.

[HaroPanosyan]

can these 4 lines be a macro call, something like this to save code lines (any name for macro):
Check_And_Return_If_Mem_Error( mem);

[ikegami-t]

Seems nvme_alloc() itself also can be added into the macro. By the way is there any actual example for the macro in nvme-cli, libnvme or linux kernel do you know? Thank you.

[HaroPanosyan]

What I am referring is to have a simple macro to eliminate lots of code with 1 liner. The macro would be:
#define SHOW_ERROR_AND_RETURN_IF_NO_MEM(mem)
if (!mem) {
nvme_show_error("failed to allocate aligned memory");
return -ENOMEM;
} \

end the code above will become:

SHOW_ERROR_AND_RETURN_IF_NO_MEM(smart_log)

or below
SHOW_ERROR_AND_RETURN_IF_NO_MEM(ctrl)

Adding nvme_alloc() to the macro, if possible, would make the code ugly, but you can try.

[ikegami-t]

The PR for the nvme_alloc() is based on the PR: #2850 for the issue: #2848 for the nvme_alloc_huge() error and just checked the PRs before then found the related PR: #2337 for -ENOMEM alloc error but closed the PR without merging. Also the PR was for the similar issue: #2336. So now I am thinking to close the PR as same.

[igaw]

That nvme_alloc_huge fails is not 0%. This is due to the type of allocation we do with it to: larg linear buffer allocation.

But in case of nvme_alloc this is very unlikely to fail. The Linux virtual memory manager normally does overcommiting (see /proc/sys/vm/overcommit_memory. That means malloc returns a pointer but when we try to use it, the OOT kills the process.

And we have many mallocs around where we don't print anything. I'd rather leave this as it is.

An idea I am toying with is that we just fail on allocation failures in nvme-cli. There is absolut no point in trying to recover from such scenario. E.g. we could use these hooks for malloc (https://www.gnu.org/software/libc/manual/html_node/Hooks-for-Malloc.html) and call abort. This would get rid of the error handling code, no one ever tests:

    ptr = malloc(sizeof(*ptr));
    if (!ptr)
        return -ENOMEM;

Though for libnvme this is not an option.

[ikegami-t]

Just fixed the patch to abort if malloc failed. About the macro for error return will do consider for other error cases later. Thank you.

[igaw]

The description needs to be updated.

[igaw]

I guess we need the same thing at least for calloc and realloc.

[igaw]

Thanks for looking into this. After looking at this again with fresh eyes, I think we don't gain alot with this approach. Because we can't really remove the p = malloc(...); if (p) { printf(...); return -1; } code. We still need it when the allocation fails.

That means it would make more sense to introduce nvme_malloc & Co which would check the allocation and update all call sites and remove the memory allocation checks. Basically like the idea from above but with function instead ugly macros.

Since the nvme-cli 2.x is entering the maintenance mode today this is something we could consider for nvme-cli3

[ikegami-t]

Thanks for your comment. I will do for nvme-cli3.

[igaw]

I don't think we need this macro as API. The whole point if this would be that we nvme-cli aborts directly when the allocation fails. Nothing should call this macro.

[ikegami-t]

just I thoght as we can use the macro for other pointer handling case but deleted the macro as mentioned. In future if really needed the macro I will do consider again to add. Thank you.

[ikegami-t]

Fixed the review comments. Thank you.