https://bugzilla.redhat.com/show_bug.cgi?id=1138516 If the provided volume name doesn't match what parted generated as the partition name, then return a failure. Update virsh.pod and formatstorage.html.in to describe the 'name' restriction for disk pools as well as the usage of the <target>'s <format type='value'>. Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> --- docs/formatstorage.html.in | 12 ++++++++++-- src/storage/storage_backend_disk.c | 30 ++++++++++++++++++++++++------ tools/virsh.pod | 17 ++++++++++++++--- 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 933268c..d2e2bb8 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -463,7 +463,13 @@ <dl> <dt><code>name</code></dt> <dd>Providing a name for the volume which is unique to the pool. - This is mandatory when defining a volume. <span class="since">Since 0.4.1</span></dd> + This is mandatory when defining a volume. For a disk pool, the + name must be combination of the <code>source</code> device path + device and next partition number to be created. For example, if + the <code>source</code> device path is /dev/sdb and there are no + partitions on the disk, then the name must be sdb1 with the next + name being sdb2 and so on. + <span class="since">Since 0.4.1</span></dd> <dt><code>key</code></dt> <dd>Providing an identifier for the volume which identifies a single volume. In some cases it's possible to have two distinct keys @@ -553,7 +559,9 @@ <span class="since">Since 0.4.1</span></dd> <dt><code>format</code></dt> <dd>Provides information about the pool specific volume format. - For disk pools it will provide the partition type. For filesystem + For disk pools it will provide the partition table format type, but is + not preserved after a pool refresh or libvirtd restart. Use extended + in order to create an extended disk extent partition. For filesystem or directory pools it will provide the file format type, eg cow, qcow, vmdk, raw. If omitted when creating a volume, the pool's default format will be used. The actual format is specified via diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 300aab3..9f4d76a 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -47,16 +47,23 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, char **const groups, virStorageVolDefPtr vol) { - char *tmp, *devpath; + char *tmp, *devpath, *partname; + + /* Prepended path will be same for all partitions, so we can + * strip the path to form a reasonable pool-unique name + */ + if ((tmp = strrchr(groups[0], '/'))) + partname = tmp + 1; + else + partname = groups[0]; if (vol == NULL) { + /* This is typically a reload/restart/refresh path where + * we're discovering the existing partitions for the pool + */ if (VIR_ALLOC(vol) < 0) return -1; - /* Prepended path will be same for all partitions, so we can - * strip the path to form a reasonable pool-unique name - */ - tmp = strrchr(groups[0], '/'); - if (VIR_STRDUP(vol->name, tmp ? tmp + 1 : groups[0]) < 0 || + if (VIR_STRDUP(vol->name, partname) < 0 || VIR_APPEND_ELEMENT_COPY(pool->volumes.objs, pool->volumes.count, vol) < 0) { virStorageVolDefFree(vol); @@ -80,6 +87,17 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, return -1; } + /* Enforce provided vol->name is the same as what parted created. + * We do this after filling target.path so that we have a chance at + * deleting the partition with this failure from CreateVol path + */ + if (STRNEQ(vol->name, partname)) { + virReportError(VIR_ERR_INVALID_ARG, + _("invalid partition name '%s', expected '%s'"), + vol->name, partname); + return -1; + } + if (vol->key == NULL) { /* XXX base off a unique key of the underlying disk */ if (VIR_STRDUP(vol->key, vol->target.path) < 0) diff --git a/tools/virsh.pod b/tools/virsh.pod index abe80c2..1591341 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2983,7 +2983,11 @@ Create and start a pool object from the XML I<file>. Create and start a pool object I<name> from the raw parameters. If I<--print-xml> is specified, then print the XML of the pool object without creating the pool. Otherwise, the pool has the specified -I<type>. +I<type>. When using B<pool-create-as> for a pool of I<type> "disk", +the existing partitions found on the I<--source-dev path> will be used +to populate the disk pool. Therefore, it is suggested to use +B<pool-define-as> and B<pool-build> with the I<--overwrite> in order +to properly initialize the disk pool. [I<--source-host hostname>] provides the source hostname for pools backed by storage from a remote server (pool types netfs, iscsi, rbd, sheepdog, @@ -3175,13 +3179,20 @@ I<vol-name-or-key-or-path>] [I<--backing-vol-format> I<string>] Create a volume from a set of arguments. I<pool-or-uuid> is the name or UUID of the storage pool to create the volume in. -I<name> is the name of the new volume. +I<name> is the name of the new volume. For a disk pool, this must match the +partition name as determined from the pool's source device path and the next +available partition. For example, a source device path of /dev/sdb and there +are no partitions on the disk, then the name must be sdb1 with the next +name being sdb2 and so on. I<capacity> is the size of the volume to be created, as a scaled integer (see B<NOTES> above), defaulting to bytes if there is no suffix. I<--allocation> I<size> is the initial size to be allocated in the volume, also as a scaled integer defaulting to bytes. I<--format> I<string> is used in file based storage pools to specify the volume -file format to use; raw, bochs, qcow, qcow2, vmdk, qed. +file format to use; raw, bochs, qcow, qcow2, vmdk, qed. Use extended for disk +storage pools in order to create an extended partition (other values are +validity checked but not preserved when libvirtd is restarted or the pool +is refreshed). I<--backing-vol> I<vol-name-or-key-or-path> is the source backing volume to be used if taking a snapshot of an existing volume. I<--backing-vol-format> I<string> is the format of the snapshot backing volume; -- 2.1.0 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list