[libvirt] [PATCH] storage_backend_rbd: check the return value of rados_conf_set

[Chen Hanxiao]

From: Chen Hanxiao 

We forget to check the return value of rados_conf_set.

Signed-off-by: Chen Hanxiao 
---
 src/storage/storage_backend_rbd.c | 21 ++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/src/storage/storage_backend_rbd.c 
b/src/storage/storage_backend_rbd.c
index 718c4d6..58bcb9a 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -162,10 +162,20 @@ 
virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
 rados_conf_set(ptr->cluster, "client_mount_timeout", client_mount_timeout);
 
 VIR_DEBUG("Setting RADOS option rados_mon_op_timeout to %s", 
mon_op_timeout);
-rados_conf_set(ptr->cluster, "rados_mon_op_timeout", mon_op_timeout);
+if (rados_conf_set(ptr->cluster, "rados_mon_op_timeout", mon_op_timeout) < 
0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("failed to set RADOS option: %s"),
+   "rados_mon_op_timeout");
+goto cleanup;
+}
 
 VIR_DEBUG("Setting RADOS option rados_osd_op_timeout to %s", 
osd_op_timeout);
-rados_conf_set(ptr->cluster, "rados_osd_op_timeout", osd_op_timeout);
+if (rados_conf_set(ptr->cluster, "rados_osd_op_timeout", osd_op_timeout) < 
0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("failed to set RADOS option: %s"),
+   "rados_osd_op_timeout");
+goto cleanup;
+}
 
 /*
  * Librbd supports creating RBD format 2 images. We no longer have to 
invoke
@@ -173,7 +183,12 @@ 
virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
  * This leaves us to simply use rbd_create() and use the default behavior 
of librbd
  */
 VIR_DEBUG("Setting RADOS option rbd_default_format to %s", 
rbd_default_format);
-rados_conf_set(ptr->cluster, "rbd_default_format", rbd_default_format);
+if (rados_conf_set(ptr->cluster, "rbd_default_format", rbd_default_format) 
< 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("failed to set RADOS option: %s"),
+   "rbd_default_format");
+goto cleanup;
+}
 
 ptr->starttime = time(0);
 if ((r = rados_connect(ptr->cluster)) < 0) {
-- 
1.8.3.1


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

[libvirt] [PATCH 0/2] Forbid new-line char in name of domains and storagepools

[Sławek Kapłoński]

As https://bugzilla.redhat.com/show_bug.cgi?id=818064 is closed and new-line
char is now forbiden in name of network, this patch forbids it also in names of
domains and storage pools.

Sławek Kapłoński (2):
  Forbid new-line char in name of new domain
  Forbid new-line char in name of new storagepool

 src/bhyve/bhyve_driver.c | 3 +++
 src/esx/esx_driver.c | 3 +++
 src/libxl/libxl_driver.c | 3 +++
 src/lxc/lxc_driver.c | 3 +++
 src/openvz/openvz_driver.c   | 3 +++
 src/qemu/qemu_driver.c   | 3 +++
 src/storage/storage_driver.c | 3 +++
 src/test/test_driver.c   | 3 +++
 src/uml/uml_driver.c | 3 +++
 src/vmware/vmware_driver.c   | 3 +++
 src/vz/vz_driver.c   | 3 +++
 src/xen/xen_driver.c | 3 +++
 src/xenapi/xenapi_driver.c   | 3 +++
 13 files changed, 39 insertions(+)

-- 
2.7.4

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

[libvirt] [PATCH 2/2] Forbid new-line char in name of new storagepool

[Sławek Kapłoński]

New line character in name of storagepool is now forbidden because it
mess virsh output and can be confusing for users.
Validation of name is done in driver, after parsing XML to avoid
problems with dissappeared pools which was already created with
new-line char in name.
---
 src/storage/storage_driver.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 4f990f4..9900596 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -780,6 +780,9 @@ storagePoolDefineXML(virConnectPtr conn,
 if (!(def = virStoragePoolDefParseString(xml)))
 goto cleanup;
 
+if (virXMLCheckIllegalChars("name", def->name, "\n") < 0)
+goto cleanup;
+
 if (virStoragePoolDefineXMLEnsureACL(conn, def) < 0)
 goto cleanup;
 
-- 
2.7.4

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

[libvirt] [PATCH 1/2] Forbid new-line char in name of new domain

[Sławek Kapłoński]

New line character in name of domain is now forbidden because it
mess virsh output and can be confusing for users.
Validation of name is done in drivers, after parsing XML to avoid
problems with dissappeared domains which was already created with
new-line char in name.
---
 src/bhyve/bhyve_driver.c   | 3 +++
 src/esx/esx_driver.c   | 3 +++
 src/libxl/libxl_driver.c   | 3 +++
 src/lxc/lxc_driver.c   | 3 +++
 src/openvz/openvz_driver.c | 3 +++
 src/qemu/qemu_driver.c | 3 +++
 src/test/test_driver.c | 3 +++
 src/uml/uml_driver.c   | 3 +++
 src/vmware/vmware_driver.c | 3 +++
 src/vz/vz_driver.c | 3 +++
 src/xen/xen_driver.c   | 3 +++
 src/xenapi/xenapi_driver.c | 3 +++
 12 files changed, 36 insertions(+)

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 38fb9f0..17f8524 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -541,6 +541,9 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flag
NULL, parse_flags)) == NULL)
 goto cleanup;
 
+if (virXMLCheckIllegalChars("name", def->name, "\n") < 0)
+goto cleanup;
+
 if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
 goto cleanup;
 
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 17ef00f..166d4bc 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -3051,6 +3051,9 @@ esxDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flags)
 if (!def)
 return NULL;
 
+if (virXMLCheckIllegalChars("name", def->name, "\n") < 0)
+goto cleanup;
+
 /* Check if an existing domain should be edited */
 if (esxVI_LookupVirtualMachineByUuid(priv->primary, def->uuid, NULL,
  &virtualMachine,
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index b2f3b16..3efa91e 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -2791,6 +2791,9 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flag
 NULL, parse_flags)))
 goto cleanup;
 
+if (virXMLCheckIllegalChars("name", def->name, "\n") < 0)
+goto cleanup;
+
 if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
 goto cleanup;
 
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 4a0165a..a6776a1 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -475,6 +475,9 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flags)
 NULL, parse_flags)))
 goto cleanup;
 
+if (virXMLCheckIllegalChars("name", def->name, "\n") < 0)
+goto cleanup;
+
 if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
 goto cleanup;
 
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 38a562e..1dfb1cc 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -1000,6 +1000,9 @@ openvzDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int fla
  NULL, parse_flags)) == NULL)
 goto cleanup;
 
+if (virXMLCheckIllegalChars("name", vmdef->name, "\n") < 0)
+goto cleanup;
+
 if (!(vm = virDomainObjListAdd(driver->domains, vmdef,
driver->xmlopt,
0, NULL)))
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a82e58b..8d337eb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7339,6 +7339,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
 NULL, parse_flags)))
 goto cleanup;
 
+if (virXMLCheckIllegalChars("name", def->name, "\n") < 0)
+goto cleanup;
+
 if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
 goto cleanup;
 
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 236874f..1b54839 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -2641,6 +2641,9 @@ static virDomainPtr 
testDomainDefineXMLFlags(virConnectPtr conn,
NULL, parse_flags)) == NULL)
 goto cleanup;
 
+if (virXMLCheckIllegalChars("name", def->name, "\n") < 0)
+goto cleanup;
+
 if (testDomainGenerateIfnames(def) < 0)
 goto cleanup;
 if (!(dom = virDomainObjListAdd(privconn->domains,
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 4f4a69b..ad89e3e 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -2074,6 +2074,9 @@ umlDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flags)
 NULL, parse_flags)))
 goto cleanup;
 
+if (virXMLCheckIllegalChars("name", def->name, "\n") < 0)
+goto cleanup;
+
 if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0)
 goto cleanup;
 
diff --git a/src/

Re: [libvirt] [PATCH] storage_backend_rbd: check the return value of rados_conf_set

[Peter Krempa]

On Fri, Nov 11, 2016 at 16:33:18 +0800, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> We forget to check the return value of rados_conf_set.
> 
> Signed-off-by: Chen Hanxiao 
> ---
>  src/storage/storage_backend_rbd.c | 21 ++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/src/storage/storage_backend_rbd.c 
> b/src/storage/storage_backend_rbd.c
> index 718c4d6..58bcb9a 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -162,10 +162,20 @@ 
> virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
>  rados_conf_set(ptr->cluster, "client_mount_timeout", 
> client_mount_timeout);

Right in this context is one instance that is not changed ^^^.

>  
>  VIR_DEBUG("Setting RADOS option rados_mon_op_timeout to %s", 
> mon_op_timeout);
> -rados_conf_set(ptr->cluster, "rados_mon_op_timeout", mon_op_timeout);
> +if (rados_conf_set(ptr->cluster, "rados_mon_op_timeout", mon_op_timeout) 
> < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("failed to set RADOS option: %s"),
> +   "rados_mon_op_timeout");
> +goto cleanup;
> +}

Did you have any problems with this? The documentation mentions only one
error code (ENOENT) if the given config option does not exist in
librados. This would point to us something doing wrong rather than a
random failure and we should address the primary cause.

Peter



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

Re: [libvirt] [PATCH] storage_backend_rbd: check the return value of rados_conf_set

[Chen Hanxiao]

At 2016-11-11 17:27:48, "Peter Krempa"  wrote:
>On Fri, Nov 11, 2016 at 16:33:18 +0800, Chen Hanxiao wrote:
>> From: Chen Hanxiao 
>> 
>> We forget to check the return value of rados_conf_set.
>> 
>> Signed-off-by: Chen Hanxiao 
>> ---
>>  src/storage/storage_backend_rbd.c | 21 ++---
>>  1 file changed, 18 insertions(+), 3 deletions(-)
>> 
>> diff --git a/src/storage/storage_backend_rbd.c 
>> b/src/storage/storage_backend_rbd.c
>> index 718c4d6..58bcb9a 100644
>> --- a/src/storage/storage_backend_rbd.c
>> +++ b/src/storage/storage_backend_rbd.c
>> @@ -162,10 +162,20 @@ 
>> virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
>>  rados_conf_set(ptr->cluster, "client_mount_timeout", 
>> client_mount_timeout);
>
>Right in this context is one instance that is not changed ^^^.

Oops...

>
>>  
>>  VIR_DEBUG("Setting RADOS option rados_mon_op_timeout to %s", 
>> mon_op_timeout);
>> -rados_conf_set(ptr->cluster, "rados_mon_op_timeout", mon_op_timeout);
>> +if (rados_conf_set(ptr->cluster, "rados_mon_op_timeout", 
>> mon_op_timeout) < 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> +   _("failed to set RADOS option: %s"),
>> +   "rados_mon_op_timeout");
>> +goto cleanup;
>> +}
>
>Did you have any problems with this? The documentation mentions only one

In a test, I failed in rados_connect once.
When I try again, it works.
So I wonder maybe something wrong in rados_conf_set.

>error code (ENOENT) if the given config option does not exist in
>librados. This would point to us something doing wrong rather than a
>random failure and we should address the primary cause.

Any hints?

Regards,
- Chen

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

Re: [libvirt] [PATCH] qemu: Emit domain events for all virtio-serial channels

[Peter Krempa]

On Mon, Nov 07, 2016 at 15:48:35 -0500, Matt Broadstone wrote:
> Presently domain events are emitted for the "agent lifecycle" which
> is a specialization on virtio-serial events specific to the channel
> named "org.qemu.guest_agent.0". This patch adds a generic event for
> "channel lifecycles", which emit state change events for all
> attached channels.
> ---
>  daemon/remote.c | 42 +
>  examples/object-events/event-test.c | 57 
>  include/libvirt/libvirt-domain.h| 41 +
>  src/conf/domain_event.c | 89 
> +
>  src/conf/domain_event.h | 12 +
>  src/libvirt_private.syms|  2 +
>  src/qemu/qemu_driver.c  |  5 +++
>  src/qemu/qemu_process.c |  6 +++
>  src/remote/remote_driver.c  | 32 +
>  src/remote/remote_protocol.x| 17 ++-
>  src/remote_protocol-structs |  7 +++
>  tools/virsh-domain.c| 35 +++
>  12 files changed, 344 insertions(+), 1 deletion(-)
> 

[...]

> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index 5f50660..d720132 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -3965,6 +3965,46 @@ typedef void 
> (*virConnectDomainEventAgentLifecycleCallback)(virConnectPtr conn,
>  int reason,
>  void *opaque);
>  
> +typedef enum {
> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_STATE_CONNECTED = 1, /* 
> channel connected */
> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_STATE_DISCONNECTED = 2, /* 
> channel disconnected */
> +
> +# ifdef VIR_ENUM_SENTINELS
> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_STATE_LAST
> +# endif
> +} virConnectDomainEventChannelLifecycleState;
> +
> +typedef enum {
> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_UNKNOWN = 0, /* 
> unknown state change reason */
> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_DOMAIN_STARTED = 1, /* 
> state changed due to domain start */

This reason is not used at all and I don't think it even makes sense to
be reported for channel events. With guest agent it might make sense and
also other possible reasons as guest agent error.

With channels you mostly care about the connect and disconnect events.
It might make sense to report client connect or disconnect in the future
for certain backend types, but I don't think that qemu supports such
report currently.

> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_CHANNEL = 2, /* 
> channel state changed */
> +
> +# ifdef VIR_ENUM_SENTINELS
> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_LAST
> +# endif
> +} virConnectDomainEventChannelLifecycleReason;
> +
> +/**
> + * virConnectDomainEventChannelLifecycleCallback:
> + * @conn: connection object
> + * @dom: domain on which the event occurred
> + * @channelName: the name of the channel on which the event occurred
> + * @state: new state of the guest channel, one of 
> virConnectDomainEventChannelLifecycleState
> + * @reason: reason for state change; one of 
> virConnectDomainEventChannelLifecycleReason
> + * @opaque: application specified data
> + *
> + * This callback occurs when libvirt detects a change in the state of a guest
> + * serial channel.

You should note that the hypervisor needs to support channel state
notifications, otherwise it won't work. (Qemu supporting this has been
around for ages now, but we still support versions older than that. Also
other hypervisors don't support this currently)

> + *
> + * The callback signature to use when registering for an event of type
> + * VIR_DOMAIN_EVENT_ID_CHANNEL_LIFECYCLE with 
> virConnectDomainEventRegisterAny()
> + */
> +typedef void (*virConnectDomainEventChannelLifecycleCallback)(virConnectPtr 
> conn,
> +  virDomainPtr 
> dom,
> +  const char 
> *channelName,
> +  int state,
> +  int reason,
> +  void *opaque);
>  
>  /**
>   * VIR_DOMAIN_EVENT_CALLBACK:
> @@ -4006,6 +4046,7 @@ typedef enum {
>  VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION = 20, /* 
> virConnectDomainEventMigrationIterationCallback */
>  VIR_DOMAIN_EVENT_ID_JOB_COMPLETED = 21,  /* 
> virConnectDomainEventJobCompletedCallback */
>  VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED = 22, /* 
> virConnectDomainEventDeviceRemovalFailedCallback */
> +VIR_DOMAIN_EVENT_ID_CHANNEL_LIFECYCLE = 23, /* 
> virConnectDomainEventChannelLifecycleCallback */
>  
>  # ifdef VIR_ENUM_SENTINELS
>  VIR_DOMAIN_EVENT_ID_LAST

[...]

> diff --git a/src/qemu/qemu_

Re: [libvirt] [Qemu-devel] FOSDEM 2017: Call For Proposals -- Virtualization & IaaS DevRoom

[Kashyap Chamarthy]

Gentle reminder: CFP submission dedline is in one week (18-NOV-2016)

On Wed, Oct 19, 2016 at 05:23:19PM +0200, Kashyap Chamarthy wrote:
> ===
> 
> The call for proposals is now open for the Virtualization & IaaS devroom
> at the upcoming FOSDEM 2017, to be hosted on February 4, 2017.
> 
> This year will mark FOSDEM’s 17th anniversary as one of the
> longest-running free and open source software developer events,
> attracting thousands of developers and users from all over the world.
> FOSDEM will be held once again in Brussels, Belgium, on February 4 & 5,
> 2017.
> 
> ---
> Important Dates
> ---
> 
> Submission deadline: 18 November 2016
> Acceptance notifications: 04 December 2016
> Final schedule announcement: 11 December 2016
> Devroom: 04 February 2017 (one day)
> 
> -
> About the Devroom
> -
> 
> The Virtualization & IaaS devroom will feature session topics such as
> open source hypervisors and virtual machine managers such as Xen
> Project, KVM, QEMU, bhyve, and VirtualBox, and
> Infrastructure-as-a-Service projects such as Apache CloudStack,
> OpenStack, oVirt, OpenNebula, and Ganeti.
> 
> This devroom will host presentations that focus on topics of shared
> interest, such as KVM; libvirt; shared storage; virtualized networking;
> cloud security; clustering and high availability; interfacing with
> multiple hypervisors; hyperconverged deployments; and scaling across
> hundreds or thousands of servers.
> 
> Presentations in this devroom will be aimed at developers working on
> these platforms who are looking to collaborate and improve shared
> infrastructure or solve common problems. We seek topics that encourage
> dialog between projects and continued work post-FOSDEM.
> 
> 
> Submit Your Proposal
> 
> 
> All submissions must be made via the Pentabarf event planning site.
> 
> https://penta.fosdem.org/submission/FOSDEM17
> 
> If you have not used Pentabarf before, you will need to create an
> account.  If you submitted proposals for FOSDEM in previous years, you
> can use your existing account.
> 
> After creating the account, select Create Event to start the submission
> process. Make sure to select Virtualisation and IaaS devroom from the
> Track list. Please fill out all the required fields, and provide a
> meaningful abstract and description of your proposed session.
> 
> -
> Submission Guidelines
> -
> 
> - We expect more proposals than we can possibly accept, so it is vitally
>   important that you submit your proposal on or before the deadline.
>   Late submissions are unlikely to be considered.
> 
> - All presentation slots are 45 minutes, with 35 minutes planned for
>   presentations, and 10 minutes for Q&A.
> 
> - All presentations will be recorded and made available under Creative
>   Commons licenses. In the Submission notes field, please indicate that
>   you agree that your presentation will be licensed under the
>   CC-By-SA-4.0 or CC-By-4.0 license and that you agree to have your
>   presentation recorded. For example:
> 
> "If my presentation is accepted for FOSDEM, I hereby agree to
> license all recordings, slides, and other associated materials under
> the Creative Commons Attribution Share-Alike 4.0 International
> License.  Sincerely, ."
> 
> - In the Submission notes field, please also confirm that if your talk
>   is accepted, you will be able to attend FOSDEM and deliver your
>   presentation. We will not consider proposals from prospective speakers
>   who are unsure whether they will be able to secure funds for travel
>   and lodging to attend FOSDEM. (Sadly, we are not able to offer travel
>   funding for prospective speakers.)
> 
> ---
> Call for Volunteers
> ---
> 
> We are also looking for volunteers to help run the devroom. We need
> assistance watching time for the speakers, and helping with video for
> the devroom. Please contact Brian Proffitt (bkp at redhat.com), for more
> information.
> 
> --
> Questions?
> --
> 
> If you have any questions about this devroom, please send your questions
> to our devroom mailing list. 
> 
> iaas-virt-devr...@lists.fosdem.org
> 
> You can also subscribe to the list to receive updates about important
> dates, session announcements, and to connect with other attendees.
> 
> ===

-- 
/kashyap

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

Re: [libvirt] [PATCH] qemu: Emit domain events for all virtio-serial channels

[Daniel P. Berrange]

On Fri, Nov 11, 2016 at 11:13:19AM +0100, Peter Krempa wrote:
> On Mon, Nov 07, 2016 at 15:48:35 -0500, Matt Broadstone wrote:
> > Presently domain events are emitted for the "agent lifecycle" which
> > is a specialization on virtio-serial events specific to the channel
> > named "org.qemu.guest_agent.0". This patch adds a generic event for
> > "channel lifecycles", which emit state change events for all
> > attached channels.
> > ---
> >  daemon/remote.c | 42 +
> >  examples/object-events/event-test.c | 57 
> >  include/libvirt/libvirt-domain.h| 41 +
> >  src/conf/domain_event.c | 89 
> > +
> >  src/conf/domain_event.h | 12 +
> >  src/libvirt_private.syms|  2 +
> >  src/qemu/qemu_driver.c  |  5 +++
> >  src/qemu/qemu_process.c |  6 +++
> >  src/remote/remote_driver.c  | 32 +
> >  src/remote/remote_protocol.x| 17 ++-
> >  src/remote_protocol-structs |  7 +++
> >  tools/virsh-domain.c| 35 +++
> >  12 files changed, 344 insertions(+), 1 deletion(-)
> > 



> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 38c8414..b464412 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -4407,6 +4407,7 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
> >  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> >  virDomainChrDeviceState newstate;
> >  virObjectEventPtr event = NULL;
> > +virObjectEventPtr channelEvent = NULL;
> >  virDomainDeviceDef dev;
> >  qemuDomainObjPrivatePtr priv = vm->privateData;
> >  int rc;
> > @@ -4482,6 +4483,10 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
> >  qemuDomainEventQueue(driver, event);
> >  }
> >  
> > +channelEvent = virDomainEventChannelLifecycleNewFromObj(vm, 
> > dev.data.chr->target.name, newstate,
> > +
> > VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_CHANNEL);
> 
> I don't think we should emit this event for the guest agent channel. It
> has a separate one and the channel is reserved for libvirt anyways, so
> client applications should use the guest agent event for this.

I don't really agree with that. I don't see any compelling reason
to special case the guest agent channel - it just creates extra
work for client apps who want to see events for all channels. There
is no harm in having the guest agent trigger both events, the old
special case one, and the new general purpose one.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [PATCH] qemu: Emit domain events for all virtio-serial channels

[Peter Krempa]

On Fri, Nov 11, 2016 at 10:18:00 +, Daniel Berrange wrote:
> On Fri, Nov 11, 2016 at 11:13:19AM +0100, Peter Krempa wrote:
> > On Mon, Nov 07, 2016 at 15:48:35 -0500, Matt Broadstone wrote:
> > > Presently domain events are emitted for the "agent lifecycle" which
> > > is a specialization on virtio-serial events specific to the channel
> > > named "org.qemu.guest_agent.0". This patch adds a generic event for
> > > "channel lifecycles", which emit state change events for all
> > > attached channels.
> > > ---

[...]

> > I don't think we should emit this event for the guest agent channel. It
> > has a separate one and the channel is reserved for libvirt anyways, so
> > client applications should use the guest agent event for this.
> 
> I don't really agree with that. I don't see any compelling reason
> to special case the guest agent channel - it just creates extra
> work for client apps who want to see events for all channels. There
> is no harm in having the guest agent trigger both events, the old
> special case one, and the new general purpose one.

My reasoning is that the clients are not supposed to connect to the
channel where the guest agent communicates and thus should not receive
any events using this event type.

The guest agent event is designed so that it can report errors of the
guest agent (e.g. reporting invalid reply, timeout etc.) whereas this
does not apply to regular channels where libvirt does not care at all
what the state of the guest application is or which protocol they talk.

The way libvirt uses the guest agent channel makes it special from point
of view of other apps, since libvirt always hogs it and apps are
supposed to use libvirt api. This does not apply to generic channels.
(E.g, spice channels are special in the very same way).

Peter


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

Re: [libvirt] [PATCH] storage_backend_rbd: check the return value of rados_conf_set

[Peter Krempa]

On Fri, Nov 11, 2016 at 17:54:57 +0800, Chen Hanxiao wrote:
> At 2016-11-11 17:27:48, "Peter Krempa"  wrote:
> >On Fri, Nov 11, 2016 at 16:33:18 +0800, Chen Hanxiao wrote:
> >> From: Chen Hanxiao 
> >> 
> >> We forget to check the return value of rados_conf_set.
> >> 
> >> Signed-off-by: Chen Hanxiao 
> >> ---
> >>  src/storage/storage_backend_rbd.c | 21 ++---
> >>  1 file changed, 18 insertions(+), 3 deletions(-)

[...]

> >>  VIR_DEBUG("Setting RADOS option rados_mon_op_timeout to %s", 
> >> mon_op_timeout);
> >> -rados_conf_set(ptr->cluster, "rados_mon_op_timeout", mon_op_timeout);
> >> +if (rados_conf_set(ptr->cluster, "rados_mon_op_timeout", 
> >> mon_op_timeout) < 0) {
> >> +virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +   _("failed to set RADOS option: %s"),
> >> +   "rados_mon_op_timeout");
> >> +goto cleanup;
> >> +}
> >
> >Did you have any problems with this? The documentation mentions only one
> 
> In a test, I failed in rados_connect once.
> When I try again, it works.

That looks more like a rados or network problem.

> So I wonder maybe something wrong in rados_conf_set.

That is very unlikely.

> 
> >error code (ENOENT) if the given config option does not exist in
> >librados. This would point to us something doing wrong rather than a
> >random failure and we should address the primary cause.
> 
> Any hints?

I'd try to debug why rados_connect failed in the first place. The
options you added debug statements for don't look critical enough.

Said that, the patch itself is reasonable if you add error reporting for
all the config options we set.

Peter


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

Re: [libvirt] [PATCH] qemu: Emit domain events for all virtio-serial channels

[Daniel P. Berrange]

On Fri, Nov 11, 2016 at 11:27:07AM +0100, Peter Krempa wrote:
> On Fri, Nov 11, 2016 at 10:18:00 +, Daniel Berrange wrote:
> > On Fri, Nov 11, 2016 at 11:13:19AM +0100, Peter Krempa wrote:
> > > On Mon, Nov 07, 2016 at 15:48:35 -0500, Matt Broadstone wrote:
> > > > Presently domain events are emitted for the "agent lifecycle" which
> > > > is a specialization on virtio-serial events specific to the channel
> > > > named "org.qemu.guest_agent.0". This patch adds a generic event for
> > > > "channel lifecycles", which emit state change events for all
> > > > attached channels.
> > > > ---
> 
> [...]
> 
> > > I don't think we should emit this event for the guest agent channel. It
> > > has a separate one and the channel is reserved for libvirt anyways, so
> > > client applications should use the guest agent event for this.
> > 
> > I don't really agree with that. I don't see any compelling reason
> > to special case the guest agent channel - it just creates extra
> > work for client apps who want to see events for all channels. There
> > is no harm in having the guest agent trigger both events, the old
> > special case one, and the new general purpose one.
> 
> My reasoning is that the clients are not supposed to connect to the
> channel where the guest agent communicates and thus should not receive
> any events using this event type.
> 
> The guest agent event is designed so that it can report errors of the
> guest agent (e.g. reporting invalid reply, timeout etc.) whereas this
> does not apply to regular channels where libvirt does not care at all
> what the state of the guest application is or which protocol they talk.
> 
> The way libvirt uses the guest agent channel makes it special from point
> of view of other apps, since libvirt always hogs it and apps are
> supposed to use libvirt api. This does not apply to generic channels.
> (E.g, spice channels are special in the very same way).

These events are not solely useful for knowing when to connect to the
channel. An application which relies on the QEMU guest agent because
of the libvirt API calls it makes, may wish to know when the guest
agent is running, so it can avoid making libvirt APIs that use it.

eg, the app can disable the ability to quiesce filesystems until it
sees the event that the agent is running.

So I don't really see the guest agent as so special that we must not
allow these events to be seen by apps - it just creates extra work
for apps to force them to pointlessly register two event callbacks.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [PATCH] qemu: Emit domain events for all virtio-serial channels

[Peter Krempa]

On Fri, Nov 11, 2016 at 10:33:11 +, Daniel Berrange wrote:
> On Fri, Nov 11, 2016 at 11:27:07AM +0100, Peter Krempa wrote:
> > On Fri, Nov 11, 2016 at 10:18:00 +, Daniel Berrange wrote:
> > > On Fri, Nov 11, 2016 at 11:13:19AM +0100, Peter Krempa wrote:
> > > > On Mon, Nov 07, 2016 at 15:48:35 -0500, Matt Broadstone wrote:
> > > > > Presently domain events are emitted for the "agent lifecycle" which
> > > > > is a specialization on virtio-serial events specific to the channel
> > > > > named "org.qemu.guest_agent.0". This patch adds a generic event for
> > > > > "channel lifecycles", which emit state change events for all
> > > > > attached channels.
> > > > > ---
> > 
> > [...]

> These events are not solely useful for knowing when to connect to the
> channel. An application which relies on the QEMU guest agent because
> of the libvirt API calls it makes, may wish to know when the guest
> agent is running, so it can avoid making libvirt APIs that use it.
> 
> eg, the app can disable the ability to quiesce filesystems until it
> sees the event that the agent is running.
> 
> So I don't really see the guest agent as so special that we must not
> allow these events to be seen by apps - it just creates extra work
> for apps to force them to pointlessly register two event callbacks.

My point was that the guest agent may still be connected but
disfunctional from libvirt's point of view. The Agent lifecycle callback
should report this, while the channel callback should not.

Such apps will need two callbacks anyways.


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

Re: [libvirt] [PATCH] qemu: Emit domain events for all virtio-serial channels

[Daniel P. Berrange]

On Fri, Nov 11, 2016 at 11:41:42AM +0100, Peter Krempa wrote:
> On Fri, Nov 11, 2016 at 10:33:11 +, Daniel Berrange wrote:
> > On Fri, Nov 11, 2016 at 11:27:07AM +0100, Peter Krempa wrote:
> > > On Fri, Nov 11, 2016 at 10:18:00 +, Daniel Berrange wrote:
> > > > On Fri, Nov 11, 2016 at 11:13:19AM +0100, Peter Krempa wrote:
> > > > > On Mon, Nov 07, 2016 at 15:48:35 -0500, Matt Broadstone wrote:
> > > > > > Presently domain events are emitted for the "agent lifecycle" which
> > > > > > is a specialization on virtio-serial events specific to the channel
> > > > > > named "org.qemu.guest_agent.0". This patch adds a generic event for
> > > > > > "channel lifecycles", which emit state change events for all
> > > > > > attached channels.
> > > > > > ---
> > > 
> > > [...]
> 
> > These events are not solely useful for knowing when to connect to the
> > channel. An application which relies on the QEMU guest agent because
> > of the libvirt API calls it makes, may wish to know when the guest
> > agent is running, so it can avoid making libvirt APIs that use it.
> > 
> > eg, the app can disable the ability to quiesce filesystems until it
> > sees the event that the agent is running.
> > 
> > So I don't really see the guest agent as so special that we must not
> > allow these events to be seen by apps - it just creates extra work
> > for apps to force them to pointlessly register two event callbacks.
> 
> My point was that the guest agent may still be connected but
> disfunctional from libvirt's point of view. The Agent lifecycle callback
> should report this, while the channel callback should not.
> 
> Such apps will need two callbacks anyways.

It depends on what they wish to report. The QEMU guest agent is not the
only one that is special from an app POV too - the SPICE guest agent is
another one that apps cannot directly connect to, but which would still
report events via the channel lifecycle event. There may be others later
too, so I'm really not seeing a reason to special case the QEMU guest
agent in any way.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

[libvirt] [PATCH v3 0/2] List only online cpus for vcpupin/emulatorpin when vcpu placement static

[Nitesh Konkar]

Currently when the vcpu placement is static and
cpuset is not specified, CPU Affinity shows 0..
CPUMAX. This patchset will result in display of
only online CPU's under CPU Affinity on linux.

Nitesh Konkar (2):
  conf: List only online cpus for virsh vcpupin
  conf: List only online cpus for virsh emulatorpin

 src/conf/domain_conf.c | 10 ++
 src/qemu/qemu_driver.c |  9 +
 2 files changed, 19 insertions(+)

-- 
2.1.0

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

[libvirt] [PATCH v3 2/2] conf: List only online cpus for virsh emulatorpin

[Nitesh Konkar]

Currently when the vcpu placement is static
and cpuset is not specified, CPU Affinity
under virsh emulatorpin shows 0..CPUMAX. This
patchset will result in display of only
online CPU's under CPU Affinity on linux.

Signed-off-by: Nitesh Konkar 
---
 src/qemu/qemu_driver.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a82e58b..51efe28 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5409,6 +5409,11 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom,
 virBitmapPtr cpumask = NULL;
 virBitmapPtr bitmap = NULL;
 virBitmapPtr autoCpuset = NULL;
+virBitmapPtr staticCpuset = NULL;
+
+#ifdef __linux__
+staticCpuset = virHostCPUGetOnlineBitmap();
+#endif
 
 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
   VIR_DOMAIN_AFFECT_CONFIG, -1);
@@ -5435,6 +5440,9 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom,
 } else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO &&
autoCpuset) {
 cpumask = autoCpuset;
+} else if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC 
&&
+   staticCpuset) {
+cpumask = staticCpuset;
 } else {
 if (!(bitmap = virBitmapNew(hostcpus)))
 goto cleanup;
@@ -5449,6 +5457,7 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom,
  cleanup:
 virDomainObjEndAPI(&vm);
 virBitmapFree(bitmap);
+virBitmapFree(staticCpuset);
 return ret;
 }
 
-- 
2.1.0

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

[libvirt] [PATCH v3 1/2] conf: List only online cpus for virsh vcpupin

[Nitesh Konkar]

Currently when the vcpu placement is static
and cpuset is not specified, CPU Affinity
under virsh vcpupin shows 0..CPUMAX. This
patchset will result in display of only
online CPU's under CPU Affinity on linux.

Signed-off-by: Nitesh Konkar 
---
 src/conf/domain_conf.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 74efe8c..6f81903 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -55,6 +55,7 @@
 #include "virtpm.h"
 #include "virstring.h"
 #include "virnetdev.h"
+#include "virhostcpu.h"
 
 #define VIR_FROM_THIS VIR_FROM_DOMAIN
 
@@ -1543,8 +1544,13 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
 {
 int maxvcpus = virDomainDefGetVcpusMax(def);
 virBitmapPtr allcpumap = NULL;
+virBitmapPtr staticCpuset = NULL;
 size_t i;
 
+#ifdef __linux__
+staticCpuset = virHostCPUGetOnlineBitmap();
+#endif
+
 if (hostcpus < 0)
 return -1;
 
@@ -1562,6 +1568,9 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
 else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO &&
  autoCpuset)
 bitmap = autoCpuset;
+else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC &&
+ staticCpuset)
+bitmap = staticCpuset;
 else if (def->cpumask)
 bitmap = def->cpumask;
 else
@@ -1571,6 +1580,7 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
 }
 
 virBitmapFree(allcpumap);
+virBitmapFree(staticCpuset);
 return i;
 }
 
-- 
2.1.0

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

Re: [libvirt] [Qemu-devel] [PATCH v4 7/8] qmp: Support abstract classes on device-list-properties

[Jiri Denemark]

On Tue, Nov 08, 2016 at 08:29:41 +0100, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> > libvirt wants to know if the QEMU binary supports a given -cpu
> > option (normally CPU features that can be enabled/disabled using
> > "+foo"/"-foo").
> 
> The obvious way to check whether a specific CPU supports it is to
> introspect that CPU.
> 
> The obvious way to check whether all CPUs of interest support it
> (assuming that is a productive question) is to introspect all CPUs of
> interest.  Works regardless of whether the option is inherited, which is
> really an implementation detail.

As Eduardo said, libvirt wants to know whether it can use a given CPU
feature with current QEMU binary. In -cpu help, we can see a list of
models and features QEMU understands, but we need to get similar info
via QMP. CPU models are easy, but how do we get the list of CPU
features? If introspection is the way to get it, I'm fine with that,
just beware that we don't actually know the name of the CPU object
(Westmere-x86_64-cpu), we only know the name of the CPU model
(Westmere). So if the object name is needed, we need to query the
mapping from CPU model names to CPU object names.

Jirka

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

Re: [libvirt] [PATCH 1/2] qemuDomainAttachNetDevice: Don't overwrite error on rollback

[John Ferlan]

On 11/04/2016 08:28 AM, Michal Privoznik wrote:
> If there is an error hotpluging a net device (for whatever
> reason) a rollback operation is performed. However, whilst doing
> so various helper functions that are called report errors on
> their own. This results in the original error to be overwritten
> and thus misleading the user.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_hotplug.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index e06862c..34f2135 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -935,6 +935,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>virDomainNetDefPtr net)
>  {
>  qemuDomainObjPrivatePtr priv = vm->privateData;
> +virErrorPtr originalError = NULL;
>  char **tapfdName = NULL;
>  int *tapfd = NULL;
>  size_t tapfdSize = 0;
> @@ -1320,6 +1321,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>  if (!virDomainObjIsActive(vm))
>  goto cleanup;
>  
> +originalError = virSaveLastError();

Coverity has found that there's a couple places between here and the
virSetError/virFreeError below where we goto cleanup and thus would leak
originalError

John
>  if (vlan < 0) {
>  if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) {
>  char *netdev_name;
> @@ -1350,6 +1352,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>  ignore_value(qemuDomainObjExitMonitor(driver, vm));
>  VIR_FREE(hostnet_name);
>  }
> +virSetError(originalError);
> +virFreeError(originalError);
>  goto cleanup;
>  }
>  
> 

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

Re: [libvirt] [Qemu-devel] [PATCH v4 7/8] qmp: Support abstract classes on device-list-properties

[Eduardo Habkost]

On Fri, Nov 11, 2016 at 01:17:44PM +0100, Jiri Denemark wrote:
> On Tue, Nov 08, 2016 at 08:29:41 +0100, Markus Armbruster wrote:
> > Eduardo Habkost  writes:
> > > libvirt wants to know if the QEMU binary supports a given -cpu
> > > option (normally CPU features that can be enabled/disabled using
> > > "+foo"/"-foo").
> > 
> > The obvious way to check whether a specific CPU supports it is to
> > introspect that CPU.
> > 
> > The obvious way to check whether all CPUs of interest support it
> > (assuming that is a productive question) is to introspect all CPUs of
> > interest.  Works regardless of whether the option is inherited, which is
> > really an implementation detail.
> 
> As Eduardo said, libvirt wants to know whether it can use a given CPU
> feature with current QEMU binary. In -cpu help, we can see a list of
> models and features QEMU understands, but we need to get similar info
> via QMP. CPU models are easy, but how do we get the list of CPU
> features? If introspection is the way to get it, I'm fine with that,
> just beware that we don't actually know the name of the CPU object
> (Westmere-x86_64-cpu), we only know the name of the CPU model
> (Westmere). So if the object name is needed, we need to query the
> mapping from CPU model names to CPU object names.

