On 07/27/2012 01:20 AM, Eric Blake wrote:
> On 07/25/2012 08:50 AM, Orit Wasserman wrote:
>> In the outgoing migration check to see if the page is cached and
>> changed than send compressed page by using save_xbrle_page function.
>
> s/changed than/changed, then/
>
>> In the incoming migration check to see if RAM_SAVE_FLAG_XBZRLE is set
>> and decompress the page (by using load_xbrle function).
>>
>> Signed-off-by: Benoit Hudzia <benoit.hud...@sap.com>
>> Signed-off-by: Petter Svard <pett...@cs.umu.se>
>> Signed-off-by: Aidan Shribman <aidan.shrib...@sap.com>
>> Signed-off-by: Orit Wasserman <owass...@redhat.com>
>
>> +/* struct contains XBZRLE cache and a static page
>> + used by the compression */
>> +static struct {
>> + /* buffer used for XBZRLE encoding */
>> + uint8_t *encoded_buf;
>> + /* buffer for storing page content */
>> + uint8_t *current_buf;
>> + /* buffer used for XBZRLE decoding */
>> + uint8_t *decoded_buf;
>> + /* Cache for XBZRLE */
>> + PageCache *cache;
>> +} XBZRLE = {
>> + .encoded_buf = NULL,
>> + .current_buf = NULL,
>> + .decoded_buf = NULL,
>> + .cache = NULL,
>> +};
>
> Explicit zero-initialization of a static object is pointless; C already
> guarantees that this will be the case without an initializer, due to the
> static lifetime.
You are right but as it harmless I will keep it.
>
>> +#define ENCODING_FLAG_XBZRLE 0x1
>> +
>> +static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
>> + ram_addr_t current_addr, RAMBlock *block,
>> + ram_addr_t offset, int cont)
>> +{
>> + int encoded_len = 0, bytes_sent = -1;
>> + XBZRLEHeader hdr = {
>> + .xh_len = 0,
>> + .xh_flags = 0,
>> + };
>
> [In contrast, this explicit zero-initialization of an automatic variable
> is essential.]
>
>> +
>> + /* we need to update the data in the cache, in order to get the same
>> data
>> + we cached we decode the encoded page on the cached data */
>> + memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
>
> Comment is out of date - a memcpy is not decoding onto the cache, but
> overwriting the entire cached page.
>
will be updated.
>> +
>> + hdr.xh_len = encoded_len;
>> + hdr.xh_flags |= ENCODING_FLAG_XBZRLE;
>> +
>> + /* Send XBZRLE based compressed page */
>> + save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_XBZRLE);
>> + qemu_put_byte(f, hdr.xh_flags);
>> + qemu_put_be16(f, hdr.xh_len);
>> + qemu_put_buffer(f, XBZRLE.encoded_buf, encoded_len);
>> + bytes_sent = encoded_len + sizeof(hdr);
>
> This looks like an off by one. sizeof(hdr) is 4 (2 bytes for xh_len, 1
> byte for xh_flags, then padded out to 2-byte multiple due to uint16_t
> member); but you really only sent 3 bytes (1 for xh_flags, 2 for
> xh_len), not 4.
correct, I was think of removing the header completely anyway.
>
>> @@ -321,7 +423,18 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>> last_offset = 0;
>> sort_ram_list();
>>
>> - /* Make sure all dirty bits are set */
>
> Why are you losing this comment? It is still appropriate for the code
> in context after your insertion.
looks like bad merge I will fix it.
>
>> @@ -436,6 +549,49 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>> return 0;
>> }
>>
>> +static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
>> +{
>> + int ret, rc = 0;
>> + XBZRLEHeader hdr = {
>> + .xh_len = 0,
>> + .xh_flags = 0,
>> + };
>> +
>> + if (!XBZRLE.decoded_buf) {
>> + XBZRLE.decoded_buf = g_malloc(TARGET_PAGE_SIZE);
>> + }
>> +
>> + /* extract RLE header */
>> + hdr.xh_flags = qemu_get_byte(f);
>> + hdr.xh_len = qemu_get_be16(f);
>> +
>> + if (!(hdr.xh_flags & ENCODING_FLAG_XBZRLE)) {
>> + fprintf(stderr, "Failed to load XBZRLE page - wrong
>> compression!\n");
>
> Shouldn't you also validate that no other bits in xh_flags are set, so
> as to allow future versions of the protocol to define additional bits if
> we come up with further compression methods and gracefully be detected
> when the receiving end does not understand those bits? That is, this
> should be:
>
> if (hdr.xh_flags != ENCODING_FLAG_XBZRLE) {
>
ok
>> + return -1;
>> + }
>> +
>> + if (hdr.xh_len > TARGET_PAGE_SIZE) {
>> + fprintf(stderr, "Failed to load XBZRLE page - len overflow!\n");
>> + return -1;
>> + }
>> + /* load data and decode */
>> + qemu_get_buffer(f, XBZRLE.decoded_buf, hdr.xh_len);
>> +
>> + /* decode RLE */
>> + ret = xbzrle_decode_buffer(XBZRLE.decoded_buf, hdr.xh_len, host,
>> + TARGET_PAGE_SIZE);
>> + if (ret == -1) {
>> + fprintf(stderr, "Failed to load XBZRLE page - decode error!\n");
>> + rc = -1;
>> + } else if (ret > TARGET_PAGE_SIZE) {
>> + fprintf(stderr, "Failed to load XBZRLE page - size %d exceeds
>> %d!\n",
>> + ret, TARGET_PAGE_SIZE);
>
> Technically, this fprintf is unreachable; xbzrle_decode_buffer should
> return -1 instead of a value larger than its incoming dlen. You could
> abort() here to indicate a programming error.
ok
>
>> @@ -549,6 +705,19 @@ static int ram_load(QEMUFile *f, void *opaque, int
>> version_id)
>> }
>>
>> qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
>> + } else if (flags & RAM_SAVE_FLAG_XBZRLE) {
>> + if (!migrate_use_xbzrle()) {
>> + return -EINVAL;
>> + }
>> + void *host = host_from_stream_offset(f, addr, flags);
>> + if (!host) {
>> + return -EINVAL;
>> + }
>> +
>> + if (load_xbzrle(f, addr, host) < 0) {
>> + ret = -EINVAL;
>> + goto done;
>
> Is there any issue by returning early instead of using 'goto done' in
> all three error locations?
>
I think that it is written in this way for tracing ...
Thanks,
Orit
