From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49331) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuWQ5-0001J8-M5 for qemu-devel@nongnu.org; Thu, 26 Jul 2012 18:20:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SuWQ3-0005dM-HO for qemu-devel@nongnu.org; Thu, 26 Jul 2012 18:20:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9219) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuWQ3-0005cw-8w for qemu-devel@nongnu.org; Thu, 26 Jul 2012 18:20:31 -0400 Message-ID: <5011C2A5.7000401@redhat.com> Date: Thu, 26 Jul 2012 16:20:21 -0600 From: Eric Blake MIME-Version: 1.0 References: <1343227834-5400-1-git-send-email-owasserm@redhat.com> <1343227834-5400-9-git-send-email-owasserm@redhat.com> In-Reply-To: <1343227834-5400-9-git-send-email-owasserm@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig38EBD1CB49CC945EEC41F4FD" Subject: Re: [Qemu-devel] [PATCH 07/11] Add XBZRLE to ram_save_block and ram_save_live List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Orit Wasserman Cc: peter.maydell@linaro.org, aliguori@us.ibm.com, quintela@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com, blauwirbel@gmail.com, Petter Svard , Benoit Hudzia , avi@redhat.com, Aidan Shribman , pbonzini@redhat.com, lcapitulino@redhat.com, chegu_vinod@hp.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig38EBD1CB49CC945EEC41F4FD Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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). >=20 > Signed-off-by: Benoit Hudzia > Signed-off-by: Petter Svard > Signed-off-by: Aidan Shribman > Signed-off-by: Orit Wasserman > +/* 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 =3D { > + .encoded_buf =3D NULL, > + .current_buf =3D NULL, > + .decoded_buf =3D NULL, > + .cache =3D 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. > +#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 =3D 0, bytes_sent =3D -1; > + XBZRLEHeader hdr =3D { > + .xh_len =3D 0, > + .xh_flags =3D 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 sa= me 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. > + > + hdr.xh_len =3D encoded_len; > + hdr.xh_flags |=3D 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 =3D 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. > @@ -321,7 +423,18 @@ static int ram_save_setup(QEMUFile *f, void *opaqu= e) > last_offset =3D 0; > sort_ram_list(); > =20 > - /* 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. > @@ -436,6 +549,49 @@ static int ram_save_complete(QEMUFile *f, void *op= aque) > return 0; > } > =20 > +static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) > +{ > + int ret, rc =3D 0; > + XBZRLEHeader hdr =3D { > + .xh_len =3D 0, > + .xh_flags =3D 0, > + }; > + > + if (!XBZRLE.decoded_buf) { > + XBZRLE.decoded_buf =3D g_malloc(TARGET_PAGE_SIZE); > + } > + > + /* extract RLE header */ > + hdr.xh_flags =3D qemu_get_byte(f); > + hdr.xh_len =3D qemu_get_be16(f); > + > + if (!(hdr.xh_flags & ENCODING_FLAG_XBZRLE)) { > + fprintf(stderr, "Failed to load XBZRLE page - wrong compressio= n!\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 !=3D ENCODING_FLAG_XBZRLE) { > + 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 =3D xbzrle_decode_buffer(XBZRLE.decoded_buf, hdr.xh_len, host,= > + TARGET_PAGE_SIZE); > + if (ret =3D=3D -1) { > + fprintf(stderr, "Failed to load XBZRLE page - decode error!\n"= ); > + rc =3D -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. > @@ -549,6 +705,19 @@ static int ram_load(QEMUFile *f, void *opaque, int= version_id) > } > =20 > qemu_get_buffer(f, host, TARGET_PAGE_SIZE); > + } else if (flags & RAM_SAVE_FLAG_XBZRLE) { > + if (!migrate_use_xbzrle()) { > + return -EINVAL; > + } > + void *host =3D host_from_stream_offset(f, addr, flags); > + if (!host) { > + return -EINVAL; > + } > + > + if (load_xbzrle(f, addr, host) < 0) { > + ret =3D -EINVAL; > + goto done; Is there any issue by returning early instead of using 'goto done' in all three error locations? --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig38EBD1CB49CC945EEC41F4FD Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBCAAGBQJQEcKlAAoJEKeha0olJ0NqOuIH+gJcOpxJjc85Fe0P2vCEddOu mzHOko4bwihG29lyFotDIEI1snF7eCboSpTVbu9QH7L+zhymKSU7k0trQwMwt5Cu FXFMt0HhOht+qdS1EcAWfCbshis+b9UH6vejSq8of5nk7vhyvjX6osVOZWPzYczx c1Vh78Cn2qhQ7zs54ELPzW6h/YetyK/jNspfTw+9on3/12LAJ0r883AZouDL3VBA EmfK8y+nPdHL1JbthF7nlZviAgSc72ssZCrXX1es5REmAdbawbjKS0/AXppkKwKq 3wHl2vTpWEDXxA0PgNeYVaJPL9b40HPb2XdIqAQP5s1JnpTDALLzTLaN4qYbAy4= =h9+l -----END PGP SIGNATURE----- --------------enig38EBD1CB49CC945EEC41F4FD--