I have patches to add the QOM type name to query-cpu-definitions.
I don't remember if I submitted them to qemu-devel already. I
will check.

-- 
Eduardo

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

Re: [libvirt] [PATCH] storage_backend_rbd: check the return value of rados_conf_set

[Chen Hanxiao]

At 2016-11-11 18:30:05, "Peter Krempa"  wrote:
>On Fri, Nov 11, 2016 at 17:54:57 +0800, Chen Hanxiao wrote:
>> At 2016-11-11 17:27:48, "Peter Krempa"  wrote:
>> >On Fri, Nov 11, 2016 at 16:33:18 +0800, Chen Hanxiao wrote:
>> >> From: Chen Hanxiao 
>> >> 
>> >> We forget to check the return value of rados_conf_set.
>> >> 
>> >> Signed-off-by: Chen Hanxiao 
>> >> ---
>> >>  src/storage/storage_backend_rbd.c | 21 ++---
>> >>  1 file changed, 18 insertions(+), 3 deletions(-)
>
>[...]
>
>> >>  VIR_DEBUG("Setting RADOS option rados_mon_op_timeout to %s", 
>> >> mon_op_timeout);
>> >> -rados_conf_set(ptr->cluster, "rados_mon_op_timeout", mon_op_timeout);
>> >> +if (rados_conf_set(ptr->cluster, "rados_mon_op_timeout", 
>> >> mon_op_timeout) < 0) {
>> >> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> >> +   _("failed to set RADOS option: %s"),
>> >> +   "rados_mon_op_timeout");
>> >> +goto cleanup;
>> >> +}
>> >
>> >Did you have any problems with this? The documentation mentions only one
>> 
>> In a test, I failed in rados_connect once.
>> When I try again, it works.
>
>That looks more like a rados or network problem.
>
>> So I wonder maybe something wrong in rados_conf_set.
>
>That is very unlikely.
>
>> 
>> >error code (ENOENT) if the given config option does not exist in
>> >librados. This would point to us something doing wrong rather than a
>> >random failure and we should address the primary cause.
>> 
>> Any hints?
>
>I'd try to debug why rados_connect failed in the first place. The
>options you added debug statements for don't look critical enough.
>
>Said that, the patch itself is reasonable if you add error reporting for
>all the config options we set.
>

Thanks for your advice.
I'll try to debug that.
v2 will come soon.

Regards,
- Chen

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

Re: [libvirt] [PATCH] qemu: Emit domain events for all virtio-serial channels

[Matt Broadstone]

On Fri, Nov 11, 2016 at 5:58 AM, Daniel P. Berrange  wrote:
> On Fri, Nov 11, 2016 at 11:41:42AM +0100, Peter Krempa wrote:
>> On Fri, Nov 11, 2016 at 10:33:11 +, Daniel Berrange wrote:
>> > On Fri, Nov 11, 2016 at 11:27:07AM +0100, Peter Krempa wrote:
>> > > On Fri, Nov 11, 2016 at 10:18:00 +, Daniel Berrange wrote:
>> > > > On Fri, Nov 11, 2016 at 11:13:19AM +0100, Peter Krempa wrote:
>> > > > > On Mon, Nov 07, 2016 at 15:48:35 -0500, Matt Broadstone wrote:
>> > > > > > Presently domain events are emitted for the "agent lifecycle" which
>> > > > > > is a specialization on virtio-serial events specific to the channel
>> > > > > > named "org.qemu.guest_agent.0". This patch adds a generic event for
>> > > > > > "channel lifecycles", which emit state change events for all
>> > > > > > attached channels.
>> > > > > > ---
>> > >
>> > > [...]
>>
>> > These events are not solely useful for knowing when to connect to the
>> > channel. An application which relies on the QEMU guest agent because
>> > of the libvirt API calls it makes, may wish to know when the guest
>> > agent is running, so it can avoid making libvirt APIs that use it.
>> >
>> > eg, the app can disable the ability to quiesce filesystems until it
>> > sees the event that the agent is running.
>> >
>> > So I don't really see the guest agent as so special that we must not
>> > allow these events to be seen by apps - it just creates extra work
>> > for apps to force them to pointlessly register two event callbacks.
>>
>> My point was that the guest agent may still be connected but
>> disfunctional from libvirt's point of view. The Agent lifecycle callback
>> should report this, while the channel callback should not.
>>
>> Such apps will need two callbacks anyways.
>
> It depends on what they wish to report. The QEMU guest agent is not the
> only one that is special from an app POV too - the SPICE guest agent is
> another one that apps cannot directly connect to, but which would still
> report events via the channel lifecycle event. There may be others later
> too, so I'm really not seeing a reason to special case the QEMU guest
> agent in any way.
>
> Regards,
> Daniel
> --
> |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org  -o- http://virt-manager.org :|
> |: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

I'm inclined to agree with Daniel here. Having a generic callback for
all paths here doesn't actually adversely affect anything, and is
arguably more "correct" in terms of reporting. Rather think of it this
way: this current behavior accurately models what I see when
monitoring the domstatus file.

Matt

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

[libvirt] [PATCH] vsh: Fix the incorrect environment variable prefix in error message

[Erik Skultety]

Unlike the other error messages in vshInitDebug, this one relied on a hardcoded
name of a variable instead of using the prefix of the tool calling the init
routine.

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

Signed-off-by: Erik Skultety 
---
Pushed under trivial rule.

 tools/vsh.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index f7ba070..3ba09dd 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3016,8 +3016,8 @@ vshInitDebug(vshControl *ctl)
 int debug;
 if (virStrToLong_i(debugEnv, NULL, 10, &debug) < 0 ||
 debug < VSH_ERR_DEBUG || debug > VSH_ERR_ERROR) {
-vshError(ctl, "%s",
- _("VSH_DEBUG not set with a valid numeric value"));
+vshError(ctl, _("%s_DEBUG not set with a valid numeric value"),
+ ctl->env_prefix);
 } else {
 ctl->debug = debug;
 }
-- 
2.5.5

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

Re: [libvirt] [PATCH] qemu: Emit domain events for all virtio-serial channels

[Matt Broadstone]

On Fri, Nov 11, 2016 at 5:13 AM, Peter Krempa  wrote:
> On Mon, Nov 07, 2016 at 15:48:35 -0500, Matt Broadstone wrote:
>> Presently domain events are emitted for the "agent lifecycle" which
>> is a specialization on virtio-serial events specific to the channel
>> named "org.qemu.guest_agent.0". This patch adds a generic event for
>> "channel lifecycles", which emit state change events for all
>> attached channels.
>> ---
>>  daemon/remote.c | 42 +
>>  examples/object-events/event-test.c | 57 
>>  include/libvirt/libvirt-domain.h| 41 +
>>  src/conf/domain_event.c | 89 
>> +
>>  src/conf/domain_event.h | 12 +
>>  src/libvirt_private.syms|  2 +
>>  src/qemu/qemu_driver.c  |  5 +++
>>  src/qemu/qemu_process.c |  6 +++
>>  src/remote/remote_driver.c  | 32 +
>>  src/remote/remote_protocol.x| 17 ++-
>>  src/remote_protocol-structs |  7 +++
>>  tools/virsh-domain.c| 35 +++
>>  12 files changed, 344 insertions(+), 1 deletion(-)
>>
>
> [...]
>
>> diff --git a/include/libvirt/libvirt-domain.h 
>> b/include/libvirt/libvirt-domain.h
>> index 5f50660..d720132 100644
>> --- a/include/libvirt/libvirt-domain.h
>> +++ b/include/libvirt/libvirt-domain.h
>> @@ -3965,6 +3965,46 @@ typedef void 
>> (*virConnectDomainEventAgentLifecycleCallback)(virConnectPtr conn,
>>  int reason,
>>  void *opaque);
>>
>> +typedef enum {
>> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_STATE_CONNECTED = 1, /* 
>> channel connected */
>> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_STATE_DISCONNECTED = 2, /* 
>> channel disconnected */
>> +
>> +# ifdef VIR_ENUM_SENTINELS
>> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_STATE_LAST
>> +# endif
>> +} virConnectDomainEventChannelLifecycleState;
>> +
>> +typedef enum {
>> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_UNKNOWN = 0, /* 
>> unknown state change reason */
>> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_DOMAIN_STARTED = 1, 
>> /* state changed due to domain start */
>
> This reason is not used at all and I don't think it even makes sense to
> be reported for channel events. With guest agent it might make sense and
> also other possible reasons as guest agent error.
>

This reason is, in fact, being used in qemu_process.c:1925
(qemuProcessRefreshChannelVirtioState). The state is actually aligned
with the virConnectDomainEventChannelLifecycleReason enum in order to
reuse the existing `agentReason` for emitting this event. I can
explicitly add a `channelReason` if that makes the code clearer/more
readable.

> With channels you mostly care about the connect and disconnect events.
> It might make sense to report client connect or disconnect in the future
> for certain backend types, but I don't think that qemu supports such
> report currently.

I agree that with chanels you generally care about `connected` and
`disconnected`, but the real problem here is that there is no framing
over the virtio-serial channel. As a result, it's important for
developers of guest agents to know when a channel connection is being
cycled or started for the first time. Remember that the reason for
this patch in the first place is that I am creating an agent myself.

>
>> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_CHANNEL = 2, /* 
>> channel state changed */
>> +
>> +# ifdef VIR_ENUM_SENTINELS
>> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_LAST
>> +# endif
>> +} virConnectDomainEventChannelLifecycleReason;
>> +
>> +/**
>> + * virConnectDomainEventChannelLifecycleCallback:
>> + * @conn: connection object
>> + * @dom: domain on which the event occurred
>> + * @channelName: the name of the channel on which the event occurred
>> + * @state: new state of the guest channel, one of 
>> virConnectDomainEventChannelLifecycleState
>> + * @reason: reason for state change; one of 
>> virConnectDomainEventChannelLifecycleReason
>> + * @opaque: application specified data
>> + *
>> + * This callback occurs when libvirt detects a change in the state of a 
>> guest
>> + * serial channel.
>
> You should note that the hypervisor needs to support channel state
> notifications, otherwise it won't work. (Qemu supporting this has been
> around for ages now, but we still support versions older than that. Also
> other hypervisors don't support this currently)
>

Agreed.

>> + *
>> + * The callback signature to use when registering for an event of type
>> + * VIR_DOMAIN_EVENT_ID_CHANNEL_LIFECYCLE with 
>> virConnectDomainEventRegisterAny()
>> + */
>> +typedef void (*virConnectDomainEventChannelLifecycleCallback)(virConnectPtr 
>> conn,
>> +  virDomainPtr 
>> dom,
>> +

Re: [libvirt] [systemd-devel] How to make udev not touch my device?

[Michal Sekletar]

On Mon, Nov 7, 2016 at 1:20 PM, Daniel P. Berrange  wrote:

> So if libvirt creates a private mount namespace for each QEMU and mounts
> a custom /dev there, this is invisible to udev, and thus udev won't/can't
> mess with permissions we set in our private /dev.
>
> For hotplug, the libvirt QEMU would do the same as the libvirt LXC driver
> currently does. It would fork and setns() into the QEMU mount namespace
> and run mknod()+chmod() there, before doing the rest of its normal hotplug
> logic. See lxcDomainAttachDeviceMknodHelper() for what LXC does.

We try to migrate people away from using mknod and messing with /dev/
from user-space. For example, we had to deal with non-trivial problems
wrt. mknod and Veritas storage stack in the past (most of these issues
remain unsolved to date). I don't like to hear that you plan to get
into /dev management business in libvirt too. I am judging based on
past experiences, nevertheless, I don't like this plan.

Also, managing separate mount namespace for each qemu process and
forking helper that joins the namespace to do some work seems quite
complex too.

Michal

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

[libvirt] [PATCH v2] storage_backend_rbd: check the return value of rados_conf_set

[Chen Hanxiao]

From: Chen Hanxiao 

We forget to check the return value of rados_conf_set.

Signed-off-by: Chen Hanxiao 
---
v2: add another missing return value check

 src/storage/storage_backend_rbd.c | 27 +++
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/src/storage/storage_backend_rbd.c 
b/src/storage/storage_backend_rbd.c
index 718c4d6..85ff99b 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -159,13 +159,27 @@ 
virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
  * Operations in librados will return -ETIMEDOUT when the timeout is 
reached.
  */
 VIR_DEBUG("Setting RADOS option client_mount_timeout to %s", 
client_mount_timeout);
-rados_conf_set(ptr->cluster, "client_mount_timeout", client_mount_timeout);
+if (rados_conf_set(ptr->cluster, "client_mount_timeout", 
client_mount_timeout) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("failed to set RADOS option: %s"),
+   "client_mount_timeout");
+goto cleanup;
 
 VIR_DEBUG("Setting RADOS option rados_mon_op_timeout to %s", 
mon_op_timeout);
-rados_conf_set(ptr->cluster, "rados_mon_op_timeout", mon_op_timeout);
+if (rados_conf_set(ptr->cluster, "rados_mon_op_timeout", mon_op_timeout) < 
0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("failed to set RADOS option: %s"),
+   "rados_mon_op_timeout");
+goto cleanup;
+}
 
 VIR_DEBUG("Setting RADOS option rados_osd_op_timeout to %s", 
osd_op_timeout);
-rados_conf_set(ptr->cluster, "rados_osd_op_timeout", osd_op_timeout);
+if (rados_conf_set(ptr->cluster, "rados_osd_op_timeout", osd_op_timeout) < 
0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("failed to set RADOS option: %s"),
+   "rados_osd_op_timeout");
+goto cleanup;
+}
 
 /*
  * Librbd supports creating RBD format 2 images. We no longer have to 
invoke
@@ -173,7 +187,12 @@ 
virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
  * This leaves us to simply use rbd_create() and use the default behavior 
of librbd
  */
 VIR_DEBUG("Setting RADOS option rbd_default_format to %s", 
rbd_default_format);
-rados_conf_set(ptr->cluster, "rbd_default_format", rbd_default_format);
+if (rados_conf_set(ptr->cluster, "rbd_default_format", rbd_default_format) 
< 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("failed to set RADOS option: %s"),
+   "rbd_default_format");
+goto cleanup;
+}
 
 ptr->starttime = time(0);
 if ((r = rados_connect(ptr->cluster)) < 0) {
-- 
1.8.3.1


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

Re: [libvirt] [systemd-devel] How to make udev not touch my device?

[Daniel P. Berrange]

On Fri, Nov 11, 2016 at 02:15:38PM +0100, Michal Sekletar wrote:
> On Mon, Nov 7, 2016 at 1:20 PM, Daniel P. Berrange  
> wrote:
> 
> > So if libvirt creates a private mount namespace for each QEMU and mounts
> > a custom /dev there, this is invisible to udev, and thus udev won't/can't
> > mess with permissions we set in our private /dev.
> >
> > For hotplug, the libvirt QEMU would do the same as the libvirt LXC driver
> > currently does. It would fork and setns() into the QEMU mount namespace
> > and run mknod()+chmod() there, before doing the rest of its normal hotplug
> > logic. See lxcDomainAttachDeviceMknodHelper() for what LXC does.
> 
> We try to migrate people away from using mknod and messing with /dev/
> from user-space. For example, we had to deal with non-trivial problems
> wrt. mknod and Veritas storage stack in the past (most of these issues

What kind of issues ? 

> remain unsolved to date). I don't like to hear that you plan to get
> into /dev management business in libvirt too. I am judging based on
> past experiences, nevertheless, I don't like this plan.

Libvirt is already doing this for its LXC driver, populating a private
/dev with only the devices permitted for the container in question.

> Also, managing separate mount namespace for each qemu process and
> forking helper that joins the namespace to do some work seems quite
> complex too.

Again, libvirt is already doing this for LXC so its not any great
burden.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [PATCH v3 python 2/2] don't overrun buffer when converting cpumap

[Peter Krempa]

On Thu, Nov 03, 2016 at 20:05:52 +0300, Konstantin Neumoin wrote:
> If we pass large(more than cpunum) cpu mask to any libvirt_virDomainPin*
> function, it could leads to crash. So we have to check tuple size in
> virPyCpumapConvert and ignore extra tuple members.
> 
> Signed-off-by: Konstantin Neumoin 
> ---
>  libvirt-utils.c | 13 +
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/libvirt-utils.c b/libvirt-utils.c
> index 09cc1c3..ac3606b 100644
> --- a/libvirt-utils.c
> +++ b/libvirt-utils.c
> @@ -623,7 +623,15 @@ virPyCpumapConvert(int cpunum,
>  return -1;
>  }
>  
> -for (i = 0; i < tuple_size; i++) {
> +/* Not presented elements of the tuple will be filled by zeros.
> + * Only first "cpunum" elements make sense, so the rest
> + * of the bits from the tuple will be ignored. */
> +for (i = 0; i < cpunum; i++) {
> +if (i >= tuple_size) {
> +VIR_UNUSE_CPU(*cpumapptr, i);

You don't really need to UNUSE the cpus since the array was cleared when
allocated. I'll tweak it and push the patch in a while.

Thanks for fixing the bug.

Peter


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

Re: [libvirt] [PATCH v3 1/2] conf: List only online cpus for virsh vcpupin

[Viktor Mihajlovski]

On 11.11.2016 12:26, Nitesh Konkar wrote:
> Currently when the vcpu placement is static
> and cpuset is not specified, CPU Affinity
> under virsh vcpupin shows 0..CPUMAX. This
> patchset will result in display of only
> online CPU's under CPU Affinity on linux.
> 
> Signed-off-by: Nitesh Konkar 
> ---
>  src/conf/domain_conf.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 74efe8c..6f81903 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -55,6 +55,7 @@
>  #include "virtpm.h"
>  #include "virstring.h"
>  #include "virnetdev.h"
> +#include "virhostcpu.h"
> 
>  #define VIR_FROM_THIS VIR_FROM_DOMAIN
> 
> @@ -1543,8 +1544,13 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
>  {
>  int maxvcpus = virDomainDefGetVcpusMax(def);
>  virBitmapPtr allcpumap = NULL;
> +virBitmapPtr staticCpuset = NULL;
>  size_t i;
> 
> +#ifdef __linux__
> +staticCpuset = virHostCPUGetOnlineBitmap();
> +#endif
> +
>  if (hostcpus < 0)
>  return -1;
> 
> @@ -1562,6 +1568,9 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
>  else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO &&
>   autoCpuset)
>  bitmap = autoCpuset;
> +else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC 
> &&
> + staticCpuset)
> +bitmap = staticCpuset;
I wonder whether you suggest this to reconcile the results of virsh
vcpupin and virsh vcpuinfo for a running domain? If so, this will not
work because then you would rather have to query the process' cpu
affinity, but this can obviously not be done here.
>  else if (def->cpumask)
>  bitmap = def->cpumask;
>  else
> @@ -1571,6 +1580,7 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
>  }
> 
>  virBitmapFree(allcpumap);
> +virBitmapFree(staticCpuset);
>  return i;
>  }
> 


-- 

Mit freundlichen Grüßen/Kind Regards
   Viktor Mihajlovski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
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] BlockCopy support rbd external destination file

