STATUS
------
Patch looks OK. No requests, but a few suggestions.
REQUESTS
--------
None.
COMMENTARY
----------
The errors reported from these methods to bstream_close() are ignored.
Should this be fixed here or in a separate task?
SUGGESTIONS
-----------
1. Consider explicitly returning BSTREAM_ERROR instead of the return
value which in theory may also be BSTREAM_EOS etc. Marked inline
as [1] in code below.
2. [Minor] Inconsistent use of blank lines between call and error check.
(Some places a blank line is added, some places not.) I prefer no
blank line since that signal that it is part of the functional call
handling and not the start of something new. Marked inline
as [2] in code below.
3. Suggest to add "Never errors"-comment to call to
append_to_buffer().
4. Instead of adding to lines for every if block, did you consider
creating a function, e.g. stream_error(), that sets s->state and
returns BSTREAM_ERROR. Then error checks could be like this.
if (ret != BSTREAM_OK)
return stream_error();
DETAILS
-------
See inline.
--
Øystein
Rafal Somla wrote:
> #At file:///ext/mysql/bzr/backup/bug37522/
>
> 2691 Rafal Somla 2008-09-04
> BUG#37522 (Backup: stream_v1_transport: Error condition should be checked for
> as_* calls)
>
> This patch adds checks for errors in stream_v1_transport.c where the low level
> I/O
> functions are called.
> modified:
> sql/backup/stream_v1.h
> sql/backup/stream_v1_transport.c
>
> per-file messages:
> sql/backup/stream_v1.h
> Add specifications for the low-level I/O functions implementing abstract stream
> interface.
> sql/backup/stream_v1_transport.c
> - Make sure error is reported and stream state set to ERROR whenever the
> low-level I/O functions report error.
> - Turn append_to_buffer() into void function as it can never error.
> === modified file 'sql/backup/stream_v1.h'
> --- a/sql/backup/stream_v1.h 2008-05-14 00:35:24 +0000
> +++ b/sql/backup/stream_v1.h 2008-09-04 09:01:30 +0000
> @@ -378,6 +378,16 @@ int bstream_next_chunk(backup_stream*);
> the bytes being written to/read from a backup stream. This is done by
> storing pointers to functions performing basic I/O operations inside
> this structure.
> +
> + Low-level @c write(), @c read() and @c forward() functions should return
> + BSTREAM_OK on success and BSTREAM_ERROR on error. Additionally, @c read()
> + should return BSTREAM_EOS if there is no more data in the input stream.
> +
> + Functions @c write() and @c read() should behave like functions
> + @c bstream_write_part() and @c bstream_read_part(), respectively. The latter
> + are documented in stream_v1_transport.c. The only difference is that low-level
> + @c read() doesn't deal with chunk boundaries and never returns BSTREAM_EOC.
> + However, it should detect end of stream and return BSTREAM_EOS in that case.
> */
> struct st_abstract_stream
> {
>
> === modified file 'sql/backup/stream_v1_transport.c'
> --- a/sql/backup/stream_v1_transport.c 2008-07-19 03:03:39 +0000
> +++ b/sql/backup/stream_v1_transport.c 2008-09-04 09:01:30 +0000
> @@ -386,7 +386,14 @@ int write_buffer(backup_stream *s)
> int ret= BSTREAM_OK;
>
> if (s->buf.pos > s->buf.begin)
> + {
> ret= as_write_all(&s->stream,*(bstream_blob*)&s->buf,s->mem);
> + if (ret != BSTREAM_OK)
> + {
> + s->state= ERROR;
> + return BSTREAM_ERROR;
> + }
> + }
>
> /*
> Now buffer should be empty. If whole block has been written, reset buffer
> @@ -398,7 +405,7 @@ int write_buffer(backup_stream *s)
> else
> s->buf.begin= s->buf.header= s->buf.pos; /* now invariant should hold
> */
>
> - return ret;
> + return BSTREAM_OK;
> }
>
> /**
> @@ -408,18 +415,16 @@ int write_buffer(backup_stream *s)
> is left for fragment`s header.
> */
> static
> -int append_to_buffer(backup_stream *s, blob *b)
> +void append_to_buffer(backup_stream *s, blob *b)
> {
> if (b->begin >= b->end) /* no data to append */
> - return BSTREAM_OK;
> + return;
>
> if(s->buf.pos == s->buf.header) /* current fragment empty */
> s->buf.pos++;
>
> while ((s->buf.pos < s->buf.end) && (b->begin <
> b->end))
> *(s->buf.pos++)= *(b->begin++);
> -
> - return BSTREAM_OK;
> }
>
> /**
> @@ -658,6 +663,12 @@ int load_buffer(backup_stream *s)
>
> ret= as_read(&s->stream,&data,s->mem);
>
> + if (ret == BSTREAM_ERROR)
> + {
> + s->state= ERROR;
> + return BSTREAM_ERROR;
> + }
> +
> s->buf.pos= data.begin;
[2] Here you have added a blank line before the error test. In the
case above you did not.
>
> /*
> @@ -751,6 +762,7 @@ int load_buffer(backup_stream *s)
> @retval BSTREAM_OK next fragment has been entered
> @retval BSTREAM_EOC the entered fragment is the last fragment of a chunk
> @retval BSTREAM_EOS there are no more fragments in the stream
> + @retval BSTREAM_ERROR error when reading from underlying stream
> */
> static
> int load_next_fragment(backup_stream *s)
> @@ -766,7 +778,7 @@ int load_next_fragment(backup_stream *s)
>
> s->state= NORMAL; /* default, unless changed below */
>
> - ret= read_fragment_header(&s->buf.header);
> + ret= read_fragment_header(&s->buf.header); // Never errors.
>
> /*
> If buf.header was not moved, it means that the fragment extends to
> @@ -1052,7 +1064,10 @@ int bstream_write_part(backup_stream *s,
>
> ret= as_write_all(&s->stream,*data,buf);
> if (ret != BSTREAM_OK)
> + {
> + s->state= ERROR;
> return ret;
> + }
[1] I suggest returning BSTREAM_ERROR here.
>
> data->begin= data->end;
> data->end= saved_end;
> @@ -1088,6 +1103,11 @@ int bstream_write_part(backup_stream *s,
> data->end= data->begin + (fragment.begin - s->buf.pos);
>
> ret= as_write_all(&s->stream,*data,*data);
> + if (ret != BSTREAM_OK)
> + {
> + s->state= ERROR;
> + return ret;
> + }
[1] I suggest returning BSTREAM_ERROR here.
>
> data->begin= data->end;
> data->end= saved_end;
> @@ -1185,6 +1205,8 @@ int bstream_end_chunk(backup_stream *s)
> else
> {
> ret= close_current_fragment(s);
> + if (ret != BSTREAM_OK)
> + return BSTREAM_ERROR;
>
> /*
> If the last fragment in the buffer is a small fragment, we can mark it as
> @@ -1423,7 +1445,6 @@ int bstream_read_part(backup_stream *s,
> if ((s->buf.begin == s->buf.end) || (s->buf.begin == s->buf.header))
> {
> ret= load_buffer(s);
> -
> if (ret != BSTREAM_OK)
> return ret;
> }
> @@ -1482,11 +1503,17 @@ int bstream_read_part(backup_stream *s,
> saved= *data;
> data->end= data->begin + howmuch;
>
> - if (as_read(&s->stream, data, buf) == BSTREAM_EOS)
> + ret= as_read(&s->stream, data, buf);
> +
> + if (ret == BSTREAM_ERROR)
> {
> - s->state= EOS;
> + s->state= ERROR;
> + return BSTREAM_ERROR;
> }
>
> + if (ret == BSTREAM_EOS)
> + s->state= EOS;
> +
> s->buf.begin += data->begin - saved.begin;
> s->buf.pos= s->buf.begin;
>
> @@ -1592,7 +1619,14 @@ int bstream_next_chunk(backup_stream *s)
> else
> {
> howmuch= s->buf.header - s->buf.pos;
> - as_forward(&s->stream, &howmuch);
> + ret= as_forward(&s->stream, &howmuch);
> +
> + if (ret != BSTREAM_OK)
> + {
> + s->state= ERROR;
> + return BSTREAM_ERROR;
> + }
> +
> s->buf.begin= s->buf.pos= s->buf.header;
> }
> }
> @@ -1621,6 +1655,9 @@ int bstream_next_chunk(backup_stream *s)
>
> ret= load_next_fragment(s);
>
> + if (ret == BSTREAM_ERROR) // To avoid invariant check below.
> + return ret;
> +
> /* if we hit EOC marker here, we treat it as empty chunk */
> if (ret == BSTREAM_EOC)
> ret= BSTREAM_OK;
> @@ -1638,5 +1675,13 @@ int bstream_next_chunk(backup_stream *s)
> */
> int bstream_skip(backup_stream *s, unsigned long int howmuch)
> {
> - return as_forward(&s->stream, &howmuch);
> + int ret= as_forward(&s->stream, &howmuch);
> +
> + if (ret != BSTREAM_OK)
> + {
> + s->state= ERROR;
> + return BSTREAM_ERROR;
> + }
> +
> + return BSTREAM_OK;
> }
>
>
--
Øystein Grøvlen, Senior Staff Engineer
Sun Microsystems, Database Group
Trondheim, Norway
