Re: [libvirt] [PATCH 1/2] build: use correct type for pid and similar types

2012-03-02 Thread Peter Krempa 

On 02/11/2012 12:55 AM, Eric Blake wrote:

No thanks to 64-bit windows, with 64-bit pid_t, we have to avoid
constructs like 'int pid'.  Our API in libvirt-qemu cannot be
changed without breaking ABI; but then again, libvirt-qemu can
only be used on systems that support UNIX sockets, which rules
out Windows (even if qemu could be compiled there) - so for all
points on the call chain that interact with this API decision,
we require a different variable name to make it clear that we
audited the use for safety.

Adding a syntax-check rule only solves half the battle; anywhere
that uses printf on a pid_t still needs to be converted, but that
will be a separate patch.



Sorry for remembering really late to review your patch.


diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1b92aa4..2e63193 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7925,7 +7926,7 @@ virDomainDefPtr qemuParseCommandLinePid(virCapsPtr caps,
   pidfile, monConfig, monJSON)))
  goto cleanup;

-if (virAsprintf(exepath, /proc/%u/exe, pid)  0) {
+if (virAsprintf(exepath, /proc/%u/exe, (int) pid)  0) {


I'd use /proc/%lld/exe and cast pid to (long long). Or at least change 
%u to %d for the (int) typecast. I agree that linux does not use 
long pids now, but it's nicer when the code is uniform and you used the 
long long variant later on and in the 2/2 patch of this series.



  virReportOOMError();
  goto cleanup;
  }
@@ -7933,7 +7934,7 @@ virDomainDefPtr qemuParseCommandLinePid(virCapsPtr caps,
  if (virFileResolveLink(exepath,emulator)  0) {
  virReportSystemError(errno,
   _(Unable to resolve %s for pid %u),
- exepath, pid);
+ exepath, (int) pid);


Same as above.


  goto cleanup;
  }
  VIR_FREE(def-emulator);



diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 160cb37..abe73f1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1065,10 +1065,12 @@ qemudGetProcessInfo(unsigned long long *cpuTime, int 
*lastCpu, long *vm_rss,
  int cpu;
  int ret;

+/* In general, we cannot assume pid_t fits in int; but /proc parsing
+ * is specific to Linux where int works fine.  */


The format string is correct here


  if (tid)
-ret = virAsprintf(proc, /proc/%d/task/%d/stat, pid, tid);
+ret = virAsprintf(proc, /proc/%d/task/%d/stat, (int) pid, tid);
  else
-ret = virAsprintf(proc, /proc/%d/stat, pid);
+ret = virAsprintf(proc, /proc/%d/stat, (int) pid);
  if (ret  0)
  return -1;

@@ -1120,7 +1122,7 @@ qemudGetProcessInfo(unsigned long long *cpuTime, int 
*lastCpu, long *vm_rss,


  VIR_DEBUG(Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld,
-  pid, tid, usertime, systime, cpu, rss);
+  (int) pid, tid, usertime, systime, cpu, rss);

  VIR_FORCE_FCLOSE(pidinfo);




diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 3e1a72f..e71dc20 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -94,9 +94,10 @@ static const char * 
virSecurityDACGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNU
  }

  static int
-virSecurityDACSetOwnership(const char *path, int uid, int gid)
+virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
  {
-VIR_INFO(Setting DAC user and group on '%s' to '%d:%d', path, uid, gid);
+VIR_INFO(Setting DAC user and group on '%s' to '%ld:%ld',
+ path, (long) uid, (long) gid);

  if (chown(path, uid, gid)  0) {
  struct stat sb;
@@ -111,18 +112,22 @@ virSecurityDACSetOwnership(const char *path, int uid, int 
gid)
  }

  if (chown_errno == EOPNOTSUPP || chown_errno == EINVAL) {
-VIR_INFO(Setting user and group to '%d:%d' on '%s' not supported by 
filesystem,
- uid, gid, path);
+VIR_INFO(Setting user and group to '%ld:%ld' on '%s' not 
+ supported by filesystem,
+ (long) uid, (long) gid, path);
  } else if (chown_errno == EPERM) {
-VIR_INFO(Setting user and group to '%d:%d' on '%s' not permitted,
- uid, gid, path);
+VIR_INFO(Setting user and group to '%ld:%ld' on '%s' not 
+ permitted,
+ (long) uid, (long) gid, path);
  } else if (chown_errno == EROFS) {
-VIR_INFO(Setting user and group to '%d:%d' on '%s' not possible on 
readonly filesystem,
- uid, gid, path);
+VIR_INFO(Setting user and group to '%ld:%ld' on '%s' not 
+ possible on readonly filesystem,
+ (long) uid, (long) gid, path);
  } else {
  virReportSystemError(chown_errno,
- _(unable to set user and group to

Re: [libvirt] [PATCH 1/2] build: use correct type for pid and similar types

2012-03-02 Thread Eric Blake 
On 03/02/2012 05:42 AM, Peter Krempa wrote:
 
 Sorry for remembering really late to review your patch.

No problem.  There's still some work to do to get things happy now that
F17 has switched cross toolchains to mingw64.

 
 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
 index 1b92aa4..2e63193 100644
 --- a/src/qemu/qemu_command.c
 +++ b/src/qemu/qemu_command.c
 @@ -7925,7 +7926,7 @@ virDomainDefPtr
 qemuParseCommandLinePid(virCapsPtr caps,
pidfile, monConfig, monJSON)))
   goto cleanup;

 -if (virAsprintf(exepath, /proc/%u/exe, pid)  0) {
 +if (virAsprintf(exepath, /proc/%u/exe, (int) pid)  0) {
 
 I'd use /proc/%lld/exe and cast pid to (long long). Or at least change
 %u to %d for the (int) typecast. I agree that linux does not use
 long pids now, but it's nicer when the code is uniform and you used the
 long long variant later on and in the 2/2 patch of this series.

I'll go with the short %d, along with a comment why it is safe.

   virReportSystemError(chown_errno,
 - _(unable to set user and group to
 '%d:%d' on '%s'),
 - uid, gid, path);
 + _(unable to set user and group to
 '%ld:%ld' 
 +   on '%s'),
 + (long) uid, (long) gid, path);
   return -1;
   }
   }
 
 Again, I'd prefer long longs here to line up with other instances.

We already have a compile-time assertion that uid_t and gid_t fit in
long, and this is true for all known platforms including ming64.  It is
only pid_t (and thus id_t) that is problematic.  We also have other
instances of commits that just cast to long when printing uids, so if I
were to make things consistent with long long, I'd have to touch more
code.  So I don't think this one is worth changing.

 
 ACK with the mismatch of signedness of the formating string and typecast
 in the first and second hunk of this reply. The other comments are just
 cosmetic so I'm ok if you leave the rest as is.

Sounds like I'll just fix the first two hunks, then :)

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/2] build: use correct type for pid and similar types

2012-02-10 Thread Eric Blake 
No thanks to 64-bit windows, with 64-bit pid_t, we have to avoid
constructs like 'int pid'.  Our API in libvirt-qemu cannot be
changed without breaking ABI; but then again, libvirt-qemu can
only be used on systems that support UNIX sockets, which rules
out Windows (even if qemu could be compiled there) - so for all
points on the call chain that interact with this API decision,
we require a different variable name to make it clear that we
audited the use for safety.

Adding a syntax-check rule only solves half the battle; anywhere
that uses printf on a pid_t still needs to be converted, but that
will be a separate patch.

* cfg.mk (sc_correct_id_types): New syntax check.
* src/libvirt-qemu.c (virDomainQemuAttach): Document why we didn't
use pid_t for pid, and validate for overflow.
* include/libvirt/libvirt-qemu.h (virDomainQemuAttach): Tweak name
for syntax check.
* src/vmware/vmware_conf.c (vmwareExtractPid): Likewise.
* src/driver.h (virDrvDomainQemuAttach): Likewise.
* tools/virsh.c (cmdQemuAttach): Likewise.
* src/remote/qemu_protocol.x (qemu_domain_attach_args): Likewise.
* src/qemu_protocol-structs (qemu_domain_attach_args): Likewise.
* src/util/cgroup.c (virCgroupPidCode, virCgroupKillInternal):
Likewise.
* src/qemu/qemu_command.c(qemuParseProcFileStrings): Likewise.
(qemuParseCommandLinePid): Use pid_t for pid.
* daemon/libvirtd.c (daemonForkIntoBackground): Likewise.
* src/conf/domain_conf.h (_virDomainObj): Likewise.
* src/probes.d (rpc_socket_new): Likewise.
* src/qemu/qemu_command.h (qemuParseCommandLinePid): Likewise.
* src/qemu/qemu_driver.c (qemudGetProcessInfo, qemuDomainAttach):
Likewise.
* src/qemu/qemu_process.c (qemuProcessAttach): Likewise.
* src/qemu/qemu_process.h (qemuProcessAttach): Likewise.
* src/uml/uml_driver.c (umlGetProcessInfo): Likewise.
* src/util/virnetdev.h (virNetDevSetNamespace): Likewise.
* src/util/virnetdev.c (virNetDevSetNamespace): Likewise.
* tests/testutils.c (virtTestCaptureProgramOutput): Likewise.
* src/conf/storage_conf.h (_virStoragePerms): Use mode_t, uid_t,
and gid_t rather than int.
* src/security/security_dac.c (virSecurityDACSetOwnership): Likewise.
* src/conf/storage_conf.c (virStorageDefParsePerms): Avoid
compiler warning.
---
 cfg.mk |6 ++
 daemon/libvirtd.c  |4 ++--
 include/libvirt/libvirt-qemu.h |4 ++--
 src/conf/domain_conf.h |2 +-
 src/conf/storage_conf.c|4 ++--
 src/conf/storage_conf.h|8 
 src/driver.h   |3 ++-
 src/libvirt-qemu.c |   17 +++--
 src/probes.d   |2 +-
 src/qemu/qemu_command.c|   13 +++--
 src/qemu/qemu_command.h|4 ++--
 src/qemu/qemu_driver.c |   24 ++--
 src/qemu/qemu_process.c|2 +-
 src/qemu/qemu_process.h|2 +-
 src/qemu_protocol-structs  |2 +-
 src/remote/qemu_protocol.x |4 ++--
 src/security/security_dac.c|   27 ---
 src/uml/uml_driver.c   |6 +++---
 src/util/cgroup.c  |   17 +
 src/util/virnetdev.c   |4 ++--
 src/util/virnetdev.h   |2 +-
 src/vmware/vmware_conf.c   |   10 ++
 tests/testutils.c  |2 +-
 tools/virsh.c  |   10 +-
 24 files changed, 102 insertions(+), 77 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index dcf44bb..9e50a5a 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -416,6 +416,12 @@ sc_prohibit_ctype_h:
halt=don't use ctype.h; instead, use c-ctype.h\
  $(_sc_search_regexp)

+# Insist on correct types for [pug]id.
+sc_correct_id_types:
+   @prohibit='\(int|long) *[pug]id\' \
+   halt=use pid_t for pid, uid_t for uid, gid_t for gid  \
+ $(_sc_search_regexp)
+
 # Ensure that no C source file, docs, or rng schema uses TABs for
 # indentation.  Also match *.h.in files, to get libvirt.h.in.  Exclude
 # files in gnulib, since they're imported.
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index b1b542b..db2977e 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1,7 +1,7 @@
 /*
  * libvirtd.c: daemon start of day, guest process  i/o management
  *
- * Copyright (C) 2006-2011 Red Hat, Inc.
+ * Copyright (C) 2006-2012 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -186,7 +186,7 @@ static int daemonForkIntoBackground(const char *argv0)
 if (pipe(statuspipe)  0)
 return -1;

-int pid = fork();
+pid_t pid = fork();
 switch (pid) {
 case 0:
 {
diff --git a/include/libvirt/libvirt-qemu.h b/include/libvirt/libvirt-qemu.h
index 7f12e4f..ba75ae3 100644
--- a/include/libvirt/libvirt-qemu.h
+++ b/include/libvirt/libvirt-qemu.h
@@ -4,7 +4,7 @@
  * Description: Provides the interfaces of the libvirt library to handle
  *  qemu