[pacman-dev] [PATCH 3/3] Remove rmrf function from frontend
* The alpm_rmrf function is available from the frontend, which does the same
as this function did.
* It was worth to modify _alpm_rmrf to alpm_rmrf for pacman frontend to be
able to use it in the future or for other frontend, so the function
declaration was moved into the api header file, named alpm.h, and the
definition of this function got a SYMEXPORT attribute.
* The rmrf funtion calling was changed in ./src/pacman/sync.c for alpm_rmrf
* The _alpm_rmrf functions were changed in api for alpm_rmrf
Signed-off-by: Laszlo Papp
On Sat, Oct 24, 2009 at 9:07 AM, Laszlo Papp
* The alpm_rmrf function is available from the frontend, which does the same as this function did.
* It was worth to modify _alpm_rmrf to alpm_rmrf for pacman frontend to be able to use it in the future or for other frontend, so the function declaration was moved into the api header file, named alpm.h, and the definition of this function got a SYMEXPORT attribute.
* The rmrf funtion calling was changed in ./src/pacman/sync.c for alpm_rmrf
* The _alpm_rmrf functions were changed in api for alpm_rmrf
Why would we do that ? alpm is not a general system utility. Besides this function is not as safe as it could be. http://bugs.archlinux.org/task/16363
On Sat, Oct 24, 2009 at 6:26 AM, Xavier
On Sat, Oct 24, 2009 at 9:07 AM, Laszlo Papp
wrote: * The alpm_rmrf function is available from the frontend, which does the same as this function did.
* It was worth to modify _alpm_rmrf to alpm_rmrf for pacman frontend to be able to use it in the future or for other frontend, so the function declaration was moved into the api header file, named alpm.h, and the definition of this function got a SYMEXPORT attribute.
* The rmrf funtion calling was changed in ./src/pacman/sync.c for alpm_rmrf
* The _alpm_rmrf functions were changed in api for alpm_rmrf
Why would we do that ? alpm is not a general system utility.
Besides this function is not as safe as it could be. http://bugs.archlinux.org/task/16363
I agree with Xavier here; especially knowing the function isn't perfect it doesn't make sense to expose it and then have to debug people's issues with it when they use it in ways we did not intend (which is to remove database directory hierarchies only, etc.). Wow, that was a rambling sentence. Sorry. :) -Dan
* The alpm_rmrf function is available from the api, which does the
same as this function did, with a small sanity check.
* It was worth to establish alpm_rmrf for _alpm_rmrf for pacman frontend
as a wrapper to be able to use it in the future or for other frontend, so the
function declaration was deleted in the frontend, and the new alpm_rmrf
wrapper function was established for future usage with SYMEXPORT modifier.
Signed-off-by: Laszlo Papp
Laszlo Papp wrote:
* The alpm_rmrf function is available from the api, which does the same as this function did, with a small sanity check.
* It was worth to establish alpm_rmrf for _alpm_rmrf for pacman frontend as a wrapper to be able to use it in the future or for other frontend, so the function declaration was deleted in the frontend, and the new alpm_rmrf wrapper function was established for future usage with SYMEXPORT modifier.
Signed-off-by: Laszlo Papp
--- What about this approach ?
What is different and how does this address the comments Xavier and Dan made earlier? Allan
On Sun, Oct 25, 2009 at 11:58 AM, Allan McRae
Laszlo Papp wrote:
* The alpm_rmrf function is available from the api, which does the same as this function did, with a small sanity check.
* It was worth to establish alpm_rmrf for _alpm_rmrf for pacman frontend as a wrapper to be able to use it in the future or for other frontend, so the function declaration was deleted in the frontend, and the new alpm_rmrf wrapper function was established for future usage with SYMEXPORT modifier.
Signed-off-by: Laszlo Papp
--- What about this approach ?
What is different and how does this address the comments Xavier and Dan made earlier?
Allan
Allan, as you see this is another approach for avoiding the unneccesary function definitition duplication between the library and the frontend. Tt's not exactly the continue of the previous theory. That's what I tried to do it, just making a wrapper and 'visible' function for the frontend to avoid the unneccesary duplication in the codebase, using the existing, working internal _alpm_rmrf function, without introducing a new insecure function or something. I can't mention easier solution for it to avoid the unneccesary replicating, maybe you've got better idea. Thanks the feedback! Best Regards, Laszlo Papp
Laszlo Papp wrote:
On Sun, Oct 25, 2009 at 11:58 AM, Allan McRae
wrote: Laszlo Papp wrote:
* The alpm_rmrf function is available from the api, which does the same as this function did, with a small sanity check.
* It was worth to establish alpm_rmrf for _alpm_rmrf for pacman frontend as a wrapper to be able to use it in the future or for other frontend, so the function declaration was deleted in the frontend, and the new alpm_rmrf wrapper function was established for future usage with SYMEXPORT modifier.
Signed-off-by: Laszlo Papp
--- What about this approach ?
What is different and how does this address the comments Xavier and Dan made earlier?
Allan
Allan, as you see this is another approach for avoiding the unneccesary function definitition duplication between the library and the frontend. Tt's not exactly the continue of the previous theory.
That's what I tried to do it, just making a wrapper and 'visible' function for the frontend to avoid the unneccesary duplication in the codebase, using the existing, working internal _alpm_rmrf function, without introducing a new insecure function or something. I can't mention easier solution for it to avoid the unneccesary replicating, maybe you've got better idea.
But Dan and Xavier both pointed out that exposing this function publicly was not a good idea. You seem to have just exposed it in a different way. Allan
On Sun, Oct 25, 2009 at 12:13 PM, Allan McRae
Laszlo Papp wrote:
On Sun, Oct 25, 2009 at 11:58 AM, Allan McRae
wrote: Laszlo Papp wrote:
* The alpm_rmrf function is available from the api, which does the same as this function did, with a small sanity check.
* It was worth to establish alpm_rmrf for _alpm_rmrf for pacman frontend as a wrapper to be able to use it in the future or for other frontend, so the function declaration was deleted in the frontend, and the new alpm_rmrf wrapper function was established for future usage with SYMEXPORT modifier.
Signed-off-by: Laszlo Papp
--- What about this approach ?
What is different and how does this address the comments Xavier and Dan made earlier?
Allan
Allan, as you see this is another approach for avoiding the unneccesary function definitition duplication between the library and the frontend. Tt's not exactly the continue of the previous theory.
That's what I tried to do it, just making a wrapper and 'visible' function for the frontend to avoid the unneccesary duplication in the codebase, using the existing, working internal _alpm_rmrf function, without introducing a new insecure function or something. I can't mention easier solution for it to avoid the unneccesary replicating, maybe you've got better idea.
But Dan and Xavier both pointed out that exposing this function publicly was not a good idea. You seem to have just exposed it in a different way.
Allan
Hm, you're the pacman developers :) I can't mention better idea to avoid the duplication then, because I think it's a typical common used function by frontends, so it's not a frontend specific function for pacman-frontend only. (e.g. mkdir -p like _alpm_makepath_mode too) I don't understand Xavier's sentence: "alpm is not a general system utility." Just see _alpm_makepath_mode or similar functions. (mkdir -p). Isn't this a system utility like function ? And I've never said it's a general system utility. Otherwise Dan's right maybe, if there is such an issued function. Best Regards, Laszlo Papp
On Sat, Oct 24, 2009 at 10:32 AM, Dan McGee
On Sat, Oct 24, 2009 at 6:26 AM, Xavier
wrote: On Sat, Oct 24, 2009 at 9:07 AM, Laszlo Papp
wrote: * The alpm_rmrf function is available from the frontend, which does the same as this function did.
* It was worth to modify _alpm_rmrf to alpm_rmrf for pacman frontend to be able to use it in the future or for other frontend, so the function declaration was moved into the api header file, named alpm.h, and the definition of this function got a SYMEXPORT attribute.
* The rmrf funtion calling was changed in ./src/pacman/sync.c for alpm_rmrf
* The _alpm_rmrf functions were changed in api for alpm_rmrf
Why would we do that ? alpm is not a general system utility.
Besides this function is not as safe as it could be. http://bugs.archlinux.org/task/16363
I agree with Xavier here; especially knowing the function isn't perfect it doesn't make sense to expose it and then have to debug people's issues with it when they use it in ways we did not intend (which is to remove database directory hierarchies only, etc.).
It might, however, make sense to create a "common" dir where the object files are linked to BOTH pacman and libalpm. That's not actually all that hard. It would prevent code duplication and also not expose these things publicly.
On Sun, Oct 25, 2009 at 5:57 PM, Aaron Griffin
On Sat, Oct 24, 2009 at 10:32 AM, Dan McGee
wrote: On Sat, Oct 24, 2009 at 6:26 AM, Xavier
wrote: On Sat, Oct 24, 2009 at 9:07 AM, Laszlo Papp
wrote: * The alpm_rmrf function is available from the frontend, which
does the same
as this function did.
* It was worth to modify _alpm_rmrf to alpm_rmrf for pacman
frontend to be
able to use it in the future or for other frontend, so the
function
declaration was moved into the api header file, named alpm.h,
and the
definition of this function got a SYMEXPORT attribute.
* The rmrf funtion calling was changed in ./src/pacman/sync.c
for alpm_rmrf
* The _alpm_rmrf functions were changed in api for alpm_rmrf
Why would we do that ? alpm is not a general system utility.
Besides this function is not as safe as it could be. http://bugs.archlinux.org/task/16363
I agree with Xavier here; especially knowing the function isn't perfect it doesn't make sense to expose it and then have to debug people's issues with it when they use it in ways we did not intend (which is to remove database directory hierarchies only, etc.).
It might, however, make sense to create a "common" dir where the object files are linked to BOTH pacman and libalpm. That's not actually all that hard. It would prevent code duplication and also not expose these things publicly.
I can deal with it, because I don't know mention better one, if Dan, Xavier agree with it, opinion ? Best Regards, Laszlo Papp
On Sun, Oct 25, 2009 at 5:57 PM, Aaron Griffin
I agree with Xavier here; especially knowing the function isn't perfect it doesn't make sense to expose it and then have to debug people's issues with it when they use it in ways we did not intend (which is to remove database directory hierarchies only, etc.).
It might, however, make sense to create a "common" dir where the object files are linked to BOTH pacman and libalpm. That's not actually all that hard. It would prevent code duplication and also not expose these things publicly.
That's definitely good to know, thanks for the information. However I checked libalpm and pacman util.c for duplicates, and I found exactly two : rmrf and strtrim. So not sure if it is worth it. But in any cases, it's really not a big deal :) And I really hate code duplication. But this case is a special one which has never bothered me. It concerns utility functions which are hardly ever modified, they are in well known place (util.c) and it's a simple copy of a whole function. The kind of code duplication that is really annoying is for instance the one we have between upgrade.c , sync.c and remove.c . These three have a lot of similarities but are still different so it's not obvious how to factor the code. But it happened several times to me that because of a change in the backend or somewhere else, I had to update these 3 files in exactly the same way.
participants (6)
-
Aaron Griffin
-
Allan McRae
-
Dan McGee
-
Laszlo Papp
-
Laszlo Papp
-
Xavier