Re: [libvirt] [PATCH] qemu: make attaching disk partition to VM illegal
On Fri, Sep 27, 2019 at 12:41:29 -0300, Daniel Henrique Barboza wrote:
On 9/27/19 12:11 PM, Pavel Mores wrote:
> The way in which the qemu driver generates aliases for disks involves
> ignoring the partition number part of a target dev name. This means that
> all partitions of a block device and the device itself all end up with the
> same alias. If multiple such disks are specified in XML, the resulting
> name clash makes qemu invocation fail.
>
> Since attaching partitions to qemu VMs doesn't seem to make much sense
> anyway, disallow partitions in target specifications altogether.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1346265
>
> Signed-off-by: Pavel Mores <pmores(a)redhat.com>
> ---
I have a small nit below, but patch seems fine.
Unfortunately it breaks 'make check' in my machine, in virschematest:
297 281) Checking chardev-reconnect-generated-path.xml against domain.rng
... OK
298 282) Checking disk-attaching-partition-invalid.xml against domain.rng
... FAILED
Ah this is a sneaky one. The virschematest was written so that it
iterates all XML files in the tests to validate against the schema so
that it doesn't require any action. There was just 1 bit of metadata it
needs and that is if the given XML file is expected to fail schema
validation. We chose to identify this by having the 'invalid.xml' suffix
so that we don't have to keep an external database.
This means that the new test data is annotated as if it should fail
schema validation but it passes, thus the test fails.
Thus you must rename the test.
299 283) Checking iommu-smmuv3.xml against
domain.rng
... OK
> src/qemu/qemu_domain.c | 10 +++++++
> .../disk-attaching-partition-invalid.xml | 27 +++++++++++++++++++
> tests/qemuxml2argvtest.c | 1 +
> 3 files changed, 38 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/disk-attaching-partition-invalid.xml
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index e8e895d9aa..d03f3bed5f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5880,6 +5880,8 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk,
> {
> const char *driverName = virDomainDiskGetDriver(disk);
> virStorageSourcePtr n;
> + int idx;
> + int partition;
> if (disk->src->shared && !disk->src->readonly &&
> !qemuBlockStorageSourceSupportsConcurrentAccess(disk->src)) {
> @@ -5948,6 +5950,14 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef
*disk,
> return -1;
> }
> + int result = virDiskNameParse(disk->dst, &idx, &partition);
> + if (result != 0 || partition != 0) {
So virDiskNameParse returns -1 if it fails to parse the whole target
string. This means for example also if it has the wrong prefix where the
error message would be wrong.
Please add a separate error message for that case. Also you don't really
need to store the return value so you can check it directly.
The error message can read something like VIR_ERR_CONFIG_UNSUPPORTED,
"invalid disk target '%s'".
Additionally once you add the check suggested above, you can in a
separate patch remove the call to virDiskNameToIndex from
qemuCheckDiskConfig which will become redundant. (with a nice bonus of
improving the error message)
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("can't attach disk partition '%s', please
attach whole disk instead"),
> + disk->dst);
Break line to keep the line <= 80 chars plz.
Note that 80 columns is no longer a strict requirement especially if it
would make code harder to read. In this case it can be broken.
> + return -1;
> + }
> +
> for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore)
{
> if (qemuDomainValidateStorageSource(n, qemuCaps) < 0)
> return -1;