Re: [libvirt] [PATCH v2 00/10] hyperv: add support for Hyper-V 2012 and newer.

[Dawid Zamirski]

On Tue, 2017-04-04 at 00:11 +0200, Matthias Bolte wrote:
> 2017-04-02 6:50 GMT+02:00 Dawid Zamirski :
> > Hello,
> > 
> > This is v2 with all the code review feedback addressed. Please see
> > the
> > cover letter in v1 here:
> > https://www.redhat.com/archives/libvir-list/2017-March/msg01624.htm
> > l
> > 
> > Changes in v2:
> > * checked each patch with make syntax-check individually
> > * fixed missing notices in hyperv_wmi_generator.py
> > * addressed an issue where a case of "v2-only" WMI classes was not
> >   handled properly.
> 
> There is one final question one patch 9/10.
> 
> One thing that's missing is a potential documentation update (see
> docs/drvhyperv.html.in) and an addition to the news file (see
> docs/news.xml)
> 

Ok, I've just posted v3 addressing those:
https://www.redhat.com/archives/libvir-list/2017-April/msg00071.html

Thanks a lot for taking time to review my code and catching bugs :-)

Regards,
Dawid

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

[libvirt] [PATCH v3 02/12] hyperv: introduce hypervWmiClassInfo struct.

[Dawid Zamirski]

This struct is to be used to carry all the information necessary to
issue wsman requests for given WMI class. Those will be defined by the
generator code (as lists) so that they are handy for the driver code to
"extract" needed info depending on which hyper-v we're connected to.
For example:

hypervWmiClassInfoListPtr Msvm_ComputerSystem_WmiInfo = {
.count = 2
{
{
.name = "Msvm_ComputerSystem",
.version = "v1",
.rootUri = "http://asdf.com;,
...
},
{
.name = "Msvm_ComputerSystem",
.version = "v2",
.rootUri = "http://asdf.com/v2;,
...
},
}
};

Then the driver code will grab either "v1" or "v2" to pass info wsman
API, depending on hypervPrivate->wmiVersion value.
---
 src/hyperv/hyperv_wmi_classes.h | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/src/hyperv/hyperv_wmi_classes.h b/src/hyperv/hyperv_wmi_classes.h
index ac7a3b8..b0f3e3c 100644
--- a/src/hyperv/hyperv_wmi_classes.h
+++ b/src/hyperv/hyperv_wmi_classes.h
@@ -87,6 +87,28 @@ enum _Msvm_ConcreteJob_JobState {
 };
 
 
+typedef struct _hypervWmiClassInfo hypervWmiClassInfo;
+typedef hypervWmiClassInfo *hypervWmiClassInfoPtr;
+struct _hypervWmiClassInfo {
+/* The WMI class name */
+const char *name;
+/* The version of the WMI class as in "v1" or "v2" */
+const char *version;
+/* The URI for wsman enumerate request */
+const char *rootUri;
+/* The namespace URI for XML serialization */
+const char *resourceUri;
+/* The wsman serializer info - one of the *_TypeInfo structs */
+XmlSerializerInfo *serializerInfo;
+};
+
+
+typedef struct _hypervWmiClassInfoList hypervWmiClassInfoList;
+typedef hypervWmiClassInfoList *hypervWmiClassInfoListPtr;
+struct _hypervWmiClassInfoList {
+size_t count;
+hypervWmiClassInfoPtr *objs;
+};
 
 # include "hyperv_wmi_classes.generated.h"
 
-- 
2.9.3

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

[libvirt] [PATCH v3 06/12] hyperv: update generator input file.

[Dawid Zamirski]

Adds defintiions for "v2" WMI class variants that are needed by the
driver to handle both hyperv 2008 and 2012+
---
 src/hyperv/hyperv_wmi_generator.input | 239 +-
 1 file changed, 206 insertions(+), 33 deletions(-)

diff --git a/src/hyperv/hyperv_wmi_generator.input 
b/src/hyperv/hyperv_wmi_generator.input
index 209a9ff..d7f819e 100644
--- a/src/hyperv/hyperv_wmi_generator.input
+++ b/src/hyperv/hyperv_wmi_generator.input
@@ -56,6 +56,55 @@ class Msvm_ComputerSystem
 end
 
 
+class v2/Msvm_ComputerSystem
+string   InstanceID
+string   Caption
+string   Description
+string   ElementName
+datetime InstallDate
+uint16   OperationalStatus[]
+string   StatusDescriptions[]
+string   Status
+uint16   HealthState
+uint16   CommunicationStatus
+uint16   DetailedStatus
+uint16   OperatingStatus
+uint16   PrimaryStatus
+uint16   EnabledState
+string   OtherEnabledState
+uint16   RequestedState
+uint16   EnabledDefault
+datetime TimeOfLastStateChange
+uint16   AvailableRequestedStates[]
+uint16   TransitioningToState
+string   CreationClassName
+string   Name
+string   PrimaryOwnerName
+string   PrimaryOwnerContact
+string   Roles[]
+string   NameFormat
+string   OtherIdentifyingInfo[]
+string   IdentifyingDescriptions[]
+uint16   Dedicated[]
+string   OtherDedicatedDescriptions[]
+uint16   ResetCapability
+uint16   PowerManagementCapabilities[]
+uint64   OnTimeInMilliseconds
+uint32   ProcessID
+datetime TimeOfLastConfigurationChange
+uint16   NumberOfNumaNodes
+uint16   ReplicationState
+uint16   ReplicationHealth
+uint16   ReplicationMode
+uint16   FailedOverReplicationType
+uint16   LastReplicationType
+datetime LastApplicationConsistentReplicationTime
+datetime LastReplicationTime
+datetime LastSuccessfulBackupTime
+uint16   EnhancedSessionModeState
+end
+
+
 class Msvm_ConcreteJob
 string   Caption
 string   Description
@@ -96,6 +145,51 @@ class Msvm_ConcreteJob
 end
 
 
+class v2/Msvm_ConcreteJob
+string   InstanceID
+string   Caption
+string   Description
+string   ElementName
+datetime InstallDate
+string   Name
+uint16   OperationalStatus[]
+string   StatusDescriptions[]
+string   Status
+uint16   HealthState
+uint16   CommunicationStatus
+uint16   DetailedStatus
+uint16   OperatingStatus
+uint16   PrimaryStatus
+string   JobStatus
+datetime TimeSubmitted
+datetime ScheduledStartTime
+datetime StartTime
+datetime ElapsedTime
+uint32   JobRunTimes
+uint8RunMonth
+sint8RunDay
+sint8RunDayOfWeek
+datetime RunStartInterval
+uint16   LocalOrUtcTime
+datetime UntilTime
+string   Notify
+string   Owner
+uint32   Priority
+uint16   PercentComplete
+boolean  DeleteOnCompletion
+uint16   ErrorCode
+string   ErrorDescription
+string   ErrorSummaryDescription
+uint16   RecoveryAction
+string   OtherRecoveryAction
+uint16   JobState
+datetime TimeOfLastStateChange
+datetime TimeBeforeRemoval
+boolean  Cancellable
+uint16   JobType
+end
+
+
 class Msvm_MemorySettingData
 string   Caption
 string   Description
@@ -126,6 +220,38 @@ class Msvm_MemorySettingData
 end
 
 
+class v2/Msvm_MemorySettingData
+string  InstanceID
+string  Caption
+string  Description
+string  ElementName
+uint16  ResourceType
+string  OtherResourceType
+string  ResourceSubType
+string  PoolID
+uint16  ConsumerVisibility
+string  HostResource[]
+string  AllocationUnits
+uint64  VirtualQuantity
+uint64  Reservation
+uint64  Limit
+uint32  Weight
+boolean AutomaticAllocation
+boolean AutomaticDeallocation
+string  Parent
+string  Connection[]
+string  Address
+uint16  MappingBehavior
+string  AddressOnParent
+string  VirtualQuantityUnits
+boolean DynamicMemoryEnabled
+uint32  TargetMemoryBuffer
+boolean IsVirtualized
+boolean SwapFilesInUse
+uint64  MaxMemoryBlocksPerNumaNode
+end
+
+
 class Msvm_ProcessorSettingData
 string   Caption
 string   Description
@@ -159,6 +285,37 @@ class Msvm_ProcessorSettingData
 end
 
 
+class v2/Msvm_ProcessorSettingData
+string  InstanceID
+string  Caption
+string  Description
+string  ElementName
+uint16  ResourceType
+string  OtherResourceType
+string  ResourceSubType
+string  PoolID
+uint16  ConsumerVisibility
+string  HostResource[]
+string  AllocationUnits
+uint64  VirtualQuantity
+uint64  Reservation
+uint64  Limit
+uint32  Weight
+boolean AutomaticAllocation
+boolean AutomaticDeallocation
+string  Parent
+string  Connection[]
+string  Address
+uint16  MappingBehavior
+string  AddressOnParent
+string  

[libvirt] [PATCH v3 10/12] hyperv: add hypervInitConnection.

[Dawid Zamirski]

This function detects hyperv version by issuing a simple query using
"v2" namespace and falling back to "v1".
---
 src/hyperv/hyperv_driver.c | 92 --
 1 file changed, 56 insertions(+), 36 deletions(-)

diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 5d01c1f..0913517 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -55,7 +55,62 @@ hypervFreePrivate(hypervPrivate **priv)
 VIR_FREE(*priv);
 }
 
+static int
+hypervInitConnection(virConnectPtr conn, hypervPrivate *priv,
+ char *username, char *password)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+hypervWqlQuery query = HYPERV_WQL_QUERY_INITIALIZER;
+hypervObject *computerSystem = NULL;
+int ret = -1;
 
+/* Initialize the openwsman connection */
+priv->client = wsmc_create(conn->uri->server, conn->uri->port, "/wsman",
+   priv->parsedUri->transport, username, password);
+
+if (priv->client == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Could not create openwsman client"));
+goto cleanup;
+}
+
+if (wsmc_transport_init(priv->client, NULL) != 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Could not initialize openwsman transport"));
+goto cleanup;
+}
+
+/* FIXME: Currently only basic authentication is supported  */
+wsman_transport_set_auth_method(priv->client, "basic");
+
+virBufferAddLit(, MSVM_COMPUTERSYSTEM_WQL_SELECT);
+virBufferAddLit(, "WHERE ");
+virBufferAddLit(, MSVM_COMPUTERSYSTEM_WQL_PHYSICAL);
+
+query.query = virBufferContentAndReset();
+query.info = Msvm_ComputerSystem_WmiInfo;
+
+/* try query using V2 namespace (for Hyperv 2012+) */
+priv->wmiVersion = HYPERV_WMI_VERSION_V2;
+
+if (hypervEnumAndPull(priv, , ) < 0) {
+/* fall back to V1 namespace (for Hyper-v 2008) */
+priv->wmiVersion = HYPERV_WMI_VERSION_V1;
+
+if (hypervEnumAndPull(priv, , ) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("%s is not a Hyper-V server"), conn->uri->server);
+goto cleanup;
+}
+}
+
+ret = 0;
+
+ cleanup:
+hypervFreeObject(priv, computerSystem);
+
+return ret;
+}
 
 static virDrvOpenStatus
 hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
@@ -67,8 +122,6 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
 hypervPrivate *priv = NULL;
 char *username = NULL;
 char *password = NULL;
-virBuffer query = VIR_BUFFER_INITIALIZER;
-Msvm_ComputerSystem *computerSystem = NULL;
 
 virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
 
@@ -147,41 +200,9 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr 
auth,
 goto cleanup;
 }
 
-/* Initialize the openwsman connection */
-priv->client = wsmc_create(conn->uri->server, conn->uri->port, "/wsman",
-   priv->parsedUri->transport, username, password);
-
-if (priv->client == NULL) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Could not create openwsman client"));
-goto cleanup;
-}
-
-if (wsmc_transport_init(priv->client, NULL) != 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Could not initialize openwsman transport"));
-goto cleanup;
-}
-
-/* FIXME: Currently only basic authentication is supported  */
-wsman_transport_set_auth_method(priv->client, "basic");
-
-/* Check if the connection can be established and if the server has the
- * Hyper-V role installed. If the call to hypervGetMsvmComputerSystemList
- * succeeds than the connection has been established. If the returned list
- * is empty than the server isn't a Hyper-V server. */
-virBufferAddLit(, MSVM_COMPUTERSYSTEM_WQL_SELECT);
-virBufferAddLit(, "where ");
-virBufferAddLit(, MSVM_COMPUTERSYSTEM_WQL_PHYSICAL);
-
-if (hypervGetMsvmComputerSystemList(priv, , ) < 0)
-goto cleanup;
 
-if (computerSystem == NULL) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("%s is not a Hyper-V server"), conn->uri->server);
+if (hypervInitConnection(conn, priv, username, password) < 0)
 goto cleanup;
-}
 
 conn->privateData = priv;
 priv = NULL;
@@ -191,7 +212,6 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr 
auth,
 hypervFreePrivate();
 VIR_FREE(username);
 VIR_FREE(password);
-hypervFreeObject(priv, (hypervObject *)computerSystem);
 
 return result;
 }
-- 
2.9.3

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

[libvirt] [PATCH v3 03/12] hyperv: update hypervObject struct.

[Dawid Zamirski]

Currently named as hypervObjecUnified to keep code
compilable/functional until all bits are in place.

This struct is a result of unserializing WMI request response.
Therefore, it needs to be able to deal with different "versions" of the
same WMI class. To accomplish this, the "data" member was turned in to
a union which:

* has a "common" member that contains only WMI class fields that are
  safe to access and are present in all "versions". This is ensured by
  the code generator that takes care of proper struct memory alignment
  between "common", "v1", "v2" etc members. This memeber is to be used
  by the driver code wherever the API implementation can be shared for
  all supported hyper-v versions.
* the "v1" and "v2" member can be used by the driver code to handle
  version specific cases.

Example:

Msvm_ComputerSystem *vm = NULL;
...
hypervGetVirtualMachineList(priv, wqlQuery, *vm);
...
/* safe for "v1" and "v2" */
char *vmName = vm->data.common->Name;

/* or if one really needs special handling for "v2" */
if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) {
char *foo = vm->data.v2->SomeV2OnlyField;
}

In other words, driver should not concern itself with existence of "v1"
or "v2" of WMI class unless absolutely necessary.
---
 src/hyperv/hyperv_wmi.h | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h
