Various disk-related fixes/cleanups

[libvirt] [PATCH 2/6] qemu: Move disk driver name validation into disk validation callback

Posted by Peter Krempa 7 years, 8 months ago

There were two places where we'd check this independently. Move it to
the disk definition validation callback. This also fixes possible use of
NULL in a printf for network storage.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_command.c | 12 ------------
 src/qemu/qemu_domain.c  |  9 +++++++++
 src/qemu/qemu_hotplug.c |  7 -------
 3 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a1a9e91e49..0f45a25038 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9708,18 +9708,6 @@ qemuBuildCommandLineValidate(virQEMUDriverPtr driver,
         return -1;
     }

-    for (i = 0; i < def->ndisks; i++) {
-        virDomainDiskDefPtr disk = def->disks[i];
-
-        if (disk->src->driverName != NULL &&
-            STRNEQ(disk->src->driverName, "qemu")) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("unsupported driver name '%s' for disk '%s'"),
-                           disk->src->driverName, disk->src->path);
-            return -1;
-        }
-    }
-
     return 0;
 }

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 4c4a9a428d..a3431696af 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4165,6 +4165,7 @@ static int
 qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk,
                                 virQEMUCapsPtr qemuCaps)
 {
+    const char *driverName;
     virStorageSourcePtr n;

     if (disk->src->shared && !disk->src->readonly) {
@@ -4183,6 +4184,14 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk,
         }
     }

+    if ((driverName = virDomainDiskGetDriver(disk)) &&
+        STRNEQ(driverName, "qemu")) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("unsupported driver name '%s' for disk '%s'"),
+                       driverName, disk->dst);
+        return -1;
+    }
+
     for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
         if (qemuDomainValidateStorageSource(n, qemuCaps) < 0)
             return -1;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 8d3191f971..df9e8aa716 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -699,13 +699,6 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver,
     virDomainDiskDefPtr orig_disk = NULL;
     int ret = -1;

-    if (STRNEQ_NULLABLE(virDomainDiskGetDriver(disk), "qemu")) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                       _("unsupported driver name '%s' for disk '%s'"),
-                       virDomainDiskGetDriver(disk), disk->dst);
-        goto cleanup;
-    }
-
     if (virDomainDiskTranslateSourcePool(disk) < 0)
         goto cleanup;

-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/6] qemu: Move disk driver name validation into disk validation callback

Posted by Ján Tomko 7 years, 8 months ago

On Wed, Apr 18, 2018 at 12:55:39PM +0200, Peter Krempa wrote:
>There were two places where we'd check this independently. Move it to
>the disk definition validation callback. This also fixes possible use of
>NULL in a printf for network storage.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/qemu/qemu_command.c | 12 ------------
> src/qemu/qemu_domain.c  |  9 +++++++++
> src/qemu/qemu_hotplug.c |  7 -------
> 3 files changed, 9 insertions(+), 19 deletions(-)
>
>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>index a1a9e91e49..0f45a25038 100644
>--- a/src/qemu/qemu_command.c
>+++ b/src/qemu/qemu_command.c
>@@ -9708,18 +9708,6 @@ qemuBuildCommandLineValidate(virQEMUDriverPtr driver,
>         return -1;
>     }
>
>-    for (i = 0; i < def->ndisks; i++) {
>-        virDomainDiskDefPtr disk = def->disks[i];
>-
>-        if (disk->src->driverName != NULL &&
>-            STRNEQ(disk->src->driverName, "qemu")) {
>-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>-                           _("unsupported driver name '%s' for disk '%s'"),
>-                           disk->src->driverName, disk->src->path);
>-            return -1;
>-        }
>-    }
>-
>     return 0;
> }
>
>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index 4c4a9a428d..a3431696af 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -4165,6 +4165,7 @@ static int
> qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk,
>                                 virQEMUCapsPtr qemuCaps)
> {
>+    const char *driverName;

Consider initializing the variable here

>     virStorageSourcePtr n;
>
>     if (disk->src->shared && !disk->src->readonly) {
>@@ -4183,6 +4184,14 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk,
>         }
>     }
>
>+    if ((driverName = virDomainDiskGetDriver(disk)) &&
>+        STRNEQ(driverName, "qemu")) {

and/or using STRNEQ_NULLABLE here.

>+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>+                       _("unsupported driver name '%s' for disk '%s'"),
>+                       driverName, disk->dst);
>+        return -1;
>+    }
>+
>     for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
>         if (qemuDomainValidateStorageSource(n, qemuCaps) < 0)
>             return -1;

Regardless

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/6] qemu: Move disk driver name validation into disk validation callback

Posted by Peter Krempa 7 years, 8 months ago

