From 6cc4d6a3fe82653c607c4f159901790298e80e1f Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 20 Nov 2013 17:04:05 -0700 Subject: [PATCH] storage: use valid XML for awkward volume names $ touch /var/lib/libvirt/images/'ac' $ virsh pool-refresh default $ virsh vol-dumpxml 'ac' default | head -n2 ac Oops. That's not valid XML. And when we fix the XML generation, it fails RelaxNG validation. I'm also tired of seeing (null) in the example output for volume xml; while we used NULLSTR() to avoid a NULL deref rather than relying on glibc's printf extension behavior, it's even better if we avoid the issue in the first place. But this requires being careful that we don't invalidate any storage backends that were relying on key being unassigned during virStoragVolCreateXML[From]. I would have split this into two patches (one for escaping, one for avoiding (null)), but since they both end up touching a lot of the same test files, I ended up merging it into one. Note that this patch allows pretty much any volume name that can appear in a directory (excluding . and .. because those are special), but does nothing to change the current (unenforced) RelaxNG claim that pool names will consist only of letters, numbers, _, -, and +. Tightening the C code to match RelaxNG patterns and/or relaxing the grammar to match the C code for pool names is a task for another day (but remember, we DID recently tighten C code for domain names to exclude a leading '.'). * src/conf/storage_conf.c (virStoragePoolSourceFormat) (virStoragePoolDefFormat, virStorageVolTargetDefFormat) (virStorageVolDefFormat): Escape user-controlled strings. (virStorageVolDefParseXML): Parse key, for use in unit tests. * src/storage/storage_driver.c (storageVolCreateXML) (storageVolCreateXMLFrom): Ensure parsed key doesn't confuse volume creation. * docs/schemas/basictypes.rng (volName): Relax definition. * tests/storagepoolxml2xmltest.c (mymain): Test it. * tests/storagevolxml2xmltest.c (mymain): Likewise. * tests/storagepoolxml2xmlin/pool-dir-naming.xml: New file. * tests/storagepoolxml2xmlout/pool-dir-naming.xml: Likewise. * tests/storagevolxml2xmlin/vol-file-naming.xml: Likewise. * tests/storagevolxml2xmlout/vol-file-naming.xml: Likewise. * tests/storagevolxml2xmlout/vol-*.xml: Fix fallout. Signed-off-by: Eric Blake --- docs/schemas/basictypes.rng | 9 ++- src/conf/storage_conf.c | 72 +++++++++---------- src/storage/storage_driver.c | 8 ++- .../storagepoolxml2xmlin/pool-dir-naming.xml | 18 +++++ .../storagepoolxml2xmlout/pool-dir-naming.xml | 18 +++++ tests/storagepoolxml2xmltest.c | 1 + .../storagevolxml2xmlin/vol-file-backing.xml | 1 + tests/storagevolxml2xmlin/vol-file-naming.xml | 20 ++++++ .../storagevolxml2xmlout/vol-file-backing.xml | 2 +- .../storagevolxml2xmlout/vol-file-naming.xml | 17 +++++ tests/storagevolxml2xmlout/vol-file.xml | 1 - .../vol-logical-backing.xml | 2 +- tests/storagevolxml2xmlout/vol-logical.xml | 2 +- tests/storagevolxml2xmlout/vol-partition.xml | 2 +- .../vol-qcow2-0.10-lazy.xml | 2 +- tests/storagevolxml2xmlout/vol-qcow2-1.1.xml | 2 +- tests/storagevolxml2xmlout/vol-qcow2-lazy.xml | 2 +- .../vol-qcow2-nobacking.xml | 2 +- tests/storagevolxml2xmlout/vol-qcow2.xml | 2 +- tests/storagevolxml2xmlout/vol-sheepdog.xml | 1 - tests/storagevolxml2xmltest.c | 1 + 21 files changed, 132 insertions(+), 53 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-dir-naming.xml create mode 100644 tests/storagepoolxml2xmlout/pool-dir-naming.xml create mode 100644 tests/storagevolxml2xmlin/vol-file-naming.xml create mode 100644 tests/storagevolxml2xmlout/vol-file-naming.xml diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 7fbfa5340a..268bc5a7b4 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -291,8 +291,15 @@ + - [a-zA-Z0-9_\+\-\.]+ + [^/]+ + + + . + .. + + diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 33e4cafc56..8b378c208e 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1056,7 +1056,8 @@ virStoragePoolSourceFormat(virBufferPtr buf, virBufferAddLit(buf, " \n"); if ((options->flags & VIR_STORAGE_POOL_SOURCE_HOST) && src->nhost) { for (i = 0; i < src->nhost; i++) { - virBufferAsprintf(buf, " hosts[i].name); + virBufferEscapeString(buf, " hosts[i].name); if (src->hosts[i].port) virBufferAsprintf(buf, " port='%d'", src->hosts[i].port); virBufferAddLit(buf, "/>\n"); @@ -1067,8 +1068,8 @@ virStoragePoolSourceFormat(virBufferPtr buf, src->ndevice) { for (i = 0; i < src->ndevice; i++) { if (src->devices[i].nfreeExtent) { - virBufferAsprintf(buf, " \n", - src->devices[i].path); + virBufferEscapeString(buf, " \n", + src->devices[i].path); for (j = 0; j < src->devices[i].nfreeExtent; j++) { virBufferAsprintf(buf, " \n", src->devices[i].freeExtents[j].start, @@ -1076,15 +1077,14 @@ virStoragePoolSourceFormat(virBufferPtr buf, } virBufferAddLit(buf, " \n"); } else { - virBufferAsprintf(buf, " \n", - src->devices[i].path); + virBufferEscapeString(buf, " \n", + src->devices[i].path); } } } - if ((options->flags & VIR_STORAGE_POOL_SOURCE_DIR) && - src->dir) - virBufferAsprintf(buf, " \n", src->dir); + if (options->flags & VIR_STORAGE_POOL_SOURCE_DIR) + virBufferEscapeString(buf, " \n", src->dir); if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER)) { if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST || @@ -1104,9 +1104,8 @@ virStoragePoolSourceFormat(virBufferPtr buf, } } - if ((options->flags & VIR_STORAGE_POOL_SOURCE_NAME) && - src->name) - virBufferAsprintf(buf, " %s\n", src->name); + if (options->flags & VIR_STORAGE_POOL_SOURCE_NAME) + virBufferEscapeString(buf, " %s\n", src->name); if ((options->flags & VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN) && src->initiator.iqn) { @@ -1129,11 +1128,12 @@ virStoragePoolSourceFormat(virBufferPtr buf, if (src->authType == VIR_STORAGE_POOL_AUTH_CHAP || src->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { - virBufferAsprintf(buf, " \n", - virStoragePoolAuthTypeTypeToString(src->authType), - (src->authType == VIR_STORAGE_POOL_AUTH_CHAP ? - src->auth.chap.username : - src->auth.cephx.username)); + virBufferAsprintf(buf, " authType)); + virBufferEscapeString(buf, "username='%s'>\n", + (src->authType == VIR_STORAGE_POOL_AUTH_CHAP ? + src->auth.chap.username : + src->auth.cephx.username)); virBufferAddLit(buf, " auth.cephx.secret.uuidUsable) { @@ -1149,13 +1149,8 @@ virStoragePoolSourceFormat(virBufferPtr buf, virBufferAddLit(buf, " \n"); } - if (src->vendor != NULL) { - virBufferEscapeString(buf, " \n", src->vendor); - } - - if (src->product != NULL) { - virBufferEscapeString(buf, " \n", src->product); - } + virBufferEscapeString(buf, " \n", src->vendor); + virBufferEscapeString(buf, " \n", src->product); virBufferAddLit(buf, " \n"); @@ -1182,7 +1177,7 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) goto cleanup; } virBufferAsprintf(&buf, "\n", type); - virBufferAsprintf(&buf, " %s\n", def->name); + virBufferEscapeString(&buf, " %s\n", def->name); virUUIDFormat(def->uuid, uuid); virBufferAsprintf(&buf, " %s\n", uuid); @@ -1203,8 +1198,7 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) def->type != VIR_STORAGE_POOL_SHEEPDOG) { virBufferAddLit(&buf, " \n"); - if (def->target.path) - virBufferAsprintf(&buf, " %s\n", def->target.path); + virBufferEscapeString(&buf, " %s\n", def->target.path); virBufferAddLit(&buf, " \n"); virBufferAsprintf(&buf, " 0%o\n", @@ -1214,9 +1208,8 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) virBufferAsprintf(&buf, " %d\n", (int) def->target.perms.gid); - if (def->target.perms.label) - virBufferAsprintf(&buf, " \n", - def->target.perms.label); + virBufferEscapeString(&buf, " \n", + def->target.perms.label); virBufferAddLit(&buf, " \n"); virBufferAddLit(&buf, " \n"); @@ -1282,8 +1275,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, goto error; } - /* Auto-generated so deliberately ignore */ - /* ret->key = virXPathString("string(./key)", ctxt); */ + /* Normally generated by pool refresh, but useful for unit tests */ + ret->key = virXPathString("string(./key)", ctxt); capacity = virXPathString("string(./capacity)", ctxt); unit = virXPathString("string(./capacity/@unit)", ctxt); @@ -1485,11 +1478,11 @@ static int virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferPtr buf, virStorageVolTargetPtr def, - const char *type) { + const char *type) +{ virBufferAsprintf(buf, " <%s>\n", type); - if (def->path) - virBufferAsprintf(buf, " %s\n", def->path); + virBufferEscapeString(buf, " %s\n", def->path); if (options->formatToString) { const char *format = (options->formatToString)(def->format); @@ -1511,8 +1504,7 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, (unsigned int) def->perms.gid); - if (def->perms.label) - virBufferAsprintf(buf, " \n", + virBufferEscapeString(buf, " \n", def->perms.label); virBufferAddLit(buf, " \n"); @@ -1572,8 +1564,8 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, return NULL; virBufferAddLit(&buf, "\n"); - virBufferAsprintf(&buf, " %s\n", def->name); - virBufferAsprintf(&buf, " %s\n", NULLSTR(def->key)); + virBufferEscapeString(&buf, " %s\n", def->name); + virBufferEscapeString(&buf, " %s\n", def->key); virBufferAddLit(&buf, " \n"); if (def->source.nextent) { @@ -1585,8 +1577,8 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, if (thispath != NULL) virBufferAddLit(&buf, " \n"); - virBufferAsprintf(&buf, " \n", - def->source.extents[i].path); + virBufferEscapeString(&buf, " \n", + def->source.extents[i].path); } virBufferAsprintf(&buf, diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index b3f0871fce..3b4715a240 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1554,6 +1554,9 @@ storageVolCreateXML(virStoragePoolPtr obj, goto cleanup; } + /* Wipe any key the user may have suggested, as volume creation + * will generate the canonical key. */ + VIR_FREE(voldef->key); if (backend->createVol(obj->conn, pool, voldef) < 0) { goto cleanup; } @@ -1729,7 +1732,10 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, pool->volumes.count+1) < 0) goto cleanup; - /* 'Define' the new volume so we get async progress reporting */ + /* 'Define' the new volume so we get async progress reporting. + * Wipe any key the user may have suggested, as volume creation + * will generate the canonical key. */ + VIR_FREE(newvol->key); if (backend->createVol(obj->conn, pool, newvol) < 0) { goto cleanup; } diff --git a/tests/storagepoolxml2xmlin/pool-dir-naming.xml b/tests/storagepoolxml2xmlin/pool-dir-naming.xml new file mode 100644 index 0000000000..aa043beb64 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-dir-naming.xml @@ -0,0 +1,18 @@ + + virtimages + 70a7eb15-6c34-ee9c-bf57-69e8e5ff3fb2 + 0 + 0 + 0 + + + + ///var/////lib/libvirt/<images>// + + 0700 + -1 + -1 + + + + diff --git a/tests/storagepoolxml2xmlout/pool-dir-naming.xml b/tests/storagepoolxml2xmlout/pool-dir-naming.xml new file mode 100644 index 0000000000..536f58c7b9 --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-dir-naming.xml @@ -0,0 +1,18 @@ + + virtimages + 70a7eb15-6c34-ee9c-bf57-69e8e5ff3fb2 + 0 + 0 + 0 + + + + /var/lib/libvirt/<images> + + 0700 + -1 + -1 + + + + diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 743e1cb86d..a81cf99704 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -85,6 +85,7 @@ mymain(void) ret = -1 DO_TEST("pool-dir"); + DO_TEST("pool-dir-naming"); DO_TEST("pool-fs"); DO_TEST("pool-logical"); DO_TEST("pool-logical-nopath"); diff --git a/tests/storagevolxml2xmlin/vol-file-backing.xml b/tests/storagevolxml2xmlin/vol-file-backing.xml index d23349e658..73e7f288df 100644 --- a/tests/storagevolxml2xmlin/vol-file-backing.xml +++ b/tests/storagevolxml2xmlin/vol-file-backing.xml @@ -1,5 +1,6 @@ sparse.img + /var/lib/libvirt/images/sparse.img 10 0 diff --git a/tests/storagevolxml2xmlin/vol-file-naming.xml b/tests/storagevolxml2xmlin/vol-file-naming.xml new file mode 100644 index 0000000000..9a33e2ba31 --- /dev/null +++ b/tests/storagevolxml2xmlin/vol-file-naming.xml @@ -0,0 +1,20 @@ + + <sparse>.img + + 1 + 0 + + /var/lib/libvirt/images/<sparse>.img + + 0 + 0744 + 0 + + + + 1341933637.273190990 + 1341930622.047245868 + 1341930622.047245868 + + + diff --git a/tests/storagevolxml2xmlout/vol-file-backing.xml b/tests/storagevolxml2xmlout/vol-file-backing.xml index c0f152e84f..8d2fb57624 100644 --- a/tests/storagevolxml2xmlout/vol-file-backing.xml +++ b/tests/storagevolxml2xmlout/vol-file-backing.xml @@ -1,6 +1,6 @@ sparse.img - (null) + /var/lib/libvirt/images/sparse.img 10000000000 diff --git a/tests/storagevolxml2xmlout/vol-file-naming.xml b/tests/storagevolxml2xmlout/vol-file-naming.xml new file mode 100644 index 0000000000..7022b0287e --- /dev/null +++ b/tests/storagevolxml2xmlout/vol-file-naming.xml @@ -0,0 +1,17 @@ + + <sparse>.img + + + 1099511627776 + 0 + + /var/lib/libvirt/images/<sparse>.img + + + 00 + 744 + 0 + + + + diff --git a/tests/storagevolxml2xmlout/vol-file.xml b/tests/storagevolxml2xmlout/vol-file.xml index a3d6473867..b97dd50401 100644 --- a/tests/storagevolxml2xmlout/vol-file.xml +++ b/tests/storagevolxml2xmlout/vol-file.xml @@ -1,6 +1,5 @@ sparse.img - (null) 1099511627776 diff --git a/tests/storagevolxml2xmlout/vol-logical-backing.xml b/tests/storagevolxml2xmlout/vol-logical-backing.xml index 6b010e33bb..bf34b08678 100644 --- a/tests/storagevolxml2xmlout/vol-logical-backing.xml +++ b/tests/storagevolxml2xmlout/vol-logical-backing.xml @@ -1,6 +1,6 @@ Swap - (null) + r4xkCv-MQhr-WKIT-R66x-Epn2-e8hG-1Z5gY0 2080374784 diff --git a/tests/storagevolxml2xmlout/vol-logical.xml b/tests/storagevolxml2xmlout/vol-logical.xml index 7bf309eddd..e9b4e4baf4 100644 --- a/tests/storagevolxml2xmlout/vol-logical.xml +++ b/tests/storagevolxml2xmlout/vol-logical.xml @@ -1,6 +1,6 @@ Swap - (null) + r4xkCv-MQhr-WKIT-R66x-Epn2-e8hG-1Z5gY0 2080374784 diff --git a/tests/storagevolxml2xmlout/vol-partition.xml b/tests/storagevolxml2xmlout/vol-partition.xml index 271964f5bf..9be1cf175b 100644 --- a/tests/storagevolxml2xmlout/vol-partition.xml +++ b/tests/storagevolxml2xmlout/vol-partition.xml @@ -1,6 +1,6 @@ sda1 - (null) + /dev/sda1 106896384 diff --git a/tests/storagevolxml2xmlout/vol-qcow2-0.10-lazy.xml b/tests/storagevolxml2xmlout/vol-qcow2-0.10-lazy.xml index a7b5fedfb8..fd3d6069e0 100644 --- a/tests/storagevolxml2xmlout/vol-qcow2-0.10-lazy.xml +++ b/tests/storagevolxml2xmlout/vol-qcow2-0.10-lazy.xml @@ -1,6 +1,6 @@ OtherDemo.img - (null) + /var/lib/libvirt/images/OtherDemo.img 5368709120 diff --git a/tests/storagevolxml2xmlout/vol-qcow2-1.1.xml b/tests/storagevolxml2xmlout/vol-qcow2-1.1.xml index b7df8a6cae..99fb5acafb 100644 --- a/tests/storagevolxml2xmlout/vol-qcow2-1.1.xml +++ b/tests/storagevolxml2xmlout/vol-qcow2-1.1.xml @@ -1,6 +1,6 @@ OtherDemo.img - (null) + /var/lib/libvirt/images/OtherDemo.img 5368709120 diff --git a/tests/storagevolxml2xmlout/vol-qcow2-lazy.xml b/tests/storagevolxml2xmlout/vol-qcow2-lazy.xml index 92b7875668..3708ea742b 100644 --- a/tests/storagevolxml2xmlout/vol-qcow2-lazy.xml +++ b/tests/storagevolxml2xmlout/vol-qcow2-lazy.xml @@ -1,6 +1,6 @@ OtherDemo.img - (null) + /var/lib/libvirt/images/OtherDemo.img 5368709120 diff --git a/tests/storagevolxml2xmlout/vol-qcow2-nobacking.xml b/tests/storagevolxml2xmlout/vol-qcow2-nobacking.xml index e2da702bcd..f6a2e2183d 100644 --- a/tests/storagevolxml2xmlout/vol-qcow2-nobacking.xml +++ b/tests/storagevolxml2xmlout/vol-qcow2-nobacking.xml @@ -1,6 +1,6 @@ OtherDemo.img - (null) + /var/lib/libvirt/images/OtherDemo.img 5368709120 diff --git a/tests/storagevolxml2xmlout/vol-qcow2.xml b/tests/storagevolxml2xmlout/vol-qcow2.xml index f931a629cd..b9adcb4e20 100644 --- a/tests/storagevolxml2xmlout/vol-qcow2.xml +++ b/tests/storagevolxml2xmlout/vol-qcow2.xml @@ -1,6 +1,6 @@ OtherDemo.img - (null) + /var/lib/libvirt/images/OtherDemo.img 5368709120 diff --git a/tests/storagevolxml2xmlout/vol-sheepdog.xml b/tests/storagevolxml2xmlout/vol-sheepdog.xml index 2f19af8f19..bd5d6d8f06 100644 --- a/tests/storagevolxml2xmlout/vol-sheepdog.xml +++ b/tests/storagevolxml2xmlout/vol-sheepdog.xml @@ -1,6 +1,5 @@ test2 - (null) 1024 diff --git a/tests/storagevolxml2xmltest.c b/tests/storagevolxml2xmltest.c index 1fd01f15c4..e1db4653f0 100644 --- a/tests/storagevolxml2xmltest.c +++ b/tests/storagevolxml2xmltest.c @@ -110,6 +110,7 @@ mymain(void) while (0); DO_TEST("pool-dir", "vol-file"); + DO_TEST("pool-dir", "vol-file-naming"); DO_TEST("pool-dir", "vol-file-backing"); DO_TEST("pool-dir", "vol-qcow2"); DO_TEST("pool-dir", "vol-qcow2-1.1");