| Message ID | 20200424044044.33857-1-anatol.pomozov@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [pacman-dev] Add a helper macro to free() dload_payload structure | expand |
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); >
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?
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
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));
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
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.
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);
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(+)