[pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions
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
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. 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. Thoughts from anyone else also welcome. -Dan
Dan McGee wrote:
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.
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.
Thoughts from anyone else also welcome.
-Dan OK. That is easy enough to do. I followed the only other uses of alpm_list_remove (in libalpm/{cache,db}.c). It looks like there is a leak in db.c with the "data" variable not being cleared. In cache.c, "vdata" gets freed in _alpm_db_remove_pkgfromcache as it is assigned to "data" which is freed. I don't get why "data" is not used in alpm_list_remove in the first place - there could be something about C I am missing there as I only really know C++. Anyway, I can tidy these up tomorrow.
Allan
... 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
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
On Dec 22, 2007 8:52 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
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.
Um...I want to indicate *whether or not the operation was successful*. That in turn does mean that we have removed an item from ignorepkg, holdpkg, etc. That seems perfectly reasonable to me. I'm confused why you would ever want to hid that information from an end user. If they don't want it, just don't check it at all. I couldn't give a crap whether a certain function is "reliable" or whatever. That has NO bearing whatsoever on how another function should work. And on Linux at least, unless you have set up your kernel otherwise, a malloc call never fails. -Dan
Before I resubmit an updated patch with all the functions updated, is this function better? Specifically, do I free vdata correctly? int SYMEXPORT alpm_option_remove_noupgrade(const char *pkg) { void *vdata = NULL; handle->noupgrade = alpm_list_remove(handle->noupgrade, pkg, _alpm_str_cmp, &vdata); if(vdata != NULL) { FREELIST(vdata); return 1; } return 0; } Cheers, Allan
Allan McRae wrote:
Before I resubmit an updated patch with all the functions updated, is this function better? Specifically, do I free vdata correctly?
int SYMEXPORT alpm_option_remove_noupgrade(const char *pkg) { void *vdata = NULL; handle->noupgrade = alpm_list_remove(handle->noupgrade, pkg, _alpm_str_cmp, &vdata); if(vdata != NULL) { FREELIST(vdata); return 1; } return 0; }
If vdata is just a string (not a list), just do : free(vdata); Or you can also do FREE(vdata), which is equivalent to: free(vdata); vdata = NULL; Also, following pacman syntax, it would rather look like : int SYMEXPORT alpm_option_remove_noupgrade(const char *pkg) { void *vdata = NULL; handle->noupgrade = alpm_list_remove(handle->noupgrade, pkg, _alpm_str_cmp, &vdata); if(vdata != NULL) { FREE(vdata); return(1); } return(0); }
On Dec 22, 2007 11:03 AM, Xavier <shiningxc@gmail.com> wrote:
Allan McRae wrote:
Before I resubmit an updated patch with all the functions updated, is this function better? Specifically, do I free vdata correctly?
int SYMEXPORT alpm_option_remove_noupgrade(const char *pkg) { void *vdata = NULL; handle->noupgrade = alpm_list_remove(handle->noupgrade, pkg, _alpm_str_cmp, &vdata); if(vdata != NULL) { FREELIST(vdata); return 1; } return 0; }
If vdata is just a string (not a list), just do : free(vdata);
Or you can also do FREE(vdata), which is equivalent to: free(vdata); vdata = NULL;
Also, following pacman syntax, it would rather look like :
int SYMEXPORT alpm_option_remove_noupgrade(const char *pkg) { void *vdata = NULL; handle->noupgrade = alpm_list_remove(handle->noupgrade, pkg, _alpm_str_cmp, &vdata); if(vdata != NULL) { FREE(vdata); return(1); } return(0);
}
Yep, this looks good, and I don't see any memleaks anymore. -Dan
Updated patch attached without the memory leaks and adding return of removal success. Cheers, Allan
Updated patch attached without the memory leaks and adding return of removal success.
Cheers, Allan
Is newcachedir freed?;-) Bye
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.
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
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
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
Dan McGee wrote: 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
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
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!
participants (5)
-
Allan McRae
-
Allan McRae
-
Dan McGee
-
Nagy Gabor
-
Xavier