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

On Thu, Jul 24, 2014 at 10:17:45AM +0800, Wang Rui wrote: > From: James <james.wangyufei(a)huawei.com&gt; > > 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(a)huawei.com&gt; > --- > 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(...);

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.

> 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