[libvirt] [PATCH] Reject duplicate disk targets

[Daniel P. Berrange]

On Wed, Jul 10, 2013 at 07:22:11PM +0200, Martin Kletzander wrote:
> On 07/10/2013 04:03 PM, Daniel P. Berrange wrote:
> > On Wed, Jul 10, 2013 at 03:51:45PM +0200, Martin Kletzander wrote:
> >> On 07/10/2013 02:39 PM, Daniel P. Berrange wrote:
> >>> On Wed, Jul 10, 2013 at 02:34:30PM +0200, Martin Kletzander wrote:
> >>>> We never check for disks with duplicate targets connected to the same
> >>>> controller/bus/etc.  That means we go ahead, create the same IDs for
> >>>> them and pass them to qemu, which subsequently fails to start.
> >>>>
> >>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=968899
> >>>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> >>>> ---
> >>>>  src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> >>>>  1 file changed, 41 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >>>> index 3398d8b..01720e1 100644
> >>>> --- a/src/conf/domain_conf.c
> >>>> +++ b/src/conf/domain_conf.c
> >>>> @@ -2629,6 +2629,45 @@ virDomainDeviceInfoIterate(virDomainDefPtr def,
> >>>>
> >>>>
> >>>>  static int
> >>>> +virDomainDefRejectDuplicateDiskTargets(virDomainDefPtr def)
> >>>> +{
> >>>> +    char *disk_id = NULL;
> >>>> +    int ret = -1;
> >>>> +    size_t i = 0;
> >>>> +    virHashTablePtr targets = NULL;
> >>>> +
> >>>> +    if (!(targets = virHashCreate(def->ndisks, NULL)))
> >>>> +        goto cleanup;
> >>>> +
> >>>> +    for (i = 0; i < def->ndisks; i++) {
> >>>> +        virDomainDiskDefPtr disk = def->disks[i];
> >>>> +
> >>>> +        if (virAsprintf(&disk_id, "%d%s%d%d%d%d",
> >>>> +                        disk->bus,
> >>>> +                        NULLSTR(disk->dst),
> >>>> +                        disk->info.addr.drive.controller,
> >>>> +                        disk->info.addr.drive.bus,
> >>>> +                        disk->info.addr.drive.target,
> >>>> +                        disk->info.addr.drive.unit) < 0)
> >>>
> >>> Err, disk->info.addr is a union of many different types of
> >>> address. You can't just arbitrarily reference info.addr.drive
> >>> without first validating the type of the union.
> >>>
> >>
> >> I jumped straight into that because we know it is disk and we do it the
> >> same way when assigning aliases.  I'd be glad to see what option I
> >> missed, thanks for any pointers.
> > 
> > Assigning aliases is not the same, because the alias is stored
> > in the same part of the struct, regardless of address type.
> > 
> > Any disk with bus=virtio uses PCI addressing. Any disk with
> > bus=usb uses USB addresing.
> > 
> 
> In order to make this as unified as possible, I suggest we check it
> when the domain is starting, right after these aliases are assigned.
> 
> Reasoning -- there is no problem with using same addresses, this is
> already being checked for.  The problem is only with our internal
> naming which is externally seen only when we use it for describing
> disks to qemu.  Because the way we choose how to describe is based upon
> qemu capabilities, it would make sense to do it when starting the
> domain then.  Looking back at this patch, it might break some non-qemu
> machines.  The patch would then look like the following, would you be
> OK with that?
> 
> Martin
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 325ef38..f19c83f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -864,6 +864,32 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller)
>  }
> 
> 
> +static int
> +qemuRejectDuplicateDiskIDs(virDomainDefPtr def)
> +{
> +    int ret = -1;
> +    size_t i = 0;
> +    virHashTablePtr targets = NULL;
> +
> +    if (!(targets = virHashCreate(def->ndisks, NULL)))
> +        goto cleanup;
> +
> +    for (i = 0; i < def->ndisks; i++) {
> +        if (virHashAddEntry(targets, def->disks[i]->info.alias, NULL) < 0) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("Multiple disks with the same alias ('%s')"),
> +                           def->disks[i]->info.alias);
> +            goto cleanup;
> +        }
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    virHashFree(targets);
> +    return ret;
> +}
> +
> +
>  int
>  qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
>  {
> @@ -886,6 +912,9 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
>          }
>      }
> 
> +    if (qemuRejectDuplicateDiskIDs(def) < 0)
> +        return -1;
> +
>      if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
>          return 0;

Huh, that doesn't make any sense. The 'alias' data is not user specified.
If they attempt to set it in the XML, it'll be dropped by the parser. The
alias names are assigned by the QEMU driver unconditionally and unless
that code is broken in some way, you'll never get a duplicate alias


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|