Re: [PATCH 01/12] qemu: Add a hash table for the shared disks

[Osier Yang <jyang@xxxxxxxxxx>]

On 2012年12月13日 01:09, Daniel P. Berrange wrote:
> On Tue, Dec 11, 2012 at 09:37:18PM +0800, Osier Yang wrote:
> > This introduces a hash table for qemu driver, to store the shared
> > disk's info as (@disk_path, {@ref_count, @orig_cdbfilter}). @ref_count
> > is the number of domains which shares the disk. @orig_cdbfilter is
> > the original cdbfilter setting of the shared disk, it will be used
> > to restore the the disk's cdbfilter setting to original value by
> > later patches.
> >
> > * src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type
> >                           virHashTablePtr; New struct qemuSharedDiskEntry;
> >                           Declare helpers qemuAddSharedDisk,
> >                           qemuRemoveSharedDisk)
> > * src/qemu/qemu_conf.c (Implement the two helpers)
> > * src/qemu/qemu_process.c (Update 'sharedDisks' when domain
> >                             starting and shutdown)
> > * src/qemu/qemu_driver.c (Update 'sharedDisks' when attaching
> >                            or detaching disk).
> >
> > 0 is passed for orig_cdbfilter temporarily, later patches will update
> > it.
> > ---
> >   src/qemu/qemu_conf.c    |   48 +++++++++++++++++++++++++++++++++++++++++++++++
> >   src/qemu/qemu_conf.h    |   18 +++++++++++++++++
> >   src/qemu/qemu_driver.c  |   17 ++++++++++++++++
> >   src/qemu/qemu_process.c |   17 +++++++++++++++-
> >   4 files changed, 99 insertions(+), 1 deletions(-)
> >
> > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> > index 8d380a1..2b21186 100644
> > --- a/src/qemu/qemu_conf.c
> > +++ b/src/qemu/qemu_conf.c
> > @@ -556,3 +556,51 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
> >
> >       virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun,&data);
> >   }
> > +
> > +/* Increase ref count if the entry already exists, otherwise
> > + * add a new entry.
> > + */
> > +int
> > +qemuAddSharedDisk(virHashTablePtr sharedDisks,
> > +                  const char *disk_path,
> > +                  int orig_cdbfilter)
> > +{
> > +    qemuSharedDiskEntryPtr entry = NULL;
> > +
> > +    if ((entry = virHashLookup(sharedDisks, disk_path))) {
> > +        entry->ref++;
> > +    } else {
> > +        if (VIR_ALLOC(entry)<  0)
> > +            return -1;
> > +
> > +        entry->ref = 1;
> > +        entry->orig_cdbfilter = orig_cdbfilter;
> > +
> > +        if (virHashAddEntry(sharedDisks, disk_path, entry))
> > +            return -1;
>
> Leaking 'entry' in failure path.

Okay, it's missed when changing the hash value from
only 'ref' count to a struct containing the original
cdbfilter value.

> > @@ -6035,6 +6039,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> >               VIR_WARN("Failed to teardown cgroup for disk path %s",
> >                        NULLSTR(disk->src));
> >       }
> > +
> > +    if (ret == 0&&  disk->shared) {
> > +        if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0)<  0)
> > +            VIR_WARN("Failed to add disk '%s' to shared disk table",
> > +                     disk->src);
> > +    }
>
> You are not allowed to reference 'disk->src' unless you've validate
> disk->type is FILE or BLOCK.
>
> We only need to track this for TYPE_BLOCK in any case

Oh, right, okay.

> >   end:
> >       if (cgroup)
> >           virCgroupFree(&cgroup);
> > @@ -6149,6 +6159,13 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
> >                          virDomainDiskDeviceTypeToString(disk->type));
> >           break;
> >       }
> > +
> > +    if (ret == 0&&  disk->shared) {
> > +        if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src)<  0)
> > +             VIR_WARN("Failed to remove disk '%s' from shared disk table",
> > +                      disk->src);
> > +    }
>
> Same crash problem here
>
>
> > +
> >       return ret;
> >   }
> >
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index ab04599..89152b8 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -3706,8 +3706,15 @@ int qemuProcessStart(virConnectPtr conn,
> >
> >       /* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */
> >       for (i = 0; i<  vm->def->ndisks; i++) {
> > -        if (vm->def->disks[i]->rawio == 1)
> > +        virDomainDiskDefPtr disk = vm->def->disks[i];
> > +
> > +        if (disk->rawio == 1)
> >               virCommandAllowCap(cmd, CAP_SYS_RAWIO);
> > +
> > +        if (disk->shared) {
> > +            if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0)<  0)
> > +                goto cleanup;
> > +        }
>
>
> And here
>
> >       }
> >
> >       virCommandSetPreExecHook(cmd, qemuProcessHook,&hookData);
> > @@ -4104,6 +4111,14 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> >                                             flags&  VIR_QEMU_PROCESS_STOP_MIGRATED);
> >       virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
> >
> > +    for (i = 0; i<  vm->def->ndisks; i++) {
> > +        virDomainDiskDefPtr disk = vm->def->disks[i];
> > +
> > +        if (disk->shared) {
> > +            ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src));
> > +        }
> > +    }
>
> And here
>
> Daniel

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