[pacman-dev] libalpm: fix documentation for alpm_pkg_mtree_next

Message ID 20190614015113.19264-1-morganamilo@gmail.com
State New
Headers show
Series
  • [pacman-dev] libalpm: fix documentation for alpm_pkg_mtree_next
Related show

Commit Message

morganamilo June 14, 2019, 1:51 a.m. UTC
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?

Comments

Dave Reisner June 14, 2019, 1:09 p.m. UTC | #1
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
morganamilo June 14, 2019, 1:19 p.m. UTC | #2
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.
Dave Reisner June 14, 2019, 1:26 p.m. UTC | #3
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.
Allan McRae June 20, 2019, 4:45 a.m. UTC | #4
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) {
morganamilo June 21, 2019, 2:24 p.m. UTC | #5
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?
Andrew Gregory July 3, 2019, 6:22 a.m. UTC | #6
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.

Patch

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).
  */
 int alpm_pkg_mtree_next(const alpm_pkg_t *pkg, struct archive *archive,
 		struct archive_entry **entry);