[libvirt] [PATCH] virt-xml-validate: Add schema for nwfilterbinding
Add nwfilterbinding schema in virt-xml-validate for autoprobing. Add document of nwfilterbinding schema in tools/virt-xml-validate.pod. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1600330 Signed-off-by: Han Han --- tools/virt-xml-validate.in | 3 +++ tools/virt-xml-validate.pod | 4 2 files changed, 7 insertions(+) diff --git a/tools/virt-xml-validate.in b/tools/virt-xml-validate.in index 81fde4d832..7513413189 100644 --- a/tools/virt-xml-validate.in +++ b/tools/virt-xml-validate.in @@ -77,6 +77,9 @@ if [ -z "$TYPE" ]; then *device*) TYPE="nodedev" ;; + *filterbinding*) +TYPE="nwfilterbinding" +;; *filter*) TYPE="nwfilter" ;; diff --git a/tools/virt-xml-validate.pod b/tools/virt-xml-validate.pod index f8e67503fd..a51a57002a 100644 --- a/tools/virt-xml-validate.pod +++ b/tools/virt-xml-validate.pod @@ -52,6 +52,10 @@ The schema for the XML format used to declare driver capabilities The schema for the XML format used by network traffic filters +=item C + +The schema for XML format used by network filter bindings. + =item C The schema for the XML format used by secrets descriptions -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Fix ATTRIBUTE_NONNULL for qemuMonitorAddObject
Commit id fac0dacd was trying to make things more robust; however, the ATTRIBUTE_NONNULL(1) would be for the @mon, not the intended (2) and the @props argument as described in the commit message. Found by Coverity build. Signed-off-by: John Ferlan --- Pushed as trivial and Coverity build breaker. src/qemu/qemu_monitor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index e8ed2d044c..81474a04f6 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -824,7 +824,7 @@ int qemuMonitorCreateObjectProps(virJSONValuePtr *propsret, int qemuMonitorAddObject(qemuMonitorPtr mon, virJSONValuePtr *props, char **alias) -ATTRIBUTE_NONNULL(1); +ATTRIBUTE_NONNULL(2); int qemuMonitorDelObject(qemuMonitorPtr mon, const char *objalias); -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 7/9] rpc: Alter virNetDaemonQuit processing
On 07/09/2018 03:11 AM, Peter Krempa wrote:
> On Sat, Jul 07, 2018 at 08:11:05 -0400, John Ferlan wrote:
>> When virNetDaemonQuit is called from libvirtd's shutdown
>> handler (daemonShutdownHandler) we need to perform the quit
>> in multiple steps. The first part is to "request" the quit
>> and notify the NetServer's of the impending quit which causes
>> the NetServers to inform their workers that a quit was requested.
>>
>> Still because we cannot guarantee a quit will happen or it's
>> possible there's no workers pending, use a virNetDaemonQuitTimer
>> to not only break the event loop but keep track of how long we're
>> waiting and we've waited too long, force an ungraceful exit so
>> that we don't hang waiting forever or cause some sort of SEGV
>> because something is still pending and we Unref things.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/libvirt_remote.syms| 1 +
>> src/remote/remote_daemon.c | 1 +
>> src/rpc/virnetdaemon.c | 68 +-
>> src/rpc/virnetdaemon.h | 4 +++
>> 4 files changed, 73 insertions(+), 1 deletion(-)
>
> [...]
>
>> @@ -855,10 +904,27 @@ virNetDaemonRun(virNetDaemonPtr dmn)
>> virObjectLock(dmn);
>>
>> virHashForEach(dmn->servers, daemonServerProcessClients, NULL);
>> +
>> +/* HACK: Add a dummy timeout to break event loop */
>> +if (dmn->quitRequested && quitTimer == -1)
>> +quitTimer = virEventAddTimeout(500, virNetDaemonQuitTimer,
>> + , NULL);
>> +
>> +if (dmn->quitRequested && daemonServerWorkersDone(dmn)) {
>> +dmn->quit = true;
>> +} else {
>> +/* Firing every 1/2 second and quitTimeout in seconds, force
>> + * an exit when there are still worker threads running and we
>> + * have waited long enough */
>> +if (quitCount > dmn->quitTimeout * 2)
>> +_exit(EXIT_FAILURE);
>
> If you have a legitimate long-running job which would finish eventually
> and e.g. write an updated status XML this will break things. I'm not
> persuaded that this is a systematic solution to some API getting stuck.
>
> The commit message also does not help persuading me that this is a good
> idea.
>
> NACK
>
I understand the sentiment and this hasn't been a "top of the list" item
for me to think about, but to a degree the purpose of re-posting the
series was to try to come to a consensus over some solution. A naked
NACK almost makes it appear as if you would prefer to not do anything in
this situation, letting nature takes it's course (so to speak).
Do you have any thoughts or suggestions related to what processing you
believe is appropriate if someone issues a SIG{INT|QUIT|TERM} to a
daemon (libvirtd|lockd|logd}? That is what to do if we reach the
cleanup: label in main() of src/remote/remote_daemon.c and
virNetDaemonClose() gets run? Right now IIRC it's either going to be a
SEGV or possible long wait for some worker thread to complete. The
latter is the don't apply this example to cause a wait in block stats
fetch. Naturally the long term wait only matters up through the point
while someone is patient before issuing a SIGKILL.
Do you favor waiting forever for some worker thread to finish? To some
degree if some signal is sent to libvirtd, then I would think the
expectation is to not wait for some indeterminate time. And most
definitely trying to avoid some sort of defunct state resolvable by a
host reboot. Knowing exactly what a worker thread is doing may help, but
that'd take a bit (;-)) of code to trace the stack. Perhaps providing a
list of client/workers still active or even some indication that we're
waiting on some worker rather than no feedback? How much is "too much"?
Another option that was proposed, but didn't really gain any support was
introduction of a stateShutdown in virStateDriver that would be run
during libvirtd's cleanup: processing prior to virNetDaemonClose. That
would be before virStateCleanup. For qemu, the priv->mon and priv->agent
would be closed. That would seemingly generate an error and cause the
worker to exit thus theoretically not needing any forced quit of the
thread "if" the reason was some sort of hang as a result of
monitor/agent processing. Doesn't mean there wouldn't be some other
issue causing a hang. Refreshing my memory on this - there was some
discussion or investigation related to using virStateStop, but I since
that's gated by "WITH_DBUS" being defined, it wasn't clear if/how it
could be used (daemonStop is only called/setup if daemonRunStateInit has
set it up).
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/35] util: alloc: add macros for implementing automatic cleanup functionality
On Thu, 12 Jul 2018 at 22:09, Erik Skultety wrote:
>
> On Sat, Jun 30, 2018 at 02:30:05PM +0530, Sukrit Bhatnagar wrote:
> > New macros are introduced which help in adding GNU C's cleanup
> > attribute to variable declarations. Variables declared with these
> > macros will have their allocated memory freed automatically when
> > they go out of scope.
> >
> > Signed-off-by: Sukrit Bhatnagar
> > ---
> > src/util/viralloc.h | 44
> > 1 file changed, 44 insertions(+)
> >
> > diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> > index 69d0f90..5c1d0d5 100644
> > --- a/src/util/viralloc.h
> > +++ b/src/util/viralloc.h
> > @@ -596,4 +596,48 @@ void virAllocTestInit(void);
> > int virAllocTestCount(void);
> > void virAllocTestOOM(int n, int m);
> > void virAllocTestHook(void (*func)(int, void*), void *data);
> > +
> > +# define VIR_AUTOPTR_TYPE_NAME(type) type##AutoPtr
> > +# define VIR_AUTOPTR_FUNC_NAME(type) type##AutoPtrFree
> > +
> > +/**
> > + * VIR_DEFINE_AUTOPTR_FUNC:
> > + * @type: type of the variable to be freed automatically
> > + * @func: cleanup function to be automatically called
> > + *
> > + * This macro defines a function for automatic freeing of
> > + * resources allocated to a variable of type @type. This newly
> > + * defined function works as a necessary wrapper around @func.
> > + */
> > +# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > +typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
>
> So, it's not visible at first glance how ^this typedef is used...
>
> > +static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> > +{ \
> > +if (*_ptr) \
> > +(func)(*_ptr); \
> > +*_ptr = NULL; \
> > +} \
>
> ...therefore I'd write it explicitly as
>
> VIR_AUTOPTR_FUNC_NAME(type)(VIR_AUTOPTR_TYPE_NAME(type) *_ptr)
>
> > +
> > +# define VIR_AUTOPTR(type) \
> > +__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type
> > VIR_AUTOPTR_TYPE_NAME(type)
> > +
>
> Also, since we're going to use it like this:
> VIR_AUTOPTR(virDomainDef) foo
>
> instead of this:
> VIR_AUTOPTR(virDomainDefPtr) foo
>
> why do we need VIR_AUTOPTR_TYPE_NAME anyway, since you could just use
> "type *" in the VIR_AUTOPTR macro definition and that should work for external
> types as well.
I agree. We don't really need it. Here is how the code will look after
the changes
you suggested:
# define VIR_AUTOPTR_FUNC_NAME(type) type##AutoPtrFree
# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
{ \
if (*_ptr) \
(func)(*_ptr); \
*_ptr = NULL; \
} \
# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
# define VIR_AUTOPTR(type) \
__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type type *
Shall I proceed to send the first series of patches?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/35] util: alloc: add macros for implementing automatic cleanup functionality
On Sat, Jun 30, 2018 at 02:30:05PM +0530, Sukrit Bhatnagar wrote:
> New macros are introduced which help in adding GNU C's cleanup
> attribute to variable declarations. Variables declared with these
> macros will have their allocated memory freed automatically when
> they go out of scope.
>
> Signed-off-by: Sukrit Bhatnagar
> ---
> src/util/viralloc.h | 44
> 1 file changed, 44 insertions(+)
>
> diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> index 69d0f90..5c1d0d5 100644
> --- a/src/util/viralloc.h
> +++ b/src/util/viralloc.h
> @@ -596,4 +596,48 @@ void virAllocTestInit(void);
> int virAllocTestCount(void);
> void virAllocTestOOM(int n, int m);
> void virAllocTestHook(void (*func)(int, void*), void *data);
> +
> +# define VIR_AUTOPTR_TYPE_NAME(type) type##AutoPtr
> +# define VIR_AUTOPTR_FUNC_NAME(type) type##AutoPtrFree
> +
> +/**
> + * VIR_DEFINE_AUTOPTR_FUNC:
> + * @type: type of the variable to be freed automatically
> + * @func: cleanup function to be automatically called
> + *
> + * This macro defines a function for automatic freeing of
> + * resources allocated to a variable of type @type. This newly
> + * defined function works as a necessary wrapper around @func.
> + */
> +# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> +typedef type *VIR_AUTOPTR_TYPE_NAME(type); \
So, it's not visible at first glance how ^this typedef is used...
> +static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> +{ \
> +if (*_ptr) \
> +(func)(*_ptr); \
> +*_ptr = NULL; \
> +} \
...therefore I'd write it explicitly as
VIR_AUTOPTR_FUNC_NAME(type)(VIR_AUTOPTR_TYPE_NAME(type) *_ptr)
> +
> +# define VIR_AUTOPTR(type) \
> +__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type
> VIR_AUTOPTR_TYPE_NAME(type)
> +
Also, since we're going to use it like this:
VIR_AUTOPTR(virDomainDef) foo
instead of this:
VIR_AUTOPTR(virDomainDefPtr) foo
why do we need VIR_AUTOPTR_TYPE_NAME anyway, since you could just use
"type *" in the VIR_AUTOPTR macro definition and that should work for external
types as well.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 04/11] qemu_monitor: Introduce qemuMonitorCPUModelInfoInit and qemuMonitorCPUModelInfoFreeContents
Quoting Jiri Denemark (2018-07-12 08:13:07) > On Mon, Jul 09, 2018 at 22:56:48 -0500, Chris Venteicher wrote: > > These forms modify contents of a qemuMonitorCPUModelInfo structure but > > do not allocate or free the actual structure. > > > > Init - Initialize model name and empty properties within existing structure > > FreeContents - Free model name and properties within existing structure > > We call such function with "Clear" suffix, i.e., > qemuMonitorCPUModelInfoClear. > > But it is usually used when we have a structure stored somewhere > directly rather than having a pointer to it. And this was not the case > so far and I don't think there's any reason to introduce a code which > would need any of these functions. > > NACK to this patch. > Hi Jirka. I see what you mean about combining dependent patches... It would be helpful if this patch was coupled with the qemuMonitorGetCPUModelExpansion patch. Could I get you're thoughts on the qemuMonitorGetCPUModelExpansion patch to know what to do with this one? Specifically, I seem to need to send a full CPUModelInfo to QEMU (w/ model name + properties) and get a full CPUModelInfo back from QEMU (again w/ model name + expanded properties). I implemented this by rewriting the contents (property list) of the CPUModelInfo structure that is passed in to qemuMonitorGetCPUModelExpansion. I do a similar thing in qemuMonitorCPUModelInfoRemovePropByBoolValue... I rewrite the property list rather than allocating and returning a completely new CPUModelInfo for output. Is this consistent with other functions or would I be better off allocating and returning a completely new CPUModelInfo for the output? Or something else. Thanks for feedback. Chris > > Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 05/11] qemu_monitor: CPUModelExpansion on both model name and properties
On Mon, Jul 09, 2018 at 22:56:49 -0500, Chris Venteicher wrote:
> Send both model name and a set of features/properties to QEMU for
> expansion rather than just the model name.
>
> Required to expand name+props models of the form computed by baseline
> into fully expanded (all props/features listed) output.
This patch is doing too much at once and is quite hard to review.
> ---
> src/qemu/qemu_capabilities.c | 42 +-
> src/qemu/qemu_monitor.c | 38
> src/qemu/qemu_monitor.h | 5 ++-
> src/qemu/qemu_monitor_json.c | 69
> src/qemu/qemu_monitor_json.h | 7 ++--
> tests/cputest.c | 7 +++-
> 6 files changed, 122 insertions(+), 46 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 3d78e2e29b..72ab012a78 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2343,23 +2343,32 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
> qemuMonitorCPUModelInfoPtr modelInfo = NULL;
> qemuMonitorCPUModelInfoPtr nonMigratable = NULL;
> virHashTablePtr hash = NULL;
> -const char *model;
> +const char *model_name;
Why? If we really want to do this, it should go into a separate patch
explaining why it is needed.
> qemuMonitorCPUModelExpansionType type;
> virDomainVirtType virtType;
> virQEMUCapsHostCPUDataPtr cpuData;
> int ret = -1;
> +int err = -1;
We usually call such variable 'rc' and leave it uninitialized.
>
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION))
> return 0;
>
> if (tcg || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
> virtType = VIR_DOMAIN_VIRT_QEMU;
> -model = "max";
> +model_name = "max";
> } else {
> virtType = VIR_DOMAIN_VIRT_KVM;
> -model = "host";
> +model_name = "host";
> }
>
> +if ((VIR_ALLOC(modelInfo) < 0) ||
> +(VIR_ALLOC(nonMigratable) < 0))
> +goto cleanup;
> +
> +if ((qemuMonitorCPUModelInfoInit(model_name, modelInfo) < 0) ||
> +(qemuMonitorCPUModelInfoInit(model_name, nonMigratable) < 0))
> +goto cleanup;
Redundant parentheses. But anyway, VIR_ALLOC +
qemuMonitorCPUModelInfoInit combo should be replaced with a single
qemuMonitorCPUModelInfoNew function which would do both. As a bonus
point, you could never end up with a non-NULL modelInfo structure with
name == NULL.
> +
> cpuData = virQEMUCapsGetHostCPUData(qemuCaps, virtType);
>
> /* Some x86_64 features defined in cpu_map.xml use spelling which differ
> @@ -2372,16 +2381,31 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
> else
> type = QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC;
>
> -if (qemuMonitorGetCPUModelExpansion(mon, type, model, true, )
> < 0)
> +if ((err = qemuMonitorGetCPUModelExpansion(mon, type, true, modelInfo))
> < 0)
> goto cleanup;
>
> -/* Try to check migratability of each feature. */
> -if (modelInfo &&
> -qemuMonitorGetCPUModelExpansion(mon, type, model, false,
> -) < 0)
> +if (err == 1) {
> +ret = 0; /* Qemu can't do expansion 1, exit without error */
> +goto cleanup; /* We don't have info so don't update cpuData->info */
> +}
It's not very clear to me why qemuMonitorGetCPUModelExpansion needs to
report 1. A separate patch with an explanation would be better.
> +
> +if ((err = qemuMonitorGetCPUModelExpansion(mon, type, false,
> nonMigratable)) < 0)
> goto cleanup;
>
> -if (nonMigratable) {
> +/* Try to check migratability of each feature */
> +/* Expansion 1 sets migratable features true
> + * Expansion 2 sets migratable and non-migratable features true
> + * (non-migratable set true only in some archs like X86)
> + *
> + * If delta between Expansion 1 and 2 exists...
> + * - both migratable and non-migratable features set prop->value = true
> + * - migratable features set prop->migatable = VIR_TRISTATE_BOOL_YES
> + * - non-migratable features set prop->migatable = VIR_TRISTATE_BOOL_NO
> + */
> +if (err == 0) {
> +/* Expansion 2 succeded
> + * Qemu expanded both migratable and nonMigratable features */
This comment seems redundant. It's pretty clear from the code that both
expansions were successful at this point.
> +
> qemuMonitorCPUPropertyPtr prop;
> qemuMonitorCPUPropertyPtr nmProp;
> size_t i;
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 2d9297c3a7..91b946c8b4 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3619,20 +3619,46 @@ qemuMonitorCPUDefInfoFree(qemuMonitorCPUDefInfoPtr
> cpu)
> }
>
>
> +/*
> + * type static:
> + * Expand to static base model + delta property changes
> + * Returned model is invariant and
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On 12.07.2018 08:32, Markus Armbruster wrote: > Daniel P. Berrangé writes: [...] >> For libvirt, I think whenever something is proposed for deprecation >> we could just CC libvir-list, or ask one of the libvirt people to >> confirm its not being used. If it is, then we should file BZ against >> libvirt. > > Makes sense, but relying on developers getting their cc: right every > time is a setting us up for failure. > > Our tool to help with getting cc: wrong less often is the MAINTAINERS > file. Could one of the libvirt developers watch changes to qemu-doc > appendix "Deprecated features"? Would putting the appendix in its own > .texi help with that? Sound like a good idea to put the appendix in its own texi file. Then add an "R: libvir-list" entry for that file to MAINTAINERS and we should be fine (at least for the people who use the get_maintainers.pl script). Thomas -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] New virsh feature: domif-setlink --domain --interface --state completer
On 07/12/2018 04:07 PM, Simon Kobyda wrote:
> After you go through command mentioned above, completer
> finds what state the device is currently in, and suggests
> an opposite state.
>
> Signed-off-by: Simon Kobyda
> ---
>
> Changes in V2:
> - Added "Signed-off-by"
> - Changed format of commit message to make it more
> readable
>
> tools/virsh-completer.c | 73 -
> tools/virsh-completer.h | 4 +++
> tools/virsh-domain.c| 1 +
> 3 files changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
> index 2327e08340..e32fd82211 100644
> --- a/tools/virsh-completer.c
> +++ b/tools/virsh-completer.c
> @@ -32,6 +32,7 @@
> #include "internal.h"
> #include "virutil.h"
> #include "viralloc.h"
> +#include "virmacaddr.h"
> #include "virstring.h"
> #include "virxml.h"
>
> @@ -750,7 +751,6 @@ virshDomainEventNameCompleter(vshControl *ctl
> ATTRIBUTE_UNUSED,
> return NULL;
> }
>
> -
This is a spurious change and has nothing to do with the feature you're
proposing.
> char **
> virshPoolEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED,
> const vshCmd *cmd ATTRIBUTE_UNUSED,
> @@ -776,6 +776,77 @@ virshPoolEventNameCompleter(vshControl *ctl
> ATTRIBUTE_UNUSED,
> return NULL;
> }
>
> +char **
> +virshDomainInterfaceStateCompleter(vshControl *ctl ATTRIBUTE_UNUSED,
> +const vshCmd *cmd ATTRIBUTE_UNUSED,
> +unsigned int flags)
Misaligned function arguments. Also, I'm not quite sure why you marked
@ctl as unused when it's being used at the very next line. Same goes for
@cmd.
> +{
> +virshControlPtr priv = ctl->privData;
> +const char *iface = NULL;
> +char **ret = NULL;
> +xmlDocPtr xml = NULL;
> +virMacAddr macaddr;
> +char *state = NULL;
> +char *xpath = NULL;
> +char macstr[18] = "";
What's wrong with VIR_MAC_STRING_BUFLEN?
> +xmlXPathContextPtr ctxt = NULL;
> +xmlNodePtr *interfaces = NULL;
> +int ninterfaces;
> +
> +virCheckFlags(0, NULL);
> +
> +if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
> +return NULL;
> +
> +if (vshCommandOptStringReq(ctl, cmd, "interface", ) < 0)
> +goto cleanup;
> +
> +if (virshDomainGetXML(ctl, cmd, flags, , ) < 0)
> +goto cleanup;
> +
> +/* normalize the mac addr */
> +if (virMacAddrParse(iface, ) == 0)
> +virMacAddrFormat(, macstr);
> +
> +if (virAsprintf(, "/domain/devices/interface[(mac/@address = '%s')
> or "
> +" (target/@dev =
> '%s')]",
> + macstr, iface) < 0)
> +goto cleanup;
> +
> +if ((ninterfaces = virXPathNodeSet(xpath, ctxt, )) < 0)
> +goto cleanup;
> +
> +if (ninterfaces != 1)
> +goto cleanup;
> +
> +ctxt->node = interfaces[0];
> +
> +if (VIR_ALLOC_N(ret, 2) < 0)
> +goto error;
> +
> +if ((state = virXPathString("string(./link/@state)", ctxt)) &&
> +STREQ(state, "down")) {
> +if (VIR_STRDUP(ret[0], "up") < 0)
> +goto error;
> +} else {
> +if (VIR_STRDUP(ret[0], "down") < 0)
> +goto error;
> +}
> +
> + cleanup:
> +VIR_FREE(state);
> +VIR_FREE(interfaces);
> +VIR_FREE(xpath);
> +xmlXPathFreeContext(ctxt);
> +xmlFreeDoc(xml);
> +return ret;
> +
> + error:
> +virStringListFree(ret);
> +ret = NULL;
> +goto cleanup;
> +}
> +
>
Fixed all the small nits I've found, ACKed and pushed.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v2 12/12] lcitool: Implement the 'dockerfile' action
This is basically the exact same algorithm used by the
Ansible playbooks to process package mappings, implemented
in pure Python.
Signed-off-by: Andrea Bolognani
---
guests/lcitool | 86 ++
1 file changed, 80 insertions(+), 6 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index d42b7e7..61cae97 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -299,7 +299,10 @@ class Application:
hosts list all known hosts
projects list all known projects
-glob patterns are supported for HOSTS
+uncommon actions:
+ dockerfile generate Dockerfile (doesn't access the host)
+
+glob patterns are supported for HOSTS and PROJECTS
"""),
)
self._parser.add_argument(
@@ -313,16 +316,21 @@ class Application:
metavar = "HOSTS",
help = "list of hosts to act on",
)
+self._parser.add_argument(
+"-p",
+metavar = "PROJECTS",
+help = "list of projects to consider",
+)
-def _action_list(self, hosts):
+def _action_hosts(self, hosts, projects):
for host in self._inventory.expand_pattern("all"):
print(host)
-def _action_projects(self, hosts):
+def _action_projects(self, hosts, projects):
for project in self._projects.expand_pattern("all"):
print(project)
-def _action_install(self, hosts):
+def _action_install(self, hosts, projects):
flavor = self._config.get_flavor()
for host in self._inventory.expand_pattern(hosts):
@@ -380,7 +388,7 @@ class Application:
except:
raise Error("Failed to install '{}'".format(host))
-def _action_update(self, hosts):
+def _action_update(self, hosts, projects):
flavor = self._config.get_flavor()
vault_pass_file = self._config.get_vault_password_file()
root_pass_file = self._config.get_root_password_file()
@@ -409,15 +417,81 @@ class Application:
except:
raise Error("Failed to update '{}'".format(hosts))
+def _action_dockerfile(self, hosts, projects):
+mappings = self._projects.get_mappings()
+
+hosts = self._inventory.expand_pattern(hosts)
+if len(hosts) > 1:
+raise Error("Can't generate Dockerfile for multiple hosts")
+host = hosts[0]
+
+facts = self._inventory.get_facts(host)
+package_format = facts["package_format"]
+os_name = facts["os_name"]
+os_full = os_name + str(facts["os_version"])
+
+if package_format != "deb" and package_format != "rpm":
+raise Error("Host {} doesn't support Dockerfiles".format(host))
+
+projects = self._projects.expand_pattern(projects)
+for project in projects:
+if project not in facts['projects']:
+raise Error(
+"Host {} doesn't support project {}".format(
+host,
+project,
+)
+)
+
+temp = {}
+
+# We need to add the base project manually here: the standard
+# machinery hides it because it's an implementation detail
+for project in projects + [ "base" ]:
+for package in self._projects.get_packages(project):
+if "default" in mappings[package]:
+temp[package] = mappings[package]["default"]
+if package_format in mappings[package]:
+temp[package] = mappings[package][package_format]
+if os_name in mappings[package]:
+temp[package] = mappings[package][os_name]
+if os_full in mappings[package]:
+temp[package] = mappings[package][os_full]
+
+flattened = []
+for item in temp:
+if temp[item] != None and temp[item] not in flattened:
+flattened += [ temp[item] ]
+
+print("FROM {}".format(facts['docker_base']))
+
+sys.stdout.write("ENV PACKAGES ")
+sys.stdout.write(" \\\n ".join(sorted(flattened)))
+
+if package_format == "deb":
+sys.stdout.write(textwrap.dedent("""
+RUN apt-get update && \\
+apt-get install -y ${PACKAGES} && \\
+apt-get autoremove -y && \\
+apt-get autoclean -y
+"""))
+elif package_format == "rpm":
+sys.stdout.write(textwrap.dedent("""
+RUN yum install -y ${PACKAGES} && \\
+yum autoremove -y && \\
+yum clean all -y
+"""))
+
def run(self):
cmdline = self._parser.parse_args()
action = cmdline.a
hosts = cmdline.h
+projects = cmdline.p
method =
[libvirt] [jenkins-ci PATCH v2 10/12] lcitool: Add projects information handling
The original tool's limited scope meant loadins this
information was not needed, but we're going to start
making use of it pretty soon.
Signed-off-by: Andrea Bolognani
---
guests/lcitool | 47 +++
1 file changed, 47 insertions(+)
diff --git a/guests/lcitool b/guests/lcitool
index d82c36f..3bd5fa7 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -233,11 +233,58 @@ class Inventory:
def get_facts(self, host):
return self._facts[host]
+class Projects:
+
+def __init__(self):
+try:
+with open("./vars/mappings.yml", "r") as f:
+mappings = yaml.load(f)
+self._mappings = mappings["mappings"]
+except:
+raise Error("Can't load mappings")
+
+self._packages = {}
+source = "./vars/projects/"
+for item in os.listdir(source):
+yaml_path = os.path.join(source, item)
+if not os.path.isfile(yaml_path):
+continue
+if not yaml_path.endswith(".yml"):
+continue
+
+project = os.path.splitext(item)[0]
+
+try:
+with open(yaml_path, "r") as f:
+packages = yaml.load(f)
+self._packages[project] = packages["packages"]
+except:
+raise Error("Can't load packages for '{}'".format(project))
+
+def expand_pattern(self, pattern):
+projects = Util.expand_pattern(pattern, self._packages, "project")
+
+# Some projects are internal implementation details and should
+# not be exposed to the user
+internal_projects = [ "base", "blacklist", "jenkins" ]
+for project in internal_projects:
+if project in projects:
+projects.remove(project)
+
+return projects
+
+def get_mappings(self):
+return self._mappings
+
+def get_packages(self, project):
+return self._packages[project]
+
class Application:
def __init__(self):
self._config = Config()
self._inventory = Inventory()
+self._projects = Projects()
self._parser = argparse.ArgumentParser(
conflict_handler = "resolve",
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v2 04/12] lcitool: Add inventory handling
We use an actual YAML parser this time around, and bring
the behavior more in line with what Ansible is doing, so
interoperability should be more solid overall.
New in this implementation is more flexibility in defining
host lists, including support for explicit lists as well
as glob patterns.
Signed-off-by: Andrea Bolognani
---
guests/lcitool | 91 ++
1 file changed, 91 insertions(+)
diff --git a/guests/lcitool b/guests/lcitool
index e03b388..e90a33b 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -19,11 +19,13 @@
import argparse
import crypt
+import fnmatch
import os
import random
import string
import sys
import textwrap
+import yaml
# This is necessary to maintain Python 2.7 compatibility
try:
@@ -44,6 +46,32 @@ class Util:
salt = "".join(random.choice(alphabeth) for x in range(0, 16))
return "$6${}$".format(salt)
+@staticmethod
+def expand_pattern(pattern, source, name):
+if pattern == None:
+raise Error("Missing {} list".format(name))
+
+if pattern == "all":
+pattern = "*"
+
+# This works correctly for single items as well as more complex
+# cases such as explicit lists, glob patterns and any combination
+# of the above
+matches = []
+for partial_pattern in pattern.split(","):
+
+partial_matches = []
+for item in source:
+if fnmatch.fnmatch(item, partial_pattern):
+partial_matches += [item]
+
+if len(partial_matches) == 0:
+raise Error("Invalid {} list '{}'".format(name, pattern))
+
+matches += partial_matches
+
+return sorted(set(matches))
+
class Config:
def _get_config_file(self, name):
@@ -142,10 +170,73 @@ class Config:
return root_hash_file
+class Inventory:
+
+def __init__(self):
+try:
+parser = configparser.SafeConfigParser()
+parser.read("./ansible.cfg")
+inventory_path = parser.get("defaults", "inventory")
+except:
+raise Error("Can't find inventory location in ansible.cfg")
+
+self._facts = {}
+try:
+# We can only deal with trivial inventories, but that's
+# all we need right now and we can expand support further
+# later on if necessary
+with open(inventory_path, "r") as f:
+for line in f:
+host = line.strip()
+self._facts[host] = {}
+except:
+raise Error(
+"Missing or invalid inventory ({})".format(
+inventory_path,
+)
+)
+
+for host in self._facts:
+try:
+self._facts[host] = self._read_all_facts(host)
+self._facts[host]["inventory_hostname"] = host
+except:
+raise Error("Can't load facts for '{}'".format(host))
+
+def _add_facts_from_file(self, facts, yaml_path):
+with open(yaml_path, "r") as f:
+some_facts = yaml.load(f)
+for fact in some_facts:
+facts[fact] = some_facts[fact]
+
+def _read_all_facts(self, host):
+facts = {}
+
+# We load from group_vars/ first and host_vars/ second, sorting
+# files alphabetically; doing so should result in our view of
+# the facts matching Ansible's
+for source in ["./group_vars/all/", "./host_vars/{}/".format(host)]:
+for item in sorted(os.listdir(source)):
+yaml_path = os.path.join(source, item)
+if not os.path.isfile(yaml_path):
+continue
+if not yaml_path.endswith(".yml"):
+continue
+self._add_facts_from_file(facts, yaml_path)
+
+return facts
+
+def expand_pattern(self, pattern):
+return Util.expand_pattern(pattern, self._facts, "host")
+
+def get_facts(self, host):
+return self._facts[host]
+
class Application:
def __init__(self):
self._config = Config()
+self._inventory = Inventory()
self._parser = argparse.ArgumentParser(
conflict_handler = "resolve",
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v2 05/12] lcitool: Implement the 'hosts' action
This replaces the 'list' action from the original
implementation. We're going to list more than just hosts
over time, so a more specific name is warranted.
Signed-off-by: Andrea Bolognani
---
guests/lcitool | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/guests/lcitool b/guests/lcitool
index e90a33b..f11b92e 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -243,7 +243,8 @@ class Application:
formatter_class = argparse.RawDescriptionHelpFormatter,
description = "libvirt CI guest management tool",
epilog = textwrap.dedent("""
-supported actions:
+informational actions:
+ hosts list all known hosts
"""),
)
self._parser.add_argument(
@@ -253,6 +254,10 @@ class Application:
help = "action to perform (see below)",
)
+def _action_list(self):
+for host in self._inventory.expand_pattern("all"):
+print(host)
+
def run(self):
cmdline = self._parser.parse_args()
action = cmdline.a
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v2 06/12] lcitool: Implement the 'install' action
Signed-off-by: Andrea Bolognani
---
guests/lcitool | 74 --
1 file changed, 72 insertions(+), 2 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index f11b92e..486f82f 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -23,6 +23,7 @@ import fnmatch
import os
import random
import string
+import subprocess
import sys
import textwrap
import yaml
@@ -243,8 +244,13 @@ class Application:
formatter_class = argparse.RawDescriptionHelpFormatter,
description = "libvirt CI guest management tool",
epilog = textwrap.dedent("""
+common actions:
+ install perform unattended host installation
+
informational actions:
hosts list all known hosts
+
+glob patterns are supported for HOSTS
"""),
)
self._parser.add_argument(
@@ -253,19 +259,83 @@ class Application:
required = True,
help = "action to perform (see below)",
)
+self._parser.add_argument(
+"-h",
+metavar = "HOSTS",
+help = "list of hosts to act on",
+)
-def _action_list(self):
+def _action_list(self, hosts):
for host in self._inventory.expand_pattern("all"):
print(host)
+def _action_install(self, hosts):
+flavor = self._config.get_flavor()
+
+for host in self._inventory.expand_pattern(hosts):
+facts = self._inventory.get_facts(host)
+
+# Both memory size and disk size are stored as GiB in the
+# inventory, but virt-install expects the disk size in GiB
+# and the memory size in *MiB*, so perform conversion here
+memory_arg = str(int(facts["install_memory_size"]) * 1024)
+
+vcpus_arg = str(facts["install_vcpus"])
+
+disk_arg = "size={},pool={},bus=virtio".format(
+facts["install_disk_size"],
+facts["install_storage_pool"],
+)
+network_arg = "network={},model=virtio".format(
+facts["install_network"],
+)
+
+# preseed files must use a well-known name to be picked up by
+# d-i; for kickstart files, we can use whatever name we please
+# but we need to point anaconda in the right direction through
+# a kernel argument
+extra_arg = "console=ttyS0 ks=file:/{}".format(
+facts["install_config"],
+)
+
+cmd = [
+"virt-install",
+"--name", host,
+"--location", facts["install_url"],
+"--virt-type", facts["install_virt_type"],
+"--arch", facts["install_arch"],
+"--machine", facts["install_machine"],
+"--cpu", facts["install_cpu_model"],
+"--vcpus", vcpus_arg,
+"--memory", memory_arg,
+"--disk", disk_arg,
+"--network", network_arg,
+"--graphics", "none",
+"--console", "pty",
+"--sound", "none",
+"--initrd-inject", facts["install_config"],
+"--extra-args", extra_arg,
+"--wait", "0",
+]
+
+# Only configure autostart for the guest for the jenkins flavor
+if flavor == "jenkins":
+cmd += [ "--autostart" ]
+
+try:
+subprocess.check_call(cmd)
+except:
+raise Error("Failed to install '{}'".format(host))
+
def run(self):
cmdline = self._parser.parse_args()
action = cmdline.a
+hosts = cmdline.h
method = "_action_{}".format(action.replace("-", "_"))
if hasattr(self, method):
-getattr(self, method).__call__()
+getattr(self, method).__call__(hosts)
else:
raise Error("Invalid action '{}'".format(action))
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v2 07/12] lcitool: Implement the 'update' action
The 'prepare' alias was kinda redundant and offered
dubious value, so it has been dropped.
Signed-off-by: Andrea Bolognani
---
guests/lcitool | 30 ++
1 file changed, 30 insertions(+)
diff --git a/guests/lcitool b/guests/lcitool
index 486f82f..d82c36f 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -246,6 +246,7 @@ class Application:
epilog = textwrap.dedent("""
common actions:
install perform unattended host installation
+ update prepare hosts and keep them updated
informational actions:
hosts list all known hosts
@@ -327,6 +328,35 @@ class Application:
except:
raise Error("Failed to install '{}'".format(host))
+def _action_update(self, hosts):
+flavor = self._config.get_flavor()
+vault_pass_file = self._config.get_vault_password_file()
+root_pass_file = self._config.get_root_password_file()
+
+ansible_hosts = ",".join(self._inventory.expand_pattern(hosts))
+
+extra_vars = "flavor={} root_password_file={}".format(
+flavor,
+root_pass_file,
+)
+
+cmd = [ "ansible-playbook" ]
+
+# Provide the vault password if available
+if vault_pass_file is not None:
+cmd += [ "--vault-password-file", vault_pass_file ]
+
+cmd += [
+"--limit", ansible_hosts,
+"--extra-vars", extra_vars,
+"./site.yml",
+]
+
+try:
+subprocess.check_call(cmd)
+except:
+raise Error("Failed to update '{}'".format(hosts))
+
def run(self):
cmdline = self._parser.parse_args()
action = cmdline.a
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v2 09/12] guests: Add Docker-related information to the inventory
Signed-off-by: Andrea Bolognani --- guests/host_vars/libvirt-centos-7/docker.yml | 2 ++ guests/host_vars/libvirt-debian-8/docker.yml | 2 ++ guests/host_vars/libvirt-debian-9/docker.yml | 2 ++ guests/host_vars/libvirt-debian-sid/docker.yml | 2 ++ guests/host_vars/libvirt-fedora-27/docker.yml | 2 ++ guests/host_vars/libvirt-fedora-28/docker.yml | 2 ++ guests/host_vars/libvirt-fedora-rawhide/docker.yml | 2 ++ guests/host_vars/libvirt-ubuntu-16/docker.yml | 2 ++ guests/host_vars/libvirt-ubuntu-18/docker.yml | 2 ++ 9 files changed, 18 insertions(+) create mode 100644 guests/host_vars/libvirt-centos-7/docker.yml create mode 100644 guests/host_vars/libvirt-debian-8/docker.yml create mode 100644 guests/host_vars/libvirt-debian-9/docker.yml create mode 100644 guests/host_vars/libvirt-debian-sid/docker.yml create mode 100644 guests/host_vars/libvirt-fedora-27/docker.yml create mode 100644 guests/host_vars/libvirt-fedora-28/docker.yml create mode 100644 guests/host_vars/libvirt-fedora-rawhide/docker.yml create mode 100644 guests/host_vars/libvirt-ubuntu-16/docker.yml create mode 100644 guests/host_vars/libvirt-ubuntu-18/docker.yml diff --git a/guests/host_vars/libvirt-centos-7/docker.yml b/guests/host_vars/libvirt-centos-7/docker.yml new file mode 100644 index 000..59f7f12 --- /dev/null +++ b/guests/host_vars/libvirt-centos-7/docker.yml @@ -0,0 +1,2 @@ +--- +docker_base: centos:centos7 diff --git a/guests/host_vars/libvirt-debian-8/docker.yml b/guests/host_vars/libvirt-debian-8/docker.yml new file mode 100644 index 000..235f0fd --- /dev/null +++ b/guests/host_vars/libvirt-debian-8/docker.yml @@ -0,0 +1,2 @@ +--- +docker_base: debian:8 diff --git a/guests/host_vars/libvirt-debian-9/docker.yml b/guests/host_vars/libvirt-debian-9/docker.yml new file mode 100644 index 000..0b4ccee --- /dev/null +++ b/guests/host_vars/libvirt-debian-9/docker.yml @@ -0,0 +1,2 @@ +--- +docker_base: debian:9 diff --git a/guests/host_vars/libvirt-debian-sid/docker.yml b/guests/host_vars/libvirt-debian-sid/docker.yml new file mode 100644 index 000..e20a37e --- /dev/null +++ b/guests/host_vars/libvirt-debian-sid/docker.yml @@ -0,0 +1,2 @@ +--- +docker_base: debian:sid diff --git a/guests/host_vars/libvirt-fedora-27/docker.yml b/guests/host_vars/libvirt-fedora-27/docker.yml new file mode 100644 index 000..dcada18 --- /dev/null +++ b/guests/host_vars/libvirt-fedora-27/docker.yml @@ -0,0 +1,2 @@ +--- +docker_base: fedora:27 diff --git a/guests/host_vars/libvirt-fedora-28/docker.yml b/guests/host_vars/libvirt-fedora-28/docker.yml new file mode 100644 index 000..a874018 --- /dev/null +++ b/guests/host_vars/libvirt-fedora-28/docker.yml @@ -0,0 +1,2 @@ +--- +docker_base: fedora:28 diff --git a/guests/host_vars/libvirt-fedora-rawhide/docker.yml b/guests/host_vars/libvirt-fedora-rawhide/docker.yml new file mode 100644 index 000..39dda1c --- /dev/null +++ b/guests/host_vars/libvirt-fedora-rawhide/docker.yml @@ -0,0 +1,2 @@ +--- +docker_base: fedora:rawhide diff --git a/guests/host_vars/libvirt-ubuntu-16/docker.yml b/guests/host_vars/libvirt-ubuntu-16/docker.yml new file mode 100644 index 000..2d4eb25 --- /dev/null +++ b/guests/host_vars/libvirt-ubuntu-16/docker.yml @@ -0,0 +1,2 @@ +--- +docker_base: ubuntu:16.04 diff --git a/guests/host_vars/libvirt-ubuntu-18/docker.yml b/guests/host_vars/libvirt-ubuntu-18/docker.yml new file mode 100644 index 000..13d6cc1 --- /dev/null +++ b/guests/host_vars/libvirt-ubuntu-18/docker.yml @@ -0,0 +1,2 @@ +--- +docker_base: ubuntu:18.04 -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v2 01/12] lcitool: Drop shell implementation
We're going to rewrite the script completely, and getting
the current implementation out of the way firts will make
the diffs more reasonable.
Signed-off-by: Andrea Bolognani
---
guests/lcitool | 227 -
1 file changed, 227 deletions(-)
delete mode 100755 guests/lcitool
diff --git a/guests/lcitool b/guests/lcitool
deleted file mode 100755
index 0c1520e..000
--- a/guests/lcitool
+++ /dev/null
@@ -1,227 +0,0 @@
-#!/bin/sh
-
-# ---
-# Utility functions
-# ---
-
-# die MESSAGE
-#
-# Abort the program after displaying $MESSAGE on standard error.
-die() {
-echo "$1" >&2
-exit 1
-}
-
-# hash_file PASS_FILE
-#
-# Generate a password hash from the contents of PASS_FILE.
-hash_file() {
-PASS_FILE="$1"
-
-perl -le '
-my @chars = ("A".."Z", "a".."z", "0".."9");
-my $salt; $salt .= $chars[rand @chars] for 1..16;
-my $handle; open($handle, "'"$PASS_FILE"'");
-my $pass = <$handle>; chomp($pass);
-print crypt("$pass", "\$6\$$salt\$");'
-}
-
-# yaml_var FILE VAR
-#
-# Read $FILE and output the value of YAML variable $VAR. Only trivial YAML
-# values are supported, eg. strings and numbers that don't depend on the
-# value of other variables. That's enough for our use case.
-yaml_var() {
-grep "^$2:\\s*" "$1" 2>/dev/null | tail -1 | sed "s/$2:\\s*//g"
-}
-
-# load_install_config FILE
-#
-# Read all known configuration variables from $FILE and set them in the
-# environment. Configuration variables that have already been set in
-# the environment will not be updated.
-load_install_config() {
-INSTALL_URL=${INSTALL_URL:-$(yaml_var "$1" install_url)}
-INSTALL_CONFIG=${INSTALL_CONFIG:-$(yaml_var "$1" install_config)}
-INSTALL_VIRT_TYPE=${INSTALL_VIRT_TYPE:-$(yaml_var "$1" install_virt_type)}
-INSTALL_ARCH=${INSTALL_ARCH:-$(yaml_var "$1" install_arch)}
-INSTALL_MACHINE=${INSTALL_MACHINE:-$(yaml_var "$1" install_machine)}
-INSTALL_CPU_MODEL=${INSTALL_CPU_MODEL:-$(yaml_var "$1" install_cpu_model)}
-INSTALL_VCPUS=${INSTALL_VCPUS:-$(yaml_var "$1" install_vcpus)}
-INSTALL_MEMORY_SIZE=${INSTALL_MEMORY_SIZE:-$(yaml_var "$1"
install_memory_size)}
-INSTALL_DISK_SIZE=${INSTALL_DISK_SIZE:-$(yaml_var "$1" install_disk_size)}
-INSTALL_STORAGE_POOL=${INSTALL_STORAGE_POOL:-$(yaml_var "$1"
install_storage_pool)}
-INSTALL_NETWORK=${INSTALL_NETWORK:-$(yaml_var "$1" install_network)}
-}
-
-# load_config
-#
-# Read tool configuration and perform the necessary validation.
-load_config() {
-CONFIG_DIR="$HOME/.config/$PROGRAM_NAME"
-
-mkdir -p "$CONFIG_DIR" >/dev/null 2>&1 || {
-die "$PROGRAM_NAME: $CONFIG_DIR: Unable to create config directory"
-}
-
-FLAVOR_FILE="$CONFIG_DIR/flavor"
-VAULT_PASS_FILE="$CONFIG_DIR/vault-password"
-ROOT_PASS_FILE="$CONFIG_DIR/root-password"
-
-# Two flavors are supported: test (default) and jenkins. Read the
-# flavor from configuration, validate it and write it back in case
-# it was not present
-FLAVOR="$(cat "$FLAVOR_FILE" 2>/dev/null)"
-FLAVOR=${FLAVOR:-test}
-test "$FLAVOR" = test || test "$FLAVOR" = jenkins || {
-die "$PROGRAM_NAME: Invalid flavor '$FLAVOR'"
-}
-echo "$FLAVOR" >"$FLAVOR_FILE" || {
-die "$PROGRAM_NAME: $FLAVOR_FILE: Unable to save flavor"
-}
-
-test "$FLAVOR" = jenkins && {
-# The vault password is only needed for the jenkins flavor, so only
-# validate it in that case
-test -f "$VAULT_PASS_FILE" && test "$(cat "$VAULT_PASS_FILE")" || {
-die "$PROGRAM_NAME: $VAULT_PASS_FILE: Missing or invalid password"
-}
-} || {
-# For other flavors, undefine the variable so that Ansible
-# will not try to read the file at all
-VAULT_PASS_FILE=
-}
-
-# Make sure the root password has been configured properly
-test -f "$ROOT_PASS_FILE" && test "$(cat "$ROOT_PASS_FILE")" || {
-die "$PROGRAM_NAME: $ROOT_PASS_FILE: Missing or invalid password"
-}
-
-ROOT_HASH_FILE="$CONFIG_DIR/.root-password.hash"
-
-# Regenerate root password hash. Ansible expects passwords as hashes but
-# doesn't provide a built-in facility to generate one from plain text
-hash_file "$ROOT_PASS_FILE" >"$ROOT_HASH_FILE" || {
-die "$PROGRAM_NAME: Failure while hashing root password"
-}
-}
-
-# --
-# User-visible actions
-# --
-
-do_help() {
-echo "\
-Usage: $CALL_NAME ACTION [OPTIONS]
-
-Actions:
- list List known guests
- install GUESTInstall GUEST
- prepare GUEST|allPrepare or update GUEST. Can be run multiple times
- update GUEST|allAlias for prepare
- help Display this help"
-}
-
-do_list() {
-# List all guests present in the inventory. Skip group names,
-# comments and empty lines
-grep -vE '^#|^\[|^$' inventory | sort -u
-}
-
[libvirt] [jenkins-ci PATCH v2 03/12] lcitool: Add tool configuration handling
The on-disk configuration format and its behavior
are fully backwards compatible with the previous
implementation.
Signed-off-by: Andrea Bolognani
---
guests/lcitool | 112 +
1 file changed, 112 insertions(+)
diff --git a/guests/lcitool b/guests/lcitool
index 1cba8ad..e03b388 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -18,6 +18,10 @@
# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
import argparse
+import crypt
+import os
+import random
+import string
import sys
import textwrap
@@ -32,9 +36,117 @@ class Error(Exception):
def __init__(self, message):
self.message = message
+class Util:
+
+@staticmethod
+def mksalt():
+alphabeth = string.ascii_letters + string.digits
+salt = "".join(random.choice(alphabeth) for x in range(0, 16))
+return "$6${}$".format(salt)
+
+class Config:
+
+def _get_config_file(self, name):
+try:
+config_dir = os.environ["XDG_CONFIG_HOME"]
+except KeyError:
+config_dir = os.path.join(os.environ["HOME"], ".config/")
+config_dir = os.path.join(config_dir, "lcitool/")
+
+# Create the directory if it doesn't already exist
+if not os.path.exists(config_dir):
+try:
+os.mkdir(config_dir)
+except:
+raise Error(
+"Can't create configuration directory ({})".format(
+config_dir,
+)
+)
+
+return os.path.join(config_dir, name)
+
+def get_flavor(self):
+flavor_file = self._get_config_file("flavor")
+
+try:
+with open(flavor_file, "r") as f:
+flavor = f.readline().strip()
+except:
+# If the flavor has not been configured, we choose the default
+# and store it on disk to ensure consistent behavior from now on
+flavor = "test"
+try:
+with open(flavor_file, "w") as f:
+f.write("{}\n".format(flavor))
+except:
+raise Error(
+"Can't write flavor file ({})".format(
+flavor_file,
+)
+)
+
+if flavor != "test" and flavor != "jenkins":
+raise Error("Invalid flavor '{}'".format(flavor))
+
+return flavor
+
+def get_vault_password_file(self):
+vault_pass_file = None
+
+# The vault password is only needed for the jenkins flavor, but in
+# that case we want to make sure there's *something* in there
+if self.get_flavor() != "test":
+vault_pass_file = self._get_config_file("vault-password")
+
+try:
+with open(vault_pass_file, 'r') as f:
+if len(f.readline().strip()) == 0:
+raise ValueError
+except:
+raise Error(
+"Missing or invalid vault password file ({})".format(
+vault_pass_file,
+)
+)
+
+return vault_pass_file
+
+def get_root_password_file(self):
+root_pass_file = self._get_config_file("root-password")
+root_hash_file = self._get_config_file(".root-password.hash")
+
+try:
+with open(root_pass_file, "r") as f:
+root_pass = f.readline().strip()
+except:
+raise Error(
+"Missing or invalid root password file ({})".format(
+root_pass_file,
+)
+)
+
+# The hash will be different every time we run, but that doesn't
+# matter - it will still validate the correct root password
+root_hash = crypt.crypt(root_pass, Util.mksalt())
+
+try:
+with open(root_hash_file, "w") as f:
+f.write("{}\n".format(root_hash))
+except:
+raise Error(
+"Can't write hashed root password file ({})".format(
+root_hash_file,
+)
+)
+
+return root_hash_file
+
class Application:
def __init__(self):
+self._config = Config()
+
self._parser = argparse.ArgumentParser(
conflict_handler = "resolve",
formatter_class = argparse.RawDescriptionHelpFormatter,
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v2 08/12] guests: Update documentation
Signed-off-by: Andrea Bolognani --- guests/README.markdown | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/guests/README.markdown b/guests/README.markdown index bc780f3..4a40619 100644 --- a/guests/README.markdown +++ b/guests/README.markdown @@ -6,16 +6,16 @@ of the guests used by the Jenkins-based libvirt CI environment. There are two steps to bringing up a guest: -* `./lcitool install $guest` will perform an unattended installation +* `./lcitool -a install -h $guest` will perform an unattended installation of `$guest`. Not all guests can be installed this way: see the "FreeBSD" section below; -* `./lcitool prepare $guest` will go through all the post-installation +* `./lcitool -a update -h $guest` will go through all the post-installation configuration steps required to make the newly-created guest usable; Once those steps have been performed, maintainance will involve running: -* `./lcitool update $guest` +* `./lcitool -a update -h $guest` periodically to ensure the guest configuration is sane and all installed packages are updated. @@ -40,7 +40,7 @@ you'll want to use the `libvirt_guest` variant of the plugin. To keep guests up to date over time, it's recommended to have an entry along the lines of -0 0 * * * cd ~/libvirt-jenkins-ci/guests && ./lcitool update all +0 0 * * * cd ~/libvirt-jenkins-ci/guests && ./lcitool -a update -h all in your crontab. -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v2 11/12] lcitool: Implement the 'projects' action
Signed-off-by: Andrea Bolognani
---
guests/lcitool | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/guests/lcitool b/guests/lcitool
index 3bd5fa7..d42b7e7 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -296,7 +296,8 @@ class Application:
update prepare hosts and keep them updated
informational actions:
- hosts list all known hosts
+ hosts list all known hosts
+ projects list all known projects
glob patterns are supported for HOSTS
"""),
@@ -317,6 +318,10 @@ class Application:
for host in self._inventory.expand_pattern("all"):
print(host)
+def _action_projects(self, hosts):
+for project in self._projects.expand_pattern("all"):
+print(project)
+
def _action_install(self, hosts):
flavor = self._config.get_flavor()
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v2 02/12] lcitool: Stub out Python implementation
Doesn't do much right now, but it's a start :)
Signed-off-by: Andrea Bolognani
---
guests/lcitool | 69 ++
1 file changed, 69 insertions(+)
create mode 100755 guests/lcitool
diff --git a/guests/lcitool b/guests/lcitool
new file mode 100755
index 000..1cba8ad
--- /dev/null
+++ b/guests/lcitool
@@ -0,0 +1,69 @@
+#!/usr/bin/env python
+
+# lcitool - libvirt CI guest management tool
+# Copyright (C) 2017-2018 Andrea Bolognani
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by the
+# Free Software Foundation; either version 2 of the License, or (at your
+# option) any later version.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General
+# Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+import argparse
+import sys
+import textwrap
+
+# This is necessary to maintain Python 2.7 compatibility
+try:
+import configparser
+except ImportError:
+import ConfigParser as configparser
+
+class Error(Exception):
+
+def __init__(self, message):
+self.message = message
+
+class Application:
+
+def __init__(self):
+self._parser = argparse.ArgumentParser(
+conflict_handler = "resolve",
+formatter_class = argparse.RawDescriptionHelpFormatter,
+description = "libvirt CI guest management tool",
+epilog = textwrap.dedent("""
+supported actions:
+"""),
+)
+self._parser.add_argument(
+"-a",
+metavar = "ACTION",
+required = True,
+help = "action to perform (see below)",
+)
+
+def run(self):
+cmdline = self._parser.parse_args()
+action = cmdline.a
+
+method = "_action_{}".format(action.replace("-", "_"))
+
+if hasattr(self, method):
+getattr(self, method).__call__()
+else:
+raise Error("Invalid action '{}'".format(action))
+
+if __name__ == "__main__":
+try:
+Application().run()
+except Error as e:
+sys.stderr.write("{}: {}\n".format(sys.argv[0], e.message))
+sys.exit(1)
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v2 00/12] lcitool: Rewrite in Python (and add Dockefile generator)
Read the initial cover letter for background and motivations. Changes from [v1]: * add Dockerfile generator; * rename the 'list' action to 'hosts' to better fit along with the additional 'projects' action; * always list items in alphabetical order; * move some generic functions to an Util class. [v1] https://www.redhat.com/archives/libvir-list/2018-July/msg00717.html Andrea Bolognani (12): lcitool: Drop shell implementation lcitool: Stub out Python implementation lcitool: Add tool configuration handling lcitool: Add inventory handling lcitool: Implement the 'hosts' action lcitool: Implement the 'install' action lcitool: Implement the 'update' action guests: Update documentation guests: Add Docker-related information to the inventory lcitool: Add projects information handling lcitool: Implement the 'projects' action lcitool: Implement the 'dockerfile' action guests/README.markdown| 8 +- guests/host_vars/libvirt-centos-7/docker.yml | 2 + guests/host_vars/libvirt-debian-8/docker.yml | 2 + guests/host_vars/libvirt-debian-9/docker.yml | 2 + .../host_vars/libvirt-debian-sid/docker.yml | 2 + guests/host_vars/libvirt-fedora-27/docker.yml | 2 + guests/host_vars/libvirt-fedora-28/docker.yml | 2 + .../libvirt-fedora-rawhide/docker.yml | 2 + guests/host_vars/libvirt-ubuntu-16/docker.yml | 2 + guests/host_vars/libvirt-ubuntu-18/docker.yml | 2 + guests/lcitool| 722 -- 11 files changed, 521 insertions(+), 227 deletions(-) create mode 100644 guests/host_vars/libvirt-centos-7/docker.yml create mode 100644 guests/host_vars/libvirt-debian-8/docker.yml create mode 100644 guests/host_vars/libvirt-debian-9/docker.yml create mode 100644 guests/host_vars/libvirt-debian-sid/docker.yml create mode 100644 guests/host_vars/libvirt-fedora-27/docker.yml create mode 100644 guests/host_vars/libvirt-fedora-28/docker.yml create mode 100644 guests/host_vars/libvirt-fedora-rawhide/docker.yml create mode 100644 guests/host_vars/libvirt-ubuntu-16/docker.yml create mode 100644 guests/host_vars/libvirt-ubuntu-18/docker.yml -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/3] docs: news: Provide an update about the video type 'none'
Signed-off-by: Erik Skultety --- docs/news.xml | 14 ++ 1 file changed, 14 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 773c95b0da..93832acc4c 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -46,6 +46,20 @@ + + + qemu: Introduce a new video model of type 'none' + + + Historically, libvirt would add a default 'cirrus' video device to + a guest if the XML specified 'graphics' but lacked 'video'. This can + be incovenient with GPU mediated devices which can serve as the only + rendering devices within the guest, rather than still relying on an + emulated GPU which would also be the primary device. Having a 'none' + model is our only backwards compatible option how to turn this legacy + behaviour off when needed. + + -- 2.14.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/3] docs: Rephrase the mediated devices hostdev section a bit
Currently it reads: Refer MDEV to create a mediated device on the host ...even though it resembles English, it's not a proper English. Signed-off-by: Erik Skultety --- docs/formatdomain.html.in | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a3afe137bf..84ab5a9d12 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4509,10 +4509,12 @@ determines how the host's vfio driver will expose the device to the guest. Currently, model='vfio-pci' and model='vfio-ccw' (Since 4.4.0) - is supported. Refer MDEV to create - a mediated device on the host. There are also some implications on the - usage of guest's address type depending on the model - attribute, see the address element below. + is supported. MDEV section + provides more information about mediated devices as well as how to + create mediated devices on the host. There are also some implications + on the usage of guest's address type depending on the + model attribute, see the address element + below. -- 2.14.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/3] conf: Introduce new video type 'none'
Historically, we've always enabled an emulated video device every time we
see that graphics should be supported with a guest. With the appearance
of mediated devices which can support QEMU's vfio-display capability,
users might want to use such a device as the only video device.
Therefore introduce a new, effectively a 'disable', type for video
device.
Signed-off-by: Erik Skultety
---
docs/formatdomain.html.in | 13 -
docs/schemas/domaincommon.rng | 1 +
src/conf/domain_conf.c | 55 --
src/conf/domain_conf.h | 1 +
src/qemu/qemu_command.c| 14 --
src/qemu/qemu_domain.c | 3 ++
src/qemu/qemu_domain_address.c | 10
tests/domaincapsschemadata/full.xml| 1 +
.../video-invalid-multiple-devices.xml | 33 +
tests/qemuxml2argvdata/video-none-device.args | 27 +++
tests/qemuxml2argvdata/video-none-device.xml | 39 +++
tests/qemuxml2argvtest.c | 4 +-
tests/qemuxml2xmloutdata/video-none-device.xml | 42 +
tests/qemuxml2xmltest.c| 1 +
14 files changed, 223 insertions(+), 21 deletions(-)
create mode 100644 tests/qemuxml2argvdata/video-invalid-multiple-devices.xml
create mode 100644 tests/qemuxml2argvdata/video-none-device.args
create mode 100644 tests/qemuxml2argvdata/video-none-device.xml
create mode 100644 tests/qemuxml2xmloutdata/video-none-device.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 84ab5a9d12..03d94f0533 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6622,9 +6622,18 @@ qemu-kvm -net nic,model=? /dev/null
The model element has a mandatory type
attribute which takes the value "vga", "cirrus", "vmvga", "xen",
"vbox", "qxl" (since 0.8.6),
- "virtio" (since 1.3.0)
- or "gop" (since 3.2.0)
+ "virtio" (since 1.3.0),
+ "gop" (since 3.2.0), or
+ "none" (since 4.6.0)
depending on the hypervisor features available.
+ The purpose of the type none is to instruct libvirt not
+ to add a default video device in the guest (see the paragraph above).
+ This legacy behaviour can be inconvenient in cases where GPU mediated
+ devices are meant to be the only rendering device within a guest and
+ so specifying another video device along with type
+ none.
+ Refer to Host device assignment to see
+ how to add a mediated device into a guest.
You can provide the amount of video memory in kibibytes (blocks of
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index bd687ce9d3..ed989c000f 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3451,6 +3451,7 @@
vbox
virtio
gop
+none
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7396616eda..d4927f0226 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -590,7 +590,8 @@ VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
"qxl",
"parallels",
"virtio",
- "gop")
+ "gop",
+ "none")
VIR_ENUM_IMPL(virDomainVideoVGAConf, VIR_DOMAIN_VIDEO_VGACONF_LAST,
"io",
@@ -5091,25 +5092,48 @@ static int
virDomainDefPostParseVideo(virDomainDefPtr def,
void *opaque)
{
+size_t i;
+
if (def->nvideos == 0)
return 0;
-virDomainDeviceDef device = {
-.type = VIR_DOMAIN_DEVICE_VIDEO,
-.data.video = def->videos[0],
-};
-
-/* Mark the first video as primary. If the user specified
- * primary="yes", the parser already inserted the device at
- * def->videos[0]
+/* it doesn't make sense to pair video device type 'none' with any other
+ * types, there can be only a single video device in such case
*/
-def->videos[0]->primary = true;
+for (i = 0; i < def->nvideos; i++) {
+if (def->videos[i]->type == VIR_DOMAIN_VIDEO_TYPE_NONE &&
+def->nvideos > 1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("a '%s' video type must be the only video device "
+ "defined for the domain"));
+return -1;
+}
+}
-/* videos[0] might have been added in AddImplicitDevices, after we've
- * done the per-device post-parse */
-if (virDomainDefPostParseDeviceIterator(def, ,
-NULL, opaque) < 0)
-return -1;
+if (def->videos[0]->type ==
[libvirt] [PATCH v2 0/3] Introduce new video model type 'none'
v1 here https://www.redhat.com/archives/libvir-list/2018-June/msg01793.html Since v1: - there were only small fixes needed as per the review - decided not to split the patch as requested by the reviewer because the first patch would contain 90% of the changes, both in qemu driver and domain_conf to make the test suite happy (PCI address auto-assignment issues) and a single hunk in qemu_command.c in the second patch to actually enable the feature - this just wasn't worth doing, better keep it together in this case - added some tiny reword patch 1 - added a news update Erik Skultety (3): docs: Rephrase the mediated devices hostdev section a bit conf: Introduce new video type 'none' docs: news: Provide an update about the video type 'none' docs/formatdomain.html.in | 23 ++--- docs/news.xml | 14 ++ docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 55 -- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c| 14 -- src/qemu/qemu_domain.c | 3 ++ src/qemu/qemu_domain_address.c | 10 tests/domaincapsschemadata/full.xml| 1 + .../video-invalid-multiple-devices.xml | 33 + tests/qemuxml2argvdata/video-none-device.args | 27 +++ tests/qemuxml2argvdata/video-none-device.xml | 39 +++ tests/qemuxml2argvtest.c | 4 +- tests/qemuxml2xmloutdata/video-none-device.xml | 42 + tests/qemuxml2xmltest.c| 1 + 15 files changed, 243 insertions(+), 25 deletions(-) create mode 100644 tests/qemuxml2argvdata/video-invalid-multiple-devices.xml create mode 100644 tests/qemuxml2argvdata/video-none-device.args create mode 100644 tests/qemuxml2argvdata/video-none-device.xml create mode 100644 tests/qemuxml2xmloutdata/video-none-device.xml -- 2.14.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/8] qemu: monitor: Remove old code for dual handling of 'transaction' data
On Tue, Jul 03, 2018 at 02:33:06PM +0200, Peter Krempa wrote: Now that we use only the separate function for creating data for the 'transaction' command we can remove all the boilerplate which was necessary before. Signed-off-by: Peter Krempa --- src/qemu/qemu_monitor_json.c | 42 ++ 1 file changed, 10 insertions(+), 32 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/8] qemu: monitor: Remove old external snapshot code
On Tue, Jul 03, 2018 at 02:33:05PM +0200, Peter Krempa wrote: Remove the dual mode code which allowed to create snapshots without support for 'transaction'. Signed-off-by: Peter Krempa --- src/qemu/qemu_monitor.c | 17 - src/qemu/qemu_monitor.h | 6 -- src/qemu/qemu_monitor_json.c | 37 - src/qemu/qemu_monitor_json.h | 8 4 files changed, 68 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/8] qemu: block: Create helper for creating data for legacy snapshots
On Tue, Jul 03, 2018 at 02:33:04PM +0200, Peter Krempa wrote:
With 'transaction' support we don't need to keep around the multipurpose
code which would create the snapshot if 'transaction' is not supported.
To simplify this add a new helper that just wraps the arguments for
'blockdev-snapshot-sync' operation in 'transaction' and use it instead
of qemuBlockSnapshotAddLegacy.
Additionally this allows to format the arguments prior to creating the
file for simpler cleanup.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 37 +
src/qemu/qemu_block.h | 6 ++
src/qemu/qemu_driver.c | 18 +++---
3 files changed, 46 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 0ebf2d2aff..db1579ca20 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -19,8 +19,10 @@
#include
#include "qemu_block.h"
+#include "qemu_command.h"
#include "qemu_domain.h"
#include "qemu_alias.h"
+#include "qemu_monitor_json.h"
#include "viralloc.h"
#include "virstring.h"
@@ -1683,3 +1685,38 @@ qemuBlockStorageSourceDetachOneBlockdev(virQEMUDriverPtr
driver,
return ret;
}
+
+
+int
+qemuBlockSnapshotAddLegacy(virJSONValuePtr actions,
+ virDomainDiskDefPtr disk,
+ virStorageSourcePtr newsrc,
+ bool reuse)
+{
+const char *format = virStorageFileFormatTypeToString(newsrc->format);
+char *device = NULL;
+char *source = NULL;
+int ret = -1;
+
+if (!(device = qemuAliasDiskDriveFromDisk(disk)))
+goto cleanup;
+
+if (qemuGetDriveSourceString(newsrc, NULL, ) < 0)
+goto cleanup;
+
+if (qemuMonitorJSONTransactionAdd(actions, "blockdev-snapshot-sync",
+ "s:device", device,
+ "s:snapshot-file", source,
+ "s:format", format,
+ "S:mode", reuse ? "existing" : NULL,
+ NULL) < 0)
Indentation is off.
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(device);
+VIR_FREE(source);
+
+return ret;
+}
diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index 418b5064b5..fd8984e60b 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -117,4 +117,10 @@ qemuBlockStorageSourceDetachOneBlockdev(virQEMUDriverPtr
driver,
qemuDomainAsyncJob asyncJob,
virStorageSourcePtr src);
+int
+qemuBlockSnapshotAddLegacy(virJSONValuePtr actions,
+ virDomainDiskDefPtr disk,
+ virStorageSourcePtr newsrc,
+ bool reuse);
+
#endif /* __QEMU_BLOCK_H__ */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ea06e23ff1..2d8af5daaa 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14932,23 +14932,16 @@
qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
virJSONValuePtr actions,
bool reuse)
{
-qemuDomainObjPrivatePtr priv = vm->privateData;
-char *device = NULL;
-char *source = NULL;
-const char *formatStr = NULL;
int ret = -1;
-if (!(device = qemuAliasDiskDriveFromDisk(dd->disk)))
-goto cleanup;
-
-if (qemuGetDriveSourceString(dd->src, NULL, ) < 0)
+if (qemuBlockSnapshotAddLegacy(actions, dd->disk, dd->src, reuse) < 0)
goto cleanup;
/* pre-create the image file so that we can label it before handing it to
qemu */
if (!reuse && dd->src->type != VIR_STORAGE_TYPE_BLOCK) {
if (virStorageFileCreate(dd->src) < 0) {
virReportSystemError(errno, _("failed to create image file '%s'"),
- source);
+ NULLSTR(dd->src->path));
Doesn't this make the error message less usable?
goto cleanup;
}
dd->created = true;
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/8] qemu: monitor: Add API to help creating 'transaction' arguments
On Tue, Jul 03, 2018 at 02:33:03PM +0200, Peter Krempa wrote:
Add a new helper that will be solely used to create arguments for the
transaction command. Later on this will make it possible to remove the
overloading which was caused by the fact that snapshots were created
without transaction and also will help in blockdevizing of snapshots.
Have you considered 'blockdevification' or 'make blockdevable'?
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor_json.c | 46
src/qemu/qemu_monitor_json.h | 4
2 files changed, 50 insertions(+)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 3e90279b71..54fefcb612 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -464,6 +464,52 @@ qemuMonitorJSONHasError(virJSONValuePtr reply,
}
+/**
+ * qemuMonitorJSONTransactionAdd:
+ * @actions: array of actions for the 'transaction' command
+ * @cmdname: command to add to @actions
+ * @...: arguments for @cmdname (see virJSONValueObjectAddVArgs for formatting)
+ *
+ * Add a new command with arguments to the existing ones. The resulting array
+ * is used as argument for the 'transaction' command.
"is intended to be used" or "can be used"?
+ *
+ * Returns 0 on success and -1 on error.
+ */
+int
+qemuMonitorJSONTransactionAdd(virJSONValuePtr actions,
+ const char *cmdname,
+ ...)
+{
+virJSONValuePtr entry = NULL;
+virJSONValuePtr data = NULL;
+va_list args;
+int ret = -1;
+
+va_start(args, cmdname);
+
+if (virJSONValueObjectCreateVArgs(, args) < 0)
+goto cleanup;
+
+if (virJSONValueObjectCreate(,
+ "s:type", cmdname,
+ "A:data", , NULL) < 0)
+goto cleanup;
+
+if (virJSONValueArrayAppend(actions, entry) < 0)
+goto cleanup;
+
+entry = NULL;
+ret = 0;
+
+ cleanup:
+virJSONValueFree(entry);
+virJSONValueFree(data);
+va_end(args);
+
I'm not a fan of this empty line.
+return ret;
+}
+
+
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/8] qemu: snapshot: Audit actual disk snapshot creation
On Tue, Jul 03, 2018 at 02:33:02PM +0200, Peter Krempa wrote: Currently we'd audit that we managed to format the data for the 'transaction' command rather than the (un)successful attempt to create the snapshot. Move the auditing code so that it can actually audit the result of the 'transaction' command. Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 19 +++ 1 file changed, 11 insertions(+), 8 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] New virsh feature: domif-setlink --domain --interface --state completer
After you go through command mentioned above, completer
finds what state the device is currently in, and suggests
an opposite state.
Signed-off-by: Simon Kobyda
---
Changes in V2:
- Added "Signed-off-by"
- Changed format of commit message to make it more
readable
tools/virsh-completer.c | 73 -
tools/virsh-completer.h | 4 +++
tools/virsh-domain.c| 1 +
3 files changed, 77 insertions(+), 1 deletion(-)
diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
index 2327e08340..e32fd82211 100644
--- a/tools/virsh-completer.c
+++ b/tools/virsh-completer.c
@@ -32,6 +32,7 @@
#include "internal.h"
#include "virutil.h"
#include "viralloc.h"
+#include "virmacaddr.h"
#include "virstring.h"
#include "virxml.h"
@@ -750,7 +751,6 @@ virshDomainEventNameCompleter(vshControl *ctl
ATTRIBUTE_UNUSED,
return NULL;
}
-
char **
virshPoolEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED,
const vshCmd *cmd ATTRIBUTE_UNUSED,
@@ -776,6 +776,77 @@ virshPoolEventNameCompleter(vshControl *ctl
ATTRIBUTE_UNUSED,
return NULL;
}
+char **
+virshDomainInterfaceStateCompleter(vshControl *ctl ATTRIBUTE_UNUSED,
+const vshCmd *cmd ATTRIBUTE_UNUSED,
+unsigned int flags)
+{
+virshControlPtr priv = ctl->privData;
+const char *iface = NULL;
+char **ret = NULL;
+xmlDocPtr xml = NULL;
+virMacAddr macaddr;
+char *state = NULL;
+char *xpath = NULL;
+char macstr[18] = "";
+xmlXPathContextPtr ctxt = NULL;
+xmlNodePtr *interfaces = NULL;
+int ninterfaces;
+
+virCheckFlags(0, NULL);
+
+if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
+return NULL;
+
+if (vshCommandOptStringReq(ctl, cmd, "interface", ) < 0)
+goto cleanup;
+
+if (virshDomainGetXML(ctl, cmd, flags, , ) < 0)
+goto cleanup;
+
+/* normalize the mac addr */
+if (virMacAddrParse(iface, ) == 0)
+virMacAddrFormat(, macstr);
+
+if (virAsprintf(, "/domain/devices/interface[(mac/@address = '%s')
or "
+" (target/@dev = '%s')]",
+ macstr, iface) < 0)
+goto cleanup;
+
+if ((ninterfaces = virXPathNodeSet(xpath, ctxt, )) < 0)
+goto cleanup;
+
+if (ninterfaces != 1)
+goto cleanup;
+
+ctxt->node = interfaces[0];
+
+if (VIR_ALLOC_N(ret, 2) < 0)
+goto error;
+
+if ((state = virXPathString("string(./link/@state)", ctxt)) &&
+STREQ(state, "down")) {
+if (VIR_STRDUP(ret[0], "up") < 0)
+goto error;
+} else {
+if (VIR_STRDUP(ret[0], "down") < 0)
+goto error;
+}
+
+ cleanup:
+VIR_FREE(state);
+VIR_FREE(interfaces);
+VIR_FREE(xpath);
+xmlXPathFreeContext(ctxt);
+xmlFreeDoc(xml);
+return ret;
+
+ error:
+virStringListFree(ret);
+ret = NULL;
+goto cleanup;
+}
+
char **
virshNodedevEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED,
diff --git a/tools/virsh-completer.h b/tools/virsh-completer.h
index 1c14bb2a54..b4fd2a86c6 100644
--- a/tools/virsh-completer.h
+++ b/tools/virsh-completer.h
@@ -94,6 +94,10 @@ char ** virshPoolEventNameCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
+char ** virshDomainInterfaceStateCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags);
+
char ** virshNodedevEventNameCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 3959b5475b..8adec1d9b1 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3064,6 +3064,7 @@ static const vshCmdOptDef opts_domif_setlink[] = {
{.name = "state",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
+ .completer = virshDomainInterfaceStateCompleter,
.help = N_("new state of the device")
},
{.name = "persistent",
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] completer: Doesn't alloc enough space for null terminated array of strings
On 07/12/2018 04:00 PM, Simon Kobyda wrote: > Functions virshSecretEventNameCompleter, virshPoolEventNameCompleter, > virshNodedevEventNameCompleter allocates only enough space > for array of N strings. > > However these are null terminated strings, so program needs to > alloc space for array of N+1 strings. > > How to replicate error: valgrind virsh, use completer for > '-nodedev-event --event' or '-pool-event --event' or > '-secret-event --event'. Well, "-nodedev-event" is not a command, "nodedev-event" is ;-) I'll fix these before pushing. > > Signed-off-by: Simon Kobyda > > --- > > Changes in V2: > - Added "Signed-off-by" > - Changed format of commit message to make it more > readable > > tools/virsh-completer.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 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
Re: [libvirt] [PATCH 3/8] qemu: snapshot: Unify conditions checking whether snapshot needs to be taken
On Tue, Jul 03, 2018 at 02:33:01PM +0200, Peter Krempa wrote:
In the cleanup path we already checked whether a snapshot needed to be
taken by looking into the collected data. Use the same approach when
creating the snapshot command data and when commiting the changes to the
*committing
domain definition.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 39b745b1db..e5005fd829 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15012,7 +15012,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr
driver,
* VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL with a valid file name and
* qcow2 format. */
for (i = 0; i < snap->def->ndisks; i++) {
-if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)
+if (!diskdata[i].src)
continue;
ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm,
@@ -15036,8 +15036,14 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr
driver,
goto error;
}
-for (i = 0; i < snap->def->ndisks; i++)
+for (i = 0; i < snap->def->ndisks; i++) {
+qemuDomainSnapshotDiskDataPtr dd = [i];
+
+if (!dd->src)
+continue;
+
qemuDomainSnapshotUpdateDiskSources([i], );
qemuDomainSnapshotUpdateDiskSources already is a no-op for NULL dd->src.
Also, the other occurrence of '[i]' can be replaced by dd now.
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] completer: Doesn't alloc enough space for null terminated array of strings
Functions virshSecretEventNameCompleter, virshPoolEventNameCompleter,
virshNodedevEventNameCompleter allocates only enough space
for array of N strings.
However these are null terminated strings, so program needs to
alloc space for array of N+1 strings.
How to replicate error: valgrind virsh, use completer for
'-nodedev-event --event' or '-pool-event --event' or
'-secret-event --event'.
Signed-off-by: Simon Kobyda
---
Changes in V2:
- Added "Signed-off-by"
- Changed format of commit message to make it more
readable
tools/virsh-completer.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
index 2327e08340..be59ea2e82 100644
--- a/tools/virsh-completer.c
+++ b/tools/virsh-completer.c
@@ -709,7 +709,7 @@ virshSecretEventNameCompleter(vshControl *ctl
ATTRIBUTE_UNUSED,
virCheckFlags(0, NULL);
-if (VIR_ALLOC_N(ret, VIR_SECRET_EVENT_ID_LAST) < 0)
+if (VIR_ALLOC_N(ret, VIR_SECRET_EVENT_ID_LAST + 1) < 0)
goto error;
for (i = 0; i < VIR_SECRET_EVENT_ID_LAST; i++) {
@@ -761,7 +761,7 @@ virshPoolEventNameCompleter(vshControl *ctl
ATTRIBUTE_UNUSED,
virCheckFlags(0, NULL);
-if (VIR_ALLOC_N(ret, VIR_STORAGE_POOL_EVENT_ID_LAST) < 0)
+if (VIR_ALLOC_N(ret, VIR_STORAGE_POOL_EVENT_ID_LAST + 1) < 0)
goto error;
for (i = 0; i < VIR_STORAGE_POOL_EVENT_ID_LAST; i++) {
@@ -787,7 +787,7 @@ virshNodedevEventNameCompleter(vshControl *ctl
ATTRIBUTE_UNUSED,
virCheckFlags(0, NULL);
-if (VIR_ALLOC_N(ret, VIR_NODE_DEVICE_EVENT_ID_LAST) < 0)
+if (VIR_ALLOC_N(ret, VIR_NODE_DEVICE_EVENT_ID_LAST + 1) < 0)
goto error;
for (i = 0; i < VIR_NODE_DEVICE_EVENT_ID_LAST; i++) {
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/8] qemu: snapshot: Remove monitor code now that 'transaction' is always used
On Tue, Jul 03, 2018 at 02:33:00PM +0200, Peter Krempa wrote: Since we now always do the snapshot via the 'transaction' command we can drop the code which would enter monitor for individual disk snapshots. Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 25 +++-- 1 file changed, 3 insertions(+), 22 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/8] qemu: snapshot: Require support of 'transaction' command for external snapshots
On Tue, Jul 03, 2018 at 02:32:59PM +0200, Peter Krempa wrote: While qemu supports the 'transaction' command since v1.1.0 (52e7c241ac766406f05fa) and the 'blockdev-snapshot-sync' command since v0.14.0-rc0 we need to keep the capability bits present since some qemu downstreams (RHEL/CentOS 7 for example) chose to cripple qemu by arbitrarily compile out some stuff which was already present at that s/compile/compiling/ time. To simplify the crazy code just require both commands to be present at the beginning of a external snapshot so that we can remove the case when s/a external/an external/ 'transaction' would not be supported. This also allows to drop any logic connected to the VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC flag since snapshots are atomic with the 'transaction' command. Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 62 +++--- 1 file changed, 13 insertions(+), 49 deletions(-) @@ -15265,13 +15234,8 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_DEFAULT_MASK); } -/* now the domain is now paused if: - * - if a memory snapshot was requested - * - an atomic snapshot was requested AND - * qemu does not support transactions - * - * Next we snapshot the disks. - */ +/* now the domain is now paused if a memory snapshot was requested */ Double now. + if ((ret = qemuDomainSnapshotCreateDiskActive(driver, vm, snap, flags, QEMU_ASYNC_JOB_SNAPSHOT)) < 0) goto cleanup; Reviewed-by: Ján Tomko Jano 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] conf: virDomainDefValidateInternal prohibit some characters in shmem name
Signed-off-by: Simon Kobyda
On Thu, Jul 12, 2018 at 3:10 PM Simon Kobyda wrote:
> XML shmem name will not include character '/', and will not be equal to
> strings
> "." or "..", as shmem name is used in a path.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1192400
> ---
>
> Changes in V2
> - Added error reports
> - Error situation will happen only if shmem name is equal to
> "." or "..", however their occurence in a name compromised of
> more
> characters is allowed.
>
> src/conf/domain_conf.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7ab2953d83..6b34c17de4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6107,6 +6107,8 @@ virDomainDefLifecycleActionValidate(const
> virDomainDef *def)
> static int
> virDomainDefValidateInternal(const virDomainDef *def)
> {
> +size_t i;
> +
> if (virDomainDefCheckDuplicateDiskInfo(def) < 0)
> return -1;
>
> @@ -6136,6 +6138,26 @@ virDomainDefValidateInternal(const virDomainDef
> *def)
> return -1;
> }
>
> +for (i = 0; i < def->nshmems; i++) {
> +if (strchr(def->shmems[i]->name, '/')) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("shmem name cannot include '/' character"));
> +return -1;
> +}
> +
> +if (STREQ(def->shmems[i]->name, ".")) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("shmem name cannot be equal to '.'"));
> +return -1;
> +}
> +
> +if (STREQ(def->shmems[i]->name, "..")) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("shmem name cannot be equal to '..'"));
> +return -1;
> +}
> +}
> +
> if (virDomainDefLifecycleActionValidate(def) < 0)
> return -1;
>
> --
> 2.17.1
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 04/11] qemu_monitor: Introduce qemuMonitorCPUModelInfoInit and qemuMonitorCPUModelInfoFreeContents
On Mon, Jul 09, 2018 at 22:56:48 -0500, Chris Venteicher wrote: > These forms modify contents of a qemuMonitorCPUModelInfo structure but > do not allocate or free the actual structure. > > Init - Initialize model name and empty properties within existing structure > FreeContents - Free model name and properties within existing structure We call such function with "Clear" suffix, i.e., qemuMonitorCPUModelInfoClear. But it is usually used when we have a structure stored somewhere directly rather than having a pointer to it. And this was not the case so far and I don't think there's any reason to introduce a code which would need any of these functions. NACK to this patch. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] completer: Doesn't alloc enough space for null terminated array of strings
On 07/12/2018 01:46 PM, Simon Kobyda wrote: > Functions virshSecretEventNameCompleter, virshPoolEventNameCompleter, > virshNodedevEventNameCompleter allocates only enough space for array of N > strings. > However these are null terminated strings, so program needs to alloc > space for array of N+1 strings. > How to replicate error: valgrind virsh, use completer for > '-nodedev-event --event' or '-pool-event --event' or '-secret-event --event'. > --- > tools/virsh-completer.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) The commit message could use some cleaning up. Firstly, there's no need to prepend every line with a TAB (btw remember to put 'set expandtab' into your .vimrc). Secondly, the is no 'Signed-off-By' line which prevents the patch from being merged. Thirdly, the lines are too long and should be cut. The patch looks okay though. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] conf: virDomainDefValidateInternal prohibit some characters in shmem name
XML shmem name will not include character '/', and will not be equal to strings
"." or "..", as shmem name is used in a path.
https://bugzilla.redhat.com/show_bug.cgi?id=1192400
---
Changes in V2
- Added error reports
- Error situation will happen only if shmem name is equal to
"." or "..", however their occurence in a name compromised of more
characters is allowed.
src/conf/domain_conf.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7ab2953d83..6b34c17de4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6107,6 +6107,8 @@ virDomainDefLifecycleActionValidate(const virDomainDef
*def)
static int
virDomainDefValidateInternal(const virDomainDef *def)
{
+size_t i;
+
if (virDomainDefCheckDuplicateDiskInfo(def) < 0)
return -1;
@@ -6136,6 +6138,26 @@ virDomainDefValidateInternal(const virDomainDef *def)
return -1;
}
+for (i = 0; i < def->nshmems; i++) {
+if (strchr(def->shmems[i]->name, '/')) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("shmem name cannot include '/' character"));
+return -1;
+}
+
+if (STREQ(def->shmems[i]->name, ".")) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("shmem name cannot be equal to '.'"));
+return -1;
+}
+
+if (STREQ(def->shmems[i]->name, "..")) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("shmem name cannot be equal to '..'"));
+return -1;
+}
+}
+
if (virDomainDefLifecycleActionValidate(def) < 0)
return -1;
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/6] tests: qemuschema: Add line break to debug message
Message stating which schema replies file is being used would be
squashed with other messages.
Signed-off-by: Peter Krempa
---
tests/testutilsqemuschema.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c
index fb22803a22..aa846e1e79 100644
--- a/tests/testutilsqemuschema.c
+++ b/tests/testutilsqemuschema.c
@@ -539,7 +539,7 @@ testQEMUSchemaGetLatest(void)
return NULL;
}
-VIR_TEST_DEBUG("replies file: '%s'", capsLatestFile);
+VIR_TEST_DEBUG("replies file: '%s'\n", capsLatestFile);
if (virTestLoadFile(capsLatestFile, ) < 0)
goto cleanup;
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/6] tests: qemumonitorjson: Raise the necessary debug level for QAPI schema checks
The debug output of the schema validator on success is not so
interresting that it should be printed when basic debugging is enabled.
Print it only when test debugging is set to 3 and more.
Signed-off-by: Peter Krempa
---
tests/qemumonitorjsontest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 9039cef423..fce2108932 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -2838,7 +2838,7 @@ testQAPISchema(const void *opaque)
ret = 0;
}
-if (virTestGetDebug() ||
+if (virTestGetDebug() >= 3 ||
(ret < 0 && virTestGetVerbose())) {
char *debugstr = virBufferContentAndReset();
fprintf(stderr, "\n%s\n", debugstr);
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 03/11] qemu_monitor: Indicate when CPUModelInfo props report migratablity
On Mon, Jul 09, 2018 at 22:56:47 -0500, Chris Venteicher wrote:
> Renamed variable in CPUModelInfo such that
> props_migratable_valid is true when properties in CPUModelInfo
> have been updated to accurately indicate if property is / isn't
> migratable.
>
> Property migratability is not returned directly in QMP messages but
> rather is sometimes calculated within Libvirt by other means and then
> stored in CPUModelInfo properties by Libvirt. props_migratable_valid is
> set to true when this calculation has been done by Libvirt.
> ---
> src/qemu/qemu_capabilities.c | 10 +-
> src/qemu/qemu_monitor.c | 2 +-
> src/qemu/qemu_monitor.h | 2 +-
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
...
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 18b59be985..208a7f5d21 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1005,7 +1005,7 @@ struct _qemuMonitorCPUModelInfo {
> char *name;
> size_t nprops;
> qemuMonitorCPUPropertyPtr props;
> -bool migratability;
> +bool props_migratable_valid;
> };
I don't see a reason for renaming the variable. The new name is uglier
and may be confusing in exactly the same way you found migratability to
be confusing. Just add a comment which would explain that the
migratability tells whether we can use the prop->migratable value to
check if a particular feature is migratable.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 6/6] tests: qemumonitorjson: Do QMP schema validation for DO_TEST_GEN
Test some more QMP commands against the schema.
Signed-off-by: Peter Krempa
---
tests/qemumonitorjsontest.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 95b718ab37..e9b2632655 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1297,7 +1297,7 @@ testQemuMonitorJSON ## funcName(const void *opaque) \
{ \
const testQemuMonitorJSONSimpleFuncData *data = opaque; \
virDomainXMLOptionPtr xmlopt = data->xmlopt; \
-qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); \
+qemuMonitorTestPtr test = qemuMonitorTestNewSchema(xmlopt, data->schema); \
const char *reply = data->reply; \
int ret = -1; \
\
@@ -2894,6 +2894,7 @@ mymain(void)
#define DO_TEST_GEN(name, ...) \
simpleFunc = (testQemuMonitorJSONSimpleFuncData) {.xmlopt = driver.xmlopt,
\
+ .schema =
qapiData.schema \
__VA_ARGS__ }; \
if (virTestRun(# name, testQemuMonitorJSON ## name, ) < 0) \
ret = -1
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/6] tests: qemumonitorjson: Fix schema testing of monitor commands
The 'simpleFunc' data structure is overwritten by the code generated
from the macros which initiate the tests. This means that most of the
tests would get NULL 'schema' member which means that the schema
validation would not take place.
Signed-off-by: Peter Krempa
---
tests/qemumonitorjsontest.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index fce2108932..95b718ab37 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -2879,7 +2879,6 @@ mymain(void)
ret = -1;
goto cleanup;
}
-simpleFunc.schema = qapiData.schema;
#define DO_TEST(name) \
if (virTestRun(# name, testQemuMonitorJSON ## name, driver.xmlopt) < 0) \
@@ -2887,7 +2886,9 @@ mymain(void)
#define DO_TEST_SIMPLE(CMD, FNC, ...) \
simpleFunc = (testQemuMonitorJSONSimpleFuncData) {.cmd = CMD, .func = FNC,
\
- .xmlopt = driver.xmlopt, __VA_ARGS__ };
\
+ .xmlopt = driver.xmlopt, \
+ .schema = qapiData.schema, \
+ __VA_ARGS__ }; \
if (virTestRun(# FNC, testQemuMonitorJSONSimpleFunc, ) < 0) \
ret = -1
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/6] tests: qemumonitorutils: Don't crash on wrong monitor command
virQEMUQAPISchemaPathGet returns success when a given schema path was
not found but the returned object is set to NULL. This meant that we'd
call testQEMUSchemaValidate with the schemaroot being NULL which lead to
a crash if a mistyped monitor command was tested.
Signed-off-by: Peter Krempa
---
tests/qemumonitortestutils.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c
index d857c381a4..15eba5898e 100644
--- a/tests/qemumonitortestutils.c
+++ b/tests/qemumonitortestutils.c
@@ -564,7 +564,8 @@
qemuMonitorTestProcessCommandDefaultValidate(qemuMonitorTestPtr test,
if (virAsprintf(, "%s/arg-type", cmdname) < 0)
goto cleanup;
-if (virQEMUQAPISchemaPathGet(schemapath, test->qapischema, ) <
0) {
+if (virQEMUQAPISchemaPathGet(schemapath, test->qapischema, ) <
0 ||
+!schemaroot) {
if (qemuMonitorReportError(test,
"command '%s' not found in QAPI schema",
cmdname) == 0)
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/6] tests: qemu: Fix testing QMP commands against the schema
I was trying to add some new commands and made a typo. The tests did not catch it. I must have messed up something when sending the original schema validation impl. Peter Krempa (6): tests: qemuschema: Fix copy-paste error in function name tests: qemuschema: Add line break to debug message tests: qemumonitorutils: Don't crash on wrong monitor command tests: qemumonitorjson: Raise the necessary debug level for QAPI schema checks tests: qemumonitorjson: Fix schema testing of monitor commands tests: qemumonitorjson: Do QMP schema validation for DO_TEST_GEN tests/qemumonitorjsontest.c | 10 ++ tests/qemumonitortestutils.c | 3 ++- tests/testutilsqemuschema.c | 10 +- 3 files changed, 13 insertions(+), 10 deletions(-) -- 2.16.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/6] tests: qemuschema: Fix copy-paste error in function name
s/testQEMUSchemaValidateArrayBuiltin/testQEMUSchemaValidateBuiltin/
Signed-off-by: Peter Krempa
---
tests/testutilsqemuschema.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c
index 1cec5265e1..fb22803a22 100644
--- a/tests/testutilsqemuschema.c
+++ b/tests/testutilsqemuschema.c
@@ -28,9 +28,9 @@ testQEMUSchemaValidateRecurse(virJSONValuePtr obj,
virBufferPtr debug);
static int
-testQEMUSchemaValidateArrayBuiltin(virJSONValuePtr obj,
- virJSONValuePtr root,
- virBufferPtr debug)
+testQEMUSchemaValidateBuiltin(virJSONValuePtr obj,
+ virJSONValuePtr root,
+ virBufferPtr debug)
{
const char *t = virJSONValueObjectGetString(root, "json-type");
const char *s = NULL;
@@ -476,7 +476,7 @@ testQEMUSchemaValidateRecurse(virJSONValuePtr obj,
const char *t = virJSONValueObjectGetString(root, "meta-type");
if (STREQ_NULLABLE(t, "builtin")) {
-return testQEMUSchemaValidateArrayBuiltin(obj, root, debug);
+return testQEMUSchemaValidateBuiltin(obj, root, debug);
} else if (STREQ_NULLABLE(t, "object")) {
return testQEMUSchemaValidateObject(obj, root, schema, debug);
} else if (STREQ_NULLABLE(t, "enum")) {
--
2.16.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 02/11] qemu_monitor: Introduce qemuMonitorGetCPUModelBaseline (query-cpu-model-baseline)
Drop "(query-cpu-model-baseline)" from the subject to make it shorter.
On Mon, Jul 09, 2018 at 22:56:46 -0500, Chris Venteicher wrote:
> Wrap QMP query-cpu-model-baseline command transaction with QEMU.
> ---
> src/qemu/qemu_monitor.c | 13
> src/qemu/qemu_monitor.h | 6
> src/qemu/qemu_monitor_json.c | 63
> src/qemu/qemu_monitor_json.h | 7
> 4 files changed, 89 insertions(+)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 6ed475ede0..a3278c018e 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3707,6 +3707,19 @@ qemuMonitorCPUModelInfoCopy(const
> qemuMonitorCPUModelInfo *orig)
> return NULL;
> }
>
> +int
> +qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon,
> + qemuMonitorCPUModelInfoPtr model_a,
> + qemuMonitorCPUModelInfoPtr model_b,
> + qemuMonitorCPUModelInfoPtr *model_baseline)
> +{
> +VIR_DEBUG("model_a->name=%s, model_a->nprops=%lu", model_a->name,
> model_a->nprops);
> +VIR_DEBUG("model_b->name=%s, model_b->nprops=%lu", model_b->name,
> model_b->nprops);
Please, merge this into a single VIR_DEBUG, otherwise they would be
logged on two lines possibly with logs from other threads in between.
VIR_DEBUG("model_a->name=%s, model_a->nprops=%lu "
"model_b->name=%s, model_b->nprops=%lu",
model_a->name, model_a->nprops,
model_b->name, model_b->nprops);
> +
> +QEMU_CHECK_MONITOR(mon);
> +
> +return qemuMonitorJSONGetCPUModelBaseline(mon, model_a, model_b,
> model_baseline);
> +}
>
> int
> qemuMonitorGetCommands(qemuMonitorPtr mon,
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index b3d62324b4..18b59be985 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1025,6 +1025,12 @@ void
> qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info);
> qemuMonitorCPUModelInfoPtr
> qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig);
>
> +int qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon,
> + qemuMonitorCPUModelInfoPtr model_a,
> + qemuMonitorCPUModelInfoPtr model_b,
> + qemuMonitorCPUModelInfoPtr
> *model_baseline)
> +ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);
> +
> int qemuMonitorGetCommands(qemuMonitorPtr mon,
> char ***commands);
> int qemuMonitorGetEvents(qemuMonitorPtr mon,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index a18a1a1bf1..90d43eee97 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5540,6 +5540,69 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
> }
>
>
> +/* Note: *model_baseline == NULL && return == 0 if command not supported by
> QEMU
> + */
> +int
> +qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon,
> + qemuMonitorCPUModelInfoPtr model_a,
> + qemuMonitorCPUModelInfoPtr model_b,
> + qemuMonitorCPUModelInfoPtr
> *model_baseline)
> +{
> +int ret = -1;
> +virJSONValuePtr cmd = NULL;
> +virJSONValuePtr reply = NULL;
> +virJSONValuePtr data = NULL;
> +virJSONValuePtr modela = NULL;
> +virJSONValuePtr modelb = NULL;
> +virJSONValuePtr cpu_model = NULL;
> +
> +*model_baseline = NULL;
> +
> +if (!(modela = qemuMonitorJSONBuildCPUModelInfoToJSON(model_a)) ||
> +!(modelb = qemuMonitorJSONBuildCPUModelInfoToJSON(model_b)))
> +goto cleanup;
> +
> +if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-baseline",
> + "a:modela", ,
> + "a:modelb", ,
> + NULL)))
> +goto cleanup;
> +
> +if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> +goto cleanup;
> +
> +if (qemuMonitorJSONHasError(reply, "GenericError")) {
> +/* QEMU does not support query-cpu-model-baseline or cpu model */
> +ret = 0;
> +goto cleanup;
> +}
This function is not called for any capabilities probing, is it? We
should normally report an error if QEMU does not support the command and
propagate it back to the user who asked for CPU baseline. Not reporting
an error is only useful if we don't care if the command is supported by
QEMU (i.e., when we probe for capabilities) or when we have a fallback
code.
> +
> +if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0)
> +goto cleanup;
> +
> +data = virJSONValueObjectGetObject(reply, "return");
> +
> +if (!(cpu_model = virJSONValueObjectGetObject(data, "model"))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +
[libvirt] [PATCH] completer: Doesn't alloc enough space for null terminated array of strings
Functions virshSecretEventNameCompleter, virshPoolEventNameCompleter,
virshNodedevEventNameCompleter allocates only enough space for array of N
strings.
However these are null terminated strings, so program needs to alloc
space for array of N+1 strings.
How to replicate error: valgrind virsh, use completer for
'-nodedev-event --event' or '-pool-event --event' or '-secret-event --event'.
---
tools/virsh-completer.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
index 2327e08340..be59ea2e82 100644
--- a/tools/virsh-completer.c
+++ b/tools/virsh-completer.c
@@ -709,7 +709,7 @@ virshSecretEventNameCompleter(vshControl *ctl
ATTRIBUTE_UNUSED,
virCheckFlags(0, NULL);
-if (VIR_ALLOC_N(ret, VIR_SECRET_EVENT_ID_LAST) < 0)
+if (VIR_ALLOC_N(ret, VIR_SECRET_EVENT_ID_LAST + 1) < 0)
goto error;
for (i = 0; i < VIR_SECRET_EVENT_ID_LAST; i++) {
@@ -761,7 +761,7 @@ virshPoolEventNameCompleter(vshControl *ctl
ATTRIBUTE_UNUSED,
virCheckFlags(0, NULL);
-if (VIR_ALLOC_N(ret, VIR_STORAGE_POOL_EVENT_ID_LAST) < 0)
+if (VIR_ALLOC_N(ret, VIR_STORAGE_POOL_EVENT_ID_LAST + 1) < 0)
goto error;
for (i = 0; i < VIR_STORAGE_POOL_EVENT_ID_LAST; i++) {
@@ -787,7 +787,7 @@ virshNodedevEventNameCompleter(vshControl *ctl
ATTRIBUTE_UNUSED,
virCheckFlags(0, NULL);
-if (VIR_ALLOC_N(ret, VIR_NODE_DEVICE_EVENT_ID_LAST) < 0)
+if (VIR_ALLOC_N(ret, VIR_NODE_DEVICE_EVENT_ID_LAST + 1) < 0)
goto error;
for (i = 0; i < VIR_NODE_DEVICE_EVENT_ID_LAST; i++) {
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Peter Krempa writes: > On Thu, Jul 12, 2018 at 08:59:44 +0200, Markus Armbruster wrote: >> Peter Krempa writes: >> > On Tue, Jul 10, 2018 at 17:01:22 +0200, Cornelia Huck wrote: >> >> On Tue, 10 Jul 2018 16:39:31 +0200 >> >> Peter Krempa wrote: > > [...] > >> > An option is to do a automatic testing where one of this approaches will >> > be enabled. For that you need a way to generate configurations which >> > libvirt would use in real life. We have a rather big collection of XMLs >> > which describe a valid configuration but the problem with using them on >> > a real qemu is that most of the disk paths/network targets/other >> > resources are made up and making them work with a real qemu would range >> > from being painful to being impossible. >> >> I sympathize. However, it's not clear which one's harder, providing >> environments for a sufficiently wide range of configurations (possibly >> mockups), or hacking QEMU to do nothing but check configuration. QEMU > > That's the main reason I think we should make it possible to use the > data for the 'qemuxml2argv' test suite in libvirt. We require that new > features are covered by this so that means that the testsuite is > possibly the most comprehensive collection of libvirt configurations I > know of. > > It's not perfect as we in many cases don't test any possible > value but just try to excercise the code to generate them and others are > left behind. > > Another historical problem was that we've defined a set of capabilities > rather than using any real example to do this so many of the > commandlines generated and tested are basically impossible to get in > real life. > > That's why I added testing with real capabilities. We'll just need to > generate a bunch of files to achieve full coverage here. > > >> isn't designed for that, and configuration checking is intertwined with >> everything else. Complete disentanglement looks impractical to me. I > > I agree. Getting anything special than the real codepath may create > bubbles of problems still. On the other hand we'll need some guidance on > what's sufficient to do to execute the deprecation detection code. > > This may require some coding style guidelines in qemu. E.g. no > deprecation warnings after the vCPUs are started. Running a full > operating system to check the warinigns would be utterly impractical. Hot-plugging may get you deprecation warnings after vCPU start. But I get what you mean. Rule of thumb: first check configuration is well-formed, then do stuff that may fail when configuration asks for the impossible, and only then do stuff that doesn't use configuration. > Preferrably we would get away with starting qemu and waiting for the > monitor to start. We'll see how far that gets us. >> guess we could do something useful at the QAPI level, though. Yet >> another reason to qapify the command line... > > That would be great, but I think that there's a subset of things that > can be deprecated but can't be expressed by schema. In such case we > still need to run the programatic checks to see. There will always be stuff the schema can't express without complicating the schema language a lot, and stuff the schema could express, but only at a cost in readability we prefer not to pay. To get the most mileage out of schema introspection, we should strive for making things visible in there whenever practical. >> > If we start from scratch you then lack coverage. >> > >> >> If we fail with exit(1), can libvirt check any message that is logged >> >> right before that? >> > >> > Yes we currently use this for very early failures which occur prior to >> > the monitor working. >> > >> >> > To make any reasonable use of -no-deprecated-options we'd also need >> >> > something that simulates qemu startup (no resources are touched in fact) >> >> > so that we can run it against the testsuite. Otherwise the use will be >> >> > limited to developers using it with the configuration they are >> >> > currently testing. >> >>> >> >> Would that moan loudly that you should poke the libvirt developers if >> >> some kind of testsuite failure is detected? Or am I misunderstanding? >> > >> > Generally it should make somebody complain. But there is a problem. >> > Since we are talking deprecation it can't be enabled by default. And by >> > not making it default most of the users will not enable that option. >> >> I don't think end users should do the work of catching use of deprecated >> features. It's a CI job. >> >> In a CI context, we don't need fancy QMP infrastructure to communicate >> "you used a deprecated feature", we can get away with printing an >> explanation to stderr and exit(1). That should make CI fail, and the >> failure should make a developer read the explanation. To unbreak CI, he >> can either fix the problem right away, or file a BZ and suppress the CI >> failure until it's fixed, say by downgrading --deprecated=error to >> --deprecated=warn. > > Definitely. Plain
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Peter Krempa writes: > On Thu, Jul 12, 2018 at 08:38:25 +0200, Markus Armbruster wrote: >> Kevin Wolf writes: >> > Am 10.07.2018 um 16:22 hat Cornelia Huck geschrieben: >> >> On Tue, 10 Jul 2018 07:59:15 +0200 >> >> Markus Armbruster wrote: >> >> Would that be workable? >> > >> > I think the function should just take a message: >> > >> > /* Works like error_report(), except for the WARNING/ERROR prefix >> > * and exit(1) if -no-deprecated-options is set */ >> > void deprecation_report(const char *fmt, ...); >> >> I like it. The contract could use a bit of polish, but that's detail. >> >> > We don't necessarily deprecate only options, but we might also deprecate >> > monitor commands, specific options values (while keeping other values of >> > the same option) etc. >> >> Exactly. > > For monitor commands we luckily have QMP introspection which can help a > lot in this case. At least for deprecating stuff that is expressable by > the schema. Introspection doesn't convey "deprecated", but... > In libvirt we are actually doing schema validation of the blockdev-add > arguments generators and most commands which are covered by the > qemumonitorjsontest. The schema used is based on our capability > detection so it's gathered from the most-recent version of qemu we have > required for our tests (which is most of the time based on GIT version > of qemu if there are any significant new features). > > If the deprecation will be expressable by the schema it should be rather > simple to modify the schema validator to catch the deprecation flags and > report errors in our testsuite. ... we can certainly make it if it's useful. > CI-ifying of the above should be then also very simple. We'd just gather > fresh QMP schema rather than using one from our test case pantry. > > Some time ago I also added testing of the commandline generator in > libvirt with the most recent capabilities rather than using the > historically declared capabilities that we've added when the test was > created. This means that we actually test some valid combinations and > also if stuff covered by our capability probing is removed the tests > will catch it. > > I was also thinking of adding a tool which would use the above tests to > attempt starting of a qemu process until the monitor shows up. That test > then could also use -no-deprecated-options. I'm hoping waiting for the > monitor is sufficient to excercise most of the code which could contain > deprecation warnings. (Alternatively we can go through the > pre-cpu-startup setup done on the monitor as well). Unfortunately doing > this will not be as simple asi the test cases contain random disk paths > and other resources which may not be available. This means that it will > require some in-place modification and creative temporary resource > usage. Yes. If you find QEMU makes testing something hard, we should talk. Together we might find a reasonable way to make it easier. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Cornelia Huck writes:
> On Thu, 12 Jul 2018 08:51:16 +0200
> Markus Armbruster wrote:
>
>> Markus Armbruster writes:
>>
>> > Kevin Wolf writes:
>> >
>> >> I think the function should just take a message:
>> >>
>> >> /* Works like error_report(), except for the WARNING/ERROR prefix
>> >> * and exit(1) if -no-deprecated-options is set */
>> >> void deprecation_report(const char *fmt, ...);
>> >
>> > I like it. The contract could use a bit of polish, but that's detail.
>>
>> Suggest --deprecated={silent,warn,error}, default silent.
>
> I like that, but I'd prefer to default to warn (so that command line
> users have a better chance to notice it).
Fair enough.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 01/11] qemu_monitor_json: Introduce qemuMonitorCPUModelInfo / JSON conversion
On Mon, Jul 09, 2018 at 22:56:45 -0500, Chris Venteicher wrote:
> Bidirectional conversion functions between Monitor data structure and
> QMP Message JSON.
>
> Commit creates reusable functions usable anywhere CPUModelInfo structure
> is input or output from QMP Commands.
>
> JSON of form:
> {"model": {"name": "IvyBridge", "props": {}}}
> ---
> src/qemu/qemu_monitor_json.c | 126 ++-
> 1 file changed, 96 insertions(+), 30 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index f9fe9e35ba..a18a1a1bf1 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5345,6 +5345,101 @@ qemuMonitorJSONParseCPUModelProperty(const char *key,
> return 0;
> }
>
> +
> +/* model_json: {"name": "z13-base", "props": {}}
> + */
> +static virJSONValuePtr
> +qemuMonitorJSONBuildCPUModelInfoToJSON(qemuMonitorCPUModelInfoPtr model)
This is a static function which is not used anywhere yet, you need to
move its definition to the patch which will make use of it. Otherwise
libvirt would fail to compile at this point. Remember the goal is to be
able to compile (and check + syntax-check) libvirt after every single
commit. It's not enough if it compiles at the end of a series.
...
> +/* model_json: {"name": "IvyBridge", "props": {}}
> + */
> +static qemuMonitorCPUModelInfoPtr
> +qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr cpu_model)
> +{
> +virJSONValuePtr cpu_props;
> +qemuMonitorCPUModelInfoPtr machine_model = NULL;
> +qemuMonitorCPUModelInfoPtr model = NULL;
I would call this variable 'ret' since it is only used to steal the
pointer from machine_model and return it. Then, you can even do
s/machine_model/model/ if you want.
Otherwise the patch looks good.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
On 07/12/2018 11:08 AM, Peter Krempa wrote: > On Thu, Jul 12, 2018 at 11:00:42 +0200, Michal Privoznik wrote: >> On 07/12/2018 10:56 AM, Peter Krempa wrote: > > [...] > >> Also, can I get review on the rest of the patches please? I noticed some >> people started replying only to a single patch in a series leaving the >> rest unreviewed. I don't think that's good. > > I did not plan to review this series. I think it's warranted to point > out a problem with a patch even if you would not review that series > otherwise. > Well, unless reviewers are reading every e-mail in every thread, they will see a discussion to this thread thinking there is an review going on and thus skip to next (unreviewed) thread. This is usually my experience. So while pointing out problems is a good thing to do, leaving the rest unreviewed isn't IMO. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
On Thu, Jul 12, 2018 at 11:00:42 +0200, Michal Privoznik wrote: > On 07/12/2018 10:56 AM, Peter Krempa wrote: [...] > Also, can I get review on the rest of the patches please? I noticed some > people started replying only to a single patch in a series leaving the > rest unreviewed. I don't think that's good. I did not plan to review this series. I think it's warranted to point out a problem with a patch even if you would not review that series otherwise. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Am 12.07.2018 um 09:48 hat Cornelia Huck geschrieben:
> On Thu, 12 Jul 2018 08:51:16 +0200
> Markus Armbruster wrote:
>
> > Markus Armbruster writes:
> >
> > > Kevin Wolf writes:
> > >
> > >> Am 10.07.2018 um 16:22 hat Cornelia Huck geschrieben:
> > >>> On Tue, 10 Jul 2018 07:59:15 +0200
> > >>> Markus Armbruster wrote:
> > >>>
> > >>> > In addition to actively pulling libvirt developers into review of
> > >>> > deprecation patches, we should pursue the idea to optionally let QEMU
> > >>> > fail on use of deprecated features, then have libvirt run its test
> > >>> > suite
> > >>> > that way.
> > >>>
> > >>> What about the following:
> > >>>
> > >>> qemu_deprecated_option("old_option", "modern_option");
> > >>>
> > >>> Which would then print (in normal operation)
> > >>>
> > >>> "WARNING: 'old_option' is deprecated and will be removed; use
> > >>> 'modern_option' instead"
> > >>>
> > >>> to the monitor (or to stderr? to both?).
> > >>>
> > >>> If you start QEMU with a -no-deprecated-options switch, it would print
> > >>>
> > >>> "ERROR: 'old_option' is deprecated and will be removed; use
> > >>> 'modern_option' instead"
> > >>>
> > >>> and do an exit(1).
> > >>>
> > >>> Would that be workable?
> > >>
> > >> I think the function should just take a message:
> > >>
> > >> /* Works like error_report(), except for the WARNING/ERROR prefix
> > >> * and exit(1) if -no-deprecated-options is set */
> > >> void deprecation_report(const char *fmt, ...);
> > >
> > > I like it. The contract could use a bit of polish, but that's detail.
Obviously, this comment wasn't meant to be copied into the source code,
but just to explain what I'm actually proposing there.
> > Suggest --deprecated={silent,warn,error}, default silent.
>
> I like that, but I'd prefer to default to warn (so that command line
> users have a better chance to notice it).
I agree that warn is the better default. (It's also consistent with what
we have been doing for deprecations so far.)
Kevin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
On 07/12/2018 10:56 AM, Peter Krempa wrote: > On Thu, Jul 12, 2018 at 10:51:33 +0200, Michal Privoznik wrote: >> On 07/12/2018 10:01 AM, Peter Krempa wrote: >>> On Thu, Jul 12, 2018 at 09:37:47 +0200, Michal Privoznik wrote: So far we are setting only fake secret and storage drivers. Therefore if the code wants to call a public NWFilter API (like qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are doing) the virGetConnectNWFilter() function will try to actually spawn session daemon because there's no connection object set to handle NWFilter driver. Even though I haven't experienced the same problem with the rest of the drivers (interface, network and node dev), the reasoning above can be applied to them as well. At the same time, now that connection object is registered for the drivers, the public APIs will throw virReportUnsupportedError(). And since we don't provide any error func the error is printed to stderr. Fix this by setting dummy error func. Signed-off-by: Michal Privoznik --- tests/qemuxml2argvtest.c | 6 ++ 1 file changed, 6 insertions(+) >>> >>> [...] >>> @@ -652,6 +656,8 @@ mymain(void) return EXIT_FAILURE; } +virTestQuiesceLibvirtErrors(true); + >>> >>> NACK, this suppresses legitimate errors in the testsuite. >>> >>> I've mangled one of the XML files and ran the qemuxml2argvtest with >>> VIR_TEST_DEBUG=1 and got: >>> >>> 249) QEMU XML-2-startup-XML disk-drive-cache-directsync... >>> SKIP >>> 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... >>> FAILED >>> 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe... >>> SKIP >>> >>> Without this patch I'd get: >>> >>> 249) QEMU XML-2-startup-XML disk-drive-cache-directsync... >>> SKIP >>> 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... >>> libvirt: Domain Config error : unsupported configuration: unknown disk >>> cache mode 'unafe' >>> FAILED >>> 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe... >>> SKIP >>> >> >> Well, without it I get: >> >> tests $ ./qemuxml2argvtest >> TEST: qemuxml2argvtest >> ._._._._..._._._._._._._._._._._._._._._ 40 >> ._._._._._._._._._._._._._._._._._._._._ 80 >> ._._._._._._._._._._._._._._._._._._._._ 120 >> ._._._._._._._._._._._._._._._._._._._._ 160 >> ._._._._._._._._._._._._._._._._._._._._ 200 >> ._._._._._._._._._._._._._._._._..._._._ 240 >> .._._._._.__._._._._._._._._._._._._ 280 >> ._._._._._._._._._._._._._._._._._._._._ 320 >> ._._._._._._._._._._._._._._._._._._._._ 360 >> ._._._._._._._._._._._._._._._._._._._._ 400 >> ._._._._._._._._._._._._._._._._._._._._ 440 >> ._._._._._._._._libvirt: Network Filter Driver error : internal >> error: unexpected nwfilter URI path '/session', try nwfilter:///system >> libvirt: Network Filter Driver error : internal error: unexpected >> nwfilter URI path '/session', try nwfilter:///system >> libvirt: Network Filter Driver error : internal error: unexpected >> nwfilter URI path '/session', try nwfilter:///system >> libvirt: Network Filter Driver error : internal error: unexpected >> nwfilter URI path '/session', try nwfilter:///system >> libvirt: Network Filter Driver error : internal error: unexpected >> nwfilter URI path '/session', try nwfilter:///system >> ._._._._._._._._._._._._ 480 >> >> >> So do you have any other idea? I came up with two already and neither of >> them got through review. Just to remind everybody, we are possibly >> touching live user data here so we need a resolution rather sooner than >> later. > > I specifically NACKd the part that installs the callback for suppressing > error messages. The messages here need to be suppressed by some other > way, but we should not decrease the debugability of tests. > > The error handler installation does not seem to have to do with live > user data touching. > > I did not try to see what the other part of this patch does. > Okay, I will drop it. The output of the test would be ugly then, but I don't care anymore. Also, can I get review on the rest of the patches please? I noticed some people started replying only to a single patch in a series leaving the rest unreviewed. I don't think that's good. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
On Thu, Jul 12, 2018 at 10:51:33 +0200, Michal Privoznik wrote: > On 07/12/2018 10:01 AM, Peter Krempa wrote: > > On Thu, Jul 12, 2018 at 09:37:47 +0200, Michal Privoznik wrote: > >> So far we are setting only fake secret and storage drivers. > >> Therefore if the code wants to call a public NWFilter API (like > >> qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are > >> doing) the virGetConnectNWFilter() function will try to actually > >> spawn session daemon because there's no connection object set to > >> handle NWFilter driver. > >> > >> Even though I haven't experienced the same problem with the rest > >> of the drivers (interface, network and node dev), the reasoning > >> above can be applied to them as well. > >> > >> At the same time, now that connection object is registered for > >> the drivers, the public APIs will throw > >> virReportUnsupportedError(). And since we don't provide any error > >> func the error is printed to stderr. Fix this by setting dummy > >> error func. > >> > >> Signed-off-by: Michal Privoznik > >> --- > >> tests/qemuxml2argvtest.c | 6 ++ > >> 1 file changed, 6 insertions(+) > > > > [...] > > > >> > >> @@ -652,6 +656,8 @@ mymain(void) > >> return EXIT_FAILURE; > >> } > >> > >> +virTestQuiesceLibvirtErrors(true); > >> + > > > > NACK, this suppresses legitimate errors in the testsuite. > > > > I've mangled one of the XML files and ran the qemuxml2argvtest with > > VIR_TEST_DEBUG=1 and got: > > > > 249) QEMU XML-2-startup-XML disk-drive-cache-directsync... > > SKIP > > 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... > > FAILED > > 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe... > > SKIP > > > > Without this patch I'd get: > > > > 249) QEMU XML-2-startup-XML disk-drive-cache-directsync... > > SKIP > > 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... > > libvirt: Domain Config error : unsupported configuration: unknown disk > > cache mode 'unafe' > > FAILED > > 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe... > > SKIP > > > > Well, without it I get: > > tests $ ./qemuxml2argvtest > TEST: qemuxml2argvtest > ._._._._..._._._._._._._._._._._._._._._ 40 > ._._._._._._._._._._._._._._._._._._._._ 80 > ._._._._._._._._._._._._._._._._._._._._ 120 > ._._._._._._._._._._._._._._._._._._._._ 160 > ._._._._._._._._._._._._._._._._._._._._ 200 > ._._._._._._._._._._._._._._._._..._._._ 240 > .._._._._.__._._._._._._._._._._._._ 280 > ._._._._._._._._._._._._._._._._._._._._ 320 > ._._._._._._._._._._._._._._._._._._._._ 360 > ._._._._._._._._._._._._._._._._._._._._ 400 > ._._._._._._._._._._._._._._._._._._._._ 440 > ._._._._._._._._libvirt: Network Filter Driver error : internal > error: unexpected nwfilter URI path '/session', try nwfilter:///system > libvirt: Network Filter Driver error : internal error: unexpected > nwfilter URI path '/session', try nwfilter:///system > libvirt: Network Filter Driver error : internal error: unexpected > nwfilter URI path '/session', try nwfilter:///system > libvirt: Network Filter Driver error : internal error: unexpected > nwfilter URI path '/session', try nwfilter:///system > libvirt: Network Filter Driver error : internal error: unexpected > nwfilter URI path '/session', try nwfilter:///system > ._._._._._._._._._._._._ 480 > > > So do you have any other idea? I came up with two already and neither of > them got through review. Just to remind everybody, we are possibly > touching live user data here so we need a resolution rather sooner than > later. I specifically NACKd the part that installs the callback for suppressing error messages. The messages here need to be suppressed by some other way, but we should not decrease the debugability of tests. The error handler installation does not seem to have to do with live user data touching. I did not try to see what the other part of this patch does. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
On 07/12/2018 10:01 AM, Peter Krempa wrote: > On Thu, Jul 12, 2018 at 09:37:47 +0200, Michal Privoznik wrote: >> So far we are setting only fake secret and storage drivers. >> Therefore if the code wants to call a public NWFilter API (like >> qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are >> doing) the virGetConnectNWFilter() function will try to actually >> spawn session daemon because there's no connection object set to >> handle NWFilter driver. >> >> Even though I haven't experienced the same problem with the rest >> of the drivers (interface, network and node dev), the reasoning >> above can be applied to them as well. >> >> At the same time, now that connection object is registered for >> the drivers, the public APIs will throw >> virReportUnsupportedError(). And since we don't provide any error >> func the error is printed to stderr. Fix this by setting dummy >> error func. >> >> Signed-off-by: Michal Privoznik >> --- >> tests/qemuxml2argvtest.c | 6 ++ >> 1 file changed, 6 insertions(+) > > [...] > >> >> @@ -652,6 +656,8 @@ mymain(void) >> return EXIT_FAILURE; >> } >> >> +virTestQuiesceLibvirtErrors(true); >> + > > NACK, this suppresses legitimate errors in the testsuite. > > I've mangled one of the XML files and ran the qemuxml2argvtest with > VIR_TEST_DEBUG=1 and got: > > 249) QEMU XML-2-startup-XML disk-drive-cache-directsync... > SKIP > 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... > FAILED > 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe... > SKIP > > Without this patch I'd get: > > 249) QEMU XML-2-startup-XML disk-drive-cache-directsync... > SKIP > 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... > libvirt: Domain Config error : unsupported configuration: unknown disk cache > mode 'unafe' > FAILED > 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe... > SKIP > Well, without it I get: tests $ ./qemuxml2argvtest TEST: qemuxml2argvtest ._._._._..._._._._._._._._._._._._._._._ 40 ._._._._._._._._._._._._._._._._._._._._ 80 ._._._._._._._._._._._._._._._._._._._._ 120 ._._._._._._._._._._._._._._._._._._._._ 160 ._._._._._._._._._._._._._._._._._._._._ 200 ._._._._._._._._._._._._._._._._..._._._ 240 .._._._._.__._._._._._._._._._._._._ 280 ._._._._._._._._._._._._._._._._._._._._ 320 ._._._._._._._._._._._._._._._._._._._._ 360 ._._._._._._._._._._._._._._._._._._._._ 400 ._._._._._._._._._._._._._._._._._._._._ 440 ._._._._._._._._libvirt: Network Filter Driver error : internal error: unexpected nwfilter URI path '/session', try nwfilter:///system libvirt: Network Filter Driver error : internal error: unexpected nwfilter URI path '/session', try nwfilter:///system libvirt: Network Filter Driver error : internal error: unexpected nwfilter URI path '/session', try nwfilter:///system libvirt: Network Filter Driver error : internal error: unexpected nwfilter URI path '/session', try nwfilter:///system libvirt: Network Filter Driver error : internal error: unexpected nwfilter URI path '/session', try nwfilter:///system ._._._._._._._._._._._._ 480 So do you have any other idea? I came up with two already and neither of them got through review. Just to remind everybody, we are possibly touching live user data here so we need a resolution rather sooner than later. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 04/35] cfg.mk: single variable declaration per line when using cleanup macro
On Wed, Jul 11, 2018 at 08:36:59PM +0530, Sukrit Bhatnagar wrote: > The problem is not that it is initialized to a non-NULL value. > If we were to detect multiple declarations in a line. we would > search for a comma (separator), right? In the case I mentioned, > the comma inside the function has to be avoided by the rule. Syntax-wise, our macro signatures follow the ones of a function, i.e. you also have parentheses. If we're ever going to create a syntax-check rule for that it won't be as simple as matching commas, you'd use more context for such a regex, for the reasons you've mentioned. Therefore, the example you provided would never be affected by such a rule. Erik > On Wed, 11 Jul 2018 at 15:57, Erik Skultety wrote: > > > > On Wed, Jul 11, 2018 at 10:35:25AM +0200, Pavel Hrdina wrote: > > > On Wed, Jul 11, 2018 at 12:42:43AM +0530, Sukrit Bhatnagar wrote: > > > > On Tue, 10 Jul 2018 at 16:24, Erik Skultety wrote: > > > > > > > > > > On Sat, Jun 30, 2018 at 02:30:08PM +0530, Sukrit Bhatnagar wrote: > > > > > > Add rule to ensure that each variable declaration made using > > > > > > a cleanup macro is in its own separate line. > > > > > > > > > > > > Sometimes a variable might be initialized from a value returned > > > > > > by a macro or a function, which may take on more than one > > > > > > parameter, thereby introducing a comma, which might be mistaken > > > > > > for multiple declarations in a line. This rule takes care of > > > > > > that too. > > > > > > > > > > I can't think of an example or I'm just not seeing it, can you please > > > > > give me > > > > > an example where you actually need the rule below? Because right now > > > > > I don't > > > > > see a need for it. > > > > > > > > In src/util/virfile.c in virFileAbsPath function: > > > > ... > > > > VIR_AUTOFREE(char *) buf = getcwd(NULL, 0); > > > > ... > > > > > > I don't see anything wrong with it, it is properly initialized to some > > > value, it doesn't have to be only NULL. > > > > > > Pavel > > > > Agreed, > > > > Erik > > > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
On Thu, Jul 12, 2018 at 09:37:47 +0200, Michal Privoznik wrote: > So far we are setting only fake secret and storage drivers. > Therefore if the code wants to call a public NWFilter API (like > qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are > doing) the virGetConnectNWFilter() function will try to actually > spawn session daemon because there's no connection object set to > handle NWFilter driver. > > Even though I haven't experienced the same problem with the rest > of the drivers (interface, network and node dev), the reasoning > above can be applied to them as well. > > At the same time, now that connection object is registered for > the drivers, the public APIs will throw > virReportUnsupportedError(). And since we don't provide any error > func the error is printed to stderr. Fix this by setting dummy > error func. > > Signed-off-by: Michal Privoznik > --- > tests/qemuxml2argvtest.c | 6 ++ > 1 file changed, 6 insertions(+) [...] > > @@ -652,6 +656,8 @@ mymain(void) > return EXIT_FAILURE; > } > > +virTestQuiesceLibvirtErrors(true); > + NACK, this suppresses legitimate errors in the testsuite. I've mangled one of the XML files and ran the qemuxml2argvtest with VIR_TEST_DEBUG=1 and got: 249) QEMU XML-2-startup-XML disk-drive-cache-directsync... SKIP 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... FAILED 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe... SKIP Without this patch I'd get: 249) QEMU XML-2-startup-XML disk-drive-cache-directsync... SKIP 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... libvirt: Domain Config error : unsupported configuration: unknown disk cache mode 'unafe' FAILED 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe... SKIP > if (qemuTestDriverInit() < 0) > return EXIT_FAILURE; > > -- > 2.16.4 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Thu, 12 Jul 2018 08:51:16 +0200
Markus Armbruster wrote:
> Markus Armbruster writes:
>
> > Kevin Wolf writes:
> >
> >> Am 10.07.2018 um 16:22 hat Cornelia Huck geschrieben:
> >>> On Tue, 10 Jul 2018 07:59:15 +0200
> >>> Markus Armbruster wrote:
> >>>
> >>> > In addition to actively pulling libvirt developers into review of
> >>> > deprecation patches, we should pursue the idea to optionally let QEMU
> >>> > fail on use of deprecated features, then have libvirt run its test suite
> >>> > that way.
> >>>
> >>> What about the following:
> >>>
> >>> qemu_deprecated_option("old_option", "modern_option");
> >>>
> >>> Which would then print (in normal operation)
> >>>
> >>> "WARNING: 'old_option' is deprecated and will be removed; use
> >>> 'modern_option' instead"
> >>>
> >>> to the monitor (or to stderr? to both?).
> >>>
> >>> If you start QEMU with a -no-deprecated-options switch, it would print
> >>>
> >>> "ERROR: 'old_option' is deprecated and will be removed; use
> >>> 'modern_option' instead"
> >>>
> >>> and do an exit(1).
> >>>
> >>> Would that be workable?
> >>
> >> I think the function should just take a message:
> >>
> >> /* Works like error_report(), except for the WARNING/ERROR prefix
> >> * and exit(1) if -no-deprecated-options is set */
> >> void deprecation_report(const char *fmt, ...);
> >
> > I like it. The contract could use a bit of polish, but that's detail.
>
> Suggest --deprecated={silent,warn,error}, default silent.
I like that, but I'd prefer to default to warn (so that command line
users have a better chance to notice it).
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 4/5] check-file-access: Allow specifying action
The check-file-access.pl script is used to match access list
generated by virtestmock against whitelisted rules stored in
file_access_whitelist.txt. So far the rules are in form:
$path: $progname: $testname
This is not sufficient because the rule does not take into
account 'action' that caused $path to appear in the list of
accessed files. After this commit the rule can be in new form:
$path: $action: $progname: $testname
where $action is one from ("open", "fopen", "access", "stat",
"lstat", "connect"). This way the white list can be fine tuned to
allow say access() but not connect().
Signed-off-by: Michal Privoznik
---
tests/check-file-access.pl | 32 +++-
tests/file_access_whitelist.txt | 15 ++-
2 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/tests/check-file-access.pl b/tests/check-file-access.pl
index 977a2bc533..ea0b7a18a2 100755
--- a/tests/check-file-access.pl
+++ b/tests/check-file-access.pl
@@ -27,18 +27,21 @@ use warnings;
my $access_file = "test_file_access.txt";
my $whitelist_file = "file_access_whitelist.txt";
+my @known_actions = ("open", "fopen", "access", "stat", "lstat", "connect");
+
my @files;
my @whitelist;
open FILE, "<", $access_file or die "Unable to open $access_file: $!";
while () {
chomp;
-if (/^(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
+if (/^(\S*):\s*(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
my %rec;
${rec}{path} = $1;
-${rec}{progname} = $2;
-if (defined $4) {
-${rec}{testname} = $4;
+${rec}{action} = $2;
+${rec}{progname} = $3;
+if (defined $5) {
+${rec}{testname} = $5;
}
push (@files, \%rec);
} else {
@@ -52,7 +55,21 @@ while () {
chomp;
if (/^\s*#.*$/) {
# comment
+} elsif (/^(\S*):\s*(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/ and
+grep /^$2$/, @known_actions) {
+# $path: $action: $progname: $testname
+my %rec;
+${rec}{path} = $1;
+${rec}{action} = $3;
+if (defined $4) {
+${rec}{progname} = $4;
+}
+if (defined $6) {
+${rec}{testname} = $6;
+}
+push (@whitelist, \%rec);
} elsif (/^(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/) {
+# $path: $progname: $testname
my %rec;
${rec}{path} = $1;
if (defined $3) {
@@ -79,6 +96,11 @@ for my $file (@files) {
next;
}
+if (defined %${rule}{action} and
+not %${file}{action} =~ m/^$rule->{action}$/) {
+next;
+}
+
if (defined %${rule}{progname} and
not %${file}{progname} =~ m/^$rule->{progname}$/) {
next;
@@ -95,7 +117,7 @@ for my $file (@files) {
if (not $match) {
$error = 1;
-print "$file->{path}: $file->{progname}";
+print "$file->{path}: $file->{action}: $file->{progname}";
print ": $file->{testname}" if defined %${file}{testname};
print "\n";
}
diff --git a/tests/file_access_whitelist.txt b/tests/file_access_whitelist.txt
index 850b28506e..3fb318cbab 100644
--- a/tests/file_access_whitelist.txt
+++ b/tests/file_access_whitelist.txt
@@ -1,14 +1,17 @@
# This is a whitelist that allows accesses to files not in our
# build directory nor source directory. The records are in the
-# following format:
+# following formats:
#
# $path: $progname: $testname
+# $path: $action: $progname: $testname
#
-# All these three are evaluated as perl RE. So to allow /dev/sda
-# and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
+# All these variables are evaluated as perl RE. So to allow
+# /dev/sda and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
# /proc/$pid/status you can '/proc/\d+/status' and so on.
-# Moreover, $progname and $testname can be empty, in which which
-# case $path is allowed for all tests.
+# Moreover, $action, $progname and $testname can be empty, in which
+# which case $path is allowed for all tests. However, $action (if
+# specified) must be one of "open", "fopen", "access", "stat",
+# "lstat", "connect".
/bin/cat: sysinfotest
/bin/dirname: sysinfotest: x86 sysinfo
@@ -19,5 +22,7 @@
/etc/hosts
/proc/\d+/status
+/etc/passwd: fopen
+
# This is just a dummy example, DO NOT USE IT LIKE THAT!
.*: nonexistent-test-touching-everything
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 5/5] virtestmock: Track action
As advertised in the previous commit, we need the list of
accessed files to also contain action that caused the $path to
appear on the list. Not only this enables us to fine tune our
white list rules it also helps us to see why $path is reported.
For instance:
/run/user/1000/libvirt/libvirt-sock: connect: qemuxml2argvtest: QEMU
XML-2-ARGV net-vhostuser-multiq
Signed-off-by: Michal Privoznik
---
tests/virtestmock.c | 39 ++-
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/tests/virtestmock.c b/tests/virtestmock.c
index 654af24a10..25aadf8aea 100644
--- a/tests/virtestmock.c
+++ b/tests/virtestmock.c
@@ -88,7 +88,8 @@ static void init_syms(void)
}
static void
-printFile(const char *file)
+printFile(const char *file,
+ const char *func)
{
FILE *fp;
const char *testname = getenv("VIR_TEST_MOCK_TESTNAME");
@@ -116,9 +117,9 @@ printFile(const char *file)
}
/* Now append the following line into the output file:
- * $file: $progname $testname */
+ * $file: $progname: $func: $testname */
-fprintf(fp, "%s: %s", file, progname);
+fprintf(fp, "%s: %s: %s", file, func, progname);
if (testname)
fprintf(fp, ": %s", testname);
@@ -128,8 +129,12 @@ printFile(const char *file)
fclose(fp);
}
+#define CHECK_PATH(path) \
+checkPath(path, __FUNCTION__)
+
static void
-checkPath(const char *path)
+checkPath(const char *path,
+ const char *func)
{
char *fullPath = NULL;
char *relPath = NULL;
@@ -160,7 +165,7 @@ checkPath(const char *path)
if (!STRPREFIX(path, abs_topsrcdir) &&
!STRPREFIX(path, abs_topbuilddir)) {
-printFile(path);
+printFile(path, func);
}
VIR_FREE(crippledPath);
@@ -180,7 +185,7 @@ int open(const char *path, int flags, ...)
init_syms();
-checkPath(path);
+CHECK_PATH(path);
if (flags & O_CREAT) {
va_list ap;
@@ -199,7 +204,7 @@ FILE *fopen(const char *path, const char *mode)
{
init_syms();
-checkPath(path);
+CHECK_PATH(path);
return real_fopen(path, mode);
}
@@ -209,7 +214,7 @@ int access(const char *path, int mode)
{
init_syms();
-checkPath(path);
+CHECK_PATH(path);
return real_access(path, mode);
}
@@ -239,7 +244,7 @@ int stat(const char *path, struct stat *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "stat");
return real_stat(path, sb);
}
@@ -250,7 +255,7 @@ int stat64(const char *path, struct stat64 *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "stat");
return real_stat64(path, sb);
}
@@ -262,7 +267,7 @@ __xstat(int ver, const char *path, struct stat *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "stat");
return real___xstat(ver, path, sb);
}
@@ -274,7 +279,7 @@ __xstat64(int ver, const char *path, struct stat64 *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "stat");
return real___xstat64(ver, path, sb);
}
@@ -286,7 +291,7 @@ lstat(const char *path, struct stat *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "lstat");
return real_lstat(path, sb);
}
@@ -298,7 +303,7 @@ lstat64(const char *path, struct stat64 *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "lstat");
return real_lstat64(path, sb);
}
@@ -310,7 +315,7 @@ __lxstat(int ver, const char *path, struct stat *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "lstat");
return real___lxstat(ver, path, sb);
}
@@ -322,7 +327,7 @@ __lxstat64(int ver, const char *path, struct stat64 *sb)
{
init_syms();
-checkPath(path);
+checkPath(path, "lstat");
return real___lxstat64(ver, path, sb);
}
@@ -337,7 +342,7 @@ int connect(int sockfd, const struct sockaddr *addr,
socklen_t addrlen)
if (addrlen == sizeof(struct sockaddr_un)) {
struct sockaddr_un *tmp = (struct sockaddr_un *) addr;
if (tmp->sun_family == AF_UNIX)
-checkPath(tmp->sun_path);
+CHECK_PATH(tmp->sun_path);
}
#endif
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
So far we are setting only fake secret and storage drivers. Therefore if the code wants to call a public NWFilter API (like qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are doing) the virGetConnectNWFilter() function will try to actually spawn session daemon because there's no connection object set to handle NWFilter driver. Even though I haven't experienced the same problem with the rest of the drivers (interface, network and node dev), the reasoning above can be applied to them as well. At the same time, now that connection object is registered for the drivers, the public APIs will throw virReportUnsupportedError(). And since we don't provide any error func the error is printed to stderr. Fix this by setting dummy error func. Signed-off-by: Michal Privoznik --- tests/qemuxml2argvtest.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a929e4314e..28073e2c40 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -471,6 +471,10 @@ testCompareXMLToArgv(const void *data) conn->secretDriver = conn->storageDriver = +virSetConnectInterface(conn); +virSetConnectNetwork(conn); +virSetConnectNWFilter(conn); +virSetConnectNodeDev(conn); virSetConnectSecret(conn); virSetConnectStorage(conn); @@ -652,6 +656,8 @@ mymain(void) return EXIT_FAILURE; } +virTestQuiesceLibvirtErrors(true); + if (qemuTestDriverInit() < 0) return EXIT_FAILURE; -- 2.16.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/5] Don't touch user data from tests
This is a v2 of: https://www.redhat.com/archives/libvir-list/2018-July/msg00425.html diff to v1: - Patch 1/6 from original series was ACKed and thus pushed, - Patch 2/6 from original series is replaced with 1/5, - The rest of the patches remained unreviewed, therefore I'm resending them. Michal Prívozník (5): qemuxml2argvtest: Set more fake drivers Forget last daemon/ dir artefacts virtestmock: Track connect() too check-file-access: Allow specifying action virtestmock: Track action Makefile.am | 2 +- run.in | 2 +- tests/check-file-access.pl | 32 ++ tests/file_access_whitelist.txt | 15 +++ tests/libvirtd-fail | 4 +-- tests/qemuxml2argvtest.c| 6 + tests/virtestmock.c | 59 ++--- 7 files changed, 90 insertions(+), 30 deletions(-) -- 2.16.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/5] Forget last daemon/ dir artefacts
The most important part is LIBVIRTD_PATH env var fix. It is used in virFileFindResourceFull() from tests. The libvirtd no longer lives under daemon/. Then, libvirtd-fail test was still failing (as expected) but not because of missing config file but because it was trying to execute (nonexistent) top_builddir/daemon/libvirtd which fulfilled expected outcome and thus test did not fail. Thirdly, lcov was told to generate coverage for daemon/ dir too. Signed-off-by: Michal Privoznik --- Makefile.am | 2 +- run.in | 2 +- tests/libvirtd-fail | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Makefile.am b/Makefile.am index 1926e21b7a..709064c6a6 100644 --- a/Makefile.am +++ b/Makefile.am @@ -80,7 +80,7 @@ check-access: cov: clean-cov $(MKDIR_P) $(top_builddir)/coverage $(LCOV) -c -o $(top_builddir)/coverage/libvirt.info.tmp \ - -d $(top_builddir)/src -d $(top_builddir)/daemon \ + -d $(top_builddir)/src \ -d $(top_builddir)/tests $(LCOV) -r $(top_builddir)/coverage/libvirt.info.tmp \ -o $(top_builddir)/coverage/libvirt.info diff --git a/run.in b/run.in index cbef61a674..06ad54b62b 100644 --- a/run.in +++ b/run.in @@ -63,7 +63,7 @@ export PKG_CONFIG_PATH export LIBVIRT_DRIVER_DIR="$b/src/.libs" export LIBVIRT_LOCK_MANAGER_PLUGIN_DIR="$b/src/.libs" export VIRTLOCKD_PATH="$b/src" -export LIBVIRTD_PATH="$b/daemon" +export LIBVIRTD_PATH="$b/src" # This is a cheap way to find some use-after-free and uninitialized # read problems when using glibc. diff --git a/tests/libvirtd-fail b/tests/libvirtd-fail index 6c61b892cb..f9e927b61f 100755 --- a/tests/libvirtd-fail +++ b/tests/libvirtd-fail @@ -5,12 +5,12 @@ if test "$VERBOSE" = yes; then set -x - $abs_top_builddir/daemon/libvirtd --version + $abs_top_builddir/src/libvirtd --version fi fail=0 -$abs_top_builddir/daemon/libvirtd --config=no-such-conf --timeout=5 2> log +$abs_top_builddir/src/libvirtd --config=no-such-conf --timeout=5 2> log RET=$? test "$RET" != "0" && exit 0 || exit 1 -- 2.16.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/5] virtestmock: Track connect() too
The aim of this mock is to track if a test doesn't touch anything
in live system. Well, connect() which definitely falls into that
category isn't tracked yet.
Signed-off-by: Michal Privoznik
---
tests/virtestmock.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/tests/virtestmock.c b/tests/virtestmock.c
index 9b91adec77..654af24a10 100644
--- a/tests/virtestmock.c
+++ b/tests/virtestmock.c
@@ -27,6 +27,10 @@
#include
#include
#include
+#include
+#ifdef HAVE_SYS_UN_H
+# include
+#endif
#include "internal.h"
#include "configmake.h"
@@ -61,6 +65,7 @@ static int (*real_lstat)(const char *path, struct stat *sb);
static int (*real_lstat64)(const char *path, void *sb);
static int (*real___lxstat)(int ver, const char *path, struct stat *sb);
static int (*real___lxstat64)(int ver, const char *path, void *sb);
+static int (*real_connect)(int fd, const struct sockaddr *addr, socklen_t
addrlen);
static const char *progname;
const char *output;
@@ -79,6 +84,7 @@ static void init_syms(void)
VIR_MOCK_REAL_INIT_ALT(stat64, __xstat64);
VIR_MOCK_REAL_INIT_ALT(lstat, __lxstat);
VIR_MOCK_REAL_INIT_ALT(lstat64, __lxstat64);
+VIR_MOCK_REAL_INIT(connect);
}
static void
@@ -321,3 +327,19 @@ __lxstat64(int ver, const char *path, struct stat64 *sb)
return real___lxstat64(ver, path, sb);
}
#endif
+
+
+int connect(int sockfd, const struct sockaddr *addr, socklen_t addrlen)
+{
+init_syms();
+
+#ifdef HAVE_SYS_UN_H
+if (addrlen == sizeof(struct sockaddr_un)) {
+struct sockaddr_un *tmp = (struct sockaddr_un *) addr;
+if (tmp->sun_family == AF_UNIX)
+checkPath(tmp->sun_path);
+}
+#endif
+
+return real_connect(sockfd, addr, addrlen);
+}
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Thu, Jul 12, 2018 at 08:59:44 +0200, Markus Armbruster wrote: > Peter Krempa writes: > > On Tue, Jul 10, 2018 at 17:01:22 +0200, Cornelia Huck wrote: > >> On Tue, 10 Jul 2018 16:39:31 +0200 > >> Peter Krempa wrote: [...] > > An option is to do a automatic testing where one of this approaches will > > be enabled. For that you need a way to generate configurations which > > libvirt would use in real life. We have a rather big collection of XMLs > > which describe a valid configuration but the problem with using them on > > a real qemu is that most of the disk paths/network targets/other > > resources are made up and making them work with a real qemu would range > > from being painful to being impossible. > > I sympathize. However, it's not clear which one's harder, providing > environments for a sufficiently wide range of configurations (possibly > mockups), or hacking QEMU to do nothing but check configuration. QEMU That's the main reason I think we should make it possible to use the data for the 'qemuxml2argv' test suite in libvirt. We require that new features are covered by this so that means that the testsuite is possibly the most comprehensive collection of libvirt configurations I know of. It's not perfect as we in many cases don't test any possible value but just try to excercise the code to generate them and others are left behind. Another historical problem was that we've defined a set of capabilities rather than using any real example to do this so many of the commandlines generated and tested are basically impossible to get in real life. That's why I added testing with real capabilities. We'll just need to generate a bunch of files to achieve full coverage here. > isn't designed for that, and configuration checking is intertwined with > everything else. Complete disentanglement looks impractical to me. I I agree. Getting anything special than the real codepath may create bubbles of problems still. On the other hand we'll need some guidance on what's sufficient to do to execute the deprecation detection code. This may require some coding style guidelines in qemu. E.g. no deprecation warnings after the vCPUs are started. Running a full operating system to check the warinigns would be utterly impractical. Preferrably we would get away with starting qemu and waiting for the monitor to start. > guess we could do something useful at the QAPI level, though. Yet > another reason to qapify the command line... That would be great, but I think that there's a subset of things that can be deprecated but can't be expressed by schema. In such case we still need to run the programatic checks to see. > > If we start from scratch you then lack coverage. > > > >> If we fail with exit(1), can libvirt check any message that is logged > >> right before that? > > > > Yes we currently use this for very early failures which occur prior to > > the monitor working. > > > >> > To make any reasonable use of -no-deprecated-options we'd also need > >> > something that simulates qemu startup (no resources are touched in fact) > >> > so that we can run it against the testsuite. Otherwise the use will be > >> > limited to developers using it with the configuration they are > >> > currently testing. > >>> > >> Would that moan loudly that you should poke the libvirt developers if > >> some kind of testsuite failure is detected? Or am I misunderstanding? > > > > Generally it should make somebody complain. But there is a problem. > > Since we are talking deprecation it can't be enabled by default. And by > > not making it default most of the users will not enable that option. > > I don't think end users should do the work of catching use of deprecated > features. It's a CI job. > > In a CI context, we don't need fancy QMP infrastructure to communicate > "you used a deprecated feature", we can get away with printing an > explanation to stderr and exit(1). That should make CI fail, and the > failure should make a developer read the explanation. To unbreak CI, he > can either fix the problem right away, or file a BZ and suppress the CI > failure until it's fixed, say by downgrading --deprecated=error to > --deprecated=warn. Definitely. Plain untranslated error message is fine. The only thing is that it should be easy to detect. exit(1) is that solution. Or rather exit($VALUE_SPECIFIC_FOR_DEPRECATION) so that we can automatically discriminate test failures from deprecation warnings. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
On Thu, Jul 12, 2018 at 08:38:25 +0200, Markus Armbruster wrote: > Kevin Wolf writes: > > Am 10.07.2018 um 16:22 hat Cornelia Huck geschrieben: > >> On Tue, 10 Jul 2018 07:59:15 +0200 > >> Markus Armbruster wrote: > >> Would that be workable? > > > > I think the function should just take a message: > > > > /* Works like error_report(), except for the WARNING/ERROR prefix > > * and exit(1) if -no-deprecated-options is set */ > > void deprecation_report(const char *fmt, ...); > > I like it. The contract could use a bit of polish, but that's detail. > > > We don't necessarily deprecate only options, but we might also deprecate > > monitor commands, specific options values (while keeping other values of > > the same option) etc. > > Exactly. For monitor commands we luckily have QMP introspection which can help a lot in this case. At least for deprecating stuff that is expressable by the schema. In libvirt we are actually doing schema validation of the blockdev-add arguments generators and most commands which are covered by the qemumonitorjsontest. The schema used is based on our capability detection so it's gathered from the most-recent version of qemu we have required for our tests (which is most of the time based on GIT version of qemu if there are any significant new features). If the deprecation will be expressable by the schema it should be rather simple to modify the schema validator to catch the deprecation flags and report errors in our testsuite. CI-ifying of the above should be then also very simple. We'd just gather fresh QMP schema rather than using one from our test case pantry. Some time ago I also added testing of the commandline generator in libvirt with the most recent capabilities rather than using the historically declared capabilities that we've added when the test was created. This means that we actually test some valid combinations and also if stuff covered by our capability probing is removed the tests will catch it. I was also thinking of adding a tool which would use the above tests to attempt starting of a qemu process until the monitor shows up. That test then could also use -no-deprecated-options. I'm hoping waiting for the monitor is sufficient to excercise most of the code which could contain deprecation warnings. (Alternatively we can go through the pre-cpu-startup setup done on the monitor as well). Unfortunately doing this will not be as simple asi the test cases contain random disk paths and other resources which may not be available. This means that it will require some in-place modification and creative temporary resource usage. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Peter Krempa writes: > On Tue, Jul 10, 2018 at 17:01:22 +0200, Cornelia Huck wrote: >> On Tue, 10 Jul 2018 16:39:31 +0200 >> Peter Krempa wrote: >> > On Tue, Jul 10, 2018 at 16:22:08 +0200, Cornelia Huck wrote: >> > > On Tue, 10 Jul 2018 07:59:15 +0200 >> > > Markus Armbruster wrote: > > [...] > >> > > "ERROR: 'old_option' is deprecated and will be removed; use >> > > 'modern_option' instead" >> > > >> > > and do an exit(1). >> > > >> > > Would that be workable? >> > >> > For delivering the warnings via monitor you'll need a store that will >> > collect all the warnings and prepare them for delivery. You've got >> > basically two options: >> > >> > 1) monitor command to poll for deprecated options >> > 2) event with deprecated options >> > >> > Both require storing them since libvirt connects to the monitor only >> > after the command line is processed. >> > >> > Warnings printed to stderr are nearly useless because until something >> > breaks nobody bothers to read the log files. >> >> So, from that I gather that a hard failure would be the easiest for >> libvirt to detect (and everything else would become complicated really >> quickly), right? > > People start complaining only when stuff breaks. If anything is optional > people will usually not enable it. That makes any non-mandatory option > not work in most cases. > > Since we are talking about deprecation we can't really make any of this > default though so there will always be a level of user interaction > required. > > An option is to do a automatic testing where one of this approaches will > be enabled. For that you need a way to generate configurations which > libvirt would use in real life. We have a rather big collection of XMLs > which describe a valid configuration but the problem with using them on > a real qemu is that most of the disk paths/network targets/other > resources are made up and making them work with a real qemu would range > from being painful to being impossible. I sympathize. However, it's not clear which one's harder, providing environments for a sufficiently wide range of configurations (possibly mockups), or hacking QEMU to do nothing but check configuration. QEMU isn't designed for that, and configuration checking is intertwined with everything else. Complete disentanglement looks impractical to me. I guess we could do something useful at the QAPI level, though. Yet another reason to qapify the command line... > If we start from scratch you then lack coverage. > >> If we fail with exit(1), can libvirt check any message that is logged >> right before that? > > Yes we currently use this for very early failures which occur prior to > the monitor working. > >> > To make any reasonable use of -no-deprecated-options we'd also need >> > something that simulates qemu startup (no resources are touched in fact) >> > so that we can run it against the testsuite. Otherwise the use will be >> > limited to developers using it with the configuration they are >> > currently testing. >>> >> Would that moan loudly that you should poke the libvirt developers if >> some kind of testsuite failure is detected? Or am I misunderstanding? > > Generally it should make somebody complain. But there is a problem. > Since we are talking deprecation it can't be enabled by default. And by > not making it default most of the users will not enable that option. I don't think end users should do the work of catching use of deprecated features. It's a CI job. In a CI context, we don't need fancy QMP infrastructure to communicate "you used a deprecated feature", we can get away with printing an explanation to stderr and exit(1). That should make CI fail, and the failure should make a developer read the explanation. To unbreak CI, he can either fix the problem right away, or file a BZ and suppress the CI failure until it's fixed, say by downgrading --deprecated=error to --deprecated=warn. [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Markus Armbruster writes:
> Kevin Wolf writes:
>
>> Am 10.07.2018 um 16:22 hat Cornelia Huck geschrieben:
>>> On Tue, 10 Jul 2018 07:59:15 +0200
>>> Markus Armbruster wrote:
>>>
>>> > In addition to actively pulling libvirt developers into review of
>>> > deprecation patches, we should pursue the idea to optionally let QEMU
>>> > fail on use of deprecated features, then have libvirt run its test suite
>>> > that way.
>>>
>>> What about the following:
>>>
>>> qemu_deprecated_option("old_option", "modern_option");
>>>
>>> Which would then print (in normal operation)
>>>
>>> "WARNING: 'old_option' is deprecated and will be removed; use
>>> 'modern_option' instead"
>>>
>>> to the monitor (or to stderr? to both?).
>>>
>>> If you start QEMU with a -no-deprecated-options switch, it would print
>>>
>>> "ERROR: 'old_option' is deprecated and will be removed; use 'modern_option'
>>> instead"
>>>
>>> and do an exit(1).
>>>
>>> Would that be workable?
>>
>> I think the function should just take a message:
>>
>> /* Works like error_report(), except for the WARNING/ERROR prefix
>> * and exit(1) if -no-deprecated-options is set */
>> void deprecation_report(const char *fmt, ...);
>
> I like it. The contract could use a bit of polish, but that's detail.
Suggest --deprecated={silent,warn,error}, default silent.
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Thomas Huth writes: > On 10.07.2018 17:24, Peter Krempa wrote: >> On Tue, Jul 10, 2018 at 17:01:22 +0200, Cornelia Huck wrote: >>> On Tue, 10 Jul 2018 16:39:31 +0200 >>> Peter Krempa wrote: On Tue, Jul 10, 2018 at 16:22:08 +0200, Cornelia Huck wrote: > On Tue, 10 Jul 2018 07:59:15 +0200 > Markus Armbruster wrote: >> >> [...] >> > "ERROR: 'old_option' is deprecated and will be removed; use > 'modern_option' instead" > > and do an exit(1). > > Would that be workable? For delivering the warnings via monitor you'll need a store that will collect all the warnings and prepare them for delivery. You've got basically two options: 1) monitor command to poll for deprecated options 2) event with deprecated options Both require storing them since libvirt connects to the monitor only after the command line is processed. Warnings printed to stderr are nearly useless because until something breaks nobody bothers to read the log files. >>> >>> So, from that I gather that a hard failure would be the easiest for >>> libvirt to detect (and everything else would become complicated really >>> quickly), right? >> >> People start complaining only when stuff breaks. If anything is optional >> people will usually not enable it. That makes any non-mandatory option >> not work in most cases. > > So would it help if we "invert" the logic, i.e. deprecated_report() > would do exit(1) by default? Then, if the (human) users still want to > continue with the deprecated option, they have to add a > "--ignore-deprecation" command line switch to make QEMU start > successfully... You owe the God of Backward Compatibility one rubber chicken for thinking this heretic thought! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Kevin Wolf writes:
> Am 10.07.2018 um 16:22 hat Cornelia Huck geschrieben:
>> On Tue, 10 Jul 2018 07:59:15 +0200
>> Markus Armbruster wrote:
>>
>> > In addition to actively pulling libvirt developers into review of
>> > deprecation patches, we should pursue the idea to optionally let QEMU
>> > fail on use of deprecated features, then have libvirt run its test suite
>> > that way.
>>
>> What about the following:
>>
>> qemu_deprecated_option("old_option", "modern_option");
>>
>> Which would then print (in normal operation)
>>
>> "WARNING: 'old_option' is deprecated and will be removed; use
>> 'modern_option' instead"
>>
>> to the monitor (or to stderr? to both?).
>>
>> If you start QEMU with a -no-deprecated-options switch, it would print
>>
>> "ERROR: 'old_option' is deprecated and will be removed; use 'modern_option'
>> instead"
>>
>> and do an exit(1).
>>
>> Would that be workable?
>
> I think the function should just take a message:
>
> /* Works like error_report(), except for the WARNING/ERROR prefix
> * and exit(1) if -no-deprecated-options is set */
> void deprecation_report(const char *fmt, ...);
I like it. The contract could use a bit of polish, but that's detail.
> We don't necessarily deprecate only options, but we might also deprecate
> monitor commands, specific options values (while keeping other values of
> the same option) etc.
Exactly.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial
Daniel P. Berrangé writes: > On Mon, Jul 09, 2018 at 01:08:38PM +0200, Cornelia Huck wrote: >> On Mon, 09 Jul 2018 08:33:05 +0200 >> Markus Armbruster wrote: >> >> > Peter Maydell writes: >> > >> > > On 6 July 2018 at 15:56, Kevin Wolf wrote: >> > >> Am 06.07.2018 um 13:11 hat Cornelia Huck geschrieben: >> > >>> That way, we can still easily remove old cruft (case (a)), but still >> > >>> accommodate cases like this (case (c)). The obvious drawback is that >> > >>> we'd need someone to curate the deprecation watchlist, to poke the >> > >>> users we're waiting for, and probably remove anyway after some time if >> > >>> they don't get their act together. >> > >> >> > >> The problem is that things are only starting to move after two releases >> > >> have passed. >> > > >> > > Right, so clearly just "put a note in the documentation" isn't >> > > sufficient advertisement/prodding of things going away. >> > >> > Yes. Ideas on more forceful notification have been tossed around, we >> > just have to act on them. >> > >> > > (Also, two >> > > releases is pretty fast. Many of our users will be using distro >> > > packaged versions of QEMU which will lag further behind than >> > > bleeding-edge users. The system version of QEMU on my desktop >> > > machine is 2.5...) >> > >> > If you consume QEMU in a way that's impacted by the changes the >> > deprecation policy guards, you have two sane options: >> > >> > * Track upstream deprecation, either continuously, or at least right >> > after a QEMU release. Since 2.10, they're collected in qemu-doc >> > appendix "Deprecated features". >> >> Can we draw more attention to this in any way? Point it out prominently >> in the release notes? Send a list to known consumers (e.g. libvirt) on >> release time? > > Yes, we should all newly deprecated stuff in the release notes. No-brainer. > For libvirt, I think whenever something is proposed for deprecation > we could just CC libvir-list, or ask one of the libvirt people to > confirm its not being used. If it is, then we should file BZ against > libvirt. Makes sense, but relying on developers getting their cc: right every time is a setting us up for failure. Our tool to help with getting cc: wrong less often is the MAINTAINERS file. Could one of the libvirt developers watch changes to qemu-doc appendix "Deprecated features"? Would putting the appendix in its own .texi help with that? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] docs: schema: Add missing to vsock device
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1600345 Signed-off-by: Han Han --- docs/schemas/domaincommon.rng | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index bd687ce9d3..f24a56392a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4234,6 +4234,9 @@ + + + -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v1] storage: prefer using newDef to save configfile
When re-defining an active storage pool, the configfile has not
changed. This issue was introduced by bfcd8fc, storage: Use
virStoragePoolObjGetDef accessor for driver. So we prefer using
newDef to save configfile.
Signed-off-by: shichangkuo
---
src/storage/storage_driver.c |6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 254818e..8070d15 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -810,13 +810,14 @@ storagePoolDefineXML(virConnectPtr conn,
if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef)))
goto cleanup;
-newDef = NULL;
+newDef = virStoragePoolObjGetNewDef(obj);
def = virStoragePoolObjGetDef(obj);
-if (virStoragePoolObjSaveDef(driver, obj, def) < 0) {
+if (virStoragePoolObjSaveDef(driver, obj, newDef ? newDef : def) < 0) {
virStoragePoolObjRemove(driver->pools, obj);
virObjectUnref(obj);
obj = NULL;
+newDef = NULL;
goto cleanup;
}
@@ -826,6 +827,7 @@ storagePoolDefineXML(virConnectPtr conn,
VIR_INFO("Defining storage pool '%s'", def->name);
pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
+newDef = NULL;
cleanup:
virObjectEventStateQueue(driver->storageEventState, event);
--
1.7.9.5
-
本邮件及其附件含有新华三集团的保密信息，仅限于发送给上面地址中列出
的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、
或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本
邮件！
This e-mail and its attachments contain confidential information from New H3C,
which is
intended only for the person or entity whose address is listed above. Any use
of the
information contained herein in any way (including, but not limited to, total
or partial
disclosure, reproduction, or dissemination) by persons other than the intended
recipient(s) is prohibited. If you receive this e-mail in error, please notify
the sender
by phone or email immediately and delete it!
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
