On Wed, Oct 29, 2014 at 04:38:07PM +0300, Denis V. Lunev wrote:
> Typically Parallels disk bundle consists of several images which are
> glued by XML disk descriptor. Also XML hides inside several important
> parameters which are not available in the image header.
>
> This patch allows to specify this XML file as a filename for an image
> to open. It is allowed only to open Compressed images with the only
> snapshot inside. No additional options are parsed at the moment.
>
> The code itself is dumb enough for a while. If XML file is specified,
> the file is parsed and the image is reopened as bs->file to keep the
> rest of the driver untouched. This would be changed later with more
> features added.
>
> Signed-off-by: Denis V. Lunev <d...@openvz.org>
> Acked-by: Roman Kagan <rka...@parallels.com>
> CC: Jeff Cody <jc...@redhat.com>
> CC: Kevin Wolf <kw...@redhat.com>
> CC: Stefan Hajnoczi <stefa...@redhat.com>
> ---
> block/parallels.c | 231
> ++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 226 insertions(+), 5 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 4f9cd8d..201c0f1 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -27,6 +27,11 @@
> #include "block/block_int.h"
> #include "qemu/module.h"
>
> +#if CONFIG_LIBXML2
> +#include <libxml/parser.h>
> +#include <libxml/tree.h>
> +#endif
> +
> /**************************************************************/
>
> #define HEADER_MAGIC "WithoutFreeSpace"
> @@ -34,6 +39,10 @@
> #define HEADER_VERSION 2
> #define HEADER_SIZE 64
>
> +#define PARALLELS_XML 100
> +#define PARALLELS_IMAGE 101
> +
> +
> // always little-endian
> struct parallels_header {
> char magic[16]; // "WithoutFreeSpace"
> @@ -59,6 +68,194 @@ typedef struct BDRVParallelsState {
> unsigned int off_multiplier;
> } BDRVParallelsState;
>
> +static int parallels_open_image(BlockDriverState *bs, Error **errp);
You shouldn't need this forward declaration, if you put your new
function parallels_open_xml() after parallels_open_image().
> +
> +#if CONFIG_LIBXML2
> +static xmlNodePtr xml_find(xmlNode *node, const char *elem)
> +{
> + xmlNode *child;
> +
> + for (child = node->xmlChildrenNode; child != NULL; child = child->next) {
> + if (!xmlStrcmp(child->name, (const xmlChar *)elem) &&
> + child->type == XML_ELEMENT_NODE) {
> + return child;
> + }
> + }
> + return NULL;
> +}
> +
> +static xmlNodePtr xml_seek(xmlNode *root, const char *elem)
> +{
> + xmlNode *child = root;
> + const char *path;
> + char nodename[128];
> + int last = 0;
> +
> + path = elem;
> + if (path[0] == '/') {
> + path++;
> + }
> + if (path[0] == 0) {
> + return NULL;
> + }
> + while (!last) {
> + const char *p = strchr(path, '/');
> + int length;
> + if (p == NULL) {
> + length = strlen(path);
> + last = 1;
> + } else {
> + length = p - path;
> + }
> + memcpy(nodename, path, length);
> + nodename[length] = 0;
It looks like "elem" is always controlled by us, and not passed by the
user - will this always be the case?
If not, this doesn't seem safe, with nodename being a local array of
128 bytes. How about using g_strdup() or g_strndup() here?
> + child = xml_find(child, nodename);
> + if (child == NULL) {
> + return NULL;
> + }
> + path = ++p;
> + }
> + return child;
> +}
> +
> +static const char *xml_get_text(xmlNode *node, const char *name)
> +{
> + xmlNode *child;
> +
> + node = xml_seek(node, name);
> + if (node == NULL) {
> + return NULL;
> + }
> +
> + for (child = node->xmlChildrenNode; child; child = child->next) {
> + if (child->type == XML_TEXT_NODE) {
> + return (const char *)child->content;
> + }
> + }
> + return NULL;
> +}
> +
> +static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
> +{
> + int size, ret;
> + xmlDoc *doc = NULL;
> + xmlNode *root, *image;
> + char *xml = NULL;
> + const char *data;
> + char image_path[PATH_MAX];
> + Error *local_err = NULL;
> +
> + ret = size = bdrv_getlength(bs->file);
> + if (ret < 0) {
> + goto fail;
> + }
> + /* XML file size should be resonable */
s/resonable/reasonable
> + ret = -EFBIG;
> + if (size > 65536) {
> + goto fail;
> + }
> +
> + xml = g_malloc(size + 1);
> +
> + ret = bdrv_pread(bs->file, 0, xml, size);
> + if (ret != size) {
> + goto fail;
> + }
> + xml[size] = 0;
> +
> + ret = -EINVAL;
> + doc = xmlReadMemory(xml, size, NULL, NULL,
> + XML_PARSE_NOERROR | XML_PARSE_NOWARNING);
> + if (doc == NULL) {
> + goto fail;
> + }
> + root = xmlDocGetRootElement(doc);
> + if (root == NULL) {
> + goto fail;
> + }
> + image = xml_seek(root, "/StorageData/Storage/Image");
> + data = ""; /* make gcc happy */
What gcc warning are you trying to suppress here?
> + for (size = 0; image != NULL; image = image->next) {
> + if (image->type != XML_ELEMENT_NODE) {
> + continue;
> + }
> +
> + size++;
> + data = xml_get_text(image, "Type");
> + if (data != NULL && strcmp(data, "Compressed")) {
> + error_setg(errp, "Only compressed Parallels images are
> supported");
> + goto done;
> + }
> +
> + data = xml_get_text(image, "File");
> + if (data == NULL) {
> + goto fail;
> + }
> + }
> + /* Images with more than 1 snapshots are not supported at the moment */
> + if (size != 1) {
> + error_setg(errp, "Parallels images with snapshots are not
> supported");
> + goto done;
> + }
> +
> + path_combine(image_path, sizeof(image_path), bs->file->filename, data);
> + /* the caller (top layer bdrv_open) will close file for us if bs->file
> + is changed. */
> + bs->file = NULL;
> +
> + ret = bdrv_open(&bs->file, image_path, NULL, NULL, flags |
> BDRV_O_PROTOCOL,
> + NULL, &local_err);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Could not open '%s': %s",
> + image_path, error_get_pretty(local_err));
> + error_free(local_err);
error_get_pretty() is not NULL friendly - but I don't think it is an
issue, because if (ret < 0) is true, then (local_err) should be set as
well. Just an observation, I don't think you need to do anything here.
> + } else {
> + ret = parallels_open_image(bs, errp);
> + }
> +
> +done:
> + if (doc != NULL) {
> + xmlFreeDoc(doc);
> + }
> + if (xml != NULL) {
> + g_free(xml);
> + }
> + return ret;
> +
> +fail:
> + error_setg(errp, "Failed to parse Parallels disk descriptor XML");
> + goto done;
> +}
> +#endif
> +
> +static int parallels_probe_xml(const uint8_t *buf, int buf_size)
> +{
> + int score = 0;
> +#if CONFIG_LIBXML2
> + xmlDoc *doc;
> + xmlNode *root;
> +
> + doc = xmlReadMemory((const char *)buf, buf_size, NULL, NULL,
> + XML_PARSE_NOERROR | XML_PARSE_NOWARNING);
> + if (doc == NULL) {
> + return 0; /* This is not an XML, we don't care */
> + }
> +
> + root = xmlDocGetRootElement(doc);
> + if (root == NULL) {
> + goto done;
> + }
> + if (!xmlStrcmp(root->name, (const xmlChar *)"Parallels_disk_image")) {
> + score = PARALLELS_XML;
> + }
> +
> +done:
> + xmlFreeDoc(doc);
> +#endif
> + return score;
> +}
> +
> +
> static int parallels_probe(const uint8_t *buf, int buf_size, const char
> *filename)
> {
> const struct parallels_header *ph = (const void *)buf;
> @@ -69,13 +266,12 @@ static int parallels_probe(const uint8_t *buf, int
> buf_size, const char *filenam
> if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
> !memcmp(ph->magic, HEADER_MAGIC2, 16)) &&
> (le32_to_cpu(ph->version) == HEADER_VERSION))
> - return 100;
> + return PARALLELS_IMAGE;
>
> - return 0;
> + return parallels_probe_xml(buf, buf_size);
> }
>
Hmm. So PARALLELS_XML is 100, and PARALLELS_IMAGE is 101 - and, this
is used later in this patch, to differentiate between XML and non-XML
images. This is somewhat abusing the .bdrv_probe(), I think - the
intention of .bdrv_probe() is to return a confidence score between
0-100.
I think you'd be better off just checking the magic in
parallels_open() rather than overloading the .bdrv_probe() function.
> -static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> - Error **errp)
> +static int parallels_open_image(BlockDriverState *bs, Error **errp)
> {
> BDRVParallelsState *s = bs->opaque;
> int i;
> @@ -139,13 +335,38 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
> return 0;
>
> fail_format:
> - error_setg(errp, "Image not in Parallels format");
> + error_setg(errp, "Image is not in Parallels format");
> ret = -EINVAL;
> fail:
> g_free(s->catalog_bitmap);
> return ret;
> }
>
> +static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> + Error **errp)
> +{
> + uint8_t buf[1024];
> + int score;
> +
> + score = bdrv_pread(bs->file, 0, buf, sizeof(buf));
> + if (score < 0) {
> + return score;
> + }
> +
> + score = parallels_probe(buf, (unsigned)score, NULL);
> + if (score == PARALLELS_XML) {
> +#if CONFIG_LIBXML2
> + return parallels_open_xml(bs, flags, errp);
> +#endif
> + } else if (score == PARALLELS_IMAGE) {
> + return parallels_open_image(bs, errp);
> + }
> +
> + error_setg(errp, "Image is not in Parallels format");
> + return -EINVAL;
> +}
> +
> +
> static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
> {
> BDRVParallelsState *s = bs->opaque;
> --
> 1.9.1
>