index ce1fe05..5fc36e8 100644
--- a/src/hyperv/hyperv_wmi.h
+++ b/src/hyperv/hyperv_wmi.h
@@ -41,6 +41,25 @@ int hypervVerifyResponse(WsManClient *client, WsXmlDocH 
response,
 /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
  * Object
  */
+typedef struct _hypervObjectUnified hypervObjectUnified;
+struct _hypervObjectUnified {
+/* Unserialized data from wsman response. The member called "common" has
+ * properties that are the same type and name for all "versions" of given
+ * WMI class. This means that calling code does not have to make any
+ * conditional checks based on which version was returned as long as it
+ * only needs to read common values. The alignment of structs is ensured
+ * by the generator.
+ */
+union {
+XML_TYPE_PTR common;
+XML_TYPE_PTR v1;
+XML_TYPE_PTR v2;
+} data;
+/* The info used to make wsman request */
+hypervWmiClassInfoPtr info;
+
+hypervObjectUnified *next;
+};
 
 struct _hypervObject {
 XmlSerializerInfo *serializerInfo;
-- 
2.9.3

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

[libvirt] [PATCH v3 01/12] hyperv: store WMI version in hypervPrivate.

[Dawid Zamirski]

Hyper-V 2012+ uses a new "v2" version of Msvm_* WMI classes so we will
store that info in hypervPrivate so that it is easily accessbile in the
driver API callbacks and handled accordingly.
---
 src/hyperv/hyperv_private.h | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/hyperv/hyperv_private.h b/src/hyperv/hyperv_private.h
index 574bb5f..4e70699 100644
--- a/src/hyperv/hyperv_private.h
+++ b/src/hyperv/hyperv_private.h
@@ -28,11 +28,17 @@
 # include "hyperv_util.h"
 # include "openwsman.h"
 
-typedef struct _hypervPrivate hypervPrivate;
+typedef enum _hypervWmiVersion hypervWmiVersion;
+enum _hypervWmiVersion {
+HYPERV_WMI_VERSION_V1,
+HYPERV_WMI_VERSION_V2,
+};
 
+typedef struct _hypervPrivate hypervPrivate;
 struct _hypervPrivate {
 hypervParsedUri *parsedUri;
 WsManClient *client;
+hypervWmiVersion wmiVersion;
 };
 
 #endif /* __HYPERV_PRIVATE_H__ */
-- 
2.9.3

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

[libvirt] [PATCH v3 07/12] hyperv: update wmi code generator.

[Dawid Zamirski]

This patch updates the code generator that outputs C headers and code
for WMI classes. It has been updated to handle multiple versions (or
namespaces) of the same class which were introduced with Hyperv 2012+
---
 src/hyperv/hyperv_wmi_generator.py | 385 +++--
 1 file changed, 288 insertions(+), 97 deletions(-)

diff --git a/src/hyperv/hyperv_wmi_generator.py 
b/src/hyperv/hyperv_wmi_generator.py
index 8c62882..f2c9cde 100755
--- a/src/hyperv/hyperv_wmi_generator.py
+++ b/src/hyperv/hyperv_wmi_generator.py
@@ -24,130 +24,310 @@ import sys
 import os
 import os.path
 
+separator = "/*" + ("*" * 50) + "*\n"
+wmi_version_separator = "/"
+wmi_classes_by_name = {}
+
+class WmiClass:
+"""Represents WMI class and provides methods to generate C code.
+
+This class holds one or more instances of WmiClassVersion because with the
+Windows 2012 release, Microsoft introduced "v2" version of Msvm_* family of
+classes that need different URI for making wsman requests and also have
+some additional/changed properties (though many of the properies are the
+same as in "v1". Therefore, this class makes sure that C code is generated
+for each of them while avoiding name conflics, identifies common members,
+and defined *_WmiInfo structs holding info about each version so the driver
+code can make the right choices based on which Hyper-v host it's connected
+to.
+"""
+
+def __init__(self, name, versions = []):
+self.name = name
+self.versions = versions
+self.common = None
 
 
-separator = "/* " + ("* " * 37) + "*\n"
+def prepare(self):
+"""Prepares the class for code generation
 
+Makes sure that "versioned" classes are sorted by version, identfies
+common properies and ensures that they are aligned by name and
+type in each version
+"""
+# sort vesioned classes by version in case input file did not have them
+# in order
+self.versions = sorted(self.versions, key=lambda cls: cls.version)
 
+# if there's more than one verion make sure first one has name suffixed
+# because we'll generate "common" memeber and will be the "base" name
+if len(self.versions) > 1:
+first = self.versions[0]
+if first.version == None:
+first.version = "v1"
+first.name = "%s_%s" % (first.name, first.version)
 
-class Class:
-def __init__(self, name, properties):
-self.name = name
-self.properties = properties
+# finally, identify common members in all versions and make sure they
+# are in the same order - to ensure C struc member alignment
+self._align_property_members()
 
 
-def generate_header(self):
+def generate_classes_header(self):
+"""Generate C header code and return it as string
+
+Declares:
+  _Data - used as one of hypervObject->data members
+  _TypeInfo - used as wsman XmlSerializerInfo
+   - "inherits" hypervObject struct
+"""
+
 name_upper = self.name.upper()
 
 header = separator
 header += " * %s\n" % self.name
 header += " */\n"
 header += "\n"
-header += "int hypervGet%sList(hypervPrivate *priv, virBufferPtr 
query, %s **list);\n" \
-  % (self.name.replace("_", ""), self.name)
-header += "\n"
+header += "#define %s_CLASSNAME \\\n" % name_upper
+header += "\"%s\"\n" % self.name
 header += "\n"
+header += "#define %s_WQL_SELECT \\\n" % name_upper
+header += "\"SELECT * FROM %s \"\n" % self.name
 header += "\n"
+header += "extern hypervWmiClassInfoListPtr %s_WmiInfo;\n\n" % 
self.name
+
+header += self._declare_data_structs()
+header += self._declare_hypervObject_struct()
 
 return header
 
 
+def generate_classes_source(self):
+"""Returns a C code string defining wsman data structs
+
+Defines:
+  _Data structs
+  _WmiInfo - list holding metadata (e.g. request URIs) for
+ each known version of WMI class.
+"""
+
+source = separator
+source += " * %s\n" % self.name
+source += " */\n"
+
+for cls in self.versions:
+source += "SER_START_ITEMS(%s_Data)\n" % cls.name
+
+for property in cls.properties:
+source += property.generate_classes_source(cls.name)
+
+source += "SER_END_ITEMS(%s_Data);\n\n" % cls.name
+
+
+source += self._define_WmiInfo_struct()
+source += "\n\n"
+
+return source
+
+
 def generate_classes_typedef(self):
-typedef = "typedef struct _%s_Data %s_Data;\n" % (self.name, self.name)
-typedef += "typedef struct _%s %s;\n" % (self.name, self.name)
+"""Returns C string for typdefs"""
+
+typedef = "typedef struct _%s 

[libvirt] [PATCH v3 05/12] hyperv: make hypervEnumAndPull use hypervWqlQuery

[Dawid Zamirski]

This enables this function to handle "v1" and "v2" WMI requests.

Since this commit and the ones that follow should be squashed on
previous one:
* rename hypervObjectUnified -> hypervObject as we've already broken
  compilation here so there's no point in keeping those in parallel
  anymore.
* do not mark hypervGetWmiClassInfo as unused
---
 src/hyperv/hyperv_wmi.c | 40 ++--
 src/hyperv/hyperv_wmi.h | 17 -
 2 files changed, 22 insertions(+), 35 deletions(-)

diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
index 069bcc9..5cac58d 100644
--- a/src/hyperv/hyperv_wmi.c
+++ b/src/hyperv/hyperv_wmi.c
@@ -45,7 +45,7 @@
 #define VIR_FROM_THIS VIR_FROM_HYPERV
 
 
-static int ATTRIBUTE_UNUSED
+static int
 hypervGetWmiClassInfo(hypervPrivate *priv, hypervWmiClassInfoListPtr list,
   hypervWmiClassInfoPtr *info)
 {
@@ -143,14 +143,13 @@ hypervVerifyResponse(WsManClient *client, WsXmlDocH 
response,
 
 /* This function guarantees that query is freed, even on failure */
 int
-hypervEnumAndPull(hypervPrivate *priv, virBufferPtr query, const char *root,
-  XmlSerializerInfo *serializerInfo, const char *resourceUri,
-  const char *className, hypervObject **list)
+hypervEnumAndPull(hypervPrivate *priv, hypervWqlQueryPtr wqlQuery,
+  hypervObject **list)
 {
 int result = -1;
 WsSerializerContextH serializerContext;
 client_opt_t *options = NULL;
-char *query_string = NULL;
+hypervWmiClassInfoPtr wmiInfo = NULL;
 filter_t *filter = NULL;
 WsXmlDocH response = NULL;
 char *enumContext = NULL;
@@ -160,18 +159,14 @@ hypervEnumAndPull(hypervPrivate *priv, virBufferPtr 
query, const char *root,
 XML_TYPE_PTR data = NULL;
 hypervObject *object;
 
-if (virBufferCheckError(query) < 0) {
-virBufferFreeAndReset(query);
-return -1;
-}
-query_string = virBufferContentAndReset(query);
-
 if (list == NULL || *list != NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-VIR_FREE(query_string);
 return -1;
 }
 
+if (hypervGetWmiClassInfo(priv, wqlQuery->info, ) < 0)
+return -1;
+
 serializerContext = wsmc_get_serialization_context(priv->client);
 
 options = wsmc_options_init();
@@ -182,7 +177,7 @@ hypervEnumAndPull(hypervPrivate *priv, virBufferPtr query, 
const char *root,
 goto cleanup;
 }
 
-filter = filter_create_simple(WSM_WQL_FILTER_DIALECT, query_string);
+filter = filter_create_simple(WSM_WQL_FILTER_DIALECT, wqlQuery->query);
 
 if (filter == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -190,7 +185,8 @@ hypervEnumAndPull(hypervPrivate *priv, virBufferPtr query, 
const char *root,
 goto cleanup;
 }
 
-response = wsmc_action_enumerate(priv->client, root, options, filter);
+response = wsmc_action_enumerate(priv->client, wmiInfo->rootUri, options,
+ filter);
 
 if (hypervVerifyResponse(priv->client, response, "enumeration") < 0)
 goto cleanup;
@@ -201,7 +197,7 @@ hypervEnumAndPull(hypervPrivate *priv, virBufferPtr query, 
const char *root,
 response = NULL;
 
 while (enumContext != NULL && *enumContext != '\0') {
-response = wsmc_action_pull(priv->client, resourceUri, options,
+response = wsmc_action_pull(priv->client, wmiInfo->resourceUri, 
options,
 filter, enumContext);
 
 if (hypervVerifyResponse(priv->client, response, "pull") < 0)
@@ -231,11 +227,12 @@ hypervEnumAndPull(hypervPrivate *priv, virBufferPtr 
query, const char *root,
 goto cleanup;
 }
 
-if (ws_xml_get_child(node, 0, resourceUri, className) == NULL)
+if (ws_xml_get_child(node, 0, wmiInfo->resourceUri,
+ wmiInfo->name) == NULL)
 break;
 
-data = ws_deserialize(serializerContext, node, serializerInfo,
-  className, resourceUri, NULL, 0, 0);
+data = ws_deserialize(serializerContext, node, wmiInfo->serializerInfo,
+  wmiInfo->name, wmiInfo->resourceUri, NULL, 0, 0);
 
 if (data == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -246,8 +243,8 @@ hypervEnumAndPull(hypervPrivate *priv, virBufferPtr query, 
const char *root,
 if (VIR_ALLOC(object) < 0)
 goto cleanup;
 
-object->serializerInfo = serializerInfo;
-object->data = data;
+object->info = wmiInfo;
+object->data.common = data;
 
 data = NULL;
 
@@ -283,13 +280,12 @@ hypervEnumAndPull(hypervPrivate *priv, virBufferPtr 
query, const char *root,
 /* FIXME: ws_serializer_free_mem is broken in openwsman <= 2.2.6,
  *see hypervFreeObject for a detailed explanation. */
 if 

[libvirt] [PATCH v3 04/12] hyperv: add hypervWqlQuery struct.

[Dawid Zamirski]

Since this is the last commit that will compile in the series, the
remaining patches should be squashed into this one - they are kept
separate only for code review purposes.

This struct is to be passed to enumerate-and-pull wsman request (to run
"Select" queries) and provides the hypervWmiClassInfoListPtr instance
from which we can extract the version specific info using the new
hypervGetWmiClassInfo function (currently unused)
---
 src/hyperv/hyperv_wmi.c | 35 +++
 src/hyperv/hyperv_wmi.h |  8 
 2 files changed, 43 insertions(+)

diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
index b2b5243..069bcc9 100644
--- a/src/hyperv/hyperv_wmi.c
+++ b/src/hyperv/hyperv_wmi.c
@@ -45,6 +45,41 @@
 #define VIR_FROM_THIS VIR_FROM_HYPERV
 
 
+static int ATTRIBUTE_UNUSED
+hypervGetWmiClassInfo(hypervPrivate *priv, hypervWmiClassInfoListPtr list,
+  hypervWmiClassInfoPtr *info)
+{
+const char *version = "v2";
+size_t i;
+
+if (list->count == 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("The WMI class info list is empty"));
+return -1;
+}
+
+/* if there's just one WMI class and isn't versioned, assume "shared" */
+if (list->count == 1 && list->objs[0]->version == NULL) {
+*info = list->objs[0];
+return 0;
+}
+
+if (priv->wmiVersion == HYPERV_WMI_VERSION_V1)
+version = "v1";
+
+for (i = 0; i < list->count; i++) {
+   if (STRCASEEQ(list->objs[i]->version, version)) {
+   *info = list->objs[i];
+   return 0;
+   }
+}
+
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Could not match WMI class info for version %s"),
+   version);
+
+return -1;
+}
 
 int
 hypervVerifyResponse(WsManClient *client, WsXmlDocH response,
diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h
index 5fc36e8..12b94af 100644
--- a/src/hyperv/hyperv_wmi.h
+++ b/src/hyperv/hyperv_wmi.h
@@ -30,6 +30,7 @@
 # include "openwsman.h"
 
 
+# define HYPERV_WQL_QUERY_INITIALIZER {NULL, NULL}
 
 typedef struct _hypervObject hypervObject;
 
@@ -61,6 +62,13 @@ struct _hypervObjectUnified {
 hypervObjectUnified *next;
 };
 
+typedef struct _hypervWqlQuery hypervWqlQuery;
+typedef hypervWqlQuery *hypervWqlQueryPtr;
+struct _hypervWqlQuery {
+const char *query;
+hypervWmiClassInfoListPtr info;
+};
+
 struct _hypervObject {
 XmlSerializerInfo *serializerInfo;
 XML_TYPE_PTR data;
-- 
2.9.3

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

[libvirt] [PATCH v3 08/12] hyperv: add helper for getting WMI class lists.

[Dawid Zamirski]

Those used to be auto-generated and are hand-written now intead. The
reason being that those are not very useful and better replacements are
in order once the driver itself implements more of the API and common
patterns start to emerge.
---
 src/Makefile.am |  2 -
 src/hyperv/hyperv_wmi.c | 81 +
 src/hyperv/hyperv_wmi.h | 24 +++-
 src/hyperv/hyperv_wmi_classes.h |  8 
 4 files changed, 104 insertions(+), 11 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 75e4344..99f5229 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -910,8 +910,6 @@ HYPERV_DRIVER_SOURCES = 
\
hyperv/openwsman.h
 
 HYPERV_DRIVER_GENERATED =  
\
-   hyperv/hyperv_wmi.generated.c   
\
-   hyperv/hyperv_wmi.generated.h   
\
hyperv/hyperv_wmi_classes.generated.c   
\
hyperv/hyperv_wmi_classes.generated.h   
\
hyperv/hyperv_wmi_classes.generated.typedef
diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
index 5cac58d..c2d64ba 100644
--- a/src/hyperv/hyperv_wmi.c
+++ b/src/hyperv/hyperv_wmi.c
@@ -36,12 +36,6 @@
 
 #define WS_SERIALIZER_FREE_MEM_WORKS 0
 
-#define ROOT_CIMV2 \
-"http://schemas.microsoft.com/wbem/wsman/1/wmi/root/cimv2/*;
-
-#define ROOT_VIRTUALIZATION \
-"http://schemas.microsoft.com/wbem/wsman/1/wmi/root/virtualization/*;
-
 #define VIR_FROM_THIS VIR_FROM_HYPERV
 
 
@@ -81,6 +75,18 @@ hypervGetWmiClassInfo(hypervPrivate *priv, 
hypervWmiClassInfoListPtr list,
 return -1;
 }
 
+static int
+hypervGetWmiClassList(hypervPrivate *priv, hypervWmiClassInfoListPtr wmiInfo,
+  virBufferPtr query, hypervObject **wmiClass)
+{
+hypervWqlQuery wqlQuery = HYPERV_WQL_QUERY_INITIALIZER;
+
+wqlQuery.info = wmiInfo;
+wqlQuery.query = virBufferContentAndReset(query);
+
+return hypervEnumAndPull(priv, , wmiClass);
+}
+
 int
 hypervVerifyResponse(WsManClient *client, WsXmlDocH response,
  const char *detail)
@@ -694,5 +700,66 @@ hypervMsvmComputerSystemFromDomain(virDomainPtr domain,
 }
 
 
+int
+hypervGetMsvmComputerSystemList(hypervPrivate *priv, virBufferPtr query,
+Msvm_ComputerSystem **list)
+{
+return hypervGetWmiClassList(priv, Msvm_ComputerSystem_WmiInfo, query,
+ (hypervObject **) list);
+}
+
+
+int
+hypervGetMsvmConcreteJobList(hypervPrivate *priv, virBufferPtr query,
+ Msvm_ConcreteJob **list)
+{
+return hypervGetWmiClassList(priv, Msvm_ConcreteJob_WmiInfo, query,
+ (hypervObject **) list);
+}
+
+
+int
+hypervGetWin32ComputerSystemList(hypervPrivate *priv, virBufferPtr query,
+ Win32_ComputerSystem **list)
+{
+return hypervGetWmiClassList(priv, Win32_ComputerSystem_WmiInfo, query,
+ (hypervObject **) list);
+}
+
+
+int
+hypervGetWin32ProcessorList(hypervPrivate *priv, virBufferPtr query,
+Win32_Processor **list)
+{
+return hypervGetWmiClassList(priv, Win32_Processor_WmiInfo, query,
+ (hypervObject **) list);
+}
+
+
+int
+hypervGetMsvmVirtualSystemSettingDataList(hypervPrivate *priv,
+  virBufferPtr query,
+  Msvm_VirtualSystemSettingData **list)
+{
+return hypervGetWmiClassList(priv, Msvm_VirtualSystemSettingData_WmiInfo, 
query,
+ (hypervObject **) list);
+}
+
+
+int
+hypervGetMsvmProcessorSettingDataList(hypervPrivate *priv,
+  virBufferPtr query,
+  Msvm_ProcessorSettingData **list)
+{
+return hypervGetWmiClassList(priv, Msvm_ProcessorSettingData_WmiInfo, 
query,
+ (hypervObject **) list);
+}
+
 
-#include "hyperv_wmi.generated.c"
+int
+hypervGetMsvmMemorySettingDataList(hypervPrivate *priv, virBufferPtr query,
+   Msvm_MemorySettingData **list)
+{
+return hypervGetWmiClassList(priv, Msvm_MemorySettingData_WmiInfo, query,
+ (hypervObject **) list);
+}
diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h
index 8ce32a9..2d90a3b 100644
--- a/src/hyperv/hyperv_wmi.h
+++ b/src/hyperv/hyperv_wmi.h
@@ -109,7 +109,29 @@ enum _Msvm_ReturnCode {
 
 const char *hypervReturnCodeToString(int returnCode);
 
+/* Generic "Get WMI class list" */
+int hypervGetMsvmComputerSystemList(hypervPrivate *priv, virBufferPtr query,
+Msvm_ComputerSystem **list);
 
+int 

[libvirt] [PATCH v3 12/12] news: update for Hyper-V 2012+ support.

[Dawid Zamirski]

---
 docs/news.xml | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 9c0dcfd..39631c3 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -37,6 +37,17 @@
 
 
 
+  
+
+  The libvirt Hyper-V driver now supports Hyper-V 2012 and newer.
+
+
+Starting with Hyper-V 2012 the API has changed causing the existing
+driver to be unable to send and process requests properly. This has
+been resolved by adding abstractions to handle the differences and
+ease handling such breaks if they happen in the future.
+   
+  
 
 
 
-- 
2.9.3

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

[libvirt] [PATCH v3 00/12] hyperv: add support for Hyper-V 2012 and newer.

[Dawid Zamirski]

Changes since v2[1]:
* address the issue with incorrect request URI used in invoke
* update driver doc
* add news entry

[1] https://www.redhat.com/archives/libvir-list/2017-April/msg00018.html

Dawid Zamirski (12):
  hyperv: store WMI version in hypervPrivate.
  hyperv: introduce hypervWmiClassInfo struct.
  hyperv: update hypervObject struct.
  hyperv: add hypervWqlQuery struct.
  hyperv: make hypervEnumAndPull use hypervWqlQuery
  hyperv: update generator input file.
  hyperv: update wmi code generator.
  hyperv: add helper for getting WMI class lists.
  hyperv: port rest of the driver to new stucts.
  hyperv: add hypervInitConnection.
  hyperv: update driver documentation.
  news: update for Hyper-V 2012+ support.

 docs/drvhyperv.html.in|   2 +-
 docs/news.xml |  11 +
 src/Makefile.am   |   2 -
 src/hyperv/hyperv_driver.c| 168 ---
 src/hyperv/hyperv_private.h   |   8 +-
 src/hyperv/hyperv_wmi.c   | 173 +++
 src/hyperv/hyperv_wmi.h   |  56 -
 src/hyperv/hyperv_wmi_classes.h   |  30 +++
 src/hyperv/hyperv_wmi_generator.input | 239 ++---
 src/hyperv/hyperv_wmi_generator.py| 385 +-
 10 files changed, 822 insertions(+), 252 deletions(-)

-- 
2.9.3

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

[libvirt] [PATCH v3 11/12] hyperv: update driver documentation.

[Dawid Zamirski]

---
 docs/drvhyperv.html.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/drvhyperv.html.in b/docs/drvhyperv.html.in
index 7acf86f..e87d8cb 100644
--- a/docs/drvhyperv.html.in
+++ b/docs/drvhyperv.html.in
@@ -5,7 +5,7 @@
 Microsoft Hyper-V hypervisor driver
 
 
-The libvirt Microsoft Hyper-V driver can manage Hyper-V 2008 R2.
+The libvirt Microsoft Hyper-V driver can manage Hyper-V 2008 R2 and 
newer.
 
 
 
-- 
2.9.3

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

[libvirt] [PATCH v3 09/12] hyperv: port rest of the driver to new stucts.

[Dawid Zamirski]

basically s/data->/data.common->/

Because the data member of hypervObject is a union, get the data via
the "common" member everywhere - existing driver does not require
special handling for v1 vs v2 separately.
---
 src/hyperv/hyperv_driver.c | 76 +++---
 src/hyperv/hyperv_wmi.c| 21 -
 2 files changed, 51 insertions(+), 46 deletions(-)

diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index b642a02..5d01c1f 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -240,7 +240,7 @@ hypervConnectGetHostname(virConnectPtr conn)
 goto cleanup;
 }
 
-ignore_value(VIR_STRDUP(hostname, computerSystem->data->DNSHostName));
+ignore_value(VIR_STRDUP(hostname, 
computerSystem->data.common->DNSHostName));
 
  cleanup:
 hypervFreeObject(priv, (hypervObject *)computerSystem);
@@ -282,7 +282,7 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
   "{Win32_ComputerSystem.Name=\"%s\"} "
   "where AssocClass = Win32_ComputerSystemProcessor "
   "ResultClass = Win32_Processor",
-  computerSystem->data->Name);
+  computerSystem->data.common->Name);
 
 if (hypervGetWin32ProcessorList(priv, , ) < 0)
 goto cleanup;
@@ -295,7 +295,7 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
 }
 
 /* Strip the string to fit more relevant information in 32 chars */
-tmp = processorList->data->Name;
+tmp = processorList->data.common->Name;
 
 while (*tmp != '\0') {
 if (STRPREFIX(tmp, "  ")) {
@@ -313,16 +313,16 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
 }
 
 /* Fill struct */
-if (virStrncpy(info->model, processorList->data->Name,
+if (virStrncpy(info->model, processorList->data.common->Name,
sizeof(info->model) - 1, sizeof(info->model)) == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("CPU model %s too long for destination"),
-   processorList->data->Name);
+   processorList->data.common->Name);
 goto cleanup;
 }
 
-info->memory = computerSystem->data->TotalPhysicalMemory / 1024; /* byte 
to kilobyte */
-info->mhz = processorList->data->MaxClockSpeed;
+info->memory = computerSystem->data.common->TotalPhysicalMemory / 1024; /* 
byte to kilobyte */
+info->mhz = processorList->data.common->MaxClockSpeed;
 info->nodes = 1;
 info->sockets = 0;
 
@@ -331,8 +331,8 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
 ++info->sockets;
 }
 
-info->cores = processorList->data->NumberOfCores;
-info->threads = info->cores / 
processorList->data->NumberOfLogicalProcessors;
+info->cores = processorList->data.common->NumberOfCores;
+info->threads = info->cores / 
processorList->data.common->NumberOfLogicalProcessors;
 info->cpus = info->sockets * info->cores;
 
 result = 0;
@@ -372,7 +372,7 @@ hypervConnectListDomains(virConnectPtr conn, int *ids, int 
maxids)
 
 for (computerSystem = computerSystemList; computerSystem != NULL;
  computerSystem = computerSystem->next) {
-ids[count++] = computerSystem->data->ProcessID;
+ids[count++] = computerSystem->data.common->ProcessID;
 
 if (count >= maxids)
 break;
@@ -532,7 +532,7 @@ hypervDomainSuspend(virDomainPtr domain)
 if (hypervMsvmComputerSystemFromDomain(domain, ) < 0)
 goto cleanup;
 
-if (computerSystem->data->EnabledState !=
+if (computerSystem->data.common->EnabledState !=
 MSVM_COMPUTERSYSTEM_ENABLEDSTATE_ENABLED) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("Domain is not active"));
@@ -560,7 +560,7 @@ hypervDomainResume(virDomainPtr domain)
 if (hypervMsvmComputerSystemFromDomain(domain, ) < 0)
 goto cleanup;
 
-if (computerSystem->data->EnabledState !=
+if (computerSystem->data.common->EnabledState !=
 MSVM_COMPUTERSYSTEM_ENABLEDSTATE_PAUSED) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("Domain is not paused"));
@@ -666,7 +666,7 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr 
info)
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not lookup %s for domain %s"),
"Msvm_VirtualSystemSettingData",
-   computerSystem->data->ElementName);
+   computerSystem->data.common->ElementName);
 goto cleanup;
 }
 
@@ -676,7 +676,7 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr 
info)
   "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
   "where AssocClass = 
Msvm_VirtualSystemSettingDataComponent "
   "ResultClass = Msvm_ProcessorSettingData",
-

Re: [libvirt] [PATCH 0/5] qemu: fix individual vcpu hotplug code

[John Ferlan]

On 03/31/2017 07:52 AM, Peter Krempa wrote:
> Resolve a few corner cases which would create invalid configurations or 
> produce
> bad error messages.
> 
> Peter Krempa (5):
>   qemu: hotplug: Iterate over vcpu 0 in individual vcpu hotplug code
>   qemu: hotplug: Fix formatting strings in
> qemuDomainFilterHotplugVcpuEntities
>   qemu: hotplug: Clear vcpu ordering for coldplug of vcpus
>   qemu: hotplug: Add validation for coldplug of individual vcpus
>   qemu: hotplug: Validate that vcpu-hotplug does not break config
> 
>  src/qemu/qemu_hotplug.c | 73 
> +++--
>  1 file changed, 65 insertions(+), 8 deletions(-)
> 

ACK series (left a note on patch 4)

John

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

Re: [libvirt] [PATCH 4/5] qemu: hotplug: Add validation for coldplug of individual vcpus

[John Ferlan]

On 03/31/2017 07:52 AM, Peter Krempa wrote:
> Validate that users don't try to disable vcpu 0 and reject attempt to
> modify a vcpu to the state it is currently in.
> ---
>  src/qemu/qemu_hotplug.c | 36 
>  1 file changed, 36 insertions(+)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 5488b1dd4..18a8df33a 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -5875,6 +5875,37 @@ qemuDomainFilterHotplugVcpuEntities(virDomainDefPtr 
> def,
>  }
> 
> 
> +static int
> +qemuDomainVcpuValidateConfig(virDomainDefPtr def,
> + virBitmapPtr map,
> + bool state)
> +{
> +virDomainVcpuDefPtr vcpu;
> +ssize_t next = -1;
> +
> +/* vcpu 0 can't be disabled */
> +if (!state && virBitmapIsBitSet(map, 0)) {
> +virReportError(VIR_ERR_INVALID_ARG, "%s",
> +   _("vCPU '0' must be enabled"));
> +return -1;
> +}
> +
> +/* make sure that all selected vcpus are in the correct state */
> +while ((next = virBitmapNextSetBit(map, next)) >= 0) {
> +if (!(vcpu = virDomainDefGetVcpu(def, next)))
> +continue;
> +
> +if (vcpu->online == state) {
> +virReportError(VIR_ERR_INVALID_ARG,
> +   _("vcpu '%zd' is already in requested state"), 
> next);
> +return -1;
> +}

Does this really matter for this path? (config file). Wouldn't they just
be changing to what they already have and is that really a big deal.
IDC either way, but since there was no bz attached to this patch, I was
just curious why the check.

John

> +}
> +
> +return 0;
> +}
> +
> +
>  int
>  qemuDomainSetVcpuInternal(virQEMUDriverPtr driver,
>virDomainObjPtr vm,
> @@ -5909,6 +5940,11 @@ qemuDomainSetVcpuInternal(virQEMUDriverPtr driver,
>  }
>  }
> 
> +if (persistentDef) {
> +if (qemuDomainVcpuValidateConfig(persistentDef, map, state) < 0)
> +goto cleanup;
> +}
> +
>  if (livevcpus &&
>  qemuDomainSetVcpusLive(driver, cfg, vm, livevcpus, state) < 0)
>  goto cleanup;
> 

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

Re: [libvirt] [PATCH v2 00/10] hyperv: add support for Hyper-V 2012 and newer.

[Matthias Bolte]

2017-04-02 6:50 GMT+02:00 Dawid Zamirski :
> Hello,
>
> This is v2 with all the code review feedback addressed. Please see the
> cover letter in v1 here:
> https://www.redhat.com/archives/libvir-list/2017-March/msg01624.html
>
> Changes in v2:
> * checked each patch with make syntax-check individually
> * fixed missing notices in hyperv_wmi_generator.py
> * addressed an issue where a case of "v2-only" WMI classes was not
>   handled properly.

There is one final question one patch 9/10.

One thing that's missing is a potential documentation update (see
docs/drvhyperv.html.in) and an addition to the news file (see
docs/news.xml)

-- 
Matthias Bolte
http://photron.blogspot.com

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

Re: [libvirt] [PATCH v3 09/10] hyperv: port rest of the driver to new stucts.

[Matthias Bolte]

2017-04-03 19:00 GMT+02:00 Dawid Zamirski :
> basically s/data->/data.common->/
>
> Because the data member of hypervObject is a union, get the data via
> the "common" member everywhere - existing driver does not require
> special handling for v1 vs v2 separately.
> ---
>
> In v3:
> * solved conflict with recently pushed 5683b21309
>
>  src/hyperv/hyperv_driver.c | 76 
> +++---
>  src/hyperv/hyperv_wmi.c| 21 -
>  2 files changed, 51 insertions(+), 46 deletions(-)

> diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
> index c2d64ba..cdfb55f 100644
> --- a/src/hyperv/hyperv_wmi.c
> +++ b/src/hyperv/hyperv_wmi.c
> @@ -439,6 +439,8 @@ 
> hypervInvokeMsvmComputerSystemRequestStateChange(virDomainPtr domain,
>  virBuffer query = VIR_BUFFER_INITIALIZER;
>  Msvm_ConcreteJob *concreteJob = NULL;
>  bool completed = false;
> +const char *resourceUri = MSVM_COMPUTERSYSTEM_V2_RESOURCE_URI;

Shouldn't this say v1 ...

> @@ -447,6 +449,9 @@ 
> hypervInvokeMsvmComputerSystemRequestStateChange(virDomainPtr domain,
>  virAsprintf(, "RequestedState=%d", requestedState) < 0)
>  goto cleanup;
>
> +if (priv->wmiVersion == HYPERV_WMI_VERSION_V1)
> +resourceUri = MSVM_COMPUTERSYSTEM_V2_RESOURCE_URI;

... and then you should check here for HYPERV_WMI_VERSION_V2 and
change it to v2?

No need to send a new patch version to fix this. I already have the
patches 4-10 merged and ready to be pushed. I'll just incorporate any
necessary change before pushing.

-- 
Matthias Bolte
http://photron.blogspot.com

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

Re: [libvirt] [PATCH v2] virNetDevIPCheckIPv6ForwardingCallback fixes

[John Ferlan]

On 03/28/2017 10:00 AM, Cédric Bosdonnat wrote:
> Add check for more than one RTA_OIF, even though this is rather
> unlikely.
> 
> Get rid of the buggy switch / break as this code won't need to
> handle more attributes.
> 
> Use VIR_WARNINGS_NO_CAST_ALIGN to fix impossible to fix
> util/virnetdevip.c:560:17: error: cast increases required alignment of target 
> type [-Werror=cast-align]
> ---
>  Diff to v1:
>* Add error message
>* Use VIR_WARNINGS_NO_CAST_ALIGN
>  src/util/virnetdevip.c | 17 +
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 

ACK (and pushed)

John

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

[libvirt] [PATCH v3 09/10] hyperv: port rest of the driver to new stucts.

[Dawid Zamirski]

basically s/data->/data.common->/

Because the data member of hypervObject is a union, get the data via
the "common" member everywhere - existing driver does not require
special handling for v1 vs v2 separately.
---

In v3:
* solved conflict with recently pushed 5683b21309

 src/hyperv/hyperv_driver.c | 76 +++---
 src/hyperv/hyperv_wmi.c| 21 -
 2 files changed, 51 insertions(+), 46 deletions(-)

diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index b642a02..5d01c1f 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -240,7 +240,7 @@ hypervConnectGetHostname(virConnectPtr conn)
 goto cleanup;
 }
 
-ignore_value(VIR_STRDUP(hostname, computerSystem->data->DNSHostName));
+ignore_value(VIR_STRDUP(hostname, 
computerSystem->data.common->DNSHostName));
 
  cleanup:
 hypervFreeObject(priv, (hypervObject *)computerSystem);
@@ -282,7 +282,7 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
   "{Win32_ComputerSystem.Name=\"%s\"} "
   "where AssocClass = Win32_ComputerSystemProcessor "
   "ResultClass = Win32_Processor",
-  computerSystem->data->Name);
+  computerSystem->data.common->Name);
 
 if (hypervGetWin32ProcessorList(priv, , ) < 0)
 goto cleanup;
@@ -295,7 +295,7 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
 }
 
 /* Strip the string to fit more relevant information in 32 chars */
-tmp = processorList->data->Name;
+tmp = processorList->data.common->Name;
 
 while (*tmp != '\0') {
 if (STRPREFIX(tmp, "  ")) {
@@ -313,16 +313,16 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
 }
 
 /* Fill struct */
-if (virStrncpy(info->model, processorList->data->Name,
+if (virStrncpy(info->model, processorList->data.common->Name,
sizeof(info->model) - 1, sizeof(info->model)) == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("CPU model %s too long for destination"),
-   processorList->data->Name);
+   processorList->data.common->Name);
 goto cleanup;
 }
 
-info->memory = computerSystem->data->TotalPhysicalMemory / 1024; /* byte 
to kilobyte */
-info->mhz = processorList->data->MaxClockSpeed;
+info->memory = computerSystem->data.common->TotalPhysicalMemory / 1024; /* 
byte to kilobyte */
+info->mhz = processorList->data.common->MaxClockSpeed;
 info->nodes = 1;
 info->sockets = 0;
 
@@ -331,8 +331,8 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
 ++info->sockets;
 }
 
-info->cores = processorList->data->NumberOfCores;
-info->threads = info->cores / 
processorList->data->NumberOfLogicalProcessors;
+info->cores = processorList->data.common->NumberOfCores;
+info->threads = info->cores / 
processorList->data.common->NumberOfLogicalProcessors;
 info->cpus = info->sockets * info->cores;
 
 result = 0;
@@ -372,7 +372,7 @@ hypervConnectListDomains(virConnectPtr conn, int *ids, int 
maxids)
 
 for (computerSystem = computerSystemList; computerSystem != NULL;
  computerSystem = computerSystem->next) {
-ids[count++] = computerSystem->data->ProcessID;
+ids[count++] = computerSystem->data.common->ProcessID;
 
 if (count >= maxids)
 break;
@@ -532,7 +532,7 @@ hypervDomainSuspend(virDomainPtr domain)
 if (hypervMsvmComputerSystemFromDomain(domain, ) < 0)
 goto cleanup;
 
-if (computerSystem->data->EnabledState !=
+if (computerSystem->data.common->EnabledState !=
 MSVM_COMPUTERSYSTEM_ENABLEDSTATE_ENABLED) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("Domain is not active"));
@@ -560,7 +560,7 @@ hypervDomainResume(virDomainPtr domain)
 if (hypervMsvmComputerSystemFromDomain(domain, ) < 0)
 goto cleanup;
 
-if (computerSystem->data->EnabledState !=
+if (computerSystem->data.common->EnabledState !=
 MSVM_COMPUTERSYSTEM_ENABLEDSTATE_PAUSED) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("Domain is not paused"));
@@ -666,7 +666,7 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr 
info)
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not lookup %s for domain %s"),
"Msvm_VirtualSystemSettingData",
-   computerSystem->data->ElementName);
+   computerSystem->data.common->ElementName);
 goto cleanup;
 }
 
