nih-at / libzip

A C library for reading, creating, and modifying zip archives.
https://libzip.org/
Other
806 stars 265 forks source link

Enhance documentation of `zip_source_zip_file` and `zip_source_zip` #433

Open fdegros opened 1 month ago

fdegros commented 1 month ago

The functions zip_source_zip_file and zip_source_zip take two parameters each of type zip_t* named archive and srcarchive.

zip_source_t* zip_source_zip_file(zip_t* archive, zip_t* srcarchive, zip_uint64_t srcidx, zip_flags_t flags, zip_uint64_t start, zip_int64_t length, const char* password);
zip_source_t* zip_source_zip(zip_t* archive, zip_t* srcarchive, zip_uint64_t srcidx, zip_flags_t flags, zip_uint64_t start, zip_int64_t len);

The role of the first archive parameter is not immediately obvious. Reading the documentation carefully, it seems that the archive parameter is used when setting the error status of a zip_t object in case of error. This raises a couple of questions:

  1. Is it possible for the archive and srcarchive pointers to point to the same zip_t object?
  2. Is it possible to pass a NULL pointer to the archive parameter? If yes, what happens in case of error? Is the error status then stored in *srcarchive or lost entirely?
  3. Would it make sense to rename the archive parameters to something more indicative of its role, like maybe error, since it is playing a similar role as the error parameter in zip_source_zip_file_create and zip_source_zip_create?

The role of the password parameter passed to zip_source_zip_file and zip_source_zip_file_create is not described in the documentation. Is it possible to pass a NULL pointer as password? If yes, does it have the same effect as passing an empty string?

dillof commented 3 weeks ago

To answer your questions:

  1. Yes, that's no problem.
  2. No, passing NULL would crash. If you pass NULL as error to zip_source_zip_file_create(), the error information is lost.
  3. There are more zip_source_foo() / zip_source_foo_create() function pairs that all take archive or error as arguments, and keeping them consistent seems like a good idea. We'll add a paragraph to all their man pages to document these arguments.

If you pass NULL as password, the default password for the archive is used. If you pass the empty string, that is used as password.

fdegros commented 3 weeks ago

If you pass NULL as password, the default password for the archive is used. If you pass the empty string, that is used as password.

This might be worth documenting very clearly. In particular, this is a place where a NULL string and an empty string have different meanings and effects. This can be quite tricky.

This is also subtly different from the way zip_set_default_password treats its password parameter. See zip_set_default_password's documentation:

If password is NULL or the empty string, the default password is unset.

In that case, a NULL or an empty string have the same meaning and effect.