[pacman-dev] [PATCH 2/2] Fix some errors that cppcheck gave back

[Dan McGee]

On Fri, Oct 23, 2009 at 3:54 PM, Laszlo Papp <djszapi2 at gmail.com> wrote:
>        Fix some warning that cppcheck gave back
>
>        * opendir(path)) == (DIR *)-1 is maybe the result of miss
>        understanding of the man page, if the opendir wasn't successful
>        it gives back NULL instead of '(DIR *)-1'.
>
>        * return(PM_ERR_NOT_A_DIR); was established instead of hard coding
>        numbers for return statement.
>
>        * The ambiguity while cycle with EINTR condition was refactored
>        for a do {} while () cycle to be easier to read/understand
>
> Signed-off-by: Laszlo Papp <djszapi at archlinux.us>
> ---

Cool, these are good catches. Just a few suggestions.
1. Stop tabbing your commit message
2. do/while is a bit more understandable; can you fix the other two
places we use that construct as well? (lib/libalpm/trans.c,
src/pacman/util.c, one more in lib/libalpm/util.c)

>  lib/libalpm/util.c |   11 ++++++-----
>  src/pacman/util.c  |    4 ++--
>  2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
> index d910809..ad3dc2c 100644
> --- a/lib/libalpm/util.c
> +++ b/lib/libalpm/util.c
> @@ -212,8 +212,9 @@ int _alpm_lckmk()
>        _alpm_makepath(dir);
>        FREE(dir);
>
> -       while((fd = open(file, O_WRONLY | O_CREAT | O_EXCL, 0000)) == -1
> -                       && errno == EINTR);
> +       do {
> +               fd = open(file, O_WRONLY | O_CREAT | O_EXCL, 0000);
> +       } while(fd == -1 && errno == EINTR);
>        if(fd > 0) {
>                FILE *f = fdopen(fd, "w");
>                fprintf(f, "%ld\n", (long)getpid());
> @@ -315,7 +316,7 @@ int _alpm_unpack(const char *archive, const char *prefix, alpm_list_t *list, int
>
>                st = archive_entry_stat(entry);
>                entryname = archive_entry_pathname(entry);
> -
> +
>                if(S_ISREG(st->st_mode)) {
>                        archive_entry_set_perm(entry, 0644);
>                } else if(S_ISDIR(st->st_mode)) {
> @@ -389,8 +390,8 @@ int _alpm_rmrf(const char *path)
>                                }
>                        }
>                } else {
> -                       if((dirp = opendir(path)) == (DIR *)-1) {
> -                               return(1);
> +                       if((dirp = opendir(path)) == NULL) {
> +                               return(PM_ERR_NOT_A_DIR);
This is not how these error codes are intended to be used; instead
pmerrno_t should be set. However, since this is an internal function
only, we shouldn't be doing this (it should be handled only in a
exposed function that would fail if this particular call failed. Take
a look at the RET_ERR macro for an example.

>                        }
>                        for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) {
>                                if(dp->d_ino) {
> diff --git a/src/pacman/util.c b/src/pacman/util.c
> index 1143bef..e3dbfbb 100644
> --- a/src/pacman/util.c
> +++ b/src/pacman/util.c
> @@ -122,8 +122,8 @@ int rmrf(const char *path)
>                        return(1);
>                }
>
> -               if((dirp = opendir(path)) == (DIR *)-1) {
> -                       return(1);
> +               if((dirp = opendir(path)) == NULL) {
> +                       return(PM_ERR_NOT_A_DIR);
This doesn't make a whole lot of sense in the frontend.

>                }
>                for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) {
>                        if(dp->d_ino) {
> --
> 1.6.5
>
>
>