[pacman-dev] [PATCH] added doxygen documentation
Dan McGee
dpmcgee at gmail.com
Mon Nov 21 23:27:26 EST 2011
On Mon, Nov 21, 2011 at 9:35 PM, <andrew.gregory.8 at gmail.com> wrote:
> From: Andrew Gregory <andrew.gregory.8 at gmail.com>
>
> Made existing documentation more consistent and added
> documentation where there was none. A couple functions
> I did not fully understand still need documentation and
> are marked with 'TODO'.
>
> Signed-off-by: Andrew Gregory <andrew.gregory.8 at gmail.com>
> ---
> lib/libalpm/util.c | 122 +++++++++++++++++++++++++++++++++++++++++----------
> 1 files changed, 98 insertions(+), 24 deletions(-)
>
> diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
> index f5b7968..35bc059 100644
> --- a/lib/libalpm/util.c
> +++ b/lib/libalpm/util.c
> @@ -173,8 +173,11 @@ cleanup:
> return ret;
> }
>
> -/* Trim whitespace and newlines from a string
> -*/
> +/** Trim leading and trailing whitespace, including newlines, from a string.
> + * Modifies str in place.
> + * @param str a string to trim
> + * @return str
> + */
> char *_alpm_strtrim(char *str)
> {
> char *pch = str;
> @@ -190,6 +193,7 @@ char *_alpm_strtrim(char *str)
> if(pch != str) {
> size_t len = strlen(pch);
> if(len) {
> + /* move the remaining string to the beginning of str */
> memmove(str, pch, len + 1);
> } else {
> *str = '\0';
> @@ -210,8 +214,7 @@ char *_alpm_strtrim(char *str)
> return str;
> }
>
> -/**
> - * Trim trailing newline from a string (if one exists).
> +/** Trim trailing newlines from a string (if one exists).
you changed to a plural, but kept "if one"- now we're totally
split-brained here, so this probably needs fixing to if in fact the
function removes all trailing newlines.
> * @param str a single line of text
> * @return the length of the trimmed string
> */
> @@ -232,8 +235,7 @@ size_t _alpm_strip_newline(char *str)
>
> /* Compression functions */
>
> -/**
> - * Open an archive for reading and perform the necessary boilerplate.
> +/** Open an archive for reading and perform the necessary boilerplate.
> * This takes care of creating the libarchive 'archive' struct, setting up
> * compression and format options, opening a file descriptor, setting up the
> * buffer size, and performing a stat on the path once opened.
> @@ -290,9 +292,7 @@ int _alpm_open_archive(alpm_handle_t *handle, const char *path,
> return fd;
> }
>
> -/**
> - * @brief Unpack a specific file in an archive.
> - *
> +/** Unpack a specific file in an archive.
> * @param handle the context handle
> * @param archive the archive to unpack
> * @param prefix where to extract the files
> @@ -313,15 +313,12 @@ int _alpm_unpack_single(alpm_handle_t *handle, const char *archive,
> return ret;
> }
>
> -/**
> - * @brief Unpack a list of files in an archive.
> - *
> +/** Unpack a list of files in an archive.
> * @param handle the context handle
> * @param path the archive to unpack
> * @param prefix where to extract the files
> * @param list a list of files within the archive to unpack or NULL for all
> * @param breakfirst break after the first entry found
> - *
> * @return 0 on success, 1 on failure
> */
> int _alpm_unpack(alpm_handle_t *handle, const char *path, const char *prefix,
> @@ -421,7 +418,10 @@ cleanup:
> return ret;
> }
>
> -/* does the same thing as 'rm -rf' */
> +/** Recursively removes a path similar to 'rm -rf'.
> + * @param path path to remove
> + * @return 0 on success, positive int on failure
If you saw return codes other than 1, then they should all be
documented. Otherwise the normal "@return 0 on success, 1 on failure"
is pretty standard.
> + */
> int _alpm_rmrf(const char *path)
> {
> int errflag = 0;
> @@ -464,8 +464,7 @@ int _alpm_rmrf(const char *path)
> return 0;
> }
>
> -/**
> - * Determine if there are files in a directory
> +/** Determine if there are files in a directory.
> * @param handle the context handle
> * @param path the full absolute directory path
> * @param full_count whether to return an exact count of files
> @@ -505,7 +504,13 @@ ssize_t _alpm_files_in_directory(alpm_handle_t *handle, const char *path,
> closedir(dir);
> return files;
> }
> -
Why no newline between functions? Please put this back.
> +/** Write formatted message to log.
> + * @param handle the context handle
> + * @param format formatted string to write out
> + * @param args formatting arguments
> + * @return 0 or number of characters written on success, negative int on
> + * failure
Once again, if we are doing things other than -1, document them
individually please.
> + */
> int _alpm_logaction(alpm_handle_t *handle, const char *fmt, va_list args)
> {
> int ret = 0;
> @@ -537,6 +542,12 @@ int _alpm_logaction(alpm_handle_t *handle, const char *fmt, va_list args)
> return ret;
> }
>
> +/** TODO.
Execute the given command with the given arguments under a chroot call
to handle->chroot.
> + * @param handle the context handle
> + * @param path
path should really be renamed to "command" or "cmd", this is
deceiving. Make sure it gets renamed in the header too.
> + * @param argv
> + * @return
looks like 0 success, 1 error here
> + */
> int _alpm_run_chroot(alpm_handle_t *handle, const char *path, char *const argv[])
> {
> pid_t pid;
> @@ -656,6 +667,10 @@ cleanup:
> return retval;
> }
>
> +/** Run ldconfig in a chroot.
> + * @param handle the context handle
> + * @return 0
Wow. We should at least be returning the value from _alpm_run_chroot,
I'll fix that, even if the callers of this aren't looking at it yet.
Good catch by way of documentation.
> + */
> int _alpm_ldconfig(alpm_handle_t *handle)
> {
> char line[PATH_MAX];
> @@ -674,8 +689,13 @@ int _alpm_ldconfig(alpm_handle_t *handle)
> return 0;
> }
>
> -/* Helper function for comparing strings using the
> - * alpm "compare func" signature */
> +/** Helper function for comparing strings using the alpm "compare func"
> + * signature.
> + * @param s1 string to be compared
> + * @param s1 string to be compared
s2. and perhaps "first string...", "second string..."
> + * @return 0 if strings are equal, positive int if first unequal character
> + * has a greater value in s1, negative if it has a greater value in s2
> + */
> int _alpm_str_cmp(const void *s1, const void *s2)
> {
> return strcmp(s1, s2);
> @@ -784,6 +804,11 @@ int _alpm_lstat(const char *path, struct stat *buf)
> }
>
> #ifdef HAVE_LIBSSL
> +/** Compute the md5 of a file.
"MD5 message digest" would be more proper, and should use caps everywhere.
> + * @param path file path to md5
"to md5"? That is just confusing. "file path of file to compute digest
on" would make more sense, and even that is pretty wordy.
> + * @param output string to hold computed md5
> + * @return 0 on success, 0 on file open error, 2 on file read error
> + */
> static int md5_file(const char *path, unsigned char output[16])
> {
> MD5_CTX ctx;
> @@ -820,6 +845,12 @@ static int md5_file(const char *path, unsigned char output[16])
> }
>
> /* third param is so we match the PolarSSL definition */
> +/** Compute the sha2 of a file.
Same concerns as above, "SHA-224 or SHA-256 message digest". and all that.
> + * @param path file path to sha2
> + * @param output string to hold computed sha2
> + * @param is224 use sha224 instead of sha256
> + * @return 0 on success, 1 on file open error, 2 on file read error
> + */
> static int sha2_file(const char *path, unsigned char output[32], int is224)
> {
> SHA256_CTX ctx;
> @@ -936,6 +967,13 @@ char SYMEXPORT *alpm_compute_sha256sum(const char *filename)
> return sha256sum;
> }
>
> +/** Calculates a files md5 or sha2 and compares it to an expected value.
file's
> + * @param filepath path of the file to check
> + * @param expected hash value to compare against
> + * @param type hash type to use
> + * @return 0 if file matches the expected hash, 1 if they do not match, -1 on
> + * error
> + */
> int _alpm_test_checksum(const char *filepath, const char *expected,
> enum _alpm_csum type)
> {
> @@ -963,6 +1001,12 @@ int _alpm_test_checksum(const char *filepath, const char *expected,
> }
>
> /* Note: does NOT handle sparse files on purpose for speed. */
> +/** TODO.
> + * Does not handle sparse files on purpose for speed.
> + * @param a
> + * @param b
> + * @return
> + */
> int _alpm_archive_fgets(struct archive *a, struct archive_read_buffer *b)
> {
> /* ensure we start populating our line buffer at the beginning */
> @@ -1058,6 +1102,13 @@ cleanup:
> }
> }
>
> +/** Parse a package db entry.
I wouldn't consider this a "db entry". I'd call it a "full package
specifier" or something; we tend to think of db entries as the files
themselves. Maybe given an example:
target is a string such as "pacman-4.0.1-2", "pacman-4.0.1-2/", or
"pacman-4.0.1-2/desc".
> + * @param target db entry to parse
> + * @param name string pointer to hold package name
> + * @param version string pointer to hold package version
> + * @param name_hash string pointer to hold package name hash
Except that this one isn't a string, its an unsigned long. Putting the
type name is also redundant; I'd drop it in all three of these as
Doxygen will document that for you.
> + * @return 0 on success, -1 on error
> + */
> int _alpm_splitname(const char *target, char **name, char **version,
> unsigned long *name_hash)
> {
> @@ -1110,8 +1161,7 @@ int _alpm_splitname(const char *target, char **name, char **version,
> return 0;
> }
>
> -/**
> - * Hash the given string to an unsigned long value.
> +/** Hash the given string to an unsigned long value.
> * This is the standard sdbm hashing algorithm.
> * @param str string to hash
> * @return the hash value of the given string
> @@ -1131,6 +1181,10 @@ unsigned long _alpm_hash_sdbm(const char *str)
> return hash;
> }
>
> +/** Convert a string to a file offset.
Although "off_t" does in fact represent this quantity, we actually use
it for file sizes (where it is also used). I'd just say off_t in the
description, and also add a bit more that you can glean from the
comments in the method:
This parses bare positive integers only.
> + * @param line string to convert
> + * @return file offset on success, -1 on error
> + */
> off_t _alpm_strtoofft(const char *line)
> {
> char *end;
> @@ -1156,6 +1210,10 @@ off_t _alpm_strtoofft(const char *line)
> return (off_t)result;
> }
>
> +/** Parses a date into an alpm_time_t struct.
> + * @param line date to parse
> + * @return time struct on success, 0 on error
> + */
> alpm_time_t _alpm_parsedate(const char *line)
> {
> char *end;
> @@ -1189,8 +1247,7 @@ alpm_time_t _alpm_parsedate(const char *line)
> return (alpm_time_t)result;
> }
>
> -/**
> - * Wrapper around access() which takes a dir and file argument
> +/** Wrapper around access() which takes a dir and file argument
> * separately and generates an appropriate error message.
> * If dir is NULL file will be treated as the whole path.
> * @param handle an alpm handle
> @@ -1199,7 +1256,6 @@ alpm_time_t _alpm_parsedate(const char *line)
> * @param amode access mode as described in access()
> * @return int value returned by access()
> */
> -
> int _alpm_access(alpm_handle_t *handle, const char *dir, const char *file, int amode)
> {
> size_t len = 0;
> @@ -1240,6 +1296,13 @@ int _alpm_access(alpm_handle_t *handle, const char *dir, const char *file, int a
> return ret;
> }
>
> +/** Checks whether a string matches a shell wildcard pattern.
> + * Wrapper around fnmatch.
> + * @param pattern pattern to match aganist
> + * @param string string to check against pattern
> + * @return 0 if string matches pattern, non-zero if they don't match and on
> + * error
> + */
> int _alpm_fnmatch(const void *pattern, const void *string)
> {
> return fnmatch(pattern, string, 0);
> @@ -1247,6 +1310,11 @@ int _alpm_fnmatch(const void *pattern, const void *string)
>
> #ifndef HAVE_STRNDUP
> /* A quick and dirty implementation derived from glibc */
> +/** Determines the length of a fixed-size string.
> + * @param s string to be measured
> + * @param max maximum number of characters to search for the string end
> + * @return length of s or max, whichever is smaller
> + */
> static size_t strnlen(const char *s, size_t max)
> {
> register const char *p;
> @@ -1254,6 +1322,12 @@ static size_t strnlen(const char *s, size_t max)
> return (p - s);
> }
>
> +/** Copies a string.
> + * Returned string needs to be freed
> + * @param s string to be copied
> + * @param n maximum number of characters to copy
> + * @return pointer to the new string on success, NULL on error
> + */
> char *strndup(const char *s, size_t n)
> {
> size_t len = strnlen(s, n);
> --
> 1.7.7.3
Thanks for all of this! Definitely will apply after you address my comments.
-Dan
More information about the pacman-dev
mailing list