[pacman-dev] [PATCH] new upgade042 pactest + bufix in chk_filedifference.
This adds a pactest for the relocation of a config file between two packages (case of etc/profile moving from bash to filesystem). While running this pactest, I found out that chk_filedifference didn't work correctly whith an empty list as second argument. So that's fixed now. Ref: http://www.archlinux.org/pipermail/pacman-dev/2007-December/010610.html Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- lib/libalpm/conflict.c | 4 ++++ pactest/tests/upgrade042.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 0 deletions(-) create mode 100644 pactest/tests/upgrade042.py diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 91f4360..c1aac4d 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -251,6 +251,10 @@ static alpm_list_t *chk_filedifference(alpm_list_t *filesA, alpm_list_t *filesB) alpm_list_t *ret = NULL; alpm_list_t *pA = filesA, *pB = filesB; + if(pB == NULL) { + return(pA); + } + while(pA && pB) { const char *strA = pA->data; const char *strB = pB->data; diff --git a/pactest/tests/upgrade042.py b/pactest/tests/upgrade042.py new file mode 100644 index 0000000..d6140d4 --- /dev/null +++ b/pactest/tests/upgrade042.py @@ -0,0 +1,28 @@ +self.description = "Backup file relocation" + +lp1 = pmpkg("bash") +lp1.files = ["etc/profile*"] +lp1.backup = ["etc/profile"] +self.addpkg2db("local", lp1) + +p1 = pmpkg("bash", "1.0-2") +self.addpkg(p1) + +lp2 = pmpkg("filesystem") +self.addpkg2db("local", lp2) + +p2 = pmpkg("filesystem", "1.0-2") +p2.files = ["etc/profile**"] +p2.backup = ["etc/profile"] +p2.depends = [ "bash" ] +self.addpkg(p2) + +self.args = "-U %s" % " ".join([p.filename() for p in p1, p2]) + +self.filesystem = ["etc/profile"] + +self.addrule("PKG_VERSION=bash|1.0-2") +self.addrule("PKG_VERSION=filesystem|1.0-2") +self.addrule("!FILE_PACSAVE=etc/profile") +self.addrule("FILE_PACNEW=etc/profile") +self.addrule("FILE_EXIST=etc/profile") -- 1.5.3.7
Whoa, you caused me a lot of pain with this one. See below. On Jan 1, 2008 10:25 AM, Chantry Xavier <shiningxc@gmail.com> wrote:
This adds a pactest for the relocation of a config file between two packages (case of etc/profile moving from bash to filesystem). While running this pactest, I found out that chk_filedifference didn't work correctly whith an empty list as second argument. So that's fixed now.
Ref: http://www.archlinux.org/pipermail/pacman-dev/2007-December/010610.html
Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- lib/libalpm/conflict.c | 4 ++++ pactest/tests/upgrade042.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 0 deletions(-) create mode 100644 pactest/tests/upgrade042.py
diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 91f4360..c1aac4d 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -251,6 +251,10 @@ static alpm_list_t *chk_filedifference(alpm_list_t *filesA, alpm_list_t *filesB) alpm_list_t *ret = NULL; alpm_list_t *pA = filesA, *pB = filesB;
+ if(pB == NULL) { + return(pA); + } + while(pA && pB) { const char *strA = pA->data; const char *strB = pB->data;
Note that chk_filedifference always returned a NEW list, thus the return value could always be freed by the calling function. Thus, your change here gave me a big fat segfault because you didn't dupe the list first. I've fixed this locally and replaced the return with a alpm_list_strdup(pA) call. This segfault didn't get exposed until I made a *completely* unrelated change (removing a gettext call around a message that doesn't need to be gettexted), so it took some time to track down. Lesson to be learned here: know whether your function allocates new objects or not, and ensure ALL return paths do it the same way. Then be sure to check that the calling function does a free if necessary. -Dan
On Jan 1, 2008 6:56 PM, Dan McGee <dpmcgee@gmail.com> wrote:
Lesson to be learned here: know whether your function allocates new objects or not, and ensure ALL return paths do it the same way. Then be sure to check that the calling function does a free if necessary.
Weird. The lesson learned in my eyes is to stick with python ;-) Scott
Dan McGee wrote:
Note that chk_filedifference always returned a NEW list, thus the return value could always be freed by the calling function. Thus, your change here gave me a big fat segfault because you didn't dupe the list first. I've fixed this locally and replaced the return with a alpm_list_strdup(pA) call.
This segfault didn't get exposed until I made a *completely* unrelated change (removing a gettext call around a message that doesn't need to be gettexted), so it took some time to track down.
Lesson to be learned here: know whether your function allocates new objects or not, and ensure ALL return paths do it the same way. Then be sure to check that the calling function does a free if necessary.
Oh sorry, I totally overlooked that indeed.. If only it had segfaulted in my test case, I would have caught it immediatly. That's bad, I wanted to fix a minor bug and caused a much bigger one :p
On Jan 2, 2008 1:55 AM, Xavier <shiningxc@gmail.com> wrote:
Dan McGee wrote:
Note that chk_filedifference always returned a NEW list, thus the return value could always be freed by the calling function. Thus, your change here gave me a big fat segfault because you didn't dupe the list first. I've fixed this locally and replaced the return with a alpm_list_strdup(pA) call.
This segfault didn't get exposed until I made a *completely* unrelated change (removing a gettext call around a message that doesn't need to be gettexted), so it took some time to track down.
Lesson to be learned here: know whether your function allocates new objects or not, and ensure ALL return paths do it the same way. Then be sure to check that the calling function does a free if necessary.
Oh sorry, I totally overlooked that indeed.. If only it had segfaulted in my test case, I would have caught it immediatly. That's bad, I wanted to fix a minor bug and caused a much bigger one :p
Well your pactest did end up causing the segfault, but not until *after* I made those completely unrelated changes. Memory allocation can do weird things sometimes. -Dan
participants (4)
-
Chantry Xavier
-
Dan McGee
-
Scott Horowitz
-
Xavier