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

[Laszlo Papp]

        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@...>
---
 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);
  }
  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);
  }
  for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) {
  if(dp->d_ino) {
--
1.6.5

[Dan McGee]

On Fri, Oct 23, 2009 at 3:54 PM, Laszlo Papp <djszapi2@...> 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@...>
> ---

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
>
>
>

[Laszlo Papp]

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'.

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@...>
---
 lib/libalpm/trans.c |    5 ++++-
 lib/libalpm/util.c  |   11 +++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c
index aea71db..22ff3fd 100644
--- a/lib/libalpm/trans.c
+++ b/lib/libalpm/trans.c
@@ -243,6 +243,7 @@ int SYMEXPORT alpm_trans_interrupt()
 int SYMEXPORT alpm_trans_release()
 {
  pmtrans_t *trans;
+ int fd;

  ALPM_LOG_FUNC;

@@ -261,7 +262,9 @@ int SYMEXPORT alpm_trans_release()
  /* unlock db */
  if(!nolock_flag) {
  if(handle->lckfd != -1) {
- while(close(handle->lckfd) == -1 && errno == EINTR);
+ do {
+ fd = close(handle->lckfd);
+ } while(fd == -1 && pm_errno == EINTR);
  handle->lckfd = -1;
  }
  if(_alpm_lckrm()) {
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
index 1340da9..cf0deed 100644
--- a/lib/libalpm/util.c
+++ b/lib/libalpm/util.c
@@ -224,8 +224,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 && pm_errno == EINTR);
  if(fd > 0) {
  FILE *f = fdopen(fd, "w");
  fprintf(f, "%ld\n", (long)getpid());
@@ -401,7 +402,7 @@ int _alpm_rmrf(const char *path)
  }
  }
  } else {
- if((dirp = opendir(path)) == (DIR *)-1) {
+ if((dirp = opendir(path)) == NULL) {
  return(1);
  }
  for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) {
@@ -522,7 +523,9 @@ int _alpm_run_chroot(const char *root, const char *cmd)
  /* this code runs for the parent only (wait on the child) */
  pid_t retpid;
  int status;
- while((retpid = waitpid(pid, &status, 0)) == -1 && errno == EINTR);
+ do {
+ retpid = waitpid(pid, &status, 0);
+ } while(retpid == -1 && pm_errno == EINTR);
  if(retpid == -1) {
  _alpm_log(PM_LOG_ERROR, _("call to waitpid failed (%s)\n"),
           strerror(errno));
--
1.6.5

[Xavier Chantry-2]

On Sun, Oct 25, 2009 at 5:15 AM, Laszlo Papp <djszapi2@...> wrote:
> 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'.
>
> The ambiguity while cycle with EINTR condition was refactored for a do {} while ()
> cycle to be easier to read/understand
>

And where is the explanation for the errno -> pm_errno change ?

[Laszlo Papp]

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'.

The ambiguity while cycle with EINTR condition was refactored for a do {} while ()
cycle to be easier to read/understand

The 'errno == EINTR' condition examinations were changed for 'pm_errno == EINTR'
in the api, in trans.c and util.c, where the do {} while {} changings happened too.

Signed-off-by: Laszlo Papp <djszapi@...>
---
 lib/libalpm/trans.c |    5 ++++-
 lib/libalpm/util.c  |   11 +++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c
index aea71db..22ff3fd 100644
--- a/lib/libalpm/trans.c
+++ b/lib/libalpm/trans.c
@@ -243,6 +243,7 @@ int SYMEXPORT alpm_trans_interrupt()
 int SYMEXPORT alpm_trans_release()
 {
  pmtrans_t *trans;
+ int fd;

  ALPM_LOG_FUNC;

@@ -261,7 +262,9 @@ int SYMEXPORT alpm_trans_release()
  /* unlock db */
  if(!nolock_flag) {
  if(handle->lckfd != -1) {
- while(close(handle->lckfd) == -1 && errno == EINTR);
+ do {
+ fd = close(handle->lckfd);
+ } while(fd == -1 && pm_errno == EINTR);
  handle->lckfd = -1;
  }
  if(_alpm_lckrm()) {
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
index 1340da9..cf0deed 100644
--- a/lib/libalpm/util.c
+++ b/lib/libalpm/util.c
@@ -224,8 +224,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 && pm_errno == EINTR);
  if(fd > 0) {
  FILE *f = fdopen(fd, "w");
  fprintf(f, "%ld\n", (long)getpid());
@@ -401,7 +402,7 @@ int _alpm_rmrf(const char *path)
  }
  }
  } else {
- if((dirp = opendir(path)) == (DIR *)-1) {
+ if((dirp = opendir(path)) == NULL) {
  return(1);
  }
  for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) {
@@ -522,7 +523,9 @@ int _alpm_run_chroot(const char *root, const char *cmd)
  /* this code runs for the parent only (wait on the child) */
  pid_t retpid;
  int status;
- while((retpid = waitpid(pid, &status, 0)) == -1 && errno == EINTR);
+ do {
+ retpid = waitpid(pid, &status, 0);
+ } while(retpid == -1 && pm_errno == EINTR);
  if(retpid == -1) {
  _alpm_log(PM_LOG_ERROR, _("call to waitpid failed (%s)\n"),
           strerror(errno));
--
1.6.5

[Dan McGee]

On Sun, Oct 25, 2009 at 5:33 AM, Laszlo Papp <djszapi2@...> wrote:
> 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'.
>
> The ambiguity while cycle with EINTR condition was refactored for a do {} while ()
> cycle to be easier to read/understand
>
> The 'errno == EINTR' condition examinations were changed for 'pm_errno == EINTR'
> in the api, in trans.c and util.c, where the do {} while {} changings happened too.

What the heck? Do you understand C and the standard API or are you
just doing blind "make it look good" patches?

the open() *system* function will never ever EVER set pm_errno. I'm so
confused what you are trying to do there...

-Dan

[Laszlo Papp]

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'.

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@...>
---

I was very supprised on this post. It was very rude and not constructive in this
way even I did bad thing in it. I've never said I'm proficient and I do failures
sometimes because of missunderstanding e.g., and I don't think it was a big
error that can't be corrected. but Dan! I spend(spent?) with my freetime
with helping you and pacman... I've never experienced this nowhere in any community
when some one try to be helpful and he is named foolish and crappy. I think so it's
absolutely uncorrect and unfair... Why is it hard to keep the contructive way ?
It would be much easier for you and me too.

 lib/libalpm/trans.c |    5 ++++-
 lib/libalpm/util.c  |   11 +++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c
index aea71db..59b0c87 100644
--- a/lib/libalpm/trans.c
+++ b/lib/libalpm/trans.c
@@ -243,6 +243,7 @@ int SYMEXPORT alpm_trans_interrupt()
 int SYMEXPORT alpm_trans_release()
 {
  pmtrans_t *trans;
+ int fd;

  ALPM_LOG_FUNC;

@@ -261,7 +262,9 @@ int SYMEXPORT alpm_trans_release()
  /* unlock db */
  if(!nolock_flag) {
  if(handle->lckfd != -1) {
- while(close(handle->lckfd) == -1 && errno == EINTR);
+ do {
+ fd = close(handle->lckfd);
+ } while(fd == -1 && errno == EINTR);
  handle->lckfd = -1;
  }
  if(_alpm_lckrm()) {
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
index 1340da9..0719096 100644
--- a/lib/libalpm/util.c
+++ b/lib/libalpm/util.c
@@ -224,8 +224,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());
@@ -401,7 +402,7 @@ int _alpm_rmrf(const char *path)
  }
  }
  } else {
- if((dirp = opendir(path)) == (DIR *)-1) {
+ if((dirp = opendir(path)) == NULL) {
  return(1);
  }
  for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) {
@@ -522,7 +523,9 @@ int _alpm_run_chroot(const char *root, const char *cmd)
  /* this code runs for the parent only (wait on the child) */
  pid_t retpid;
  int status;
- while((retpid = waitpid(pid, &status, 0)) == -1 && errno == EINTR);
+ do {
+ retpid = waitpid(pid, &status, 0);
+ } while(retpid == -1 && errno == EINTR);
  if(retpid == -1) {
  _alpm_log(PM_LOG_ERROR, _("call to waitpid failed (%s)\n"),
           strerror(errno));
--
1.6.5.1

[Dan McGee]

On Sun, Oct 25, 2009 at 10:49 PM, Laszlo Papp <djszapi2@...> wrote:

> 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'.
>
> 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@...>
> ---
>
> I was very supprised on this post. It was very rude and not constructive in this
> way even I did bad thing in it. I've never said I'm proficient and I do failures
> sometimes because of missunderstanding e.g., and I don't think it was a big
> error that can't be corrected. but Dan! I spend(spent?) with my freetime
> with helping you and pacman... I've never experienced this nowhere in any community
> when some one try to be helpful and he is named foolish and crappy. I think so it's
> absolutely uncorrect and unfair... Why is it hard to keep the contructive way ?
> It would be much easier for you and me too.

I'm glad the context was preserved here so I know specifically what I
said that was out of line, but I apologize if you took it that way.

With that said, I'm going to lay out some things that may or may not
be what you wanted to hear. But it is for my sake that I say these
things, so I apologize if we can't all agree.

If a patch is "trivial" but takes three review cycles and continues to
be problematic, and it keeps getting resubmitted without much evidence
of it changing for the better (and even adding more broken stuff),
people on this list start to get a bit rough on the edges. We aren't
doing it out of spite; we do it because our time is worth something
too. Many of us could have taken this patch, fixed the things I
suggested, and got it in, but we like to not steal the credit from new
people. This one has just not been easy. What more could we have said
to be constructive? I'm not sure.

Foolish? Crappy? Did those words leave my mouth? I try to check my ego
at the door as much as possible when reading mails on this list. I
asked a legitimate question, maybe in a slightly sarcastic way as this
was the third or so time this patch had crossed my inbox.

We're all volunteers here. I try to help out new people as much as
possible, and I realize the code isn't always easy to get at first
glance, but we only have so much time to hand hold and we all like to
spend at least some of our time doing coding on our own (and not
having to write novels like this email).

I will not continue on this topic after this email, so don't expect
another reply from me if anyone replies to this. I'd rather be coding
or something.

-Dan

[Laszlo Papp-2]

On Wed, Oct 28, 2009 at 3:28 AM, Dan McGee <dpmcgee@...> wrote:

> On Sun, Oct 25, 2009 at 10:49 PM, Laszlo Papp <djszapi2@...> wrote:
> > 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'.
> >
> > 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@...>
> > ---
> >
> > I was very supprised on this post. It was very rude and not constructive
> in this
> > way even I did bad thing in it. I've never said I'm proficient and I do
> failures
> > sometimes because of missunderstanding e.g., and I don't think it was a
> big
> > error that can't be corrected. but Dan! I spend(spent?) with my freetime
> > with helping you and pacman... I've never experienced this nowhere in any
> community
> > when some one try to be helpful and he is named foolish and crappy. I
> think so it's
> > absolutely uncorrect and unfair... Why is it hard to keep the contructive
> way ?
> > It would be much easier for you and me too.
>
> I'm glad the context was preserved here so I know specifically what I
> said that was out of line, but I apologize if you took it that way.
>
> With that said, I'm going to lay out some things that may or may not
> be what you wanted to hear. But it is for my sake that I say these
> things, so I apologize if we can't all agree.
>
> If a patch is "trivial" but takes three review cycles and continues to
> be problematic, and it keeps getting resubmitted without much evidence
> of it changing for the better (and even adding more broken stuff),
> people on this list start to get a bit rough on the edges. We aren't
> doing it out of spite; we do it because our time is worth something
> too.

I try to get more knowledge and be more useful from day to day, so I'm not
an expert now, but if these patches are trivial, why are these issues in the
codebase, or why weren't they corrected ? And at last why didn't Xavier e.g.
mention it, just a missing describtion for that nonsense correction ? (I
know you won't answer Dan, and i don't wait any response)


> Many of us could have taken this patch, fixed the things I
> suggested, and got it in, but we like to not steal the credit from new
> people. This one has just not been easy. What more could we have said
> to be constructive? I'm not sure.
>
>
Nice to hear you don't like to steal the credit :)


> Foolish? Crappy? Did those words leave my mouth? I try to check my ego
> at the door as much as possible when reading mails on this list. I
> asked a legitimate question, maybe in a slightly sarcastic way as this
> was the third or so time this patch had crossed my inbox.
>
>
No need to be sarcastic, it can be very desctructive for a newbie. I saw
patches more than three times touch the inbox for that I thought it's a
simple fix, but I don't say it's trivial, because it's trivial for me but
for others not.


> We're all volunteers here. I try to help out new people as much as
> possible, and I realize the code isn't always easy to get at first
> glance, but we only have so much time to hand hold and we all like to
> spend at least some of our time doing coding on our own (and not
> having to write novels like this email).
>
>
No need such a mail, if you're not sarcastic, but helpful, I think so. (you
could give more good advices, instead of talking about this)


> I will not continue on this topic after this email, so don't expect
> another reply from me if anyone replies to this. I'd rather be coding
> or something.
>
> -Dan
>
>
Good Luck!

Best Regards,
Laszlo Papp