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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list