[libvirt] [PATCH] src: Threat pid as signed

[Martin Kletzander]

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 at redhat.com>
>---
>
>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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161013/4dc9960d/attachment-0001.sig>