1. Patchew
  2. QEMU
  3. View series

[PATCH] migration: Remove time_t cast for OpenBSD

Brad Smith posted 1 patch 3 years, 2 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/YDNdAiOkEYOfmbhT@humpty.home.comstyle.com
Maintainers: Brad Smith <brad@comstyle.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Juan Quintela <quintela@redhat.com>
[PATCH] migration: Remove time_t cast for OpenBSD
Posted by Brad Smith 3 years, 2 months ago 
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
Posted by Thomas Huth 3 years, 2 months ago 
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
Posted by Laurent Vivier 3 years, 2 months ago 
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
Posted by Dr. David Alan Gilbert 3 years, 2 months ago 
* 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
Posted by Daniel P. Berrangé 3 years, 2 months ago 
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
Posted by Brad Smith 3 years, 2 months ago 
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
Posted by Brad Smith 3 years, 1 month ago 
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
Posted by Daniel P. Berrangé 3 years, 1 month ago 
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
Posted by Brad Smith 3 years, 1 month ago 
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?
Re: [PATCH] migration: Remove time_t cast for OpenBSD
Posted by Brad Smith 3 years, 2 months ago 
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.

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.