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

[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 '%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.

>

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.

Peter