[libvirt] [PATCH] doc: better explanation for virStoragePoolSetAutostart

[Dan Kenigsberg]

The former documentation was an unhelpful tautology. The suggested doc
mimicks^Wcopies the one of virDomainSetAutostart.

Signed-off-by: Dan Kenigsberg 
---
 src/libvirt-storage.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c
index 1879c6bd60..ed305b9b5e 100644
--- a/src/libvirt-storage.c
+++ b/src/libvirt-storage.c
@@ -1127,7 +1127,8 @@ virStoragePoolGetAutostart(virStoragePoolPtr pool,
  * @pool: pointer to storage pool
  * @autostart: new flag setting
  *
- * Sets the autostart flag
+ * Configure the storage pool to be automatically started
+ * when the host machine boots.
  *
  * Returns 0 on success, -1 on failure
  */
-- 
2.17.1

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

Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line

[Yi Min Zhao]

在 2018/8/20 下午7:14, Andrea Bolognani 写道:

On Mon, 2018-08-20 at 16:45 +0800, Yi Min Zhao wrote:

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

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

+virBufferAddLit(, "zpci");
+virBufferAsprintf(, ",uid=%u", dev->addr.pci.zpci->zpci_uid);
+virBufferAsprintf(, ",fid=%u", dev->addr.pci.zpci->zpci_fid);
+virBufferAsprintf(, ",target=%s", dev->alias);
+virBufferAsprintf(, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid);

Just making sure: is the uid a good choice for naming the zpci
device? I would perhaps expect the fid to be in there as well,
but since both have to be unique (right?) I guess it doesn't
really matter...

Either uid or fid, the value must be unique. But uid is defined by user.
Of course, we also give the permission to define fid. But uid is more
appropriate.

So which is unique: uid, fid, or the combination of the two?
Could I have

   -device zpci,uid=1,fid=1
   -device zpci,uid=1,fid=2

or would that not work? What about

   -device zpci,uid=1,fid=1
   -device zpci,uid=2,fid=1

would that be okay?

Both can't work. FID must be unique and UID must be also unique.
They are independent.



+int
+qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)

Based on the name, this is a predicate: it should return a
boolean value rather than an int.

0 means it's not zpci. 1 means it's zpci. -1 means error.

My point is that a funtion called fooIsBaz() should only ever
check whether the foo in question is indeed a baz, without taking
any other action such as reporting errors. Leave that to the
caller, or give the function a different name.

I think moving to the caller is proper.



Either way, calling a predicate should never result in an error
being reported, so you need to move this check somewhere else.

Acutally I have found out a proper place to add this check for a long time.
So this is the best place to add, at least I think for now.

qemuDomainDeviceDefValidate() looks like a reasonable entry point
for checks such as "you specified a zPCI address but this is not
an s390 guest" or "your configuration requires zPCI but the QEMU
binary doesn't support that feature". Both of the cases above
should have proper test suite coverage, by the way.


How about my idea mentioned above?

--
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/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!


[...]

+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.

I don't think that's what we want.

Also all functions that return an int should be called like

   if (virDomainZPCIAddressReserveNextUid() < 0) {
   ...
   }


OK.



[...]

+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.



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

The error message is very generic here, it should at the very least
make it clear that zPCI is not supported *for the specific device*.

I'm not sure this is the best place to perform that kind of check,
either.


This is only called by virDomainPCIAddressEnsureAddr() for hotplug case.
I thought this again. If we remove those two boolean members from zPCI
address structure as what we discuss on the 1st patch, we could move this
check to virDomainPCIAddressEnsureAddr() like what PCI addr check does.

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

[libvirt] [PATCH v3 1/2] qemu: implementing qemuAgentGetHostname() function.

[Julio Faracco]

This commit implements the function qemuAgentGetHostname() that uses
the QEMU command 'guest-get-host-name' to retrieve the guest hostname
of Virtual Machine. It is a possibility where QEMU-GA is running.

Signed-off-by: Julio Faracco 
---
 src/qemu/qemu_agent.c | 39 +++
 src/qemu/qemu_agent.h |  4 
 2 files changed, 43 insertions(+)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index bf08871f18..7aba4ed366 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1682,6 +1682,45 @@ qemuAgentUpdateCPUInfo(unsigned int nvcpus,
 return 0;
 }
 
+int
+qemuAgentGetHostname(qemuAgentPtr mon,
+ char **hostname)
+{
+int ret = -1;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+virJSONValuePtr data = NULL;
+
+cmd = qemuAgentMakeCommand("guest-get-host-name",
+   NULL);
+
+if (!cmd)
+return ret;
+
+if (qemuAgentCommand(mon, cmd, , true,
+ VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+goto cleanup;
+
+if (!(data = virJSONValueObjectGet(reply, "return"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("malformed return value"));
+goto cleanup;
+}
+
+if (VIR_STRDUP(*hostname,
+   virJSONValueObjectGetString(data, "host-name")) <= 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("'host-name' missing in reply of 
guest-get-host-name"));
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
 
 int
 qemuAgentGetTime(qemuAgentPtr mon,
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 6dd9c702dd..4354b7e0cf 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -105,6 +105,10 @@ int qemuAgentUpdateCPUInfo(unsigned int nvcpus,
qemuAgentCPUInfoPtr cpuinfo,
int ncpuinfo);
 
+int
+qemuAgentGetHostname(qemuAgentPtr mon,
+ char **hostname);
+
 int qemuAgentGetTime(qemuAgentPtr mon,
  long long *seconds,
  unsigned int *nseconds);
-- 
2.17.1

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

[libvirt] [PATCH v3 0/2] Add .domainGetHostname() support for QEMU driver.

[Julio Faracco]

This serie adds a new function into QEMU Guest Agent handlers to use 
the QEMU command 'guest-get-host-name' to retrieve the domain hostname.
This approach requires QEMU-GA running inside the guest, but it is the 
fastest and easiest way to get this info.

Julio Faracco (2):
  qemu: implementing qemuAgentGetHostname() function.
  qemu: adding domainGetHostname support for QEMU.

 src/qemu/qemu_agent.c  | 39 +++
 src/qemu/qemu_agent.h  |  4 
 src/qemu/qemu_driver.c | 40 
 3 files changed, 83 insertions(+)

-- 
2.17.1

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

[libvirt] [PATCH v3 2/2] qemu: adding domainGetHostname support for QEMU.

[Julio Faracco]

This commit add the support to use the function qemuAgentGetHostname()
for obtain the domain hostname using QEMU-GA command.

Signed-off-by: Julio Faracco 
---
 src/qemu/qemu_driver.c | 40 
 1 file changed, 40 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 21e9e87ddd..5f53cbea15 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19351,6 +19351,45 @@ qemuConnectGetCPUModelNames(virConnectPtr conn,
 return virCPUGetModels(arch, models);
 }
 
+static char *
+qemuDomainGetHostname(virDomainPtr dom,
+  unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm = NULL;
+qemuAgentPtr agent;
+char *hostname = NULL;
+
+virCheckFlags(0, NULL);
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+return NULL;
+
+if (virDomainGetHostnameEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
+goto cleanup;
+
+if (virDomainObjCheckActive(vm) < 0)
+goto endjob;
+
+if (!qemuDomainAgentAvailable(vm, true))
+goto endjob;
+
+agent = qemuDomainObjEnterAgent(vm);
+ignore_value(qemuAgentGetHostname(agent, ));
+qemuDomainObjExitAgent(vm, agent);
+
+ endjob:
+qemuDomainObjEndAgentJob(vm);
+
+ cleanup:
+virDomainObjEndAPI();
+return hostname;
+}
+
+
 static int
 qemuDomainGetTime(virDomainPtr dom,
   long long *seconds,
@@ -21955,6 +21994,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
 .connectGetCPUModelNames = qemuConnectGetCPUModelNames, /* 1.1.3 */
 .domainFSFreeze = qemuDomainFSFreeze, /* 1.2.5 */
 .domainFSThaw = qemuDomainFSThaw, /* 1.2.5 */
+.domainGetHostname = qemuDomainGetHostname, /* 4.7.0 */
 .domainGetTime = qemuDomainGetTime, /* 1.2.5 */
 .domainSetTime = qemuDomainSetTime, /* 1.2.5 */
 .nodeGetFreePages = qemuNodeGetFreePages, /* 1.2.6 */
-- 
2.17.1

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

Re: [libvirt] Matching the type of mediated devices in the migration

[Tian, Kevin]

> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Wednesday, August 22, 2018 10:08 AM
> 
> On Wed, 22 Aug 2018 01:27:05 +
> "Tian, Kevin"  wrote:
> 
> > > From: Wang, Zhi A
> > > Sent: Wednesday, August 22, 2018 2:43 AM
> > > >
> > > > Are there any suggestions how we can deal with security issues?
> > > > Allowing userspace to provide a data stream representing the internal
> > > > state of a virtual device model living within the kernel seems
> > > > troublesome.  If we need to trust the data stream, do we need to
> > > > somehow make the operation more privileged than what a vfio user
> > > might
> > > > have otherwise?  Does the data stream need to be somehow signed
> and
> > > how
> > > > might we do that?  How can we build in protection against an
> untrusted
> > > > restore image?  Thanks,
> >
> > imo it is not necessary. restoring mdev state should be handled as if
> > guest is programming the mdev.
> 
> To me this suggests that a state save/restore is just an algorithm
> executed by userspace using the existing vfio device accesses.  This is
> not at all what we've been discussing for migration.  I believe the

not algorithm by userspace. It's kernel driver to apply the audit when
receiving opaque state data.

> interface we've been hashing out exposes opaque device state through a
> vfio region.  We therefore must assume that that opaque data contains
> not only device state, but also emulation state, similar to what we see
> for any QEMU device.  Not only is there internal emulation state, but
> we have no guarantee that the device state goes through the same
> auditing as it does through the vfio interface.  Since this device and
> emulation state live inside the kernel and not just within the user's
> own process, a malicious user can do far more than shoot themselves.  It
> would be one thing devices were IOMMU isolated, but they're not,
> they're isolated through vendor and device specific mechanism, and for
> all we know the parameters of that isolation are included in the
> restore state.  I don't see how we can say this is not an issue.

I didn't quite get this. My understanding is that isolation configuration
is completed when a mdev is created on DEST machine given a type
definition. The state image contains just runtime data reflecting what
guest driver does on SRC machine. Restoring such state shouldn't
change the isolation policy.

> 
> > Then all the audits/security checks
> > enforced in normal emulation path should still apply. vendor driver
> > may choose to audit every state restore operation one-by-one, and
> > do it altoghter at a synchronization point (e.g. when the mdev is re-
> > scheduled, similar to what we did before VMENTRY).
> 
> Giving the vendor driver the choice of whether to be secure or not is
> exactly what I'm trying to propose we spend some time thinking about.
> For instance, what if instead of allowing the user to load device state
> through a region, the kernel could side load it using sometime similar
> to the firmware loading path.  The user could be provided with a file
> name token that they push through the vfio interface to trigger the
> state loading from a location with proper file level ACLs such that the
> image can be considered trusted.  Unfortunately the collateral is that
> libvirt would need to become the secure delivery entity, somehow
> stripping this section of the migration stream into a file and
> providing a token for the user to ask the kernel to load it.  What are
> some other options?  Could save/restore be done simply as an
> algorithmic script matched to stack of data, as I read into your first
> statement above?  I have doubts that we can achieve the internal state
> we need, or maybe even the performance we need using such a process.
> Thanks,
> 

for GVT-g I think we invoke common functions as used in emulation path
to recover vGPU state, e.g. gtt rw handler, etc. Zhi can correct me if
I'm wrong.

Can you elaborate the difference between device state and emulation
state which you mentioned earlier? We may need look at some concrete
example to understand the actual problem here.

Thanks
Kevin

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

Re: [libvirt] [PATCH v2 1/2] qemu: implementing qemuAgentGetHostname() function.

[Julio Faracco]

Em seg, 20 de ago de 2018 às 23:35, Julio Faracco
 escreveu:
>
> This commit implements the function qemuAgentGetHostname() that uses the
> QEMU command 'guest-get-host-name' to retrieve the guest hostname of
> Virtual Machine. It is a possibility where QEMU-GA is running.
>
> Signed-off-by: Julio Faracco 
> ---
>  src/qemu/qemu_agent.c | 42 ++
>  src/qemu/qemu_agent.h |  4 
>  2 files changed, 46 insertions(+)
>
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index bf08871f18..2d006b8070 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -1682,6 +1682,48 @@ qemuAgentUpdateCPUInfo(unsigned int nvcpus,
>  return 0;
>  }
>
> +int
> +qemuAgentGetHostname(qemuAgentPtr mon,
> + char **hostname)
> +{
> +int ret = -1;
> +virJSONValuePtr cmd;
> +virJSONValuePtr reply = NULL;
> +virJSONValuePtr data = NULL;
> +
> +cmd = qemuAgentMakeCommand("guest-get-host-name",
> +   NULL);
> +
> +if (!cmd)
> +return ret;
> +
> +if (qemuAgentCommand(mon, cmd, , true,
> + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
> +goto cleanup;
> +
> +if (qemuAgentCheckError(cmd, reply) < 0)
> +goto cleanup;

There is no need to check error here. This function is already being executed
inside qemuAgentCommand(). This is redundant.

> +
> +if (!(data = virJSONValueObjectGet(reply, "return"))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("malformed return value"));
> +goto cleanup;
> +}
> +
> +if (VIR_STRDUP(*hostname,
> +   virJSONValueObjectGetString(data, "host-name")) <= 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("'host-name' missing in reply of 
> guest-get-host-name"));
> +goto cleanup;
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +virJSONValueFree(cmd);
> +virJSONValueFree(reply);
> +return ret;
> +}
>
>  int
>  qemuAgentGetTime(qemuAgentPtr mon,
> diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
> index 6dd9c702dd..4354b7e0cf 100644
> --- a/src/qemu/qemu_agent.h
> +++ b/src/qemu/qemu_agent.h
> @@ -105,6 +105,10 @@ int qemuAgentUpdateCPUInfo(unsigned int nvcpus,
> qemuAgentCPUInfoPtr cpuinfo,
> int ncpuinfo);
>
> +int
> +qemuAgentGetHostname(qemuAgentPtr mon,
> + char **hostname);
> +
>  int qemuAgentGetTime(qemuAgentPtr mon,
>   long long *seconds,
>   unsigned int *nseconds);
> --
> 2.17.1
>

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

Re: [libvirt] Matching the type of mediated devices in the migration

[Alex Williamson]

On Wed, 22 Aug 2018 01:27:05 +
"Tian, Kevin"  wrote:

> > From: Wang, Zhi A
> > Sent: Wednesday, August 22, 2018 2:43 AM  
> > >
> > > Are there any suggestions how we can deal with security issues?
> > > Allowing userspace to provide a data stream representing the internal
> > > state of a virtual device model living within the kernel seems
> > > troublesome.  If we need to trust the data stream, do we need to
> > > somehow make the operation more privileged than what a vfio user  
> > might  
> > > have otherwise?  Does the data stream need to be somehow signed and  
> > how  
> > > might we do that?  How can we build in protection against an untrusted
> > > restore image?  Thanks,  
> 
> imo it is not necessary. restoring mdev state should be handled as if
> guest is programming the mdev.

To me this suggests that a state save/restore is just an algorithm
executed by userspace using the existing vfio device accesses.  This is
not at all what we've been discussing for migration.  I believe the
interface we've been hashing out exposes opaque device state through a
vfio region.  We therefore must assume that that opaque data contains
not only device state, but also emulation state, similar to what we see
for any QEMU device.  Not only is there internal emulation state, but
we have no guarantee that the device state goes through the same
auditing as it does through the vfio interface.  Since this device and
emulation state live inside the kernel and not just within the user's
own process, a malicious user can do far more than shoot themselves.  It
would be one thing devices were IOMMU isolated, but they're not,
they're isolated through vendor and device specific mechanism, and for
all we know the parameters of that isolation are included in the
restore state.  I don't see how we can say this is not an issue.

> Then all the audits/security checks
> enforced in normal emulation path should still apply. vendor driver
> may choose to audit every state restore operation one-by-one, and 
> do it altoghter at a synchronization point (e.g. when the mdev is re-
> scheduled, similar to what we did before VMENTRY).

Giving the vendor driver the choice of whether to be secure or not is
exactly what I'm trying to propose we spend some time thinking about.
For instance, what if instead of allowing the user to load device state
through a region, the kernel could side load it using sometime similar
to the firmware loading path.  The user could be provided with a file
name token that they push through the vfio interface to trigger the
state loading from a location with proper file level ACLs such that the
image can be considered trusted.  Unfortunately the collateral is that
libvirt would need to become the secure delivery entity, somehow
stripping this section of the migration stream into a file and
providing a token for the user to ask the kernel to load it.  What are
some other options?  Could save/restore be done simply as an
algorithmic script matched to stack of data, as I read into your first
statement above?  I have doubts that we can achieve the internal state
we need, or maybe even the performance we need using such a process.
Thanks,

Alex

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

Re: [libvirt] Matching the type of mediated devices in the migration

[Tian, Kevin]

> From: Wang, Zhi A
> Sent: Wednesday, August 22, 2018 2:43 AM
> >
> > Are there any suggestions how we can deal with security issues?
> > Allowing userspace to provide a data stream representing the internal
> > state of a virtual device model living within the kernel seems
> > troublesome.  If we need to trust the data stream, do we need to
> > somehow make the operation more privileged than what a vfio user
> might
> > have otherwise?  Does the data stream need to be somehow signed and
> how
> > might we do that?  How can we build in protection against an untrusted
> > restore image?  Thanks,

imo it is not necessary. restoring mdev state should be handled as if
guest is programming the mdev. Then all the audits/security checks
enforced in normal emulation path should still apply. vendor driver
may choose to audit every state restore operation one-by-one, and 
do it altoghter at a synchronization point (e.g. when the mdev is re-
scheduled, similar to what we did before VMENTRY).

> What a good point!
> 
> I dig the kernel module security case, which seems similar with this
> case. The security of loading kernel module relies on root privilege and
> signature.
> 
> For root privilege, QEMU could run as non root in libvirtd. So this
> wouldn't be an option.
> 
> For signature, I am wondering if there is any similar cases in other
> kernel components, like KVM or another modules which provides ioctls to
> userspace. Maybe they don't even load some binary from userspace, but
> they could suffer from DDOS flood from userspace. Maybe some ioctls or
> interfaces in kernel should only allow signed/trusted userspace
> application to call. (previously it's "allow signed kernel module to load")
> 
> Thanks,
> Zhi.
> 
> >
> > Alex
> >

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

[libvirt] [PATCH] qemu: Start domain on a node without cpu affinity

[Roman Bolshakov]

libvirt affinity wrappers don't support macOS Thread Affinity API:
https://developer.apple.com/library/archive/releasenotes/Performance/RN-AffinityAPI/index.html

virProcessSetAffinity stub prevents libvirt from starting a qemu domain
on macOS:

$ virsh start vm
error: Failed to start domain vm
error: Process CPU affinity is not supported on this platform: Function not 
implemented

With the patch a VM can be started on macOS but some affinity-related
commands will return errors:

$ virsh vcpuinfo vm
error: Requested operation is not valid: cpu affinity is not supported

$ virsh vcpupin vm
VCPU: CPU Affinity
--
   0: 0-7

$ virsh vcpupin vm --live --vcpu 0 --cpulist 7
error: operation failed: Virt type 'qemu' does not support vCPU pinning

$ virsh emulatorpin vm
emulator: CPU Affinity
--
   *: 0-7

$ virsh emulatorpin vm --live --cpulist 7
error: Requested operation is not valid: cpu affinity is not supported

The patch also fixes virmacmaptest on macOS

Signed-off-by: Roman Bolshakov 
---
 src/qemu/qemu_driver.c | 6 ++
 src/util/virprocess.c  | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 21e9e87ddd..2e225b1ede 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5245,6 +5245,12 @@ qemuDomainPinEmulator(virDomainPtr dom,
 if (virDomainPinEmulatorEnsureACL(dom->conn, vm->def, flags) < 0)
 goto cleanup;
 
+if (!qemuDomainHasVcpuPids(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   "%s", _("cpu affinity is not supported"));
+goto cleanup;
+}
+
 if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
 goto cleanup;
 
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 3988f5546c..7eaafd29f2 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -598,7 +598,7 @@ int virProcessSetAffinity(pid_t pid ATTRIBUTE_UNUSED,
 {
 virReportSystemError(ENOSYS, "%s",
  _("Process CPU affinity is not supported on this 
platform"));
-return -1;
+return 0;
 }
 
 virBitmapPtr
-- 
2.15.2 (Apple Git-101.1)

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

[libvirt] [PATCH] util: eventpoll: Survive EBADF on macOS

[Roman Bolshakov]

Fixes:
https://www.redhat.com/archives/libvir-list/2017-January/msg00978.html

QEMU is probed through monitor fd to check capabilities during libvirtd init.
The monitor fd is closed after probing by virQEMUCapsInitQMPCommandFree
that calls virQEMUCapsInitQMPCommandAbort that calls qemuMonitorClose,
the latter one notifies the event loop via an interrupt handle in
qemuMonitorUnregister and after then closes monitor fd.

There could be a case when interrupt is sent after eventLoop is unlocked
but before virEventPollRunOnce blocks in poll, shortly before file
descriptor is closed by qemuMonitorClose. Then poll receives closed monitor
fd in fdset and returns EBADF.

EBADF is not mentioned as a valid errno on macOS poll man-page but such
behaviour can appear release-to-release, according to cpython:
https://github.com/python/cpython/blob/master/Modules/selectmodule.c#L1161

The change also fixes the issue in qemucapabilitiestest. It returns
Bad file descriptor message 25 times without the fix.

Signed-off-by: Roman Bolshakov 
---
 src/util/vireventpoll.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
index 13d278df13..6d19374c52 100644
--- a/src/util/vireventpoll.c
+++ b/src/util/vireventpoll.c
@@ -643,10 +643,18 @@ int virEventPollRunOnce(void)
 EVENT_DEBUG("Poll got error event %d", errno);
 if (errno == EINTR || errno == EAGAIN)
 goto retry;
+#ifdef __APPLE__
+if (errno == EBADF) {
+ret = 0;
+goto dispatch;
+}
+#endif
 virReportSystemError(errno, "%s",
  _("Unable to poll on file handles"));
 return -1;
 }
+
+ dispatch:
 EVENT_DEBUG("Poll got %d event(s)", ret);
 
 virMutexLock();
-- 
2.15.2 (Apple Git-101.1)

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

[libvirt] [PATCH] access: Fix nwfilter-binding ACL access API name generation

[John Ferlan]

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

Generation of the ACL API policy is a "automated process"
based on this perl script which "worked" with the changes to
add nwfilter binding API's because they had the "nwfilter"
prefix; however, the generated output name was incorrect
based on the remote protocol algorithm which expected to
generate names such as 'nwfilter-binding.action' instead
of 'nwfilter.binding-action'.

This effectively changes src/access/org.libvirt.api.policy entries:

  org.libvirt.api.nwfilter.binding-create ==>
  org.libvirt.api.nwfilter-binding.create

  org.libvirt.api.nwfilter.binding-delete ==>
  org.libvirt.api.nwfilter-binding.delete

  org.libvirt.api.nwfilter.binding-getattr ==>
  org.libvirt.api.nwfilter-binding.getattr

  org.libvirt.api.nwfilter.binding-read ==>
  org.libvirt.api.nwfilter-binding.read

Signed-off-by: John Ferlan 
---

 If someone can explain better exactly what is happening in this
 processing, I'd be more than willing to update the commit message.
 I'm sure my wording isn't "precise" enough, but I feel like I hit
 the lottery finding this needle in the haystack.

 src/access/genpolkit.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/access/genpolkit.pl b/src/access/genpolkit.pl
index 968cb8c55c..e074c90eb6 100755
--- a/src/access/genpolkit.pl
+++ b/src/access/genpolkit.pl
@@ -22,8 +22,8 @@ use warnings;
 
 my @objects = (
 "CONNECT", "DOMAIN", "INTERFACE",
-"NETWORK","NODE_DEVICE", "NWFILTER",
- "SECRET", "STORAGE_POOL", "STORAGE_VOL",
+"NETWORK","NODE_DEVICE", "NWFILTER_BINDING", "NWFILTER",
+"SECRET", "STORAGE_POOL", "STORAGE_VOL",
 );
 
 my $objects = join ("|", @objects);
-- 
2.17.1

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

Re: [libvirt] Matching the type of mediated devices in the migration

[Zhi Wang]

On 08/21/18 07:08, Alex Williamson wrote:

On Sun, 19 Aug 2018 22:25:19 +0800
Zhi Wang  wrote:


Share some updates of my work on this topic recently:

Thanks for Erik's guide and advices. Now my PoC patches almost works.
Will send the RFC soon.

Mostly the ideas are based on Alex's idea: a match between a device
state version and a minimum required version


"Match of versions" in Libvirt

Initialization stage:

- Libvirt would detect if there is any device state version in a
"mdev_type" of a mediated device when creating a mdev node in node
device tree.
- If the "mdev_type" of a mediated device *has* a device state version,
then this mediated device supports migration.
- If not, (compatibility case, mostly for old vendor drivers which
don't support migration), this mediated device doesn't support migration

Migration stage:

- Libvirt would put the mdev information inside cookies and send them
between src machine and dst machine. So a new type of cookie would be
added here.

There are different versions of migration protocols in libvirt. Each of
them starts to send cookies in different sequence. The idea here is to
let the match happens as early as possible. Looks like QEMU driver in
libvirt only support V2/V3 proto.


V2 proto:

- The match would happen in SRC machine after the DST machine transfers
the cookies with mdev information back to the SRC machine during the
"preparation" stage. The disadvantage is the DST virtual machine has
already been created in "preparation" stage. If the match fails, the
virtual machine in DST machine has to be killed as well, which would
waste some time.

V3 proto:

- The match would happen in DST machine after the SRC machine transfers
the cookies to the DST machine during the "begin" stage. As the DST
machine hasn't entered into "preparation" stage at this time, the
virtual machine hasn't been created in DST machine at this point. No
extra VM destroy is needed if the match fails. This would be the ideal
place for a match.

"Match of version" in QEMU level

As there are several different types of migration in libvirt. In a
migration with hypervisor native transport, the target machine could
even not have libvirtd, the migration happens between device models
directly. So we need a match in QEMU level as well. We might still need
Kirti's approach as the last level match.


The kernel and vendor driver will always have a last opportunity to nak
a migration, the purpose of making certain information readily
available to libvirt is only to allow userspace some insight into where
a migration is likely to be successful.  Even if we expose these things
to userspace, it's the kernel's responsibility to validate the
migration data.  


Yes. The vendor driver should be the last keeper to nak a migration. It 
should be implemented inside the vendor driver.


In fact, pushing state information for a device into

the kernel would seem to be a massive security target.  For instance
how many vulnerabilities might a malicious user be able to exploit in
the code that parses the device specific state information?  How do we
even detect non-malicious user errors, like trying to migrate GVTg
device state to an NVIDIA vGPU?


For now, we only depends on mdev_type, after the discussion of vendor id 
or device id.


The latter at least suggests that the kernel needs to perform the same
set of validation that we're trying to enable userspace to do.
Cornelia also mentioned that some mdev devices are more or less shells
within which a device is configured, such as ccw and likely the crypto
ap devices.  In those cases the mdev type might not be sufficient meta
data about what we're dealing with.  This might suggest some sort of
header within the migration region parsed by common code for basic
validation.
Yes. If we could validate it earlier then better since, we don't need to 
wait until the DST machine start the VM and try to load the 1st states.


Are there any suggestions how we can deal with security issues?
Allowing userspace to provide a data stream representing the internal
state of a virtual device model living within the kernel seems
troublesome.  If we need to trust the data stream, do we need to
somehow make the operation more privileged than what a vfio user might
have otherwise?  Does the data stream need to be somehow signed and how
might we do that?  How can we build in protection against an untrusted
restore image?  Thanks,

What a good point!

I dig the kernel module security case, which seems similar with this 
case. The security of loading kernel module relies on root privilege and 
signature.


For root privilege, QEMU could run as non root in libvirtd. So this 
wouldn't be an option.


For signature, I am wondering if there is any similar cases in other 
kernel components, like KVM or another modules which provides ioctls to 
userspace. Maybe they don't even load some binary from userspace, but 
they could suffer from DDOS flood from userspace. Maybe some 

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

[Laine Stump]

On 08/17/2018 06:35 AM, Andrea Bolognani wrote:
> On Fri, 2018-08-17 at 10:29 +0100, Daniel P. Berrangé wrote:
>> On Thu, Aug 16, 2018 at 06:20:29PM -0400, Laine Stump wrote:
>>> 5) Some guest OSes that we still want to support (and which would
>>> otherwise work okay on a Q35 virtual machine) have virtio drivers too
>>> old to support virtio-1.0 (CentOS6 and RHEL6 are examples of such OSes),
>>> but due to the chain of reasons listed above, the "standard" config for
>>> a Q35 guest generated by libvirt doesn't support virtio-0.9, hence
>>> doesn't support these guest OSes.
>> Note when talking about "support" you're really saying it from the
>> downstream vendor, specifically RHEL, POV. From upstream or Fedora POV
>> essentially all x86 OS ever made are in scope for running under QEMU
>> if suitable virtual hardware models have been provided. QEMU doesn't
>> maintain any whitelist of "supported" OS that differs from what is
>> technically capable of being run, in the way downstream vendors do.
> Well, at least in the case of RHEL 6, "not supported" means that it
> will not boot at all on q35 with the default guest topology created
> by libvirt, so that's not really a downstream-only problem :)
>
>>> a) we don't really need the virtio-1.0 model, since that's what you
>>> currently get anyway when you ask for "virtio" on Q35 (and on 440fx,
>>> "virtio" gives you transitional, which works for everybody).
>> At some point we might have a virtio-2.0 and find ourselves in a
>> similar problem again. IMHO it is preferrable to have both explicit
>> versioned models, and discourage use of the magical 'virtio' model from
>> mgmt apps. Use libosinfo to identify which virtio model is supported
>> for the OS in question and use that explicitly.  Only use the magical
>> 'virtio' model if there's no information about what version the OS
>> supports.
> Agreed.
>
>>> c) Even if it's possible to force a device on an Express slot into
>>> transitional mode, this is extremely wasteful of io port space, so
>>> libvirt should consider virtio-0.9 devices to be legacy PCI, and thus
>>> plug them into legacy PCI slots. And once we're doing this, it's
>>> unnecessary to add any extra option to the qemu commandline to force
>>> legacy support (i.e. transitional mode), as that is what QEMU already
>>> does when the device is connected to a legacy PCI slot.
>> Yes, it should plug them into legacy PCI slots by default, but if a
>> mgmt app has done explicit placement itself, it should honour that
>> even if it means wasting IO space.
> This is consistent with what already happens with PCI addresses: a
> PCIe device will never be assigned to a PCI slot automatically, but
> the user can still force that to happen by providing the PCI address
> themselves.
>
>>> C) inside libvirt, the implementation of the "virtio-0.9" model is
>>> identical to "virtio", except that the VIR_PCI_CONNECT_TYPE flags for
>>> these devices contain VIR_PCI_CONNECT_TYPE_PCI rather than
>>> VIR_PCI_CONNECT_TYPE_PCIE, resulting in those devices being assigned to
>>> a legacy PCI slot, and thus they would be transitional mode by default.
>> For 'virtio-0.9' libvirt should set "disable-modern=yes" in QEMU args
>>
>> For 'virtio-1.0' libvirt should set "disable-legacy=yes" in QEMU args
> 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...


The problem I can see with the virtio-1.0 model name is that if
management applications start putting that into their XML (even though
"virtio" would yield a working guest), the guests will be unable to
migrate to another machine that has the same version of qemu, but an
older libvirt that doesn't understand the virtio-1.0 model number. If
that's acceptable, then management apps can being always specifying the
version for virtio whether it's old or new. If not, then they should
continue to use plain "virtio" unless they specifically need to force
virtio-0.9.


An additional task for this is that I've noticed that libvirt's domain
capabilities info only contains info for devices of type disk, graphics,
video, and hostdev. So even if oVirt learns from libosinfo that a
particular guest OS only supports virtio-0.9 netdevs (from libosinfo's
POV this means that it shows support for "virtio-net, but not
"virtio1.0-net", it currently has no way of determining if the version

[libvirt] [PATCH 3/3] storage: Allow inputvol to be encrypted

[John Ferlan]

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

When processing the inputvol for encryption, we need to handle
the case where the inputvol is encrypted. This then allows for
the encrypted inputvol to be used either for an output encrypted
volume or an output volume of some XML provided type.

Add tests to show the various conversion options when either input
or output is encrypted. This includes when both are encrypted.

Signed-off-by: John Ferlan 
---
 src/storage/storage_util.c| 62 ---
 src/storage/storage_util.h|  1 +
 .../luks-convert-encrypt.argv | 11 
 .../luks-convert-encrypt2fileqcow2.argv   |  7 +++
 .../luks-convert-encrypt2fileraw.argv |  7 +++
 tests/storagevolxml2argvtest.c| 15 -
 tests/storagevolxml2xmlin/vol-encrypt1.xml| 21 +++
 tests/storagevolxml2xmlin/vol-encrypt2.xml| 21 +++
 8 files changed, 137 insertions(+), 8 deletions(-)
 create mode 100644 tests/storagevolxml2argvdata/luks-convert-encrypt.argv
 create mode 100644 
tests/storagevolxml2argvdata/luks-convert-encrypt2fileqcow2.argv
 create mode 100644 
tests/storagevolxml2argvdata/luks-convert-encrypt2fileraw.argv
 create mode 100644 tests/storagevolxml2xmlin/vol-encrypt1.xml
 create mode 100644 tests/storagevolxml2xmlin/vol-encrypt2.xml

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index cc49a5b9f7..3c1e875b27 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -1084,6 +1084,7 @@ 
virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
  unsigned int flags,
  const char *create_tool,
  const char *secretPath,
+ const char *inputSecretPath,
  virStorageVolEncryptConvertStep 
convertStep)
 {
 virCommandPtr cmd = NULL;
@@ -1101,6 +1102,8 @@ 
virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
 .secretAlias = NULL,
 };
 virStorageEncryptionPtr enc = vol->target.encryption;
+char *inputSecretAlias = NULL;
+virStorageEncryptionPtr inputenc = inputvol ? inputvol->target.encryption 
: NULL;
 virStorageEncryptionInfoDefPtr encinfo = NULL;
 
 virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
@@ -1114,6 +1117,12 @@ 
virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
 goto error;
 }
 
+if (inputenc && inputenc->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("encryption format of inputvol must be LUKS"));
+goto error;
+}
+
 if (virStorageBackendCreateQemuImgSetInfo(pool, vol, inputvol,
   convertStep, ) < 0)
 goto error;
@@ -1153,6 +1162,20 @@ 
virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
 encinfo = >encinfo;
 }
 
+if (inputenc && convertStep == VIR_STORAGE_VOL_ENCRYPT_CONVERT) {
+if (!inputSecretPath) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("path to inputvol secret data file is required"));
+goto error;
+}
+if (virAsprintf(, "%s_encrypt0",
+inputvol->name) < 0)
+goto error;
+if (storageBackendCreateQemuImgSecretObject(cmd, inputSecretPath,
+inputSecretAlias) < 0)
+goto error;
+}
+
 if (convertStep != VIR_STORAGE_VOL_ENCRYPT_CONVERT) {
 if (storageBackendCreateQemuImgSetOptions(cmd, encinfo, info) < 0)
 goto error;
@@ -1163,19 +1186,32 @@ 
virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
 virCommandAddArgFormat(cmd, "%lluK", info.size_arg);
 } else {
 /* source */
-virCommandAddArgFormat(cmd, "driver=%s,file.filename=%s",
-   info.inputType, info.inputPath);
+if (inputenc)
+virCommandAddArgFormat(cmd,
+   
"driver=luks,file.filename=%s,key-secret=%s",
+   info.inputPath, inputSecretAlias);
+else
+virCommandAddArgFormat(cmd, "driver=%s,file.filename=%s",
+   info.inputType, info.inputPath);
 
 /* dest */
-virCommandAddArgFormat(cmd, "driver=%s,file.filename=%s,key-secret=%s",
-   info.type, info.path, info.secretAlias);
+if (enc)
+virCommandAddArgFormat(cmd,
+   "driver=%s,file.filename=%s,key-secret=%s",
+   info.type, info.path, info.secretAlias);
+else
+virCommandAddArgFormat(cmd, 

[libvirt] [PATCH 1/3] storage: Remove secretPath from _virStorageBackendQemuImgInfo

[John Ferlan]

There's really no need for it to be there since it's only ever
used inside virStorageBackendCreateQemuImgCmdFromVol

Signed-off-by: John Ferlan 
---
 src/storage/storage_util.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 42a9b6abf0..b32e3ccf7d 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -716,7 +716,6 @@ struct _virStorageBackendQemuImgInfo {
 int inputFormat;
 
 char *secretAlias;
-const char *secretPath;
 };
 
 
@@ -1088,7 +1087,6 @@ 
virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
 .compat = vol->target.compat,
 .features = vol->target.features,
 .nocow = vol->target.nocow,
-.secretPath = secretPath,
 .secretAlias = NULL,
 };
 virStorageEncryptionPtr enc = vol->target.encryption;
@@ -1131,14 +1129,14 @@ 
virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
 virCommandAddArgList(cmd, "-b", info.backingPath, NULL);
 
 if (enc) {
-if (!info.secretPath) {
+if (!secretPath) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("path to secret data file is required"));
 goto error;
 }
 if (virAsprintf(, "%s_encrypt0", vol->name) < 0)
 goto error;
-if (storageBackendCreateQemuImgSecretObject(cmd, info.secretPath,
+if (storageBackendCreateQemuImgSecretObject(cmd, secretPath,
 info.secretAlias) < 0)
 goto error;
 encinfo = >encinfo;
-- 
2.17.1

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

[libvirt] [PATCH 2/3] storage: Allow for inputvol to have any format for encryption

[John Ferlan]

Commit 39cef12a9 altered/fixed the inputvol processing to create
a multistep process when using an inputvol to create an encrypted
output volume; however, it unnecessarily assumed/restricted the
inputvol to be of 'raw' format only.

Modify the processing code to allow the inputvol format to be checked
and used in order to create the encrypted volume.

Signed-off-by: John Ferlan 
---
 src/storage/storage_util.c| 15 +++--
 .../luks-convert-qcow2.argv   |  9 
 tests/storagevolxml2argvtest.c|  4 
 tests/storagevolxml2xmlin/vol-file-qcow2.xml  | 21 +++
 4 files changed, 47 insertions(+), 2 deletions(-)
 create mode 100644 tests/storagevolxml2argvdata/luks-convert-qcow2.argv
 create mode 100644 tests/storagevolxml2xmlin/vol-file-qcow2.xml

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index b32e3ccf7d..cc49a5b9f7 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -699,6 +699,7 @@ storagePloopResize(virStorageVolDefPtr vol,
 struct _virStorageBackendQemuImgInfo {
 int format;
 const char *type;
+const char *inputType;
 const char *path;
 unsigned long long size_arg;
 unsigned long long allocation;
@@ -1021,6 +1022,15 @@ 
virStorageBackendCreateQemuImgSetInfo(virStoragePoolObjPtr pool,
 return -1;
 }
 
+if (inputvol &&
+!(info->inputType =
+  virStorageFileFormatTypeToString(inputvol->target.format))) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("unknown inputvol storage vol type %d"),
+   inputvol->target.format);
+return -1;
+}
+
 if (info->preallocate && info->format != VIR_STORAGE_FILE_QCOW2) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("metadata preallocation only available with qcow2"));
@@ -1080,6 +1090,7 @@ 
virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
 struct _virStorageBackendQemuImgInfo info = {
 .format = vol->target.format,
 .type = NULL,
+.inputType = NULL,
 .path = vol->target.path,
 .allocation = vol->target.allocation,
 .encryption = !!vol->target.encryption,
@@ -1152,8 +1163,8 @@ 
virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
 virCommandAddArgFormat(cmd, "%lluK", info.size_arg);
 } else {
 /* source */
-virCommandAddArgFormat(cmd, "driver=raw,file.filename=%s",
-   info.inputPath);
+virCommandAddArgFormat(cmd, "driver=%s,file.filename=%s",
+   info.inputType, info.inputPath);
 
 /* dest */
 virCommandAddArgFormat(cmd, "driver=%s,file.filename=%s,key-secret=%s",
diff --git a/tests/storagevolxml2argvdata/luks-convert-qcow2.argv 
b/tests/storagevolxml2argvdata/luks-convert-qcow2.argv
new file mode 100644
index 00..9124f5f27c
--- /dev/null
+++ b/tests/storagevolxml2argvdata/luks-convert-qcow2.argv
@@ -0,0 +1,9 @@
+qemu-img create -f luks \
+--object secret,id=OtherDemo.img_encrypt0,file=/path/to/secretFile \
+-o key-secret=OtherDemo.img_encrypt0 \
+/var/lib/libvirt/images/OtherDemo.img 5242880K
+qemu-img convert --image-opts -n --target-image-opts \
+--object secret,id=OtherDemo.img_encrypt0,file=/path/to/secretFile \
+driver=qcow2,file.filename=/var/lib/libvirt/images/sparse-qcow2.img \
+driver=luks,file.filename=/var/lib/libvirt/images/OtherDemo.img,\
+key-secret=OtherDemo.img_encrypt0
diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c
index b795f83aee..6a9a080dd1 100644
--- a/tests/storagevolxml2argvtest.c
+++ b/tests/storagevolxml2argvtest.c
@@ -284,6 +284,10 @@ mymain(void)
 "pool-dir", "vol-file",
 "luks-convert", 0);
 
+DO_TEST("pool-dir", "vol-luks-convert",
+"pool-dir", "vol-file-qcow2",
+"luks-convert-qcow2", 0);
+
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
 
diff --git a/tests/storagevolxml2xmlin/vol-file-qcow2.xml 
b/tests/storagevolxml2xmlin/vol-file-qcow2.xml
new file mode 100644
index 00..025e7e0239
--- /dev/null
+++ b/tests/storagevolxml2xmlin/vol-file-qcow2.xml
@@ -0,0 +1,21 @@
+
+  sparse-qcow2.img
+  
+  1
+  0
+  
+/var/lib/libvirt/images/sparse-qcow2.img
+
+
+  0
+  0744
+  0
+  virt_image_t
+
+
+  1341933637.273190990
+  1341930622.047245868
+  1341930622.047245868
+
+  
+
-- 
2.17.1

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

[libvirt] [PATCH 0/3] Allow inputvol when creating vol from inputvol to be encrypted

[John Ferlan]

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

Details in the patches (and even more in the bz).

John Ferlan (3):
  storage: Remove secretPath from _virStorageBackendQemuImgInfo
  storage: Allow for inputvol to have any format for encryption
  storage: Allow inputvol to be encrypted

 src/storage/storage_util.c| 79 ---
 src/storage/storage_util.h|  1 +
 .../luks-convert-encrypt.argv | 11 +++
 .../luks-convert-encrypt2fileqcow2.argv   |  7 ++
 .../luks-convert-encrypt2fileraw.argv |  7 ++
 .../luks-convert-qcow2.argv   |  9 +++
 tests/storagevolxml2argvtest.c| 19 -
 tests/storagevolxml2xmlin/vol-encrypt1.xml| 21 +
 tests/storagevolxml2xmlin/vol-encrypt2.xml| 21 +
 tests/storagevolxml2xmlin/vol-file-qcow2.xml  | 21 +
 10 files changed, 184 insertions(+), 12 deletions(-)
 create mode 100644 tests/storagevolxml2argvdata/luks-convert-encrypt.argv
 create mode 100644 
tests/storagevolxml2argvdata/luks-convert-encrypt2fileqcow2.argv
 create mode 100644 
tests/storagevolxml2argvdata/luks-convert-encrypt2fileraw.argv
 create mode 100644 tests/storagevolxml2argvdata/luks-convert-qcow2.argv
 create mode 100644 tests/storagevolxml2xmlin/vol-encrypt1.xml
 create mode 100644 tests/storagevolxml2xmlin/vol-encrypt2.xml
 create mode 100644 tests/storagevolxml2xmlin/vol-file-qcow2.xml

-- 
2.17.1

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

Re: [libvirt] [PATCH v4 0/2] Fix detection of slow guest shutdown

[Christian Ehrhardt]

On Tue, Aug 21, 2018 at 2:34 PM Christian Ehrhardt <
christian.ehrha...@canonical.com> wrote:

> Hi,
> after a good discussion a few days ago in
>  https://www.redhat.com/archives/libvir-list/2018-August/msg00122.html
> and a short lived but back then untested v2 in
>  https://www.redhat.com/archives/libvir-list/2018-August/msg00199.html
> I finally get access to the right HW again and completed the series.
>
> Being finally retested and working I finally feel safe to submit without
> a RFC prefix. I think this would be a great addition for a better handling
> of guests with plenty of host devices passed through.
>
> With the new code in place I can shutdown systems that have 12, 16 or
> even more hostdevs attached without getting into the "zombie" mode where
> libvirt will forever consider the guest as "in shutdown" as it gave up
> waiting too early because the signal zero still was able to reach it.
>
> Scaling examples (extracted with gdb):
> 16 Devices: virProcessKillPainfullyDelay (pid=67096, force=true,
> extradelay=32)
> 12 Devices: virProcessKillPainfullyDelay (pid=68251, force=true,
> extradelay=24)
>
> *Updates in v4*
> - virDebug now reports the extradelay as requested (in seconds) and
>   thereby mostly matches the gdb output seen above
> - header function prototype defines the variable name
> - clarify the usage of delay units
>   - seconds (API call)
>   - 5th of seconds (internal poll loop)
> - explain the request for 2*nhostdevs from the qemu shutdown code
>
> *Updates in v3*
> - fixup some issues found in testing and code checks
>
> *Updates in v2*
> - removed the "accept the lack of /proc/ as valid process removal"
>   approach due to valid concerns about reusing ressources.
> - added a dynamic extra wait scaling with the amount of hostdevs
>
> Christian Ehrhardt (2):
>   process: wait longer on kill per assigned Hostdev
>   process: wait longer 5->30s on hard shutdown
>

FYI after there was no further feedback I pushed the v4 with the
appropriate reviewed by tags.
Thanks everybody for your participation!


>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_process.c  |  7 +--
>  src/util/virprocess.c| 22 ++
>  src/util/virprocess.h|  3 +++
>  4 files changed, 27 insertions(+), 6 deletions(-)
>
> --
> 2.17.1
>
>

-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virsh: Don't break loop of domblkinfo for disks

[Peter Krempa]

On Tue, Aug 21, 2018 at 21:23:42 +0800, Han Han wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1619625
> 
> --all option is added to cmdDomblkinfo since commit 62c39193 allowing to
> show all block devices info. Remove its 'goto cleanup' part in case it breaks
> the loop of domblkinfo for all disks. Remove unnecessary variables and the
> condition part after virDomainGetBlockInfo returning fail.
> 
> Signed-off-by: Han Han 
> ---
>  tools/virsh-domain-monitor.c | 18 ++
>  1 file changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index b9b4f9739b..ee926baae8 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -476,7 +476,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>  size_t i;
>  xmlNodePtr *disks = NULL;
>  char *target = NULL;
> -char *protocol = NULL;
>  
>  if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
>  return false;
> @@ -490,7 +489,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>  human = vshCommandOptBool(cmd, "human");
>  
>  if (all) {
> -bool active = virDomainIsActive(dom) == 1;
>  int rc;
>  
>  if (virshDomainGetXML(ctl, cmd, 0, , ) < 0)
> @@ -505,29 +503,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>  
>  for (i = 0; i < ndisks; i++) {
>  ctxt->node = disks[i];
> -protocol = virXPathString("string(./source/@protocol)", ctxt);
>  target = virXPathString("string(./target/@dev)", ctxt);

Well, here you can check also whether the  element is present
...

>  
>  rc = virDomainGetBlockInfo(dom, target, , 0);

... and skip this.

>  
>  if (rc < 0) {
> -/* If protocol is present that's an indication of a networked
> - * storage device which cannot provide statistics, so 
> generate
> - * 0 based data and get the next disk. */
> -if (protocol && !active &&
> -virGetLastErrorCode() == VIR_ERR_INTERNAL_ERROR &&
> -virGetLastErrorDomain() == VIR_FROM_STORAGE) {
> -memset(, 0, sizeof(info));
> -vshResetLibvirtError();
> -} else {
> -goto cleanup;
> -}
> +memset(, 0, sizeof(info));
> +vshResetLibvirtError();

Rather than skipping errors.

This solution may skip other errors as well. Depends whether we are okay
with that in such case.

On the other hand getting rid of the code above looks good.

>  }
>  
>  cmdDomblkinfoPrint(ctl, , target, human, false);
>  
>  VIR_FREE(target);
> -VIR_FREE(protocol);
>  }
>  } else {
>  if (virDomainGetBlockInfo(dom, device, , 0) < 0)
> @@ -541,7 +528,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>   cleanup:
>  virshDomainFree(dom);
>  VIR_FREE(target);
> -VIR_FREE(protocol);
>  VIR_FREE(disks);
>  xmlXPathFreeContext(ctxt);
>  xmlFreeDoc(xmldoc);
> -- 
> 2.18.0
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


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

[libvirt] [jenkins-ci PATCH] projects: Fix comments for libvirt-dbus

[Andrea Bolognani]

The comments were accurate but misplaced. Fix that.

Signed-off-by: Andrea Bolognani 
---
Pushed as somewhat embarrassing and utterly trivial.

 projects/libvirt-dbus.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/projects/libvirt-dbus.yaml b/projects/libvirt-dbus.yaml
index 0fde390..74163b7 100644
--- a/projects/libvirt-dbus.yaml
+++ b/projects/libvirt-dbus.yaml
@@ -18,7 +18,7 @@
   parent_jobs: 'libvirt-glib-master-build'
   - autotools-syntax-check-job:
   parent_jobs: 'libvirt-dbus-master-build'
-  # The test suite uses Python 3, which CentOS 7 doesn't include
+  # syntax-check uses Python 3, which CentOS 7 doesn't include
   machines:
 - libvirt-debian-9
 - libvirt-fedora-27
@@ -28,7 +28,7 @@
 - libvirt-freebsd-11
   - autotools-check-job:
   parent_jobs: 'libvirt-dbus-master-syntax-check'
-  # syntax-check uses Python 3, which CentOS 7 doesn't include
+  # The test suite uses Python 3, which CentOS 7 doesn't include
   machines:
 - libvirt-debian-9
 - libvirt-fedora-27
-- 
2.17.1

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

Re: [libvirt] [jenkins-ci PATCH v2 2/2] projects: Document rationale for skipping jobs

[Erik Skultety]

On Tue, Aug 21, 2018 at 01:40:49PM +0200, Andrea Bolognani wrote:
> On Tue, 2018-08-21 at 13:21 +0200, Erik Skultety wrote:
> > On Tue, Aug 21, 2018 at 12:38:06PM +0200, Andrea Bolognani wrote:
> > > In general, we strive for full coverage and build all
> > > projects on all targets; however, in some cases that's
> > > simply not feasible and we have to skip the corresponding
> > > job. Document the rationale for each such case.
> > >
> > > Signed-off-by: Andrea Bolognani 
> > > ---
> > >  projects/libvirt-dbus.yaml| 3 +++
> > >  projects/libvirt-sandbox.yaml | 3 +++
> > >  projects/libvirt-tck.yaml | 2 ++
> > >  projects/libvirt.yaml | 2 ++
> > >  projects/virt-manager.yaml| 1 +
> > >  projects/virt-viewer.yaml | 2 ++
> > >  6 files changed, 13 insertions(+)
> > >
> > > diff --git a/projects/libvirt-dbus.yaml b/projects/libvirt-dbus.yaml
> > > index fdfb615..459bd96 100644
> > > --- a/projects/libvirt-dbus.yaml
> > > +++ b/projects/libvirt-dbus.yaml
> > > @@ -2,6 +2,7 @@
> > >  - project:
> > >  name: libvirt-dbus
> > >  machines:
> >
> > I'd appreciate an empty line above the comments, it's adds to the 
> > readability.
> >
> > > +  # Debian 8 doesn't have a recent enough GLib
> > >- libvirt-centos-7
> > >- libvirt-debian-9
> > >- libvirt-fedora-27
>
> Existing comments don't have that, and in general the job definitions

Oh, well, I must have missed the few occurrences, never mind, it doesn't bother
me much really.

Erik

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

Re: [libvirt] [PATCH v2 0/2] Add .domainGetHostname() support for QEMU driver.

[Julio Faracco]

Em ter, 21 de ago de 2018 às 00:28, Han Han  escreveu:
>
>
>
> On Tue, Aug 21, 2018 at 10:34 AM, Julio Faracco  wrote:
>>
>> This serie adds a new function into QEMU Guest Agent handlers to use
>> the QEMU command 'guest-get-host-name' to retrieve the domain hostname.
>> This approach requires QEMU-GA running inside the guest, but it is the
>> fastest and easiest way to get this info.
>>
>> Julio Faracco (2):
>>   qemu: implementing qemuAgentGetHostname() function.
>>   qemu: adding domainGetHostname support for QEMU.
>
> How about updating improvements part of news.xml to inform users that they 
> could use
> virDomainGetHostname to qemu VMs :)?

I will wait for others feedback before sending a single patch to add
info into news.xml.
Thanks Han for the suggestion.

>>
>>
>>  src/qemu/qemu_agent.c  | 42 ++
>>  src/qemu/qemu_agent.h  |  4 
>>  src/qemu/qemu_driver.c | 40 
>>  3 files changed, 86 insertions(+)
>>
>> --
>> 2.17.1
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
>
>
>
> --
> Best regards,
> ---
> Han Han
> Quality Engineer
> Redhat.
>
> Email: h...@redhat.com
> Phone: +861065339333

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

[libvirt] [PATCH] virsh: Don't break loop of domblkinfo for disks

[Han Han]

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

--all option is added to cmdDomblkinfo since commit 62c39193 allowing to
show all block devices info. Remove its 'goto cleanup' part in case it breaks
the loop of domblkinfo for all disks. Remove unnecessary variables and the
condition part after virDomainGetBlockInfo returning fail.

Signed-off-by: Han Han 
---
 tools/virsh-domain-monitor.c | 18 ++
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index b9b4f9739b..ee926baae8 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -476,7 +476,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 size_t i;
 xmlNodePtr *disks = NULL;
 char *target = NULL;
-char *protocol = NULL;
 
 if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
 return false;
@@ -490,7 +489,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 human = vshCommandOptBool(cmd, "human");
 
 if (all) {
-bool active = virDomainIsActive(dom) == 1;
 int rc;
 
 if (virshDomainGetXML(ctl, cmd, 0, , ) < 0)
@@ -505,29 +503,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 
 for (i = 0; i < ndisks; i++) {
 ctxt->node = disks[i];
-protocol = virXPathString("string(./source/@protocol)", ctxt);
 target = virXPathString("string(./target/@dev)", ctxt);
 
 rc = virDomainGetBlockInfo(dom, target, , 0);
 
 if (rc < 0) {
-/* If protocol is present that's an indication of a networked
- * storage device which cannot provide statistics, so generate
- * 0 based data and get the next disk. */
-if (protocol && !active &&
-virGetLastErrorCode() == VIR_ERR_INTERNAL_ERROR &&
-virGetLastErrorDomain() == VIR_FROM_STORAGE) {
-memset(, 0, sizeof(info));
-vshResetLibvirtError();
-} else {
-goto cleanup;
-}
+memset(, 0, sizeof(info));
+vshResetLibvirtError();
 }
 
 cmdDomblkinfoPrint(ctl, , target, human, false);
 
 VIR_FREE(target);
-VIR_FREE(protocol);
 }
 } else {
 if (virDomainGetBlockInfo(dom, device, , 0) < 0)
@@ -541,7 +528,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
  cleanup:
 virshDomainFree(dom);
 VIR_FREE(target);
-VIR_FREE(protocol);
 VIR_FREE(disks);
 xmlXPathFreeContext(ctxt);
 xmlFreeDoc(xmldoc);
-- 
2.18.0

--
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 Tue, 2018-08-21 at 19:55 +0800, Yi Min Zhao wrote:
> 在 2018/8/21 下午7:00, Andrea Bolognani 写道:
> > On Tue, 2018-08-21 at 17:35 +0800, Yi Min Zhao wrote:
> > > I want to ask a question. For pci address, any pci device can't use slot 
> > > 0.
> > > Is that a reason why PCI part could treat all zeros as empty address?
> > 
> > A PCI address where all attributes are zero can't be used, so
> > there's no ambiguity there; same for a zPCI address where all
> > attributes are zero, which also can't be used.
> 
> Yes, uid must be non-zero value. But the user could only define fid like
> you mentioned below. Validation check happens during parsing xml.
> So if the user only defines fid=0, zpci address (fid=0, uid=0 although the
> user doesn't specify uid value) is going to be invalid as PCI address
> check logic.

In the context of PCI addresses, empty implies invalid but during
XML parsing the address is only validated if it's not empty. If
that was not the case, an empty address would never have a chance
to be automatically filled in by libvirt :)

> UID and FID must be unique separately, but we can't treat
> them as a combination. This doesn't like PCI
> address(domain:bus:slot:function).

I asked a question about this in reply to patch 09/12 yesterday
by the way, and it would be great if you could answer it.

> > > For zPCI address, if we use the same strategy as PCI part and user
> > > wants to assign fid=0 to the specific device, he might encounter
> > > assignment failure. At least, according to the doc, 0 shoud be a valid
> > > value to assign to fid.
> > 
> > If the user wants to use a specific zPCI address they can simply
> > specify both attributes, eg. uid=1,fid=0 will work just fine with
> > the proposed approach and won't produce errors or cause a new zPCI
> > address to be automatically assigned.
> > 
> > If the user only specified fid=0 while leaving uid unspecified,
> > well, an address is going to be picked for them.
> 
> This would be an empty zpci address while parsing XML so that we might
> pick a zpci address with non-zero fid. For example:
> 
> dev1: fid=0 (this address would be treated as an unassigned zpci address)
> dev2 (no definition for both fid and uid)
> 
> Then the process of assigning addresses will iterate devices as type by 
> type.
> If dev2 is processed before dev1, dev2's fid would be assigned by 0. 
> This causes
> dev1's fid=1. The result doesn't match what the user wants.

This would be the same as specifying a partial PCI address such as

  

and getting an address with slot != 0x00 back: surprising, perhaps,
but that's the way it's been with PCI addresses forever so you can
assume users are familiar with it by now.

For zPCI addresses to be inconsistent with this behavior would be
utterly confusing; moreover, if the user really needs the uid and
fid to have certain values, all they have to do is specify both
and libvirt will not attempt to override them.

-- 
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/2] process: wait longer on kill per assigned Hostdev

[Daniel P . Berrangé]

On Tue, Aug 21, 2018 at 02:33:25PM +0200, Christian Ehrhardt wrote:
> It was found that in cases with host devices virProcessKillPainfully
> might be able to send signal zero to the target PID for quite a while
> with the process already being gone from /proc/.
> 
> That is due to cleanup and reset of devices which might include a
> secondary bus reset that on top of the actions taken has a 1s delay
> to let the bus settle. Due to that guests with plenty of Host devices
> could easily exceed the default timeouts.
> 
> To solve that, this adds an extra delay of 2s per hostdev that is associated
> to a VM.
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_process.c  |  7 +--
>  src/util/virprocess.c| 20 +---
>  src/util/virprocess.h|  3 +++
>  4 files changed, 26 insertions(+), 5 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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 v4 1/2] process: wait longer on kill per assigned Hostdev

[Christian Ehrhardt]

It was found that in cases with host devices virProcessKillPainfully
might be able to send signal zero to the target PID for quite a while
with the process already being gone from /proc/.

That is due to cleanup and reset of devices which might include a
secondary bus reset that on top of the actions taken has a 1s delay
to let the bus settle. Due to that guests with plenty of Host devices
could easily exceed the default timeouts.

To solve that, this adds an extra delay of 2s per hostdev that is associated
to a VM.

Signed-off-by: Christian Ehrhardt 
---
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_process.c  |  7 +--
 src/util/virprocess.c| 20 +---
 src/util/virprocess.h|  3 +++
 4 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ca4a192a4a..47ea35f864 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2605,6 +2605,7 @@ virProcessGetPids;
 virProcessGetStartTime;
 virProcessKill;
 virProcessKillPainfully;
+virProcessKillPainfullyDelay;
 virProcessNamespaceAvailable;
 virProcessRunInMountNamespace;
 virProcessSchedPolicyTypeFromString;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index b42fda850f..64097b29cb 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6817,8 +6817,11 @@ qemuProcessKill(virDomainObjPtr vm, unsigned int flags)
 return 0;
 }
 
-ret = virProcessKillPainfully(vm->pid,
-  !!(flags & VIR_QEMU_PROCESS_KILL_FORCE));
+/* Request an extra delay of two seconds per current nhostdevs
+ * to be safe against stalls by the kernel freeing up the resources */
+ret = virProcessKillPainfullyDelay(vm->pid,
+   !!(flags & VIR_QEMU_PROCESS_KILL_FORCE),
+   vm->def->nhostdevs * 2);
 
 return ret;
 }
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index ecea27a2d4..4c7f2ed97c 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -341,15 +341,21 @@ int virProcessKill(pid_t pid, int sig)
  * Returns 0 if it was killed gracefully, 1 if it
  * was killed forcibly, -1 if it is still alive,
  * or another error occurred.
+ *
+ * Callers can proide an extra delay in seconds to
+ * wait longer than the default.
  */
 int
-virProcessKillPainfully(pid_t pid, bool force)
+virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay)
 {
 size_t i;
 int ret = -1;
+/* This is in 1/5th seconds since polling is on a 0.2s interval */
+unsigned int polldelay = 75 + (extradelay*5);
 const char *signame = "TERM";
 
-VIR_DEBUG("vpid=%lld force=%d", (long long)pid, force);
+VIR_DEBUG("vpid=%lld force=%d extradelay=%u",
+  (long long)pid, force, extradelay);
 
 /* This loop sends SIGTERM, then waits a few iterations (10 seconds)
  * to see if it dies. If the process still hasn't exited, and
@@ -357,9 +363,12 @@ virProcessKillPainfully(pid_t pid, bool force)
  * wait up to 5 seconds more for the process to exit before
  * returning.
  *
+ * An extra delay can be passed by the caller for cases that are
+ * expected to clean up slower than usual.
+ *
  * Note that setting @force could result in dataloss for the process.
  */
-for (i = 0; i < 75; i++) {
+for (i = 0; i < polldelay; i++) {
 int signum;
 if (i == 0) {
 signum = SIGTERM; /* kindly suggest it should exit */
@@ -402,6 +411,11 @@ virProcessKillPainfully(pid_t pid, bool force)
 }
 
 
+int virProcessKillPainfully(pid_t pid, bool force)
+{
+return virProcessKillPainfullyDelay(pid, force, 0);
+}
+
 #if HAVE_SCHED_GETAFFINITY
 
 int virProcessSetAffinity(pid_t pid, virBitmapPtr map)
diff --git a/src/util/virprocess.h b/src/util/virprocess.h
index 3c5a882772..5faa0892fe 100644
--- a/src/util/virprocess.h
+++ b/src/util/virprocess.h
@@ -55,6 +55,9 @@ virProcessWait(pid_t pid, int *exitstatus, bool raw)
 int virProcessKill(pid_t pid, int sig);
 
 int virProcessKillPainfully(pid_t pid, bool force);
+int virProcessKillPainfullyDelay(pid_t pid,
+ bool force,
+ unsigned int extradelay);
 
 int virProcessSetAffinity(pid_t pid, virBitmapPtr map);
 
-- 
2.17.1

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

[libvirt] [PATCH v4 2/2] process: wait longer 5->30s on hard shutdown

[Christian Ehrhardt]

In cases where virProcessKillPainfully already reailizes that
SIGTERM wasn't enough we are partially on a bad path already.
Maybe the system is overloaded or having serious trouble to free and
reap resources in time.

In those case give the SIGKILL that was sent after 10 seconds some more
time to take effect if force was set (only then we are falling back to
SIGKILL anyway).

Signed-off-by: Christian Ehrhardt 
Reviewed-by: Daniel P. Berrangé 
---
 src/util/virprocess.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 4c7f2ed97c..3988f5546c 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -351,7 +351,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, 
unsigned int extradelay)
 size_t i;
 int ret = -1;
 /* This is in 1/5th seconds since polling is on a 0.2s interval */
-unsigned int polldelay = 75 + (extradelay*5);
+unsigned int polldelay = (force ? 200 : 75) + (extradelay*5);
 const char *signame = "TERM";
 
 VIR_DEBUG("vpid=%lld force=%d extradelay=%u",
@@ -360,7 +360,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, 
unsigned int extradelay)
 /* This loop sends SIGTERM, then waits a few iterations (10 seconds)
  * to see if it dies. If the process still hasn't exited, and
  * @force is requested, a SIGKILL will be sent, and this will
- * wait up to 5 seconds more for the process to exit before
+ * wait up to 30 seconds more for the process to exit before
  * returning.
  *
  * An extra delay can be passed by the caller for cases that are
-- 
2.17.1

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

[libvirt] [PATCH v4 0/2] Fix detection of slow guest shutdown

[Christian Ehrhardt]

Hi,
after a good discussion a few days ago in
 https://www.redhat.com/archives/libvir-list/2018-August/msg00122.html
and a short lived but back then untested v2 in
 https://www.redhat.com/archives/libvir-list/2018-August/msg00199.html
I finally get access to the right HW again and completed the series.

Being finally retested and working I finally feel safe to submit without
a RFC prefix. I think this would be a great addition for a better handling
of guests with plenty of host devices passed through.

With the new code in place I can shutdown systems that have 12, 16 or
even more hostdevs attached without getting into the "zombie" mode where
libvirt will forever consider the guest as "in shutdown" as it gave up
waiting too early because the signal zero still was able to reach it.

Scaling examples (extracted with gdb):
16 Devices: virProcessKillPainfullyDelay (pid=67096, force=true, extradelay=32)
12 Devices: virProcessKillPainfullyDelay (pid=68251, force=true, extradelay=24)

*Updates in v4*
- virDebug now reports the extradelay as requested (in seconds) and
  thereby mostly matches the gdb output seen above
- header function prototype defines the variable name
- clarify the usage of delay units
  - seconds (API call)
  - 5th of seconds (internal poll loop)
- explain the request for 2*nhostdevs from the qemu shutdown code

*Updates in v3*
- fixup some issues found in testing and code checks

*Updates in v2*
- removed the "accept the lack of /proc/ as valid process removal"
  approach due to valid concerns about reusing ressources.
- added a dynamic extra wait scaling with the amount of hostdevs

Christian Ehrhardt (2):
  process: wait longer on kill per assigned Hostdev
  process: wait longer 5->30s on hard shutdown

 src/libvirt_private.syms |  1 +
 src/qemu/qemu_process.c  |  7 +--
 src/util/virprocess.c| 22 ++
 src/util/virprocess.h|  3 +++
 4 files changed, 27 insertions(+), 6 deletions(-)

-- 
2.17.1

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

Re: [libvirt] [PATCH v3 1/2] process: wait longer on kill per assigned Hostdev

[Daniel P . Berrangé]

On Tue, Aug 21, 2018 at 02:20:21PM +0200, Christian Ehrhardt wrote:
> On Tue, Aug 21, 2018 at 1:15 PM Daniel P. Berrangé 
> wrote:
> 
> > On Tue, Aug 14, 2018 at 11:27:33AM +0200, Christian Ehrhardt wrote:
> > > It was found that in cases with host devices virProcessKillPainfully
> > > might be able to send signal zero to the target PID for quite a while
> > > with the process already being gone from /proc/.
> > >
> > > That is due to cleanup and reset of devices which might include a
> > > secondary bus reset that on top of the actions taken has a 1s delay
> > > to let the bus settle. Due to that guests with plenty of Host devices
> > > could easily exceed the default timeouts.
> > >
> > > To solve that, this adds an extra delay of 2s per hostdev that is
> > associated
> > > to a VM.
> > >
> > > Signed-off-by: Christian Ehrhardt 
> > > ---
> > >  src/libvirt_private.syms |  1 +
> > >  src/qemu/qemu_process.c  |  5 +++--
> > >  src/util/virprocess.c| 18 +++---
> > >  src/util/virprocess.h|  1 +
> > >  4 files changed, 20 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > > index ca4a192a4a..47ea35f864 100644
> > > --- a/src/libvirt_private.syms
> > > +++ b/src/libvirt_private.syms
> > > @@ -2605,6 +2605,7 @@ virProcessGetPids;
> > >  virProcessGetStartTime;
> > >  virProcessKill;
> > >  virProcessKillPainfully;
> > > +virProcessKillPainfullyDelay;
> > >  virProcessNamespaceAvailable;
> > >  virProcessRunInMountNamespace;
> > >  virProcessSchedPolicyTypeFromString;
> > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > > index 02fdc55156..b7bf8813da 100644
> > > --- a/src/qemu/qemu_process.c
> > > +++ b/src/qemu/qemu_process.c
> > > @@ -6814,8 +6814,9 @@ qemuProcessKill(virDomainObjPtr vm, unsigned int
> > flags)
> > >  return 0;
> > >  }
> > >
> > > -ret = virProcessKillPainfully(vm->pid,
> > > -  !!(flags &
> > VIR_QEMU_PROCESS_KILL_FORCE));
> > > +ret = virProcessKillPainfullyDelay(vm->pid,
> > > +   !!(flags &
> > VIR_QEMU_PROCESS_KILL_FORCE),
> > > +   vm->def->nhostdevs * 2);
> >
> > This API contract is a bit wierd. You've got an arbitray x2 multiplier
> > here...
> >
> 
> Out of the discussion with Alex I derived that due to the 1 sec settling of
> the bus plus some extra work 2 seconds per device would be a good rule of
> thumb.
> The delay is meant to be in seconds and here it requests 2*nhostdevs to get
> the amount needed.
> 
> And I'd not want to make the "unit" of the call "double-seconds" :-)
> 
> Let me add a comment at this call to explain the reasoning on the
> multiplier here.


> > > -virProcessKillPainfully(pid_t pid, bool force)
> > > +virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int
> > extradelay)
> > >  {
> > >  size_t i;
> > >  int ret = -1;
> > > +unsigned int delay = 75 + (extradelay*5);
> >
> > ...and it gets another x5 multiplier here.
> >
> 
> That *5 is due to the internal unit it polls being 0.2 seconds.
> I think externally anything other than seconds (double-seconds,
> half-deca-seconds, ...) makes no sense.

Opps, yes, I mis-understood the behaviour of the code. A comment is fine


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 1/2] process: wait longer on kill per assigned Hostdev

[Christian Ehrhardt]

On Tue, Aug 21, 2018 at 1:15 PM Daniel P. Berrangé 
wrote:

> On Tue, Aug 14, 2018 at 11:27:33AM +0200, Christian Ehrhardt wrote:
> > It was found that in cases with host devices virProcessKillPainfully
> > might be able to send signal zero to the target PID for quite a while
> > with the process already being gone from /proc/.
> >
> > That is due to cleanup and reset of devices which might include a
> > secondary bus reset that on top of the actions taken has a 1s delay
> > to let the bus settle. Due to that guests with plenty of Host devices
> > could easily exceed the default timeouts.
> >
> > To solve that, this adds an extra delay of 2s per hostdev that is
> associated
> > to a VM.
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >  src/libvirt_private.syms |  1 +
> >  src/qemu/qemu_process.c  |  5 +++--
> >  src/util/virprocess.c| 18 +++---
> >  src/util/virprocess.h|  1 +
> >  4 files changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index ca4a192a4a..47ea35f864 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -2605,6 +2605,7 @@ virProcessGetPids;
> >  virProcessGetStartTime;
> >  virProcessKill;
> >  virProcessKillPainfully;
> > +virProcessKillPainfullyDelay;
> >  virProcessNamespaceAvailable;
> >  virProcessRunInMountNamespace;
> >  virProcessSchedPolicyTypeFromString;
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 02fdc55156..b7bf8813da 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -6814,8 +6814,9 @@ qemuProcessKill(virDomainObjPtr vm, unsigned int
> flags)
> >  return 0;
> >  }
> >
> > -ret = virProcessKillPainfully(vm->pid,
> > -  !!(flags &
> VIR_QEMU_PROCESS_KILL_FORCE));
> > +ret = virProcessKillPainfullyDelay(vm->pid,
> > +   !!(flags &
> VIR_QEMU_PROCESS_KILL_FORCE),
> > +   vm->def->nhostdevs * 2);
>
> This API contract is a bit wierd. You've got an arbitray x2 multiplier
> here...
>

Out of the discussion with Alex I derived that due to the 1 sec settling of
the bus plus some extra work 2 seconds per device would be a good rule of
thumb.
The delay is meant to be in seconds and here it requests 2*nhostdevs to get
the amount needed.

And I'd not want to make the "unit" of the call "double-seconds" :-)

Let me add a comment at this call to explain the reasoning on the
multiplier here.

> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> > index ecea27a2d4..46360cc051 100644
> > --- a/src/util/virprocess.c
> > +++ b/src/util/virprocess.c
> > @@ -341,15 +341,19 @@ int virProcessKill(pid_t pid, int sig)
> >   * Returns 0 if it was killed gracefully, 1 if it
> >   * was killed forcibly, -1 if it is still alive,
> >   * or another error occurred.
> > + *
> > + * Callers can proide an extra delay to wait longer
> > + * than the default.
>
> Mention that it is in "seconds"
>

Ack

>   */
> >  int
> > -virProcessKillPainfully(pid_t pid, bool force)
> > +virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int
> extradelay)
> >  {
> >  size_t i;
> >  int ret = -1;
> > +unsigned int delay = 75 + (extradelay*5);
>
> ...and it gets another x5 multiplier here.
>

That *5 is due to the internal unit it polls being 0.2 seconds.
I think externally anything other than seconds (double-seconds,
half-deca-seconds, ...) makes no sense.

I'll call it "polldelay" instead to make it more clear that.
That uncommon *5 ratio was there before, just not as redable (it just
defined 75 in the loop header).
I'll add some text about it as well to be sure.


> Feels nicer to just have the caller provide a x10 multiplier and
> honour that directly
>

See above, the unit being seconds seems the most readable IMHO.
With the changes made I think it won't be as confusing.

>  const char *signame = "TERM";
> >
> > -VIR_DEBUG("vpid=%lld force=%d", (long long)pid, force);
> > +VIR_DEBUG("vpid=%lld force=%d delay=%u", (long long)pid, force,
> pid);
>
> s/pid/extradelay/
>

Yes, that was mentioned by Bjoern and queued that way.
Also in the text the delay= should be extradelay= which is ready that way.

>
> >  /* This loop sends SIGTERM, then waits a few iterations (10 seconds)
> >   * to see if it dies. If the process still hasn't exited, and
> > @@ -357,9 +361,12 @@ virProcessKillPainfully(pid_t pid, bool force)
> >   * wait up to 5 seconds more for the process to exit before
> >   * returning.
> >   *
> > + * An extra delay can be specified for cases that are expected to
> clean
> > + * up slower than usual.
> > + *
> >   * Note that setting @force could result in dataloss for the
> process.
> >   */
> > -for (i = 0; i < 75; i++) {
> > +for (i = 0; i < delay; i++) {
> >  int signum;
> >  if (i == 0) {
> >  

Re: [libvirt] [PATCH v3 2/2] process: wait longer 5->30s on hard shutdown

[Christian Ehrhardt]

On Tue, Aug 21, 2018 at 1:16 PM Daniel P. Berrangé 
wrote:

> On Tue, Aug 14, 2018 at 11:27:34AM +0200, Christian Ehrhardt wrote:
> > In cases where virProcessKillPainfully already reailizes that
> > SIGTERM wasn't enough we are partially on a bad path already.
> > Maybe the system is overloaded or having serious trouble to free and
> > reap resources in time.
> >
> > In those case give the SIGKILL that was sent after 10 seconds some more
> > time to take effect if force was set (only then we are falling back to
> > SIGKILL anyway).
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >  src/util/virprocess.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> > index 46360cc051..dda8916284 100644
> > --- a/src/util/virprocess.c
> > +++ b/src/util/virprocess.c
> > @@ -350,7 +350,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force,
> unsigned int extradelay)
> >  {
> >  size_t i;
> >  int ret = -1;
> > -unsigned int delay = 75 + (extradelay*5);
> > +unsigned int delay = (force ? 200 : 75) + (extradelay*5);
> >  const char *signame = "TERM";
> >
> >  VIR_DEBUG("vpid=%lld force=%d delay=%u", (long long)pid, force,
> pid);
> > @@ -358,7 +358,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force,
> unsigned int extradelay)
> >  /* This loop sends SIGTERM, then waits a few iterations (10 seconds)
> >   * to see if it dies. If the process still hasn't exited, and
> >   * @force is requested, a SIGKILL will be sent, and this will
> > - * wait up to 5 seconds more for the process to exit before
> > + * wait up to 30 seconds more for the process to exit before
> >   * returning.
> >   *
> >   * An extra delay can be specified for cases that are expected to
> clean
>
> Reviewed-by: Daniel P. Berrangé 
>

Thanks, added to the v4 submission queue ...


> 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 :|
>


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] conf: Parse guestfwd channel device info again

[Michal Privoznik]

On 08/21/2018 02:15 PM, Andrea Bolognani wrote:
> On Tue, 2018-08-21 at 11:01 +0200, Andrea Bolognani wrote:
>> On Mon, 2018-08-20 at 18:52 +0200, Michal Privoznik wrote:
>> [...]
>>> -if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
>>> -def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) {
>>> -VIR_DEBUG("Ignoring device address for gustfwd channel");
>>> -} else if (virDomainDeviceInfoParseXML(xmlopt, node,
>>> -   >info, flags) < 0) {
>>> +if (virDomainDeviceInfoParseXML(xmlopt, node, >info, flags) < 0)
>>>  goto error;
>>> -}
>>> -
>>
>> I agree that fixing Bug 1610072 is more important than preventing
>> Bug 1172526 from showing up again, but it would be great if we could
>> make it so both are fixed...
>>
>> How about parsing the info and then clearing out the address only if
>> it's a guestfwd channel? The existing virDomainDeviceInfoClear() is
>> a bit too thorough, but perhaps you can introduce a new
>> virDomainDeviceInfoClearAddress() that only zeroes out the address
>> and use that here.
> 
> Given the issues with the approach I proposed, let's just cut our
> losses and merge your original attempt instead. Sorry for wasting
> your time :(
> 

No worries.

> Reviewed-by: Andrea Bolognani 
> 

Pushed thanks.

Michal

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

Re: [libvirt] [PATCH] conf: Parse guestfwd channel device info again

[Andrea Bolognani]

On Tue, 2018-08-21 at 11:01 +0200, Andrea Bolognani wrote:
> On Mon, 2018-08-20 at 18:52 +0200, Michal Privoznik wrote:
> [...]
> > -if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
> > -def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) {
> > -VIR_DEBUG("Ignoring device address for gustfwd channel");
> > -} else if (virDomainDeviceInfoParseXML(xmlopt, node,
> > -   >info, flags) < 0) {
> > +if (virDomainDeviceInfoParseXML(xmlopt, node, >info, flags) < 0)
> >  goto error;
> > -}
> > -
> 
> I agree that fixing Bug 1610072 is more important than preventing
> Bug 1172526 from showing up again, but it would be great if we could
> make it so both are fixed...
> 
> How about parsing the info and then clearing out the address only if
> it's a guestfwd channel? The existing virDomainDeviceInfoClear() is
> a bit too thorough, but perhaps you can introduce a new
> virDomainDeviceInfoClearAddress() that only zeroes out the address
> and use that here.

Given the issues with the approach I proposed, let's just cut our
losses and merge your original attempt instead. Sorry for wasting
your time :(

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 v2 1/2] device_conf: Split virDomainDeviceInfoClear

[Andrea Bolognani]

On Tue, 2018-08-21 at 13:45 +0200, Michal Privoznik wrote:
> On 08/21/2018 01:15 PM, Andrea Bolognani wrote:
> > On Tue, 2018-08-21 at 12:12 +0200, Michal Privoznik wrote:
> > >  void
> > > -virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
> > > +virDomainDeviceInfoClearAddress(virDomainDeviceInfoPtr info)
> > >  {
> > > -VIR_FREE(info->alias);
> > >  memset(>addr, 0, sizeof(info->addr));
> > >  info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
> > >  VIR_FREE(info->romfile);
> > 
> > Now virDomainDeviceInfoClearAddress() clears out *way more* than
> > just the address, including romfile and other information not
> > visible in the context... It should really only call memset and
> > reset info->type in order for the name not to be very misleading.
> > 
> > [...]
> > > +void
> > > +virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
> > > +{
> > > +VIR_FREE(info->alias);
> > > +virDomainDeviceInfoClearAddress(info);
> > > +}
> > 
> > romfile and friends should be cleared out here along with the alias
> > and, of course, the address :)
> 
> Actually, this is not going to work either. If somebody specifies:
>  into guestfwd XML we will parse it and format it
> back. The only solution I see is to have a flag to
> virDomainDeviceInfoParseXML() that parses just alias and nothing else.
> Which of course doesn't fit into the rest of the flags it already has.
> So the flag has to be the opposite: allow alias parsing and then fix all
> the places where it is called.
> 
> Sigh. This is gotten too far for such unusual corner case. I'm just
> going to close the bug as WONTFIX.
> 
> Thanks anyway.

You're right, I failed to consider that.

Given the situation I'd say that we should merge the initial fix
and err on the side of parsing/formatting information that doesn't
apply to the device rather than skipping information that does: the
former is suboptimal, but the latter is outright broken.

I'll ACK the v1 patch.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
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 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

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

[Gerd Hoffmann]

On Tue, Aug 21, 2018 at 11:45:20AM +0200, Paolo Bonzini wrote:
> On 21/08/2018 09:45, Gerd Hoffmann wrote:
> > +qemu_mutex_lock(>lock);
> >  if (ssd->cursor) {
> > +QEMUCursor *c = ssd->cursor;
> >  assert(ssd->dcl.con);
> > +cursor_get(c);
> > +qemu_mutex_unlock(>lock);
> >  dpy_cursor_define(ssd->dcl.con, ssd->cursor);
> 
> Gerd,
> 
> this ssd->cursor should be "c" in the call to dpy_cursor_define.  My
> apologies; please tell me if you'd like me to send a follow-up fix.

Fixed, pull v2 on the way.

cheers,
  Gerd

--
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 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 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 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

[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 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 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

[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 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 00/12] Ui 20180821 v2 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-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.



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 via Qemu-devel (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 04/12] sdl2: redraw correctly when scanout_mode enabled.

[Gerd Hoffmann]

From: Tao Wu via Qemu-devel 

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

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

[Yi Min Zhao]

在 2018/8/21 下午7:00, Andrea Bolognani 写道:

On Tue, 2018-08-21 at 17:35 +0800, Yi Min Zhao wrote:

I want to ask a question. For pci address, any pci device can't use slot 0.
Is that a reason why PCI part could treat all zeros as empty address?

A PCI address where all attributes are zero can't be used, so
there's no ambiguity there; same for a zPCI address where all
attributes are zero, which also can't be used.

Yes, uid must be non-zero value. But the user could only define fid like
you mentioned below. Validation check happens during parsing xml.
So if the user only defines fid=0, zpci address (fid=0, uid=0 although the
user doesn't specify uid value) is going to be invalid as PCI address
check logic. UID and FID must be unique separately, but we can't treat
them as a combination. This doesn't like PCI 
address(domain:bus:slot:function).



For zPCI address, if we use the same strategy as PCI part and user
wants to assign fid=0 to the specific device, he might encounter
assignment failure. At least, according to the doc, 0 shoud be a valid
value to assign to fid.

If the user wants to use a specific zPCI address they can simply
specify both attributes, eg. uid=1,fid=0 will work just fine with
the proposed approach and won't produce errors or cause a new zPCI
address to be automatically assigned.

If the user only specified fid=0 while leaving uid unspecified,
well, an address is going to be picked for them. This is consistent

This would be an empty zpci address while parsing XML so that we might
pick a zpci address with non-zero fid. For example:

dev1: fid=0 (this address would be treated as an unassigned zpci address)
dev2 (no definition for both fid and uid)

Then the process of assigning addresses will iterate devices as type by 
type.
If dev2 is processed before dev1, dev2's fid would be assigned by 0. 
This causes

dev1's fid=1. The result doesn't match what the user wants.

with how PCI addresses are treated so it shouldn't be a problem: if
anything, *deviating* from this behavior would cause confusion.



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

Re: [libvirt] [PATCH v2 1/2] device_conf: Split virDomainDeviceInfoClear

[Michal Privoznik]

On 08/21/2018 01:15 PM, Andrea Bolognani wrote:
> On Tue, 2018-08-21 at 12:12 +0200, Michal Privoznik wrote:
>>  void
>> -virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
>> +virDomainDeviceInfoClearAddress(virDomainDeviceInfoPtr info)
>>  {
>> -VIR_FREE(info->alias);
>>  memset(>addr, 0, sizeof(info->addr));
>>  info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
>>  VIR_FREE(info->romfile);
> 
> Now virDomainDeviceInfoClearAddress() clears out *way more* than
> just the address, including romfile and other information not
> visible in the context... It should really only call memset and
> reset info->type in order for the name not to be very misleading.
> 
> [...]
>> +void
>> +virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
>> +{
>> +VIR_FREE(info->alias);
>> +virDomainDeviceInfoClearAddress(info);
>> +}
> 
> romfile and friends should be cleared out here along with the alias
> and, of course, the address :)
> 
> With that fixed
> 
>   Reviewed-by: Andrea Bolognani 
> 

Actually, this is not going to work either. If somebody specifies:
 into guestfwd XML we will parse it and format it
back. The only solution I see is to have a flag to
virDomainDeviceInfoParseXML() that parses just alias and nothing else.
Which of course doesn't fit into the rest of the flags it already has.
So the flag has to be the opposite: allow alias parsing and then fix all
the places where it is called.

Sigh. This is gotten too far for such unusual corner case. I'm just
going to close the bug as WONTFIX.

Thanks anyway.

Michal

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

Re: [libvirt] [jenkins-ci PATCH v2 2/2] projects: Document rationale for skipping jobs

[Andrea Bolognani]

On Tue, 2018-08-21 at 13:21 +0200, Erik Skultety wrote:
> On Tue, Aug 21, 2018 at 12:38:06PM +0200, Andrea Bolognani wrote:
> > In general, we strive for full coverage and build all
> > projects on all targets; however, in some cases that's
> > simply not feasible and we have to skip the corresponding
> > job. Document the rationale for each such case.
> > 
> > Signed-off-by: Andrea Bolognani 
> > ---
> >  projects/libvirt-dbus.yaml| 3 +++
> >  projects/libvirt-sandbox.yaml | 3 +++
> >  projects/libvirt-tck.yaml | 2 ++
> >  projects/libvirt.yaml | 2 ++
> >  projects/virt-manager.yaml| 1 +
> >  projects/virt-viewer.yaml | 2 ++
> >  6 files changed, 13 insertions(+)
> > 
> > diff --git a/projects/libvirt-dbus.yaml b/projects/libvirt-dbus.yaml
> > index fdfb615..459bd96 100644
> > --- a/projects/libvirt-dbus.yaml
> > +++ b/projects/libvirt-dbus.yaml
> > @@ -2,6 +2,7 @@
> >  - project:
> >  name: libvirt-dbus
> >  machines:
> 
> I'd appreciate an empty line above the comments, it's adds to the readability.
> 
> > +  # Debian 8 doesn't have a recent enough GLib
> >- libvirt-centos-7
> >- libvirt-debian-9
> >- libvirt-fedora-27

Existing comments don't have that, and in general the job definitions
are very "vertically compact" (there are pretty much no empty lines),
I'm not sure it would fit very well. Syntax highlighting should also
make it pretty much a non-issue.

Putting the comment on the line *after* 'machines:' instead of the
one *before* it was a mistake, though, and I'll make sure to fix it.

[...]
> >  - project:
> >  name: libvirt-sandbox
> > +# libvirt-sandbox is Linux only; among Linux platforms, CentOS 7 has
> > +# to be excluded because it doesn't ship a version of xz suitable for
> > +# statically linking against
> 
> Hmm, for some reason this doesn't sound "English" to me, how about:
> 
> s/statically linking against/linking statically/

Yeah, that's indeed much better :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH] ppc: Remove deprecated ppcemb target

[Thomas Huth]

There is no known available OS for ppc around anymore that uses page
sizes below 4k, so it does not make much sense that we keep wasting
our time on building and testing the ppcemb-softmmu target. It has
been deprecated since two releases, and nobody complained, so let's
remove this now.

Signed-off-by: Thomas Huth 
---
 configure  | 13 +++--
 cpus.c |  1 -
 default-configs/ppcemb-softmmu.mak | 23 ---
 hw/ppc/ppc405_boards.c | 14 --
 hw/ppc/ppc440_bamboo.c |  7 ---
 hw/ppc/sam460ex.c  |  7 ---
 hw/ppc/virtex_ml507.c  |  7 ---
 include/exec/poison.h  |  1 -
 qapi/common.json   |  2 +-
 qemu-deprecated.texi   |  6 --
 target/ppc/cpu-qom.h   |  2 --
 target/ppc/cpu.h   | 16 
 target/ppc/kvm.c   |  4 +---
 target/ppc/mmu_helper.c|  6 +++---
 target/ppc/translate_init.inc.c| 35 +--
 tests/machine-none-test.c  |  1 -
 16 files changed, 9 insertions(+), 136 deletions(-)
 delete mode 100644 default-configs/ppcemb-softmmu.mak

diff --git a/configure b/configure
index e7bddc0..0fc55eb 100755
--- a/configure
+++ b/configure
@@ -195,8 +195,7 @@ supported_kvm_target() {
 i386:i386 | i386:x86_64 | i386:x32 | \
 x86_64:i386 | x86_64:x86_64 | x86_64:x32 | \
 mips:mips | mipsel:mips | \
-ppc:ppc | ppcemb:ppc | ppc64:ppc | \
-ppc:ppc64 | ppcemb:ppc64 | ppc64:ppc64 | \
+ppc:ppc | ppc64:ppc | ppc:ppc64 | ppc64:ppc64 | \
 s390x:s390x)
 return 0
 ;;
@@ -6935,7 +6934,7 @@ if test "$linux" = "yes" ; then
   i386|x86_64|x32)
 linux_arch=x86
 ;;
-  ppcemb|ppc|ppc64)
+  ppc|ppc64)
 linux_arch=powerpc
 ;;
   s390x)
@@ -6965,7 +6964,7 @@ target_name=$(echo $target | cut -d '-' -f 1)
 target_bigendian="no"
 
 case "$target_name" in
-  
armeb|aarch64_be|hppa|lm32|m68k|microblaze|mips|mipsn32|mips64|moxie|or1k|ppc|ppcemb|ppc64|ppc64abi32|s390x|sh4eb|sparc|sparc64|sparc32plus|xtensaeb)
+  
armeb|aarch64_be|hppa|lm32|m68k|microblaze|mips|mipsn32|mips64|moxie|or1k|ppc|ppc64|ppc64abi32|s390x|sh4eb|sparc|sparc64|sparc32plus|xtensaeb)
   target_bigendian=yes
   ;;
 esac
@@ -7093,12 +7092,6 @@ case "$target_name" in
 gdb_xml_files="power-core.xml power-fpu.xml power-altivec.xml 
power-spe.xml"
 target_compiler=$cross_cc_powerpc
   ;;
-  ppcemb)
-TARGET_BASE_ARCH=ppc
-TARGET_ABI_DIR=ppc
-gdb_xml_files="power-core.xml power-fpu.xml power-altivec.xml 
power-spe.xml"
-target_compiler=$cross_cc_ppcemb
-  ;;
   ppc64)
 TARGET_BASE_ARCH=ppc
 TARGET_ABI_DIR=ppc
diff --git a/cpus.c b/cpus.c
index b5844b7..80ca683 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2203,7 +2203,6 @@ static CpuInfoArch 
sysemu_target_to_cpuinfo_arch(SysEmuTarget target)
 return CPU_INFO_ARCH_X86;
 
 case SYS_EMU_TARGET_PPC:
-case SYS_EMU_TARGET_PPCEMB:
 case SYS_EMU_TARGET_PPC64:
 return CPU_INFO_ARCH_PPC;
 
diff --git a/default-configs/ppcemb-softmmu.mak 
b/default-configs/ppcemb-softmmu.mak
deleted file mode 100644
index ac44f15..000
--- a/default-configs/ppcemb-softmmu.mak
+++ /dev/null
@@ -1,23 +0,0 @@
-# Default configuration for ppcemb-softmmu
-
-include pci.mak
-include sound.mak
-include usb.mak
-CONFIG_PPC4XX=y
-CONFIG_M48T59=y
-CONFIG_SERIAL=y
-CONFIG_SERIAL_ISA=y
-CONFIG_I8257=y
-CONFIG_OPENPIC=y
-CONFIG_PFLASH_CFI01=y
-CONFIG_PFLASH_CFI02=y
-CONFIG_PTIMER=y
-CONFIG_I8259=y
-CONFIG_XILINX=y
-CONFIG_XILINX_ETHLITE=y
-CONFIG_USB_EHCI_SYSBUS=y
-CONFIG_SM501=y
-CONFIG_DDC=y
-CONFIG_IDE_SII3112=y
-CONFIG_I2C=y
-CONFIG_BITBANG_I2C=y
diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
index 7011107..3a54d52 100644
--- a/hw/ppc/ppc405_boards.c
+++ b/hw/ppc/ppc405_boards.c
@@ -202,13 +202,6 @@ static void ref405ep_init(MachineState *machine)
 DriveInfo *dinfo;
 MemoryRegion *sysmem = get_system_memory();
 
-#ifdef TARGET_PPCEMB
-if (!qtest_enabled()) {
-warn_report("qemu-system-ppcemb is deprecated, "
-"please use qemu-system-ppc instead.");
-}
-#endif
-
 /* XXX: fix this */
 memory_region_allocate_system_memory(_memories[0], NULL, "ef405ep.ram",
  0x0800);
@@ -503,13 +496,6 @@ static void taihu_405ep_init(MachineState *machine)
 int fl_idx, fl_sectors;
 DriveInfo *dinfo;
 
-#ifdef TARGET_PPCEMB
-if (!qtest_enabled()) {
-warn_report("qemu-system-ppcemb is deprecated, "
-"please use qemu-system-ppc instead.");
-}
-#endif
-
 /* RAM is soldered to the board so the size cannot be changed */
 ram_size = 0x0800;
 memory_region_allocate_system_memory(ram, NULL, "taihu_405ep.ram",
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 3d4c43b..f5720f9 

Re: [libvirt] [jenkins-ci PATCH v2 2/2] projects: Document rationale for skipping jobs

[Erik Skultety]

On Tue, Aug 21, 2018 at 12:38:06PM +0200, Andrea Bolognani wrote:
> In general, we strive for full coverage and build all
> projects on all targets; however, in some cases that's
> simply not feasible and we have to skip the corresponding
> job. Document the rationale for each such case.
>
> Signed-off-by: Andrea Bolognani 
> ---
>  projects/libvirt-dbus.yaml| 3 +++
>  projects/libvirt-sandbox.yaml | 3 +++
>  projects/libvirt-tck.yaml | 2 ++
>  projects/libvirt.yaml | 2 ++
>  projects/virt-manager.yaml| 1 +
>  projects/virt-viewer.yaml | 2 ++
>  6 files changed, 13 insertions(+)
>
> diff --git a/projects/libvirt-dbus.yaml b/projects/libvirt-dbus.yaml
> index fdfb615..459bd96 100644
> --- a/projects/libvirt-dbus.yaml
> +++ b/projects/libvirt-dbus.yaml
> @@ -2,6 +2,7 @@
>  - project:
>  name: libvirt-dbus
>  machines:

I'd appreciate an empty line above the comments, it's adds to the readability.

> +  # Debian 8 doesn't have a recent enough GLib
>- libvirt-centos-7
>- libvirt-debian-9
>- libvirt-fedora-27
> @@ -17,6 +18,7 @@
>parent_jobs: 'libvirt-glib-master-build'
>- autotools-syntax-check-job:
>parent_jobs: 'libvirt-dbus-master-build'
> +  # The test suite uses Python 3, which CentOS 7 doesn't include
>machines:
>  - libvirt-debian-9
>  - libvirt-fedora-27
> @@ -26,6 +28,7 @@
>  - libvirt-freebsd-11
>- autotools-check-job:
>parent_jobs: 'libvirt-dbus-master-syntax-check'
> +  # syntax-check uses Python 3, which CentOS 7 doesn't include
>machines:
>  - libvirt-debian-9
>  - libvirt-fedora-27
> diff --git a/projects/libvirt-sandbox.yaml b/projects/libvirt-sandbox.yaml
> index da9bf77..6a474c8 100644
> --- a/projects/libvirt-sandbox.yaml
> +++ b/projects/libvirt-sandbox.yaml
> @@ -1,6 +1,9 @@
>
>  - project:
>  name: libvirt-sandbox
> +# libvirt-sandbox is Linux only; among Linux platforms, CentOS 7 has
> +# to be excluded because it doesn't ship a version of xz suitable for
> +# statically linking against

Hmm, for some reason this doesn't sound "English" to me, how about:

s/statically linking against/linking statically/

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v2 2/2] conf: Parse guestfwd channel device info again

[Andrea Bolognani]

On Tue, 2018-08-21 at 12:12 +0200, Michal Privoznik wrote:
[...]
>  if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
> -def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) {
> -VIR_DEBUG("Ignoring device address for gustfwd channel");
> -} else if (virDomainDeviceInfoParseXML(xmlopt, node,
> -   >info, flags) < 0) {
> -goto error;
> -}
> -
> +def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD)
> +virDomainDeviceInfoClearAddress(>info);

Our style guidelines require curly braces around the if body in this
case - which is great, because it means your diff can be smaller and
more readable! :)

With that fixed

  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] [jenkins-ci PATCH v2 1/2] projects: Add default machines for libvirt-dbus

[Erik Skultety]

On Tue, Aug 21, 2018 at 12:38:05PM +0200, Andrea Bolognani wrote:
> This doesn't change the results but brings it more in line
> with how other projects are defined and will help us out
> later.
>
> Signed-off-by: Andrea Bolognani 
> ---
>  projects/libvirt-dbus.yaml | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/projects/libvirt-dbus.yaml b/projects/libvirt-dbus.yaml
> index f01ea9e..fdfb615 100644
> --- a/projects/libvirt-dbus.yaml
> +++ b/projects/libvirt-dbus.yaml
> @@ -1,20 +1,20 @@
>
>  - project:
>  name: libvirt-dbus
> +machines:
> +  - libvirt-centos-7
> +  - libvirt-debian-9
> +  - libvirt-fedora-27
> +  - libvirt-fedora-28
> +  - libvirt-fedora-rawhide
> +  - libvirt-freebsd-10
> +  - libvirt-freebsd-11
>  title: Libvirt D-Bus
>  archive_format: xz
>  git-url: https://github.com/libvirt/libvirt-dbus.git
>  jobs:
>- autotools-build-job:
>parent_jobs: 'libvirt-glib-master-build'
> -  machines:
> -- libvirt-centos-7
> -- libvirt-debian-9
> -- libvirt-fedora-27
> -- libvirt-fedora-28
> -- libvirt-fedora-rawhide
> -- libvirt-freebsd-10
> -- libvirt-freebsd-11
>- autotools-syntax-check-job:
>parent_jobs: 'libvirt-dbus-master-build'
>machines:

I wish there was something like exclusion, so that when we want to exclude just
a single machine from the default machine list defined for the project (or
from {all_machines} for that matter), we don't have to override the whole
machine list all over again minus the ones we can't/don't want to use.
Anyway,

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v3 2/2] process: wait longer 5->30s on hard shutdown

[Daniel P . Berrangé]

On Tue, Aug 14, 2018 at 11:27:34AM +0200, Christian Ehrhardt wrote:
> In cases where virProcessKillPainfully already reailizes that
> SIGTERM wasn't enough we are partially on a bad path already.
> Maybe the system is overloaded or having serious trouble to free and
> reap resources in time.
> 
> In those case give the SIGKILL that was sent after 10 seconds some more
> time to take effect if force was set (only then we are falling back to
> SIGKILL anyway).
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  src/util/virprocess.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 46360cc051..dda8916284 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -350,7 +350,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, 
> unsigned int extradelay)
>  {
>  size_t i;
>  int ret = -1;
> -unsigned int delay = 75 + (extradelay*5);
> +unsigned int delay = (force ? 200 : 75) + (extradelay*5);
>  const char *signame = "TERM";
>  
>  VIR_DEBUG("vpid=%lld force=%d delay=%u", (long long)pid, force, pid);
> @@ -358,7 +358,7 @@ virProcessKillPainfullyDelay(pid_t pid, bool force, 
> unsigned int extradelay)
>  /* This loop sends SIGTERM, then waits a few iterations (10 seconds)
>   * to see if it dies. If the process still hasn't exited, and
>   * @force is requested, a SIGKILL will be sent, and this will
> - * wait up to 5 seconds more for the process to exit before
> + * wait up to 30 seconds more for the process to exit before
>   * returning.
>   *
>   * An extra delay can be specified for cases that are expected to clean

Reviewed-by: Daniel P. Berrangé 


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 1/2] process: wait longer on kill per assigned Hostdev

[Daniel P . Berrangé]

On Tue, Aug 14, 2018 at 11:27:33AM +0200, Christian Ehrhardt wrote:
> It was found that in cases with host devices virProcessKillPainfully
> might be able to send signal zero to the target PID for quite a while
> with the process already being gone from /proc/.
> 
> That is due to cleanup and reset of devices which might include a
> secondary bus reset that on top of the actions taken has a 1s delay
> to let the bus settle. Due to that guests with plenty of Host devices
> could easily exceed the default timeouts.
> 
> To solve that, this adds an extra delay of 2s per hostdev that is associated
> to a VM.
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_process.c  |  5 +++--
>  src/util/virprocess.c| 18 +++---
>  src/util/virprocess.h|  1 +
>  4 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index ca4a192a4a..47ea35f864 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2605,6 +2605,7 @@ virProcessGetPids;
>  virProcessGetStartTime;
>  virProcessKill;
>  virProcessKillPainfully;
> +virProcessKillPainfullyDelay;
>  virProcessNamespaceAvailable;
>  virProcessRunInMountNamespace;
>  virProcessSchedPolicyTypeFromString;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 02fdc55156..b7bf8813da 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6814,8 +6814,9 @@ qemuProcessKill(virDomainObjPtr vm, unsigned int flags)
>  return 0;
>  }
>  
> -ret = virProcessKillPainfully(vm->pid,
> -  !!(flags & VIR_QEMU_PROCESS_KILL_FORCE));
> +ret = virProcessKillPainfullyDelay(vm->pid,
> +   !!(flags & 
> VIR_QEMU_PROCESS_KILL_FORCE),
> +   vm->def->nhostdevs * 2);

This API contract is a bit wierd. You've got an arbitray x2 multiplier
here...

> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index ecea27a2d4..46360cc051 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -341,15 +341,19 @@ int virProcessKill(pid_t pid, int sig)
>   * Returns 0 if it was killed gracefully, 1 if it
>   * was killed forcibly, -1 if it is still alive,
>   * or another error occurred.
> + *
> + * Callers can proide an extra delay to wait longer
> + * than the default.

Mention that it is in "seconds"

>   */
>  int
> -virProcessKillPainfully(pid_t pid, bool force)
> +virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay)
>  {
>  size_t i;
>  int ret = -1;
> +unsigned int delay = 75 + (extradelay*5);

...and it gets another x5 multiplier here.

Feels nicer to just have the caller provide a x10 multiplier and
honour that directly

>  const char *signame = "TERM";
>  
> -VIR_DEBUG("vpid=%lld force=%d", (long long)pid, force);
> +VIR_DEBUG("vpid=%lld force=%d delay=%u", (long long)pid, force, pid);

s/pid/extradelay/

>  
>  /* This loop sends SIGTERM, then waits a few iterations (10 seconds)
>   * to see if it dies. If the process still hasn't exited, and
> @@ -357,9 +361,12 @@ virProcessKillPainfully(pid_t pid, bool force)
>   * wait up to 5 seconds more for the process to exit before
>   * returning.
>   *
> + * An extra delay can be specified for cases that are expected to clean
> + * up slower than usual.
> + *
>   * Note that setting @force could result in dataloss for the process.
>   */
> -for (i = 0; i < 75; i++) {
> +for (i = 0; i < delay; i++) {
>  int signum;
>  if (i == 0) {
>  signum = SIGTERM; /* kindly suggest it should exit */
> @@ -402,6 +409,11 @@ virProcessKillPainfully(pid_t pid, bool force)
>  }
>  
>  
> +int virProcessKillPainfully(pid_t pid, bool force)
> +{
> +return virProcessKillPainfullyDelay(pid, force, 0);
> +}
> +
>  #if HAVE_SCHED_GETAFFINITY
>  
>  int virProcessSetAffinity(pid_t pid, virBitmapPtr map)
> diff --git a/src/util/virprocess.h b/src/util/virprocess.h
> index 3c5a882772..b72603ca8e 100644
> --- a/src/util/virprocess.h
> +++ b/src/util/virprocess.h
> @@ -55,6 +55,7 @@ virProcessWait(pid_t pid, int *exitstatus, bool raw)
>  int virProcessKill(pid_t pid, int sig);
>  
>  int virProcessKillPainfully(pid_t pid, bool force);
> +int virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int);

Missing parameter name here

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 1/2] device_conf: Split virDomainDeviceInfoClear

[Andrea Bolognani]

On Tue, 2018-08-21 at 12:12 +0200, Michal Privoznik wrote:
>  void
> -virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
> +virDomainDeviceInfoClearAddress(virDomainDeviceInfoPtr info)
>  {
> -VIR_FREE(info->alias);
>  memset(>addr, 0, sizeof(info->addr));
>  info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
>  VIR_FREE(info->romfile);

Now virDomainDeviceInfoClearAddress() clears out *way more* than
just the address, including romfile and other information not
visible in the context... It should really only call memset and
reset info->type in order for the name not to be very misleading.

[...]
> +void
> +virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
> +{
> +VIR_FREE(info->alias);
> +virDomainDeviceInfoClearAddress(info);
> +}

romfile and friends should be cleared out here along with the alias
and, of course, the address :)

With that fixed

  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] util: Don't delete the original file for truncation

[Daniel P . Berrangé]

On Tue, Aug 21, 2018 at 12:37:54PM +0200, Marc Hartmayer wrote:
> On Tue, Aug 21, 2018 at 11:03 AM +0200, "Daniel P. Berrangé" 
>  wrote:
> > On Tue, Aug 21, 2018 at 10:49:28AM +0200, Marc Hartmayer wrote:
> >> Truncate means that if a file exists it's length will be truncated to
> >> 0, but the mode and the owner shall be unchanged. The current behavior
> >> is that the original file is deleted and a new file is created. Let's
> >> fix this by using O_TRUNC.
> >
> > This is just describing what you've changed, leaving out why you are
> > trying todo this ?
> 
> Two things:
> 
>  1. The behavior for a console that logs all data to a file has changed
> with the patch series “qemu: use FD passing for chardev UNIX
> sockets”. Before this patch series the owner of the log file was the
> QEMU process user (since the QEMU process was responsible for
> creating it). Now it’s the virtlogd user.
> 
> e.g.
> 
> 
>   
>   
> 

Yes, that ownership change is *good* because the point of the change was
to prevent the QEMU process from ever writing to the logs directly, so
that virtlogd can enforce rollover policies.

>  2. What is currently done by virtlogd is not a truncation (at least
> IMHO). An alternative to this patch would be to rename the parameter
> (API change… so probably no option) or document the behavior.

It depends on your POV. If you are looking at the inode it isn't
truncation, because we've just given the inode a new name, and created
a new inode for the original name. If you are looking at the filename
this is truncation, because the file "foo.log" used  to have a size and
now it is zero length.

> >> The function virRotatingFileWriterDelete is now unused but may be used
> >> in the future and is therefore still defined.
> >>
> >> Signed-off-by: Marc Hartmayer 
> >> Reviewed-by: Boris Fiuczynski 
> >> ---
> >> Note:
> >>
> >> This change has the (potentially unwanted) security effect that the
> >> owner/group of the log file does not change. Before this patch the old
> >> log file was deleted and the newly created log file was owned by the
> >> virtlogd user. Now, if a user has created the log file before, he can
> >> read the logs. If we don't wanna have this effect we can either
> >> adjust/add a virtlogd API or do a chown within the calling driver
> >> (e.g. QEMU driver).
> >
> > Pre-creating the log file and/or messing around with ownership are
> > not things we ever intended to support.
> 
> Okay. What should happen if the log file is already pre-created?
> Silently overwrite/delete?

It'll just have the same behaviour is if the file already existed from
a previous boot attempt of the VM. Normal rollover should occurr when
the file size hits the limit. Any changes to ownership are liable to be
thrown away.

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 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes

[Andrea Bolognani]

On Tue, 2018-08-21 at 17:35 +0800, Yi Min Zhao wrote:
> I want to ask a question. For pci address, any pci device can't use slot 0.
> Is that a reason why PCI part could treat all zeros as empty address?

A PCI address where all attributes are zero can't be used, so
there's no ambiguity there; same for a zPCI address where all
attributes are zero, which also can't be used.

> For zPCI address, if we use the same strategy as PCI part and user
> wants to assign fid=0 to the specific device, he might encounter
> assignment failure. At least, according to the doc, 0 shoud be a valid
> value to assign to fid.

If the user wants to use a specific zPCI address they can simply
specify both attributes, eg. uid=1,fid=0 will work just fine with
the proposed approach and won't produce errors or cause a new zPCI
address to be automatically assigned.

If the user only specified fid=0 while leaving uid unspecified,
well, an address is going to be picked for them. This is consistent
with how PCI addresses are treated so it shouldn't be a problem: if
anything, *deviating* from this behavior would cause confusion.

-- 
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 1/3] vsh: Add API for printing tables.

[Daniel P . Berrangé]

On Tue, Aug 21, 2018 at 12:27:34PM +0200, Michal Privoznik wrote:
> On 08/21/2018 11:18 AM, Simon Kobyda wrote:
> > On Thu, 2018-08-16 at 12:28 +0100, Daniel P. Berrangé wrote:
> >> On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
> >>>
> >>
> >> After asking around I have found the right solution that we need to
> >> use
> >> for measuring string width.  mbstowcs()/wcswidth() will get the
> >> answer
> >> wrong wrt zero-width characters, combining characters, non-printable
> >> characters, etc. We need to use the libunistring library:
> >>
> >>   
> >> https://www.gnu.org/software/libunistring/manual/libunistring.html#uniwidth_002eh
> >>
> >>
> > I've tried what you've suggested, but it seems that it doesn't work
> > well with all unicode characters. I'm looking into the code of the
> > library, and each function uN_strwidth calls function uN_width, and
> > that function calls uc_width for calculation of width of characters.
> > And if we look into the code of uc_width here:
> > 
> > http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/uniwidth/width.c;h=269cfc77f50a3b9802e5fb5620ff8bcf95e05e40;hb=HEAD#l415
> > it seems that this library is limited only to certain unicodes, e.g.:
> > hangul characters, angle brackets, CJK characters... But it doesn't
> > cover all multiple-width characters. Example: I try to throw any emoji
> > (e.g. , 呂, ), it returns width of 1 column for each charact
> > er, nevertheless these characters have width of 2 columns on terminal.
> > 
> > BTW, it seems unistring library imports those funcions from gnulib.
> 
> I guess the only option then is to try smartcols [1]. If it is good for
> util-linux it's going to be good for us too. Although, I'd prefer to
> have our own wrappers over their API.
> 
> https://github.com/karelzak/util-linux/tree/master/libsmartcols

The util-linux code does something that uses mbstowcs / wcwidth to
convert the characters and count their width, sort of like the original
version of this patch. They have further code that decides to convert
certain unicode characters into "\xNN" escaped sequences, which avoids
the problems I raised wrt non-printable strings.

   https://github.com/karelzak/util-linux/blob/master/lib/mbsalign.c

So we could pull that helper API into our code, since its LGPL loicensed.
I'm unclear if this correctly handles all the cases or not though as
there's no unit tests for it in util-linux AFACT.

Really the only way for us to be sure is to provide a unit test which
stresses our the code with a variety of unicode input strings.

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] [jenkins-ci PATCH v2 0/2] projects: Document rationale for skipping jobs

[Andrea Bolognani]

Changes from [v1]:

  * refresh after recent changes to the jobs.


[v1] https://www.redhat.com/archives/libvir-list/2018-August/msg00580.html

Andrea Bolognani (2):
  projects: Add default machines for libvirt-dbus
  projects: Document rationale for skipping jobs

 projects/libvirt-dbus.yaml| 19 +++
 projects/libvirt-sandbox.yaml |  3 +++
 projects/libvirt-tck.yaml |  2 ++
 projects/libvirt.yaml |  2 ++
 projects/virt-manager.yaml|  1 +
 projects/virt-viewer.yaml |  2 ++
 6 files changed, 21 insertions(+), 8 deletions(-)

-- 
2.17.1

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

[libvirt] [jenkins-ci PATCH v2 2/2] projects: Document rationale for skipping jobs

[Andrea Bolognani]

In general, we strive for full coverage and build all
projects on all targets; however, in some cases that's
simply not feasible and we have to skip the corresponding
job. Document the rationale for each such case.

Signed-off-by: Andrea Bolognani 
---
 projects/libvirt-dbus.yaml| 3 +++
 projects/libvirt-sandbox.yaml | 3 +++
 projects/libvirt-tck.yaml | 2 ++
 projects/libvirt.yaml | 2 ++
 projects/virt-manager.yaml| 1 +
 projects/virt-viewer.yaml | 2 ++
 6 files changed, 13 insertions(+)

diff --git a/projects/libvirt-dbus.yaml b/projects/libvirt-dbus.yaml
index fdfb615..459bd96 100644
--- a/projects/libvirt-dbus.yaml
+++ b/projects/libvirt-dbus.yaml
@@ -2,6 +2,7 @@
 - project:
 name: libvirt-dbus
 machines:
+  # Debian 8 doesn't have a recent enough GLib
   - libvirt-centos-7
   - libvirt-debian-9
   - libvirt-fedora-27
@@ -17,6 +18,7 @@
   parent_jobs: 'libvirt-glib-master-build'
   - autotools-syntax-check-job:
   parent_jobs: 'libvirt-dbus-master-build'
+  # The test suite uses Python 3, which CentOS 7 doesn't include
   machines:
 - libvirt-debian-9
 - libvirt-fedora-27
@@ -26,6 +28,7 @@
 - libvirt-freebsd-11
   - autotools-check-job:
   parent_jobs: 'libvirt-dbus-master-syntax-check'
+  # syntax-check uses Python 3, which CentOS 7 doesn't include
   machines:
 - libvirt-debian-9
 - libvirt-fedora-27
diff --git a/projects/libvirt-sandbox.yaml b/projects/libvirt-sandbox.yaml
index da9bf77..6a474c8 100644
--- a/projects/libvirt-sandbox.yaml
+++ b/projects/libvirt-sandbox.yaml
@@ -1,6 +1,9 @@
 
 - project:
 name: libvirt-sandbox
+# libvirt-sandbox is Linux only; among Linux platforms, CentOS 7 has
+# to be excluded because it doesn't ship a version of xz suitable for
+# statically linking against
 machines:
   - libvirt-debian-8
   - libvirt-debian-9
diff --git a/projects/libvirt-tck.yaml b/projects/libvirt-tck.yaml
index c406fda..38f8c09 100644
--- a/projects/libvirt-tck.yaml
+++ b/projects/libvirt-tck.yaml
@@ -1,6 +1,8 @@
 
 - project:
 name: libvirt-tck
+# CentOS 7 doesn't include perl-generators, which is necessary to
+# build libvirt-tck
 machines:
   - libvirt-debian-8
   - libvirt-debian-9
diff --git a/projects/libvirt.yaml b/projects/libvirt.yaml
index c64ac5b..ce4698e 100644
--- a/projects/libvirt.yaml
+++ b/projects/libvirt.yaml
@@ -10,6 +10,8 @@
   parent_jobs:
   - autotools-syntax-check-job:
   parent_jobs: 'libvirt-master-build'
+  # We limit syntax-check to Linux platforms because it calls some
+  # commands with more arguments than FreeBSD supports
   machines:
 - libvirt-centos-7
 - libvirt-debian-8
diff --git a/projects/virt-manager.yaml b/projects/virt-manager.yaml
index 84a95a6..6f806af 100644
--- a/projects/virt-manager.yaml
+++ b/projects/virt-manager.yaml
@@ -1,6 +1,7 @@
 
 - project:
 name: virt-manager
+# virt-manager is Python 3 only, so it can't be built on CentOS 7
 machines:
   - libvirt-debian-9
   - libvirt-fedora-27
diff --git a/projects/virt-viewer.yaml b/projects/virt-viewer.yaml
index e68a23b..9188239 100644
--- a/projects/virt-viewer.yaml
+++ b/projects/virt-viewer.yaml
@@ -13,6 +13,8 @@
   parent_jobs: 'virt-viewer-master-syntax-check'
   - autotools-rpm-job:
   parent_jobs: 'virt-viewer-master-check'
+  # The spec file for virt-viewer requires a very recent version
+  # of spice-gtk, so we have to skip this job on older distros
   machines:
 - libvirt-fedora-28
 - libvirt-fedora-rawhide
-- 
2.17.1

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

[libvirt] [jenkins-ci PATCH v2 1/2] projects: Add default machines for libvirt-dbus

[Andrea Bolognani]

This doesn't change the results but brings it more in line
with how other projects are defined and will help us out
later.

Signed-off-by: Andrea Bolognani 
---
 projects/libvirt-dbus.yaml | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/projects/libvirt-dbus.yaml b/projects/libvirt-dbus.yaml
index f01ea9e..fdfb615 100644
--- a/projects/libvirt-dbus.yaml
+++ b/projects/libvirt-dbus.yaml
@@ -1,20 +1,20 @@
 
 - project:
 name: libvirt-dbus
+machines:
+  - libvirt-centos-7
+  - libvirt-debian-9
+  - libvirt-fedora-27
+  - libvirt-fedora-28
+  - libvirt-fedora-rawhide
+  - libvirt-freebsd-10
+  - libvirt-freebsd-11
 title: Libvirt D-Bus
 archive_format: xz
 git-url: https://github.com/libvirt/libvirt-dbus.git
 jobs:
   - autotools-build-job:
   parent_jobs: 'libvirt-glib-master-build'
-  machines:
-- libvirt-centos-7
-- libvirt-debian-9
-- libvirt-fedora-27
-- libvirt-fedora-28
-- libvirt-fedora-rawhide
-- libvirt-freebsd-10
-- libvirt-freebsd-11
   - autotools-syntax-check-job:
   parent_jobs: 'libvirt-dbus-master-build'
   machines:
-- 
2.17.1

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

Re: [libvirt] [PATCH] util: Don't delete the original file for truncation

[Marc Hartmayer]

On Tue, Aug 21, 2018 at 11:03 AM +0200, "Daniel P. Berrangé" 
 wrote:
> On Tue, Aug 21, 2018 at 10:49:28AM +0200, Marc Hartmayer wrote:
>> Truncate means that if a file exists it's length will be truncated to
>> 0, but the mode and the owner shall be unchanged. The current behavior
>> is that the original file is deleted and a new file is created. Let's
>> fix this by using O_TRUNC.
>
> This is just describing what you've changed, leaving out why you are
> trying todo this ?

Two things:

 1. The behavior for a console that logs all data to a file has changed
with the patch series “qemu: use FD passing for chardev UNIX
sockets”. Before this patch series the owner of the log file was the
QEMU process user (since the QEMU process was responsible for
creating it). Now it’s the virtlogd user.

e.g.


  
  


 2. What is currently done by virtlogd is not a truncation (at least
IMHO). An alternative to this patch would be to rename the parameter
(API change… so probably no option) or document the behavior.

>
>> The function virRotatingFileWriterDelete is now unused but may be used
>> in the future and is therefore still defined.
>>
>> Signed-off-by: Marc Hartmayer 
>> Reviewed-by: Boris Fiuczynski 
>> ---
>> Note:
>>
>> This change has the (potentially unwanted) security effect that the
>> owner/group of the log file does not change. Before this patch the old
>> log file was deleted and the newly created log file was owned by the
>> virtlogd user. Now, if a user has created the log file before, he can
>> read the logs. If we don't wanna have this effect we can either
>> adjust/add a virtlogd API or do a chown within the calling driver
>> (e.g. QEMU driver).
>
> Pre-creating the log file and/or messing around with ownership are
> not things we ever intended to support.

Okay. What should happen if the log file is already pre-created?
Silently overwrite/delete?

>
> 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 :|
>
--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


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

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

[Michal Privoznik]

On 08/21/2018 11:18 AM, Simon Kobyda wrote:
> On Thu, 2018-08-16 at 12:28 +0100, Daniel P. Berrangé wrote:
>> On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
>>>
>>
>> After asking around I have found the right solution that we need to
>> use
>> for measuring string width.  mbstowcs()/wcswidth() will get the
>> answer
>> wrong wrt zero-width characters, combining characters, non-printable
>> characters, etc. We need to use the libunistring library:
>>
>>   
>> https://www.gnu.org/software/libunistring/manual/libunistring.html#uniwidth_002eh
>>
>>
> I've tried what you've suggested, but it seems that it doesn't work
> well with all unicode characters. I'm looking into the code of the
> library, and each function uN_strwidth calls function uN_width, and
> that function calls uc_width for calculation of width of characters.
> And if we look into the code of uc_width here:
> 
> http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/uniwidth/width.c;h=269cfc77f50a3b9802e5fb5620ff8bcf95e05e40;hb=HEAD#l415
> it seems that this library is limited only to certain unicodes, e.g.:
> hangul characters, angle brackets, CJK characters... But it doesn't
> cover all multiple-width characters. Example: I try to throw any emoji
> (e.g. , 呂, ), it returns width of 1 column for each charact
> er, nevertheless these characters have width of 2 columns on terminal.
> 
> BTW, it seems unistring library imports those funcions from gnulib.

I guess the only option then is to try smartcols [1]. If it is good for
util-linux it's going to be good for us too. Although, I'd prefer to
have our own wrappers over their API.

https://github.com/karelzak/util-linux/tree/master/libsmartcols

Michal

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

Re: [libvirt] [PATCH] Remove deprecated -balloon option

[Michael S. Tsirkin]

On Tue, Aug 21, 2018 at 12:05:30PM +0200, Thomas Huth wrote:
> The "-balloon" option has been replaced by "-device virtio-balloon".
> It's been marked as deprecated since two releases, and nobody
> complained, so let's remove it now.
> 
> Signed-off-by: Thomas Huth 

Reviewed-by: Michael S. Tsirkin 


> ---
>  docs/virtio-balloon-stats.txt |  6 +++---
>  qemu-deprecated.texi  |  5 -
>  qemu-options.hx   | 10 --
>  vl.c  | 36 
>  4 files changed, 3 insertions(+), 54 deletions(-)
> 
> diff --git a/docs/virtio-balloon-stats.txt b/docs/virtio-balloon-stats.txt
> index 9985e1d..1732cc8 100644
> --- a/docs/virtio-balloon-stats.txt
> +++ b/docs/virtio-balloon-stats.txt
> @@ -61,9 +61,9 @@ It's also important to note the following:
> respond to the request the timer will never be re-armed, which has
> the same effect as disabling polling
>  
> -Here are a few examples. QEMU is started with '-balloon virtio', which
> -generates '/machine/peripheral-anon/device[1]' as the QOM path for the
> -balloon device.
> +Here are a few examples. QEMU is started with '-device virtio-balloon',
> +which generates '/machine/peripheral-anon/device[1]' as the QOM path for
> +the balloon device.
>  
>  Enable polling with 2 seconds interval:
>  
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 67b7211..0714017 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -106,11 +106,6 @@ enabled via the ``-machine usb=on'' argument.
>  
>  The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
>  
> -@subsection -balloon (since 2.12.0)
> -
> -The @option{--balloon virtio} argument has been superseded by
> -@option{--device virtio-balloon}.
> -
>  @subsection -fsdev handle (since 2.12.0)
>  
>  The ``handle'' fsdev backend does not support symlinks and causes the 9p
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 4efdedf..47c6b92 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -454,16 +454,6 @@ modprobe i810_audio clocking=48000
>  @end example
>  ETEXI
>  
> -DEF("balloon", HAS_ARG, QEMU_OPTION_balloon,
> -"-balloon virtio[,addr=str]\n"
> -"enable virtio balloon device (deprecated)\n", 
> QEMU_ARCH_ALL)
> -STEXI
> -@item -balloon virtio[,addr=@var{addr}]
> -@findex -balloon
> -Enable virtio balloon device, optionally with PCI address @var{addr}. This
> -option is deprecated, use @option{-device virtio-balloon} instead.
> -ETEXI
> -
>  DEF("device", HAS_ARG, QEMU_OPTION_device,
>  "-device driver[,prop[=value][,...]]\n"
>  "add device (based on driver)\n"
> diff --git a/vl.c b/vl.c
> index 16b913f..f952f01 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2127,36 +2127,6 @@ static void parse_display(const char *p)
>  }
>  }
>  
> -static int balloon_parse(const char *arg)
> -{
> -QemuOpts *opts;
> -
> -warn_report("This option is deprecated. "
> -"Use '--device virtio-balloon' to enable the balloon 
> device.");
> -
> -if (strcmp(arg, "none") == 0) {
> -return 0;
> -}
> -
> -if (!strncmp(arg, "virtio", 6)) {
> -if (arg[6] == ',') {
> -/* have params -> parse them */
> -opts = qemu_opts_parse_noisily(qemu_find_opts("device"), arg + 7,
> -   false);
> -if (!opts)
> -return  -1;
> -} else {
> -/* create empty opts */
> -opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
> -_abort);
> -}
> -qemu_opt_set(opts, "driver", "virtio-balloon", _abort);
> -return 0;
> -}
> -
> -return -1;
> -}
> -
>  char *qemu_find_file(int type, const char *name)
>  {
>  int i;
> @@ -3659,12 +3629,6 @@ int main(int argc, char **argv, char **envp)
>  case QEMU_OPTION_no_hpet:
>  no_hpet = 1;
>  break;
> -case QEMU_OPTION_balloon:
> -if (balloon_parse(optarg) < 0) {
> -error_report("unknown -balloon argument %s", optarg);
> -exit(1);
> -}
> -break;
>  case QEMU_OPTION_no_reboot:
>  no_reboot = 1;
>  break;
> -- 
> 1.8.3.1

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

[libvirt] [PATCH v2 2/2] conf: Parse guestfwd channel device info again

[Michal Privoznik]

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

Due to historical reasons we were not parsing device info on
guestfwd channel. Sure, it doesn't make much sense to parse
 but it surely makes sense to parse its alias (which
might be an user alias).

This partially reverts commit
47a3dd46ead20e6fdc30bcdc1b8e707e250d33da which fixed
https://bugzilla.redhat.com/show_bug.cgi?id=1172526.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3c254801cd..abbc6e8a85 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12807,14 +12807,13 @@ virDomainChrDefParseXML(virDomainXMLOptionPtr xmlopt,
 }
 }
 
+if (virDomainDeviceInfoParseXML(xmlopt, node,
+>info, flags) < 0)
+goto error;
+
 if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
-def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) {
-VIR_DEBUG("Ignoring device address for gustfwd channel");
-} else if (virDomainDeviceInfoParseXML(xmlopt, node,
-   >info, flags) < 0) {
-goto error;
-}
-
+def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD)
+virDomainDeviceInfoClearAddress(>info);
 
 if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
 def->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB &&
