hi all
just wanted to suggest let's make a list of bugs that blocks us to release a public rc (and then the first stable) release of pacman3
we have several ideas, for example replacing the ugly chroot system() call, adding gettext support, etc, but these features were not in pacman2, and i think they're not "musthave"
so what are the points from TODO that must be solved before a release?
i think the only one is the lack of handling of package conflicts during packages replacement, and maybe the manpages of public funtions (via doxygen)
udv / greetings, VMiklos
VMiklos wrote:
hi all
just wanted to suggest let's make a list of bugs that blocks us to release a public rc (and then the first stable) release of pacman3
we have several ideas, for example replacing the ugly chroot system() call, adding gettext support, etc, but these features were not in pacman2, and i think they're not "musthave"
so what are the points from TODO that must be solved before a release?
The ORE tags from the file lib/libalpm/sync.c are placeholders for missing chunks of code. The commentaries associated with the tags were copied/pasted from commentaries in the pacman.c file from pacman 2.9.7 to remind me to implement the missing sections. Implementing code for these parts may not be obvious and may need to be discussed. This is the most important task to achieve before pacman 3.0 can be released.
i think the only one is the lack of handling of package conflicts during packages replacement, and maybe the manpages of public funtions (via doxygen)
On the library side, I really would like to have error messages and log callbacks be reviewed. pm_errno usage is missing in some parts of the code. manpages are needed too. Upon release, the library must be usable (documented, and errors returned to the frontend must be really clean).
On pacman side, outputs should be checked too. The ORE tag in src/pacman/log.c must be fixed.
Last, but not least, more tests are needed, especially for backward compatibility between pacman 2.9.x and the library.
For instance, I'm trying usually to install/removing the same set of packages inside two different subdirectories. For the first directory, I'm using pacman 2.9, and for the second, I'm using the library. Then I'm diffing both. But, my last test campaign is quite old... And I poorly tested the "sync" part, especially since the code is not complete.
-- Aurelien
On Sun, Oct 30, 2005 at 06:46:14PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
On the library side, I really would like to have error messages and log callbacks be reviewed. pm_errno usage is missing in some parts of the code. manpages are needed too.
ok, as a first step, here is a patch to add a Doxyfile for libalpm
http://frugalware.org/~vmiklos/patches/libpacman-proposed/pacman-lib-doxyfil...
regarding the documentation, after this: 1) groups have to be renamed from foo to alpm_foo 2) we have to run "doxygen Doxyfile" at compile time, and install the generated manpages from doc/man3 at install time
udv / greetings, VMiklos
On Sun, Oct 30, 2005 at 10:39:41PM +0100, VMiklos vmiklos@frugalware.org wrote:
On Sun, Oct 30, 2005 at 06:46:14PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
On the library side, I really would like to have error messages and log callbacks be reviewed. pm_errno usage is missing in some parts of the code. manpages are needed too.
ok, as a first step, here is a patch to add a Doxyfile for libalpm
http://frugalware.org/~vmiklos/patches/libpacman-proposed/pacman-lib-doxyfil...
any comments?
udv / greetings, VMiklos
On Sat, Nov 05, 2005 at 10:34:48AM +0100, VMiklos vmiklos@frugalware.org wrote:
ok, as a first step, here is a patch to add a Doxyfile for libalpm
http://frugalware.org/~vmiklos/patches/libpacman-proposed/pacman-lib-doxyfil...
any comments?
still not comments?
udv / greetings, VMiklos
On Mon, Nov 14, 2005 at 11:39:46AM +0100, VMiklos vmiklos@frugalware.org wrote:
On Sat, Nov 05, 2005 at 10:34:48AM +0100, VMiklos vmiklos@frugalware.org wrote:
ok, as a first step, here is a patch to add a Doxyfile for libalpm
http://frugalware.org/~vmiklos/patches/libpacman-proposed/pacman-lib-doxyfil...
any comments?
still not comments?
just for who does not co the cvs regulary: applied by judd
udv / greetings, VMiklos
On Mon, Nov 21, 2005 at 11:43:44PM +0100, VMiklos wrote:
just for who does not co the cvs regulary: applied by judd
Yup, just committed. Didn't reply to this message, as I accidentally nuked most of my inbox the other day. :(
- J
On Sun, Oct 30, 2005 at 10:39:41PM +0100, VMiklos vmiklos@frugalware.org wrote:
ok, as a first step, here is a patch to add a Doxyfile for libalpm
http://frugalware.org/~vmiklos/patches/libpacman-proposed/pacman-lib-doxyfil...
regarding the documentation, after this:
- groups have to be renamed from foo to alpm_foo
http://frugalware.org/~vmiklos/patches/libpacman-proposed/rename_doxygen_gro...
here is the next patch to add the alpm_ foo prefix to manpages, i.e. alpm_sync.3 instead of sync.3
udv / greetings, VMiklos
On Sun, Oct 30, 2005 at 06:46:14PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
The ORE tags from the file lib/libalpm/sync.c are placeholders for missing chunks of code. The commentaries associated with the tags were copied/pasted from commentaries in the pacman.c file from pacman 2.9.7 to remind me to implement the missing sections. Implementing code for these parts may not be obvious and may need to be discussed. This is the most important task to achieve before pacman 3.0 can be released.
the question is if you plan to implement these or we should figure out the situation (probably it'll take some time for us) and try to do this?
udv / greetings, VMiklos
VMiklos wrote:
On Sun, Oct 30, 2005 at 06:46:14PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
The ORE tags from the file lib/libalpm/sync.c are placeholders for missing chunks of code. The commentaries associated with the tags were copied/pasted from commentaries in the pacman.c file from pacman 2.9.7 to remind me to implement the missing sections. Implementing code for these parts may not be obvious and may need to be discussed. This is the most important task to achieve before pacman 3.0 can be released.
the question is if you plan to implement these or we should figure out the situation (probably it'll take some time for us) and try to do this?
udv / greetings, VMiklos
I've already implemented sporadic parts of it, but it is only a first try, and I'm not ready to commit or share it until I'm sure it is possible to get it working right.
It is not really a simple task, and eventually, the transaction structure need to be extended to support sync operations: sync_commit() must be able to deal with both a list of packages to remove and to install in the same operation.
To sum-up, there's nothing frozen for now, so I'm still interested to hear ideas or suggestions on this topic, or to get some patches too!
-- Aurelien
On Mon, Nov 28, 2005 at 08:20:36PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
I've already implemented sporadic parts of it, but it is only a first try, and I'm not ready to commit or share it until I'm sure it is possible to get it working right.
It is not really a simple task, and eventually, the transaction structure need to be extended to support sync operations: sync_commit() must be able to deal with both a list of packages to remove and to install in the same operation.
i know, that's why asked you, if maybe you have already done it
To sum-up, there's nothing frozen for now, so I'm still interested to hear ideas or suggestions on this topic, or to get some patches too!
ok :)
udv / greetings, VMiklos
On Mon, Nov 28, 2005 at 08:20:36PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
It is not really a simple task, and eventually, the transaction structure need to be extended to support sync operations: sync_commit() must be able to deal with both a list of packages to remove and to install in the same operation.
To sum-up, there's nothing frozen for now, so I'm still interested to hear ideas or suggestions on this topic, or to get some patches too!
http://frugalware.org/~vmiklos/patches/libpacman-proposed/conflict-remove.di...
here is a possible implementation
$ sudo pacman -S mtr-nc --noconfirm resolving dependencies... done. looking for inter-conflicts... done.
Remove: mtr
Targets: mtr-nc-0.69-2
Total Package Size: 0.1 MB
Beginning upgrade process...
checking package integrity... done. removing mtr... done. (1/1) installing mtr-nc [################] 100%
udv / greetings, VMiklos
On Tue, Dec 20, 2005 at 12:09:19AM +0100, VMiklos wrote:
http://frugalware.org/~vmiklos/patches/libpacman-proposed/conflict-remove.di...
here is a possible implementation
Ah, thank you! I will look at this as soon as I can.
- J
On Sun, Oct 30, 2005 at 06:46:14PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
The ORE tags from the file lib/libalpm/sync.c are placeholders for missing chunks of code. The commentaries associated with the tags were copied/pasted from commentaries in the pacman.c file from pacman 2.9.7 to remind me to implement the missing sections. Implementing code for these parts may not be obvious and may need to be discussed. This is the most important task to achieve before pacman 3.0 can be released.
imho this is now done
On the library side, I really would like to have error messages and log callbacks be reviewed. pm_errno usage is missing in some parts of the code. manpages are needed too.
cmiiw, but these are solved, too
Upon release, the library must be usable (documented, and errors returned to the frontend must be really clean).
same
On pacman side, outputs should be checked too. The ORE tag in src/pacman/log.c must be fixed.
a patch to fix that ore tag is just sent
Last, but not least, more tests are needed, especially for backward compatibility between pacman 2.9.x and the library.
this is something you can decide, i mean what tests fail
so it would be nice if we could make a new todo about what must be done before the first 3.x release
udv / greetings, VMiklos
VMiklos wrote:
so it would be nice if we could make a new todo about what must be done before the first 3.x release
Here is my list of things (sorted by priority) that should be worked out before we release something. There's no necessarily a need to implement everything, some points may just need a status. Everyone's welcome to challenge these items or to add more :)
For information, I've already completed points 4 and 5, although not committed yet (waiting for possible feedbacks on the need for these points)
Then, I think we should be ready for a first beta release. Something like pacman 3.0beta1.
Thoughts? -- Aurelien
[1] bug in the conflict resolution in sync_prepare:
Example: :: Starting local database upgrade... ... :: avahi conflicts with nss-mdns. Remove nss-mdns? [Y/n] ... Remove: nss-mdns
Targets: avahi-0.6.6-2 nss-mdns-0.7-4
it seems that the "rmtargs" list was dropped from the implementation during the merge with pacman 2.9.x code...
[2] possible db corruption because of pmsyncpkg_t struct:
the spkg field is just a pointer to data from the cache, which is likely to change during sync_commit
[3] cvs code synchronization with pacman 2.9.8:
is it already done? or is there something missing?
[4] review file conflicts notification
as of now, the library is returning a simple string explaining the conflict reason. we should pack the information into a real structure so that the frontend can decide how to notify it to users.
[5] database cleanup
- do not write empty entries in database files with db_write, (same thing for gensync? makepkg for .PKGINFO?) - do not add name and version entries in "desc" file (already available in the directory name), also for .PKGINFO file?
[6] more debug messages in libalpm
some parts of the code still remain obscure (file conflicts handling for instance) even when running pacman with "--debug=-1"
[7] remove exit() calls from libalpm
the library should never call exit() but always return an error code to the frontend (for instance, drop MALLOC macro)
[8] .lastupdate review
Is the .lastupdate file really needed? Using mtime isn't enough? If not, is it possible to make .lastupdate support a frontend only feature (i.e drop it from the library code)? The idea is to keep the database format as simple as possible to ease possible integration with other backends.
On Sat, Feb 04, 2006 at 11:31:49AM +0100, Aurelien Foret aurelien@archlinux.org wrote:
[1] bug in the conflict resolution in sync_prepare:
Example: :: Starting local database upgrade... ... :: avahi conflicts with nss-mdns. Remove nss-mdns? [Y/n] ... Remove: nss-mdns
Targets: avahi-0.6.6-2 nss-mdns-0.7-4
it seems that the "rmtargs" list was dropped from the implementation during the merge with pacman 2.9.x code...
interesting, i can't reproduce this: $ sudo pacman -S libnfsidmap resolving dependencies... done. looking for inter-conflicts... :: libnfsidmap conflicts with nfsidmap. Remove nfsidmap? [Y/n] done.
Remove: nfsidmap
Targets: libnfsidmap-0.12-1
i even tried the official cvs, but that works fine, too
could you put out the buildscript of these packages to clarify the problem? (maybe it depends on if the packages is also provides the other, or something)
[3] cvs code synchronization with pacman 2.9.8:
is it already done? or is there something missing?
judd can confirm this, but what i saw the manpages were outdated, i sent a patch for this, but judd did it himself. so probably this is done
[5] database cleanup
- do not write empty entries in database files with db_write, (same
thing for gensync? makepkg for .PKGINFO?)
- do not add name and version entries in "desc" file (already available
in the directory name), also for .PKGINFO file?
is this important for now? this would prevent people to downgrade to pacman2 if they want
+ my [9]: review file conflict code: what i posted already here, imho the state "dirs can't become files (symlinks) during an upgrade (it's a conflict)" is too strict, we should allow it
udv / greetings, VMiklos
VMiklos wrote:
On Sat, Feb 04, 2006 at 11:31:49AM +0100, Aurelien Foret aurelien@archlinux.org wrote:
[1] bug in the conflict resolution in sync_prepare:
Example: :: Starting local database upgrade... ... :: avahi conflicts with nss-mdns. Remove nss-mdns? [Y/n] ... Remove: nss-mdns
Targets: avahi-0.6.6-2 nss-mdns-0.7-4
it seems that the "rmtargs" list was dropped from the implementation during the merge with pacman 2.9.x code...
interesting, i can't reproduce this: $ sudo pacman -S libnfsidmap resolving dependencies... done. looking for inter-conflicts... :: libnfsidmap conflicts with nfsidmap. Remove nfsidmap? [Y/n] done.
Remove: nfsidmap
Targets: libnfsidmap-0.12-1
i even tried the official cvs, but that works fine, too
could you put out the buildscript of these packages to clarify the problem? (maybe it depends on if the packages is also provides the other, or something)
I've committed a fix.
When a conflicting package was added to the replaces list, it was not removed from the target list (if present in it).
-- Aurelien
Aurelien Foret wrote:
VMiklos wrote:
so it would be nice if we could make a new todo about what must be done before the first 3.x release
Here is my list of things (sorted by priority) that should be worked out before we release something. There's no necessarily a need to implement everything, some points may just need a status. Everyone's welcome to challenge these items or to add more :)
Here's an update.
I've closed items [1] and [4], and added items [9] to [11].
If someone's willing to take ownership for an item, thanks to mention it on the ML. -- Aurelien
[2] possible db corruption because of pmsyncpkg_t struct: the spkg field is just a pointer to data from the cache, which is likely to change during sync_commit
[3] cvs code synchronization with pacman 2.9.8: I had a quick look at diffs between 2.9.7 and 2.9.8, and here's what seems to be missing: - makepkg - pacman.c (diff 2.9.7-2.9.8 = @@ -1338,11 +1378,50 @@) to be merged in lib/libalpm/sync.s (sync_prepare)
[5] database cleanup a) do not write empty entries in database files with db_write() b) do not add name and version entries in "desc" file (already available in the directory name), and also for .PKGINFO file (in makepkg)?
=> on hold for now
[6] more debug messages in libalpm some parts of the code still remain obscure (file conflicts handling for instance) even when running pacman with "--debug=-1"
[7] remove exit() calls from libalpm the library should never call exit() but always return an error code to the frontend (for instance, drop MALLOC macro)
[8] .lastupdate review Is the .lastupdate file really needed? Using mtime isn't enough? If not, is it possible to make .lastupdate support a frontend only feature (i.e drop it from the library code)? The idea is to keep the database format as simple as possible to ease possible integration with other backends.
[9] review file conflict code See this thread: http://www.archlinux.org/pipermail/pacman-dev/2006-February/000202.html
[10] pacman -Sw Outputs are wrong compared to pacman 2.9 Do not use log callbacks when the -w is used? or filter all outputs in trans.c depending on the downloadonly flag?
[11] library symbols naming prepends private functions with "_alpm_"
=> it should be done as the last step before a first release to avoid introducing too much changes to the CVS code at once.
On Mon, Feb 06, 2006 at 11:18:08PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
[10] pacman -Sw Outputs are wrong compared to pacman 2.9 Do not use log callbacks when the -w is used? or filter all outputs in trans.c depending on the downloadonly flag?
hmm. what is the problem here?
$ sudo pacman2 -Suw :: fontconfig-2.3.2-7: ignoring package upgrade (2.3.93-1) :: wireless_tools: local (28.pre9-1) appears to be newer than repo (frugalware-current/27-6) :: Above packages will be skipped. To manually upgrade use 'pacman -S <pkg>'
Targets: bash-completion-20050721-3 bochs-2.2.6-1 fox-1.4.29-2 gaim-1.5.0-4 gnome-session-2.12.0-3 icewm-1.2.25-1 imlib2-1.2.1-2 intltool-0.34.2-1 libwnck-2.12.2-2 mdadm-2.3-1 nmap-4.00-1 pacman-tools-0.6.4-1 pango-1.10.3-1 poppler-0.5.0-3 postfix-2.2.8-2 rrdtool-1.2.12-1
Total Package Size: 16.1 MB
Proceed with download? [Y/n]
:: Retrieving packages from extra-current... fox-1.4.29-2-i686 [----------------] 100% 3479K 11225K/s 00:00:00
checking package integrity... done.
$ sudo pacman -Suw :: Starting local database upgrade... warning: fontconfig-2.3.2-7: ignoring package upgrade (2.3.93-1) warning: wireless_tools-28.pre9-1: local version is newer resolving dependencies... done. looking for inter-conflicts... done.
Targets: bash-completion-20050721-3 bochs-2.2.6-1 fox-1.4.29-2 gaim-1.5.0-4 gnome-session-2.12.0-3 icewm-1.2.25-1 imlib2-1.2.1-2 intltool-0.34.2-1 pango-1.10.3-1 libwnck-2.12.2-2 mdadm-2.3-1 nmap-4.00-1 pacman-tools-0.6.4-1 poppler-0.5.0-3 postfix-2.2.8-2 rrdtool-1.2.12-1
Total Package Size: 16.1 MB
Proceed with download? [Y/n]
:: Retrieving packages from extra-current... fox-1.4.29-2-i686 [----------------] 100% 3479K 13672K/s 00:00:00
checking package integrity... done.
btw it's interesting that the download speed is higher with pacman3 :)
[11] library symbols naming prepends private functions with "_alpm_"
=> it should be done as the last step before a first release to avoid introducing too much changes to the CVS code at once.
hmm. you mean _every_ function that is not declared in alpm.h?
udv / greetings, VMiklos
VMiklos wrote:
On Mon, Feb 06, 2006 at 11:18:08PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
[10] pacman -Sw Outputs are wrong compared to pacman 2.9 Do not use log callbacks when the -w is used? or filter all outputs in trans.c depending on the downloadonly flag?
hmm. what is the problem here?
$ sudo pacman2 -Suw
I do mean -Sw, not -Suw.
# pacman-cvs -Sw zsnes :: zsnes-1.42-1: local version is up to date. Upgrade anyway? [Y/n]
resolving dependencies... done. looking for inter-conflicts... done.
Targets: zsnes-1.42-1
Total Package Size: 0.5 MB
Proceed with download? [Y/n] n
# pacman-2.9.8 -Sw zsnes
Targets: zsnes-1.42-1
Total Package Size: 0.5 MB
Proceed with download? [Y/n] n
-- Aurelien
VMiklos wrote:
[11] library symbols naming prepends private functions with "_alpm_"
=> it should be done as the last step before a first release to avoid introducing too much changes to the CVS code at once.
hmm. you mean _every_ function that is not declared in alpm.h?
Excepted static ones, yes.
The library and the frontend are sharing the same namespace, so that symbol conflicts can arise. Pre-pending function names with a common prefix ("_alpm_" or just "_") helps to avoid such conflicts.
This is all but mandatory, but it is usually a good practice.
I've already done it to some extent in the past (see list.h or util.h) for the most common function names which are more likely to be defined in the frontend.
It makes sense to me to extend it to all internal functions. -- Aurelien
On Tue, Feb 07, 2006 at 07:58:57PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
VMiklos wrote:
[11] library symbols naming prepends private functions with "_alpm_"
=> it should be done as the last step before a first release to avoid introducing too much changes to the CVS code at once.
hmm. you mean _every_ function that is not declared in alpm.h?
Excepted static ones, yes.
ok, then i take this
udv / greetings, VMiklos
On Wed, Feb 08, 2006 at 02:07:59PM +0100, VMiklos vmiklos@frugalware.org wrote:
hmm. you mean _every_ function that is not declared in alpm.h?
Excepted static ones, yes.
ok, then i take this
http://frugalware.org/~vmiklos/patches/libpacman-proposed/_alpm.diff
udv / greetings, VMiklos
On Wed, Feb 08, 2006 at 03:11:36PM +0100, VMiklos vmiklos@frugalware.org wrote:
On Wed, Feb 08, 2006 at 02:07:59PM +0100, VMiklos vmiklos@frugalware.org wrote:
hmm. you mean _every_ function that is not declared in alpm.h?
Excepted static ones, yes.
ok, then i take this
http://frugalware.org/~vmiklos/patches/libpacman-proposed/_alpm.diff
is there any reason why this was not commented?
1) as reviewing the patches takes sometimes weeks, we had to create our own tree 2) so every patch has to be backported manually 3) this patch is rather trivial 4) it implements Aurel's idea (not a Frugalware one) 5) you know that the patch was affected almost everything, and yes, it no longer applies
sorry, but this is a bit annoying. i really don't want to port it again to your cvs
possible sollutions i see: - speeding up reviewing the patches a bit (apply/reject them in a week) - giving cvs write access
comments?
udv / greetings, VMiklos
VMiklos wrote:
http://frugalware.org/~vmiklos/patches/libpacman-proposed/_alpm.diff
is there any reason why this was not commented?
- as reviewing the patches takes sometimes weeks, we had to create our
own tree 2) so every patch has to be backported manually 3) this patch is rather trivial 4) it implements Aurel's idea (not a Frugalware one) 5) you know that the patch was affected almost everything, and yes, it no longer applies
sorry, but this is a bit annoying. i really don't want to port it again to your cvs
6) if you had read my TODO completely, you would have notice that I suggested this change to be the last one to be done before a release. But you chose to do it and send it anyway...
Should I feel sorry because you're working on a modified version of pacman sources, and that you have to backport your changes to the official sources in order send us patches, or that you have to merge our cvs code with yours? To be honest, I don't think so. You're maintaining a parallel version of pacman, and I'm not responsible for this choice.
By the way, the fact that a patch does not apply cleanly is absolutely not an issue. For your information, I already applied it to my local code sources, although I didn't committed it yet. I'm still reviewing it (especially the need to rename symbols for db_XXX functions).
possible sollutions i see:
- speeding up reviewing the patches a bit (apply/reject them in a week)
- giving cvs write access
I don't see the need for it. If it takes more than a week to review and commit, what's the deal? I can't even call the current pacman cvs code a beta version, it is still code in development. So, there's no reason to put us under pressure.
On Fri, Feb 17, 2006 at 09:10:28AM +0100, Aurelien Foret aurelien@archlinux.org wrote:
- if you had read my TODO completely, you would have notice that I
suggested this change to be the last one to be done before a release. But you chose to do it and send it anyway...
okay, then just forgot that patch
Should I feel sorry because you're working on a modified version of pacman sources, and that you have to backport your changes to the official sources in order send us patches, or that you have to merge our cvs code with yours? To be honest, I don't think so. You're maintaining a parallel version of pacman, and I'm not responsible for this choice.
of course that's our choice, that's not a problem. my problem is that submitting the patches here requires some work. you know, a one-liner sometimes a result of a half day of headache. then i submit here a patch, and i even don't get a "rejected" answer after a week
By the way, the fact that a patch does not apply cleanly is absolutely not an issue.
good to know :)
For your information, I already applied it to my local code sources, although I didn't committed it yet. I'm still reviewing it (especially the need to rename symbols for db_XXX functions).
thanks for the info
I don't see the need for it. If it takes more than a week to review and commit, what's the deal?
nothing just i can't write down everything about the patch in the mail and then if you ask something, i won't know why i did something. look at my "chroot - change working dir" patch: probably you haven't answered because it takes time to prepare again the testsuite for verifying what i wrote
the other problem with the slow review is that you reguralry find the same bug what one of the patches fix, then you fix that, and then a) just don't reply to my mail b) ask here why this fix is needed, it's already been fixed. yes. two hours ago. and the patch was sent two _weeks_ ago
So, there's no reason to put us under pressure.
ok, as you want. just i see less and less motivation to submit patches here, but that's your choice
udv / greetings, VMiklos
On Mon, Feb 06, 2006 at 11:18:08PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
[7] remove exit() calls from libalpm the library should never call exit() but always return an error code to the frontend (for instance, drop MALLOC macro)
hmm, is it really necessary to drop that macro? what about this patch? http://darcs.frugalware.org/darcsweb/darcsweb.cgi?r=pacman;a=plain_commitdif...;
udv / greetings, VMiklos
VMiklos wrote:
On Mon, Feb 06, 2006 at 11:18:08PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
[7] remove exit() calls from libalpm the library should never call exit() but always return an error code to the frontend (for instance, drop MALLOC macro)
hmm, is it really necessary to drop that macro? what about this patch? http://darcs.frugalware.org/darcsweb/darcsweb.cgi?r=pacman;a=plain_commitdif...;
I don't think it would fit: it drops all possibilities to take needed actions to cleanup the environment (freeing locally allocated memory, ...) before leaving a function.
Anyway, it is only used 6 times in the whole library... not a big deal to remove it.
On Tue, Feb 07, 2006 at 08:05:21PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
VMiklos wrote:
On Mon, Feb 06, 2006 at 11:18:08PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
[7] remove exit() calls from libalpm the library should never call exit() but always return an error code to the frontend (for instance, drop MALLOC macro)
hmm, is it really necessary to drop that macro? what about this patch? http://darcs.frugalware.org/darcsweb/darcsweb.cgi?r=pacman;a=plain_commitdif...;
I don't think it would fit: it drops all possibilities to take needed actions to cleanup the environment (freeing locally allocated memory, ...) before leaving a function.
ok, here is an updated patch: http://frugalware.org/~vmiklos/patches/libpacman-proposed/drop_malloc.diff
i haven't added any extra FREE() for other allocated memory, if the patch is ok, then i can add
udv / greetings, VMiklos
VMiklos wrote:
ok, here is an updated patch: http://frugalware.org/~vmiklos/patches/libpacman-proposed/drop_malloc.diff
i haven't added any extra FREE() for other allocated memory, if the patch is ok, then i can add
It is not that simple in all circumstances.
For instance, you chose to set pm_errno in db_open and pkg_new, but the pm_errno value can be overwritten. See code chunks where pkg_new is called: in case of a failure, pm_errno is set afterwards, meaning that the pm_errno value set in pkg_new is lost. It may or may not be more interesting to set pm_errno in the offending function, or upon return of the function. It depends on what is more meaningful from the frontend point of view...
This is what I meant with this item of the TODO: "- review errors handling (globalise pm_errno usage, improve error meanings)" -- Aurelien
On Sat, Feb 11, 2006 at 07:44:00PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
For instance, you chose to set pm_errno in db_open and pkg_new, but the pm_errno value can be overwritten. See code chunks where pkg_new is called: in case of a failure, pm_errno is set afterwards, meaning that the pm_errno value set in pkg_new is lost.
i think this is the right way, the "failed to prepare the transaction" error message is more infomative as the "memory error" one for a frontend. or don't you think so?
It may or may not be more interesting to set pm_errno in the offending function, or upon return of the function. It depends on what is more meaningful from the frontend point of view...
i would say set pm_errno to PM_ERR_MEMORY, and if it's overwritten, then it's not a problem, any error message other than PM_ERR_MEMORY is more informative to the fronend. that's what my patch does.
This is what I meant with this item of the TODO: "- review errors handling (globalise pm_errno usage, improve error meanings)"
good to know. i thought you meant here the - already implemented - error list on conflicting files or missing deps
udv / greetings, VMiklos
VMiklos wrote:
On Sat, Feb 11, 2006 at 07:44:00PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
For instance, you chose to set pm_errno in db_open and pkg_new, but the pm_errno value can be overwritten. See code chunks where pkg_new is called: in case of a failure, pm_errno is set afterwards, meaning that the pm_errno value set in pkg_new is lost.
i think this is the right way, the "failed to prepare the transaction" error message is more infomative as the "memory error" one for a frontend. or don't you think so?
Agreed. Patch applied.
It may or may not be more interesting to set pm_errno in the offending function, or upon return of the function. It depends on what is more meaningful from the frontend point of view...
i would say set pm_errno to PM_ERR_MEMORY, and if it's overwritten, then it's not a problem, any error message other than PM_ERR_MEMORY is more informative to the fronend. that's what my patch does.
To go even further, we should maybe add a LOG_ERROR logs mentioning the issue? That would give some clues even it pm_errno got overwritten...
On Mon, Feb 20, 2006 at 09:42:45PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
To go even further, we should maybe add a LOG_ERROR logs mentioning the issue? That would give some clues even it pm_errno got overwritten...
maybe adding debug messages also could do the task? that sounds a bit more simpler to me
udv / greetings, VMiklos
VMiklos wrote:
On Mon, Feb 20, 2006 at 09:42:45PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
To go even further, we should maybe add a LOG_ERROR logs mentioning the issue? That would give some clues even it pm_errno got overwritten...
maybe adding debug messages also could do the task? that sounds a bit more simpler to me
That's why I meant, but with the ERROR mask rather than the DEBUG one.
On Wed, Feb 22, 2006 at 10:17:24PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
VMiklos wrote:
On Mon, Feb 20, 2006 at 09:42:45PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
To go even further, we should maybe add a LOG_ERROR logs mentioning the issue? That would give some clues even it pm_errno got overwritten...
maybe adding debug messages also could do the task? that sounds a bit more simpler to me
That's why I meant, but with the ERROR mask rather than the DEBUG one.
should i make a patch to implement this? (before we get to a situation like with the symbol renaming ;) )
udv / greetings, VMiklos
VMiklos wrote:
should i make a patch to implement this? (before we get to a situation like with the symbol renaming ;) )
If you're inclined to, then yes :)
On Thu, Feb 23, 2006 at 10:10:48PM +0100, Aurelien Foret aurelien@archlinux.org wrote:
VMiklos wrote:
should i make a patch to implement this? (before we get to a situation like with the symbol renaming ;) )
If you're inclined to, then yes :)
http://frugalware.org/~vmiklos/patches/libpacman-proposed/malloc_error.diff
udv / greetings, VMiklos
VMiklos wrote:
http://frugalware.org/~vmiklos/patches/libpacman-proposed/malloc_error.diff
Applied.
pacman-dev@lists.archlinux.org