Re: [libvirt] [PATCH 8/8] qemu: monitor: Separate probing for active block commit

[John Ferlan]

On 08/15/2018 07:52 AM, Peter Krempa wrote:
> Extract the code used to probe for the functionality so that it does not
> litter the code used for actual work.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_monitor.c  |  2 +-
>  src/qemu/qemu_monitor_json.c | 58 
> ++--
>  src/qemu/qemu_monitor_json.h |  3 +++
>  3 files changed, 39 insertions(+), 24 deletions(-)
> 

This one somehow feels related to the other series I reviewed where the
*top was or wasn't present to determine whether capability was there,
but I see that it'd still be useful - just the irony of it all.

Reviewed-by: John Ferlan 

John


> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 9bc7aa9ed1..a60e78d967 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c

NB: My gitk view shows a few lines above here the call to
qemuMonitorJSONBlockCommit has a second row of args that are not aligned
properly...  Trivialities.


[...]

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

Re: [libvirt] [PATCH 7/8] qemu: monitor: Rename 'device' argument for block job control APIs

[John Ferlan]

On 08/15/2018 07:52 AM, Peter Krempa wrote:
> Starting from qemu 2.7 the 'device' argument is in fact a name of the
> job itself. Change our APIs accordingly and adjust the error message.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_monitor.c  | 18 +-
>  src/qemu/qemu_monitor.h  |  6 +++---
>  src/qemu/qemu_monitor_json.c | 22 +++---
>  src/qemu/qemu_monitor_json.h |  6 +++---
>  4 files changed, 26 insertions(+), 26 deletions(-)
> 

Really, bizarre... The 'git am' of this series changed the
qemuMonitorJSONBlockJobError in qemuMonitorJSONBlockStream to pass
@jobname instead of @device, *BUT* that is not changed here.  Very,
very, odd git am behaviour.  Too many closely named functions too close
together is my guess for the pattern match.

What's in the patch is fine, just was confusing when my build failed!

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 6/8] qemu: monitor: Use qemuMonitorJSONBlockJobError in qemuMonitorJSONDrivePivot

[John Ferlan]

On 08/15/2018 07:52 AM, Peter Krempa wrote:
> The API deals with a block job so use the common error reporting
> function for it.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_monitor_json.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Very strange - this got applied to qemuMonitorJSONBlockStream in my "git
am" of the patches... Gets stranger on the next patch too, but at least
this explains what happened there...

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 5/8] qemu: monitor: Move qemuMonitorJSONDrivePivot together with block-job APIs

[John Ferlan]

On 08/15/2018 07:52 AM, Peter Krempa wrote:
> Move all relevant APIs dealing with existing jobs together.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_monitor_json.c | 55 
> ++--
>  1 file changed, 28 insertions(+), 27 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 4/8] qemu: monitor: Use qemuMonitorJSONCheckError in qemuMonitorJSONBlockStream

[John Ferlan]

On 08/15/2018 07:52 AM, Peter Krempa wrote:
> The API does not report any special job-related error so the generic
> error function should be used.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_monitor_json.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 3/8] qemu: monitor: Remove temporary variables

[John Ferlan]

On 08/15/2018 07:52 AM, Peter Krempa wrote:
> Now that the job name is used in single place in the respective
> functions remove the temporary strings.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_monitor_json.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 2/8] qemu: monitor: Use qemuMonitorJSONCheckError in qemuMonitorJSONBlockJobError

[John Ferlan]

On 08/15/2018 07:52 AM, Peter Krempa wrote:
> Report the generic errors using the existing function so that we don't
> reimplement the same functionality multiple times.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_monitor_json.c | 27 +--
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 1/8] qemu: monitor: Remove error classes not conforming to QAPI schema

[John Ferlan]

On 08/15/2018 07:52 AM, Peter Krempa wrote:
> Both were removed prior to qemu v1.2.0-rc0 when switching to the new
> error format where almost all error types were converted to GenericError.
> 
> Relevant qemu commits are  and 
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_monitor_json.c | 6 --
>  1 file changed, 6 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 10/10] FIXUP: regenerate ordering in replies files

[John Ferlan]

On 08/15/2018 05:18 AM, Peter Krempa wrote:
> Split up for simpler reviews.
> ---
>  .../qemucapabilitiesdata/caps_1.5.3.x86_64.replies | 152 
>  .../qemucapabilitiesdata/caps_1.6.0.x86_64.replies | 152 
>  .../qemucapabilitiesdata/caps_1.7.0.x86_64.replies | 152 
>  .../qemucapabilitiesdata/caps_2.1.1.x86_64.replies | 152 
>  .../caps_2.10.0.aarch64.replies| 148 
>  .../qemucapabilitiesdata/caps_2.10.0.ppc64.replies | 148 
>  .../qemucapabilitiesdata/caps_2.10.0.s390x.replies | 148 
>  .../caps_2.10.0.x86_64.replies | 180 +--
>  .../qemucapabilitiesdata/caps_2.11.0.s390x.replies | 152 
>  .../caps_2.11.0.x86_64.replies | 180 +--
>  .../caps_2.12.0.aarch64.replies| 156 -
>  .../qemucapabilitiesdata/caps_2.12.0.ppc64.replies | 156 -
>  .../qemucapabilitiesdata/caps_2.12.0.s390x.replies | 160 -
>  .../caps_2.12.0.x86_64.replies | 192 
> ++---
>  .../qemucapabilitiesdata/caps_2.4.0.x86_64.replies | 164 +-
>  .../qemucapabilitiesdata/caps_2.5.0.x86_64.replies | 164 +-
>  .../caps_2.6.0.aarch64.replies | 148 
>  .../qemucapabilitiesdata/caps_2.6.0.ppc64.replies  | 148 
>  .../qemucapabilitiesdata/caps_2.6.0.x86_64.replies | 164 +-
>  .../qemucapabilitiesdata/caps_2.7.0.s390x.replies  | 140 +++
>  .../qemucapabilitiesdata/caps_2.7.0.x86_64.replies | 164 +-
>  .../qemucapabilitiesdata/caps_2.8.0.s390x.replies  | 148 
>  .../qemucapabilitiesdata/caps_2.8.0.x86_64.replies | 164 +-
>  .../qemucapabilitiesdata/caps_2.9.0.ppc64.replies  | 148 
>  .../qemucapabilitiesdata/caps_2.9.0.s390x.replies  | 148 
>  .../qemucapabilitiesdata/caps_2.9.0.x86_64.replies | 180 +--
>  .../qemucapabilitiesdata/caps_3.0.0.ppc64.replies  | 156 -
>  .../qemucapabilitiesdata/caps_3.0.0.x86_64.replies | 192 
> ++---
>  28 files changed, 2228 insertions(+), 2228 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 09/10] qemu: capabilities: Always assume QEMU_CAPS_ADD_FD

[John Ferlan]

On 08/15/2018 05:18 AM, Peter Krempa wrote:
> The capability was usable since qemu 1.3 so we can remove all the
> detection code.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_capabilities.c   | 17 
>  src/qemu/qemu_capabilities.h   |  2 +-
>  src/qemu/qemu_command.c| 32 
> ++
>  .../qemucapabilitiesdata/caps_1.5.3.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_1.6.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_1.7.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.1.1.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml   |  3 +-
>  .../caps_2.10.0.aarch64.replies| 17 
>  tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml |  3 +-
>  .../qemucapabilitiesdata/caps_2.10.0.ppc64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.10.0.s390x.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   |  3 +-
>  .../caps_2.10.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  |  3 +-
>  .../qemucapabilitiesdata/caps_2.11.0.s390x.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   |  3 +-
>  .../caps_2.11.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml  |  3 +-
>  .../caps_2.12.0.aarch64.replies| 17 
>  tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml |  3 +-
>  .../qemucapabilitiesdata/caps_2.12.0.ppc64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.12.0.s390x.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   |  3 +-
>  .../caps_2.12.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  |  3 +-
>  .../qemucapabilitiesdata/caps_2.4.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.5.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   |  3 +-
>  .../caps_2.6.0.aarch64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml  |  3 +-
>  .../qemucapabilitiesdata/caps_2.6.0.ppc64.replies  | 17 
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml|  3 +-
>  .../qemucapabilitiesdata/caps_2.6.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.7.0.s390x.replies  | 17 
>  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml|  3 +-
>  .../qemucapabilitiesdata/caps_2.7.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.8.0.s390x.replies  | 17 
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml|  3 +-
>  .../qemucapabilitiesdata/caps_2.8.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.9.0.ppc64.replies  | 17 
>  tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml|  3 +-
>  .../qemucapabilitiesdata/caps_2.9.0.s390x.replies  | 17 
>  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml|  3 +-
>  .../qemucapabilitiesdata/caps_2.9.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_3.0.0.ppc64.replies  | 17 
>  tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml|  3 +-
>  .../qemucapabilitiesdata/caps_3.0.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml   |  3 +-
>  tests/qemuxml2argvtest.c   |  2 --
>  60 files changed, 43 insertions(+), 570 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 08/10] tests: qemuxml2argv: modernize TPM passthrough tests

[John Ferlan]

On 08/15/2018 05:18 AM, Peter Krempa wrote:
> All supported qemus support FD passing so modify the tests to test the
> proper code path.
> 
> Signed-off-by: Peter Krempa 
> ---
>  tests/qemuxml2argvdata/tpm-passthrough-crb.args |  5 +++--
>  tests/qemuxml2argvdata/tpm-passthrough.args |  5 +++--
>  tests/qemuxml2argvmock.c| 18 +-
>  tests/qemuxml2argvtest.c|  2 ++
>  4 files changed, 25 insertions(+), 5 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 07/10] qemu: command: Extract opening of TPM backend FDs for mocking purposes

[John Ferlan]

On 08/15/2018 05:18 AM, Peter Krempa wrote:
> Allow mocking of the file descriptor numbers used for the TPM
> passthrough mode by extracting the relevant code into an exported
> function.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_command.c | 41 +++--
>  src/qemu/qemu_command.h |  7 +++
>  2 files changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ddb90895e0..fa66b8affb 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9586,6 +9586,31 @@ qemuBuildTPMDevStr(const virDomainDef *def,
>  }
> 

[...]

>  static char *
>  qemuBuildTPMBackendStr(const virDomainDef *def,
> virCommandPtr cmd,
> @@ -9624,12 +9649,8 @@ qemuBuildTPMBackendStr(const virDomainDef *def,
>  goto error;
> 
>  if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ADD_FD)) {
> -*tpmfd = open(tpmdev, O_RDWR);
> -if (*tpmfd < 0) {
> -virReportSystemError(errno, _("Could not open TPM device 
> %s"),
> - tpmdev);
> +if (qemuBuildTPMOpenBackendFDs(tpmdev, cancel_path, tpmfd, 
> cancelfd) < 0)
>  goto error;
> -}
> 
>  virCommandPassFD(cmd, *tpmfd,
>   VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> @@ -9637,17 +9658,9 @@ qemuBuildTPMBackendStr(const virDomainDef *def,
>  if (devset == NULL)

@cancelfd would be leaked...

>  goto error;
> 

[...]

> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 283bf3120d..7f84f904ce 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -216,4 +216,11 @@ qemuBuildVsockDevStr(virDomainDefPtr def,
>  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
>  ATTRIBUTE_NONNULL(4);
> 
> +/* this function is exported so that tests can mock the FDs */
> +int
> +qemuBuildTPMOpenBackendFDs(const char *tpmdev,
> +   const char *cancel_path,
> +   int *tpmfd,
> +   int *cancelfd);

should this follow others in here w/r/t ATTRIBUTE_NONNULL for args?

> +
>  #endif /* __QEMU_COMMAND_H__*/
> 

With at lest the leak resolved,

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 06/10] tests: qemuxml2argvmock: Allow 'safe' file descriptors in mocked virCommandPassFD

[John Ferlan]

On 08/15/2018 05:18 AM, Peter Krempa wrote:
> Allow FDs which are marked as safe for FD passing.
> 
> Signed-off-by: Peter Krempa 
> ---
>  tests/qemuxml2argvmock.c | 22 ++
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Markus Armbruster]

Eduardo Habkost  writes:

> On Thu, Aug 23, 2018 at 05:26:47PM +0100, Daniel P. Berrangé wrote:
> [...]
>> There are countless mistakes in both QEMU & libvirt, but only some of
>> them are worth the cost of changing.

Agreed.

>>  I'm not seeing a compelling reason
>> why this change is worthwhile. The impact of the design mistake is narrow
>> and only raised because of downstream desire to change even legacy OS
>> to use Q35 when there's no benefit to those OS of such a change.
>
> I think you underestimate the impact of the design mistake.

And overstate the "this is just for a downstream need".

> Maintaining and working around badly designed interfaces have
> costs.
>
> The virtio device model was already an obstacle when designing
> new bus/device introspection interfaces.  It will be an obstacle
> for adding mechanisms to tell applications that legacy virtio
> devices can't be plugged on PCI Express slots.

Thus, there's a genuine upstream motivation to clean up this mess.
Whether it's worthwhile is of course a fair question.

The argument for "it is worthwhile" I like to see in general is patches.

> Anyway, if we want to fix the design mistake it wouldn't make
> sense to do it only on the libvirt side and not on QEMU.  We can
> address that on QEMU first, and then let libvirt decide how to
> handle it.

Yes.

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

Re: [libvirt] [PATCH v2 05/10] FIXUP: regenerate ordering in replies files

[John Ferlan]

On 08/15/2018 05:18 AM, Peter Krempa wrote:
> Split up for simpler reviews.

thanks! it helped.
> ---
>  .../caps_2.10.0.aarch64.replies| 148 
>  .../qemucapabilitiesdata/caps_2.10.0.ppc64.replies | 148 
>  .../qemucapabilitiesdata/caps_2.10.0.s390x.replies | 148 
>  .../caps_2.10.0.x86_64.replies | 180 +--
>  .../qemucapabilitiesdata/caps_2.11.0.s390x.replies | 152 
>  .../caps_2.11.0.x86_64.replies | 180 +--
>  .../caps_2.12.0.aarch64.replies| 156 -
>  .../qemucapabilitiesdata/caps_2.12.0.ppc64.replies | 156 -
>  .../qemucapabilitiesdata/caps_2.12.0.s390x.replies | 160 -
>  .../caps_2.12.0.x86_64.replies | 192 
> ++---
>  .../qemucapabilitiesdata/caps_2.5.0.x86_64.replies | 164 +-
>  .../caps_2.6.0.aarch64.replies | 148 
>  .../qemucapabilitiesdata/caps_2.6.0.ppc64.replies  | 148 
>  .../qemucapabilitiesdata/caps_2.6.0.x86_64.replies | 164 +-
>  .../qemucapabilitiesdata/caps_2.7.0.s390x.replies  | 140 +++
>  .../qemucapabilitiesdata/caps_2.7.0.x86_64.replies | 164 +-
>  .../qemucapabilitiesdata/caps_2.8.0.s390x.replies  | 148 
>  .../qemucapabilitiesdata/caps_2.8.0.x86_64.replies | 164 +-
>  .../qemucapabilitiesdata/caps_2.9.0.ppc64.replies  | 148 
>  .../qemucapabilitiesdata/caps_2.9.0.s390x.replies  | 148 
>  .../qemucapabilitiesdata/caps_2.9.0.x86_64.replies | 180 +--
>  .../qemucapabilitiesdata/caps_3.0.0.ppc64.replies  | 156 -
>  .../qemucapabilitiesdata/caps_3.0.0.x86_64.replies | 192 
> ++---
>  23 files changed, 1842 insertions(+), 1842 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v2 04/10] qemu: capabilities: Detect active block commit via QMP schema probing if possible

[John Ferlan]

On 08/15/2018 05:18 AM, Peter Krempa wrote:
> For versions where we can probe that the arguments are optional we can
> perform the probing by a schema query rather than sending a separate
> command to do so.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_capabilities.c   |  8 +---
>  tests/qemucapabilitiesdata/caps_2.10.0.aarch64.replies | 16 
>  tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml |  2 +-
>  tests/qemucapabilitiesdata/caps_2.10.0.ppc64.replies   | 16 
>  tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   |  2 +-
>  tests/qemucapabilitiesdata/caps_2.10.0.s390x.replies   | 16 
>  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   |  2 +-
>  tests/qemucapabilitiesdata/caps_2.10.0.x86_64.replies  | 16 
>  tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  |  2 +-
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.replies   | 16 
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   |  2 +-
>  tests/qemucapabilitiesdata/caps_2.11.0.x86_64.replies  | 16 
>  tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml  |  2 +-
>  tests/qemucapabilitiesdata/caps_2.12.0.aarch64.replies | 16 
>  tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml |  2 +-
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.replies   | 16 
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   |  2 +-
>  tests/qemucapabilitiesdata/caps_2.12.0.s390x.replies   | 16 
>  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   |  2 +-
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies  | 16 
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  |  2 +-
>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.replies   | 16 
>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   |  2 +-
>  tests/qemucapabilitiesdata/caps_2.6.0.aarch64.replies  | 16 
>  tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml  |  2 +-
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64.replies| 16 
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml|  2 +-
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.replies   | 16 
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   |  2 +-
>  tests/qemucapabilitiesdata/caps_2.7.0.s390x.replies| 16 
>  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml|  2 +-
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.replies   | 16 
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |  2 +-
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.replies| 16 
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml|  2 +-
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.replies   | 16 
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   |  2 +-
>  tests/qemucapabilitiesdata/caps_2.9.0.ppc64.replies| 16 
>  tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml|  2 +-
>  tests/qemucapabilitiesdata/caps_2.9.0.s390x.replies| 16 
>  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml|  2 +-
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.replies   | 16 
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   |  2 +-
>  tests/qemucapabilitiesdata/caps_3.0.0.ppc64.replies| 16 
>  tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml|  2 +-
>  tests/qemucapabilitiesdata/caps_3.0.0.x86_64.replies   | 16 
>  tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml   |  2 +-
>  47 files changed, 28 insertions(+), 394 deletions(-)
> 

Until I looked at the history of qapi/block-core.json, the "*" didn't
make sense. Still, it seems "top" means required argument "top" while
"*top" means optional argument "top". Does that mean "theoretically
speaking" we could have used "*tls-creds" since that's listed as
optional for nbd-server-start?  Suffice to say screendump doesn't make
much sense either, although in light of this "*", perhaps it too could
be "*device"?  I dunno, just guessing and grousing.

Different problem for a different day, but documenting the syntax of the
entries in the virQEMUCapsQMPSchemaQueries would be nice.

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v3 0/9] RISC-V Guest Support

[Andrea Bolognani]

On Wed, 2018-08-22 at 11:15 +0200, Lubomir Rintel wrote:
[...]
>  * Dropped:
> qemu: no USB by default on RISC-V machines
> (not sure why I thought this was needed)

I won't hold up the series because of it - in fact I intend to
push it as soon as you have given me feedback for the few trivial
questions I've raised during review - but I think this decision
should be reverted.

I've noticed that the generated command line contains -usb, which
is the legacy syntax for USB support: these days guests are
supposed to use -device instead, hopefully with a virt-friendly
USB controller such as qemu-xhci. All those controllers are PCI,
though, so they won't be usable on RISC-V until the architecture
grows PCI support in QEMU...

Still, we have spent a long time moving away from -usb and I
don't think we should be introducing any more uses of it, so we
should write a follow-up patch that gets rid of that in addition
to getting patch 5/11 from v2 in.

It's not like RISC-V guests would have much use for a USB
controller anyway...

  $ qemu-system-riscv64 -device help 2>&1 | grep -i usb
  name "usb-redir", bus usb-bus
  $

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2 03/10] qemu: qapi: Allow selecting specifically optional schema entries in virQEMUQAPISchemaTraverse

[John Ferlan]

On 08/15/2018 05:18 AM, Peter Krempa wrote:
> Add a new modifier character which will select given schema entry only
> when it is optional.
> 

So when the object has a "*" modifier, then we need to ...?

And the check for the "default" key means what? Is it only ever present
when "*" is the modifier?

I bet I learn in subsequent patches, but I'm going 1 by 1.

> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_qapi.c | 18 +-
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 

Please just fill in the above details - as it's not "obvious" to this
reader, but I don't believe the code is wrong, so...

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH v3 6/9] qemu: add qemuDomainAssignVirtioMMIOAddresses()

[Lubomir Rintel]

On Thu, 2018-08-23 at 17:43 +0200, Andrea Bolognani wrote:
> On Wed, 2018-08-22 at 11:15 +0200, Lubomir Rintel wrote:
> 
> A short not explaining why you need to do this, along the lines of
> 
>   We're going to need to assign virtio-mmio addresses to non-ARM
>   guests soon, so let's create a generic wrapper that calls to
>   the architecture-specific implementation.
> 
> sure would be nice... Are you okay with me adding it to the commit
> message before pushing?

Yes, that would be fine.

> 
> Either way the code is fine, so
> 
>   Reviewed-by: Andrea Bolognani 

Thanks,
Lubo

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

Re: [libvirt] [PATCH v3 9/9] news: Add a mention of RISC-V guest support

[Andrea Bolognani]

On Wed, 2018-08-22 at 11:15 +0200, Lubomir Rintel wrote:
[...]
> +  
> +
> +  qemu: Add support for RISC-V guests
> +
> +
> +  riscv32 and riscv64 guest architectures are now supported.
> +
> +  

Bit terse, but okay :)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v3 8/9] tests: Add RISC-V architectures

[Andrea Bolognani]

On Wed, 2018-08-22 at 11:15 +0200, Lubomir Rintel wrote:
> Signed-off-by: Lubomir Rintel 
> ---
>  tests/capabilityschemadata/caps-qemu-kvm.xml  |36 +
>  .../caps_3.0.0.riscv32.replies| 14819 
>  .../caps_3.0.0.riscv32.xml|   118 +
>  .../caps_3.0.0.riscv64.replies| 14819 
>  .../caps_3.0.0.riscv64.xml|   118 +
>  tests/qemucapabilitiestest.c  | 2 +
>  tests/qemuxml2argvdata/riscv64-virt.args  |30 +
>  tests/qemuxml2argvdata/riscv64-virt.xml   |32 +
>  tests/qemuxml2argvtest.c  | 3 +
>  .../riscv64-virt.xml  |42 +
>  tests/qemuxml2xmloutdata/riscv64-virt.xml |36 +
>  tests/qemuxml2xmltest.c   | 2 +
>  tests/testutilsqemu.c |72 +
>  tests/vircapstest.c   | 6 +
>  14 files changed, 30135 insertions(+)
>  create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.riscv32.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml
>  create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.riscv64.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml
>  create mode 100644 tests/qemuxml2argvdata/riscv64-virt.args
>  create mode 100644 tests/qemuxml2argvdata/riscv64-virt.xml
>  create mode 100644 tests/qemuxml2startupxmloutdata/riscv64-virt.xml
>  create mode 100644 tests/qemuxml2xmloutdata/riscv64-virt.xml

I think I didn't explain myself clearly enough while reviewing
v2: I expected patches 07-09 and 11 to be squashed together, but
patch 10 to remain standalone. No big deal, I'll take care of
splitting it up again myself.

[...]
> +++ b/tests/qemuxml2argvdata/riscv64-virt.xml
> @@ -0,0 +1,32 @@
> +
> +  riscv64
> +  fd65fc03-8838-4c4d-9d8d-395802488790
> +  2097152
> +  2097152
> +  1
> +  
> +hvm
> +/var/lib/libvirt/images/bbl
> +console=ttyS0 ro root=/dev/vda
> +
> +  
> +  
> +  destroy
> +  restart
> +  destroy
> +  
> +/usr/bin/qemu-system-riscv64
> +
> +  
> +  
> +  
> +  
> +
> +
> +  
> +
> +
> +  
> +
> +  
> +

