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 eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org