On Fri, 14 Jun 2019 at 14:09, Dave Reisner <d@falconindy.com> wrote:

On Fri, Jun 14, 2019 at 02:51:14AM +0100, morganamilo wrote:

...

libarchive uses 1 for EOF, not 0. Instead of using the actual ints, use libarchive's error codes. ---

By the way, not familiar with doxygen. Is my wording fine or is there some built in "see also" functionality?

diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index ffb2ad96..ece894cf 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -1326,7 +1326,8 @@ struct archive *alpm_pkg_mtree_open(alpm_pkg_t *pkg); * @param pkg the package that the mtree file is being read from * @param archive the archive structure reading from the mtree file * @param entry an archive_entry to store the entry header information - * @return 0 if end of archive is reached, non-zero otherwise. + * @return ARCHIVE_OK on success, ARCHIVE_EOF if end of archive is reached, + * otherwise an error occured (see archive.h).

Please, no. Let's not leak details from libarchive in our own API.

...

*/ int alpm_pkg_mtree_next(const alpm_pkg_t *pkg, struct archive *archive, struct archive_entry **entry); -- 2.21.0

Why not? The return value is exactly that. If libarchive's return codes suddenly changed then so would libalpms's. Plus pacman itself already uses ARCHIVE_OK to check the return code. And finally if we did not depend on magic numbers then the doc wouldn't be wrong in the first place.

On 14/6/19 11:26 pm, Dave Reisner wrote:

On Fri, Jun 14, 2019 at 02:19:52PM +0100, Morgan Adamiec wrote:

...

On Fri, 14 Jun 2019 at 14:09, Dave Reisner <d@falconindy.com> wrote:

...

On Fri, Jun 14, 2019 at 02:51:14AM +0100, morganamilo wrote:

...

libarchive uses 1 for EOF, not 0. Instead of using the actual ints, use libarchive's error codes. ---

By the way, not familiar with doxygen. Is my wording fine or is there some built in "see also" functionality?

diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index ffb2ad96..ece894cf 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -1326,7 +1326,8 @@ struct archive *alpm_pkg_mtree_open(alpm_pkg_t *pkg); * @param pkg the package that the mtree file is being read from * @param archive the archive structure reading from the mtree file * @param entry an archive_entry to store the entry header information - * @return 0 if end of archive is reached, non-zero otherwise. + * @return ARCHIVE_OK on success, ARCHIVE_EOF if end of archive is reached, + * otherwise an error occured (see archive.h).

Please, no. Let's not leak details from libarchive in our own API.

...

*/ int alpm_pkg_mtree_next(const alpm_pkg_t *pkg, struct archive *archive, struct archive_entry **entry); -- 2.21.0

Why not? The return value is exactly that. If libarchive's return codes suddenly changed then so would libalpms's. Plus pacman itself already uses ARCHIVE_OK to check the return code. And finally if we did not depend on magic numbers then the doc wouldn't be wrong in the first place.

Because users of libalpm should only need to understand libalpm and not concern themselves with details of libarchive. Exposing ARCHIVE_* in libalpm is a leaky abstraction.

If the code is broken (and it sounds like it is), then it should be fixed along with the documentation.

Agreed. Not this is the only place in pacman we use an ARCHIVE_* value, so this is broken. src/pacman/check.c: while(alpm_pkg_mtree_next(pkg, mtree, &entry) == ARCHIVE_OK) {

On 06/21/19 at 03:24pm, Morgan Adamiec wrote:

On Thu, 20 Jun 2019 at 05:46, Allan McRae <allan@archlinux.org> wrote:

...

On 14/6/19 11:26 pm, Dave Reisner wrote:

...

On Fri, Jun 14, 2019 at 02:19:52PM +0100, Morgan Adamiec wrote:

...

On Fri, 14 Jun 2019 at 14:09, Dave Reisner <d@falconindy.com> wrote:

...

On Fri, Jun 14, 2019 at 02:51:14AM +0100, morganamilo wrote:

...

libarchive uses 1 for EOF, not 0. Instead of using the actual ints, use libarchive's error codes. ---

By the way, not familiar with doxygen. Is my wording fine or is there some built in "see also" functionality?

diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index ffb2ad96..ece894cf 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -1326,7 +1326,8 @@ struct archive *alpm_pkg_mtree_open(alpm_pkg_t *pkg); * @param pkg the package that the mtree file is being read from * @param archive the archive structure reading from the mtree file * @param entry an archive_entry to store the entry header information - * @return 0 if end of archive is reached, non-zero otherwise. + * @return ARCHIVE_OK on success, ARCHIVE_EOF if end of archive is reached, + * otherwise an error occured (see archive.h).

Please, no. Let's not leak details from libarchive in our own API.

...

*/ int alpm_pkg_mtree_next(const alpm_pkg_t *pkg, struct archive *archive, struct archive_entry **entry); -- 2.21.0

Why not? The return value is exactly that. If libarchive's return codes suddenly changed then so would libalpms's. Plus pacman itself already uses ARCHIVE_OK to check the return code. And finally if we did not depend on magic numbers then the doc wouldn't be wrong in the first place.

Because users of libalpm should only need to understand libalpm and not concern themselves with details of libarchive. Exposing ARCHIVE_* in libalpm is a leaky abstraction.

If the code is broken (and it sounds like it is), then it should be fixed along with the documentation.

Agreed. Not this is the only place in pacman we use an ARCHIVE_* value, so this is broken.

src/pacman/check.c: while(alpm_pkg_mtree_next(pkg, mtree, &entry) == ARCHIVE_OK) {

Would is then also make sense to do `typedef struct archive alpm_mtree_t` or something similar?

I already said this on IRC, but, for the record, I don't see a need for a full alpm mtree implementation. I wouldn't reject it if somebody really wanted to put in that effort, but it seems like a lot of work to reimplement libarchive's entire archive_entry_* API for little gain. Personally, I'd just delete this function and alpm_pkg_mtree_close.