You didn't minimize the input file like I requested during
review :( Oh well, I'll take care of it.


Reviewed-by: Andrea Bolognani 


[...]
> +static int testQemuAddRISCV32Guest(virCapsPtr caps)
> +{
> +static const char *names[] = { "spike_v1.10",
> +   "spike_v1.9.1",
> +   "sifive_e",
> +   "virt",
> +   "sifive_u" };
> +static const int nmachines = ARRAY_CARDINALITY(names);
> +virCapsGuestMachinePtr *machines = NULL;
> +virCapsGuestPtr guest;
> +
> +machines = virCapabilitiesAllocMachines(names, nmachines);
> +if (!machines)
> +goto error;
> +
> +guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, 
> VIR_ARCH_RISCV32,
> +QEMUBinList[TEST_UTILS_QEMU_BIN_RISCV32],
> +NULL, nmachines, machines);
> +if (!guest)
> +goto error;
> +
> +if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, 
> NULL, 0, NULL))
> +goto error;
> +
> +return 0;
> +
> + error:
> +virCapabilitiesFreeMachines(machines, nmachines);
> +return -1;
> +}
> +
> +static int testQemuAddRISCV64Guest(virCapsPtr caps)
> +{
> +static const char *names[] = { "spike_v1.10",
> +   "spike_v1.9.1",
> +   "sifive_e",
> +   "virt",
> +   "sifive_u" };
> +static const int nmachines = ARRAY_CARDINALITY(names);
> +virCapsGuestMachinePtr *machines = NULL;
> +virCapsGuestPtr guest;
> +
> +machines = virCapabilitiesAllocMachines(names, nmachines);
> +if (!machines)
> +goto error;
> +
> +guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, 
> VIR_ARCH_RISCV64,
> +QEMUBinList[TEST_UTILS_QEMU_BIN_RISCV64],
> +NULL, nmachines, machines);
> +if (!guest)
> +goto error;
> +
> +if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, 
> NULL, 0, NULL))
> +goto error;
> +
> +return 0;
> +
> + error:
> +virCapabilitiesFreeMachines(machines, nmachines);
> +return -1;
> +}
> +
>  static int testQemuAddS390Guest(virCapsPtr caps)
>  {
>  static const char *s390_machines[] = { "s390-virtio",
> @@ -440,6 +506,12 @@ virCapsPtr testQemuCapsInit(void)
>  if (testQemuAddPPCGuest(caps))
>  goto cleanup;
>  
> +if (testQemuAddRISCV32Guest(caps) < 0)
> +goto cleanup;
> +
> +if (testQemuAddRISCV64Guest(caps) < 0)
> +goto cleanup;
> +
>  if 

Re: [libvirt] [PATCH v2 02/10] qemu: qapi: Split up virQEMUQAPISchemaObjectGetType

[John Ferlan]

On 08/15/2018 05:18 AM, Peter Krempa wrote:
> Split it into a function that returns the whole schema entry so that we
> can do additional checks and a helper getting the type string from the
> schema entry.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_qapi.c | 51 ---
>  1 file changed, 40 insertions(+), 11 deletions(-)
> 

With pointed out nits below handled..

Reviewed-by: John Ferlan 

John

> diff --git a/src/qemu/qemu_qapi.c b/src/qemu/qemu_qapi.c
> index fea6683336..cd28c69a96 100644
> --- a/src/qemu/qemu_qapi.c
> +++ b/src/qemu/qemu_qapi.c
> @@ -33,24 +33,23 @@ VIR_LOG_INIT("qemu.qemu_qapi");
> 
> 
>  /**
> - * virQEMUQAPISchemaObjectGetType:
> + * virQEMUQAPISchemaObjectGet:
>   * @field: name of the object containing the requested type
>   * @name: name of the requested type
>   * @namefield: name of the object property holding @name

existing, but @elem isn't described

>   *
>   * Helper that selects the type of a QMP schema object member or it's variant
> - * member. Returns the type string on success or NULL on error.
> + * member. Returns the QMP entry on success or NULL on error.
>   */
> -static const char *
> -virQEMUQAPISchemaObjectGetType(const char *field,
> -   const char *name,
> -   const char *namefield,
> -   virJSONValuePtr elem)

[...]

> +/**
> + * virQEMUQAPISchemaObjectGetType:
> + * @field: name of the object containing the requested type
> + * @name: name of the requested type
> + * @namefield: name of the object property holding @name

@elem not described

> + *
> + * Helper that selects the type of a QMP schema object member or it's variant
> + * member. Returns the type string on success or NULL on error.
> + */
> +static const char *
> +virQEMUQAPISchemaObjectGetType(const char *field,
> +   const char *name,
> +   const char *namefield,
> +   virJSONValuePtr elem)
> +{
> +virJSONValuePtr obj = virQEMUQAPISchemaObjectGet(field, name, namefield, 
> elem);
> +
> +return virQEMUQAPISchemaTypeFromObject(obj);
> +}
> +
> +
>  static virJSONValuePtr
>  virQEMUQAPISchemaTraverse(const char *baseName,
>char **query,
> 

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

Re: [libvirt] [PATCH v2 01/10] qemu: qapi: Simplify value handling in virQEMUQAPISchemaTraverse

[John Ferlan]

On 08/15/2018 05:18 AM, Peter Krempa wrote:
> Introduce a few variables so that we can easily access the modifier
> character and also don't have to do pointer aritmetics when selecting

arithmetic

> the schema entries. This will simplify adding of new modifier
> characters.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_qapi.c | 19 ---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Eduardo Habkost]

On Thu, Aug 23, 2018 at 05:26:47PM +0100, Daniel P. Berrangé wrote:
[...]
> There are countless mistakes in both QEMU & libvirt, but only some of
> them are worth the cost of changing. I'm not seeing a compelling reason
> why this change is worthwhile. The impact of the design mistake is narrow
> and only raised because of downstream desire to change even legacy OS
> to use Q35 when there's no benefit to those OS of such a change.

I think you underestimate the impact of the design mistake.
Maintaining and working around badly designed interfaces have
costs.

The virtio device model was already an obstacle when designing
new bus/device introspection interfaces.  It will be an obstacle
for adding mechanisms to tell applications that legacy virtio
devices can't be plugged on PCI Express slots.

Anyway, if we want to fix the design mistake it wouldn't make
sense to do it only on the libvirt side and not on QEMU.  We can
address that on QEMU first, and then let libvirt decide how to
handle it.

-- 
Eduardo

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

[libvirt] [PATCH] qemu: Make sure preferredMachines is not missing any entry

[Andrea Bolognani]

With the current implementation, adding a new architecture
and not updating preferredMachines accordingly will not
cause a build failure, making it very likely that subtle
bugs will be introduced in the process. Rework the code
so that such issues will be caught by the compiler.

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c | 84 ++--
 1 file changed, 43 insertions(+), 41 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 8a44d32c59..230beeab8c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2266,49 +2266,51 @@ virQEMUCapsProbeQMPDevices(virQEMUCapsPtr qemuCaps,
  * that we're not vulnerable to changes in QEMU defaults or machine
  * list ordering.
  */
-static const char *preferredMachines[VIR_ARCH_LAST] =
+static const char *preferredMachines[] =
 {
-[VIR_ARCH_ALPHA] = "clipper",
-[VIR_ARCH_ARMV6L] = NULL, /* No QEMU impl */
-[VIR_ARCH_ARMV7L] = "integratorcp",
-[VIR_ARCH_ARMV7B] = "integratorcp",
-
-[VIR_ARCH_AARCH64] = "integratorcp",
-[VIR_ARCH_CRIS] = "axis-dev88",
-[VIR_ARCH_I686] = "pc",
-[VIR_ARCH_ITANIUM] = NULL, /* doesn't exist in QEMU any more */
-[VIR_ARCH_LM32] = "lm32-evr",
-
-[VIR_ARCH_M68K] = "mcf5208evb",
-[VIR_ARCH_MICROBLAZE] = "petalogix-s3adsp1800",
-[VIR_ARCH_MICROBLAZEEL] = "petalogix-s3adsp1800",
-[VIR_ARCH_MIPS] = "malta",
-[VIR_ARCH_MIPSEL] = "malta",
-
-[VIR_ARCH_MIPS64] = "malta",
-[VIR_ARCH_MIPS64EL] = "malta",
-[VIR_ARCH_OR32] = "or1k-sim",
-[VIR_ARCH_PARISC] = NULL, /* No QEMU impl */
-[VIR_ARCH_PARISC64] = NULL, /* No QEMU impl */
-
-[VIR_ARCH_PPC] = "g3beige",
-[VIR_ARCH_PPCLE] = "g3beige",
-[VIR_ARCH_PPC64] = "pseries",
-[VIR_ARCH_PPC64LE] = "pseries",
-[VIR_ARCH_PPCEMB] = "bamboo",
-
-[VIR_ARCH_S390] = NULL, /* No QEMU impl*/
-[VIR_ARCH_S390X] = "s390-ccw-virtio",
-[VIR_ARCH_SH4] = "shix",
-[VIR_ARCH_SH4EB] = "shix",
-[VIR_ARCH_SPARC] = "SS-5",
-
-[VIR_ARCH_SPARC64] = "sun4u",
-[VIR_ARCH_UNICORE32] = "puv3",
-[VIR_ARCH_X86_64] = "pc",
-[VIR_ARCH_XTENSA] = "sim",
-[VIR_ARCH_XTENSAEB] = "sim",
+NULL, /* VIR_ARCH_NONE (not a real arch :) */
+"clipper", /* VIR_ARCH_ALPHA */
+NULL, /* VIR_ARCH_ARMV6L (no QEMU impl) */
+"integratorcp", /* VIR_ARCH_ARMV7L */
+"integratorcp", /* VIR_ARCH_ARMV7B */
+
+"integratorcp", /* VIR_ARCH_AARCH64 */
+"axis-dev88", /* VIR_ARCH_CRIS */
+"pc", /* VIR_ARCH_I686 */
+NULL, /* VIR_ARCH_ITANIUM (doesn't exist in QEMU any more) */
+"lm32-evr", /* VIR_ARCH_LM32 */
+
+"mcf5208evb", /* VIR_ARCH_M68K */
+"petalogix-s3adsp1800", /* VIR_ARCH_MICROBLAZE */
+"petalogix-s3adsp1800", /* VIR_ARCH_MICROBLAZEEL */
+"malta", /* VIR_ARCH_MIPS */
+"malta", /* VIR_ARCH_MIPSEL */
+
+"malta", /* VIR_ARCH_MIPS64 */
+"malta", /* VIR_ARCH_MIPS64EL */
+"or1k-sim", /* VIR_ARCH_OR32 */
+NULL, /* VIR_ARCH_PARISC (no QEMU impl) */
+NULL, /* VIR_ARCH_PARISC64 (no QEMU impl) */
+
+"g3beige", /* VIR_ARCH_PPC */
+"g3beige", /* VIR_ARCH_PPCLE */
+"pseries", /* VIR_ARCH_PPC64 */
+"pseries", /* VIR_ARCH_PPC64LE */
+"bamboo", /* VIR_ARCH_PPCEMB */
+
+NULL, /* VIR_ARCH_S390 (no QEMU impl) */
+"s390-ccw-virtio", /* VIR_ARCH_S390X */
+"shix", /* VIR_ARCH_SH4 */
+"shix", /* VIR_ARCH_SH4EB */
+"SS-5", /* VIR_ARCH_SPARC */
+
+"sun4u", /* VIR_ARCH_SPARC64 */
+"puv3", /* VIR_ARCH_UNICORE32 */
+"pc", /* VIR_ARCH_X86_64 */
+"sim", /* VIR_ARCH_XTENSA */
+"sim", /* VIR_ARCH_XTENSAEB */
 };
+verify(ARRAY_CARDINALITY(preferredMachines) == VIR_ARCH_LAST);
 
 
 static int
-- 
2.17.1

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

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Daniel P . Berrangé]

On Thu, Aug 23, 2018 at 06:08:55PM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
> > On Wed, Aug 22, 2018 at 01:26:01PM +0100, Daniel P. Berrangé wrote:
> >> On Wed, Aug 22, 2018 at 09:01:35AM -0300, Eduardo Habkost wrote:
> >> > On Wed, Aug 22, 2018 at 12:36:27PM +0200, Andrea Bolognani wrote:
> >> > > On Tue, 2018-08-21 at 14:21 -0400, Laine Stump wrote:
> >> > > > On 08/17/2018 06:35 AM, Andrea Bolognani wrote:
> >> > > > > If we decide we want to explicitly spell out the options instead
> >> > > > > of relying on QEMU changing behavior based on the slot type, which
> >> > > > > is probably a good idea anyway, I think we should have
> >> > > > > 
> >> > > > >   virtio-0.9 => disable-legacy=no,disable-modern=no
> >> > > > >   virtio-1.0 => disable-legacy=yes,disable-modern=no
> >> > > > > 
> >> > > > > There's basically no reason to have a device legacy-only rather
> >> > > > > than transitional, and spelling out both options instead of only
> >> > > > > one of them just seems more robust.
> >> > > > 
> >> > > > I agree with both of those, but the counter-argument is that "virtio"
> >> > > > already describes a transitional device like your proposal for
> >> > > > virtio-0.9 (at least today), and it makes the versioned models less
> >> > > > orthogonal. In the end, I could go either way...
> >> > > 
> >> > > Yeah, Dan already made that argument and convinced me that we
> >> > > should use virtio-0.9 for legacy only, virtio-1.0 for modern only
> >> > > and plain virtio for no enforced behavior / transitional.
> >> > 
> >> > I don't understand why we are optimizing the new system for the
> >> > less useful use cases:
> >> > 
> >> > I don't see a use case where virtio-0.9 (legacy-only) would be
> >> > more useful than virtio-transitional.  I don't see why anybody
> >> > would prefer a legacy-only device instead of a transitional
> >> > device.  Even if your guest has only legacy drivers, it might be
> >> > upgraded and get new drivers in the future.
> >> > 
> >> > I don't see a use case where virtio-1.0 (modern-only) would be
> >> > more useful than "virtio".  If you are running i440fx, you get a
> >> > transitional device with "virtio", and I don't see why anybody
> >> > would prefer a modern-only device.  If you are running Q35, you
> >> > already get a modern-only device with "virtio".
> >> > 
> >> > The most useful feature users need is the ability to ask for a
> >> > transitional virtio device on Q35, and this use case is
> >> > explicitly being left out of the proposal.  Why?
> >> 
> >> You can already get a transitional device on Q35, albeit with manual
> >> placement. Adding flags for magic placement for the existing devices
> >> is not something that is suitable for the XML. The ability to get
> >> legacy-only, or modern-only doesn't exist today in any way, so that
> >> would be a valid new feature.
> >
> > Transitional devices and modern-only devices are different kinds
> > of devices.  Making the guest see a different type of device
> > depending on where it's plugged is why we got into this mess,
> 
> Every time we make -device FOO result in a different device depending on
> context, device configuration or placement, it eventually joins our
> collection of Very Bad Ideas.  Different PCI device IDs are a clear
> indicator of device difference.
> 
> Instances of this class of Very Bad Ideas I've addressed myself:
> 
> * I deprecated "ivshmem" in favor of "ivshmem-plain" and
>   "ivshmem-doorbell".
> 
> * I split "ide-drive" into of "ide-hd" and "ide-cd" (deprecation wasn't
>   fashionable back then)
> 
> * I split "scsi-disk" into "scsi-hd" and "scsi-cd" (likewise)
> 
> One time pain, long term gain.

The pain in those three cases was largely non-existant and/or
hidden. Almost no one ever used ivshmem, so by extension almost
no one was impacted by need to use different names. It was also
not migratable, so there was no need to care about compatibility
with older versions. For ide-drive/scsi-disk, the change was
completely hidden inside libvirt and wasn't ABI sensitive so
didn't affect apps or migration.

> We should consider addressing virtio devices, too: deprecate the
> chameleon device models an adequate grace period.

The change discussed here would have major impact by comparison
as it would be telling every single app to change what they do,
and the change would prevent their guests being compatible with
older libvirt. I don't think the gain outweighs the costs of
making the changes across every mgmt app, even ignoring the cost
of the backcompatibility problems

> >> Honestly though, the longer this discussion goes on, the more I think
> >> the answer is just "do nothing". All this time spent on discussion,
> >> and future time spent on implementing new logic in apps, is merely
> >> to support running RHEL-6 on Q35.
> 
> I strenously disagree.  This is first and foremost about correcting a
> design mistake we made.
>
> > I'm not sure if you are saying that we (Red Hat) 

Re: [libvirt] [PATCH] storage: fix the error message when encrypted raw volume resize

[John Ferlan]

On 08/20/2018 02:27 AM, Shivaprasad G Bhat wrote:
> The vol-dumpxml shows the volume target format type as raw for
> encrypted volumes. The error message when attempting to resize
> with prealloc is confusing here.
> 
> Signed-off-by: Shivaprasad G Bhat 
> ---
>  src/storage/storage_util.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
> index 42a9b6abf0..2abedb24b0 100644
> --- a/src/storage/storage_util.c
> +++ b/src/storage/storage_util.c
> @@ -2343,7 +2343,7 @@ virStorageBackendVolResizeLocal(virStoragePoolObjPtr 
> pool,
>  } else if (vol->target.format == VIR_STORAGE_FILE_RAW && 
> vol->target.encryption) {
>  if (pre_allocate) {
>  virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -   _("preallocate is only supported for raw "
> +   _("preallocate is only supported for unencrypted 
> raw "
>   "type volume"));

Reviewed-by: John Ferlan 

NB: I adjusted the text to be "for an unencrypted raw volume" and pushed.

Tks,

John

>  return -1;
>  }
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

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

Re: [libvirt] [PATCH v2 7/7] qemu_security: Lock metadata while relabelling

[Michal Privoznik]

On 08/23/2018 05:51 PM, Daniel P. Berrangé wrote:
> On Thu, Aug 23, 2018 at 05:45:42PM +0200, Michal Privoznik wrote:
>> On 08/23/2018 05:01 PM, Daniel P. Berrangé wrote:
>>> On Thu, Aug 23, 2018 at 04:54:01PM +0200, Michal Privoznik wrote:
 On 08/20/2018 07:17 PM, Michal Prívozník wrote:
> On 08/20/2018 05:16 PM, Daniel P. Berrangé wrote:
>> On Tue, Aug 14, 2018 at 01:19:43PM +0200, Michal Privoznik wrote:
>>> Fortunately, we have qemu wrappers so it's sufficient to put
>>> lock/unlock call only there.
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  src/qemu/qemu_security.c | 107 
>>> +++
>>>  1 file changed, 107 insertions(+)
>>>




> 
> Oh right, yes, that kills the idea of using a WAIT flag for lockspaces,
> unless we want to introduce threads ingto virtlockd, but we can't do
> that because Linux has totally fubard  locking across execve() :-(

But we need WAIT. I guess we do not want to do:

lockForMetadata(const char *path) {
  int retries;

  while (retries) {
rc = try_lock(path, wait=false);

if (!rc) break;
if (errno = EAGAIN && retries--) {sleep(.1); continue;}

errorOut();
  }
}

However, if we make sure that virtlockd never forks() nor execs() we
have nothing to fear about. Or am I missing something? And to be 100%
sure we can always provide dummy impl for fork() + execve() in
src/locking/lock_daemon.c which fails everytime.

Michal

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

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Markus Armbruster]

Eduardo Habkost  writes:

> On Wed, Aug 22, 2018 at 01:26:01PM +0100, Daniel P. Berrangé wrote:
>> On Wed, Aug 22, 2018 at 09:01:35AM -0300, Eduardo Habkost wrote:
>> > On Wed, Aug 22, 2018 at 12:36:27PM +0200, Andrea Bolognani wrote:
>> > > On Tue, 2018-08-21 at 14:21 -0400, Laine Stump wrote:
>> > > > On 08/17/2018 06:35 AM, Andrea Bolognani wrote:
>> > > > > If we decide we want to explicitly spell out the options instead
>> > > > > of relying on QEMU changing behavior based on the slot type, which
>> > > > > is probably a good idea anyway, I think we should have
>> > > > > 
>> > > > >   virtio-0.9 => disable-legacy=no,disable-modern=no
>> > > > >   virtio-1.0 => disable-legacy=yes,disable-modern=no
>> > > > > 
>> > > > > There's basically no reason to have a device legacy-only rather
>> > > > > than transitional, and spelling out both options instead of only
>> > > > > one of them just seems more robust.
>> > > > 
>> > > > I agree with both of those, but the counter-argument is that "virtio"
>> > > > already describes a transitional device like your proposal for
>> > > > virtio-0.9 (at least today), and it makes the versioned models less
>> > > > orthogonal. In the end, I could go either way...
>> > > 
>> > > Yeah, Dan already made that argument and convinced me that we
>> > > should use virtio-0.9 for legacy only, virtio-1.0 for modern only
>> > > and plain virtio for no enforced behavior / transitional.
>> > 
>> > I don't understand why we are optimizing the new system for the
>> > less useful use cases:
>> > 
>> > I don't see a use case where virtio-0.9 (legacy-only) would be
>> > more useful than virtio-transitional.  I don't see why anybody
>> > would prefer a legacy-only device instead of a transitional
>> > device.  Even if your guest has only legacy drivers, it might be
>> > upgraded and get new drivers in the future.
>> > 
>> > I don't see a use case where virtio-1.0 (modern-only) would be
>> > more useful than "virtio".  If you are running i440fx, you get a
>> > transitional device with "virtio", and I don't see why anybody
>> > would prefer a modern-only device.  If you are running Q35, you
>> > already get a modern-only device with "virtio".
>> > 
>> > The most useful feature users need is the ability to ask for a
>> > transitional virtio device on Q35, and this use case is
>> > explicitly being left out of the proposal.  Why?
>> 
>> You can already get a transitional device on Q35, albeit with manual
>> placement. Adding flags for magic placement for the existing devices
>> is not something that is suitable for the XML. The ability to get
>> legacy-only, or modern-only doesn't exist today in any way, so that
>> would be a valid new feature.
>
> Transitional devices and modern-only devices are different kinds
> of devices.  Making the guest see a different type of device
> depending on where it's plugged is why we got into this mess,

Every time we make -device FOO result in a different device depending on
context, device configuration or placement, it eventually joins our
collection of Very Bad Ideas.  Different PCI device IDs are a clear
indicator of device difference.

Instances of this class of Very Bad Ideas I've addressed myself:

* I deprecated "ivshmem" in favor of "ivshmem-plain" and
  "ivshmem-doorbell".

* I split "ide-drive" into of "ide-hd" and "ide-cd" (deprecation wasn't
  fashionable back then)

* I split "scsi-disk" into "scsi-hd" and "scsi-cd" (likewise)

One time pain, long term gain.

We should consider addressing virtio devices, too: deprecate the
chameleon device models an adequate grace period.

>   why
> would we recommend applications to rely on this behavior?
>
> That's why I like your virtio-0.9/virtio-1.0 proposal.  I just
> don't see why you think virtio-transitional should be out of it.
>
>> 
>> Honestly though, the longer this discussion goes on, the more I think
>> the answer is just "do nothing". All this time spent on discussion,
>> and future time spent on implementing new logic in apps, is merely
>> to support running RHEL-6 on Q35.

I strenously disagree.  This is first and foremost about correcting a
design mistake we made.

>>I think we should just say that
>> RHEL-6 should use i440fx forever and be done with it.
>
> I'm not sure if you are saying that we (Red Hat) shouldn't spend
> time implementing it, or that the libvirt upstream project should
> reject the patches if somebody implements it.  I would understand
> the former, but not the latter.

I would be willing to listen a reasoned argument why correcting the
design mistake is not worthwhile.  I'm unwilling to listen to more
downstream blaming.  Please stop it.

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

[libvirt] [PATCH v5 2/3] virsh: Implement new table API for virsh list

[Simon Kobyda]

Instead of printing it straight in virsh, it creates table struct
which is filled with header and rows(domains). It allows us to know
more about table before printing to calculate alignment right.

Signed-off-by: Simon Kobyda 
---
 tests/virshtest.c| 14 ++--
 tools/virsh-domain-monitor.c | 43 
 2 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/tests/virshtest.c b/tests/virshtest.c
index 94548a82d1..10cd0d356b 100644
--- a/tests/virshtest.c
+++ b/tests/virshtest.c
@@ -98,9 +98,9 @@ static int testCompareListDefault(const void *data 
ATTRIBUTE_UNUSED)
 {
   const char *const argv[] = { VIRSH_DEFAULT, "list", NULL };
   const char *exp = "\
- IdName   State\n\
-\n\
- 1 test   running\n\
+ Id   Name   State\n\
+--\n\
+ 1test   running  \n\
 \n";
   return testCompareOutputLit(exp, NULL, argv);
 }
@@ -109,10 +109,10 @@ static int testCompareListCustom(const void *data 
ATTRIBUTE_UNUSED)
 {
   const char *const argv[] = { VIRSH_CUSTOM, "list", NULL };
   const char *exp = "\
- IdName   State\n\
-\n\
- 1 fv0running\n\
- 2 fc4running\n\
+ Id   Name   State\n\
+--\n\
+ 1fv0running  \n\
+ 2fc4running  \n\
 \n";
   return testCompareOutputLit(exp, NULL, argv);
 }
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index b9b4f9739b..adc5bb1a7a 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -39,6 +39,7 @@
 #include "virmacaddr.h"
 #include "virxml.h"
 #include "virstring.h"
+#include "vsh-table.h"
 
 VIR_ENUM_DECL(virshDomainIOError)
 VIR_ENUM_IMPL(virshDomainIOError,
@@ -1901,6 +1902,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 char id_buf[INT_BUFSIZE_BOUND(unsigned int)];
 unsigned int id;
 unsigned int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE;
+vshTablePtr table = NULL;
 
 /* construct filter flags */
 if (vshCommandOptBool(cmd, "inactive") ||
@@ -1940,15 +1942,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 /* print table header in legacy mode */
 if (optTable) {
 if (optTitle)
-vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n",
-  _("Id"), _("Name"), _("State"), _("Title"),
-  "-"
-  "-");
+table = vshTableNew("Id", "Name", "State", "Title", NULL);
 else
-vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n",
-  _("Id"), _("Name"), _("State"),
-  "-"
-  "---");
+table = vshTableNew("Id", "Name", "State", NULL);
+
+if (!table)
+goto cleanup;
 }
 
 for (i = 0; i < list->ndomains; i++) {
@@ -1973,20 +1972,22 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 if (optTitle) {
 if (!(title = virshGetDomainDescription(ctl, dom, true, 0)))
 goto cleanup;
-
-vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", id_buf,
- virDomainGetName(dom),
- state == -2 ? _("saved")
- : virshDomainStateToString(state),
- title);
-
+if (vshTableRowAppend(table, id_buf,
+  virDomainGetName(dom),
+  state == -2 ? _("saved")
+  : virshDomainStateToString(state),
+  title, NULL) < 0)
+goto cleanup;
 VIR_FREE(title);
 } else {
-vshPrint(ctl, " %-5s %-30s %s\n", id_buf,
- virDomainGetName(dom),
- state == -2 ? _("saved")
- : virshDomainStateToString(state));
+if (vshTableRowAppend(table, id_buf,
+  virDomainGetName(dom),
+  state == -2 ? _("saved")
+  : virshDomainStateToString(state),
+  NULL) < 0)
+goto cleanup;
 }
+
 } else if (optUUID && optName) {
 if (virDomainGetUUIDString(dom, uuid) < 0) {
 vshError(ctl, "%s", _("Failed to get domain's UUID"));
@@ -2004,8 +2005,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 }
 }
 
+if (optTable)
+vshTablePrintToStdout(table, ctl);
+
 ret = true;
  cleanup:
+vshTableFree(table);
 

[libvirt] [PATCH v5 0/3] vsh: Introduce new API for printing tables

[Simon Kobyda]

Created new API for priting tables, mainly to solve alignment problems.
Implemented these test to virsh list. In the future, API may be
everywhere in virsh and virt-admin.
Also wrote basic tests for the new API, and corrected tests in virshtest
which are influenced by implementation of the API in virsh list.

Changes in v5:
- cleanup and merged code for calculating zero-width, nonprintable and combined
character.
- replaced virBufferAddStr with virBufferAddChar in some places
- in tests moved code for setting correct locale
- fixed few leaks and unitialized values

Changes in v4:
- fixed width calculation for zero-width, nonprintable and combined
character. (pulled some code from linux-util)
- added tests for cases mentioned above
- changed usage of vshControl variables. From now on PrintToStdout calls
PrintToString and then prints returned string to stdout

Changes in v3:
- changed encoding of 3/3 patch, otherwise it cannot be applied

Changes in v2:
- added tests
- fixed alignment for unicode character which span more spaces
- moved ncolumns check to vshTableRowAppend
- changed arguments for functions vshTablePrint, vshTablePrintToStdout,
vshTablePrintToString

Simon Kobyda (3):
  vsh: Add API for printing tables.
  virsh: Implement new table API for virsh list
  vsh: Added tests

 tests/Makefile.am|   8 +
 tests/virshtest.c|  14 +-
 tests/vshtabletest.c | 377 +
 tools/Makefile.am|   4 +-
 tools/virsh-domain-monitor.c |  43 ++--
 tools/vsh-table.c| 449 +++
 tools/vsh-table.h|  42 
 7 files changed, 910 insertions(+), 27 deletions(-)
 create mode 100644 tests/vshtabletest.c
 create mode 100644 tools/vsh-table.c
 create mode 100644 tools/vsh-table.h

-- 
2.17.1

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

[libvirt] [PATCH 2/2] qemu: Add check for unpriv sgio for SCSI generic host device

[John Ferlan]

Check if the hostdev has set the sgio filtered/unfiltered and handle
appropriately.

This restores functionality removed by commit id 'ce346623' to remove
sgio support for the SCSI generic host device in the formerly named
method qemuCheckSharedDevice.

For most kernels the result of this operation is a no-op; however, for
those that do support it the check is necessary.

Note that this patch fixes a bug found in the reverted change where
if the qemuGetDeviceUnprivSGIO returned either 0 or 1 in 'val', the
'disk' would be dereferenced; however, since a hostdev didn't have
one - that dereference would have caused a segfault. That was fixed
by commit id 'cd01d2ad5' using virDomainDiskGetSource. Instead these
changes use the hostdev's address value.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_conf.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 3d28c03fa1..3f24c8b8fc 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1473,6 +1473,8 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver,
 {
 char *dev_path = NULL;
 char *key = NULL;
+virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
+virDomainHostdevSubsysSCSIHostPtr scsihostsrc = >u.host;
 int ret = -1;
 
 if (!qemuIsSharedHostdev(hostdev))
@@ -1481,6 +1483,19 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver,
 if (!(dev_path = qemuGetHostdevPath(hostdev)))
 goto cleanup;
 
+if ((ret = qemuCheckUnprivSGIO(driver->sharedDevices, dev_path,
+   scsisrc->sgio)) < 0) {
+if (ret == -2) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("sgio of shared scsi host device '%s-%u-%u-%llu' "
+ "conflicts with other active domains"),
+   scsihostsrc->adapter, scsihostsrc->bus,
+   scsihostsrc->target, scsihostsrc->unit);
+ret = -1;
+}
+goto cleanup;
+}
+
 if (!(key = qemuGetSharedDeviceKey(dev_path)))
 goto cleanup;
 
-- 
2.17.1

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

[libvirt] [PATCH 0/2] Allow usage of unpriv_sgio for SCSI generic hostdev

[John Ferlan]

In what is perhaps a couple lifetimes ago at this point, patches
were posted and "mostly" all eventually accepted upstream to support
unpriv_sgio on SCSI generic hostdev devices. Since the needed parts
of the functionality from the kernel perspective were not yet present
upstream, a couple of the patches were not accepted. See:

https://www.redhat.com/archives/libvir-list/2015-July/msg00204.html

and in particular patches 9 and 10. Since that time it seems things
have changed and this essentially reposts those two patches with
some minor adjustments in logic in order to "restore" the support.
There are lots of interesting tidbits in unreadable by the general
public bz's, so I won't include links here.

John Ferlan (2):
  qemu: Add ability to set sgio values for hostdev
  qemu: Add check for unpriv sgio for SCSI generic host device

 src/qemu/qemu_conf.c | 34 --
 1 file changed, 24 insertions(+), 10 deletions(-)

-- 
2.17.1

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

[libvirt] [PATCH v5 1/3] vsh: Add API for printing tables.

[Simon Kobyda]

It solves problems with alignment of columns. Width of each column
is calculated by its biggest cell. Should solve unicode bug.
In future, it may be implemented in virsh, virt-admin...

This API has 5 public functions:
- vshTableNew - adds new table and defines its header
- vshTableRowAppend - appends new row (for same number of columns as in
header)
- vshTablePrintToStdout
- vshTablePrintToString
- vshTableFree

https://bugzilla.redhat.com/show_bug.cgi?id=1574624
https://bugzilla.redhat.com/show_bug.cgi?id=1584630

Signed-off-by: Simon Kobyda 
---
 tools/Makefile.am |   4 +-
 tools/vsh-table.c | 449 ++
 tools/vsh-table.h |  42 +
 3 files changed, 494 insertions(+), 1 deletion(-)
 create mode 100644 tools/vsh-table.c
 create mode 100644 tools/vsh-table.h

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 1452d984a0..f069167acc 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -144,7 +144,9 @@ libvirt_shell_la_LIBADD = \
$(READLINE_LIBS) \
../gnulib/lib/libgnu.la \
$(NULL)
-libvirt_shell_la_SOURCES = vsh.c vsh.h
+libvirt_shell_la_SOURCES = \
+   vsh.c vsh.h \
+   vsh-table.c vsh-table.h
 
 virt_host_validate_SOURCES = \
virt-host-validate.c \
diff --git a/tools/vsh-table.c b/tools/vsh-table.c
new file mode 100644
index 00..ca4e9265c5
--- /dev/null
+++ b/tools/vsh-table.c
@@ -0,0 +1,449 @@
+/*
+ * vsh-table.c: table printing helper
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Authors:
+ *   Simon Kobyda 
+ *
+ */
+
+#include 
+#include "vsh-table.h"
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "c-ctype.h"
+
+#include "viralloc.h"
+#include "virbuffer.h"
+#include "virstring.h"
+#include "virsh-util.h"
+
+#define HEX_ENCODE_LENGTH 4 /* represents length of '\xNN' */
+
+struct _vshTableRow {
+char **cells;
+size_t ncells;
+};
+
+struct _vshTable {
+vshTableRowPtr *rows;
+size_t nrows;
+};
+
+static void
+vshTableRowFree(vshTableRowPtr row)
+{
+size_t i;
+
+if (!row)
+return;
+
+for (i = 0; i < row->ncells; i++)
+VIR_FREE(row->cells[i]);
+
+VIR_FREE(row->cells);
+VIR_FREE(row);
+}
+
+void
+vshTableFree(vshTablePtr table)
+{
+size_t i;
+
+if (!table)
+return;
+
+for (i = 0; i < table->nrows; i++)
+vshTableRowFree(table->rows[i]);
+VIR_FREE(table->rows);
+VIR_FREE(table);
+}
+
+/**
+ * vshTableRowNew:
+ * @arg: the first argument.
+ * @ap: list of variadic arguments
+ *
+ * Create a new row in the table. Each argument passed
+ * represents a cell in the row.
+ *
+ * Return: pointer to vshTableRowPtr row or NULL.
+ */
+static vshTableRowPtr
+vshTableRowNew(const char *arg, va_list ap)
+{
+vshTableRowPtr row = NULL;
+
+if (!arg) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Table row cannot be empty"));
+goto error;
+}
+
+if (VIR_ALLOC(row) < 0)
+goto error;
+
+while (arg) {
+char *tmp = NULL;
+
+if (VIR_STRDUP(tmp, arg) < 0)
+goto error;
+
+if (VIR_APPEND_ELEMENT(row->cells, row->ncells, tmp) < 0) {
+VIR_FREE(tmp);
+goto error;
+}
+
+arg = va_arg(ap, const char *);
+}
+
+return row;
+
+ error:
+vshTableRowFree(row);
+return NULL;
+}
+
+/**
+ * vshTableNew:
+ * @arg: List of column names (NULL terminated)
+ *
+ * Create a new table.
+ *
+ * Returns: pointer to table or NULL.
+ */
+vshTablePtr
+vshTableNew(const char *arg, ...)
+{
+vshTablePtr table = NULL;
+vshTableRowPtr header = NULL;
+va_list ap;
+
+if (VIR_ALLOC(table) < 0)
+goto error;
+
+va_start(ap, arg);
+header = vshTableRowNew(arg, ap);
+va_end(ap);
+
+if (!header)
+goto error;
+
+if (VIR_APPEND_ELEMENT(table->rows, table->nrows, header) < 0)
+goto error;
+
+return table;
+ error:
+vshTableRowFree(header);
+vshTableFree(table);
+return NULL;
+}
+
+/**
+ * vshTableRowAppend:
+ * @table: table to append to
+ * @arg: cells of the row (NULL terminated)
+ *
+ * Append new row into the @table. The number of cells in the row has
+ * to be equal to the number of cells in the table 

[libvirt] [PATCH v5 3/3] vsh: Added tests

[Simon Kobyda]

For now, there are 9 test cases
- testVshTableNew: Creating table with empty header
- testVshTableHeader: Printing table with/without header
- testVshTableRowAppend: Appending row with various number of cells.
  Only row with same number of cells as in header is accepted.
- testUnicode: Printing table with unicode characters.
  Checking correct alignment.
- testUnicodeArabic: test opposite (right to left) writing
- testUnicodeZeroWidthChar
- testUnicodeCombiningChar
- testUnicodeNonPrintableChar,
- testNTables: Create and print varios types of tables - one column,
  one row table, table without content, standart table...

Signed-off-by: Simon Kobyda 
---
 tests/Makefile.am|   8 +
 tests/vshtabletest.c | 377 +++
 2 files changed, 385 insertions(+)
 create mode 100644 tests/vshtabletest.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 21a6c823d9..136fe16f71 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -206,6 +206,7 @@ test_programs = virshtest sockettest \
virhostdevtest \
virnetdevtest \
virtypedparamtest \
+   vshtabletest \
$(NULL)
 
 test_libraries = libshunload.la \
@@ -938,6 +939,13 @@ metadatatest_SOURCES = \
testutils.c testutils.h
 metadatatest_LDADD = $(LDADDS) $(LIBXML_LIBS)
 
+vshtabletest_SOURCES = \
+   vshtabletest.c \
+   testutils.c testutils.h
+vshtabletest_LDADD = \
+   $(LDADDS) \
+   ../tools/libvirt_shell.la
+
 virshtest_SOURCES = \
virshtest.c \
testutils.c testutils.h
diff --git a/tests/vshtabletest.c b/tests/vshtabletest.c
new file mode 100644
index 00..1b07c37c56
--- /dev/null
+++ b/tests/vshtabletest.c
@@ -0,0 +1,377 @@
+/*
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include 
+#include 
+#include 
+
+#include "internal.h"
+#include "testutils.h"
+#include "viralloc.h"
+#include "../tools/vsh-table.h"
+
+static int
+testVshTableNew(const void *opaque ATTRIBUTE_UNUSED)
+{
+if (vshTableNew(NULL)) {
+fprintf(stderr, "expected failure when passing null to vshTableNew\n");
+return -1;
+}
+
+return 0;
+}
+
+static int
+testVshTableHeader(const void *opaque ATTRIBUTE_UNUSED)
+{
+int ret = 0;
+char *act = NULL;
+const char *exp =
+" 1   fedora28   running  \n"
+" 2   rhel7.5running  \n";
+const char *exp2 =
+" Id   Name   State\n"
+"--\n"
+" 1fedora28   running  \n"
+" 2rhel7.5running  \n";
+
+vshTablePtr table = vshTableNew("Id", "Name", "State",
+NULL); //to ask about return
+if (!table)
+goto cleanup;
+
+vshTableRowAppend(table, "1", "fedora28", "running", NULL);
+vshTableRowAppend(table, "2", "rhel7.5", "running",
+  NULL);
+
+act = vshTablePrintToString(table, false);
+if (virTestCompareToString(exp, act) < 0)
+ret = -1;
+
+VIR_FREE(act);
+act = vshTablePrintToString(table, true);
+if (virTestCompareToString(exp2, act) < 0)
+ret = -1;
+
+ cleanup:
+VIR_FREE(act);
+vshTableFree(table);
+return ret;
+}
+
+static int
+testVshTableRowAppend(const void *opaque ATTRIBUTE_UNUSED)
+{
+int ret = 0;
+
+vshTablePtr table = vshTableNew("Id", "Name", NULL);
+if (!table)
+goto cleanup;
+
+if (vshTableRowAppend(table, NULL) >= 0) {
+fprintf(stderr, "Appending NULL shouldn't work\n");
+ret = -1;
+}
+
+if (vshTableRowAppend(table, "2", NULL) >= 0) {
+fprintf(stderr, "Appending less items than in header\n");
+ret = -1;
+}
+
+if (vshTableRowAppend(table, "2", "rhel7.5", "running",
+  NULL) >= 0) {
+fprintf(stderr, "Appending more items than in header\n");
+ret = -1;
+}
+
+if (vshTableRowAppend(table, "2", "rhel7.5", NULL) < 0) {
+fprintf(stderr, "Appending same number of items as in header"
+" should not return NULL\n");
+ret = -1;
+}
+
+ cleanup:
+vshTableFree(table);
+return ret;
+}
+
+static int
+testUnicode(const void *opaque ATTRIBUTE_UNUSED)
+{
+
+int ret = 0;
+char *act = NULL;
+
+const char *exp =
+" Id   名稱  государство  \n"

Re: [libvirt] [PATCH v2 7/7] qemu_security: Lock metadata while relabelling

[Daniel P . Berrangé]

On Thu, Aug 23, 2018 at 05:45:42PM +0200, Michal Privoznik wrote:
> On 08/23/2018 05:01 PM, Daniel P. Berrangé wrote:
> > On Thu, Aug 23, 2018 at 04:54:01PM +0200, Michal Privoznik wrote:
> >> On 08/20/2018 07:17 PM, Michal Prívozník wrote:
> >>> On 08/20/2018 05:16 PM, Daniel P. Berrangé wrote:
>  On Tue, Aug 14, 2018 at 01:19:43PM +0200, Michal Privoznik wrote:
> > Fortunately, we have qemu wrappers so it's sufficient to put
> > lock/unlock call only there.
> >
> > Signed-off-by: Michal Privoznik 
> > ---
> >  src/qemu/qemu_security.c | 107 
> > +++
> >  1 file changed, 107 insertions(+)
> >
> >>
> >>
>  I'm wondering if this is really the right level to plug in the metadata
>  locking ?  What about if we just pass a virLockManagerPtr to the security
>  drivers and let them lock each resource at the time they need to modify
>  its metadata. This will trivially guarantee that we always lock the exact
>  set of files that we'll be changing metadata on.
> 
>  eg in SELinux driver the virSecuritySELinuxSetFileconHelper method
>  would directly call virLockManagerAcquire & virLockManagerRelease,
>  avoiding the entire virDomainLock  abstraction which was really
>  focused around managing the content locks around lifecycle state
>  changes.
> 
> >>>
> >>> Yeah, I vaguely recall writing code like this (when I was trying to
> >>> solve this some time ago). Okay, let me see if that's feasible.
> >>>
> >>> But boy, this is getting hairy.
> >>
> >> So as I'm writing these patches I came to realize couple of things:
> >>
> >> a) instead of domain PID we should pass libvirtd PID because we want to
> >> release the locks if libvirtd dies not domain.
> >>
> >> b) that, however, leads to a problem because
> >> virLockManagerLockDaemonAcquire() closes the socket to virtlockd causing
> >> it to kill the owner of the lock, i.e. it kills libvirtd immediately.
> > 
> > This is fine ;-P
> > 
> >> c) even if I hack around b) so that we connect only once for each
> >> lock+unlock pair call, we would still connect dozens of times when
> >> starting a domain (all the paths we label times all active secdrivers).
> >> So we should share connection here too.
> > 
> > Yeah, makes sense.
> > 
> >> Now question is how do do this effectively and cleanly (code-wise). For
> >> solving b) we can have a new flag, say VIR_LOCK_MANAGER_KEEP_CONNECT
> >> that would cause  virLockManagerLockDaemonAcquire() to save the
> >> (connection, program, counter) tuple somewhere into lock driver private
> >> data so that virLockManagerLockDaemonRelease() called with the same flag
> >> can re-use the data.
> >>
> >> For c) I guess we will need to open the connection in
> >> virLockManagerLockDaemonNew(), put the socket FD into event loop so that
> >> EOF is caught and initiate reopen in that case. However, I'm not sure if
> >> this is desirable - to be constantly connected to virtlockd.
> > 
> > Can we use a model similar to what I did for the shared secondary
> > driver connections.
> > 
> > By default a call to virGetConnectNetwork() will open a new connection.
> > 
> > To avoid opening & closing 100's of connections though, some places
> > will call virSetConnectNetwork() to store a pre-opened connection in
> > a thread local. That stays open until virSetConnectNetwork is called
> > again passing in a NULL.
> > 
> > We would put such a cache around any bits of code that triggers
> > many connection calls to virlockd.
> 
> Actually, would sharing connection for case c) work?
> 
> Consider the following scenario: two threads A and B starting two
> different domains, both of them which want to relabel /dev/blah.
> 
> Now, say thread A gets to lock the path first. It does so, and proceed
> to chown().
> 
> Then thread B wants to lock the same path. It tries to do so, but has to
> wait until A unlocks it. However, at this point virtlockd is stuck in
> virFileLock() call (it waits for the lock to be released).
> 
> Because virtlockd runs single threaded it doesn't matter anymore that A
> will try to unlock the path eventually. virtlockd has deadlocked.
> 
> 
> I don't see any way around this :(

Oh right, yes, that kills the idea of using a WAIT flag for lockspaces,
unless we want to introduce threads ingto virtlockd, but we can't do
that because Linux has totally fubard  locking across execve() :-(

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH 1/2] qemu: Add ability to set sgio values for hostdev

[John Ferlan]

Add necessary checks in order to allow setting sgio values for a scsi
host device.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_conf.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index a4f545ef92..3d28c03fa1 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1633,13 +1633,15 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
 virDomainDiskDefPtr disk = NULL;
 virDomainHostdevDefPtr hostdev = NULL;
 char *sysfs_path = NULL;
+char *hostdev_path = NULL;
 const char *path = NULL;
 int val = -1;
 int ret = -1;
 
 /* "sgio" is only valid for block disk; cdrom
  * and floopy disk can have empty source.
- */
+ * NB: The default is to filter SG_IO commands
+ * (i.e. set unpriv_sgio to 0). */
 if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
 disk = dev->data.disk;
 
