[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]