On Thu, 20 Jun 2019 at 05:46, Allan McRae <[email protected]> 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 <[email protected]> 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?
