[PATCH 3/3] Remove rmrf function from frontend

View: New views
11 Messages — Rating Filter:   Alert me  

[PATCH 3/3] Remove rmrf function from frontend

by Laszlo Papp :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

        * 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 <djszapi@...>
---
 lib/libalpm/alpm.h     |    3 +++
 lib/libalpm/be_files.c |    4 ++--
 lib/libalpm/dload.c    |    2 +-
 lib/libalpm/trans.c    |    2 +-
 lib/libalpm/util.c     |    4 ++--
 lib/libalpm/util.h     |    1 -
 src/pacman/sync.c      |    2 +-
 src/pacman/util.c      |   43 -------------------------------------------
 src/pacman/util.h      |    1 -
 9 files changed, 10 insertions(+), 52 deletions(-)

diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index c433b73..666a84f 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -467,6 +467,9 @@ const char *alpm_fileconflict_get_ctarget(pmfileconflict_t *conflict);
 /* checksums */
 char *alpm_compute_md5sum(const char *name);
 
+/* does the same thing as 'rm -rf' */
+int alpm_rmrf(const char *path);
+
 /*
  * Errors
  */
diff --git a/lib/libalpm/be_files.c b/lib/libalpm/be_files.c
index ffbaa8d..70625ed 100644
--- a/lib/libalpm/be_files.c
+++ b/lib/libalpm/be_files.c
@@ -230,7 +230,7 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db)
  } else {
  const char *syncdbpath = _alpm_db_path(db);
  /* remove the old dir */
- if(_alpm_rmrf(syncdbpath) != 0) {
+ if(alpm_rmrf(syncdbpath) != 0) {
  _alpm_log(PM_LOG_ERROR, _("could not remove database %s\n"), db->treename);
  RET_ERR(PM_ERR_DB_REMOVE, -1);
  }
@@ -919,7 +919,7 @@ int _alpm_db_remove(pmdb_t *db, pmpkg_t *info)
 
  pkgpath = get_pkgpath(db, info);
 
- ret = _alpm_rmrf(pkgpath);
+ ret = alpm_rmrf(pkgpath);
  free(pkgpath);
  if(ret != 0) {
  ret = -1;
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index a4c9f1f..218a87b 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -182,7 +182,7 @@ static int download_internal(const char *url, const char *localpath,
  }
 
  if(localf == NULL) {
- _alpm_rmrf(tempfile);
+ alpm_rmrf(tempfile);
  fileurl->offset = (off_t)0;
  dl_thisfile = 0;
  localf = fopen(tempfile, "wb");
diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c
index aea71db..3eac9b0 100644
--- a/lib/libalpm/trans.c
+++ b/lib/libalpm/trans.c
@@ -401,7 +401,7 @@ int _alpm_runscriptlet(const char *root, const char *installfn,
  retval = _alpm_run_chroot(root, cmdline);
 
 cleanup:
- if(clean_tmpdir && _alpm_rmrf(tmpdir)) {
+ if(clean_tmpdir && alpm_rmrf(tmpdir)) {
  _alpm_log(PM_LOG_WARNING, _("could not remove tmpdir %s\n"), tmpdir);
  }
 
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
index ad3dc2c..6b0cf34 100644
--- a/lib/libalpm/util.c
+++ b/lib/libalpm/util.c
@@ -370,7 +370,7 @@ cleanup:
 }
 
 /* does the same thing as 'rm -rf' */
-int _alpm_rmrf(const char *path)
+int SYMEXPORT alpm_rmrf(const char *path)
 {
  int errflag = 0;
  struct dirent *dp;
@@ -397,7 +397,7 @@ int _alpm_rmrf(const char *path)
  if(dp->d_ino) {
  sprintf(name, "%s/%s", path, dp->d_name);
  if(strcmp(dp->d_name, "..") && strcmp(dp->d_name, ".")) {
- errflag += _alpm_rmrf(name);
+ errflag += alpm_rmrf(name);
  }
  }
  }
diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h
index 37893c8..2e783c3 100644
--- a/lib/libalpm/util.h
+++ b/lib/libalpm/util.h
@@ -66,7 +66,6 @@ int _alpm_lckmk();
 int _alpm_lckrm();
 int _alpm_unpack_single(const char *archive, const char *prefix, const char *fn);
 int _alpm_unpack(const char *archive, const char *prefix, alpm_list_t *list, int breakfirst);
-int _alpm_rmrf(const char *path);
 int _alpm_logaction(int usesyslog, FILE *f, const char *fmt, va_list args);
 int _alpm_run_chroot(const char *root, const char *cmd);
 int _alpm_ldconfig(const char *root);
diff --git a/src/pacman/sync.c b/src/pacman/sync.c
index a2ef616..0164ca9 100644
--- a/src/pacman/sync.c
+++ b/src/pacman/sync.c
@@ -90,7 +90,7 @@ static int sync_cleandb(const char *dbpath, int keep_used) {
  continue;
  }
 
- if(rmrf(path)) {
+ if(alpm_rmrf(path)) {
  pm_fprintf(stderr, PM_LOG_ERROR,
  _("could not remove repository directory\n"));
  closedir(dir);
diff --git a/src/pacman/util.c b/src/pacman/util.c
index e3dbfbb..9adc0e5 100644
--- a/src/pacman/util.c
+++ b/src/pacman/util.c
@@ -99,49 +99,6 @@ int getcols(void)
  return 0;
 }
 
-/* does the same thing as 'rm -rf' */
-int rmrf(const char *path)
-{
- int errflag = 0;
- struct dirent *dp;
- DIR *dirp;
-
- if(!unlink(path)) {
- return(0);
- } else {
- if(errno == ENOENT) {
- return(0);
- } else if(errno == EPERM) {
- /* fallthrough */
- } else if(errno == EISDIR) {
- /* fallthrough */
- } else if(errno == ENOTDIR) {
- return(1);
- } else {
- /* not a directory */
- return(1);
- }
-
- if((dirp = opendir(path)) == NULL) {
- return(PM_ERR_NOT_A_DIR);
- }
- for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) {
- if(dp->d_ino) {
- char name[PATH_MAX];
- sprintf(name, "%s/%s", path, dp->d_name);
- if(strcmp(dp->d_name, "..") && strcmp(dp->d_name, ".")) {
- errflag += rmrf(name);
- }
- }
- }
- closedir(dirp);
- if(rmdir(path)) {
- errflag++;
- }
- return(errflag);
- }
-}
-
 /** Parse the basename of a program from a path.
 * Grabbed from the uClibc source.
 * @param path path to parse basename from
diff --git a/src/pacman/util.h b/src/pacman/util.h
index 7a8c39d..30e5996 100644
--- a/src/pacman/util.h
+++ b/src/pacman/util.h
@@ -41,7 +41,6 @@ int trans_init(pmtransflag_t flags);
 int trans_release(void);
 int needs_root(void);
 int getcols(void);
-int rmrf(const char *path);
 char *mbasename(const char *path);
 char *mdirname(const char *path);
 void indentprint(const char *str, int indent);
--
1.6.5



Re: [PATCH 3/3] Remove rmrf function from frontend

by Xavier Chantry-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Sat, Oct 24, 2009 at 9:07 AM, Laszlo Papp <djszapi2@...> 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


Re: [PATCH 3/3] Remove rmrf function from frontend

by Dan McGee :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Sat, Oct 24, 2009 at 6:26 AM, Xavier <shiningxc@...> wrote:

> On Sat, Oct 24, 2009 at 9:07 AM, Laszlo Papp <djszapi2@...> 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


[PATCH] Remove rmrf function from frontend

by Laszlo Papp :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

* 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 <djszapi@...>
---

What about this approach ?

 lib/libalpm/alpm.h |    2 ++
 lib/libalpm/util.c |   14 +++++++++++++-
 src/pacman/sync.c  |    2 +-
 src/pacman/util.c  |   43 -------------------------------------------
 src/pacman/util.h  |    1 -
 5 files changed, 16 insertions(+), 46 deletions(-)

diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index c433b73..3e24fd3 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -467,6 +467,8 @@ const char *alpm_fileconflict_get_ctarget(pmfileconflict_t *conflict);
 /* checksums */
 char *alpm_compute_md5sum(const char *name);

+int alpm_rmrf(char *path);
+
 /*
  * Errors
  */
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
index d910809..1340da9 100644
--- a/lib/libalpm/util.c
+++ b/lib/libalpm/util.c
@@ -79,6 +79,18 @@ char* strsep(char** str, const char* delims)
 }
 #endif

+
+/** does the same thing as 'rm -rf'
+ * @param path name of the file to delete
+ */
+int SYMEXPORT alpm_rmrf(char *path)
+{
+ ALPM_LOG_FUNC;
+
+  ASSERT(path != NULL, RET_ERR(PM_ERR_WRONG_ARGS, -1));
+ return(_alpm_rmrf(path));
+}
+
 int _alpm_makepath(const char *path)
 {
  return(_alpm_makepath_mode(path, 0755));
@@ -315,7 +327,7 @@ int _alpm_unpack(const char *archive, const char *prefix, alpm_list_t *list, int

  st = archive_entry_stat(entry);
  entryname = archive_entry_pathname(entry);
-
+
  if(S_ISREG(st->st_mode)) {
  archive_entry_set_perm(entry, 0644);
  } else if(S_ISDIR(st->st_mode)) {
diff --git a/src/pacman/sync.c b/src/pacman/sync.c
index a2ef616..0164ca9 100644
--- a/src/pacman/sync.c
+++ b/src/pacman/sync.c
@@ -90,7 +90,7 @@ static int sync_cleandb(const char *dbpath, int keep_used) {
  continue;
  }

- if(rmrf(path)) {
+ if(alpm_rmrf(path)) {
  pm_fprintf(stderr, PM_LOG_ERROR,
  _("could not remove repository directory\n"));
  closedir(dir);
diff --git a/src/pacman/util.c b/src/pacman/util.c
index 1143bef..9adc0e5 100644
--- a/src/pacman/util.c
+++ b/src/pacman/util.c
@@ -99,49 +99,6 @@ int getcols(void)
  return 0;
 }

-/* does the same thing as 'rm -rf' */
-int rmrf(const char *path)
-{
- int errflag = 0;
- struct dirent *dp;
- DIR *dirp;
-
- if(!unlink(path)) {
- return(0);
- } else {
- if(errno == ENOENT) {
- return(0);
- } else if(errno == EPERM) {
- /* fallthrough */
- } else if(errno == EISDIR) {
- /* fallthrough */
- } else if(errno == ENOTDIR) {
- return(1);
- } else {
- /* not a directory */
- return(1);
- }
-
- if((dirp = opendir(path)) == (DIR *)-1) {
- return(1);
- }
- for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) {
- if(dp->d_ino) {
- char name[PATH_MAX];
- sprintf(name, "%s/%s", path, dp->d_name);
- if(strcmp(dp->d_name, "..") && strcmp(dp->d_name, ".")) {
- errflag += rmrf(name);
- }
- }
- }
- closedir(dirp);
- if(rmdir(path)) {
- errflag++;
- }
- return(errflag);
- }
-}
-
 /** Parse the basename of a program from a path.
 * Grabbed from the uClibc source.
 * @param path path to parse basename from
diff --git a/src/pacman/util.h b/src/pacman/util.h
index 7a8c39d..30e5996 100644
--- a/src/pacman/util.h
+++ b/src/pacman/util.h
@@ -41,7 +41,6 @@ int trans_init(pmtransflag_t flags);
 int trans_release(void);
 int needs_root(void);
 int getcols(void);
-int rmrf(const char *path);
 char *mbasename(const char *path);
 char *mdirname(const char *path);
 void indentprint(const char *str, int indent);
--
1.6.5



Re: [PATCH] Remove rmrf function from frontend

by Allan McRae-3 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

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 <djszapi@...>
> ---
>
> What about this approach ?
>  

What is different and how does this address the comments Xavier and Dan
made earlier?

Allan


Re: [PATCH] Remove rmrf function from frontend

by Laszlo Papp-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Sun, Oct 25, 2009 at 11:58 AM, Allan McRae <allan@...> 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 <djszapi@...>
>> ---
>>
>> 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


Re: [PATCH] Remove rmrf function from frontend

by Allan McRae-3 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Laszlo Papp wrote:

> On Sun, Oct 25, 2009 at 11:58 AM, Allan McRae <allan@...> 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 <djszapi@...>
>>> ---
>>>
>>> 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




Re: [PATCH] Remove rmrf function from frontend

by Laszlo Papp-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Sun, Oct 25, 2009 at 12:13 PM, Allan McRae <allan@...> wrote:

> Laszlo Papp wrote:
>
>> On Sun, Oct 25, 2009 at 11:58 AM, Allan McRae <allan@...>
>> 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 <djszapi@...>
>>>> ---
>>>>
>>>> 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


Re: [PATCH 3/3] Remove rmrf function from frontend

by Aaron Griffin :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Sat, Oct 24, 2009 at 10:32 AM, Dan McGee <dpmcgee@...> wrote:

> On Sat, Oct 24, 2009 at 6:26 AM, Xavier <shiningxc@...> wrote:
>> On Sat, Oct 24, 2009 at 9:07 AM, Laszlo Papp <djszapi2@...> 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.


Re: [PATCH 3/3] Remove rmrf function from frontend

by Laszlo Papp-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Sun, Oct 25, 2009 at 5:57 PM, Aaron Griffin <aaronmgriffin@...>wrote:

> On Sat, Oct 24, 2009 at 10:32 AM, Dan McGee <dpmcgee@...> wrote:
> > On Sat, Oct 24, 2009 at 6:26 AM, Xavier <shiningxc@...> wrote:
> >> On Sat, Oct 24, 2009 at 9:07 AM, Laszlo Papp <djszapi2@...>
> 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


Re: [PATCH 3/3] Remove rmrf function from frontend

by Xavier Chantry-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Sun, Oct 25, 2009 at 5:57 PM, Aaron Griffin <aaronmgriffin@...> wrote:

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