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
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
12-22-2007, 01:50 PM
Allan McRae
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
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
12-22-2007, 02:13 PM
"Dan McGee"
Add remove counterparts to alpm_option_add_* functions
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
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
12-22-2007, 03:49 PM
Allan McRae
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?
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
12-22-2007, 05:42 PM
"Dan McGee"
Add remove counterparts to alpm_option_add_* functions
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
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
12-23-2007, 11:31 PM
"Dan McGee"
Add remove counterparts to alpm_option_add_* functions
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
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
Add remove counterparts to alpm_option_add_* functions
Both are fixed in attached