@@ -676,7 +676,7 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr 
info)
   "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
   "where AssocClass = 
Msvm_VirtualSystemSettingDataComponent "

Re: [libvirt] [PATCH 2/2] news: Allow empty elements

[Daniel P. Berrange]

On Thu, Mar 30, 2017 at 03:45:27PM +0200, Andrea Bolognani wrote:
> Creating dummy  elements was a workaround for the
> HTML DTD not allowing empty  elements, but we can do
> better by tweaking the the XSLT stylesheet.
> ---
>  docs/news-html.xsl|  8 +---
>  docs/news.xml |  9 -
>  docs/schemas/news.rng | 17 -
>  3 files changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/docs/news-html.xsl b/docs/news-html.xsl
> index dcbab86..dd323f9 100644
> --- a/docs/news-html.xsl
> +++ b/docs/news-html.xsl
> @@ -57,9 +57,11 @@
>
>  
>
> -  
> -
> -  
> +  
> +
> +  
> +
> +  
>  
>
>  
> diff --git a/docs/news.xml b/docs/news.xml
> index 9eb4d21..732d359 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -23,19 +23,10 @@
>  
>
>  
> -  
> -
> -  
>  
>  
> -  
> -
> -  
>  
>  
> -  
> -
> -  
>  
>
>  
> diff --git a/docs/schemas/news.rng b/docs/schemas/news.rng
> index 94a6870..9212c3c 100644
> --- a/docs/schemas/news.rng
> +++ b/docs/schemas/news.rng
> @@ -35,21 +35,20 @@
>
>  
>
> -  
> -
> -  
> +  
> +
> +  
> +
> +  
>  
>
>  
>
>  
>
> -
> -  
> -\n[^\n]+\n +
> -  
> -  
> -
> +
> +  \n[^\n]+\n +
> +
>
>
>  

