[pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

newer
[pacman-dev] [PATCH] Add check for...

older
[pacman-dev] [PATCH]...

Allan McRae

22 Dec 2007 22 Dec '07

11:29 a.m.

Attachments:

0001-Add-remove-counterparts-to-alpm_option_add_-functio.patch (text/x-patch — 5.2 KB)

Show replies by date

Nagy Gabor

22 Dec 22 Dec

1:19 p.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

...

From: Allan McRae To: Discussion list for pacman development 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 User-Agent: Thunderbird 2.0.0.9 (X11/20071213)

...
From 0f9238992b6edd47efaf9b6647f416aab7b679f4 Mon Sep 17 00:00:00 2001 From: Allan McRae 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 --- 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

Dan McGee

2:14 p.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

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

Allan McRae

2:50 p.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

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

Nagy Gabor

3:12 p.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

...

... 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

Nagy Gabor

2:52 p.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

...

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

Dan McGee

3:13 p.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

On Dec 22, 2007 8:52 AM, Nagy Gabor 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

Allan McRae

4:49 p.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

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

Xavier

5:03 p.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

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); }

Dan McGee

6:42 p.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

On Dec 22, 2007 11:03 AM, Xavier 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

Allan McRae

23 Dec 23 Dec

2:43 a.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

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

Nagy Gabor

12:50 p.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

...

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

Cheers, Allan

Is newcachedir freed?;-) Bye

Allan McRae

10:39 p.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

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

24 Dec 24 Dec

12:23 a.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

...

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

Dan McGee

12:31 a.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

On Dec 23, 2007 6:23 PM, Nagy Gabor 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

Allan McRae

12:50 a.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

...

On Dec 23, 2007 6:23 PM, Nagy Gabor 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

Allan McRae

1:13 p.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

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

Nagy Gabor

12:20 p.m.

New subject: [pacman-dev] [PATCH] Add remove counterparts to alpm_option_add_* functions

...

On Dec 23, 2007 6:23 PM, Nagy Gabor 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!

5974

Age (days ago)

5976

Last active (days ago)

List overview

Download

17 comments

5 participants

participants (5)

Allan McRae
Allan McRae
Dan McGee
Nagy Gabor
Xavier