[PATCH] migration: Remove time_t cast for OpenBSD

[PATCH] migration: Remove time_t cast for OpenBSD

[Brad Smith]

OpenBSD has supported 64-bit time_t across all archs since 5.5 released in 2014.

Remove a time_t cast that is no longer necessary.


Signed-off-by: Brad Smith <brad@comstyle.com>

diff --git a/migration/savevm.c b/migration/savevm.c
index 52e2d72e4b..9557f85ba9 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2849,8 +2849,7 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
     if (name) {
         pstrcpy(sn->name, sizeof(sn->name), name);
     } else {
-        /* cast below needed for OpenBSD where tv_sec is still 'long' */
-        localtime_r((const time_t *)&tv.tv_sec, &tm);
+        localtime_r(&tv.tv_sec, &tm);
         strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
     }
 

Re: [PATCH] migration: Remove time_t cast for OpenBSD

[Thomas Huth]

On 22/02/2021 08.28, Brad Smith wrote:
> OpenBSD has supported 64-bit time_t across all archs since 5.5 released in 2014.
> 
> Remove a time_t cast that is no longer necessary.
> 
> 
> Signed-off-by: Brad Smith <brad@comstyle.com>
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 52e2d72e4b..9557f85ba9 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2849,8 +2849,7 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
>       if (name) {
>           pstrcpy(sn->name, sizeof(sn->name), name);
>       } else {
> -        /* cast below needed for OpenBSD where tv_sec is still 'long' */
> -        localtime_r((const time_t *)&tv.tv_sec, &tm);
> +        localtime_r(&tv.tv_sec, &tm);
>           strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
>       }

Please make sure to CC: the maintainers (see MAINTAINERS file). Done now.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] migration: Remove time_t cast for OpenBSD

[Laurent Vivier]

Le 08/03/2021 à 12:46, Thomas Huth a écrit :
> On 22/02/2021 08.28, Brad Smith wrote:
>> OpenBSD has supported 64-bit time_t across all archs since 5.5 released in 2014.
>>
>> Remove a time_t cast that is no longer necessary.
>>
>>
>> Signed-off-by: Brad Smith <brad@comstyle.com>
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 52e2d72e4b..9557f85ba9 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2849,8 +2849,7 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
>>       if (name) {
>>           pstrcpy(sn->name, sizeof(sn->name), name);
>>       } else {
>> -        /* cast below needed for OpenBSD where tv_sec is still 'long' */
>> -        localtime_r((const time_t *)&tv.tv_sec, &tm);
>> +        localtime_r(&tv.tv_sec, &tm);
>>           strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
>>       }
> 

but the qemu_timeval from "include/sysemu/os-win32.h" still uses a long: is this file compiled for
win32?

Thanks,
Laurent

Re: [PATCH] migration: Remove time_t cast for OpenBSD

[Brad Smith]

On 3/8/2021 6:46 AM, Thomas Huth wrote:
> On 22/02/2021 08.28, Brad Smith wrote:
>> OpenBSD has supported 64-bit time_t across all archs since 5.5 
>> released in 2014.
>>
>> Remove a time_t cast that is no longer necessary.
>>
>>
>> Signed-off-by: Brad Smith <brad@comstyle.com>
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 52e2d72e4b..9557f85ba9 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2849,8 +2849,7 @@ bool save_snapshot(const char *name, bool 
>> overwrite, const char *vmstate,
>>       if (name) {
>>           pstrcpy(sn->name, sizeof(sn->name), name);
>>       } else {
>> -        /* cast below needed for OpenBSD where tv_sec is 
>> still 'long' */
>> -        localtime_r((const time_t *)&tv.tv_sec, &tm);
>> +        localtime_r(&tv.tv_sec, &tm);
>>           strftime(sn->name, sizeof(sn->name), 
>> "vm-%Y%m%d%H%M%S", &tm);
>>       }
>
> Please make sure to CC: the maintainers (see MAINTAINERS file). Done now.
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>


My bad. That was an oversight on my part. I was quick to send it to the 
list without thinking
about something like that.

Re: [PATCH] migration: Remove time_t cast for OpenBSD

[Dr. David Alan Gilbert]

* Laurent Vivier (laurent@vivier.eu) wrote:
> Le 08/03/2021 à 12:46, Thomas Huth a écrit :
> > On 22/02/2021 08.28, Brad Smith wrote:
> >> OpenBSD has supported 64-bit time_t across all archs since 5.5 released in 2014.
> >>
> >> Remove a time_t cast that is no longer necessary.
> >>
> >>
> >> Signed-off-by: Brad Smith <brad@comstyle.com>
> >>
> >> diff --git a/migration/savevm.c b/migration/savevm.c
> >> index 52e2d72e4b..9557f85ba9 100644
> >> --- a/migration/savevm.c
> >> +++ b/migration/savevm.c
> >> @@ -2849,8 +2849,7 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
> >>       if (name) {
> >>           pstrcpy(sn->name, sizeof(sn->name), name);
> >>       } else {
> >> -        /* cast below needed for OpenBSD where tv_sec is still 'long' */
> >> -        localtime_r((const time_t *)&tv.tv_sec, &tm);
> >> +        localtime_r(&tv.tv_sec, &tm);
> >>           strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
> >>       }
> > 
> 
> but the qemu_timeval from "include/sysemu/os-win32.h" still uses a long: is this file compiled for
> win32?

Yep this fails for me when built with x86_64-w64-mingw32- (it's fine
with i686-w64-mingw32- )

Dave

> Thanks,
> Laurent
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] migration: Remove time_t cast for OpenBSD

