> From: Allan McRae <allan.mcrae@qimr.edu.au>
> To: Discussion list for pacman development <pacman-dev@archlinux.org>
> Subject: [pacman-dev] [PATCH] Add remove counterparts to
> alpm_option_add_* functions Date: Sat, 22 Dec 2007 21:29:22 +1000
> Reply-To: Discussion list for pacman development
> <pacman-dev@archlinux.org> User-Agent: Thunderbird 2.0.0.9
> (X11/20071213)
>
>
>
> >From 0f9238992b6edd47efaf9b6647f416aab7b679f4 Mon Sep 17 00:00:00
> >2001
> From: Allan McRae <mcrae_allan@hotmail.com>
> Date: Sat, 22 Dec 2007 21:28:08 +1000
> Subject: [PATCH] Add remove counterparts to alpm_option_add_*
> functions
>
> Fixes FS#7428. Added functions to remove cachedir, noupgrade,
> noextract, ignorepkg, holdpkg and ignoregrp.
>
> Signed-off-by: Allan McRae <mcrae_allan@hotmail.com>
> ---
> lib/libalpm/alpm.h | 6 +++++
> lib/libalpm/handle.c | 55
> ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed,
> 61 insertions(+), 0 deletions(-)
>
> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> index 3a484be..e2d90fb 100644
> --- a/lib/libalpm/alpm.h
> +++ b/lib/libalpm/alpm.h
> @@ -104,6 +104,7 @@ int alpm_option_set_dbpath(const char *dbpath);
> alpm_list_t *alpm_option_get_cachedirs();
> int alpm_option_add_cachedir(const char *cachedir);
> void alpm_option_set_cachedirs(alpm_list_t *cachedirs);
> +void alpm_option_remove_cachedir(const char *cachedir);
>
> const char *alpm_option_get_logfile();
> int alpm_option_set_logfile(const char *logfile);
> @@ -117,22 +118,27 @@ void alpm_option_set_usesyslog(unsigned short
> usesyslog); alpm_list_t *alpm_option_get_noupgrades();
> void alpm_option_add_noupgrade(const char *pkg);
> void alpm_option_set_noupgrades(alpm_list_t *noupgrade);
> +void alpm_option_remove_noupgrade(const char *pkg);
>
> alpm_list_t *alpm_option_get_noextracts();
> void alpm_option_add_noextract(const char *pkg);
> void alpm_option_set_noextracts(alpm_list_t *noextract);
> +void alpm_option_remove_noextract(const char *pkg);
>
> alpm_list_t *alpm_option_get_ignorepkgs();
> void alpm_option_add_ignorepkg(const char *pkg);
> void alpm_option_set_ignorepkgs(alpm_list_t *ignorepkgs);
> +void alpm_option_remove_ignorepkg(const char *pkg);
>
> alpm_list_t *alpm_option_get_holdpkgs();
> void alpm_option_add_holdpkg(const char *pkg);
> void alpm_option_set_holdpkgs(alpm_list_t *holdpkgs);
> +void alpm_option_remove_holdpkg(const char *pkg);
>
> alpm_list_t *alpm_option_get_ignoregrps();
> void alpm_option_add_ignoregrp(const char *grp);
> void alpm_option_set_ignoregrps(alpm_list_t *ignoregrps);
> +void alpm_option_remove_ignoregrp(const char *grp);
>
> time_t alpm_option_get_upgradedelay();
> void alpm_option_set_upgradedelay(time_t delay);
> diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c
> index ab6fc7e..4359c91 100644
> --- a/lib/libalpm/handle.c
> +++ b/lib/libalpm/handle.c
> @@ -395,6 +395,26 @@ void SYMEXPORT
> alpm_option_set_cachedirs(alpm_list_t *cachedirs) if(cachedirs)
> handle->cachedirs = cachedirs; }
>
> +void SYMEXPORT alpm_option_remove_cachedir(const char *cachedir)
> +{
> + void *vdata;
> + char *newcachedir;
> + size_t cachedirlen;
> +
> + /* verify cachedir ends in a '/' */
> + cachedirlen = strlen(cachedir);
> + if(cachedir[cachedirlen-1] != '/') {
> + cachedirlen += 1;
> + }
> + newcachedir = calloc(cachedirlen + 1, sizeof(char));
> + strncpy(newcachedir, cachedir, cachedirlen);
> + newcachedir[cachedirlen-1] = '/';
> +
> + handle->cachedirs = alpm_list_remove(handle->cachedirs,
> newcachedir,
> + _alpm_str_cmp, &vdata);
> +
> +}
> +
> int SYMEXPORT alpm_option_set_logfile(const char *logfile)
> {
> char *oldlogfile = handle->logfile;
> @@ -436,6 +456,13 @@ void SYMEXPORT
> alpm_option_set_noupgrades(alpm_list_t *noupgrade) if(noupgrade)
> handle->noupgrade = noupgrade; }
>
> +void SYMEXPORT alpm_option_remove_noupgrade(const char *pkg)
> +{
> + void *vdata;
> + handle->noupgrade = alpm_list_remove(handle->noupgrade,
> strdup(pkg),
> + _alpm_str_cmp, &vdata);
> +}
> +
> void SYMEXPORT alpm_option_add_noextract(const char *pkg)
> {
> handle->noextract = alpm_list_add(handle->noextract,
> strdup(pkg)); @@ -447,6 +474,13 @@ void SYMEXPORT
> alpm_option_set_noextracts(alpm_list_t *noextract) if(noextract)
> handle->noextract = noextract; }
>
> +void SYMEXPORT alpm_option_remove_noextract(const char *pkg)
> +{
> + void *vdata;
> + handle->noextract = alpm_list_remove(handle->noextract,
> strdup(pkg),
> + _alpm_str_cmp, &vdata);
> +}
> +
> void SYMEXPORT alpm_option_add_ignorepkg(const char *pkg)
> {
> handle->ignorepkg = alpm_list_add(handle->ignorepkg,
> strdup(pkg)); @@ -458,6 +492,13 @@ void
> alpm_option_set_ignorepkgs(alpm_list_t *ignorepkgs) if(ignorepkgs)
> handle->ignorepkg = ignorepkgs; }
>
> +void SYMEXPORT alpm_option_remove_ignorepkg(const char *pkg)
> +{
> + void *vdata;
> + handle->ignorepkg = alpm_list_remove(handle->ignorepkg,
> strdup(pkg),
> + _alpm_str_cmp, &vdata);
> +}
> +
> void SYMEXPORT alpm_option_add_holdpkg(const char *pkg)
> {
> handle->holdpkg = alpm_list_add(handle->holdpkg,
> strdup(pkg)); @@ -469,6 +510,13 @@ void SYMEXPORT
> alpm_option_set_holdpkgs(alpm_list_t *holdpkgs) if(holdpkgs)
> handle->holdpkg = holdpkgs; }
>
> +void SYMEXPORT alpm_option_remove_holdpkg(const char *pkg)
> +{
> + void *vdata;
> + handle->holdpkg = alpm_list_remove(handle->holdpkg,
> strdup(pkg),
> + _alpm_str_cmp, &vdata);
> +}
> +
> void SYMEXPORT alpm_option_add_ignoregrp(const char *grp)
> {
> handle->ignoregrp = alpm_list_add(handle->ignoregrp,
> strdup(grp)); @@ -480,6 +528,13 @@ void
> alpm_option_set_ignoregrps(alpm_list_t *ignoregrps) if(ignoregrps)
> handle->ignoregrp = ignoregrps; }
>
> +void SYMEXPORT alpm_option_remove_ignoregrp(const char *grp)
> +{
> + void *vdata;
> + handle->ignoregrp = alpm_list_remove(handle->ignoregrp,
> strdup(grp),
> + _alpm_str_cmp, &vdata);
> +}
> +
> void SYMEXPORT alpm_option_set_upgradedelay(time_t delay)
> {
> handle->upgradedelay = delay;

Hmm. This patch seems useful, however it introduces some memleaks
(the "result" of alpm_list_remove in vdata should be freed, and we
don't need strdup(pkg) here). After fixing these, this patch can be
applied imho.

Bye

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

> Quick first thought here is these functions should not be void return
> type, but int so they can return whether they were successful or not.
> Of course, that provokes a second thought that notices that the list
> remove function has a rather odd function signature.

Well, I don't know. alpm_list_remove is one of our most reliable
functions, I cannot see any reason why it should fail (for example
alpm_list_add is not so reliable, because it must allocate some memory
for the new node header).
So if I understood correctly, you want to indicate whether the
to-be-removed element was found or not. I don't know if this is needed
or not, because as I said above we can guarantee, that if the
to-be-removed element exists, we remove it (one of them); and user
probably wants to remove an existing element.
But returning with your preferred integer doesn't hurt anything, so /me
shrugs.

> It looks like you could pass in a data pointer (set your vdata ptr to
> null first to be safe), and check after the remove call if something
> is in this pointer. If there was something, you actually have to free
> it (because of memory leak issues, which I just realized would
> happen). Finally, return true (1) if there was data returned, and
> false (0) if data was not found.

This data pointer is vdata in the patch (that's why I also said:
memleak).

> Thoughts from anyone else also welcome.
>
> -Dan
>

Bye

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

> ... It looks like there is a
> leak in db.c with the "data" variable not being cleared.
That is cleared by _alpm_db_unregister(db); [Not very suggestive, I
know ;-). And usage of pointercmp instead of _alpm_db_cmp would be
safer here, indeed.]

Bye

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

Updated patch attached without the memory leaks and adding return of
removal success.


Cheers,
Allan
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

> Updated patch attached without the memory leaks and adding return of
> removal success.
>
> Cheers,
> Allan

Is newcachedir freed?;-)
Bye

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

