Re: [PATCH 5/8] util/compatfd.c: Replaced a malloc with GLib's variant

[Alex Bennée]

Thomas Huth <thuth@redhat.com> writes:

> On 15/03/2021 07.43, Mahmoud Mandour wrote:
>>     If it's unrelated, then maybe better do it in a separate patch.
>> I thought so but I didn't know whether it was a so-small change
>> that it didn't require its own patch or not. I will amend that.
>>     Since this is only a very small allocation, I think it would be
>> better to
>>     use g_malloc() here and then simply remove the "if (info == NULL) ..." 
>> part.
>> I was thinking of always maintaining the semantics of the existing
>> code and since g_malloc() does not behave like malloc() on
>> error, I refrained from using g_malloc() anywhere, but of course
>> I'll do it since it's the better thing to do.
>
> Keeping the semantics is normally a good idea, but the common sense in
> the QEMU project is to rather use g_malloc() for small allocations (if
> allocating some few bytes already fails, then the system is pretty
> much dead anyway), and only g_try_malloc() for huge allocations that
> really might fail on a healthy system, too.
>
> We should likely add some text to our coding style document to make
> this more obvious...

So while there are some places where we may try to dynamically scale the
memory we allocate on failure of a large allocation generally memory
allocation failure is considered fatal (ergo g_malloc, no NULL check).
However some care has to be taken depending on where we are - for
example calling abort() because something the guest did triggered us to
try an allocate more memory than we could is a no no.

We could certainly be clearer in style.rst though.

>> I will split the patches to a two-patch series regarding the
>> util/compactfd.c file (one for the style change and one for
>> changing the malloc() call into g_malloc()) and send them
>> again, is that ok?
>
> Sounds good, thanks!
>
>  Thomas


-- 
Alex Bennée