Re: [libvirt] [PATCH 7/8] qemu: Support newer ivshmem device variants

2016-10-14 Thread Martin Kletzander

On Fri, Oct 14, 2016 at 10:19:58AM -0400, John Ferlan wrote:

[...]


+char *
+qemuBuildShmemDevStr(virDomainDefPtr def,
+ virDomainShmemDefPtr shmem,
+ virQEMUCapsPtr qemuCaps)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+virBufferAsprintf(, "%s",
virDomainShmemModelTypeToString(shmem->model));
+virBufferAsprintf(, ",id=%s", shmem->info.alias);
+
+if (shmem->server.enabled)
+virBufferAsprintf(, ",chardev=char%s", shmem->info.alias);
+else
+virBufferAsprintf(, ",memdev=shmmem-%s",
shmem->info.alias);
+
+if (shmem->msi.vectors)
+virBufferAsprintf(, ",vectors=%u", shmem->msi.vectors);
+if (shmem->msi.ioeventfd) {
+virBufferAsprintf(, ",ioeventfd=%s",
+
virTristateSwitchTypeToString(shmem->msi.ioeventfd));
+}
+
+if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps)
< 0) {
+virBufferFreeAndReset();
+return NULL;
+}
+
+if (virBufferCheckError() < 0)


Still would need to FreeAndReset - I'd be OK if it were an || to the
previous if, although I know that causes agita for others.



Well, not really.  The content gets free()'d whenever the buf->error is
set, so it is already cleared if there was an error.



That's not how I read/see other callers of virBufferCheckError and I see
no free in virBufferCheckErrorInternal



It's free()'d when it is *set*, see virBufferSetError (the only place in
virbuffer.c that sets the ->error member).



John

[...]


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 7/8] qemu: Support newer ivshmem device variants

2016-10-14 Thread John Ferlan
[...]

>>> +char *
>>> +qemuBuildShmemDevStr(virDomainDefPtr def,
>>> + virDomainShmemDefPtr shmem,
>>> + virQEMUCapsPtr qemuCaps)
>>> +{
>>> +virBuffer buf = VIR_BUFFER_INITIALIZER;
>>> +
>>> +virBufferAsprintf(, "%s",
>>> virDomainShmemModelTypeToString(shmem->model));
>>> +virBufferAsprintf(, ",id=%s", shmem->info.alias);
>>> +
>>> +if (shmem->server.enabled)
>>> +virBufferAsprintf(, ",chardev=char%s", shmem->info.alias);
>>> +else
>>> +virBufferAsprintf(, ",memdev=shmmem-%s",
>>> shmem->info.alias);
>>> +
>>> +if (shmem->msi.vectors)
>>> +virBufferAsprintf(, ",vectors=%u", shmem->msi.vectors);
>>> +if (shmem->msi.ioeventfd) {
>>> +virBufferAsprintf(, ",ioeventfd=%s",
>>> + 
>>> virTristateSwitchTypeToString(shmem->msi.ioeventfd));
>>> +}
>>> +
>>> +if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps)
>>> < 0) {
>>> +virBufferFreeAndReset();
>>> +return NULL;
>>> +}
>>> +
>>> +if (virBufferCheckError() < 0)
>>
>> Still would need to FreeAndReset - I'd be OK if it were an || to the
>> previous if, although I know that causes agita for others.
>>
> 
> Well, not really.  The content gets free()'d whenever the buf->error is
> set, so it is already cleared if there was an error.
> 

That's not how I read/see other callers of virBufferCheckError and I see
no free in virBufferCheckErrorInternal


John

[...]

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


Re: [libvirt] [PATCH 7/8] qemu: Support newer ivshmem device variants

2016-10-14 Thread Martin Kletzander

On Fri, Oct 07, 2016 at 12:20:19PM -0400, John Ferlan wrote:



On 09/27/2016 08:24 AM, Martin Kletzander wrote:

QEMU added support for ivshmem-plain and ivshmem-doorbell.  Those are
reworked varians of legacy ivshmem that are compatible from the guest
POV, but not from host's POV and have sane specification and handling.



It seems less "added support for" and more "forced libvirt to choose"
between one or the other as of qemu v2.6.



Well, not really, it's not removed from newer QEMU, it's just deprecated.


Details about the newer device type can be found in qemu's commit
5400c02b90bb:

  http://git.qemu.org/?p=qemu.git;a=commit;h=5400c02b90bb

