| Message ID | 20231108182431.4005745-1-thaller@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft,1/2] utils: add memory_allocation_check() helper | expand |
On Wed, Nov 08, 2023 at 07:24:24PM +0100, Thomas Haller wrote: > libnftables kills the process on out of memory (xmalloc()), so > when we use libraries that propagate ENOMEM to libnftables, we > also abort the process. > > For example: > > nlr = nftnl_rule_alloc(); > if (!nlr) > memory_allocation_error(); > > Add memory_allocation_check() macro which can simplify this common > check to: > > nlr = memory_allocation_check(nftnl_rule_alloc()); > > Signed-off-by: Thomas Haller <thaller@redhat.com> > --- > include/utils.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/utils.h b/include/utils.h > index 36a28f893667..fcd7c598fe9f 100644 > --- a/include/utils.h > +++ b/include/utils.h > @@ -142,6 +142,16 @@ extern void __memory_allocation_error(const char *filename, uint32_t line) __nor > #define memory_allocation_error() \ > __memory_allocation_error(__FILE__, __LINE__); > > +#define memory_allocation_check(cmd) \ > + ({ \ > + typeof((cmd)) _v = (cmd); \ > + const void *const _v2 = _v; \ > + \ > + if (!_v2) \ please don't hide a if branch inside a macro. > + memory_allocation_error(); \ > + _v; \ > + }) > + > extern void xfree(const void *ptr); > extern void *xmalloc(size_t size); > extern void *xmalloc_array(size_t nmemb, size_t size); > -- > 2.41.0 >
On Thu, 2023-11-09 at 16:24 +0100, Pablo Neira Ayuso wrote: > On Wed, Nov 08, 2023 at 07:24:24PM +0100, Thomas Haller wrote: > > > > +#define memory_allocation_check(cmd) \ > > + ({ \ > > + typeof((cmd)) _v = (cmd); \ > > + const void *const _v2 = _v; \ > > + \ > > + if (!_v2) \ > > please don't hide a if branch inside a macro. What is the reason for this rule? Is there a style guide somewhere? > > > + memory_allocation_error(); \ > > + _v; \ > > + }) It could be instead: static inline void *__memory_allocation_check(const char *file, unsigned line, const void *ptr) { if (!ptr) __memory_allocation_error(file, line); return (void*) ptr; } #define memory_allocation_check(cmd) \ ((typeof(cmd) __memory_allocation_check(__FILE__, __LINE__, (cmd)) Doesn't seem to make a difference either way. Thomas
Thomas Haller <thaller@redhat.com> wrote: > static inline void *__memory_allocation_check(const char *file, unsigned line, const void *ptr) { > if (!ptr) > __memory_allocation_error(file, line); > return (void*) ptr; > } > > #define memory_allocation_check(cmd) \ > ((typeof(cmd) __memory_allocation_check(__FILE__, __LINE__, (cmd)) > > Doesn't seem to make a difference either way. We seem to be moving in circles. I suspect your agenda is to avoid repeating the existing x = alloc() if (!x) barf() pattern when adding userhandle support? If so I think its best to just add a specific ubuf alloc wrapper that can't fail (i.e. like the 'xmalloc' wrappers). Like Pablo said, I don't see any added value in providing FILE/LINE errors on stderr here. It could be as simple as exit().
On Wed, 2023-11-15 at 09:52 +0100, Florian Westphal wrote: > Thomas Haller <thaller@redhat.com> wrote: > > static inline void *__memory_allocation_check(const char *file, > > unsigned line, const void *ptr) { > > if (!ptr) > > __memory_allocation_error(file, line); > > return (void*) ptr; > > } > > > > #define memory_allocation_check(cmd) \ > > ((typeof(cmd) __memory_allocation_check(__FILE__, __LINE__, > > (cmd)) > > > > Doesn't seem to make a difference either way. > > We seem to be moving in circles. > > I suspect your agenda is to avoid repeating the existing > > x = alloc() > if (!x) > barf() > > pattern when adding userhandle support? exactly. > > If so I think its best to just add a specific ubuf alloc wrapper that > can't fail (i.e. like the 'xmalloc' wrappers). I don't understand. _nftnl_udata_buf_alloc() *is* that specific wrapper. Maybe the name is bad... and such wrappers should have a "nft_x_" prefix (nft_x_nftnl_udata_buf_alloc()). > > Like Pablo said, I don't see any added value in providing FILE/LINE > errors on stderr here. It could be as simple as exit(). > I don't understand. The two patches don't change anything about that. There is no change in behavior -- aside introducing a convenience wrapper for the repeated ENOMEM check. If you want to change memory_allocation_error() to not use FILE/LINE or call exit(), then that is a separate discussion (I'd like to avoid). Thomas
diff --git a/include/utils.h b/include/utils.h index 36a28f893667..fcd7c598fe9f 100644 --- a/include/utils.h +++ b/include/utils.h @@ -142,6 +142,16 @@ extern void __memory_allocation_error(const char *filename, uint32_t line) __nor #define memory_allocation_error() \ __memory_allocation_error(__FILE__, __LINE__); +#define memory_allocation_check(cmd) \ + ({ \ + typeof((cmd)) _v = (cmd); \ + const void *const _v2 = _v; \ + \ + if (!_v2) \ + memory_allocation_error(); \ + _v; \ + }) + extern void xfree(const void *ptr); extern void *xmalloc(size_t size); extern void *xmalloc_array(size_t nmemb, size_t size);
libnftables kills the process on out of memory (xmalloc()), so when we use libraries that propagate ENOMEM to libnftables, we also abort the process. For example: nlr = nftnl_rule_alloc(); if (!nlr) memory_allocation_error(); Add memory_allocation_check() macro which can simplify this common check to: nlr = memory_allocation_check(nftnl_rule_alloc()); Signed-off-by: Thomas Haller <thaller@redhat.com> --- include/utils.h | 10 ++++++++++ 1 file changed, 10 insertions(+)