[pacman-dev] [PATCH 2/2] Fix some errors that cppcheck gave back
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@archlinux.us> --- 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
On Fri, Oct 23, 2009 at 3:54 PM, Laszlo Papp <djszapi2@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@archlinux.us> ---
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
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) 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
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@archlinux.us> --- 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
On Sun, Oct 25, 2009 at 5:15 AM, Laszlo Papp <djszapi2@gmail.com> 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 ?
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@archlinux.us> --- 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
On Sun, Oct 25, 2009 at 5:33 AM, Laszlo Papp <djszapi2@gmail.com> 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
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@archlinux.us> --- 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
On Sun, Oct 25, 2009 at 10:49 PM, Laszlo Papp <djszapi2@gmail.com> 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@archlinux.us> ---
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
On Wed, Oct 28, 2009 at 3:28 AM, Dan McGee <dpmcgee@gmail.com> wrote:
opendir(path)) == (DIR *)-1 is maybe the result of miss understanding of
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@archlinux.us> ---
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
On Sun, Oct 25, 2009 at 10:49 PM, Laszlo Papp <djszapi2@gmail.com> wrote: the man 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
participants (4)
-
Dan McGee
-
Laszlo Papp
-
Laszlo Papp
-
Xavier