FYI, it looks like something in this change has broken schema validation
on RHEL-6 vintage libxml

https://ci.centos.org/view/libvirt/job/libvirt-master-check/systems=libvirt-centos-6/298/console

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 2/3] qemu: Initialize 'data' argument

[John Ferlan]

Initialize stack variable to {0}

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e1ad243..388af4f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10265,7 +10265,7 @@ qemuDomainGetSchedulerParametersFlags(virDomainPtr dom,
 {
 virQEMUDriverPtr driver = dom->conn->privateData;
 virDomainObjPtr vm = NULL;
-virDomainCputune data;
+virDomainCputune data = {0};
 int ret = -1;
 bool cpu_bw_status = true;
 virDomainDefPtr persistentDef;
-- 
2.9.3

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

[libvirt] [PATCH 1/3] tests: Pass BlockIOThrottle arguments by reference not value

[John Ferlan]

Pass the data by reference rather than everything on the stack.

Signed-off-by: John Ferlan 
---
 tests/qemumonitorjsontest.c | 38 +++---
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 5d53609..ac87c9c 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -2035,27 +2035,27 @@ testQemuMonitorJSONqemuMonitorJSONGetChardevInfo(const 
void *data)
 
 
 static int
-testValidateGetBlockIoThrottle(virDomainBlockIoTuneInfo info,
-   virDomainBlockIoTuneInfo expectedInfo)
+testValidateGetBlockIoThrottle(const virDomainBlockIoTuneInfo *info,
+   const virDomainBlockIoTuneInfo *expectedInfo)
 {
 #define VALIDATE_IOTUNE(field) \
-if (info.field != expectedInfo.field) { \
+if (info->field != expectedInfo->field) { \
 virReportError(VIR_ERR_INTERNAL_ERROR, \
-   "info.%s=%llu != expected=%llu",  \
-   #field, info.field, expectedInfo.field); \
+   "info->%s=%llu != expected=%llu",  \
+   #field, info->field, expectedInfo->field); \
 return -1; \
 } \
-if (info.field##_max != expectedInfo.field##_max) { \
+if (info->field##_max != expectedInfo->field##_max) { \
 virReportError(VIR_ERR_INTERNAL_ERROR, \
-   "info.%s_max=%llu != expected=%llu",  \
-   #field, info.field##_max, expectedInfo.field##_max); \
+   "info->%s_max=%llu != expected=%llu",  \
+   #field, info->field##_max, expectedInfo->field##_max); \
 return -1; \
 } \
-if (info.field##_max_length != expectedInfo.field##_max_length) { \
+if (info->field##_max_length != expectedInfo->field##_max_length) { \
 virReportError(VIR_ERR_INTERNAL_ERROR, \
-   "info.%s_max_length=%llu != expected=%llu",  \
-   #field, info.field##_max_length, \
-   expectedInfo.field##_max_length); \
+   "info->%s_max_length=%llu != expected=%llu",  \
+   #field, info->field##_max_length, \
+   expectedInfo->field##_max_length); \
 return -1; \
 }
 VALIDATE_IOTUNE(total_bytes_sec);
@@ -2064,16 +2064,16 @@ testValidateGetBlockIoThrottle(virDomainBlockIoTuneInfo 
info,
 VALIDATE_IOTUNE(total_iops_sec);
 VALIDATE_IOTUNE(read_iops_sec);
 VALIDATE_IOTUNE(write_iops_sec);
-if (info.size_iops_sec != expectedInfo.size_iops_sec) {
+if (info->size_iops_sec != expectedInfo->size_iops_sec) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   "info.size_iops_sec=%llu != expected=%llu",
-   info.size_iops_sec, expectedInfo.size_iops_sec);
+   "info->size_iops_sec=%llu != expected=%llu",
+   info->size_iops_sec, expectedInfo->size_iops_sec);
 return -1;
 }
-if (STRNEQ(info.group_name, expectedInfo.group_name)) {
+if (STRNEQ(info->group_name, expectedInfo->group_name)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   "info.group_name=%s != expected=%s",
-   info.group_name, expectedInfo.group_name);
+   "info->group_name=%s != expected=%s",
+   info->group_name, expectedInfo->group_name);
 return -1;
 }
 #undef VALIDATE_IOTUNE
@@ -2121,7 +2121,7 @@ 
testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data)
   "drive-virtio-disk0", ) < 0)
 goto cleanup;
 
-if (testValidateGetBlockIoThrottle(info, expectedInfo) < 0)
+if (testValidateGetBlockIoThrottle(, ) < 0)
 goto cleanup;
 
 if (qemuMonitorJSONSetBlockIoThrottle(qemuMonitorTestGetMonitor(test),
-- 
2.9.3

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

[libvirt] [PATCH 0/3] A couple of Coverity found issues

[John Ferlan]

There's more, but many are false positives and there's also a couple
more that I have to think more deeply about...

John Ferlan (3):
  tests: Pass BlockIOThrottle arguments by reference not value
  qemu: Initialize 'data' argument
  qemu: Fix resource leak in qemuDomainAddChardevTLSObjects error path

 src/qemu/qemu_driver.c  |  2 +-
 src/qemu/qemu_migration.c   | 11 ---
 tests/qemumonitorjsontest.c | 38 +++---
 3 files changed, 28 insertions(+), 23 deletions(-)

-- 
2.9.3

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

[libvirt] [PATCH 3/3] qemu: Fix resource leak in qemuDomainAddChardevTLSObjects error path

[John Ferlan]

On any failure, call virJSONValueFree for the *Props.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_migration.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 87d7dcd..852d85b 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -213,7 +213,7 @@ qemuMigrationAddTLSObjects(virQEMUDriverPtr driver,
 cfg->migrateTLSx509verify,
 QEMU_MIGRATION_TLS_ALIAS_BASE,
 , tlsAlias, , secAlias) < 0)
-return -1;
+goto error;
 
 /* Ensure the domain doesn't already have the TLS objects defined...
  * This should prevent any issues just in case some cleanup wasn't
@@ -223,12 +223,17 @@ qemuMigrationAddTLSObjects(virQEMUDriverPtr driver,
 
 if (qemuDomainAddTLSObjects(driver, vm, asyncJob, *secAlias, ,
 *tlsAlias, ) < 0)
-return -1;
+goto error;
 
 if (VIR_STRDUP(migParams->migrateTLSAlias, *tlsAlias) < 0)
-return -1;
+goto error;
 
 return 0;
+
+ error:
+virJSONValueFree(tlsProps);
+virJSONValueFree(secProps);
+return -1;
 }
 
 
-- 
2.9.3

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

Re: [libvirt] [PATCH v2 5/5] storage: gluster: Use volume name as "" field in the XML

[Andrea Bolognani]

On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote:
[...]
> +if (netfs) {
> +src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS;
> +src->dir = volname;
> +} else {
> +src->name = volname;
> +
> +if (VIR_STRDUP(src->dir, "/") < 0)
> +goto cleanup;

I'll admit I didn't dig, but is hardcoding "/" here the
right thing to do?

Assuming it is because some other code takes care of setting
it to a different value when needed,

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2 4/5] storage: Fix XPath for looking up gluster volume name

[Andrea Bolognani]

On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote:
> Use the relative lookup specifier rather than the global one. Otherwise
> only the first name would be looked up. Add a test case to cover the
> scenario.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1436574

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2 2/5] storage: util: Split out the gluster volume extraction code into new function

[Andrea Bolognani]

On Mon, 2017-04-03 at 16:38 +0200, Peter Krempa wrote:
> > > +int virStorageUtilGlusterExtractPoolSources(const char *host,
> > > +const char *xml,
> > > +virStoragePoolSourceListPtr 
> > > list,
> > > +bool netfs);
> > 
> > Please add a comment along the lines of "For test suite use
> > only" here. Ideally we'd have a separate *priv.h header file
> > to be used for the purpose, but that's out of scope.
> 
> Why? This function can be used wherever it's necessary. It's by no-means
> specific to the test suite.

It would probably be a static function, or would never have
been ripped out of virStorageBackendFindGlusterPoolSources()
to begin with, were not for the test suite, so I'd say the
comment would be warranted until a non-theoretical use for
it is found outside of the test suite.

If you don't feel the same way, you can safely disregard my
remark though.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2 3/5] test: Introduce testing of virStorageUtilGlusterExtractPoolSources

[Andrea Bolognani]

On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote:
[...]
> @@ -352,7 +354,7 @@ test_programs += nwfilterxml2firewalltest
>  endif WITH_NWFILTER
> 
>  if WITH_STORAGE
> -test_programs += storagevolxml2argvtest
> +test_programs += storagevolxml2argvtest virstorageutiltest

Since you have to touch these lines regardless, please turn
this...

[...]
> @@ -859,6 +861,13 @@ genericxml2xmltest_LDADD = $(LDADDS)
> 
> 
>  if WITH_STORAGE
> +virstorageutiltest_SOURCES = \
> + virstorageutiltest.c testutils.c testutils.h

... and this ...

> +virstorageutiltest_LDADD = \
> + ../src/libvirt_driver_storage_impl.la \
> + $(LDADDS) \
> + $(NULL)
> +
>  storagevolxml2argvtest_SOURCES = \
>  storagevolxml2argvtest.c \
>  testutils.c testutils.h
> @@ -867,7 +876,7 @@ storagevolxml2argvtest_LDADD = \
>   ../src/libvirt_driver_storage_impl.la $(LDADDS)
> 
>  else ! WITH_STORAGE
> -EXTRA_DIST += storagevolxml2argvtest.c
> +EXTRA_DIST += storagevolxml2argvtest.c virstorageutiltest.c

... and finally this into proper, one-file-per-line,
$(NULL)-terminated file lists as a courtesy to the next
developer :)

You should also be able to use $(qemu_LDADDS) instead of
spelling out ../src/libvirt_driver_storage_impl.la above.

[...]
> +   uuid="a6f5ddea-bc6a-44db-ae1d-5aa1db743490">virt-gluster-node1:/bricks/brick1/brickvirt-gluster-
node1:/bricks/brick1/bricka6f5ddea-bc6a-44db-ae1d-5aa1db7434900
> +   uuid="f4ab9fb1-44ec-443b-8783-e5f70ed78da3">virt-gluster-node2:/bricks/brick1/brickvirt-gluster-
node2:/bricks/brick1/brickf4ab9fb1-44ec-443b-8783-e5f70ed78da30

Does Gluster's  element really contain a bare text
node followed by a bunch of other elements? Ewww.

[...]
> +#define DO_TEST_GLUSTER_LOOKUP_FULL(testname, sffx, testnetfs, pooltype) 
>   \
> +do { 
>   \
> +struct testGlusterLookupParseData data;  
>   \
> +data.srcxml = abs_srcdir "/virstorageutildata/"  
>   \
> +  "gluster-parse-" testname "-src.xml";  
>   \
> +data.dstxml = abs_srcdir "/virstorageutildata/"  
>   \
> +  "gluster-parse-" testname "-" sffx  ".xml";
>   \

There's a spurious space between sffx and ".xml".

> +data.netfs = testnetfs;  
>   \
> +data.type = pooltype;
>   \
> +if (virTestRun("gluster lookup " sffx " " testname,  
>   \
> +   testGlusterLookupParse, ) < 0)   
>   \
> +ret = -1;
>   \
> +} while (0)
> +
> +#define DO_TEST_GLUSTER_LOOKUP_NATIVE(testname)  
>   \
> +DO_TEST_GLUSTER_LOOKUP_FULL(testname, "native", false, 
> VIR_STORAGE_POOL_GLUSTER)
> +#define DO_TEST_GLUSTER_LOOKUP_NETFS(testname)   
>   \
> +DO_TEST_GLUSTER_LOOKUP_FULL(testname, "netfs", true, 
> VIR_STORAGE_POOL_NETFS)
> +
> +DO_TEST_GLUSTER_LOOKUP_NATIVE("basic");
> +DO_TEST_GLUSTER_LOOKUP_NETFS("basic");

Between GLUSTER_LOOKUP, gluster-parse and
GlusterLookupParse the naming is a bit all over the place.

Do you think you could make it more consistent? Especially
since none of those seems to have any direct connection with
virStorageUtilGlusterExtractPoolSources(), the API being
tested.

[...]
> +#undef DO_TEST_GLUSTER_LOOKUP_NATIVE
> +#undef DO_TEST_GLUSTER_LOOKUP_NETFS
> +#undef DO_TEST_GLUSTER_LOOKUP_FULL

The #undefs are a bit unnecessary in this context, I'd leave
them out.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] qemu: Add device id for mediated devices on qemu command line

[Peter Krempa]

On Mon, Apr 03, 2017 at 16:12:17 +0200, Erik Skultety wrote:
> Like all devices, add the 'id' option for mdevs as well. Patch also
> adjusts the test accordingly.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1438431
> 
> Signed-off-by: Erik Skultety 
> ---
>  src/qemu/qemu_command.c  | 3 ++-
>  tests/qemuxml2argvdata/qemuxml2argv-hostdev-mdev-precreated.args | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)

ACK


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

Re: [libvirt] [PATCH v2 2/5] storage: util: Split out the gluster volume extraction code into new function

[Peter Krempa]

On Mon, Apr 03, 2017 at 16:20:26 +0200, Andrea Bolognani wrote:
> On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote:
> [...]
> > @@ -93,6 +93,10 @@ int virStorageBackendDeleteLocal(virConnectPtr conn,
> >  int virStorageBackendRefreshLocal(virConnectPtr conn,
> >virStoragePoolObjPtr pool);
> > 
> > +int virStorageUtilGlusterExtractPoolSources(const char *host,
> > +const char *xml,
> > +virStoragePoolSourceListPtr 
> > list,
> > +bool netfs);
> 
> Please add a comment along the lines of "For test suite use
> only" here. Ideally we'd have a separate *priv.h header file
> to be used for the purpose, but that's out of scope.

Why? This function can be used wherever it's necessary. It's by no-means
specific to the test suite.


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

Re: [libvirt] [PATCH v2 2/5] storage: util: Split out the gluster volume extraction code into new function

[Andrea Bolognani]

On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote:
[...]
> @@ -93,6 +93,10 @@ int virStorageBackendDeleteLocal(virConnectPtr conn,
>  int virStorageBackendRefreshLocal(virConnectPtr conn,
>virStoragePoolObjPtr pool);
> 
> +int virStorageUtilGlusterExtractPoolSources(const char *host,
> +const char *xml,
> +virStoragePoolSourceListPtr list,
> +bool netfs);

Please add a comment along the lines of "For test suite use
only" here. Ideally we'd have a separate *priv.h header file
to be used for the purpose, but that's out of scope.


ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2 0/2] Fix possible use-after-free when sending event message

[John Ferlan]

On 03/27/2017 12:47 PM, John Ferlan wrote:
> v1: https://www.redhat.com/archives/libvir-list/2017-March/msg01228.html
> 
> Change since v1, add the derefFcn as an argument to the renamed macro
> (not quite sure how I missed that originally.
> 
> John Ferlan (2):
>   daemon: Rework remoteClientFreeFunc cleanup loops into C macro
>   remote: Fix possible use-after-free when sending event message
> 
>  daemon/remote.c | 164 
> 
>  1 file changed, 58 insertions(+), 106 deletions(-)
> 


Laine took a look at patch 1/2 - anyone want to look at 2/2 which he
didn't feel comfortable looking at?

Essentially it follows similar logic to virObjectEventCallbackListAddID
when processing virObjectRef(conn), except this time the virObjectRef is
on virNetServerClientPtr client whenever the callback functions grab
it's address.  When the callback is free'd the reference is removed (in
remoteEventCallbackFree) so that virNetServerProcessClients doesn't
inadvertently free the client before the callback code is done with it
(sending an event message).

Tks -

John

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

[libvirt] [PATCH] qemu: Add device id for mediated devices on qemu command line

[Erik Skultety]

Like all devices, add the 'id' option for mdevs as well. Patch also
adjusts the test accordingly.

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

Signed-off-by: Erik Skultety 
---
 src/qemu/qemu_command.c  | 3 ++-
 tests/qemuxml2argvdata/qemuxml2argv-hostdev-mdev-precreated.args | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 5cf383aaea..445c696d6e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5231,7 +5231,8 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
 char *ret = NULL;
 
 virBufferAddLit(, "vfio-pci");
-virBufferAsprintf(, ",sysfsdev=%s",
+virBufferAsprintf(, ",id=%s,sysfsdev=%s",
+  dev->info->alias,
   virMediatedDeviceGetSysfsPath(mdevsrc->uuidstr));
 
 if (qemuBuildDeviceAddressStr(, def, dev->info, qemuCaps) < 0)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-mdev-precreated.args 
b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-mdev-precreated.args
index fdefeb6104..76e77707bf 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-mdev-precreated.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-mdev-precreated.args
@@ -20,6 +20,6 @@ QEMU_AUDIO_DRV=none \
 -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 vfio-pci,\
+-device vfio-pci,id=hostdev0,\
 sysfsdev=/sys/bus/mdev/devices/53764d0e-85a0-42b4-af5c-2046b460b1dc,bus=pci.0,\
 addr=0x3
-- 
2.12.2

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

Re: [libvirt] [PATCH 0/5] Split out network object into its own module

[John Ferlan]

ping?

Laine only had a question about the altered format for network_conf.h
files.  I don't think there's been enough change to cause any merge
conflicts with top of tree, but I can repost if necessary.

I have a few more ready to roll as well in the continuing effort to make
a common driver object model. I'm into the object code now and would
like to be sure the "names" I choose are well received before I get too
far ahead and need to change everything.

Tks -

John

On 03/18/2017 09:26 AM, John Ferlan wrote:
> Reached the last of the code from my RFC for making a common pool
> object - networkobj...  For reference see patch 3 of:
> 
> http://www.redhat.com/archives/libvir-list/2017-February/msg00519.html
> 
> This series works through the network conf adjustments. This pile is
> fairly straightforward, like other series we split out anything referencing
> virNetworkPoolObj and go from there. There was one small detour to avoid
> a potentially conflict in names.
> 
> John Ferlan (5):
>   conf: Introduce virnetworkobj
>   conf: Adjust coding style for network conf sources
>   conf: Alter coding style of network conf function prototypes
>   conf: Rename virNetworkObjAssignDef to virNetworkObjUpdateAssignDef
>   conf: Use consistent function name prefixes for virnetworkobj
> 
>  po/POTFILES.in   |1 +
>  src/Makefile.am  |3 +-
>  src/conf/network_conf.c  | 1387 
> ++
>  src/conf/network_conf.h  |  204 ++---
>  src/conf/virnetworkobj.c | 1371 +
>  src/conf/virnetworkobj.h |  185 +
>  src/libvirt_private.syms |   57 +-
>  src/network/bridge_driver.c  |   62 +-
>  src/network/bridge_driver.h  |2 +-
>  src/network/bridge_driver_platform.h |2 +-
>  src/test/test_driver.c   |   16 +-
>  11 files changed, 1762 insertions(+), 1528 deletions(-)
>  create mode 100644 src/conf/virnetworkobj.c
>  create mode 100644 src/conf/virnetworkobj.h
> 

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

Re: [libvirt] [PATCH v2 1/5] storage: util: Add boolean differentiating between gluster lookup type

[Andrea Bolognani]

On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote:
> The native gluster pool source list data differs from the data used for
> attaching gluster volumes as
netfs pools. Currently the only difference
> was the format. Since native pools don't use it and later there will be
> more difference add a boolean to swithc
between the types instead.

s/difference/differences/
s/swithc/switch/

[...]
> @@ -2839,18 +2839,21 @@ virStorageBackendDeleteLocal(virConnectPtr conn 
> ATTRIBUTE_UNUSED,
>  /**
>   * virStorageBackendFindGlusterPoolSources:
>   * @host: host to detect volumes on
> - * @pooltype: src->format is set to this value
>   * @list: list of storage pool sources to be filled
> + * @netfs: lookup will be used with netfs pools
>   * @report: report error if the 'gluster' cli tool is missing
>   *
>   * Looks up gluster volumes on @host and fills them to @list.
>   *
> + * If @netfs is specified the data is tweaked so that it can be used with 
> netfs
> + * type pools. Otherwise the data is for use with native gluster pools.
> + *
>   * Returns number of volumes on the host on success, or -1 on error.
>   */
>  int
>  virStorageBackendFindGlusterPoolSources(const char *host,
> -int pooltype,
>  virStoragePoolSourceListPtr list,
> +bool netfs,

I suggest using virStoragePoolType instead of bool here, eg.
callers will pass either VIR_STORAGE_POOL_GLUSTER for native
gluster pools or VIR_STORAGE_POOL_NETFS for netfs pools.
Passing any other value in the enumeration would of course
result in an error.

This would make the calling sites less opaque.

[...]
> @@ -2918,7 +2921,8 @@ virStorageBackendFindGlusterPoolSources(const char 
> *host,
>  if (VIR_STRDUP(src->hosts[0].name, host) < 0)
>  goto cleanup;
> 
> -src->format = pooltype;
> +if (netfs)
> +src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS;

In patch 5/5 you're going to move this chunk of code earlier
in the function while changing it: I suggest you move it in
this patch instead, so there's only a single code motion
instead of two.


ACK with the above suggestions addressed, or if you manage
to convince me that they're best left unaddressed :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [libvirt-perl][PATCH 2/3] Add virDomainSetBlockThreshold API

[Michal Privoznik]

On 04/03/2017 12:17 PM, Daniel P. Berrange wrote:

On Wed, Mar 29, 2017 at 03:56:28PM +0200, Michal Privoznik wrote:

Signed-off-by: Michal Privoznik 
---
 Changes|  1 +
 Virt.xs| 12 
 lib/Sys/Virt/Domain.pm |  9 +
 3 files changed, 22 insertions(+)

diff --git a/Changes b/Changes
index 4d6136f..3faaf08 100644
--- a/Changes
+++ b/Changes
@@ -12,6 +12,7 @@ Revision history for perl module Sys::Virt
  - Add PERF_PARAM_ALIGNMENT_FAULTS constant
  - Add PERF_PARAM_EMULATION_FAULTS constant
  - Add block threshold event
+ - Add virDomainSetBlockThreshold API

 3.1.0 2017-03-03

diff --git a/Virt.xs b/Virt.xs
index e0588f0..a05cf4d 100644
--- a/Virt.xs
+++ b/Virt.xs
@@ -6120,6 +6120,18 @@ send_process_signal(dom, pidsv, signum, flags=0)
   if (virDomainSendProcessSignal(dom, pid, signum, flags) < 0)
   _croak_error();

+void
+set_block_threshold(dom, dev, thresholdsv, flags=0)
+virDomainPtr dom;
+const char *dev;
+SV *thresholdsv;
+unsigned int flags;
+ PREINIT:
+unsigned long long threshold;
+  PPCODE:
+  threshold = virt_SvIVull(thresholdsv);
+  if (virDomainSetBlockThreshold(dom, dev, threshold, flags) < 0)
+  _croak_error();


nit-pick - the stuff below PPCODE shoudl be indented to the same level
as variable declarations. Yes, some pre-existing methods are inconsistent,
as editors get very confused by perl XS format indenting.


Ah, yeah. My vim did that. Thank you, I've fixed this and pushed all the 
patches.


Michal

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

Re: [libvirt] [libvirt-perl][PATCH 2/3] Add virDomainSetBlockThreshold API

[Daniel P. Berrange]

On Wed, Mar 29, 2017 at 03:56:28PM +0200, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  Changes|  1 +
>  Virt.xs| 12 
>  lib/Sys/Virt/Domain.pm |  9 +
>  3 files changed, 22 insertions(+)
> 
> diff --git a/Changes b/Changes
> index 4d6136f..3faaf08 100644
> --- a/Changes
> +++ b/Changes
> @@ -12,6 +12,7 @@ Revision history for perl module Sys::Virt
>   - Add PERF_PARAM_ALIGNMENT_FAULTS constant
>   - Add PERF_PARAM_EMULATION_FAULTS constant
>   - Add block threshold event
> + - Add virDomainSetBlockThreshold API
>  
>  3.1.0 2017-03-03
>  
> diff --git a/Virt.xs b/Virt.xs
> index e0588f0..a05cf4d 100644
> --- a/Virt.xs
> +++ b/Virt.xs
> @@ -6120,6 +6120,18 @@ send_process_signal(dom, pidsv, signum, flags=0)
>if (virDomainSendProcessSignal(dom, pid, signum, flags) < 0)
>_croak_error();
>  
> +void
> +set_block_threshold(dom, dev, thresholdsv, flags=0)
> +virDomainPtr dom;
> +const char *dev;
> +SV *thresholdsv;
> +unsigned int flags;
> + PREINIT:
> +unsigned long long threshold;
> +  PPCODE:
> +  threshold = virt_SvIVull(thresholdsv);
> +  if (virDomainSetBlockThreshold(dom, dev, threshold, flags) < 0)
> +  _croak_error();

nit-pick - the stuff below PPCODE shoudl be indented to the same level
as variable declarations. Yes, some pre-existing methods are inconsistent,
as editors get very confused by perl XS format indenting.

ACK

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] [libvirt-perl][PATCH 3/3] Add VIR_MIGRATE_TLS constant

[Daniel P. Berrange]

On Wed, Mar 29, 2017 at 03:56:29PM +0200, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  Changes| 1 +
>  Virt.xs| 1 +
>  lib/Sys/Virt/Domain.pm | 7 +++
>  3 files changed, 9 insertions(+)

ACK


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] [libvirt-perl][PATCH 1/3] Add block threshold event

[Daniel P. Berrange]

On Wed, Mar 29, 2017 at 03:56:27PM +0200, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  Changes|  1 +
>  Virt.xs| 48 
>  lib/Sys/Virt/Domain.pm |  6 ++
>  t/030-api-coverage.t   |  1 +
>  4 files changed, 56 insertions(+)

ACK


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] storage: Fix capacity value for LUKS encrypted volumes

[Daniel P. Berrange]

On Fri, Mar 24, 2017 at 10:56:38AM -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1371892
> 
> The 'capacity' value (e.g. guest logical size) for a LUKS volume is
> smaller than the 'physical' value of the file in the file system, so
> we need to account for that.
> 
> When peeking at the encryption information about the volume add a fetch
> of the payload_offset which is described as the offset to the start of
> the volume data (in 512 byte sectors) in QEMU's QCryptoBlockLUKSHeader.
> 
> Then adjust the ->capacity appropriately when we determine that the
> volume target encryption has a payload_offset value.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_util.c  |  3 +++
>  src/util/virstorageencryption.h |  1 +
>  src/util/virstoragefile.c   | 39 +++
>  3 files changed, 43 insertions(+)

ACK


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 v2 2/2] Debug: Report VM errors more concisely.

[Prerna Saxena]

Current logs:
error : qemuProcessFindDomainDiskByAlias:411 : internal error: no disk found 
with alias ide0-0-0

There is no way to find which VM was seeing this error.
Makes debugging very hard, and the message itself is no good.

Signed-off-by: Prerna Saxena 
---
 src/qemu/qemu_process.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e450d06..0a052e8 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -367,8 +367,8 @@ qemuProcessFindDomainDiskByAlias(virDomainObjPtr vm,
 }
 
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("no disk found with alias %s"),
-   alias);
+   _("VM %s: no disk found with alias %s"),
+   vm->def->name, alias);
 return NULL;
 }
 