[Peter Krempa]

On Wed, Nov 09, 2016 at 10:44:54 +0800, Liu Qing wrote:
> Currently qemuDomainBlockCopy only support local file. This patch make
> the rbd external destination file could be passed to qemu driver_mirror.
> If the external rbd file is not exist, qemuDomainBlockCopy will report
> an error.

Qemu will report an errro, sice libvirt can't detect currently that the
file does not exist.

> 
> Signed-off-by: Liu Qing 
> ---
>  src/qemu/qemu_driver.c | 82 
> +++---
>  1 file changed, 51 insertions(+), 31 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 38c8414..ee8db69 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c

[...]

> @@ -16675,42 +16676,50 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>  }
>  
>  /* Prepare the destination file.  */
> -/* XXX Allow non-file mirror destinations */
> -if (!virStorageSourceIsLocalStorage(mirror)) {
> +if (!virStorageSourceIsLocalStorage(mirror) &&
> +(mirror->type != VIR_STORAGE_TYPE_NETWORK ||
> + mirror->protocol != VIR_STORAGE_NET_PROTOCOL_RBD)) {
>  virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> _("non-file destination not supported yet"));
>  goto endjob;
>  }
> -if (stat(mirror->path, &st) < 0) {
> -if (errno != ENOENT) {
> -virReportSystemError(errno, _("unable to stat for disk %s: %s"),
> - disk->dst, mirror->path);
> -goto endjob;
> -} else if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT ||
> -   desttype == VIR_STORAGE_TYPE_BLOCK) {
> -virReportSystemError(errno,
> - _("missing destination file for disk %s: 
> %s"),
> - disk->dst, mirror->path);
> -goto endjob;
> -}
> -} else if (!S_ISBLK(st.st_mode)) {
> -if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("external destination file for disk %s already "
> - "exists and is not a block device: %s"),
> -   disk->dst, mirror->path);
> -goto endjob;
> -}
> -if (desttype == VIR_STORAGE_TYPE_BLOCK) {
> -virReportError(VIR_ERR_INVALID_ARG,
> -   _("blockdev flag requested for disk %s, but file "
> - "'%s' is not a block device"),
> -   disk->dst, mirror->path);
> -goto endjob;
> +if (virStorageSourceIsLocalStorage(mirror)) {
> +if (stat(mirror->path, &st) < 0) {
> +if (errno != ENOENT) {
> +virReportSystemError(errno, _("unable to stat for disk %s: 
> %s"),
> + disk->dst, mirror->path);
> +goto endjob;
> +} else if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT ||
> +   desttype == VIR_STORAGE_TYPE_BLOCK) {
> +virReportSystemError(errno,
> + _("missing destination file for disk 
> %s: %s"),
> + disk->dst, mirror->path);
> +goto endjob;
> +}
> +} else if (!S_ISBLK(st.st_mode)) {
> +if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("external destination file for disk %s 
> already "
> + "exists and is not a block device: %s"),
> +   disk->dst, mirror->path);
> +goto endjob;
> +}
> +if (desttype == VIR_STORAGE_TYPE_BLOCK) {
> +virReportError(VIR_ERR_INVALID_ARG,
> +   _("blockdev flag requested for disk %s, but 
> file "
> + "'%s' is not a block device"),
> +   disk->dst, mirror->path);
> +goto endjob;
> +}
>  }
> +} else if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
> +virReportError(VIR_ERR_INVALID_ARG,

You are missing a formating character in the error string. For strings
which don't need one you need to add a separate argument with a "%s"
formatting string.

Please make sure you run make syntax-check before posting since it
catches those:

prohibit_diagnostic_without_format
src/qemu/qemu_driver.c:16716:virReportError(VIR_ERR_INVALID_ARG,
src/qemu/qemu_driver.c-16717-   _("only external 
destination file is supported for "
src/qemu/qemu_driver.c-16718- "rbd protocol"));
maint.mk: found diagnostic without %
make: *** [cfg.mk:653: sc_prohibit_diagnostic_without_format] Error 1

> +   _

Re: [libvirt] [PATCH v3 1/2] conf: List only online cpus for virsh vcpupin

[Peter Krempa]

On Fri, Nov 11, 2016 at 14:51:31 +0100, Viktor Mihajlovski wrote:
> On 11.11.2016 12:26, Nitesh Konkar wrote:
> > Currently when the vcpu placement is static
> > and cpuset is not specified, CPU Affinity
> > under virsh vcpupin shows 0..CPUMAX. This
> > patchset will result in display of only
> > online CPU's under CPU Affinity on linux.
> > 
> > Signed-off-by: Nitesh Konkar 
> > ---
> >  src/conf/domain_conf.c | 10 ++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 74efe8c..6f81903 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c

[...]

> > @@ -1562,6 +1568,9 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
> >  else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO 
> > &&
> >   autoCpuset)
> >  bitmap = autoCpuset;
> > +else if (def->placement_mode == 
> > VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC &&
> > + staticCpuset)
> > +bitmap = staticCpuset;
> I wonder whether you suggest this to reconcile the results of virsh
> vcpupin and virsh vcpuinfo for a running domain? If so, this will not
> work because then you would rather have to query the process' cpu
> affinity, but this can obviously not be done here.

Indeed. This should be used instead of allcpumap in case we are going to
report all ones. Putting it here will report incorrect data in cases
when e.g the correct pinning is set in def->cpumask.


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

Re: [libvirt] [Qemu-devel] [PATCH v4 7/8] qmp: Support abstract classes on device-list-properties

[Markus Armbruster]

Jiri Denemark  writes:

> On Tue, Nov 08, 2016 at 08:29:41 +0100, Markus Armbruster wrote:
>> Eduardo Habkost  writes:
>> > libvirt wants to know if the QEMU binary supports a given -cpu
>> > option (normally CPU features that can be enabled/disabled using
>> > "+foo"/"-foo").
>> 
>> The obvious way to check whether a specific CPU supports it is to
>> introspect that CPU.
>> 
>> The obvious way to check whether all CPUs of interest support it
>> (assuming that is a productive question) is to introspect all CPUs of
>> interest.  Works regardless of whether the option is inherited, which is
>> really an implementation detail.
>
> As Eduardo said, libvirt wants to know whether it can use a given CPU
> feature with current QEMU binary. In -cpu help, we can see a list of
> models and features QEMU understands, but we need to get similar info
> via QMP. CPU models are easy, but how do we get the list of CPU
> features? If introspection is the way to get it, I'm fine with that,
> just beware that we don't actually know the name of the CPU object
> (Westmere-x86_64-cpu), we only know the name of the CPU model
> (Westmere).

Actually, you do:

{ "execute": "qom-list-types", "arguments": { "implements": "cpu" } }
{"return": [{"name": "Westmere-x86_64-cpu"}, ...]}

> So if the object name is needed, we need to query the
> mapping from CPU model names to CPU object names.

Hmm.  The problem is that some interfaces such as -cpu require a "CPU
model name", but introspection gives you the QOM type name.  The mapping
from "model name" to type name even depends on the target: it's CPUClass
method class_by_name().

Can't say I like that, but we got to play the hand we were dealt, and
that means we need to provide a way to introspect the mapping between
CPU model name and QOM type name.  Eduardo's plan to add the QOM type
name to query-cpu-definitions makes more sense to me now.

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

[libvirt] [PATCH 1/2] tools: use vshError rather than vshPrint on failure

[Erik Skultety]

There were a few places in our virsh* code where instead of calling vshError
on failure we called vshPrint.

Signed-off-by: Erik Skultety 
---
 tools/virsh-volume.c | 2 +-
 tools/vsh.c  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index e8cef39..8831f44 100644
--- a/tools/virsh-volume.c
+++ b/tools/virsh-volume.c
@@ -605,7 +605,7 @@ cmdVolClone(vshControl *ctl, const vshCmd *cmd)
 
 newxml = virshMakeCloneXML(origxml, name);
 if (!newxml) {
-vshPrint(ctl, "%s", _("Failed to allocate XML buffer"));
+vshError(ctl, "%s", _("Failed to allocate XML buffer"));
 goto cleanup;
 }
 
diff --git a/tools/vsh.c b/tools/vsh.c
index 3ba09dd..f69c6dd 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3269,7 +3269,7 @@ cmdEcho(vshControl *ctl, const vshCmd *cmd)
 if (xml) {
 virBufferEscapeString(&xmlbuf, "%s", arg);
 if (virBufferError(&xmlbuf)) {
-vshPrint(ctl, "%s", _("Failed to allocate XML buffer"));
+vshError(ctl, "%s", _("Failed to allocate XML buffer"));
 return false;
 }
 str = virBufferContentAndReset(&xmlbuf);
@@ -3286,7 +3286,7 @@ cmdEcho(vshControl *ctl, const vshCmd *cmd)
 }
 
 if (virBufferError(&buf)) {
-vshPrint(ctl, "%s", _("Failed to allocate XML buffer"));
+vshError(ctl, "%s", _("Failed to allocate XML buffer"));
 return false;
 }
 arg = virBufferContentAndReset(&buf);
-- 
2.5.5

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

Re: [libvirt] [PATCH] qemu: Emit domain events for all virtio-serial channels

[Peter Krempa]

On Fri, Nov 11, 2016 at 07:51:25 -0500, Matt Broadstone wrote:
> On Fri, Nov 11, 2016 at 5:13 AM, Peter Krempa  wrote:
> > On Mon, Nov 07, 2016 at 15:48:35 -0500, Matt Broadstone wrote:

[...]

> >> diff --git a/include/libvirt/libvirt-domain.h 
> >> b/include/libvirt/libvirt-domain.h
> >> index 5f50660..d720132 100644
> >> --- a/include/libvirt/libvirt-domain.h
> >> +++ b/include/libvirt/libvirt-domain.h
> >> @@ -3965,6 +3965,46 @@ typedef void 
> >> (*virConnectDomainEventAgentLifecycleCallback)(virConnectPtr conn,
> >>  int reason,
> >>  void *opaque);
> >>
> >> +typedef enum {
> >> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_STATE_CONNECTED = 1, /* 
> >> channel connected */
> >> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_STATE_DISCONNECTED = 2, /* 
> >> channel disconnected */
> >> +
> >> +# ifdef VIR_ENUM_SENTINELS
> >> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_STATE_LAST
> >> +# endif
> >> +} virConnectDomainEventChannelLifecycleState;
> >> +
> >> +typedef enum {
> >> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_UNKNOWN = 0, /* 
> >> unknown state change reason */
> >> +VIR_CONNECT_DOMAIN_EVENT_CHANNEL_LIFECYCLE_REASON_DOMAIN_STARTED = 1, 
> >> /* state changed due to domain start */
> >
> > This reason is not used at all and I don't think it even makes sense to
> > be reported for channel events. With guest agent it might make sense and
> > also other possible reasons as guest agent error.
> >
> 
> This reason is, in fact, being used in qemu_process.c:1925
> (qemuProcessRefreshChannelVirtioState). The state is actually aligned
> with the virConnectDomainEventChannelLifecycleReason enum in order to
> reuse the existing `agentReason` for emitting this event. I can
> explicitly add a `channelReason` if that makes the code clearer/more
> readable.

It uses the AGENT_LIFECYCLE value not this declared here. Thus that
exact value is not used. If you want to rely on the fact that they stay
in sync, please note it in a comment. I've used 'git grep' and thus did
not see it.

> > With channels you mostly care about the connect and disconnect events.
> > It might make sense to report client connect or disconnect in the future
> > for certain backend types, but I don't think that qemu supports such
> > report currently.
> 
> I agree that with chanels you generally care about `connected` and
> `disconnected`, but the real problem here is that there is no framing
> over the virtio-serial channel. As a result, it's important for
> developers of guest agents to know when a channel connection is being
> cycled or started for the first time. Remember that the reason for
> this patch in the first place is that I am creating an agent myself.

Okay, fair enough. Note that, for channels, this basically will always
state that the VM has booted and the channel is disconnected as there is
no other option. With the regular guest agent with qemu that did not
support the channel state notification we always connected to the agent.

Peter


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

[libvirt] [PATCH 0/2] Replace some leftover vshPrint calls with vshPrintExtra

[Erik Skultety]

Erik Skultety (2):
  tools: use vshError rather than vshPrint on failure
  tools: Replace vshPrint with vshPrintExtra on places we forgot about

 tools/virsh-domain.c| 73 +
 tools/virsh-edit.c  |  4 +--
 tools/virsh-interface.c | 28 +--
 tools/virsh-network.c   | 37 +
 tools/virsh-nwfilter.c  |  6 ++--
 tools/virsh-pool.c  | 21 +++---
 tools/virsh-snapshot.c  | 20 +++---
 tools/virsh-volume.c| 14 +-
 tools/virt-admin.c  |  4 +--
 tools/vsh.c |  4 +--
 10 files changed, 107 insertions(+), 104 deletions(-)

-- 
2.5.5

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

[libvirt] [PATCH 2/2] tools: Replace vshPrint with vshPrintExtra on places we forgot about

[Erik Skultety]

Although there already was an effort (b620bdee) to replace vshPrint occurrences
with vshPrintExtra due to '--quiet' flag, there were still some leftovers. So
this patch fixes them, hopefully for good.

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

Signed-off-by: Erik Skultety 
---
 tools/virsh-domain.c| 73 +
 tools/virsh-edit.c  |  4 +--
 tools/virsh-interface.c | 28 +--
 tools/virsh-network.c   | 37 +
 tools/virsh-nwfilter.c  |  6 ++--
 tools/virsh-pool.c  | 21 +++---
 tools/virsh-snapshot.c  | 20 +++---
 tools/virsh-volume.c| 12 
 tools/virt-admin.c  |  4 +--
 9 files changed, 104 insertions(+), 101 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 184f64d..91308e8 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2080,9 +2080,9 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
 
 if (!blocking) {
 if (active)
-vshPrint(ctl, "%s", _("Active Block Commit started"));
+vshPrintExtra(ctl, "%s", _("Active Block Commit started"));
 else
-vshPrint(ctl, "%s", _("Block Commit started"));
+vshPrintExtra(ctl, "%s", _("Block Commit started"));
 
 ret = true;
 goto cleanup;
@@ -2094,12 +2094,12 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 
 case VIR_DOMAIN_BLOCK_JOB_CANCELED:
-vshPrint(ctl, "\n%s", _("Commit aborted"));
+vshPrintExtra(ctl, "\n%s", _("Commit aborted"));
 goto cleanup;
 break;
 
 case VIR_DOMAIN_BLOCK_JOB_FAILED:
-vshPrint(ctl, "\n%s", _("Commit failed"));
+vshPrintExtra(ctl, "\n%s", _("Commit failed"));
 goto cleanup;
 break;
 
@@ -2116,19 +2116,20 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 }
 
-vshPrint(ctl, "\n%s", _("Successfully pivoted"));
+vshPrintExtra(ctl, "\n%s", _("Successfully pivoted"));
 } else if (finish) {
 if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) {
 vshError(ctl, _("failed to finish job for disk %s"), path);
 goto cleanup;
 }
 
-vshPrint(ctl, "\n%s", _("Commit complete, overlay image kept"));
+vshPrintExtra(ctl, "\n%s", _("Commit complete, overlay "
+ "image kept"));
 } else {
-vshPrint(ctl, "\n%s", _("Now in synchronized phase"));
+vshPrintExtra(ctl, "\n%s", _("Now in synchronized phase"));
 }
 } else {
-vshPrint(ctl, "\n%s", _("Commit complete"));
+vshPrintExtra(ctl, "\n%s", _("Commit complete"));
 }
 
 ret = true;
@@ -2392,7 +2393,7 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
 }
 
 if (!blocking) {
-vshPrint(ctl, "%s", _("Block Copy started"));
+vshPrintExtra(ctl, "%s", _("Block Copy started"));
 ret = true;
 goto cleanup;
 }
@@ -2403,12 +2404,12 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 
 case VIR_DOMAIN_BLOCK_JOB_CANCELED:
-vshPrint(ctl, "\n%s", _("Copy aborted"));
+vshPrintExtra(ctl, "\n%s", _("Copy aborted"));
 goto cleanup;
 break;
 
 case VIR_DOMAIN_BLOCK_JOB_FAILED:
-vshPrint(ctl, "\n%s", _("Copy failed"));
+vshPrintExtra(ctl, "\n%s", _("Copy failed"));
 goto cleanup;
 break;
 
@@ -2424,16 +2425,16 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 }
 
-vshPrint(ctl, "\n%s", _("Successfully pivoted"));
+vshPrintExtra(ctl, "\n%s", _("Successfully pivoted"));
 } else if (finish) {
 if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) {
 vshError(ctl, _("failed to finish job for disk %s"), path);
 goto cleanup;
 }
 
-vshPrint(ctl, "\n%s", _("Successfully copied"));
+vshPrintExtra(ctl, "\n%s", _("Successfully copied"));
 } else {
-vshPrint(ctl, "\n%s", _("Now in mirroring phase"));
+vshPrintExtra(ctl, "\n%s", _("Now in mirroring phase"));
 }
 
 ret = true;
@@ -2571,7 +2572,7 @@ virshBlockJobInfo(vshControl *ctl,
 
 if (rc == 0) {
 if (!raw)
-vshPrint(ctl, _("No current block job for %s"), path);
+vshPrintExtra(ctl, _("No current block job for %s"), path);
 ret = true;
 goto cleanup;
 }
@@ -2802,7 +2803,7 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd)
 }
 
 if (!blocking) {
-vshPrint(ctl, "%s", _("Block Pull started"));
+vshPrintExtra(ctl, "%s", _("Block Pull started"));
 ret = true;
 goto cleanup;
 }
@@ -2813,18 +2814,18 @@ cmdBlockP

Re: [libvirt] [PATCH 0/2] Replace some leftover vshPrint calls with vshPrintExtra

[Michal Privoznik]

On 11.11.2016 15:51, Erik Skultety wrote:
> Erik Skultety (2):
>   tools: use vshError rather than vshPrint on failure
>   tools: Replace vshPrint with vshPrintExtra on places we forgot about
> 
>  tools/virsh-domain.c| 73 
> +
>  tools/virsh-edit.c  |  4 +--
>  tools/virsh-interface.c | 28 +--
>  tools/virsh-network.c   | 37 +
>  tools/virsh-nwfilter.c  |  6 ++--
>  tools/virsh-pool.c  | 21 +++---
>  tools/virsh-snapshot.c  | 20 +++---
>  tools/virsh-volume.c| 14 +-
>  tools/virt-admin.c  |  4 +--
>  tools/vsh.c |  4 +--
>  10 files changed, 107 insertions(+), 104 deletions(-)
> 

ACK

Michal

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

Re: [libvirt] [PATCH 1/2] tools: use vshError rather than vshPrint on failure

[Michal Privoznik]