@@ -1648,20 +1650,19 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
 return 0;
 
 path = virDomainDiskGetSource(disk);
+val = (disk->sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED);
 } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
 hostdev = dev->data.hostdev;
 
 if (!qemuIsSharedHostdev(hostdev))
 return 0;
 
-if (hostdev->source.subsys.u.scsi.sgio) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("'sgio' is not supported for SCSI "
- "generic device yet "));
+if (!(hostdev_path = qemuGetHostdevPath(hostdev)))
 goto cleanup;
-}
 
-return 0;
+path = hostdev_path;
+val = (hostdev->source.subsys.u.scsi.sgio ==
+   VIR_DOMAIN_DEVICE_SGIO_UNFILTERED);
 } else {
 return 0;
 }
@@ -1669,9 +1670,6 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
 if (!(sysfs_path = virGetUnprivSGIOSysfsPath(path, NULL)))
 goto cleanup;
 
-/* By default, filter the SG_IO commands, i.e. set unpriv_sgio to 0.  */
-val = (disk->sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED);
-
 /* Do not do anything if unpriv_sgio is not supported by the kernel and the
  * whitelist is enabled.  But if requesting unfiltered access, always call
  * virSetDeviceUnprivSGIO, to report an error for unsupported unpriv_sgio.
@@ -1683,6 +1681,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
 ret = 0;
 
  cleanup:
+VIR_FREE(hostdev_path);
 VIR_FREE(sysfs_path);
 return ret;
 }
-- 
2.17.1

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

Re: [libvirt] [PATCH v2 7/7] qemu_security: Lock metadata while relabelling

[Michal Privoznik]

On 08/23/2018 05:01 PM, Daniel P. Berrangé wrote:
> On Thu, Aug 23, 2018 at 04:54:01PM +0200, Michal Privoznik wrote:
>> On 08/20/2018 07:17 PM, Michal Prívozník wrote:
>>> On 08/20/2018 05:16 PM, Daniel P. Berrangé wrote:
 On Tue, Aug 14, 2018 at 01:19:43PM +0200, Michal Privoznik wrote:
> Fortunately, we have qemu wrappers so it's sufficient to put
> lock/unlock call only there.
>
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_security.c | 107 
> +++
>  1 file changed, 107 insertions(+)
>
>>
>>
 I'm wondering if this is really the right level to plug in the metadata
 locking ?  What about if we just pass a virLockManagerPtr to the security
 drivers and let them lock each resource at the time they need to modify
 its metadata. This will trivially guarantee that we always lock the exact
 set of files that we'll be changing metadata on.

 eg in SELinux driver the virSecuritySELinuxSetFileconHelper method
 would directly call virLockManagerAcquire & virLockManagerRelease,
 avoiding the entire virDomainLock  abstraction which was really
 focused around managing the content locks around lifecycle state
 changes.

>>>
>>> Yeah, I vaguely recall writing code like this (when I was trying to
>>> solve this some time ago). Okay, let me see if that's feasible.
>>>
>>> But boy, this is getting hairy.
>>
>> So as I'm writing these patches I came to realize couple of things:
>>
>> a) instead of domain PID we should pass libvirtd PID because we want to
>> release the locks if libvirtd dies not domain.
>>
>> b) that, however, leads to a problem because
>> virLockManagerLockDaemonAcquire() closes the socket to virtlockd causing
>> it to kill the owner of the lock, i.e. it kills libvirtd immediately.
> 
> This is fine ;-P
> 
>> c) even if I hack around b) so that we connect only once for each
>> lock+unlock pair call, we would still connect dozens of times when
>> starting a domain (all the paths we label times all active secdrivers).
>> So we should share connection here too.
> 
> Yeah, makes sense.
> 
>> Now question is how do do this effectively and cleanly (code-wise). For
>> solving b) we can have a new flag, say VIR_LOCK_MANAGER_KEEP_CONNECT
>> that would cause  virLockManagerLockDaemonAcquire() to save the
>> (connection, program, counter) tuple somewhere into lock driver private
>> data so that virLockManagerLockDaemonRelease() called with the same flag
>> can re-use the data.
>>
>> For c) I guess we will need to open the connection in
>> virLockManagerLockDaemonNew(), put the socket FD into event loop so that
>> EOF is caught and initiate reopen in that case. However, I'm not sure if
>> this is desirable - to be constantly connected to virtlockd.
> 
> Can we use a model similar to what I did for the shared secondary
> driver connections.
> 
> By default a call to virGetConnectNetwork() will open a new connection.
> 
> To avoid opening & closing 100's of connections though, some places
> will call virSetConnectNetwork() to store a pre-opened connection in
> a thread local. That stays open until virSetConnectNetwork is called
> again passing in a NULL.
> 
> We would put such a cache around any bits of code that triggers
> many connection calls to virlockd.

Actually, would sharing connection for case c) work?

Consider the following scenario: two threads A and B starting two
different domains, both of them which want to relabel /dev/blah.

Now, say thread A gets to lock the path first. It does so, and proceed
to chown().

Then thread B wants to lock the same path. It tries to do so, but has to
wait until A unlocks it. However, at this point virtlockd is stuck in
virFileLock() call (it waits for the lock to be released).

Because virtlockd runs single threaded it doesn't matter anymore that A
will try to unlock the path eventually. virtlockd has deadlocked.


I don't see any way around this :(

Michal

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

Re: [libvirt] [PATCH v3 7/9] qemu: assign addresses to virtio devices on RISC-V

[Andrea Bolognani]

On Wed, 2018-08-22 at 11:15 +0200, Lubomir Rintel wrote:
> Signed-off-by: Lubomir Rintel 
> ---
>  src/qemu/qemu_domain_address.c | 16 
>  1 file changed, 16 insertions(+)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v3 5/9] qemu: add qemuDomainIsRISCVVirt() and qemuDomainMachineIsRISCVVirt()

[Andrea Bolognani]

On Wed, 2018-08-22 at 11:15 +0200, Lubomir Rintel wrote:
> Signed-off-by: Lubomir Rintel 
> ---
>  src/qemu/qemu_domain.c | 22 ++
>  src/qemu/qemu_domain.h |  3 +++
>  2 files changed, 25 insertions(+)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v3 6/9] qemu: add qemuDomainAssignVirtioMMIOAddresses()

[Andrea Bolognani]

On Wed, 2018-08-22 at 11:15 +0200, Lubomir Rintel wrote:

A short not explaining why you need to do this, along the lines of

  We're going to need to assign virtio-mmio addresses to non-ARM
  guests soon, so let's create a generic wrapper that calls to
  the architecture-specific implementation.

sure would be nice... Are you okay with me adding it to the commit
message before pushing?

Either way the code is fine, so

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v3 4/9] qemu: RISC-V machines have no PCI

[Andrea Bolognani]

On Wed, 2018-08-22 at 11:15 +0200, Lubomir Rintel wrote:
> Signed-off-by: Lubomir Rintel 
> ---
>  src/qemu/qemu_domain_address.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 6c540a8094..551883e989 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -2157,7 +2157,9 @@ static bool
>  qemuDomainSupportsPCI(virDomainDefPtr def,
>virQEMUCapsPtr qemuCaps)
>  {
> -if ((def->os.arch != VIR_ARCH_ARMV7L) && (def->os.arch != 
> VIR_ARCH_AARCH64))
> +if ((def->os.arch != VIR_ARCH_ARMV7L) &&
> +(def->os.arch != VIR_ARCH_AARCH64) &&
> +!ARCH_IS_RISCV(def->os.arch))
>  return true;
>  
>  if (STREQ(def->os.machine, "versatilepb"))

Someday we should really rewrite this function.
But today is not that day.

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v4 1/3] vsh: Add API for printing tables.

[Daniel P . Berrangé]

On Thu, Aug 23, 2018 at 03:19:39PM +0200, Michal Privoznik wrote:
> On 08/22/2018 07:42 PM, Simon Kobyda wrote:
> > It solves problems with alignment of columns. Width of each column
> > is calculated by its biggest cell. Should solve unicode bug.
> > In future, it may be implemented in virsh, virt-admin...
> > 
> > This API has 5 public functions:
> > - vshTableNew - adds new table and defines its header
> > - vshTableRowAppend - appends new row (for same number of columns as in
> > header)
> > - vshTablePrintToStdout
> > - vshTablePrintToString
> > - vshTableFree
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1574624
> > https://bugzilla.redhat.com/show_bug.cgi?id=1584630
> > 
> > Signed-off-by: Simon Kobyda 
> > ---
> >  tools/Makefile.am |   4 +-
> >  tools/vsh-table.c | 483 ++
> >  tools/vsh-table.h |  42 
> >  3 files changed, 528 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/vsh-table.c
> >  create mode 100644 tools/vsh-table.h


> > +/**
> > + * Function pulled from util-linux
> > + *
> > + * Function's name in util-linux: mbs_safe_encode_to_buffer
> > + *
> > + * Copy @s to @buf and replace control and non-printable chars with
> > + * \x?? hex sequence. The @width returns number of cells. The @safechars
> > + * are not encoded.
> > + *
> > + * The @buf has to be big enough to store mbs_safe_encode_size(strlen(s)))
> > + * bytes.
> 
> It's nice to give others credit, but the arguments make no sense to me.
> mbs_safe_encode_size ain't no libvirt function. But
> vshTableSafeEncodeSize() is.
> 
> Also, since we don't need the intermediate buffer anywhere, nor the safe
> buffer size can we merge those two functions together? This would have
> also a benefit of not duplicating some operations (e.g. strlen).
> 
> Moreover, safechars is always NULL. Do we need that argument?
> 
> NB, do we need to re-encode the string? All that we care about is its
> width, isn't it?

It is a design tradeoff to make the implementation more practical.

The code that determines the width doesn't work correctly for all
possibly unicode characters. By encoding the string we rewrite the
characters we can't handle into something we can handle. This means
our width calculation is always correct, but there are some
characters that we will end up displaying as escape sequences
instead.

Alternatively we can display every charcter normally, but have possibly
screwed up alignment due to incorrect width calculation.

IMHO if the escaping was good enough for util-linux to use, it is good
enough for us to use too.

Or to put it another way, if someone complains about this, we can
we can file the same bug against util-linux, let them fix it, and
then copy their fix back into our code :-)

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v3 3/9] util: add RISC-V architectures

[Andrea Bolognani]

On Wed, 2018-08-22 at 11:15 +0200, Lubomir Rintel wrote:
> +VIR_ARCH_RISCV32,  /* RISC-V  64 LE 
> http://en.wikipedia.org/wiki/RISC-V */