Signed-off-by: Martin Kletzander 
---
 src/qemu/qemu_command.c| 88 +-
 src/qemu/qemu_command.h| 10 +++
 .../qemuxml2argv-shmem-plain-doorbell.args | 43 +++
 .../qemuxml2argv-shmem-plain-doorbell.xml  | 63 
 tests/qemuxml2argvtest.c   |  3 +
 5 files changed, 204 insertions(+), 3 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index aa8cff80e8b1..29f7130ef911 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8456,6 +8456,39 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def,
 return NULL;
 }

+char *
+qemuBuildShmemDevStr(virDomainDefPtr def,
+ virDomainShmemDefPtr shmem,
+ virQEMUCapsPtr qemuCaps)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+virBufferAsprintf(, "%s", 
virDomainShmemModelTypeToString(shmem->model));
+virBufferAsprintf(, ",id=%s", shmem->info.alias);
+
+if (shmem->server.enabled)
+virBufferAsprintf(, ",chardev=char%s", shmem->info.alias);
+else
+virBufferAsprintf(, ",memdev=shmmem-%s", shmem->info.alias);
+
+if (shmem->msi.vectors)
+virBufferAsprintf(, ",vectors=%u", shmem->msi.vectors);
+if (shmem->msi.ioeventfd) {
+virBufferAsprintf(, ",ioeventfd=%s",
+  virTristateSwitchTypeToString(shmem->msi.ioeventfd));
+}
+
+if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps) < 0) {
+virBufferFreeAndReset();
+return NULL;
+}
+
+if (virBufferCheckError() < 0)


Still would need to FreeAndReset - I'd be OK if it were an || to the
previous if, although I know that causes agita for others.



Well, not really.  The content gets free()'d whenever the buf->error is
set, so it is already cleared if there was an error.


I suppose you could to the error: label path/option too.


