[libvirt] [PATCH v3 4/9] Refuse to reattach from vfio if the iommu group is in use by any domain

[Shivaprasad G Bhat]

It is incorrect to attempt the device reattach of a function,
when some other domain is using some functions belonging to the same iommu
group.

Signed-off-by: Shivaprasad G Bhat 
---
 src/util/virhostdev.c |   11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index de029a0..4c441b9 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -1590,6 +1590,7 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr 
hostdev_mgr,
 virPCIDevicePtr pci)
 {
 virPCIDeviceAddressPtr devAddr = NULL;
+bool usesVfio = STREQ_NULLABLE(virPCIDeviceGetStubDriver(pci), "vfio-pci");
 struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL,
  false};
 int ret = -1;
@@ -1600,8 +1601,16 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr 
hostdev_mgr,
 if (!(devAddr = virPCIDeviceGetAddress(pci)))
 goto out;
 
-if (virHostdevIsPCINodeDeviceUsed(devAddr, ))
+if (usesVfio) {
+/* Doesn't matter which function. If any domain is actively using the
+ * iommu group, refuse to reattach */
+if (virPCIDeviceAddressIOMMUGroupIterate(devAddr,
+ virHostdevIsPCINodeDeviceUsed,
+ ) < 0)
+goto out;
+} else if (virHostdevIsPCINodeDeviceUsed(devAddr, )) {
 goto out;
+}
 
 virPCIDeviceReattachInit(pci);
 

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

[libvirt] [PATCH v3 5/9] Wait for vfio-pci device cleanups before reassinging the device to host driver

[Shivaprasad G Bhat]

Before unbind from stub driver libvirt should be sure the guest is not using
the device anymore. The libvirt today waits for pci-stub driver alone in 
/proc/iommu.
The same wait is needed for vfio devices too.

Signed-off-by: Shivaprasad G Bhat 
---
 src/util/virhostdev.c |7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 4c441b9..c5a6553 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -756,6 +756,13 @@ virHostdevReattachPCIDevice(virPCIDevicePtr dev, 
virHostdevManagerPtr mgr)
 usleep(100*1000);
 retries--;
 }
+} else if (STREQ(virPCIDeviceGetStubDriver(dev), "vfio-pci")) {
+int retries = 100;
+while (virPCIDeviceWaitForCleanup(dev, "vfio-pci")
+   && retries) {
+usleep(100*1000);
+retries--;
+}
 }
 
 if (virPCIDeviceReattach(dev, mgr->activePCIHostdevs,

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

[libvirt] [PATCH v3 3/9] Add iommu group number info to virPCIDevice

[Shivaprasad G Bhat]

The iommu group number need not be fetched from the sysfs
everytime as it remains constant. Fetch it once during
allocation

Signed-off-by: Shivaprasad G Bhat 
---
 src/libvirt_private.syms |1 +
 src/util/virpci.c|   11 +++
 src/util/virpci.h|2 ++
 3 files changed, 14 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a835f18..1e624fe 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1971,6 +1971,7 @@ virPCIDeviceNew;
 virPCIDeviceReattach;
 virPCIDeviceReattachInit;
 virPCIDeviceReset;
+virPCIDeviceSetIommuGroup;
 virPCIDeviceSetManaged;
 virPCIDeviceSetRemoveSlot;
 virPCIDeviceSetReprobe;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 876da70..b03521c 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -75,6 +75,7 @@ struct _virPCIDevice {
 bool  has_pm_reset;
 bool  managed;
 char  *stubDriver;
+int   iommuGroup;
 
 /* used by reattach function */
 bool  unbind_from_stub;
@@ -1566,6 +1567,8 @@ virPCIDeviceNew(unsigned int domain,
 char *product = NULL;
 char *drvpath = NULL;
 char *driver = NULL;
+virPCIDeviceAddress devAddr = { domain, bus,
+slot, function };
 
 if (VIR_ALLOC(dev) < 0)
 return NULL;
@@ -1613,6 +1616,8 @@ virPCIDeviceNew(unsigned int domain,
 goto error;
 }
 
+dev->iommuGroup = virPCIDeviceAddressGetIOMMUGroupNum();
+
 if (virPCIDeviceGetDriverPathAndName(dev, , ) < 0)
 goto cleanup;
 
@@ -1726,6 +1731,12 @@ virPCIDeviceSetStubDriver(virPCIDevicePtr dev, const 
char *driver)
 return VIR_STRDUP(dev->stubDriver, driver);
 }
 
+void
+virPCIDeviceSetIommuGroup(virPCIDevicePtr dev, int iommu)
+{
+   dev->iommuGroup = iommu;
+}
+
 const char *
 virPCIDeviceGetStubDriver(virPCIDevicePtr dev)
 {
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 64b9e96..a6c3628 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -87,6 +87,8 @@ int virPCIDeviceReset(virPCIDevicePtr dev,
   virPCIDeviceListPtr activeDevs,
   virPCIDeviceListPtr inactiveDevs);
 
+void virPCIDeviceSetIommuGroup(virPCIDevice *dev,
+   int iommu);
 void virPCIDeviceSetManaged(virPCIDevice *dev,
 bool managed);
 unsigned int virPCIDeviceGetManaged(virPCIDevice *dev);

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

Re: [libvirt] [PATCH v2] gobject: Add wrapper virDomainSetTime()

[Christophe Fergeau]

Some minor comments below, ACK.

Christophe

On Thu, Oct 29, 2015 at 08:44:17PM +, Zeeshan Ali (Khattak) wrote:
> ---
> 
> This version:
> 
> * Replaces the seconds and nseconds arguments by a GDateTime.
> * Drops the use of flags argument, since caller can specify the only flag 
> currently possible (VIR_DOMAIN_TIME_SYNC) by simply passing a NULL as the 
> GDateTime argument.
> * Add some needed articles to doc comment.
> 
>  libvirt-gobject/libvirt-gobject-domain.c | 121 
> +++
>  libvirt-gobject/libvirt-gobject-domain.h |  15 +++-
>  libvirt-gobject/libvirt-gobject.sym  |   9 +++
>  3 files changed, 144 insertions(+), 1 deletion(-)
> 
> diff --git a/libvirt-gobject/libvirt-gobject-domain.c 
> b/libvirt-gobject/libvirt-gobject-domain.c
> index 34eb7ca..debae2d 100644
> --- a/libvirt-gobject/libvirt-gobject-domain.c
> +++ b/libvirt-gobject/libvirt-gobject-domain.c
> @@ -1886,3 +1886,124 @@ gboolean 
> gvir_domain_get_has_current_snapshot(GVirDomain *dom,
>  
>  return TRUE;
>  }
> +
> +/**
> + * gvir_domain_set_time:
> + * @dom: the domain
> + * @date_time: (allow-none)(transfer none): the time to set as #GDateTime.
> + * @flags: Unused, Pass 0.

'pass 0' rather than 'Pass 0' ?
Missing "@err: (allow-none): Place-holder for error or NULL"?

> + *
> + * This function tries to set guest time to the given value. The passed
> + * time must in UTC.
> + *
> + * If @date_time is %NULL, the time is reset using the domain's RTC.
> + *
> + * Please note that some hypervisors may require guest agent to be configured
> + * and running in order for this function to work.
> + */
> +gboolean gvir_domain_set_time(GVirDomain *dom,
> +  GDateTime *date_time,
> +  guint flags G_GNUC_UNUSED,
> +  GError **err)
> +{
> +GVirDomainPrivate *priv;
> +int ret;
> +GTimeVal tv;
> +gint64 seconds;
> +guint nseconds;

They are going to store GTimeVal fields, glong could be used for
seconds, and glong or guint64 for nseconds. (but I think the types you
picked are going to fit tv_sec and tv_usec * 1000).

> +guint settime_flags;
> +
> +g_return_val_if_fail(GVIR_IS_DOMAIN(dom), FALSE);
> +g_return_val_if_fail(err == NULL || *err == NULL, FALSE);

I'd keep a g_return_val_if_fail(flags == 0, FALSE);

> +
> +if (date_time != NULL) {
> +if (!g_date_time_to_timeval(date_time, )) {
> +gvir_set_error_literal(err, GVIR_DOMAIN_ERROR,
> +   0,
> +   "Failed to parse given time argument");

g_set_error_literal, not gvir_set_error_literal.

> +return FALSE;
> +}
> +
> +seconds = tv.tv_sec;
> +nseconds = tv.tv_usec * 1000;
> +settime_flags = 0;
> +} else {
> +seconds = 0;
> +nseconds = 0;
> +settime_flags = VIR_DOMAIN_TIME_SYNC;
> +}
> +
> +priv = dom->priv;
> +ret = virDomainSetTime(priv->handle, seconds, nseconds, settime_flags);

Nit: I'd just go with dom->priv->handle rather than having a local
'priv' used only once.

> +if (ret < 0) {
> +gvir_set_error_literal(err, GVIR_DOMAIN_ERROR,
> +   0,
> +   "Unable to set domain time");
> +return FALSE;
> +}
> +
> +return TRUE;
> +}
> +
> +static void
> +gvir_domain_set_time_helper(GTask *task,
> +gpointer object,
> +gpointer task_data,
> +GCancellable *cancellable G_GNUC_UNUSED)
> +{
> +GVirDomain *dom = GVIR_DOMAIN(object);
> +GDateTime *date_time = (GDateTime *) task_data;
> +GError *err = NULL;
> +
> +if (!gvir_domain_set_time(dom, date_time, 0, ))
> +g_task_return_error(task, err);
> +else
> +g_task_return_boolean(task, TRUE);
> +}
> +
> +/**
> + * gvir_domain_set_time_async:
> + * @dom: the domain
> + * @date_time: (allow-none)(transfer none): the time to set as #GDateTime.
> + * @flags: bitwise-OR of #GVirDomainSetTimeFlags.
> + * @cancellable: (allow-none)(transfer none): cancellation object
> + * @callback: (scope async): completion callback
> + * @user_data: (closure): opaque data for callback
> + *
> + * Asynchronous variant of #gvir_domain_set_time.
> + */
> +void gvir_domain_set_time_async(GVirDomain *dom,
> +GDateTime *date_time,
> +guint flags G_GNUC_UNUSED,
> +GCancellable *cancellable,
> +GAsyncReadyCallback callback,
> +gpointer user_data)
> +{
> +GTask *task;
> +
> +g_return_if_fail(GVIR_IS_DOMAIN(dom));
> +g_return_if_fail((cancellable == NULL) || G_IS_CANCELLABLE(cancellable));

Same comment about testing that flags is actually 0

> +
> +task = g_task_new(G_OBJECT(dom),
> + 

Re: [libvirt] [libvirt-glib 2/3] gconfig: Drop redundant glib compatibility code

[Christophe Fergeau]

On Thu, Oct 29, 2015 at 08:39:07PM +, Zeeshan Ali (Khattak) wrote:
> We already require and use glib >= 2.36 so there is no reason to keep
> around code to ensure compatibility with glib oler than that.

Same 'older' typo, ACK.

Christophe

> ---
>  libvirt-gconfig/libvirt-gconfig-compat.h | 20 
>  1 file changed, 20 deletions(-)
> 
> diff --git a/libvirt-gconfig/libvirt-gconfig-compat.h 
> b/libvirt-gconfig/libvirt-gconfig-compat.h
> index fbf552c..c9ac645 100644
> --- a/libvirt-gconfig/libvirt-gconfig-compat.h
> +++ b/libvirt-gconfig/libvirt-gconfig-compat.h
> @@ -25,26 +25,6 @@
>  
>  #include 
>  
> -#if !GLIB_CHECK_VERSION(2,32,0)
> -
> -#if__GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 1)
> -#define G_DEPRECATED __attribute__((__deprecated__))
> -#elif defined(_MSC_VER) && (_MSC_VER >= 1300)
> -#define G_DEPRECATED __declspec(deprecated)
> -#else
> -#define G_DEPRECATED
> -#endif
> -
> -#if__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5)
> -#define G_DEPRECATED_FOR(f) __attribute__((__deprecated__("Use '" #f "' 
> instead")))
> -#elif defined(_MSC_FULL_VER) && (_MSC_FULL_VER > 140050320)
> -#define G_DEPRECATED_FOR(f) __declspec(deprecated("is deprecated. Use '" #f 
> "' instead"))
> -#else
> -#define G_DEPRECATED_FOR(f) G_DEPRECATED
> -#endif
> -
> -#endif
> -
>  #if GLIB_CHECK_VERSION(2, 35, 0)
>  #define g_type_init()
>  #endif
> -- 
> 2.5.0
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


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

Re: [libvirt] [libvirt-glib 1/3] gobject: Drop redundant glib compatibility code

[Christophe Fergeau]

On Thu, Oct 29, 2015 at 08:39:06PM +, Zeeshan Ali (Khattak) wrote:
> We already require and use glib >= 2.36 so there is no reason to keep
> around code to ensure compatibility with glib oler than that.

'older'. Nice catch/cleanup, thanks!

Christophe


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

Re: [libvirt] oVirt considers using macTableManager='libvirt'

[Vlad Yasevich]

On 11/01/2015 01:05 PM, Laine Stump wrote:
> On 11/01/2015 08:52 AM, Ido Barkan wrote:
>> Hi guys,
>>
>> We, at oVirt, are considring using the automatic bridge management
>> feature of libvirt for our hosts
>> (macTableManager='libvirt').
>> I could find any discussion in the mailing list archives about the
>> motivation for this feature.
>>(was there?). If there wasn't I would like to start a new one, about
>> the possible trade offs it would
>>have in oVirt.
>>Specifically I have a few questions:
>>
>> 1) The obvious performance motivation is clear: considering N hosts
>> with M vms each connected to
>> the same LAN, the first packet to any unknown yet host will flood
>> all the vms in all N bridges.
>>-- was there any other motivation that I do not understand
>> (apart from slightly better security?
> 
> It allows turning off both learning and flood, which improves performance, 
> and also causes
> the physdev attached to the bridge to be automatically switched *out of * 
> promiscuous mode
> (since the bridge always knows the list of MAC addresses that it should be 
> listening for,
> it can just keep the physdev's mac filter table appropriately loaded, and 
> have no need for
> promiscuous mode). Note that promisc mode can't be turned off if the bridge 
> is connected
> to multiple physdevs (unless they are hooked together as a bond).
> 
>> 2) oVirt uses TC for port mirroring, in case this is requested by
>> users, to mirror traffic from a chosen
>> bridge to a chosen NIC in the host. I could not understand if
>> macTableManager will interfere
>> with it, or not.
> 

As long as mirroring is only done in one direction (bridge to host NIC), there 
shouldn't
be any interference.   The bridge is is still programmed with the correct set 
of mac
addresses and should still be able to receive traffic destined for the VM.

> I'm not sure, as I'm unfamiliar with this. I'm Cc's Vlad from qemu/kernel 
> networking to
> see if he can give better information. (Vlad, please correct anything else 
> I've gotten
> wrong in this response).
> 
> 
>> 3) Are there any drawbacks to enabling this feature?
> 
> If a guest changes its MAC address, or expects to have traffic for multiple 
> MAC addresses
> sent to it, you'll have problems. I don't remember right now if I also setup 
> libvirt to
> respond to mac filter change events for tap devices connected to a bridge (as 
> I have done
> for macvtap devices), but will look it up tomorrow and tell you.
> 
>> 4) We aim for rhel7.2. Will the feature be supported (or partially
>> supported) for kernels older then
>>  3.17? And if so, in what way?
> 
> I'm pretty sure that anything necessary to support it was backported to the 
> kernel used in
> RHEL/CentOS7.2 (if it wasn't there already). Vlad will know for sure.

I think we initially turned this on in 7.2, so everything should be there.

> 
>> 5) oVirt currently builds its own bridges and tell libvirt about them.
>> Does that have any affect on the
>>  functionality of that feature?
> 
> No. It works both for bridges created by libvirt and those created outside of 
> libvirt.
> 
>> 6) are there any plans to support OVS for this feature in the future?
> 

Yes, we have plans to support this for OVS.

-vlad

> No concrete plans, but if someone wants to implement it, I'd be happy to 
> assist/review :-)
> 
> 

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

[libvirt] [PATCH v3 6/9] Split reprobe action from the virPCIUnbindFromStub into a new function

[Shivaprasad G Bhat]

The reprobe needs to be called multiple times for vfio devices for each
device in the iommu group in future patch. So split the reprobe into a
new function. No functional change.

Signed-off-by: Shivaprasad G Bhat 
---
 src/util/virpci.c |   47 +++
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index b03521c..f395fdd 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1099,6 +1099,36 @@ virPCIIsKnownStub(char *driver)
 }
 
 static int
+virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev,
+  const char *driver,
+  const char *drvdir)
+{
+char *path = NULL;
+int ret = -1;
+
+/* Trigger a re-probe of the device is not in the stub's dynamic
+ * ID table. If the stub is available, but 'remove_id' isn't
+ * available, then re-probing would just cause the device to be
+ * re-bound to the stub.
+ */
+if (driver && !(path = virPCIDriverFile(driver, "remove_id")))
+goto cleanup;
+
+if (!driver || !virFileExists(drvdir) || virFileExists(path)) {
+if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) {
+virReportSystemError(errno,
+ _("Failed to trigger a re-probe for PCI 
device '%s'"),
+ dev->name);
+goto cleanup;
+}
+}
+ret = 0;
+ cleanup:
+VIR_FREE(path);
+return ret;
+}
+
+static int
 virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
 {
 int result = -1;
@@ -1150,24 +1180,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
 goto cleanup;
 }
 
-/* Trigger a re-probe of the device is not in the stub's dynamic
- * ID table. If the stub is available, but 'remove_id' isn't
- * available, then re-probing would just cause the device to be
- * re-bound to the stub.
- */
-VIR_FREE(path);
-if (driver && !(path = virPCIDriverFile(driver, "remove_id")))
+if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0)
 goto cleanup;
 
-if (!driver || !virFileExists(drvdir) || virFileExists(path)) {
-if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) {
-virReportSystemError(errno,
- _("Failed to trigger a re-probe for PCI 
device '%s'"),
- dev->name);
-goto cleanup;
-}
-}
-
 result = 0;
 
  cleanup:

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

Re: [libvirt] [PATCH] bhyve: add bootrom support

[Daniel P. Berrange]

On Mon, Nov 02, 2015 at 04:24:09PM +0300, Roman Bogorodskiy wrote:
> Recently, bhyve got UEFI support that eliminates necessity of
> having two-step process of starting a VM, i.e. loading OS
> using loader like bhyveloader or grub and then calling bhyve itself.
> 
> From user perspective usage looks the following: if a domain XML
> contains the '' element and it's value is a path
> to non-executable file, then it's treated as a path to bootrom
> file and external loader is not execute.

Actually,  is used to signify a helper program that
runs in host context to acquire a kernel + initrd image to boot
a guest from. So that's not the right element to be using here

If you want to provide a guest BIOS image, then you want to use
the  element instead. IIUC, for UEFI, we use this with
QEMU/KVM:

  /usr/share/OVMF/OVMF_CODE.fd

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

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

[libvirt] [PATCH 7/9] Pass activeDevs and inactiveDevs to virPCIDeviceUnbindFromStub and virPCIDeviceBindToStub

[Shivaprasad G Bhat]

The inactiveDevs need to be selectively altered for more than one
device in case of vfio devices. So, pass the whole list.

Signed-off-by: Shivaprasad G Bhat 
---
 src/util/virpci.c |   36 +++-
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index f395fdd..0c20053 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1129,7 +1129,9 @@ virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev,
 }
 
 static int
-virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
+virPCIDeviceUnbindFromStub(virPCIDevicePtr dev,
+   virPCIDeviceListPtr activeDevs ATTRIBUTE_UNUSED,
+   virPCIDeviceListPtr inactiveDevs)
 {
 int result = -1;
 char *drvdir = NULL;
@@ -1186,6 +1188,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
 result = 0;
 
  cleanup:
+if ((result == 0) && inactiveDevs)
+virPCIDeviceListDel(inactiveDevs, dev);
+
 /* do not do it again */
 dev->unbind_from_stub = false;
 dev->remove_slot = false;
@@ -1201,7 +1206,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
 
 static int
 virPCIDeviceBindToStub(virPCIDevicePtr dev,
-   const char *stubDriverName)
+   const char *stubDriverName,
+   virPCIDeviceListPtr activeDevs,
+   virPCIDeviceListPtr inactiveDevs)
 {
 int result = -1;
 bool reprobe = false;
@@ -1328,9 +1335,15 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev,
 VIR_FREE(driverLink);
 VIR_FREE(path);
 
+/* Add *a copy of* the dev into list inactiveDevs, if
+ * it's not already there. */
+if ((result == 0) && inactiveDevs && !virPCIDeviceListFind(inactiveDevs, 
dev) &&
+virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) {
+result = -1;
+}
 if (result < 0) {
 VIR_FREE(newDriverName);
-virPCIDeviceUnbindFromStub(dev);
+virPCIDeviceUnbindFromStub(dev, activeDevs, inactiveDevs);
 } else {
 VIR_FREE(dev->stubDriver);
 dev->stubDriver = newDriverName;
@@ -1377,16 +1390,9 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
 return -1;
 }
 
-if (virPCIDeviceBindToStub(dev, dev->stubDriver) < 0)
-return -1;
-
-/* Add *a copy of* the dev into list inactiveDevs, if
- * it's not already there.
- */
-if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) &&
-virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) {
+if (virPCIDeviceBindToStub(dev, dev->stubDriver,
+   activeDevs, inactiveDevs) < 0)
 return -1;
-}
 
 return 0;
 }
@@ -1402,13 +1408,9 @@ virPCIDeviceReattach(virPCIDevicePtr dev,
 return -1;
 }
 
-if (virPCIDeviceUnbindFromStub(dev) < 0)
+if (virPCIDeviceUnbindFromStub(dev, activeDevs, inactiveDevs) < 0)
 return -1;
 
-/* Steal the dev from list inactiveDevs */
-if (inactiveDevs)
-virPCIDeviceListDel(inactiveDevs, dev);
-
 return 0;
 }
 

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

[libvirt] [PATCH v3 2/9] Initialize the stubDriver of pci devices if bound to a valid one

[Shivaprasad G Bhat]

The stubDriver name can be used to make useful decisions if readily available.
Set it if bound to a valid one during initialisation.

Signed-off-by: Shivaprasad G Bhat 
---
 src/util/virpci.c |   12 
 1 file changed, 12 insertions(+)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index bff37d7..876da70 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1564,6 +1564,8 @@ virPCIDeviceNew(unsigned int domain,
 virPCIDevicePtr dev;
 char *vendor = NULL;
 char *product = NULL;
+char *drvpath = NULL;
+char *driver = NULL;
 
 if (VIR_ALLOC(dev) < 0)
 return NULL;
@@ -1611,9 +1613,19 @@ virPCIDeviceNew(unsigned int domain,
 goto error;
 }
 
+if (virPCIDeviceGetDriverPathAndName(dev, , ) < 0)
+goto cleanup;
+
 VIR_DEBUG("%s %s: initialized", dev->id, dev->name);
 
+if (!virPCIIsKnownStub(driver)) {
+VIR_FREE(driver);
+goto cleanup;
+}
+dev->stubDriver = driver;
+
  cleanup:
+VIR_FREE(drvpath);
 VIR_FREE(product);
 VIR_FREE(vendor);
 return dev;

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

Re: [libvirt] [PATCH] bhyve: add bootrom support

[Roman Bogorodskiy]

  Daniel P. Berrange wrote:

> On Mon, Nov 02, 2015 at 04:24:09PM +0300, Roman Bogorodskiy wrote:
> > Recently, bhyve got UEFI support that eliminates necessity of
> > having two-step process of starting a VM, i.e. loading OS
> > using loader like bhyveloader or grub and then calling bhyve itself.
> > 
> > From user perspective usage looks the following: if a domain XML
> > contains the '' element and it's value is a path
> > to non-executable file, then it's treated as a path to bootrom
> > file and external loader is not execute.
> 
> Actually,  is used to signify a helper program that
> runs in host context to acquire a kernel + initrd image to boot
> a guest from. So that's not the right element to be using here
> 
> If you want to provide a guest BIOS image, then you want to use
> the  element instead. IIUC, for UEFI, we use this with
> QEMU/KVM:
> 
>   /usr/share/OVMF/OVMF_CODE.fd

Ah, I should read formatdomain.html more carefully!

I'll work on v2, thanks for the feedback.

Roman Bogorodskiy


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

Re: [libvirt] [PATCH 7/8] Postpone reprobing till all the devices in iommu group are unbound from vfio

[Andrea Bolognani]

On Mon, 2015-11-02 at 10:06 +0530, Shivaprasad bhat wrote:
> > > +{
> > > +int ret = 0;
> > > +virPCIDevicePtr pci = NULL;
> > > +
> > > +if (!(pci = virPCIDeviceNew(devAddr->domain, devAddr->bus,
> > > +devAddr->slot, devAddr->function)))
> > > +goto cleanup;
> > > +
> > > +if (STREQ_NULLABLE(pci->stubDriver, "vfio-pci"))
> > > +ret = -1;
> > 
> > As mentioned in the comment for Patch 1, I think this is
> > fairly obscure: if I had not read throught the whole
> > series, I'd assume this checks whether the device is
> > configured to use vfio-pci as stub driver, not whether
> > it is currently bound to it, and it would not be
> > immediately clear to me how this check fits in a function
> > called virPCIDeviceBoundToVFIODriver().
> > 
> > I think it would be much cleaner to query the driver
> > explicitly using virPCIDeviceGetDriverPathAndName() and
> > remove that call from virPCIDeviceNew().
> > 
> 
> The PCIDeviceNew does exactly the same of going through
> the sysfs and populating it. Its only code which is as though
> the stubDriver scan is happening inside than explicitly outside.
> 
> The Iterator today takes virPCIDeviceAddressPtr and
> virPCIDeviceGetDriverPathAndName for which we anyway need to
> call virPCIDevNew. I am just avoiding one extra function call by
> moving it inside virPCIDevNew

The problem I have with this approach is that, as far as I
understand, before your patch dev->stubDriver is a configuration
item: you set it to whatever's appropriate, and the value
is later inspected in virPCIDeviceBindToStub()[1] to decide
what stub driver the device should be bound to.

After your patch, depending on the context, it could be either
a configuration item, as before, or state, eg. the current
stub driver the device is bound to, with no clear way to
tell one situation from the other.

I think using a single attribute to store related but different
information should be avoided, as it has the potential to make
the code much harder to understand, especially later on for
someone who's not been involved in the implementation.

Moreover you only need that value for comparison purposes in a
couple of places, and by moving the code to virPCIDeviceNew()
you make it so the value is loaded (by performing filesystem
operations) every single time a virDevice is created, whether
the value is actually used or not.

Cheers.


[1] Rather virPCIDeviceDetach() in reality - I have a pending
patch that moves it to virPCIDeviceBindToStub() where it
IMHO belongs
-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

[libvirt] [PATCH v3 1/9] Implement virPCIIsKnownStub function

[Shivaprasad G Bhat]

The checks to knwon stubs can be easily done having this
implementation.

Signed-off-by: Shivaprasad G Bhat 
---
 src/util/virpci.c |   28 ++--
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 35b1459..bff37d7 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1080,6 +1080,23 @@ static const char *virPCIKnownStubs[] = {
 NULL
 };
 
+static bool
+virPCIIsKnownStub(char *driver)
+{
+const char **stubTest;
+bool ret = false;
+
+for (stubTest = virPCIKnownStubs; *stubTest != NULL; stubTest++) {
+if (STREQ_NULLABLE(driver, *stubTest)) {
+ret = true;
+VIR_DEBUG("Found stub driver %s", *stubTest);
+break;
+}
+}
+
+return ret;
+}
+
 static int
 virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
 {
@@ -1087,8 +1104,6 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
 char *drvdir = NULL;
 char *path = NULL;
 char *driver = NULL;
-const char **stubTest;
-bool isStub = false;
 
 /* If the device is currently bound to one of the "well known"
  * stub drivers, then unbind it, otherwise ignore it.
@@ -1105,14 +1120,7 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
 goto remove_slot;
 
 /* If the device isn't bound to a known stub, skip the unbind. */
-for (stubTest = virPCIKnownStubs; *stubTest != NULL; stubTest++) {
-if (STREQ(driver, *stubTest)) {
-isStub = true;
-VIR_DEBUG("Found stub driver %s", *stubTest);
-break;
-}
-}
-if (!isStub)
+if (!virPCIIsKnownStub(driver))
 goto remove_slot;
 
 if (virPCIDeviceUnbind(dev, dev->reprobe) < 0)

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

[libvirt] [PATCH 8/9] Postpone reprobing till all the devices in iommu group are unbound from vfio

[Shivaprasad G Bhat]

The host will crash if a device is bound to host driver when the device
belonging to same iommu group is in use by any of the guests. So,
do the host driver probe only after all the devices in the iommu group
have unbound from the vfio. The patch also adds the test cases. The negative
test case is removed. We need to add a new one with a new driver.

The patch fixes
https://bugzilla.redhat.com/show_bug.cgi?id=1272300

Signed-off-by: Shivaprasad G Bhat 
---
 src/util/virpci.c  |  141 ++
 tests/virpcimock.c |  173 +++-
 tests/virpcitest.c |  152 ++
 3 files changed, 410 insertions(+), 56 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 0c20053..caa2458 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1129,6 +1129,24 @@ virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev,
 }
 
 static int
+virPCIDeviceBoundToVFIODriver(virPCIDeviceAddressPtr devAddr, void *opaque 
ATTRIBUTE_UNUSED)
+{
+int ret = 0;
+virPCIDevicePtr pci = NULL;
+
+if (!(pci = virPCIDeviceNew(devAddr->domain, devAddr->bus,
+devAddr->slot, devAddr->function)))
+goto cleanup;
+
+if (STREQ_NULLABLE(pci->stubDriver, "vfio-pci"))
+ret = -1;
+
+ cleanup:
+virPCIDeviceFree(pci);
+return ret;
+}
+
+static int
 virPCIDeviceUnbindFromStub(virPCIDevicePtr dev,
virPCIDeviceListPtr activeDevs ATTRIBUTE_UNUSED,
virPCIDeviceListPtr inactiveDevs)
@@ -1145,7 +1163,12 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev,
 goto cleanup;
 
 if (!driver) {
-/* The device is not bound to any driver and we are almost done. */
+/* This device was probably unbound before and libvirt restared.
+ * Add to the inactive list if not already there so that we
+ * reprobe it if needed.
+ */
+if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) &&
+virPCIDeviceListAddCopy(inactiveDevs, dev) < 0)
 goto reprobe;
 }
 
@@ -1156,10 +1179,69 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev,
 if (!virPCIIsKnownStub(driver))
 goto remove_slot;
 
-if (virPCIDeviceUnbind(dev, dev->reprobe) < 0)
-goto cleanup;
-dev->unbind_from_stub = false;
+if (STREQ_NULLABLE(dev->stubDriver, "vfio-pci")) {
+size_t i = 0;
+bool inInactiveList = false;
+virPCIDevicePtr temp;
+while (activeDevs && (i < virPCIDeviceListCount(activeDevs))) {
+virPCIDevicePtr pcidev = virPCIDeviceListGet(activeDevs, i);
+if (dev->iommuGroup == pcidev->iommuGroup) {
+/* If Part of activelist, deal with current device later */
+if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) &&
+virPCIDeviceListAddCopy(inactiveDevs, dev) < 0)
+goto cleanup;
+result = 0;
+goto revisit;
+}
+i++;
+}
+/* No guests are using any of the devices in the iommu.
+ * This is the first device after guest shutdown/unplug
+ * of all devices. So, rest of the devices are in inactiveDevs.
+ * OR This is the first device after libvirt start and nothing is in
+ * inactiveDevs.
+ * If the device is in inactiveList(User can issue reattach multiple 
times
+ * for the same device), mark it as already unbound.
+ */
+if (virPCIDeviceUnbind(dev, dev->reprobe) < 0)
+goto cleanup;
+dev->unbind_from_stub = false;
+if (inactiveDevs && (temp = virPCIDeviceListFind(inactiveDevs, dev))) {
+temp->unbind_from_stub = false;
+inInactiveList = true;
+}
 
+/* Unbind rest of the devices in the same iommu group.
+ * After the fresh libvirt start, nothing is detached in the step
+ * as inactiveDevs is empty.*/
+i = 0;
+while (inactiveDevs && (i < virPCIDeviceListCount(inactiveDevs))) {
+virPCIDevicePtr pcidev = virPCIDeviceListGet(inactiveDevs, i);
+if (dev->iommuGroup == pcidev->iommuGroup) {
+if (pcidev->unbind_from_stub &&
+virPCIDeviceUnbind(pcidev, pcidev->reprobe) < 0) {
+goto cleanup;
+}
+pcidev->unbind_from_stub = false;
+}
+i++;
+}
+/* Libvirt just restarted and inactive list is empty or yet to get into
+ * the list. But no devices used actively from same iommu group.
+ * Basically this could be the first one to unbind from vfio but
+ * could be the last function to be bound to vfio if this is after
+ * libvirt restart.
+ */
+if (!inInactiveList) {
+if (inactiveDevs 

[libvirt] [PATCH 9/9] Wait for iommmu device to go away before reprobing the host driver

[Shivaprasad G Bhat]

There could be a delay of 1 or 2 seconds before the vfio-pci driver
is unbound and the device file /dev/vfio/ is actually
removed. If the file exists, the host driver probing the device
can lead to crash. So, wait and avoid the crash.

Signed-off-by: Shivaprasad G Bhat 
---
 src/util/virpci.c  |   41 +
 tests/virpcimock.c |   14 --
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index caa2458..157327f 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1098,6 +1098,43 @@ virPCIIsKnownStub(char *driver)
 return ret;
 }
 
+#define VFIO_UNBIND_TIMEOUT 100
+
+/* It is not safe to initiate host driver probe if the vfio driver has not
+ * completely unbound the device. Usual wait time is 1 to 2 seconds.
+ * So, return if the unbind didn't complete in 10 seconds.
+ */
+static int
+virPCIWaitForVFIOUnbindCompletion(virPCIDevicePtr dev)
+{
+int retry = 0;
+int ret = -1;
+char *path = NULL;
+
+if (!(path = virPCIDeviceGetIOMMUGroupDev(dev)))
+goto cleanup;
+
+while (retry++ < VFIO_UNBIND_TIMEOUT) {
+if (!virFileExists(path))
+break;
+ usleep(10); /* Sleep for 1/10th of a second */
+}
+
+if (virFileExists(path)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("VFIO unbind not completed even after %d seconds"
+ " for device %s"), retry, dev->name);
+goto cleanup;
+}
+
+ret = 0;
+ cleanup:
+VIR_FREE(path);
+return ret;
+
+}
+
+
 static int
 virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev,
   const char *driver,
@@ -1275,6 +1312,10 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev,
 /* This device is the last to unbind from vfio. As we explicitly
  * add a missing device in the list to inactiveList, we will just
  * go through the list. */
+
+if (virPCIWaitForVFIOUnbindCompletion(dev) < 0)
+goto cleanup;
+
 while (inactiveDevs && (i < virPCIDeviceListCount(inactiveDevs))) {
 virPCIDevicePtr pcidev = virPCIDeviceListGet(inactiveDevs, i);
 if (dev->iommuGroup == pcidev->iommuGroup) {
diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 926a548..9ee46d9 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -52,6 +52,7 @@ static DIR * (*realopendir)(const char *name);
 char *fakesysfsdir;
 
 # define PCI_SYSFS_PREFIX "/sys/bus/pci/"
+# define ROOT_DEV_PREFIX "/dev/"
 
 # define STDERR(...)\
 fprintf(stderr, "%s %zu: ", __FUNCTION__, (size_t) __LINE__);   \
@@ -248,6 +249,13 @@ getrealpath(char **newpath,
 errno = ENOMEM;
 return -1;
 }
+} else if (STRPREFIX(path, ROOT_DEV_PREFIX)) {
+if (virAsprintfQuiet(newpath, "%s/%s",
+ fakesysfsdir,
+ path) < 0) {
+errno = ENOMEM;
+return -1;
+}
 } else {
 if (VIR_STRDUP_QUIET(*newpath, path) < 0)
 return -1;
@@ -995,7 +1003,8 @@ access(const char *path, int mode)
 
 init_syms();
 
-if (STRPREFIX(path, PCI_SYSFS_PREFIX)) {
+if (STRPREFIX(path, PCI_SYSFS_PREFIX) ||
+STRPREFIX(path, ROOT_DEV_PREFIX)) {
 char *newpath;
 if (getrealpath(, path) < 0)
 return -1;
@@ -1014,7 +1023,8 @@ __lxstat(int ver, const char *path, struct stat *sb)
 
 init_syms();
 
-if (STRPREFIX(path, PCI_SYSFS_PREFIX)) {
+if (STRPREFIX(path, PCI_SYSFS_PREFIX) ||
+STRPREFIX(path, ROOT_DEV_PREFIX)) {
 char *newpath;
 if (getrealpath(, path) < 0)
 return -1;

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

Re: [libvirt] [libvirt-glib 3/3] gobject: Port to GTask API

[Christophe Fergeau]

On Thu, Oct 29, 2015 at 08:39:08PM +, Zeeshan Ali (Khattak) wrote:
> Drop usage of deprecated GSimpleAsyncResult API.
> ---
>  libvirt-gobject/libvirt-gobject-domain.c| 270 ++-
>  libvirt-gobject/libvirt-gobject-input-stream.c  |  76 +++
>  libvirt-gobject/libvirt-gobject-output-stream.c |  75 +++
>  libvirt-gobject/libvirt-gobject-storage-pool.c  | 281 
> ++--
>  libvirt-gobject/libvirt-gobject-stream.c|  46 ++--
>  5 files changed, 326 insertions(+), 422 deletions(-)
> 
> diff --git a/libvirt-gobject/libvirt-gobject-domain.c 
> b/libvirt-gobject/libvirt-gobject-domain.c
> index 34eb7ca..dda9268 100644
> --- a/libvirt-gobject/libvirt-gobject-domain.c
> +++ b/libvirt-gobject/libvirt-gobject-domain.c
> @@ -380,18 +380,19 @@ static void domain_start_data_free(DomainStartData 
> *data)
>  }
>  
>  static void
> -gvir_domain_start_helper(GSimpleAsyncResult *res,
> - GObject *object,
> +gvir_domain_start_helper(GTask *task,
> + gpointer source_object,
> + gpointer task_data,
>   GCancellable *cancellable G_GNUC_UNUSED)
>  {
> -GVirDomain *dom = GVIR_DOMAIN(object);
> -DomainStartData *data;
> +GVirDomain *dom = GVIR_DOMAIN(source_object);
> +DomainStartData *data = (DomainStartData *) task_data;
>  GError *err = NULL;
>  
> -data = g_simple_async_result_get_op_res_gpointer(res);
> -
>  if (!gvir_domain_start(dom, data->flags, ))
> -g_simple_async_result_take_error(res, err);
> +g_task_return_error(task, err);
> +else
> +g_task_return_boolean(task, TRUE);
>  }
>  
>  /**
> @@ -410,7 +411,7 @@ void gvir_domain_start_async(GVirDomain *dom,
>   GAsyncReadyCallback callback,
>   gpointer user_data)
>  {
> -GSimpleAsyncResult *res;
> +GTask *task;
>  DomainStartData *data;
>  
>  g_return_if_fail(GVIR_IS_DOMAIN(dom));
> @@ -419,16 +420,13 @@ void gvir_domain_start_async(GVirDomain *dom,
>  data = g_slice_new0(DomainStartData);
>  data->flags = flags;
>  
> -res = g_simple_async_result_new(G_OBJECT(dom),
> -callback,
> -user_data,
> -gvir_domain_start_async);
> -g_simple_async_result_set_op_res_gpointer (res, data, 
> (GDestroyNotify)domain_start_data_free);
> -g_simple_async_result_run_in_thread(res,
> -gvir_domain_start_helper,
> -G_PRIORITY_DEFAULT,
> -cancellable);
> -g_object_unref(res);
> +task = g_task_new(G_OBJECT(dom),
> +  cancellable,
> +  callback,
> +  user_data);
> +g_task_set_task_data(task, data, (GDestroyNotify)domain_start_data_free);
> +g_task_run_in_thread(task, gvir_domain_start_helper);
> +g_object_unref(task);
>  }
>  
>  gboolean gvir_domain_start_finish(GVirDomain *dom,
> @@ -436,13 +434,10 @@ gboolean gvir_domain_start_finish(GVirDomain *dom,
>GError **err)
>  {
>  g_return_val_if_fail(GVIR_IS_DOMAIN(dom), FALSE);
> -g_return_val_if_fail(g_simple_async_result_is_valid(result, 
> G_OBJECT(dom), gvir_domain_start_async), FALSE);
> +g_return_val_if_fail(g_task_is_valid(result, dom), FALSE);
>  g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
>  
> -if (g_simple_async_result_propagate_error(G_SIMPLE_ASYNC_RESULT(result), 
> err))
> -return FALSE;
> -
> -return TRUE;
> +return g_task_propagate_boolean(G_TASK(result), err);
>  }
>  
>  /**
> @@ -472,15 +467,18 @@ gboolean gvir_domain_resume(GVirDomain *dom,
>  }
>  
>  static void
> -gvir_domain_resume_helper(GSimpleAsyncResult *res,
> -  GObject *object,
> +gvir_domain_resume_helper(GTask *task,
> +  gpointer source_object,
> +  gpointer task_data G_GNUC_UNUSED,
>GCancellable *cancellable G_GNUC_UNUSED)
>  {
> -GVirDomain *dom = GVIR_DOMAIN(object);
> +GVirDomain *dom = GVIR_DOMAIN(source_object);
>  GError *err = NULL;
>  
>  if (!gvir_domain_resume(dom, ))
> -g_simple_async_result_take_error(res, err);
> +g_task_return_error(task, err);
> +else
> +g_task_return_boolean(task, TRUE);
>  }
>  
>  /**
> @@ -497,20 +495,17 @@ void gvir_domain_resume_async(GVirDomain *dom,
>GAsyncReadyCallback callback,
>gpointer user_data)
>  {
> -GSimpleAsyncResult *res;
> +GTask *task;
>  
>  g_return_if_fail(GVIR_IS_DOMAIN(dom));
>  g_return_if_fail((cancellable == NULL) || G_IS_CANCELLABLE(cancellable));
>  
> -res = 

Re: [libvirt] oVirt considers using macTableManager='libvirt'

[Laine Stump]

On 11/02/2015 08:22 AM, Vlad Yasevich wrote:

On 11/01/2015 01:05 PM, Laine Stump wrote:

On 11/01/2015 08:52 AM, Ido Barkan wrote:


> 6) are there any plans to support OVS for this feature in the future?

Yes, we have plans to support this for OVS.

-vlad


No concrete plans, but if someone wants to implement it, I'd be happy to 
assist/review :-)



These two statements might seem contradictory, but Vlad is of course 
speaking from kernel (and OVS?) POV, and I'm speaking from libvirt POV 
(actually, I'm ignorant enough of the capabilities of OVS that I assumed 
such capabilities already existed in OVS; is that not the case?)


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

Re: [libvirt] oVirt considers using macTableManager='libvirt'

[Vlad Yasevich]

On 11/02/2015 01:18 PM, Laine Stump wrote:
> On 11/02/2015 08:22 AM, Vlad Yasevich wrote:
>> On 11/01/2015 01:05 PM, Laine Stump wrote:
>>> On 11/01/2015 08:52 AM, Ido Barkan wrote:
>>>
>>>
>>> > 6) are there any plans to support OVS for this feature in the future?
>> Yes, we have plans to support this for OVS.
>>
>> -vlad
>>
>>> No concrete plans, but if someone wants to implement it, I'd be happy to 
>>> assist/review :-)
>>>
> 
> These two statements might seem contradictory, but Vlad is of course speaking 
> from kernel
> (and OVS?) POV, and I'm speaking from libvirt POV (actually, I'm ignorant 
> enough of the
> capabilities of OVS that I assumed such capabilities already existed in OVS; 
> is that not
> the case?)

No, OVS still needs similar support as bridge has.

-vlad

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

Re: [libvirt] [PATCH v6 05/13] virstoragefile: change backingStore to backingStores.

[Peter Krempa]

On Thu, Oct 29, 2015 at 14:43:12 +0100, Matthias Gatto wrote:
> The backingStore field was a virStorageSourcePtr.
> Because a quorum can contain more that one backingStore at the same level,
> it's now an array of 'virStorageSourcePtr'.
> 
> This patch rename  src->backingStore to src->backingStores,
> Made the necessary changes to virStorageSourceSetBackingStore
> and virStorageSourceGetBackingStore.
> virStorageSourceSetBackingStore can now expand the size of src->backingStores.
> 
> Signed-off-by: Matthias Gatto 
> ---
>  src/storage/storage_backend.c|  2 +-
>  src/storage/storage_backend_fs.c |  2 +-
>  src/util/virstoragefile.c| 48 
> +---
>  src/util/virstoragefile.h|  3 ++-
>  4 files changed, 44 insertions(+), 11 deletions(-)

[...]

> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index cb96c5b..44bce91 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1809,21 +1809,48 @@ virStorageSourcePoolDefCopy(const 
> virStorageSourcePoolDef *src)
>  }
>  
>  
> +/**
> + * virStorageSourceGetBackingStore:
> + * @src: virStorageSourcePtr containing the backing stores
> + * @pos: position of the backing store to get
> + *
> + * return the backingStore at the position of @pos
> + */
>  virStorageSourcePtr
> -virStorageSourceGetBackingStore(const virStorageSource *src,
> -size_t pos ATTRIBUTE_UNUSED)
> +virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos)
>  {
> -return src->backingStore;
> +if (!src || !src->backingStores || pos >= src->nBackingStores)
> +return NULL;

The first two condition statements should be in the patch that
introduced this function.

> +return src->backingStores[pos];
>  }
>  
>  
> +/**
> + * virStorageSourceSetBackingStore:
> + * @src: virStorageSourcePtr to hold @backingStore
> + * @backingStore: backingStore to store
> + * @pos: position of the backing store to store
> + *
> + * Set @backingStore at @pos in src->backingStores.
> + * If src->backingStores don't have the space to contain
> + * @backingStore, we expand src->backingStores
> + */
>  bool

As I've said earlier, libvirt's convention is to return -1 in case when
something failed.

>  virStorageSourceSetBackingStore(virStorageSourcePtr src,
>  virStorageSourcePtr backingStore,
> -size_t pos ATTRIBUTE_UNUSED)
> +size_t pos)
>  {
> -src->backingStore = backingStore;
> -return !!src->backingStore;
> +if (!src)
> +return false;
> +
> +if (pos >= src->nBackingStores) {
> +int nbr = pos - src->nBackingStores + 1;
> +if (VIR_EXPAND_N(src->backingStores, src->nBackingStores, nbr) < 0)
> +return false;
> +}
> +
> +src->backingStores[pos] = backingStore;

Shouldn't this free the backing store if it's being overwritten? Or
perhaps fail if it's already assigned?

> +return true;
>  }
>  
>  


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

Re: [libvirt] [PATCH 2/9] Initialize the stubDriver of pci devices if bound to a valid one

[Peter Krempa]

On Mon, Nov 02, 2015 at 03:19:13 +0530, Shivaprasad G Bhat wrote:
> The stubDriver name can be used to make useful decisions if readily available.
> Set it if bound to a valid one during initialisation.
> 
> Signed-off-by: Shivaprasad G Bhat 
> ---
>  src/util/virpci.c |9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index f3b4700..be1b6de 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1564,6 +1564,8 @@ virPCIDeviceNew(unsigned int domain,
>  virPCIDevicePtr dev;
>  char *vendor = NULL;
>  char *product = NULL;
> +char *drvpath = NULL;
> +char *driver = NULL;
>  
>  if (VIR_ALLOC(dev) < 0)
>  return NULL;
> @@ -1611,9 +1613,16 @@ virPCIDeviceNew(unsigned int domain,
>  goto error;
>  }
>  
> +if (virPCIDeviceGetDriverPathAndName(dev, , ) < 0)
> +goto cleanup;

This allocates 'driver'.

> +
> +if (virPCIIsAKnownStub(driver))

So if the driver is not 'stub'.

> +dev->stubDriver = driver;

This doesn't get set.

> +
>  VIR_DEBUG("%s %s: initialized", dev->id, dev->name);
>  
>   cleanup:
> +VIR_FREE(drvpath);

And here it will leak the driver.

>  VIR_FREE(product);
>  VIR_FREE(vendor);
>  return dev;

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 v6 11/11] quorum: Block snapshot operation with RAID

[Peter Krempa]

On Thu, Oct 29, 2015 at 14:43:19 +0100, Matthias Gatto wrote:
> For now we block all snapshot operations with quorum, because it would require
> a lot more code, espacially because Qemu doesn't really suport it.

Snapshots aren't the only thing that will not work. Basically every
other block operation needs to be either checked that it works or it has
to be forbidden too. Namely: blockRebase, blockPull, blockCopy (this
will probably work, if not used as a target), and possibly other I now
can't remember.

> 
> I guess, we can use node-name, and manually snapshoot all qcow from a
> virStorageSource and use this as a quorum's snapshot,
> but libvirt doesn't support node-name, and we don't need node-name
> anymore to use a quorum in qemu.

In libvirt we actually do want to start using node names. It's the only
way to do some more complex operations.

> 
> I actually have some patchs which could partially support quorum snapshot
> on my computer, but nothing suitable to be upstream, so I prefer to have
> a stable versions of quorum inside libvirt before dealling with
> snapshot.

This paragraph doesn't belong to a commit message.

> 
> Signed-off-by: Matthias Gatto 
> ---
>  src/qemu/qemu_driver.c | 6 ++
>  1 file changed, 6 insertions(+)

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 v2 1/9] Implement virPCIIsAKnownStub function

[Peter Krempa]

On Mon, Nov 02, 2015 at 03:18:20 +0530, Shivaprasad G Bhat wrote:
> The checks to known stubs can be easily done having this
> implementation and easy to reuse.
> 
> Signed-off-by: Shivaprasad G Bhat 
> ---
>  src/util/virpci.c |   28 ++--
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 35b1459..f3b4700 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1080,6 +1080,23 @@ static const char *virPCIKnownStubs[] = {
>  NULL
>  };
>  
> +static bool
> +virPCIIsAKnownStub(char *driver)

I'd drop the 'A' here, so the function would be called:
virPCIIsKnownStub.

> +{

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 v2 3/9] Add iommu group number info to virPCIDevice

[Peter Krempa]

On Mon, Nov 02, 2015 at 03:21:13 +0530, Shivaprasad G Bhat wrote:
> The iommu group number need not be fetched from the sysfs
> everytime as it remains constant. Fetch it once during
> allocation. Add a helper to set the value to set it from tests.
> 
> Signed-off-by: Shivaprasad G Bhat 
> ---
>  src/libvirt_private.syms |1 +
>  src/util/virpci.c|   11 +++
>  src/util/virpci.h|2 ++
>  3 files changed, 14 insertions(+)
> 

[...]

> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index be1b6de..f401e1d 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -75,6 +75,7 @@ struct _virPCIDevice {
>  bool  has_pm_reset;
>  bool  managed;
>  char  *stubDriver;
> +int  iommuGroup;

This is misaligned. It either should go with the alignment of the rest
of the fields or have exactly one space between the type and name.

>  
>  /* used by reattach function */
>  bool  unbind_from_stub;

[...]

> @@ -1723,6 +1728,12 @@ virPCIDeviceSetStubDriver(virPCIDevicePtr dev, const 
> char *driver)
>  return VIR_STRDUP(dev->stubDriver, driver);
>  }
>  
> +void
> +virPCIDeviceSetIommuGroup(virPCIDevicePtr dev, int iommu)

This accessor is used only in virpci.c. Is it really worth adding it?

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] syntax-check: Add sc_prohibit_space_in_label rule

[Peter Krempa]

On Mon, Nov 02, 2015 at 10:34:35 +0100, Andrea Bolognani wrote:
> This guards against code such as
> 
>  cleanup :
> 
> which is happily accepted by the compiler but does not conform
> to our style guidelines.
> ---
>  cfg.mk | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/cfg.mk b/cfg.mk
> index a9bba38..8462051 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -919,6 +919,12 @@ sc_require_space_before_label:
>   halt="Top-level labels should be indented by one space"\
> $(_sc_search_regexp)
>  
> +sc_prohibit_space_in_label:
> + @prohibit='^[_a-zA-Z0-9]+ +:$$'\

Our labels enforce at least one space before a label so I don't think
this will work. If you will be adding a multi-space match, please make
sure that "case" is not matched in that case.

Peter


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

[libvirt] Happy Birthday Libvirt

[Daniel Veillard]

Hello everybody,

  as the first commits were done on Nov 2 2005, libvirt is more or less
ten years old. This has been quite a bit of a journey, but a very successful
one in my opinion, the initial goal was to provide a long term stable API
for virtualization in the Red Hat ecosystem, and I'm very happy to see the
goal has been reached, and we went way beyond that. In the end I think
libvirt helped avoid the "balkanization" which is classic in highly
competitive domains where you end up with one vertical set of tools per
vendor. I also think that libvirt played a key role to keep free and open
source software in the main virtualization offerings and even more so for
cloud management systems.

  So it's time to celebrate, there is obviously lot to be done still on
libvirt (as the steady stream of patches show !), but I would like to thank
everybody who has contributed to the project so far, with a special mention
to Dan Berrange who has been leading the effort technically for most of that
time. I would also say that I have been very pelased with how the project
has been working in that decade, the community is technical but friendly,
and it's my hope libvirt, the project as well as the code, will remain as
successful for the next decade !

   So Thank You everybody, for making this project and community successful !

 Enjoy that very special day :-)

Daniel

-- 
Daniel Veillard  | Open Source and Standards, Red Hat
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [PATCH v2 9/9] Wait for iommmu device to go away before reprobing the host driver

[Peter Krempa]

On Mon, Nov 02, 2015 at 03:28:00 +0530, Shivaprasad G Bhat wrote:
> There could be a delay of 1 or 2 seconds before the vfio-pci driver
> is unbound and the device file /dev/vfio/ is actually
> removed. If the file exists, the host driver probing the device
> can lead to crash. So, wait and avoid the crash.
> 
> Signed-off-by: Shivaprasad G Bhat 
> ---
>  src/util/virpci.c  |   42 ++
>  tests/virpcimock.c |   14 --
>  2 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 0bb465b..68fd54c 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1098,6 +1098,43 @@ virPCIIsAKnownStub(char *driver)
>  return ret;
>  }
>  
> +#define VFIO_UNBIND_TIMEOUT 10
> +
> +/* It is not safe to initiate host driver probe if the vfio driver has not
> + * completely unbound the device. Usual wait time is 1 to 2 seconds.
> + * So, return if the unbind didn't complete in 10 seconds.
> + */
> +static int
> +virPCIWaitForVFIOUnbindCompletion(virPCIDevicePtr dev)
> +{
> +int retry = 0;
> +int ret = -1;
> +char *path = NULL;
> +
> +if (!(path = virPCIDeviceGetIOMMUGroupDev(dev)))
> +goto cleanup;
> +
> +while (retry++ < VFIO_UNBIND_TIMEOUT) {
> +if (!virFileExists(path))
> +break;
> + sleep(1);

I don't particularly like the 1 second sleep granularity here. I'd
rather see a 1/10s or 1/100s delay so that if the time will be decreased
in the future we will not have to wait for a full second.

Peter


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

Re: [libvirt] Ten years of libvirt

[Daniel P. Berrange]

On Mon, Nov 02, 2015 at 09:57:24AM +0100, Michal Privoznik wrote:
> Dear list,
> 
> join me on this big day in congratulations to libvirt that has just
> turned 10 and is starting new decade of its life. At the same time big
> thanks to all who contributed in any way.

Yes, this is a great milestone for the project. Here is a quick look at
what the last 10 years has achieved...

For the first month of existance the project was actually called libxen[1],
but on Dec 5[2], Daniel renamed it to libvir, and then further to libvirt
on Feb 9 2006[3].

We had our second contributor on Dec 7 2005[4], Karel Zak, also of Red Hat.
Our third contributor, Anthony Liguori, was also our first non-Red Hat
contributor on Jan 11 2006[5]. Since then it looks like[6] the core libvirt.git
repository has had a total of 515 different contributors from countless
different companies. We have about another 30 git repositories hosted on
libvirt.org with contributions from many more, not to mention an unknown
number of 3rd party projects...

The library only supported Xen for the first year of its existence, but
on Feb 14 2007[7] our initial QEMU driver was added. Fast forward to today
and we have many different drivers, Hyper-V, VMWare ESX, OpenVZ, LXC,
Parallels, Power-Hypervisor, QEMU, BHyve, Xen / libxl, User Mode Linux,
Xen API, VirtualBox, VMWare Desktop/GSX and our dummy test driver and
our remote RPC driver. This covers pretty much every important hypervisor
that exists today.

The python language bindings were the first non-C language we supported
in Dec 2005[8], but since then we gained many other bindings including
Perl, Java, Ruby, PHP, OCaml, as well as bindings to other object
models such as GObject, SNMP, and CIM.

We added support for translation in Sep 2006[9], and now have translation
files covering 63 different languages, largely thanks to the help of the
Fedora translation teams, first via Transifex and now via Zanata.

Aside from virsh which has been around since the start, the first 3rd
party application built with libvirt was xenguest-install which used
the libvirt APIs to provision Fedora/RHEL guests on Xen. This was initially
part of Fedora Xen RPMs, but in Aug 2006 it moved into its own project
becoming the virt-install tool[10] everyone knows. The first graphical
user interface, virt-manager, has been around since Mar 2006 [11]. Since
that since, libvirt has found use in a wide variety of places whether as
a plugin to tools like Vagrant and Nagios, are in large scale data center
management apps like oVirt or the cloud platform OpenStack.

It is not all about code development either. We have had many contributors
in other important areas of the project. Many people have made presentations
about the project at various open source conferences and user meetups
worldwide which help to spread knowledge & build interest & enthusiasm
around the project. We've had many contributors to our documentation too,
most recently creating a brand new python development guide[10], but also
work on our in-tree docs, and wiki pages. Countless users have written
blog postings about the project which have helped our community of users
to learn about the many neat features libvirt provides. When we started the
libvirt-users mailing list, we weren't sure how it would work out, but 5
years later it is great to see a sustained high level of activity on the
users mailing list, which a wide variety of people helping each other out.

You might think that after 10 years of development, we'd be slowing down,
but anyone who follows the development list and/or git history will see
we're developing as fast as ever. It seems we're no where near "done"

http://dilbert.com/strip/2008-02-12

A big thankyou to all who have contributed to making libvirt a successful
project, whether by deploying it, writing applications using it, writing
documentation, blog posting, providing end user support, developing new
features, translating the code, providing presentations / talks and much
more that I can't remember off hand...

I look forward to seeing what another 10 years of hard work will bring to
the project.

Regards,
Daniel

[1] Initially libxen

http://libvirt.org/git/?p=libvirt.git;a=commit;h=d77e1a9642fe1efe9aa5f737a640354c27d04e02

[2] Renamed libvir

http://libvirt.org/git/?p=libvirt.git;a=commit;h=1192a2ade33bccb54e68daaba60043be995a59e1

[3] Renamed libvirt

http://libvirt.org/git/?p=libvirt.git;a=commit;h=8c423e6c83406efd85257cab65cfa121f7997a1b

[4] Second contributor

http://libvirt.org/git/?p=libvirt.git;a=commit;h=d5f9611f4a53ad68bfb88c02384ef40fb9454775

[5] First non-Red Hat contributor

http://libvirt.org/git/?p=libvirt.git;a=commit;h=f18b6c0ec68f150e94eec60a1f2f980ae5865600

[6] Counting total contributors is an inexact science, since when we used
CVS instead of GIT, attribution was not tracked very well.

[7] QEMU driver

[libvirt] Ten years of libvirt

[Michal Privoznik]

Dear list,

join me on this big day in congratulations to libvirt that has just
turned 10 and is starting new decade of its life. At the same time big
thanks to all who contributed in any way.

Cheers!

Michal

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

Re: [libvirt] [PATCH v2 6/9] Split reprobe action from the virPCIUnbindFromStub into a new function

[Peter Krempa]

On Mon, Nov 02, 2015 at 03:23:26 +0530, Shivaprasad G Bhat wrote:
> The reprobe needs to be called multiple times for vfio devices for each
> device in the iommu group in future patch. So split the reprobe into a
> new function. No functional change.
> 
> Signed-off-by: Shivaprasad G Bhat 
> ---
>  src/util/virpci.c |   44 
>  1 file changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index f401e1d..77ae455 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1098,6 +1098,33 @@ virPCIIsAKnownStub(char *driver)
>  return ret;
>  }
>  
> +static int virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev, char *driver, 
> char *drvdir)

Please format the header the same way as other headers are formatted in
this file.

> +{
> +char *path = NULL;
> +int result = -1;

We usually use 'ret' for this kind of variable.

Peter


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

[libvirt] [PATCH] syntax-check: Add sc_prohibit_space_in_label rule

[Andrea Bolognani]

This guards against code such as

 cleanup :

which is happily accepted by the compiler but does not conform
to our style guidelines.
---
 cfg.mk | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/cfg.mk b/cfg.mk
index a9bba38..8462051 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -919,6 +919,12 @@ sc_require_space_before_label:
halt="Top-level labels should be indented by one space"\
  $(_sc_search_regexp)
 
+sc_prohibit_space_in_label:
+   @prohibit='^[_a-zA-Z0-9]+ +:$$'\
+   in_vc_files='\.[ch]$$' \
+   halt="There should be no space between label name and colon"   \
+ $(_sc_search_regexp)
+
 # Doesn't catch all cases of mismatched braces across if-else, but it helps
 sc_require_if_else_matching_braces:
@prohibit='(  else( if .*\))? {|} else( if .*\))?$$)'   \
-- 
2.4.3

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

[libvirt] [PATCH v2] syntax-check: Add prohibit_space_in_label rule

[Andrea Bolognani]

This guards against code such as

 cleanup :

which is happily accepted by the compiler but does not conform
to our style guidelines.
---
 cfg.mk | 9 +
 1 file changed, 9 insertions(+)

diff --git a/cfg.mk b/cfg.mk
index a9bba38..7a5a1de 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -919,6 +919,15 @@ sc_require_space_before_label:
halt="Top-level labels should be indented by one space"\
  $(_sc_search_regexp)
 
+# Allow for up to three spaces before the label: this is to avoid running
+# into situations where require_space_before_label is not applied, eg. a
+# line matching ^[a-zA-Z0-9]+ :$
+sc_prohibit_space_in_label:
+   @prohibit='^ ? ? ?[_a-zA-Z0-9]+ +:$$'  \
+   in_vc_files='\.[ch]$$' \
+   halt="There should be no space between label name and colon"   \
+ $(_sc_search_regexp)
+
 # Doesn't catch all cases of mismatched braces across if-else, but it helps
 sc_require_if_else_matching_braces:
@prohibit='(  else( if .*\))? {|} else( if .*\))?$$)'   \
-- 
2.4.3

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

[libvirt] [PATCH 1/4] General structures and interfaces were added

[rodinasophie]

From: sonya 

---
 tools/Makefile.am   |   1 +
 tools/virsh-domain.c| 195 
 tools/virsh-edit.c  |  97 +---
 tools/virsh-interface.c |  66 
 tools/virsh-network.c   |  66 
 tools/virsh-nwfilter.c  |  67 +
 tools/virsh-pool.c  |  66 
 tools/virsh-snapshot.c  |  78 ++-
 8 files changed, 437 insertions(+), 199 deletions(-)

diff --git a/tools/Makefile.am b/tools/Makefile.am
index d5638d9..88bb1b2 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -205,6 +205,7 @@ virsh_SOURCES = 
\
virsh-domain.c virsh-domain.h   \
virsh-domain-monitor.c virsh-domain-monitor.h   \
virsh-host.c virsh-host.h   \
+   virsh-edit.c virsh-edit.h   \
virsh-interface.c virsh-interface.h \
virsh-network.c virsh-network.h \
virsh-nodedev.c virsh-nodedev.h \
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 12e85e3..648b086 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -56,6 +56,7 @@
 #include "virtime.h"
 #include "virtypedparam.h"
 #include "virxml.h"
+#include "virsh-edit.h"
 
 /* Gnulib doesn't guarantee SA_SIGINFO support.  */
 #ifndef SA_SIGINFO
@@ -4622,6 +4623,45 @@ static const vshCmdOptDef opts_save_image_edit[] = {
 {.name = NULL}
 };
 
+typedef struct editDomainUniversalStruct
+{
+   vshControl *ctl;
+   virshControlPtr priv;
+   const char *file;
+   virDomainPtr dom;
+   unsigned int getxml_flags;
+   bool *ret;
+   unsigned int *define_flags;
+   const char *key;
+   virDomainPtr *dom_edited;
+} editDomainUniversalStruct;
+
+static char*
+editDomainSaveImageGetXML(void *args)
+{
+   editDomainUniversalStruct *str = (editDomainUniversalStruct*)args;
+   return virDomainSaveImageGetXMLDesc(str->priv->conn, str->file, 
str->getxml_flags);
+}
+
+static void
+editDomainSaveImageNotChanged(void *args)
+{
+   editDomainUniversalStruct *str = (editDomainUniversalStruct*)args;
+  vshPrint(str->ctl, _("Saved image %s XML configuration not changed.\n"), 
str->file);
+   *(str->ret) = true;
+}
+
+static bool
+editDomainSaveImageDefine(void *args, char *doc_edited, char *doc)
+{
+   if (!doc)
+   return false;
+   editDomainUniversalStruct *str = (editDomainUniversalStruct*)args;
+   return
+   (virDomainSaveImageDefineXML(str->priv->conn, str->file,
+   
 doc_edited, 
*(str->define_flags)) == 0);
+}
+
 static bool
 cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd)
 {
@@ -4647,21 +4687,24 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd)
 
 if (vshCommandOptStringReq(ctl, cmd, "file", ) < 0)
 return false;
-
-#define EDIT_GET_XML \
-virDomainSaveImageGetXMLDesc(priv->conn, file, getxml_flags)
-#define EDIT_NOT_CHANGED\
-do {\
-vshPrint(ctl, _("Saved image %s XML configuration " \
-"not changed.\n"), file);   \
-ret = true; \
-goto edit_cleanup;  \
-} while (0)
-#define EDIT_DEFINE \
-(virDomainSaveImageDefineXML(priv->conn, file, doc_edited, define_flags) 
== 0)
-#include "virsh-edit.c"
-
-vshPrint(ctl, _("State file %s edited.\n"), file);
+   {
+   editDomainUniversalStruct editDomainArgs = {
+   NULL, priv, file, NULL, getxml_flags,
+   , _flags, NULL, NULL
+   };
+
+   vshEditControl editCtl = {
+   ,
+   ,
+   ,
+   NULL,
+   ,
+   };
+
+   if (!vshEdit(ctl, ))
+   goto cleanup;
+   }
+   vshPrint(ctl, _("State file %s edited.\n"), file);
 ret = true;
 
  cleanup:
@@ -8145,6 +8188,30 @@ virshDomainGetEditMetadata(vshControl *ctl,
 return ret;
 }
 
+static char*
+editDomainMetadataGetXML(void *args)
+{
+   editDomainUniversalStruct *str = (editDomainUniversalStruct*)(args);
+  return virshDomainGetEditMetadata(str->ctl, str->dom, str->file, 
str->getxml_flags);
+}
+
+static void
+editMetadataNotChanged(void *args)
+{
+   editDomainUniversalStruct *str = (editDomainUniversalStruct*)args;
+  

[libvirt] [PATCH 3/4] Comment in virsh-edit.h was changed

[rodinasophie]

From: sonya 

---
 tools/virsh-edit.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virsh-edit.h b/tools/virsh-edit.h
index 55af613..d8ab7ff 100644
--- a/tools/virsh-edit.h
+++ b/tools/virsh-edit.h
@@ -1,5 +1,5 @@
 /*
- * virsh-edit.h: Implementation of generic virsh *-edit intelligence
+ * virsh-edit.h: Definition of generic virsh *-edit interface
  *
  * Copyright (C) 2012, 2015 Red Hat, Inc.
  *
-- 
2.1.4

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

[libvirt] [PATCH 4/4] Style-errors are fixed

[rodinasophie]

From: sonya 

---
 tools/virsh-domain.c| 182 +++-
 tools/virsh-edit.c  |  23 +++---
 tools/virsh-edit.h  |  12 ++--
 tools/virsh-interface.c |  64 +
 tools/virsh-network.c   |  64 +
 tools/virsh-nwfilter.c  |  63 +
 tools/virsh-pool.c  |  64 +
 tools/virsh-snapshot.c  |  74 ++--
 8 files changed, 266 insertions(+), 280 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 648b086..364cffe 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -4625,41 +4625,40 @@ static const vshCmdOptDef opts_save_image_edit[] = {
 
 typedef struct editDomainUniversalStruct
 {
-   vshControl *ctl;
-   virshControlPtr priv;
-   const char *file;
-   virDomainPtr dom;
-   unsigned int getxml_flags;
-   bool *ret;
-   unsigned int *define_flags;
-   const char *key;
-   virDomainPtr *dom_edited;
+vshControl *ctl;
+virshControlPtr priv;
+const char *file;
+virDomainPtr dom;
+unsigned int getxml_flags;
+bool *ret;
+unsigned int *define_flags;
+const char *key;
+virDomainPtr *dom_edited;
 } editDomainUniversalStruct;
 
 static char*
 editDomainSaveImageGetXML(void *args)
 {
-   editDomainUniversalStruct *str = (editDomainUniversalStruct*)args;
-   return virDomainSaveImageGetXMLDesc(str->priv->conn, str->file, 
str->getxml_flags);
+editDomainUniversalStruct *str = (editDomainUniversalStruct*)args;
+return virDomainSaveImageGetXMLDesc(str->priv->conn, str->file, 
str->getxml_flags);
 }
 
 static void
 editDomainSaveImageNotChanged(void *args)
 {
-   editDomainUniversalStruct *str = (editDomainUniversalStruct*)args;
-  vshPrint(str->ctl, _("Saved image %s XML configuration not changed.\n"), 
str->file);
-   *(str->ret) = true;
+editDomainUniversalStruct *str = (editDomainUniversalStruct*)args;
+vshPrint(str->ctl, _("Saved image %s XML configuration not changed.\n"), 
str->file);
+*(str->ret) = true;
 }
 
 static bool
 editDomainSaveImageDefine(void *args, char *doc_edited, char *doc)
 {
-   if (!doc)
-   return false;
-   editDomainUniversalStruct *str = (editDomainUniversalStruct*)args;
-   return
-   (virDomainSaveImageDefineXML(str->priv->conn, str->file,
-   
 doc_edited, 
*(str->define_flags)) == 0);
+if (!doc)
+return false;
+editDomainUniversalStruct *str = (editDomainUniversalStruct*)args;
+return (virDomainSaveImageDefineXML(str->priv->conn, str->file,
+doc_edited, *(str->define_flags)) == 0);
 }
 
 static bool
@@ -4687,24 +4686,23 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd)
 
 if (vshCommandOptStringReq(ctl, cmd, "file", ) < 0)
 return false;
-   {
-   editDomainUniversalStruct editDomainArgs = {
-   NULL, priv, file, NULL, getxml_flags,
-   , _flags, NULL, NULL
-   };
-
-   vshEditControl editCtl = {
-   ,
-   ,
-   ,
-   NULL,
-   ,
-   };
-
-   if (!vshEdit(ctl, ))
-   goto cleanup;
-   }
-   vshPrint(ctl, _("State file %s edited.\n"), file);
+{
+editDomainUniversalStruct editDomainArgs = {
+NULL, priv, file, NULL, getxml_flags,
+, _flags, NULL, NULL
+};
+vshEditControl editCtl = {
+,
+,
+,
+NULL,
+,
+};
+
+if (!vshEdit(ctl, ))
+goto cleanup;
+}
+vshPrint(ctl, _("State file %s edited.\n"), file);
 ret = true;
 
  cleanup:
@@ -8191,26 +8189,26 @@ virshDomainGetEditMetadata(vshControl *ctl,
 static char*
 editDomainMetadataGetXML(void *args)
 {
-   editDomainUniversalStruct *str = (editDomainUniversalStruct*)(args);
-  return virshDomainGetEditMetadata(str->ctl, str->dom, str->file, 
str->getxml_flags);
+editDomainUniversalStruct *str = (editDomainUniversalStruct*)(args);
+return virshDomainGetEditMetadata(str->ctl, str->dom, str->file, 
str->getxml_flags);
 }
 
 static void
 editMetadataNotChanged(void *args)
 {
-   editDomainUniversalStruct *str = (editDomainUniversalStruct*)args;
-  vshPrint(str->ctl, "%s", _("Metadata not changed"));
-   *(str->ret) = true;
+editDomainUniversalStruct *str = (editDomainUniversalStruct*)args;
+vshPrint(str->ctl, "%s", _("Metadata not changed"));
+*(str->ret) = true;
 }
 
 static bool
 editMetadataDefine(void *args, char *doc_edited, 

[libvirt] [PATCH 0/4] *** Don't include .c file for editing support ***

[rodinasophie]

From: sonya 

*** BLURB HERE ***

sonya (4):
  General structures and interfaces were added
  Virsh-edit interface was added
  Comment in virsh-edit.h was changed
  Style-errors are fixed

 tools/Makefile.am   |   1 +
 tools/virsh-domain.c| 187 
 tools/virsh-edit.c  | 100 ++
 tools/virsh-edit.h  |  42 +++
 tools/virsh-interface.c |  64 +
 tools/virsh-network.c   |  62 
 tools/virsh-nwfilter.c  |  64 +
 tools/virsh-pool.c  |  64 +
 tools/virsh-snapshot.c  |  76 ++--
 9 files changed, 464 insertions(+), 196 deletions(-)
 create mode 100644 tools/virsh-edit.h

-- 
2.1.4

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

[libvirt] [PATCH 2/4] Virsh-edit interface was added

[rodinasophie]

From: sonya 

---
 tools/virsh-edit.h | 44 
 1 file changed, 44 insertions(+)
 create mode 100644 tools/virsh-edit.h

diff --git a/tools/virsh-edit.h b/tools/virsh-edit.h
new file mode 100644
index 000..55af613
--- /dev/null
+++ b/tools/virsh-edit.h
@@ -0,0 +1,44 @@
+/*
+ * virsh-edit.h: Implementation of generic virsh *-edit intelligence
+ *
+ * Copyright (C) 2012, 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ */
+
+#ifndef __VIRSH_EDIT_H_
+#define __VIRSH_EDIT_H_
+
+#include 
+#include "internal.h"
+#include "vsh.h"
+
+typedef char *(editGetXMLCallback)(void *args);
+typedef void (editNotChangedCallback)(void *args);
+typedef bool (editDefineCallback)(void *args, char *doc_edited, char *doc);
+typedef void (editRelaxCallback)(void *args);
+
+typedef struct vshEditControl {
+   editGetXMLCallback *editGetXML;
+   editNotChangedCallback *editNotChanged;
+   editDefineCallback *editDefine;
+   editRelaxCallback *editRelax;
+   void *editArgs;
+} vshEditControl;
+
+bool vshEdit(vshControl *ctl, vshEditControl *editCtl);
+
+#endif // __VIRSH_EDIT_H_
-- 
2.1.4

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

Re: [libvirt] [PATCH] syntax-check: Add sc_prohibit_space_in_label rule

[Andrea Bolognani]

On Mon, 2015-11-02 at 10:49 +0100, Peter Krempa wrote:
> On Mon, Nov 02, 2015 at 10:34:35 +0100, Andrea Bolognani wrote:
> > This guards against code such as
> > 
> >  cleanup :
> > 
> > which is happily accepted by the compiler but does not conform
> > to our style guidelines.
> > ---
> >  cfg.mk | 6 ++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/cfg.mk b/cfg.mk
> > index a9bba38..8462051 100644
> > --- a/cfg.mk
> > +++ b/cfg.mk
> > @@ -919,6 +919,12 @@ sc_require_space_before_label:
> > halt="Top-level labels should be indented by one space"\
> >   $(_sc_search_regexp)
> >  
> > +sc_prohibit_space_in_label:
> > +   @prohibit='^[_a-zA-Z0-9]+ +:$$'\
> 
> Our labels enforce at least one space before a label so I don't think
> this will work.

You are of course absolutely right. I was trying to catch the case
where two mistakes are present in the same line, eg.

myfancylabel :

but my implementation was flawed. v2 on its way.

> If you will be adding a multi-space match, please make
> sure that "case" is not matched in that case.

That shouldn't happen as there will always be some other token
between the whitespace after 'case' and the colon, right?

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 v6 08/13] domain_conf: Read and Write quorum config

[Peter Krempa]

On Thu, Oct 29, 2015 at 14:43:15 +0100, Matthias Gatto wrote:
> Add the capabiltty to libvirt to parse and format the quorum syntax
> as described here:
> http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html
> 
> As explain Peter Krempa in the V5, we need a different index for each child to
> manipulate individually each child of a quorum,
> this tack is done in this patch.
> 
> Sadly this versions is a little buggy: if one day we allow a quorum child
> to have a backing store, we would be unable to made a difference
> between a quorum child and a backing store, worst than that,
> if we have a quorum, with a quorum as a child, we have no way to
> use the index for quorum child manipulation.

Erm, this can happen already today if you use qcow as backing for the
quorum, so we'll need to take care of it right away. Additionally in
such case the code needs to be able to traverse the backing chain for
every single image backing the quorum so that they can be labelled later
on.

The subsequent backing store elements NEED to be uniquely numbered/named
in any case.

> 
> For now, this serie of patch forbid all actions which need
> to use indexes with quorum.

It actually doesn't forbid all of them.

> Therefore even if the index manipulation is buggy, this should not be a 
> problem
> because the buggy code should never be call.

I'm afraid that by not doing this properly right away this will just
lead to more and more problems.

> 
> Signed-off-by: Matthias Gatto 
> ---
>  src/conf/domain_conf.c | 178 
> ++---
>  1 file changed, 126 insertions(+), 52 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ce8edef..363ef5d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6607,20 +6607,56 @@ virDomainDiskSourceParse(xmlNodePtr node,
>  }
>  
>  
> +static bool

This reports libvirt error so you should return -1/0 instead of bool.

> +virDomainDiskThresholdParse(virStorageSourcePtr src,
> +xmlNodePtr node)
> +{
> +char *threshold = virXMLPropString(node, "threshold");

I'd suggest you use virXPathString.

> +int ret;
> +
> +if (!threshold) {
> +virReportError(VIR_ERR_XML_ERROR,
> +   "%s", _("missing threshold in quorum"));
> +return false;
> +}
> +ret = virStrToLong_ul(threshold, NULL, 10, >threshold);
> +if (ret < 0 || src->threshold < 2) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("unexpected threshold %s"),
> +   "threshold must be a decimal number superior to 2 "

greater than

> +   "and inferior to the number of children");

less than

> +VIR_FREE(threshold);
> +return false;
> +}
> +VIR_FREE(threshold);
> +return true;
> +}
> +
> +
>  static int
>  virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
> -   virStorageSourcePtr src)
> +   virStorageSourcePtr src,
> +   xmlNodePtr node,
> +   size_t pos)
>  {
>  virStorageSourcePtr backingStore = NULL;
>  xmlNodePtr save_ctxt = ctxt->node;
> -xmlNodePtr source;
> +xmlNodePtr source = NULL;
>  char *type = NULL;
>  char *format = NULL;
>  int ret = -1;
>  
> -if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) {
> -ret = 0;
> -goto cleanup;
> +if (virStorageSourceIsRAID(src)) {
> +if (!node) {
> +ret = 0;
> +goto cleanup;
> +}
> +ctxt->node = node;
> +} else {
> +if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) {
> +ret = 0;
> +goto cleanup;

This looks weird. Why are there two cases? The function should really
parse anything regardless whether it's a quorum backing node or not.

The function might need to be turned around so that it actually returns
the parsed backing store rather than directly setting it though.

> +}
>  }
>  
>  if (VIR_ALLOC(backingStore) < 0)
> @@ -6652,17 +6688,36 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr 
> ctxt,
>  goto cleanup;
>  }
>  
> -if (!(source = virXPathNode("./source", ctxt))) {
> -virReportError(VIR_ERR_XML_ERROR, "%s",
> -   _("missing disk backing store source"));
> -goto cleanup;
> -}
> +if (virStorageSourceIsRAID(backingStore)) {
> +xmlNodePtr cur = NULL;
>  
> -if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 ||
> -virDomainDiskBackingStoreParse(ctxt, backingStore) < 0)
> -goto cleanup;
> +if (!virDomainDiskThresholdParse(backingStore, node))
> +goto cleanup;
>  
> -if (!virStorageSourceSetBackingStore(src, backingStore, 0))
> +for (cur = node->children; cur != 

[libvirt] [PATCH] bhyve: add bootrom support

[Roman Bogorodskiy]

Recently, bhyve got UEFI support that eliminates necessity of
having two-step process of starting a VM, i.e. loading OS
using loader like bhyveloader or grub and then calling bhyve itself.

>From user perspective usage looks the following: if a domain XML
contains the '' element and it's value is a path
to non-executable file, then it's treated as a path to bootrom
file and external loader is not execute.

All the existing behavior remains the same, e.g. if bootloader
is not specified, bhyveloader is used. If loader is specified and
it points to an executable file, it's executed before running
bhyve.

Implementation consists of two major parts:

 * Add probing for capability by checking if the current bhyve
   binary accepts '-l bootrom' argument meaning that UEFI is
   supported
 * virBhyveProcessBuildBhyveCmd is extended with 'external_loader'
   argument. If it decides (based on the schema described above)
   that external loader is not needed, it sets it to false. All
   the related code changed accordingly to act based on value
   of external_loader.
---
 src/bhyve/bhyve_capabilities.c | 11 
 src/bhyve/bhyve_capabilities.h |  1 +
 src/bhyve/bhyve_command.c  | 20 +++-
 src/bhyve/bhyve_command.h  |  1 +
 src/bhyve/bhyve_driver.c   | 15 +++---
 src/bhyve/bhyve_process.c  | 58 --
 .../bhyvexml2argvdata/bhyvexml2argv-bhyveload.args |  3 ++
 .../bhyvexml2argv-bhyveload.ldargs |  1 +
 .../bhyvexml2argvdata/bhyvexml2argv-bhyveload.xml  | 24 +
 tests/bhyvexml2argvdata/bhyvexml2argv-bootrom.args |  4 ++
 tests/bhyvexml2argvdata/bhyvexml2argv-bootrom.xml  | 23 +
 tests/bhyvexml2argvmock.c  | 13 +
 tests/bhyvexml2argvtest.c  | 39 ---
 13 files changed, 162 insertions(+), 51 deletions(-)
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload.args
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload.ldargs
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload.xml
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bootrom.args
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bootrom.xml

diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
index d2970a2..fbdd63c 100644
--- a/src/bhyve/bhyve_capabilities.c
+++ b/src/bhyve/bhyve_capabilities.c
@@ -166,6 +166,17 @@ virBhyveProbeCaps(unsigned int *caps)
 if (strstr(help, "-u:") != NULL)
 *caps |= BHYVE_CAP_RTC_UTC;
 
+virCommandFree(cmd);
+
+cmd = virCommandNew(binary);
+virCommandAddArgList(cmd, "-l", "bootrom", NULL);
+if (virCommandRun(cmd, ) < 0) {
+ret = -1;
+goto out;
+}
+if (exit == 1)
+*caps |= BHYVE_CAP_BOOTROM;
+
  out:
 VIR_FREE(help);
 virCommandFree(cmd);
diff --git a/src/bhyve/bhyve_capabilities.h b/src/bhyve/bhyve_capabilities.h
index 0eb22a4..25dfb2c 100644
--- a/src/bhyve/bhyve_capabilities.h
+++ b/src/bhyve/bhyve_capabilities.h
@@ -33,6 +33,7 @@ typedef enum {
 
 typedef enum {
 BHYVE_CAP_RTC_UTC = 1,
+BHYVE_CAP_BOOTROM = 2,
 } virBhyveCapsFlags;
 
 int virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps);
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 6576029..10d504e 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -216,7 +216,9 @@ bhyveBuildDiskArgStr(const virDomainDef *def 
ATTRIBUTE_UNUSED,
 
 virCommandPtr
 virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
- virDomainDefPtr def, bool dryRun)
+ virDomainDefPtr def,
+ bool *external_loader,
+ bool dryRun)
 {
 /*
  * /usr/sbin/bhyve -c 2 -m 256 -AI -H -P \
@@ -230,6 +232,22 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
 
 virCommandPtr cmd = virCommandNew(BHYVE);
 
+/* Check what loader to use */
+if ((def->os.bootloader != NULL) &&
+(!virFileIsExecutable(def->os.bootloader))) {
+*external_loader = false;
+if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_BOOTROM) == 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Installed bhyve binary does not support "
+  "bootrom"));
+goto error;
+}
+virCommandAddArg(cmd, "-l");
+virCommandAddArgFormat(cmd, "bootrom,%s", def->os.bootloader);
+} else {
+*external_loader = true;
+}
+
 /* CPUs */
 virCommandAddArg(cmd, "-c");
 virCommandAddArgFormat(cmd, "%d", def->vcpus);
diff --git a/src/bhyve/bhyve_command.h b/src/bhyve/bhyve_command.h
index 22a959d..20a3bf0 100644
--- a/src/bhyve/bhyve_command.h
+++ b/src/bhyve/bhyve_command.h
@@ -32,6 +32,7 @@
 
 virCommandPtr virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
   

[libvirt] [PATCH v3 0/9] Few VFIO related fixes

[Shivaprasad G Bhat]

The following series implements...

---
Addressed comments on V2 and V1
Fixed the while loop exit in P8 virPCIDeviceUnbindFromStub()

Shivaprasad G Bhat (9):
  Implement virPCIIsKnownStub function
  Initialize the stubDriver of pci devices if bound to a valid one
  Add iommu group number info to virPCIDevice
  Refuse to reattach from vfio if the iommu group is in use by any domain
  Wait for vfio-pci device cleanups before reassinging the device to host 
driver
  Split reprobe action from the virPCIUnbindFromStub into a new function
  Pass activeDevs and inactiveDevs to virPCIDeviceUnbindFromStub and 
virPCIDeviceBindToStub
  Postpone reprobing till all the devices in iommu group are unbound from 
vfio
  Wait for iommmu device to go away before reprobing the host driver


 src/libvirt_private.syms |1 
 src/util/virhostdev.c|   18 +++
 src/util/virpci.c|  300 +++---
 src/util/virpci.h|2 
 tests/virpcimock.c   |  187 ++---
 tests/virpcitest.c   |  152 +++
 6 files changed, 566 insertions(+), 94 deletions(-)

--
Signature

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

Re: [libvirt] [PATCH v6 02/13] virstoragefile: Always use virStorageSourceGetBackingStore to get backing store

[Matthias Gatto]

On Mon, Nov 2, 2015 at 8:42 AM, Peter Krempa  wrote:

>
>> @@ -99,37 +100,39 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
>>  if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
>>  goto cleanup;
>>
>> -target->backingStore->format = backingStoreFormat;
>> +backingStore = virStorageSourceGetBackingStore(target, 0);
>> +backingStore->format = backingStoreFormat;
>>
>>  /* XXX: Remote storage doesn't play nicely with volumes backed by
>>   * remote storage. To avoid trouble, just fake the backing store is 
>> RAW
>>   * and put the string from the metadata as the path of the target. 
>> */
>> -if (!virStorageSourceIsLocalStorage(target->backingStore)) {
>> -virStorageSourceFree(target->backingStore);
>> +if (!virStorageSourceIsLocalStorage(backingStore)) {
>> +virStorageSourceFree(backingStore);
>
> So this frees the new backingStore variable, which also corresponds to
> target->backingStore at this point, but ...
>
>>
>> -if (VIR_ALLOC(target->backingStore) < 0)
>> +if (VIR_ALLOC(backingStore) < 0)
>
> ... here only the local copy is allocated, so target->backingStore
> contains an invalid pointer ...
>
>>  goto cleanup;
>>
>> -target->backingStore->type = VIR_STORAGE_TYPE_NETWORK;
>> -target->backingStore->path = meta->backingStoreRaw;
>> +target->backingStore = backingStore;
>> +backingStore->type = VIR_STORAGE_TYPE_NETWORK;
>> +backingStore->path = meta->backingStoreRaw;
>>  meta->backingStoreRaw = NULL;
>> -target->backingStore->format = VIR_STORAGE_FILE_RAW;
>> +backingStore->format = VIR_STORAGE_FILE_RAW;
>>  }
>>
>> -if (target->backingStore->format == VIR_STORAGE_FILE_AUTO) {
>> -if ((rc = virStorageFileProbeFormat(target->backingStore->path,
>> +if (backingStore->format == VIR_STORAGE_FILE_AUTO) {
>> +if ((rc = virStorageFileProbeFormat(backingStore->path,
>>  -1, -1)) < 0) {
>>  /* If the backing file is currently unavailable or is
>>   * accessed via remote protocol only log an error, fake the
>>   * format as RAW and continue. Returning -1 here would
>>   * disable the whole storage pool, making it unavailable for
>>   * even maintenance. */
>> -target->backingStore->format = VIR_STORAGE_FILE_RAW;
>> +backingStore->format = VIR_STORAGE_FILE_RAW;
>>  virReportError(VIR_ERR_INTERNAL_ERROR,
>> _("cannot probe backing volume format: %s"),
>> -   target->backingStore->path);
>> +   backingStore->path);
>>  } else {
>> -target->backingStore->format = rc;
>> +backingStore->format = rc;
>
> ... and I couldn't find a place where you update it back.


>> -target->backingStore->type = VIR_STORAGE_TYPE_NETWORK;
>> -target->backingStore->path = meta->backingStoreRaw;
>> +target->backingStore = backingStore;

I do it here.


Still, you have a good point about temp variables, and long line,
I'm working on this now.

Thank you for the review.

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

Re: [libvirt] [PATCH 2/4] Virsh-edit interface was added

[Peter Krempa]

On Mon, Nov 02, 2015 at 14:34:44 +0300, rodinasop...@gmail.com wrote:
> From: sonya 
> 
> ---
>  tools/virsh-edit.h | 44 
>  1 file changed, 44 insertions(+)
>  create mode 100644 tools/virsh-edit.h
> 
> diff --git a/tools/virsh-edit.h b/tools/virsh-edit.h
> new file mode 100644

This should be squashed into the previous commit.


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

Re: [libvirt] [PATCH 1/4] General structures and interfaces were added

[Peter Krempa]

On Mon, Nov 02, 2015 at 14:34:43 +0300, rodinasop...@gmail.com wrote:
> From: sonya 

The commit message is used to explain the changes.

> 
> ---
>  tools/Makefile.am   |   1 +
>  tools/virsh-domain.c| 195 
> 
>  tools/virsh-edit.c  |  97 +---
>  tools/virsh-interface.c |  66 
>  tools/virsh-network.c   |  66 
>  tools/virsh-nwfilter.c  |  67 +
>  tools/virsh-pool.c  |  66 
>  tools/virsh-snapshot.c  |  78 ++-
>  8 files changed, 437 insertions(+), 199 deletions(-)
> 
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index d5638d9..88bb1b2 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -205,6 +205,7 @@ virsh_SOURCES =   
> \
>   virsh-domain.c virsh-domain.h   \
>   virsh-domain-monitor.c virsh-domain-monitor.h   \
>   virsh-host.c virsh-host.h   \
> + virsh-edit.c virsh-edit.h   \
>   virsh-interface.c virsh-interface.h \
>   virsh-network.c virsh-network.h \
>   virsh-nodedev.c virsh-nodedev.h \
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 12e85e3..648b086 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -56,6 +56,7 @@
>  #include "virtime.h"
>  #include "virtypedparam.h"
>  #include "virxml.h"
> +#include "virsh-edit.h"

This file is not created in this commit, so this will fail to compile.

>  
>  /* Gnulib doesn't guarantee SA_SIGINFO support.  */
>  #ifndef SA_SIGINFO
> @@ -4622,6 +4623,45 @@ static const vshCmdOptDef opts_save_image_edit[] = {
>  {.name = NULL}
>  };
>  
> +typedef struct editDomainUniversalStruct

We tend to prefix struct names with 'vir', 'vsh' or 'virsh' according to
the section it's used in.

> +{
> + vshControl *ctl;
> + virshControlPtr priv;
> + const char *file;
> + virDomainPtr dom;
> + unsigned int getxml_flags;
> + bool *ret;
> + unsigned int *define_flags;
> + const char *key;
> + virDomainPtr *dom_edited;
> +} editDomainUniversalStruct;

This complete patch has broken formatting. Please use 4 spaces per level
to indent code. (See our contributors documentation for guidelines)

> +
> +static char*

the pointer should be separated with a space

> +editDomainSaveImageGetXML(void *args)

No prefix.

> +{
> + editDomainUniversalStruct *str = (editDomainUniversalStruct*)args;

This typecast will be done automatically.

> + return virDomainSaveImageGetXMLDesc(str->priv->conn, str->file, 
> str->getxml_flags);
> +}
> +

At this point, formatting and the fact that this doesn't compile is
enough to break the review here. Please post a fixed version and make
sure that 'make check' and 'make syntax-check' pass after every single
commit. Also plese fill the commit message and fix the formatting
according to guidelines.

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 v6 09/13] qemu: Add quorum support in qemuBuildDriveDevStr

[Peter Krempa]

On Thu, Oct 29, 2015 at 14:43:16 +0100, Matthias Gatto wrote:
> Allow libvirt to build the quorum string used by quemu.
> 
> Add 2 static functions: qemuBuildRAIDStr and
> qemuBuildRAIDFileSourceStr.
> qemuBuildRAIDStr is made because a quorum can have another quorum
> as a child, so we may need to call qemuBuildRAIDStr recursively.
> 
> qemuBuildRAIDFileSourceStr was basically made to share
> the code use to build the source between qemuBuildRAIDStr and
> qemuBuildDriveStr, but there is some difference betwin the syntax
> use by libvirt to declare a disk and the one qemu need to build a quorum:
> a quorum need a syntaxe like:
> "domaine_name.children.X.file.filename=filename"
> where libvirt don't use "file.filename=" but directly "file=".
> Therfore I use this function only for quorum.
> 
> Signed-off-by: Matthias Gatto 
> ---
>  src/qemu/qemu_command.c | 93 
> +
>  1 file changed, 93 insertions(+)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 50cf8cc..4ca0011 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3612,6 +3612,93 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk)
>  return -1;
>  }
>  
> +static bool

The same comment regarding return value as in previous cases.

> +qemuBuildRAIDFileSourceStr(virConnectPtr conn,
> +   virStorageSourcePtr src,
> +   virBuffer *opt,
> +   const char *toAppend)

Since 'toAppend' is always pre-pended I'd rather call it prefix.

> +{
> +char *source = NULL;
> +int actualType = virStorageSourceGetActualType(src);
> +
> +if (qemuGetDriveSourceString(src, conn, ) < 0)

Are you sure that it will work with remote storage too?

> +return false;
> +
> +if (source) {
> +virBufferStrcat(opt, ",", toAppend, "filename=", NULL);

Since you already have 'source' here you can append it right away ...

> +
> +if (actualType == VIR_STORAGE_TYPE_DIR) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("unsupported disk driver type for '%s'"),
> +   virStorageFileFormatTypeToString(src->format));

This should probably be extracted somewhere else so that there's a
single point where we can check it.

> +return false;
> +}
> +virBufferAdd(opt, source, -1);

... rather than here.

> +}
> +
> +return true;
> +}
> +
> +
> +static bool
> +qemuBuildRAIDStr(virConnectPtr conn,
> + virDomainDiskDefPtr disk,
> + virStorageSourcePtr src,
> + virBuffer *opt,
> + const char *toAppend)
> +{
> +char *tmp = NULL;
> +int ret;
> +virStorageSourcePtr backingStore;
> +size_t i;
> +
> +if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_QUORUM) {
> +if (!src->threshold) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("threshold missing in the quorum 
> configuration"));
> +return false;
> +}
> +if (src->nBackingStores < 2) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("a quorum must have at last 2 children"));
> +return false;
> +}
> +if (src->threshold > src->nBackingStores) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("threshold must not exceed the number of 
> children"));
> +return false;
> +}
> +virBufferAsprintf(opt, ",%svote-threshold=%lu",
> +  toAppend, src->threshold);
> +}
> +
> +for (i = 0;  i < src->nBackingStores; ++i) {
> +backingStore = virStorageSourceGetBackingStore(src, i);
> +ret = virAsprintf(, "%schildren.%lu.file.", toAppend, i);

So here you create 'tmp' ...

> +if (ret < 0)
> +return false;
> +
> +virBufferAsprintf(opt, ",%schildren.%lu.driver=%s",
> +  toAppend, i,
> +  
> virStorageFileFormatTypeToString(backingStore->format));
> +
> +if (qemuBuildRAIDFileSourceStr(conn, backingStore, opt, tmp) == 
> false)

.. this function reads it ... 

> +goto error;
> +
> +/* This operation avoid us to made another copy */
> +tmp[ret - sizeof("file")] = '\0';

... so why is this necessary? Also I think it has a off-by-one which is
transparet since the string is containing a trailing dot, and sizeof
returns the size including the 0-byte at the end of the string.


> +if (virStorageSourceIsRAID(backingStore)) {
> +if (!qemuBuildRAIDStr(conn, disk, backingStore, opt, tmp))
> +goto error;
> +}
> +VIR_FREE(tmp);
> +}
> +return true;
> + error:
> +VIR_FREE(tmp);
> +return false;
> +}
> +
>  
>  /* Check whether