| Message ID | 20111106160022.GF3225@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/06/2011 11:00 AM, Gleb Natapov wrote: > The caller of qemu_timedate_diff() does not expect that tm it passes to > the function will be modified, but mktime() is destructive and modifies > its argument. Pass a copy of tm to it and set tm_isdst so that mktime() > will not rely on it since its value may be outdated. Ohhh, nice catch. > Signed-off-by: Gleb Natapov<gleb@redhat.com> Acked-by: Rik van Riel <riel@redhat.com>
On 11/06/2011 06:00 PM, Gleb Natapov wrote: > The caller of qemu_timedate_diff() does not expect that tm it passes to > the function will be modified, but mktime() is destructive and modifies > its argument. Pass a copy of tm to it and set tm_isdst so that mktime() > will not rely on it since its value may be outdated. I believe that the original issue was not related to outdated data at the moment of the daylight saving time transition. using tmp.tm_isdst = -1 sounds good, but why use a copy of tm? The only significant field that will change in the tm is the tm_isdst itself that will be set to 0/1 (correctly). Acked-by: Ronen Hod <rhod@redhat.com> > Signed-off-by: Gleb Natapov<gleb@redhat.com> > diff --git a/vl.c b/vl.c > index 624da0f..641629b 100644 > --- a/vl.c > +++ b/vl.c > @@ -460,8 +460,11 @@ int qemu_timedate_diff(struct tm *tm) > if (rtc_date_offset == -1) > if (rtc_utc) > seconds = mktimegm(tm); > - else > - seconds = mktime(tm); > + else { > + struct tm tmp = *tm; > + tmp.tm_isdst = -1; /* use timezone to figure it out */ > + seconds = mktime(&tmp); > + } > else > seconds = mktimegm(tm) + rtc_date_offset; > > -- > Gleb. >
On Mon, Nov 07, 2011 at 10:16:29PM +0200, Ronen Hod wrote: > On 11/06/2011 06:00 PM, Gleb Natapov wrote: > >The caller of qemu_timedate_diff() does not expect that tm it passes to > >the function will be modified, but mktime() is destructive and modifies > >its argument. Pass a copy of tm to it and set tm_isdst so that mktime() > >will not rely on it since its value may be outdated. > > I believe that the original issue was not related to outdated data > at the moment of the daylight saving time transition. Don't know what do you mean here. The problem is definitely related to incorrect data about daylight saving time. > using tmp.tm_isdst = -1 sounds good, but why use a copy of tm? The Hmm, what is not clear about "qemu_timedate_diff() shouldn't modify its argument"? :) > only significant field that will change in the tm is the tm_isdst > itself that will be set to 0/1 (correctly). This is incorrect (read man), but event if it would breed a new kind of especially handsome ponies, if this is what caller wants it should call mktime() by itself and not be a side effect of some random function call. > > Acked-by: Ronen Hod <rhod@redhat.com> > > >Signed-off-by: Gleb Natapov<gleb@redhat.com> > >diff --git a/vl.c b/vl.c > >index 624da0f..641629b 100644 > >--- a/vl.c > >+++ b/vl.c > >@@ -460,8 +460,11 @@ int qemu_timedate_diff(struct tm *tm) > > if (rtc_date_offset == -1) > > if (rtc_utc) > > seconds = mktimegm(tm); > >- else > >- seconds = mktime(tm); > >+ else { > >+ struct tm tmp = *tm; > >+ tmp.tm_isdst = -1; /* use timezone to figure it out */ > >+ seconds = mktime(&tmp); > >+ } > > else > > seconds = mktimegm(tm) + rtc_date_offset; > > > >-- > > Gleb. > > -- Gleb.
On 11/06/2011 10:00 AM, Gleb Natapov wrote: > The caller of qemu_timedate_diff() does not expect that tm it passes to > the function will be modified, but mktime() is destructive and modifies > its argument. Pass a copy of tm to it and set tm_isdst so that mktime() > will not rely on it since its value may be outdated. > > Signed-off-by: Gleb Natapov<gleb@redhat.com> Applied. Thanks. Regards, Anthony Liguori > diff --git a/vl.c b/vl.c > index 624da0f..641629b 100644 > --- a/vl.c > +++ b/vl.c > @@ -460,8 +460,11 @@ int qemu_timedate_diff(struct tm *tm) > if (rtc_date_offset == -1) > if (rtc_utc) > seconds = mktimegm(tm); > - else > - seconds = mktime(tm); > + else { > + struct tm tmp = *tm; > + tmp.tm_isdst = -1; /* use timezone to figure it out */ > + seconds = mktime(&tmp); > + } > else > seconds = mktimegm(tm) + rtc_date_offset; > > -- > Gleb. > >
diff --git a/vl.c b/vl.c index 624da0f..641629b 100644 --- a/vl.c +++ b/vl.c @@ -460,8 +460,11 @@ int qemu_timedate_diff(struct tm *tm) if (rtc_date_offset == -1) if (rtc_utc) seconds = mktimegm(tm); - else - seconds = mktime(tm); + else { + struct tm tmp = *tm; + tmp.tm_isdst = -1; /* use timezone to figure it out */ + seconds = mktime(&tmp); + } else seconds = mktimegm(tm) + rtc_date_offset;
The caller of qemu_timedate_diff() does not expect that tm it passes to the function will be modified, but mktime() is destructive and modifies its argument. Pass a copy of tm to it and set tm_isdst so that mktime() will not rely on it since its value may be outdated. Signed-off-by: Gleb Natapov <gleb@redhat.com> -- Gleb.