Re: [PATCHv2 2/4] qemu: Implement shared memory device cold (un)plug

[lhuang <lhuang@xxxxxxxxxx>]

On 12/10/2015 08:53 AM, John Ferlan wrote:
> On 11/26/2015 04:06 AM, Luyao Huang wrote:
> > Add support for using the attach/detach device APIs on the inactive
> > configuration to add/del shared memory devices.
> >
> > Signed-off-by: Luyao Huang <lhuang@xxxxxxxxxx>
> > ---
> >   src/libvirt_private.syms |  1 +
> >   src/qemu/qemu_driver.c   | 21 +++++++++++++++++++--
> >   2 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 88c2c53..c89d27a 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -451,6 +451,7 @@ virDomainSaveStatus;
> >   virDomainSaveXML;
> >   virDomainSeclabelTypeFromString;
> >   virDomainSeclabelTypeToString;
> > +virDomainShmemDefFree;
> >   virDomainShmemFind;
> >   virDomainShmemInsert;
> >   virDomainShmemRemove;
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 65ccf99..5ded9ef 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -8174,6 +8174,15 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
> >           dev->data.memory = NULL;
> >           break;
> >   
> > +    case VIR_DOMAIN_DEVICE_SHMEM:
> > +        if (virDomainShmemInsert(vmdef, dev->data.shmem) < 0)
> > +            return -1;
> > +        dev->data.shmem = NULL;
> > +
> > +        if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
> > +            return -1;
> > +        break;
> > +
> >       case VIR_DOMAIN_DEVICE_INPUT:
> >       case VIR_DOMAIN_DEVICE_SOUND:
> >       case VIR_DOMAIN_DEVICE_VIDEO:
> > @@ -8183,7 +8192,6 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
> >       case VIR_DOMAIN_DEVICE_SMARTCARD:
> >       case VIR_DOMAIN_DEVICE_MEMBALLOON:
> >       case VIR_DOMAIN_DEVICE_NVRAM:
> > -    case VIR_DOMAIN_DEVICE_SHMEM:
> >       case VIR_DOMAIN_DEVICE_REDIRDEV:
> >       case VIR_DOMAIN_DEVICE_NONE:
> >       case VIR_DOMAIN_DEVICE_TPM:
> > @@ -8310,6 +8318,16 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
> >           virDomainMemoryDefFree(virDomainMemoryRemove(vmdef, idx));
> >           break;
> >   
> > +    case VIR_DOMAIN_DEVICE_SHMEM:
> > +        if ((idx = virDomainShmemFind(vmdef, dev->data.shmem, true)) < 0) {
> Here rather than 'true' which to me only has meaning if I go read the
> code... Of course same thing holds true when passing a 0 (zero) flags value.
>
> BTW: My idea around flags matches what I recently added for
> VolReadErrorMode in storage_backend.h... But for this it'd be something
> like the following in (I assume) domain_conf.h near _virDomainShmemDef.
>
>      /* DomainShmemMatchFlags
>       * Flags to dictate the level of matching when searching for a
>       * shmem object in the domain 'nshmems' list.
>       */
>      enum {
>          VIR_DOMAIN_SHMEM_MATCH_NAME_ONLY = 1 << 0,
>      }
>
>
> In general though it seems fine.

Okay, i have saw your comment mentioned this in another mail, and i will 
post a new version include your idea.

Thanks a lot for your review and suggestion.

Luyao

> John
>
> > +            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> > +                           _("no matching shared memory device was found"));
> > +            return -1;
> > +        }
> > +
> > +        virDomainShmemDefFree(virDomainShmemRemove(vmdef, idx));
> > +        break;
> > +
> >       case VIR_DOMAIN_DEVICE_INPUT:
> >       case VIR_DOMAIN_DEVICE_SOUND:
> >       case VIR_DOMAIN_DEVICE_VIDEO:
> > @@ -8319,7 +8337,6 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
> >       case VIR_DOMAIN_DEVICE_SMARTCARD:
> >       case VIR_DOMAIN_DEVICE_MEMBALLOON:
> >       case VIR_DOMAIN_DEVICE_NVRAM:
> > -    case VIR_DOMAIN_DEVICE_SHMEM:
> >       case VIR_DOMAIN_DEVICE_REDIRDEV:
> >       case VIR_DOMAIN_DEVICE_NONE:
> >       case VIR_DOMAIN_DEVICE_TPM:

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