On 11.11.2016 15:51, Erik Skultety wrote:
> There were a few places in our virsh* code where instead of calling vshError
> on failure we called vshPrint.
> 
> Signed-off-by: Erik Skultety 
> ---
>  tools/virsh-volume.c | 2 +-
>  tools/vsh.c  | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
> index e8cef39..8831f44 100644
> --- a/tools/virsh-volume.c
> +++ b/tools/virsh-volume.c
> @@ -605,7 +605,7 @@ cmdVolClone(vshControl *ctl, const vshCmd *cmd)
>  
>  newxml = virshMakeCloneXML(origxml, name);
>  if (!newxml) {
> -vshPrint(ctl, "%s", _("Failed to allocate XML buffer"));
> +vshError(ctl, "%s", _("Failed to allocate XML buffer"));
>  goto cleanup;
>  }
>  
> diff --git a/tools/vsh.c b/tools/vsh.c
> index 3ba09dd..f69c6dd 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -3269,7 +3269,7 @@ cmdEcho(vshControl *ctl, const vshCmd *cmd)
>  if (xml) {
>  virBufferEscapeString(&xmlbuf, "%s", arg);
>  if (virBufferError(&xmlbuf)) {
> -vshPrint(ctl, "%s", _("Failed to allocate XML buffer"));
> +vshError(ctl, "%s", _("Failed to allocate XML buffer"));
>  return false;
>  }
>  str = virBufferContentAndReset(&xmlbuf);
> @@ -3286,7 +3286,7 @@ cmdEcho(vshControl *ctl, const vshCmd *cmd)
>  }
>  
>  if (virBufferError(&buf)) {
> -vshPrint(ctl, "%s", _("Failed to allocate XML buffer"));
> +vshError(ctl, "%s", _("Failed to allocate XML buffer"));
>  return false;
>  }
>  arg = virBufferContentAndReset(&buf);
> 

There are couple of other locations:
cmdBlockCopy
cmdUndefine

But this is better too.

Michal

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

[libvirt] [PATCH 4/4] admin: Use the newly introduced close callback handling helpers

[Erik Skultety]

Use the newly introduced close callback helpers to make the code look just a
bit cleaner and more importantly, to fix the following memleak regarding a
dangling virAdmConnect object reference caused by assigning NULL to the close
callback data once the catch-disconnect routine used the callback followed
by a comparison of NULL to the originally defined close callback (which at that
moment had already been NULL'd by remoteAdminClientCloseFunc) in
virAdmConnectCloseCallbackUnregister.

717 (88 direct, 629 indirect) bytes in 1 blocks are definitely lost record
 110 of 141
at 0x4C2A988: calloc (vg_replace_malloc.c:711)
by 0x530696F: virAllocVar (viralloc.c:560)
by 0x53689E6: virObjectNew (virobject.c:193)
by 0x5368B5E: virObjectLockableNew (virobject.c:219)
by 0x4E3E7EE: virAdmConnectNew (datatypes.c:900)
by 0x4E398BB: virAdmConnectOpen (libvirt-admin.c:220)
by 0x10D3E3: vshAdmConnect (virt-admin.c:161)
by 0x10D624: vshAdmReconnect (virt-admin.c:215)
by 0x10DB0A: cmdConnect (virt-admin.c:353)
by 0x11288F: vshCommandRun (vsh.c:1313)
by 0x10FDB6: main (virt-admin.c:1439)

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

Signed-off-by: Erik Skultety 
---
 src/admin/admin_remote.c |  6 +-
 src/datatypes.c  |  5 +
 src/libvirt-admin.c  | 18 +-
 3 files changed, 3 insertions(+), 26 deletions(-)

diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c
index 10a3b18..f37ff5e 100644
--- a/src/admin/admin_remote.c
+++ b/src/admin/admin_remote.c
@@ -144,11 +144,7 @@ remoteAdminClientCloseFunc(virNetClientPtr client 
ATTRIBUTE_UNUSED,
 VIR_DEBUG("Triggering connection close callback %p reason=%d, 
opaque=%p",
   cbdata->callback, reason, cbdata->opaque);
 cbdata->callback(cbdata->conn, reason, cbdata->opaque);
-
-if (cbdata->freeCallback)
-cbdata->freeCallback(cbdata->opaque);
-cbdata->callback = NULL;
-cbdata->freeCallback = NULL;
+virAdmConnectCloseCallbackDataReset(cbdata);
 }
 virObjectUnlock(cbdata);
 }
diff --git a/src/datatypes.c b/src/datatypes.c
index 24e4f77..c8a46b0 100644
--- a/src/datatypes.c
+++ b/src/datatypes.c
@@ -928,10 +928,7 @@ virAdmConnectCloseCallbackDataDispose(void *obj)
 virAdmConnectCloseCallbackDataPtr cb_data = obj;
 
 virObjectLock(cb_data);
-
-if (cb_data->freeCallback)
-cb_data->freeCallback(cb_data->opaque);
-
+virAdmConnectCloseCallbackDataReset(cb_data);
 virObjectUnlock(cb_data);
 }
 
diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c
index 1b5fd44..8877e49 100644
--- a/src/libvirt-admin.c
+++ b/src/libvirt-admin.c
@@ -513,29 +513,13 @@ int virAdmConnectUnregisterCloseCallback(virAdmConnectPtr 
conn,
 virResetLastError();
 
 virCheckAdmConnectReturn(conn, -1);
-
-virObjectLock(conn->closeCallback);
-
 virCheckNonNullArgGoto(cb, error);
 
-if (conn->closeCallback->callback != cb) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-   _("A different callback was requested"));
+if (virAdmConnectCloseCallbackDataUnregister(conn->closeCallback, cb) < 0)
 goto error;
-}
-
-conn->closeCallback->callback = NULL;
-if (conn->closeCallback->freeCallback)
-conn->closeCallback->freeCallback(conn->closeCallback->opaque);
-conn->closeCallback->freeCallback = NULL;
-
-virObjectUnlock(conn->closeCallback);
-virObjectUnref(conn);
 
 return 0;
-
  error:
-virObjectUnlock(conn->closeCallback);
 virDispatchError(NULL);
 return -1;
 }
-- 
2.5.5

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

[libvirt] [PATCH 1/4] vsh: Drop conditional error reporting in vshErrorHandler

[Erik Skultety]

First, since commit 834c5720 the error reporting within the vshErrorHandler
doesn't work because there was a lot of renaming going on (dull mechanical
renaming without much thinking about it, yep - shame on me) and so the original
env variable VIRSH_DEBUG got renamed to VSH_DEBUG which we don't support nor
document anywhere. Second, by specifying this env variable, the last libvirt
error gets reported twice despite the fact we say the error reporting should be
deferred until the command finishes, and last but not least the vintage code's
logic is a bit 'odd', since the error would get reported iff the env variable
is set, even if the value should be equal to our DEFAULT value in which case it
doesn't make sense that we behave differently when an env variable is set to
some value and when there's no env variable at all but we use the same value
automatically as default.

Signed-off-by: Erik Skultety 
---
 tools/vsh.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index 3ba09dd..07ceb7b 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -246,8 +246,6 @@ vshErrorHandler(void *opaque ATTRIBUTE_UNUSED, virErrorPtr 
error)
 {
 virFreeError(last_error);
 last_error = virSaveLastError();
-if (virGetEnvAllowSUID("VSH_DEBUG") != NULL)
-virDefaultErrorFunc(error);
 }
 
 /* Store a libvirt error that is from a helper API that doesn't raise errors
-- 
2.5.5

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

[libvirt] [PATCH 0/4] admin: Fix a leaking conn reference and other improvements

[Erik Skultety]

This series fixes a memory leak in a dangling conn object reference caused by
assigning NULL to conn->closeCallback object by the RPC catch-disconnect routine
followed by a comparison of equality of conn->closeCallback->callback and the
client-registered callback reference.
Before fixing the issue itself, there are some new helpers introduced following
the same idea as the helpers introduced by series
https://www.redhat.com/archives/libvir-list/2016-February/msg00849.html


Erik Skultety (4):
  vsh: Drop conditional error reporting in vshErrorHandler
  admin: Remove unnecessary @conn object locking
  datatypes: Introduce some admin-related close callback handling
helpers
  admin: Use the newly introduced close callback handling helpers

 po/POTFILES.in |  1 +
 src/admin/admin_remote.c   |  6 +---
 src/datatypes.c| 67 +++---
 src/datatypes.h|  8 +
 src/libvirt-admin.c| 24 +--
 src/libvirt_admin_private.syms |  3 ++
 tools/vsh.c|  2 --
 7 files changed, 77 insertions(+), 34 deletions(-)

-- 
2.5.5

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

[libvirt] [PATCH 3/4] datatypes: Introduce some admin-related close callback handling helpers

[Erik Skultety]

Well, there were three different spots where closeCallback->freeCallback was
called, not looking the same --> potential for bugs - and there indeed is a bug
with refcounting of the @conn object. So this patch partially follows the path
set by commit 24dbb69f by introducing some close callback helpers both to
replace all the spots where we call clean the close callback data with a
dedicated function and to be able to fix the refcounting bug causing a memleak.

Signed-off-by: Erik Skultety 
---
 po/POTFILES.in |  1 +
 src/datatypes.c| 62 ++
 src/datatypes.h|  8 ++
 src/libvirt_admin_private.syms |  3 ++
 4 files changed, 74 insertions(+)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 1469240..5a37083 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -47,6 +47,7 @@ src/cpu/cpu_arm.c
 src/cpu/cpu_map.c
 src/cpu/cpu_ppc64.c
 src/cpu/cpu_x86.c
+src/datatypes.c
 src/driver.c
 src/esx/esx_driver.c
 src/esx/esx_network_driver.c
diff --git a/src/datatypes.c b/src/datatypes.c
index ff0c46f..24e4f77 100644
--- a/src/datatypes.c
+++ b/src/datatypes.c
@@ -935,6 +935,68 @@ virAdmConnectCloseCallbackDataDispose(void *obj)
 virObjectUnlock(cb_data);
 }
 
+void
+virAdmConnectCloseCallbackDataReset(virAdmConnectCloseCallbackDataPtr cbdata)
+{
+if (cbdata->freeCallback)
+cbdata->freeCallback(cbdata->opaque);
+
+virObjectUnref(cbdata->conn);
+cbdata->conn = NULL;
+cbdata->freeCallback = NULL;
+cbdata->callback = NULL;
+cbdata->opaque = NULL;
+}
+
+int
+virAdmConnectCloseCallbackDataUnregister(virAdmConnectCloseCallbackDataPtr 
cbdata,
+ virAdmConnectCloseFunc cb)
+{
+int ret = -1;
+
+virObjectLock(cbdata);
+if (cbdata->callback != cb) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("A different callback was requested"));
+goto cleanup;
+}
+
+virAdmConnectCloseCallbackDataReset(cbdata);
+ret = 0;
+ cleanup:
+virObjectUnlock(cbdata);
+return ret;
+}
+
+int
+virAdmConnectCloseCallbackDataRegister(virAdmConnectCloseCallbackDataPtr 
cbdata,
+   virAdmConnectPtr conn,
+   virAdmConnectCloseFunc cb,
+   void *opaque,
+   virFreeCallback freecb)
+{
+int ret = -1;
+
+virObjectLock(cbdata);
+
+if (cbdata->callback) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("A close callback is already registered"));
+goto cleanup;
+}
+
+virObjectRef(conn);
+cbdata->conn = conn;
+cbdata->callback = cb;
+cbdata->opaque = opaque;
+cbdata->freeCallback = freecb;
+
+ret = 0;
+ cleanup:
+virObjectUnlock(conn->closeCallback);
+return ret;
+}
+
 virAdmServerPtr
 virAdmGetServer(virAdmConnectPtr conn, const char *name)
 {
diff --git a/src/datatypes.h b/src/datatypes.h
index 2b6adb4..9a5fbbc 100644
--- a/src/datatypes.h
+++ b/src/datatypes.h
@@ -722,5 +722,13 @@ void 
virConnectCloseCallbackDataCall(virConnectCloseCallbackDataPtr close,
  int reason);
 virConnectCloseFunc
 virConnectCloseCallbackDataGetCallback(virConnectCloseCallbackDataPtr close);
+void virAdmConnectCloseCallbackDataReset(virAdmConnectCloseCallbackDataPtr 
cbdata);
+int virAdmConnectCloseCallbackDataRegister(virAdmConnectCloseCallbackDataPtr 
cbdata,
+   virAdmConnectPtr conn,
+   virAdmConnectCloseFunc cb,
+   void *opaque,
+   virFreeCallback freecb);
+int virAdmConnectCloseCallbackDataUnregister(virAdmConnectCloseCallbackDataPtr 
cbdata,
+ virAdmConnectCloseFunc cb);
 
 #endif /* __VIR_DATATYPES_H__ */
diff --git a/src/libvirt_admin_private.syms b/src/libvirt_admin_private.syms
index 8c173ab..191e3f2 100644
--- a/src/libvirt_admin_private.syms
+++ b/src/libvirt_admin_private.syms
@@ -29,6 +29,9 @@ xdr_admin_server_set_threadpool_parameters_args;
 # datatypes.h
 virAdmClientClass;
 virAdmConnectClass;
+virAdmConnectCloseCallbackDataRegister;
+virAdmConnectCloseCallbackDataReset;
+virAdmConnectCloseCallbackDataUnregister;
 virAdmGetServer;
 virAdmServerClass;
 
-- 
2.5.5

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

[libvirt] [PATCH 2/4] admin: Remove unnecessary @conn object locking

[Erik Skultety]

The only place we change the @conn object is actually virAdmConnectOpen
routine, thus at the moment we don't really need to lock it, given the fact that
what we're trying to do here is to change the closeCallback object which is a
lockable object itself, so that should be enough to avoid races.

Signed-off-by: Erik Skultety 
---
 src/libvirt-admin.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c
index 88eef54..1b5fd44 100644
--- a/src/libvirt-admin.c
+++ b/src/libvirt-admin.c
@@ -465,7 +465,6 @@ int virAdmConnectRegisterCloseCallback(virAdmConnectPtr 
conn,
 
 virObjectRef(conn);
 
-virObjectLock(conn);
 virObjectLock(conn->closeCallback);
 
 virCheckNonNullArgGoto(cb, error);
@@ -482,13 +481,11 @@ int virAdmConnectRegisterCloseCallback(virAdmConnectPtr 
conn,
 conn->closeCallback->freeCallback = freecb;
 
 virObjectUnlock(conn->closeCallback);
-virObjectUnlock(conn);
 
 return 0;
 
  error:
 virObjectUnlock(conn->closeCallback);
-virObjectUnlock(conn);
 virDispatchError(NULL);
 virObjectUnref(conn);
 return -1;
@@ -517,7 +514,6 @@ int virAdmConnectUnregisterCloseCallback(virAdmConnectPtr 
conn,
 
 virCheckAdmConnectReturn(conn, -1);
 
-virObjectLock(conn);
 virObjectLock(conn->closeCallback);
 
 virCheckNonNullArgGoto(cb, error);
@@ -534,14 +530,12 @@ int virAdmConnectUnregisterCloseCallback(virAdmConnectPtr 
conn,
 conn->closeCallback->freeCallback = NULL;
 
 virObjectUnlock(conn->closeCallback);
-virObjectUnlock(conn);
 virObjectUnref(conn);
 
 return 0;
 
  error:
 virObjectUnlock(conn->closeCallback);
-virObjectUnlock(conn);
 virDispatchError(NULL);
 return -1;
 }
-- 
2.5.5

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

[libvirt] [PATCH] qemuDomainAttachNetDevice: Avoid @originalError leak

[Michal Privoznik]

Coverity identified that this variable might be leaked. And it's
right. If an error occurred and we have to roll back the control
jumps to try_remove label where we save the current error (see
0e82fa4c34 for more info). However, inside the code a jump onto
other label is possible thus leaking the error object.

Signed-off-by: Michal Privoznik 
---

Best viewed with 'git show --ignore-space-change'

 src/qemu/qemu_hotplug.c | 42 +-
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1bc2706..0dcbee6 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1326,32 +1326,32 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 if (vlan < 0) {
 if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) {
 char *netdev_name;
-if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0)
-goto cleanup;
-qemuDomainObjEnterMonitor(driver, vm);
-if (charDevPlugged &&
-qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0)
-VIR_WARN("Failed to remove associated chardev %s", 
charDevAlias);
-if (netdevPlugged &&
-qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0)
-VIR_WARN("Failed to remove network backend for netdev %s",
- netdev_name);
-ignore_value(qemuDomainObjExitMonitor(driver, vm));
-VIR_FREE(netdev_name);
+if (virAsprintf(&netdev_name, "host%s", net->info.alias) >= 0) {
+qemuDomainObjEnterMonitor(driver, vm);
+if (charDevPlugged &&
+qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0)
+VIR_WARN("Failed to remove associated chardev %s", 
charDevAlias);
+if (netdevPlugged &&
+qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0)
+VIR_WARN("Failed to remove network backend for netdev %s",
+ netdev_name);
+ignore_value(qemuDomainObjExitMonitor(driver, vm));
+VIR_FREE(netdev_name);
+}
 } else {
 VIR_WARN("Unable to remove network backend");
 }
 } else {
 char *hostnet_name;
-if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0)
-goto cleanup;
-qemuDomainObjEnterMonitor(driver, vm);
-if (hostPlugged &&
-qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0)
-VIR_WARN("Failed to remove network backend for vlan %d, net %s",
- vlan, hostnet_name);
-ignore_value(qemuDomainObjExitMonitor(driver, vm));
-VIR_FREE(hostnet_name);
+if (virAsprintf(&hostnet_name, "host%s", net->info.alias) >= 0) {
+qemuDomainObjEnterMonitor(driver, vm);
+if (hostPlugged &&
+qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 
0)
+VIR_WARN("Failed to remove network backend for vlan %d, net 
%s",
+ vlan, hostnet_name);
+ignore_value(qemuDomainObjExitMonitor(driver, vm));
+VIR_FREE(hostnet_name);
+}
 }
 virSetError(originalError);
 virFreeError(originalError);
-- 
2.8.4

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

Re: [libvirt] [PATCH 0/4] admin: Fix a leaking conn reference and other improvements

[Michal Privoznik]

On 11.11.2016 16:32, Erik Skultety wrote:
> This series fixes a memory leak in a dangling conn object reference caused by
> assigning NULL to conn->closeCallback object by the RPC catch-disconnect 
> routine
> followed by a comparison of equality of conn->closeCallback->callback and the
> client-registered callback reference.
> Before fixing the issue itself, there are some new helpers introduced 
> following
> the same idea as the helpers introduced by series
> https://www.redhat.com/archives/libvir-list/2016-February/msg00849.html
> 
> 
> Erik Skultety (4):
>   vsh: Drop conditional error reporting in vshErrorHandler
>   admin: Remove unnecessary @conn object locking
>   datatypes: Introduce some admin-related close callback handling
> helpers
>   admin: Use the newly introduced close callback handling helpers
> 
>  po/POTFILES.in |  1 +
>  src/admin/admin_remote.c   |  6 +---
>  src/datatypes.c| 67 
> +++---
>  src/datatypes.h|  8 +
>  src/libvirt-admin.c| 24 +--
>  src/libvirt_admin_private.syms |  3 ++
>  tools/vsh.c|  2 --
>  7 files changed, 77 insertions(+), 34 deletions(-)
> 

ACK series

Michal

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

Re: [libvirt] [PATCH 2/4] admin: Remove unnecessary @conn object locking

[Michal Privoznik]

On 11.11.2016 16:32, Erik Skultety wrote:
> The only place we change the @conn object is actually virAdmConnectOpen
> routine, thus at the moment we don't really need to lock it, given the fact 
> that
> what we're trying to do here is to change the closeCallback object which is a
> lockable object itself, so that should be enough to avoid races.
> 
> Signed-off-by: Erik Skultety 
> ---
>  src/libvirt-admin.c | 6 --
>  1 file changed, 6 deletions(-)
> 
> diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c
> index 88eef54..1b5fd44 100644
> --- a/src/libvirt-admin.c
> +++ b/src/libvirt-admin.c
> @@ -465,7 +465,6 @@ int virAdmConnectRegisterCloseCallback(virAdmConnectPtr 
> conn,
>  
>  virObjectRef(conn);
>  
> -virObjectLock(conn);
>  virObjectLock(conn->closeCallback);
>  
>  virCheckNonNullArgGoto(cb, error);
> @@ -482,13 +481,11 @@ int virAdmConnectRegisterCloseCallback(virAdmConnectPtr 
> conn,
>  conn->closeCallback->freeCallback = freecb;
>  
>  virObjectUnlock(conn->closeCallback);
> -virObjectUnlock(conn);
>  

Well, sometimes we are locking objects just for the sake of lock
ordering. But looking into the code I cannot find any other place where
they would rely on some specific lock ordering.

Michal

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

Re: [libvirt] [systemd-devel] How to make udev not touch my device?

[Michal Sekletar]

On Fri, Nov 11, 2016 at 2:20 PM, Daniel P. Berrange  wrote:

> What kind of issues ?

General problem with manually created device nodes is that udev and
systemd do not know about them. Device units do not exist for these
device nodes. Hence these device units can not be a dependency of some
other unit. Typical example is manually created device node referenced
from /etc/fstab. Then corresponding mount unit is bound to a device
that never shows up and hence it always fails to mount even tough
device node is there.

Michal

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

Re: [libvirt] [PATCH v6 10/17] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed

[Laine Stump]

On 11/10/2016 09:10 AM, Andrea Bolognani wrote:

On Wed, 2016-11-09 at 15:23 -0500, Laine Stump wrote:

+} else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE &&
+   addrs->buses[0].model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) 
{
+model = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
  
Mh, what about the case where we want to add a pci-bridge

but the machine has a pci-root instead of a pcie-root?
Shouldn't that case be handled as well?
  
It is handled - we'll want to add a pci-bridge if flags &

VIR_PCI_TYPE_PCI_DEVICE is true (i.e. the first clause). In that case,
since pci-root has the flag VIR_PCI_CONNECT_TYPE_PCI_BRIDGE set, the for
loop will set neetDMIToPCIBridge = false, so we will end up adding just
the one pci-bridge that we need.

Sorry, I was not clear enough.

What if the function is called like

   virDomainPCIAddressSetGrow(addrs, addr,
  VIR_PCI_CONNECT_TYPE_PCI_BRIDGE)

because *the caller* is going through a virDomainDef and,
after finding a pci-bridge in it, wants to make sure the
address set can actually fit it? The case when

   flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE

can clearly happen, as we're handling it above, but we error
out unless the guest has a pcie-root?


Okay, two different scenarios, each for either pci-root or pcie-root:

1) pci-bridge requested with no address supplied

   a) pci-root - if there is an open slot it will be assigned to the 
pci-bridge.
   If there isn't an open slot, then there is no chance for success 
anyway,

   so the correct error message will be correct:

  a PCI slot is neede to connect a PCI coontller model='pci-bridge'
  but none is available, and it cannot be automatically added.

 (because the only way to add a new open slot is to add a pci-bridge,
 and that's what we're already trying and failing to do).

   b) pcie-root - if there is an open slot, then we would never get 
into ...Grow(),
   if there isn't then we need to add a dmi-to-pci-bridge (because 
we can
   only plug into a pci-bridge or a dmi-to-pci-bridge, and we've 
already established
   that we can't directly plug in a pci-bridge. So again, behavior 
is correct


2) pci-bridge was requested with user-supplied address that is on a 
nonexistent bus, and that bus number is >1 over the current last bus 
(i.e. there is a gap in the controller indexes). This would happen if, 
for example, someone put this in their xml:





(if libvirt was setting the index, it would have set the index for the 
pci-bridge to '1').


a) pci-root - okay, here's where what you're saying would apply - 
we need to add extra pci-bridges

to fill in the "empty space" in the list of controllers.

b) pcie-root - actually we have the same issue in this case.

Personally I think that it's nonsensical for a user to do that, and 
trying to support "empty regions" in the PCI address space by 
auto-adding pci-bridges is pointless busywork, but we have supported it 
in the past, so I guess we need to now. Sigh.




That's the bit that I
can't quite wrap my head around.


Because it's a silly thing to support. But we already do, so we have to 
continue :-(





+} else if (flags & (VIR_PCI_CONNECT_TYPE_PCIE_DEVICE |
+VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)) {
+model = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
+} else {
+int existingContModel = 
virDomainPCIControllerConnectTypeToModel(flags);
+
+if (existingContModel >= 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("a PCI slot is needed to connect a PCI controller 
"
+ "model='%s', but none is available, and it "
+ "cannot be automatically added"),
+   
virDomainControllerModelPCITypeToString(existingContModel));
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Cannot automatically add a new PCI bus for a "
+ "device with connect flags %.2x"), flags);
+}
  
So we error out for dmi-to-pci-bridge and

pci{,e}-expander-bus... Shouldn't we be able to plug either
into into pcie-root or pci-root?
  
Exactly. And since those buses already exist from the start, and can't

be added later, there wouldn't ever be a situation where we needed to
automatically grow the bus structure due to one of those devices and
growing could actually do anything (i.e. you can't add a pcie-root or
pci-root).

I'm clearly missing something here :(

Don't we (potentially) grow the address set using this
function every time we reserve an address for an endpoint
device or a controller?


No. It is only called when we look for an open slot that matches the 
connectFlags of the device/controller, and don't find any open slot with 
compatible flags.




  Including when we're going through
a virDomainDef,

Re: [libvirt] [PATCH v6 11/17] [RFC] qemu: if pci-bridge is in PCIe config w/o dmi-to-pci-bridge, add it

[Laine Stump]

On 11/09/2016 11:26 AM, Andrea Bolognani wrote:

On Mon, 2016-11-07 at 14:50 -0500, Laine Stump wrote:

I'm undecided if it is worthwhile to add this...
  
Up until now it has been legal to have something like this in the xml:
  



  
(for example, see the existing test case

"usb-controller-default-q35").  This is handled in
qemuDomainPCIAddressSetCreate() when it's adding in controllers to
fill holes in the indexes.)
  
Since we have always added the following to every config:
  

  
there has always been a proper place for the unaddressed pci-bridge to

plug in.
  
A upcoming commit will eliminate the forced adding of a

dmi-to-pci-bridge to *every* Q35 domain config, though. This would
mean the above-mentioned test case (and one other) would begin to
fail. There are two possible solutions to this problem:
  
1) change the test cases, and made the above construct an error (note

that in the future it will work just fine to not list *any* pci
controller, and they will all be auto-added as needed).
  
or
  
2) This patch, which specifically looks for the case where we have a

pci-bridge (but no dmi-to-pci-bridge) in a PCIe domain config and
auto-adds the necessary dmi-to-pci-bridge. This can only work in the
case that the pci-bridge has been given an index such that there is an
unused index with a lower value for the dmi-to-pci-bridge to occupy.
  
This seems like a kind of odd corner case that nobody should need to

do in the future anyway. On the other hand, I already wrote the code
and it works, so I might as well send it and see what opinions (if
any) it generates.

I say let's forget about this.


Forgotten. I may need to replace this patch with one that changes the 
test case. Alternately I could change the test case in the same patch 
that breaks it. Yeah, I'll probably do the latter.




I'm fairly certain nobody is using this ludicrous pattern
outside of our test suite, and even if they did their guests
configurations would already have had the dmi-to-pci-bridge
added long ago, so no risks of breakage there.

So, unless somebody has very compelling arguments for
maintaining such a "feature", consider this a

NACK

--
Andrea Bolognani / Red Hat / Virtualization



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

Re: [libvirt] [systemd-devel] How to make udev not touch my device?

[Daniel P. Berrange]

On Fri, Nov 11, 2016 at 05:01:40PM +0100, Michal Sekletar wrote:
> On Fri, Nov 11, 2016 at 2:20 PM, Daniel P. Berrange  
> wrote:
> 
> > What kind of issues ?
> 
> General problem with manually created device nodes is that udev and
> systemd do not know about them. Device units do not exist for these
> device nodes. Hence these device units can not be a dependency of some
> other unit. Typical example is manually created device node referenced
> from /etc/fstab. Then corresponding mount unit is bound to a device
> that never shows up and hence it always fails to mount even tough
> device node is there.

Ok, that sounds irrelevant to libvirt's usage wrt QEMU, so I don't
see any problem for us here.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [PATCH] qemuDomainAttachNetDevice: Avoid @originalError leak

[John Ferlan]

On 11/11/2016 10:33 AM, Michal Privoznik wrote:
> Coverity identified that this variable might be leaked. And it's
> right. If an error occurred and we have to roll back the control
> jumps to try_remove label where we save the current error (see
> 0e82fa4c34 for more info). However, inside the code a jump onto
> other label is possible thus leaking the error object.
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> Best viewed with 'git show --ignore-space-change'
> 
>  src/qemu/qemu_hotplug.c | 42 +-
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 

Yuck ;-)  All because qemuBuildHostNetStr adds the "id=" string into the
middle somewhere...

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 1bc2706..0dcbee6 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1326,32 +1326,32 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>  if (vlan < 0) {
>  if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) {
>  char *netdev_name;
> -if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0)
> -goto cleanup;
> -qemuDomainObjEnterMonitor(driver, vm);
> -if (charDevPlugged &&
> -qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0)
> -VIR_WARN("Failed to remove associated chardev %s", 
> charDevAlias);
> -if (netdevPlugged &&
> -qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0)
> -VIR_WARN("Failed to remove network backend for netdev %s",
> - netdev_name);
> -ignore_value(qemuDomainObjExitMonitor(driver, vm));
> -VIR_FREE(netdev_name);
> +if (virAsprintf(&netdev_name, "host%s", net->info.alias) >= 0) {
> +qemuDomainObjEnterMonitor(driver, vm);
> +if (charDevPlugged &&
> +qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0)
> +VIR_WARN("Failed to remove associated chardev %s", 
> charDevAlias);
> +if (netdevPlugged &&
> +qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0)
> +VIR_WARN("Failed to remove network backend for netdev 
> %s",
> + netdev_name);
> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +VIR_FREE(netdev_name);
> +}
>  } else {
>  VIR_WARN("Unable to remove network backend");
>  }
>  } else {
>  char *hostnet_name;
> -if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0)
> -goto cleanup;
> -qemuDomainObjEnterMonitor(driver, vm);
> -if (hostPlugged &&
> -qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0)
> -VIR_WARN("Failed to remove network backend for vlan %d, net %s",
> - vlan, hostnet_name);
> -ignore_value(qemuDomainObjExitMonitor(driver, vm));
> -VIR_FREE(hostnet_name);
> +if (virAsprintf(&hostnet_name, "host%s", net->info.alias) >= 0) {

In qemuBuildHostNetStr when vlan >= 0, the printing of the id is gated
on net->info.alias - that doesn't happen here, but then again it's
printing into "name="..

So looking at qemuDomainRemoveNetDevice - I see a slightly different
removal algorithm...


John
> +qemuDomainObjEnterMonitor(driver, vm);
> +if (hostPlugged &&
> +qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) 
> < 0)
> +VIR_WARN("Failed to remove network backend for vlan %d, net 
> %s",
> + vlan, hostnet_name);
> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +VIR_FREE(hostnet_name);
> +}
>  }
>  virSetError(originalError);
>  virFreeError(originalError);
> 

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

[libvirt] FYI: virnettlssession broken by gnutls 3.5.6

[Daniel P. Berrange]

The gnutls 3.5.6 release changed the format is used to report the
x509 DN field, breaking the virnettlssession test suite. This
will also break anyone using the x509 whitelist config parameter
in libvirtd.conf.

I've discussed it here:

  https://lists.gnupg.org/pipermail/gnutls-devel/2016-November/008224.html

and its likely that a new gnutls release will revert the changes,
thus fixing libvirt.

I've temporaraily downgraded gnutls on the CI machines to avoid us
hitting the bug in 3.5.6 (via excludepkgs in /etc/yum.repo.d/rawhide.repo)

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [PATCH] qemuDomainAttachNetDevice: Avoid @originalError leak

[John Ferlan]

On 11/11/2016 11:37 AM, John Ferlan wrote:
> 
> 
> On 11/11/2016 10:33 AM, Michal Privoznik wrote:
>> Coverity identified that this variable might be leaked. And it's
>> right. If an error occurred and we have to roll back the control
>> jumps to try_remove label where we save the current error (see
>> 0e82fa4c34 for more info). However, inside the code a jump onto
>> other label is possible thus leaking the error object.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>
>> Best viewed with 'git show --ignore-space-change'
>>
>>  src/qemu/qemu_hotplug.c | 42 +-
>>  1 file changed, 21 insertions(+), 21 deletions(-)
>>
> 
> Yuck ;-)  All because qemuBuildHostNetStr adds the "id=" string into the
> middle somewhere...
> 
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 1bc2706..0dcbee6 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -1326,32 +1326,32 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>>  if (vlan < 0) {
>>  if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) {
>>  char *netdev_name;
>> -if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0)
>> -goto cleanup;
>> -qemuDomainObjEnterMonitor(driver, vm);
>> -if (charDevPlugged &&
>> -qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0)
>> -VIR_WARN("Failed to remove associated chardev %s", 
>> charDevAlias);
>> -if (netdevPlugged &&
>> -qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0)
>> -VIR_WARN("Failed to remove network backend for netdev %s",
>> - netdev_name);
>> -ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> -VIR_FREE(netdev_name);
>> +if (virAsprintf(&netdev_name, "host%s", net->info.alias) >= 0) {
>> +qemuDomainObjEnterMonitor(driver, vm);
>> +if (charDevPlugged &&
>> +qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0)
>> +VIR_WARN("Failed to remove associated chardev %s", 
>> charDevAlias);
>> +if (netdevPlugged &&
>> +qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0)
>> +VIR_WARN("Failed to remove network backend for netdev 
>> %s",
>> + netdev_name);
>> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> +VIR_FREE(netdev_name);
>> +}
>>  } else {
>>  VIR_WARN("Unable to remove network backend");
>>  }
>>  } else {
>>  char *hostnet_name;
>> -if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0)
>> -goto cleanup;
>> -qemuDomainObjEnterMonitor(driver, vm);
>> -if (hostPlugged &&
>> -qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0)
>> -VIR_WARN("Failed to remove network backend for vlan %d, net %s",
>> - vlan, hostnet_name);
>> -ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> -VIR_FREE(hostnet_name);
>> +if (virAsprintf(&hostnet_name, "host%s", net->info.alias) >= 0) {
> 
> In qemuBuildHostNetStr when vlan >= 0, the printing of the id is gated
> on net->info.alias - that doesn't happen here, but then again it's
> printing into "name="..
> 
> So looking at qemuDomainRemoveNetDevice - I see a slightly different
> removal algorithm...
> 
> 
> John

Oy... Let me revisit... To the degree that this patch fixes the issue I
noted from Coverity - that's true - so ACK for that.

John

The error path logic itself is still a bit confusing as the attach and
remove logic is based on QEMU_CAPS_NETDEV while this error path is based
on vlan < 0 logic and isn't necessarily self documenting ~/~

>> +qemuDomainObjEnterMonitor(driver, vm);
>> +if (hostPlugged &&
>> +qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) 
>> < 0)
>> +VIR_WARN("Failed to remove network backend for vlan %d, net 
>> %s",
>> + vlan, hostnet_name);
>> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> +VIR_FREE(hostnet_name);
>> +}
>>  }
>>  virSetError(originalError);
>>  virFreeError(originalError);
>>
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

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

[libvirt] [PATCH] docs: change "Learn" to "Documentation"

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---

I know that this is not a big issue, but I don't like the "Learn".  As I've
mentioned in my review [1] IMO "Documentation" is better.

[1] 

 docs/page.xsl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/page.xsl b/docs/page.xsl
index fcb5191..837060b 100644
--- a/docs/page.xsl
+++ b/docs/page.xsl
@@ -106,7 +106,7 @@
 
   Download
   Contribute
-  Learn
+  Documentation
 
   
   
-- 
2.10.2

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

Re: [libvirt] [PATCH] docs: change "Learn" to "Documentation"

[Daniel P. Berrange]

On Fri, Nov 11, 2016 at 06:43:18PM +0100, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
> 
> I know that this is not a big issue, but I don't like the "Learn".  As I've
> mentioned in my review [1] IMO "Documentation" is better.
> 
> [1] 
> 
>  docs/page.xsl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/page.xsl b/docs/page.xsl
> index fcb5191..837060b 100644
> --- a/docs/page.xsl
> +++ b/docs/page.xsl
> @@ -106,7 +106,7 @@
>  
>Download
>Contribute
> -  Learn
> +  Documentation
>  
>
>

FYI, this will make the nav bar too wide when viewed on mobile browsers,
at least on my android phone. So if we made this change it would have
to be "Docs" not "Documentation". Personally I still favour Learn.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [systemd-devel] How to make udev not touch my device?

[Lennart Poettering]

On Mon, 07.11.16 09:17, Daniel P. Berrange (berra...@redhat.com) wrote:

> On Fri, Nov 04, 2016 at 08:47:34AM +0100, Michal Privoznik wrote:
> > Hey udev developers,
> > 
> > I'm a libvirt developer and I've been facing an interesting issue
> > recently. Libvirt is a library for managing virtual machines and as such
> > allows basically any device to be exposed to a virtual machine. For
> > instance, a virtual machine can use /dev/sdX as its own disk. Because of
> > security reasons we allow users to configure their VMs to run under
> > different UID/GID and also SELinux context. That means that whenever a
> > VM is being started up, libvirtd (our daemon we have) relabels all the
> > necessary paths that QEMU process (representing VM) can touch.
> > However, I'm facing an issue that I don't know how to fix. In some cases
> > QEMU can close & reopen a block device. However, closing a block device
> > triggers an event and hence if there is a rule that sets a security
> > label on a device the QEMU process is unable to reopen the device again.
> > 
> > My question is, whet we can do to prevent udev from mangling with our
> > security labels that we've set on the devices?
> > 
> > One of the ideas our lead developer had was for libvirt to set some kind
> > of udev label on devices managed by libvirt (when setting up security
> > labels) and then whenever udev sees such labelled device it won't touch
> > it at all (this could be achieved by a rule perhaps?). Later, when
> > domain is shutting down libvirt removes that label. But I don't think
> > setting an arbitrary label on devices is supported, is it?
> 
> Having thought about this over the weekend, I'm strongly inclined to
> just take udev out of the equation by starting a new mount namespace
> for each QEMU we launch and setting up a custom /dev containing just
> the devices we need. This will be both a security improvement and
> avoid the udev races, with no complex code required in libvirt and
> will work for libvirt all the way back to RHEL6

