[libvirt] [PATCH] util: virTimeFieldsThenRaw never return negative

[Wang Rui]

On 2014/7/24 13:18, Martin Kletzander wrote:
> On Thu, Jul 24, 2014 at 10:17:45AM +0800, Wang Rui wrote:
>> From: James <james.wangyufei at huawei.com>
>>
>> virTimeFieldsThenRaw will never return negative result, so I delete related
>> judgement.
>>
> 
> Looking at the code I think it was pretty nicely prepared for error
> reporting, but then there was no error that could happen in the
> function, so this cleanup makes sense.
> 
>> Signed-off-by: James <james.wangyufei at huawei.com>
>> ---
>> src/util/virtime.c | 7 ++-----
>> src/util/virtime.h | 4 ++--
>> 2 files changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/util/virtime.c b/src/util/virtime.c
>> index 2a91ea5..662c161 100644
>> --- a/src/util/virtime.c
>> +++ b/src/util/virtime.c
>> @@ -121,9 +121,8 @@ const unsigned short int __mon_yday[2][13] = {
>>  * Converts the timestamp @when into broken-down field format.
>>  * Time time is always in UTC
>>  *
>> - * Returns 0 on success, -1 on error with errno set
>>  */
>> -int virTimeFieldsThenRaw(unsigned long long when, struct tm *fields)
>> +void virTimeFieldsThenRaw(unsigned long long when, struct tm *fields)
>> {
>>     /* This code is taken from GLibC under terms of LGPLv2+ */
>>     long int days, rem, y;
>> @@ -171,7 +170,6 @@ int virTimeFieldsThenRaw(unsigned long long when, struct tm *fields)
>>     days -= ip[y];
>>     fields->tm_mon = y;
>>     fields->tm_mday = days + 1;
>> -    return 0;
>> }
>>
>>
>> @@ -209,8 +207,7 @@ int virTimeStringThenRaw(unsigned long long when, char *buf)
>> {
>>     struct tm fields;
>>
>> -    if (virTimeFieldsThenRaw(when, &fields) < 0)
>> -        return -1;
>> +    virTimeFieldsThenRaw(when, &fields);
>>
> 
> This is OK, but there's a virTimeFieldsNowRaw() function that does:
> return virTimeFieldsThenRaw(...);

Thanks for your review.
You are right. Function virTimeFieldsNowRaw() should return 0 in case of success.

> 
> That should be fixed as well.  And virTimeFieldsThen() encapsulates
> the Raw version with an error message that's pointless now.  So that
> function can be void too.  The whole chain of function calls should be
> fixed up, not just one call to it.
> 
OK, that will be fixed in V2.

>>     fields.tm_year += 1900;
>>     fields.tm_mon += 1;
>> diff --git a/src/util/virtime.h b/src/util/virtime.h
>> index 25332db..61f36dc 100644
>> --- a/src/util/virtime.h
>> +++ b/src/util/virtime.h
>> @@ -43,12 +43,12 @@ int virTimeMillisNowRaw(unsigned long long *now)
>>     ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>> int virTimeFieldsNowRaw(struct tm *fields)
>>     ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>> -int virTimeFieldsThenRaw(unsigned long long when, struct tm *fields)
>> -    ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>> int virTimeStringNowRaw(char *buf)
>>     ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>> int virTimeStringThenRaw(unsigned long long when, char *buf)
>>     ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>> +void virTimeFieldsThenRaw(unsigned long long when, struct tm *fields)
>> +     ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>>
> 
> No need to move this; I think it's probably sorted by function name or
> it has the same order as the .c file.
> 
> Martin