-- 
1.8.1.2

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

[libvirt] [PATCH v2 1/2] Debug: Add DEBUG messages for when a client has opened/closed connection.

[Prerna Saxena]

While tracing connections from a remote client, it helps to keep track
of the connection lifecycle. Messages such as the following :

error : virNetSocketReadWire:1574 : End of file while reading data: 
Input/output error

are rather unhelpful. They do not indicate if the client had earlier asked for
connection closure via libvirt API.
This patch introduces messages to annotate when a client connected/disconnected.

Signed-off-by: Prerna Saxena 
---
 src/rpc/virnetserverclient.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 85857bc..24c9c33 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -964,8 +964,11 @@ void virNetServerClientClose(virNetServerClientPtr client)
 virNetServerClientCloseFunc cf;
 virKeepAlivePtr ka;
 
+VIR_DEBUG("Free'ing up resources for client=%p sock=%d", client,
+   virNetServerClientGetFD(client));
+
 virObjectLock(client);
-VIR_DEBUG("client=%p", client);
+
 if (!client->sock) {
 virObjectUnlock(client);
 return;
@@ -1039,10 +1042,14 @@ void 
virNetServerClientDelayedClose(virNetServerClientPtr client)
 virObjectLock(client);
 client->delayedClose = true;
 virObjectUnlock(client);
+VIR_DEBUG("Client=%p sock=%d requested closure of connection.",
+  client, virNetServerClientGetFD(client));
 }
 
 void virNetServerClientImmediateClose(virNetServerClientPtr client)
 {
+VIR_DEBUG("Client %p sock %d closed the connection", client,
+virNetServerClientGetFD(client));
 virObjectLock(client);
 client->wantClose = true;
 virObjectUnlock(client);
@@ -1151,6 +1158,7 @@ static void 
virNetServerClientDispatchRead(virNetServerClientPtr client)
 if (client->rx->nfds == 0) {
 if (virNetServerClientRead(client) < 0) {
 client->wantClose = true;
+VIR_WARN("Client=%p sock=%p closed connection", client, 
client->sock);
 return; /* Error */
 }
 }
-- 
1.8.1.2

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

[libvirt] [PATCH v2 0/2] Augment debug messages.

[Prerna Saxena]

This is a v2 of the previous set of debug enhancements
posted at : https://www.redhat.com/archives/libvir-list/2017-March/msg00959.html

Changelog:
-
1) Patch 1/3 : virNetSocketGetFD() is reverted to its old state,
and only new DEBUG messages are introduced.
2) Patch 2/3 : Dropped, per list suggestions.
3) Patch 3/3 : Same as v1

