Re: [PATCH 2/3] qemu: don't use vmdef without domain lock

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 11/24/2016 04:19 AM, Nikolay Shirokovskiy wrote:
> Current call to qemuAgentGetFSInfo in qemuDomainGetFSInfo is
> unsafe. Domain lock is dropped and we use vm->def. To fix that
> let's fill in intermediate qemuAgentFsInfo structure in
> qemuAgentGetFSInfo and use vm->def to convert result later when
> lock is hold.
> ---
>  src/qemu/qemu_agent.c                    | 52 +++++++++++--------
>  src/qemu/qemu_agent.h                    | 25 ++++++++-
>  src/qemu/qemu_driver.c                   | 88 +++++++++++++++++++++++++++++++-
>  tests/Makefile.am                        |  1 -
>  tests/qemuagentdata/qemuagent-fsinfo.xml | 39 --------------
>  tests/qemuagenttest.c                    | 47 +++++++----------
>  6 files changed, 159 insertions(+), 93 deletions(-)
>  delete mode 100644 tests/qemuagentdata/qemuagent-fsinfo.xml
> 

This failed to compile for me as 'num' wasn't initialized in
qemuDomainGetFSInfo

> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index c5cf403..5230cbc 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -1835,18 +1835,15 @@ qemuAgentSetTime(qemuAgentPtr mon,
>  
>  
>  int
> -qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
> -                   virDomainDefPtr vmdef)
> +qemuAgentGetFSInfo(qemuAgentPtr mon, qemuAgentFsInfoPtr **info)
>  {
>      size_t i, j, k;
>      int ret = -1;
>      ssize_t ndata = 0, ndisk;
> -    char **alias;
>      virJSONValuePtr cmd;
>      virJSONValuePtr reply = NULL;
>      virJSONValuePtr data;
> -    virDomainFSInfoPtr *info_ret = NULL;
> -    virPCIDeviceAddress pci_address;
> +    qemuAgentFsInfoPtr *info_ret = NULL;
>  
>      cmd = qemuAgentMakeCommand("guest-get-fsinfo", NULL);
>      if (!cmd)
> @@ -1879,6 +1876,8 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
>          goto cleanup;
>  
>      for (i = 0; i < ndata; i++) {
> +        qemuAgentFsDiskAliasPtr alias;
> +
>          /* Reverse the order to arrange in mount order */
>          virJSONValuePtr entry = virJSONValueArrayGet(data, ndata - 1 - i);
>  
> @@ -1941,7 +1940,6 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
>              int diskaddr[3], pciaddr[4];
>              const char *diskaddr_comp[] = {"bus", "target", "unit"};
>              const char *pciaddr_comp[] = {"domain", "bus", "slot", "function"};
> -            virDomainDiskDefPtr diskDef;
>  
>              if (!disk) {
>                  virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -1967,6 +1965,11 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
>                      goto cleanup;
>                  }
>              }
> +
> +            alias->bus = diskaddr[0];
> +            alias->target = diskaddr[1];
> +            alias->unit = diskaddr[2];
> +
>              for (k = 0; k < 4; k++) {
>                  if (virJSONValueObjectGetNumberInt(
>                          pci, pciaddr_comp[k], &pciaddr[k]) < 0) {
> @@ -1977,22 +1980,13 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
>                  }
>              }
>  
> -            pci_address.domain = pciaddr[0];
> -            pci_address.bus = pciaddr[1];
> -            pci_address.slot = pciaddr[2];
> -            pci_address.function = pciaddr[3];
> -            if (!(diskDef = virDomainDiskByAddress(
> -                     vmdef, &pci_address,
> -                     diskaddr[0], diskaddr[1], diskaddr[2])))
> -                continue;
> +            alias->address.domain = pciaddr[0];
> +            alias->address.bus = pciaddr[1];
> +            alias->address.slot = pciaddr[2];
> +            alias->address.function = pciaddr[3];
>  
> -            if (VIR_STRDUP(*alias, diskDef->dst) < 0)
> -                goto cleanup;
> -
> -            if (*alias) {
> -                alias++;
> -                info_ret[i]->ndevAlias++;
> -            }
> +            alias++;
> +            info_ret[i]->ndevAlias++;
>          }
>      }
>  
> @@ -2003,7 +1997,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
>   cleanup:
>      if (info_ret) {
>          for (i = 0; i < ndata; i++)
> -            virDomainFSInfoFree(info_ret[i]);
> +            qemuAgentFsInfoFree(info_ret[i]);
>          VIR_FREE(info_ret);
>      }
>      virJSONValueFree(cmd);
> @@ -2242,3 +2236,17 @@ qemuAgentSetUserPassword(qemuAgentPtr mon,
>      VIR_FREE(password64);
>      return ret;
>  }
> +
> +void
> +qemuAgentFsInfoFree(qemuAgentFsInfoPtr info)
> +{
> +    if (!info)
> +        return;
> +
> +    VIR_FREE(info->mountpoint);
> +    VIR_FREE(info->name);
> +    VIR_FREE(info->fstype);
> +    VIR_FREE(info->devAlias);

You'd have to free each 'ndevAlias' here.

> +
> +    VIR_FREE(info);
> +}
> diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
> index 6dd9c70..4b2277c 100644
> --- a/src/qemu/qemu_agent.h
> +++ b/src/qemu/qemu_agent.h
> @@ -75,8 +75,29 @@ int qemuAgentShutdown(qemuAgentPtr mon,
>  int qemuAgentFSFreeze(qemuAgentPtr mon,
>                        const char **mountpoints, unsigned int nmountpoints);
>  int qemuAgentFSThaw(qemuAgentPtr mon);
> -int qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
> -                       virDomainDefPtr vmdef);
> +
> +typedef struct _qemuAgentFsDiskAlias qemuAgentFsDiskAlias;
> +typedef qemuAgentFsDiskAlias *qemuAgentFsDiskAliasPtr;
> +struct _qemuAgentFsDiskAlias {
> +    virPCIDeviceAddress address;
> +    unsigned int bus;
> +    unsigned int target;
> +    unsigned int unit;
> +};
> +
> +typedef struct _qemuAgentFsInfo qemuAgentFsInfo;
> +typedef qemuAgentFsInfo *qemuAgentFsInfoPtr;
> +struct _qemuAgentFsInfo {
> +    char *mountpoint; /* path to mount point */
> +    char *name;       /* device name in the guest (e.g. "sda1") */
> +    char *fstype;     /* filesystem type */
> +    size_t ndevAlias; /* number of elements in devAlias */
> +    qemuAgentFsDiskAliasPtr devAlias;  /* array of disk device aliases */
> +};
> +
> +void qemuAgentFsInfoFree(qemuAgentFsInfoPtr info);
> +
> +int qemuAgentGetFSInfo(qemuAgentPtr mon, qemuAgentFsInfoPtr **info);
>  
>  int qemuAgentSuspend(qemuAgentPtr mon,
>                       unsigned int target);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index fdfe912..1bc5dc6 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19672,14 +19672,89 @@ qemuNodeAllocPages(virConnectPtr conn,
>  
>  
>  static int
> +qemuDomainConvertFsInfo(qemuAgentFsInfoPtr *agent_info, int num,
> +                        virDomainDefPtr def,
> +                        virDomainFSInfoPtr **info)

qemuDomainFsInfoConvert

> +{
> +    int ret = -1;
> +    size_t i;
> +    virDomainFSInfoPtr *info_ret = NULL;
> +
> +    if (num == 0) {
> +        *info = NULL;
> +        return 0;
> +    }
> +
> +    if (VIR_ALLOC_N(info_ret, num) < 0)
> +        return -1;
> +
> +    for (i = 0; i < num; i++) {
> +        size_t j;
> +        int devnum;
> +
> +        if (VIR_ALLOC(info_ret[i]) < 0)
> +            goto cleanup;
> +
> +        if (VIR_STRDUP(info_ret[i]->mountpoint, agent_info[i]->mountpoint) < 0 ||
> +            VIR_STRDUP(info_ret[i]->name, agent_info[i]->name) < 0 ||
> +            VIR_STRDUP(info_ret[i]->fstype, agent_info[i]->fstype) < 0)
> +            goto cleanup;

I think you could certainly make use of VIR_STEAL_PTR here

> +
> +        if (agent_info[i]->ndevAlias == 0)
> +            continue;
> +
> +        if (VIR_EXPAND_N(info_ret[i]->devAlias,
> +                         info_ret[i]->ndevAlias,
> +                         agent_info[i]->ndevAlias) < 0)
> +            goto cleanup;

Not sure EXPAND/SHRINK will be necessary

> +
> +        devnum = 0;
> +        for (j = 0; j < agent_info[i]->ndevAlias; j++) {
> +            virDomainDiskDefPtr diskDef;
> +            qemuAgentFsDiskAliasPtr alias = &agent_info[i]->devAlias[j];
> +
> +            if (!(diskDef = virDomainDiskByAddress(def, &alias->address,
> +                                                   alias->bus, alias->target,
> +                                                   alias->unit)))
> +                continue;
> +
> +            if (VIR_STRDUP(info_ret[i]->devAlias[devnum++], diskDef->dst) < 0)
> +                goto cleanup;

Would it be possible to use something like VIR_APPEND_ELEMENT of a
VIR_STEAL_PTR on diskDef->dst; ?


> +        }
> +
> +        if (devnum < info_ret[i]->ndevAlias)
> +            VIR_SHRINK_N(info_ret[i]->devAlias,
> +                         info_ret[i]->ndevAlias,
> +                         info_ret[i]->ndevAlias - devnum);
> +    }
> +
> +    *info = info_ret;
> +    info_ret = NULL;
> +    ret = num;
> +
> + cleanup:
> +    if (info_ret) {
> +        for (i = 0; i < num; i++)
> +            virDomainFSInfoFree(info_ret[i]);
> +        VIR_FREE(info_ret);
> +    }
> +
> +    return ret;
> +}
> +
> +
> +static int
>  qemuDomainGetFSInfo(virDomainPtr dom,
>                      virDomainFSInfoPtr **info,
>                      unsigned int flags)
>  {
>      virQEMUDriverPtr driver = dom->conn->privateData;
> +    qemuAgentFsInfoPtr *agent_info = NULL;
>      virDomainObjPtr vm;
>      qemuAgentPtr agent;
>      int ret = -1;
> +    int num;

num needs to be initialized since we can jump to cleanup without setting it.

> +    size_t i;
>  
>      virCheckFlags(0, ret);
>  
> @@ -19702,13 +19777,24 @@ qemuDomainGetFSInfo(virDomainPtr dom,
>          goto endjob;
>  
>      agent = qemuDomainObjEnterAgent(vm);
> -    ret = qemuAgentGetFSInfo(agent, info, vm->def);
> +    num = qemuAgentGetFSInfo(agent, &agent_info);
>      qemuDomainObjExitAgent(vm, agent);
>  

Wouldn't it be possible to pass a copy (eg virDomainDefCopy prior to
dropping the vm lock) of the vm->def that virDomainDiskByAddress is
going to need?

In the long run all it's using the data for is a frozen view in time.


> +    if (num < 0)
> +        goto endjob;
> +
> +    ret = qemuDomainConvertFsInfo(agent_info, num, vm->def, info);
> +
>   endjob:
>      qemuDomainObjEndJob(driver, vm);
>  
>   cleanup:
> +    if (agent_info) {
> +        for (i = 0; i < num; i++)
> +            qemuAgentFsInfoFree(agent_info[i]);
> +        VIR_FREE(agent_info);
> +    }
> +
>      virDomainObjEndAPI(&vm);
>      return ret;
>  }


Depending on the above, could mean changes below, so I'll leave it for
now...

John

> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 924029a..d34cc95 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -116,7 +116,6 @@ EXTRA_DIST =		\
>  	nwfilterxml2xmlin \
>  	nwfilterxml2xmlout \
>  	oomtrace.pl \
> -	qemuagentdata \
>  	qemuargv2xmldata \
>  	qemucapabilitiesdata \
>  	qemucaps2xmldata \
> diff --git a/tests/qemuagentdata/qemuagent-fsinfo.xml b/tests/qemuagentdata/qemuagent-fsinfo.xml
> deleted file mode 100644
> index 9638feb..0000000
> --- a/tests/qemuagentdata/qemuagent-fsinfo.xml
> +++ /dev/null
> @@ -1,39 +0,0 @@
> -<domain type='qemu'>
> -  <name>QEMUGuest1</name>
> -  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> -  <memory unit='KiB'>219136</memory>
> -  <currentMemory unit='KiB'>219136</currentMemory>
> -  <vcpu placement='static'>1</vcpu>
> -  <os>
> -    <type arch='i686' machine='pc'>hvm</type>
> -    <boot dev='hd'/>
> -  </os>
> -  <clock offset='utc'/>
> -  <on_poweroff>destroy</on_poweroff>
> -  <on_reboot>restart</on_reboot>
> -  <on_crash>destroy</on_crash>
> -  <devices>
> -    <emulator>/usr/bin/qemu</emulator>
> -    <disk type='file' device='disk'>
> -      <source file='/tmp/idedisk.img'/>
> -      <target dev='hdc' bus='ide'/>
> -      <address type='drive' controller='0' bus='1' target='0' unit='0'/>
> -    </disk>
> -    <disk type='file' device='disk'>
> -      <driver name='qemu' type='qcow2'/>
> -      <source file='/tmp/virtio-blk1.qcow2'/>
> -      <target dev='vda' bus='virtio'/>
> -      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>
> -    </disk>
> -    <disk type='file' device='disk'>
> -      <driver name='qemu' type='qcow2'/>
> -      <source file='/tmp/virtio-blk2.qcow2'/>
> -      <target dev='vdb' bus='virtio'/>
> -      <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/>
> -    </disk>
> -    <controller type='ide' index='0'>
> -      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> -    </controller>
> -    <memballoon model='virtio'/>
> -  </devices>
> -</domain>
> diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
> index 5dfa58e..97f340c 100644
> --- a/tests/qemuagenttest.c
> +++ b/tests/qemuagenttest.c
> @@ -169,22 +169,16 @@ testQemuAgentGetFSInfo(const void *data)
>      virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
>      virCapsPtr caps = testQemuCapsInit();
>      qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt);
> -    char *domain_filename = NULL;
> -    virDomainDefPtr def = NULL;
> -    virDomainFSInfoPtr *info = NULL;
> +    qemuAgentFsInfoPtr *info = NULL;
>      int ret = -1, ninfo = 0, i;
>  
> +    qemuAgentFsDiskAlias alias_sda1 = { { 0, 0, 1, 1, 0 }, 1, 0, 0 };
> +    qemuAgentFsDiskAlias alias_vda = { { 0, 0, 6, 0, 0 }, 0, 0, 0 };
> +    qemuAgentFsDiskAlias alias_vdb = { { 0, 0, 7, 0, 0 }, 0, 0, 0 };
> +
>      if (!test)
>          return -1;
>  
> -    if (virAsprintf(&domain_filename, "%s/qemuagentdata/qemuagent-fsinfo.xml",
> -                    abs_srcdir) < 0)
> -        goto cleanup;
> -
> -    if (!(def = virDomainDefParseFile(domain_filename, caps, xmlopt,
> -                                      NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE)))
> -        goto cleanup;
> -
>      if (qemuMonitorTestAddAgentSyncResponse(test) < 0)
>          goto cleanup;
>  
> @@ -220,8 +214,7 @@ testQemuAgentGetFSInfo(const void *data)
>                                 "   \"disk\": [], \"type\": \"xfs\"}]}") < 0)
>          goto cleanup;
>  
> -    if ((ninfo = qemuAgentGetFSInfo(qemuMonitorTestGetAgent(test),
> -                                    &info, def)) < 0)
> +    if ((ninfo = qemuAgentGetFSInfo(qemuMonitorTestGetAgent(test), &info)) < 0)
>          goto cleanup;
>  
>      if (ninfo != 3) {
> @@ -234,11 +227,11 @@ testQemuAgentGetFSInfo(const void *data)
>          STRNEQ(info[2]->mountpoint, "/") ||
>          STRNEQ(info[2]->fstype, "ext4") ||
>          info[2]->ndevAlias != 1 ||
> -        !info[2]->devAlias || !info[2]->devAlias[0] ||
> -        STRNEQ(info[2]->devAlias[0], "hdc")) {
> +        !info[2]->devAlias ||
> +        memcmp(&info[2]->devAlias[0], &alias_sda1, sizeof(alias_sda1)) != 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> -            "unexpected filesystems information returned for sda1 (%s,%s)",
> -            info[2]->name, info[2]->devAlias ? info[2]->devAlias[0] : "null");
> +            "unexpected filesystems information returned for sda1 (%s)",
> +            info[2]->name);
>          ret = -1;
>          goto cleanup;
>      }
> @@ -246,12 +239,12 @@ testQemuAgentGetFSInfo(const void *data)
>          STRNEQ(info[1]->mountpoint, "/opt") ||
>          STRNEQ(info[1]->fstype, "vfat") ||
>          info[1]->ndevAlias != 2 ||
> -        !info[1]->devAlias || !info[1]->devAlias[0] || !info[1]->devAlias[1] ||
> -        STRNEQ(info[1]->devAlias[0], "vda") ||
> -        STRNEQ(info[1]->devAlias[1], "vdb")) {
> +        !info[1]->devAlias ||
> +        memcmp(&info[1]->devAlias[0], &alias_vda, sizeof(alias_vda)) != 0 ||
> +        memcmp(&info[1]->devAlias[1], &alias_vdb, sizeof(alias_vdb)) != 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> -            "unexpected filesystems information returned for dm-1 (%s,%s)",
> -            info[0]->name, info[0]->devAlias ? info[0]->devAlias[0] : "null");
> +            "unexpected filesystems information returned for dm-1 (%s)",
> +            info[0]->name);
>          ret = -1;
>          goto cleanup;
>      }
> @@ -260,8 +253,8 @@ testQemuAgentGetFSInfo(const void *data)
>          STRNEQ(info[0]->fstype, "xfs") ||
>          info[0]->ndevAlias != 0 || info[0]->devAlias) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> -            "unexpected filesystems information returned for sdb1 (%s,%s)",
> -            info[0]->name, info[0]->devAlias ? info[0]->devAlias[0] : "null");
> +            "unexpected filesystems information returned for sdb1 (%s)",
> +            info[0]->name);
>          ret = -1;
>          goto cleanup;
>      }
> @@ -280,7 +273,7 @@ testQemuAgentGetFSInfo(const void *data)
>                                 "}") < 0)
>          goto cleanup;
>  
> -    if (qemuAgentGetFSInfo(qemuMonitorTestGetAgent(test), &info, def) != -1) {
> +    if (qemuAgentGetFSInfo(qemuMonitorTestGetAgent(test), &info) != -1) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         "agent get-fsinfo command should have failed");
>          goto cleanup;
> @@ -290,11 +283,9 @@ testQemuAgentGetFSInfo(const void *data)
>  
>   cleanup:
>      for (i = 0; i < ninfo; i++)
> -        virDomainFSInfoFree(info[i]);
> +        qemuAgentFsInfoFree(info[i]);
>      VIR_FREE(info);
> -    VIR_FREE(domain_filename);
>      virObjectUnref(caps);
> -    virDomainDefFree(def);
>      qemuMonitorTestFree(test);
>      return ret;
>  }
> 

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux