From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44981) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XlyBi-0006XP-QO for qemu-devel@nongnu.org; Wed, 05 Nov 2014 05:51:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XlyBe-0005Mo-8F for qemu-devel@nongnu.org; Wed, 05 Nov 2014 05:51:42 -0500 Received: from relay.parallels.com ([195.214.232.42]:42380) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XlyBd-0005Mb-S5 for qemu-devel@nongnu.org; Wed, 05 Nov 2014 05:51:38 -0500 Message-ID: <545A0136.2080208@openvz.org> Date: Wed, 5 Nov 2014 13:51:34 +0300 From: "Denis V. Lunev" MIME-Version: 1.0 References: <1414589891-32736-1-git-send-email-den@openvz.org> <1414589891-32736-3-git-send-email-den@openvz.org> <20141105021207.GI26767@localhost.localdomain> In-Reply-To: <20141105021207.GI26767@localhost.localdomain> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/6] block/parallels: allow to specify DiskDescriptor.xml instead of image file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 05/11/14 05:12, Jeff Cody wrote: > 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 >> Acked-by: Roman Kagan >> CC: Jeff Cody >> CC: Kevin Wolf >> CC: Stefan Hajnoczi >> --- >> 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 >> +#include >> +#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(). ok, fair enough. This is a rudiment from my previous internal version which has been implemented in the other way. >> + >> +#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? yes, this constraint is used now and will be used in the future. XML structure is known and it is fixed. Here we just shortcut the navigation in the tree in a convenient way to pass entire path is one call. I think that we can switch to interface like static xmlNodePtr xml_seek(xmlNode *root, ...) and call it like xml_seek(root, "StorageData", "Storage", "Image", NULL); in this case we will be able to drop a lot of processing and parsing inside including strchr, memcpy, strlen etc >> + 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 ok >> + 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? use of uninitialized variable in the line below. path_combine(image_path, sizeof(image_path), bs->file->filename, data); which can not happen due to check size == 1 a line above. > >> + 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. thank you for pointing this out >> + } 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. > you are right. I have missed this thing. Need some thinking on this, but, anyway, this should be fixed. >> -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 >>