Prerna Saxena (2):
  Debug: Add DEBUG messages for when a client has opened/closed
connection.
  Debug: Report VM errors more concisely.

 src/qemu/qemu_process.c  |  4 ++--
 src/rpc/virnetserverclient.c | 10 +-
 2 files changed, 11 insertions(+), 3 deletions(-)

-- 
1.8.1.2

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

Re: [libvirt] [PATCH V3] util: Add more virsysfs functions for handling resctrl sysfs

[Martin Kletzander]

On Mon, Apr 03, 2017 at 09:29:37AM +0200, Erik Skultety wrote:

On Fri, Mar 31, 2017 at 12:59:33PM +0200, Martin Kletzander wrote:

On Fri, Mar 31, 2017 at 12:28:26PM +0200, Erik Skultety wrote:
> >  #define VIR_SYSFS_VALUE_MAXLEN 8192
> >  #define SYSFS_SYSTEM_PATH "/sys/devices/system"
> > +#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl"
> >
> >  static const char *sysfs_system_path = SYSFS_SYSTEM_PATH;
> > +static const char *sysfs_resctrl_path = SYSFS_RESCTRL_PATH;
> >
> >
> >  void virSysfsSetSystemPath(const char *path)
> > @@ -55,6 +58,20 @@ virSysfsGetSystemPath(void)
> >  return sysfs_system_path;
> >  }
> >
> > +void virSysfsSetResctrlPath(const char *path)
> > +{
> > +if (path)
> > +sysfs_resctrl_path  = path;
> > +else
> > +sysfs_resctrl_path = SYSFS_RESCTRL_PATH;
> > +}
> > +
> > +const char *
> > +virSysfsGetResctrlPath(void)
> > +{
> > +return sysfs_resctrl_path;
> > +}
> > +
>
> NACK
>
> This leads to an unnecessary code duplication (applies for most of the
> functions introduced by this patch). Instead, virsysfs should be made generic
> enough so it could be reused by any module doing sysfs related tasks, like for
> example the recently added mediated device framework (otherwise a new
> sysfs_foo_path = "/sys/bus/mdev/devices/" would need to be created as well).
>

I was thinking about this, but let's discuss how to select the proper
line between the amount of code duplication (which I didn't like even
in my series) and the generality of the code (which at the end leads to
worse code in callers).

Adding new set of functions for each path prefix is gross, I agree, but
how else could we approach this?  One of the ideas would be to have a
function that registers "path overrides", but that would lead to
unnecessary code in production where there are no tests involved.
Another approach is to just set the "/sys" path differently, but that
would mean we have to have bigger directory structures in the tests.
Yet another approach is to ditch virsysfs altogether and just use
virFile* functions.  We can mock open() and others in tests anyway
(like, for example, vircgrouptest does even for the sysfs system paths).
However, if I look at the code in the caller functions, the last
approach would, over time, end up duplicating more code than we do
currently in virsysfs.  Also, even though this is highly subjective, the
callers are very easy to read and understand.  More wrappers, if not
overused, of course, lead to cleaner codebase proportionally to the
codebase size.



The thing with code duplication is very subjective, while I say that copying
function bodies and enclosing them with a different name, then exporting the
symbol via a header and also adding it to the private syms is the kind of
duplication I'd like to avoid, I understand you can say the same about my idea
(yet to come) about having this in every caller:

...
if (!virAsprintf(path, "%s/%s", sysfs_prefix, attr))
   goto cleanup;

if (virSysfsRead[String,Int,Whatever] < 0)
   goto cleanup;
...

So in my opinion, this kind of duplication is more acceptable than having a ton
of symbols exporting the same functionality (aka function bodies). Now, to the
idea, to deal with the prefixes, each of the util/virmodule.c would declare a
string constant representing the path prefix, like virpci does with PCI_SYSFS
and mdev with MDEV_SYSFS_DEVICES, then the burden of constructing the path would
be on the caller (as prefaced above). I must admit that I haven't looked at the
tests, but I'm afraid you can't get both - generic functions without introducing
any duplicates in the code. But as we spoke privately, I think the concept of
mocking can be reused to our liking with the sysfs accesses. So, looking at the
tests, we will need especially need to figure out how to deal with the amount of
files created/present under virhostcpudata. You had a specific idea in mind
about this, so feel free to share it here.



Yeah, there's always pros and cons to any solution and since this is
pretty subjective, I'd opted for the one I liked the most as I feel like
code duplication of *very understandable* parts is better because it
speeds up reading the code for its callers.  Anyway, I had an idea how
to reach pretty good compromise.  I'll construct all the paths in the
callers, there will be no path masking in a global variable.  So only
virFileRead* will be used.  We might do like a few super wrappers with
various number of arguments, for example:

 virFileReadUint(_uint, "/sys/devices/system/cpu/cpu%d/online", cpu_num)

And that'd be it.  I was also thinking how not to do one wrapper per
type, but I don't think that's possible to do easily with C now.
However we can have virFileReadObject() that would call a function on a
virObject and for that we need to add interfaces to our classes for
which I have an idea how to do as well, but I'm getting too tangled up
in that.

And I think I have an idea how to do all the testing of this only 

Re: [libvirt] [PATCH v2] virNetDevIPCheckIPv6ForwardingCallback fixes

[Cedric Bosdonnat]

Hi all,

Has that one been forgotten?