I think this would be a pretty nice solution, indeed!

Lennart

-- 
Lennart Poettering, Red Hat

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

Re: [libvirt] [systemd-devel] How to make udev not touch my device?

[Lennart Poettering]

On Fri, 11.11.16 14:15, Michal Sekletar (msekl...@redhat.com) wrote:

> On Mon, Nov 7, 2016 at 1:20 PM, Daniel P. Berrange  
> wrote:
> 
> > So if libvirt creates a private mount namespace for each QEMU and mounts
> > a custom /dev there, this is invisible to udev, and thus udev won't/can't
> > mess with permissions we set in our private /dev.
> >
> > For hotplug, the libvirt QEMU would do the same as the libvirt LXC driver
> > currently does. It would fork and setns() into the QEMU mount namespace
> > and run mknod()+chmod() there, before doing the rest of its normal hotplug
> > logic. See lxcDomainAttachDeviceMknodHelper() for what LXC does.
> 
> We try to migrate people away from using mknod and messing with /dev/
> from user-space. For example, we had to deal with non-trivial problems
> wrt. mknod and Veritas storage stack in the past (most of these issues
> remain unsolved to date). I don't like to hear that you plan to get
> into /dev management business in libvirt too. I am judging based on
> past experiences, nevertheless, I don't like this plan.

Well, I'd say: if people create their own /dev, they are welcome to do
in it whatever they want. They should just stay away from the host's
/dev however, and not interfere with udev's own managing of that.

Lennart

-- 
Lennart Poettering, Red Hat

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

Re: [libvirt] [PATCH v3 01/10] Cleanup switch statements on the hostdev subsystem type

[John Ferlan]

On 11/08/2016 01:26 PM, Eric Farman wrote:
> As was suggested in an earlier review comment[1], we can
> catch some additional code points by cleaning up how we use the
> hostdev subsystem type in some switch statements.
> 
> [1] End of 
> https://www.redhat.com/archives/libvir-list/2016-September/msg00399.html
> 
> Signed-off-by: Eric Farman 
> ---
>  src/conf/domain_conf.c   | 11 +--
>  src/qemu/qemu_cgroup.c   | 11 +++
>  src/security/security_apparmor.c |  4 ++--
>  src/security/security_selinux.c  |  8 
>  4 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a233c0c..043f0e2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -12992,7 +12992,7 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr 
> xmlopt,
>  }
>  
>  if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
> -switch (def->source.subsys.type) {
> +switch ((virDomainHostdevSubsysType) def->source.subsys.type) {
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
>  if (def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
>  def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
> @@ -13014,6 +13014,11 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr 
> xmlopt,
>  if (virXPathBoolean("boolean(./shareable)", ctxt))
>  def->shareable = true;
>  break;
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
> +break;
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> +goto error;

Since virDomainHostdevDefParseXMLSubsys will validate that 'type' is
valid w/ a call to virDomainHostdevSubsysTypeFromString, that means for
this code there's no way the subsys.type could be invalid. So no need to
jump to error.  FWIW: Even if you did jump to error, there should be an
error message.

In fact both USB and LAST "fall through" for now.

Later though when SCSI_HOST is added, that's when you can do some more
address validation since it'll be a new type. As opposed to the lack of
validation for USB since USB existed before we added having 
validation (which is in the post parse callback IIRC).

I will adjust this before pushing (shortly)...

ACK -

John

> +break;
>  }
>  }
>  
> @@ -13880,7 +13885,7 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a,
>  if (a->source.subsys.type != b->source.subsys.type)
>  return 0;
>  
> -switch (a->source.subsys.type) {
> +switch ((virDomainHostdevSubsysType) a->source.subsys.type) {
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
>  return virDomainHostdevMatchSubsysPCI(a, b);
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
> @@ -13894,6 +13899,8 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a,
>  return virDomainHostdevMatchSubsysSCSIiSCSI(a, b);
>  else
>  return virDomainHostdevMatchSubsysSCSIHost(a, b);
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> +return 0;
>  }
>  return 0;
>  }
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 4489c64..1443f7e 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -301,7 +301,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
>  
>  if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
>  
> -switch (dev->source.subsys.type) {
> +switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
>  if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
>  int rv;
> @@ -376,7 +376,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
>  break;
>  }
>  
> -default:
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>  break;
>  }
>  }
> @@ -410,7 +410,7 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
>  
>  if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
>  
> -switch (dev->source.subsys.type) {
> +switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
>  if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
>  int rv;
> @@ -437,7 +437,10 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
>  /* nothing to tear down for USB */
>  break;
> -default:
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
> +/* nothing to tear down for SCSI */
> +break;
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>  break;
>  }
>  }
> diff --git a/src/security/security_apparmor.c 
> b/src/security/security_apparmor.c
> index af2b639..beddf6d 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -856,7 +856,7 @@ AppArmorSetSecurityHostdevLabel(virSecurity

Re: [libvirt] [PATCH v3 02/10] qemu: Introduce vhost-scsi capability

[John Ferlan]

On 11/08/2016 01:26 PM, Eric Farman wrote:
> Do all the stuff for the vhost-scsi capability in QEMU,
> so it's in place for our checks later.
> 
> Signed-off-by: Eric Farman 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/qemu/qemu_capabilities.c| 2 ++
>  src/qemu/qemu_capabilities.h| 1 +
>  tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml| 1 +
>  tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml| 1 +
>  tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml| 1 +
>  tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml| 1 +
>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml| 1 +
>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml| 1 +
>  tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml   | 1 +
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml| 1 +
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml| 1 +
>  13 files changed, 14 insertions(+)
> 

Although (again) required some more merges - they were trivial for me to
do...  I'm debating whether to push just to get them out of the way or
to hold off to ensure they go in at the same time as the other
changes...  I'll probably hold off, but I have a set of other changes
that are also modifying the capabilities, so it's a tossup.

John

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

Re: [libvirt] [PATCH v3 03/10] Introduce a "scsi_host" hostdev type

[John Ferlan]

s/$SUBJ/Introduce framework for hostdev SCSI_Host subsys type

On 11/08/2016 01:26 PM, Eric Farman wrote:
> We already have a "scsi" hostdev type, which refers to a single LUN
> that is passed through to a guest.  But what of things where multiple
> LUNs are passed through via a single SCSI HBA, such as with the
> vhost-scsi target?  Create a new hostdev type that will carry this.

s/hostdev type/hostdev subsys type/

The XML will eventually be:

  

  

NB: The "hostdev" RNG definition does list an optional ""
element which it doesn't seem you save in the guest config during any of
these patches. I do see a remnant of CCW for the running guest, but
nothing for PCI (which in the end is what's used - vhost-scsi-pci or
vhost-scsi-ccw). In any case, having "predictable" or "saved" addresses
is something we've found through history (USB and more recently Memory
dimms) to be a good idea.

