[pacman-dev] [PATCH] Fix compilation without gpgme
Two issues have snuck in that prevent the compile from working.
Signed-off-by: Dan McGee
Signed-off-by: Dan McGee
Signed-off-by: Dan McGee
On 03/01/14 04:37, Dan McGee wrote:
Signed-off-by: Dan McGee
---
I had submitted this a patch flagging the parameters as potentially unused: https://patchwork.archlinux.org/patch/1788/
lib/libalpm/diskspace.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/lib/libalpm/diskspace.c b/lib/libalpm/diskspace.c index dcab3b0..cfd0a7a 100644 --- a/lib/libalpm/diskspace.c +++ b/lib/libalpm/diskspace.c @@ -84,6 +84,9 @@ static int mount_point_load_fsinfo(alpm_handle_t *handle, alpm_mountpoint_t *mou _alpm_log(handle, ALPM_LOG_DEBUG, "loading fsinfo for %s\n", mountpoint->mount_dir); mountpoint->read_only = mountpoint->fsp.f_flag & ST_RDONLY; mountpoint->fsinfo_loaded = MOUNT_FSINFO_LOADED; +#else + (void)handle; + (void)mountpoint; #endif
return 0;
On Thu, Jan 2, 2014 at 4:03 PM, Allan McRae
On 03/01/14 04:37, Dan McGee wrote:
Signed-off-by: Dan McGee
--- I had submitted this a patch flagging the parameters as potentially unused: https://patchwork.archlinux.org/patch/1788/
I am not a huge fan of flagging things as unused when they are in fact used on certain code paths, but I'm not going to object either way. You're in charge! :)
lib/libalpm/diskspace.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/lib/libalpm/diskspace.c b/lib/libalpm/diskspace.c index dcab3b0..cfd0a7a 100644 --- a/lib/libalpm/diskspace.c +++ b/lib/libalpm/diskspace.c @@ -84,6 +84,9 @@ static int mount_point_load_fsinfo(alpm_handle_t *handle, alpm_mountpoint_t *mou _alpm_log(handle, ALPM_LOG_DEBUG, "loading fsinfo for %s\n", mountpoint->mount_dir); mountpoint->read_only = mountpoint->fsp.f_flag & ST_RDONLY; mountpoint->fsinfo_loaded = MOUNT_FSINFO_LOADED; +#else + (void)handle; + (void)mountpoint; #endif
return 0;
On 09/01/14 03:03, Dan McGee wrote:
On Thu, Jan 2, 2014 at 4:03 PM, Allan McRae
wrote: On 03/01/14 04:37, Dan McGee wrote:
Signed-off-by: Dan McGee
--- I had submitted this a patch flagging the parameters as potentially unused: https://patchwork.archlinux.org/patch/1788/
I am not a huge fan of flagging things as unused when they are in fact used on certain code paths, but I'm not going to object either way. You're in charge! :)
That flag just say "potentially unused", so it is perfect for this situation. Until the variable becomes used in all code paths and it unused flags is not removed... I'll flip a coin to pick a patch in the next merge.
lib/libalpm/diskspace.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/lib/libalpm/diskspace.c b/lib/libalpm/diskspace.c index dcab3b0..cfd0a7a 100644 --- a/lib/libalpm/diskspace.c +++ b/lib/libalpm/diskspace.c @@ -84,6 +84,9 @@ static int mount_point_load_fsinfo(alpm_handle_t *handle, alpm_mountpoint_t *mou _alpm_log(handle, ALPM_LOG_DEBUG, "loading fsinfo for %s\n", mountpoint->mount_dir); mountpoint->read_only = mountpoint->fsp.f_flag & ST_RDONLY; mountpoint->fsinfo_loaded = MOUNT_FSINFO_LOADED; +#else + (void)handle; + (void)mountpoint; #endif
return 0;
Noticed using clang and `-Wpadded`.
Signed-off-by: Dan McGee
On 03/01/14 04:37, Dan McGee wrote:
Noticed using clang and `-Wpadded`.
Should that be added in our --enable-warning-flags option?
Signed-off-by: Dan McGee
--- lib/libalpm/dload.h | 4 ++-- lib/libalpm/graph.h | 4 ++-- lib/libalpm/handle.h | 4 +++- src/pacman/conf.c | 2 +- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h index 6c9f7a7..964c338 100644 --- a/lib/libalpm/dload.h +++ b/lib/libalpm/dload.h @@ -31,6 +31,8 @@ struct dload_payload { char *destfile_name; char *content_disp_name; char *fileurl; + alpm_list_t *servers; + long respcode; off_t initial_size; off_t max_size; off_t prevprogress; @@ -39,11 +41,9 @@ struct dload_payload { int errors_ok; int unlink_on_fail; int trust_remote_name; - alpm_list_t *servers; #ifdef HAVE_LIBCURL CURLcode curlerr; /* last error produced by curl */ #endif - long respcode; };
void _alpm_dload_payload_reset(struct dload_payload *payload); diff --git a/lib/libalpm/graph.h b/lib/libalpm/graph.h index bf06047..8db35bf 100644 --- a/lib/libalpm/graph.h +++ b/lib/libalpm/graph.h @@ -24,12 +24,12 @@ #include "alpm_list.h"
typedef struct __alpm_graph_t { - char state; /* 0: untouched, -1: entered, other: leaving time */ - off_t weight; /* weight of the node */ void *data; struct __alpm_graph_t *parent; /* where did we come from? */ alpm_list_t *children; alpm_list_t *childptr; /* points to a child in children list */ + off_t weight; /* weight of the node */ + char state; /* 0: untouched, -1: entered, other: leaving time */ } alpm_graph_t;
alpm_graph_t *_alpm_graph_new(void); diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 4126e1a..b6b8e2b 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -55,7 +55,6 @@ struct __alpm_handle_t { alpm_db_t *db_local; /* local db pointer */ alpm_list_t *dbs_sync; /* List of (alpm_db_t *) */ FILE *logstream; /* log file stream pointer */ - int lockfd; /* lock file descriptor */ alpm_trans_t *trans;
#ifdef HAVE_LIBCURL @@ -100,6 +99,9 @@ struct __alpm_handle_t { /* error code */ alpm_errno_t pm_errno;
+ /* lock file descriptor */ + int lockfd; + /* for delta parsing efficiency */ int delta_regex_compiled; regex_t delta_regex; diff --git a/src/pacman/conf.c b/src/pacman/conf.c index cf8a417..258d538 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -746,8 +746,8 @@ struct section_t { int is_options; int parse_options; /* db section option gathering */ - alpm_siglevel_t siglevel; alpm_list_t *servers; + alpm_siglevel_t siglevel; alpm_db_usage_t usage; };
On 03/01/14 04:37, Dan McGee wrote:
Noticed using clang and `-Wpadded`.
Should that be added in our --enable-warning-flags option?
It is very noisy; I added it locally to find these but I wouldn't recommend doing it all the time. It unfortunately nags on both padding within a single struct, as well as padding between individual structs in arrays. One is easily fixable, the other not as much if you truly need all
On Thu, Jan 2, 2014 at 6:13 PM, Allan McRae
This was a hack done by me in commit d8e88aa0175fd back in 2007 that is
no longer necessary, given a sufficiently smart compiler and one that
supports the inline keyword.
Signed-off-by: Dan McGee
This shrinks down the total size of the package struct by a handful of
bytes, saving us some memory and cache pressure when we are loading up
the entirety of the sync and local databases.
Signed-off-by: Dan McGee
Signed-off-by: Dan McGee
When calling open(), use O_CLOEXEC as much as possible to ensure the
file descriptor is closed when and if a process using libalpm forks.
For most of these cases, and especially in utility functions, the file
descriptor is opened and closed in the same function, so we don't have
too much to worry about. However, for things like the log file and
database lock file, we should ensure descriptors aren't left hanging
around for children to touch.
This patch is inspired by the problem in FS#36161, where an open file
descriptor to the current working directory prevents chroot() from
working on FreeBSD. We don't need this file descriptor in the child
process, so open it (and now several others) with O_CLOEXEC.
Signed-off-by: Dan McGee
Signed-off-by: Dan McGee
On 03/01/14 04:37, Dan McGee wrote:
Two issues have snuck in that prevent the compile from working.
Signed-off-by: Dan McGee
--- This and the rest of the patches available on my 'random-fixes' branch.
lib/libalpm/sync.c | 2 +- src/pacman/package.c | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index e358585..9531fa2 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -1225,7 +1225,7 @@ int _alpm_sync_commit(alpm_handle_t *handle, alpm_list_t **data) return -1; }
-#if HAVE_LIBGPGME +#ifdef HAVE_LIBGPGME
I not that #if and #ifdef are both used in many places. $ git grep "#if HAVE_" lib/libalpm/signing.c:#if HAVE_LIBGPGME lib/libalpm/signing.c:#if HAVE_LIBGPGME lib/libalpm/signing.c:#if HAVE_LIBGPGME lib/libalpm/sync.c:#if HAVE_LIBGPGME m4/acinclude.m4:#if HAVE_SYS_UCRED_H Should all these change?
/* make sure all required signatures are in keyring */ if(check_keyring(handle)) { return -1; diff --git a/src/pacman/package.c b/src/pacman/package.c index 52219ff..c44696b 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -193,6 +193,10 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) }
if(from == ALPM_PKG_FROM_SYNCDB && extra) { + string_display(_("MD5 Sum :"), alpm_pkg_get_md5sum(pkg), cols); + string_display(_("SHA-256 Sum :"), alpm_pkg_get_sha256sum(pkg), cols); + +#ifdef HAVE_LIBGPGME
I went for the approach of exposing alpm_decode_signature even if GPGME is unavailable, because it does not require any GPGME functions and it could still be useful for a frontend: https://patchwork.archlinux.org/patch/1790/
const char *base64_sig = alpm_pkg_get_base64_sig(pkg); alpm_list_t *keys = NULL; if(base64_sig) { @@ -204,10 +208,8 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) } else { keys = alpm_list_add(keys, _("None")); } - - string_display(_("MD5 Sum :"), alpm_pkg_get_md5sum(pkg), cols); - string_display(_("SHA-256 Sum :"), alpm_pkg_get_sha256sum(pkg), cols); list_display(_("Signatures :"), keys, cols); +#endif } else { list_display(_("Validated By :"), validation, cols); }
On Thu, Jan 2, 2014 at 4:07 PM, Allan McRae
On 03/01/14 04:37, Dan McGee wrote:
Two issues have snuck in that prevent the compile from working.
Signed-off-by: Dan McGee
--- This and the rest of the patches available on my 'random-fixes' branch.
lib/libalpm/sync.c | 2 +- src/pacman/package.c | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index e358585..9531fa2 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -1225,7 +1225,7 @@ int _alpm_sync_commit(alpm_handle_t *handle, alpm_list_t **data) return -1; }
-#if HAVE_LIBGPGME +#ifdef HAVE_LIBGPGME
I not that #if and #ifdef are both used in many places.
I think you want to stick with one of two approaches: * #if defined(HAVE_LIBGPGME) * #ifdef HAVE_LIBGPGME We seem to use both of these; however we don't use the `#if <symbol>` approach anywhere outside of signing.c. I agree that all of them in signing.c should be brought into compliance, not sure why I only changed one.
$ git grep "#if HAVE_" lib/libalpm/signing.c:#if HAVE_LIBGPGME lib/libalpm/signing.c:#if HAVE_LIBGPGME lib/libalpm/signing.c:#if HAVE_LIBGPGME lib/libalpm/sync.c:#if HAVE_LIBGPGME m4/acinclude.m4:#if HAVE_SYS_UCRED_H
Should all these change?
commit 87ffc648b7bce35d shows that we use #ifdef and not #if in sync.c; are we looking at different codebases?
/* make sure all required signatures are in keyring */ if(check_keyring(handle)) { return -1; diff --git a/src/pacman/package.c b/src/pacman/package.c index 52219ff..c44696b 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -193,6 +193,10 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) }
if(from == ALPM_PKG_FROM_SYNCDB && extra) { + string_display(_("MD5 Sum :"),
alpm_pkg_get_md5sum(pkg), cols);
+ string_display(_("SHA-256 Sum :"), alpm_pkg_get_sha256sum(pkg), cols); + +#ifdef HAVE_LIBGPGME
I went for the approach of exposing alpm_decode_signature even if GPGME is unavailable, because it does not require any GPGME functions and it could still be useful for a frontend: https://patchwork.archlinux.org/patch/1790/
I like your approach, that makes more sense and means frontend code doesn't have to know how the backend library was built.
const char *base64_sig = alpm_pkg_get_base64_sig(pkg); alpm_list_t *keys = NULL; if(base64_sig) { @@ -204,10 +208,8 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) } else { keys = alpm_list_add(keys, _("None")); } - - string_display(_("MD5 Sum :"),
alpm_pkg_get_md5sum(pkg), cols);
- string_display(_("SHA-256 Sum :"), alpm_pkg_get_sha256sum(pkg), cols); list_display(_("Signatures :"), keys, cols); +#endif } else { list_display(_("Validated By :"), validation, cols); }
Given the overlap in our changes and thoughts here, do you want me to bother resubmitting this? I can go back and review your patches if that makes more sense, let me know.
-Dan
On 10/01/14 01:55, Dan McGee wrote:
On Thu, Jan 2, 2014 at 4:07 PM, Allan McRae
wrote: On 03/01/14 04:37, Dan McGee wrote:
Two issues have snuck in that prevent the compile from working.
Signed-off-by: Dan McGee
--- This and the rest of the patches available on my 'random-fixes' branch.
lib/libalpm/sync.c | 2 +- src/pacman/package.c | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index e358585..9531fa2 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -1225,7 +1225,7 @@ int _alpm_sync_commit(alpm_handle_t *handle, alpm_list_t **data) return -1; }
-#if HAVE_LIBGPGME +#ifdef HAVE_LIBGPGME
I not that #if and #ifdef are both used in many places.
I think you want to stick with one of two approaches:
* #if defined(HAVE_LIBGPGME) * #ifdef HAVE_LIBGPGME
We seem to use both of these; however we don't use the `#if <symbol>` approach anywhere outside of signing.c. I agree that all of them in signing.c should be brought into compliance, not sure why I only changed one.
Great. #ifdef it is then.
$ git grep "#if HAVE_" lib/libalpm/signing.c:#if HAVE_LIBGPGME lib/libalpm/signing.c:#if HAVE_LIBGPGME lib/libalpm/signing.c:#if HAVE_LIBGPGME lib/libalpm/sync.c:#if HAVE_LIBGPGME m4/acinclude.m4:#if HAVE_SYS_UCRED_H
Should all these change?
commit 87ffc648b7bce35d shows that we use #ifdef and not #if in sync.c; are we looking at different codebases?
Probably the codebase with my alternative patch to this issue.
/* make sure all required signatures are in keyring */ if(check_keyring(handle)) { return -1; diff --git a/src/pacman/package.c b/src/pacman/package.c index 52219ff..c44696b 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -193,6 +193,10 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) }
if(from == ALPM_PKG_FROM_SYNCDB && extra) { + string_display(_("MD5 Sum :"),
alpm_pkg_get_md5sum(pkg), cols);
+ string_display(_("SHA-256 Sum :"), alpm_pkg_get_sha256sum(pkg), cols); + +#ifdef HAVE_LIBGPGME
I went for the approach of exposing alpm_decode_signature even if GPGME is unavailable, because it does not require any GPGME functions and it could still be useful for a frontend: https://patchwork.archlinux.org/patch/1790/
I like your approach, that makes more sense and means frontend code doesn't have to know how the backend library was built.
const char *base64_sig = alpm_pkg_get_base64_sig(pkg); alpm_list_t *keys = NULL; if(base64_sig) { @@ -204,10 +208,8 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) } else { keys = alpm_list_add(keys, _("None")); } - - string_display(_("MD5 Sum :"),
alpm_pkg_get_md5sum(pkg), cols);
- string_display(_("SHA-256 Sum :"), alpm_pkg_get_sha256sum(pkg), cols); list_display(_("Signatures :"), keys, cols); +#endif } else { list_display(_("Validated By :"), validation, cols); }
Given the overlap in our changes and thoughts here, do you want me to bother resubmitting this? I can go back and review your patches if that makes more sense, let me know.
My patch was rather simple, so no need (unless you are bored...). I'll add the extra #if -> #ifdef. Allan
participants (3)
-
Allan McRae
-
Dan McGee
-
Dan McGee