On Tue, 2017-03-28 at 16:00 +0200, Cédric Bosdonnat wrote:
> Add check for more than one RTA_OIF, even though this is rather
> unlikely.
> 
> Get rid of the buggy switch / break as this code won't need to
> handle more attributes.
> 
> Use VIR_WARNINGS_NO_CAST_ALIGN to fix impossible to fix
> util/virnetdevip.c:560:17: error: cast increases required alignment of target 
> type [-Werror=cast-align]
> ---
>  Diff to v1:
>    * Add error message
>    * Use VIR_WARNINGS_NO_CAST_ALIGN
>  src/util/virnetdevip.c | 17 +
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> index c9ac6baf7..726fa6c3e 100644
> --- a/src/util/virnetdevip.c
> +++ b/src/util/virnetdevip.c
> @@ -556,15 +556,24 @@ virNetDevIPCheckIPv6ForwardingCallback(const struct 
> nlmsghdr *resp,
>  if (resp->nlmsg_type != RTM_NEWROUTE)
>  return ret;
>  
> -/* Extract a few attributes */
> +/* Extract a device ID attribute */
> +VIR_WARNINGS_NO_CAST_ALIGN
>  for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
> -switch (rta->rta_type) {
> -case RTA_OIF:
> +VIR_WARNINGS_RESET
> +if (rta->rta_type == RTA_OIF) {
>  oif = *(int *)RTA_DATA(rta);
>  
> +/* Should never happen: netlink message would be broken */
> +if (ifname) {
> +char *ifname2 = virNetDevGetName(oif);
> +VIR_WARN("Single route has unexpected 2nd interface "
> + "- '%s' and '%s'", ifname, ifname2);
> +VIR_FREE(ifname2);
> +break;
> +}
> +
>  if (!(ifname = virNetDevGetName(oif)))
>  goto error;
> -break;
>  }
>  }
>  

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

[libvirt] [PATCH 2/5] qemu: Turn qemuDomainLogContext into virObject

[Marc Hartmayer]

This way qemuDomainLogContextRef() and qemuDomainLogContextFree() is
no longer needed. The naming qemuDomainLogContextFree() was also
somewhat misleading. Additionally, it's easier to turn
qemuDomainLogContext in a self-locking object.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Bjoern Walk 
---
 src/qemu/qemu_domain.c  | 72 ++---
 src/qemu/qemu_domain.h  |  2 --
 src/qemu/qemu_process.c | 10 +++
 3 files changed, 44 insertions(+), 40 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b733505..6be7a4e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -111,7 +111,8 @@ VIR_ENUM_IMPL(qemuDomainNamespace, QEMU_DOMAIN_NS_LAST,
 
 
 struct _qemuDomainLogContext {
-int refs;
+virObject parent;
+
 int writefd;
 int readfd; /* Only used if manager == NULL */
 off_t pos;
@@ -120,6 +121,36 @@ struct _qemuDomainLogContext {
 virLogManagerPtr manager;
 };
 
+static virClassPtr qemuDomainLogContextClass;
+
+static void qemuDomainLogContextDispose(void *obj);
+
+static int
+qemuDomainLogContextOnceInit(void)
+{
+if (!(qemuDomainLogContextClass = virClassNew(virClassForObject(),
+ "qemuDomainLogContext",
+ sizeof(qemuDomainLogContext),
+ qemuDomainLogContextDispose)))
+return -1;
+
+return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(qemuDomainLogContext)
+
+static void
+qemuDomainLogContextDispose(void *obj)
+{
+qemuDomainLogContextPtr ctxt = obj;
+VIR_DEBUG("ctxt=%p", ctxt);
+
+virLogManagerFree(ctxt->manager);
+VIR_FREE(ctxt->path);
+VIR_FORCE_CLOSE(ctxt->writefd);
+VIR_FORCE_CLOSE(ctxt->readfd);
+}
+
 const char *
 qemuDomainAsyncJobPhaseToString(qemuDomainAsyncJob job,
 int phase ATTRIBUTE_UNUSED)
@@ -4175,7 +4206,7 @@ void qemuDomainObjTaint(virQEMUDriverPtr driver,
  cleanup:
 VIR_FREE(timestamp);
 if (closeLog)
-qemuDomainLogContextFree(logCtxt);
+virObjectUnref(logCtxt);
 if (orig_err) {
 virSetError(orig_err);
 virFreeError(orig_err);
@@ -4287,13 +4318,15 @@ qemuDomainLogContextPtr 
qemuDomainLogContextNew(virQEMUDriverPtr driver,
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 qemuDomainLogContextPtr ctxt = NULL;
 
-if (VIR_ALLOC(ctxt) < 0)
-goto error;
+if (qemuDomainLogContextInitialize() < 0)
+goto cleanup;
+
+if (!(ctxt = virObjectNew(qemuDomainLogContextClass)))
+goto cleanup;
 
 VIR_DEBUG("Context new %p stdioLogD=%d", ctxt, cfg->stdioLogD);
 ctxt->writefd = -1;
 ctxt->readfd = -1;
-virAtomicIntSet(>refs, 1);
 
 if (virAsprintf(>path, "%s/%s.log", cfg->logDir, vm->def->name) < 0)
 goto error;
@@ -4361,7 +4394,7 @@ qemuDomainLogContextPtr 
qemuDomainLogContextNew(virQEMUDriverPtr driver,
 return ctxt;
 
  error:
-qemuDomainLogContextFree(ctxt);
+virObjectUnref(ctxt);
 ctxt = NULL;
 goto cleanup;
 }
@@ -4530,39 +4563,12 @@ void 
qemuDomainLogContextMarkPosition(qemuDomainLogContextPtr ctxt)
 }
 
 
-void qemuDomainLogContextRef(qemuDomainLogContextPtr ctxt)
-{
-VIR_DEBUG("Context ref %p", ctxt);
-virAtomicIntInc(>refs);
-}
-
-
 virLogManagerPtr qemuDomainLogContextGetManager(qemuDomainLogContextPtr ctxt)
 {
 return ctxt->manager;
 }
 
 
-void qemuDomainLogContextFree(qemuDomainLogContextPtr ctxt)
-{
-bool lastRef;
-
-if (!ctxt)
-return;
-
-lastRef = virAtomicIntDecAndTest(>refs);
-VIR_DEBUG("Context free %p lastref=%d", ctxt, lastRef);
-if (!lastRef)
-return;
-
-virLogManagerFree(ctxt->manager);
-VIR_FREE(ctxt->path);
-VIR_FORCE_CLOSE(ctxt->writefd);
-VIR_FORCE_CLOSE(ctxt->readfd);
-VIR_FREE(ctxt);
-}
-
-
 /* Locate an appropriate 'qemu-img' binary.  */
 const char *
 qemuFindQemuImgBinary(virQEMUDriverPtr driver)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 91573ff..caac5d5 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -540,8 +540,6 @@ ssize_t qemuDomainLogContextRead(qemuDomainLogContextPtr 
ctxt,
  char **msg);
 int qemuDomainLogContextGetWriteFD(qemuDomainLogContextPtr ctxt);
 void qemuDomainLogContextMarkPosition(qemuDomainLogContextPtr ctxt);
-void qemuDomainLogContextRef(qemuDomainLogContextPtr ctxt);
-void qemuDomainLogContextFree(qemuDomainLogContextPtr ctxt);
 
 virLogManagerPtr qemuDomainLogContextGetManager(qemuDomainLogContextPtr ctxt);
 
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e450d06..028f0c5 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1692,7 +1692,7 @@ static void
 qemuProcessMonitorLogFree(void *opaque)
 {
 qemuDomainLogContextPtr logCtxt = opaque;
-

[libvirt] [PATCH 4/5] qemu: remove ATTRIBUTE_UNUSED in qemuProcessHandleMonitorEOF

[Marc Hartmayer]

This attribute is not needed here, since @mon is in use.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Bjoern Walk 
---
 src/qemu/qemu_process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 028f0c5..c060847 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -279,7 +279,7 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr 
vm)
  * performed
  */
 static void
-qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
+qemuProcessHandleMonitorEOF(qemuMonitorPtr mon,
 virDomainObjPtr vm,
 void *opaque)
 {
-- 
2.5.5

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

[libvirt] [PATCH 5/5] refactoring: Use the return value of virObjectRef directly

[Marc Hartmayer]

Use the return value of virObjectRef directly. This way, it's easier
for another reader to identify the reason why the additional reference
is required.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Bjoern Walk 
---
 src/datatypes.c  | 6 ++
 src/rpc/virnetclientstream.c | 4 +---
 src/rpc/virnetserver.c   | 9 +++--
 tests/qemumonitortestutils.c | 3 +--
 4 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/src/datatypes.c b/src/datatypes.c
index 3e3148d..59ba956 100644
--- a/src/datatypes.c
+++ b/src/datatypes.c
@@ -196,8 +196,7 @@ void 
virConnectCloseCallbackDataRegister(virConnectCloseCallbackDataPtr closeDat
 return;
 }
 
-closeData->conn = conn;
-virObjectRef(closeData->conn);
+closeData->conn = virObjectRef(conn);
 closeData->callback = cb;
 closeData->opaque = opaque;
 closeData->freeCallback = freecb;
@@ -985,8 +984,7 @@ 
virAdmConnectCloseCallbackDataRegister(virAdmConnectCloseCallbackDataPtr cbdata,
 goto cleanup;
 }
 
-virObjectRef(conn);
-cbdata->conn = conn;
+cbdata->conn = virObjectRef(conn);
 cbdata->callback = cb;
 cbdata->opaque = opaque;
 cbdata->freeCallback = freecb;
diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c
index 34989a9..2105bd0 100644
--- a/src/rpc/virnetclientstream.c
+++ b/src/rpc/virnetclientstream.c
@@ -145,12 +145,10 @@ virNetClientStreamPtr 
virNetClientStreamNew(virNetClientProgramPtr prog,
 if (!(st = virObjectLockableNew(virNetClientStreamClass)))
 return NULL;
 
-st->prog = prog;
+st->prog = virObjectRef(prog);
 st->proc = proc;
 st->serial = serial;
 
-virObjectRef(prog);
-
 return st;
 }
 
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index f06643a..c02db74 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -213,8 +213,7 @@ static int 
virNetServerDispatchNewMessage(virNetServerClientPtr client,
 job->msg = msg;
 
 if (prog) {
-virObjectRef(prog);
-job->prog = prog;
+job->prog = virObjectRef(prog);
 priority = virNetServerProgramGetPriority(prog, msg->header.proc);
 }
 
@@ -284,8 +283,7 @@ int virNetServerAddClient(virNetServerPtr srv,
 
 if (VIR_EXPAND_N(srv->clients, srv->nclients, 1) < 0)
 goto error;
-srv->clients[srv->nclients-1] = client;
-virObjectRef(client);
+srv->clients[srv->nclients-1] = virObjectRef(client);
 
 if (virNetServerClientNeedAuth(client))
 virNetServerTrackPendingAuthLocked(srv);
@@ -695,8 +693,7 @@ int virNetServerAddService(virNetServerPtr srv,
 }
 }
 
-srv->services[srv->nservices-1] = svc;
-virObjectRef(svc);
+srv->services[srv->nservices-1] = virObjectRef(svc);
 
 virNetServerServiceSetDispatcher(svc,
  virNetServerDispatchNewClient,
diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c
index 89857a6..5e30fb0 100644
--- a/tests/qemumonitortestutils.c
+++ b/tests/qemumonitortestutils.c
@@ -1064,8 +1064,7 @@ qemuMonitorCommonTestNew(virDomainXMLOptionPtr xmlopt,
 goto error;
 
 if (vm) {
-virObjectRef(vm);
-test->vm = vm;
+test->vm = virObjectRef(vm);
 } else {
 test->vm = virDomainObjNew(xmlopt);
 if (!test->vm)
-- 
2.5.5

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

[libvirt] [PATCH 3/5] qemu: Implement qemuMonitorRegister()

[Marc Hartmayer]

Implement qemuMonitorRegister() as there is already a
qemuMonitorUnregister() function. This way it may be easier to
understand the code paths.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Bjoern Walk 
---
 src/qemu/qemu_monitor.c | 38 +-
 src/qemu/qemu_monitor.h |  2 ++
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index b41aaed..34037ac 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -837,15 +837,7 @@ qemuMonitorOpenInternal(virDomainObjPtr vm,
 
 
 virObjectLock(mon);
-virObjectRef(mon);
-if ((mon->watch = virEventAddHandle(mon->fd,
-VIR_EVENT_HANDLE_HANGUP |
-VIR_EVENT_HANDLE_ERROR |
-VIR_EVENT_HANDLE_READABLE,
-qemuMonitorIO,
-mon,
-virObjectFreeCallback)) < 0) {
-virObjectUnref(mon);
+if (!qemuMonitorRegister(mon)) {
 virObjectUnlock(mon);
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("unable to register monitor events"));
@@ -944,6 +936,34 @@ qemuMonitorOpenFD(virDomainObjPtr vm,
 }
 
 
+/**
+ * qemuMonitorRegister:
+ * @mon: QEMU monitor
+ *
+ * Registers the monitor in the event loop. The caller has to hold the
+ * lock for @mon.
+ *
+ * Returns true in case of success, false otherwise
+ */
+bool
+qemuMonitorRegister(qemuMonitorPtr mon)
+{
+virObjectRef(mon);
+if ((mon->watch = virEventAddHandle(mon->fd,
+VIR_EVENT_HANDLE_HANGUP |
+VIR_EVENT_HANDLE_ERROR |
+VIR_EVENT_HANDLE_READABLE,
+qemuMonitorIO,
+mon,
+virObjectFreeCallback)) < 0) {
+virObjectUnref(mon);
+return false;
+}
+
+return true;
+}
+
+
 void
 qemuMonitorUnregister(qemuMonitorPtr mon)
 {
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 2e42d16..12f98be 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -296,6 +296,8 @@ qemuMonitorPtr qemuMonitorOpenFD(virDomainObjPtr vm,
  void *opaque)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4);
 
+bool qemuMonitorRegister(qemuMonitorPtr mon)
+ATTRIBUTE_NONNULL(1);
 void qemuMonitorUnregister(qemuMonitorPtr mon)
 ATTRIBUTE_NONNULL(1);
 void qemuMonitorClose(qemuMonitorPtr mon);
-- 
2.5.5

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

[libvirt] [PATCH 1/5] qemu: Fix two use-after-free situations

[Marc Hartmayer]

There were multiple race conditions that could lead to segmentation
faults. The first precondition for this is qemuProcessLaunch must fail
sometime shortly after starting the new QEMU process. The second
precondition for the segmentation faults is that the new QEMU process
dies - or to be more precise the QEMU monitor has to be closed
irregularly. If both happens during qemuProcessStart (starting a
domain) there are race windows between the thread with the event
loop (T1) and the thread that is starting the domain (T2).

First segmentation fault scenario:
If qemuProcessLaunch fails during qemuProcessStart the code branches
to the 'stop' path where 'qemuMonitorSetDomainLog(priv->mon, NULL,
NULL, NULL)' will set the log function of the monitor to NULL (done in
T2). In the meantime the event loop of T1 will wake up with an EOF
event for the QEMU monitor because the QEMU process has died. The
crash occurs if T1 has checked 'mon->logFunc != NULL' in qemuMonitorIO
just before the logFunc was set to NULL by T2. If this situation
occurs T1 will try to call mon->logFunc which leads to the
segmentation fault.

Solution:
Require the monitor lock for setting the log function.

Backtrace:
0  0x in ?? ()
1  0x03ffe9e45316 in qemuMonitorIO (watch=,
fd=, events=, opaque=0x3ffe08aa860) at
../../src/qemu/qemu_monitor.c:727
2  0x03fffda2e1a4 in virEventPollDispatchHandles (nfds=, fds=0x2aa000fd980) at ../../src/util/vireventpoll.c:508
3  0x03fffda2e398 in virEventPollRunOnce () at
../../src/util/vireventpoll.c:657
4  0x03fffda2ca10 in virEventRunDefaultImpl () at
../../src/util/virevent.c:314
5  0x03fffdba9366 in virNetDaemonRun (dmn=0x2aa000cc550) at
../../src/rpc/virnetdaemon.c:818
6  0x02aa00024668 in main (argc=, argv=) at ../../daemon/libvirtd.c:1541

Second segmentation fault scenario:
If qemuProcessLaunch fails it will unref the log context and with
invoking qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL)
qemuDomainLogContextFree() will be invoked. qemuDomainLogContextFree()
invokes virNetClientClose() to close the client and cleans everything
up (including unref of _virLogManager.client) when virNetClientClose()
returns. When T1 is now trying to report 'qemu unexpectedly closed the
monitor' libvirtd will crash because the client has already been
freed.

Solution:
As the critical section in qemuMonitorIO is protected with the monitor
lock we can use the same solution as proposed for the first
segmentation fault.

Backtrace:
0  virClassIsDerivedFrom (klass=0x3100979797979797,
parent=0x2aa000d92f0) at ../../src/util/virobject.c:169
1  0x03fffda659e6 in virObjectIsClass (anyobj=,
klass=) at ../../src/util/virobject.c:365
2  0x03fffda65a24 in virObjectLock (anyobj=0x3ffe08c1db0) at
../../src/util/virobject.c:317
3  0x03fffdba4688 in
virNetClientIOEventLoop (client=client@entry=0x3ffe08c1db0,
thiscall=thiscall@entry=0x2aa000fbfa0) at
../../src/rpc/virnetclient.c:1668
4  0x03fffdba4b4c in
virNetClientIO (client=client@entry=0x3ffe08c1db0,
thiscall=0x2aa000fbfa0) at ../../src/rpc/virnetclient.c:1944
5  0x03fffdba4d42 in
virNetClientSendInternal (client=client@entry=0x3ffe08c1db0,
msg=msg@entry=0x2aa000cc710, expectReply=expectReply@entry=true,
nonBlock=nonBlock@entry=false) at ../../src/rpc/virnetclient.c:2116
6  0x03fffdba6268 in
virNetClientSendWithReply (client=0x3ffe08c1db0, msg=0x2aa000cc710) at
../../src/rpc/virnetclient.c:2144
7  0x03fffdba6e8e in virNetClientProgramCall (prog=0x3ffe08c1120,
client=, serial=, proc=,
noutfds=, outfds=0x0, ninfds=0x0, infds=0x0,
args_filter=0x3fffdb64440
, args=0x3ffe010,
ret_filter=0x3fffdb644c0
, ret=0x3ffe008) at
../../src/rpc/virnetclientprogram.c:329
8  0x03fffdb64042 in
virLogManagerDomainReadLogFile (mgr=, path=, inode=, offset=, maxlen=, flags=0) at ../../src/logging/log_manager.c:272
9  0x03ffe9e0315c in qemuDomainLogContextRead (ctxt=0x3ffe08c2980,
msg=0x3ffe1c0) at ../../src/qemu/qemu_domain.c:4422
10 0x03ffe9e280a8 in qemuProcessReadLog (logCtxt=,
msg=msg@entry=0x3ffe288) at ../../src/qemu/qemu_process.c:1800
11 0x03ffe9e28206 in qemuProcessReportLogError (logCtxt=, msgprefix=0x3ffe9ec276a "qemu unexpectedly closed the monitor")
at ../../src/qemu/qemu_process.c:1836
12 0x03ffe9e28306 in
qemuProcessMonitorReportLogError (mon=mon@entry=0x3ffe085cf10,
msg=, opaque=) at
../../src/qemu/qemu_process.c:1856
13 0x03ffe9e452b6 in qemuMonitorIO (watch=,
fd=, events=, opaque=0x3ffe085cf10) at
../../src/qemu/qemu_monitor.c:726
14 0x03fffda2e1a4 in virEventPollDispatchHandles (nfds=, fds=0x2aa000fd980) at ../../src/util/vireventpoll.c:508
15 0x03fffda2e398 in virEventPollRunOnce () at
../../src/util/vireventpoll.c:657
16 0x03fffda2ca10 in virEventRunDefaultImpl () at
../../src/util/virevent.c:314
17 0x03fffdba9366 in virNetDaemonRun (dmn=0x2aa000cc550) at
../../src/rpc/virnetdaemon.c:818
18 0x02aa00024668 in main (argc=, argv=) at 

[libvirt] [PATCH 0/5] Use-after-free fix and cleanups

[Marc Hartmayer]

While looking at a use-after-free situation going through how the QEMU
monitor is set up I noticed some things. These cleanups and the fix
for the use-after-free are the result of that.

Marc Hartmayer (5):
  qemu: Fix two use-after-free situations
  qemu: Turn qemuDomainLogContext into virObject
  qemu: Implement qemuMonitorRegister()
  qemu: remove ATTRIBUTE_UNUSED in qemuProcessHandleMonitorEOF
  refactoring: Use the return value of virObjectRef directly

 src/datatypes.c  |  6 ++--
 src/qemu/qemu_domain.c   | 72 --
 src/qemu/qemu_domain.h   |  2 --
 src/qemu/qemu_monitor.c  | 82 ++--
 src/qemu/qemu_monitor.h  |  6 
 src/qemu/qemu_process.c  | 12 +++
 src/rpc/virnetclientstream.c |  4 +--
 src/rpc/virnetserver.c   |  9 ++---
 tests/qemumonitortestutils.c |  3 +-
 9 files changed, 121 insertions(+), 75 deletions(-)

-- 
2.5.5

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

Re: [libvirt] [PATCH V3] util: Add more virsysfs functions for handling resctrl sysfs

[Erik Skultety]

On Fri, Mar 31, 2017 at 12:59:33PM +0200, Martin Kletzander wrote:
> On Fri, Mar 31, 2017 at 12:28:26PM +0200, Erik Skultety wrote:
> > >  #define VIR_SYSFS_VALUE_MAXLEN 8192
> > >  #define SYSFS_SYSTEM_PATH "/sys/devices/system"
> > > +#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl"
> > >
> > >  static const char *sysfs_system_path = SYSFS_SYSTEM_PATH;
> > > +static const char *sysfs_resctrl_path = SYSFS_RESCTRL_PATH;
> > >
> > >
> > >  void virSysfsSetSystemPath(const char *path)
> > > @@ -55,6 +58,20 @@ virSysfsGetSystemPath(void)
> > >  return sysfs_system_path;
> > >  }
> > >
> > > +void virSysfsSetResctrlPath(const char *path)
> > > +{
> > > +if (path)
> > > +sysfs_resctrl_path  = path;
> > > +else
> > > +sysfs_resctrl_path = SYSFS_RESCTRL_PATH;
> > > +}
> > > +
> > > +const char *
> > > +virSysfsGetResctrlPath(void)
> > > +{
> > > +return sysfs_resctrl_path;
> > > +}
> > > +
> >
> > NACK
> >
> > This leads to an unnecessary code duplication (applies for most of the
> > functions introduced by this patch). Instead, virsysfs should be made 
> > generic
> > enough so it could be reused by any module doing sysfs related tasks, like 
> > for
> > example the recently added mediated device framework (otherwise a new
> > sysfs_foo_path = "/sys/bus/mdev/devices/" would need to be created as well).
> >
>
> I was thinking about this, but let's discuss how to select the proper
> line between the amount of code duplication (which I didn't like even
> in my series) and the generality of the code (which at the end leads to
> worse code in callers).
>
> Adding new set of functions for each path prefix is gross, I agree, but
> how else could we approach this?  One of the ideas would be to have a
> function that registers "path overrides", but that would lead to
> unnecessary code in production where there are no tests involved.
> Another approach is to just set the "/sys" path differently, but that
> would mean we have to have bigger directory structures in the tests.
> Yet another approach is to ditch virsysfs altogether and just use
> virFile* functions.  We can mock open() and others in tests anyway
> (like, for example, vircgrouptest does even for the sysfs system paths).
> However, if I look at the code in the caller functions, the last
> approach would, over time, end up duplicating more code than we do
> currently in virsysfs.  Also, even though this is highly subjective, the
> callers are very easy to read and understand.  More wrappers, if not
> overused, of course, lead to cleaner codebase proportionally to the
> codebase size.
>

The thing with code duplication is very subjective, while I say that copying
function bodies and enclosing them with a different name, then exporting the
symbol via a header and also adding it to the private syms is the kind of
duplication I'd like to avoid, I understand you can say the same about my idea
(yet to come) about having this in every caller:

...
if (!virAsprintf(path, "%s/%s", sysfs_prefix, attr))
goto cleanup;

if (virSysfsRead[String,Int,Whatever] < 0)
goto cleanup;
...

So in my opinion, this kind of duplication is more acceptable than having a ton
of symbols exporting the same functionality (aka function bodies). Now, to the
idea, to deal with the prefixes, each of the util/virmodule.c would declare a
string constant representing the path prefix, like virpci does with PCI_SYSFS
and mdev with MDEV_SYSFS_DEVICES, then the burden of constructing the path would
be on the caller (as prefaced above). I must admit that I haven't looked at the
tests, but I'm afraid you can't get both - generic functions without introducing
any duplicates in the code. But as we spoke privately, I think the concept of
mocking can be reused to our liking with the sysfs accesses. So, looking at the
tests, we will need especially need to figure out how to deal with the amount of
files created/present under virhostcpudata. You had a specific idea in mind
about this, so feel free to share it here.

Erik

> I'm not saying that what I did was the best approach, but please be
> constructive.  If we don't have any better solutions for now, we can go
> with one that's Good Enough™ and refactor it later when we figure out
> how to approach it.  I'm open to suggestions and I know Eli is as well.
>
> Martin



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

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

Re: [libvirt] Live attaching a disk to a VM fails with apparmor enabled

[Guido Günther]

On Thu, Mar 30, 2017 at 03:00:06PM +, Frank Schreuder wrote:
> Hello Guido,
> 
> I have great news. I'm able to successfully live attach a disk to a running 
> VM with a loaded apparmor profile.
> 
> My setup:
> Debian 8
> Kernel 4.9.11
> Libvirt 3.1.0
> Apparmor 2.10 from Debian backports
> 
> With same software and apparmor 2.9 from the stable Debian repo it fails. So 
> apparently 2.10 has upstream fixes/patches which solve the reload profile 
> bug? Hope this new insight helps you find the commit and backport it to 
> apparmor 2.9 stable?

Thanks for reporting, I added a note to #805002. It's unlikely we'll
have a backport of both the kernel changes and appamor for Jessie but we
can make things work for stretch (which currently shows a different
error I'll have to look into).
Cheers,
 -- Guido

> 
> Thanks,
> Frank
> 
> 
> Sent from my iPhone
> 
> > On 24 Mar 2017, at 09:17, Guido Günther  wrote:
> > 
> >> On Thu, Mar 23, 2017 at 01:28:57PM +0100, Cedric Bosdonnat wrote:
> >> Hello Frank,
> >> 
> >> I'm currently investigating some apparmor-related bug with namespaces. 
> >> This one
> >> is surely related. I'll look into it when I'm done with the one I'm 
> >> working on.
> > 
> > Assuming you're running the Jessie Kernel its likely:
> > 
> >https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=805002
> > 
> > To make sure it's the kernel and not libvirt have a look at:
> > 
> >https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=805002#51
> > 
> > Cheers,
> > -- Guido
> > 
> >> 
> >> --
> >> Cedric
> >> 
> >>> On Thu, 2017-03-23 at 12:07 +, Frank Schreuder wrote:
> >>> Hello,
> >>> 
> >>> I'm running libvirt 3.1.0 on a Debian 8 server. I installed apparmor and 
> >>> configured libvirt to use apparmor as
> >>> security driver.
> >>> After booting a VM, virsh dumpxml shows an apparmor seclabel.
> >>> 
> >>> As soon as I try to attach a second disk to the VM, apparmor blocks this.
> >>> 
> >>> virsh attach-device test-vps /tmp/virshXmlDefinition
> >>> error: Failed to attach device from /tmp/virshXmlDefinition
> >>> error: operation failed: Could not open '/mnt/images/disk2.raw': 
> >>> Permission denied
> >>> 
> >>> Syslogs shows me the following:
> >>> Mar 22 17:45:20 vps0 kernel: [1136647.318314] audit: type=1400 
> >>> audit(1490201120.577:30): apparmor="DENIED"
> >>> operation="open" profile="libvirt-5747e4db-a3b7-fd69-ca89-7b0bf859" 
> >>> name="/mnt/images/disk2.raw" pid=13453
> >>> comm="kvm" requested_mask="r" denied_mask="r" fsuid=996 ouid=33
> >>> Mar 22 17:45:20 vps0 kernel: [1136647.325155] audit: type=1400 
> >>> audit(1490201120.577:31): apparmor="DENIED"
> >>> operation="open" profile="libvirt-5747e4db-a3b7-fd69-ca89-7b0bf859" 
> >>> name="/mnt/images/disk2.raw" pid=13453
> >>> comm="kvm" requested_mask="rw" denied_mask="rw" fsuid=996 ouid=33
> >>> Mar 22 17:45:20 vps0 libvirtd[10282]: 2017-03-22 16:45:20.596+: 
> >>> 10283: error : qemuMonitorTextAddDrive:1968 :
> >>> operation failed: Could not open '/mnt/images/disk2.raw': Permission 
> >>> denied
> >>> 
> >>> In the VM specific apparmor file 
> >>> /etc/apparmor.d/libvirt/libvirt-5747e4db-a3b7-fd69-ca89-7b0bf859.files
> >>>  I see:
> >>> "/mnt/images/disk1.raw" rw,
> >>> 
> >>> Which is my primary VM disk, I expected a virsh attach-device to append 
> >>> /mnt/images/disk2.raw to this file and
> >>> reload/refresh the apparmor profile?
> >>> 
> >>> I'm not able to attach a live disk to a running VM with apparmor. Am I 
> >>> missing something? Or is this a bug/missing
> >>> feature in libvirt?
> >>> 
> >>> Thanks,
> >>> Frank
> >>> --
> >>> 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
> >> 

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