-- 
2.16.4

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

Re: [libvirt] [Qemu-devel] [PATCH] Remove deprecated -balloon option

[Cornelia Huck]

On Tue, 21 Aug 2018 12:05:30 +0200
Thomas Huth  wrote:

> The "-balloon" option has been replaced by "-device virtio-balloon".
> It's been marked as deprecated since two releases, and nobody
> complained, so let's remove it now.
> 
> Signed-off-by: Thomas Huth 
> ---
>  docs/virtio-balloon-stats.txt |  6 +++---
>  qemu-deprecated.texi  |  5 -
>  qemu-options.hx   | 10 --
>  vl.c  | 36 
>  4 files changed, 3 insertions(+), 54 deletions(-)

Reviewed-by: Cornelia Huck 

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

Re: [libvirt] [Qemu-devel] [PATCH] Remove deprecated -balloon option

[Paolo Bonzini]

On 21/08/2018 12:05, Thomas Huth wrote:
> The "-balloon" option has been replaced by "-device virtio-balloon".
> It's been marked as deprecated since two releases, and nobody
> complained, so let's remove it now.
> 
> Signed-off-by: Thomas Huth 
> ---
>  docs/virtio-balloon-stats.txt |  6 +++---
>  qemu-deprecated.texi  |  5 -
>  qemu-options.hx   | 10 --
>  vl.c  | 36 
>  4 files changed, 3 insertions(+), 54 deletions(-)
> 
> diff --git a/docs/virtio-balloon-stats.txt b/docs/virtio-balloon-stats.txt
> index 9985e1d..1732cc8 100644
> --- a/docs/virtio-balloon-stats.txt
> +++ b/docs/virtio-balloon-stats.txt
> @@ -61,9 +61,9 @@ It's also important to note the following:
> respond to the request the timer will never be re-armed, which has
> the same effect as disabling polling
>  
> -Here are a few examples. QEMU is started with '-balloon virtio', which
> -generates '/machine/peripheral-anon/device[1]' as the QOM path for the
> -balloon device.
> +Here are a few examples. QEMU is started with '-device virtio-balloon',
> +which generates '/machine/peripheral-anon/device[1]' as the QOM path for
> +the balloon device.
>  
>  Enable polling with 2 seconds interval:
>  
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 67b7211..0714017 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -106,11 +106,6 @@ enabled via the ``-machine usb=on'' argument.
>  
>  The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
>  
> -@subsection -balloon (since 2.12.0)
> -
> -The @option{--balloon virtio} argument has been superseded by
> -@option{--device virtio-balloon}.
> -
>  @subsection -fsdev handle (since 2.12.0)
>  
>  The ``handle'' fsdev backend does not support symlinks and causes the 9p
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 4efdedf..47c6b92 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -454,16 +454,6 @@ modprobe i810_audio clocking=48000
>  @end example
>  ETEXI
>  
> -DEF("balloon", HAS_ARG, QEMU_OPTION_balloon,
> -"-balloon virtio[,addr=str]\n"
> -"enable virtio balloon device (deprecated)\n", 
> QEMU_ARCH_ALL)
> -STEXI
> -@item -balloon virtio[,addr=@var{addr}]
> -@findex -balloon
> -Enable virtio balloon device, optionally with PCI address @var{addr}. This
> -option is deprecated, use @option{-device virtio-balloon} instead.
> -ETEXI
> -
>  DEF("device", HAS_ARG, QEMU_OPTION_device,
>  "-device driver[,prop[=value][,...]]\n"
>  "add device (based on driver)\n"
> diff --git a/vl.c b/vl.c
> index 16b913f..f952f01 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2127,36 +2127,6 @@ static void parse_display(const char *p)
>  }
>  }
>  
> -static int balloon_parse(const char *arg)
> -{
> -QemuOpts *opts;
> -
> -warn_report("This option is deprecated. "
> -"Use '--device virtio-balloon' to enable the balloon 
> device.");
> -
> -if (strcmp(arg, "none") == 0) {
> -return 0;
> -}
> -
> -if (!strncmp(arg, "virtio", 6)) {
> -if (arg[6] == ',') {
> -/* have params -> parse them */
> -opts = qemu_opts_parse_noisily(qemu_find_opts("device"), arg + 7,
> -   false);
> -if (!opts)
> -return  -1;
> -} else {
> -/* create empty opts */
> -opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
> -_abort);
> -}
> -qemu_opt_set(opts, "driver", "virtio-balloon", _abort);
> -return 0;
> -}
> -
> -return -1;
> -}
> -
>  char *qemu_find_file(int type, const char *name)
>  {
>  int i;
> @@ -3659,12 +3629,6 @@ int main(int argc, char **argv, char **envp)
>  case QEMU_OPTION_no_hpet:
>  no_hpet = 1;
>  break;
> -case QEMU_OPTION_balloon:
> -if (balloon_parse(optarg) < 0) {
> -error_report("unknown -balloon argument %s", optarg);
> -exit(1);
> -}
> -break;
>  case QEMU_OPTION_no_reboot:
>  no_reboot = 1;
>  break;
> 

Acked-by: Paolo Bonzini 

Please send a pull request yourself with all deprecation removals.

Paolo

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

[libvirt] [PATCH v2 1/2] device_conf: Split virDomainDeviceInfoClear

[Michal Privoznik]

Some callers might want to clear the address but not the alias.
Split the virDomainDeviceInfoClear() function to allow that.

Signed-off-by: Michal Privoznik 
---
 src/conf/device_conf.c | 10 --
 src/conf/device_conf.h |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index d69f94fadf..8fa6bb4fe0 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -54,9 +54,8 @@ virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
 }
 
 void
-virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
+virDomainDeviceInfoClearAddress(virDomainDeviceInfoPtr info)
 {
-VIR_FREE(info->alias);
 memset(>addr, 0, sizeof(info->addr));
 info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
 VIR_FREE(info->romfile);
@@ -65,6 +64,13 @@ virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
 info->isolationGroupLocked = false;
 }
 
+void
+virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
+{
+VIR_FREE(info->alias);
+virDomainDeviceInfoClearAddress(info);
+}
+
 void
 virDomainDeviceInfoFree(virDomainDeviceInfoPtr info)
 {
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index a31ce9c376..1d0e9e6925 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -179,6 +179,7 @@ struct _virDomainDeviceInfo {
 
 int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
 virDomainDeviceInfoPtr src);
+void virDomainDeviceInfoClearAddress(virDomainDeviceInfoPtr info);
 void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info);
 void virDomainDeviceInfoFree(virDomainDeviceInfoPtr info);
 
-- 
2.16.4

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

[libvirt] [PATCH v2 0/2] Parse guestfwd channel device info again

[Michal Privoznik]

v2 of:

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

diff to v1:
- did what Andrea suggested

Michal Prívozník (2):
  device_conf: Split virDomainDeviceInfoClear
  conf: Parse guestfwd channel device info again

 src/conf/device_conf.c | 10 --
 src/conf/device_conf.h |  1 +
 src/conf/domain_conf.c | 13 ++---
 3 files changed, 15 insertions(+), 9 deletions(-)

-- 
2.16.4

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

Re: [libvirt] [PATCH] Remove deprecated -balloon option

[Daniel P . Berrangé]

On Tue, Aug 21, 2018 at 12:05:30PM +0200, Thomas Huth wrote:
> The "-balloon" option has been replaced by "-device virtio-balloon".
> It's been marked as deprecated since two releases, and nobody
> complained, so let's remove it now.
> 
> Signed-off-by: Thomas Huth 
> ---
>  docs/virtio-balloon-stats.txt |  6 +++---
>  qemu-deprecated.texi  |  5 -
>  qemu-options.hx   | 10 --
>  vl.c  | 36 
>  4 files changed, 3 insertions(+), 54 deletions(-)

Confirmed that this is no longer used by libvirt, as we assume we have
new enough QEMU for -device

  Reviewed-by: Daniel P. Berrangé 


> 
> diff --git a/docs/virtio-balloon-stats.txt b/docs/virtio-balloon-stats.txt
> index 9985e1d..1732cc8 100644
> --- a/docs/virtio-balloon-stats.txt
> +++ b/docs/virtio-balloon-stats.txt
> @@ -61,9 +61,9 @@ It's also important to note the following:
> respond to the request the timer will never be re-armed, which has
> the same effect as disabling polling
>  
> -Here are a few examples. QEMU is started with '-balloon virtio', which
> -generates '/machine/peripheral-anon/device[1]' as the QOM path for the
> -balloon device.
> +Here are a few examples. QEMU is started with '-device virtio-balloon',
> +which generates '/machine/peripheral-anon/device[1]' as the QOM path for
> +the balloon device.