+return NULL;
+
+return virBufferContentAndReset();
+}
+
 static char *
 qemuBuildShmemBackendChrStr(virLogManagerPtr logManager,
 virCommandPtr cmd,
@@ -8476,6 +8509,50 @@ qemuBuildShmemBackendChrStr(virLogManagerPtr logManager,
 return devstr;
 }

+
+virJSONValuePtr
+qemuBuildShmemBackendMemProps(virDomainShmemDefPtr shmem)
+{
+char *mem_path = NULL;
+virJSONValuePtr ret = NULL;
+
+if (virAsprintf(_path, "/dev/shm/%s", shmem->name) < 0)
+return NULL;
+
+virJSONValueObjectCreate(,
+ "s:mem-path", mem_path,
+ "U:size", shmem->size,
+ NULL);


Hmm... perhaps not so much of an issue as previously thought... This
will only be called for the -plain anyway and well shmem->size had
better not be zero (since you're using "U:")..

So still leaves me wondering if we should fail if a size is provided for
-doorbell



OK, I added that.


+
+VIR_FREE(mem_path);
+return ret;
+}
+
+
+static char *
+qemuBuildShmemBackendMemStr(virDomainShmemDefPtr shmem)
+{
+char *ret = NULL;
+char *alias = NULL;
+virJSONValuePtr props = qemuBuildShmemBackendMemProps(shmem);
+
+if (!props)
+return NULL;
+
+if (virAsprintf(, "shmmem-%s", shmem->info.alias) < 0)
+goto cleanup;
+
+ret = virQEMUBuildObjectCommandlineFromJSON("memory-backend-file",
+alias,
+props);
+ cleanup:
+VIR_FREE(alias);
+virJSONValueFree(props);
+
+return ret;
+}
+
+
 static int
 qemuBuildShmemCommandLine(virLogManagerPtr logManager,
   virCommandPtr cmd,
@@ -8518,10 +8595,15 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
 break;



Should there be caps checks for :

QEMU_CAPS_DEVICE_IVSHMEM_PLAIN
QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL



Sure, I should've added that, good point.


You added the caps in the xml2argvtest, so I'd expect them...  Obviously
they wouldn't work on qemu 2.5 or earlier.

As long as the memory leak is handled and there's an answer for the caps
checks, this 

Re: [libvirt] [PATCH 7/8] qemu: Support newer ivshmem device variants

2016-10-07 Thread John Ferlan


On 09/27/2016 08:24 AM, Martin Kletzander wrote:
> QEMU added support for ivshmem-plain and ivshmem-doorbell.  Those are
> reworked varians of legacy ivshmem that are compatible from the guest
> POV, but not from host's POV and have sane specification and handling.
> 

It seems less "added support for" and more "forced libvirt to choose"
between one or the other as of qemu v2.6.

> Details about the newer device type can be found in qemu's commit
> 5400c02b90bb:
> 
>   http://git.qemu.org/?p=qemu.git;a=commit;h=5400c02b90bb
> 
> Signed-off-by: Martin Kletzander 
> ---
>  src/qemu/qemu_command.c| 88 
> +-
>  src/qemu/qemu_command.h| 10 +++
>  .../qemuxml2argv-shmem-plain-doorbell.args | 43 +++
>  .../qemuxml2argv-shmem-plain-doorbell.xml  | 63 
>  tests/qemuxml2argvtest.c   |  3 +
>  5 files changed, 204 insertions(+), 3 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.xml
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index aa8cff80e8b1..29f7130ef911 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8456,6 +8456,39 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def,
>  return NULL;
>  }
> 
> +char *
> +qemuBuildShmemDevStr(virDomainDefPtr def,
> + virDomainShmemDefPtr shmem,
> + virQEMUCapsPtr qemuCaps)
> +{
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +virBufferAsprintf(, "%s", 
> virDomainShmemModelTypeToString(shmem->model));
> +virBufferAsprintf(, ",id=%s", shmem->info.alias);
> +
> +if (shmem->server.enabled)
> +virBufferAsprintf(, ",chardev=char%s", shmem->info.alias);
> +else
> +virBufferAsprintf(, ",memdev=shmmem-%s", shmem->info.alias);
> +
> +if (shmem->msi.vectors)
> +virBufferAsprintf(, ",vectors=%u", shmem->msi.vectors);
> +if (shmem->msi.ioeventfd) {
> +virBufferAsprintf(, ",ioeventfd=%s",
> +  
> virTristateSwitchTypeToString(shmem->msi.ioeventfd));
> +}
> +
> +if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps) < 0) {
> +virBufferFreeAndReset();
> +return NULL;
> +}
> +
> +if (virBufferCheckError() < 0)

Still would need to FreeAndReset - I'd be OK if it were an || to the
previous if, although I know that causes agita for others.

I suppose you could to the error: label path/option too.

> +return NULL;
> +
> +return virBufferContentAndReset();
> +}
> +
>  static char *
>  qemuBuildShmemBackendChrStr(virLogManagerPtr logManager,
>  virCommandPtr cmd,
> @@ -8476,6 +8509,50 @@ qemuBuildShmemBackendChrStr(virLogManagerPtr 
> logManager,
>  return devstr;
>  }
> 
> +
> +virJSONValuePtr
> +qemuBuildShmemBackendMemProps(virDomainShmemDefPtr shmem)
> +{
> +char *mem_path = NULL;
> +virJSONValuePtr ret = NULL;
> +
> +if (virAsprintf(_path, "/dev/shm/%s", shmem->name) < 0)
> +return NULL;
> +
> +virJSONValueObjectCreate(,
> + "s:mem-path", mem_path,
> + "U:size", shmem->size,
> + NULL);

Hmm... perhaps not so much of an issue as previously thought... This
will only be called for the -plain anyway and well shmem->size had
better not be zero (since you're using "U:")..

So still leaves me wondering if we should fail if a size is provided for
-doorbell

> +
> +VIR_FREE(mem_path);
> +return ret;
> +}
> +
> +
> +static char *
> +qemuBuildShmemBackendMemStr(virDomainShmemDefPtr shmem)
> +{
> +char *ret = NULL;
> +char *alias = NULL;
> +virJSONValuePtr props = qemuBuildShmemBackendMemProps(shmem);
> +
> +if (!props)
> +return NULL;
> +
> +if (virAsprintf(, "shmmem-%s", shmem->info.alias) < 0)
> +goto cleanup;
> +
> +ret = virQEMUBuildObjectCommandlineFromJSON("memory-backend-file",
> +alias,
> +props);
> + cleanup:
> +VIR_FREE(alias);
> +virJSONValueFree(props);
> +
> +return ret;
> +}
> +
> +
>  static int
>  qemuBuildShmemCommandLine(virLogManagerPtr logManager,
>virCommandPtr cmd,
> @@ -8518,10 +8595,15 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
>  break;
> 

Should there be caps checks for :

QEMU_CAPS_DEVICE_IVSHMEM_PLAIN
QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL

You added the caps in the xml2argvtest, so I'd expect them...  Obviously
they wouldn't work on qemu 2.5 or earlier.

As long as the memory leak is handled and there's an answer for the caps
checks, this is ACK-able from my POV.

John
>  case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN:
> +