On Fri, Oct 07, 2016 at 11:22:34AM +0200, Michal Privoznik wrote:
This initially started as a fix of some debug printing in virCgroupDetect. However it turned out that other places suffer from the similar problem. While dealing with pids, esp. in cases where we cannot use pid_t for ABI stability reasons, we often chose an unsigned integer type. This makes no sense as pid_t is signed. Also, new syntax-check rule is introduced so we won't repeat this mistake. Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> --- Technically, this is v2 of [1], but like 99% of the patch is different, so I'm sending this as a completely new patch. 1: https://www.redhat.com/archives/libvir-list/2016-October/msg00254.html cfg.mk | 8 ++++++++ src/locking/lock_driver_sanlock.c | 4 ++-- src/lxc/lxc_controller.c | 4 ++-- src/lxc/lxc_domain.c | 8 ++++---- src/lxc/lxc_driver.c | 4 ++-- src/lxc/lxc_monitor.c | 3 +-- src/lxc/lxc_process.c | 8 ++++---- src/qemu/qemu_process.c | 12 ++++++------ src/util/vircgroup.c | 24 ++++++++++++------------ src/util/viridentity.c | 2 +- src/util/virpolkit.c | 1 + src/util/virprocess.c | 14 ++++++-------- src/util/virsystemd.c | 9 +++++---- src/util/virutil.c | 4 ++-- 14 files changed, 56 insertions(+), 49 deletions(-) diff --git a/cfg.mk b/cfg.mk index 9f5949c..b33b1e2 100644 --- a/cfg.mk +++ b/cfg.mk @@ -581,6 +581,11 @@ sc_prohibit_int_assign_bool: halt='use bool type for boolean values' \ $(_sc_search_regexp) +sc_prohibit_unsigned_pid: + @prohibit='\<unsigned\> [^,=]+pid' \
I'd also add ';' and '(' in this ^^ list of characters, but that's your
call on those.
+ halt='use signed type for pid values' \
+ $(_sc_search_regexp)
+
# Many of the function names below came from this filter:
# git grep -B2 '\<_('|grep -E '\.c- *[[:alpha:]_][[:alnum:]_]* ?\(.*[,;]$' \
# |sed 's/.*\.c- *//'|perl -pe 's/ ?\(.*//'|sort -u \
@@ -1214,6 +1219,9 @@ exclude_file_name_regexp--sc_prohibit_include_public_headers_brackets = \
exclude_file_name_regexp--sc_prohibit_int_ijk = \
^(src/remote_protocol-structs|src/remote/remote_protocol\.x|cfg\.mk|include/libvirt/libvirt.+|src/admin_protocol-structs|src/admin/admin_protocol\.x)$$
+exclude_file_name_regexp--sc_prohibit_unsigned_pid = \
+ ^(include/libvirt/libvirt-qemu.h|src/(driver-hypervisor.h|libvirt-qemu.c|locking/lock_protocol.x|lxc/lxc_monitor_protocol.x|qemu/qemu_driver.c|remote/qemu_protocol.x|util/virpolkit.c|util/virsystemd.c)|tests/virpolkittest.c|tools/virsh-domain.c)$$
+
It's regexp, so dots need to be escaped. Also if this needs such exceptions, I don't see the point in the syntax-check. But anyway, I think all protocols and all public APIs should have this exception, so it can be made shorter. I came up with this: ^(include/libvirt/.*\.h|src/(qemu/qemu_driver\.c|driver-hypervisor\.h|libvirt(-[a-z]*)?\.c|.*\.x|util/vir(polkit|systemd)\.c)|tests/virpolkittest\.c|tools/virsh-domain\.c)$$
diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
index e7e46b8..d8c35f1 100644
--- a/src/util/virpolkit.c
+++ b/src/util/virpolkit.c
@@ -82,6 +82,7 @@ int virPolkitCheckAuth(const char *actionid,
VIR_INFO("Checking PID %lld running as %d",
(long long) pid, uid);
+ /* Yes, PolicyKit really takes pid ad uint. */
if (virDBusCallMethod(sysbus,
&reply,
NULL,
You can safely drop this ^^ hunk.
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 8b9f9c5..718c4a2 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -608,8 +608,7 @@ int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids)
*npids = 0;
*pids = NULL;
- if (virAsprintf(&taskPath, "/proc/%llu/task",
- (unsigned long long)pid) < 0)
+ if (virAsprintf(&taskPath, "/proc/%llu/task", (long long) pid) < 0)
goto cleanup;
if (virDirOpen(&dir, taskPath) < 0)
I see you changed lot of '(type)val' casts to '(type) val', and even though I used to hate this, I'm kinda more in favour of the latter nowadays. And guess what, I looked up how it looks in the code and after this patch, the ratio of non-spaced to spaced type casts changed from 751:3394 to 727:3417 (very roughly), so Yay! ACK with nits fixed. Martin
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list