Re: [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions
> > Yes, I think so. However to really make good hints, upper layers would most > > likely need more information about the exact problem with a property - > > maybe something like an enum value per problematic property. > > (UNAVAILABLE_FEATURE, VALUE_TOO_BIG, VALUE_TOO_SMALL, UNSUPPORTED_VALUE) > > ... > > If a more powerful interface is needed, we can add extra fields, > yes. Although I'm not sure we really start providing that level > of detail in a generic way (I guess it will depend on how much > arch-independent code libvirt will use to handle runnability > information). And I would actually (later) prefer to have something like bool too_new; (name tbd) to cover the cpu-generation problem instead of having to expose read-only properties just for the sake of communicating that a cpu model is simply too new and cannot be made runnable toggling features. > > > > > > > > > > > > > We could replace this with something more generic, like: > > > > > > > > > > @runnability-blockers: List of attributes that prevent the CPU > > > > > model from running in the current host. > > > > > > > > > > A list of QOM property names that represent CPU model > > > > > attributes that prevent the CPU from running. If the QOM > > > > > property is read-only, that means the CPU model can never run > > > > > in the current host. If the property is read-write, it means > > > > > that it MAY be possible to run the CPU model in the current > > > > > host if that property is changed. > > > > > > > > > > Management software can use it as hints to suggest or choose an > > > > > alternative for the user, or just to generate meaningful error > > > > > messages explaining why the CPU model can't be used. > > > > > > > > > > (I am looking for a better name than "runnability-blockers"). > > > > > > > > > Not sure which approach would be better. > > Note that this is only a change in documentation of the new > field, to allow it to cover extra cases without any changes in > the interface. > > I also don't like the "runnability-blockers" name, but I prefer > the new documentation I wrote above because it is more explicit > about what exactly the field means. I plan to keep the > "unavailable-features" name but use the above documentation text > in the next version. Does that sound OK? > I like the read-only part about that, but still you should somehow clarify that we are dealing with boolean properties a.k.a features. At least that's my opinion. Apart from that, this is fine with me! David -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] NFS over AF_VSOCK in
On Mon, May 09, 2016 at 16:57:17 +0100, Stefan Hajnoczi wrote: > virtio-vsock support has been added to the nfs-ganesha NFS server. I'm > currently working on upstreaming virtio-vsock into Linux and QEMU. I > also have patches for the Linux NFS client and server. Just out of curiosity are you planning to add support for virtio-sock to nfsroot? Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] libxl: fix vm lock overwritten bug
In libxl driver we do virObjectRef in libxlDomainObjBeginJob,
If virCondWaitUntil failed, it goes to error, do virObjectUnref,
There's a chance that someone undefine the vm at the same time,
and refs unref to zero, vm is freed in libxlDomainObjBeginJob.
But the vm outside function is not Null, we do virObjectUnlock(vm).
That's how we overwrite the vm memory after it's freed. Because the
coding amount is much, I fix it partly in libxlDomainCreateWithFlags.
If my opinion is right and there're no problems, I'll fix them all
later.
Signed-off-by: Wang Yufei
---
.gnulib | 1 -
src/libxl/libxl_domain.c | 6 +-
src/libxl/libxl_domain.h | 2 +-
src/libxl/libxl_driver.c | 5 ++---
4 files changed, 4 insertions(+), 10 deletions(-)
delete mode 16 .gnulib
diff --git a/.gnulib b/.gnulib
deleted file mode 16
index 6cc32c6..000
--- a/.gnulib
+++ /dev/null
@@ -1 +0,0 @@
-Subproject commit 6cc32c63e80bc1a30c521b2f07f2b54909b59892
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 14a900c..a90ce53 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -123,7 +123,6 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver
ATTRIBUTE_UNUSED,
return -1;
then = now + LIBXL_JOB_WAIT_TIME;
-virObjectRef(obj);
while (priv->job.active) {
VIR_DEBUG("Wait normal job condition for starting job: %s",
@@ -157,7 +156,6 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver
ATTRIBUTE_UNUSED,
virReportSystemError(errno,
"%s", _("cannot acquire job mutex"));
-virObjectUnref(obj);
return -1;
}
@@ -171,7 +169,7 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver
ATTRIBUTE_UNUSED,
* non-zero, false if the reference count has dropped to zero
* and obj is disposed.
*/
-bool
+void
libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
virDomainObjPtr obj)
{
@@ -183,8 +181,6 @@ libxlDomainObjEndJob(libxlDriverPrivatePtr driver
ATTRIBUTE_UNUSED,
libxlDomainObjResetJob(priv);
virCondSignal(&priv->job.cond);
-
-return virObjectUnref(obj);
}
int
diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
index 1c1eba3..ce28944 100644
--- a/src/libxl/libxl_domain.h
+++ b/src/libxl/libxl_domain.h
@@ -85,7 +85,7 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver,
enum libxlDomainJob job)
ATTRIBUTE_RETURN_CHECK;
-bool
+void
libxlDomainObjEndJob(libxlDriverPrivatePtr driver,
virDomainObjPtr obj)
ATTRIBUTE_RETURN_CHECK;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index bf97c9c..a46994a 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -294,7 +294,7 @@ libxlDomObjFromDomain(virDomainPtr dom)
libxlDriverPrivatePtr driver = dom->conn->privateData;
char uuidstr[VIR_UUID_STRING_BUFLEN];
-vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
+vm = virDomainObjListFindByUUIDRef(driver->domains, dom->uuid);
if (!vm) {
virUUIDFormat(dom->uuid, uuidstr);
virReportError(VIR_ERR_NO_DOMAIN,
@@ -2691,8 +2691,7 @@ libxlDomainCreateWithFlags(virDomainPtr dom,
vm = NULL;
cleanup:
-if (vm)
-virObjectUnlock(vm);
+virDomainObjEndAPI(&vm);
return ret;
}
--
1.9.5.msysgit.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] uml: only build on Linux
Cole Robinson wrote: > On 05/08/2016 09:31 AM, Roman Bogorodskiy wrote: > > Changes since v1: > > > > - Add forgotten with_libvirtd check and unite it with the > > with_linux check > > - Minor code formatting fixes in m4/virt-driver-uml.m4 > > > > Roman Bogorodskiy (2): > > configure: split out UML driver checks > > uml: only build on Linux > > > > configure.ac | 28 ++ > > m4/virt-driver-uml.m4 | 55 > > +++ > > 2 files changed, 57 insertions(+), 26 deletions(-) > > create mode 100644 m4/virt-driver-uml.m4 > > > > ACK series Pushed, thanks! Roman Bogorodskiy -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: don't redefine virDomainCapsDeviceHostdev
Commit 5ed235c6 added unnecessary redifinition of
virDomainCapsDeviceHostdev in conf/domain_capabilities.h. This breaks
build with clang 3.4:
In file included from conf/domain_capabilities.c:25:
conf/domain_capabilities.h:88:44: error: redefinition of typedef
'virDomainCapsDeviceHostdev' is a C11 feature
[-Werror,-Wtypedef-redefinition]
typedef struct _virDomainCapsDeviceHostdev virDomainCapsDeviceHostdev;
^
conf/domain_capabilities.h:86:44: note: previous definition is here
typedef struct _virDomainCapsDeviceHostdev virDomainCapsDeviceHostdev;
So drop one of those.
Pushed under the build breaker fix rule.
---
src/conf/domain_capabilities.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index d0ca009..492a9cf 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -84,8 +84,6 @@ struct _virDomainCapsDeviceVideo {
};
typedef struct _virDomainCapsDeviceHostdev virDomainCapsDeviceHostdev;
-
-typedef struct _virDomainCapsDeviceHostdev virDomainCapsDeviceHostdev;
typedef virDomainCapsDeviceHostdev *virDomainCapsDeviceHostdevPtr;
struct _virDomainCapsDeviceHostdev {
bool supported;
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] (no subject)
On 05/09/2016 08:10 PM, Lovely K. wrote: > plz added me ...i need a some application for development Without more details (including a decent subject line), it's very hard to tell what you want help with. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] (no subject)
plz added me ...i need a some application for development -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] virsh: blkdeviotune: accept human readable values for bytes
On Mon, May 9, 2016 at 11:32 AM, Michal Privoznik wrote: > On 04.05.2016 16:25, Nishith Shah wrote: > > [1] .. here. > > > Use vshCommandOptScaledInt instead of vshCommandOptULongLong so that > > values with suffixes can be passed when bytes are being passed along. > > Values for the iops parameters still need to be given in the absolute > > form as they are not bytes but numbers. Please refer to the bug link > > https://bugzilla.redhat.com/show_bug.cgi?id=885380 which can be closed. > > I've moved this bug reference [1]. It's our common way of referring bugs > in commit messages. > Thanks for the pointer, I'll keep that in mind in future commit messages. > > > > > Signed-off-by: Nishith Shah > > --- > > tools/virsh-domain.c | 24 > > tools/virsh.pod | 18 -- > > 2 files changed, 24 insertions(+), 18 deletions(-) > > ACKed and pushed. > > Congratulations on your first libvirt contribution! > Thank you :) Looking forward to more! Nishith -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] uml: only build on Linux
On 05/08/2016 09:31 AM, Roman Bogorodskiy wrote: > Changes since v1: > > - Add forgotten with_libvirtd check and unite it with the > with_linux check > - Minor code formatting fixes in m4/virt-driver-uml.m4 > > Roman Bogorodskiy (2): > configure: split out UML driver checks > uml: only build on Linux > > configure.ac | 28 ++ > m4/virt-driver-uml.m4 | 55 > +++ > 2 files changed, 57 insertions(+), 26 deletions(-) > create mode 100644 m4/virt-driver-uml.m4 > ACK series - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] qemu_hotplug: fix checking graphics ports
On 05/09/2016 09:48 AM, Pavel Hrdina wrote: > We cannot change ports for running domain and we should error out if > autoport is enabled. > > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_hotplug.c | 13 > + > .../qemuhotplug-graphics-spice-listen-network.xml | 2 +- > 2 files changed, 6 insertions(+), 9 deletions(-) > ACK - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] qemu_process: merge graphics code into qemuProcessSetupGraphics
On 05/09/2016 09:48 AM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_process.c | 45 ++--- > 1 file changed, 14 insertions(+), 31 deletions(-) ACK - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] graphics: generate fake ports also for tests
On 05/09/2016 09:48 AM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_driver.c | 12 > src/qemu/qemu_process.c| 22 > -- > src/qemu/qemu_process.h| 8 > .../qemuxml2argv-controller-order.args | 2 +- > .../qemuxml2argv-hugepages-numa.args | 2 +- > .../qemuxml2argv-video-device-pciaddr-default.args | 2 +- > 6 files changed, 15 insertions(+), 33 deletions(-) ACK - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] qemu_process: separate graphics port reservation
On 05/09/2016 09:48 AM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina > --- > src/qemu/qemu_process.c | 27 --- > 1 file changed, 20 insertions(+), 7 deletions(-) ACK - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REPOST 20/38] virlog: Split output parsing and output defining to separate operations
On 05/04/2016 10:30 AM, Erik Skultety wrote: > Everything has been prepared to successfully split parsing and defining logic > to separate operations. > --- > src/util/virlog.c | 160 > + > src/util/virlog.h | 15 +++-- > tests/testutils.c | 19 ++- > tests/virlogtest.c | 5 +- > 4 files changed, 114 insertions(+), 85 deletions(-) Hmm. I see what you were going for with this patch layout: line up all the pieces so you can internally convert everything in one fell swoop with patch #20 and #21. However that makes it difficult to review IMO, most of the critical change is bottled up in these few patches that depend on the previous 20 patches of context. If instead you had (just an idea) renamed all the poorly named functions to more descriptive names (like virLogParse* functions to virLogParseAndSet*) up front, you could then split out virLogParse* and virLogSet* functions completely internally to virlog.c and transparent to the current users, the patches would be more individually self contained. Then you could convert all the current users to the new functions, and drop the old poorly named ones. I realize reworking that will be a pain so I'll try and review as is (more tomorrow), but I'll jump around a bit. Also when posting the next round, I suggest just splitting out the logging rework first, then once all of that is committed, post a second series with the API bits. - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REPOST 22/38] virlog: Split parsing and setting priority
On 05/04/2016 10:30 AM, Erik Skultety wrote: > Handling of outputs and filters has been changed in a way that splits > parsing and defining. Do the same thing for logging priority as well, this > however, doesn't need much of a preparation. > --- > src/util/virlog.c | 21 + > tests/eventtest.c | 3 ++- > 2 files changed, 11 insertions(+), 13 deletions(-) > ACK, and could go in separately - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REPOST 16/38] virlog: Introduce virLogOutputListFree
On 05/04/2016 10:30 AM, Erik Skultety wrote:
> This is just a convenience method for discarding a list of outputs instead of
> using a 'for' loop everywhere. It is safe to pass -1 as the number of elements
> in the list as well as passing NULL as list reference.
> ---
> src/libvirt_private.syms | 1 +
> src/util/virlog.c| 20
> src/util/virlog.h| 1 +
> 3 files changed, 22 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index b40a405..608d959 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1795,6 +1795,7 @@ virLogGetOutputs;
> virLogLock;
> virLogMessage;
> virLogOutputFree;
> +virLogOutputListFree;
> virLogOutputNew;
> virLogParseDefaultPriority;
> virLogParseFilters;
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index e36ff73..5da1af7 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -1600,3 +1600,23 @@ virLogDefineFilters(virLogFilterPtr *filters, size_t
> nfilters)
>
> return virLogNbFilters;
> }
> +
> +/**
> + * virLogOutputsFreeList:
> + * @list: list of outputs to be freed
> + * @count: number of elements in the list
> + *
> + * Frees a list of outputs.
> + */
> +void
> +virLogOutputListFree(virLogOutputPtr *list, int count)
> +{
> +size_t i;
> +
> +if (!list || count < 0)
> +return;
> +
> +for (i = 0; i < count; i++)
> +virLogOutputFree(list[i]);
> +VIR_FREE(list);
> +}
> diff --git a/src/util/virlog.h b/src/util/virlog.h
> index 7573984..4f0eea7 100644
> --- a/src/util/virlog.h
> +++ b/src/util/virlog.h
> @@ -196,6 +196,7 @@ extern int virLogOutputNew(virLogOutputFunc f,
> const char *name,
> unsigned int flags);
> extern void virLogOutputFree(virLogOutputPtr output);
> +extern void virLogOutputListFree(virLogOutputPtr *list, int count);
>
> /*
> * Internal logging API
>
Why not convert virLogResetOutputs at the same time, like was done in the
previous patch? ACK with that (unless I missed some subtlety)
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REPOST 19/38] virlog: Make virLogReset methods use of virLog(Output|Filter)ListFree
On 05/04/2016 10:30 AM, Erik Skultety wrote:
> Now that methods to free logging related lists were introduced, put them to a
> use.
> ---
> src/util/virlog.c | 15 ---
> 1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index a1f5872..7e0936c 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -239,11 +239,8 @@ virLogSetDefaultPriority(virLogPriority priority)
> static void
> virLogResetFilters(void)
> {
> -size_t i;
> -
> -for (i = 0; i < virLogNbFilters; i++)
> -VIR_FREE(virLogFilters[i]->match);
> -VIR_FREE(virLogFilters);
> +virLogFilterListFree(virLogFilters, virLogNbFilters);
> +virLogFilters = NULL;
> virLogNbFilters = 0;
> virLogFiltersSerial++;
> }
> @@ -321,12 +318,8 @@ virLogFilterNew(const char *match,
> static void
> virLogResetOutputs(void)
> {
> -size_t i;
> -
> -for (i = 0; i < virLogNbOutputs; i++)
> -virLogOutputFree(virLogOutputs[i]);
> -
> -VIR_FREE(virLogOutputs);
> +virLogOutputListFree(virLogOutputs, virLogNbOutputs);
> +virLogOutputs = NULL;
> virLogNbOutputs = 0;
> }
>
>
I'd rather see this patch split and combined in with the previous two patches.
Like you did for the virLogOutputFree patch. With those changes, ACK to the
previous two patches, and would be fine to push now since this is a reasonable
cleanup IMO
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REPOST 15/38] virlog: Introduce virLogOutputFree
On 05/04/2016 10:30 AM, Erik Skultety wrote:
> Add a complementary method to virLogOutputNew.
> ---
> src/libvirt_private.syms | 1 +
> src/util/virlog.c| 21 -
> src/util/virlog.h| 1 +
> 3 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 56dfcf4..b40a405 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1794,6 +1794,7 @@ virLogGetNbOutputs;
> virLogGetOutputs;
> virLogLock;
> virLogMessage;
> +virLogOutputFree;
> virLogOutputNew;
> virLogParseDefaultPriority;
> virLogParseFilters;
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index e101484..e36ff73 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -323,16 +323,27 @@ virLogResetOutputs(void)
> {
> size_t i;
>
> -for (i = 0; i < virLogNbOutputs; i++) {
> -if (virLogOutputs[i]->c != NULL)
> -virLogOutputs[i]->c(virLogOutputs[i]->data);
> -VIR_FREE(virLogOutputs[i]->name);
> -}
> +for (i = 0; i < virLogNbOutputs; i++)
> +virLogOutputFree(virLogOutputs[i]);
> +
> VIR_FREE(virLogOutputs);
> virLogNbOutputs = 0;
> }
>
>
> +void
> +virLogOutputFree(virLogOutputPtr output)
> +{
> +if (!output)
> +return;
> +
> +if (output->c)
> +output->c(output->data);
> +VIR_FREE(output->name);
> +VIR_FREE(output);
> +
> +}
> +
> /**
> * virLogOutputNew:
> * @f: the function to call to output a message
> diff --git a/src/util/virlog.h b/src/util/virlog.h
> index 93456db..7573984 100644
> --- a/src/util/virlog.h
> +++ b/src/util/virlog.h
> @@ -195,6 +195,7 @@ extern int virLogOutputNew(virLogOutputFunc f,
> virLogDestination dest,
> const char *name,
> unsigned int flags);
> +extern void virLogOutputFree(virLogOutputPtr output);
>
> /*
> * Internal logging API
>
ACK
(sidenote: I know you were following existing convention but the usage of
extern forward declarations in the header is confusing to me... my brain is
wired to see header declaration == public function)
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] libxl: support Xen migration stream V2
On 05/03/2016 07:19 AM, Wei Liu wrote: > On Mon, May 02, 2016 at 07:01:16PM -0600, Jim Fehlig wrote: >> Hi all, >> >> This patch adds support for Xen migration stream V2 to the libvirt >> libxl driver. In the process it fixes save/restore and migration >> tests in OSSTest, which have been failing since libvirt commit >> e7440656. >> >> Patch1 changes the libxl API requirement from version 4.2 to 4.4, >> enabling use of an extended libxl_domain_create_restore() function >> that allows passing restore parameters such as stream version. >> >> Patch2 adds support for migration stream V2 in the basic save/restore >> logic, taking advantage of a save image header that includes a version >> field. The version is set to '2' if the host produces a V2 migration >> stream. This patch fixes the failing save/restore tests in OSSTest. >> >> Patch3 adds support for migration stream V2 in the migration logic. >> The migration code does not use the save image header and instead >> uses libvirt's migration protocol to transfer domain configuration >> and other such goodies from source to destination. The patch >> enables sending version information in the Begin and Prepare >> migration phases so the correct stream version information can be >> passed to libxl_domain_create_restore(). An upshot of this approach >> is safely detecting and aborting a migration from a V2 host to a >> V1-only host. This patch fixes the failing migration tests in >> OSSTest. >> > The general approach looks good to me. Any comments on this series from libvirt maintainers? Would be nice to get this series (or a variant) committed, fixing the daily OSSTest failures. For your viewing pleasure, here's a link to today's failure http://logs.test-lab.xenproject.org/osstest/logs/93880/ Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REPOST 04/38] virlog: Export virLogOutputPtr through header
On 05/04/2016 10:30 AM, Erik Skultety wrote:
> It needs to be exported, since some caller might (for some reason) want to
> create a logging output without calling the parser which does this. Also,
> some methods will use virLogOutputPtr as data type for one of its arguments.
> ---
> src/util/virlog.c | 2 --
> src/util/virlog.h | 3 +++
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index 812e2cd..0be1701 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -106,8 +106,6 @@ struct _virLogOutput {
> virLogDestination dest;
> char *name;
> };
> -typedef struct _virLogOutput virLogOutput;
> -typedef virLogOutput *virLogOutputPtr;
>
> static virLogOutputPtr *virLogOutputs;
> static size_t virLogNbOutputs;
> diff --git a/src/util/virlog.h b/src/util/virlog.h
> index b5056f5..7706d22 100644
> --- a/src/util/virlog.h
> +++ b/src/util/virlog.h
> @@ -130,6 +130,9 @@ struct _virLogMetadata {
> typedef struct _virLogMetadata virLogMetadata;
> typedef struct _virLogMetadata *virLogMetadataPtr;
>
> +typedef struct _virLogOutput virLogOutput;
> +typedef virLogOutput *virLogOutputPtr;
> +
> /**
> * virLogOutputFunc:
> * @src: the source of the log message
>
ACK, but IMO exporting it early in a separate patch without context makes it
hard to follow the reasoning. Better would have been to export it with the
first public function that needs it, looks like virLogDefineOutputs
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REPOST 05/38] virlog: Export virLogFilterPtr through header
On 05/04/2016 10:30 AM, Erik Skultety wrote:
> As with outputs, some methods declared through a header file need to know
> the datatype because of their arguments.
> ---
> src/util/virlog.c | 2 --
> src/util/virlog.h | 3 +++
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index 0be1701..b893365 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -86,8 +86,6 @@ struct _virLogFilter {
> virLogPriority priority;
> unsigned int flags;
> };
> -typedef struct _virLogFilter virLogFilter;
> -typedef virLogFilter *virLogFilterPtr;
>
> static int virLogFiltersSerial = 1;
> static virLogFilterPtr *virLogFilters;
> diff --git a/src/util/virlog.h b/src/util/virlog.h
> index 7706d22..36c610b 100644
> --- a/src/util/virlog.h
> +++ b/src/util/virlog.h
> @@ -133,6 +133,9 @@ typedef struct _virLogMetadata *virLogMetadataPtr;
> typedef struct _virLogOutput virLogOutput;
> typedef virLogOutput *virLogOutputPtr;
>
> +typedef struct _virLogFilter virLogFilter;
> +typedef virLogFilter *virLogFilterPtr;
> +
> /**
> * virLogOutputFunc:
> * @src: the source of the log message
>
ACK
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REPOST 03/38] virlog: Convert virLogFilters to a list of pointers to filters
On 05/04/2016 10:30 AM, Erik Skultety wrote: > Same as with outputs; since the operations will be further divided into > smaller > tasks, creating a filter will become a separate operation that will return > a reference to a newly created filter. > --- > src/util/virlog.c | 41 +++-- > 1 file changed, 23 insertions(+), 18 deletions(-) > ACK - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REPOST 02/38] virlog: Convert virLogOutputs to a list of pointers to outputs
On 05/04/2016 10:30 AM, Erik Skultety wrote: > Right now, we define outputs one after another. However, the correct flow > should be to define a set of outputs as a whole unit. Therefore each output > should be first created, placed into an array/list and the list will be > defined. Output creation should be a separate operation, so an output will be > returned by a reference. From that perspective, it makes perfect sense to > only store pointers to actual outputs. ACK - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH REPOST 01/38] virlog: Return void instead of int in virLogReset methods
On 05/04/2016 10:30 AM, Erik Skultety wrote: > In this particular case, reset is meant as clearing the whole list of > outputs/filters, not resetting it to a predefined default setting. Looking at > it from that perspective, returning the number of records removed doesn't help > the caller in any way (not that any of the callers would actually check for > it). Well, callers could detect an error from the number of successfully > removed records, but the only thing that can fail in virLogReset is force > closing a file descriptor in which case the error isn't propagated back to > virLogReset anyway. Conclusion: there is no practical use for having a return > type of 'int' rather than 'void' in this case. ACK - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] domaincaps: Report graphics spiceGL
On 05/09/2016 09:48 AM, Daniel P. Berrange wrote: > On Mon, May 09, 2016 at 09:30:30AM -0400, Cole Robinson wrote: >> On 05/09/2016 07:30 AM, Pavel Hrdina wrote: >>> On Sun, May 08, 2016 at 01:49:06PM -0400, Cole Robinson wrote: Reports a tristate enum value for acceptable graphics type=spice . Wire it up for qemu too. 'no' is always a valid value, so we unconditionally report it. --- >>> >>> [...] >>> diff --git a/tests/domaincapsschemadata/domaincaps-full.xml b/tests/domaincapsschemadata/domaincaps-full.xml index 2f529ff..4eb5637 100644 --- a/tests/domaincapsschemadata/domaincaps-full.xml +++ b/tests/domaincapsschemadata/domaincaps-full.xml @@ -47,6 +47,11 @@ desktop spice + +default +yes +no + >>> >>> Ewww, this doesn't look good and I agree with Michal that what if we add >>> support for another graphics device and what if one graphics device will >>> have >>> more than one capability? >>> >>> I would suggest something like this: >>> >>> >>> >>> spice >>> vnc >>> sdl >>> >>> >>> >>> >>> >>> >>> or even do something like this: >>> >>> >>> >>> >>> >>> >>> >>> >>> >> >> Really wish someone would have chimed in with this to my (unresponded) RFC >> email about these things... >> >> http://www.redhat.com/archives/libvir-list/2016-April/msg01839.html >> >> That pattern is fine in this case, but what about the case of ports= element >> that is only supported with a controller type=usb model=nec-xhci ? Do we have >> new XML like: >> >> >> >> >> >> >> >> >> >> It seems like these types of relationships could grow deeply nested, so I >> opted for following the limited existing convention of adding unique enum >> names to represent the relationship. > > Yeah, I find flat modelling quite desirable, because the relationships > between attributes will certainly grow quite complicate, and they do > not neccessarily form a nice simple tree relationship. > > ie, whether foo=bar is permitted may depend on whether wizz=eek *AND* > when lala=ooh, and you can't have the enum for 'foo' nested underneath > the enum for 'wizz' & 'lala' at the same time. Also with nesting, if > you want the same values reported for multiple options, we end up > repeating the same information multiple times which is not good. > > At the same time the flat modelling with "spiceGL" also doesn't scale > as you have to invent new names for each one, and again it doesn't > work if the 'gl' enum depended on type="spice" *and* something else > at the same time. > > So I think we need a more flexible way to express dependancies here. > > We could let the be repeated multiple times and express conditional > rules against each instance of the enum > > So for example > > > > spice > vnc > sdl > > > > > > default > yes > no > > > default > no > > > > > This would express that the first entry only applies > when the @type == spice. If that doesn't match them fallback to the > second . The nice thing about this is that we can > make the conditions arbitrarly complex > > For example: > > > > spice > vnc > sdl > > > > > > > > > > default > yes > no > > > default > no > > > > > This shows how the "gl" option can be used with VNC, but only if you > also have the @wibble attribute set to "eek". > > Of course complex conditions like this become quite tedious & verbose > so obviously we should strive to keep them as simple as possible and > not use nested conditions unless absolutely needed. > Writing a parser for this type of XML, that maintains behavior in a future proof way, is going to be a lot of work. Hypothetical example: cirrus qxl no 6 months later, qemu gets some accel2d support for model=qxl cirrus qxl no yes no Another 6 months later, qemu gets accel2d support for cirrus only, but it requires and doesn't work for cirrus qxl pci isa no yes no yes no The app wants to answer the question 'can I specify accel2d by default for model=cirrus?' (note, cirrus, and not qxl) For the first example, the code parses the accel2d enum, doesn't see 'yes', so it doesn't enable accel2d. The code will need to contain some knowledge
Re: [libvirt] [PATCH 5/5] domaincaps: Report graphics spiceGL
On 05/09/2016 05:27 AM, Michal Privoznik wrote: > On 08.05.2016 19:49, Cole Robinson wrote: >> Reports a tristate enum value for acceptable video model=virtio >> . >> >> Wire it up for qemu too. 'no' is always a valid value, so we >> unconditionally report it. >> --- >> docs/formatdomaincaps.html.in | 9 + >> src/conf/domain_capabilities.c | 1 + >> src/conf/domain_capabilities.h | 2 ++ >> src/qemu/qemu_capabilities.c| 4 >> tests/domaincapsschemadata/domaincaps-full.xml | 5 + >> tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml | 3 +++ >> tests/domaincapsschemadata/domaincaps-qemu_2.6.0-1.xml | 4 >> tests/domaincapsschemadata/domaincaps-qemu_2.6.0-2.xml | 3 +++ >> tests/domaincapsschemadata/domaincaps-qemu_2.6.0-3.xml | 3 +++ >> tests/domaincapsschemadata/domaincaps-qemu_2.6.0-4.xml | 3 +++ >> tests/domaincapsschemadata/domaincaps-qemu_2.6.0-5.xml | 3 +++ >> tests/domaincapstest.c | 1 + >> 12 files changed, 41 insertions(+) >> >> diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in >> index c424107..b87a45a 100644 >> --- a/docs/formatdomaincaps.html.in >> +++ b/docs/formatdomaincaps.html.in >> @@ -268,6 +268,10 @@ >>qxl >>virtio >> >> +>> + >> >> ... >> >> @@ -278,6 +282,11 @@ >>modelType >>Options for the type attribute of the >>yes >> +no >> +
Re: [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions
On Mon, May 09, 2016 at 03:06:18PM +0200, David Hildenbrand wrote: > > > > > > > > > > Just FYI, on other architectures (e.g. s390x), other conditions (e.g. > > > > > cpu > > > > > generation) also define if a CPU model is runnable, so the pure > > > > > availability of > > > > > features does not mean that a cpu model is runnable. > > > > > > > > > > We could have runnable=false and unavailable-features being an empty > > > > > list. > > > > > > > > Even on those cases, can't the root cause be mapped to a QOM > > > > property name (e.g. "cpu-generation"), even if it's property that > > > > can't be changed by the user? > > > > > > In the current state, no. > > > > But it could be implemented by s390x in the future, if it's > > considered useful, right? > > Yes, we could fit that into read-only properties if we would ever need it > (like cpu-generation you mentioned and cpu-ga-level - both will always be > read-only). > > However we could come up with more optional fields for that in the future. > (like unsupported-values or sth. like that). I actually prefer > unavailable-features over runnability-blockers. > > > > > > > > > So I think for runnable=false: > > > a) unavailable-features set -> can be made runnable > > > b) unavailable-features not set -> cannot be made runnable > > > > > > would be enough. > > > > I understand it would be enough, but I would like to at least > > define semantics that would make sense for all architectures in > > case it gets implemented in the future. Would the proposal below > > make sense? > > > > Yes, I think so. However to really make good hints, upper layers would most > likely need more information about the exact problem with a property - > maybe something like an enum value per problematic property. > (UNAVAILABLE_FEATURE, VALUE_TOO_BIG, VALUE_TOO_SMALL, UNSUPPORTED_VALUE) ... If a more powerful interface is needed, we can add extra fields, yes. Although I'm not sure we really start providing that level of detail in a generic way (I guess it will depend on how much arch-independent code libvirt will use to handle runnability information). > > > > > > > > > We could replace this with something more generic, like: > > > > > > > > @runnability-blockers: List of attributes that prevent the CPU > > > > model from running in the current host. > > > > > > > > A list of QOM property names that represent CPU model > > > > attributes that prevent the CPU from running. If the QOM > > > > property is read-only, that means the CPU model can never run > > > > in the current host. If the property is read-write, it means > > > > that it MAY be possible to run the CPU model in the current > > > > host if that property is changed. > > > > > > > > Management software can use it as hints to suggest or choose an > > > > alternative for the user, or just to generate meaningful error > > > > messages explaining why the CPU model can't be used. > > > > > > > > (I am looking for a better name than "runnability-blockers"). > > > > > > Not sure which approach would be better. Note that this is only a change in documentation of the new field, to allow it to cover extra cases without any changes in the interface. I also don't like the "runnability-blockers" name, but I prefer the new documentation I wrote above because it is more explicit about what exactly the field means. I plan to keep the "unavailable-features" name but use the above documentation text in the next version. Does that sound OK? -- Eduardo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: add default pci-root device to mips*/malta guests
On 05/09/2016 01:55 PM, Aurelien Jarno wrote: > On 2016-05-04 09:54, Cole Robinson wrote: >> Thanks for the patch. >> >> Can you provide a working mips XML config? We don't have one in the unit test >> suite. >> >> On 05/03/2016 05:23 PM, Aurelien Jarno wrote: >>> The MIPS Malta board has a root PCI controller. It is not sent during >>> migration, so it needs to be added by default. >>> >> >> I'm a bit confused by the migration reference... does specifying the PCI >> controller on the command line somehow convince qemu to send the PCI device >> state during migration? Or am I > > No it doesn't convince it to send the PCI device state. The problem is > that the PCI root devices is not created at all on the target side. For > what I understood this is due to the code in qemuDomainDefFormatBuf() > dropping pci-root controller "for migration compatibility". > > This can be seen by using virsh managedsave followed by virsh start. The > QEMU command line doesn't include the pci root controller, which causes > the following error: "error: XML error: No PCI buses available". Indeed > looking at the saved image, it is missing from the XML, and this > actually not specific to MIPS. > Hmm. There was another fix in this area recently, but for USB controllers: commit 192a53e07c5fefd9dad2f310886209b76dcc5d83 Author: Shivaprasad G Bhat Date: Fri Apr 29 19:31:51 2016 +0530 send default USB controller in xml to destination during migration Maybe we can skip the migration back compat in certain cases as well. For example if mips PCI requires a pci-root controller to be manually specified in the XML (does it? or does libvirt add it by default?), then we probably want to skip that PCI check for mips, and maybe other archs too. Probably skip it in every case except those where libvirt adds a pci-root controller by default > >>> Signed-off-by: Aurelien Jarno >>> --- >>> src/qemu/qemu_domain.c | 15 ++- >>> src/qemu/qemu_domain.h | 1 + >>> 2 files changed, 15 insertions(+), 1 deletion(-) >>> >> >> The patch looks fine to me but should have some test suite representation, >> tests/qemuxml2argvtest at least > > Ok. I have tried to define a basic xml file similar to the > arm-vexpressa9-basic one. Unfortunately when running the testsuite I > get the following error: > > libvirt: Capabilities Utils error : invalid argument: could not find > capabilities for arch=mips > > I guess the capabilities for mips should be defined somewhere, but I > fail to see where. Can you please give me a hint? > See for example testQemuAddArmGuest in tests/testutilsqemu.c where this is set up for armv7l Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions
On Mon, May 09, 2016 at 09:20:15AM -0600, Eric Blake wrote: > On 05/06/2016 12:11 PM, Eduardo Habkost wrote: > > Extend query-cpu-definitions schema to allow it to return two new > > optional fields: "runnable" and "unavailable-features". > > "runnable" will tell if the CPU model can be run in the current > > host. "unavailable-features" will contain a list of CPU > > properties that are preventing the CPU model from running in the > > current host. > > > > Cc: David Hildenbrand > > Cc: Michael Mueller > > Cc: Christian Borntraeger > > Cc: Cornelia Huck > > Cc: Jiri Denemark > > Cc: libvir-list@redhat.com > > Signed-off-by: Eduardo Habkost > > --- > > qapi-schema.json | 10 +- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/qapi-schema.json b/qapi-schema.json > > index 54634c4..450e6e7 100644 > > --- a/qapi-schema.json > > +++ b/qapi-schema.json > > @@ -2948,11 +2948,19 @@ > > # Virtual CPU definition. > > # > > # @name: the name of the CPU definition > > +# @runnable: true if the CPU model is runnable using the current > > +#machine and accelerator. Optional. Since 2.6. > > You've missed 2.6. Also, the typical spelling for a per-member > designation is '(since 2.7)', not 'Since 2.7' Oops! I meant 2.7, and I didn't notice that it was not using the typical format. I will fix it, thanks. > > Why is it optional? Would it hurt to always be present in qemu new > enough to understand why it is needed? It is optional because not all architectures will return the field. This series implements it only for x86. -- Eduardo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH/RFC] Add missing delta from Ubuntu to apparmor profiles
Hi, > Stefan Bader wrote (20 May 2015 10:11:45 GMT) : > intrigeri wrote (15 Jun 2015 15:09:11 GMT) : > My (possibly incomplete) records say that I've tested the latest > proposed patch set back in February (<85iof8v6j5@boum.org>). >> Since I lost most context by now, I will try to find my most recent proposal >> again and try to get it moved into present state of packages. Ping? Cheers, -- intrigeri -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: add default pci-root device to mips*/malta guests
On 2016-05-04 09:54, Cole Robinson wrote: > Thanks for the patch. > > Can you provide a working mips XML config? We don't have one in the unit test > suite. > > On 05/03/2016 05:23 PM, Aurelien Jarno wrote: > > The MIPS Malta board has a root PCI controller. It is not sent during > > migration, so it needs to be added by default. > > > > I'm a bit confused by the migration reference... does specifying the PCI > controller on the command line somehow convince qemu to send the PCI device > state during migration? Or am I No it doesn't convince it to send the PCI device state. The problem is that the PCI root devices is not created at all on the target side. For what I understood this is due to the code in qemuDomainDefFormatBuf() dropping pci-root controller "for migration compatibility". This can be seen by using virsh managedsave followed by virsh start. The QEMU command line doesn't include the pci root controller, which causes the following error: "error: XML error: No PCI buses available". Indeed looking at the saved image, it is missing from the XML, and this actually not specific to MIPS. > > Signed-off-by: Aurelien Jarno > > --- > > src/qemu/qemu_domain.c | 15 ++- > > src/qemu/qemu_domain.h | 1 + > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > The patch looks fine to me but should have some test suite representation, > tests/qemuxml2argvtest at least Ok. I have tried to define a basic xml file similar to the arm-vexpressa9-basic one. Unfortunately when running the testsuite I get the following error: libvirt: Capabilities Utils error : invalid argument: could not find capabilities for arch=mips I guess the capabilities for mips should be defined somewhere, but I fail to see where. Can you please give me a hint? Thanks, Aurelien -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurel...@aurel32.net http://www.aurel32.net -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] NFS over AF_VSOCK in
On Mon, May 09, 2016 at 04:57:17PM +0100, Stefan Hajnoczi wrote: > virtio-vsock support has been added to the nfs-ganesha NFS server. I'm > currently working on upstreaming virtio-vsock into Linux and QEMU. I > also have patches for the Linux NFS client and server. > > Users wishing to share a file system with the guest will need to > configure the NFS server. Perhaps libvirt could handle that given that > it already has syntax. > > The basic task is setting up either the kernel nfsd or nfs-ganesha for > the VM to access the NFS export(s). When the VM is destroy the NFS > server can be shut down. Can you elaborate on the interaction between QEMU and the NFS server on the host ? What actually needed changing in nfs-ganesha to support virtio-vsock ? I thought that on the host side we wouldn't need any changes, because QEMU would just talk to a regular NFS server over TCP, and the only virtio-vsock changes would be in QEMU and the guest kernel. > Does this sound like a responsibility that libvirt should handle? Certainly, we'll want to have libvirt manage host side setup tasks that may be required, not least to control security policy wrt VMs accessing filesystem exports. 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] NFS over AF_VSOCK in
virtio-vsock support has been added to the nfs-ganesha NFS server. I'm currently working on upstreaming virtio-vsock into Linux and QEMU. I also have patches for the Linux NFS client and server. Users wishing to share a file system with the guest will need to configure the NFS server. Perhaps libvirt could handle that given that it already has syntax. The basic task is setting up either the kernel nfsd or nfs-ganesha for the VM to access the NFS export(s). When the VM is destroy the NFS server can be shut down. Does this sound like a responsibility that libvirt should handle? Stefan signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/8] Revert "conf: Validate disk lun using correct types"
On 05/09/2016 04:59 AM, Peter Krempa wrote: > On Fri, May 06, 2016 at 11:19:05 -0400, John Ferlan wrote: >> >> >> On 05/02/2016 10:32 AM, Peter Krempa wrote: >>> This reverts commit c79ebf53b5fe0a33bf407b3bcb49e3a27ec97eb4. >>> >>> We can't just add checks to the XML parser once we've accepted such >>> configuration in the past. >>> --- >>> src/conf/domain_conf.c | 22 -- >>> tests/qemuxml2argvtest.c | 3 +-- >>> 2 files changed, 1 insertion(+), 24 deletions(-) >>> >> >> There was a bz associated with that commit - that'll need to be >> addressed in some manner... > > Well, the initial assesment of that BZ was wrong. This should have been > fixed in virt manager at that point. > That bz: https://bugzilla.redhat.com/show_bug.cgi?id=1201143 Is loaded with GSS and other 'priority' tags. Maybe the original assessment of the bug is wrong. But just reverting the commit with little mention of that until John dug it up is not helpful IMO >> While I understand your point here, the configuration didn't work - that >> is it couldn't be started anyway so there could not be a domain running >> with that configuration and thus it wouldn't disappear on a subsequent >> reload, hence why checking the config and rejecting "earlier" seemed > > It does not kill any running domain, that's right. Defined domains still > vanish after that commit if they were defined before. That is still > unwanted. > Yes, this is problematic, but then again has this issue bitten us in the year since this was committed? We should still fix it, but it's not time critical... Maybe we come up with a better solution. >> proper even though we hadn't rejected such a config when the >> "mode='host'" was first implemented. > > Only when introducing a feature you are allowed to do a check that > rejects parsing XML, afterwards, no such thins should be added. > Right, these rules make technical sense, but are extremely difficult to audit, and have proven hard to enforce. People wandering into the code may follow the conventions of the code around them, and have no idea that adding a new validation check is 'bad' when the pre-existing similar checks are 'good' strictly based one when they were added. I think we need a better framework for this. I'll probably send a larger mail at some point, but basically I think we should: - rename VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS to something generic like VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATION. The original flag is used at libvirtd startup time to avoid the 'disappearing domain' problem for when a qemu is uninstalled for example, but we still get that validation check for normal runtime XML define - Make an explicit virDomainDefValidate function and move the OSTYPE validation there. - Only run virDomainDefValidate if VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATION is _not_ set. So skip validation at libvirtd startup - Then for example we can move this block of code to that function Doing all that, at least to cover this particular check, shouldn't be much work, won't potentially revive the bug, _and_ it sets us up some nice infrastructure to avoid this type of problem in the future. Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions
On 05/06/2016 12:11 PM, Eduardo Habkost wrote:
> Extend query-cpu-definitions schema to allow it to return two new
> optional fields: "runnable" and "unavailable-features".
> "runnable" will tell if the CPU model can be run in the current
> host. "unavailable-features" will contain a list of CPU
> properties that are preventing the CPU model from running in the
> current host.
>
> Cc: David Hildenbrand
> Cc: Michael Mueller
> Cc: Christian Borntraeger
> Cc: Cornelia Huck
> Cc: Jiri Denemark
> Cc: libvir-list@redhat.com
> Signed-off-by: Eduardo Habkost
> ---
> qapi-schema.json | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 54634c4..450e6e7 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2948,11 +2948,19 @@
> # Virtual CPU definition.
> #
> # @name: the name of the CPU definition
> +# @runnable: true if the CPU model is runnable using the current
> +#machine and accelerator. Optional. Since 2.6.
You've missed 2.6. Also, the typical spelling for a per-member
designation is '(since 2.7)', not 'Since 2.7'
Why is it optional? Would it hurt to always be present in qemu new
enough to understand why it is needed?
> +# @unavailable-features: List of properties that prevent the CPU
> +#model from running in the current host,
> +#if @runnable is false. Optional.
> +#Since 2.6.
> #
> # Since: 1.2.0
> ##
> { 'struct': 'CpuDefinitionInfo',
> - 'data': { 'name': 'str' } }
> + 'data': { 'name': 'str',
> +'*runnable': 'bool',
> +'*unavailable-features': [ 'str' ] } }
>
> ##
> # @query-cpu-definitions:
>
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 25/27] fdstream: Suppress use of IO helper for sparse streams
On 05.05.2016 17:51, Eric Blake wrote: > On 05/05/2016 09:32 AM, Daniel P. Berrange wrote: >> On Thu, Apr 28, 2016 at 12:05:12PM +0200, Michal Privoznik wrote: >>> This is kind of a hacky approach to the following problem, but so >>> far I am unable to come up with anything better. On some >>> occasions (esp. when dealing with regular files) libvirt_iohelper >>> is spawned to prefetch data for us. We will then have a pipe then >>> for reading the data from it. This does not fit in our sparse >>> stream implementation as one simply doesn't lseek() over a pipe. >>> Until this is resolved, let's suppress use of the IO helper and >>> read data from FD directly. >> >> This doesn't really fly - the problem is that with regular files, >> poll() on the FD will always return ready, even if the read or >> write will block in I/O. So by nomt using the iohelper this is >> going to cause our main loop to block on I/O for streams. > > The only real solution is to teach libvirt_iohelper to do structured > reads when requested. That is, you'll have to add a command-line flag > to libvirt_iohelper, which if present, says all of the output from > libvirt_iohelper will be structured as tuples of either > or of . When used in this > mode, the client HAS to parse the tuples, rather than assuming that the > pipe can be read literally. So that means we also have to teach the > consumer of libvirt_iohelper how to read tuples off the pipe, at which > point it then knows whether to send a regular VIR_NET_STREAM or the > compact VIR_NET_STREAM_SKIP. > I know. I had this approach in my mind. But before spending any time on it, I wanted to make sure my design of sparse streams is good. Moreover, this patch set is long enough jut now. My plan was to implement this approach as soon as this patch set it merged so that we can enable the sparse streams. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Introducing myself (GSoC 2016 student)
On 05/09/2016 08:42 AM, Katerina Koukiou wrote: > Hi, > my name is Katerina Koukiou and I got selected as a GSoC student this year > to work on libvirt. My project is to add libvirt support for migration of > lxc containers using a tool called CRIU. > For the time, I am in my final year of undergraduate studies in Electrical and > Computer Engineering, at NTUA university, Greece. > Looking forward to share some patches here! > Katerina Welcome Katerina! - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] domaincaps: Report graphics spiceGL
On 09.05.2016 13:30, Pavel Hrdina wrote: > On Sun, May 08, 2016 at 01:49:06PM -0400, Cole Robinson wrote: >> Reports a tristate enum value for acceptable graphics type=spice >> . >> >> Wire it up for qemu too. 'no' is always a valid value, so we >> unconditionally report it. >> --- > > [...] > >> diff --git a/tests/domaincapsschemadata/domaincaps-full.xml >> b/tests/domaincapsschemadata/domaincaps-full.xml >> index 2f529ff..4eb5637 100644 >> --- a/tests/domaincapsschemadata/domaincaps-full.xml >> +++ b/tests/domaincapsschemadata/domaincaps-full.xml >> @@ -47,6 +47,11 @@ >> desktop >> spice >> >> + >> +default >> +yes >> +no >> + > > Ewww, this doesn't look good and I agree with Michal that what if we add > support for another graphics device and what if one graphics device will have > more than one capability? > > I would suggest something like this: > > > > spice > vnc > sdl > I think we should have this enum in as it's easier for mgmt apps to create XPath to query for supported values. In the case below they would need to special case between supported graphics and the rest. That's why I ACKed 1/5. > > > I like this. The only problem I see with this approach is when we would want to express multidimensional values. I mean what if would not depend just on graphics type='spice' but also on another attribute having specific value? It is more visible in a general case: feature C can be enabled iff A && B: How would this be represented in domcaps? > > > or even do something like this: > > > > > > > > > Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/27] Introduce sparse streams
On Mon, May 09, 2016 at 04:43:44PM +0200, Michal Privoznik wrote:
> On 05.05.2016 18:10, Daniel P. Berrange wrote:
> > On Thu, Apr 28, 2016 at 12:04:47PM +0200, Michal Privoznik wrote:
> >>
> >
> > While it looks ok-ish in the context of the RecvAll function,
> > because you have skipFunc + recvFunc both being invoked
> > asynchronously, this design feels a quite odd when used
> > in combination with the plain virStreamRecv().
> >
> > Just for a minute lets consider
> >
> > st = virStreamNew(conn, 0);
> > virStreamRegisterSkip(st, skipFunc, &fd);
> > virStorageVolDownload(st, ...);
> >
> > while (1) {
> > char buf[4096];
> > virStreamRecv(st, buf, sizeof(buf));
> >
> > ..do something with buf..
> > }
> >
> > I think it is quite unpleasant to have the skipFunc be called
> > out of band to the code dealing with the Recv.
> >
> >
> >
> > I think it is preferrable that we have an explicit synchronous
> > API for consuming the hole.
> >
> > The same really applies from POV of the upload side - I think
> > we should be able to synchronously push the hole in, rather
> > than having the asynchronous InData callback.
> >
> > IIUC, the reason you've chosen this async callback approach
> > is because the virStream{Send,Recv,SendAll,RecvAll} methods
> > are not extensible to cope with holes.
> >
> >
> > I'm thinking though that we'd get a more attractive public API
> > by creating hole friendly versions of virStream{Send,Recv,etc}
> > instead of going the extra callback route.
> >
> >
> > The download side could we made to work if we had a flag for
> > virStreamRecv that instructed it to only read data up to the
> > next hole, and defined an explicit error code to use to indicate
> > that we've hit a hole.
> >
> > This would be used thus:
> >
> > while (1) {
> > char buf[4096];
> >
> > int ret = virStreamRecvFlags(st, buf, len, VIR_STREAM_STOP_AT_HOLE);
> > if (ret == -1) {
> > virErrorPtr err = virGetLastError();
> > if (err.code == VIR_ERR_STREAM_HOLE) {
> >ret = virStreamHoleSize(st, buf);
>
> Could this be made even more friendlier by returning a special value
> (say -2) in case of a hole?
>
> >...seek ret bytes in target...
> > } else {
> >return -1;
> > }
> > } else {
> > ...write buf to target...
> > }
> > }
> >
>
> Okay, so imagine we have STREAM_SKIP packet incoming what should happen
> if it is processed by bare virStreamRead()? IOW user requests sparse
> streams but sticks to calling old virStreamRecv().
Given that the app has to pass in the flag VIR_STORAGE_VOL_UPLOAD_SPARSE_STREAM
(or similar), we could declare that it is an error to use the plain
virStreamRecv
when you have already asked for a sparse stream.
Alternatively, we could just make virStreamRecv give back a buffer
full of zeros :-)
> One thing that I'm worried here about is that @flags of
> virStreamRecvFlags() would not be transferred to the sender side as one
> might expect.
Yep, that is true, but then everything about the stream APIs is
a bit different from the rest of the API.
> > To make this work with virStreamRecvAll, we would need to add
> > a virStreamSparseRecvAll() which had two callbacks eg
> >
> > typedef int (*virStreamSinkHoleFunc)(virStreamPtr st, unsigned long long
> > len):
> >
> > virStreamSparseRecvAll(virStreamPtr stream,
> > virStreamSinkFunc handler,
> > virStreamSinkHoleFunc holeHandler,
> > void *opaque) {
> > while (1) {
> > char buf[4096];
> >
> > int ret = virStreamRecvFlags(st, buf, len, VIR_STREAM_STOP_AT_HOLE);
> > if (ret == -1) {
> > virErrorPtr err = virGetLastError();
> > if (err.code == VIR_ERR_STREAM_HOLE) {
> >ret = virStreamHoleSize(st, buf);
> >holeHandler(st, ret);
> > } else {
> >return -1;
> > }
> > } else {
> > handler(st, buf, ret);
> > }
> >}
> > }
> >
> >
> > Now considering the upload side of things, the virStreamSend
> > function doesn't actually need cahnges, though it could do
> > with a flags parameter for best practice. We just need the
> > virStreamSkip() function you already add.
> >
> > while (1) {
> > char buf[4096];
> > if (..in hole...) {
> >..get hole size...
> >virStreamSkip(st, len);
> > } else {
> >...read N bytes...
> >virStreamSend(st, buf, len);
> > }
> > }
> >
> >
> > The SendAll method is again more complicated as the callback we pass
> > into it is insufficient to figure out if we have holes. We could
> > add a virStreamSparseSendAll() which had two callbacks again:
> >
> > typedef int (*virStreamSourceHoleFunc)(holeHandler);
> >
> > This returns the length of the current hole, or 0 if not at
> > a hole.
> >
> > virStreamSparseSendAll(virStreamPtr stream,
> > virStreamSourceFunc handler,
> >
Re: [libvirt] [PATCH 25/27] fdstream: Suppress use of IO helper for sparse streams
On 05.05.2016 18:12, Daniel P. Berrange wrote: > On Thu, May 05, 2016 at 09:51:22AM -0600, Eric Blake wrote: >> On 05/05/2016 09:32 AM, Daniel P. Berrange wrote: >>> On Thu, Apr 28, 2016 at 12:05:12PM +0200, Michal Privoznik wrote: This is kind of a hacky approach to the following problem, but so far I am unable to come up with anything better. On some occasions (esp. when dealing with regular files) libvirt_iohelper is spawned to prefetch data for us. We will then have a pipe then for reading the data from it. This does not fit in our sparse stream implementation as one simply doesn't lseek() over a pipe. Until this is resolved, let's suppress use of the IO helper and read data from FD directly. >>> >>> This doesn't really fly - the problem is that with regular files, >>> poll() on the FD will always return ready, even if the read or >>> write will block in I/O. So by nomt using the iohelper this is >>> going to cause our main loop to block on I/O for streams. >> >> The only real solution is to teach libvirt_iohelper to do structured >> reads when requested. That is, you'll have to add a command-line flag >> to libvirt_iohelper, which if present, says all of the output from >> libvirt_iohelper will be structured as tuples of either >> or of . When used in this >> mode, the client HAS to parse the tuples, rather than assuming that the >> pipe can be read literally. So that means we also have to teach the >> consumer of libvirt_iohelper how to read tuples off the pipe, at which >> point it then knows whether to send a regular VIR_NET_STREAM or the >> compact VIR_NET_STREAM_SKIP. > > Yeah, that doesn't sound too bad - its rather similar to the HTTP > chunked encoding idea. It isn't much extra overhead to have a > type + len field in the byte stream, as long as we put a sensible > min size on the holes we transmit. eg don't send a hole that's > less than 512 bytes in len. That wouldn't be even possible on filesystem level. The smallest hole there can be is block size. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/27] Introduce virStreamSkip
On 05.05.2016 16:37, Eric Blake wrote: > On 04/28/2016 04:04 AM, Michal Privoznik wrote: >> This API can be used to tell the other side of the stream to skip >> some bytes in the stream. This can be used to create a sparse >> file on the receiving side of a stream. >> >> It takes just one argument @offset, which says how big the hole >> is. Since our streams are not rewindable like regular files, we >> don't need @whence argument like seek(2) has. >> >> Signed-off-by: Michal Privoznik >> --- >> include/libvirt/libvirt-stream.h | 3 +++ >> src/driver-stream.h | 5 + >> src/libvirt-stream.c | 40 >> >> src/libvirt_public.syms | 5 + >> 4 files changed, 53 insertions(+) >> > >> +++ b/src/libvirt-stream.c >> @@ -286,6 +286,46 @@ virStreamRecv(virStreamPtr stream, >> >> >> /** >> + * virStreamSkip: >> + * @stream: pointer to the stream object >> + * @offset: number of bytes to skip >> + * >> + * Skip @offset bytes in the stream. This is useful when there's >> + * no actual data in the stream, just a hole. If that's the case, >> + * this API can be used to skip the hole properly instead of >> + * transmitting zeroes to the other side. >> + * >> + * Returns 0 on success, >> + *-1 error >> + */ >> +int >> +virStreamSkip(virStreamPtr stream, >> + unsigned long long offset) > > 'offset' is a bit misleading - you're not skipping _to_ the given > offset, so much as _over_ length bytes. I'd name it 'length'. > > Otherwise looks okay. > Ah, good point. Whilst implementing this I've balanced between Seek and Skip back and forth. That's why I call streem seekable even if it's really just skip what is implemented here. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/27] Introduce virStreamRegisterSkip and virStreamSkipCallback
On 05.05.2016 16:49, Eric Blake wrote:
> On 04/28/2016 04:04 AM, Michal Privoznik wrote:
>> The former is a public API and registers a callback that
>> will be called whenever the other side of a stream calls
>> virStreamSkip. The latter is a wrapper that actually calls
>> the callback. It is not made public as it is intended to be
>> used purely internally.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>
>> +/**
>> + * virStreamSkipFunc:
>> + * @stream: stream
>> + * @offset: size of hole in bytes
>
> Again, naming this 'length' makes more sense (you're not skipping TO a
> particular offset, but OVER a given length).
>
>> +++ b/src/libvirt-stream.c
>> @@ -286,6 +286,76 @@ virStreamRecv(virStreamPtr stream,
>>
>>
>> /**
>> + * virStreamRegisterSkip:
>> + * @stream: stream
>> + * @skipCb: callback function
>> + * @opaque: optional application provided data
>> + *
>> + * This function registers callback that will be called whenever
>
> s/callback/a callback/
>
>> + * the other side of the @stream is willing to skip a hole in the
>> + * stream.
>> + *
>> + * Returns 0 on success,
>> + *-1 otherwise.
>> + */
>> +int
>> +virStreamRegisterSkip(virStreamPtr stream,
>> + virStreamSkipFunc skipCb,
>> + void *opaque)
>> +{
>> +VIR_DEBUG("stream=%p, skipCb=%p opaque=%p", stream, skipCb, opaque);
>> +
>> +virResetLastError();
>> +
>> +virCheckStreamReturn(stream, -1);
>> +virCheckNonNullArgReturn(skipCb, -1);
>> +
>> +if (stream->skipCb) {
>> +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> + _("A skip callback is already registered"));
>> +return -1;
>
> I guess that means we allow passing skipCb=NULL to deregister a
> callback; does it need to be specifically documented? Are there
> scenarios where you WANT to deregister before closing something else, to
> make sure that a stale callback is not called during a race scenario?
I haven't thought of that. I mean, the line just above this check
prohibits skipCB being NULL. But it seems like this will be thrown away
anyway.
>
>> +int
>> +virStreamSkipCallback(virStreamPtr stream,
>> + unsigned long long offset)
>> +{
>> +VIR_DEBUG("stream=%p, offset=%llu", stream, offset);
>> +
>> +virCheckStreamReturn(stream, -1);
>> +
>> +if (stream->skipCb) {
>> +int ret;
>> +ret = (stream->skipCb)(stream, offset, stream->skipCbOpaque);
>
> I might have omitted the () around stream->skipCb, but I don't know if
> we have a consistent style, and yours makes it obvious that we know we
> are dereferencing a function pointer.
>
>> +++ b/src/libvirt_public.syms
>> @@ -735,6 +735,7 @@ LIBVIRT_1.3.3 {
>> LIBVIRT_1.3.5 {
>> global:
>> virStreamSkip;
>> +virStreamRegisterSkip;
>
> Worth keeping sorted?
>
Sure.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/27] Introduce sparse streams
On 05.05.2016 18:10, Daniel P. Berrange wrote:
> On Thu, Apr 28, 2016 at 12:04:47PM +0200, Michal Privoznik wrote:
>>
>
> While it looks ok-ish in the context of the RecvAll function,
> because you have skipFunc + recvFunc both being invoked
> asynchronously, this design feels a quite odd when used
> in combination with the plain virStreamRecv().
>
> Just for a minute lets consider
>
> st = virStreamNew(conn, 0);
> virStreamRegisterSkip(st, skipFunc, &fd);
> virStorageVolDownload(st, ...);
>
> while (1) {
> char buf[4096];
> virStreamRecv(st, buf, sizeof(buf));
>
> ..do something with buf..
> }
>
> I think it is quite unpleasant to have the skipFunc be called
> out of band to the code dealing with the Recv.
>
>
>
> I think it is preferrable that we have an explicit synchronous
> API for consuming the hole.
>
> The same really applies from POV of the upload side - I think
> we should be able to synchronously push the hole in, rather
> than having the asynchronous InData callback.
>
> IIUC, the reason you've chosen this async callback approach
> is because the virStream{Send,Recv,SendAll,RecvAll} methods
> are not extensible to cope with holes.
>
>
> I'm thinking though that we'd get a more attractive public API
> by creating hole friendly versions of virStream{Send,Recv,etc}
> instead of going the extra callback route.
>
>
> The download side could we made to work if we had a flag for
> virStreamRecv that instructed it to only read data up to the
> next hole, and defined an explicit error code to use to indicate
> that we've hit a hole.
>
> This would be used thus:
>
> while (1) {
> char buf[4096];
>
> int ret = virStreamRecvFlags(st, buf, len, VIR_STREAM_STOP_AT_HOLE);
> if (ret == -1) {
> virErrorPtr err = virGetLastError();
> if (err.code == VIR_ERR_STREAM_HOLE) {
> ret = virStreamHoleSize(st, buf);
Could this be made even more friendlier by returning a special value
(say -2) in case of a hole?
> ...seek ret bytes in target...
> } else {
> return -1;
> }
> } else {
> ...write buf to target...
> }
> }
>
Okay, so imagine we have STREAM_SKIP packet incoming what should happen
if it is processed by bare virStreamRead()? IOW user requests sparse
streams but sticks to calling old virStreamRecv().
One thing that I'm worried here about is that @flags of
virStreamRecvFlags() would not be transferred to the sender side as one
might expect.
>
> To make this work with virStreamRecvAll, we would need to add
> a virStreamSparseRecvAll() which had two callbacks eg
>
> typedef int (*virStreamSinkHoleFunc)(virStreamPtr st, unsigned long long
> len):
>
> virStreamSparseRecvAll(virStreamPtr stream,
> virStreamSinkFunc handler,
> virStreamSinkHoleFunc holeHandler,
> void *opaque) {
> while (1) {
> char buf[4096];
>
> int ret = virStreamRecvFlags(st, buf, len, VIR_STREAM_STOP_AT_HOLE);
> if (ret == -1) {
> virErrorPtr err = virGetLastError();
> if (err.code == VIR_ERR_STREAM_HOLE) {
> ret = virStreamHoleSize(st, buf);
> holeHandler(st, ret);
> } else {
> return -1;
> }
> } else {
> handler(st, buf, ret);
> }
>}
> }
>
>
> Now considering the upload side of things, the virStreamSend
> function doesn't actually need cahnges, though it could do
> with a flags parameter for best practice. We just need the
> virStreamSkip() function you already add.
>
> while (1) {
> char buf[4096];
> if (..in hole...) {
>..get hole size...
>virStreamSkip(st, len);
> } else {
>...read N bytes...
>virStreamSend(st, buf, len);
> }
> }
>
>
> The SendAll method is again more complicated as the callback we pass
> into it is insufficient to figure out if we have holes. We could
> add a virStreamSparseSendAll() which had two callbacks again:
>
> typedef int (*virStreamSourceHoleFunc)(holeHandler);
>
> This returns the length of the current hole, or 0 if not at
> a hole.
>
> virStreamSparseSendAll(virStreamPtr stream,
> virStreamSourceFunc handler,
> virStreamSourceHoleFunc holeHandler,
> void *opaque)
>while (1) {
> char buf[4096];
> int ret = holeHandler(st);
> if (ret > 0) {
> virStreamSkip(st, ret);
> } else {
> ret = handler(st, buf, sizeof(buf);
> virStreamSend(st, buf, ret);
> }
>}
> }
>
>
> So we would avoid the virStreamInData and virStreamRegisterInData
> and virStreamRegisterSkip and virStreamSkipCallback methods
Okay. Makes sense. Let me see if I could reuse my RPC implementation,
because that had been the hardest part to implement of them all.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listin
Re: [libvirt] [PATCH 3/5] domaincaps: Report graphics spiceGL
On Mon, May 09, 2016 at 09:30:30AM -0400, Cole Robinson wrote: > On 05/09/2016 07:30 AM, Pavel Hrdina wrote: > > On Sun, May 08, 2016 at 01:49:06PM -0400, Cole Robinson wrote: > >> Reports a tristate enum value for acceptable graphics type=spice > >> . > >> > >> Wire it up for qemu too. 'no' is always a valid value, so we > >> unconditionally report it. > >> --- > > > > [...] > > > >> diff --git a/tests/domaincapsschemadata/domaincaps-full.xml > >> b/tests/domaincapsschemadata/domaincaps-full.xml > >> index 2f529ff..4eb5637 100644 > >> --- a/tests/domaincapsschemadata/domaincaps-full.xml > >> +++ b/tests/domaincapsschemadata/domaincaps-full.xml > >> @@ -47,6 +47,11 @@ > >> desktop > >> spice > >> > >> + > >> +default > >> +yes > >> +no > >> + > > > > Ewww, this doesn't look good and I agree with Michal that what if we add > > support for another graphics device and what if one graphics device will > > have > > more than one capability? > > > > I would suggest something like this: > > > > > > > > spice > > vnc > > sdl > > > > > > > > > > > > > > or even do something like this: > > > > > > > > > > > > > > > > > > > > Really wish someone would have chimed in with this to my (unresponded) RFC > email about these things... > > http://www.redhat.com/archives/libvir-list/2016-April/msg01839.html > > That pattern is fine in this case, but what about the case of ports= element > that is only supported with a controller type=usb model=nec-xhci ? Do we have > new XML like: > > > > > > > > > > It seems like these types of relationships could grow deeply nested, so I > opted for following the limited existing convention of adding unique enum > names to represent the relationship. Yeah, I find flat modelling quite desirable, because the relationships between attributes will certainly grow quite complicate, and they do not neccessarily form a nice simple tree relationship. ie, whether foo=bar is permitted may depend on whether wizz=eek *AND* when lala=ooh, and you can't have the enum for 'foo' nested underneath the enum for 'wizz' & 'lala' at the same time. Also with nesting, if you want the same values reported for multiple options, we end up repeating the same information multiple times which is not good. At the same time the flat modelling with "spiceGL" also doesn't scale as you have to invent new names for each one, and again it doesn't work if the 'gl' enum depended on type="spice" *and* something else at the same time. So I think we need a more flexible way to express dependancies here. We could let the be repeated multiple times and express conditional rules against each instance of the enum So for example spice vnc sdl default yes no default no This would express that the first entry only applies when the @type == spice. If that doesn't match them fallback to the second . The nice thing about this is that we can make the conditions arbitrarly complex For example: spice vnc sdl default yes no default no This shows how the "gl" option can be used with VNC, but only if you also have the @wibble attribute set to "eek". Of course complex conditions like this become quite tedious & verbose so obviously we should strive to keep them as simple as possible and not use nested conditions unless absolutely needed. 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 3/4] qemu_process: merge graphics code into qemuProcessSetupGraphics
Signed-off-by: Pavel Hrdina
---
src/qemu/qemu_process.c | 45 ++---
1 file changed, 14 insertions(+), 31 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 3d46695..eddf3a7 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4402,16 +4402,21 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
for (i = 0; i < vm->def->ngraphics; ++i) {
virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
+char *listenAddr = NULL;
switch (graphics->type) {
case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
if (qemuProcessVNCAllocatePorts(driver, graphics, allocate) < 0)
goto cleanup;
+
+listenAddr = cfg->vncListen;
break;
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
if (qemuProcessSPICEAllocatePorts(driver, cfg, graphics, allocate)
< 0)
goto cleanup;
+
+listenAddr = cfg->spiceListen;
break;
case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
@@ -4420,6 +4425,14 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
break;
}
+
+if (graphics->nListens == 0 && listenAddr) {
+if (virDomainGraphicsListenAppendAddress(graphics,
+ listenAddr) < 0)
+goto cleanup;
+
+graphics->listens[0].fromConfig = true;
+}
}
ret = 0;
@@ -5177,40 +5190,10 @@ qemuProcessPrepareDomain(virConnectPtr conn,
if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0)
goto cleanup;
-VIR_DEBUG("Setting up ports for graphics");
+VIR_DEBUG("Setting graphics devices");
if (qemuProcessSetupGraphics(driver, vm, flags) < 0)
goto cleanup;
-/* Fill in run-time values for graphics devices. */
-for (i = 0; i < vm->def->ngraphics; i++) {
-virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
-char *listenAddr = NULL;
-
-switch (graphics->type) {
-case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
-listenAddr = cfg->vncListen;
-break;
-
-case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
-listenAddr = cfg->spiceListen;
-break;
-
-case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
-case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
-case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
-case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
-break;
-}
-
-if (graphics->nListens == 0 && listenAddr) {
-if (virDomainGraphicsListenAppendAddress(graphics,
- listenAddr) < 0)
-goto cleanup;
-
-graphics->listens[0].fromConfig = true;
-}
-}
-
/* "volume" type disk's source must be translated before
* cgroup and security setting.
*/
--
2.8.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/4] some graphics cleanups and improvements
Pavel Hrdina (4): qemu_process: separate graphics port reservation graphics: generate fake ports also for tests qemu_process: merge graphics code into qemuProcessSetupGraphics qemu_hotplug: fix checking graphics ports src/qemu/qemu_driver.c | 12 --- src/qemu/qemu_hotplug.c| 13 ++-- src/qemu/qemu_process.c| 86 +++--- src/qemu/qemu_process.h| 8 -- .../qemuhotplug-graphics-spice-listen-network.xml | 2 +- .../qemuxml2argv-controller-order.args | 2 +- .../qemuxml2argv-hugepages-numa.args | 2 +- .../qemuxml2argv-video-device-pciaddr-default.args | 2 +- 8 files changed, 51 insertions(+), 76 deletions(-) -- 2.8.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/4] qemu_hotplug: fix checking graphics ports
We cannot change ports for running domain and we should error out if
autoport is enabled.
Signed-off-by: Pavel Hrdina
---
src/qemu/qemu_hotplug.c | 13 +
.../qemuhotplug-graphics-spice-listen-network.xml | 2 +-
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 07b419d..f40b34d 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2666,9 +2666,8 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver,
switch (dev->type) {
case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
-if ((olddev->data.vnc.autoport != dev->data.vnc.autoport) ||
-(!dev->data.vnc.autoport &&
- (olddev->data.vnc.port != dev->data.vnc.port))) {
+if (olddev->data.vnc.autoport != dev->data.vnc.autoport ||
+olddev->data.vnc.port != dev->data.vnc.port) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("cannot change port settings on vnc graphics"));
goto cleanup;
@@ -2710,11 +2709,9 @@ qemuDomainChangeGraphics(virQEMUDriverPtr driver,
break;
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
-if ((olddev->data.spice.autoport != dev->data.spice.autoport) ||
-(!dev->data.spice.autoport &&
- (olddev->data.spice.port != dev->data.spice.port)) ||
-(!dev->data.spice.autoport &&
- (olddev->data.spice.tlsPort != dev->data.spice.tlsPort))) {
+if (olddev->data.spice.autoport != dev->data.spice.autoport ||
+olddev->data.spice.port != dev->data.spice.port ||
+olddev->data.spice.tlsPort != dev->data.spice.tlsPort) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("cannot change port settings on spice graphics"));
goto cleanup;
diff --git
a/tests/qemuhotplugtestdata/qemuhotplug-graphics-spice-listen-network.xml
b/tests/qemuhotplugtestdata/qemuhotplug-graphics-spice-listen-network.xml
index 426a14d..f2a6aeb 100644
--- a/tests/qemuhotplugtestdata/qemuhotplug-graphics-spice-listen-network.xml
+++ b/tests/qemuhotplugtestdata/qemuhotplug-graphics-spice-listen-network.xml
@@ -1,4 +1,4 @@
-
+
--
2.8.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] qemu_process: separate graphics port reservation
Signed-off-by: Pavel Hrdina
---
src/qemu/qemu_process.c | 27 ---
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 22b27b3..f110a66 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4348,12 +4348,10 @@ qemuProcessStartHook(virQEMUDriverPtr driver,
static int
-qemuProcessSetupGraphics(virQEMUDriverPtr driver,
- virDomainObjPtr vm)
+qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver,
+virDomainObjPtr vm)
{
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
size_t i;
-int ret = -1;
for (i = 0; i < vm->def->ngraphics; ++i) {
virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
@@ -4362,7 +4360,7 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
if (virPortAllocatorSetUsed(driver->remotePorts,
graphics->data.vnc.port,
true) < 0)
-goto cleanup;
+return -1;
graphics->data.vnc.portReserved = true;
} else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
@@ -4371,7 +4369,7 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
if (virPortAllocatorSetUsed(driver->remotePorts,
graphics->data.spice.port,
true) < 0)
-goto cleanup;
+return -1;
graphics->data.spice.portReserved = true;
}
@@ -4379,12 +4377,27 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
if (virPortAllocatorSetUsed(driver->remotePorts,
graphics->data.spice.tlsPort,
true) < 0)
-goto cleanup;
+return -1;
graphics->data.spice.tlsPortReserved = true;
}
}
}
+return 0;
+}
+
+
+static int
+qemuProcessSetupGraphics(virQEMUDriverPtr driver,
+ virDomainObjPtr vm)
+{
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+size_t i;
+int ret = -1;
+
+if (qemuProcessGraphicsReservePorts(driver, vm) < 0)
+goto cleanup;
+
for (i = 0; i < vm->def->ngraphics; ++i) {
virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
--
2.8.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/4] graphics: generate fake ports also for tests
Signed-off-by: Pavel Hrdina
---
src/qemu/qemu_driver.c | 12
src/qemu/qemu_process.c| 22 --
src/qemu/qemu_process.h| 8
.../qemuxml2argv-controller-order.args | 2 +-
.../qemuxml2argv-hugepages-numa.args | 2 +-
.../qemuxml2argv-video-device-pciaddr-default.args | 2 +-
6 files changed, 15 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b0cfb10..c4c4968 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7060,18 +7060,6 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr
conn,
net->mac = mac;
}
-/* do fake auto-alloc of graphics ports, if such config is used */
-for (i = 0; i < vm->def->ngraphics; ++i) {
-virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
-if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
-if (qemuProcessVNCAllocatePorts(driver, graphics, false) < 0)
-goto cleanup;
-} else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
-if (qemuProcessSPICEAllocatePorts(driver, cfg, graphics, false) <
0)
-goto cleanup;
-}
-}
-
if (!(cmd = qemuProcessCreatePretendCmd(conn, driver, vm, NULL,
qemuCheckFips(), true,
VIR_QEMU_PROCESS_START_COLD)))
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index f110a66..3d46695 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3856,7 +3856,7 @@ qemuProcessReconnectAll(virConnectPtr conn,
virQEMUDriverPtr driver)
virDomainObjListForEach(driver->domains, qemuProcessReconnectHelper,
&data);
}
-int
+static int
qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
virDomainGraphicsDefPtr graphics,
bool allocate)
@@ -3888,7 +3888,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
return 0;
}
-int
+static int
qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
virQEMUDriverConfigPtr cfg,
virDomainGraphicsDefPtr graphics,
@@ -4389,13 +4389,15 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver,
static int
qemuProcessSetupGraphics(virQEMUDriverPtr driver,
- virDomainObjPtr vm)
+ virDomainObjPtr vm,
+ unsigned int flags)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+bool allocate = !(flags & VIR_QEMU_PROCESS_START_PRETEND);
size_t i;
int ret = -1;
-if (qemuProcessGraphicsReservePorts(driver, vm) < 0)
+if (allocate && qemuProcessGraphicsReservePorts(driver, vm) < 0)
goto cleanup;
for (i = 0; i < vm->def->ngraphics; ++i) {
@@ -4403,12 +4405,12 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
switch (graphics->type) {
case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
-if (qemuProcessVNCAllocatePorts(driver, graphics, true) < 0)
+if (qemuProcessVNCAllocatePorts(driver, graphics, allocate) < 0)
goto cleanup;
break;
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
-if (qemuProcessSPICEAllocatePorts(driver, cfg, graphics, true) < 0)
+if (qemuProcessSPICEAllocatePorts(driver, cfg, graphics, allocate)
< 0)
goto cleanup;
break;
@@ -5175,6 +5177,10 @@ qemuProcessPrepareDomain(virConnectPtr conn,
if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0)
goto cleanup;
+VIR_DEBUG("Setting up ports for graphics");
+if (qemuProcessSetupGraphics(driver, vm, flags) < 0)
+goto cleanup;
+
/* Fill in run-time values for graphics devices. */
for (i = 0; i < vm->def->ngraphics; i++) {
virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
@@ -5313,10 +5319,6 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
VIR_DEBUG("Ensuring no historical cgroup is lying around");
qemuRemoveCgroup(vm);
-VIR_DEBUG("Setting up ports for graphics");
-if (qemuProcessSetupGraphics(driver, vm) < 0)
-goto cleanup;
-
if (virFileMakePath(cfg->logDir) < 0) {
virReportSystemError(errno,
_("cannot create log directory %s"),
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 9e2e036..623e1e7 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -175,14 +175,6 @@ bool qemuProcessAutoDestroyActive(virQEMUDriverPtr driver,
int qemuProcessSetSchedParams(int id, pid_t pid, size_t nsp,
virDomainThreadSchedParamPtr sp);
-int qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
-virDomainGraphicsDefPtr graphics,
-
Re: [libvirt] [PATCH 3/5] domaincaps: Report graphics spiceGL
On Mon, May 09, 2016 at 09:30:30 -0400, Cole Robinson wrote: > On 05/09/2016 07:30 AM, Pavel Hrdina wrote: > > On Sun, May 08, 2016 at 01:49:06PM -0400, Cole Robinson wrote: > > Really wish someone would have chimed in with this to my (unresponded) RFC > email about these things... > > http://www.redhat.com/archives/libvir-list/2016-April/msg01839.html > > That pattern is fine in this case, but what about the case of ports= element > that is only supported with a controller type=usb model=nec-xhci ? Do we have > new XML like: > > > > > > > > > > It seems like these types of relationships could grow deeply nested, so I > opted for following the limited existing convention of adding unique enum > names to represent the relationship. Neither of those is really human friendly, thus I prefer aggregating the information to avoid having to collate them from snippets out of place. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] domaincaps: Report graphics spiceGL
On 05/09/2016 05:26 AM, Michal Privoznik wrote: > On 08.05.2016 19:49, Cole Robinson wrote: >> Reports a tristate enum value for acceptable graphics type=spice >> . > > This ^^ is part of graphics element as follows: > > > > .. > >> >> Wire it up for qemu too. 'no' is always a valid value, so we >> unconditionally report it. >> --- >> docs/formatdomaincaps.html.in | 8 >> src/conf/domain_capabilities.c | 1 + >> src/conf/domain_capabilities.h | 1 + >> src/qemu/qemu_capabilities.c| 4 >> tests/domaincapsschemadata/domaincaps-full.xml | 5 + >> tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml | 3 +++ >> tests/domaincapsschemadata/domaincaps-qemu_2.6.0-1.xml | 4 >> tests/domaincapsschemadata/domaincaps-qemu_2.6.0-2.xml | 3 +++ >> tests/domaincapsschemadata/domaincaps-qemu_2.6.0-3.xml | 3 +++ >> tests/domaincapsschemadata/domaincaps-qemu_2.6.0-4.xml | 3 +++ >> tests/domaincapsschemadata/domaincaps-qemu_2.6.0-5.xml | 3 +++ >> tests/domaincapstest.c | 1 + >> 12 files changed, 39 insertions(+) >> >> diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in >> index d5a8414..c424107 100644 >> --- a/docs/formatdomaincaps.html.in >> +++ b/docs/formatdomaincaps.html.in >> @@ -231,6 +231,10 @@ >>vnc >>spice >> >> +>> + >> > > > .. and while it's true that just Spice supports OpenGL currently, I'd > vote for a less specific naming. BTW: wasn't virgl implemented for SDL > initially? I know we don't support it, but my point is, what if XYZ > graphics type gains support for something equivalent to virgl? Would we > end up having yet another 'xyzGL' enum? Well, on the other hand, we > already have pciBackend enum to hostdev elem where we report VFIO/KVM > caps for hostdev[@type='pci']/driver/@name. > So I guess it's your call whether to change this or not. The only name I > can suggest right now (if you're out of ideas) is just 'gl'. > I think just reporting gives insufficient info to apps. What if hypothetically VNC starts support 3D... then apps have no way of distinguishing which libvirt+qemu combos support spice + 3D vs the new VNC + 3D, and they are stuck in the current position of trying to guess what is supported by checking libvirt version numbers. So IMO the domcaps need to report this fine grained value some how. Either this way or the XML suggested by Pavel, please give your thoughts there as well Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-listyes >> +no >> +
Re: [libvirt] [PATCH 3/5] domaincaps: Report graphics spiceGL
On 05/09/2016 07:30 AM, Pavel Hrdina wrote: > On Sun, May 08, 2016 at 01:49:06PM -0400, Cole Robinson wrote: >> Reports a tristate enum value for acceptable graphics type=spice >> . >> >> Wire it up for qemu too. 'no' is always a valid value, so we >> unconditionally report it. >> --- > > [...] > >> diff --git a/tests/domaincapsschemadata/domaincaps-full.xml >> b/tests/domaincapsschemadata/domaincaps-full.xml >> index 2f529ff..4eb5637 100644 >> --- a/tests/domaincapsschemadata/domaincaps-full.xml >> +++ b/tests/domaincapsschemadata/domaincaps-full.xml >> @@ -47,6 +47,11 @@ >> desktop >> spice >> >> + >> +default >> +yes >> +no >> + > > Ewww, this doesn't look good and I agree with Michal that what if we add > support for another graphics device and what if one graphics device will have > more than one capability? > > I would suggest something like this: > > > > spice > vnc > sdl > > > > > > > or even do something like this: > > > > > > > > > Really wish someone would have chimed in with this to my (unresponded) RFC email about these things... http://www.redhat.com/archives/libvir-list/2016-April/msg01839.html That pattern is fine in this case, but what about the case of ports= element that is only supported with a controller type=usb model=nec-xhci ? Do we have new XML like: It seems like these types of relationships could grow deeply nested, so I opted for following the limited existing convention of adding unique enum names to represent the relationship. I'm not really opposed to the nesting, but I'd like others on the list to state their preference before I rework this Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions
> > > > > > > > Just FYI, on other architectures (e.g. s390x), other conditions (e.g. > > > > cpu > > > > generation) also define if a CPU model is runnable, so the pure > > > > availability of > > > > features does not mean that a cpu model is runnable. > > > > > > > > We could have runnable=false and unavailable-features being an empty > > > > list. > > > > > > Even on those cases, can't the root cause be mapped to a QOM > > > property name (e.g. "cpu-generation"), even if it's property that > > > can't be changed by the user? > > > > In the current state, no. > > But it could be implemented by s390x in the future, if it's > considered useful, right? Yes, we could fit that into read-only properties if we would ever need it (like cpu-generation you mentioned and cpu-ga-level - both will always be read-only). However we could come up with more optional fields for that in the future. (like unsupported-values or sth. like that). I actually prefer unavailable-features over runnability-blockers. > > > > > So I think for runnable=false: > > a) unavailable-features set -> can be made runnable > > b) unavailable-features not set -> cannot be made runnable > > > > would be enough. > > I understand it would be enough, but I would like to at least > define semantics that would make sense for all architectures in > case it gets implemented in the future. Would the proposal below > make sense? > Yes, I think so. However to really make good hints, upper layers would most likely need more information about the exact problem with a property - maybe something like an enum value per problematic property. (UNAVAILABLE_FEATURE, VALUE_TOO_BIG, VALUE_TOO_SMALL, UNSUPPORTED_VALUE) ... > > > > > > We could replace this with something more generic, like: > > > > > > @runnability-blockers: List of attributes that prevent the CPU > > > model from running in the current host. > > > > > > A list of QOM property names that represent CPU model > > > attributes that prevent the CPU from running. If the QOM > > > property is read-only, that means the CPU model can never run > > > in the current host. If the property is read-write, it means > > > that it MAY be possible to run the CPU model in the current > > > host if that property is changed. > > > > > > Management software can use it as hints to suggest or choose an > > > alternative for the user, or just to generate meaningful error > > > messages explaining why the CPU model can't be used. > > > > > > (I am looking for a better name than "runnability-blockers"). > > > Not sure which approach would be better. David -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Introducing myself (GSoC 2016 student)
Hi, my name is Katerina Koukiou and I got selected as a GSoC student this year to work on libvirt. My project is to add libvirt support for migration of lxc containers using a tool called CRIU. For the time, I am in my final year of undergraduate studies in Electrical and Computer Engineering, at NTUA university, Greece. Looking forward to share some patches here! Katerina -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions
On Mon, May 09, 2016 at 02:05:05PM +0200, David Hildenbrand wrote: > > On Mon, May 09, 2016 at 10:54:53AM +0200, David Hildenbrand wrote: > > > > Extend query-cpu-definitions schema to allow it to return two new > > > > optional fields: "runnable" and "unavailable-features". > > > > "runnable" will tell if the CPU model can be run in the current > > > > host. "unavailable-features" will contain a list of CPU > > > > properties that are preventing the CPU model from running in the > > > > current host. > > > > > > > > Cc: David Hildenbrand > > > > Cc: Michael Mueller > > > > Cc: Christian Borntraeger > > > > Cc: Cornelia Huck > > > > Cc: Jiri Denemark > > > > Cc: libvir-list@redhat.com > > > > Signed-off-by: Eduardo Habkost > > > > --- > > > > qapi-schema.json | 10 +- > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/qapi-schema.json b/qapi-schema.json > > > > index 54634c4..450e6e7 100644 > > > > --- a/qapi-schema.json > > > > +++ b/qapi-schema.json > > > > @@ -2948,11 +2948,19 @@ > > > > # Virtual CPU definition. > > > > # > > > > # @name: the name of the CPU definition > > > > +# @runnable: true if the CPU model is runnable using the current > > > > +#machine and accelerator. Optional. Since 2.6. > > > > +# @unavailable-features: List of properties that prevent the CPU > > > > +#model from running in the current host, > > > > +#if @runnable is false. Optional. > > > > +#Since 2.6. > > > > > > Just FYI, on other architectures (e.g. s390x), other conditions (e.g. cpu > > > generation) also define if a CPU model is runnable, so the pure > > > availability of > > > features does not mean that a cpu model is runnable. > > > > > > We could have runnable=false and unavailable-features being an empty > > > list. > > > > Even on those cases, can't the root cause be mapped to a QOM > > property name (e.g. "cpu-generation"), even if it's property that > > can't be changed by the user? > > In the current state, no. But it could be implemented by s390x in the future, if it's considered useful, right? > > So I think for runnable=false: > a) unavailable-features set -> can be made runnable > b) unavailable-features not set -> cannot be made runnable > > would be enough. I understand it would be enough, but I would like to at least define semantics that would make sense for all architectures in case it gets implemented in the future. Would the proposal below make sense? > > > > We could replace this with something more generic, like: > > > > @runnability-blockers: List of attributes that prevent the CPU > > model from running in the current host. > > > > A list of QOM property names that represent CPU model > > attributes that prevent the CPU from running. If the QOM > > property is read-only, that means the CPU model can never run > > in the current host. If the property is read-write, it means > > that it MAY be possible to run the CPU model in the current > > host if that property is changed. > > > > Management software can use it as hints to suggest or choose an > > alternative for the user, or just to generate meaningful error > > messages explaining why the CPU model can't be used. > > > > (I am looking for a better name than "runnability-blockers"). > > > > > > David > -- Eduardo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu.conf: spaces correction
libvirt/qemu.conf: spaces correction --- src/qemu/qemu.conf | 42 +- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 4fa5e8a..5cf4599 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -38,7 +38,7 @@ # # ca-cert.pem - the CA master certificate # server-cert.pem - the server certificate signed with ca-cert.pem -# server-key.pem - the server private key +# server-key.pem - the server private key # # This option allows the certificate directory to be changed # @@ -123,7 +123,7 @@ # # ca-cert.pem - the CA master certificate # server-cert.pem - the server certificate signed with ca-cert.pem -# server-key.pem - the server private key +# server-key.pem - the server private key # # This option allows the certificate directory to be changed. # @@ -179,7 +179,7 @@ #remote_display_port_max = 65535 # VNC WebSocket port policies, same rules apply as with remote display -# ports. VNC WebSockets use similar display <-> port mappings, with +# ports. VNC WebSockets use similar display <-> port mappings, with # the exception being that ports start from 5700 instead of 5900. # #remote_websocket_port_min = 5700 @@ -196,7 +196,7 @@ # security_driver = [ "selinux", "apparmor" ] # # Notes: The DAC security driver is always enabled; as a result, the -# value of security_driver cannot contain "dac". The value "none" is +# value of security_driver cannot contain "dac". The value "none" is # a special value; security_driver can be set to that value in # isolation, but it cannot appear in a list of drivers. # @@ -272,7 +272,7 @@ #"/dev/null", "/dev/full", "/dev/zero", #"/dev/random", "/dev/urandom", #"/dev/ptmx", "/dev/kvm", "/dev/kqemu", -#"/dev/rtc","/dev/hpet", "/dev/vfio/vfio" +#"/dev/rtc", "/dev/hpet", "/dev/vfio/vfio" #] # # RDMA migration requires the following extra files to be added to the list: @@ -285,11 +285,11 @@ # The default format for Qemu/KVM guest save images is raw; that is, the -# memory from the domain is dumped out directly to a file. If you have +# memory from the domain is dumped out directly to a file. If you have # guests with a large amount of memory, however, this can take up quite -# a bit of space. If you would like to compress the images while they +# a bit of space. If you would like to compress the images while they # are being saved to disk, you can also set "lzop", "gzip", "bzip2", or "xz" -# for save_image_format. Note that this means you slow down the process of +# for save_image_format. Note that this means you slow down the process of # saving a domain in order to save disk space; the list above is in descending # order by performance and ascending order by compression ratio. # @@ -319,7 +319,7 @@ # When a domain is configured to be auto-dumped, enabling this flag # has the same effect as using the VIR_DUMP_BYPASS_CACHE flag with the -# virDomainCoreDump API. That is, the system will avoid using the +# virDomainCoreDump API. That is, the system will avoid using the # file system cache while writing the dump file, but may cause # slower operation. # @@ -327,17 +327,17 @@ # When a domain is configured to be auto-started, enabling this flag # has the same effect as using the VIR_DOMAIN_START_BYPASS_CACHE flag -# with the virDomainCreateWithFlags API. That is, the system will +# with the virDomainCreateWithFlags API. That is, the system will # avoid using the file system cache when restoring any managed state # file, but may cause slower operation. # #auto_start_bypass_cache = 0 # If provided by the host and a hugetlbfs mount point is configured, -# a guest may request huge page backing. When this mount point is +# a guest may request huge page backing. When this mount point is # unspecified here, determination of a host mount point in /proc/mounts -# will be attempted. Specifying an explicit mount overrides detection -# of the same in /proc/mounts. Setting the mount point to "" will +# will be attempted. Specifying an explicit mount overrides detection +# of the same in /proc/mounts. Setting the mount point to "" will # disable guest hugepage backing. If desired, multiple mount points can # be specified at once, separated by comma and enclosed in square # brackets, for example: @@ -353,9 +353,9 @@ #hugetlbfs_mount = "/dev/hugepages" -# Path to the setuid helper for creating tap devices. This executable +# Path to the setuid helper for creating tap devices. This executable # is used to create interfaces when libvirtd is -# running unprivileged. libvirt invokes the helper directly, instead +# running unprivileged. libvirt invokes the helper directly, instead # of using "-netdev bridge", for security reasons. #bridge_helper = "/usr/libexec/qemu-bridge-helper" @@ -411,7 +411,7 @@ # # WARNING: Enabling probing is a security hole in almost all # deployments. It is stron
Re: [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions
> On Mon, May 09, 2016 at 10:54:53AM +0200, David Hildenbrand wrote: > > > Extend query-cpu-definitions schema to allow it to return two new > > > optional fields: "runnable" and "unavailable-features". > > > "runnable" will tell if the CPU model can be run in the current > > > host. "unavailable-features" will contain a list of CPU > > > properties that are preventing the CPU model from running in the > > > current host. > > > > > > Cc: David Hildenbrand > > > Cc: Michael Mueller > > > Cc: Christian Borntraeger > > > Cc: Cornelia Huck > > > Cc: Jiri Denemark > > > Cc: libvir-list@redhat.com > > > Signed-off-by: Eduardo Habkost > > > --- > > > qapi-schema.json | 10 +- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/qapi-schema.json b/qapi-schema.json > > > index 54634c4..450e6e7 100644 > > > --- a/qapi-schema.json > > > +++ b/qapi-schema.json > > > @@ -2948,11 +2948,19 @@ > > > # Virtual CPU definition. > > > # > > > # @name: the name of the CPU definition > > > +# @runnable: true if the CPU model is runnable using the current > > > +#machine and accelerator. Optional. Since 2.6. > > > +# @unavailable-features: List of properties that prevent the CPU > > > +#model from running in the current host, > > > +#if @runnable is false. Optional. > > > +#Since 2.6. > > > > Just FYI, on other architectures (e.g. s390x), other conditions (e.g. cpu > > generation) also define if a CPU model is runnable, so the pure > > availability of > > features does not mean that a cpu model is runnable. > > > > We could have runnable=false and unavailable-features being an empty list. > > Even on those cases, can't the root cause be mapped to a QOM > property name (e.g. "cpu-generation"), even if it's property that > can't be changed by the user? In the current state, no. So I think for runnable=false: a) unavailable-features set -> can be made runnable b) unavailable-features not set -> cannot be made runnable would be enough. > > We could replace this with something more generic, like: > > @runnability-blockers: List of attributes that prevent the CPU > model from running in the current host. > > A list of QOM property names that represent CPU model > attributes that prevent the CPU from running. If the QOM > property is read-only, that means the CPU model can never run > in the current host. If the property is read-write, it means > that it MAY be possible to run the CPU model in the current > host if that property is changed. > > Management software can use it as hints to suggest or choose an > alternative for the user, or just to generate meaningful error > messages explaining why the CPU model can't be used. > > (I am looking for a better name than "runnability-blockers"). > David -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/9] Add runnability info to query-cpu-definitions
> This series extends query-cpu-definitions to include two extra > fields: "runnable", and "unavailable-features". > > This will return information based on the current machine and > accelerator only. In the future we may extend these mechanisms to > allow querying other machines and other accelerators without > restarting QEMU, but it will require some reorganization of > QEMU's main code. Hi Eduardo, just as discussed during/after the last kvm forum offline, we are currently working on a new set of qmp functions to keep all the cpu model details in QEMU and not have to replicate them in libvirt and to have a nice way to query the host cpu model. We already have a prototype running, just need to clarify some s390x details (feature set of the CPU generations) before we can send out an RFC. I could, however, send out just the interface changes + details on the concept upfront, if anybody is interested. In the current implementation: - s390x cpu models will be independent of the QEMU machine, so for s390x the runnability information will only depend on the accelerator (host cpu model). However, for compatibility machines, cpu models will be disabled, and we won't therefore have any runnability information. - we will not touch query-cpu-definitions - the new interfaces will also make use of the current accelerator and the current machine, so the context of these functions are the same. Adding functionality to query for other machines is not necessary for s390x. Other accelerators don't really make sense to me, let's keep it simple here. So this change is fine from my point of view. It doesn't contradict to out concept. David -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions
> Extend query-cpu-definitions schema to allow it to return two new
> optional fields: "runnable" and "unavailable-features".
> "runnable" will tell if the CPU model can be run in the current
> host. "unavailable-features" will contain a list of CPU
> properties that are preventing the CPU model from running in the
> current host.
>
> Cc: David Hildenbrand
> Cc: Michael Mueller
> Cc: Christian Borntraeger
> Cc: Cornelia Huck
> Cc: Jiri Denemark
> Cc: libvir-list@redhat.com
> Signed-off-by: Eduardo Habkost
> ---
> qapi-schema.json | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 54634c4..450e6e7 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2948,11 +2948,19 @@
> # Virtual CPU definition.
> #
> # @name: the name of the CPU definition
> +# @runnable: true if the CPU model is runnable using the current
> +#machine and accelerator. Optional. Since 2.6.
> +# @unavailable-features: List of properties that prevent the CPU
> +#model from running in the current host,
> +#if @runnable is false. Optional.
> +#Since 2.6.
Just FYI, on other architectures (e.g. s390x), other conditions (e.g. cpu
generation) also define if a CPU model is runnable, so the pure availability of
features does not mean that a cpu model is runnable.
We could have runnable=false and unavailable-features being an empty list.
> #
> # Since: 1.2.0
> ##
> { 'struct': 'CpuDefinitionInfo',
> - 'data': { 'name': 'str' } }
> + 'data': { 'name': 'str',
> +'*runnable': 'bool',
> +'*unavailable-features': [ 'str' ] } }
>
> ##
> # @query-cpu-definitions:
David
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions
On Mon, May 09, 2016 at 10:54:53AM +0200, David Hildenbrand wrote: > > Extend query-cpu-definitions schema to allow it to return two new > > optional fields: "runnable" and "unavailable-features". > > "runnable" will tell if the CPU model can be run in the current > > host. "unavailable-features" will contain a list of CPU > > properties that are preventing the CPU model from running in the > > current host. > > > > Cc: David Hildenbrand > > Cc: Michael Mueller > > Cc: Christian Borntraeger > > Cc: Cornelia Huck > > Cc: Jiri Denemark > > Cc: libvir-list@redhat.com > > Signed-off-by: Eduardo Habkost > > --- > > qapi-schema.json | 10 +- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/qapi-schema.json b/qapi-schema.json > > index 54634c4..450e6e7 100644 > > --- a/qapi-schema.json > > +++ b/qapi-schema.json > > @@ -2948,11 +2948,19 @@ > > # Virtual CPU definition. > > # > > # @name: the name of the CPU definition > > +# @runnable: true if the CPU model is runnable using the current > > +#machine and accelerator. Optional. Since 2.6. > > +# @unavailable-features: List of properties that prevent the CPU > > +#model from running in the current host, > > +#if @runnable is false. Optional. > > +#Since 2.6. > > Just FYI, on other architectures (e.g. s390x), other conditions (e.g. cpu > generation) also define if a CPU model is runnable, so the pure availability > of > features does not mean that a cpu model is runnable. > > We could have runnable=false and unavailable-features being an empty list. Even on those cases, can't the root cause be mapped to a QOM property name (e.g. "cpu-generation"), even if it's property that can't be changed by the user? We could replace this with something more generic, like: @runnability-blockers: List of attributes that prevent the CPU model from running in the current host. A list of QOM property names that represent CPU model attributes that prevent the CPU from running. If the QOM property is read-only, that means the CPU model can never run in the current host. If the property is read-write, it means that it MAY be possible to run the CPU model in the current host if that property is changed. Management software can use it as hints to suggest or choose an alternative for the user, or just to generate meaningful error messages explaining why the CPU model can't be used. (I am looking for a better name than "runnability-blockers"). -- Eduardo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: polkit: Fix polkit agent startup
On Mon, May 09, 2016 at 11:10:15 +0200, Peter Krempa wrote: > Commit 0b36b0e9 broke polkit agent startup when attempting to fix a > coverity warning. Refactor it properly so that we don't need the 'cmd' > intermediate variable. > --- > src/util/virpolkit.c | 26 -- > 1 file changed, 12 insertions(+), 14 deletions(-) ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/8] qemu: command: Use more appropriate checking function for block devices
On Fri, May 06, 2016 at 11:09:38 -0400, John Ferlan wrote: > > > On 05/02/2016 10:32 AM, Peter Krempa wrote: > > In qemuCheckDiskConfig would now use virDomainDiskSourceIsBlockType just > > as a glorified version of virStorageSourceIsBlockLocal that reports > > error messages. Replace it with the latter including the message for > > clarity. > > --- > > src/qemu/qemu_command.c | 6 +- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > Hmmm... I wonder if the previous commit should remove or at least > reference the check in virDomainDiskSourceIsBlockType that neglected to I don't see a point in doing this since that function is removed in the next patch. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] domaincaps: Report graphics spiceGL
On Sun, May 08, 2016 at 01:49:06PM -0400, Cole Robinson wrote: > Reports a tristate enum value for acceptable graphics type=spice > . > > Wire it up for qemu too. 'no' is always a valid value, so we > unconditionally report it. > --- [...] > diff --git a/tests/domaincapsschemadata/domaincaps-full.xml > b/tests/domaincapsschemadata/domaincaps-full.xml > index 2f529ff..4eb5637 100644 > --- a/tests/domaincapsschemadata/domaincaps-full.xml > +++ b/tests/domaincapsschemadata/domaincaps-full.xml > @@ -47,6 +47,11 @@ > desktop > spice > > + > +default > +yes > +no > + Ewww, this doesn't look good and I agree with Michal that what if we add support for another graphics device and what if one graphics device will have more than one capability? I would suggest something like this: spice vnc sdl or even do something like this: Pavel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/8] util: Replace virDomainDiskSourceIsBlockType with a new helper
On Fri, May 06, 2016 at 10:33:49 -0400, John Ferlan wrote:
> On 05/02/2016 10:32 AM, Peter Krempa wrote:
> > For disks sources described by a libvirt volume we don't need to do a
> > complicated check since virStorageTranslateDiskSourcePool already
> > correctly determines the actual disk type.
> >
> > Replace the checks using a new accessor that does not open-code the
> > whole logic.
> > ---
> > src/libvirt_private.syms | 1 +
> > src/lxc/lxc_cgroup.c | 3 ++-
> > src/qemu/qemu_conf.c | 10 +++---
> > src/util/virstoragefile.c | 16 +++-
> > src/util/virstoragefile.h | 3 ++-
> > 5 files changed, 27 insertions(+), 6 deletions(-)
> >
[...]
> > diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
> > index 4afe7e2..ea86d36 100644
> > --- a/src/lxc/lxc_cgroup.c
> > +++ b/src/lxc/lxc_cgroup.c
> > @@ -393,7 +393,8 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr
> > def,
> >
> > VIR_DEBUG("Allowing any disk block devs");
> > for (i = 0; i < def->ndisks; i++) {
> > -if (!virDomainDiskSourceIsBlockType(def->disks[i]->src, false))
> > +if (virStorageSourceIsEmpty(def->disks[i]->src) ||
> > +!virStorageSourceIsBlockLocal(def->disks[i]->src))
>
> Could an LXC domain use a storage pool for a volume? If so, then
> somewhere in the LXC code wouldn't we need to translate the source pool
> in order to get "actualtype" and the volume check...
LXC won't work with 'volume' disks as as you've said it's not
translated. virDomainDiskSourceIsBlockType would return false at this
point since def->disks[i]->src->path would not be set at this point
since it was not translated and the very first check of that function
was to check the source path.
As of that this change doesn't change the behavior in LXC in any way
just uses a more sane function to do the check.
>
>
> > continue;
> >
> > if (virCgroupAllowDevicePath(cgroup,
> > /**
> > + * virStorageSourceIsBlockLocal:
> > + * @src: disk source definition
> > + *
> > + * Returns true if @src describes a locally accessible block storage
> > source.
> > + * This includes block devices and host-mapped iSCSI volumes.
> > + */
> > +bool
> > +virStorageSourceIsBlockLocal(const virStorageSource *src)
> > +{
> > +return virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_BLOCK;
>
> This assumes that virStorageTranslateDiskSourcePool has been run, which
> is true is for the non-LXC paths changed in this patch...
>
> So as long as you're comfortable with the LXC adjustment, then
Allowing to use volumes is definitely something for a separate patch.
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/8] Add support for fetching statistics of completed jobs
Hi,
FYI as tracking down that failure did cost me some time and hopefully
this summary will help other to avoid this situation:
Am 09.09.2014 um 11:54 schrieb Jiri Denemark:
> virDomainGetJobStats gains new VIR_DOMAIN_JOB_STATS_COMPLETED flag that
> can be used to fetch statistics of a completed job rather than a
> currently running job.
>
> Signed-off-by: Jiri Denemark
...
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -2500,7 +2500,8 @@ qemuMonitorJSONGetMigrationStatusReply(virJSONValuePtr
> reply,
...
> -if (status->status == QEMU_MONITOR_MIGRATION_STATUS_ACTIVE) {
> +if (status->status == QEMU_MONITOR_MIGRATION_STATUS_ACTIVE ||
> +status->status == QEMU_MONITOR_MIGRATION_STATUS_COMPLETED) {
> virJSONValuePtr ram = virJSONValueObjectGet(ret, "ram");
> if (!ram) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>_("migration was active, but RAM 'transferred' "
> "data was missing"));
This change from libvirt v1.2.9-rc1~203 breaks migration with
qemu-kvm-1.1.2, as that ancient implementation does *not* export
transfer statistics for completed jobs:
> qemuMonitorJSONCommandWithFd:286 : Send command
> '{"execute":"query-migrate","id":"libvirt-41"}' for write with FD -1
> qemuMonitorJSONIOProcessLine:179 : Line [{"return": {"status": "active",
> "ram": {"total": 2164654080, "remaining": 22474752, "transferred":
> 175117413}}, "id": "libvirt-41"}]
...
> qemuMonitorJSONCommandWithFd:286 : Send command
> '{"execute":"query-migrate","id":"libvirt-42"}' for write with FD -1
> qemuMonitorJSONIOProcessLine:179 : Line [{"return": {"status": "completed"},
> "id": "libvirt-42"}]
As you can see, there is not "ram" section and the migration is aborted
with the message:
> internal error: migration was active, but no RAM info was set
qemu-kvm/qmp-commands.hx even states this:
> - "ram": only present if "status" is "active", it is a json-object with the
> following RAM information (in bytes):
but the example some lines below is wrong:
> 2. Migration is done and has succeeded
>
> -> { "execute": "query-migrate" }
> <- { "return": {
> "status": "completed",
> "ram":{
That example has been updated by v1.2.0-rc0~29^2~2, but forgot to update
the specification above.
The attached hack for libvirt makes migration work again for me by
making "ram" optional in case of "completed".
Philipp
From c4f6dfb25042f420fdd1728865686552d928d90e Mon Sep 17 00:00:00 2001
Message-Id:
From: Philipp Hahn
Date: Mon, 9 May 2016 10:52:09 +0200
Subject: [PATCH] Bug #40318 libvirt: Handle qemu-kvm-1.1.2 migration
incompatibility
Organization: Univention GmbH, Bremen, Germany
To: libvir-list@redhat.com
The change from libvirt v1.2.9-rc1~203 breaks migration with qemu-kvm-1.1.2, as
that ancient implementation does *not* export transfer statistics for completed
jobs:
> qemuMonitorJSONCommandWithFd:286 : Send command '{"execute":"query-migrate","id":"libvirt-41"}' for write with FD -1
> qemuMonitorJSONIOProcessLine:179 : Line [{"return": {"status": "active", "ram": {"total": 2164654080, "remaining": 22474752, "transferred": 175117413}}, "id": "libvirt-41"}]
...
> qemuMonitorJSONCommandWithFd:286 : Send command '{"execute":"query-migrate","id":"libvirt-42"}' for write with FD -1
> qemuMonitorJSONIOProcessLine:179 : Line [{"return": {"status": "completed"}, "id": "libvirt-42"}]
As you can see, there is not "ram" section and the migration is aborted with
the message:
> internal error: migration was active, but no RAM info was set
qemu-kvm/qmp-commands.hx even states this:
> - "ram": only present if "status" is "active", it is a json-object with the
> following RAM information (in bytes):
but the example some lines below is wrong:
> 2. Migration is done and has succeeded
>
> -> { "execute": "query-migrate" }
> <- { "return": {
> "status": "completed",
> "ram":{
That example has been updated by v1.2.0-rc0~29^2~2, but forgot to update the
specification above.
---
src/qemu/qemu_monitor_json.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index a62c02f..d3b7b90 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2483,8 +2483,11 @@ qemuMonitorJSONGetMigrationStatusReply(virJSONValuePtr reply,
if (!ram) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("migration was active, but no RAM info was set"));
+if (status->status == QEMU_MONITOR_MIGRATION_STATUS_ACTIVE) {
+// qemu-kvm-1.1.2 does NOT report 'ram':{...} on complete
return -1;
-}
+}
+} else {
if (virJSONValueObjectGetNumberUlong(ram, "transferred",
&status->ram_transferred) < 0) {
@@ -2514,6 +2517,7 @@ qemuMonitorJSONGetMigrationS
Re: [libvirt] [PATCH 1/5] domaincaps: Report graphics type enum
On 08.05.2016 19:49, Cole Robinson wrote: > Requires adding the plumbing for > Wire it up for qemu too > --- > I stuck the element between and to match > the ordering in XML > > docs/formatdomaincaps.html.in | 30 > +- > docs/schemas/domaincaps.rng| 8 ++ > src/conf/domain_capabilities.c | 13 ++ > src/conf/domain_capabilities.h | 8 ++ > src/qemu/qemu_capabilities.c | 22 +++- > tests/domaincapsschemadata/domaincaps-basic.xml| 1 + > tests/domaincapsschemadata/domaincaps-full.xml | 9 +++ > .../domaincaps-qemu_1.6.50-1.xml | 7 + > .../domaincaps-qemu_2.6.0-1.xml| 7 + > .../domaincaps-qemu_2.6.0-2.xml| 6 + > .../domaincaps-qemu_2.6.0-3.xml| 6 + > .../domaincaps-qemu_2.6.0-4.xml| 6 + > .../domaincaps-qemu_2.6.0-5.xml| 6 + > tests/domaincapstest.c | 4 +++ > 14 files changed, 131 insertions(+), 2 deletions(-) ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] domaincaps: Report video modelType
On 08.05.2016 19:49, Cole Robinson wrote: > Requires adding the plumbing for > The value is to match the associated domain > XML of > > Wire it up for qemu too > --- > docs/formatdomaincaps.html.in | 29 > ++ > docs/schemas/domaincaps.rng| 8 ++ > src/conf/domain_capabilities.c | 13 ++ > src/conf/domain_capabilities.h | 10 > src/qemu/qemu_capabilities.c | 23 + > tests/domaincapsschemadata/domaincaps-basic.xml| 1 + > tests/domaincapsschemadata/domaincaps-full.xml | 12 + > .../domaincaps-qemu_1.6.50-1.xml | 8 ++ > .../domaincaps-qemu_2.6.0-1.xml| 9 +++ > .../domaincaps-qemu_2.6.0-2.xml| 7 ++ > .../domaincaps-qemu_2.6.0-3.xml| 7 ++ > .../domaincaps-qemu_2.6.0-4.xml| 7 ++ > .../domaincaps-qemu_2.6.0-5.xml| 7 ++ > tests/domaincapstest.c | 4 +++ > 14 files changed, 145 insertions(+) ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] domaincaps: Report graphics spiceGL
On 08.05.2016 19:49, Cole Robinson wrote: > Reports a tristate enum value for acceptable graphics type=spice > . This ^^ is part of graphics element as follows: .. > > Wire it up for qemu too. 'no' is always a valid value, so we > unconditionally report it. > --- > docs/formatdomaincaps.html.in | 8 > src/conf/domain_capabilities.c | 1 + > src/conf/domain_capabilities.h | 1 + > src/qemu/qemu_capabilities.c| 4 > tests/domaincapsschemadata/domaincaps-full.xml | 5 + > tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml | 3 +++ > tests/domaincapsschemadata/domaincaps-qemu_2.6.0-1.xml | 4 > tests/domaincapsschemadata/domaincaps-qemu_2.6.0-2.xml | 3 +++ > tests/domaincapsschemadata/domaincaps-qemu_2.6.0-3.xml | 3 +++ > tests/domaincapsschemadata/domaincaps-qemu_2.6.0-4.xml | 3 +++ > tests/domaincapsschemadata/domaincaps-qemu_2.6.0-5.xml | 3 +++ > tests/domaincapstest.c | 1 + > 12 files changed, 39 insertions(+) > > diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in > index d5a8414..c424107 100644 > --- a/docs/formatdomaincaps.html.in > +++ b/docs/formatdomaincaps.html.in > @@ -231,6 +231,10 @@ >vnc >spice > > +> + > .. and while it's true that just Spice supports OpenGL currently, I'd vote for a less specific naming. BTW: wasn't virgl implemented for SDL initially? I know we don't support it, but my point is, what if XYZ graphics type gains support for something equivalent to virgl? Would we end up having yet another 'xyzGL' enum? Well, on the other hand, we already have pciBackend enum to hostdev elem where we report VFIO/KVM caps for hostdev[@type='pci']/driver/@name. So I guess it's your call whether to change this or not. The only name I can suggest right now (if you're out of ideas) is just 'gl'. Otherwise the code looks good. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-listyes > +no > +
Re: [libvirt] [PATCH 5/5] domaincaps: Report graphics spiceGL
On 08.05.2016 19:49, Cole Robinson wrote: > Reports a tristate enum value for acceptable video model=virtio > . > > Wire it up for qemu too. 'no' is always a valid value, so we > unconditionally report it. > --- > docs/formatdomaincaps.html.in | 9 + > src/conf/domain_capabilities.c | 1 + > src/conf/domain_capabilities.h | 2 ++ > src/qemu/qemu_capabilities.c| 4 > tests/domaincapsschemadata/domaincaps-full.xml | 5 + > tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml | 3 +++ > tests/domaincapsschemadata/domaincaps-qemu_2.6.0-1.xml | 4 > tests/domaincapsschemadata/domaincaps-qemu_2.6.0-2.xml | 3 +++ > tests/domaincapsschemadata/domaincaps-qemu_2.6.0-3.xml | 3 +++ > tests/domaincapsschemadata/domaincaps-qemu_2.6.0-4.xml | 3 +++ > tests/domaincapsschemadata/domaincaps-qemu_2.6.0-5.xml | 3 +++ > tests/domaincapstest.c | 1 + > 12 files changed, 41 insertions(+) > > diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in > index c424107..b87a45a 100644 > --- a/docs/formatdomaincaps.html.in > +++ b/docs/formatdomaincaps.html.in > @@ -268,6 +268,10 @@ >qxl >virtio > > +> + > > ... > > @@ -278,6 +282,11 @@ >modelType >Options for the type attribute of the >yes > +no > +
Re: [libvirt] [PATCH 4/5] qemu: command: unconditionally allow accel3d='no'
On 08.05.2016 19:49, Cole Robinson wrote:
> This matches how we handle spice gl='no' even if spice GL isn't
> supported. Not too interesting in practice but I figure we should
> be consistent
> ---
> src/qemu/qemu_command.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2966b07..c26715f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4125,7 +4125,7 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
> virBufferAsprintf(&buf, "%s,id=%s", model, video->info.alias);
>
> if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO) {
> -if (video->accel && video->accel->accel3d) {
> +if (video->accel && video->accel->accel3d == VIR_TRISTATE_SWITCH_ON)
> {
> if (!virQEMUCapsGet(qemuCaps,
> QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> "%s", _("virtio-gpu 3d acceleration is not
> supported"));
>
In fact, this is important in practice. Because if in accel3d had been
'no' we would ignore it and execute the if() statement body. Both
VIR_TRISTATE_SWITCH_ON and _OFF have non zero values.
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: polkit: Fix polkit agent startup
Commit 0b36b0e9 broke polkit agent startup when attempting to fix a
coverity warning. Refactor it properly so that we don't need the 'cmd'
intermediate variable.
---
src/util/virpolkit.c | 26 --
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
index 2038744..e7e46b8 100644
--- a/src/util/virpolkit.c
+++ b/src/util/virpolkit.c
@@ -167,7 +167,6 @@ virPolkitAgentPtr
virPolkitAgentCreate(void)
{
virPolkitAgentPtr agent = NULL;
-virCommandPtr cmd = virCommandNewArgList(PKTTYAGENT, "--process", NULL);
int pipe_fd[2] = {-1, -1};
struct pollfd pollfd;
int outfd = STDOUT_FILENO;
@@ -181,18 +180,18 @@ virPolkitAgentCreate(void)
if (VIR_ALLOC(agent) < 0)
goto error;
-agent->cmd = cmd;
-cmd = NULL;
-
-virCommandAddArgFormat(cmd, "%lld", (long long int) getpid());
-virCommandAddArg(cmd, "--notify-fd");
-virCommandAddArgFormat(cmd, "%d", pipe_fd[1]);
-virCommandAddArg(cmd, "--fallback");
-virCommandSetInputFD(cmd, STDIN_FILENO);
-virCommandSetOutputFD(cmd, &outfd);
-virCommandSetErrorFD(cmd, &errfd);
-virCommandPassFD(cmd, pipe_fd[1], VIR_COMMAND_PASS_FD_CLOSE_PARENT);
-if (virCommandRunAsync(cmd, NULL) < 0)
+
+agent->cmd = virCommandNewArgList(PKTTYAGENT, "--process", NULL);
+
+virCommandAddArgFormat(agent->cmd, "%lld", (long long int) getpid());
+virCommandAddArg(agent->cmd, "--notify-fd");
+virCommandAddArgFormat(agent->cmd, "%d", pipe_fd[1]);
+virCommandAddArg(agent->cmd, "--fallback");
+virCommandSetInputFD(agent->cmd, STDIN_FILENO);
+virCommandSetOutputFD(agent->cmd, &outfd);
+virCommandSetErrorFD(agent->cmd, &errfd);
+virCommandPassFD(agent->cmd, pipe_fd[1], VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+if (virCommandRunAsync(agent->cmd, NULL) < 0)
goto error;
pollfd.fd = pipe_fd[0];
@@ -207,7 +206,6 @@ virPolkitAgentCreate(void)
VIR_FORCE_CLOSE(pipe_fd[0]);
VIR_FORCE_CLOSE(pipe_fd[1]);
virPolkitAgentDestroy(agent);
-virCommandFree(cmd);
return NULL;
}
--
2.8.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/8] Revert "conf: Validate disk lun using correct types"
On Fri, May 06, 2016 at 11:19:05 -0400, John Ferlan wrote: > > > On 05/02/2016 10:32 AM, Peter Krempa wrote: > > This reverts commit c79ebf53b5fe0a33bf407b3bcb49e3a27ec97eb4. > > > > We can't just add checks to the XML parser once we've accepted such > > configuration in the past. > > --- > > src/conf/domain_conf.c | 22 -- > > tests/qemuxml2argvtest.c | 3 +-- > > 2 files changed, 1 insertion(+), 24 deletions(-) > > > > There was a bz associated with that commit - that'll need to be > addressed in some manner... Well, the initial assesment of that BZ was wrong. This should have been fixed in virt manager at that point. > While I understand your point here, the configuration didn't work - that > is it couldn't be started anyway so there could not be a domain running > with that configuration and thus it wouldn't disappear on a subsequent > reload, hence why checking the config and rejecting "earlier" seemed It does not kill any running domain, that's right. Defined domains still vanish after that commit if they were defined before. That is still unwanted. > proper even though we hadn't rejected such a config when the > "mode='host'" was first implemented. Only when introducing a feature you are allowed to do a check that rejects parsing XML, afterwards, no such thins should be added. > > Commit 'c79ebf53b' is a followup of sorts to commit '33188c9f'... '33188c9f' is okay. Startup time checks can be added. > > For me it's a NACK, but someone else may feel differently I still insist on reverting the code. This should be fixed in virt manager as in libvirt it creates a vanishing VM problem. 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 0/4] qemu_monitor_json: Refactor even more
On 06.05.2016 20:03, John Ferlan wrote:
>
>
> On 05/04/2016 08:33 AM, Michal Privoznik wrote:
>> All these patches will be squashed into one, but I've split them
>> into multiple because of easier review.
>>
>> Michal Privoznik (4):
>> qemu_monitor_json: Follow refactor
>> qemu_monitor_json: Follow refactor
>> qemu_monitor_json: Follow refactor
>> qemu_monitor_json: Follow refactor
>>
>> src/qemu/qemu_monitor_json.c | 673
>> ++-
>> 1 file changed, 349 insertions(+), 324 deletions(-)
>>
>
> Not a deal breaker, but most code has:
>
> if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> goto cleanup;
>
> ret = 0;
> cleanup:
>
> while a few instances [1] have:
>
> ret = qemuMonitorJSONCheckError(cmd, reply);
>
> cleanup:
>
> IDC either way since the result is the same, but I suppose they "could"
> be all done the same way. I'd probably lean towards the latter, but
> since you already went with the former in commit '7884d089' it's
> probably best to keep using that.
>
> [1]
> qemuMonitorJSONSetMigrationCompression
> qemuMonitorJSONBlockCommit
> qemuMonitorJSONInjectNMI [2]
> qemuMonitorJSONSendKey [2]
> qemuMonitorJSONSetMigrationCapability
>
> [2]
> both instances use the same HMP call attempt to return ret - they could
> follow the qemuMonitorJSONSetCPU model...
>
Oh. Thanks for catching that. I'll fix those too.
>
> Finally, I see the comments move 4 spaces to the right in
> qemuMonitorJSONSetObjectProperty? (patch 4)... Was that a remnant or
> expected?
While working on this I didn't bother with adjusting the code that much
initially. After that I've fired up my code alignment key shortcut over
each function I've touched. I will leave the comments as they were.
>
>
> ACK series (your call if you want to adjust [2] now or in a followup
> patch. it's trivial enough as long as you remember to also remove the {}
> around what would become "one line" if then else statements).
>
Thanks, pushed now.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: domain: Don't treat unknown storage type as not having backing chain
On Fri, May 06, 2016 at 11:23:13 -0400, John Ferlan wrote: [...] > > Do you believe it still would be worthwhile to add: > > if (virDomainDiskGetFormat(def) == VIR_STORAGE_FILE_NONE) > virDomainDiskSetFormat(def, VIR_STORAGE_FILE_RAW); > > in virStorageTranslateDiskSourcePool in the ISCSI case for MODE_HOST? You can do that only if format detection is disabled, otherwise it would not be possible to use it. The rest of the code needs to handle the possibility that the format needs to be detected. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions
Eduardo Habkost writes:
> Extend query-cpu-definitions schema to allow it to return two new
> optional fields: "runnable" and "unavailable-features".
> "runnable" will tell if the CPU model can be run in the current
> host. "unavailable-features" will contain a list of CPU
> properties that are preventing the CPU model from running in the
> current host.
>
> Cc: David Hildenbrand
> Cc: Michael Mueller
> Cc: Christian Borntraeger
> Cc: Cornelia Huck
> Cc: Jiri Denemark
> Cc: libvir-list@redhat.com
> Signed-off-by: Eduardo Habkost
> ---
> qapi-schema.json | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 54634c4..450e6e7 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2948,11 +2948,19 @@
> # Virtual CPU definition.
> #
> # @name: the name of the CPU definition
> +# @runnable: true if the CPU model is runnable using the current
> +#machine and accelerator. Optional. Since 2.6.
> +# @unavailable-features: List of properties that prevent the CPU
> +#model from running in the current host,
> +#if @runnable is false. Optional.
> +#Since 2.6.
> #
> # Since: 1.2.0
> ##
> { 'struct': 'CpuDefinitionInfo',
> - 'data': { 'name': 'str' } }
> + 'data': { 'name': 'str',
> +'*runnable': 'bool',
> +'*unavailable-features': [ 'str' ] } }
>
> ##
> # @query-cpu-definitions:
Can @unavailable-features be empty or missing when @runnable is true?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] virsh: blkdeviotune: accept human readable values for bytes
On 04.05.2016 16:25, Nishith Shah wrote: [1] .. here. > Use vshCommandOptScaledInt instead of vshCommandOptULongLong so that > values with suffixes can be passed when bytes are being passed along. > Values for the iops parameters still need to be given in the absolute > form as they are not bytes but numbers. Please refer to the bug link > https://bugzilla.redhat.com/show_bug.cgi?id=885380 which can be closed. I've moved this bug reference [1]. It's our common way of referring bugs in commit messages. > > Signed-off-by: Nishith Shah > --- > tools/virsh-domain.c | 24 > tools/virsh.pod | 18 -- > 2 files changed, 24 insertions(+), 18 deletions(-) ACKed and pushed. Congratulations on your first libvirt contribution! Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