On Wed, Apr 18, 2018 at 13:30:24 +0200, Ján Tomko wrote:
> On Wed, Apr 18, 2018 at 12:55:39PM +0200, Peter Krempa wrote:
> > There were two places where we'd check this independently. Move it to
> > the disk definition validation callback. This also fixes possible use of
> > NULL in a printf for network storage.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> > src/qemu/qemu_command.c | 12 ------------
> > src/qemu/qemu_domain.c  |  9 +++++++++
> > src/qemu/qemu_hotplug.c |  7 -------
> > 3 files changed, 9 insertions(+), 19 deletions(-)

[...]

> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 4c4a9a428d..a3431696af 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -4165,6 +4165,7 @@ static int
> > qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk,
> >                                 virQEMUCapsPtr qemuCaps)
> > {
> > +    const char *driverName;
> 
> Consider initializing the variable here

Okay, I can do that but it's kind of pointless since it's always
overwritten.

> 
> >     virStorageSourcePtr n;
> > 
> >     if (disk->src->shared && !disk->src->readonly) {
> > @@ -4183,6 +4184,14 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk,
> >         }
> >     }
> > 
> > +    if ((driverName = virDomainDiskGetDriver(disk)) &&
> > +        STRNEQ(driverName, "qemu")) {
> 
> and/or using STRNEQ_NULLABLE here.

Umm why? It's guarded by checking of the assignment.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/6] qemu: Move disk driver name validation into disk validation callback

Posted by Ján Tomko 7 years, 8 months ago

On Wed, Apr 18, 2018 at 02:54:24PM +0200, Peter Krempa wrote:
>On Wed, Apr 18, 2018 at 13:30:24 +0200, Ján Tomko wrote:
>> On Wed, Apr 18, 2018 at 12:55:39PM +0200, Peter Krempa wrote:
>> > There were two places where we'd check this independently. Move it to
>> > the disk definition validation callback. This also fixes possible use of
>> > NULL in a printf for network storage.
>> >
>> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>> > ---
>> > src/qemu/qemu_command.c | 12 ------------
>> > src/qemu/qemu_domain.c  |  9 +++++++++
>> > src/qemu/qemu_hotplug.c |  7 -------
>> > 3 files changed, 9 insertions(+), 19 deletions(-)
>
>[...]
>
>> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> > index 4c4a9a428d..a3431696af 100644
>> > --- a/src/qemu/qemu_domain.c
>> > +++ b/src/qemu/qemu_domain.c
>> > @@ -4165,6 +4165,7 @@ static int
>> > qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk,
>> >                                 virQEMUCapsPtr qemuCaps)
>> > {
>> > +    const char *driverName;
>>
>> Consider initializing the variable here
>
>Okay, I can do that but it's kind of pointless since it's always
>overwritten.
>

Initializing to virDomainDiskGetDriver(disk);

>>
>> >     virStorageSourcePtr n;
>> >
>> >     if (disk->src->shared && !disk->src->readonly) {
>> > @@ -4183,6 +4184,14 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk,
>> >         }
>> >     }
>> >
>> > +    if ((driverName = virDomainDiskGetDriver(disk)) &&
>> > +        STRNEQ(driverName, "qemu")) {
>>
>> and/or using STRNEQ_NULLABLE here.
>
>Umm why? It's guarded by checking of the assignment.

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/6] qemu: Move disk driver name validation into disk validation callback

Posted by Peter Krempa 7 years, 8 months ago

On Wed, Apr 18, 2018 at 15:13:26 +0200, Ján Tomko wrote:
> On Wed, Apr 18, 2018 at 02:54:24PM +0200, Peter Krempa wrote:
> > On Wed, Apr 18, 2018 at 13:30:24 +0200, Ján Tomko wrote:
> > > On Wed, Apr 18, 2018 at 12:55:39PM +0200, Peter Krempa wrote:
> > > > There were two places where we'd check this independently. Move it to
> > > > the disk definition validation callback. This also fixes possible use of
> > > > NULL in a printf for network storage.
> > > >
> > > > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > > > ---
> > > > src/qemu/qemu_command.c | 12 ------------
> > > > src/qemu/qemu_domain.c  |  9 +++++++++
> > > > src/qemu/qemu_hotplug.c |  7 -------
> > > > 3 files changed, 9 insertions(+), 19 deletions(-)
> > 
> > [...]
> > 
> > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > > index 4c4a9a428d..a3431696af 100644
> > > > --- a/src/qemu/qemu_domain.c
> > > > +++ b/src/qemu/qemu_domain.c
> > > > @@ -4165,6 +4165,7 @@ static int
> > > > qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk,
> > > >                                 virQEMUCapsPtr qemuCaps)
> > > > {
> > > > +    const char *driverName;
> > > 
> > > Consider initializing the variable here
> > 
> > Okay, I can do that but it's kind of pointless since it's always
> > overwritten.
> > 
> 
> Initializing to virDomainDiskGetDriver(disk);

aaah, right. STREQ_NULLABLE can't be used though, since apparently it's
okay if it's left empty, at least according to other code
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list