Re: [libvirt] [PATCH 1/2] build: use correct type for pid and similar types
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)