Hi Chuck,
STATUS: Approved pending changes
REQUIRED:
4) Please change 4 to 8 in the comment about time value size.
5) Please undo accidental indentation changes.
8) Please cast to (time_t).
REQUESTS:
1) Use thread safe function instead of gmtime().
2) Please do not use single-letter variable names.
6) Please do not add space at end of line.
7) Please do not use single-letter variable names.
9) Please keep the alignment of the structure members.
SUGGESTIONS:
10) Consider to print in local time.
COMMENTARY:
3) I offer to download the test archives from Pushbuild and compile the
follow-up patch.
-) I'm surprised, how simple the change of time format is. Thanks for
doing it.
DETAILS:
Chuck Bell, 11.09.2009 19:12:
...
> 2868 Chuck Bell 2009-09-11
> BUG#43221 : Test backup_logs fails on powermac platform
>
> The backup_logs test fails on the powermac platform because the
> timezone variable is not set correctly thus the timezone
> calculation in the backup code in image_info.h (see below)
> fails.
>
> return mktime(&time) - (time_t)timezone;
>
> It was decided on 8 September 2009 to change the internal
> format for time in the backup stream from bstream_time_t
> which was a special segmented format to the standard time_t
> format.
>
> This patch effects this change and renenables the backup_logs
> test.
>
> Note: This is a patch 1 of 2 as it is necessary to apply
> this patch first with the backup_xpfm_compat_restore*
> tests disabled then submit a second patch that replaces
> the backup image files needed in those tests and reenable
> them.
>
> Note: This patch changes the backup image format.
...
> === modified file 'client/mysqlbackup.cc'
> --- a/client/mysqlbackup.cc 2009-09-10 07:35:02 +0000
> +++ b/client/mysqlbackup.cc 2009-09-11 17:12:53 +0000
> @@ -761,12 +761,13 @@ cleanup_client(void)
> */
>
> static void
> -print_time(bstream_time_t *time)
> +print_time(time_t *time)
> {
> DBUG_ASSERT(time);
> + tm *t= gmtime(time);
> printf("%04d-%02d-%02d %02d:%02d:%02d UTC",
> - time->year + 1900, time->mon + 1, time->mday,
> - time->hour, time->min, time->sec);
> + t->tm_year + 1900, t->tm_mon + 1, t->tm_mday,
> + t->tm_hour, t->tm_min, t->tm_sec);
1) Request: Though mysqlbackup is no multi-threaded program currently, I
propose to use thread-safe functions anyway. A quote from the gmtime man
page:
"
The four functions asctime(), ctime(), gmtime() and localtime() return
a pointer to static data and hence are not thread-safe. Thread-safe
versions asctime_r(), ctime_r(), gmtime_r() and localtime_r() are
specified by SUSv2, and available since libc 5.2.5.
"
However, I'm unsure, if gmtime_r() is portable enough. At least on
Windows it may not exist. But the MSDN documentation claims that
gmtime() is thread safe on Windows. Please have a look into
mysys/get_date.c. It shows a possible way, how to deal with the
portability. get_date() is broadly used in MySQL, so it should be good
enough here.
2) Request: Please do not use single-letter variable names. For 't' I
suggest 'tm_ptr' or 'tm_p'. But please don't use 'tmp', it's used
everywhere for all types of data. I don't want that.
10) Suggestion: Now that we have time_t as input, we could easily
convert the time to local time: gmtime[_r] -> localtime[_r] and no "
UTC". That would be more consistent with showing the log entries in
local time too.
...
> === modified file 'mysql-test/suite/backup/t/disabled.def'
...
> +backup_xpfm_compat_restore_lctn0 : BUG#43221 2009-09-11 cbell Disabled so that
> backup files can be generated.
> +backup_xpfm_compat_restore_lctn1 : BUG#43221 2009-09-11 cbell Disabled so that
> backup files can be generated.
> +backup_xpfm_compat_restore_lctn2 : BUG#43221 2009-09-11 cbell Disabled so that
> backup files can be generated.
3) Comment: If your internet access suffers from low bandwidth, I offer
to download the test archives from Pushbuild and compile the follow-up
patch. My downlink is usually pretty good.
...
> === modified file 'sql/backup/stream_v1.c'
...
> @@ -1944,8 +1944,8 @@ int bstream_rd_data_chunk(backup_stream
> @endverbatim
> - 0x00 (1 byte): To distinguishing the summary from preceding table data
> chunks.
> - - @c vp_time (6 bytes): Time of validity point.
> - - @c end_time (6 bytes): Time when backup ended.
> + - @c vp_time (4 bytes): Time of validity point.
> + - @c end_time (4 bytes): Time when backup ended.
4) Required: Please change 4 to 8 in the comment. I believe that's what
this patch implements. Times are written with bstream_wr_int8().
...
> @@ -2172,52 +2172,106 @@ int bstream_rd_int2(backup_stream *s, un
> }
>
> /**
> - Write 4 byte unsigned number. Least significant bytes come first.
> -*/
> + Write 4 byte unsigned number. Least significant bytes come first.
> + */
5) Required: The original indentation was in line with the coding
guidelines. The new one violates them. Please undo. This repeats a
couple of times later in the patch, even in the headers of the new
functions.
> int bstream_wr_int4(backup_stream *s, unsigned long int x)
> {
> byte buf[4];
> blob b;
> int i;
> -
> +
6) Request: Please do not add space at end of line. AFAIK we do not have
a rule for avoiding end-space, but changing nothing but to append space
is not acceptable. I guess, you didn't do that intentionally, but please
fix it anyway. This repeats a couple of times later in the patch.
...
> @@ -2417,31 +2437,14 @@ int bstream_wr_time(backup_stream *s, bs
> @retval BSTREAM_OK Read successful
> @retval BSTREAM_EOC Read successful and end of chunk has been reached
> @retval BSTREAM_EOS Read successful and end of stream has been reached
> -
> - @see @ref basic_data_time
> */
> -int bstream_rd_time(backup_stream *s, bstream_time_t *time)
> +int bstream_rd_time(backup_stream *s, time_t *time)
> {
...
> + unsigned long long t;
> +
> + int ret= bstream_rd_int8(s, &t);
> + *time= (unsigned long int) t;
7) Request: Please do not use single-letter variable names. For 't' I
suggest 'time8' or something.
8) Required: Please cast to (time_t). Do not rely on time_t being a long
type. Nor rely on it being an unsigned type.
> + return ret;
> }
>
> /**
>
> === modified file 'sql/backup/stream_v1.h'
...
> @@ -153,9 +152,9 @@ struct st_bstream_image_header
> /** version of the server which created the image */
> struct st_server_version server_version;
> unsigned int flags; /**< image options */
> - bstream_time_t start_time; /**< time when backup operation started */
> - bstream_time_t end_time; /**< time when backup operation completed */
> - bstream_time_t vp_time; /**< time of the image's validity point */
> + time_t start_time; /**< time when backup operation started */
> + time_t end_time; /**< time when backup operation completed */
> + time_t vp_time; /**< time of the image's validity point */
9) Request: Please keep the alignment of the structure members.
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
