[pacman-dev] [PATCH 1/3] add _alpm_access() wrapper

[Dan McGee]

On Wed, Jul 6, 2011 at 12:34 PM, Florian Pritz <bluewind at xinu.at> wrote:
> This is a wrapper function for access() which logs some debug
> information and eases handling in case of split directory and filename.
>
> Signed-off-by: Florian Pritz <bluewind at xinu.at>
> ---
>  lib/libalpm/util.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/libalpm/util.h |    1 +
>  2 files changed, 50 insertions(+), 0 deletions(-)
>
> diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
> index cd7f19c..332f917 100644
> --- a/lib/libalpm/util.c
> +++ b/lib/libalpm/util.c
> @@ -917,6 +917,55 @@ long _alpm_parsedate(const char *line)
>        return atol(line);
>  }
>
> +/**
> + * 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
> + * @param dir directory path ending with and slash
> + * @param file filename
> + * @param amode access mode as describe in access()
"described"
> + * @return int value returned by access()
> + */
> +
> +int _alpm_access(alpm_handle_t *handle, const char *dir, const char *file, int amode)
> +{
> +       char *check_path = NULL;
> +       size_t len = 0;
> +       int ret = 0;
> +
> +       if (dir == NULL) {
> +               len = strlen(file) + 1;
> +               CALLOC(check_path, len, sizeof(char), RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> +               snprintf(check_path, len, "%s", file);
This adds a bit of unnecessry string copying (and could use STRDUP()
instead). However, I'd suggest simply dropping the whole first part of
this block and doing something like this:
if(dir) {
    char *check_path; (no need at all to set it to null, you are
allocing memory right away)
    /* next block */;
    ret = access(check_path, amode);
    free(check_path);
} else {
    dir = "";
    ret = access(file, amode);
}

+                               _alpm_log(handle, ALPM_LOG_DEBUG,
_("\"%s\" is not readable: %s\n"), check_path, strerror(errno));
can then become
+                               _alpm_log(handle, ALPM_LOG_DEBUG,
_("\"%s%s\" is not readable: %s\n"), dir, file, strerror(errno));

> +       } else {
> +               len = strlen(dir) + strlen(file) + 1;
> +               CALLOC(check_path, len, sizeof(char), RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> +               snprintf(check_path, len, "%s%s", dir, file);
> +       }
> +
> +       ret = access(check_path, amode);
> +
> +       if(ret != 0) {
> +               switch(amode) {
> +                       case R_OK:
> +                               _alpm_log(handle, ALPM_LOG_DEBUG, _("\"%s\" is not readable: %s\n"), check_path, strerror(errno));
> +                               break;
> +                       case W_OK:
> +                               _alpm_log(handle, ALPM_LOG_DEBUG, _("\"%s\" is not writeable: %s\n"), check_path, strerror(errno));
writable?
> +                               break;
> +                       case X_OK:
> +                               _alpm_log(handle, ALPM_LOG_DEBUG, _("\"%s\" is not executable: %s\n"), check_path, strerror(errno));
> +                               break;
> +                       case F_OK:
> +                               _alpm_log(handle, ALPM_LOG_DEBUG, _("\"%s\" does not exist: %s\n"), check_path, strerror(errno));
> +                               break;
> +               }
A switch looks good, but it prevents calls such as access(file, R_OK |
X_OK) which is valid. Do we care? You could just do four consecutive
if(amode & R_OK) {
    log()
}
statements. (Except apparently F_OK is defined to 0, so the F_OK check
needs to be equality, see /usr/include/unistd.h).

> +       }
> +       free(check_path);
> +       return ret;
> +}
> +
>  #ifndef HAVE_STRNDUP
>  /* A quick and dirty implementation derived from glibc */
>  static size_t strnlen(const char *s, size_t max)
> diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h
> index 450dac9..015ed5a 100644
> --- a/lib/libalpm/util.h
> +++ b/lib/libalpm/util.h
> @@ -115,6 +115,7 @@ unsigned long _alpm_hash_sdbm(const char *str);
>  long _alpm_parsedate(const char *line);
>  int _alpm_raw_cmp(const char *first, const char *second);
>  int _alpm_raw_ncmp(const char *first, const char *second, size_t max);
> +int _alpm_access(alpm_handle_t *handle, const char *dir, const char *file, int amode);
>
>  #ifndef HAVE_STRSEP
>  char *strsep(char **, const char *);
> --
> 1.7.6