[Daniel P. Berrangé]

On Thu, Mar 11, 2021 at 06:28:57PM +0000, Dr. David Alan Gilbert wrote:
> * Laurent Vivier (laurent@vivier.eu) wrote:
> > Le 08/03/2021 à 12:46, Thomas Huth a écrit :
> > > On 22/02/2021 08.28, Brad Smith wrote:
> > >> OpenBSD has supported 64-bit time_t across all archs since 5.5 released in 2014.
> > >>
> > >> Remove a time_t cast that is no longer necessary.
> > >>
> > >>
> > >> Signed-off-by: Brad Smith <brad@comstyle.com>
> > >>
> > >> diff --git a/migration/savevm.c b/migration/savevm.c
> > >> index 52e2d72e4b..9557f85ba9 100644
> > >> --- a/migration/savevm.c
> > >> +++ b/migration/savevm.c
> > >> @@ -2849,8 +2849,7 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
> > >>       if (name) {
> > >>           pstrcpy(sn->name, sizeof(sn->name), name);
> > >>       } else {
> > >> -        /* cast below needed for OpenBSD where tv_sec is still 'long' */
> > >> -        localtime_r((const time_t *)&tv.tv_sec, &tm);
> > >> +        localtime_r(&tv.tv_sec, &tm);
> > >>           strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
> > >>       }
> > > 
> > 
> > but the qemu_timeval from "include/sysemu/os-win32.h" still uses a long: is this file compiled for
> > win32?
> 
> Yep this fails for me when built with x86_64-w64-mingw32- (it's fine
> with i686-w64-mingw32- )

We could just switch the code to use GDateTime from GLib and thus
avoid portability issues. I think this should be equivalent:

     g_autoptr(GDateTime) now = g_date_time_new_now_local();
     g_autofree char *nowstr = g_date_time_format(now, "vm-%Y%m%d%H%M%s");
     strncpy(sn->name, sizeof(sn->name), nowstr);


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] migration: Remove time_t cast for OpenBSD

[Brad Smith]

On 3/11/2021 1:39 PM, Daniel P. Berrangé wrote:
> On Thu, Mar 11, 2021 at 06:28:57PM +0000, Dr. David Alan Gilbert wrote:
>> * Laurent Vivier (laurent@vivier.eu) wrote:
>>> Le 08/03/2021 à 12:46, Thomas Huth a écrit :
>>>> On 22/02/2021 08.28, Brad Smith wrote:
>>>>> OpenBSD has supported 64-bit time_t across all archs since 5.5 released in 2014.
>>>>>
>>>>> Remove a time_t cast that is no longer necessary.
>>>>>
>>>>>
>>>>> Signed-off-by: Brad Smith <brad@comstyle.com>
>>>>>
>>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>>> index 52e2d72e4b..9557f85ba9 100644
>>>>> --- a/migration/savevm.c
>>>>> +++ b/migration/savevm.c
>>>>> @@ -2849,8 +2849,7 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
>>>>>       if (name) {
>>>>>           pstrcpy(sn->name, sizeof(sn->name), name);
>>>>>       } else {
>>>>> -        /* cast below needed for OpenBSD where tv_sec is still 'long' */
>>>>> -        localtime_r((const time_t *)&tv.tv_sec, &tm);
>>>>> +        localtime_r(&tv.tv_sec, &tm);
>>>>>           strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
>>>>>       }
>>> but the qemu_timeval from "include/sysemu/os-win32.h" still uses a long: is this file compiled for
>>> win32?
>> Yep this fails for me when built with x86_64-w64-mingw32- (it's fine
>> with i686-w64-mingw32- )
> We could just switch the code to use GDateTime from GLib and thus
> avoid portability issues. I think this should be equivalent:
>
>       g_autoptr(GDateTime) now = g_date_time_new_now_local();
>       g_autofree char *nowstr = g_date_time_format(now, "vm-%Y%m%d%H%M%s");
>       strncpy(sn->name, sizeof(sn->name), nowstr);