We should try to get in the habit of changing the docs at the time we
deprecate stuff. I keep forgetting todo this myself too :-)

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] Remove deprecated -balloon option

[Thomas Huth]

The "-balloon" option has been replaced by "-device virtio-balloon".
It's been marked as deprecated since two releases, and nobody
complained, so let's remove it now.

Signed-off-by: Thomas Huth 
---
 docs/virtio-balloon-stats.txt |  6 +++---
 qemu-deprecated.texi  |  5 -
 qemu-options.hx   | 10 --
 vl.c  | 36 
 4 files changed, 3 insertions(+), 54 deletions(-)

diff --git a/docs/virtio-balloon-stats.txt b/docs/virtio-balloon-stats.txt
index 9985e1d..1732cc8 100644
--- a/docs/virtio-balloon-stats.txt
+++ b/docs/virtio-balloon-stats.txt
@@ -61,9 +61,9 @@ It's also important to note the following:
respond to the request the timer will never be re-armed, which has
the same effect as disabling polling
 
-Here are a few examples. QEMU is started with '-balloon virtio', which
-generates '/machine/peripheral-anon/device[1]' as the QOM path for the
-balloon device.
+Here are a few examples. QEMU is started with '-device virtio-balloon',
+which generates '/machine/peripheral-anon/device[1]' as the QOM path for
+the balloon device.
 
 Enable polling with 2 seconds interval:
 
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 67b7211..0714017 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -106,11 +106,6 @@ enabled via the ``-machine usb=on'' argument.
 
 The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
 