Nagy Gabor wrote:
Updated patch attached without the memory leaks and adding return of
removal success.


Cheers,
Allan



Is newcachedir freed?;-)
Bye


No... but it wasn't in alpm_option_add_cachedir() either and I was
trying for coding style consistency. Both are fixed in attached patch.
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

> Nagy Gabor wrote:
> >> Updated patch attached without the memory leaks and adding return
> >> of removal success.
> >>
> >> Cheers,
> >> Allan
> >>
> >
> > Is newcachedir freed?;-)
> > Bye
> >
> >
> No... but it wasn't in alpm_option_add_cachedir() either and I was
> trying for coding style consistency. Both are fixed in attached
> patch.
No. You mustn't free it in add_cachedir, because alpm_list_add just adds
a POINTER to the (handle->cachedirs) list. (So you can always think
alpm_list as a list of pointers.)

Bye

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

Dan McGee wrote:

On Dec 23, 2007 6:23 PM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:


No. You mustn't free it in add_cachedir, because alpm_list_add just adds
a POINTER to the (handle->cachedirs) list. (So you can always think
alpm_list as a list of pointers.)

Fixed in attachment. For some reason I thought there was a strdup in
the alpm_list_add statement but clearly there wasn't.



Ok, enough of this junk. Sorry to rain down on the holiday spirit, but
can we please not be cryptic on this mailing list with cute little
remarks and smiley faces?