This should be   32 LE ...

> +VIR_ARCH_RISCV64,  /* RISC-V  32 BE 
> http://en.wikipedia.org/wiki/RISC-V */

... and this should be   64 LE

according to the corresponding implementation.

I'll take care of it before pushing.


Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v4 1/3] vsh: Add API for printing tables.

[Simon Kobyda]

On Thu, 2018-08-23 at 15:19 +0200, Michal Privoznik wrote:
> NB, do we need to re-encode the string? All that we care about is its
> width, isn't it?

Yes. Many nonprintable or control characters such as tabulator,
vertical tabulator, backspace... would be problematic, especially when
calculating width. So its better to just replace them with their
hexadecimal value.

Simon Kobyda

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

Re: [libvirt] [PATCH v2 7/7] qemu_security: Lock metadata while relabelling

[Daniel P . Berrangé]

On Thu, Aug 23, 2018 at 04:54:01PM +0200, Michal Privoznik wrote:
> On 08/20/2018 07:17 PM, Michal Prívozník wrote:
> > On 08/20/2018 05:16 PM, Daniel P. Berrangé wrote:
> >> On Tue, Aug 14, 2018 at 01:19:43PM +0200, Michal Privoznik wrote:
> >>> Fortunately, we have qemu wrappers so it's sufficient to put
> >>> lock/unlock call only there.
> >>>
> >>> Signed-off-by: Michal Privoznik 
> >>> ---
> >>>  src/qemu/qemu_security.c | 107 
> >>> +++
> >>>  1 file changed, 107 insertions(+)
> >>>
> 
> 
> >> I'm wondering if this is really the right level to plug in the metadata
> >> locking ?  What about if we just pass a virLockManagerPtr to the security
> >> drivers and let them lock each resource at the time they need to modify
> >> its metadata. This will trivially guarantee that we always lock the exact
> >> set of files that we'll be changing metadata on.
> >>
> >> eg in SELinux driver the virSecuritySELinuxSetFileconHelper method
> >> would directly call virLockManagerAcquire & virLockManagerRelease,
> >> avoiding the entire virDomainLock  abstraction which was really
> >> focused around managing the content locks around lifecycle state
> >> changes.
> >>
> > 
> > Yeah, I vaguely recall writing code like this (when I was trying to
> > solve this some time ago). Okay, let me see if that's feasible.
> > 
> > But boy, this is getting hairy.
> 
> So as I'm writing these patches I came to realize couple of things:
> 
> a) instead of domain PID we should pass libvirtd PID because we want to
> release the locks if libvirtd dies not domain.
> 
> b) that, however, leads to a problem because
> virLockManagerLockDaemonAcquire() closes the socket to virtlockd causing
> it to kill the owner of the lock, i.e. it kills libvirtd immediately.

This is fine ;-P

> c) even if I hack around b) so that we connect only once for each
> lock+unlock pair call, we would still connect dozens of times when
> starting a domain (all the paths we label times all active secdrivers).
> So we should share connection here too.

Yeah, makes sense.

> Now question is how do do this effectively and cleanly (code-wise). For
> solving b) we can have a new flag, say VIR_LOCK_MANAGER_KEEP_CONNECT
> that would cause  virLockManagerLockDaemonAcquire() to save the
> (connection, program, counter) tuple somewhere into lock driver private
> data so that virLockManagerLockDaemonRelease() called with the same flag
> can re-use the data.
> 
> For c) I guess we will need to open the connection in
> virLockManagerLockDaemonNew(), put the socket FD into event loop so that
> EOF is caught and initiate reopen in that case. However, I'm not sure if
> this is desirable - to be constantly connected to virtlockd.

Can we use a model similar to what I did for the shared secondary
driver connections.

By default a call to virGetConnectNetwork() will open a new connection.

To avoid opening & closing 100's of connections though, some places
will call virSetConnectNetwork() to store a pre-opened connection in
a thread local. That stays open until virSetConnectNetwork is called
again passing in a NULL.

We would put such a cache around any bits of code that triggers
many connection calls to virlockd.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v2 7/7] qemu_security: Lock metadata while relabelling

[Michal Privoznik]

On 08/20/2018 07:17 PM, Michal Prívozník wrote:
> On 08/20/2018 05:16 PM, Daniel P. Berrangé wrote:
>> On Tue, Aug 14, 2018 at 01:19:43PM +0200, Michal Privoznik wrote:
>>> Fortunately, we have qemu wrappers so it's sufficient to put
>>> lock/unlock call only there.
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  src/qemu/qemu_security.c | 107 
>>> +++
>>>  1 file changed, 107 insertions(+)
>>>


>> I'm wondering if this is really the right level to plug in the metadata
>> locking ?  What about if we just pass a virLockManagerPtr to the security
>> drivers and let them lock each resource at the time they need to modify
>> its metadata. This will trivially guarantee that we always lock the exact
>> set of files that we'll be changing metadata on.
>>
>> eg in SELinux driver the virSecuritySELinuxSetFileconHelper method
>> would directly call virLockManagerAcquire & virLockManagerRelease,
>> avoiding the entire virDomainLock  abstraction which was really
>> focused around managing the content locks around lifecycle state
>> changes.
>>
> 
> Yeah, I vaguely recall writing code like this (when I was trying to
> solve this some time ago). Okay, let me see if that's feasible.
> 
> But boy, this is getting hairy.

So as I'm writing these patches I came to realize couple of things:

a) instead of domain PID we should pass libvirtd PID because we want to
release the locks if libvirtd dies not domain.

b) that, however, leads to a problem because
virLockManagerLockDaemonAcquire() closes the socket to virtlockd causing
it to kill the owner of the lock, i.e. it kills libvirtd immediately.

c) even if I hack around b) so that we connect only once for each
lock+unlock pair call, we would still connect dozens of times when
starting a domain (all the paths we label times all active secdrivers).
So we should share connection here too.

Now question is how do do this effectively and cleanly (code-wise). For
solving b) we can have a new flag, say VIR_LOCK_MANAGER_KEEP_CONNECT
that would cause  virLockManagerLockDaemonAcquire() to save the
(connection, program, counter) tuple somewhere into lock driver private
data so that virLockManagerLockDaemonRelease() called with the same flag
can re-use the data.

For c) I guess we will need to open the connection in
virLockManagerLockDaemonNew(), put the socket FD into event loop so that
EOF is caught and initiate reopen in that case. However, I'm not sure if
this is desirable - to be constantly connected to virtlockd.

Michal

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

Re: [libvirt] [PATCH v3 2/9] qemu: rename qemuDomainArmVirt()

[Andrea Bolognani]

On Wed, 2018-08-22 at 11:15 +0200, Lubomir Rintel wrote:
> It's ARM specific.
> 
> Signed-off-by: Lubomir Rintel 
> ---
>  src/qemu/qemu_capabilities.c   |  4 ++--
>  src/qemu/qemu_command.c|  4 ++--
>  src/qemu/qemu_domain.c | 16 
>  src/qemu/qemu_domain.h |  2 +-
>  src/qemu/qemu_domain_address.c |  4 ++--
>  5 files changed, 15 insertions(+), 15 deletions(-)

https://www.redhat.com/archives/libvir-list/2018-June/msg01678.html

;)

Not worth reposting just for this though, I'll just fix it myself
before pushing - unless there are other issues that make a respin
necessary further on.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 1/5] docs: api_extension: Remove links to the stale example patches

[Erik Skultety]

On Thu, Aug 23, 2018 at 03:14:38PM +0100, Daniel P. Berrangé wrote:
> On Thu, Aug 23, 2018 at 03:55:16PM +0200, Peter Krempa wrote:
> > On Thu, Aug 23, 2018 at 15:46:30 +0200, Erik Skultety wrote:
> > > On Thu, Aug 23, 2018 at 10:50:30AM +0200, Peter Krempa wrote:
> > > > The pathches used as an example for the api_extension manual don't hold
> > >
> > > s/pathches/patches
> > >
> > > > up to the current standards any more. Carefully remove links and
> > > > mentions of the patches from the docs.
> > >
> > > While I understand the impetus for the change, I am personally not 
> > > convinced
> > > that we want to get rid of practical examples as a means of reference to 
> > > the
> > > text, it's like a product documentation without examples - "hardcore 
> > > mode".
> >
> > Any example will always become obsolete. I find trying to update it to
> > be a waste of time.
>
> I think that is rather a self fullfilling prophecy.
>
> The example becomes obsolete because no one can be bothered to spend
> time updating it. This doesn't imply we shouldn't even try. IMHO it
> is largely a sign that our priorities are screwed up, and we should
> put more effort into non-code writing parts of the project.
>
> Creating a long term healthy & viable project needs more than just
> writing lots of code. Considering feature implementation alone,
> there needs to be unit testing of the work, integration testing of
> the bigger system, documentation of the APIs and/or knowledgebase
> tutorials showing usage. In fact actually writing the core functional
> code could arguably end up being quite a small part of the overall
> work on a feature.
>
> Feature work is only one part of the project's long term success
> though. Bringing onboard new contributors is a key factor, and having
> something more to say to novices than "go read the code" is important
> to smoothing their onramp. Teaching application developers & admins
> what we've provide and how to use it is also heavily neglected in
> most cases such that we have great features no one knows about or
> uses correctly.
>
> Unfortunately we've historically not been very good at much of this,
> being very tightly focused on just writing the core code. This has
> definitely harmed our success in many areas. For example application
> developers have frequently questioned what value libvirt adds because
> we've not done a good enough job of telling people what we have
> implemented, or how to use it correctly once it exists.
>
> > > The fact that we took a patch series from the list archives as a reference
> > > probably wasn't the best thing to do, I think we should instead come up 
> > > with a
> > > dummy API exercising all the sections in the docs which will conform to 
> > > our
> >
> > Even coding style will become obsolete. Just the fact that we started
> > using the autofree stuff will make many existing examples obsolete.
> >
> > > current standards and which we can update as we please. I know that's 
> > > much more
> >
> > We don't even keep our coding style guidelines up to date most of the
> > time. Do you think this would be any different?
>
> This is a failure of our review process. In adding new code style rules,
> we should consider updating of examples as required part of the work.
> Reviewers should raise this if it isn't done. I'm as guilty of missing
> this as the next person, but we shouldn't give up otherwise we will just
> stagnate.
>
> IOW, lack of updated examples wrt autofree is a bug with the original
> autofree patch series we missed.

Right, but then again, we preferred converting the code base in batches.
Nevertheless, we can update the guidelines for sure.
Apart from that, couldn't agree more with what you wrote.

Erik

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

Re: [libvirt] [PATCH 4/5] docs: api_extension: Remove note to extend libvirt-TCK

[Erik Skultety]

On Thu, Aug 23, 2018 at 03:59:26PM +0200, Peter Krempa wrote:
> On Thu, Aug 23, 2018 at 15:25:17 +0200, Erik Skultety wrote:
> > On Thu, Aug 23, 2018 at 10:50:33AM +0200, Peter Krempa wrote:
> > > It probably does not make sense to do tests using that toolkit any more.
> >
> > At first I thought so too, I even thought that there hasn't been any 
> > commits to
> > the repo for a long time, it turned out I was wrong, there's still some work
> > going on, so I don't think we should abandon it completely. The fact that I
> > (and most probably you too - sorry, I just assumed your testing habits) 
> > don't
> > use it for testing doesn't mean, we should discourage others from giving the
> > framework a try, we're not mandating the usage so I don't see an issue with
> > the. With that said, once we have a proper continuous integration running
> > upstream, then I'd say we should start encouraging users to use the avocado
> > framework instead. However, for the time being, it's a NACK from me, maybe 
> > Dan
> > has a different feeling about this.
>
> Since the last non-maintenance commit for this section was in 2013, I
> don't think that mentioning any particular project makes sense. Avocado
> may become obsolete in another 5 years and I doubt that anybody will
> bother to update this document for the new hip project's name.

Everything will eventually become out-dated and by following that logic, the
only documentation we'd be left with is the code - "Documentation done
right". I disagree, since as I pointed out, we're not forcing anyone, we're
merely mentioning that such a thing still exists.

>
> I'm also okay just dropping this and making any poor soul which will
> follow that document to resolve any obsolete part by themselves.

Yep, let them make the decision for themselves, it's more than likely that
a thought of using it won't even cross their minds, fine by me.

Erik

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

Re: [libvirt] [PATCH 1/5] docs: api_extension: Remove links to the stale example patches

[Daniel P . Berrangé]

On Thu, Aug 23, 2018 at 03:55:16PM +0200, Peter Krempa wrote:
> On Thu, Aug 23, 2018 at 15:46:30 +0200, Erik Skultety wrote:
> > On Thu, Aug 23, 2018 at 10:50:30AM +0200, Peter Krempa wrote:
> > > The pathches used as an example for the api_extension manual don't hold
> > 
> > s/pathches/patches
> > 
> > > up to the current standards any more. Carefully remove links and
> > > mentions of the patches from the docs.
> > 
> > While I understand the impetus for the change, I am personally not convinced
> > that we want to get rid of practical examples as a means of reference to the
> > text, it's like a product documentation without examples - "hardcore mode".
> 
> Any example will always become obsolete. I find trying to update it to
> be a waste of time.

I think that is rather a self fullfilling prophecy.

The example becomes obsolete because no one can be bothered to spend
time updating it. This doesn't imply we shouldn't even try. IMHO it
is largely a sign that our priorities are screwed up, and we should
put more effort into non-code writing parts of the project.

Creating a long term healthy & viable project needs more than just
writing lots of code. Considering feature implementation alone,
there needs to be unit testing of the work, integration testing of
the bigger system, documentation of the APIs and/or knowledgebase
tutorials showing usage. In fact actually writing the core functional
code could arguably end up being quite a small part of the overall
work on a feature.

Feature work is only one part of the project's long term success
though. Bringing onboard new contributors is a key factor, and having
something more to say to novices than "go read the code" is important
to smoothing their onramp. Teaching application developers & admins
what we've provide and how to use it is also heavily neglected in
most cases such that we have great features no one knows about or
uses correctly.

Unfortunately we've historically not been very good at much of this,
being very tightly focused on just writing the core code. This has
definitely harmed our success in many areas. For example application
developers have frequently questioned what value libvirt adds because
we've not done a good enough job of telling people what we have
implemented, or how to use it correctly once it exists.

> > The fact that we took a patch series from the list archives as a reference
> > probably wasn't the best thing to do, I think we should instead come up 
> > with a
> > dummy API exercising all the sections in the docs which will conform to our
> 
> Even coding style will become obsolete. Just the fact that we started
> using the autofree stuff will make many existing examples obsolete.
> 
> > current standards and which we can update as we please. I know that's much 
> > more
> 
> We don't even keep our coding style guidelines up to date most of the
> time. Do you think this would be any different?

This is a failure of our review process. In adding new code style rules,
we should consider updating of examples as required part of the work.
Reviewers should raise this if it isn't done. I'm as guilty of missing
this as the next person, but we shouldn't give up otherwise we will just
stagnate.

IOW, lack of updated examples wrt autofree is a bug with the original
autofree patch series we missed.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 0/8] qemu: monitor: Clean up error handling for block job APIs

[Peter Krempa]

On Wed, Aug 15, 2018 at 13:52:51 +0200, Peter Krempa wrote:
> Remove handling of errors which QEMU no longer reports and rename
> fields so that it conforms with the new semantics.
> 
> Peter Krempa (8):
>   qemu: monitor: Remove error classes not conforming to QAPI schema
>   qemu: monitor: Use qemuMonitorJSONCheckError in
> qemuMonitorJSONBlockJobError
>   qemu: monitor: Remove temporary variables
>   qemu: monitor: Use qemuMonitorJSONCheckError in
> qemuMonitorJSONBlockStream
>   qemu: monitor: Move qemuMonitorJSONDrivePivot together with block-job
> APIs
>   qemu: monitor: Use qemuMonitorJSONBlockJobError in
> qemuMonitorJSONDrivePivot
>   qemu: monitor: Rename 'device' argument for block job control APIs
>   qemu: monitor: Separate probing for active block commit
> 
>  src/qemu/qemu_monitor.c  |  20 +++---
>  src/qemu/qemu_monitor.h  |   6 +-
>  src/qemu/qemu_monitor_json.c | 155 
> +--
>  src/qemu/qemu_monitor_json.h |   9 ++-
>  4 files changed, 94 insertions(+), 96 deletions(-)

Ping?


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

Re: [libvirt] [PATCH v2 00/10] qemu: Get rid of QEMU_CAPS_ADD_FD (and improve block-commit probing)

[Peter Krempa]

On Wed, Aug 15, 2018 at 11:18:37 +0200, Peter Krempa wrote:
> Diff to v1:
> - stopped probing for active block-commit support when we can use QMP
> schema
> - qemuxml2argvmock's implementation of virCommandPassFD now passes
> through specific file descriptors and ignores the rest.

Ping?


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

Re: [libvirt] [PATCH 1/5] docs: api_extension: Remove links to the stale example patches

[Erik Skultety]

On Thu, Aug 23, 2018 at 03:55:16PM +0200, Peter Krempa wrote:
> On Thu, Aug 23, 2018 at 15:46:30 +0200, Erik Skultety wrote:
> > On Thu, Aug 23, 2018 at 10:50:30AM +0200, Peter Krempa wrote:
> > > The pathches used as an example for the api_extension manual don't hold
> >
> > s/pathches/patches
> >
> > > up to the current standards any more. Carefully remove links and
> > > mentions of the patches from the docs.
> >
> > While I understand the impetus for the change, I am personally not convinced
> > that we want to get rid of practical examples as a means of reference to the
> > text, it's like a product documentation without examples - "hardcore mode".
>
> Any example will always become obsolete. I find trying to update it to
> be a waste of time.
>
> Users are always welcome to inspire from any existing piece of code. We
> are open-source in the end. I don't really think we need as much
> hand-holding in this area.
>
> > The fact that we took a patch series from the list archives as a reference
> > probably wasn't the best thing to do, I think we should instead come up 
> > with a
> > dummy API exercising all the sections in the docs which will conform to our
>
> Even coding style will become obsolete. Just the fact that we started
> using the autofree stuff will make many existing examples obsolete.
>
> > current standards and which we can update as we please. I know that's much 
> > more
>
> We don't even keep our coding style guidelines up to date most of the
> time. Do you think this would be any different?

That's a good point. In fact, not just that, once we split the daemon into
multiple daemons, it's likely that the guide itself might need updating, let's
see how that will turn out.

You already got a RB from Jano and since I'm undecided here, whatever.

Erik

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

Re: [libvirt] [PATCH 2/5] docs: api_extension: Remove example patches

[Ján Tomko]

On Thu, Aug 23, 2018 at 10:50:31AM +0200, Peter Krempa wrote:

Now that they are not linked any more remove them.

Signed-off-by: Peter Krempa 
---
docs/Makefile.am   |   4 +-
docs/api_extension/0001-add-to-xml.patch   | 145 
docs/api_extension/0002-add-new-public-API.patch   |  62 --
.../0003-define-internal-driver-API.patch  | 222 ---
.../0004-implement-the-public-APIs.patch   | 188 --
.../0005-implement-the-remote-protocol.patch   | 421 
...06-make-old-API-trivially-wrap-to-new-API.patch | 735 -
docs/api_extension/0007-add-virsh-support.patch| 388 ---
docs/api_extension/0008-support-new-xml.patch  | 519 ---
.../0009-support-all-flags-in-test-driver.patch| 197 --
...improve-vcpu-support-in-qemu-command-line.patch | 122 
...0011-complete-vcpu-support-in-qemu-driver.patch | 169 -
...-improve-vcpu-support-in-xen-command-line.patch | 294 -
.../0013-improve-getting-xen-vcpu-counts.patch | 216 --
.../0014-improve-setting-xen-vcpu-counts.patch | 342 --
docs/api_extension/0015-remove-dead-xen-code.patch | 228 ---
16 files changed, 1 insertion(+), 4251 deletions(-)
delete mode 100644 docs/api_extension/0001-add-to-xml.patch
delete mode 100644 docs/api_extension/0002-add-new-public-API.patch
delete mode 100644 docs/api_extension/0003-define-internal-driver-API.patch
delete mode 100644 docs/api_extension/0004-implement-the-public-APIs.patch
delete mode 100644 docs/api_extension/0005-implement-the-remote-protocol.patch
delete mode 100644 
docs/api_extension/0006-make-old-API-trivially-wrap-to-new-API.patch
delete mode 100644 docs/api_extension/0007-add-virsh-support.patch
delete mode 100644 docs/api_extension/0008-support-new-xml.patch
delete mode 100644 
docs/api_extension/0009-support-all-flags-in-test-driver.patch
delete mode 100644 
docs/api_extension/0010-improve-vcpu-support-in-qemu-command-line.patch
delete mode 100644 
docs/api_extension/0011-complete-vcpu-support-in-qemu-driver.patch
delete mode 100644 
docs/api_extension/0012-improve-vcpu-support-in-xen-command-line.patch
delete mode 100644 docs/api_extension/0013-improve-getting-xen-vcpu-counts.patch
delete mode 100644 docs/api_extension/0014-improve-setting-xen-vcpu-counts.patch
delete mode 100644 docs/api_extension/0015-remove-dead-xen-code.patch



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 1/5] docs: api_extension: Remove links to the stale example patches

[Ján Tomko]

On Thu, Aug 23, 2018 at 10:50:30AM +0200, Peter Krempa wrote:

The pathches used as an example for the api_extension manual don't hold


*patches, as Erik pointed out


up to the current standards any more. Carefully remove links and
mentions of the patches from the docs.

Signed-off-by: Peter Krempa 
---
docs/api_extension.html.in | 96 +-
1 file changed, 18 insertions(+), 78 deletions(-)

@@ -197,22 +178,16 @@

The public API calls are implemented in:

-src/libvirt.c
-
-See 0004-implement-the-public-APIs.patch
+src/libvirt-$SECTION.c


Unrelated change from libvirt.c to libvirt-$SECTION.c

Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 4/5] docs: api_extension: Remove note to extend libvirt-TCK

[Peter Krempa]

On Thu, Aug 23, 2018 at 15:25:17 +0200, Erik Skultety wrote:
> On Thu, Aug 23, 2018 at 10:50:33AM +0200, Peter Krempa wrote:
> > It probably does not make sense to do tests using that toolkit any more.
> 
> At first I thought so too, I even thought that there hasn't been any commits 
> to
> the repo for a long time, it turned out I was wrong, there's still some work
> going on, so I don't think we should abandon it completely. The fact that I
> (and most probably you too - sorry, I just assumed your testing habits) don't
> use it for testing doesn't mean, we should discourage others from giving the
> framework a try, we're not mandating the usage so I don't see an issue with
> the. With that said, once we have a proper continuous integration running
> upstream, then I'd say we should start encouraging users to use the avocado
> framework instead. However, for the time being, it's a NACK from me, maybe Dan
> has a different feeling about this.

Since the last non-maintenance commit for this section was in 2013, I
don't think that mentioning any particular project makes sense. Avocado
may become obsolete in another 5 years and I doubt that anybody will
bother to update this document for the new hip project's name.

I'm also okay just dropping this and making any poor soul which will
follow that document to resolve any obsolete part by themselves.


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

Re: [libvirt] [PATCH 1/5] docs: api_extension: Remove links to the stale example patches

[Peter Krempa]

On Thu, Aug 23, 2018 at 15:46:30 +0200, Erik Skultety wrote:
> On Thu, Aug 23, 2018 at 10:50:30AM +0200, Peter Krempa wrote:
> > The pathches used as an example for the api_extension manual don't hold
> 
> s/pathches/patches
> 
> > up to the current standards any more. Carefully remove links and
> > mentions of the patches from the docs.
> 
> While I understand the impetus for the change, I am personally not convinced
> that we want to get rid of practical examples as a means of reference to the
> text, it's like a product documentation without examples - "hardcore mode".

Any example will always become obsolete. I find trying to update it to
be a waste of time.

Users are always welcome to inspire from any existing piece of code. We
are open-source in the end. I don't really think we need as much
hand-holding in this area.

> The fact that we took a patch series from the list archives as a reference
> probably wasn't the best thing to do, I think we should instead come up with a
> dummy API exercising all the sections in the docs which will conform to our

Even coding style will become obsolete. Just the fact that we started
using the autofree stuff will make many existing examples obsolete.

> current standards and which we can update as we please. I know that's much 
> more

We don't even keep our coding style guidelines up to date most of the
time. Do you think this would be any different?

> work and effort which I can see you'd refuse to put into this, but I fear that
> once you drop this, we won't update the docs with another examples for
> years/ever and I'm not in favour of such practice.

No. I'll drop the series if it does not get accepted and continue
ignoring this as I've did for many years. I wanted to kill this for
some time (when I was adding syntax-check rule and the patches were
conflicting), but did not even want to bother to do that for at that
time so I've just opted to skip them.

Recent Eric's usage of the patches as an example in his mail discussing
renaming of the files triggered me to get rid of this finally.


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

Re: [libvirt] [PATCH 2/5] docs: api_extension: Remove example patches

[Erik Skultety]

On Thu, Aug 23, 2018 at 10:50:31AM +0200, Peter Krempa wrote:
> Now that they are not linked any more remove them.

See my comment in previous patch.

Erik

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

Re: [libvirt] [PATCH 1/5] docs: api_extension: Remove links to the stale example patches

[Erik Skultety]

On Thu, Aug 23, 2018 at 10:50:30AM +0200, Peter Krempa wrote:
> The pathches used as an example for the api_extension manual don't hold

s/pathches/patches

> up to the current standards any more. Carefully remove links and
> mentions of the patches from the docs.

While I understand the impetus for the change, I am personally not convinced
that we want to get rid of practical examples as a means of reference to the
text, it's like a product documentation without examples - "hardcore mode".
The fact that we took a patch series from the list archives as a reference
probably wasn't the best thing to do, I think we should instead come up with a
dummy API exercising all the sections in the docs which will conform to our
current standards and which we can update as we please. I know that's much more
work and effort which I can see you'd refuse to put into this, but I fear that
once you drop this, we won't update the docs with another examples for
years/ever and I'm not in favour of such practice.

Erik

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

Re: [libvirt] [jenkins-ci PATCH v3 10/12] lcitool: Support building arbitrary branches

[Andrea Bolognani]

On Thu, 2018-08-23 at 14:36 +0200, Erik Skultety wrote:
> On Wed, Aug 22, 2018 at 11:44:25AM +0200, Andrea Bolognani wrote:
> > Building master is useful for CI purposes and to debug CI
> > failures locally, but when developing we want to be able
> > to build a personal branch to validate in-progress changes.
> > 
> > Signed-off-by: Andrea Bolognani 
> > ---
> >  guests/lcitool   | 30 
> >  guests/playbooks/build/jobs/defaults.yml |  1 -
> >  2 files changed, 20 insertions(+), 11 deletions(-)
> > 
> > diff --git a/guests/lcitool b/guests/lcitool
> > index 2901a92..ec81448 100755
> > --- a/guests/lcitool
> > +++ b/guests/lcitool
> > @@ -351,8 +351,13 @@ class Application:
> >  metavar="PROJECTS",
> >  help="list of projects to consider",
> >  )
> > +self._parser.add_argument(
> > +"-b",
> > +metavar="BRANCH",
> > +help="git branch to use when performing builds",
> > +)
> 
> I agree with the idea, but the interface as proposed by the patch is not very
> convenient from user experience POV. The reason for that is having to specify 
> a
> branch without a repo which IMHO feels incomplete, because if you actually 
> need
> to use a custom branch, you most probably are testing a feature which means 
> you
> want to work with your private copy of the upstream repo.
> You can modify the project's config, but that IMHO stops being convenient very
> soon as one day you want to test upstream CI failures locally and then test
> your feature the other, thus having to modify the git_url in the config each
> time. So, we need a quick cmdline override to make this a better interface.

Agreed, the current interface doesn't quite work.

> We
> also might want to have this working when building multiple projects, e.g. I'm
> working on a feature in libvirt and then propagate the necessary changes into
> virt-manager, so I want to build both projects using a custom repo and branch.
> 
> The other thing I quite don't don't agree with is having the cmdline parameter
> mandatory, as a user, I'd expect that if I don't specify the repo and branch,
> the default which we already ship would be used, so my preference would be to
> have it optional.

I'll have to think hard about this. All of the current arguments
are mandatory, so I would like to stick with that unless there's
a compelling reason not to; perhaps in this case it's okay to make
the arguments optional, but maybe all we need to do is provide a
convenient enough shorthand for "build the master branch of the
upstream repository" and the problem will basically disappear.

> Since we already had a private discussion about possible approaches, I'll 
> leave
> summarizing that on the list to you. Nevertheless, the series doesn't really
> depend on this patch and we can already start profiting from what the other
> patches provide and work on top of that.

Yeah, let's drop this patch for now: being able to build upstream's
master branch conveniently is already a good feature to have, and
once we have that in adding more becomes feasible.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 5/5] docs: api_extension: Freshen up paths

[Erik Skultety]

On Thu, Aug 23, 2018 at 10:50:34AM +0200, Peter Krempa wrote:
> Some things were moved. Try to update the paths.

"Do. Or do not. There's no try." ;)

>
> Signed-off-by: Peter Krempa 
> ---
>  docs/api_extension.html.in | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/docs/api_extension.html.in b/docs/api_extension.html.in
> index ce2f457b63..38235a4241 100644
> --- a/docs/api_extension.html.in
> +++ b/docs/api_extension.html.in
> @@ -106,7 +106,7 @@
>libvirt library and call the new function:
>
>  
> -include/libvirt/libvirt.h.in
> +include/libvirt/libvirt-$SECTION.h.in

maybe $MODULE or  would be a better fit instead of $SECTION?

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v4 3/3] vsh: Added tests

[Michal Privoznik]

On 08/22/2018 07:42 PM, Simon Kobyda wrote:
> For now, there are 9 test cases
> - testVshTableNew: Creating table with empty header
> - testVshTableHeader: Printing table with/without header
> - testVshTableRowAppend: Appending row with various number of cells.
>   Only row with same number of cells as in header is accepted.
> - testUnicode: Printing table with unicode characters.
>   Checking correct alignment.
> - testUnicodeArabic: test opposite (right to left) writing
> - testUnicodeZeroWidthChar
> - testUnicodeCombiningChar
> - testUnicodeNonPrintableChar,
> - testNTables: Create and print varios types of tables - one column,
>   one row table, table without content, standart table...
> 
> Signed-off-by: Simon Kobyda 
> ---
>  tests/Makefile.am|   8 +
>  tests/vshtabletest.c | 393 +++
>  2 files changed, 401 insertions(+)
>  create mode 100644 tests/vshtabletest.c
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 21a6c823d9..136fe16f71 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -206,6 +206,7 @@ test_programs = virshtest sockettest \
>   virhostdevtest \
>   virnetdevtest \
>   virtypedparamtest \
> + vshtabletest \
>   $(NULL)
>  
>  test_libraries = libshunload.la \
> @@ -938,6 +939,13 @@ metadatatest_SOURCES = \
>   testutils.c testutils.h
>  metadatatest_LDADD = $(LDADDS) $(LIBXML_LIBS)
>  
> +vshtabletest_SOURCES = \
> + vshtabletest.c \
> + testutils.c testutils.h
> +vshtabletest_LDADD = \
> + $(LDADDS) \
> + ../tools/libvirt_shell.la
> +
>  virshtest_SOURCES = \
>   virshtest.c \
>   testutils.c testutils.h
> diff --git a/tests/vshtabletest.c b/tests/vshtabletest.c
> new file mode 100644
> index 00..48553057d4
> --- /dev/null
> +++ b/tests/vshtabletest.c
> @@ -0,0 +1,393 @@
> +/*
> + * Copyright (C) 2018 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * .
> + */
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#include "internal.h"
> +#include "testutils.h"
> +#include "viralloc.h"
> +#include "../tools/vsh-table.h"
> +
> +static int
> +testVshTableNew(const void *opaque ATTRIBUTE_UNUSED)
> +{
> +if (vshTableNew(NULL)) {
> +fprintf(stderr, "expected failure when passing null to 
> vshTableNew\n");
> +return -1;
> +}
> +
> +return 0;
> +}
> +
> +static int
> +testVshTableHeader(const void *opaque ATTRIBUTE_UNUSED)
> +{
> +int ret = 0;
> +char *act = NULL;
> +const char *exp =
> +" 1   fedora28   running  \n"
> +" 2   rhel7.5running  \n";
> +const char *exp2 =
> +" Id   Name   State\n"
> +"--\n"
> +" 1fedora28   running  \n"
> +" 2rhel7.5running  \n";
> +
> +vshTablePtr table = vshTableNew("Id", "Name", "State",
> +NULL); //to ask about return
> +if (!table)
> +goto cleanup;
> +
> +vshTableRowAppend(table, "1", "fedora28", "running", NULL);
> +vshTableRowAppend(table, "2", "rhel7.5", "running",
> +  NULL);
> +
> +act = vshTablePrintToString(table, false);
> +if (virTestCompareToString(exp, act) < 0)
> +ret = -1;
> +
> +VIR_FREE(act);
> +act = vshTablePrintToString(table, true);
> +if (virTestCompareToString(exp2, act) < 0)
> +ret = -1;
> +
> + cleanup:
> +VIR_FREE(act);
> +vshTableFree(table);
> +return ret;
> +}
> +
> +static int
> +testVshTableRowAppend(const void *opaque ATTRIBUTE_UNUSED)
> +{
> +int ret = 0;
> +
> +vshTablePtr table = vshTableNew("Id", "Name", NULL);
> +if (!table)
> +goto cleanup;
> +
> +if (vshTableRowAppend(table, NULL) >= 0) {
> +fprintf(stderr, "Appending NULL shouldn't work\n");
> +ret = -1;
> +}
> +
> +if (vshTableRowAppend(table, "2", NULL) >= 0) {
> +fprintf(stderr, "Appending less items than in header\n");
> +ret = -1;
> +}
> +
> +if (vshTableRowAppend(table, "2", "rhel7.5", "running",
> +  NULL) >= 0) {
> +fprintf(stderr, "Appending more items than in header\n");
> +ret = -1;
> +}
> +
> +if (vshTableRowAppend(table, "2", "rhel7.5", NULL) < 0) {
> +fprintf(stderr, "Appending same number of items as in header"
> + 

Re: [libvirt] [PATCH 4/5] docs: api_extension: Remove note to extend libvirt-TCK

[Erik Skultety]

On Thu, Aug 23, 2018 at 10:50:33AM +0200, Peter Krempa wrote:
> It probably does not make sense to do tests using that toolkit any more.

At first I thought so too, I even thought that there hasn't been any commits to
the repo for a long time, it turned out I was wrong, there's still some work
going on, so I don't think we should abandon it completely. The fact that I
(and most probably you too - sorry, I just assumed your testing habits) don't
use it for testing doesn't mean, we should discourage others from giving the
framework a try, we're not mandating the usage so I don't see an issue with
the. With that said, once we have a proper continuous integration running
upstream, then I'd say we should start encouraging users to use the avocado
framework instead. However, for the time being, it's a NACK from me, maybe Dan
has a different feeling about this.

Erik

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

Re: [libvirt] [PATCH v4 2/3] virsh: Implement new table API for virsh list

[Michal Privoznik]

On 08/22/2018 07:42 PM, Simon Kobyda wrote:
> Instead of printing it straight in virsh, it creates table struct
> which is filled with header and rows(domains). It allows us to know
> more about table before printing to calculate alignment right.
> 
> Signed-off-by: Simon Kobyda 
> ---
>  tests/virshtest.c| 14 ++--
>  tools/virsh-domain-monitor.c | 43 
>  2 files changed, 31 insertions(+), 26 deletions(-)

ACK

Michal

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

Re: [libvirt] [PATCH v4 1/3] vsh: Add API for printing tables.

[Michal Privoznik]

On 08/22/2018 07:42 PM, Simon Kobyda wrote:
> It solves problems with alignment of columns. Width of each column
> is calculated by its biggest cell. Should solve unicode bug.
> In future, it may be implemented in virsh, virt-admin...
> 
> This API has 5 public functions:
> - vshTableNew - adds new table and defines its header
> - vshTableRowAppend - appends new row (for same number of columns as in
> header)
> - vshTablePrintToStdout
> - vshTablePrintToString
> - vshTableFree
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1574624
> https://bugzilla.redhat.com/show_bug.cgi?id=1584630
> 
> Signed-off-by: Simon Kobyda 
> ---
>  tools/Makefile.am |   4 +-
>  tools/vsh-table.c | 483 ++
>  tools/vsh-table.h |  42 
>  3 files changed, 528 insertions(+), 1 deletion(-)
>  create mode 100644 tools/vsh-table.c
>  create mode 100644 tools/vsh-table.h
> 
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index 1452d984a0..f069167acc 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -144,7 +144,9 @@ libvirt_shell_la_LIBADD = \
>   $(READLINE_LIBS) \
>   ../gnulib/lib/libgnu.la \
>   $(NULL)
> -libvirt_shell_la_SOURCES = vsh.c vsh.h
> +libvirt_shell_la_SOURCES = \
> + vsh.c vsh.h \
> + vsh-table.c vsh-table.h
>  
>  virt_host_validate_SOURCES = \
>   virt-host-validate.c \
> diff --git a/tools/vsh-table.c b/tools/vsh-table.c
> new file mode 100644
> index 00..6e1793e4e3
> --- /dev/null
> +++ b/tools/vsh-table.c
> @@ -0,0 +1,483 @@
> +/*
> + * vsh-table.c: table printing helper
> + *
> + * Copyright (C) 2018 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * .
> + *
> + * Authors:
> + *   Simon Kobyda 
> + *
> + */
> +
> +#include 
> +#include "vsh-table.h"
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "c-ctype.h"
> +
> +#include "viralloc.h"
> +#include "virbuffer.h"
> +#include "virstring.h"
> +#include "virsh-util.h"
> +
> +#define HEX_ENCODE_LENGTH 4 /* represents length of '\xNN' */
> +
> +struct _vshTableRow {
> +char **cells;
> +size_t ncells;
> +};
> +
> +struct _vshTable {
> +vshTableRowPtr *rows;
> +size_t nrows;
> +};
> +
> +static void
> +vshTableRowFree(vshTableRowPtr row)
> +{
> +size_t i;
> +
> +if (!row)
> +return;
> +
> +for (i = 0; i < row->ncells; i++)
> +VIR_FREE(row->cells[i]);
> +
> +VIR_FREE(row->cells);
> +VIR_FREE(row);
> +}
> +
> +void
> +vshTableFree(vshTablePtr table)
> +{
> +size_t i;
> +
> +if (!table)
> +return;
> +
> +for (i = 0; i < table->nrows; i++)
> +vshTableRowFree(table->rows[i]);
> +VIR_FREE(table->rows);
> +VIR_FREE(table);
> +}
> +
> +/**
> + * vshTableRowNew:
> + * @arg: the first argument.
> + * @ap: list of variadic arguments
> + *
> + * Create a new row in the table. Each argument passed
> + * represents a cell in the row.
> + * Return: pointer to vshTableRowPtr row or NULL.

There should be an empty line between the text and "Returns:"

> + */
> +static vshTableRowPtr
> +vshTableRowNew(const char *arg, va_list ap)
> +{
> +vshTableRowPtr row = NULL;
> +char *tmp = NULL;
> +
> +if (!arg) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +_("Table row cannot be empty"));

Misaligned line ;-) Here and on some other places.

> +goto error;
> +}
> +
> +if (VIR_ALLOC(row) < 0)
> +goto error;
> +
> +while (arg) {
> +if (VIR_STRDUP(tmp, arg) < 0)
> +goto error;
> +
> +if (VIR_APPEND_ELEMENT(row->cells, row->ncells, tmp) < 0)
> +goto error;

The @tmp just leaked if VIR_APPEND_ELEMENT() fails. How about:

1) moving the variable declaration into this while() loop,
2) calling VIR_FREE(tmp) just before this goto error;

> +
> +arg = va_arg(ap, const char *);
> +}
> +
> +return row;
> +
> + error:
> +vshTableRowFree(row);
> +return NULL;
> +}
> +
> +/**
> + * vshTableNew:
> + * @arg: List of column names (NULL terminated)
> + *
> + * Create a new table.
> + *
> + * Returns: pointer to table or NULL.
> + */
> +vshTablePtr
> +vshTableNew(const char *arg, ...)
> +{
> +vshTablePtr table;
> +vshTableRowPtr 

Re: [libvirt] [PATCH 3/5] docs: api_extension: Don't encourage other tools than git

[Erik Skultety]

On Thu, Aug 23, 2018 at 10:50:32AM +0200, Peter Krempa wrote:
> Save us hassle in the list if anybody would read this.
>
> Signed-off-by: Peter Krempa 
> ---
Reviewed-by: Erik Skultety 

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

[libvirt] [PATCH] nwfilter: Handle opening for session

[John Ferlan]

https://bugzilla.redhat.com/show_bug.cgi?id=1608275

Commit id 2870419eb (in part) added virGetConnectNWFilter to
allow opening drivers (interface, network, nwfilter, nodedev,
secret, and storage) based on context and commit id f14c37ce4c
started using the API; however, the nwfilterConnectOpen did
not handle session mode resulting in the following message
being logged when virDomainConfVMNWFilterTeardown was called
during the domain shutdown processing:

error : nwfilterConnectOpen:383 : internal error: unexpected
nwfilter URI path '/session', try nwfilter:///system

So similar to the other drivers add code in to check for
/session when not privileged.

Signed-off-by: John Ferlan 
---
 src/nwfilter/nwfilter_driver.c | 19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index ac3a964388..6c25293fd9 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -377,11 +377,20 @@ nwfilterConnectOpen(virConnectPtr conn,
 return VIR_DRV_OPEN_ERROR;
 }
 
-if (STRNEQ(conn->uri->path, "/system")) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("unexpected nwfilter URI path '%s', try 
nwfilter:///system"),
-   conn->uri->path);
-return VIR_DRV_OPEN_ERROR;
+if (driver->privileged) {
+if (STRNEQ(conn->uri->path, "/system")) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("unexpected nwfilter URI path '%s', try 
nwfilter:///system"),
+   conn->uri->path);
+return VIR_DRV_OPEN_ERROR;
+}
+} else {
+if (STRNEQ(conn->uri->path, "/session")) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("unexpected nwfilter URI path '%s', try 
nwfilter:///session"),
+   conn->uri->path);
+return VIR_DRV_OPEN_ERROR;
+}
 }
 
 if (virConnectOpenEnsureACL(conn) < 0)
-- 
2.17.1

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

Re: [libvirt] [jenkins-ci PATCH v3 12/12] lcitool: Document build action

[Erik Skultety]

On Wed, Aug 22, 2018 at 11:44:27AM +0200, Andrea Bolognani wrote:
> Provide some examples and scenarios.
>
> Signed-off-by: Andrea Bolognani 
> ---

Given my comments in patch 10, this will need some tweaking. Otherwise looks
good.

Erik

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

Re: [libvirt] [jenkins-ci PATCH v3 11/12] guests: Support building on more targets

[Erik Skultety]

On Wed, Aug 22, 2018 at 11:44:26AM +0200, Andrea Bolognani wrote:
> The Jenkins build jobs can only run on the subset of
> guests that are available on CentOS CI, but when we're
> running build jobs through lcitool we don't have that
> limitation and we can build on more targets.
>
> Signed-off-by: Andrea Bolognani 
> ---
Reviewed-by: Erik Skultety 

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

Re: [libvirt] [jenkins-ci PATCH v3 10/12] lcitool: Support building arbitrary branches

[Erik Skultety]

On Wed, Aug 22, 2018 at 11:44:25AM +0200, Andrea Bolognani wrote:
> Building master is useful for CI purposes and to debug CI
> failures locally, but when developing we want to be able
> to build a personal branch to validate in-progress changes.
>
> Signed-off-by: Andrea Bolognani 
> ---
>  guests/lcitool   | 30 
>  guests/playbooks/build/jobs/defaults.yml |  1 -
>  2 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/guests/lcitool b/guests/lcitool
> index 2901a92..ec81448 100755
> --- a/guests/lcitool
> +++ b/guests/lcitool
> @@ -351,8 +351,13 @@ class Application:
>  metavar="PROJECTS",
>  help="list of projects to consider",
>  )
> +self._parser.add_argument(
> +"-b",
> +metavar="BRANCH",
> +help="git branch to use when performing builds",
> +)

I agree with the idea, but the interface as proposed by the patch is not very
convenient from user experience POV. The reason for that is having to specify a
branch without a repo which IMHO feels incomplete, because if you actually need
to use a custom branch, you most probably are testing a feature which means you
want to work with your private copy of the upstream repo.
You can modify the project's config, but that IMHO stops being convenient very
soon as one day you want to test upstream CI failures locally and then test
your feature the other, thus having to modify the git_url in the config each
time. So, we need a quick cmdline override to make this a better interface. We
also might want to have this working when building multiple projects, e.g. I'm
working on a feature in libvirt and then propagate the necessary changes into
virt-manager, so I want to build both projects using a custom repo and branch.

The other thing I quite don't don't agree with is having the cmdline parameter
mandatory, as a user, I'd expect that if I don't specify the repo and branch,
the default which we already ship would be used, so my preference would be to
have it optional.

Since we already had a private discussion about possible approaches, I'll leave
summarizing that on the list to you. Nevertheless, the series doesn't really
depend on this patch and we can already start profiting from what the other
patches provide and work on top of that.

Erik

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

Re: [libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address

[Andrea Bolognani]

On Thu, 2018-08-23 at 13:01 +0200, Andrea Bolognani wrote:
> I also think we might be at a point where enough big changes have
> been made that the best way forward is probably to post a v3 that
> includes those and then use that as the basis for more fine-tuning
> for minor stuff such as error messages and the like. Does that
> sound reasonable?

Of course I meant to say v4, not v3, above :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] nwfilter: Handle libvirtd restart if nwfilter binding deleted

[John Ferlan]

On 08/23/2018 07:27 AM, Daniel P. Berrangé wrote:
> On Wed, Aug 22, 2018 at 05:43:21PM -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1607202
>>
>> It's stated that if the admin wants to shoot themselves in
>> the foot by removing the nwfilter binding while the domain
> 
> So based on your explanation in the other reply, this message
> is what was misleading me. s/nwfilter binding/nwfilter/
> 

Actually perhaps it's more by "first removing the nwfilter binding and
subsequently undefining the nwfilter that is/was in use for the running
guest..."

If just the nwfilter binding was removed, then libvirtd restart would
have been fine because it would have recreated the nwfilter binding. Of
course that may not be expected either...

>> is running we will certainly allow that.  However, in doing
>> so we also run the risk that a libvirtd restart will cause
>> the domain to be shutdown, which isn't a good thing.
>>
>> So add another boolean to virDomainConfNWFilterInstantiate
>> which allows us to recover somewhat gracefully in the event
>> the virNWFilterBindingCreateXML fails when we come from
>> qemuProcessReconnect and we determine that the filter has
>> been deleted. It was there at some point (it had to be), but
>> if it's missing, then we don't want to cause the guest to
>> stop running, so issue a warning and continue on.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/conf/domain_nwfilter.c | 33 -
>>  src/conf/domain_nwfilter.h |  3 ++-
>>  src/lxc/lxc_process.c  |  3 ++-
>>  src/qemu/qemu_hotplug.c|  7 ---
>>  src/qemu/qemu_interface.c  |  6 --
>>  src/qemu/qemu_process.c| 10 +++---
>>  src/uml/uml_conf.c |  3 ++-
>>  7 files changed, 49 insertions(+), 16 deletions(-)
> 
> [snip]
> 
>>  static int
>> -qemuProcessFiltersInstantiate(virDomainDefPtr def, bool ignoreExists)
>> +qemuProcessFiltersInstantiate(virDomainDefPtr def,
>> +  bool ignoreExists,
>> +  bool ignoreDeleted)
>>  {
>>  size_t i;
>>  
>>  for (i = 0; i < def->nnets; i++) {
>>  virDomainNetDefPtr net = def->nets[i];
>>  if ((net->filter) && (net->ifname)) {
>> -if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net, 
>> ignoreExists) < 0)
>> +if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net,
>> + ignoreExists,
>> + ignoreDeleted) < 0)
>>  return 1;
>>  }
> 
> Rather than this extra "ignoreDeleted" arg, why can't we just do
> 
>if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net,
>  ignoreExists) < 0 &&
>ignoreDeleted)
> return 1;   
> 
> This ensures that all things which can cause a nwfilter binding failure
> on startup will be handled by avoiding tearing down the running guest.
> 

Did you mean !ignoreDeleted? There's only one caller to
qemuProcessFiltersInstantiate which does:

if (qemuProcessFiltersInstantiate(obj->def, true))
goto error;

Of course what's the purpose of distinguishing between ignoreExists and
ignoreDeleted then? Essentially what you're indicating is we wouldn't
care about any error because virDomainConfNWFilterInstantiate wouldn't
be distinguishing between any error (because there's only one caller to
qemuProcessFiltersInstantiate).

I could change the last argument to virDomainConfNWFilterInstantiate to
be a flag instead of a bool so that if we have future errors we care to
ignore we don't keep adding bool arguments, but that's just a
implementation detail.

Again, I could go back and figure out a way for the *Delete path to
remove the live net->filter based on the path taken so that when we get
here we wouldn't find the net->filter and thus wouldn't call the
instantiate function. It's just a different approach, but I'm hesitant
mainly because then one wouldn't be able to use the *CreateXML API to
reinstate the filter. I'm not as clear about all the (wacko) use cases.

John

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

Re: [libvirt] [PATCH] nwfilter: Handle libvirtd restart if nwfilter binding deleted

[Daniel P . Berrangé]

On Wed, Aug 22, 2018 at 05:43:21PM -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1607202
> 
> It's stated that if the admin wants to shoot themselves in
> the foot by removing the nwfilter binding while the domain

So based on your explanation in the other reply, this message
is what was misleading me. s/nwfilter binding/nwfilter/

> is running we will certainly allow that.  However, in doing
> so we also run the risk that a libvirtd restart will cause
> the domain to be shutdown, which isn't a good thing.
> 
> So add another boolean to virDomainConfNWFilterInstantiate
> which allows us to recover somewhat gracefully in the event
> the virNWFilterBindingCreateXML fails when we come from
> qemuProcessReconnect and we determine that the filter has
> been deleted. It was there at some point (it had to be), but
> if it's missing, then we don't want to cause the guest to
> stop running, so issue a warning and continue on.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/domain_nwfilter.c | 33 -
>  src/conf/domain_nwfilter.h |  3 ++-
>  src/lxc/lxc_process.c  |  3 ++-
>  src/qemu/qemu_hotplug.c|  7 ---
>  src/qemu/qemu_interface.c  |  6 --
>  src/qemu/qemu_process.c| 10 +++---
>  src/uml/uml_conf.c |  3 ++-
>  7 files changed, 49 insertions(+), 16 deletions(-)

[snip]

>  static int
> -qemuProcessFiltersInstantiate(virDomainDefPtr def, bool ignoreExists)
> +qemuProcessFiltersInstantiate(virDomainDefPtr def,
> +  bool ignoreExists,
> +  bool ignoreDeleted)
>  {
>  size_t i;
>  
>  for (i = 0; i < def->nnets; i++) {
>  virDomainNetDefPtr net = def->nets[i];
>  if ((net->filter) && (net->ifname)) {
> -if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net, 
> ignoreExists) < 0)
> +if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net,
> + ignoreExists,
> + ignoreDeleted) < 0)
>  return 1;
>  }

Rather than this extra "ignoreDeleted" arg, why can't we just do

   if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net,
 ignoreExists) < 0 &&
 ignoreDeleted)
return 1;   

This ensures that all things which can cause a nwfilter binding failure
on startup will be handled by avoiding tearing down the running guest.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH] nwfilter: Handle libvirtd restart if nwfilter binding deleted

[John Ferlan]

On 08/23/2018 03:44 AM, Daniel P. Berrangé wrote:
> On Wed, Aug 22, 2018 at 05:43:21PM -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1607202
>>
>> It's stated that if the admin wants to shoot themselves in
>> the foot by removing the nwfilter binding while the domain
>> is running we will certainly allow that.  However, in doing
>> so we also run the risk that a libvirtd restart will cause
>> the domain to be shutdown, which isn't a good thing.
>>
>> So add another boolean to virDomainConfNWFilterInstantiate
>> which allows us to recover somewhat gracefully in the event
>> the virNWFilterBindingCreateXML fails when we come from
>> qemuProcessReconnect and we determine that the filter has
>> been deleted. It was there at some point (it had to be), but
>> if it's missing, then we don't want to cause the guest to
>> stop running, so issue a warning and continue on.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/conf/domain_nwfilter.c | 33 -
>>  src/conf/domain_nwfilter.h |  3 ++-
>>  src/lxc/lxc_process.c  |  3 ++-
>>  src/qemu/qemu_hotplug.c|  7 ---
>>  src/qemu/qemu_interface.c  |  6 --
>>  src/qemu/qemu_process.c| 10 +++---
>>  src/uml/uml_conf.c |  3 ++-
>>  7 files changed, 49 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c
>> index f39c8a1f9b..3e6e462def 100644
>> --- a/src/conf/domain_nwfilter.c
>> +++ b/src/conf/domain_nwfilter.c
>> @@ -85,16 +85,19 @@ int
>>  virDomainConfNWFilterInstantiate(const char *vmname,
>>   const unsigned char *vmuuid,
>>   virDomainNetDefPtr net,
>> - bool ignoreExists)
>> + bool ignoreExists,
>> + bool ignoreDeleted)
>>  {
>>  virConnectPtr conn = virGetConnectNWFilter();
>>  virNWFilterBindingDefPtr def = NULL;
>>  virNWFilterBindingPtr binding = NULL;
>> +virNWFilterPtr nwfilter = NULL;
>>  char *xml = NULL;
>>  int ret = -1;
>>  
>> -VIR_DEBUG("vmname=%s portdev=%s filter=%s ignoreExists=%d",
>> -  vmname, NULLSTR(net->ifname), NULLSTR(net->filter), 
>> ignoreExists);
>> +VIR_DEBUG("vmname=%s portdev=%s filter=%s ignoreExists=%d 
>> ignoreDeleted=%d",
>> +  vmname, NULLSTR(net->ifname), NULLSTR(net->filter),
>> +  ignoreExists, ignoreDeleted);
>>  
>>  if (!conn)
>>  goto cleanup;
>> @@ -113,14 +116,34 @@ virDomainConfNWFilterInstantiate(const char *vmname,
>>  if (!(xml = virNWFilterBindingDefFormat(def)))
>>  goto cleanup;
>>  
>> -if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0)))
>> -goto cleanup;
>> +if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0))) {
>> +virErrorPtr orig_err;
>> +
>> +if (!ignoreDeleted)
>> +goto cleanup;
>> +
>> +/* Let's determine if the error was because the filter was deleted.
>> + * Save the orig_err just in case it's not a failure to find the
>> + * filter by name. */
>> +orig_err = virSaveLastError();
>> +nwfilter = virNWFilterLookupByName(conn, def->filter);
>> +virSetError(orig_err);
>> +virFreeError(orig_err);
>> +if (nwfilter)
>> +goto cleanup;
>> +
>> +VIR_WARN("filter '%s' for binding '%s' has been deleted while the "
>> + "guest was running, ignoring for restart processing",
>> + def->filter, def->portdevname);
>> +virResetLastError();
>> +}
> 
> I don't get what this code is trying to do. This method is about creating
> nwfilter bindings. If virNWFilterBindingCreateXML() fails, that means the
> filter binding already exists. But the stated problem was that the admin
> had deleted the filter binding. The code is also checking a filter, not
> a filter binding.
> 

virNWFilterBindingCreateXML eventually calls nwfilterBindingCreateXML,
which will virNWFilterBindingObjListAdd the binding, get the @ret
binding and attempt to virNWFilterInstantiateFilter the binding @def.

Instantiate will call virNWFilterInstantiateFilterInternal which calls
virNWFilterInstantiateFilterUpdate (eventually) which calls
virNWFilterObjListFindInstantiateFilter because someone deleted the
filter after deleting the portdev before hand.

As the bz shows, one cannot delete the filter because it's active in the
portdev; however, deleting the portdev allows one to delete the filter.
Once the port is deleted, the 'ebtables -t nat -L' will show it's gone.

Attempting to create the portdev again when libvirtd restarts (because
virDomainConfNWFilterInstantiate is called during that processing) will
cause a failure and the guest will be stopped.

Prior to the changes, adding a debug print in nwfilterBindingCreateXML
of the XML I see the following in a debug libvirtd session restart:

Re: [libvirt] [PATCH v3 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes

[Andrea Bolognani]

On Thu, 2018-08-23 at 16:46 +0800, Yi Min Zhao wrote:
> 在 2018/8/23 下午4:12, Andrea Bolognani 写道:
> > This got me thinking that the extension flags *also* belongs to
> > virPCIDeviceAddress, since we need them to know whether the zPCI
> > part should be taken into account when formatting and such: you
> > used to check whether the zpci pointer was NULL, but of course you
> > can no longer do that once the pointer is gone; moreover, checking
> > for a flag is more explicit so that's also an improvement. Using
> > the flags stored into virDomainDeviceInfo would be ugly because it
> > would make it so virPCIDeviceAddress is no longer a stand-alone
> > object, so we should avoid that.
> > 
> > I'm not sure you can get away with not storing the extension flags
> > in virDomainDeviceInfo, though, because IIRC you might need to use
> > them *before* you have figured out that you're going to assign a
> > PCI address to the device. We might just have to store them twice,
> > which is not great but I think preferable to introducing a reverse
> > dependency from virPCIDeviceAddress to virDomainDeviceInfo. But I
> > haven't really looked too closely into it, so perhaps there's a
> > way to avoid that duplication after all :)
> 
> I think it's enough to store extension flags in virDomainDeviceInfo in 
> my new code.

As explained above, that would cause virPCIDeviceAddress to no
longer be usable on its own, which I don't think is a good idea.

If you have to store the flags in virDomainDeviceInfo for address
allocation purposes that's fine, but once address allocation has
been performed you should no longer need to look at those and
should use virPCIDeviceAddress on its own instead.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address

[Andrea Bolognani]

On Thu, 2018-08-23 at 18:01 +0800, Yi Min Zhao wrote:
> 在 2018/8/22 下午5:56, Andrea Bolognani 写道:
> > The patches I said I'd write are now on the list:
> > 
> >https://www.redhat.com/archives/libvir-list/2018-August/msg01105.html
> > 
> > No reviews yet, we'll see whether they get ACKed or NACKed :)
> 
> I saw them. Could I give my ACKed?

Feel free to do that, but before pushing I'd like to have Laine's
ACK since he's the one who introduced the functions in the first
place.

> I have a question about your previous comment about error report.
> You thought we should report more specific information.
> 
> > +virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci;
> > +
> > +if (zpci && !dev->pciAddressExtFlags) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not 
> > supported."));
> > +return -1;
> > +}
> 
> It's called by all device types which possibly use PCI address.
> I'm not sure how to report device's name in error string.

I think reporting the address is enough. The idea is to give the
user some information they can use to figure out which device is
misconfigured rather than just telling them there is an issue and
leaving it at that.

I also think we might be at a point where enough big changes have
been made that the best way forward is probably to post a v3 that
includes those and then use that as the basis for more fine-tuning
for minor stuff such as error messages and the like. Does that
sound reasonable?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [jenkins-ci PATCH v3 08/12] lcitool: Make playbook execution generic

[Erik Skultety]

On Wed, Aug 22, 2018 at 11:44:23AM +0200, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v3 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes

[Yi Min Zhao]

在 2018/8/23 下午6:01, Andrea Bolognani 写道:

On Thu, 2018-08-23 at 17:15 +0800, Yi Min Zhao wrote:

在 2018/8/23 下午4:12, Andrea Bolognani 写道:

Exactly. You can either just add zpci_uid and zpci_fid to the
virPCIDeviceAddress struct, or have

struct _virZPCIDeviceAddress {
unsigned int uid;
unsigned int fid;
};

struct _virPCIDeviceAddress {
unsigned int domain;
unsigned int bus;
unsigned int slot;
unsigned int function;
int multi; /* virTristateSwitch */
virZPCIDeviceAddress zpci;
};

There's an error in syntax-check. I think it's from common code.

src/util/virpci.h:47:unsigned int uid;
maint.mk: use pid_t for pid, uid_t for uid, gid_t for gid

I think it mistakes zpci's uid.

Easy enough to fix: just apply the patch below, change the
struct definition to

 struct _virZPCIDeviceAddress {
 unsigned int uid; /* exempt from syntax-check */
 unsigned int fid;
 };

and it will go away :)


diff --git a/cfg.mk b/cfg.mk
index 609ae869c2..1116feb299 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -472,6 +472,7 @@ sc_prohibit_canonicalize_file_name:
  # Insist on correct types for [pug]id.
  sc_correct_id_types:
 @prohibit='\<(int|long) *[pug]id\>' \
+   exclude='exempt from syntax-check' \
 halt='use pid_t for pid, uid_t for uid, gid_t for gid' \
   $(_sc_search_regexp)


Thanks for your help! I will look into this. Never meet this before.

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

Re: [libvirt] [Qemu-devel] [PATCH v2 0/3] ui: remove deprecated UI frontends

[Andrea Bolognani]

On Wed, 2018-08-22 at 14:15 +0100, Daniel P. Berrangé wrote:
> We deprecated GTK2 and SDL1.2 in the 2.12.0 release, so they are able to
> be removed entirely in the 3.1.0 release. The min GTK3 version can also
> be bumped up based the distros we aim to support.
> 
> Note that before this can merge, the openbsd VM test image needs to be
> updated to have SDL2, as openbsd build mandates SDL availability.
> 
> The freebsd & netbsd images should also be updated, but they are not
> build blockers as they automatically disable SDL
> 
> Changed in v2:
> 
>  - Rebased to resolve conflicts
> 
> Daniel P. Berrangé (3):
>   ui: remove support for GTK2 in favour of GTK3
>   ui: increase min required GTK3 version to 3.14.0
>   ui: remove support for SDL1.2 in favour of SDL2

Nit: you might want to tweak some parts of the commit messages,
because they're no longer accurate...

> The GTK 3.0 release was made in Feb, 2011:
> 
>   https://blog.gtk.org/2011/02/10/gtk-3-0-released/
> 
> That will soon be 7 years ago, which is enough time to consider
> the 3.x series widely supported.

It's been more than 7 years now.

> The SDL 2.0 release was made in Aug, 2013:
> 
>   https://www.libsdl.org/release/
> 
> That will soon be 4 + 1/2 years ago, which is enough time to consider
> the 2.0 series widely supported.

It's been more than 5 years now.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address

[Yi Min Zhao]

在 2018/8/22 下午5:56, Andrea Bolognani 写道:

On Wed, 2018-08-22 at 11:00 +0800, Yi Min Zhao wrote:

在 2018/8/17 上午12:06, Andrea Bolognani 写道:

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]

+static inline bool
+virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info)
+{
+return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+info->addr.pci.zpci;
+}

This should be called virDeviceInfoPCIAddressExtensionIsPresent()
since it's a predicate. I know the corresponding PCI function gets
the naming wrong, but that doesn't mean you should too :)

In the same vein, I don't think this (or the PCI version, for that
matter) need to be inline... I'd rather have them both as regular
functions in the .c file. I'll probably cook up a patch cleaning
up the PCI part later, see what the feedback is.

Got it. Thank!

The patches I said I'd write are now on the list:

   https://www.redhat.com/archives/libvir-list/2018-August/msg01105.html

No reviews yet, we'll see whether they get ACKed or NACKed :)

I saw them. Could I give my ACKed?



[...]

+static int
+virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
+virZPCIDeviceAddressPtr addr)
+{
+if (!addr->uid_assigned &&
+virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) {
+return -1;
+}
+
+if (!addr->fid_assigned &&
+virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) {
+virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
+return -1;
+}

Not sure I get the logic here... ReserveNextAddress() is supposed
to pick the next available address and reserve it, but IIUC this
will skip picking either id based on whether they were assigned.

If uid/fid is assigned, we call ***Reserve***().
If uid/fid is unassigned, we call ***ReserveNext***().

But I'm not very clear about your concern.

That in ReserveNext() you're checking whether addr->*id_assigned
when you know that ids have not been assigned or you wouldn't have
called ReserveNext() in the first place... It seems unnecessary
and confusing to me, so unless I've missed something that makes it
necessary please just drop those checks.

As our discussion on the 1st patch, boolean values are removed.
So we don't need checks here.



[...]

+static int
+virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
+   virDomainDeviceInfoPtr dev)
+{

It's weird that this function doesn't get extFlags as an argument,
unlike the other ones you've introduced. Better make it consistent.

We have to pass DeviceInfo which already has extFlags. If we pass
extFlags again,
isn't it redundant? This function is only called in one place for
hotplug case.

I wanted the API to be more consistent but I realize now you have
to pass either virPCIDeviceAddress or virDomainDeviceInfo depending
on the context, so it doesn't really matter, you can leave it as it
is. The signatures for the corresponding PCI functions are not
entirely consistent either :)


I have a question about your previous comment about error report.
You thought we should report more specific information.


+virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci;
+
+if (zpci && !dev->pciAddressExtFlags) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not 
supported."));
+return -1;
+}


It's called by all device types which possibly use PCI address.
I'm not sure how to report device's name in error string.

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

Re: [libvirt] [PATCH v3 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes

[Andrea Bolognani]

On Thu, 2018-08-23 at 17:15 +0800, Yi Min Zhao wrote:
> 
> 在 2018/8/23 下午4:12, Andrea Bolognani 写道:
> > Exactly. You can either just add zpci_uid and zpci_fid to the
> > virPCIDeviceAddress struct, or have
> > 
> >struct _virZPCIDeviceAddress {
> >unsigned int uid;
> >unsigned int fid;
> >};
> > 
> >struct _virPCIDeviceAddress {
> >unsigned int domain;
> >unsigned int bus;
> >unsigned int slot;
> >unsigned int function;
> >int multi; /* virTristateSwitch */
> >virZPCIDeviceAddress zpci;
> >};
> 
> There's an error in syntax-check. I think it's from common code.
> 
> src/util/virpci.h:47:unsigned int uid;
> maint.mk: use pid_t for pid, uid_t for uid, gid_t for gid
> 
> I think it mistakes zpci's uid.

Easy enough to fix: just apply the patch below, change the
struct definition to

struct _virZPCIDeviceAddress {
unsigned int uid; /* exempt from syntax-check */
unsigned int fid;
};

and it will go away :)


diff --git a/cfg.mk b/cfg.mk
index 609ae869c2..1116feb299 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -472,6 +472,7 @@ sc_prohibit_canonicalize_file_name:
 # Insist on correct types for [pug]id.
 sc_correct_id_types:
@prohibit='\<(int|long) *[pug]id\>' \
+   exclude='exempt from syntax-check' \
halt='use pid_t for pid, uid_t for uid, gid_t for gid' \
  $(_sc_search_regexp)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PULL 02/12] vnc: fix memleak of the "vnc-worker-output" name

[Gerd Hoffmann]

From: Peter Wu 

Fixes repeated memory leaks of 18 bytes when using VNC:

Direct leak of 831024 byte(s) in 46168 object(s) allocated from:
...
#4 0x7f6d2f919bdd in g_strdup_vprintf glib/gstrfuncs.c:514
#5 0x56085cdcf660 in buffer_init util/buffer.c:59
#6 0x56085ca6a7ec in vnc_async_encoding_start ui/vnc-jobs.c:177
#7 0x56085ca6b815 in vnc_worker_thread_loop ui/vnc-jobs.c:240

Fixes: 543b95801f98 ("vnc: attach names to buffers")
Cc: Gerd Hoffmann 
CC: qemu-sta...@nongnu.org
Signed-off-by: Peter Wu 
Reviewed-by: Marc-André Lureau 
Message-id: 20180807221830.3844-1-pe...@lekensteyn.nl
Signed-off-by: Gerd Hoffmann 
---
 ui/vnc-jobs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index b0b15d42a8..929391f85d 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -193,6 +193,7 @@ static void vnc_async_encoding_start(VncState *orig, 
VncState *local)
 
 static void vnc_async_encoding_end(VncState *orig, VncState *local)
 {
+buffer_free(>output);
 orig->tight = local->tight;
 orig->zlib = local->zlib;
 orig->hextile = local->hextile;
@@ -278,7 +279,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
 /* Copy persistent encoding data */
 vnc_async_encoding_end(job->vs, );
 
-   qemu_bh_schedule(job->vs->bh);
+qemu_bh_schedule(job->vs->bh);
 }  else {
 buffer_reset();
 /* Copy persistent encoding data */
-- 
2.9.3

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

[libvirt] [PULL 07/12] spice-display: access ptr_x/ptr_y under Mutex

[Gerd Hoffmann]

From: Paolo Bonzini 

The OpenGL-enabled SPICE code was not accessing the cursor position
under the SimpleSpiceDisplay lock.  Fix this.

Signed-off-by: Paolo Bonzini 
Reviewed-by: Marc-André Lureau 
Message-id: 20180720063109.4631-2-pbonz...@redhat.com
Signed-off-by: Gerd Hoffmann 
---
 ui/spice-display.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index fe734821dd..46df673cd7 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -976,8 +976,10 @@ static void 
qemu_spice_gl_cursor_position(DisplayChangeListener *dcl,
 {
 SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
 
+qemu_mutex_lock(>lock);
 ssd->ptr_x = pos_x;
 ssd->ptr_y = pos_y;
+qemu_mutex_unlock(>lock);
 }
 
 static void qemu_spice_gl_release_dmabuf(DisplayChangeListener *dcl,
@@ -1055,10 +1057,15 @@ static void qemu_spice_gl_update(DisplayChangeListener 
*dcl,
 }
 
 if (render_cursor) {
+int x, y;
+qemu_mutex_lock(>lock);
+x = ssd->ptr_x;
+y = ssd->ptr_y;
+qemu_mutex_unlock(>lock);
 egl_texture_blit(ssd->gls, >blit_fb, >guest_fb,
  !y_0_top);
 egl_texture_blend(ssd->gls, >blit_fb, >cursor_fb,
-  !y_0_top, ssd->ptr_x, ssd->ptr_y);
+  !y_0_top, x, y);
 glFlush();
 }
 
-- 
2.9.3

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

[libvirt] [PULL 04/12] sdl2: redraw correctly when scanout_mode enabled.

[Gerd Hoffmann]

From: Tao Wu 

When scanout_mode enabled, surface is out of sync with actual screen.
In such case, we just call sdl2_gl_scanout_flush to do redraw. This
fixes bug reported in
https://lists.freedesktop.org/archives/virglrenderer-devel/2018-July/001330.html

Signed-off-by: Tao Wu 
Message-id: 20180726225900.180698-1-lep...@google.com
Signed-off-by: Gerd Hoffmann 
---
 ui/sdl2-gl.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/ui/sdl2-gl.c b/ui/sdl2-gl.c
index 83b71853d1..1bf4542d8d 100644
--- a/ui/sdl2-gl.c
+++ b/ui/sdl2-gl.c
@@ -124,6 +124,11 @@ void sdl2_gl_redraw(struct sdl2_console *scon)
 {
 assert(scon->opengl);
 
+if (scon->scanout_mode) {
+/* sdl2_gl_scanout_flush actually only care about
+ * the first argument. */
+return sdl2_gl_scanout_flush(>dcl, 0, 0, 0, 0);
+}
 if (scon->surface) {
 sdl2_gl_render_surface(scon);
 }
-- 
2.9.3

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

[libvirt] [PULL 12/12] util: promote qemu_egl_rendernode_open() to libqemuutil

[Gerd Hoffmann]

From: Marc-André Lureau 

vhost-user-gpu will share the same code to open a DRM node.

Signed-off-by: Marc-André Lureau 
Message-Id: <20180713130916.4153-20-marcandre.lur...@redhat.com>
Signed-off-by: Gerd Hoffmann 
---
 include/qemu/drm.h |  6 +
 ui/egl-helpers.c   | 51 ++---
 util/drm.c | 66 ++
 MAINTAINERS|  1 +
 util/Makefile.objs |  1 +
 5 files changed, 76 insertions(+), 49 deletions(-)
 create mode 100644 include/qemu/drm.h
 create mode 100644 util/drm.c

diff --git a/include/qemu/drm.h b/include/qemu/drm.h
new file mode 100644
index 00..4c3e622f5c
--- /dev/null
+++ b/include/qemu/drm.h
@@ -0,0 +1,6 @@
+#ifndef QEMU_DRM_H_
+#define QEMU_DRM_H_
+
+int qemu_drm_rendernode_open(const char *rendernode);
+
+#endif
diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 71b6a97bd1..4f475142fc 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -15,9 +15,7 @@
  * License along with this library; if not, see .
  */
 #include "qemu/osdep.h"
-#include 
-#include 
-
+#include "qemu/drm.h"
 #include "qemu/error-report.h"
 #include "ui/console.h"
 #include "ui/egl-helpers.h"
@@ -147,57 +145,12 @@ int qemu_egl_rn_fd;
 struct gbm_device *qemu_egl_rn_gbm_dev;
 EGLContext qemu_egl_rn_ctx;
 
-static int qemu_egl_rendernode_open(const char *rendernode)
-{
-DIR *dir;
-struct dirent *e;
-int r, fd;
-char *p;
-
-if (rendernode) {
-return open(rendernode, O_RDWR | O_CLOEXEC | O_NOCTTY | O_NONBLOCK);
-}
-
-dir = opendir("/dev/dri");
-if (!dir) {
-return -1;
-}
-
-fd = -1;
-while ((e = readdir(dir))) {
-if (e->d_type != DT_CHR) {
-continue;
-}
-
-if (strncmp(e->d_name, "renderD", 7)) {
-continue;
-}
-
-p = g_strdup_printf("/dev/dri/%s", e->d_name);
-
-r = open(p, O_RDWR | O_CLOEXEC | O_NOCTTY | O_NONBLOCK);
-if (r < 0) {
-g_free(p);
-continue;
-}
-fd = r;
-g_free(p);
-break;
-}
-
-closedir(dir);
-if (fd < 0) {
-return -1;
-}
-return fd;
-}
-
 int egl_rendernode_init(const char *rendernode, DisplayGLMode mode)
 {
 qemu_egl_rn_fd = -1;
 int rc;
 
-qemu_egl_rn_fd = qemu_egl_rendernode_open(rendernode);
+qemu_egl_rn_fd = qemu_drm_rendernode_open(rendernode);
 if (qemu_egl_rn_fd == -1) {
 error_report("egl: no drm render node available");
 goto err;
diff --git a/util/drm.c b/util/drm.c
new file mode 100644
index 00..a23ff24538
--- /dev/null
+++ b/util/drm.c
@@ -0,0 +1,66 @@
+/*
+ * Copyright (C) 2015-2016 Gerd Hoffmann 
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+#include "qemu/osdep.h"
+#include "qemu/drm.h"
+
+#include 
+#include 
+
+int qemu_drm_rendernode_open(const char *rendernode)
+{
+DIR *dir;
+struct dirent *e;
+int r, fd;
+char *p;
+
+if (rendernode) {
+return open(rendernode, O_RDWR | O_CLOEXEC | O_NOCTTY | O_NONBLOCK);
+}
+
+dir = opendir("/dev/dri");
+if (!dir) {
+return -1;
+}
+
+fd = -1;
+while ((e = readdir(dir))) {
+if (e->d_type != DT_CHR) {
+continue;
+}
+
+if (strncmp(e->d_name, "renderD", 7)) {
+continue;
+}
+
+p = g_strdup_printf("/dev/dri/%s", e->d_name);
+
+r = open(p, O_RDWR | O_CLOEXEC | O_NOCTTY | O_NONBLOCK);
+if (r < 0) {
+g_free(p);
+continue;
+}
+fd = r;
+g_free(p);
+break;
+}
+
+closedir(dir);
+if (fd < 0) {
+return -1;
+}
+return fd;
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index 6902a568f4..282d6a8ae5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1566,6 +1566,7 @@ S: Odd Fixes
 F: ui/
 F: include/ui/
 F: qapi/ui.json
+F: util/drm.c
 
 Cocoa graphics
 M: Peter Maydell 
diff --git a/util/Makefile.objs b/util/Makefile.objs
index e1c3fed4dc..1810f970ef 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -49,3 +49,4 @@ util-obj-y += stats64.o
 util-obj-y += systemd.o
 util-obj-y += iova-tree.o
 util-obj-$(CONFIG_LINUX) += vfio-helpers.o
+util-obj-$(CONFIG_LINUX) += drm.o
-- 
2.9.3

--
libvir-list mailing list

[libvirt] [PULL 00/12] Ui 20180823 v3 patches

[Gerd Hoffmann]

The following changes since commit d0092d90eb546a8bbe9e9120426c189474123797:

  Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20180820' into 
staging (2018-08-20 17:41:18 +0100)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/ui-20180823-v3-pull-request

for you to fetch changes up to 63cde1d13d0767f778a130e2c93c15ad709e1276:

  util: promote qemu_egl_rendernode_open() to libqemuutil (2018-08-23 11:53:26 
+0200)


ui: misc fixes which piled up during 3.0 release freeze



Daniel P. Berrangé (2):
  doc: switch to modern syntax for VNC TLS setup
  vnc: remove support for deprecated tls, x509, x509verify options

Marc-André Lureau (3):
  ui: use enum to string helpers
  dmabuf: add y0_top, pass it to spice
  util: promote qemu_egl_rendernode_open() to libqemuutil

Paolo Bonzini (2):
  spice-display: access ptr_x/ptr_y under Mutex
  spice-display: fix qemu_spice_cursor_refresh_bh locking

Peter Wu (1):
  vnc: fix memleak of the "vnc-worker-output" name

Philippe Mathieu-Daudé (1):
  ui/vnc: Remove useless parenthesis around DIV_ROUND_UP macro

Tao Wu (1):
  sdl2: redraw correctly when scanout_mode enabled.

Thomas Huth (2):
  ui/sdl2: Remove the obsolete SDL_INIT_NOPARACHUTE flag
  ui/sdl2: Fix broken -full-screen CLI option

 include/qemu/drm.h   |  6 
 include/ui/console.h |  1 +
 qemu-keymap.c|  2 +-
 ui/console.c |  6 ++--
 ui/egl-helpers.c | 51 ++--
 ui/sdl2-gl.c |  5 +++
 ui/sdl2.c| 13 +++-
 ui/spice-display.c   | 42 +++
 ui/vnc-enc-tight.c   |  2 +-
 ui/vnc-jobs.c|  3 +-
 ui/vnc.c | 94 ++--
 util/drm.c   | 66 
 MAINTAINERS  |  1 +
 qemu-deprecated.texi | 20 ---
 qemu-doc.texi| 20 ---
 qemu-options.hx  | 43 
 util/Makefile.objs   |  1 +
 17 files changed, 139 insertions(+), 237 deletions(-)
 create mode 100644 include/qemu/drm.h
 create mode 100644 util/drm.c

-- 
2.9.3

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

[libvirt] [PULL 08/12] spice-display: fix qemu_spice_cursor_refresh_bh locking

[Gerd Hoffmann]

From: Paolo Bonzini 

spice-display should not call the ui/console.c functions dpy_cursor_define
and dpy_moues_set with the SimpleSpiceDisplay lock taken.  That will cause
a deadlock, because the DisplayChangeListener callbacks will take the lock
again.  It is also in general a bad idea to invoke generic callbacks with a
lock taken, because it can cause AB-BA deadlocks in the long run.  The only
thing that requires care is that the cursor may disappear as soon as the
mutex is released, so you need an extra cursor_get/cursor_put pair.

Signed-off-by: Paolo Bonzini 
Reviewed-by: Marc-André Lureau 
Message-id: 20180720063109.4631-3-pbonz...@redhat.com

[ kraxel: fix dpy_cursor_define() call ]

Signed-off-by: Gerd Hoffmann 
---
 ui/spice-display.c | 30 ++
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 46df673cd7..7b230987dc 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -450,29 +450,35 @@ void qemu_spice_display_switch(SimpleSpiceDisplay *ssd,
 qemu_mutex_unlock(>lock);
 }
 
-static void qemu_spice_cursor_refresh_unlocked(SimpleSpiceDisplay *ssd)
+void qemu_spice_cursor_refresh_bh(void *opaque)
 {
+SimpleSpiceDisplay *ssd = opaque;
+
+qemu_mutex_lock(>lock);
 if (ssd->cursor) {
+QEMUCursor *c = ssd->cursor;
 assert(ssd->dcl.con);
-dpy_cursor_define(ssd->dcl.con, ssd->cursor);
+cursor_get(c);
+qemu_mutex_unlock(>lock);
+dpy_cursor_define(ssd->dcl.con, c);
+qemu_mutex_lock(>lock);
+cursor_put(c);
 }
+
 if (ssd->mouse_x != -1 && ssd->mouse_y != -1) {
+int x, y;
 assert(ssd->dcl.con);
-dpy_mouse_set(ssd->dcl.con, ssd->mouse_x, ssd->mouse_y, 1);
+x = ssd->mouse_x;
+y = ssd->mouse_y;
 ssd->mouse_x = -1;
 ssd->mouse_y = -1;
+qemu_mutex_unlock(>lock);
+dpy_mouse_set(ssd->dcl.con, x, y, 1);
+} else {
+qemu_mutex_unlock(>lock);
 }
 }
 
-void qemu_spice_cursor_refresh_bh(void *opaque)
-{
-SimpleSpiceDisplay *ssd = opaque;
-
-qemu_mutex_lock(>lock);
-qemu_spice_cursor_refresh_unlocked(ssd);
-qemu_mutex_unlock(>lock);
-}
-
 void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
 {
 graphic_hw_update(ssd->dcl.con);
-- 
2.9.3

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

[libvirt] [PULL 11/12] dmabuf: add y0_top, pass it to spice

[Gerd Hoffmann]

From: Marc-André Lureau 

Some scanouts during boot are top-down without it.

y0_top is set from VHOST_USER_GPU_DMABUF_SCANOUT code path in the last
patch of this series.

In current QEMU code base, only vfio/display uses dmabuf API. But the
VFIO query interface doesn't provide or need that detail so far.

Signed-off-by: Marc-André Lureau 
Message-Id: <20180713130916.4153-5-marcandre.lur...@redhat.com>
Signed-off-by: Gerd Hoffmann 
---
 include/ui/console.h | 1 +
 ui/spice-display.c   | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index 981b519dde..fb969caf70 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -186,6 +186,7 @@ struct QemuDmaBuf {
 uint32_t  stride;
 uint32_t  fourcc;
 uint32_t  texture;
+bool  y0_top;
 };
 
 typedef struct DisplayChangeListenerOps {
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 7b230987dc..2f8adb6b9f 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1056,7 +1056,8 @@ static void qemu_spice_gl_update(DisplayChangeListener 
*dcl,
 /* note: spice server will close the fd, so hand over a dup */
 spice_qxl_gl_scanout(>qxl, dup(dmabuf->fd),
  dmabuf->width, dmabuf->height,
- dmabuf->stride, dmabuf->fourcc, false);
+ dmabuf->stride, dmabuf->fourcc,
+ dmabuf->y0_top);
 }
 qemu_spice_gl_monitor_config(ssd, 0, 0, dmabuf->width, dmabuf->height);
 ssd->guest_dmabuf_refresh = false;
-- 
2.9.3

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

[libvirt] [PULL 06/12] vnc: remove support for deprecated tls, x509, x509verify options

[Gerd Hoffmann]

From: Daniel P. Berrangé 

The 'tls-creds' option accepts the name of a TLS credentials
object. This replaced the usage of 'tls', 'x509' and 'x509verify'
options in 2.5.0. These deprecated options were grandfathered in
when the deprecation policy was introduded in 2.10.0, so can now
finally be removed.

Signed-off-by: Daniel P. Berrangé 
Message-id: 20180725092751.21767-3-berra...@redhat.com
Signed-off-by: Gerd Hoffmann 
---
 ui/vnc.c | 91 
 qemu-deprecated.texi | 20 
 qemu-options.hx  | 43 -
 3 files changed, 154 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 359693238b..fd929b0957 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3345,10 +3345,6 @@ static QemuOptsList qemu_vnc_opts = {
 .name = "tls-creds",
 .type = QEMU_OPT_STRING,
 },{
-/* Deprecated in favour of tls-creds */
-.name = "x509",
-.type = QEMU_OPT_STRING,
-},{
 .name = "share",
 .type = QEMU_OPT_STRING,
 },{
@@ -3385,14 +3381,6 @@ static QemuOptsList qemu_vnc_opts = {
 .name = "sasl",
 .type = QEMU_OPT_BOOL,
 },{
-/* Deprecated in favour of tls-creds */
-.name = "tls",
-.type = QEMU_OPT_BOOL,
-},{
-/* Deprecated in favour of tls-creds */
-.name = "x509verify",
-.type = QEMU_OPT_STRING,
-},{
 .name = "acl",
 .type = QEMU_OPT_BOOL,
 },{
@@ -3519,51 +3507,6 @@ vnc_display_setup_auth(int *auth,
 }
 
 
-/*
- * Handle back compat with old CLI syntax by creating some
- * suitable QCryptoTLSCreds objects
- */
-static QCryptoTLSCreds *
-vnc_display_create_creds(bool x509,
- bool x509verify,
- const char *dir,
- const char *id,
- Error **errp)
-{
-gchar *credsid = g_strdup_printf("tlsvnc%s", id);
-Object *parent = object_get_objects_root();
-Object *creds;
-Error *err = NULL;
-
-if (x509) {
-creds = object_new_with_props(TYPE_QCRYPTO_TLS_CREDS_X509,
-  parent,
-  credsid,
-  ,
-  "endpoint", "server",
-  "dir", dir,
-  "verify-peer", x509verify ? "yes" : "no",
-  NULL);
-} else {
-creds = object_new_with_props(TYPE_QCRYPTO_TLS_CREDS_ANON,
-  parent,
-  credsid,
-  ,
-  "endpoint", "server",
-  NULL);
-}
-
-g_free(credsid);
-
-if (err) {
-error_propagate(errp, err);
-return NULL;
-}
-
-return QCRYPTO_TLS_CREDS(creds);
-}
-
-
 static int vnc_display_get_address(const char *addrstr,
bool websocket,
bool reverse,
@@ -3930,15 +3873,6 @@ void vnc_display_open(const char *id, Error **errp)
 credid = qemu_opt_get(opts, "tls-creds");
 if (credid) {
 Object *creds;
-if (qemu_opt_get(opts, "tls") ||
-qemu_opt_get(opts, "x509") ||
-qemu_opt_get(opts, "x509verify")) {
-error_setg(errp,
-   "'tls-creds' parameter is mutually exclusive with "
-   "'tls', 'x509' and 'x509verify' parameters");
-goto fail;
-}
-
 creds = object_resolve_path_component(
 object_get_objects_root(), credid);
 if (!creds) {
@@ -3961,31 +3895,6 @@ void vnc_display_open(const char *id, Error **errp)
"Expecting TLS credentials with a server endpoint");
 goto fail;
 }
-} else {
-const char *path;
-bool tls = false, x509 = false, x509verify = false;
-tls  = qemu_opt_get_bool(opts, "tls", false);
-if (tls) {
-path = qemu_opt_get(opts, "x509");
-
-if (path) {
-x509 = true;
-} else {
-path = qemu_opt_get(opts, "x509verify");
-if (path) {
-x509 = true;
-x509verify = true;
-}
-}
-vd->tlscreds = vnc_display_create_creds(x509,
-x509verify,
-path,
-vd->id,
-errp);
-if (!vd->tlscreds) {
-goto fail;
-}
-}
 }
 acl = qemu_opt_get_bool(opts, 

[libvirt] [PULL 05/12] doc: switch to modern syntax for VNC TLS setup

[Gerd Hoffmann]

From: Daniel P. Berrangé 

The use of 'tls', 'x509' and 'x509verify' properties is the deprecated
backcompat syntax, replaced by use of TLS creds objects.

Signed-off-by: Daniel P. Berrangé 
Message-id: 20180725092751.21767-2-berra...@redhat.com
Signed-off-by: Gerd Hoffmann 
---
 qemu-doc.texi | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/qemu-doc.texi b/qemu-doc.texi
index f74542a0e9..7bd449f398 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -1103,7 +1103,9 @@ support provides a secure session, but no authentication. 
This allows any
 client to connect, and provides an encrypted session.
 
 @example
-qemu-system-i386 [...OPTIONS...] -vnc :1,tls,x509=/etc/pki/qemu -monitor stdio
+qemu-system-i386 [...OPTIONS...] \
+  -object 
tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=no \
+  -vnc :1,tls-creds=tls0 -monitor stdio
 @end example
 
 In the above example @code{/etc/pki/qemu} should contain at least three files,
@@ -1118,10 +1120,14 @@ only be readable by the user owning it.
 Certificates can also provide a means to authenticate the client connecting.
 The server will request that the client provide a certificate, which it will
 then validate against the CA certificate. This is a good choice if deploying
-in an environment with a private internal certificate authority.
+in an environment with a private internal certificate authority. It uses the
+same syntax as previously, but with @code{verify-peer} set to @code{yes}
+instead.
 
 @example
-qemu-system-i386 [...OPTIONS...] -vnc :1,tls,x509verify=/etc/pki/qemu -monitor 
stdio
+qemu-system-i386 [...OPTIONS...] \
+  -object 
tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \
+  -vnc :1,tls-creds=tls0 -monitor stdio
 @end example
 
 
@@ -1132,7 +1138,9 @@ Finally, the previous method can be combined with VNC 
password authentication
 to provide two layers of authentication for clients.
 
 @example
-qemu-system-i386 [...OPTIONS...] -vnc :1,password,tls,x509verify=/etc/pki/qemu 
-monitor stdio
+qemu-system-i386 [...OPTIONS...] \
+  -object 
tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \
+  -vnc :1,tls-creds=tls0,password -monitor stdio
 (qemu) change vnc password
 Password: 
 (qemu)
@@ -1169,7 +1177,9 @@ credentials. This can be enabled, by combining the 'sasl' 
option
 with the aforementioned TLS + x509 options:
 
 @example
-qemu-system-i386 [...OPTIONS...] -vnc :1,tls,x509,sasl -monitor stdio
+qemu-system-i386 [...OPTIONS...] \
+  -object 
tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \
+  -vnc :1,tls-creds=tls0,sasl -monitor stdio
 @end example
 
 @node vnc_setup_sasl
-- 
2.9.3

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

Re: [libvirt] [Qemu-devel] [PULL 00/12] Ui 20180821 v2 patches

[Gerd Hoffmann]

On Thu, Aug 23, 2018 at 10:22:08AM +0100, Peter Maydell wrote:
> On 21 August 2018 at 13:05, Gerd Hoffmann  wrote:
> > The following changes since commit d0092d90eb546a8bbe9e9120426c189474123797:
> >
> >   Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20180820' into 
> > staging (2018-08-20 17:41:18 +0100)
> >
> > are available in the git repository at:
> >
> >   git://git.kraxel.org/qemu tags/ui-20180821-v2-pull-request
> >
> > for you to fetch changes up to 37bdff33227c3248ab9af784246da5b6c08111ea:
> >
> >   util: promote qemu_egl_rendernode_open() to libqemuutil (2018-08-21 
> > 14:01:48 +0200)
> >
> > 
> > ui: misc ui fixed which piled up during 3.0 release freeze.
> >
> > 
> 
> Hi; this pullrequest includes a commit whose author line
> claims an email address of the mailing list:
> 
> "Author: Tao Wu via Qemu-devel "
> 
> Could you fix it up to have the correct email for the
> author and resend, please?

Fixed, v3 sent.

cheers,
  Gerd

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

[libvirt] [PULL 10/12] ui/vnc: Remove useless parenthesis around DIV_ROUND_UP macro

[Gerd Hoffmann]

From: Philippe Mathieu-Daudé 

Patch created mechanically by rerunning:

  $  spatch --sp-file scripts/coccinelle/round.cocci \
--macro-file scripts/cocci-macro-file.h \
--dir . --in-place

Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20180704153919.12432-7-f4...@amsat.org>
Signed-off-by: Gerd Hoffmann 
---
 ui/vnc-enc-tight.c | 2 +-
 ui/vnc.c   | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index f38aceb4da..0b4a5ac71f 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -979,7 +979,7 @@ static int send_mono_rect(VncState *vs, int x, int y,
 }
 #endif
 
-bytes = (DIV_ROUND_UP(w, 8)) * h;
+bytes = DIV_ROUND_UP(w, 8) * h;
 
 vnc_write_u8(vs, (stream | VNC_TIGHT_EXPLICIT_FILTER) << 4);
 vnc_write_u8(vs, VNC_TIGHT_FILTER_PALETTE);
diff --git a/ui/vnc.c b/ui/vnc.c
index fd929b0957..ccb1335d86 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2967,7 +2967,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
 PIXMAN_FORMAT_BPP(pixman_image_get_format(vd->guest.fb));
 guest_row0 = (uint8_t *)pixman_image_get_data(vd->guest.fb);
 guest_stride = pixman_image_get_stride(vd->guest.fb);
-guest_ll = pixman_image_get_width(vd->guest.fb) * 
(DIV_ROUND_UP(guest_bpp, 8));
+guest_ll = pixman_image_get_width(vd->guest.fb)
+   * DIV_ROUND_UP(guest_bpp, 8);
 }
 line_bytes = MIN(server_stride, guest_ll);
 
-- 
2.9.3

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

[libvirt] [PULL 09/12] ui/sdl2: Fix broken -full-screen CLI option

[Gerd Hoffmann]

From: Thomas Huth 

We've got to set the gui_fullscreen variable before creating the
SDL2 window, otherwise the initial window will not be created in
fullscreen mode.

Buglink: https://bugs.launchpad.net/bugs/1780812
Signed-off-by: Thomas Huth 
Message-id: 1531161850-6860-1-git-send-email-th...@redhat.com
Signed-off-by: Gerd Hoffmann 
---
 ui/sdl2.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/ui/sdl2.c b/ui/sdl2.c
index 755a7134ff..0a9a18a964 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -790,6 +790,8 @@ static void sdl2_display_init(DisplayState *ds, 
DisplayOptions *o)
 memset(, 0, sizeof(info));
 SDL_VERSION();
 
+gui_fullscreen = o->has_full_screen && o->full_screen;
+
 for (i = 0;; i++) {
 QemuConsole *con = qemu_console_lookup_by_index(i);
 if (!con) {
@@ -842,17 +844,14 @@ static void sdl2_display_init(DisplayState *ds, 
DisplayOptions *o)
 g_free(filename);
 }
 
-if (sdl2_console->opts->has_full_screen &&
-sdl2_console->opts->full_screen) {
-gui_fullscreen = 1;
+gui_grab = 0;
+if (gui_fullscreen) {
 sdl_grab_start(0);
 }
 
 mouse_mode_notifier.notify = sdl_mouse_mode_change;
 qemu_add_mouse_mode_change_notifier(_mode_notifier);
 
-gui_grab = 0;
-
 sdl_cursor_hidden = SDL_CreateCursor(, , 8, 1, 0, 0);
 sdl_cursor_normal = SDL_GetCursor();
 
-- 
2.9.3

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

[libvirt] [PULL 01/12] ui/sdl2: Remove the obsolete SDL_INIT_NOPARACHUTE flag

[Gerd Hoffmann]

From: Thomas Huth 

SDL_INIT_NOPARACHUTE is not used in SDL2 anymore, and the define is just
a dummy (see https://wiki.libsdl.org/MigrationGuide#Some_general_truths
for example). So we can remove it and get rid of the "flags" variable
nowadays.

Signed-off-by: Thomas Huth 
Message-id: 1533721602-15763-1-git-send-email-th...@redhat.com
Signed-off-by: Gerd Hoffmann 
---
 ui/sdl2.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ui/sdl2.c b/ui/sdl2.c
index 76e59427cc..755a7134ff 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -761,7 +761,6 @@ static void sdl2_display_early_init(DisplayOptions *o)
 
 static void sdl2_display_init(DisplayState *ds, DisplayOptions *o)
 {
-int flags;
 uint8_t data = 0;
 char *filename;
 int i;
@@ -782,8 +781,7 @@ static void sdl2_display_init(DisplayState *ds, 
DisplayOptions *o)
 setenv("SDL_VIDEODRIVER", "x11", 0);
 #endif
 
-flags = SDL_INIT_VIDEO | SDL_INIT_NOPARACHUTE;
-if (SDL_Init(flags)) {
+if (SDL_Init(SDL_INIT_VIDEO)) {
 fprintf(stderr, "Could not initialize SDL(%s) - exiting\n",
 SDL_GetError());
 exit(1);
-- 
2.9.3

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

[libvirt] [PULL 03/12] ui: use enum to string helpers

[Gerd Hoffmann]

From: Marc-André Lureau 

Minor code simplification.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Markus Armbruster 
Message-id: 20180801092508.4927-1-marcandre.lur...@redhat.com
Signed-off-by: Gerd Hoffmann 
---
 qemu-keymap.c | 2 +-
 ui/console.c  | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/qemu-keymap.c b/qemu-keymap.c
index 6216371aa1..4d00468747 100644
--- a/qemu-keymap.c
+++ b/qemu-keymap.c
@@ -84,7 +84,7 @@ static void walk_map(struct xkb_keymap *map, xkb_keycode_t 
code, void *data)
 }
 fprintf(outfile, "# evdev %d (0x%x), QKeyCode \"%s\", number 0x%x\n",
 evdev, evdev,
-QKeyCode_lookup.array[qcode],
+QKeyCode_str(qcode),
 qcode_to_number(qcode));
 
 /*
diff --git a/ui/console.c b/ui/console.c
index bc58458ee8..3a285bae00 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2319,7 +2319,7 @@ bool qemu_display_find_default(DisplayOptions *opts)
 
 for (i = 0; i < ARRAY_SIZE(prio); i++) {
 if (dpys[prio[i]] == NULL) {
-ui_module_load_one(DisplayType_lookup.array[prio[i]]);
+ui_module_load_one(DisplayType_str(prio[i]));
 }
 if (dpys[prio[i]] == NULL) {
 continue;
@@ -2337,11 +2337,11 @@ void qemu_display_early_init(DisplayOptions *opts)
 return;
 }
 if (dpys[opts->type] == NULL) {
-ui_module_load_one(DisplayType_lookup.array[opts->type]);
+ui_module_load_one(DisplayType_str(opts->type));
 }
 if (dpys[opts->type] == NULL) {
 error_report("Display '%s' is not available.",
- DisplayType_lookup.array[opts->type]);
+ DisplayType_str(opts->type));
 exit(1);
 }
 if (dpys[opts->type]->early_init) {
-- 
2.9.3

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

Re: [libvirt] [Qemu-devel] [PULL 00/12] Ui 20180821 v2 patches

[Peter Maydell]

On 21 August 2018 at 13:05, Gerd Hoffmann  wrote:
> The following changes since commit d0092d90eb546a8bbe9e9120426c189474123797:
>
>   Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20180820' into 
> staging (2018-08-20 17:41:18 +0100)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/ui-20180821-v2-pull-request
>
> for you to fetch changes up to 37bdff33227c3248ab9af784246da5b6c08111ea:
>
>   util: promote qemu_egl_rendernode_open() to libqemuutil (2018-08-21 
> 14:01:48 +0200)
>
> 
> ui: misc ui fixed which piled up during 3.0 release freeze.
>
> 

Hi; this pullrequest includes a commit whose author line
claims an email address of the mailing list:

"Author: Tao Wu via Qemu-devel "

Could you fix it up to have the correct email for the
author and resend, please?

thanks
-- PMM

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

Re: [libvirt] [PATCH v3 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes

[Yi Min Zhao]

在 2018/8/23 下午4:12, Andrea Bolognani 写道:

Exactly. You can either just add zpci_uid and zpci_fid to the
virPCIDeviceAddress struct, or have

   struct _virZPCIDeviceAddress {
   unsigned int uid;
   unsigned int fid;
   };

   struct _virPCIDeviceAddress {
   unsigned int domain;
   unsigned int bus;
   unsigned int slot;
   unsigned int function;
   int multi; /* virTristateSwitch */
   virZPCIDeviceAddress zpci;
   };

There's an error in syntax-check. I think it's from common code.

src/util/virpci.h:47:    unsigned int uid;
maint.mk: use pid_t for pid, uid_t for uid, gid_t for gid

I think it mistakes zpci's uid.

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

Re: [libvirt] [PATCH v2 0/3] ui: remove deprecated UI frontends

[Fam Zheng]

On Wed, 08/22 14:15, Daniel P. Berrangé wrote:
> We deprecated GTK2 and SDL1.2 in the 2.12.0 release, so they are able to
> be removed entirely in the 3.1.0 release. The min GTK3 version can also
> be bumped up based the distros we aim to support.
> 
> Note that before this can merge, the openbsd VM test image needs to be
> updated to have SDL2, as openbsd build mandates SDL availability.

I'll send a patch to upgrade and convert openbsd to use auto_install soon.

Fam

> 
> The freebsd & netbsd images should also be updated, but they are not
> build blockers as they automatically disable SDL
> 
> Changed in v2:
> 
>  - Rebased to resolve conflicts
> 
> Daniel P. Berrangé (3):
>   ui: remove support for GTK2 in favour of GTK3
>   ui: increase min required GTK3 version to 3.14.0
>   ui: remove support for SDL1.2 in favour of SDL2
> 
>  configure  |  111 +
>  include/ui/gtk.h   |9 -
>  qemu-deprecated.texi   |   16 -
>  ui/Makefile.objs   |5 -
>  ui/gtk-egl.c   |   10 +-
>  ui/gtk.c   |  202 +---
>  ui/sdl.c   | 1027 
>  ui/sdl_zoom.c  |   93 
>  ui/sdl_zoom.h  |   25 -
>  ui/sdl_zoom_template.h |  219 -
>  10 files changed, 33 insertions(+), 1684 deletions(-)
>  delete mode 100644 ui/sdl.c
>  delete mode 100644 ui/sdl_zoom.c
>  delete mode 100644 ui/sdl_zoom.h
>  delete mode 100644 ui/sdl_zoom_template.h
> 
> -- 
> 2.17.1
> 

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

[libvirt] [PATCH 1/5] docs: api_extension: Remove links to the stale example patches

[Peter Krempa]

The pathches used as an example for the api_extension manual don't hold
up to the current standards any more. Carefully remove links and
mentions of the patches from the docs.

Signed-off-by: Peter Krempa 
---
 docs/api_extension.html.in | 96 +-
 1 file changed, 18 insertions(+), 78 deletions(-)

diff --git a/docs/api_extension.html.in b/docs/api_extension.html.in
index 9beec07602..b473d09b17 100644
--- a/docs/api_extension.html.in
+++ b/docs/api_extension.html.in
@@ -8,14 +8,9 @@

 
   This document walks you through the process of implementing a new
-  API in libvirt.  It uses as an example the addition of an API for
-  separating maximum from current vcpu usage of a domain, over
-  the course of a fifteen-patch series.
-  Remember that new API consists of any new public functions, as
-  well as the addition of flags or extensions of XML used by
-  existing functions.  The example in this document adds both new
-  functions and an XML extension.  Not all libvirt API additions
-  require quite as many patches.
+  API in libvirt.  Remember that new API consists of any new public
+  functions, as well as the addition of flags or extensions of XML used by
+  existing functions.
 

 
@@ -27,12 +22,7 @@
   added to libvirt.  Someone may already be working on the feature you
   want.  Also, recognize that everything you write is likely to undergo
   significant rework as you discuss it with the other developers, so
-  don't wait too long before getting feedback.  In the vcpu example
-  below, list feedback was first requested
-  https://www.redhat.com/archives/libvir-list/2010-September/msg00423.html;>here
-  and resulted in several rounds of improvements before coding
-  began.  In turn, this example is slightly rearranged from the actual
-  order of the commits.
+  don't wait too long before getting feedback.
 

 
@@ -81,14 +71,13 @@
 

 
-  Submit new code in the form shown in the example code: one patch
-  per step.  That's not to say submit patches before you have working
-  functionality--get the whole thing working and make sure you're happy
-  with it.  Then use git or some other version control system that lets
-  you rewrite your commit history and break patches into pieces so you
-  don't drop a big blob of code on the mailing list in one go.
-  Also, you should follow the upstream tree, and rebase your
-  series to adapt your patches to work with any other changes
+  Submit new code in the form of one patch per step.  That's not to say
+  submit patches before you have working functionality--get the whole thing
+  working and make sure you're happy with it.  Then use git or some other
+  version control system that lets you rewrite your commit history and
+  break patches into pieces so you don't drop a big blob of code on the
+  mailing list in one go.  Also, you should follow the upstream tree, and
+  rebase your series to adapt your patches to work with any other changes
   that were accepted upstream during your development.
 

@@ -101,8 +90,6 @@
   separately.
 

-With that said, let's begin.
-
 Defining the public API

 The first task is to define the public API.  If the new API
@@ -133,10 +120,6 @@
   rework it as you go through the process of implementing it.
 

-See 0001-add-to-xml.patch
-and 0002-add-new-public-API.patch
-for example code.
-
 Defining the internal API

 
@@ -164,8 +147,6 @@
   provide a NULL stub for the new function.
 

-See 0003-define-internal-driver-API.patch
-
 Implementing the public API

 
@@ -197,22 +178,16 @@

 The public API calls are implemented in:

-src/libvirt.c
-
-See 0004-implement-the-public-APIs.patch
+src/libvirt-$SECTION.c

 Implementing the remote protocol

 
   Implementing the remote protocol is essentially a
   straightforward exercise which is probably most easily
-  understood by referring to the existing code and the example
-  patch.  It involves several related changes, including the
-  regeneration of derived files, with further details below.
+  understood by referring to the existing code.
 

-See 0005-implement-the-remote-protocol.patch
-
 Defining the wire protocol format

 
@@ -298,8 +273,6 @@
   existing lines probably imply a backwards-incompatible API change.
 

-See 0005-implement-the-remote-protocol.patch
-
 Use the new API internally

 
@@ -312,8 +285,6 @@
   not necessary if the new API has no relation to existing API.
 

-See 0006-make-old-API-trivially-wrap-to-new-API.patch
-
 Expose the new API in virsh

 
@@ -343,8 +314,6 @@
 tools/virsh.pod
 

-See 0007-add-virsh-support.patch
-
 Implement the driver methods