-@subsection -balloon (since 2.12.0)
-
-The @option{--balloon virtio} argument has been superseded by
-@option{--device virtio-balloon}.
-
 @subsection -fsdev handle (since 2.12.0)
 
 The ``handle'' fsdev backend does not support symlinks and causes the 9p
diff --git a/qemu-options.hx b/qemu-options.hx
index 4efdedf..47c6b92 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -454,16 +454,6 @@ modprobe i810_audio clocking=48000
 @end example
 ETEXI
 
-DEF("balloon", HAS_ARG, QEMU_OPTION_balloon,
-"-balloon virtio[,addr=str]\n"
-"enable virtio balloon device (deprecated)\n", 
QEMU_ARCH_ALL)
-STEXI
-@item -balloon virtio[,addr=@var{addr}]
-@findex -balloon
-Enable virtio balloon device, optionally with PCI address @var{addr}. This
-option is deprecated, use @option{-device virtio-balloon} instead.
-ETEXI
-
 DEF("device", HAS_ARG, QEMU_OPTION_device,
 "-device driver[,prop[=value][,...]]\n"
 "add device (based on driver)\n"
diff --git a/vl.c b/vl.c
index 16b913f..f952f01 100644
--- a/vl.c
+++ b/vl.c
@@ -2127,36 +2127,6 @@ static void parse_display(const char *p)
 }
 }
 