The point being - I think you need to make sure that if not supplied,
then an address is generated (whether it's for PCI or CCW). While not in
this patch per se, it is something you'll need to handle in patch 7.


> 
> Signed-off-by: Eric Farman 
> ---
>  src/conf/domain_conf.c  | 12 +++-
>  src/conf/domain_conf.h  | 18 ++
>  src/qemu/qemu_cgroup.c  |  7 +++
>  src/qemu/qemu_hotplug.c |  2 ++
>  src/security/security_apparmor.c|  4 
>  src/security/security_dac.c |  8 
>  src/security/security_selinux.c |  8 
>  tests/domaincapsschemadata/full.xml |  1 +
>  8 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 043f0e2..b8a3366 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -647,7 +647,8 @@ VIR_ENUM_IMPL(virDomainHostdevMode, 
> VIR_DOMAIN_HOSTDEV_MODE_LAST,
>  VIR_ENUM_IMPL(virDomainHostdevSubsys, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST,
>"usb",
>"pci",
> -  "scsi")
> +  "scsi",
> +  "scsi_host")
>  
>  VIR_ENUM_IMPL(virDomainHostdevSubsysPCIBackend,
>VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST,
> @@ -661,6 +662,11 @@ VIR_ENUM_IMPL(virDomainHostdevSubsysSCSIProtocol,
>"adapter",
>"iscsi")
>  
> +VIR_ENUM_IMPL(virDomainHostdevSubsysHostProtocol,

In order to "follow" convention for structure names already

s/SubsysHostProtocol/SubsysSCSIHostProtocol/

Of course this has ramifications beyond this patch...

> +  VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_LAST,
> +  "none",
> +  "vhost")

[1] because of this...

> +
>  VIR_ENUM_IMPL(virDomainHostdevCaps, VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST,
>"storage",
>"misc",
> @@ -13016,6 +13022,8 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr 
> xmlopt,
>  break;
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
>  break;
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
> +break;
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>  goto error;
>  break;
> @@ -13899,6 +13907,8 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a,
>  return virDomainHostdevMatchSubsysSCSIiSCSI(a, b);
>  else
>  return virDomainHostdevMatchSubsysSCSIHost(a, b);
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
> +/* Fall through for now */
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>  return 0;
>  }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 541b600..8b03561 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -294,6 +294,7 @@ typedef enum {
>  VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB,
>  VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI,
>  VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI,
> +VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST,

[1] - I think this should use SCSI_HOST as well.

s/HOST/SCSI_HOST

There's lots of impact from hereonin...

>  
>  VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST
>  } virDomainHostdevSubsysType;
> @@ -368,6 +369,22 @@ struct _virDomainHostdevSubsysSCSI {
>  } u;
>  };
>  
> +typedef enum {
> +VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_NONE,
> +VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST,
> +
> +VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_LAST,

These would all need s/HOST/SCSI_HOST/

> +} virDomainHostdevSubsysHostProtocolType;
> +
> +VIR_ENUM_DECL(virDomainHostdevSubsysHostProtocol)

And these would have s/Host/SCSIHost/
> +
> +typedef struct _virDomainHostdevSubsysHost virDomainHostdevSubsysHost;
> +typedef virDomainHostdevSubsysHost *virDomainHostdevSubsysHostPtr;
> +struct _virDomainHostdevSubsysHost {

s/Host/SCSIHost/

> +int protocol;

We've been trying to add the enum in a comment e.g.

int protocol; /* enum virDomainHostdevSubsysSCSIHostProtocolType */

> +char *wwpn;
> +};
> +
>  typedef struct _virDo

Re: [libvirt] [PATCH v3 04/10] util: Management routines for scsi_host devices

[John Ferlan]

While perhaps mostly obvious - you need some sort of commit message here.

On 11/08/2016 01:26 PM, Eric Farman wrote:
> Signed-off-by: Eric Farman 
> ---
>  po/POTFILES.in   |   1 +
>  src/Makefile.am  |   1 +
>  src/libvirt_private.syms |  19 +++
>  src/util/virhost.c   | 299 
> +++
>  src/util/virhost.h   |  72 
>  src/util/virhostdev.c| 155 
>  src/util/virhostdev.h|  16 +++
>  tests/qemuxml2argvmock.c |   9 ++
>  8 files changed, 572 insertions(+)
>  create mode 100644 src/util/virhost.c
>  create mode 100644 src/util/virhost.h
> 

I fear someone will equate "virhost" to host virtual functions as
opposed to what it is.  You'll note there's virhostcpu, virhostdev, and
virhostmem - each of which are utility API's for that subsystem.

So I think this needs to become 'virscsihost.{c,h}' and of course the
API's are 'virSCSIHostDevice' prefixed instead of 'virHostDevice'.
Alternatively, the functions could go in virscsi.c, but I do prefer the
separation.

> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 1469240..a7cc542 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -199,6 +199,7 @@ src/util/virfirewall.c
>  src/util/virfirmware.c
>  src/util/virhash.c
>  src/util/virhook.c
> +src/util/virhost.c
>  src/util/virhostcpu.c
>  src/util/virhostdev.c
>  src/util/virhostmem.c
> diff --git a/src/Makefile.am b/src/Makefile.am
> index d417b6e..404c64e 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -122,6 +122,7 @@ UTIL_SOURCES =
> \
>   util/virhash.c util/virhash.h   \
>   util/virhashcode.c util/virhashcode.h   \
>   util/virhook.c util/virhook.h   \
> + util/virhost.c util/virhost.h   \
>   util/virhostcpu.c util/virhostcpu.h util/virhostcpupriv.h \
>   util/virhostdev.c util/virhostdev.h \
>   util/virhostmem.c util/virhostmem.h \
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 74dd527..ff535f9 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1675,6 +1675,23 @@ virHookInitialize;
>  virHookPresent;
>  
>  
> +# util/virhost.h
> +virHostDeviceFileIterate;
> +virHostDeviceFree;
> +virHostDeviceGetName;
> +virHostDeviceListAdd;
> +virHostDeviceListCount;
> +virHostDeviceListDel;
> +virHostDeviceListFind;
> +virHostDeviceListFindIndex;

This one is not used externally, so it could just be static to virhost.c

> +virHostDeviceListGet;
> +virHostDeviceListNew;
> +virHostDeviceListSteal;
> +virHostDeviceNew;
> +virHostDeviceSetUsedBy;
> +virHostOpenVhostSCSI;
> +
> +
>  # util/virhostdev.h
>  virHostdevFindUSBDevice;
>  virHostdevManagerGetDefault;
> @@ -1682,10 +1699,12 @@ virHostdevPCINodeDeviceDetach;
>  virHostdevPCINodeDeviceReAttach;
>  virHostdevPCINodeDeviceReset;
>  virHostdevPrepareDomainDevices;
> +virHostdevPrepareHostDevices;
>  virHostdevPreparePCIDevices;
>  virHostdevPrepareSCSIDevices;
>  virHostdevPrepareUSBDevices;
>  virHostdevReAttachDomainDevices;
> +virHostdevReAttachHostDevices;
>  virHostdevReAttachPCIDevices;
>  virHostdevReAttachSCSIDevices;
>  virHostdevReAttachUSBDevices;
> diff --git a/src/util/virhost.c b/src/util/virhost.c
> new file mode 100644
> index 000..9b5f524
> --- /dev/null
> +++ b/src/util/virhost.c
> @@ -0,0 +1,299 @@
> +/*
> + * virhost.c: helper APIs for managing scsi_host devices
> + *
> + * Copyright (C) 2016 IBM Corporation
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * .
> + *
> + * Authors:
> + * Eric Farman 
> + */
> +
> +#include 
> +#include 
> +
> +#include "virhost.h"
> +#include "virlog.h"
> +#include "viralloc.h"
> +#include "virerror.h"
> +#include "virfile.h"
> +#include "virstring.h"
> +
> +VIR_LOG_INIT("util.host");
> +
> +#define SYSFS_VHOST_SCSI_DEVICES "/sys/kernel/config/target/vhost/"
> +#define VHOST_SCSI_DEVICE "/dev/vhost-scsi"
> +
> +struct _virUsedByInfo {
> +char *drvname; /* which driver */
> +char *domname; /* which domain */
> +};
> +typedef struct _virUsedByInfo *virUsedByInfoPtr;

Hmm... seeing this makes me think the code should go in virscsi.c...
That would seem to 

Re: [libvirt] [PATCH v3 05/10] qemu: Add vhost-scsi string for -device parameter

[John Ferlan]

On 11/08/2016 01:26 PM, Eric Farman wrote:
> Open /dev/vhost-scsi, and record the resulting file descriptor, so that
> the guest has access to the host device outside of the libvirt daemon.
> Pass this information, along with data parsed from the XML file, to build
> a device string for the qemu command line.  That device string will be
> for either a vhost-scsi-ccw device in the case of an s390 machine, or
> vhost-scsi-pci for any others.
> 
> Signed-off-by: Eric Farman 
> ---
>  src/qemu/qemu_cgroup.c | 32 +
>  src/qemu/qemu_command.c| 79 
> ++
>  src/qemu/qemu_command.h|  5 +++
>  src/qemu/qemu_domain_address.c | 10 ++
>  src/qemu/qemu_hostdev.c| 41 ++
>  src/qemu/qemu_hostdev.h|  8 +
>  6 files changed, 175 insertions(+)
> 

Beyond the aforementioned "Host" to "SCSIHost" that will need to persist
into here...

> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index ee31d14..a22a1bf 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -277,6 +277,25 @@ qemuSetupHostSCSIDeviceCgroup(virSCSIDevicePtr dev 
> ATTRIBUTE_UNUSED,
>  return ret;
>  }
>  
> +static int
> +qemuSetupHostHostDeviceCgroup(virHostDevicePtr dev ATTRIBUTE_UNUSED,
> +  const char *path,
> +  void *opaque)
> +{
> +virDomainObjPtr vm = opaque;
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +int ret;
> +
> +VIR_DEBUG("Process path '%s' for scsi_host device", path);
> +
> +ret = virCgroupAllowDevicePath(priv->cgroup, path,
> +   VIR_CGROUP_DEVICE_RW, false);
> +
> +virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", ret == 
> 0);
> +
> +return ret;
> +}
> +
>  int
>  qemuSetupHostdevCgroup(virDomainObjPtr vm,
> virDomainHostdevDefPtr dev)
> @@ -286,9 +305,11 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
>  virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
>  virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
>  virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
> +virDomainHostdevSubsysHostPtr hostsrc = &dev->source.subsys.u.host;
>  virPCIDevicePtr pci = NULL;
>  virUSBDevicePtr usb = NULL;
>  virSCSIDevicePtr scsi = NULL;
> +virHostDevicePtr host = NULL;
>  char *path = NULL;
>  
>  /* currently this only does something for PCI devices using vfio
> @@ -377,6 +398,16 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
>  }
>  
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: {
> +if (hostsrc->protocol ==
> +VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST) {
> +if ((host = virHostDeviceNew(hostsrc->wwpn)) == NULL)

if (!host = vir...()))

is the more commonly used approach although yes I see you copied SCSI

> +goto cleanup;
> +
> +if (virHostDeviceFileIterate(host,
> + qemuSetupHostHostDeviceCgroup,
> + vm) < 0)
> +goto cleanup;
> +}
>  break;
>  }
>  
> @@ -390,6 +421,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
>  virPCIDeviceFree(pci);
>  virUSBDeviceFree(usb);
>  virSCSIDeviceFree(scsi);
> +virHostDeviceFree(host);
>  VIR_FREE(path);
>  return ret;
>  }
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 9adf0fe..ecd3286 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4730,6 +4730,43 @@ qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr 
> dev)
>  }
>  
>  char *
> +qemuBuildHostHostdevDevStr(const virDomainDef *def,
> +   virDomainHostdevDefPtr dev,
> +   virQEMUCapsPtr qemuCaps,
> +   char *vhostfdName)
> +{
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +virDomainHostdevSubsysHostPtr hostsrc = &dev->source.subsys.u.host;
> +
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VHOST_SCSI)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("This QEMU doesn't support vhost-scsi devices"));
> +goto cleanup;
> +}
> +
> +if (ARCH_IS_S390(def->os.arch))
> +virBufferAddLit(&buf, "vhost-scsi-ccw");
> +else
> +virBufferAddLit(&buf, "vhost-scsi-pci");
> +
> +virBufferAsprintf(&buf, ",wwpn=%s", hostsrc->wwpn);
> +virBufferAsprintf(&buf, ",vhostfd=%s", vhostfdName);
> +virBufferAsprintf(&buf, ",id=%s", dev->info->alias);

These could be combined into one formatted print...
> +
> +if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)

and yet no AddressStr is added to the command line - so we're missing a
place to configure that.

> +goto cleanup;
> +

Re: [libvirt] [PATCH v3 06/10] qemu: Allow hotplug of vhost-scsi device

[John Ferlan]

On 11/08/2016 01:26 PM, Eric Farman wrote:
> Adjust the device string that is built for vhost-scsi devices so that it
> can be invoked from hotplug.
> 
>>From the QEMU command line, the file descriptors are expect to be numeric 
>>only.

s/>//

Looks like a cut-n-paste carryover

> However, for hotplug, the file descriptors are expected to begin with at least
> one alphabetic character else this error occurs:
> 
>   # virsh attach-device guest_0001 ~/vhost.xml
>   error: Failed to attach device from /root/vhost.xml
>   error: internal error: unable to execute QEMU command 'getfd':
>   Parameter 'fdname' expects a name not starting with a digit
> 
> We also close the file descriptor in this case, so that shutting down the
> guest cleans up the host cgroup entries and allows future guests to use
> vhost-scsi devices.  (Otherwise the guest will silently end.)

See you're following the lead of qemuDomainAttachHostPCIDevice for the
'configfd' processing

> 
> Signed-off-by: Eric Farman 
> ---
>  src/qemu/qemu_hotplug.c | 158 
> 
>  1 file changed, 158 insertions(+)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 2d6b086..d503212 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2425,6 +2425,120 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
>  goto cleanup;
>  }
>  
> +static int
> +qemuDomainAttachHostSCSIHostDevice(virQEMUDriverPtr driver,
> +   virDomainObjPtr vm,
> +   virDomainHostdevDefPtr hostdev)
> +{
> +int ret = -1;
> +qemuDomainObjPrivatePtr priv = vm->privateData;

virErrorPtr orig_err;

> +virDomainCCWAddressSetPtr ccwaddrs = NULL;
> +char *vhostfdName = NULL;
> +int vhostfd = -1;
> +char *devstr = NULL;
> +bool teardowncgroup = false;
> +bool teardownlabel = false;
> +bool releaseaddr = false;
> +
> +if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("SCSI passthrough is not supported by this version 
> of qemu"));
> +goto cleanup;
> +}

Still not clear why SCSI_GENERIC is required - what is the guest device?

> +
> +if (qemuHostdevPrepareHostDevices(driver, vm->def->name, &hostdev, 1) < 
> 0) {
> +virDomainHostdevSubsysHostPtr hostsrc = 
> &hostdev->source.subsys.u.host;
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unable to prepare scsi_host hostdev: %s"),
> +   hostsrc->wwpn);
> +goto cleanup;
> +}
> +
> +if (qemuSetupHostdevCgroup(vm, hostdev) < 0)
> +goto cleanup;
> +teardowncgroup = true;
> +
> +if (virSecurityManagerSetHostdevLabel(driver->securityManager,
> +  vm->def, hostdev, NULL) < 0)
> +goto cleanup;
> +teardownlabel = true;
> +
> +if (virHostOpenVhostSCSI(&vhostfd) < 0)
> +goto cleanup;
> +
> +if (virAsprintf(&vhostfdName, "vhostfd-%d", vhostfd) < 0)
> +goto cleanup;
> +
> +if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> +if (qemuDomainMachineIsS390CCW(vm->def) &&
> +virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW))
> +hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
> +}
> +
> +if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
> +hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
> +if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, hostdev->info) < 0)

Here we are doing the PCI address thing, but I don't see patch7
addressing that... That is - no address is defined on the command line
(as seen the the patch9 .args file)

> +goto cleanup;
> +} else if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
> +if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def)))
> +goto cleanup;
> +if (virDomainCCWAddressAssign(hostdev->info, ccwaddrs,
> +  !hostdev->info->addr.ccw.assigned) < 0)
> +goto cleanup;
> +}
> +releaseaddr = true;
> +
> +if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0)
> +goto cleanup;
> +
> +if (!(devstr = qemuBuildHostHostdevDevStr(vm->def,
> +  hostdev,
> +  priv->qemuCaps,
> +  vhostfdName)))

If I look at the only other caller of qemuMonitorAddDeviceWithFd I note
that it does this slightly differently...  You may want to check that
out as they should be consistent.

In particular, the configfd_name is a combination of "fd-%s" where %s is
the alias (e.g. perhaps "fd-hostdev4"; whereas, this would seem to
generate "vhostfd-hostdev4"

> +goto cleanup;
> +

Re: [libvirt] [PATCH v3 07/10] conf: Wire up the vhost-scsi connection from/to XML

[John Ferlan]

need a commit message here.

On 11/08/2016 01:26 PM, Eric Farman wrote:
> Signed-off-by: Eric Farman 
> ---
>  docs/schemas/domaincommon.rng | 23 
>  src/conf/domain_audit.c   |  7 
>  src/conf/domain_conf.c| 81 
> +--
>  3 files changed, 109 insertions(+), 2 deletions(-)
> 

Beyond the "Host" to "SCSIHost" type changes...

Since this is where the device is being added - this is when the address
adjustment functions should be addressed. I got really hung up to day
trying to look for examples - hopefully I didn't make too much of a mess...

First off, the virDomainHostdevDefParseXML should be adjusted to ensure
that if the user does provide an address - that it is valid for the
device.  IOW: Follow the SUBSYS_TYPE_PCI: case more or less to ensure
the def->info->type if not NONE is either TYPE_PCI or TYPE_CCW.

The virDomainDeviceDefPostParseInternal post processing code should
handle setting an address if a valid one (checked earlier) isn't
supplied. This would save the address in the domain/config XML. For PCI
it would be qemuDomainAssignDevicePCISlots... I'm not clear if doing the
same for CCW is ever done.

In any case, qemuDomainAssignDevicePCISlots does go through the hostdev
list and reserves PCI address for PCI hostdevs, which I believe would
also work for this would be. Again, I'm not clear if/why CCW would be
handled.

NB: I haven't gone and looked for every new subsys case to ensure that
things were addressed for the case's type - just ran out of time and
energy.  But that is something that should be done by the end of patch8
(which I assume now gets merged into here eventually).

> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 19d45fd..bb903ef 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3974,6 +3974,7 @@
>
>
>
> +  
>  
>
>  
> @@ -4102,6 +4103,28 @@
>  
>
>  
> +  
> +
> +  scsi_host
> +
> +
> +  
> +
> +  
> +
> +  vhost 
> +
> +  
> +  
> +
> +  (naa\.)[0-9a-fA-F]{16}
> +
> +  
> +
> +  
> +
> +  
> +
>
>  
>storage
> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
> index 2decf02..844b3cd 100644
> --- a/src/conf/domain_audit.c
> +++ b/src/conf/domain_audit.c
> @@ -392,6 +392,7 @@ virDomainAuditHostdev(virDomainObjPtr vm, 
> virDomainHostdevDefPtr hostdev,
>  virDomainHostdevSubsysUSBPtr usbsrc = &hostdev->source.subsys.u.usb;
>  virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
>  virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi;
> +virDomainHostdevSubsysHostPtr hostsrc = &hostdev->source.subsys.u.host;
>  
>  virUUIDFormat(vm->def->uuid, uuidstr);
>  if (!(vmname = virAuditEncode("vm", vm->def->name))) {
> @@ -444,6 +445,12 @@ virDomainAuditHostdev(virDomainObjPtr vm, 
> virDomainHostdevDefPtr hostdev,
>  }
>  break;
>  }
> +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
> +if (VIR_STRDUP_QUIET(address, hostsrc->wwpn) < 0) {
> +VIR_WARN("OOM while encoding audit message");
> +goto cleanup;
> +}
> +break;
>  default:
>  VIR_WARN("Unexpected hostdev type while encoding audit message: 
> %d",
>   hostdev->source.subsys.type);
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b8a3366..75cacd9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2323,6 +2323,9 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr 
> def)
>  } else {
>  VIR_FREE(scsisrc->u.host.adapter);
>  }
> +} else if (def->source.subsys.type == 
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) {
> +virDomainHostdevSubsysHostPtr hostsrc = 
> &def->source.subsys.u.host;
> +VIR_FREE(hostsrc->wwpn);
>  }
>  break;
>  }
> @@ -6092,6 +6095,55 @@ virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr 
> sourcenode,
>  return ret;
>  }
>  
> +static int
> +virDomainHostdevSubsysHostDefParseXML(xmlNodePtr sourcenode,
> +  virDomainHostdevDefPtr def)
> +{
> +char *protocol = NULL;
> +virDomainHostdevSubsysHostPtr hostsrc = &def->source.subsys.u.host;
> +
> +if ((protocol = virXMLPropString(sourcenode, "protocol"))) {
> +hostsrc->protocol =
> +virDomainHostdevSubsysHostProtocolTypeFromString(protocol);
> +if (hostsrc->protocol <= 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Unknown scsi_host subsystem protocol '%s'"),
> +   protocol);
> +goto cleanup;
> +}
> +}

Since

Re: [libvirt] [PATCH v3 08/10] security: Include vhost-scsi in security labels

[John Ferlan]

On 11/08/2016 01:26 PM, Eric Farman wrote:
> Signed-off-by: Eric Farman 
> ---
>  src/security/security_apparmor.c | 18 -
>  src/security/security_dac.c  | 42 
> ++--
>  src/security/security_selinux.c  | 39 +++--
>  3 files changed, 94 insertions(+), 5 deletions(-)
> 

Beyond the "Host" to "SCSIHost" changes - these do seem fine to me -
whether we should combine them in the final push w/ the domain changes
is not clear right now (it's been a long week of reviews)... With a more
final set of patches I'll know better.

And of course at this point all new subsys type should have code
associated with it (or a reason why not) - I didn't go back and look.

John
> diff --git a/src/security/security_apparmor.c 
> b/src/security/security_apparmor.c
> index e7e3c8c..d7bc7d1 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -44,6 +44,7 @@
>  #include "viruuid.h"
>  #include "virpci.h"
>  #include "virusb.h"
> +#include "virhost.h"
>  #include "virfile.h"
>  #include "configmake.h"
>  #include "vircommand.h"
> @@ -357,6 +358,13 @@ AppArmorSetSecuritySCSILabel(virSCSIDevicePtr dev 
> ATTRIBUTE_UNUSED,
>  return AppArmorSetSecurityHostdevLabelHelper(file, opaque);
>  }
>  
> +static int
> +AppArmorSetSecurityHostLabel(virHostDevicePtr dev ATTRIBUTE_UNUSED,
> + const char *file, void *opaque)
> +{
> +return AppArmorSetSecurityHostdevLabelHelper(file, opaque);
> +}
> +
>  /* Called on libvirtd startup to see if AppArmor is available */
>  static int
>  AppArmorSecurityManagerProbe(const char *virtDriver ATTRIBUTE_UNUSED)
> @@ -831,6 +839,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
>  virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
>  virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
>  virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
> +virDomainHostdevSubsysHostPtr hostsrc = &dev->source.subsys.u.host;
>  
>  if (!secdef)
>  return -1;
> @@ -910,7 +919,14 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr 
> mgr,
>  }
>  
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: {
> -/* Fall through for now */
> +virHostDevicePtr host = virHostDeviceNew(hostsrc->wwpn);
> +
> +if (!host)
> +goto done;
> +
> +ret = virHostDeviceFileIterate(host, AppArmorSetSecurityHostLabel, 
> ptr);
> +virHostDeviceFree(host);
> +break;
>  }
>  
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index eba2a87..accb965 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -36,6 +36,7 @@
>  #include "virpci.h"
>  #include "virusb.h"
>  #include "virscsi.h"
> +#include "virhost.h"
>  #include "virstoragefile.h"
>  #include "virstring.h"
>  #include "virutil.h"
> @@ -582,6 +583,15 @@ virSecurityDACSetSCSILabel(virSCSIDevicePtr dev 
> ATTRIBUTE_UNUSED,
>  
>  
>  static int
> +virSecurityDACSetHostLabel(virHostDevicePtr dev ATTRIBUTE_UNUSED,
> +   const char *file,
> +   void *opaque)
> +{
> +return virSecurityDACSetHostdevLabelHelper(file, opaque);
> +}
> +
> +
> +static int
>  virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
>virDomainDefPtr def,
>virDomainHostdevDefPtr dev,
> @@ -592,6 +602,7 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
>  virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
>  virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
>  virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
> +virDomainHostdevSubsysHostPtr hostsrc = &dev->source.subsys.u.host;
>  int ret = -1;
>  
>  if (!priv->dynamicOwnership)
> @@ -677,7 +688,14 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
>  }
>  
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: {
> -/* Fall through for now */
> +virHostDevicePtr host = virHostDeviceNew(hostsrc->wwpn);
> +
> +if (!host)
> +goto done;
> +
> +ret = virHostDeviceFileIterate(host, virSecurityDACSetHostLabel, 
> &cbdata);
> +virHostDeviceFree(host);
> +break;
>  }
>  
>  case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
> @@ -724,6 +742,17 @@ virSecurityDACRestoreSCSILabel(virSCSIDevicePtr dev 
> ATTRIBUTE_UNUSED,
>  
>  
>  static int
> +virSecurityDACRestoreHostLabel(virHostDevicePtr dev ATTRIBUTE_UNUSED,
> +   const char *file,
> +   void *opaque)
> +{
> +virSecurityManagerPtr mgr = opaque;
> +virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +return virSecurityDACRestoreFileLabel(priv, file);
> +}
> +
> +
> +static int

Re: [libvirt] [PATCH v3 09/10] tests: Introduce basic vhost-scsi test

[John Ferlan]

On 11/08/2016 01:26 PM, Eric Farman wrote:
> These tests were cloned from hostdev-scsi-virtio-scsi in both
> xml2argv and xml2xml
> 
> Signed-off-by: Eric Farman 
> Reviewed-by: Boris Fiuczynski 
> ---
>  .../qemuxml2argv-hostdev-scsi-vhost-scsi.args  | 24 +
>  .../qemuxml2argv-hostdev-scsi-vhost-scsi.xml   | 41 
> ++
>  tests/qemuxml2argvtest.c   |  3 ++
>  .../qemuxml2xmlout-hostdev-scsi-vhost-scsi.xml |  1 +
>  tests/qemuxml2xmltest.c|  3 ++
>  5 files changed, 72 insertions(+)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.xml
>  create mode 12 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-vhost-scsi.xml
> 

Although nice to have the separation now - this would certainly be
merged with patch7...

Probably should add a CCW output test too.

John

> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.args 
> b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.args
> new file mode 100644
> index 000..5cd2939
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.args
> @@ -0,0 +1,24 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu \
> +-name QEMUGuest2 \
> +-S \
> +-M pc \
> +-m 214 \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid c7a5fdbd-edaf-9466-926a-d65c16db1809 \
> +-nographic \
> +-nodefaults \
> +-monitor unix:/tmp/lib/domain--1-QEMUGuest2/monitor.sock,server,nowait \
> +-no-acpi \
> +-boot c \
> +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \
> +-usb \
> +-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \
> +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
> +-device vhost-scsi-pci,wwpn=naa.5123456789abcde0,vhostfd=3,id=hostdev0 \

Note: lack of address...

> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.xml 
> b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.xml
> new file mode 100644
> index 000..4d57fb8
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.xml
> @@ -0,0 +1,41 @@
> +
> +  QEMUGuest2
> +  c7a5fdbd-edaf-9466-926a-d65c16db1809
> +  219100
> +  219100
> +  1
> +  
> +hvm
> +
> +  
> +  
> +  destroy
> +  restart
> +  destroy
> +  
> +/usr/bin/qemu
> +
> +  
> +  
> +  
> +
> +
> +   function='0x0'/>
> +
> +
> +   function='0x2'/>
> +
> +
> +   function='0x1'/>
> +
> +
> +
> +
> +
> +  
> +
> +
> +   function='0x0'/>
> +
> +  
> +
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index d025930..cfa1c85 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1886,6 +1886,9 @@ mymain(void)
>  DO_TEST("hostdev-scsi-virtio-iscsi-auth",
>  QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_SCSI,
>  QEMU_CAPS_DEVICE_SCSI_GENERIC);
> +DO_TEST("hostdev-scsi-vhost-scsi",
> +QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VHOST_SCSI,
> +QEMU_CAPS_DEVICE_SCSI_GENERIC);

Still not clear why SCSI_GENERIC is necessary, but I suppose it could
be... It's just not obvious.

>  
>  DO_TEST("mlock-on", QEMU_CAPS_REALTIME_MLOCK);
>  DO_TEST_FAILURE("mlock-on", NONE);
> diff --git 
> a/tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-vhost-scsi.xml 
> b/tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-vhost-scsi.xml
> new file mode 12
> index 000..76ebe4c
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-vhost-scsi.xml
> @@ -0,0 +1 @@
> +../qemuxml2argvdata/qemuxml2argv-hostdev-scsi-vhost-scsi.xml
> \ No newline at end of file
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 8a2b5ff..98c3523 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -737,6 +737,9 @@ mymain(void)
>  QEMU_CAPS_DEVICE_IOH3420,
>  QEMU_CAPS_HDA_DUPLEX);
>  
> +DO_TEST("hostdev-scsi-vhost-scsi",
> +QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VHOST_SCSI,
> +QEMU_CAPS_DEVICE_SCSI_GENERIC);

similar here too.

John
>  DO_TEST("hostdev-scsi-lsi",
>  QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_LSI,
>  QEMU_CAPS_DEVICE_SCSI_GENERIC);
> 

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

Re: [libvirt] [PATCH v3 10/10] docs: Add vhost-scsi

[John Ferlan]

On 11/08/2016 01:26 PM, Eric Farman wrote:
> Signed-off-by: Eric Farman 
> ---
>  docs/formatdomain.html.in | 24 
>  1 file changed, 24 insertions(+)
> 

This too would be merged...  OK for now though.

The details in your cover are really good - I'd like to see them
preserved somehow/someway...   Especially since they show how to set
things up...  Just have to figure out something - that's a thought for
another day though).

John
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 11b3330..ed96ca4 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3694,6 +3694,17 @@
>
>...
>  
> +or:
> +
> +
> +  ...
> +  
> +
> +  
> +
> +  
> +  ...
> +
>  
>hostdev
>The hostdev element is the main container for 
> describing
> @@ -3732,6 +3743,12 @@
>  If a disk lun in the domain already has the rawio capability,
>  then this setting not required.
>
> +  scsi_host
> +  since 2.5.0For SCSI devices, user
> +is responsible to make sure the device is not used by host. This
> +type passes all LUNs presented by a single HBA to
> +the guest.
> +  
>  
>  
>Note: The managed attribute is only used with PCI 
> devices
> @@ -3795,6 +3812,13 @@
>  credentials to the iSCSI server.
>  
>
> +  scsi_host
> +  Since 2.5.0, multiple LUNs behind a
> +single SCSI HBA are described by a protocol
> +attribute set to "vhost" and a wwpn attribute that
> +is the vhost_scsi wwpn (16 hexadecimal digits with a prefix of
> +"naa.") established in the host configfs.
> +  
>  
>
>vendor, product
> 

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