Something like this would have saved us all 5 minutes of our time.
"I noticed that newcachedir was not freed in remove_cachedir- as far
as I can tell, it needs to be freed because we aren't adding it to a
list and it isn't needed outside of the function."

-Dan


Fair enough. This is probably the result of me being new to the code
base and making stupid trivial mistakes. However, point taken.


Allan
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

> On Dec 23, 2007 6:23 PM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
> > > Nagy Gabor wrote:
> > > >> Updated patch attached without the memory leaks and adding
> > > >> return of removal success.
> > > >>
> > > >> Cheers,
> > > >> Allan
> > > >>
> > > >
> > > > Is newcachedir freed?;-)
> > > > Bye
> > > >
> > > >
> > > No... but it wasn't in alpm_option_add_cachedir() either and I was
> > > trying for coding style consistency. Both are fixed in
> > > attached patch.
> > No. You mustn't free it in add_cachedir, because alpm_list_add just
> > adds a POINTER to the (handle->cachedirs) list. (So you can always
> > think alpm_list as a list of pointers.)
>
> Ok, enough of this junk. Sorry to rain down on the holiday spirit, but
> can we please not be cryptic on this mailing list with cute little
> remarks and smiley faces?
>
> Something like this would have saved us all 5 minutes of our time.
> "I noticed that newcachedir was not freed in remove_cachedir- as far
> as I can tell, it needs to be freed because we aren't adding it to a
> list and it isn't needed outside of the function."
>
> -Dan
>

1. If you refer to "Is newcachedir freed?;-)", then you should
have answered to my previous mail, because we had an _other_ bug here.

2. These "junks" were addressed to Allen, who fortunately understood
them. He is new to the code (as he said), and I thought he need other
information than you/us (alpm_list_t explanation). But if this is
forbidden here, next time I will write a direct e-mail to him or be
silent.

Merry Christmas!

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

This should (hopefully) be the final incarnation of this patch... I
have removed even more of the unneeded strdup's that Nagy initially
pointed out.


Cheers,
Allan
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev