Re: [libvirt] [PATCH V2 1/4] conf: add 'state' attribute to feature

[Jim Fehlig]

Joao Martins wrote:
> 
> On 03/01/2016 04:00 AM, Jim Fehlig wrote:
>> Most hypervisors use Hardware Assisted Paging by default and don't
>> require specifying the feature in domain conf. But some hypervisors
>> support disabling HAP on a per-domain basis. To enable HAP by default
>> yet provide a knob to disable it, extend the  feature with a
>> 'state=on|off' attribute, similar to  and  features.
>>
>> In the absence of , the hypervisor default (on) is used. 
>> without the state attribute would be the same as  for
>> backwards compatibility. And of course  disables hap.
>>
>> Signed-off-by: Jim Fehlig 
>> ---
>>  docs/formatdomain.html.in | 6 --
>>  docs/schemas/domaincommon.rng | 6 +-
>>  src/conf/domain_conf.c| 4 ++--
>>  3 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 5016772..c06bcf3 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -1494,8 +1494,10 @@
>>Interrupt) for the guest.
>>
>>hap
>> -  Enable use of Hardware Assisted Paging if available in
>> -the hardware.
>> +  Depending on the state attribute (values 
>> on,
>> +off) enable or disable use of Hardware Assisted Paging.
>> +The default is on if the hypervisor detects 
>> availability
>> +of Hardware Assisted Paging.
>>
>>viridian
>>Enable Viridian hypervisor extensions for paravirtualizing
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index 67af93a..dd6e93a 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -4108,7 +4108,11 @@
>>
>>
>>  
>> -  
>> +  
>> +
>> +  
>> +
>> +  
> Perhaps  would be better (see chunk below) ? That 
> one
> appears to be a reference of what you are adding above, and it's the same as
> pvspinlock. Though some other elements don't appear to use this, not sure why.

Ah, thanks for catching that. 'featurestate' is definitely the better choice 
here.

> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 89d3a6b..141122c 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4132,9 +4132,7 @@
>
>  
>
> -
> -  
> -
> +
>
>  
>
> 
> Other that,
> 
> Reviewed-by: Joao Martins 

Thanks. I've squashed in your diff, but should probably wait for any additional
comments before pushing this series.

Regards,
Jim

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

Re: [libvirt] [PATCH 1/8] domain: Add virDomainDefAddImplicitDevices

[Laine Stump]

On 03/08/2016 11:36 AM, Cole Robinson wrote:

It's just a combination of AddImplicitControllers, and AddConsoleCompat.
Every caller that wants ImplicitControllers also wants the ConsoleCompat
AFAICT, so lump them together. We also need it for future patches.
---
  src/conf/domain_conf.c   | 19 ++-
  src/conf/domain_conf.h   |  2 +-
  src/libvirt_private.syms |  2 +-
  src/qemu/qemu_driver.c   |  6 +++---
  src/vmx/vmx.c|  2 +-
  src/vz/vz_sdk.c  |  2 +-
  6 files changed, 21 insertions(+), 12 deletions(-)


This just makes sense in general, regardless of what you need it for 
later :-)


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

[libvirt] [PATCH REPOST 2/9] qemu: Introduce qemuBuildPMCommandLine

[John Ferlan]

Add new function to manage adding the power management options to the
command line removing that task from the mainline qemuBuildCommandLine.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_command.c | 136 ++--
 1 file changed, 75 insertions(+), 61 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c890b75..3b3c958 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4893,6 +4893,79 @@ qemuBuildClockCommandLine(virCommandPtr cmd,
 
 
 static int
+qemuBuildPMCommandLine(virCommandPtr cmd,
+   const virDomainDef *def,
+   virQEMUCapsPtr qemuCaps,
+   bool monitor_json)
+{
+bool allowReboot = true;
+
+/* Only add -no-reboot option if each event destroys domain */
+if (def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY &&
+def->onPoweroff == VIR_DOMAIN_LIFECYCLE_DESTROY &&
+(def->onCrash == VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY ||
+ def->onCrash == VIR_DOMAIN_LIFECYCLE_CRASH_COREDUMP_DESTROY)) {
+allowReboot = false;
+virCommandAddArg(cmd, "-no-reboot");
+}
+
+/* If JSON monitor is enabled, we can receive an event
+ * when QEMU stops. If we use no-shutdown, then we can
+ * watch for this event and do a soft/warm reboot.
+ */
+if (monitor_json && allowReboot &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_SHUTDOWN)) {
+virCommandAddArg(cmd, "-no-shutdown");
+}
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_ACPI)) {
+if (def->features[VIR_DOMAIN_FEATURE_ACPI] != VIR_TRISTATE_SWITCH_ON)
+virCommandAddArg(cmd, "-no-acpi");
+}
+
+/* We fall back to PIIX4_PM even for q35, since it's what we did
+   pre-q35-pm support. QEMU starts up fine (with a warning) if
+   mixing PIIX PM and -M q35. Starting to reject things here
+   could mean we refuse to start existing configs in the wild.*/
+if (def->pm.s3) {
+const char *pm_object = "PIIX4_PM";
+
+if (qemuDomainMachineIsQ35(def) &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_DISABLE_S3)) {
+pm_object = "ICH9-LPC";
+} else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_DISABLE_S3)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   "%s", _("setting ACPI S3 not supported"));
+return -1;
+}
+
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd, "%s.disable_s3=%d",
+   pm_object, def->pm.s3 == VIR_TRISTATE_BOOL_NO);
+}
+
+if (def->pm.s4) {
+const char *pm_object = "PIIX4_PM";
+
+if (qemuDomainMachineIsQ35(def) &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_DISABLE_S4)) {
+pm_object = "ICH9-LPC";
+} else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_DISABLE_S4)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   "%s", _("setting ACPI S4 not supported"));
+return -1;
+}
+
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd, "%s.disable_s4=%d",
+   pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO);
+}
+
+return 0;
+}
+
+
+static int
 qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
 const virDomainDef *def,
 virBufferPtr buf,
@@ -7204,7 +7277,6 @@ qemuBuildCommandLine(virConnectPtr conn,
 bool havespice = false;
 int last_good_net = -1;
 virCommandPtr cmd = NULL;
-bool allowReboot = true;
 bool emitBootindex = false;
 int usbcontroller = 0;
 int actualSerials = 0;
@@ -7341,66 +7413,8 @@ qemuBuildCommandLine(virConnectPtr conn,
 if (qemuBuildClockCommandLine(cmd, def, qemuCaps) < 0)
 goto error;
 
-/* Only add -no-reboot option if each event destroys domain */
-if (def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY &&
-def->onPoweroff == VIR_DOMAIN_LIFECYCLE_DESTROY &&
-(def->onCrash == VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY ||
- def->onCrash == VIR_DOMAIN_LIFECYCLE_CRASH_COREDUMP_DESTROY)) {
-allowReboot = false;
-virCommandAddArg(cmd, "-no-reboot");
-}
-
-/* If JSON monitor is enabled, we can receive an event
- * when QEMU stops. If we use no-shutdown, then we can
- * watch for this event and do a soft/warm reboot.
- */
-if (monitor_json && allowReboot &&
-virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_SHUTDOWN)) {
-virCommandAddArg(cmd, "-no-shutdown");
-}
-
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_ACPI)) {
-if (def->features[VIR_DOMAIN_FEATURE_ACPI] != VIR_TRISTATE_SWITCH_ON)
-virCommandAddArg(cmd, "-no-acpi");
-}
-
-/* We fall back to PIIX4_PM even for q35, since it's what we did
-   pre-q35-pm support. QEMU starts up fine (with a warning) if
-   mixing PIIX PM and 

[libvirt] [PATCH REPOST 7/9] qemu: Introduce qemuBuildDiskDriveCommandLine

[John Ferlan]

Add new function to manage adding the disk -drive options to the
command line removing that task from the mainline qemuBuildCommandLine.

Also since using const virDomainDef in new function, that means other
functions called needed to change their usage.

Signed-off-by: John Ferlan 
---
 src/conf/domain_conf.c  |   4 +-
 src/conf/domain_conf.h  |   4 +-
 src/qemu/qemu_command.c | 317 +---
 src/qemu/qemu_command.h |   2 +-
 4 files changed, 173 insertions(+), 154 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 11ac707..4b2633a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5746,7 +5746,7 @@ virDomainHostdevDefParseXMLCaps(xmlNodePtr node 
ATTRIBUTE_UNUSED,
 }
 
 int
-virDomainDeviceFindControllerModel(virDomainDefPtr def,
+virDomainDeviceFindControllerModel(const virDomainDef *def,
virDomainDeviceInfoPtr info,
int controllerType)
 {
@@ -18327,7 +18327,7 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def)
 }
 
 virDomainIOThreadIDDefPtr
-virDomainIOThreadIDFind(virDomainDefPtr def,
+virDomainIOThreadIDFind(const virDomainDef *def,
 unsigned int iothread_id)
 {
 size_t i;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 8af3c64..468b759 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2516,7 +2516,7 @@ int virDomainDiskSetDriver(virDomainDiskDefPtr def, const 
char *name)
 ATTRIBUTE_RETURN_CHECK;
 int virDomainDiskGetFormat(virDomainDiskDefPtr def);
 void virDomainDiskSetFormat(virDomainDiskDefPtr def, int format);
-int virDomainDeviceFindControllerModel(virDomainDefPtr def,
+int virDomainDeviceFindControllerModel(const virDomainDef *def,
virDomainDeviceInfoPtr info,
int controllerType);
 virDomainDiskDefPtr virDomainDiskFindByBusAndDst(virDomainDefPtr def,
@@ -2699,7 +2699,7 @@ bool virDomainDefCheckABIStability(virDomainDefPtr src,
 
 int virDomainDefAddImplicitControllers(virDomainDefPtr def);
 
-virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(virDomainDefPtr def,
+virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(const virDomainDef *def,
   unsigned int iothread_id);
 virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def,
  unsigned int iothread_id);
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ef94af8..372f84f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1443,7 +1443,7 @@ qemuBuildDriveStr(virConnectPtr conn,
 
 
 static bool
-qemuCheckIOThreads(virDomainDefPtr def,
+qemuCheckIOThreads(const virDomainDef *def,
virDomainDiskDefPtr disk)
 {
 /* Right "type" of disk" */
@@ -1469,7 +1469,7 @@ qemuCheckIOThreads(virDomainDefPtr def,
 
 
 char *
-qemuBuildDriveDevStr(virDomainDefPtr def,
+qemuBuildDriveDevStr(const virDomainDef *def,
  virDomainDiskDefPtr disk,
  int bootindex,
  virQEMUCapsPtr qemuCaps)
@@ -1766,6 +1766,168 @@ qemuBuildDriveDevStr(virDomainDefPtr def,
 }
 
 
+static int
+qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
+  virConnectPtr conn,
+  const virDomainDef *def,
+  virQEMUCapsPtr qemuCaps,
+  bool emitBootindex)
+{
+size_t i;
+int bootCD = 0, bootFloppy = 0, bootDisk = 0;
+virBuffer fdc_opts = VIR_BUFFER_INITIALIZER;
+char *fdc_opts_str = NULL;
+
+if ((virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_BOOT) || emitBootindex)) {
+/* bootDevs will get translated into either bootindex=N or boot=on
+ * depending on what qemu supports */
+for (i = 0; i < def->os.nBootDevs; i++) {
+switch (def->os.bootDevs[i]) {
+case VIR_DOMAIN_BOOT_CDROM:
+bootCD = i + 1;
+break;
+case VIR_DOMAIN_BOOT_FLOPPY:
+bootFloppy = i + 1;
+break;
+case VIR_DOMAIN_BOOT_DISK:
+bootDisk = i + 1;
+break;
+}
+}
+}
+
+for (i = 0; i < def->ndisks; i++) {
+char *optstr;
+int bootindex = 0;
+virDomainDiskDefPtr disk = def->disks[i];
+bool withDeviceArg = false;
+bool deviceFlagMasked = false;
+
+/* Unless we have -device, then USB disks need special
+   handling */
+if ((disk->bus == VIR_DOMAIN_DISK_BUS_USB) &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
+if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+virCommandAddArg(cmd, "-usbdevice");
+virCommandAddArgFormat(cmd, "disk:%s", disk->src->path);
+ 

[libvirt] [PATCH REPOST 9/9] qemu: Introduce qemuBuildNetCommandLine

[John Ferlan]

Add new function to manage adding the network device options to the
command line removing that task from the mainline qemuBuildCommandLine.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_command.c | 133 +---
 1 file changed, 82 insertions(+), 51 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index caf6f37..20c0147 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7274,6 +7274,84 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
 return ret;
 }
 
+
+/* NOTE: Not using const virDomainDef here since eventually a call is made
+ *   into virSecurityManagerSetTapFDLabel which calls it's driver
+ *   API domainSetSecurityTapFDLabel that doesn't use the const format.
+ */
+static int
+qemuBuildNetCommandLine(virCommandPtr cmd,
+virQEMUDriverPtr driver,
+virDomainDefPtr def,
+virQEMUCapsPtr qemuCaps,
+virNetDevVPortProfileOp vmop,
+bool standalone,
+bool emitBootindex,
+size_t *nnicindexes,
+int **nicindexes,
+int *bootHostdevNet)
+{
+size_t i;
+int last_good_net = -1;
+
+if (!def->nnets) {
+/* If we have -device, then we set -nodefault already */
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
+virCommandAddArgList(cmd, "-net", "none", NULL);
+} else {
+int bootNet = 0;
+
+if (emitBootindex) {
+/* convert  to bootindex since we didn't emit
+ * -boot n
+ */
+for (i = 0; i < def->os.nBootDevs; i++) {
+if (def->os.bootDevs[i] == VIR_DOMAIN_BOOT_NET) {
+bootNet = i + 1;
+break;
+}
+}
+}
+
+for (i = 0; i < def->nnets; i++) {
+virDomainNetDefPtr net = def->nets[i];
+int vlan;
+
+/* VLANs are not used with -netdev, so don't record them */
+if (qemuDomainSupportsNetdev(def, qemuCaps, net))
+vlan = -1;
+else
+vlan = i;
+
+if (qemuBuildInterfaceCommandLine(cmd, driver, def, net,
+  qemuCaps, vlan, bootNet, vmop,
+  standalone, nnicindexes,
+  nicindexes) < 0)
+goto error;
+
+last_good_net = i;
+/* if this interface is a type='hostdev' interface and we
+ * haven't yet added a "bootindex" parameter to an
+ * emulated network device, save the bootindex - hostdev
+ * interface commandlines will be built later on when we
+ * cycle through all the hostdevs, and we'll use it then.
+ */
+if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV 
&&
+*bootHostdevNet == 0) {
+*bootHostdevNet = bootNet;
+}
+bootNet = 0;
+}
+}
+return 0;
+
+ error:
+for (i = 0; last_good_net != -1 && i <= last_good_net; i++)
+virDomainConfNWFilterTeardown(def->nets[i]);
+return -1;
+}
+
+
 char *
 qemuBuildShmemDevStr(virDomainDefPtr def,
  virDomainShmemDefPtr shmem,
@@ -7818,7 +7896,6 @@ qemuBuildCommandLine(virConnectPtr conn,
 size_t i, j;
 char uuid[VIR_UUID_STRING_BUFLEN];
 bool havespice = false;
-int last_good_net = -1;
 virCommandPtr cmd = NULL;
 bool emitBootindex = false;
 int actualSerials = 0;
@@ -7948,54 +8025,10 @@ qemuBuildCommandLine(virConnectPtr conn,
 if (qemuBuildFSDevCommandLine(cmd, def, qemuCaps) < 0)
 goto error;
 
-if (!def->nnets) {
-/* If we have -device, then we set -nodefault already */
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
-virCommandAddArgList(cmd, "-net", "none", NULL);
-} else {
-int bootNet = 0;
-
-if (emitBootindex) {
-/* convert  to bootindex since we didn't emit
- * -boot n
- */
-for (i = 0; i < def->os.nBootDevs; i++) {
-if (def->os.bootDevs[i] == VIR_DOMAIN_BOOT_NET) {
-bootNet = i + 1;
-break;
-}
-}
-}
-
-for (i = 0; i < def->nnets; i++) {
-virDomainNetDefPtr net = def->nets[i];
-int vlan;
-
-/* VLANs are not used with -netdev, so don't record them */
-if (qemuDomainSupportsNetdev(def, qemuCaps, net))
-vlan = -1;
-else
-vlan = i;
-
-if (qemuBuildInterfaceCommandLine(cmd, driver, def, net,
-  qemuCaps, vlan, bootNet, 

[libvirt] [PATCH REPOST 5/9] qemu: Introduce qemuBuildControllerDevCommandLine

[John Ferlan]

Add new function to manage adding the controller -device options to the
command line removing that task from the mainline qemuBuildCommandLine.

Also adjust to using const virDomainDef instead of virDomainDefPtr.
This causes collateral damage in order to modify called APIs to use
the const virDomainDef instead as well.

Signed-off-by: John Ferlan 
---
 src/conf/domain_conf.c |  10 +-
 src/conf/domain_conf.h |   4 +-
 src/qemu/qemu_command.c| 224 ++---
 src/qemu/qemu_command.h|   4 +-
 src/qemu/qemu_domain_address.c |   2 +-
 src/qemu/qemu_domain_address.h |   2 +-
 6 files changed, 132 insertions(+), 114 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d376a2c..11ac707 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13335,8 +13335,9 @@ void 
virDomainControllerInsertPreAlloced(virDomainDefPtr def,
 }
 
 int
-virDomainControllerFind(virDomainDefPtr def,
-int type, int idx)
+virDomainControllerFind(const virDomainDef *def,
+int type,
+int idx)
 {
 size_t i;
 
@@ -13364,8 +13365,9 @@ virDomainControllerFindUnusedIndex(virDomainDefPtr def, 
int type)
 
 
 const char *
-virDomainControllerAliasFind(virDomainDefPtr def,
- int type, int idx)
+virDomainControllerAliasFind(const virDomainDef *def,
+ int type,
+ int idx)
 {
 int contIndex;
 const char *contTypeStr = virDomainControllerTypeToString(type);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index cb7b0e2..8af3c64 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2829,12 +2829,12 @@ int virDomainControllerInsert(virDomainDefPtr def,
 ATTRIBUTE_RETURN_CHECK;
 void virDomainControllerInsertPreAlloced(virDomainDefPtr def,
  virDomainControllerDefPtr controller);
-int virDomainControllerFind(virDomainDefPtr def, int type, int idx);
+int virDomainControllerFind(const virDomainDef *def, int type, int idx);
 int virDomainControllerFindByType(virDomainDefPtr def, int type);
 int virDomainControllerFindByPCIAddress(virDomainDefPtr def,
 virDevicePCIAddressPtr addr);
 virDomainControllerDefPtr virDomainControllerRemove(virDomainDefPtr def, 
size_t i);
-const char *virDomainControllerAliasFind(virDomainDefPtr def,
+const char *virDomainControllerAliasFind(const virDomainDef *def,
  int type, int idx)
 ATTRIBUTE_NONNULL(1);
 
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 87d22f9..bf2cb1a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -276,7 +276,7 @@ char *qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk,
 
 static int
 qemuBuildDeviceAddressStr(virBufferPtr buf,
-  virDomainDefPtr domainDef,
+  const virDomainDef *domainDef,
   virDomainDeviceInfoPtr info,
   virQEMUCapsPtr qemuCaps)
 {
@@ -925,7 +925,7 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk)
  * an error and return false; otherwise, return true.
  */
 bool
-qemuCheckCCWS390AddressSupport(virDomainDefPtr def,
+qemuCheckCCWS390AddressSupport(const virDomainDef *def,
virDomainDeviceInfo info,
virQEMUCapsPtr qemuCaps,
const char *devicename)
@@ -1904,7 +1904,7 @@ qemuControllerModelUSBToCaps(int model)
 
 
 static int
-qemuBuildUSBControllerDevStr(virDomainDefPtr domainDef,
+qemuBuildUSBControllerDevStr(const virDomainDef *domainDef,
  virDomainControllerDefPtr def,
  virQEMUCapsPtr qemuCaps,
  virBuffer *buf)
@@ -1942,7 +1942,7 @@ qemuBuildUSBControllerDevStr(virDomainDefPtr domainDef,
 }
 
 char *
-qemuBuildControllerDevStr(virDomainDefPtr domainDef,
+qemuBuildControllerDevStr(const virDomainDef *domainDef,
   virDomainControllerDefPtr def,
   virQEMUCapsPtr qemuCaps,
   int *nusbcontroller)
@@ -2300,6 +2300,120 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
 }
 
 
+static int
+qemuBuildControllerDevCommandLine(virCommandPtr cmd,
+  const virDomainDef *def,
+  virQEMUCapsPtr qemuCaps)
+{
+size_t i, j;
+int usbcontroller = 0;
+bool usblegacy = false;
+int contOrder[] = {
+/*
+ * List of controller types that we add commandline args for,
+ * *in the order we want to add them*.
+ *
+ * The floppy controller is implicit on PIIX4 and older Q35
+ * machines. For newer Q35 machines it is added out of the
+ * controllers loop, 

[libvirt] [PATCH REPOST 3/9] qemu: Introduce qemuBuildBootCommandLine

[John Ferlan]

Add new function to manage adding the -boot options to the command
line removing that task from the mainline qemuBuildCommandLine.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_command.c | 282 +---
 1 file changed, 150 insertions(+), 132 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 3b3c958..2fd91a4 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4966,6 +4966,155 @@ qemuBuildPMCommandLine(virCommandPtr cmd,
 
 
 static int
+qemuBuildBootCommandLine(virCommandPtr cmd,
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps,
+ bool *emitBootindex)
+{
+size_t i;
+virBuffer boot_buf = VIR_BUFFER_INITIALIZER;
+char *boot_order_str = NULL, *boot_opts_str = NULL;
+
+/*
+ * We prefer using explicit bootindex=N parameters for predictable
+ * results even though domain XML doesn't use per device boot elements.
+ * However, we can't use bootindex if boot menu was requested.
+ */
+if (!def->os.nBootDevs) {
+/* def->os.nBootDevs is guaranteed to be > 0 unless per-device boot
+ * configuration is used
+ */
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("hypervisor lacks deviceboot feature"));
+goto error;
+}
+*emitBootindex = true;
+} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX) &&
+   (def->os.bootmenu != VIR_TRISTATE_BOOL_YES ||
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOT_MENU))) {
+*emitBootindex = true;
+}
+
+if (!*emitBootindex) {
+char boot[VIR_DOMAIN_BOOT_LAST+1];
+
+for (i = 0; i < def->os.nBootDevs; i++) {
+switch (def->os.bootDevs[i]) {
+case VIR_DOMAIN_BOOT_CDROM:
+boot[i] = 'd';
+break;
+case VIR_DOMAIN_BOOT_FLOPPY:
+boot[i] = 'a';
+break;
+case VIR_DOMAIN_BOOT_DISK:
+boot[i] = 'c';
+break;
+case VIR_DOMAIN_BOOT_NET:
+boot[i] = 'n';
+break;
+default:
+boot[i] = 'c';
+break;
+}
+}
+boot[def->os.nBootDevs] = '\0';
+
+virBufferAsprintf(_buf, "%s", boot);
+if (virBufferCheckError(_buf) < 0)
+goto error;
+boot_order_str = virBufferContentAndReset(_buf);
+}
+
+if (def->os.bootmenu) {
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOT_MENU)) {
+if (def->os.bootmenu == VIR_TRISTATE_BOOL_YES)
+virBufferAddLit(_buf, "menu=on,");
+else
+virBufferAddLit(_buf, "menu=off,");
+} else {
+/* We cannot emit an error when bootmenu is enabled but
+ * unsupported because of backward compatibility */
+VIR_WARN("bootmenu is enabled but not "
+ "supported by this QEMU binary");
+}
+}
+
+if (def->os.bios.rt_set) {
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_REBOOT_TIMEOUT)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("reboot timeout is not supported "
+ "by this QEMU binary"));
+goto error;
+}
+
+virBufferAsprintf(_buf,
+  "reboot-timeout=%d,",
+  def->os.bios.rt_delay);
+}
+
+if (def->os.bm_timeout_set) {
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPLASH_TIMEOUT)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("splash timeout is not supported "
+ "by this QEMU binary"));
+goto error;
+}
+
+virBufferAsprintf(_buf, "splash-time=%u,", def->os.bm_timeout);
+}
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOT_STRICT))
+virBufferAddLit(_buf, "strict=on,");
+
+virBufferTrim(_buf, ",", -1);
+
+if (virBufferCheckError(_buf) < 0)
+goto error;
+
+boot_opts_str = virBufferContentAndReset(_buf);
+if (boot_order_str || boot_opts_str) {
+virCommandAddArg(cmd, "-boot");
+
+if (boot_order_str && boot_opts_str) {
+virCommandAddArgFormat(cmd, "order=%s,%s",
+   boot_order_str, boot_opts_str);
+} else if (boot_order_str) {
+virCommandAddArg(cmd, boot_order_str);
+} else if (boot_opts_str) {
+virCommandAddArg(cmd, boot_opts_str);
+}
+}
+VIR_FREE(boot_opts_str);
+VIR_FREE(boot_order_str);
+
+if (def->os.kernel)
+virCommandAddArgList(cmd, "-kernel", def->os.kernel, NULL);
+if (def->os.initrd)
+

[libvirt] [PATCH REPOST 4/9] qemu: Introduce qemuBuildGlobalControllerCommandLine

[John Ferlan]

Add new function to manage adding the -global controller options to
the command line removing that task from the mainline qemuBuildCommandLine.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_command.c | 106 +++-
 1 file changed, 60 insertions(+), 46 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2fd91a4..87d22f9 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5115,6 +5115,64 @@ qemuBuildBootCommandLine(virCommandPtr cmd,
 
 
 static int
+qemuBuildGlobalControllerCommandLine(virCommandPtr cmd,
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
+{
+size_t i;
+
+for (i = 0; i < def->ncontrollers; i++) {
+virDomainControllerDefPtr cont = def->controllers[i];
+if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
+cont->opts.pciopts.pcihole64) {
+const char *hoststr = NULL;
+bool cap = false;
+bool machine = false;
+
+switch (cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+hoststr = "i440FX-pcihost";
+cap = virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_I440FX_PCI_HOLE64_SIZE);
+machine = qemuDomainMachineIsI440FX(def);
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+hoststr = "q35-pcihost";
+cap = virQEMUCapsGet(qemuCaps, QEMU_CAPS_Q35_PCI_HOLE64_SIZE);
+machine = qemuDomainMachineIsQ35(def);
+break;
+
+default:
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("64-bit PCI hole setting is only for root"
+ " PCI controllers"));
+return -1;
+}
+
+if (!machine) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Setting the 64-bit PCI hole size is not "
+ "supported for machine '%s'"), def->os.machine);
+return -1;
+}
+if (!cap) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("64-bit PCI hole size setting is not 
supported "
+ "with this QEMU binary"));
+return -1;
+}
+
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd, "%s.pci-hole64-size=%luK", hoststr,
+   cont->opts.pciopts.pcihole64size);
+}
+}
+
+return 0;
+}
+
+
+static int
 qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
 const virDomainDef *def,
 virBufferPtr buf,
@@ -7566,52 +7624,8 @@ qemuBuildCommandLine(virConnectPtr conn,
 if (qemuBuildBootCommandLine(cmd, def, qemuCaps, ) < 0)
 goto error;
 
-for (i = 0; i < def->ncontrollers; i++) {
-virDomainControllerDefPtr cont = def->controllers[i];
-if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
-cont->opts.pciopts.pcihole64) {
-const char *hoststr = NULL;
-bool cap = false;
-bool machine = false;
-
-switch (cont->model) {
-case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
-hoststr = "i440FX-pcihost";
-cap = virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_I440FX_PCI_HOLE64_SIZE);
-machine = qemuDomainMachineIsI440FX(def);
-break;
-
-case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
-hoststr = "q35-pcihost";
-cap = virQEMUCapsGet(qemuCaps, QEMU_CAPS_Q35_PCI_HOLE64_SIZE);
-machine = qemuDomainMachineIsQ35(def);
-break;
-
-default:
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("64-bit PCI hole setting is only for root"
- " PCI controllers"));
-goto error;
-}
-
-if (!machine) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Setting the 64-bit PCI hole size is not "
- "supported for machine '%s'"), def->os.machine);
-goto error;
-}
-if (!cap) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("64-bit PCI hole size setting is not 
supported "
- "with this QEMU binary"));
-goto error;
-}
-
-virCommandAddArg(cmd, "-global");
-virCommandAddArgFormat(cmd, "%s.pci-hole64-size=%luK", hoststr,
-   cont->opts.pciopts.pcihole64size);
-}
-}
+if 

[libvirt] [PATCH REPOST 8/9] qemu: Introduce qemuBuildFSDevCommandLine

[John Ferlan]

Add new function to manage adding the -fsdev options to the
command line removing that task from the mainline qemuBuildCommandLine.

Since both qemuBuildFSStr and qemuBuildFSDevStr are local, make them
static and fix their prototypes to use the const virDomainDef as well.
Make some minor formatting changes for long lines.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_command.c | 74 ++---
 src/qemu/qemu_command.h |  6 +---
 2 files changed, 46 insertions(+), 34 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 372f84f..caf6f37 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1928,8 +1928,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
 }
 
 
-char *qemuBuildFSStr(virDomainFSDefPtr fs,
- virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED)
+static char *
+qemuBuildFSStr(virDomainFSDefPtr fs,
+   virQEMUCapsPtr qemuCaps)
 {
 virBuffer opt = VIR_BUFFER_INITIALIZER;
 const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver);
@@ -2002,8 +2003,8 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs,
 }
 
 
-char *
-qemuBuildFSDevStr(virDomainDefPtr def,
+static char *
+qemuBuildFSDevStr(const virDomainDef *def,
   virDomainFSDefPtr fs,
   virQEMUCapsPtr qemuCaps)
 {
@@ -2021,7 +2022,8 @@ qemuBuildFSDevStr(virDomainDefPtr def,
 virBufferAddLit(, "virtio-9p-pci");
 
 virBufferAsprintf(, ",id=%s", fs->info.alias);
-virBufferAsprintf(, ",fsdev=%s%s", QEMU_FSDEV_HOST_PREFIX, 
fs->info.alias);
+virBufferAsprintf(, ",fsdev=%s%s",
+  QEMU_FSDEV_HOST_PREFIX, fs->info.alias);
 virBufferAsprintf(, ",mount_tag=%s", fs->dst);
 
 if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps) < 0)
@@ -2039,6 +2041,42 @@ qemuBuildFSDevStr(virDomainDefPtr def,
 
 
 static int
+qemuBuildFSDevCommandLine(virCommandPtr cmd,
+  const virDomainDef *def,
+  virQEMUCapsPtr qemuCaps)
+{
+size_t i;
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) {
+for (i = 0; i < def->nfss; i++) {
+char *optstr;
+virDomainFSDefPtr fs = def->fss[i];
+
+virCommandAddArg(cmd, "-fsdev");
+if (!(optstr = qemuBuildFSStr(fs, qemuCaps)))
+return -1;
+virCommandAddArg(cmd, optstr);
+VIR_FREE(optstr);
+
+virCommandAddArg(cmd, "-device");
+if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps)))
+return -1;
+virCommandAddArg(cmd, optstr);
+VIR_FREE(optstr);
+}
+} else {
+if (def->nfss) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("filesystem passthrough not supported by this 
QEMU"));
+return -1;
+}
+}
+
+return 0;
+}
+
+
+static int
 qemuControllerModelUSBToCaps(int model)
 {
 switch (model) {
@@ -7907,30 +7945,8 @@ qemuBuildCommandLine(virConnectPtr conn,
   emitBootindex) < 0)
 goto error;
 
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) {
-for (i = 0; i < def->nfss; i++) {
-char *optstr;
-virDomainFSDefPtr fs = def->fss[i];
-
-virCommandAddArg(cmd, "-fsdev");
-if (!(optstr = qemuBuildFSStr(fs, qemuCaps)))
-goto error;
-virCommandAddArg(cmd, optstr);
-VIR_FREE(optstr);
-
-virCommandAddArg(cmd, "-device");
-if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps)))
-goto error;
-virCommandAddArg(cmd, optstr);
-VIR_FREE(optstr);
-}
-} else {
-if (def->nfss) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("filesystem passthrough not supported by this 
QEMU"));
-goto error;
-}
-}
+if (qemuBuildFSDevCommandLine(cmd, def, qemuCaps) < 0)
+goto error;
 
 if (!def->nnets) {
 /* If we have -device, then we set -nodefault already */
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index b10fc29..6e22872 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -115,17 +115,13 @@ char *qemuBuildDriveStr(virConnectPtr conn,
 virDomainDiskDefPtr disk,
 bool bootable,
 virQEMUCapsPtr qemuCaps);
-char *qemuBuildFSStr(virDomainFSDefPtr fs,
- virQEMUCapsPtr qemuCaps);
 
 /* Current, best practice */
 char *qemuBuildDriveDevStr(const virDomainDef *def,
virDomainDiskDefPtr disk,
int bootindex,
virQEMUCapsPtr qemuCaps);
-char *qemuBuildFSDevStr(virDomainDefPtr domainDef,
-virDomainFSDefPtr fs,
-   

[libvirt] [PATCH REPOST 1/9] qemu: Introduce qemuBuildClockCommandLine

[John Ferlan]

Add new function to manage adding the '-clock' options to the command
line removing that task from the mainline qemuBuildCommandLine.

Also includes some minor formatting cleanups.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_command.c | 300 ++--
 1 file changed, 160 insertions(+), 140 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 000c29d..c890b75 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4621,6 +4621,7 @@ qemuBuildSgaCommandLine(virCommandPtr cmd,
 static char *
 qemuBuildClockArgStr(virDomainClockDefPtr def)
 {
+size_t i;
 virBuffer buf = VIR_BUFFER_INITIALIZER;
 
 switch (def->offset) {
@@ -4685,8 +4686,8 @@ qemuBuildClockArgStr(virDomainClockDefPtr def)
 goto error;
 }
 
-/* Look for an 'rtc' timer element, and add in appropriate clock= and 
driftfix= */
-size_t i;
+/* Look for an 'rtc' timer element, and add in appropriate
+ * clock= and driftfix= */
 for (i = 0; i < def->ntimers; i++) {
 if (def->timers[i]->name == VIR_DOMAIN_TIMER_NAME_RTC) {
 switch (def->timers[i]->track) {
@@ -4736,6 +4737,161 @@ qemuBuildClockArgStr(virDomainClockDefPtr def)
 return NULL;
 }
 
+
+/* NOTE: Building of commands can change def->clock->data.* values, so
+ *   virDomainDef is not const here.
+ */
+static int
+qemuBuildClockCommandLine(virCommandPtr cmd,
+  virDomainDefPtr def,
+  virQEMUCapsPtr qemuCaps)
+{
+size_t i;
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_RTC)) {
+char *rtcopt;
+virCommandAddArg(cmd, "-rtc");
+if (!(rtcopt = qemuBuildClockArgStr(>clock)))
+return -1;
+virCommandAddArg(cmd, rtcopt);
+VIR_FREE(rtcopt);
+} else {
+switch (def->clock.offset) {
+case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME:
+case VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE:
+virCommandAddArg(cmd, "-localtime");
+break;
+
+case VIR_DOMAIN_CLOCK_OFFSET_UTC:
+/* Nothing, its the default */
+break;
+
+default:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unsupported clock offset '%s'"),
+   
virDomainClockOffsetTypeToString(def->clock.offset));
+return -1;
+}
+}
+
+if (def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE &&
+def->clock.data.timezone) {
+virCommandAddEnvPair(cmd, "TZ", def->clock.data.timezone);
+}
+
+for (i = 0; i < def->clock.ntimers; i++) {
+switch ((virDomainTimerNameType) def->clock.timers[i]->name) {
+case VIR_DOMAIN_TIMER_NAME_PLATFORM:
+case VIR_DOMAIN_TIMER_NAME_TSC:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unsupported timer type (name) '%s'"),
+   
virDomainTimerNameTypeToString(def->clock.timers[i]->name));
+return -1;
+
+case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
+case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
+/* Timers above are handled when building -cpu.  */
+case VIR_DOMAIN_TIMER_NAME_LAST:
+break;
+
+case VIR_DOMAIN_TIMER_NAME_RTC:
+/* This has already been taken care of (in qemuBuildClockArgStr)
+   if QEMU_CAPS_RTC is set (mutually exclusive with
+   QEMUD_FLAG_RTC_TD_HACK) */
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_RTC_TD_HACK)) {
+switch (def->clock.timers[i]->tickpolicy) {
+case -1:
+case VIR_DOMAIN_TIMER_TICKPOLICY_DELAY:
+/* the default - do nothing */
+break;
+case VIR_DOMAIN_TIMER_TICKPOLICY_CATCHUP:
+virCommandAddArg(cmd, "-rtc-td-hack");
+break;
+case VIR_DOMAIN_TIMER_TICKPOLICY_MERGE:
+case VIR_DOMAIN_TIMER_TICKPOLICY_DISCARD:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unsupported rtc tickpolicy '%s'"),
+   
virDomainTimerTickpolicyTypeToString(def->clock.timers[i]->tickpolicy));
+return -1;
+}
+} else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_RTC) &&
+   (def->clock.timers[i]->tickpolicy
+!= VIR_DOMAIN_TIMER_TICKPOLICY_DELAY) &&
+   (def->clock.timers[i]->tickpolicy != -1)) {
+/* a non-default rtc policy was given, but there is no
+   way to implement it in this version of qemu */
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unsupported rtc tickpolicy '%s'"),
+   

[libvirt] [PATCH REPOST 6/9] qemu: Introduce qemuBuildHubCommandLine

[John Ferlan]

Add new function to manage adding the hub -device options to the
command line removing that task from the mainline qemuBuildCommandLine.

Also make qemuBuildHubDevStr static to the module since it's only
used here.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_command.c | 38 ++
 src/qemu/qemu_command.h |  3 ---
 2 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bf2cb1a..ef94af8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3744,8 +3744,8 @@ qemuBuildUSBHostdevDevStr(virDomainDefPtr def,
 }
 
 
-char *
-qemuBuildHubDevStr(virDomainDefPtr def,
+static char *
+qemuBuildHubDevStr(const virDomainDef *def,
virDomainHubDefPtr dev,
virQEMUCapsPtr qemuCaps)
 {
@@ -3780,6 +3780,28 @@ qemuBuildHubDevStr(virDomainDefPtr def,
 }
 
 
+static int
+qemuBuildHubCommandLine(virCommandPtr cmd,
+const virDomainDef *def,
+virQEMUCapsPtr qemuCaps)
+{
+size_t i;
+
+for (i = 0; i < def->nhubs; i++) {
+virDomainHubDefPtr hub = def->hubs[i];
+char *optstr;
+
+virCommandAddArg(cmd, "-device");
+if (!(optstr = qemuBuildHubDevStr(def, hub, qemuCaps)))
+return -1;
+virCommandAddArg(cmd, optstr);
+VIR_FREE(optstr);
+}
+
+return 0;
+}
+
+
 char *
 qemuBuildUSBHostdevUSBDevStr(virDomainHostdevDefPtr dev)
 {
@@ -7718,16 +7740,8 @@ qemuBuildCommandLine(virConnectPtr conn,
 if (qemuBuildControllerDevCommandLine(cmd, def, qemuCaps) < 0)
 goto error;
 
-for (i = 0; i < def->nhubs; i++) {
-virDomainHubDefPtr hub = def->hubs[i];
-char *optstr;
-
-virCommandAddArg(cmd, "-device");
-if (!(optstr = qemuBuildHubDevStr(def, hub, qemuCaps)))
-goto error;
-virCommandAddArg(cmd, optstr);
-VIR_FREE(optstr);
-}
+if (qemuBuildHubCommandLine(cmd, def, qemuCaps) < 0)
+goto error;
 
 if ((virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_BOOT) || emitBootindex)) {
 /* bootDevs will get translated into either bootindex=N or boot=on
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index ec18869..ed4f5bd 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -205,9 +205,6 @@ char *qemuBuildSCSIHostdevDevStr(virDomainDefPtr def,
  virDomainHostdevDefPtr dev,
  virQEMUCapsPtr qemuCaps);
 
-char *qemuBuildHubDevStr(virDomainDefPtr def,
- virDomainHubDefPtr dev,
- virQEMUCapsPtr qemuCaps);
 char *qemuBuildRedirdevDevStr(virDomainDefPtr def,
   virDomainRedirdevDefPtr dev,
   virQEMUCapsPtr qemuCaps);
-- 
2.5.0

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

[libvirt] [PATCH REPOST 0/9] Reorganize qemu_command.c in smaller piles

[John Ferlan]

Rather than hope for a review of 25 patches or creating some remote
place to place a branch - I'll repost the original:

http://www.redhat.com/archives/libvir-list/2016-February/msg00975.html

in smaller chunks. No differences in these patches other than a rebase
to top of tree a short while ago (commit id 'cf091094a'). There were also
no conflicts from changes since initial posting.

John Ferlan (9):
  qemu: Introduce qemuBuildClockCommandLine
  qemu: Introduce qemuBuildPMCommandLine
  qemu: Introduce qemuBuildBootCommandLine
  qemu: Introduce qemuBuildGlobalControllerCommandLine
  qemu: Introduce qemuBuildControllerDevCommandLine
  qemu: Introduce qemuBuildHubCommandLine
  qemu: Introduce qemuBuildDiskDriveCommandLine
  qemu: Introduce qemuBuildFSDevCommandLine
  qemu: Introduce qemuBuildNetCommandLine

 src/conf/domain_conf.c |   14 +-
 src/conf/domain_conf.h |8 +-
 src/qemu/qemu_command.c| 5488 +---
 src/qemu/qemu_command.h|   15 +-
 src/qemu/qemu_domain_address.c |2 +-
 src/qemu/qemu_domain_address.h |2 +-
 6 files changed, 2843 insertions(+), 2686 deletions(-)

-- 
2.5.0

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

[libvirt] [PATCH 15/10] secret: Change virSecretDef variable names

[John Ferlan]

Change 'ephemeral' to 'isephemeral' and 'private' to 'isprivate' since
both are bools.

Signed-off-by: John Ferlan 
---
 src/conf/secret_conf.c| 26 +-
 src/conf/secret_conf.h|  4 ++--
 src/secret/secret_driver.c| 10 +-
 src/storage/storage_backend.c |  4 ++--
 4 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
index 4d0cb9c..6d4de7c 100644
--- a/src/conf/secret_conf.c
+++ b/src/conf/secret_conf.c
@@ -403,7 +403,7 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets,
 goto cleanup;
 }
 
-if (secret->def->private && !def->private) {
+if (secret->def->isprivate && !def->isprivate) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot change private flag on existing secret"));
 goto cleanup;
@@ -553,17 +553,17 @@ virSecretObjMatchFlags(virSecretObjPtr secret,
 /* filter by whether it's ephemeral */
 if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL) &&
 !((MATCH(VIR_CONNECT_LIST_SECRETS_EPHEMERAL) &&
-   secret->def->ephemeral) ||
+   secret->def->isephemeral) ||
   (MATCH(VIR_CONNECT_LIST_SECRETS_NO_EPHEMERAL) &&
-   !secret->def->ephemeral)))
+   !secret->def->isephemeral)))
 return false;
 
 /* filter by whether it's private */
 if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_PRIVATE) &&
 !((MATCH(VIR_CONNECT_LIST_SECRETS_PRIVATE) &&
-   secret->def->private) ||
+   secret->def->isprivate) ||
   (MATCH(VIR_CONNECT_LIST_SECRETS_NO_PRIVATE) &&
-   !secret->def->private)))
+   !secret->def->isprivate)))
 return false;
 
 return true;
@@ -699,7 +699,7 @@ virSecretObjDeleteConfig(virSecretObjPtr secret)
 /* When the XML is missing, we'll still allow the attempt to
  * delete the secret data. Without a configFile, the secret
won't be loaded again, so we have succeeded already. */
-if (!secret->def->ephemeral &&
+if (!secret->def->isephemeral &&
 unlink(secret->configFile) < 0 && errno != ENOENT)
 return -1;
 
@@ -838,7 +838,7 @@ virSecretObjSetValue(virSecretObjPtr secret,
 secret->value = new_value;
 secret->value_size = value_size;
 
-if (!secret->def->ephemeral && virSecretObjSaveData(secret) < 0)
+if (!secret->def->isephemeral && virSecretObjSaveData(secret) < 0)
 goto error;
 
 /* Saved successfully - drop old value */
@@ -995,9 +995,9 @@ secretXMLParseNode(xmlDocPtr xml, xmlNodePtr root)
 prop = virXPathString("string(./@ephemeral)", ctxt);
 if (prop != NULL) {
 if (STREQ(prop, "yes")) {
-def->ephemeral = true;
+def->isephemeral = true;
 } else if (STREQ(prop, "no")) {
-def->ephemeral = false;
+def->isephemeral = false;
 } else {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("invalid value of 'ephemeral'"));
@@ -1009,9 +1009,9 @@ secretXMLParseNode(xmlDocPtr xml, xmlNodePtr root)
 prop = virXPathString("string(./@private)", ctxt);
 if (prop != NULL) {
 if (STREQ(prop, "yes")) {
-def->private = true;
+def->isprivate = true;
 } else if (STREQ(prop, "no")) {
-def->private = false;
+def->isprivate = false;
 } else {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("invalid value of 'private'"));
@@ -1137,8 +1137,8 @@ virSecretDefFormat(const virSecretDef *def)
 char uuidstr[VIR_UUID_STRING_BUFLEN];
 
 virBufferAsprintf(, "\n",
-  def->ephemeral ? "yes" : "no",
-  def->private ? "yes" : "no");
+  def->isephemeral ? "yes" : "no",
+  def->isprivate ? "yes" : "no");
 
 uuid = def->uuid;
 virUUIDFormat(uuid, uuidstr);
diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h
index be3bff9..a64a4d9 100644
--- a/src/conf/secret_conf.h
+++ b/src/conf/secret_conf.h
@@ -32,8 +32,8 @@ VIR_ENUM_DECL(virSecretUsage)
 typedef struct _virSecretDef virSecretDef;
 typedef virSecretDef *virSecretDefPtr;
 struct _virSecretDef {
-bool ephemeral;
-bool private;
+bool isephemeral;
+bool isprivate;
 unsigned char uuid[VIR_UUID_BUFLEN];
 char *description;  /* May be NULL */
 int usage_type;
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index ab58115..cdf8d80 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -230,23 +230,23 @@ secretDefineXML(virConnectPtr conn,
driver->configDir, )))
 goto cleanup;
 
-if (!new_attrs->ephemeral) {
+if (!new_attrs->isephemeral) {
 if (secretEnsureDirectory() < 0)
 goto cleanup;
 
-if (backup 

[libvirt] [PATCH 14/10] secret: Introduce virSecretObjGetValue and virSecretObjGetValueSize

[John Ferlan]

Introduce the final accessor's to _virSecretObject data and move the
structure from secret_conf.h to secret_conf.c

The virSecretObjSetValue logic will handle setting both the secret
value and the value_size. Some slight adjustments to the error path
over what was in secretSetValue were made.

Additionally, a slight logic change in secretGetValue where we'll
check for the internalFlags and error out before checking for
and erroring out for a NULL secret->value. That way, it won't be
obvious to anyone that the secret value wasn't set rather they'll
just know they cannot get the secret value since it's private.

Signed-off-by: John Ferlan 
---
 src/conf/secret_conf.c | 84 ++
 src/conf/secret_conf.h | 17 +-
 src/libvirt_private.syms   |  4 +++
 src/secret/secret_driver.c | 50 ---
 4 files changed, 104 insertions(+), 51 deletions(-)

diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
index 0410328..4d0cb9c 100644
--- a/src/conf/secret_conf.c
+++ b/src/conf/secret_conf.c
@@ -45,6 +45,14 @@ VIR_LOG_INIT("conf.secret_conf");
 VIR_ENUM_IMPL(virSecretUsage, VIR_SECRET_USAGE_TYPE_LAST,
   "none", "volume", "ceph", "iscsi")
 
+struct _virSecretObj {
+virObjectLockable parent;
+char *configFile;
+char *base64File;
+virSecretDefPtr def;
+unsigned char *value;   /* May be NULL */
+size_t value_size;
+};
 
 static virClassPtr virSecretObjClass;
 static void virSecretObjDispose(void *obj);
@@ -790,6 +798,82 @@ virSecretObjSetDef(virSecretObjPtr secret,
 }
 
 
+unsigned char *
+virSecretObjGetValue(virSecretObjPtr secret)
+{
+unsigned char *ret = NULL;
+
+if (!secret->value) {
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+virUUIDFormat(secret->def->uuid, uuidstr);
+virReportError(VIR_ERR_NO_SECRET,
+   _("secret '%s' does not have a value"), uuidstr);
+goto cleanup;
+}
+
+if (VIR_ALLOC_N(ret, secret->value_size) < 0)
+goto cleanup;
+memcpy(ret, secret->value, secret->value_size);
+
+ cleanup:
+return ret;
+}
+
+
+int
+virSecretObjSetValue(virSecretObjPtr secret,
+ const unsigned char *value,
+ size_t value_size)
+{
+unsigned char *old_value, *new_value;
+size_t old_value_size;
+
+if (VIR_ALLOC_N(new_value, value_size) < 0)
+return -1;
+
+old_value = secret->value;
+old_value_size = secret->value_size;
+
+memcpy(new_value, value, value_size);
+secret->value = new_value;
+secret->value_size = value_size;
+
+if (!secret->def->ephemeral && virSecretObjSaveData(secret) < 0)
+goto error;
+
+/* Saved successfully - drop old value */
+if (old_value) {
+memset(old_value, 0, old_value_size);
+VIR_FREE(old_value);
+}
+
+return  0;
+
+ error:
+/* Error - restore previous state and free new value */
+secret->value = old_value;
+secret->value_size = old_value_size;
+memset(new_value, 0, value_size);
+VIR_FREE(new_value);
+return -1;
+}
+
+
+size_t
+virSecretObjGetValueSize(virSecretObjPtr secret)
+{
+return secret->value_size;
+}
+
+
+void
+virSecretObjSetValueSize(virSecretObjPtr secret,
+ size_t value_size)
+{
+secret->value_size = value_size;
+}
+
+
 void
 virSecretDefFree(virSecretDefPtr def)
 {
diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h
index ce714c1..be3bff9 100644
--- a/src/conf/secret_conf.h
+++ b/src/conf/secret_conf.h
@@ -46,14 +46,6 @@ struct _virSecretDef {
 
 typedef struct _virSecretObj virSecretObj;
 typedef virSecretObj *virSecretObjPtr;
-struct _virSecretObj {
-virObjectLockable parent;
-char *configFile;
-char *base64File;
-virSecretDefPtr def;
-unsigned char *value;   /* May be NULL */
-size_t value_size;
-};
 
 virSecretObjPtr virSecretObjNew(void);
 
@@ -126,6 +118,15 @@ virSecretDefPtr virSecretObjGetDef(virSecretObjPtr secret);
 
 void virSecretObjSetDef(virSecretObjPtr secret, virSecretDefPtr def);
 
+unsigned char *virSecretObjGetValue(virSecretObjPtr secret);
+
+int virSecretObjSetValue(virSecretObjPtr secret,
+ const unsigned char *value, size_t value_size);
+
+size_t virSecretObjGetValueSize(virSecretObjPtr secret);
+
+void virSecretObjSetValueSize(virSecretObjPtr secret, size_t value_size);
+
 void virSecretDefFree(virSecretDefPtr def);
 virSecretDefPtr virSecretDefParseString(const char *xml);
 virSecretDefPtr virSecretDefParseFile(const char *filename);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3a417f0..15370f6 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -791,6 +791,8 @@ virSecretObjDeleteConfig;
 virSecretObjDeleteData;
 virSecretObjEndAPI;
 virSecretObjGetDef;
+virSecretObjGetValue;
+virSecretObjGetValueSize;
 virSecretObjListAdd;
 virSecretObjListExport;
 

[libvirt] [PATCH 13/10] secret: Introduce virSecretObjGetDef and virSecretObjSetDef

[John Ferlan]

Introduce fetch and set accessor to the secretObj->def field for usage
by the driver to avoid the driver needing to know the format of virSecretObj

Signed-off-by: John Ferlan 
---
 src/conf/secret_conf.c | 15 +
 src/conf/secret_conf.h |  4 
 src/libvirt_private.syms   |  2 ++
 src/secret/secret_driver.c | 54 --
 4 files changed, 54 insertions(+), 21 deletions(-)

diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
index 3528288..0410328 100644
--- a/src/conf/secret_conf.c
+++ b/src/conf/secret_conf.c
@@ -775,6 +775,21 @@ virSecretObjSaveData(virSecretObjPtr secret)
 }
 
 
+virSecretDefPtr
+virSecretObjGetDef(virSecretObjPtr secret)
+{
+return secret->def;
+}
+
+
+void
+virSecretObjSetDef(virSecretObjPtr secret,
+   virSecretDefPtr def)
+{
+secret->def = def;
+}
+
+
 void
 virSecretDefFree(virSecretDefPtr def)
 {
diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h
index d40b510..ce714c1 100644
--- a/src/conf/secret_conf.h
+++ b/src/conf/secret_conf.h
@@ -122,6 +122,10 @@ int virSecretObjSaveConfig(virSecretObjPtr secret);
 
 int virSecretObjSaveData(virSecretObjPtr secret);
 
+virSecretDefPtr virSecretObjGetDef(virSecretObjPtr secret);
+
+void virSecretObjSetDef(virSecretObjPtr secret, virSecretDefPtr def);
+
 void virSecretDefFree(virSecretDefPtr def);
 virSecretDefPtr virSecretDefParseString(const char *xml);
 virSecretDefPtr virSecretDefParseFile(const char *filename);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9e1a09e..3a417f0 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -790,6 +790,7 @@ virSecretLoadAllConfigs;
 virSecretObjDeleteConfig;
 virSecretObjDeleteData;
 virSecretObjEndAPI;
+virSecretObjGetDef;
 virSecretObjListAdd;
 virSecretObjListExport;
 virSecretObjListFindByUsage;
@@ -800,6 +801,7 @@ virSecretObjListNumOfSecrets;
 virSecretObjListRemove;
 virSecretObjSaveConfig;
 virSecretObjSaveData;
+virSecretObjSetDef;
 virSecretUsageIDForDef;
 virSecretUsageTypeFromString;
 virSecretUsageTypeToString;
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 1b4dfea..676c02e 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -152,6 +152,7 @@ secretLookupByUUID(virConnectPtr conn,
 {
 virSecretPtr ret = NULL;
 virSecretObjPtr secret;
+virSecretDefPtr def;
 
 if (!(secret = virSecretObjListFindByUUID(driver->secrets, uuid))) {
 char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -161,13 +162,14 @@ secretLookupByUUID(virConnectPtr conn,
 goto cleanup;
 }
 
-if (virSecretLookupByUUIDEnsureACL(conn, secret->def) < 0)
+def = virSecretObjGetDef(secret);
+if (virSecretLookupByUUIDEnsureACL(conn, def) < 0)
 goto cleanup;
 
 ret = virGetSecret(conn,
-   secret->def->uuid,
-   secret->def->usage_type,
-   virSecretUsageIDForDef(secret->def));
+   def->uuid,
+   def->usage_type,
+   virSecretUsageIDForDef(def));
 
  cleanup:
 virSecretObjEndAPI();
@@ -182,6 +184,7 @@ secretLookupByUsage(virConnectPtr conn,
 {
 virSecretPtr ret = NULL;
 virSecretObjPtr secret;
+virSecretDefPtr def;
 
 if (!(secret = virSecretObjListFindByUsage(driver->secrets,
usageType, usageID))) {
@@ -190,13 +193,14 @@ secretLookupByUsage(virConnectPtr conn,
 goto cleanup;
 }
 
-if (virSecretLookupByUsageEnsureACL(conn, secret->def) < 0)
+def = virSecretObjGetDef(secret);
+if (virSecretLookupByUsageEnsureACL(conn, def) < 0)
 goto cleanup;
 
 ret = virGetSecret(conn,
-   secret->def->uuid,
-   secret->def->usage_type,
-   virSecretUsageIDForDef(secret->def));
+   def->uuid,
+   def->usage_type,
+   virSecretUsageIDForDef(def));
 
  cleanup:
 virSecretObjEndAPI();
@@ -249,22 +253,22 @@ secretDefineXML(virConnectPtr conn,
 virSecretObjDeleteData(secret);
 }
 /* Saved successfully - drop old values */
-new_attrs = NULL;
 virSecretDefFree(backup);
 
 ret = virGetSecret(conn,
-   secret->def->uuid,
-   secret->def->usage_type,
-   virSecretUsageIDForDef(secret->def));
+   new_attrs->uuid,
+   new_attrs->usage_type,
+   virSecretUsageIDForDef(new_attrs));
+new_attrs = NULL;
 goto cleanup;
 
  restore_backup:
 /* If we have a backup, then secret was defined before, so just restore
- * the backup. The current secret->def (new_attrs) will be handled below.
+ * the backup. The current (new_attrs) will be handled below.
  * Otherwise, this is a new secret, thus remove it.
 

[libvirt] [PATCH 12/10] secret: Introduce virSecretObjSaveConfig and virSecretObjSaveData

[John Ferlan]

Move and rename the secretRewriteFile, secretSaveDef, and secretSaveValue
from secret_driver to secret_conf

Need to make some slight adjustments since the secretSave* functions
called secretEnsureDirectory, but otherwise mostly just a move of code.

Signed-off-by: John Ferlan 
---
 src/conf/secret_conf.c | 69 +++
 src/conf/secret_conf.h |  4 +++
 src/libvirt_private.syms   |  2 ++
 src/secret/secret_driver.c | 90 +++---
 4 files changed, 87 insertions(+), 78 deletions(-)

diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
index 52f78bd..3528288 100644
--- a/src/conf/secret_conf.c
+++ b/src/conf/secret_conf.c
@@ -706,6 +706,75 @@ virSecretObjDeleteData(virSecretObjPtr secret)
 }
 
 
+/* Permament secret storage */
+
+/* Secrets are stored in virSecretDriverStatePtr->configDir.  Each secret
+   has virSecretDef stored as XML in "$basename.xml".  If a value of the
+   secret is defined, it is stored as base64 (with no formatting) in
+   "$basename.base64".  "$basename" is in both cases the base64-encoded UUID. 
*/
+
+static int
+virSecretRewriteFile(int fd,
+ void *opaque)
+{
+char *data = opaque;
+
+if (safewrite(fd, data, strlen(data)) < 0)
+return -1;
+
+return 0;
+}
+
+
+int
+virSecretObjSaveConfig(virSecretObjPtr secret)
+{
+char *xml = NULL;
+int ret = -1;
+
+if (!(xml = virSecretDefFormat(secret->def)))
+goto cleanup;
+
+if (virFileRewrite(secret->configFile, S_IRUSR | S_IWUSR,
+   virSecretRewriteFile, xml) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(xml);
+return ret;
+}
+
+
+int
+virSecretObjSaveData(virSecretObjPtr secret)
+{
+char *base64 = NULL;
+int ret = -1;
+
+if (!secret->value)
+return 0;
+
+base64_encode_alloc((const char *)secret->value, secret->value_size,
+);
+if (base64 == NULL) {
+virReportOOMError();
+goto cleanup;
+}
+
+if (virFileRewrite(secret->base64File, S_IRUSR | S_IWUSR,
+   virSecretRewriteFile, base64) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(base64);
+return ret;
+}
+
+
 void
 virSecretDefFree(virSecretDefPtr def)
 {
diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h
index e2f69b5..d40b510 100644
--- a/src/conf/secret_conf.h
+++ b/src/conf/secret_conf.h
@@ -118,6 +118,10 @@ int virSecretObjDeleteConfig(virSecretObjPtr secret);
 
 void virSecretObjDeleteData(virSecretObjPtr secret);
 
+int virSecretObjSaveConfig(virSecretObjPtr secret);
+
+int virSecretObjSaveData(virSecretObjPtr secret);
+
 void virSecretDefFree(virSecretDefPtr def);
 virSecretDefPtr virSecretDefParseString(const char *xml);
 virSecretDefPtr virSecretDefParseFile(const char *filename);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2437b0b..9e1a09e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -798,6 +798,8 @@ virSecretObjListGetUUIDs;
 virSecretObjListNew;
 virSecretObjListNumOfSecrets;
 virSecretObjListRemove;
+virSecretObjSaveConfig;
+virSecretObjSaveData;
 virSecretUsageIDForDef;
 virSecretUsageTypeFromString;
 virSecretUsageTypeToString;
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index e4315f3..1b4dfea 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -91,26 +91,6 @@ secretObjFromSecret(virSecretPtr secret)
 }
 
 
-/* Permament secret storage */
-
-/* Secrets are stored in virSecretDriverStatePtr->configDir.  Each secret
-   has virSecretDef stored as XML in "$basename.xml".  If a value of the
-   secret is defined, it is stored as base64 (with no formatting) in
-   "$basename.base64".  "$basename" is in both cases the base64-encoded UUID. 
*/
-
-static int
-secretRewriteFile(int fd,
-  void *opaque)
-{
-char *data = opaque;
-
-if (safewrite(fd, data, strlen(data)) < 0)
-return -1;
-
-return 0;
-}
-
-
 static int
 secretEnsureDirectory(void)
 {
@@ -122,59 +102,6 @@ secretEnsureDirectory(void)
 return 0;
 }
 
-static int
-secretSaveDef(const virSecretObj *secret)
-{
-char *xml = NULL;
-int ret = -1;
-
-if (secretEnsureDirectory() < 0)
-goto cleanup;
-
-if (!(xml = virSecretDefFormat(secret->def)))
-goto cleanup;
-
-if (virFileRewrite(secret->configFile, S_IRUSR | S_IWUSR,
-   secretRewriteFile, xml) < 0)
-goto cleanup;
-
-ret = 0;
-
- cleanup:
-VIR_FREE(xml);
-return ret;
-}
-
-static int
-secretSaveValue(const virSecretObj *secret)
-{
-char *base64 = NULL;
-int ret = -1;
-
-if (secret->value == NULL)
-return 0;
-
-if (secretEnsureDirectory() < 0)
-goto cleanup;
-
-base64_encode_alloc((const char *)secret->value, secret->value_size,
-);
-if (base64 == NULL) {
-

[libvirt] [PATCH 11/10] secret: Introduce virSecretObjDeleteConfig and virSecretObjDeleteData

[John Ferlan]

Move and rename secretDeleteSaved from secret_driver into secret_conf and
split it up into two parts since there is error path code that looks to
just delete the secret data file

Signed-off-by: John Ferlan 
---
 src/conf/secret_conf.c | 21 +
 src/conf/secret_conf.h |  4 
 src/libvirt_private.syms   |  2 ++
 src/secret/secret_driver.c | 22 ++
 4 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
index f6eee6f..52f78bd 100644
--- a/src/conf/secret_conf.c
+++ b/src/conf/secret_conf.c
@@ -685,6 +685,27 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
 }
 
 
+int
+virSecretObjDeleteConfig(virSecretObjPtr secret)
+{
+/* When the XML is missing, we'll still allow the attempt to
+ * delete the secret data. Without a configFile, the secret
+   won't be loaded again, so we have succeeded already. */
+if (!secret->def->ephemeral &&
+unlink(secret->configFile) < 0 && errno != ENOENT)
+return -1;
+
+return 0;
+}
+
+
+void
+virSecretObjDeleteData(virSecretObjPtr secret)
+{
+(void)unlink(secret->base64File);
+}
+
+
 void
 virSecretDefFree(virSecretDefPtr def)
 {
diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h
index d3bd10c..e2f69b5 100644
--- a/src/conf/secret_conf.h
+++ b/src/conf/secret_conf.h
@@ -114,6 +114,10 @@ int virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
  virSecretObjListACLFilter filter,
  virConnectPtr conn);
 
+int virSecretObjDeleteConfig(virSecretObjPtr secret);
+
+void virSecretObjDeleteData(virSecretObjPtr secret);
+
 void virSecretDefFree(virSecretDefPtr def);
 virSecretDefPtr virSecretDefParseString(const char *xml);
 virSecretDefPtr virSecretDefParseFile(const char *filename);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cbc36de..2437b0b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -787,6 +787,8 @@ virSecretDefFree;
 virSecretDefParseFile;
 virSecretDefParseString;
 virSecretLoadAllConfigs;
+virSecretObjDeleteConfig;
+virSecretObjDeleteData;
 virSecretObjEndAPI;
 virSecretObjListAdd;
 virSecretObjListExport;
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index b8d9ecc..e4315f3 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -175,19 +175,6 @@ secretSaveValue(const virSecretObj *secret)
 return ret;
 }
 
-static int
-secretDeleteSaved(const virSecretObj *secret)
-{
-if (unlink(secret->configFile) < 0 && errno != ENOENT)
-return -1;
-
-/* When the XML is missing, the rest may waste disk space, but the secret
-   won't be loaded again, so we have succeeded already. */
-(void)unlink(secret->base64File);
-
-return 0;
-}
-
 /* Driver functions */
 
 static int
@@ -325,8 +312,10 @@ secretDefineXML(virConnectPtr conn,
 goto restore_backup;
 }
 } else if (backup && !backup->ephemeral) {
-if (secretDeleteSaved(secret) < 0)
+if (virSecretObjDeleteConfig(secret) < 0)
 goto restore_backup;
+
+virSecretObjDeleteData(secret);
 }
 /* Saved successfully - drop old values */
 new_attrs = NULL;
@@ -489,10 +478,11 @@ secretUndefine(virSecretPtr obj)
 if (virSecretUndefineEnsureACL(obj->conn, secret->def) < 0)
 goto cleanup;
 
-if (!secret->def->ephemeral &&
-secretDeleteSaved(secret) < 0)
+if (virSecretObjDeleteConfig(secret) < 0)
 goto cleanup;
 
+virSecretObjDeleteData(secret);
+
 virSecretObjListRemove(driver->secrets, secret);
 
 ret = 0;
-- 
2.5.0

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

Re: [libvirt] sVirt shouldn't let Nova do stupid things

[Daniel P. Berrange]

On Tue, Mar 08, 2016 at 04:24:51PM +, Matthew Booth wrote:
> Nova just released a fix for this critical CVE:
> https://bugs.launchpad.net/nova/+bug/1548450
> 
> To summarise, it's a qcow2 backing file exploit. The user writes a
> malicious qcow2 header to the top of a raw disk, then triggers a bug in
> Nova which causes it to do format detection.
> 
> If you read the bug and comments, you'll see that when I initially reported
> it I was fairly dismissive of its impact because it's only exploitable
> through libvirt, and the instance is going to be confined by SELinux. But
> then Dan B points out that sVirt is going to trust whatever Nova tells it
> to do and label it appropriately. Cue rapid ramping of severity, and it
> turns out this allows an unprivileged user to read anything on the host,
> including all raw block devices.
> 
> I'm not sure exactly where, but something in this stack has failed us.
> Let's be clear a couple of things, though:
> 
> 1. This is an egregious, stupid bug in Nova, and Nova shouldn't have
> egregious, stupid bugs.
> 2. SELinux should prevent obviously bad things from happening, even in the
> presence of egregious, stupid bugs.
> 
> I point that out to head off: 'Well Nova shouldn't do that'. Of course it
> shouldn't. However, it might, and when it does, I'd like to think that
> SELinux has its back. It doesn't, though.
> 
> As I understand it, sVirt is the mechanism libvirt uses for controlling
> SELinux. I wonder if the current sVirt model is enough to cover the use
> case where the thing connecting to libvirt is large enough to have its own
> serious bugs. Is there any way we could define a sane set of operations
> independent of Nova?

You need to distinguish SELinux and sVirt in this discussion.

What I mean by this is that SELinux refers to the general MAC framework
that is limited only by the smarts of the person designing the policy.
It is entirely possible to write an SELinux policy that prevents libvirtd
from doing whatever it likes. Whether it is *practical* to write such a
policy that works in a general purpose linux host is a different issue,
I'll come back to shortly...

sVirt refers to the use of MAC to mitigate against bugs in guest VMs that
would otherwise allow a compromised VM to attack other VMs or the host. It
is an explicit *non-goal* of sVirt to protect the host from applications
connected to libvirt. Or to put it another way, as far as sVirt is concerned,
it is an explicitly assumed that the application configuring libvirt guests
knows what it is doing. sVirt will honour whatever it is told to allow the
guest todo. This design decision was fundamental to making sVirt something
that was practical to actually run with zero-configuration on any Linux
host with virtualization. Libvirt does not want to define or impose policy
on usage of its APIs, by default, it wants to provide the mechanism and
leave policy up to the virt mgmt application to decide what's acceptable
or not.

So for both reasons of practicality (to hard to enumerate what is valid
todo via libvirt without blocking valid use cases) and design (we explicitly
don't want to be second guessing what apps might consider valid), the
libvirtd daemon SELinux policy is essentially equivalent to unconfined_t.

Thus applications connecting to privileged libvirtd are considered to have
privileges equivalent to root, and it is their responsibility to tell
libvirt todo sensible things.

I don't see us changing this as a default out of the box approach, because
defining valid usage policy is a fundamentally hard problem and would likely
just result in giving you a false sense of security.

For example, consider you wrote some policy rule that prevented libvirtd
from relabelling /dev/sda to allow access by a guest because /dev/sda was
your host root filesystem device. You might think you are now secure ?
Wrong. Nova still has ability to relabel /lib/libc.so and expose this
shared library as /dev/vda in the guest. The malicious guest now writes
to this disk inserting a backdoor in the 'open' function. A completely
different process running on the host will happily now execute that
backdoor as whatever privileges they have (probably unconfined_t.

Ok, so now we blacklist /lib/*. Better do the same for /bin, and /etc
too, oh but we explicitly want access to some files in /etc. We better
black list /var too, but by valid VM config XYZ requires access to
/var/foo/bar. And so on.

This is the problem with confining privileged mgmt applications on a
general purpose Linux install. Now if there was a *non-general purpose*
compute host image whose sole job was to run nova-compute, that would be
a different matter. There are a (hopefulyl) finite number of things that
a nova-compute instance should be permitted todo. So you could write a
SELinux policy for libvirtd, that was targetted solely to Nova that could
give you some better protection.

Such a policy isn't really something for libvirt to care about though - it

[libvirt] [PATCH] domain: Remove controller/net address whitelists

[Cole Robinson]

Judging by how the whitelist has skewed quite far from the original
error message, I think it's better to just drop these.

If someone wants to revive this check I suggest implementing it on
a per-HV driver basis with PostParse callbacks.
---
 src/conf/domain_conf.c | 24 
 1 file changed, 24 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d376a2c..ec14577 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7969,17 +7969,6 @@ virDomainControllerDefParseXML(xmlNodePtr node,
 break;
 }
 
-if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
-def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO &&
-def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
-def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 &&
-def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO &&
-def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Controllers must use the 'pci' address type"));
-goto error;
-}
-
  cleanup:
 ctxt->node = saved;
 VIR_FREE(typeStr);
@@ -8670,19 +8659,6 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
 goto error;
 }
 
-/* XXX what about ISA/USB based NIC models - once we support
- * them we should make sure address type is correct */
-if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
-def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO &&
-def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
-def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 &&
-def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO &&
-def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Network interfaces must use 'pci' address type"));
-goto error;
-}
-
 switch (def->type) {
 case VIR_DOMAIN_NET_TYPE_NETWORK:
 if (network == NULL) {
-- 
2.5.0

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

[libvirt] [PATCH 1/8] domain: Add virDomainDefAddImplicitDevices

[Cole Robinson]

It's just a combination of AddImplicitControllers, and AddConsoleCompat.
Every caller that wants ImplicitControllers also wants the ConsoleCompat
AFAICT, so lump them together. We also need it for future patches.
---
 src/conf/domain_conf.c   | 19 ++-
 src/conf/domain_conf.h   |  2 +-
 src/libvirt_private.syms |  2 +-
 src/qemu/qemu_driver.c   |  6 +++---
 src/vmx/vmx.c|  2 +-
 src/vz/vz_sdk.c  |  2 +-
 6 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 39cedbd..40b1929 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3842,9 +3842,6 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
 if (virDomainDefPostParseMemory(def, parseFlags) < 0)
 return -1;
 
-if (virDomainDefAddConsoleCompat(def) < 0)
-return -1;
-
 if (virDomainDefRejectDuplicateControllers(def) < 0)
 return -1;
 
@@ -3854,7 +3851,7 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
 if (virDomainDefPostParseTimer(def) < 0)
 return -1;
 
-if (virDomainDefAddImplicitControllers(def) < 0)
+if (virDomainDefAddImplicitDevices(def) < 0)
 return -1;
 
 /* clean up possibly duplicated metadata entries */
@@ -18289,7 +18286,7 @@ virDomainDefMaybeAddSmartcardController(virDomainDefPtr 
def)
  * in the XML. This is for compat with existing apps which will
  * not know/care about  info in the XML
  */
-int
+static int
 virDomainDefAddImplicitControllers(virDomainDefPtr def)
 {
 if (virDomainDefAddDiskControllersForType(def,
@@ -18324,6 +18321,18 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def)
 return 0;
 }
 
+int
+virDomainDefAddImplicitDevices(virDomainDefPtr def)
+{
+if (virDomainDefAddConsoleCompat(def) < 0)
+return -1;
+
+if (virDomainDefAddImplicitControllers(def) < 0)
+return -1;
+
+return 0;
+}
+
 virDomainIOThreadIDDefPtr
 virDomainIOThreadIDFind(virDomainDefPtr def,
 unsigned int iothread_id)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 06305f0..6f044d2 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2697,7 +2697,7 @@ virDomainObjPtr virDomainObjParseFile(const char 
*filename,
 bool virDomainDefCheckABIStability(virDomainDefPtr src,
virDomainDefPtr dst);
 
-int virDomainDefAddImplicitControllers(virDomainDefPtr def);
+int virDomainDefAddImplicitDevices(virDomainDefPtr def);
 
 virDomainIOThreadIDDefPtr virDomainIOThreadIDFind(virDomainDefPtr def,
   unsigned int iothread_id);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3a1b9e1..5399117 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -200,7 +200,7 @@ virDomainControllerRemove;
 virDomainControllerTypeToString;
 virDomainCpuPlacementModeTypeFromString;
 virDomainCpuPlacementModeTypeToString;
-virDomainDefAddImplicitControllers;
+virDomainDefAddImplicitDevices;
 virDomainDefAddUSBController;
 virDomainDefCheckABIStability;
 virDomainDefCheckDuplicateDiskInfo;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 102fade..9c60518 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7924,7 +7924,7 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
 /* vmdef has the pointer. Generic codes for vmdef will do all jobs */
 dev->data.disk = NULL;
 if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
-if (virDomainDefAddImplicitControllers(vmdef) < 0)
+if (virDomainDefAddImplicitDevices(vmdef) < 0)
 return -1;
 if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
 return -1;
@@ -7949,7 +7949,7 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
 if (virDomainHostdevInsert(vmdef, hostdev))
 return -1;
 dev->data.hostdev = NULL;
-if (virDomainDefAddImplicitControllers(vmdef) < 0)
+if (virDomainDefAddImplicitDevices(vmdef) < 0)
 return -1;
 if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
 return -1;
@@ -7991,7 +7991,7 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
 if (qemuDomainChrInsert(vmdef, dev->data.chr) < 0)
 return -1;
 dev->data.chr = NULL;
-if (virDomainDefAddImplicitControllers(vmdef) < 0)
+if (virDomainDefAddImplicitDevices(vmdef) < 0)
 return -1;
 if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
 return -1;
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 4fd0a1d..893c77a 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -1691,7 +1691,7 @@ virVMXParseConfig(virVMXContext *ctx,
 }
 
 /* def:controllers */
-if (virDomainDefAddImplicitControllers(def) < 0) {
+if (virDomainDefAddImplicitDevices(def) < 0) {
 

Re: [libvirt] [PATCH V2 4/4] libxl: support enabling and disabling feature

[Joao Martins]

On 03/01/2016 04:00 AM, Jim Fehlig wrote:
> Until now, the libxl driver ignored any  setting in domain XML
> and deferred to libxl, which enables hap if not specified. While
> this is a good default, it prevents disabling hap if desired.
> 
> This change allows disabling hap with . hap is
> explicitly enabled with  or 

Re: [libvirt] [PATCH V2 1/4] conf: add 'state' attribute to feature

[Joao Martins]

On 03/01/2016 04:00 AM, Jim Fehlig wrote:
> Most hypervisors use Hardware Assisted Paging by default and don't
> require specifying the feature in domain conf. But some hypervisors
> support disabling HAP on a per-domain basis. To enable HAP by default
> yet provide a knob to disable it, extend the  feature with a
> 'state=on|off' attribute, similar to  and  features.
> 
> In the absence of , the hypervisor default (on) is used. 
> without the state attribute would be the same as  for
> backwards compatibility. And of course  disables hap.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  docs/formatdomain.html.in | 6 --
>  docs/schemas/domaincommon.rng | 6 +-
>  src/conf/domain_conf.c| 4 ++--
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 5016772..c06bcf3 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1494,8 +1494,10 @@
>Interrupt) for the guest.
>
>hap
> -  Enable use of Hardware Assisted Paging if available in
> -the hardware.
> +  Depending on the state attribute (values 
> on,
> +off) enable or disable use of Hardware Assisted Paging.
> +The default is on if the hypervisor detects availability
> +of Hardware Assisted Paging.
>
>viridian
>Enable Viridian hypervisor extensions for paravirtualizing
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 67af93a..dd6e93a 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4108,7 +4108,11 @@
>
>
>  
> -  
> +  
> +
> +  
> +
> +  
Perhaps  would be better (see chunk below) ? That one
appears to be a reference of what you are adding above, and it's the same as
pvspinlock. Though some other elements don't appear to use this, not sure why.

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 89d3a6b..141122c 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4132,9 +4132,7 @@
   
 
   
-
-  
-
+
   
 
   

Other that,

Reviewed-by: Joao Martins 

>  
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 79758d4..714bbfc 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15296,7 +15296,6 @@ virDomainDefParseXML(xmlDocPtr xml,
>  /* fallthrough */
>  case VIR_DOMAIN_FEATURE_ACPI:
>  case VIR_DOMAIN_FEATURE_PAE:
> -case VIR_DOMAIN_FEATURE_HAP:
>  case VIR_DOMAIN_FEATURE_VIRIDIAN:
>  case VIR_DOMAIN_FEATURE_PRIVNET:
>  case VIR_DOMAIN_FEATURE_HYPERV:
> @@ -15321,6 +15320,7 @@ virDomainDefParseXML(xmlDocPtr xml,
>  ctxt->node = node;
>  break;
>  
> +case VIR_DOMAIN_FEATURE_HAP:
>  case VIR_DOMAIN_FEATURE_PMU:
>  case VIR_DOMAIN_FEATURE_PVSPINLOCK:
>  case VIR_DOMAIN_FEATURE_VMPORT:
> @@ -22043,7 +22043,6 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>  switch ((virDomainFeature) i) {
>  case VIR_DOMAIN_FEATURE_ACPI:
>  case VIR_DOMAIN_FEATURE_PAE:
> -case VIR_DOMAIN_FEATURE_HAP:
>  case VIR_DOMAIN_FEATURE_VIRIDIAN:
>  case VIR_DOMAIN_FEATURE_PRIVNET:
>  switch ((virTristateSwitch) def->features[i]) {
> @@ -22065,6 +22064,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>  
>  break;
>  
> +case VIR_DOMAIN_FEATURE_HAP:
>  case VIR_DOMAIN_FEATURE_PMU:
>  case VIR_DOMAIN_FEATURE_PVSPINLOCK:
>  case VIR_DOMAIN_FEATURE_VMPORT:
> 

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

Re: [libvirt] [PATCH V2 2/4] xenconfig: change 'hap' setting to align with Xen behavior

[Joao Martins]

On 03/01/2016 04:00 AM, Jim Fehlig wrote:
> hap is enabled by default in xm and xl config and usually only
> specified when it is desirable to disable hap (hap = 0). Change
> the xm,xl <-> xml converter to behave similarly. I.e. only
> produce 'hap = 0' when  and vice versa.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  src/xenconfig/xen_common.c | 14 ++---
>  .../test-disk-positional-parms-full.cfg|  1 -
>  .../test-disk-positional-parms-partial.cfg |  1 -
>  ...est-fullvirt-direct-kernel-boot-bogus-extra.cfg |  1 -
>  .../test-fullvirt-direct-kernel-boot-extra.cfg |  1 -
>  .../test-fullvirt-direct-kernel-boot.cfg   |  1 -
>  tests/xlconfigdata/test-fullvirt-multiusb.cfg  |  1 -
>  tests/xlconfigdata/test-fullvirt-nohap.cfg | 26 ++
>  tests/xlconfigdata/test-fullvirt-nohap.xml | 59 
> ++
>  tests/xlconfigdata/test-new-disk.cfg   |  1 -
>  tests/xlconfigdata/test-rbd-multihost-noauth.cfg   |  1 -
>  tests/xlconfigdata/test-spice-features.cfg |  1 -
>  tests/xlconfigdata/test-spice.cfg  |  1 -
>  tests/xlconfigdata/test-vif-rate.cfg   |  1 -
>  tests/xlconfigtest.c   |  1 +
>  tests/xmconfigdata/test-escape-paths.cfg   |  1 -
>  .../xmconfigdata/test-fullvirt-default-feature.cfg |  1 -
>  tests/xmconfigdata/test-fullvirt-force-hpet.cfg|  1 -
>  tests/xmconfigdata/test-fullvirt-force-nohpet.cfg  |  1 -
>  tests/xmconfigdata/test-fullvirt-localtime.cfg |  1 -
>  tests/xmconfigdata/test-fullvirt-net-netfront.cfg  |  1 -
>  tests/xmconfigdata/test-fullvirt-new-cdrom.cfg |  1 -
>  tests/xmconfigdata/test-fullvirt-nohap.cfg | 28 ++
>  tests/xmconfigdata/test-fullvirt-nohap.xml | 51 +++
>  tests/xmconfigdata/test-fullvirt-parallel-tcp.cfg  |  1 -
>  .../test-fullvirt-serial-dev-2-ports.cfg   |  1 -
>  .../test-fullvirt-serial-dev-2nd-port.cfg  |  1 -
>  tests/xmconfigdata/test-fullvirt-serial-file.cfg   |  1 -
>  tests/xmconfigdata/test-fullvirt-serial-null.cfg   |  1 -
>  tests/xmconfigdata/test-fullvirt-serial-pipe.cfg   |  1 -
>  tests/xmconfigdata/test-fullvirt-serial-pty.cfg|  1 -
>  tests/xmconfigdata/test-fullvirt-serial-stdio.cfg  |  1 -
>  .../test-fullvirt-serial-tcp-telnet.cfg|  1 -
>  tests/xmconfigdata/test-fullvirt-serial-tcp.cfg|  1 -
>  tests/xmconfigdata/test-fullvirt-serial-udp.cfg|  1 -
>  tests/xmconfigdata/test-fullvirt-serial-unix.cfg   |  1 -
>  tests/xmconfigdata/test-fullvirt-sound.cfg |  1 -
>  tests/xmconfigdata/test-fullvirt-usbmouse.cfg  |  1 -
>  tests/xmconfigdata/test-fullvirt-usbtablet.cfg |  1 -
>  tests/xmconfigdata/test-fullvirt-utc.cfg   |  1 -
>  tests/xmconfigdata/test-no-source-cdrom.cfg|  1 -
>  tests/xmconfigdata/test-pci-devs.cfg   |  1 -
>  tests/xmconfigtest.c   |  1 +
>  43 files changed, 173 insertions(+), 43 deletions(-)
> 
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 828c8e9..4dcd484 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -528,11 +528,11 @@ xenParseCPUFeatures(virConfPtr conf, virDomainDefPtr 
> def)
>  
>  else if (val)
>  def->features[VIR_DOMAIN_FEATURE_APIC] = VIR_TRISTATE_SWITCH_ON;
> -if (xenConfigGetBool(conf, "hap", , 0) < 0)
> +if (xenConfigGetBool(conf, "hap", , 1) < 0)
>  return -1;
>  
> -else if (val)
> -def->features[VIR_DOMAIN_FEATURE_HAP] = VIR_TRISTATE_SWITCH_ON;
> +else if (!val)
> +def->features[VIR_DOMAIN_FEATURE_HAP] = VIR_TRISTATE_SWITCH_OFF;
>  if (xenConfigGetBool(conf, "viridian", , 0) < 0)
>  return -1;
>  
> @@ -1572,10 +1572,10 @@ xenFormatCPUFeatures(virConfPtr conf, virDomainDefPtr 
> def)
>  VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0)
>  return -1;
>  
> -if (xenConfigSetInt(conf, "hap",
> -(def->features[VIR_DOMAIN_FEATURE_HAP] ==
> - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0)
> -return -1;
> +if (def->features[VIR_DOMAIN_FEATURE_HAP] == 
> VIR_TRISTATE_SWITCH_OFF) {
> +if (xenConfigSetInt(conf, "hap", 0) < 0)
> +return -1;
> +}
>  
>  if (xenConfigSetInt(conf, "viridian",
>  (def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] ==
> diff --git a/tests/xlconfigdata/test-disk-positional-parms-full.cfg 
> b/tests/xlconfigdata/test-disk-positional-parms-full.cfg
> index 026e451..c5bbb03 100644
> --- a/tests/xlconfigdata/test-disk-positional-parms-full.cfg
> +++ b/tests/xlconfigdata/test-disk-positional-parms-full.cfg
> @@ -6,7 +6,6 @@ vcpus = 1
>  pae = 1
>  acpi = 1
>  apic = 1
> -hap = 0
>  viridian = 0
>  rtc_timeoffset 

[libvirt] [PATCH 2/8] domain: conf: Export virDomainDefPostParseDevices

[Cole Robinson]

We will use this in upcoming patches
---
 src/conf/domain_conf.c   | 33 +++--
 src/conf/domain_conf.h   |  5 +
 src/libvirt_private.syms |  1 +
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 40b1929..bc4e369 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4191,12 +4191,11 @@ virDomainDefPostParseDeviceIterator(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
data->parseFlags, data->xmlopt);
 }
 
-
 int
-virDomainDefPostParse(virDomainDefPtr def,
-  virCapsPtr caps,
-  unsigned int parseFlags,
-  virDomainXMLOptionPtr xmlopt)
+virDomainDefPostParseDevices(virDomainDefPtr def,
+ virCapsPtr caps,
+ unsigned int parseFlags,
+ virDomainXMLOptionPtr xmlopt)
 {
 int ret;
 struct virDomainDefPostParseDeviceIteratorData data = {
@@ -4206,6 +4205,23 @@ virDomainDefPostParse(virDomainDefPtr def,
 .parseFlags = parseFlags,
 };
 
+if ((ret = virDomainDeviceInfoIterateInternal(def,
+  
virDomainDefPostParseDeviceIterator,
+  true,
+  )) < 0)
+return ret;
+
+return 0;
+}
+
+int
+virDomainDefPostParse(virDomainDefPtr def,
+  virCapsPtr caps,
+  unsigned int parseFlags,
+  virDomainXMLOptionPtr xmlopt)
+{
+int ret;
+
 /* call the domain config callback */
 if (xmlopt->config.domainPostParseCallback) {
 ret = xmlopt->config.domainPostParseCallback(def, caps, parseFlags,
@@ -4215,13 +4231,10 @@ virDomainDefPostParse(virDomainDefPtr def,
 }
 
 /* iterate the devices */
-if ((ret = virDomainDeviceInfoIterateInternal(def,
-  
virDomainDefPostParseDeviceIterator,
-  true,
-  )) < 0)
+if ((ret = virDomainDefPostParseDevices(def, caps,
+parseFlags, xmlopt)) < 0)
 return ret;
 
-
 if ((ret = virDomainDefPostParseInternal(def, caps, parseFlags)) < 0)
 return ret;
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 6f044d2..aba53a2 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2476,6 +2476,11 @@ virDomainDefPostParse(virDomainDefPtr def,
   virCapsPtr caps,
   unsigned int parseFlags,
   virDomainXMLOptionPtr xmlopt);
+int
+virDomainDefPostParseDevices(virDomainDefPtr def,
+ virCapsPtr caps,
+ unsigned int parseFlags,
+ virDomainXMLOptionPtr xmlopt);
 
 static inline bool
 virDomainObjIsActive(virDomainObjPtr dom)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5399117..51d2721 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -235,6 +235,7 @@ virDomainDefParseFile;
 virDomainDefParseNode;
 virDomainDefParseString;
 virDomainDefPostParse;
+virDomainDefPostParseDevices;
 virDomainDefSetMemoryInitial;
 virDomainDefSetMemoryTotal;
 virDomainDefSetVcpus;
-- 
2.5.0

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

[libvirt] [PATCH 0/8] domain: Support allocation

[Cole Robinson]

This patch series allows the user to specify bare device
 to explicitly request PCI address allocation.

This has several uses, but the motivating one is providing an
easy way to request PCI address allocation where it normally isn't
the default address type, like for aarch64 VMs.


Cole Robinson (8):
  domain: Add virDomainDefAddImplicitDevices
  domain: conf: Export virDomainDefPostParseDevices
  qemu: Assign device addresses in PostParse
  util: xml: add virXMLPropertyCount
  tests: Add failure flags to CompareDomainXML2XML
  domain: Make  request address allocation
  qemu: Wire up address type=pci auto_allocate
  tests: qemu: test  with aarch64

 docs/schemas/domaincommon.rng  |  5 +-
 src/conf/domain_conf.c | 81 +-
 src/conf/domain_conf.h |  8 ++-
 src/libvirt_private.syms   |  3 +-
 src/qemu/qemu_domain.c | 13 +++-
 src/qemu/qemu_domain_address.c | 47 +
 src/qemu/qemu_driver.c |  6 +-
 src/util/virxml.c  | 17 +
 src/util/virxml.h  |  1 +
 src/vmx/vmx.c  |  2 +-
 src/vz/vz_sdk.c|  2 +-
 tests/bhyvexml2xmltest.c   |  2 +-
 .../generic-pci-autofill-addr.xml  | 27 
 tests/genericxml2xmltest.c | 17 +++--
 tests/lxcxml2xmltest.c |  2 +-
 .../qemuargv2xmldata/qemuargv2xml-pseries-disk.xml |  4 +-
 ...l2argv-aarch64-virtio-pci-manual-addresses.args |  4 +-
 ...ml2argv-aarch64-virtio-pci-manual-addresses.xml |  5 ++
 .../qemuxml2argv-pci-autofill-addr.args| 24 +++
 .../qemuxml2argv-pci-autofill-addr.xml | 44 
 tests/qemuxml2argvtest.c   | 21 +++---
 ...2xmlout-aarch64-virtio-pci-manual-addresses.xml |  5 ++
 .../qemuxml2xmlout-pci-autofill-addr.xml   | 46 
 tests/qemuxml2xmltest.c| 18 +++--
 tests/testutils.c  | 10 ++-
 tests/testutils.h  |  4 ++
 26 files changed, 359 insertions(+), 59 deletions(-)
 create mode 100644 tests/genericxml2xmlindata/generic-pci-autofill-addr.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autofill-addr.xml

-- 
2.5.0

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

[libvirt] [PATCH 3/8] qemu: Assign device addresses in PostParse

[Cole Robinson]

In order to make this work, we need to short circuit the normal
virDomainDefPostParse ordering, and manually add stock devices
ourselves, since we need them in the XML before assigning addresses.

The motivation for this is that upcoming patches will want to perform
generic PostParse checks that need to run _after_ the driver has
assigned all its addresses.

Peter objected to this here:
https://www.redhat.com/archives/libvir-list/2016-January/msg00200.html

Suggesting adding an extra PostParse callback instead. I argued
that change isn't necessarily sufficient either, and that this
method should be safe since all these functions already need to be
idemptotent.

The lone test suite change is due to DomainNativeToXML now calling
qemuDomainAssignAddresses... apparently that's the only test which
hits qemu specific address logic.

There's still quite a few manual callers of qemuDomainAssignAddresses
that could be dropped too but it would need additional testing, and
they shouldn't disrupt anything in the interim since extra calls
will be no-ops.
---
 src/qemu/qemu_domain.c   | 13 -
 tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml |  4 +++-
 tests/qemuxml2argvtest.c | 20 +++-
 tests/qemuxml2xmltest.c  | 12 
 4 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9044792..d697e99 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1369,7 +1369,7 @@ qemuCanonicalizeMachine(virDomainDefPtr def, 
virQEMUCapsPtr qemuCaps)
 static int
 qemuDomainDefPostParse(virDomainDefPtr def,
virCapsPtr caps,
-   unsigned int parseFlags ATTRIBUTE_UNUSED,
+   unsigned int parseFlags,
void *opaque)
 {
 virQEMUDriverPtr driver = opaque;
@@ -1408,6 +1408,17 @@ qemuDomainDefPostParse(virDomainDefPtr def,
 if (virSecurityManagerVerify(driver->securityManager, def) < 0)
 goto cleanup;
 
+/* Device defaults are normally set after calling the driver specific
+   PostParse routine (this function), but we need them earlier. */
+if (virDomainDefPostParseDevices(def, caps,
+ parseFlags, driver->xmlopt) < 0)
+goto cleanup;
+if (virDomainDefAddImplicitDevices(def) < 0)
+goto cleanup;
+
+if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0)
+goto cleanup;
+
 ret = 0;
  cleanup:
 virObjectUnref(qemuCaps);
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml 
b/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml
index 97225f4..c0d7e94 100644
--- a/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml
+++ b/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml
@@ -29,7 +29,9 @@
 
 
 
-
+
+  
+
 
 
 
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index d29073d..aaec9de 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -277,6 +277,11 @@ static int testCompareXMLToArgvFiles(const char *xml,
 if (virBitmapParse("0-3", '\0', , 4) < 0)
 goto out;
 
+virQEMUCapsSetList(extraFlags,
+   QEMU_CAPS_NO_ACPI,
+   QEMU_CAPS_DEVICE,
+   QEMU_CAPS_LAST);
+
 if (!(vmdef = virDomainDefParseFile(xml, driver.caps, driver.xmlopt,
 (VIR_DOMAIN_DEF_PARSE_INACTIVE |
  parseFlags {
@@ -302,11 +307,6 @@ static int testCompareXMLToArgvFiles(const char *xml,
 if (qemuProcessPrepareMonitorChr(_chr, domainLibDir) < 0)
 goto out;
 
-virQEMUCapsSetList(extraFlags,
-   QEMU_CAPS_NO_ACPI,
-   QEMU_CAPS_DEVICE,
-   QEMU_CAPS_LAST);
-
 if (STREQ(vmdef->os.machine, "pc") &&
 STREQ(vmdef->emulator, "/usr/bin/qemu-system-x86_64")) {
 VIR_FREE(vmdef->os.machine);
@@ -316,12 +316,6 @@ static int testCompareXMLToArgvFiles(const char *xml,
 
 virQEMUCapsFilterByMachineType(extraFlags, vmdef->os.machine);
 
-if (qemuDomainAssignAddresses(vmdef, extraFlags, NULL)) {
-if (flags & FLAG_EXPECT_ERROR)
-goto ok;
-goto out;
-}
-
 log = virtTestLogContentAndReset();
 VIR_FREE(log);
 virResetLastError();
@@ -1420,7 +1414,7 @@ mymain(void)
 QEMU_CAPS_PCI_OHCI, QEMU_CAPS_PCI_MULTIFUNCTION);
 DO_TEST("pseries-vio-user-assigned",
 QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG);
-DO_TEST_ERROR("pseries-vio-address-clash",
+DO_TEST_PARSE_ERROR("pseries-vio-address-clash",
 QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG);
 DO_TEST("pseries-nvram", QEMU_CAPS_DEVICE_NVRAM);
 DO_TEST("pseries-usb-kbd", QEMU_CAPS_PCI_OHCI,
@@ -1583,7 +1577,7 @@ mymain(void)
 

[libvirt] [PATCH 6/8] domain: Make request address allocation

[Cole Robinson]

If a bare device  is specified, set an internal
flag address->auto_allocate. Individual hv drivers can then check for
this and act on it if they want, nothing is allocated in generic code.

If drivers allocate an address, they are expected to unset auto_allocate.
Generic domain conf code then checks at XML format time to ensure no
device addresses still have auto_allocate set; this ensures we aren't
formatting any bogus address XML, and informing the user if their
request didn't work. Add a genericxml2xml test case for this.

The auto_allocate property is a part of the generic address structure
and not the PCI specific bits, this will make it easier to reuse with
other address types too.

One note: we detect  by counting it's XML properties,
rather than comparing specifically against parsed values, which seems
easier to maintain.
---
 docs/schemas/domaincommon.rng  |  5 +++-
 src/conf/domain_conf.c | 29 +-
 src/conf/domain_conf.h |  1 +
 .../generic-pci-autofill-addr.xml  | 27 
 tests/genericxml2xmltest.c | 17 +
 5 files changed, 72 insertions(+), 7 deletions(-)
 create mode 100644 tests/genericxml2xmlindata/generic-pci-autofill-addr.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 6ca937c..d083250 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4471,7 +4471,10 @@
   
 pci
   
-  
+  
+
+
+  
 
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bc4e369..bbc42a4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3827,6 +3827,23 @@ virDomainDefPostParseTimer(virDomainDefPtr def)
 }
 
 
+ static int
+virDomainCheckUnallocatedDeviceAddrs(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceDefPtr dev,
+ virDomainDeviceInfoPtr info,
+ void *data ATTRIBUTE_UNUSED)
+{
+if (!info->auto_allocate)
+return 0;
+
+virReportError(VIR_ERR_INTERNAL_ERROR,
+_("driver didn't allocate requested address type '%s' for device 
'%s'"),
+virDomainDeviceAddressTypeToString(info->type),
+virDomainDeviceTypeToString(dev->type));
+return -1;
+}
+
+
 static int
 virDomainDefPostParseInternal(virDomainDefPtr def,
   virCapsPtr caps ATTRIBUTE_UNUSED,
@@ -3851,6 +3868,11 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
 if (virDomainDefPostParseTimer(def) < 0)
 return -1;
 
+/* ensure the driver filled in any auto_allocate addrs */
+if (virDomainDeviceInfoIterate(def, virDomainCheckUnallocatedDeviceAddrs,
+   NULL) < 0)
+return -1;
+
 if (virDomainDefAddImplicitDevices(def) < 0)
 return -1;
 
@@ -4872,8 +4894,13 @@ virDomainDeviceInfoParseXML(xmlNodePtr node,
 
 switch ((virDomainDeviceAddressType) info->type) {
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
-if (virDevicePCIAddressParseXML(address, >addr.pci) < 0)
+if (virXMLPropertyCount(address) == 1) {
+/* Bare  is a request to allocate
+   the address. */
+info->auto_allocate = true;
+} else if (virDevicePCIAddressParseXML(address, >addr.pci) < 0) {
 goto cleanup;
+}
 break;
 
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index aba53a2..dd9d0b1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -346,6 +346,7 @@ struct _virDomainDeviceInfo {
  */
 char *alias;
 int type; /* virDomainDeviceAddressType */
+bool auto_allocate;
 union {
 virDevicePCIAddress pci;
 virDomainDeviceDriveAddress drive;
diff --git a/tests/genericxml2xmlindata/generic-pci-autofill-addr.xml 
b/tests/genericxml2xmlindata/generic-pci-autofill-addr.xml
new file mode 100644
index 000..06eadb6
--- /dev/null
+++ b/tests/genericxml2xmlindata/generic-pci-autofill-addr.xml
@@ -0,0 +1,27 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu
+
+  
+  
+  
+  
+
+
+  
+
+  
+
diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
index 666fc86..b329d10 100644
--- a/tests/genericxml2xmltest.c
+++ b/tests/genericxml2xmltest.c
@@ -21,6 +21,7 @@ struct testInfo {
 const char *name;
 int different;
 bool inactive_only;
+testCompareDomXML2XMLFlags compare_flags;
 };
 
 static int
@@ -39,7 +40,7 @@ testCompareXMLToXMLHelper(const void *data)
 
 ret = testCompareDomXML2XMLFiles(caps, xmlopt, xml_in,
   

[libvirt] [PATCH 7/8] qemu: Wire up address type=pci auto_allocate

[Cole Robinson]

We do this in 2 passes: before PCI addresses are about to be collected,
we convert type=pci auto_allocate=true to type=none auto_allocate=true,
since the existing code is already expecting type=none here.

After all PCI allocation should be complete, we do another pass of the
device addresses converting type=pci auto_allocate=true to
auto_allocate=false, so we don't trigger the unallocated address
validation check in generic domain code.
---
 src/qemu/qemu_domain_address.c | 47 ++
 .../qemuxml2argv-pci-autofill-addr.args| 24 +++
 .../qemuxml2argv-pci-autofill-addr.xml | 44 
 tests/qemuxml2argvtest.c   |  1 +
 .../qemuxml2xmlout-pci-autofill-addr.xml   | 46 +
 tests/qemuxml2xmltest.c|  1 +
 6 files changed, 163 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autofill-addr.xml

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index eff33fc..74d13b6 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1399,6 +1399,45 @@ qemuDomainSupportsPCI(virDomainDefPtr def,
 
 
 static int
+qemuDomainPrepPCIAutoAllocate(virDomainDefPtr def ATTRIBUTE_UNUSED,
+  virDomainDeviceDefPtr device ATTRIBUTE_UNUSED,
+  virDomainDeviceInfoPtr info,
+  void *opaque ATTRIBUTE_UNUSED)
+{
+/* If PCI auto_allocate requested, set type to NONE since the rest
+   of the code expects it. */
+if (info->auto_allocate &&
+info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
+
+return 0;
+}
+
+
+static int
+qemuDomainFinishPCIAutoAllocate(virDomainDefPtr def ATTRIBUTE_UNUSED,
+virDomainDeviceDefPtr device ATTRIBUTE_UNUSED,
+virDomainDeviceInfoPtr info,
+void *opaque ATTRIBUTE_UNUSED)
+{
+/* A PCI device was allocated as requested, unset auto_allocate so
+   we don't trip the domain error about unallocated addresses */
+if (info->auto_allocate &&
+info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+info->auto_allocate = false;
+
+/* We wanted to allocate a PCI address but it was never filled in...
+   this is likely an XML error. Re-set type=PCI to give a correct
+   error from domain conf */
+if (info->auto_allocate &&
+info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
+info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
+
+return 0;
+}
+
+
+static int
 qemuDomainAssignPCIAddresses(virDomainDefPtr def,
  virQEMUCapsPtr qemuCaps,
  virDomainObjPtr obj)
@@ -1423,6 +1462,10 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
 }
 }
 
+if (virDomainDeviceInfoIterate(def, qemuDomainPrepPCIAutoAllocate,
+   NULL) < 0)
+goto cleanup;
+
 nbuses = max_idx + 1;
 
 if (nbuses > 0 &&
@@ -1553,6 +1596,10 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
 }
 }
 }
+
+if (virDomainDeviceInfoIterate(def, qemuDomainFinishPCIAutoAllocate,
+   NULL) < 0)
+goto cleanup;
 }
 
 if (obj && obj->privateData) {
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args 
b/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args
new file mode 100644
index 000..2ab305e
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args
@@ -0,0 +1,24 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/libexec/qemu-kvm \
+-name fdr-br \
+-S \
+-M pc-1.2 \
+-m 2048 \
+-smp 2 \
+-uuid 3ec6cbe1-b5a2-4515-b800-31a61855df41 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-fdr-br/monitor.sock,server,nowait \
+-boot c \
+-usb \
+-drive file=/var/iso/f18kde.iso,format=raw,if=none,media=cdrom,\
+id=drive-virtio-disk0 \
+-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\
+id=virtio-disk0 \
+-vga cirrus \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml
new file mode 100644
index 000..8e7b6ab
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml
@@ -0,0 +1,44 @@
+
+  fdr-br
+  3ec6cbe1-b5a2-4515-b800-31a61855df41
+  2097152
+  2097152
+  2
+  
+hvm
+
+  
+  
+
+
+
+  
+  
+  destroy
+  restart
+  restart
+  
+ 

Re: [libvirt] [PATCH V2 3/4] Xen drivers: show hap enabled by default in capabilities

[Joao Martins]

On 03/01/2016 04:00 AM, Jim Fehlig wrote:
> Hardware Assisted Paging is enabled by default in Xen. Change
> the capabilities output to reflect this.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  src/libxl/libxl_conf.c   | 2 +-
>  src/xen/xen_hypervisor.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 93c943b..6efd9b5 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -493,7 +493,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
>  
>  if (virCapabilitiesAddGuestFeature(guest,
> "hap",
> -   0,
> +   1,
> 1) == NULL)
>  return -1;
>  }
> diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
> index c1834cb..fc9e1c6 100644
> --- a/src/xen/xen_hypervisor.c
> +++ b/src/xen/xen_hypervisor.c
> @@ -2206,7 +2206,7 @@ xenHypervisorBuildCapabilities(virConnectPtr conn, 
> virArch hostarch,
>  if ((hv_major == 3 && hv_minor >= 3) || (hv_major > 3))
>  if (virCapabilitiesAddGuestFeature(guest,
> "hap",
> -   false,
> +   true,
> true) == NULL)
>  goto no_memory;
>  
> 

For the libxl part,

Reviewed-by: Joao Martins 

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

[libvirt] [PATCH 5/8] tests: Add failure flags to CompareDomainXML2XML

[Cole Robinson]

Will be used in future patches
---
 tests/bhyvexml2xmltest.c   |  2 +-
 tests/genericxml2xmltest.c |  2 +-
 tests/lxcxml2xmltest.c |  2 +-
 tests/qemuxml2xmltest.c|  5 +++--
 tests/testutils.c  | 10 --
 tests/testutils.h  |  4 
 6 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/tests/bhyvexml2xmltest.c b/tests/bhyvexml2xmltest.c
index 8f556ee..87bc39c 100644
--- a/tests/bhyvexml2xmltest.c
+++ b/tests/bhyvexml2xmltest.c
@@ -32,7 +32,7 @@ testCompareXMLToXMLHelper(const void *data)
 
 ret = testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, xml_in,
  info->different ? xml_out : xml_in,
- false,
+ false, 0,
  NULL, NULL, 0);
 
  cleanup:
diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
index bf9b11d..666fc86 100644
--- a/tests/genericxml2xmltest.c
+++ b/tests/genericxml2xmltest.c
@@ -39,7 +39,7 @@ testCompareXMLToXMLHelper(const void *data)
 
 ret = testCompareDomXML2XMLFiles(caps, xmlopt, xml_in,
  info->different ? xml_out : xml_in,
- !info->inactive_only,
+ !info->inactive_only, 0,
  NULL, NULL, 0);
  cleanup:
 VIR_FREE(xml_in);
diff --git a/tests/lxcxml2xmltest.c b/tests/lxcxml2xmltest.c
index 0b51340..c2140bc 100644
--- a/tests/lxcxml2xmltest.c
+++ b/tests/lxcxml2xmltest.c
@@ -45,7 +45,7 @@ testCompareXMLToXMLHelper(const void *data)
 
 ret = testCompareDomXML2XMLFiles(caps, xmlopt, xml_in,
  info->different ? xml_out : xml_in,
- !info->inactive_only,
+ !info->inactive_only, 0,
  NULL, NULL, info->parse_flags);
  cleanup:
 VIR_FREE(xml_in);
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 251effd..b3568df 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -52,7 +52,8 @@ testXML2XMLActive(const void *opaque)
 const struct testInfo *info = opaque;
 
 return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt,
-  info->inName, info->outActiveName, true,
+  info->inName, info->outActiveName,
+  true, 0,
   qemuXML2XMLPreFormatCallback, opaque, 0);
 }
 
@@ -63,7 +64,7 @@ testXML2XMLInactive(const void *opaque)
 const struct testInfo *info = opaque;
 
 return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, info->inName,
-  info->outInactiveName, false,
+  info->outInactiveName, false, 0,
   qemuXML2XMLPreFormatCallback, opaque, 0);
 }
 
diff --git a/tests/testutils.c b/tests/testutils.c
index b1bd4e8..ec1ee38 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -1052,7 +1052,8 @@ virDomainXMLOptionPtr 
virTestGenericDomainXMLConfInit(void)
 
 int
 testCompareDomXML2XMLFiles(virCapsPtr caps, virDomainXMLOptionPtr xmlopt,
-   const char *infile, const char *outfile, bool live,
+   const char *infile, const char *outfile,
+   bool live, testCompareDomXML2XMLFlags flags,
testCompareDomXML2XMLPreFormatCallback cb,
const void *opaque, unsigned int parseFlags)
 {
@@ -1067,8 +1068,12 @@ testCompareDomXML2XMLFiles(virCapsPtr caps, 
virDomainXMLOptionPtr xmlopt,
 if (!live)
 format_flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
 
-if (!(def = virDomainDefParseFile(infile, caps, xmlopt, parse_flags)))
+if (!(def = virDomainDefParseFile(infile, caps, xmlopt, parse_flags))) {
+if (!virtTestOOMActive() &&
+(flags & TEST_COMPARE_DOM_XML2XML_FLAG_EXPECT_PARSE_ERROR))
+goto ok;
 goto fail;
+}
 
 if (!virDomainDefCheckABIStability(def, def)) {
 VIR_TEST_DEBUG("ABI stability check failed on %s", infile);
@@ -1084,6 +1089,7 @@ testCompareDomXML2XMLFiles(virCapsPtr caps, 
virDomainXMLOptionPtr xmlopt,
 if (virtTestCompareToFile(actual, outfile) < 0)
 goto fail;
 
+ ok:
 ret = 0;
  fail:
 VIR_FREE(actual);
diff --git a/tests/testutils.h b/tests/testutils.h
index 752fa52..a09fd58 100644
--- a/tests/testutils.h
+++ b/tests/testutils.h
@@ -134,6 +134,9 @@ int virtTestMain(int argc,
 virCapsPtr virTestGenericCapsInit(void);
 virDomainXMLOptionPtr virTestGenericDomainXMLConfInit(void);
 
+typedef enum {
+TEST_COMPARE_DOM_XML2XML_FLAG_EXPECT_PARSE_ERROR = 1 << 0,
+} testCompareDomXML2XMLFlags;
 typedef int (*testCompareDomXML2XMLPreFormatCallback)(virDomainDefPtr def,
 

[libvirt] [PATCH 8/8] tests: qemu: test with aarch64

[Cole Robinson]

This is an interesting test case since PCI isn't the default for
aarch64.
---
 .../qemuxml2argv-aarch64-virtio-pci-manual-addresses.args| 4 +++-
 .../qemuxml2argv-aarch64-virtio-pci-manual-addresses.xml | 5 +
 .../qemuxml2xmlout-aarch64-virtio-pci-manual-addresses.xml   | 5 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git 
a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.args 
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.args
index e3e962b..f6a148c 100644
--- 
a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.args
+++ 
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.args
@@ -29,4 +29,6 @@ QEMU_AUDIO_DRV=none \
 -device 
scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,\
 id=scsi0-0-0-0 \
 -device 
virtio-net-pci,vlan=0,id=net0,mac=52:54:00:09:a4:37,bus=pcie.0,addr=0x2 \
--net user,vlan=0,name=hostnet0
+-net user,vlan=0,name=hostnet0 \
+-device virtio-net-pci,vlan=1,id=net1,mac=52:54:00:09:a4:38,bus=pci.2,addr=0x1 
\
+-net user,vlan=1,name=hostnet1
diff --git 
a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.xml
index 6a44f19..bf0f249 100644
--- 
a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.xml
+++ 
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-manual-addresses.xml
@@ -39,5 +39,10 @@
   
   
 
+
+  
+  
+  
+
   
 
diff --git 
a/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-manual-addresses.xml
 
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-manual-addresses.xml
index 0ee40f5..8a43d75 100644
--- 
a/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-manual-addresses.xml
+++ 
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-manual-addresses.xml
@@ -50,5 +50,10 @@
   
   
 
+
+  
+  
+  
+
   
 
-- 
2.5.0

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

[libvirt] [PATCH 4/8] util: xml: add virXMLPropertyCount

[Cole Robinson]

Returns an integer count of the number of XML properties an element
has. Will be used in future patches.
---
 src/util/virxml.c | 17 +
 src/util/virxml.h |  1 +
 2 files changed, 18 insertions(+)

diff --git a/src/util/virxml.c b/src/util/virxml.c
index 489bad8..86c021c 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -890,6 +890,23 @@ virXMLChildElementCount(xmlNodePtr node)
 return ret;
 }
 
+/* Returns the number of node's properties, or -1 on error.  */
+long
+virXMLPropertyCount(xmlNodePtr node)
+{
+long ret = 0;
+xmlAttrPtr cur = NULL;
+
+if (!node || node->type != XML_ELEMENT_NODE)
+return -1;
+cur = node->properties;
+while (cur) {
+ret++;
+cur = cur->next;
+}
+return ret;
+}
+
 
 /**
  * virXMLNodeToString: convert an XML node ptr to an XML string
diff --git a/src/util/virxml.h b/src/util/virxml.h
index b94de74..5cf082e 100644
--- a/src/util/virxml.h
+++ b/src/util/virxml.h
@@ -72,6 +72,7 @@ int  virXPathNodeSet(const char *xpath,
 char *  virXMLPropString(xmlNodePtr node,
  const char *name);
 long virXMLChildElementCount(xmlNodePtr node);
+long virXMLPropertyCount(xmlNodePtr node);
 
 /* Internal function; prefer the macros below.  */
 xmlDocPtr  virXMLParseHelper(int domcode,
-- 
2.5.0

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

[libvirt] sVirt shouldn't let Nova do stupid things

[Matthew Booth]

Nova just released a fix for this critical CVE:
https://bugs.launchpad.net/nova/+bug/1548450

To summarise, it's a qcow2 backing file exploit. The user writes a
malicious qcow2 header to the top of a raw disk, then triggers a bug in
Nova which causes it to do format detection.

If you read the bug and comments, you'll see that when I initially reported
it I was fairly dismissive of its impact because it's only exploitable
through libvirt, and the instance is going to be confined by SELinux. But
then Dan B points out that sVirt is going to trust whatever Nova tells it
to do and label it appropriately. Cue rapid ramping of severity, and it
turns out this allows an unprivileged user to read anything on the host,
including all raw block devices.

I'm not sure exactly where, but something in this stack has failed us.
Let's be clear a couple of things, though:

1. This is an egregious, stupid bug in Nova, and Nova shouldn't have
egregious, stupid bugs.
2. SELinux should prevent obviously bad things from happening, even in the
presence of egregious, stupid bugs.

I point that out to head off: 'Well Nova shouldn't do that'. Of course it
shouldn't. However, it might, and when it does, I'd like to think that
SELinux has its back. It doesn't, though.

As I understand it, sVirt is the mechanism libvirt uses for controlling
SELinux. I wonder if the current sVirt model is enough to cover the use
case where the thing connecting to libvirt is large enough to have its own
serious bugs. Is there any way we could define a sane set of operations
independent of Nova?

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [python PATCH] Add support for JOB_COMPLETED event

[Jiri Denemark]

Signed-off-by: Jiri Denemark 
---
 examples/event-test.py |  3 ++
 libvirt-override-virConnect.py |  9 ++
 libvirt-override.c | 64 ++
 3 files changed, 76 insertions(+)

diff --git a/examples/event-test.py b/examples/event-test.py
index 615f86c..5be4978 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -533,6 +533,8 @@ def myDomainEventDeviceAddedCallback(conn, dom, dev, 
opaque):
 def myDomainEventMigrationIteration(conn, dom, iteration, opaque):
 print("myDomainEventMigrationIteration: Domain %s(%s) started migration 
iteration %d" % (
 dom.name(), dom.ID(), iteration))
+def myDomainEventJobCompletedCallback(conn, dom, params, opaque):
+print("myDomainEventJobCompletedCallback: Domain %s(%s) %s" % (dom.name(), 
dom.ID(), params))
 
 ##
 # Network events
@@ -646,6 +648,7 @@ def main():
 vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE, 
myDomainEventAgentLifecycleCallback, None)
 vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_DEVICE_ADDED, 
myDomainEventDeviceAddedCallback, None)
 vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION, 
myDomainEventMigrationIteration, None)
+vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_JOB_COMPLETED, 
myDomainEventJobCompletedCallback, None)
 
 vc.networkEventRegisterAny(None, libvirt.VIR_NETWORK_EVENT_ID_LIFECYCLE, 
myNetworkEventLifecycleCallback, None)
 
diff --git a/libvirt-override-virConnect.py b/libvirt-override-virConnect.py
index 4231195..396a6ed 100644
--- a/libvirt-override-virConnect.py
+++ b/libvirt-override-virConnect.py
@@ -225,6 +225,15 @@
 cb(self, virDomain(self, _obj=dom), iteration, opaque)
 return 0
 
+def _dispatchDomainEventJobCompletedCallback(self, dom, params, cbData):
+"""Dispatches event to python user domain job completed callbacks
+"""
+cb = cbData["cb"]
+opaque = cbData["opaque"]
+
+cb(self, virDomain(self, _obj=dom), params, opaque)
+return 0
+
 def domainEventDeregisterAny(self, callbackID):
 """Removes a Domain Event Callback. De-registering for a
domain callback will disable delivery of this event type """
diff --git a/libvirt-override.c b/libvirt-override.c
index 2308802..ce36280 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -6835,6 +6835,65 @@ 
libvirt_virConnectDomainEventMigrationIterationCallback(virConnectPtr conn ATTRI
 }
 #endif /* VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION */
 
+#ifdef VIR_DOMAIN_EVENT_ID_JOB_COMPLETED
+static int
+libvirt_virConnectDomainEventJobCompletedCallback(virConnectPtr conn 
ATTRIBUTE_UNUSED,
+  virDomainPtr dom,
+  virTypedParameterPtr params,
+  int nparams,
+  void *opaque)
+{
+PyObject *pyobj_cbData = (PyObject*)opaque;
+PyObject *pyobj_dom;
+PyObject *pyobj_ret = NULL;
+PyObject *pyobj_conn;
+PyObject *dictKey;
+PyObject *pyobj_dict = NULL;
+int ret = -1;
+
+LIBVIRT_ENSURE_THREAD_STATE;
+
+pyobj_dict = getPyVirTypedParameter(params, nparams);
+if (!pyobj_dict)
+goto cleanup;
+
+if (!(dictKey = libvirt_constcharPtrWrap("conn")))
+goto cleanup;
+pyobj_conn = PyDict_GetItem(pyobj_cbData, dictKey);
+Py_DECREF(dictKey);
+
+/* Create a python instance of this virDomainPtr */
+virDomainRef(dom);
+if (!(pyobj_dom = libvirt_virDomainPtrWrap(dom))) {
+virDomainFree(dom);
+goto cleanup;
+}
+Py_INCREF(pyobj_cbData);
+
+/* Call the Callback Dispatcher */
+pyobj_ret = PyObject_CallMethod(pyobj_conn,
+
(char*)"_dispatchDomainEventJobCompletedCallback",
+(char*)"OOO",
+pyobj_dom, pyobj_dict, pyobj_cbData);
+
+Py_DECREF(pyobj_cbData);
+Py_DECREF(pyobj_dom);
+
+ cleanup:
+if (!pyobj_ret) {
+DEBUG("%s - ret:%p\n", __FUNCTION__, pyobj_ret);
+PyErr_Print();
+Py_XDECREF(pyobj_dict);
+} else {
+Py_DECREF(pyobj_ret);
+ret = 0;
+}
+
+LIBVIRT_RELEASE_THREAD_STATE;
+return ret;
+}
+#endif /* VIR_DOMAIN_EVENT_ID_JOB_COMPLETED */
+
 static PyObject *
 libvirt_virConnectDomainEventRegisterAny(PyObject *self ATTRIBUTE_UNUSED,
  PyObject *args)
@@ -6940,6 +6999,11 @@ libvirt_virConnectDomainEventRegisterAny(PyObject *self 
ATTRIBUTE_UNUSED,
 cb = 
VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventMigrationIterationCallback);
 break;
 #endif /* VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION */
+#ifdef 

Re: [libvirt] [PATCH v4 5/8] nss: Implement _nss_libvirt_gethostbyname3_r

[Martin Kletzander]

On Thu, Mar 03, 2016 at 06:11:43PM +0100, Michal Privoznik wrote:

The implementation is pretty straightforward. Moreover, because
of the nature of things, gethostbyname_r and gethostbyname2_r can
be implemented at the same time too.

Signed-off-by: Michal Privoznik 
---
config-post.h  |  24 
src/Makefile.am|  59 
src/util/virfile.c |   2 +-
tests/Makefile.am  |   2 +-
tools/Makefile.am  |   8 +-
tools/nss/libvirt_nss.c| 341 -
tools/nss/libvirt_nss.h|  14 +-
tools/nss/libvirt_nss.syms |   4 +-
8 files changed, 446 insertions(+), 8 deletions(-)

diff --git a/config-post.h b/config-post.h
index 8367200..2398d3d 100644
--- a/config-post.h
+++ b/config-post.h
@@ -43,3 +43,27 @@
# undef WITH_YAJL
# undef WITH_YAJL2
#endif
+
+/*
+ * With the NSS module it's the same story as virt-login-shell. See the
+ * explanation above.
+ */
+#ifdef LIBVIRT_NSS
+# undef HAVE_LIBDEVMAPPER_H
+# undef HAVE_LIBNL
+# undef HAVE_LIBNL3
+# undef HAVE_LIBSASL2
+# undef WITH_CAPNG
+# undef WITH_CURL
+# undef WITH_DTRACE_PROBES
+# undef WITH_GNUTLS
+# undef WITH_GNUTLS_GCRYPT
+# undef WITH_MACVTAP
+# undef WITH_NUMACTL
+# undef WITH_SASL
+# undef WITH_SSH2
+# undef WITH_VIRTUALPORT
+# undef WITH_SECDRIVER_SELINUX
+# undef WITH_SECDRIVER_APPARMOR
+# undef WITH_CAPNG
+#endif /* LIBVIRT_NSS */
diff --git a/src/Makefile.am b/src/Makefile.am
index 2ba397f..8ea5422 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -2946,6 +2946,65 @@ endif WITH_LIBVIRTD
endif WITH_SECDRIVER_APPARMOR
EXTRA_DIST += $(SECURITY_DRIVER_APPARMOR_HELPER_SOURCES)

+if WITH_NSS


Remove this condition, otherwise you won't be able to build tests
without nss.


+noinst_LTLIBRARIES += libvirt-nss.la
+
+libvirt_nss_la_SOURCES =   \
+   util/viralloc.c \
+   util/viralloc.h \
+   util/virbitmap.c\
+   util/virbitmap.h\
+   util/virbuffer.c\
+   util/virbuffer.h\
+   util/vircommand.c   \
+   util/vircommand.h   \
+   util/virerror.c \
+   util/virerror.h \
+   util/virfile.c  \
+   util/virfile.h  \
+   util/virjson.c  \
+   util/virjson.h  \
+   util/virkmod.c  \
+   util/virkmod.h  \
+   util/virlease.c \
+   util/virlease.h \
+   util/virlog.c   \
+   util/virlog.h   \
+   util/virobject.c\
+   util/virobject.h\
+   util/virpidfile.c   \
+   util/virpidfile.h   \
+   util/virprocess.c   \
+   util/virprocess.h   \
+   util/virsocketaddr.c\
+   util/virsocketaddr.h\
+   util/virstring.c\
+   util/virstring.h\
+   util/virthread.c\
+   util/virthread.h\
+   util/virthreadjob.c \
+   util/virthreadjob.h \
+   util/virtime.c  \
+   util/virtime.h  \
+   util/virutil.c  \
+   util/virutil.h  \
+   $(NULL)
+
+libvirt_nss_la_CFLAGS =\
+   -DLIBVIRT_NSS   \
+   $(AM_CFLAGS)\
+   $(YAJL_CFLAGS)  \
+   $(NULL)
+libvirt_nss_la_LDFLAGS =   \
+   $(AM_LDFLAGS)   \
+   $(NULL)
+
+libvirt_nss_la_LIBADD =\
+   $(YAJL_LIBS)\
+   $(NULL)
+endif WITH_NSS
+


^^ of course, here too :)


+
install-data-local: install-init install-systemd
if WITH_LIBVIRTD
$(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/lockd"
diff --git a/src/util/virfile.c b/src/util/virfile.c
index f45e18f..0fae0f5 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -554,7 +554,7 @@ int virFileUpdatePerm(const char *path,


#if defined(__linux__) && HAVE_DECL_LO_FLAGS_AUTOCLEAR && \
-!defined(LIBVIRT_SETUID_RPC_CLIENT)
+!defined(LIBVIRT_SETUID_RPC_CLIENT) && !defined(LIBVIRT_NSS)

# if HAVE_DECL_LOOP_CTL_GET_FREE

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 90981dc..55e8432 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -65,7 +65,7 @@ GNULIB_LIBS = \
   ../gnulib/lib/libgnu.la

Re: [libvirt] [PATCH v4 7/8] nss: Introduce a test

[Martin Kletzander]

On Thu, Mar 03, 2016 at 06:11:45PM +0100, Michal Privoznik wrote:

A small test to see how is the nss module working.

Signed-off-by: Michal Privoznik 
---
cfg.mk  |   2 +-
tests/Makefile.am   |  18 
tests/nssdata/virbr0.status |  20 +
tests/nssdata/virbr1.status |  14 
tests/nssmock.c | 140 +++
tests/nsstest.c | 195 
6 files changed, 388 insertions(+), 1 deletion(-)
create mode 100644 tests/nssdata/virbr0.status
create mode 100644 tests/nssdata/virbr1.status
create mode 100644 tests/nssmock.c
create mode 100644 tests/nsstest.c

+int
+open(const char *path, int flags, ...)
+{
+int ret;
+char *newpath = NULL;
+
+init_syms();
+
+if (STRPREFIX(path, LEASEDIR) &&


No need to prefix it since getrealpath() does that.  Call it
unconditionally and then...


+getrealpath(, path) < 0)
+return -1;
+
+if (flags & O_CREAT) {
+va_list ap;
+mode_t mode;
+va_start(ap, flags);
+mode = va_arg(ap, mode_t);
+va_end(ap);
+ret = realopen(newpath ? newpath : path, flags, mode);


you don't need to do this ternary stuff here.

[...]

+if (data->ipAddr[i]) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "Address mismatch. Expected %s got nothing",
+   data->ipAddr[i]);
+goto cleanup;
+}


We could get more info out of this more easily if those ERROR and DEBUG
macros in PATCH 5/8 were defined and used based on environment
variable.  I know it's a bit slower, but if you build --with-debug or
something like that, I think it might be useful.

Otherwise this and all patches I did not reply too look fine (apart from
all the unnecessary arithmetic, but I already said that, didn't I).

Martin


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

Re: [libvirt] [PATCH v4 3/8] Initial support for NSS plugin skeleton

[Martin Kletzander]

On Thu, Mar 03, 2016 at 06:11:41PM +0100, Michal Privoznik wrote:

Name Service Switch is a glibc feature responsible for many
things. Translating domain names into IP addresses and vice versa
is just one of them. However, currently it's the only
functionality that this commit is tickling. Well, in this commit
the plugin skeleton is introduced. Implementation to come in next
patches.
Because of the future testing, where the implementation is to be
linked with a test, this needs to go into static library. Linking
a program with an .so statically is not portable. Therefore a
dummy libnss_libvirt_impl library is being introduced too.

Signed-off-by: Michal Privoznik 
---
configure.ac   |  2 ++
m4/virt-nss.m4 | 51 ++
tools/Makefile.am  | 38 ++
tools/nss/libvirt_nss.c| 36 
tools/nss/libvirt_nss.h| 36 
tools/nss/libvirt_nss.syms |  9 
6 files changed, 172 insertions(+)
create mode 100644 m4/virt-nss.m4
create mode 100644 tools/nss/libvirt_nss.c
create mode 100644 tools/nss/libvirt_nss.h
create mode 100644 tools/nss/libvirt_nss.syms

diff --git a/configure.ac b/configure.ac
index eed2050..4d64998 100644
--- a/configure.ac
+++ b/configure.ac
@@ -257,6 +257,7 @@ LIBVIRT_CHECK_SSH2
LIBVIRT_CHECK_SYSTEMD_DAEMON
LIBVIRT_CHECK_UDEV
LIBVIRT_CHECK_WIRESHARK
+LIBVIRT_CHECK_NSS
LIBVIRT_CHECK_YAJL

AC_MSG_CHECKING([for CPUID instruction])
@@ -2843,6 +2844,7 @@ LIBVIRT_RESULT_SSH2
LIBVIRT_RESULT_SYSTEMD_DAEMON
LIBVIRT_RESULT_UDEV
LIBVIRT_RESULT_WIRESHARK
+LIBVIRT_RESULT_NSS
LIBVIRT_RESULT_YAJL
AC_MSG_NOTICE([  libxml: $LIBXML_CFLAGS $LIBXML_LIBS])
AC_MSG_NOTICE([  dlopen: $DLOPEN_LIBS])
diff --git a/m4/virt-nss.m4 b/m4/virt-nss.m4
new file mode 100644
index 000..207cd34
--- /dev/null
+++ b/m4/virt-nss.m4
@@ -0,0 +1,51 @@
+dnl The libvirt nsswitch plugin
+dnl
+dnl Copyright (C) 2016 Red Hat, Inc.
+dnl
+dnl This library is free software; you can redistribute it and/or
+dnl modify it under the terms of the GNU Lesser General Public
+dnl License as published by the Free Software Foundation; either
+dnl version 2.1 of the License, or (at your option) any later version.
+dnl
+dnl This library is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+dnl Lesser General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU Lesser General Public
+dnl License along with this library.  If not, see
+dnl .
+dnl
+
+AC_DEFUN([LIBVIRT_CHECK_NSS],[
+  AC_ARG_WITH([nss-plugin],
+[AS_HELP_STRING([--with-nss-plugin],
+  [enable Name Servie Switch plugin for resolving guest IP addresses])],
+  [], [with_nss_plugin=check])
+
+  fail=0
+  if test "x$with_nss_plugin" != "xno" ; then
+AC_CHECK_HEADERS([nss.h], [
+with_nss_plugin=yes
+  ],[
+if test "x$with_nss_plugin" = "xyes" ; then
+  fail = 1
+fi
+  ])
+
+if test $fail = 1 ; then
+  AC_MSG_ERROR([Can't build nss plugin without nss.h])
+fi
+
+if test "x$with_nss_plugin" = "xyes" ; then
+  AC_DEFINE_UNQUOTED([NSS], 1, [whether nss plugin is enabled])
+fi
+
+AM_CONDITIONAL(WITH_NSS, [test "x$with_nss_plugin" = "xyes"])


This ^^ needs to be ...


+  fi
+


... here, otherwise the build will fail without nss.


+])
+
+AC_DEFUN([LIBVIRT_RESULT_NSS],[
+  LIBVIRT_RESULT([nss], [$with_nss_plugin])
+])
diff --git a/tools/Makefile.am b/tools/Makefile.am
index 0be3567..9754e42 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -417,6 +417,44 @@ CLEANFILES += wireshark/src/plugin.c

endif WITH_WIRESHARK_DISSECTOR

+LIBVIRT_NSS_SYMBOL_FILE = \
+   $(srcdir)/nss/libvirt_nss.syms
+
+LIBVIRT_NSS_SOURCES = \
+   nss/libvirt_nss.c   \
+   nss/libvirt_nss.h
+
+if WITH_NSS


Also move this ^^ ...


+noinst_LTLIBRARIES += nss/libnss_libvirt_impl.la
+nss_libnss_libvirt_impl_la_SOURCES = \
+   $(LIBVIRT_NSS_SOURCES)
+
+nss_libnss_libvirt_impl_la_CFLAGS = \
+   $(AM_CFLAGS)\
+   $(WARN_CFLAGS)  \
+   $(PIE_CFLAGS)   \
+   $(COVERAGE_CFLAGS)
+


... here, so that we can run tests even without NSS (which is what you
have in place in later tests, they will now work only WITH_NSS)

ACK with that changed.


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

Re: [libvirt] [PATCH v4 2/8] virjson: Resolve const correctness

[Martin Kletzander]

On Thu, Mar 03, 2016 at 06:11:40PM +0100, Michal Privoznik wrote:

Plenty of our virJSON*() APIs don't modify passed object. They
merely get a value stored in it. Note this fact in their
definition and enforce const correctness.

Signed-off-by: Michal Privoznik 
---
src/util/virjson.c | 58 +++---
src/util/virjson.h | 54 +-
2 files changed, 56 insertions(+), 56 deletions(-)

diff --git a/src/util/virjson.c b/src/util/virjson.c
index ae6362b..0a595b9 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -735,7 +735,7 @@ virJSONValueArrayAppend(virJSONValuePtr array,


int
-virJSONValueObjectHasKey(virJSONValuePtr object,
+virJSONValueObjectHasKey(const virJSONValue *object,
 const char *key)
{
size_t i;
@@ -753,7 +753,7 @@ virJSONValueObjectHasKey(virJSONValuePtr object,


virJSONValuePtr
-virJSONValueObjectGet(virJSONValuePtr object,
+virJSONValueObjectGet(const virJSONValue *object,
  const char *key)


You're giving the function a const pointer, but it gives you back
non-const pointer to a data in that object.  That is because the only
const part is the above object, however all the data in it (all
pointers) are non-const.  The function prototype might create false
sense of security (the user will think it does not modify the contents,
but it can modify most of it).  This is one of the few exemptions in
which I would rather pass a non-const pointer just so someone (who does
not know how the objects are stored internally) is not fooled into
thinking it will not do anything with the data.  It might cause a
problem when they think the returning value is not connected to the
object and they can modify it without changing the object because the
function they used took just const.  Don't take me wrong, I'm all for
const-correctness, but to any rule there has to be an exception, right?

I would rather make const only those that are not the exception, like
virJSONValueObjectGet{Key,String}(), virJSONValueObjectKeysNumber() and
so on, but not every one that works.

ACK to those from this list that return non-pointers (like int) or const
pointers.


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

Re: [libvirt] [PATCH v4 4/8] libvirt.spec.in: Introduce libvirt-nss package

[Martin Kletzander]

On Thu, Mar 03, 2016 at 06:11:42PM +0100, Michal Privoznik wrote:

Lets put the NSS module into its own package.

Signed-off-by: Michal Privoznik 
---
libvirt.spec.in | 21 +
1 file changed, 21 insertions(+)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 03e2438..733b347 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -155,6 +155,7 @@
# Non-server/HV driver defaults which are always enabled
%define with_sasl  0%{!?_without_sasl:1}
%define with_audit 0%{!?_without_audit:1}
+%define with_nss_plugin0%{!?_without_nss_plugin:1}


# Finally set the OS / architecture specific special cases
@@ -1218,6 +1219,16 @@ Includes the Sanlock lock manager plugin for the QEMU
driver
%endif

+%if %{with_nss_plugin}
+%package nss
+Summary: Libvirt plugin for Name Service Switch
+Group: Development/Libraries
+Requires: %{name}-client = %{version}-%{release}
+


I would instead require libvirt-daemon-driver-network so it's clean, but
not that it would change anything.  Wanting network driver without the
daemon doesn't make sense and the daemon requires the client.  It' sjust
that it would be a bit cleaner.  ACK either way.


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

Re: [libvirt] [PATCH v4 1/8] Export virLease* functions for leases file handling

[Martin Kletzander]

On Thu, Mar 03, 2016 at 06:11:39PM +0100, Michal Privoznik wrote:

These functions are going to be reused very shortly. So instead
of duplicating the code, lets move them into utils module.

Signed-off-by: Michal Privoznik 
---
po/POTFILES.in |   1 +
src/Makefile.am|   1 +
src/libvirt_private.syms   |   6 +
src/network/leaseshelper.c | 271 +---
src/util/virlease.c| 304 +
src/util/virlease.h|  44 +++
6 files changed, 357 insertions(+), 270 deletions(-)
create mode 100644 src/util/virlease.c
create mode 100644 src/util/virlease.h

diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
index 55ddd58..94a6163 100644
--- a/src/network/leaseshelper.c
+++ b/src/network/leaseshelper.c
@@ -28,34 +28,18 @@
#include 
#include 
#include 
-#include 

-#include "virutil.h"
#include "virthread.h"
#include "virfile.h"
#include "virpidfile.h"
-#include "virbuffer.h"
#include "virstring.h"
#include "virerror.h"
#include "viralloc.h"
-#include "virjson.h"


Keep this ^^ here, it's used in this file. ACK with that.


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

Re: [libvirt] [PATCH] Report error when both live and offline flags are used for migration.

[Jiri Denemark]

On Thu, Mar 03, 2016 at 06:08:20 -0500, Nitesh Konkar wrote:
> ---
>  src/libvirt-domain.c | 27 +++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 9491845..dc11945 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3617,6 +3617,15 @@ virDomainMigrate(virDomainPtr domain,
>   error);
>  
> +if (flags & VIR_MIGRATE_OFFLINE) {

I asked you to move the following if () {...} block...

> +if (flags & VIR_MIGRATE_LIVE) {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +   _("Live and offline migration flags are "
> + "mutually exclusive"));
> +goto error;
> +}
> +}
> +
>  if (flags & VIR_MIGRATE_OFFLINE) {

... here.

>  if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
>VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
>  virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",

I wanted to do that and push the patch, but I realized the patch is
incomplete (it doesn't cover MigrateToURI* APIs) and there is even a
better place to add these checks...

Something like the following (untested):

diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c
index 64cbffa..36a939d 100644
--- i/src/qemu/qemu_migration.c
+++ w/src/qemu/qemu_migration.c
@@ -3081,6 +3081,12 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
 goto cleanup;
 
 if (flags & VIR_MIGRATE_OFFLINE) {
+if (flags & VIR_MIGRATE_LIVE) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("live and offline migration flags are "
+ "mutually exclusive"));
+goto cleanup;
+}
 if (flags & (VIR_MIGRATE_NON_SHARED_DISK |
  VIR_MIGRATE_NON_SHARED_INC)) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -3335,6 +3341,12 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
 virNWFilterReadLockFilterUpdates();
 
 if (flags & VIR_MIGRATE_OFFLINE) {
+if (flags & VIR_MIGRATE_LIVE) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("live and offline migration flags are "
+ "mutually exclusive"));
+goto cleanup;
+}
 if (flags & (VIR_MIGRATE_NON_SHARED_DISK |
  VIR_MIGRATE_NON_SHARED_INC)) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",

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

Re: [libvirt] [PATCH 2/9] conf: Extract code filling data for virDomainGetVcpuPinInfo

[Peter Krempa]

On Fri, Mar 04, 2016 at 07:30:01 -0500, John Ferlan wrote:
> 
> 
> On 02/24/2016 09:22 AM, Peter Krempa wrote:
> > The implementation of the inner guts of the function is similar for all
> > drivers, so we can add a helper and not have to reimplement it three
> > times.
> > ---
> >  src/conf/domain_conf.c   | 64 
> > 
> >  src/conf/domain_conf.h   |  8 ++
> >  src/libvirt_private.syms |  1 +
> >  src/libxl/libxl_driver.c | 38 +++-
> >  src/qemu/qemu_driver.c   | 43 +++-
> >  src/test/test_driver.c   | 38 
> >  6 files changed, 84 insertions(+), 108 deletions(-)
> > 
> 
> ACK with one nit...
> 
> One difference I noted, the 'libxl' code would:
> 
> memset(cpumaps, 0x00, maplen * ncpumaps);
> 
> just before filling.  Should that be replicated in the common code?  If
> not then for the libxl code should the lines be kept?

The array members set in our code are cleared by virBitmapToDataBuf. If
the array is larger than filled by this code (which it shouldn't be
really) the rest of the array will be now kept untouched, but the caller
should not use that data either, so this should be safe.

Peter


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

Re: [libvirt] [PATCH 06/24] tests: hostdev: Group test cases

[Andrea Bolognani]

On Mon, 2016-03-07 at 21:57 -0500, Laine Stump wrote:
> On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
> > 
> > Instead of considering each single step its own test case, create
> > high level test cases that reproduce a certain scenario.
> > ---
> >   tests/virhostdevtest.c | 119 
> >-
> >   1 file changed, 97 insertions(+), 22 deletions(-)
> I like the idea, just have two comments:
> 
> 1) would it maybe be useful to keep the individual tests, in order to 
> make it simpler to determine which piece had broken in the case of a 
> regression?

If some test case fails you're probably going to be using
VIR_TEST_DEBUG, gdb or a combination of the two to figure out
what went wrong, so I'm not convinced testing the functions
separately would add much value.

> 2) It looks like this only tests for legacy kvm (i.e. 
> attaching/detaching pci-stub). It would be good to have it test for vfio 
> as well (although maybe that can be saved for after we add support for 
> using individual devices' driver_override to attach different drivers).

Sure, adding more test cases is always good :)

I'm not sure what this driver_override you refer to is
supposed to be though, care to expand on that?

> ACK as is, the comments are food for thought.

Pushed along with the rest.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH 08/24] hostdev: Remove false comment

[Andrea Bolognani]

On Mon, 2016-03-07 at 22:02 -0500, Laine Stump wrote:
> On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
> > 
> > ---
> >   src/util/virhostdev.c | 3 ---
> >   1 file changed, 3 deletions(-)
> > 
> > diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> > index adc4eec..5be61cd 100644
> > --- a/src/util/virhostdev.c
> > +++ b/src/util/virhostdev.c
> > @@ -727,9 +727,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr 
> > hostdev_mgr,
> >   virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
> >   
> >   if (virPCIDeviceGetManaged(dev)) {
> > -/* NB: This doesn't actually re-bind to original driver, just
> > - * unbinds from the stub driver
> > - */
> >   VIR_DEBUG("Reattaching managed PCI device %s",
> > virPCIDeviceGetName(dev));
> >   ignore_value(virPCIDeviceReattach(dev,
> ACK.

The commit message was supposed to be a placeholder, so I
reworked it a bit before pushing.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH 03/24] tests: hostdev: Declare count inside CHECK_LIST_COUNT()

[Andrea Bolognani]

On Mon, 2016-03-07 at 13:18 -0500, Laine Stump wrote:
> > +# define CHECK_LIST_COUNT(list, cnt)   
> >  \
> > +do {   
> >  \
> > +int count; 
> >  \
> > +if ((count = virPCIDeviceListCount(list)) != cnt) {
> >  \
> > +virReportError(VIR_ERR_INTERNAL_ERROR, 
> >  \
> > +   "Unexpected count of items in " #list ": %d, "  
> >  \
> > +   "expecting %zu", count, (size_t) cnt);  
> >  \
> > +goto cleanup;  
> >  \
> > +}  
> >  \
> > +} while (0)
> 
> The only suggestion I would have is to make "count" something less 
> common, so that you're less likely to end up causing a compiler warning 
> later if someone adds a variable called "count" to a function that uses 
> this macro.
> 
> Otherwise ACK.

I changed it to 'actualCount' before pushing, and explained the
reason for the change in the commit message.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH 10/24] hostdev: Remove explicit NULL checks

[Andrea Bolognani]

On Mon, 2016-03-07 at 22:03 -0500, Laine Stump wrote:
> On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
> > 
> > NULL checks are performed implicitly in the rest of the module,
> > including other allocations in the very same function.
> > ---
> >   src/util/virhostdev.c | 8 
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> ACK.

Series pushed until here, thanks for the review so far :)

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] GSoC 2016

[Peter Krempa]

On Mon, Mar 07, 2016 at 19:28:46 +0100, Michal Privoznik wrote:
> On 07.03.2016 18:20, Peter Krempa wrote:
> > On Mon, Mar 07, 2016 at 16:50:41 +0100, Martin Kletzander wrote:

[...]

> How come! I see 'View Ideas List' button in the very top right corner.
> It links to:
> 
> http://wiki.libvirt.org/page/Google_Summer_of_Code_2016
> 
> The design of GSoC pages has changed this year so it might be confusing
> to some users to get all the information at the first glance.

Indeed. Apparently the new version has bad design too as I didn't look
at any previous version. I've noticed the link to our webpage and
the mailing list address but not the ideas list at first glance.

Also the "successful proposal guidance" doesn't really help by providing
information in regard to contributing and the selection process. I think
they should be presented more obviously so that we prevent recurring
questions.


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