[libvirt] [PATCH] qemu: Copy console definition from serial

[Eric Blake]

On 11/16/2011 06:14 AM, Michal Privoznik wrote:
> Now, when we support multiple consoles per domain,
> the vm->def->console[0] can still remain an alias
> for vm->def->serial[0]; However, we need to copy
> it's source definition as well otherwise we'll regress
> on virDomainOpenConsole.
> ---
>  src/conf/domain_conf.c   |   72 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h   |    2 +
>  src/libvirt_private.syms |    1 +
>  src/qemu/qemu_process.c  |   19 +++++++++--
>  4 files changed, 90 insertions(+), 4 deletions(-)

I do agree that we need the deep copy - when we first connect to qemu,
we do a lookup to see what ptys got allocated, and modify vm->def
accordingly.  If vm->def->serials[0] is modified by the pty lookup, and
vm->def->consoles[0] is an alias to serials[0], then the allocations
that were done to modify the serials lookup have to be cloned to
vm->def->consoles[0].

> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6b78d97..9b2eb86 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -956,6 +956,78 @@ virDomainChrSourceDefClear(virDomainChrSourceDefPtr def)
>      }
>  }
>  
> +int
> +virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
> +                          virDomainChrSourceDefPtr src)
> +{
> +    if (!dest || !src)
> +        return -1;
> +
> +    switch (src->type) {
> +    case VIR_DOMAIN_CHR_TYPE_PTY:
> +    case VIR_DOMAIN_CHR_TYPE_DEV:
> +    case VIR_DOMAIN_CHR_TYPE_FILE:
> +    case VIR_DOMAIN_CHR_TYPE_PIPE:
> +        if (src->data.file.path &&
> +            !(dest->data.file.path = strdup(src->data.file.path))) {
> +            virReportOOMError();
> +            return -1;
> +        }
> +        break;
> +
> +    case VIR_DOMAIN_CHR_TYPE_UDP:
> +        if (src->data.udp.bindHost &&
> +            !(dest->data.udp.bindHost = strdup(src->data.udp.bindHost))) {
> +            virReportOOMError();
> +            return -1;
> +        }

Hmm.  If this strdup() succeeds,

> +
> +        if (src->data.udp.bindService &&
> +            !(dest->data.udp.bindService = strdup(src->data.udp.bindService))) {
> +            virReportOOMError();
> +            return -1;
> +        }

but this fails, then the burden is on the caller to free up dest on
error in order to avoid a memory leak.  But...

> +++ b/src/qemu/qemu_process.c
> @@ -1163,11 +1163,22 @@ qemuProcessFindCharDevicePTYs(virDomainObjPtr vm,
>  
>      for (i = 0 ; i < vm->def->nconsoles ; i++) {
>          virDomainChrDefPtr chr = vm->def->consoles[i];
> -        if (chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY &&
> -            chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
> -            if ((ret = qemuProcessExtractTTYPath(output, &offset,
> -                                                 &chr->source.data.file.path)) != 0)
> +        /* For historical reasons, console[0] can be just an alias
> +         * for serial[0]; That's why we need to update it as well */
> +        if (i == 0 && vm->def->nserials &&
> +            chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
> +            chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) {
> +            if ((ret = virDomainChrSourceDefCopy(&chr->source,
> +                                                 &((vm->def->serials[0])->source))) != 0)
>                  return ret;

at first glance, the lone caller doesn't directly free dest.  On the
other hand, the lone caller passed in dest of vm->def->consoles[0],
which eventually gets cleaned up when vm->def is freed, so you've
escaped the original leak that I was worried might exist.  However, if
qemuProcessFindCharDevicePTYs ever gets called multiple times on the
same vm->def, then the second call overwrites the pointer from the first
call, and you have a memory leak in that aspect; and since it is
cross-file between allocation and error path, it's hard to chase down.

ACK if you squash this in to avoid the memory leak (you may want to wait
for Dave to confirm that squashing this in still works in his testing):

diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index 9b2eb86..e6f97b8 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -956,6 +956,8 @@ virDomainChrSourceDefClear(virDomainChrSourceDefPtr def)
     }
 }

+/* Deep copies the contents of src into dest.  Return -1 and report
+ * error on failure.  */
 int
 virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
                           virDomainChrSourceDefPtr src)
@@ -963,6 +965,8 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
     if (!dest || !src)
         return -1;

+    virDomainChrSourceDefClear(dest);
+
     switch (src->type) {
     case VIR_DOMAIN_CHR_TYPE_PTY:
     case VIR_DOMAIN_CHR_TYPE_DEV:


-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111116/676b4079/attachment-0001.sig>