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

Message ID 20200424044044.33857-1-anatol.pomozov@gmail.com
State New
Headers show
Series
  • [pacman-dev] Add a helper macro to free() dload_payload structure
Related show

Commit Message

Anatol Pomozov April 24, 2020, 4:40 a.m. UTC
It frees all the dynamically allocated fields plus the struct itself

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 lib/libalpm/dload.h | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Allan McRae April 26, 2020, 11:54 p.m. UTC | #1
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?

We usually #unset defines not used globally too.


> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
>  lib/libalpm/dload.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
> index a40b51b7..3f2fb9ea 100644
> --- a/lib/libalpm/dload.h
> +++ b/lib/libalpm/dload.h
> @@ -56,6 +56,11 @@ void _alpm_dload_payload_reset_for_retry(struct dload_payload *payload);
>  int _alpm_download(struct dload_payload *payload, const char *localpath,
>  		char **final_file, const char **final_url);
>  
> +#define DLOAD_PAYLOAD_FREE(payload) { \
> +	_alpm_dload_payload_reset(payload); \
> +	FREE(payload); \
> +}
> +
>  int _alpm_multi_download(alpm_handle_t *handle,
>  		alpm_list_t *payloads /* struct dload_payload */,
>  		const char *localpath);
>
Anatol Pomozov April 27, 2020, 1:54 a.m. UTC | #2
Hi

On Sun, Apr 26, 2020 at 4:54 PM Allan McRae <allan@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));

> 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?
Allan McRae April 27, 2020, 2:18 a.m. UTC | #3
On 27/4/20 11:54 am, Anatol Pomozov wrote:
> Hi
> 
> On Sun, Apr 26, 2020 at 4:54 PM Allan McRae <allan@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 .

Allan
Anatol Pomozov April 27, 2020, 2:24 a.m. UTC | #4
Hi

On Sun, Apr 26, 2020 at 7:18 PM Allan McRae <allan@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@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));
Allan McRae April 29, 2020, 12:55 a.m. UTC | #5
On 27/4/20 12:24 pm, Anatol Pomozov wrote:
> Hi
> 
> On Sun, Apr 26, 2020 at 7:18 PM Allan McRae <allan@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@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.

Allan
Anatol Pomozov May 5, 2020, 10:58 p.m. UTC | #6
Hi

On Tue, Apr 28, 2020 at 5:56 PM Allan McRae <allan@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@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@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.

Patch

diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
index a40b51b7..3f2fb9ea 100644
--- a/lib/libalpm/dload.h
+++ b/lib/libalpm/dload.h
@@ -56,6 +56,11 @@  void _alpm_dload_payload_reset_for_retry(struct dload_payload *payload);
 int _alpm_download(struct dload_payload *payload, const char *localpath,
 		char **final_file, const char **final_url);
 
+#define DLOAD_PAYLOAD_FREE(payload) { \
+	_alpm_dload_payload_reset(payload); \
+	FREE(payload); \
+}
+
 int _alpm_multi_download(alpm_handle_t *handle,
 		alpm_list_t *payloads /* struct dload_payload */,
 		const char *localpath);