[pacman-dev] [PATCH] Add a helper macro to free() dload_payload structure

[Anatol Pomozov]

Hi

On Tue, Apr 28, 2020 at 5:56 PM Allan McRae <allan at archlinux.org> wrote:
>
> On 27/4/20 12:24 pm, Anatol Pomozov wrote:
> > Hi
> >
> > On Sun, Apr 26, 2020 at 7:18 PM Allan McRae <allan at archlinux.org> wrote:
> >>
> >> On 27/4/20 11:54 am, Anatol Pomozov wrote:
> >>> Hi
> >>>
> >>> On Sun, Apr 26, 2020 at 4:54 PM Allan McRae <allan at archlinux.org> wrote:
> >>>>
> >>>> On 24/4/20 2:40 pm, Anatol Pomozov wrote:
> >>>>> It frees all the dynamically allocated fields plus the struct itself
> >>>>>
> >>>>
> >>>> How many times will you use this?   Across how many functions?
> >>>
> >>> Currently my tree uses this macro 6 times. All of them in error
> >>> codepath like this one:
> >>>
> >>> STRDUP(payload->fileurl, url,
> >>>     DLOAD_PAYLOAD_FREE(payload); GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> >>>
> >>> An alternative to it is to inline DLOAD_PAYLOAD_FREE() macro into the
> >>> STRDUP parameter.
> >>> But it might look too verbose in this use-case:
> >>>
> >>> STRDUP(payload->fileurl, url,
> >>>     _alpm_dload_payload_reset(payload);  FREE(payload);
> >>>     GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> >>>
> >>
> >> OK - that seems fine.
> >>
> >>>> We usually #unset defines not used globally too.
> >>>
> >>> I am not sure I understand this #unset requirement. Could you please
> >>> give an example how it should work here?
> >>>
> >>
> >> I was trying to establish of the usage of this define would be all in
> >> one function.  Or is it needed in multiple.
> >>
> >> If it was in one function, we can #define it in the function and #undef
> >> it at the end.  There are examples in libalpm/hook.c and util.c .
> >
> > Currently this macro is used in 3 different files:
> >
> > lib/libalpm/be_sync.c:192:
> > DLOAD_PAYLOAD_FREE(payload); GOTO_ERR(handle, ALPM_ERR_MEMORY,
> > cleanup));
> > lib/libalpm/be_sync.c:208:
> > DLOAD_PAYLOAD_FREE(sig_payload); GOTO_ERR(handle, ALPM_ERR_MEMORY,
> > cleanup));
> > lib/libalpm/dload.c:825:
> > DLOAD_PAYLOAD_FREE(payload); GOTO_ERR(handle, ALPM_ERR_MEMORY, err));
> > lib/libalpm/dload.h:59:#define DLOAD_PAYLOAD_FREE(payload) { \
> > lib/libalpm/sync.c:741:
> > DLOAD_PAYLOAD_FREE(payload); GOTO_ERR(handle, ALPM_ERR_MEMORY,
> > finish));
> > lib/libalpm/sync.c:743:
> > DLOAD_PAYLOAD_FREE(payload); GOTO_ERR(handle, ALPM_ERR_MEMORY,
> > finish));
> > lib/libalpm/sync.c:766:
> > DLOAD_PAYLOAD_FREE(sig_payload); GOTO_ERR(handle, ALPM_ERR_MEMORY,
> > finish));
> >
>
> Thanks - that is the context I needed.
>
> Please make this a two line _alpm_dload_payload_free() function, instead
> of a macro.  It will be optimised to the same thing by the compiler, and
> comes with type safety etc.  Also, I'd prefer not to have macros outside
> the utility functions in util.c.

Sure I can make it a function. Though an advantage of using macro here
is that it sets the variable to NULL the same way as FREE() macro
does. I found such defensive programming technique useful.