[pacman-dev] [PATCH] alpm/handle.c: ensure handle is not NULL before proceeding
Many alpm_option_get/set_*() functions already check this and set pm_errno to the right value, but not all, so this improves consistency. Signed-off-by: Rémy Oudompheng <remy@archlinux.org> --- lib/libalpm/handle.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 80 insertions(+), 0 deletions(-) diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index d4ebe82..c37a0f9 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -302,6 +302,10 @@ int SYMEXPORT alpm_option_set_root(const char *root) ALPM_LOG_FUNC; + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return(-1); + } if(!root) { pm_errno = PM_ERR_WRONG_ARGS; return(-1); @@ -342,6 +346,10 @@ int SYMEXPORT alpm_option_set_dbpath(const char *dbpath) ALPM_LOG_FUNC; + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return(-1); + } if(!dbpath) { pm_errno = PM_ERR_WRONG_ARGS; return(-1); @@ -380,6 +388,10 @@ int SYMEXPORT alpm_option_add_cachedir(const char *cachedir) ALPM_LOG_FUNC; + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return(-1); + } if(!cachedir) { pm_errno = PM_ERR_WRONG_ARGS; return(-1); @@ -402,6 +414,10 @@ int SYMEXPORT alpm_option_add_cachedir(const char *cachedir) void SYMEXPORT alpm_option_set_cachedirs(alpm_list_t *cachedirs) { + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return; + } if(handle->cachedirs) FREELIST(handle->cachedirs); if(cachedirs) handle->cachedirs = cachedirs; } @@ -411,6 +427,10 @@ int SYMEXPORT alpm_option_remove_cachedir(const char *cachedir) char *vdata = NULL; char *newcachedir; size_t cachedirlen; + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return(-1); + } /* verify cachedir ends in a '/' */ cachedirlen = strlen(cachedir); if(cachedir[cachedirlen-1] != '/') { @@ -434,6 +454,10 @@ int SYMEXPORT alpm_option_set_logfile(const char *logfile) ALPM_LOG_FUNC; + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return(-1); + } if(!logfile) { pm_errno = PM_ERR_WRONG_ARGS; return(-1); @@ -456,16 +480,28 @@ int SYMEXPORT alpm_option_set_logfile(const char *logfile) void SYMEXPORT alpm_option_set_usesyslog(int usesyslog) { + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return; + } handle->usesyslog = usesyslog; } void SYMEXPORT alpm_option_add_noupgrade(const char *pkg) { + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return; + } handle->noupgrade = alpm_list_add(handle->noupgrade, strdup(pkg)); } void SYMEXPORT alpm_option_set_noupgrades(alpm_list_t *noupgrade) { + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return; + } if(handle->noupgrade) FREELIST(handle->noupgrade); if(noupgrade) handle->noupgrade = noupgrade; } @@ -473,6 +509,10 @@ void SYMEXPORT alpm_option_set_noupgrades(alpm_list_t *noupgrade) int SYMEXPORT alpm_option_remove_noupgrade(const char *pkg) { char *vdata = NULL; + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return(-1); + } handle->noupgrade = alpm_list_remove_str(handle->noupgrade, pkg, &vdata); if(vdata != NULL) { FREE(vdata); @@ -495,6 +535,10 @@ void SYMEXPORT alpm_option_set_noextracts(alpm_list_t *noextract) int SYMEXPORT alpm_option_remove_noextract(const char *pkg) { char *vdata = NULL; + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return(-1); + } handle->noextract = alpm_list_remove_str(handle->noextract, pkg, &vdata); if(vdata != NULL) { FREE(vdata); @@ -505,11 +549,19 @@ int SYMEXPORT alpm_option_remove_noextract(const char *pkg) void SYMEXPORT alpm_option_add_ignorepkg(const char *pkg) { + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return; + } handle->ignorepkg = alpm_list_add(handle->ignorepkg, strdup(pkg)); } void SYMEXPORT alpm_option_set_ignorepkgs(alpm_list_t *ignorepkgs) { + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return; + } if(handle->ignorepkg) FREELIST(handle->ignorepkg); if(ignorepkgs) handle->ignorepkg = ignorepkgs; } @@ -517,6 +569,10 @@ void SYMEXPORT alpm_option_set_ignorepkgs(alpm_list_t *ignorepkgs) int SYMEXPORT alpm_option_remove_ignorepkg(const char *pkg) { char *vdata = NULL; + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return(-1); + } handle->ignorepkg = alpm_list_remove_str(handle->ignorepkg, pkg, &vdata); if(vdata != NULL) { FREE(vdata); @@ -527,11 +583,19 @@ int SYMEXPORT alpm_option_remove_ignorepkg(const char *pkg) void SYMEXPORT alpm_option_add_ignoregrp(const char *grp) { + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return; + } handle->ignoregrp = alpm_list_add(handle->ignoregrp, strdup(grp)); } void SYMEXPORT alpm_option_set_ignoregrps(alpm_list_t *ignoregrps) { + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return; + } if(handle->ignoregrp) FREELIST(handle->ignoregrp); if(ignoregrps) handle->ignoregrp = ignoregrps; } @@ -539,6 +603,10 @@ void SYMEXPORT alpm_option_set_ignoregrps(alpm_list_t *ignoregrps) int SYMEXPORT alpm_option_remove_ignoregrp(const char *grp) { char *vdata = NULL; + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return(-1); + } handle->ignoregrp = alpm_list_remove_str(handle->ignoregrp, grp, &vdata); if(vdata != NULL) { FREE(vdata); @@ -549,17 +617,29 @@ int SYMEXPORT alpm_option_remove_ignoregrp(const char *grp) void SYMEXPORT alpm_option_set_arch(const char *arch) { + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return; + } if(handle->arch) FREE(handle->arch); if(arch) handle->arch = strdup(arch); } void SYMEXPORT alpm_option_set_usedelta(int usedelta) { + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return; + } handle->usedelta = usedelta; } void SYMEXPORT alpm_option_set_checkspace(int checkspace) { + if (handle == NULL) { + pm_errno = PM_ERR_HANDLE_NULL; + return; + } handle->checkspace = checkspace; } -- 1.7.4.2
On 2011/3/28 Rémy Oudompheng <remyoudompheng@gmail.com> wrote:
Many alpm_option_get/set_*() functions already check this and set pm_errno to the right value, but not all, so this improves consistency.
I just noticed the ASSERT..RET_ERR construct, should it be the preferred way of implementing such things ? -- Rémy.
On Mon, Mar 28, 2011 at 9:26 PM, Rémy Oudompheng <remyoudompheng@gmail.com> wrote:
On 2011/3/28 Rémy Oudompheng <remyoudompheng@gmail.com> wrote:
Many alpm_option_get/set_*() functions already check this and set pm_errno to the right value, but not all, so this improves consistency.
I just noticed the ASSERT..RET_ERR construct, should it be the preferred way of implementing such things ?
I was going to say a macro would be nice for that repetitive stuff. And indeed we already have them and there is already that code : ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1));
Many alpm_option_get/set_*() functions already check this and set pm_errno to the right value, but not all, so this improves consistency. Signed-off-by: Rémy Oudompheng <remy@archlinux.org> --- lib/libalpm/handle.c | 23 +++++++++++++++++++++++ lib/libalpm/util.h | 4 ++++ 2 files changed, 27 insertions(+), 0 deletions(-) diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index d4ebe82..d8ce9fd 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -302,6 +302,8 @@ int SYMEXPORT alpm_option_set_root(const char *root) ALPM_LOG_FUNC; + ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1)); + if(!root) { pm_errno = PM_ERR_WRONG_ARGS; return(-1); @@ -342,6 +344,7 @@ int SYMEXPORT alpm_option_set_dbpath(const char *dbpath) ALPM_LOG_FUNC; + ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1)); if(!dbpath) { pm_errno = PM_ERR_WRONG_ARGS; return(-1); @@ -380,6 +383,7 @@ int SYMEXPORT alpm_option_add_cachedir(const char *cachedir) ALPM_LOG_FUNC; + ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1)); if(!cachedir) { pm_errno = PM_ERR_WRONG_ARGS; return(-1); @@ -402,6 +406,7 @@ int SYMEXPORT alpm_option_add_cachedir(const char *cachedir) void SYMEXPORT alpm_option_set_cachedirs(alpm_list_t *cachedirs) { + ASSERT(handle != NULL, RET_ERR_VOID(PM_ERR_HANDLE_NULL)); if(handle->cachedirs) FREELIST(handle->cachedirs); if(cachedirs) handle->cachedirs = cachedirs; } @@ -411,6 +416,7 @@ int SYMEXPORT alpm_option_remove_cachedir(const char *cachedir) char *vdata = NULL; char *newcachedir; size_t cachedirlen; + ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1)); /* verify cachedir ends in a '/' */ cachedirlen = strlen(cachedir); if(cachedir[cachedirlen-1] != '/') { @@ -434,6 +440,7 @@ int SYMEXPORT alpm_option_set_logfile(const char *logfile) ALPM_LOG_FUNC; + ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1)); if(!logfile) { pm_errno = PM_ERR_WRONG_ARGS; return(-1); @@ -456,16 +463,19 @@ int SYMEXPORT alpm_option_set_logfile(const char *logfile) void SYMEXPORT alpm_option_set_usesyslog(int usesyslog) { + ASSERT(handle != NULL, RET_ERR_VOID(PM_ERR_HANDLE_NULL)); handle->usesyslog = usesyslog; } void SYMEXPORT alpm_option_add_noupgrade(const char *pkg) { + ASSERT(handle != NULL, RET_ERR_VOID(PM_ERR_HANDLE_NULL)); handle->noupgrade = alpm_list_add(handle->noupgrade, strdup(pkg)); } void SYMEXPORT alpm_option_set_noupgrades(alpm_list_t *noupgrade) { + ASSERT(handle != NULL, RET_ERR_VOID(PM_ERR_HANDLE_NULL)); if(handle->noupgrade) FREELIST(handle->noupgrade); if(noupgrade) handle->noupgrade = noupgrade; } @@ -473,6 +483,7 @@ void SYMEXPORT alpm_option_set_noupgrades(alpm_list_t *noupgrade) int SYMEXPORT alpm_option_remove_noupgrade(const char *pkg) { char *vdata = NULL; + ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1)); handle->noupgrade = alpm_list_remove_str(handle->noupgrade, pkg, &vdata); if(vdata != NULL) { FREE(vdata); @@ -483,11 +494,13 @@ int SYMEXPORT alpm_option_remove_noupgrade(const char *pkg) void SYMEXPORT alpm_option_add_noextract(const char *pkg) { + ASSERT(handle != NULL, RET_ERR_VOID(PM_ERR_HANDLE_NULL)); handle->noextract = alpm_list_add(handle->noextract, strdup(pkg)); } void SYMEXPORT alpm_option_set_noextracts(alpm_list_t *noextract) { + ASSERT(handle != NULL, RET_ERR_VOID(PM_ERR_HANDLE_NULL)); if(handle->noextract) FREELIST(handle->noextract); if(noextract) handle->noextract = noextract; } @@ -495,6 +508,7 @@ void SYMEXPORT alpm_option_set_noextracts(alpm_list_t *noextract) int SYMEXPORT alpm_option_remove_noextract(const char *pkg) { char *vdata = NULL; + ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1)); handle->noextract = alpm_list_remove_str(handle->noextract, pkg, &vdata); if(vdata != NULL) { FREE(vdata); @@ -505,11 +519,13 @@ int SYMEXPORT alpm_option_remove_noextract(const char *pkg) void SYMEXPORT alpm_option_add_ignorepkg(const char *pkg) { + ASSERT(handle != NULL, RET_ERR_VOID(PM_ERR_HANDLE_NULL)); handle->ignorepkg = alpm_list_add(handle->ignorepkg, strdup(pkg)); } void SYMEXPORT alpm_option_set_ignorepkgs(alpm_list_t *ignorepkgs) { + ASSERT(handle != NULL, RET_ERR_VOID(PM_ERR_HANDLE_NULL)); if(handle->ignorepkg) FREELIST(handle->ignorepkg); if(ignorepkgs) handle->ignorepkg = ignorepkgs; } @@ -517,6 +533,7 @@ void SYMEXPORT alpm_option_set_ignorepkgs(alpm_list_t *ignorepkgs) int SYMEXPORT alpm_option_remove_ignorepkg(const char *pkg) { char *vdata = NULL; + ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1)); handle->ignorepkg = alpm_list_remove_str(handle->ignorepkg, pkg, &vdata); if(vdata != NULL) { FREE(vdata); @@ -527,11 +544,13 @@ int SYMEXPORT alpm_option_remove_ignorepkg(const char *pkg) void SYMEXPORT alpm_option_add_ignoregrp(const char *grp) { + ASSERT(handle != NULL, RET_ERR_VOID(PM_ERR_HANDLE_NULL)); handle->ignoregrp = alpm_list_add(handle->ignoregrp, strdup(grp)); } void SYMEXPORT alpm_option_set_ignoregrps(alpm_list_t *ignoregrps) { + ASSERT(handle != NULL, RET_ERR_VOID(PM_ERR_HANDLE_NULL)); if(handle->ignoregrp) FREELIST(handle->ignoregrp); if(ignoregrps) handle->ignoregrp = ignoregrps; } @@ -539,6 +558,7 @@ void SYMEXPORT alpm_option_set_ignoregrps(alpm_list_t *ignoregrps) int SYMEXPORT alpm_option_remove_ignoregrp(const char *grp) { char *vdata = NULL; + ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1)); handle->ignoregrp = alpm_list_remove_str(handle->ignoregrp, grp, &vdata); if(vdata != NULL) { FREE(vdata); @@ -549,17 +569,20 @@ int SYMEXPORT alpm_option_remove_ignoregrp(const char *grp) void SYMEXPORT alpm_option_set_arch(const char *arch) { + ASSERT(handle != NULL, RET_ERR_VOID(PM_ERR_HANDLE_NULL)); if(handle->arch) FREE(handle->arch); if(arch) handle->arch = strdup(arch); } void SYMEXPORT alpm_option_set_usedelta(int usedelta) { + ASSERT(handle != NULL, RET_ERR_VOID(PM_ERR_HANDLE_NULL)); handle->usedelta = usedelta; } void SYMEXPORT alpm_option_set_checkspace(int checkspace) { + ASSERT(handle != NULL, RET_ERR_VOID(PM_ERR_HANDLE_NULL)); handle->checkspace = checkspace; } diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h index 3232f00..c66e988 100644 --- a/lib/libalpm/util.h +++ b/lib/libalpm/util.h @@ -57,6 +57,10 @@ #define ASSERT(cond, action) do { if(!(cond)) { action; } } while(0) +#define RET_ERR_VOID(err) do { pm_errno = (err); \ + _alpm_log(PM_LOG_DEBUG, "returning error %d from %s : %s\n", err, __func__, alpm_strerrorlast()); \ + return; } while(0) + #define RET_ERR(err, ret) do { pm_errno = (err); \ _alpm_log(PM_LOG_DEBUG, "returning error %d from %s : %s\n", err, __func__, alpm_strerrorlast()); \ return(ret); } while(0) -- 1.7.4.2
On Mon, Mar 28, 2011 at 2:47 PM, Rémy Oudompheng <remyoudompheng@gmail.com> wrote:
Many alpm_option_get/set_*() functions already check this and set pm_errno to the right value, but not all, so this improves consistency.
Signed-off-by: Rémy Oudompheng <remy@archlinux.org> --- +#define RET_ERR_VOID(err) do { pm_errno = (err); \ + _alpm_log(PM_LOG_DEBUG, "returning error %d from %s : %s\n", err, __func__, alpm_strerrorlast()); \ + return; } while(0) + #define RET_ERR(err, ret) do { pm_errno = (err); \ _alpm_log(PM_LOG_DEBUG, "returning error %d from %s : %s\n", err, __func__, alpm_strerrorlast()); \ return(ret); } while(0) --
I can tell from this bit I've snipped you are patching maint, and not master, but I did not know this right away. It helps all of us greatly when reviewing if we know where you intend the patch to land- indicate that below the --- lines in your patch submission, right before the diffstat. Otherwise, +1 from me given a bit more testing especially if we are going to maint with this. -Dan
On Mon 28 March 2011 at 21:28 +0200, Xavier Chantry wrote:
On Mon, Mar 28, 2011 at 9:26 PM, Rémy Oudompheng <remyoudompheng@gmail.com> wrote:
On 2011/3/28 Rémy Oudompheng <remyoudompheng@gmail.com> wrote:
Many alpm_option_get/set_*() functions already check this and set pm_errno to the right value, but not all, so this improves consistency.
I just noticed the ASSERT..RET_ERR construct, should it be the preferred way of implementing such things ?
I was going to say a macro would be nice for that repetitive stuff.
And indeed we already have them and there is already that code : ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1));
I just sent another patch using the macro. Rémy.
participants (3)
-
Dan McGee
-
Rémy Oudompheng
-
Xavier Chantry