Which way do you guys want to go? Something like above, remove the comment
or some variation on the comment but not mentioning OpenBSD since it is no
longer relevant?

Re: [PATCH] migration: Remove time_t cast for OpenBSD

[Brad Smith]

On 3/13/2021 6:33 PM, Brad Smith wrote:
> On 3/11/2021 1:39 PM, Daniel P. Berrangé wrote:
>> On Thu, Mar 11, 2021 at 06:28:57PM +0000, Dr. David Alan Gilbert wrote:
>>> * Laurent Vivier (laurent@vivier.eu) wrote:
>>>> Le 08/03/2021 à 12:46, Thomas Huth a écrit :
>>>>> On 22/02/2021 08.28, Brad Smith wrote:
>>>>>> OpenBSD has supported 64-bit time_t across all archs since 5.5 
>>>>>> released in 2014.
>>>>>>
>>>>>> Remove a time_t cast that is no longer necessary.
>>>>>>
>>>>>>
>>>>>> Signed-off-by: Brad Smith <brad@comstyle.com>
>>>>>>
>>>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>>>> index 52e2d72e4b..9557f85ba9 100644
>>>>>> --- a/migration/savevm.c
>>>>>> +++ b/migration/savevm.c
>>>>>> @@ -2849,8 +2849,7 @@ bool save_snapshot(const char *name, bool 
>>>>>> overwrite, const char *vmstate,
>>>>>>       if (name) {
>>>>>>           pstrcpy(sn->name, sizeof(sn->name), name);
>>>>>>       } else {
>>>>>> -        /* cast below needed for OpenBSD where tv_sec is 
>>>>>> still 'long' */
>>>>>> -        localtime_r((const time_t *)&tv.tv_sec, &tm);
>>>>>> +        localtime_r(&tv.tv_sec, &tm);
>>>>>>           strftime(sn->name, sizeof(sn->name), 
>>>>>> "vm-%Y%m%d%H%M%S", &tm);
>>>>>>       }
>>>> but the qemu_timeval from "include/sysemu/os-win32.h" still uses a 
>>>> long: is this file compiled for
>>>> win32?
>>> Yep this fails for me when built with x86_64-w64-mingw32- (it's fine
>>> with i686-w64-mingw32- )
>> We could just switch the code to use GDateTime from GLib and thus
>> avoid portability issues. I think this should be equivalent:
>>
>>       g_autoptr(GDateTime) now = g_date_time_new_now_local();
>>       g_autofree char *nowstr = g_date_time_format(now, 
>> "vm-%Y%m%d%H%M%s");
>>       strncpy(sn->name, sizeof(sn->name), nowstr);
>
> Which way do you guys want to go? Something like above, remove the 
> comment
> or some variation on the comment but not mentioning OpenBSD since it 
> is no
> longer relevant?


Anyone?

Re: [PATCH] migration: Remove time_t cast for OpenBSD

[Daniel P. Berrangé]

On Wed, Mar 31, 2021 at 03:26:16PM -0400, Brad Smith wrote:
> On 3/13/2021 6:33 PM, Brad Smith wrote:
> > On 3/11/2021 1:39 PM, Daniel P. Berrangé wrote:
> > > On Thu, Mar 11, 2021 at 06:28:57PM +0000, Dr. David Alan Gilbert wrote:
> > > > * Laurent Vivier (laurent@vivier.eu) wrote:
> > > > > Le 08/03/2021 à 12:46, Thomas Huth a écrit :
> > > > > > On 22/02/2021 08.28, Brad Smith wrote:
> > > > > > > OpenBSD has supported 64-bit time_t across all archs
> > > > > > > since 5.5 released in 2014.
> > > > > > > 
> > > > > > > Remove a time_t cast that is no longer necessary.
> > > > > > > 
> > > > > > > 
> > > > > > > Signed-off-by: Brad Smith <brad@comstyle.com>
> > > > > > > 
> > > > > > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > > > > > index 52e2d72e4b..9557f85ba9 100644
> > > > > > > --- a/migration/savevm.c
> > > > > > > +++ b/migration/savevm.c
> > > > > > > @@ -2849,8 +2849,7 @@ bool save_snapshot(const char
> > > > > > > *name, bool overwrite, const char *vmstate,
> > > > > > >       if (name) {
> > > > > > >           pstrcpy(sn->name, sizeof(sn->name), name);
> > > > > > >       } else {
> > > > > > > -        /* cast below needed for OpenBSD where
> > > > > > > tv_sec is still 'long' */
> > > > > > > -        localtime_r((const time_t *)&tv.tv_sec, &tm);
> > > > > > > +        localtime_r(&tv.tv_sec, &tm);
> > > > > > >           strftime(sn->name, sizeof(sn->name),
> > > > > > > "vm-%Y%m%d%H%M%S", &tm);
> > > > > > >       }
> > > > > but the qemu_timeval from "include/sysemu/os-win32.h" still
> > > > > uses a long: is this file compiled for
> > > > > win32?
> > > > Yep this fails for me when built with x86_64-w64-mingw32- (it's fine
> > > > with i686-w64-mingw32- )
> > > We could just switch the code to use GDateTime from GLib and thus
> > > avoid portability issues. I think this should be equivalent:
> > > 
> > >       g_autoptr(GDateTime) now = g_date_time_new_now_local();
> > >       g_autofree char *nowstr = g_date_time_format(now,
> > > "vm-%Y%m%d%H%M%s");
> > >       strncpy(sn->name, sizeof(sn->name), nowstr);
> > 
> > Which way do you guys want to go? Something like above, remove the
> > comment
> > or some variation on the comment but not mentioning OpenBSD since it is
> > no
> > longer relevant?
> 
> Anyone?

Personally I always favour using GLib APIs if there's an applicable one,
since it eliminates portability problems - or rather offloads them to
the GLib maintainers, who have already solved them generally.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] migration: Remove time_t cast for OpenBSD

[Brad Smith]

On 4/1/2021 4:14 AM, Daniel P. Berrangé wrote:
> On Wed, Mar 31, 2021 at 03:26:16PM -0400, Brad Smith wrote:
>> On 3/13/2021 6:33 PM, Brad Smith wrote:
>>> On 3/11/2021 1:39 PM, Daniel P. Berrangé wrote:
>>>> On Thu, Mar 11, 2021 at 06:28:57PM +0000, Dr. David Alan Gilbert wrote:
>>>>> * Laurent Vivier (laurent@vivier.eu) wrote:
>>>>>> Le 08/03/2021 à 12:46, Thomas Huth a écrit :
>>>>>>> On 22/02/2021 08.28, Brad Smith wrote:
>>>>>>>> OpenBSD has supported 64-bit time_t across all archs
>>>>>>>> since 5.5 released in 2014.
>>>>>>>>
>>>>>>>> Remove a time_t cast that is no longer necessary.
>>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Brad Smith <brad@comstyle.com>
>>>>>>>>
>>>>>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>>>>>> index 52e2d72e4b..9557f85ba9 100644
>>>>>>>> --- a/migration/savevm.c
>>>>>>>> +++ b/migration/savevm.c
>>>>>>>> @@ -2849,8 +2849,7 @@ bool save_snapshot(const char
>>>>>>>> *name, bool overwrite, const char *vmstate,
>>>>>>>>       if (name) {
>>>>>>>>           pstrcpy(sn->name, sizeof(sn->name), name);
>>>>>>>>       } else {
>>>>>>>> -        /* cast below needed for OpenBSD where
>>>>>>>> tv_sec is still 'long' */
>>>>>>>> -        localtime_r((const time_t *)&tv.tv_sec, &tm);
>>>>>>>> +        localtime_r(&tv.tv_sec, &tm);
>>>>>>>>           strftime(sn->name, sizeof(sn->name),
>>>>>>>> "vm-%Y%m%d%H%M%S", &tm);
>>>>>>>>       }
>>>>>> but the qemu_timeval from "include/sysemu/os-win32.h" still
>>>>>> uses a long: is this file compiled for
>>>>>> win32?
>>>>> Yep this fails for me when built with x86_64-w64-mingw32- (it's fine
>>>>> with i686-w64-mingw32- )
>>>> We could just switch the code to use GDateTime from GLib and thus
>>>> avoid portability issues. I think this should be equivalent:
>>>>
>>>>       g_autoptr(GDateTime) now = g_date_time_new_now_local();
>>>>       g_autofree char *nowstr = g_date_time_format(now,
>>>> "vm-%Y%m%d%H%M%s");
>>>>       strncpy(sn->name, sizeof(sn->name), nowstr);
>>> Which way do you guys want to go? Something like above, remove the
>>> comment
>>> or some variation on the comment but not mentioning OpenBSD since it is
>>> no
>>> longer relevant?
>> Anyone?
> Personally I always favour using GLib APIs if there's an applicable one,
> since it eliminates portability problems - or rather offloads them to
> the GLib maintainers, who have already solved them generally.

Can you please submit a proper diff and I'll drop this?