-static int balloon_parse(const char *arg)
-{
-QemuOpts *opts;
-
-warn_report("This option is deprecated. "
-"Use '--device virtio-balloon' to enable the balloon device.");
-
-if (strcmp(arg, "none") == 0) {
-return 0;
-}
-
-if (!strncmp(arg, "virtio", 6)) {
-if (arg[6] == ',') {
-/* have params -> parse them */
-opts = qemu_opts_parse_noisily(qemu_find_opts("device"), arg + 7,
-   false);
-if (!opts)
-return  -1;
-} else {
-/* create empty opts */
-opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
-_abort);
-}
-qemu_opt_set(opts, "driver", "virtio-balloon", _abort);
-return 0;
-}
-
-return -1;
-}
-
 char *qemu_find_file(int type, const char *name)
 {
 int i;
@@ -3659,12 +3629,6 @@ int main(int argc, char **argv, char **envp)
 case QEMU_OPTION_no_hpet:
 no_hpet = 1;
 break;
-case QEMU_OPTION_balloon:
-if (balloon_parse(optarg) < 0) {
-error_report("unknown -balloon argument %s", optarg);
-exit(1);
-}
-break;
 case QEMU_OPTION_no_reboot:
 no_reboot = 1;
 break;
-- 
1.8.3.1

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

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

[Paolo Bonzini]

On 21/08/2018 09:45, Gerd Hoffmann wrote:
> +qemu_mutex_lock(>lock);
>  if (ssd->cursor) {
> +QEMUCursor *c = ssd->cursor;
>  assert(ssd->dcl.con);
> +cursor_get(c);
> +qemu_mutex_unlock(>lock);
>  dpy_cursor_define(ssd->dcl.con, ssd->cursor);

Gerd,

this ssd->cursor should be "c" in the call to dpy_cursor_define.  My
apologies; please tell me if you'd like me to send a follow-up fix.

Paolo

> +qemu_mutex_lock(>lock);
> +cursor_put(c);
>  }

--
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/21 下午4:06, Andrea Bolognani 写道:

On Tue, 2018-08-21 at 11:24 +0800, Yi Min Zhao wrote:

在 2018/8/20 下午6:35, Andrea Bolognani 写道:

You could do the same thing those functions do:

* the zPCI address is empty if both uid and fid are zero;

uid=0 and fid=0 can't mean zPCI address is empty, because the user might
only define fid with 0. If fid=0 has been assigned, we should report
error. If
it is not defined by user, fid is also 0, then we should allocate a
valid value
for fid.

For the PCI part

   

behaves the same as

   

and results in a PCI address being allocated automatically by
libvirt rather than an error being reported.

As with zPCI, zero is a valid value for some, but not all, of the
attributes: validation is performed if at least one of them is
non-zero and might result in an error being reported.

Doing the same with the zPCI part would not only allow you to get
rid of the extra booleans but would also guarantee a consistent
behavior, which is a worthy goal in and of itself.


I want to ask a question. For pci address, any pci device can't use slot 0.
Is that a reason why PCI part could treat all zeros as empty address?

For zPCI address, if we use the same strategy as PCI part and user
wants to assign fid=0 to the specific device, he might encounter
assignment failure. At least, according to the doc, 0 shoud be a valid
value to assign to fid.

Anyway, when I wrote this code, I also wanted to use the similar logic to
check if zPCI address is empty.

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

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

[Simon Kobyda]

On Thu, 2018-08-16 at 12:28 +0100, Daniel P. Berrangé wrote:
> On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
> > 
> 
> After asking around I have found the right solution that we need to
> use
> for measuring string width.  mbstowcs()/wcswidth() will get the
> answer
> wrong wrt zero-width characters, combining characters, non-printable
> characters, etc. We need to use the libunistring library:
> 
>   
> https://www.gnu.org/software/libunistring/manual/libunistring.html#uniwidth_002eh
> 
> 
I've tried what you've suggested, but it seems that it doesn't work
well with all unicode characters. I'm looking into the code of the
library, and each function uN_strwidth calls function uN_width, and
that function calls uc_width for calculation of width of characters.
And if we look into the code of uc_width here:

http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/uniwidth/width.c;h=269cfc77f50a3b9802e5fb5620ff8bcf95e05e40;hb=HEAD#l415
it seems that this library is limited only to certain unicodes, e.g.:
hangul characters, angle brackets, CJK characters... But it doesn't
cover all multiple-width characters. Example: I try to throw any emoji
(e.g. , 呂, ), it returns width of 1 column for each charact
er, nevertheless these characters have width of 2 columns on terminal.

BTW, it seems unistring library imports those funcions from gnulib.

Simon Kobyda.

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

Re: [libvirt] [PATCH] util: Don't delete the original file for truncation

[Daniel P . Berrangé]

On Tue, Aug 21, 2018 at 10:49:28AM +0200, Marc Hartmayer wrote:
> Truncate means that if a file exists it's length will be truncated to
> 0, but the mode and the owner shall be unchanged. The current behavior
> is that the original file is deleted and a new file is created. Let's
> fix this by using O_TRUNC.

This is just describing what you've changed, leaving out why you are
trying todo this ?

> The function virRotatingFileWriterDelete is now unused but may be used
> in the future and is therefore still defined.
> 
> Signed-off-by: Marc Hartmayer 
> Reviewed-by: Boris Fiuczynski 
> ---
> Note:
> 
> This change has the (potentially unwanted) security effect that the
> owner/group of the log file does not change. Before this patch the old
> log file was deleted and the newly created log file was owned by the
> virtlogd user. Now, if a user has created the log file before, he can
> read the logs. If we don't wanna have this effect we can either
> adjust/add a virtlogd API or do a chown within the calling driver
> (e.g. QEMU driver).

Pre-creating the log file and/or messing around with ownership are
not things we ever intended to support.

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] conf: Parse guestfwd channel device info again

[Andrea Bolognani]

On Mon, 2018-08-20 at 18:52 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1610072
> 
> Due to historical reasons we were not parsing device info on

You spelled "hysterical raisins" wrong ;)

[...]
> -if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
> -def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) {
> -VIR_DEBUG("Ignoring device address for gustfwd channel");
> -} else if (virDomainDeviceInfoParseXML(xmlopt, node,
> -   >info, flags) < 0) {
> +if (virDomainDeviceInfoParseXML(xmlopt, node, >info, flags) < 0)
>  goto error;
> -}
> -

I agree that fixing Bug 1610072 is more important than preventing
Bug 1172526 from showing up again, but it would be great if we could
make it so both are fixed...

How about parsing the info and then clearing out the address only if
it's a guestfwd channel? The existing virDomainDeviceInfoClear() is
a bit too thorough, but perhaps you can introduce a new
virDomainDeviceInfoClearAddress() that only zeroes out the address
and use that here.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH] util: Don't delete the original file for truncation

[Marc Hartmayer]

Truncate means that if a file exists it's length will be truncated to
0, but the mode and the owner shall be unchanged. The current behavior
is that the original file is deleted and a new file is created. Let's
fix this by using O_TRUNC.

The function virRotatingFileWriterDelete is now unused but may be used
in the future and is therefore still defined.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Boris Fiuczynski 
---
Note:

This change has the (potentially unwanted) security effect that the
owner/group of the log file does not change. Before this patch the old
log file was deleted and the newly created log file was owned by the
virtlogd user. Now, if a user has created the log file before, he can
read the logs. If we don't wanna have this effect we can either
adjust/add a virtlogd API or do a chown within the calling driver
(e.g. QEMU driver).

Side note: the original behavior has changed with the patch series
"qemu: use FD passing for chardev UNIX sockets".

Example where it the behavior has changed:

 
  
  
 
---
 src/util/virrotatingfile.c | 49 --
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/src/util/virrotatingfile.c b/src/util/virrotatingfile.c
index ca62a8e02641..d6ab3780aae3 100644
--- a/src/util/virrotatingfile.c
+++ b/src/util/virrotatingfile.c
@@ -98,17 +98,25 @@ 
virRotatingFileReaderEntryFree(virRotatingFileReaderEntryPtr entry)
 
 static virRotatingFileWriterEntryPtr
 virRotatingFileWriterEntryNew(const char *path,
-  mode_t mode)
+  mode_t mode,
+  bool truncate)
 {
 virRotatingFileWriterEntryPtr entry;
 struct stat sb;
+/* O_APPEND is also useful in combination with O_TRUNC since it
+ * guarantees the atomicity of a write operation (at least for
+ * POSIX systems) */
+int oflag = O_CREAT|O_APPEND|O_WRONLY|O_CLOEXEC;
 
 VIR_DEBUG("Opening %s mode=0%02o", path, mode);
 
 if (VIR_ALLOC(entry) < 0)
 return NULL;
 
-if ((entry->fd = open(path, O_CREAT|O_APPEND|O_WRONLY|O_CLOEXEC, mode)) < 
0) {
+if (truncate)
+oflag |= O_TRUNC;
+
+if ((entry->fd = open(path, oflag, mode)) < 0) {
 virReportSystemError(errno,
  _("Unable to open file: %s"), path);
 goto error;
@@ -182,18 +190,10 @@ virRotatingFileReaderEntryNew(const char *path)
 
 
 static int
-virRotatingFileWriterDelete(virRotatingFileWriterPtr file)
+virRotatingFileWriterDeleteBackup(virRotatingFileWriterPtr file)
 {
 size_t i;
 
-if (unlink(file->basepath) < 0 &&
-errno != ENOENT) {
-virReportSystemError(errno,
- _("Unable to delete file %s"),
- file->basepath);
-return -1;
-}
-
 for (i = 0; i < file->maxbackup; i++) {
 char *oldpath;
 if (virAsprintf(, "%s.%zu", file->basepath, i) < 0)
@@ -214,6 +214,24 @@ virRotatingFileWriterDelete(virRotatingFileWriterPtr file)
 }
 
 
+static int ATTRIBUTE_UNUSED
+virRotatingFileWriterDelete(virRotatingFileWriterPtr file)
+{
+if (unlink(file->basepath) < 0 &&
+errno != ENOENT) {
+virReportSystemError(errno,
+ _("Unable to delete file %s"),
+ file->basepath);
+return -1;
+}
+
+if (virRotatingFileWriterDeleteBackup(file) < 0)
+return -1;
+
+return 0;
+}
+
+
 /**
  * virRotatingFileWriterNew
  * @path: the base path for files
@@ -257,12 +275,12 @@ virRotatingFileWriterNew(const char *path,
 file->maxbackup = maxbackup;
 file->maxlen = maxlen;
 
-if (trunc &&
-virRotatingFileWriterDelete(file) < 0)
+if (trunc && virRotatingFileWriterDeleteBackup(file) < 0)
 goto error;
 
 if (!(file->entry = virRotatingFileWriterEntryNew(file->basepath,
-  mode)))
+  mode,
+  trunc)))
 goto error;
 
 return file;
@@ -491,7 +509,8 @@ virRotatingFileWriterAppend(virRotatingFileWriterPtr file,
 return -1;
 
 if (!(tmp = virRotatingFileWriterEntryNew(file->basepath,
-  file->mode)))
+  file->mode,
+  false)))
 return -1;
 
 virRotatingFileWriterEntryFree(file->entry);
-- 
2.13.4

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

Re: [libvirt] [PATCH v2 6/7] domain_lock: Implement metadata locking

[Daniel P . Berrangé]

On Mon, Aug 20, 2018 at 07:13:46PM +0200, Michal Prívozník wrote:
> On 08/20/2018 05:07 PM, Daniel P. Berrangé wrote:
> > On Tue, Aug 14, 2018 at 01:19:42PM +0200, Michal Privoznik wrote:
> >> In order for our drivers to lock resources for metadata change we
> >> need set of new APIs. Fortunately, we don't have to care about
> >> every possible device a domain can have. We care only about those
> >> which can live on a network filesystem and hence can be accessed
> >> by multiple daemons at the same time. These devices are covered
> >> in virDomainLockMetadataLock() and only a small fraction of
> >> those can be hotplugged (covered in the rest of the introduced
> >> APIs).
> > 
> > I'm not sure I understand the rationale behind saying we only care
> > about resources on network filesystems.
> > 
> > If I have 2 locally running guests, and both have a serial port
> > backed by a physical serial port, eg
> > 
> >   
> > 
> > 
> >   
> > 
> > we *do* care about locking /dev/ttyS0, as libvirtd isn't doing
> > mutual exclusion checks anywhere else for the /dev/ttyS0 device
> > node.
> 
> Ah you mean that the system wide daemon and session daemon could clash
> when relabeling /dev/ttyS0? Well, we don't do relabeling for session
> daemons and running two system daemons is not supported (you're gonna
> hit more serious problems when trying that anyway).

We certainly *do* have relabelling for session daemons, for SELinux:

$ ls -alZ ~/VirtualMachines/demo.img 
-rw-r--r--. 1 berrange berrange unconfined_u:object_r:virt_home_t:s0 1073741824 
Aug 21 09:25 /home/berrange/VirtualMachines/demo.img

$ virsh uri
qemu:///session

$ virsh start QEMUGuest1
Domain QEMUGuest1 started

$ ls -alZ ~/VirtualMachines/demo.img 
-rw-r--r--. 1 berrange berrange 
unconfined_u:object_r:svirt_image_t:s0:c310,c831 1073741824 Aug 21 09:25 
/home/berrange/VirtualMachines/demo.img


but I was more thinking about the future where we have the libvirt shim
concept that allows starting & managing of QEMU processes independantly,
and libvirtd is merely there for aggregated data reporting. In that case
you'd have many instances of the security driver operating in parallel.

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 5/7] lock_driver_sanlock: Handle metadata flag gracefully

[Daniel P . Berrangé]

On Mon, Aug 20, 2018 at 07:29:30PM +0200, Michal Prívozník wrote:
> On 08/20/2018 05:04 PM, Daniel P. Berrangé wrote:
> > On Mon, Aug 20, 2018 at 03:59:23PM +0100, Daniel P. Berrangé wrote:
> >> On Fri, Aug 17, 2018 at 10:49:12AM -0400, John Ferlan wrote:
> >>>
> >>>
> >>> On 08/14/2018 07:19 AM, Michal Privoznik wrote:
>  No real support implemented here. But hey, at least we will not
>  fail.
> 
>  Signed-off-by: Michal Privoznik 
>  ---
>   src/locking/lock_driver_sanlock.c | 25 ++---
>   1 file changed, 18 insertions(+), 7 deletions(-)
> 
>  diff --git a/src/locking/lock_driver_sanlock.c 
>  b/src/locking/lock_driver_sanlock.c
>  index 3e5f0e37b0..c1996fb937 100644
>  --- a/src/locking/lock_driver_sanlock.c
>  +++ b/src/locking/lock_driver_sanlock.c
>  @@ -791,7 +791,8 @@ static int 
>  virLockManagerSanlockAddResource(virLockManagerPtr lock,
>   virLockManagerSanlockPrivatePtr priv = lock->privateData;
>   
>   virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY |
>  -  VIR_LOCK_MANAGER_RESOURCE_SHARED, -1);
>  +  VIR_LOCK_MANAGER_RESOURCE_SHARED |
>  +  VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);
>   
>   if (priv->res_count == SANLK_MAX_RESOURCES) {
>   virReportError(VIR_ERR_INTERNAL_ERROR,
>  @@ -804,6 +805,11 @@ static int 
>  virLockManagerSanlockAddResource(virLockManagerPtr lock,
>   if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
>   return 0;
>   
>  +/* No metadata locking support for now.
>  + * TODO: implement it. */
>  +if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA)
>  +return 0;
>  +
> >>>
> >>> Doesn't this give someone the false impression that their resource is
> >>> locked if they choose METADATA?
> >>>
> >>> Something doesn't feel right about that - giving the impression that
> >>> it's supported and the consumer is protected, but when push comes to
> >>> shove they aren't.
> >>>
> >>> I'd be inclined to believe that we may want to do nothing with/for
> >>> sanlock allowing the virCheckFlags above take care of the consumer.
> >>
> >> Yeah, this doesn't feel right to me. I think we need to treat the
> >> metadata locking as completely independant of the content locking.
> >> This implies we should have a separate configuration for metadata
> >> locking, where the only valid options are "lockd" or "nop".
> >>
> >> eg our available matrix looks like
> >>
> >> METADATA
> >>  | nop   lockd  sanlock
> >> -+ 
> >>  nop |  Y  Y  N
> >> CONTENTlockd |  Y  Y  N
> >>  sanlock |  Y  Y  N
> 
> Having some troubles parsing the table. Do you mean:
> 
>  | Content | Metadata
> -+---
>  nop |Y|Y
>lockd |Y|Y
>  sanlock |Y|N
> 
> Where Y says the respective type (content/metadata) can/cannot be locked
> by lock driver in question? I.e. we would have 'metadata_lock_manager'
> config value in qemu.conf and it'd accept only 'nop' and 'lockd'.

Heh, ok, yes, that is a simpler table :-)

> Then we can have 'metadata_lockspace_dir' in
> /etc/libvirt/qemu-lockd.conf where the lockspace would be created.

No need for a metadata_lockspace_dir, since we should always be locking
the resource itself, never an out of band proxy file.

> > Oh and even for virtlockd we need to consider the config separately
> > for content vs metdata.
> 
> Do you mean like completely new config file? qemu-lockd-metadata.conf?
> Okay. So far we only need one config option (metadata_lockspace_dir) but
> it might turn out we need more and it would make sens to keep options
> separated from content locking options.

I don't think we need a config file for now.


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 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes

[Andrea Bolognani]

On Tue, 2018-08-21 at 11:24 +0800, Yi Min Zhao wrote:
> 在 2018/8/20 下午6:35, Andrea Bolognani 写道:
> > You could do the same thing those functions do:
> > 
> >* the zPCI address is empty if both uid and fid are zero;
> 
> uid=0 and fid=0 can't mean zPCI address is empty, because the user might
> only define fid with 0. If fid=0 has been assigned, we should report 
> error. If
> it is not defined by user, fid is also 0, then we should allocate a 
> valid value
> for fid.

For the PCI part

  

behaves the same as

  

and results in a PCI address being allocated automatically by
libvirt rather than an error being reported.

As with zPCI, zero is a valid value for some, but not all, of the
attributes: validation is performed if at least one of them is
non-zero and might result in an error being reported.

Doing the same with the zPCI part would not only allow you to get
rid of the extra booleans but would also guarantee a consistent
behavior, which is a worthy goal in and of itself.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
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

[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 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 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

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

[Gerd Hoffmann]

On Wed, Aug 08, 2018 at 11:49:27AM +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.

Doesn't apply any more.
Can you rebase and resend?

thanks,
  Gerd

--
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 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 20180821 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-20180821-pull-request

for you to fetch changes up to 8a674d96a572c7ec1219a24ef223160dc6a27ca8:

  util: promote qemu_egl_rendernode_open() to libqemuutil (2018-08-21 08:24:30 
+0200)


ui: misc ui fixed 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 via Qemu-devel (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   | 40 ++
 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, 138 insertions(+), 236 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 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 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 f1d341091a..e3d0fde77a 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 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 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

[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
Signed-off-by: Gerd Hoffmann 
---
 ui/spice-display.c | 28 +---
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 46df673cd7..f1d341091a 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);
+cursor_get(c);
+qemu_mutex_unlock(>lock);
 dpy_cursor_define(ssd->dcl.con, ssd->cursor);
+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 04/12] sdl2: redraw correctly when scanout_mode enabled.

[Gerd Hoffmann]

From: Tao Wu via Qemu-devel 

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