[libvirt] GSoC 2017

[Michal Privoznik]

Dear list,

the new round of Google Summer of Code has been announced just now [1].
I believe libvirt wants to participate again, therefore I've started a
new wikipage where we can collect ideas for students to work on [2].
If you have any idea just put it on the list please. If you are shy,
drop me an e-mail and I'll put it there for you.

Also, if you want to participate as a mentor, just add yourself to a
project that has none yet (or talk to other mentors for cooperation).

Please note that nothing is decided yet, the program has been just
announced, we have been not selected yet. I haven't even filled out the
application form yet. But in order to do so, I need a list of possible
ideas and mentors.

Thank you!

Michal


1:
https://opensource.googleblog.com/2016/10/announcing-google-code-in-2016-and.html
2: http://wiki.libvirt.org/page/Google_Summer_of_Code_2017

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

Re: [libvirt] [PATCH 2/2 RFC] Purge marked callbacks before dispatching events

[Michal Privoznik]

On 10.10.2016 21:13, Martin Kletzander wrote:
> On Mon, Oct 10, 2016 at 04:03:32PM +0800, Michal Privoznik wrote:
>> On 07.10.2016 19:52, Martin Kletzander wrote:
>>> I don't have any justification for this except an empirical one: with
>>> this patch the code from Bug 1363628 doesn't crash after "leaking".
>>>
>>> I currently don't have properly working valgrind, but I'm working on it
>>> and it might shed some light into this (although it might also not
>>> happen due to the slowdown).
>>>
>>> However in the meantime I attempted some analysis and I got even more
>>> confused than before, I guess.  With this patch the code doesn't crash,
>>> even though virObjectEventStateQueueDispatch() properly skips callbacks
>>> marked as deleted.  What I feel is even more weird, that if I duplicate
>>> the purgatory function (instead of moving it), it crashes again, and I
>>> feel like even more often.
>>>
>>> Weirdly-fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1363628
>>>
>>> Signed-off-by: Martin Kletzander 
>>> ---
>>>  src/conf/object_event.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/conf/object_event.c b/src/conf/object_event.c
>>> index e5af4be68a7e..4066b4673b42 100644
>>> --- a/src/conf/object_event.c
>>> +++ b/src/conf/object_event.c
>>> @@ -821,13 +821,13 @@ virObjectEventStateFlush(virObjectEventStatePtr
>>> state)
>>>  if (state->timer != -1)
>>>  virEventUpdateTimeout(state->timer, -1);
>>>
>>> +/* Purge any deleted callbacks */
>>> +virObjectEventCallbackListPurgeMarked(state->callbacks);
>>> +
>>>  virObjectEventStateQueueDispatch(state,
>>>   ,
>>>   state->callbacks);
>>>
>>> -/* Purge any deleted callbacks */
>>> -virObjectEventCallbackListPurgeMarked(state->callbacks);
>>> -
>>>  state->isDispatching = false;
>>>  virObjectEventStateUnlock(state);
>>>  }
>>
>> Well, I have no idea either, because virObjectEventStateQueueDispatch()
>> calls virObjectEventStateDispatchCallbacks() which in turn calls
>> virObjectEventDispatchMatchCallback() which returns false if the
>> callback->deleted is truem and thus the callback is skipped - just like
>> it is after this patch of yours.
>>
>> This whole bug feels like a refcounting problem and IMO your patch is
>> just making the scenario more unlikely.
>>
> 
> So I've found out what it is thanks to Luayo Huang.  And if I understand
> it correctly, it won't happen with stateful drivers.  Stateless,
> however, will have this problem.
> 
> The problem is that testConnectClose() clears the driver structure,
> including calling virObjectEventStateFree() on the eventState.  It
> clears the whole eventState, but it can happen while the eventState is
> dispatching.  So there's the race.  And it would also explain what I was
> seeing all the time.  qemuCleanup() is the only function in qemu driver
> that would cleanup the eventState and since that gets called from the
> daemon only after other stuff is cleaned up, it won't happen there.
> 

Ah, yeah. I can see that in the code now too. Good job!

> I'll try turning virObjectEventStatePtr into virObject, we'll have
> refcounting then and it will not disappear until it's stopped being
> used.  I'll see if that helps or not, but it could.

Yep, this is classical example of why we need refcounted objects.

Michal

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

[libvirt] [PATCH 0/2] Forbid new-line char in name of networks

[Sławek Kapłoński]

*** BLURB HERE ***

Sławek Kapłoński (2):
  util: Add function to check if string contains some chars
  Forbid new-line char in name of networks

 src/conf/network_conf.c |  5 +
 src/util/virxml.c   | 18 ++
 src/util/virxml.h   |  4 
 3 files changed, 23 insertions(+), 4 deletions(-)

-- 
2.10.0

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

[libvirt] [PATCH 2/2] Forbid new-line char in name of networks

[Sławek Kapłoński]

New line character in name of network is now forbidden because it
mess virsh output and can be confusing for users.

Closes-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=818064
---
 src/conf/network_conf.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index aa39776..a7f802b 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2117,11 +2117,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
 goto error;
 }
 
-if (strchr(def->name, '/')) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("name %s cannot contain '/'"), def->name);
+if (virXMLCheckString("name", def->name, "/\n"))
 goto error;
-}
 
 /* Extract network uuid */
 tmp = virXPathString("string(./uuid[1])", ctxt);
-- 
2.10.0

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

[libvirt] [PATCH 1/2] util: Add function to check if string contains some chars

[Sławek Kapłoński]

This new function can be used to check if e.g. name of XML node don't
contains forbidden chars like "/" or new-line.
---
 src/util/virxml.c | 18 ++
 src/util/virxml.h |  4 
 2 files changed, 22 insertions(+)

diff --git a/src/util/virxml.c b/src/util/virxml.c
index 03bd784..450487e 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -890,6 +890,24 @@ virXMLChildElementCount(xmlNodePtr node)
 return ret;
 }
 
+/**
+ * virXMLCheckString: checks if string contains at least one of
+ * forbidden characters
+ *
+ * Returns: 0 if string don't contains any of given characters, -1 otherwise
+ */
+int virXMLCheckString(const char *nodeName, char *str,
+  const char *forbiddenChars)
+{
+char *c;
+c = strpbrk(str, forbiddenChars);
+if (c) {
+virReportError(VIR_ERR_XML_DETAIL,
+_("invalid char in %s: %c"), nodeName, *c);
+return -1;
+}
+return 0;
+}
 
 /**
  * virXMLNodeToString: convert an XML node ptr to an XML string
diff --git a/src/util/virxml.h b/src/util/virxml.h
index 7a0a1da..676bf5e 100644
--- a/src/util/virxml.h
+++ b/src/util/virxml.h
@@ -75,6 +75,10 @@ char *  virXMLPropString(xmlNodePtr node,
  const char *name);
 long virXMLChildElementCount(xmlNodePtr node);
 
+intvirXMLCheckString(const char *nodeName,
+ char *str,
+ const char *forbiddenChars);
+
 /* Internal function; prefer the macros below.  */
 xmlDocPtr  virXMLParseHelper(int domcode,
  const char *filename,
-- 
2.10.0

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

Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed

[Laine Stump]

On 10/10/2016 02:14 PM, Andrea Bolognani wrote:

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:

Previously libvirt would only add pci-bridge devices automatically
when an address was requested for a device that required a legacy PCI
slot and none was available. This patch expands that support to
dmi-to-pci-bridge (which is needed in order to add a pci-bridge on a
machine with a pcie-root), and pcie-root-port (which is needed to add
a hotpluggable PCIe device). It does *not* automatically add
pcie-switch-upstream-ports or pcie-switch-downstream-ports (and
currently there are no plans for that).
  
Given the existing code to auto-add pci-bridge devices, automatically

adding pcie-root-ports is fairly straightforward. The
dmi-to-pci-bridge support is a bit tricky though, for a few reasons:
  
1) Although the only reason to add a dmi-to-pci-bridge is so that

 there is a reasonable place to plug in a pci-bridge controller,
 most of the time it's not the presence of a pci-bridge *in the
 config* that triggers the requirement to add a dmi-to-pci-bridge.
 Rather, it is the presence of a legacy-PCI device in the config,
 which triggers auto-add of a pci-bridge, which triggers auto-add of
 a dmi-to-pci-bridge (this is handled in
 virDomainPCIAddressSetGrow() - if there's a request to add a
 pci-bridge we'll check if there is a suitable bus to plug it into;
 if not, we first add a dmi-to-pci-bridge).
  
2) Once there is already a single dmi-to-pci-bridge on the system,

 there won't be a need for any more, even if it's full, as long as
 there is a pci-bridge with an open slot - you can also plug
 pci-bridges into existing pci-bridges. So we have to make sure we
 don't add a dmi-to-pci-bridge unless there aren't any
 dmi-to-pci-bridges *or* any pci-bridges.
  
3) Although it is strongly discouraged, it is legal for a pci-bridge

 to be directly plugged into pcie-root, and we don't want to
 auto-add a dmi-to-pci-bridge if there is already a pci-bridge
 that's been forced directly into pcie-root. Finally, although I
 fail to see the utility of it, it is legal to have something like
 this in the xml:
  
   

   
  
 and that will lead to an automatically added dmi-to-pci-bridge at

 index=1 (to give the unaddressed pci-bridge a proper place to plug
 in):
  
   
  
 (for example, see the existing test case

 "usb-controller-default-q35"). This is handled in
 qemuDomainPCIAddressSetCreate() when it's adding in controllers to
 fill holes in the indexes.

I wonder how this "feature" came to be... It seems to be the
reason for quite a bit of convoluted code down below, which
we could avoid if this had never worked at all. As is so
often the case, too late for that :(


Maybe not. The only place I ever saw that was in the above test case, 
and another named "usb-controller-explicit-q35", and the author of both 
of those tests was (wait for it!), no, not Albert Einstein.  Andrea 
Bolognani!


The only reason it worked in the past was because we always 
automatically added the dmi-to-pci-bridge very early in the post-parse 
processing. This implies that any saved config anywhere will already 
have the necessary dmi-to-pci-bridge at index='1', so we only need to 
preserve the behavior for new domain definitions that have a pci-bridge 
at index='2' but nothing at index='1'.


Since you're the only person documented to have ever created a config 
like that, and it would only be problematic if someone tried to create 
another new config, maybe we should just stop accidentally supporting it 
and count it as a bug that's being fixed. What's your opinion?






Although libvirt will now automatically create a dmi-to-pci-bridge
when it's needed, the code still remains for now that forces a
dmi-to-pci-bridge on all domains with pcie-root (in
qemuDomainDefAddDefaultDevices()). That will be removed in the next
patch.
  
For now, the pcie-root-ports are added one to a slot, which is a bit

wasteful and means it will fail after 31 total PCIe devices (30 if
there are also some PCI devices), but helps keep the changeset down
for this patch. A future patch will have 8 pcie-root-ports sharing the
functions on a single slot.
---
   src/conf/domain_addr.c |  88 ++--
   src/qemu/qemu_domain_address.c |  91 ++---
   .../qemuxml2argv-q35-pcie-autoadd.args |  57 
   .../qemuxml2argv-q35-pcie-autoadd.xml  |  51 +++
   tests/qemuxml2argvtest.c   |  23 
   .../qemuxml2xmlout-q35-pcie-autoadd.xml| 147 
+
   tests/qemuxml2xmltest.c|  23 
   7 files changed, 448 insertions(+), 32 deletions(-)
   create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.args
   create mode 100644 

Re: [libvirt] Analysis of the effect of adding PCIe root ports

[Laine Stump]

On 10/10/2016 05:27 AM, Andrea Bolognani wrote:

On Fri, 2016-10-07 at 12:11 -0400, Laine Stump wrote:

So instead of guaranteeing that there will always be an empty
slot available for hotplug during a single start/destroy
cycle of the guest, we would be guaranteeing that there will
be 3/4 empty slots available for either hotplug or coldplug
throughout the entire life of the guest.
  
A better way to put it is that we guarantee there will be "N" (3 or 4 or

whatever) slots available when the domain is originally defined. Once
any change has been made, all bets are off.

Okay, your alternative reading matches with my understanding
of your proposal :)


Sounds like a pretty good compromise to me.
  
The only problem I can think of is that there might be

management applications that add eg. a pcie-root in the XML
when first defining a guest, and after the change such guests
would get zero hotpluggable ports. Then again it's probably
okay to expect such management applications to add the
necessary number of pcie-root-ports themselves.
  
Yeah, if they know enough to be adding a root-port, then they know

enough to add extras.

Note that I wrote pcie-root, not pcie-root-port.


Maybe we could relax the wording on A) and ignore any
pci{,e}-root? Even though there is really no reason for
either a user or a management application to add them
explicitly when defining a guest, I feel like they might be
special enough to deserve an exception.
  
I thought about that; I'm not sure. In the case of libguestfs, even if

Rich added a pcie-root, I guess he would still be manually addressing
his pci devices, so that would be clue enough that he knew what he was
doing and didn't want any extra.

Yeah, I'm not sure about that either, I just felt like it
would be good to think about it. I don't really see problems
going one way or the other, and I guess your initial
proposal is slightly more strict, so we might as well go for
it and relax it later if a compelling use case is found.


I implemented this over the weekend, and it turns out that it's much 
less disruptive to ignore whether the pcie-root controller was added  by 
the user or by libvirt. This is because pcie-root is added much earlier 
(when we are adding the implicit/default devices for the domain), so by 
the time we get to the function that assigns PCI addresses (which is 
where we auto-add all the additional PCI controllers) we no longer have 
any information about whether pcie-root was specified in the XML, or if 
it was auto-added.


The way I have it implemented now is to add the extra controllers if the 
initial controller count was <= 1 *and* any new PCI controllers were 
auto-added during address assignment (which implies that an unaddressed 
device needed an address, so we added a controller to satisfy it). I 
suppose if I want to be pedantic, I need to actually keep track of 
whether or not there were any unaddressed devices (in the case that the 
only devices they requested were primary video + USB2 + SATA controller, 
no new PCI controllers would be added).


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

Re: [libvirt] [PATCH 5/7] vsh: Fix some issues in auto completion code

[John Ferlan]

On 10/10/2016 12:33 PM, Peter Krempa wrote:
> On Mon, Oct 10, 2016 at 11:42:16 -0400, John Ferlan wrote:
>> 1. Move the declaration of const vshCmdDef *help - it should be at the
>>top of the "if" rather than in the middle.
>>
>> 2. Change a comparison from && to || - without doing so we could core
> 
> Same comment as in 3/7 on 'core' not being the appropriate term.
> 
>>on commands like 'virsh list' which would allow completion of some
>>non -- option based on whatever was found in the current working
>>directory and then as soon as that was completed, the next 
>>would core since "opt" would be returned as NULL, but the check
> 
> aaand again.
> 

OK changed them to crash.

>>was dereferencing "&& opt->type"
>>
>> 3. Before dereferencing opt->completer, be sure opt isn't NULL.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  tools/vsh.c | 7 ---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/vsh.c b/tools/vsh.c
>> index 420eb79..9558dad 100644
>> --- a/tools/vsh.c
>> +++ b/tools/vsh.c
>> @@ -1523,10 +1523,11 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
>> *parser)
>>  /* if we encountered --help, replace parsed command with
>>   * 'help ' */
>>  for (tmpopt = first; tmpopt; tmpopt = tmpopt->next) {
>> +const vshCmdDef *help;
>>  if (STRNEQ(tmpopt->def->name, "help"))
>>  continue;
>>  
>> -const vshCmdDef *help = vshCmddefSearch("help");
>> +help = vshCmddefSearch("help");
>>  vshCommandOptFree(first);
>>  first = vshMalloc(ctl, sizeof(vshCmdOpt));
>>  first->def = help->opts;
>> @@ -2788,7 +2789,7 @@ vshReadlineParse(const char *text, int state)
>>   * Try to find the default option.
>>   */
>>  if (!(opt = vshCmddefGetData(cmd, _need_arg, 
>> _seen))
>> -&& opt->type == VSH_OT_BOOL)
>> +|| opt->type == VSH_OT_BOOL)
>>  goto error;
>>  opt_exists = true;
>>  opts_need_arg = const_opts_need_arg;
>> @@ -2824,7 +2825,7 @@ vshReadlineParse(const char *text, int state)
>>  res = vshReadlineCommandGenerator(sanitized_text, state);
>>  } else if (opts_filled && !non_bool_opt_exists) {
>>  res = vshReadlineOptionsGenerator(sanitized_text, state, cmd);
>> -} else if (non_bool_opt_exists && data_complete && opt->completer) {
>> +} else if (non_bool_opt_exists && data_complete && opt && 
>> opt->completer) {
>>  if (!completed_list)
>>  completed_list = opt->completer(autoCompleteOpaque,
>>  opt->completer_flags);
> 
> This function makes my eyes bleed.
> 

Yep - couple shots of tequila or whatever that Borovicka stuff is we had
at KVM Forum was could help ;-)

John

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

Re: [libvirt] [PATCH 0/2] network: add dnsmasq option 'dhcp-authoritative'

[Martin Wilck]

On Mon, 2016-10-10 at 15:28 -0400, Laine Stump wrote:
> Heh. I finally got around to pushing your patch maybe 5 or 10
> minutes 
> before you resent, and just now hit send on the reply message, then
> saw 
> this in my inbox :-) Sorry again for seeming so much like a
> government 
> bureaucracy.

No problem at all, I'm glad you merged my patch despite its formal
problems. Thanks a lot! Weird coincidence that we both worked on this
literally in the same minute :-)

Regards
Martin

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

Re: [libvirt] [PATCH 0/2] network: add dnsmasq option 'dhcp-authoritative'

[Laine Stump]

Heh. I finally got around to pushing your patch maybe 5 or 10 minutes 
before you resent, and just now hit send on the reply message, then saw 
this in my inbox :-) Sorry again for seeming so much like a government 
bureaucracy.




On 10/10/2016 03:20 PM, Martin Wilck wrote:

Lest it be forgotten, here is a rebased version of my "dhcp-authoritative"
patch (against 77d24de). According to dnsmasq(8), this option "should
be set when dnsmasq is definitely the only DHCP server on a network",
whis is the case for libvirt-managed networks.
In practice, this option has the effect that an expired lease can be
reacquired by the client using a DHCPREQUEST unless it has been given
to another client in the meantime. Without "dhcp-authoritative", this
operation always fails, which can be quite cumbersome.
There is no protection against rogue clients hijacking other client's
IP addresses, but that isn't specific to libvirt, and IP addresses don't
provide security anyway.
This is ovbiously not aimed at production environments; it's a convenience
for developers and casual users who'd rather not be bothered with network
XML host entries or the like.

Original submission:
https://www.redhat.com/archives/libvir-list/2016-September/msg00739.html

Daniel's post where he said that "unless there's a obvious downside to it,
it seems reasonable to add that":
https://www.redhat.com/archives/libvir-list/2016-September/msg01305.html

Regards,
Martin

Martin Wilck (2):
   network: add dnsmasq option 'dhcp-authoritative'
   tests/networkxml2confdata: add dhcp-authoritative option

  src/network/bridge_driver.c   | 4 +++-
  tests/networkxml2confdata/dhcp6-nat-network.conf  | 1 +
  tests/networkxml2confdata/dhcp6host-routed-network.conf   | 1 +
  tests/networkxml2confdata/isolated-network.conf   | 1 +
  tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf | 1 +
  tests/networkxml2confdata/nat-network-dns-srv-record.conf | 1 +
  tests/networkxml2confdata/nat-network-dns-txt-record.conf | 1 +
  tests/networkxml2confdata/nat-network-name-with-quotes.conf   | 1 +
  tests/networkxml2confdata/nat-network.conf| 1 +
  tests/networkxml2confdata/netboot-network.conf| 1 +
  tests/networkxml2confdata/netboot-proxy-network.conf  | 1 +
  11 files changed, 13 insertions(+), 1 deletion(-)



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

Re: [libvirt] [PATCH 3/7] qemu: Remove possibility of NULL dereference

[John Ferlan]

On 10/10/2016 12:19 PM, Peter Krempa wrote:
> On Mon, Oct 10, 2016 at 11:42:14 -0400, John Ferlan wrote:
>> If qemubinCaps is NULL, then calling virQEMUCapsGetMachineTypesCaps and
>   ^
> This symbol name refers to the name in the caller. The function uses a
> different name.
> 
>> dereferencing to get the nmachineTypes will cause a core. Rework the code
> 
> It will cause a crash, not a core. A core dump is caused by the crash
> and can be turned off.
> 

Ok - semantics...

>> slightly to avoid the issue and return immediately if !qemubinCaps or
>> !nmachineTypes
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_capabilities.c | 7 +--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index da8f3d1..ee3e50f 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -2405,10 +2405,13 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr 
>> qemuCaps,
>>  size_t i;
>>  
>>  *machines = NULL;
>> +*nmachines = 0;
>> +
>> +if (!qemuCaps || !qemuCaps->nmachineTypes)
>> +return 0;
>>  *nmachines = qemuCaps->nmachineTypes;
>>  
>> -if (*nmachines &&
>> -VIR_ALLOC_N(*machines, qemuCaps->nmachineTypes) < 0)
>> +if (VIR_ALLOC_N(*machines, qemuCaps->nmachineTypes) < 0)
>>  goto error;
> 
> This is a false positive, the caller checks that 'binary' is set and
> does not call this function at all. Coverity probably doesn't see it as
> it depends on the state of 'binary' rather than 'qemubinCaps' in the
> caller.

Hmm... true. Coverity generally has an issue with similar conditions
where there's two arguments that are related and only one is checked
while the other is used. This one's not so obvious. Interesting that the
subsequent call wasn't flagged, but that's perhaps because it's preceded
by "kvmbin &&".

In any case, I've dropped this one and tagged it in my personal coverity
branch as a false positive.

John

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

Re: [libvirt] [RFC/PATCH 1/2] network: add dnsmasq option 'dhcp-authoritative'

[Laine Stump]

On 09/21/2016 04:49 AM, Martin Wilck wrote:

The dnsmasq man page recommends that dhcp-authoritative "should be
set when dnsmasq is definitely the only DHCP server on a network".
This is the case for libvirt-managed virtual networks.

The effect of this is that VMs that fail to renew their DHCP lease
in time (e.g. if the VM or host is suspended) will be able to
re-acquire the lease even if it's expired, unless the IP address has
been taken by some other host. This avoids various annoyances caused
by changing VM IP addresses.


We had earlier (finally!) agreed in principle on pushing this patch, but 
were in freeze at the time, and I later forgot to do it.


I checked to be sure that the dnsmasq on one of the oldest Linux distros 
that we still support does have the dhcp-authoritative option. It does 
(it's dnsmasq-2.48-something) so we don't need any extra code to verify 
the option is there.


I do have a couple comments below, so don't hit the "next" button yet! :-)


---
  src/network/bridge_driver.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 7b99aca..cb4fb1c 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1258,8 +1258,10 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
  
  /* Note: the following is IPv4 only */

  if (VIR_SOCKET_ADDR_IS_FAMILY(>address, AF_INET)) {
-if (ipdef->nranges || ipdef->nhosts)
+if (ipdef->nranges || ipdef->nhosts) {
  virBufferAddLit(, "dhcp-no-override\n");
+virBufferAddLit(, "dhcp-authoritative\n");
+   }


You accidentally got a tab character in the line below, so I replaced it 
with spaces so that make syntax-check passes.


  
  if (ipdef->tftproot) {

  virBufferAddLit(, "enable-tftp\n");



Although your cover letter included diff statistics for the 
networkxml2conftest test cases that needed the extra line for 
dhcp-authoritative, those diffs weren't included in the patch itself. 
Fortunately it's simple to regenerate the test case outputs 
(VIR_TEST_REGENERATE_OUTPUT=1 tests/networkxml2conftest), so I 
regenerated and verified them.



ACK, and pushed. Thanks for the submission, and sorry for the delay (and 
the seemingly protracted debate about a single line - we've been burned 
in the past by unforeseen consequences of adding a seemingly innocuous 
dnsmasq option to our dnsmasq conf files, and I was nervous about that 
happening again, so I wanted to make sure we explored all possibilities.)


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

[libvirt] [PATCH 2/2] tests/networkxml2confdata: add dhcp-authoritative option

[Martin Wilck]

The previous patch changes the confdata output for dnsmasq. Adapt
the expected test results to match.

Signed-off-by: Martin Wilck 
---
 tests/networkxml2confdata/dhcp6-nat-network.conf  | 1 +
 tests/networkxml2confdata/dhcp6host-routed-network.conf   | 1 +
 tests/networkxml2confdata/isolated-network.conf   | 1 +
 tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf | 1 +
 tests/networkxml2confdata/nat-network-dns-srv-record.conf | 1 +
 tests/networkxml2confdata/nat-network-dns-txt-record.conf | 1 +
 tests/networkxml2confdata/nat-network-name-with-quotes.conf   | 1 +
 tests/networkxml2confdata/nat-network.conf| 1 +
 tests/networkxml2confdata/netboot-network.conf| 1 +
 tests/networkxml2confdata/netboot-proxy-network.conf  | 1 +
 10 files changed, 10 insertions(+)

diff --git a/tests/networkxml2confdata/dhcp6-nat-network.conf 
b/tests/networkxml2confdata/dhcp6-nat-network.conf
index 17076b8..d1058df 100644
--- a/tests/networkxml2confdata/dhcp6-nat-network.conf
+++ b/tests/networkxml2confdata/dhcp6-nat-network.conf
@@ -10,6 +10,7 @@ bind-dynamic
 interface=virbr0
 dhcp-range=192.168.122.2,192.168.122.254
 dhcp-no-override
+dhcp-authoritative
 dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64
 dhcp-lease-max=493
 dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile
diff --git a/tests/networkxml2confdata/dhcp6host-routed-network.conf 
b/tests/networkxml2confdata/dhcp6host-routed-network.conf
index 5728ee4..87a1498 100644
--- a/tests/networkxml2confdata/dhcp6host-routed-network.conf
+++ b/tests/networkxml2confdata/dhcp6host-routed-network.conf
@@ -10,6 +10,7 @@ bind-dynamic
 interface=virbr1
 dhcp-range=192.168.122.1,static
 dhcp-no-override
+dhcp-authoritative
 dhcp-range=2001:db8:ac10:fd01::1,static,64
 dhcp-hostsfile=/var/lib/libvirt/dnsmasq/local.hostsfile
 addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts
diff --git a/tests/networkxml2confdata/isolated-network.conf 
b/tests/networkxml2confdata/isolated-network.conf
index fbdf75a..ce4a59f 100644
--- a/tests/networkxml2confdata/isolated-network.conf
+++ b/tests/networkxml2confdata/isolated-network.conf
@@ -12,6 +12,7 @@ dhcp-option=3
 no-resolv
 dhcp-range=192.168.152.2,192.168.152.254
 dhcp-no-override
+dhcp-authoritative
 dhcp-lease-max=253
 dhcp-hostsfile=/var/lib/libvirt/dnsmasq/private.hostsfile
 addn-hosts=/var/lib/libvirt/dnsmasq/private.addnhosts
diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf 
b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
index 08ed672..f35ea1d 100644
--- a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
+++ b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
@@ -15,6 +15,7 @@ listen-address=10.24.10.1
 srv-host=_name._tcp
 dhcp-range=192.168.122.2,192.168.122.254
 dhcp-no-override
+dhcp-authoritative
 dhcp-lease-max=253
 dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile
 addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record.conf 
b/tests/networkxml2confdata/nat-network-dns-srv-record.conf
index d7de422..af1ed70 100644
--- a/tests/networkxml2confdata/nat-network-dns-srv-record.conf
+++ b/tests/networkxml2confdata/nat-network-dns-srv-record.conf
@@ -17,6 +17,7 @@ srv-host=_name6._tcp.test6.com,test6.example.com,,0,666
 srv-host=_name7._tcp.test7.com,test7.example.com,1,0,777
 dhcp-range=192.168.122.2,192.168.122.254
 dhcp-no-override
+dhcp-authoritative
 dhcp-lease-max=253
 dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile
 addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
diff --git a/tests/networkxml2confdata/nat-network-dns-txt-record.conf 
b/tests/networkxml2confdata/nat-network-dns-txt-record.conf
index 44ed6bd..7f560fb 100644
--- a/tests/networkxml2confdata/nat-network-dns-txt-record.conf
+++ b/tests/networkxml2confdata/nat-network-dns-txt-record.conf
@@ -11,6 +11,7 @@ interface=virbr0
 txt-record=example,example value
 dhcp-range=192.168.122.2,192.168.122.254
 dhcp-no-override
+dhcp-authoritative
 dhcp-lease-max=253
 dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile
 addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
diff --git a/tests/networkxml2confdata/nat-network-name-with-quotes.conf 
b/tests/networkxml2confdata/nat-network-name-with-quotes.conf
index a1c839e..36e11d1 100644
--- a/tests/networkxml2confdata/nat-network-name-with-quotes.conf
+++ b/tests/networkxml2confdata/nat-network-name-with-quotes.conf
@@ -15,6 +15,7 @@ listen-address=10.24.10.1
 srv-host=_name._tcp
 dhcp-range=192.168.122.2,192.168.122.254
 dhcp-no-override
+dhcp-authoritative
 dhcp-lease-max=253
 
dhcp-hostsfile=/var/lib/libvirt/dnsmasq/defaultwithquotes.hostsfile
 
addn-hosts=/var/lib/libvirt/dnsmasq/defaultwithquotes.addnhosts
diff --git a/tests/networkxml2confdata/nat-network.conf 

[libvirt] [PATCH 1/2] network: add dnsmasq option 'dhcp-authoritative'

[Martin Wilck]

The dnsmasq man page recommends that dhcp-authoritative "should be
set when dnsmasq is definitely the only DHCP server on a network".
This is the case for libvirt-managed virtual networks.

The effect of this is that VMs that fail to renew their DHCP lease
in time (e.g. if the VM or host is suspended) will be able to
re-acquire the lease even if it's expired, unless the IP address has
been taken by some other host. This avoids various annoyances caused
by changing VM IP addresses.

Signed-off-by: Martin Wilck 
---
 src/network/bridge_driver.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 7b99aca..cb4fb1c 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1258,8 +1258,10 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
 
 /* Note: the following is IPv4 only */
 if (VIR_SOCKET_ADDR_IS_FAMILY(>address, AF_INET)) {
-if (ipdef->nranges || ipdef->nhosts)
+if (ipdef->nranges || ipdef->nhosts) {
 virBufferAddLit(, "dhcp-no-override\n");
+virBufferAddLit(, "dhcp-authoritative\n");
+   }
 
 if (ipdef->tftproot) {
 virBufferAddLit(, "enable-tftp\n");
-- 
2.10.0

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

[libvirt] [PATCH 0/2] network: add dnsmasq option 'dhcp-authoritative'

[Martin Wilck]

Lest it be forgotten, here is a rebased version of my "dhcp-authoritative"
patch (against 77d24de). According to dnsmasq(8), this option "should 
be set when dnsmasq is definitely the only DHCP server on a network",
whis is the case for libvirt-managed networks.
In practice, this option has the effect that an expired lease can be
reacquired by the client using a DHCPREQUEST unless it has been given
to another client in the meantime. Without "dhcp-authoritative", this
operation always fails, which can be quite cumbersome.
There is no protection against rogue clients hijacking other client's
IP addresses, but that isn't specific to libvirt, and IP addresses don't
provide security anyway.
This is ovbiously not aimed at production environments; it's a convenience
for developers and casual users who'd rather not be bothered with network
XML host entries or the like.

Original submission:
https://www.redhat.com/archives/libvir-list/2016-September/msg00739.html

Daniel's post where he said that "unless there's a obvious downside to it,
it seems reasonable to add that":
https://www.redhat.com/archives/libvir-list/2016-September/msg01305.html

Regards,
Martin

Martin Wilck (2):
  network: add dnsmasq option 'dhcp-authoritative'
  tests/networkxml2confdata: add dhcp-authoritative option

 src/network/bridge_driver.c   | 4 +++-
 tests/networkxml2confdata/dhcp6-nat-network.conf  | 1 +
 tests/networkxml2confdata/dhcp6host-routed-network.conf   | 1 +
 tests/networkxml2confdata/isolated-network.conf   | 1 +
 tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf | 1 +
 tests/networkxml2confdata/nat-network-dns-srv-record.conf | 1 +
 tests/networkxml2confdata/nat-network-dns-txt-record.conf | 1 +
 tests/networkxml2confdata/nat-network-name-with-quotes.conf   | 1 +
 tests/networkxml2confdata/nat-network.conf| 1 +
 tests/networkxml2confdata/netboot-network.conf| 1 +
 tests/networkxml2confdata/netboot-proxy-network.conf  | 1 +
 11 files changed, 13 insertions(+), 1 deletion(-)

-- 
2.10.0

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

Re: [libvirt] libvirt-guest.sh bug fixes

[Eric Blake]

On 10/10/2016 11:48 AM, Stefan Bader wrote:

>> I did not hear about that before. But revisiting things again I think what
>> happened is that the Xen patch which I had done before (but at that time 
>> forgot
>> to submit upstream) was adding quotes there because without, the grep does 
>> not
>> work. Back then I did not realize this broke the shutdown as that also had
>> quotes around the list. All the other users of the list are ok because they 
>> wrap
>> the processing into for ... loops and for those newline or space does not 
>> matter.
> 
> I think the surprising part is that the calls to virsh like:
> 
>   list=$(virsh ... list)
> 
> seem to retain the newline seperator despite having no quotes. Only when using
> echo with the unquoted variable seems to do that. That is with dash as sh at 
> least.

That's true for ALL shells.  Word splitting does NOT happen during
variable assignment.  list=$() is _strictly_ equivalent to list="$()",
no matter the shell.

It's just that there are so many other places where $() and "$()" behave
differently that it's (usually) a good habit to always use "", rather
than learning the rules on when you NEED them, vs. telling the special
cases when you DON'T want them apart from the cases where they are optional.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed

[Andrea Bolognani]

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> Previously libvirt would only add pci-bridge devices automatically
> when an address was requested for a device that required a legacy PCI
> slot and none was available. This patch expands that support to
> dmi-to-pci-bridge (which is needed in order to add a pci-bridge on a
> machine with a pcie-root), and pcie-root-port (which is needed to add
> a hotpluggable PCIe device). It does *not* automatically add
> pcie-switch-upstream-ports or pcie-switch-downstream-ports (and
> currently there are no plans for that).
> 
> Given the existing code to auto-add pci-bridge devices, automatically
> adding pcie-root-ports is fairly straightforward. The
> dmi-to-pci-bridge support is a bit tricky though, for a few reasons:
> 
> 1) Although the only reason to add a dmi-to-pci-bridge is so that
>there is a reasonable place to plug in a pci-bridge controller,
>most of the time it's not the presence of a pci-bridge *in the
>config* that triggers the requirement to add a dmi-to-pci-bridge.
>Rather, it is the presence of a legacy-PCI device in the config,
>which triggers auto-add of a pci-bridge, which triggers auto-add of
>a dmi-to-pci-bridge (this is handled in
>virDomainPCIAddressSetGrow() - if there's a request to add a
>pci-bridge we'll check if there is a suitable bus to plug it into;
>if not, we first add a dmi-to-pci-bridge).
> 
> 2) Once there is already a single dmi-to-pci-bridge on the system,
>there won't be a need for any more, even if it's full, as long as
>there is a pci-bridge with an open slot - you can also plug
>pci-bridges into existing pci-bridges. So we have to make sure we
>don't add a dmi-to-pci-bridge unless there aren't any
>dmi-to-pci-bridges *or* any pci-bridges.
> 
> 3) Although it is strongly discouraged, it is legal for a pci-bridge
>to be directly plugged into pcie-root, and we don't want to
>auto-add a dmi-to-pci-bridge if there is already a pci-bridge
>that's been forced directly into pcie-root. Finally, although I
>fail to see the utility of it, it is legal to have something like
>this in the xml:
> 
>  
>  
> 
>and that will lead to an automatically added dmi-to-pci-bridge at
>index=1 (to give the unaddressed pci-bridge a proper place to plug
>in):
> 
>  
> 
>(for example, see the existing test case
>"usb-controller-default-q35"). This is handled in
>qemuDomainPCIAddressSetCreate() when it's adding in controllers to
>fill holes in the indexes.

I wonder how this "feature" came to be... It seems to be the
reason for quite a bit of convoluted code down below, which
we could avoid if this had never worked at all. As is so
often the case, too late for that :(

> Although libvirt will now automatically create a dmi-to-pci-bridge
> when it's needed, the code still remains for now that forces a
> dmi-to-pci-bridge on all domains with pcie-root (in
> qemuDomainDefAddDefaultDevices()). That will be removed in the next
> patch.
> 
> For now, the pcie-root-ports are added one to a slot, which is a bit
> wasteful and means it will fail after 31 total PCIe devices (30 if
> there are also some PCI devices), but helps keep the changeset down
> for this patch. A future patch will have 8 pcie-root-ports sharing the
> functions on a single slot.
> ---
>  src/conf/domain_addr.c |  88 ++--
>  src/qemu/qemu_domain_address.c |  91 ++---
>  .../qemuxml2argv-q35-pcie-autoadd.args |  57 
>  .../qemuxml2argv-q35-pcie-autoadd.xml  |  51 +++
>  tests/qemuxml2argvtest.c   |  23 
>  .../qemuxml2xmlout-q35-pcie-autoadd.xml| 147 
>+
>  tests/qemuxml2xmltest.c|  23 
>  7 files changed, 448 insertions(+), 32 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-pcie-autoadd.xml
>  create mode 100644 
>tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie-autoadd.xml
> 
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 12b5cb2..5f6f6f7 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -82,6 +82,30 @@ 
> virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
>  return 0;
>  }
>  
> +
> +static int

s/int/virDomainControllerModelPCI/

> +virDomainPCIControllerConnectTypeToModel(virDomainPCIConnectFlags flags)
> +{
> +if (flags & VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT)
> +return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
> +if (flags & VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)
> +return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT;
> +if (flags & VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT)
> +return 

Re: [libvirt] libvirt-guest.sh bug fixes

[Cole Robinson]

On 10/10/2016 12:32 PM, Stefan Bader wrote:
> On 10.10.2016 17:06, Cole Robinson wrote:
>> On 10/07/2016 03:56 AM, Stefan Bader wrote:
>>> Two small changes, before I forget about submitting them...
>>>
>>> First one affects all environments the same. The list of UIDs which
>>> is generated has each element on a separate line. And using quotes
>>> in the echo preserves those newlines. However the processing assumes
>>> one line per URI and all UIDs separated by spaces. So without dropping
>>> the quotes only one guest will get shutdown/suspended.
>>>
>>> The second change is for Xen environments only. Domain-0 appears in
>>> the list of guests and it is a persistent one. So on shutdown, the
>>> script tries to stop Domain-0 (which is not working) and then waits
>>> the whole timeout for it to stop.
>>>
>>
>> Note these patches were flagged as problematic a few months back:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1335585
>>
>> Stefan, have you heard about that issue? Was it resolved with these versions?
> 
> I did not hear about that before. But revisiting things again I think what
> happened is that the Xen patch which I had done before (but at that time 
> forgot
> to submit upstream) was adding quotes there because without, the grep does not
> work. Back then I did not realize this broke the shutdown as that also had
> quotes around the list. All the other users of the list are ok because they 
> wrap
> the processing into for ... loops and for those newline or space does not 
> matter.
> 
> So together with the other patch wich drops the quotes when writing the list
> file this should be resolved. list_guests returns a list of uids (separated by
> newline), the output of the info message is correctly showing names in one 
> line
> separated by komma, and the produced list file had only one line per uri with
> uids separated by spaces.

Sounds good, I'll close that bug then

Thanks,
Cole

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

Re: [libvirt] [PATCH v6 1/3] target-i386: Move warning code outside x86_cpu_filter_features()

[Eduardo Habkost]

On Mon, Oct 10, 2016 at 01:57:08PM +0200, Igor Mammedov wrote:
> On Fri,  7 Oct 2016 17:29:00 -0300
> Eduardo Habkost  wrote:
> 
> > x86_cpu_filter_features() will be reused by code that shouldn't
> > print any warning. Move the warning code to a new
> > x86_cpu_report_filtered_features() function, and call it from
> > x86_cpu_realizefn().
> > 
> > Signed-off-by: Eduardo Habkost 
> Reviewed-by: Igor Mammedov 

Thanks. Applied to x86-next.

-- 
Eduardo

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

Re: [libvirt] [PATCH v6 3/3] target-i386: Return runnability information on query-cpu-definitions

[Eduardo Habkost]

On Mon, Oct 10, 2016 at 02:27:49PM +0200, Igor Mammedov wrote:
> On Fri,  7 Oct 2016 17:29:02 -0300
> Eduardo Habkost  wrote:
> 
> > Fill the "unavailable-features" field on the x86 implementation
> > of query-cpu-definitions.
> > 
> > Cc: Jiri Denemark 
> > Cc: libvir-list@redhat.com
> > Signed-off-by: Eduardo Habkost 
> > ---
> > Changes v5 -> v6:
> > * Call x86_cpu_filter_features(), now that x86_cpu_load_features()
> >   won't run it automatically
> > 
> > Changes v4 -> v5:
> > * (none)
> > 
> > Changes v3 -> v4:
> > * Handle missing XSAVE components cleanly, but looking up
> >   the original feature that required it
> > * Use x86_cpu_load_features() function
> > 
> > Changes v2 -> v3:
> > * Create a x86_cpu_feature_name() function, to
> >   isolate the code that returns the property name
> > 
> > Changes v1 -> v2:
> > * Updated to the new schema: no @runnable field, and
> >   always report @unavailable-features as present
> > ---
> >  target-i386/cpu.c | 76 
> > +++
> >  1 file changed, 76 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 23cc19b..63330ce 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1945,6 +1945,27 @@ static inline void feat2prop(char *s)
> >  }
> >  }
> >  
> > +/* Return the feature property name for a feature flag bit */
> > +static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
> > +{
> > +/* XSAVE components are automatically enabled by other features,
> > + * so return the original feature name instead
> > + */
> > +if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) {
> > +int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr;
> > +
> > +if (comp < ARRAY_SIZE(x86_ext_save_areas) &&
> > +x86_ext_save_areas[comp].bits) {
> > +w = x86_ext_save_areas[comp].feature;
> > +bitnr = ctz32(x86_ext_save_areas[comp].bits);
> > +}
> > +}
> > +
> > +assert(bitnr < 32);
> > +assert(w < FEATURE_WORDS);
> > +return feature_word_info[w].feat_names[bitnr];
> > +}
> > +
> >  /* Compatibily hack to maintain legacy +-feat semantic,
> >   * where +-feat overwrites any feature set by
> >   * feat=on|feat even if the later is parsed after +-feat
> > @@ -2030,6 +2051,59 @@ static void x86_cpu_parse_featurestr(const char 
> > *typename, char *features,
> >  }
> >  }
> >  
> > +static void x86_cpu_load_features(X86CPU *cpu, Error **errp);
> > +static int x86_cpu_filter_features(X86CPU *cpu);
> > +
> > +/* Check for missing features that may prevent the CPU class from
> > + * running using the current machine and accelerator.
> > + */
> > +static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
> > + strList **missing_feats)
> > +{
> > +X86CPU *xc;
> > +FeatureWord w;
> > +Error *err = NULL;
> > +strList **next = missing_feats;
> > +
> > +if (xcc->kvm_required && !kvm_enabled()) {
> > +strList *new = g_new0(strList, 1);
> > +new->value = g_strdup("kvm");;
> > +*missing_feats = new;
> > +return;
> > +}
> > +
> > +xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
> > +
> > +x86_cpu_load_features(xc, );
> > +if (err) {
> > +/* Errors at x86_cpu_load_features should never happen,
> > + * but in case it does, just report the model as not
> > + * runnable at all using the "type" property.
> > + */
> > +strList *new = g_new0(strList, 1);
> > +new->value = g_strdup("type");
> > +*next = new;
> > +next = >next;
> > +}
> > +
> > +x86_cpu_filter_features(xc);
> > +
> > +for (w = 0; w < FEATURE_WORDS; w++) {
> > +uint32_t filtered = xc->filtered_features[w];
> > +int i;
> > +for (i = 0; i < 32; i++) {
> > +if (filtered & (1UL << i)) {
> > +strList *new = g_new0(strList, 1);
> > +new->value = g_strdup(x86_cpu_feature_name(w, i));
> > +*next = new;
> > +next = >next;
> > +}
> > +}
> > +}
> Shouldn't you add 
>if (IS_AMD_CPU(env)) { 
> fixup here, that realize does right after calling x86_cpu_filter_features()

What would it be useful for? The IS_AMD_CPU fixup runs after
x86_cpu_filter_features() (so it doesn't affect filtered_features
at all), and filtered_features is the only field used as input to
build missing_feats.

-- 
Eduardo

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

[libvirt] [PATCH] fixup! target-i386: x86_cpu_load_features() function

[Eduardo Habkost]

On Mon, Oct 10, 2016 at 02:25:32PM +0200, Igor Mammedov wrote:
> On Fri,  7 Oct 2016 17:29:01 -0300
> Eduardo Habkost  wrote:
> 
> > When probing for CPU model information, we need to reuse the code
> > that initializes CPUID fields, but not the remaining side-effects
> > of x86_cpu_realizefn(). Move that code to a separate function
> > that can be reused later.
> > 
> > Signed-off-by: Eduardo Habkost 
> > ---
> > Changes v5 -> v6:
> > * Move x86_cpu_filter_features() outside x86_cpu_load_features(),
> >   as the CPU model querying API won't run
> >   x86_cpu_filter_features() on most cases
> 
> > 
> > Changes v4 -> v5:
> > * Fix typo on x86_cpu_load_features() comment
> >   Reported-by: Paolo Bonzini 
> > 
> > Changes series v3 -> v4:
> > * New patch added to series
> > ---
> >  target-i386/cpu.c | 67 
> > +++
> >  1 file changed, 43 insertions(+), 24 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 1e8127b..23cc19b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2993,34 +2993,13 @@ static void x86_cpu_enable_xsave_components(X86CPU 
> > *cpu)
> >  env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
> >  }
> >  
> > -#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && 
> > \
> > -   (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && 
> > \
> > -   (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
> > -#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
> > - (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
> > - (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> > -static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> > +/* Load CPUID data based on configured features */
> > +static void x86_cpu_load_features(X86CPU *cpu, Error **errp)
> >  {
> > -CPUState *cs = CPU(dev);
> > -X86CPU *cpu = X86_CPU(dev);
> > -X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> >  CPUX86State *env = >env;
> > -Error *local_err = NULL;
> > -static bool ht_warned;
> >  FeatureWord w;
> >  GList *l;
> > -
> > -if (xcc->kvm_required && !kvm_enabled()) {
> > -char *name = x86_cpu_class_get_model_name(xcc);
> > -error_setg(_err, "CPU model '%s' requires KVM", name);
> > -g_free(name);
> > -goto out;
> > -}
> > -
> > -if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> > -error_setg(errp, "apic-id property was not initialized properly");
> > -return;
> > -}
> > +Error *local_err = NULL;
> >  
> >  /*TODO: cpu->host_features incorrectly overwrites features
> >   * set using "feat=on|off". Once we fix this, we can convert
> > @@ -3086,6 +3065,46 @@ static void x86_cpu_realizefn(DeviceState *dev, 
> > Error **errp)
> >  env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
> >  }
> >  
> > +out:
> > +if (local_err != NULL) {
> > +error_propagate(errp, local_err);
> > +}
> > +}
> > +
> > +#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && 
> > \
> > +   (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && 
> > \
> > +   (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
> > +#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
> > + (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
> > + (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> > +static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> > +{
> > +CPUState *cs = CPU(dev);
> > +X86CPU *cpu = X86_CPU(dev);
> > +X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> > +CPUX86State *env = >env;
> > +Error *local_err = NULL;
> > +static bool ht_warned;
> > +
> > +if (xcc->kvm_required && !kvm_enabled()) {
> > +char *name = x86_cpu_class_get_model_name(xcc);
> > +error_setg(_err, "CPU model '%s' requires KVM", name);
> > +g_free(name);
> > +goto out;
> > +}
> > +
> > +if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> > +error_setg(errp, "apic-id property was not initialized properly");
> > +return;
> > +}
> > +
> > +x86_cpu_load_features(cpu, _err);
> > +if (local_err) {
> > +goto out;
> > +}
> > +
> > +x86_cpu_filter_features(cpu);
> that makes 2 invocations of ^^ inside realize,
> see followup line 

Oops, leftover from v5. Thanks for spotting that! Fixup below.

Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 23cc19b..4294746 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3103,8 +3103,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
**errp)
 goto out;
 }
 
-

Re: [libvirt] libvirt-guest.sh bug fixes

[Stefan Bader]

On 10.10.2016 18:32, Stefan Bader wrote:
> On 10.10.2016 17:06, Cole Robinson wrote:
>> On 10/07/2016 03:56 AM, Stefan Bader wrote:
>>> Two small changes, before I forget about submitting them...
>>>
>>> First one affects all environments the same. The list of UIDs which
>>> is generated has each element on a separate line. And using quotes
>>> in the echo preserves those newlines. However the processing assumes
>>> one line per URI and all UIDs separated by spaces. So without dropping
>>> the quotes only one guest will get shutdown/suspended.
>>>
>>> The second change is for Xen environments only. Domain-0 appears in
>>> the list of guests and it is a persistent one. So on shutdown, the
>>> script tries to stop Domain-0 (which is not working) and then waits
>>> the whole timeout for it to stop.
>>>
>>
>> Note these patches were flagged as problematic a few months back:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1335585
>>
>> Stefan, have you heard about that issue? Was it resolved with these versions?
> 
> I did not hear about that before. But revisiting things again I think what
> happened is that the Xen patch which I had done before (but at that time 
> forgot
> to submit upstream) was adding quotes there because without, the grep does not
> work. Back then I did not realize this broke the shutdown as that also had
> quotes around the list. All the other users of the list are ok because they 
> wrap
> the processing into for ... loops and for those newline or space does not 
> matter.

I think the surprising part is that the calls to virsh like:

  list=$(virsh ... list)

seem to retain the newline seperator despite having no quotes. Only when using
echo with the unquoted variable seems to do that. That is with dash as sh at 
least.

> 
> So together with the other patch wich drops the quotes when writing the list
> file this should be resolved. list_guests returns a list of uids (separated by
> newline), the output of the info message is correctly showing names in one 
> line
> separated by komma, and the produced list file had only one line per uri with
> uids separated by spaces.
> 
> -Stefan
>>
>> Thanks,
>> Cole
>>
> 
> 




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

Re: [libvirt] libvirt-guest.sh bug fixes

[Stefan Bader]

On 10.10.2016 17:06, Cole Robinson wrote:
> On 10/07/2016 03:56 AM, Stefan Bader wrote:
>> Two small changes, before I forget about submitting them...
>>
>> First one affects all environments the same. The list of UIDs which
>> is generated has each element on a separate line. And using quotes
>> in the echo preserves those newlines. However the processing assumes
>> one line per URI and all UIDs separated by spaces. So without dropping
>> the quotes only one guest will get shutdown/suspended.
>>
>> The second change is for Xen environments only. Domain-0 appears in
>> the list of guests and it is a persistent one. So on shutdown, the
>> script tries to stop Domain-0 (which is not working) and then waits
>> the whole timeout for it to stop.
>>
> 
> Note these patches were flagged as problematic a few months back:
> https://bugzilla.redhat.com/show_bug.cgi?id=1335585
> 
> Stefan, have you heard about that issue? Was it resolved with these versions?

I did not hear about that before. But revisiting things again I think what
happened is that the Xen patch which I had done before (but at that time forgot
to submit upstream) was adding quotes there because without, the grep does not
work. Back then I did not realize this broke the shutdown as that also had
quotes around the list. All the other users of the list are ok because they wrap
the processing into for ... loops and for those newline or space does not 
matter.

So together with the other patch wich drops the quotes when writing the list
file this should be resolved. list_guests returns a list of uids (separated by
newline), the output of the info message is correctly showing names in one line
separated by komma, and the produced list file had only one line per uri with
uids separated by spaces.

-Stefan
> 
> Thanks,
> Cole
> 




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

Re: [libvirt] [PATCH 5/7] vsh: Fix some issues in auto completion code

[Peter Krempa]

On Mon, Oct 10, 2016 at 11:42:16 -0400, John Ferlan wrote:
> 1. Move the declaration of const vshCmdDef *help - it should be at the
>top of the "if" rather than in the middle.
> 
> 2. Change a comparison from && to || - without doing so we could core

Same comment as in 3/7 on 'core' not being the appropriate term.

>on commands like 'virsh list' which would allow completion of some
>non -- option based on whatever was found in the current working
>directory and then as soon as that was completed, the next 
>would core since "opt" would be returned as NULL, but the check

aaand again.

>was dereferencing "&& opt->type"
> 
> 3. Before dereferencing opt->completer, be sure opt isn't NULL.
> 
> Signed-off-by: John Ferlan 
> ---
>  tools/vsh.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/tools/vsh.c b/tools/vsh.c
> index 420eb79..9558dad 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -1523,10 +1523,11 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
> *parser)
>  /* if we encountered --help, replace parsed command with
>   * 'help ' */
>  for (tmpopt = first; tmpopt; tmpopt = tmpopt->next) {
> +const vshCmdDef *help;
>  if (STRNEQ(tmpopt->def->name, "help"))
>  continue;
>  
> -const vshCmdDef *help = vshCmddefSearch("help");
> +help = vshCmddefSearch("help");
>  vshCommandOptFree(first);
>  first = vshMalloc(ctl, sizeof(vshCmdOpt));
>  first->def = help->opts;
> @@ -2788,7 +2789,7 @@ vshReadlineParse(const char *text, int state)
>   * Try to find the default option.
>   */
>  if (!(opt = vshCmddefGetData(cmd, _need_arg, 
> _seen))
> -&& opt->type == VSH_OT_BOOL)
> +|| opt->type == VSH_OT_BOOL)
>  goto error;
>  opt_exists = true;
>  opts_need_arg = const_opts_need_arg;
> @@ -2824,7 +2825,7 @@ vshReadlineParse(const char *text, int state)
>  res = vshReadlineCommandGenerator(sanitized_text, state);
>  } else if (opts_filled && !non_bool_opt_exists) {
>  res = vshReadlineOptionsGenerator(sanitized_text, state, cmd);
> -} else if (non_bool_opt_exists && data_complete && opt->completer) {
> +} else if (non_bool_opt_exists && data_complete && opt && 
> opt->completer) {
>  if (!completed_list)
>  completed_list = opt->completer(autoCompleteOpaque,
>  opt->completer_flags);

This function makes my eyes bleed.

ACK


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/7] conf: Remove incorrect check when encoding shmem audit message

[Peter Krempa]

On Mon, Oct 10, 2016 at 11:42:12 -0400, John Ferlan wrote:
> Remove the !size check since size is initialized to NULL and thus
> causing the condition to always be true
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/domain_audit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

ACK


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

Re: [libvirt] [PATCH 2/7] tests: Prefer virGetLastErrorMessage in testSELinuxLabeling

[Peter Krempa]

On Mon, Oct 10, 2016 at 11:42:13 -0400, John Ferlan wrote:
> Yet another case of not needing virGetLastError processing
> 
> Signed-off-by: John Ferlan 
> ---
>  tests/securityselinuxlabeltest.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)

ACK


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

Re: [libvirt] [PATCH 3/7] qemu: Remove possibility of NULL dereference

[Peter Krempa]

On Mon, Oct 10, 2016 at 11:42:14 -0400, John Ferlan wrote:
> If qemubinCaps is NULL, then calling virQEMUCapsGetMachineTypesCaps and
  ^
This symbol name refers to the name in the caller. The function uses a
different name.

> dereferencing to get the nmachineTypes will cause a core. Rework the code

It will cause a crash, not a core. A core dump is caused by the crash
and can be turned off.

> slightly to avoid the issue and return immediately if !qemubinCaps or
> !nmachineTypes
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_capabilities.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index da8f3d1..ee3e50f 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2405,10 +2405,13 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr 
> qemuCaps,
>  size_t i;
>  
>  *machines = NULL;
> +*nmachines = 0;
> +
> +if (!qemuCaps || !qemuCaps->nmachineTypes)
> +return 0;
>  *nmachines = qemuCaps->nmachineTypes;
>  
> -if (*nmachines &&
> -VIR_ALLOC_N(*machines, qemuCaps->nmachineTypes) < 0)
> +if (VIR_ALLOC_N(*machines, qemuCaps->nmachineTypes) < 0)
>  goto error;

This is a false positive, the caller checks that 'binary' is set and
does not call this function at all. Coverity probably doesn't see it as
it depends on the state of 'binary' rather than 'qemubinCaps' in the
caller.

As of such I disagree with this fix. The only acceptable way is to make
the caller better introspectable. This fix obscures what is actually
happening.

Peter






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

Re: [libvirt] [PATCH 7/7] tests: Need to initialize data

[Peter Krempa]

On Mon, Oct 10, 2016 at 11:42:18 -0400, John Ferlan wrote:
> If not initialized and the virAsprintf to jsonreply or fulllablel fails,
> then the call to qemuMonitorTestFree will take stack data.
> 
> Signed-off-by: John Ferlan 
> ---
>  tests/qemumonitorjsontest.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

ACK


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/7] util: Resolve memory leaks in virLogParse{Output|Filter}

[Peter Krempa]

On Mon, Oct 10, 2016 at 11:42:15 -0400, John Ferlan wrote:
> In both virLogParseOutput and virLogParseFilter, rather than returning
> NULL, goto cleanup since it's possible that for each the first condition
> passes, but the || condition doesn't and thus we leak memory.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/util/virlog.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

ACK


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/7] util: Remove need for local 'nelems'

[Peter Krempa]

On Mon, Oct 10, 2016 at 11:42:17 -0400, John Ferlan wrote:
> Since it's only used in loop - just go direct.

Not sure how deep coverity sees stuff, but in some cases this would
increase the complexity of the loop from O(n) to O(n^2) since the
function gets evaluated on every single loop, but the returned value
never changes.

In this case it's okay, since virJSONValueArraySize does not do any
iteration over the list.

> 
> Signed-off-by: John Ferlan 
> ---

ACK in this case, but we need to be careful.


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

[libvirt] [PATCH 3/4] qemu: command: escape smbios entry strings

[Peter Krempa]

We pass free-form strings from the users to qemu, thus we need escape
commas since they are passed to qemu monitor.

Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535
---
 src/qemu/qemu_command.c | 102 +++-
 1 file changed, 66 insertions(+), 36 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 11ae363..fffeeef 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5705,17 +5705,25 @@ qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def)
 virBufferAddLit(, "type=0");

 /* 0:Vendor */
-if (def->vendor)
-virBufferAsprintf(, ",vendor=%s", def->vendor);
+if (def->vendor) {
+virBufferAddLit(, ",vendor=");
+virQEMUBuildBufferEscapeComma(, def->vendor);
+}
 /* 0:BIOS Version */
-if (def->version)
-virBufferAsprintf(, ",version=%s", def->version);
+if (def->version) {
+virBufferAddLit(, ",version=");
+virQEMUBuildBufferEscapeComma(, def->version);
+}
 /* 0:BIOS Release Date */
-if (def->date)
-virBufferAsprintf(, ",date=%s", def->date);
+if (def->date) {
+virBufferAddLit(, ",date=");
+virQEMUBuildBufferEscapeComma(, def->date);
+}
 /* 0:System BIOS Major Release and 0:System BIOS Minor Release */
-if (def->release)
-virBufferAsprintf(, ",release=%s", def->release);
+if (def->release) {
+virBufferAddLit(, ",release=");
+virQEMUBuildBufferEscapeComma(, def->release);
+}

 return virBufferContentAndReset();
 }
@@ -5736,27 +5744,40 @@ qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def,
 virBufferAddLit(, "type=1");

 /* 1:Manufacturer */
-if (def->manufacturer)
-virBufferAsprintf(, ",manufacturer=%s",
-  def->manufacturer);
+if (def->manufacturer) {
+virBufferAddLit(, ",manufacturer=");
+virQEMUBuildBufferEscapeComma(, def->manufacturer);
+}
  /* 1:Product Name */
-if (def->product)
-virBufferAsprintf(, ",product=%s", def->product);
+if (def->product) {
+virBufferAddLit(, ",product=");
+virQEMUBuildBufferEscapeComma(, def->product);
+}
 /* 1:Version */
-if (def->version)
-virBufferAsprintf(, ",version=%s", def->version);
+if (def->version) {
+virBufferAddLit(, ",version=");
+virQEMUBuildBufferEscapeComma(, def->version);
+}
 /* 1:Serial Number */
-if (def->serial)
-virBufferAsprintf(, ",serial=%s", def->serial);
+if (def->serial) {
+virBufferAddLit(, ",serial=");
+virQEMUBuildBufferEscapeComma(, def->serial);
+}
 /* 1:UUID */
-if (def->uuid && !skip_uuid)
-virBufferAsprintf(, ",uuid=%s", def->uuid);
+if (def->uuid && !skip_uuid) {
+virBufferAddLit(, ",uuid=");
+virQEMUBuildBufferEscapeComma(, def->uuid);
+}
 /* 1:SKU Number */
-if (def->sku)
-virBufferAsprintf(, ",sku=%s", def->sku);
+if (def->sku) {
+virBufferAddLit(, ",sku=");
+virQEMUBuildBufferEscapeComma(, def->sku);
+}
 /* 1:Family */
-if (def->family)
-virBufferAsprintf(, ",family=%s", def->family);
+if (def->family) {
+virBufferAddLit(, ",family=");
+virQEMUBuildBufferEscapeComma(, def->family);
+}

 return virBufferContentAndReset();
 }
@@ -5773,24 +5794,33 @@ qemuBuildSmbiosBaseBoardStr(virSysinfoBaseBoardDefPtr 
def)
 virBufferAddLit(, "type=2");

 /* 2:Manufacturer */
-if (def->manufacturer)
-virBufferAsprintf(, ",manufacturer=%s",
-  def->manufacturer);
+virBufferAddLit(, ",manufacturer=");
+virQEMUBuildBufferEscapeComma(, def->manufacturer);
 /* 2:Product Name */
-if (def->product)
-virBufferAsprintf(, ",product=%s", def->product);
+if (def->product) {
+virBufferAddLit(, ",product=");
+virQEMUBuildBufferEscapeComma(, def->product);
+}
 /* 2:Version */
-if (def->version)
-virBufferAsprintf(, ",version=%s", def->version);
+if (def->version) {
+virBufferAddLit(, ",version=");
+virQEMUBuildBufferEscapeComma(, def->version);
+}
 /* 2:Serial Number */
-if (def->serial)
-virBufferAsprintf(, ",serial=%s", def->serial);
+if (def->serial) {
+virBufferAddLit(, ",serial=");
+virQEMUBuildBufferEscapeComma(, def->serial);
+}
 /* 2:Asset Tag */
-if (def->asset)
-virBufferAsprintf(, ",asset=%s", def->asset);
+if (def->asset) {
+virBufferAddLit(, ",asset=");
+virQEMUBuildBufferEscapeComma(, def->asset);
+}
 /* 2:Location */
-if (def->location)
-virBufferAsprintf(, ",location=%s", def->location);
+if (def->location) {
+virBufferAddLit(, ",location=");
+virQEMUBuildBufferEscapeComma(, def->location);
+}

 if (virBufferCheckError() < 0)
   

[libvirt] [PATCH 0/4] qemu: escape smbios strings passed on commandline

[Peter Krempa]

See patch 3 for the core change.

Peter Krempa (4):
  qemu: command: Fix up coding style of smbios commandine formatters
  qemu: command: Don't bother reporting errors in smbios formatters
  qemu: command: escape smbios entry strings
  schema: smbios: allow any strings

 docs/schemas/domaincommon.rng |   4 +-
 src/qemu/qemu_command.c   | 129 --
 2 files changed, 76 insertions(+), 57 deletions(-)

-- 
2.10.0

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

[libvirt] [PATCH 4/4] schema: smbios: allow any strings

[Peter Krempa]

The smbios docs allow any string to be passed and libvirt does not
really do any validation on them. Allow passing any string.

Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535
---
 docs/schemas/domaincommon.rng | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 6eeb4e9..f1609f9 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4517,9 +4517,7 @@
   

   
-
-  [a-zA-Z0-9/\-_\. \(\)]+
-
+
   

   
-- 
2.10.0

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

[libvirt] [PATCH 1/4] qemu: command: Fix up coding style of smbios commandine formatters

[Peter Krempa]

---
 src/qemu/qemu_command.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0804133..13c2cde 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5694,7 +5694,8 @@ qemuBuildRNGCommandLine(virLogManagerPtr logManager,
 }


-static char *qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def)
+static char *
+qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def)
 {
 virBuffer buf = VIR_BUFFER_INITIALIZER;

@@ -5726,8 +5727,10 @@ static char *qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr 
def)
 return NULL;
 }

-static char *qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def,
-  bool skip_uuid)
+
+static char *
+qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def,
+ bool skip_uuid)
 {
 virBuffer buf = VIR_BUFFER_INITIALIZER;

@@ -5772,7 +5775,9 @@ static char 
*qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def,
 return NULL;
 }

-static char *qemuBuildSmbiosBaseBoardStr(virSysinfoBaseBoardDefPtr def)
+
+static char *
+qemuBuildSmbiosBaseBoardStr(virSysinfoBaseBoardDefPtr def)
 {
 virBuffer buf = VIR_BUFFER_INITIALIZER;

-- 
2.10.0

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

[libvirt] [PATCH 2/4] qemu: command: Don't bother reporting errors in smbios formatters

[Peter Krempa]

qemuBuildSmbiosBiosStr and qemuBuildSmbiosSystemStr return NULL if
there's noting to format on the commandline. Reporting errors from
buffer creation doesn't make sense since it would be ignored.
---
 src/qemu/qemu_command.c | 14 --
 1 file changed, 14 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 13c2cde..11ae363 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5717,14 +5717,7 @@ qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def)
 if (def->release)
 virBufferAsprintf(, ",release=%s", def->release);

-if (virBufferCheckError() < 0)
-goto error;
-
 return virBufferContentAndReset();
-
- error:
-virBufferFreeAndReset();
-return NULL;
 }


@@ -5765,14 +5758,7 @@ qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def,
 if (def->family)
 virBufferAsprintf(, ",family=%s", def->family);

-if (virBufferCheckError() < 0)
-goto error;
-
 return virBufferContentAndReset();
-
- error:
-virBufferFreeAndReset();
-return NULL;
 }


-- 
2.10.0

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

[libvirt] [PATCH 5/7] vsh: Fix some issues in auto completion code

[John Ferlan]

1. Move the declaration of const vshCmdDef *help - it should be at the
   top of the "if" rather than in the middle.

2. Change a comparison from && to || - without doing so we could core
   on commands like 'virsh list' which would allow completion of some
   non -- option based on whatever was found in the current working
   directory and then as soon as that was completed, the next 
   would core since "opt" would be returned as NULL, but the check
   was dereferencing "&& opt->type"

3. Before dereferencing opt->completer, be sure opt isn't NULL.

Signed-off-by: John Ferlan 
---
 tools/vsh.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index 420eb79..9558dad 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -1523,10 +1523,11 @@ vshCommandParse(vshControl *ctl, vshCommandParser 
*parser)
 /* if we encountered --help, replace parsed command with
  * 'help ' */
 for (tmpopt = first; tmpopt; tmpopt = tmpopt->next) {
+const vshCmdDef *help;
 if (STRNEQ(tmpopt->def->name, "help"))
 continue;
 
-const vshCmdDef *help = vshCmddefSearch("help");
+help = vshCmddefSearch("help");
 vshCommandOptFree(first);
 first = vshMalloc(ctl, sizeof(vshCmdOpt));
 first->def = help->opts;
@@ -2788,7 +2789,7 @@ vshReadlineParse(const char *text, int state)
  * Try to find the default option.
  */
 if (!(opt = vshCmddefGetData(cmd, _need_arg, _seen))
-&& opt->type == VSH_OT_BOOL)
+|| opt->type == VSH_OT_BOOL)
 goto error;
 opt_exists = true;
 opts_need_arg = const_opts_need_arg;
@@ -2824,7 +2825,7 @@ vshReadlineParse(const char *text, int state)
 res = vshReadlineCommandGenerator(sanitized_text, state);
 } else if (opts_filled && !non_bool_opt_exists) {
 res = vshReadlineOptionsGenerator(sanitized_text, state, cmd);
-} else if (non_bool_opt_exists && data_complete && opt->completer) {
+} else if (non_bool_opt_exists && data_complete && opt && opt->completer) {
 if (!completed_list)
 completed_list = opt->completer(autoCompleteOpaque,
 opt->completer_flags);
-- 
2.7.4

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

[libvirt] [PATCH 4/7] util: Resolve memory leaks in virLogParse{Output|Filter}

[John Ferlan]

In both virLogParseOutput and virLogParseFilter, rather than returning
NULL, goto cleanup since it's possible that for each the first condition
passes, but the || condition doesn't and thus we leak memory.

Signed-off-by: John Ferlan 
---
 src/util/virlog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/virlog.c b/src/util/virlog.c
index 14ee701..52b0eea 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -1466,7 +1466,7 @@ virLogParseOutput(const char *src)
 if (!(tokens = virStringSplitCount(src, ":", 0, )) || count < 2) {
 virReportError(VIR_ERR_INVALID_ARG,
_("Malformed format for output '%s'"), src);
-return NULL;
+goto cleanup;
 }
 
 if (virStrToLong_uip(tokens[0], NULL, 10, ) < 0 ||
@@ -1575,7 +1575,7 @@ virLogParseFilter(const char *src)
 if (!(tokens = virStringSplitCount(src, ":", 0, )) || count != 2) {
 virReportError(VIR_ERR_INVALID_ARG,
_("Malformed format for filter '%s'"), src);
-return NULL;
+goto cleanup;
 }
 
 if (virStrToLong_uip(tokens[0], NULL, 10, ) < 0 ||
-- 
2.7.4

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

[libvirt] [PATCH 2/7] tests: Prefer virGetLastErrorMessage in testSELinuxLabeling

[John Ferlan]

Yet another case of not needing virGetLastError processing

Signed-off-by: John Ferlan 
---
 tests/securityselinuxlabeltest.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
index e98d9bb..1178acf 100644
--- a/tests/securityselinuxlabeltest.c
+++ b/tests/securityselinuxlabeltest.c
@@ -331,10 +331,8 @@ testSELinuxLabeling(const void *opaque)
 VIR_FREE(files[i].context);
 }
 VIR_FREE(files);
-if (ret < 0) {
-virErrorPtr err = virGetLastError();
-VIR_TEST_VERBOSE("%s\n", err ? err->message : "");
-}
+if (ret < 0)
+VIR_TEST_VERBOSE("%s\n", virGetLastErrorMessage());
 return ret;
 }
 
-- 
2.7.4

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

[libvirt] [PATCH 1/7] conf: Remove incorrect check when encoding shmem audit message

[John Ferlan]

Remove the !size check since size is initialized to NULL and thus
causing the condition to always be true

Signed-off-by: John Ferlan 
---
 src/conf/domain_audit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index fd20ace..2423f34 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -983,7 +983,7 @@ virDomainAuditShmem(virDomainObjPtr vm,
 
 virUUIDFormat(vm->def->uuid, uuidstr);
 
-if (!vmname || !src || !size || !shmem ||
+if (!vmname || !src || !shmem ||
 virAsprintfQuiet(, "%llu", def->size) < 0) {
 VIR_WARN("OOM while encoding audit message");
 goto cleanup;
@@ -997,7 +997,7 @@ virDomainAuditShmem(virDomainObjPtr vm,
 
 VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success,
   "virt=%s resrc=shmem reason=%s %s uuid=%s size=%s %s %s",
-  virt, reason, vmname, uuidstr, size ?: "?", shmem, src);
+  virt, reason, vmname, uuidstr, size, shmem, src);
 
  cleanup:
 VIR_FREE(vmname);
-- 
2.7.4

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

[libvirt] [PATCH 6/7] util: Remove need for local 'nelems'

[John Ferlan]

Since it's only used in loop - just go direct.

Signed-off-by: John Ferlan 
---
 src/util/virqemu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/util/virqemu.c b/src/util/virqemu.c
index 0b516fc..7849d8b 100644
--- a/src/util/virqemu.c
+++ b/src/util/virqemu.c
@@ -85,12 +85,11 @@ virQEMUBuildCommandLineJSONArrayNumbered(const char *key,
  virBufferPtr buf)
 {
 const virJSONValue *member;
-ssize_t nelems = virJSONValueArraySize(array);
 char *prefix = NULL;
 size_t i;
 int ret = 0;
 
-for (i = 0; i < nelems; i++) {
+for (i = 0; i < virJSONValueArraySize(array); i++) {
 member = virJSONValueArrayGet((virJSONValuePtr) array, i);
 
 if (virAsprintf(, "%s.%zu", key, i) < 0)
-- 
2.7.4

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

[libvirt] [PATCH 7/7] tests: Need to initialize data

[John Ferlan]

If not initialized and the virAsprintf to jsonreply or fulllablel fails,
then the call to qemuMonitorTestFree will take stack data.

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

diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 0574f8c..993a373 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -753,7 +753,7 @@ qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr 
xmlopt,
 bool fail)
 
 {
-struct qemuMonitorJSONTestAttachChardevData data;
+struct qemuMonitorJSONTestAttachChardevData data = {0};
 char *jsonreply = NULL;
 char *fulllabel = NULL;
 int ret = -1;
-- 
2.7.4

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

[libvirt] [PATCH 0/7] Fixes to various different issues

[John Ferlan]

All found by Coverity

Not being so diligent lately... There are a few more, but they're all
false positives so I'll keep those local.

John Ferlan (7):
  conf: Remove incorrect check when encoding shmem audit message
  tests: Prefer virGetLastErrorMessage in testSELinuxLabeling
  qemu: Remove possibility of NULL dereference
  util: Resolve memory leaks in virLogParse{Output|Filter}
  vsh: Fix some issues in auto completion code
  util: Remove need for local 'nelems'
  tests: Need to initialize data

 src/conf/domain_audit.c  | 4 ++--
 src/qemu/qemu_capabilities.c | 7 +--
 src/util/virlog.c| 4 ++--
 src/util/virqemu.c   | 3 +--
 tests/qemumonitorjsontest.c  | 2 +-
 tests/securityselinuxlabeltest.c | 6 ++
 tools/vsh.c  | 7 ---
 7 files changed, 17 insertions(+), 16 deletions(-)

-- 
2.7.4

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

[libvirt] [PATCH 3/7] qemu: Remove possibility of NULL dereference

[John Ferlan]

If qemubinCaps is NULL, then calling virQEMUCapsGetMachineTypesCaps and
dereferencing to get the nmachineTypes will cause a core. Rework the code
slightly to avoid the issue and return immediately if !qemubinCaps or
!nmachineTypes

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_capabilities.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index da8f3d1..ee3e50f 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2405,10 +2405,13 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr 
qemuCaps,
 size_t i;
 
 *machines = NULL;
+*nmachines = 0;
+
+if (!qemuCaps || !qemuCaps->nmachineTypes)
+return 0;
 *nmachines = qemuCaps->nmachineTypes;
 
-if (*nmachines &&
-VIR_ALLOC_N(*machines, qemuCaps->nmachineTypes) < 0)
+if (VIR_ALLOC_N(*machines, qemuCaps->nmachineTypes) < 0)
 goto error;
 
 for (i = 0; i < qemuCaps->nmachineTypes; i++) {
-- 
2.7.4

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

Re: [libvirt] libvirt-guest.sh bug fixes

[Cole Robinson]

On 10/07/2016 03:56 AM, Stefan Bader wrote:
> Two small changes, before I forget about submitting them...
> 
> First one affects all environments the same. The list of UIDs which
> is generated has each element on a separate line. And using quotes
> in the echo preserves those newlines. However the processing assumes
> one line per URI and all UIDs separated by spaces. So without dropping
> the quotes only one guest will get shutdown/suspended.
> 
> The second change is for Xen environments only. Domain-0 appears in
> the list of guests and it is a persistent one. So on shutdown, the
> script tries to stop Domain-0 (which is not working) and then waits
> the whole timeout for it to stop.
> 

Note these patches were flagged as problematic a few months back:
https://bugzilla.redhat.com/show_bug.cgi?id=1335585

Stefan, have you heard about that issue? Was it resolved with these versions?

Thanks,
Cole

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

[libvirt] [PATCH 1/2] virsh: Fix commas in manpage to enhance readability.

[Nitesh Konkar]

Signed-off-by: Nitesh Konkar 
---
 tools/virsh.pod | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/virsh.pod b/tools/virsh.pod
index f38aacf..72ace96 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -909,7 +909,7 @@ I<--balloon> returns:
 "balloon.rss" - Resident Set Size of running domain's process (in kB),
 "balloon.usable" - the amount of memory which can be reclaimed by balloon
 without causing host swapping (in KB),
-"balloon.last-update" - timestamp of the last update of statistics (in 
seconds),
+"balloon.last-update" - timestamp of the last update of statistics (in seconds)
 
 I<--vcpu> returns:
 "vcpu.current" - current number of online virtual CPUs,
@@ -931,12 +931,12 @@ I<--interface> returns:
 "net..tx.drop" - number of transmit packets dropped
 
 I<--perf> returns the statistics of all enabled perf events:
-"perf.cmt" - the cache usage in Byte currently used
-"perf.mbmt" - total system bandwidth from one level of cache
-"perf.mbml" - bandwidth of memory traffic for a memory controller
-"perf.cpu_cycles" - the count of cpu cycles (total/elapsed)
-"perf.instructions" - the count of instructions
-"perf.cache_references" - the count of cache hits
+"perf.cmt" - the cache usage in Byte currently used,
+"perf.mbmt" - total system bandwidth from one level of cache,
+"perf.mbml" - bandwidth of memory traffic for a memory controller,
+"perf.cpu_cycles" - the count of cpu cycles (total/elapsed),
+"perf.instructions" - the count of instructions,
+"perf.cache_references" - the count of cache hits,
 "perf.cache_misses" - the count of caches misses
 
 See the B command for more details about each event.
-- 
2.1.0

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

[libvirt] [PATCH 2/2] virsh: Fix typos in manpage

[Nitesh Konkar]

Signed-off-by: Nitesh Konkar 
---
 tools/virsh.pod | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/virsh.pod b/tools/virsh.pod
index 72ace96..1fe4269 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -2286,28 +2286,28 @@ any existing per-device weights for other devices 
remain unchanged.
 B is a single string listing one or more 
device/read_iops_sec
 pairs, int the format of 
/path/to/device,read_iops_sec,/path/to/device,read_iops_sec.
 Each read_iops_sec is a number which type is unsigned int, value 0 to remove 
that
-device from per-decice listing.
+device from per-device listing.
 Only the devices listed in the string are modified;
 any existing per-device read_iops_sec for other devices remain unchanged.
 
 B is a single string listing one or more 
device/write_iops_sec
 pairs, int the format of 
/path/to/device,write_iops_sec,/path/to/device,write_iops_sec.
 Each write_iops_sec is a number which type is unsigned int, value 0 to remove 
that
-device from per-decice listing.
+device from per-device listing.
 Only the devices listed in the string are modified;
 any existing per-device write_iops_sec for other devices remain unchanged.
 
 B is a single string listing one or more 
device/read_bytes_sec
 pairs, int the format of 
/path/to/device,read_bytes_sec,/path/to/device,read_bytes_sec.
 Each read_bytes_sec is a number which type is unsigned long long, value 0 to 
remove
-that device from per-decice listing.
+that device from per-device listing.
 Only the devices listed in the string are modified;
 any existing per-device read_bytes_sec for other devices remain unchanged.
 
 B is a single string listing one or more 
device/write_bytes_sec
 pairs, int the format of 
/path/to/device,write_bytes_sec,/path/to/device,write_bytes_sec.
 Each write_bytes_sec is a number which type is unsigned long long, value 0 to 
remove
-that device from per-decice listing.
+that device from per-device listing.
 Only the devices listed in the string are modified;
 any existing per-device write_bytes_sec for other devices remain unchanged.
 
@@ -2401,7 +2401,7 @@ managedsave state is discarded and a fresh boot occurs.
 
 If I<--pass-fds> is specified, the argument is a comma separated list
 of open file descriptors which should be pass on into the guest. The
-file descriptors will be re-numered in the guest, starting from 3. This
+file descriptors will be re-numbered in the guest, starting from 3. This
 is only supported with container based virtualization.
 
 =item B I
-- 
2.1.0

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

[libvirt] [PATCH 0/2] Fix virsh manpage commas and typos.

[Nitesh Konkar]

Nitesh Konkar (2):
  virsh: Fix commas in manpage to enhance readability.
  virsh: Fix typos in manpage

 tools/virsh.pod | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

-- 
2.1.0

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

[libvirt] [PATCH 1/2] conf: Sanitize cpu topology numbers

[Peter Krempa]

Make sure that the topology results into a sane number of cpus (up to
UINT_MAX) so that it can be sanely compared to the vcpu count of the VM.

Additionally the helper added in this patch allows to fetch the total
number the topology results to so that it does not have to be
reimplemented later.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1378290
---
 src/conf/domain_conf.c   | 39 +++
 src/conf/domain_conf.h   |  2 ++
 src/libvirt_private.syms |  1 +
 3 files changed, 42 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f562323..44752b9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1570,6 +1570,42 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
 }


+/**
+ * virDomainDeGetVcpusTopology:
+ * @def: domain definition
+ * @maxvcpus: optionally filled with number of vcpus the domain topology 
describes
+ *
+ * Calculates and validates that the vcpu topology is in sane bounds and
+ * optionally returns the total number of vcpus described by given topology.
+ *
+ * Returns 0 on success, 1 if topology is not configured and -1 on error.
+ */
+int
+virDomainDefGetVcpusTopology(const virDomainDef *def,
+ unsigned int *maxvcpus)
+{
+unsigned long long tmp;
+
+if (!def->cpu || def->cpu->sockets == 0)
+return 1;
+
+tmp = def->cpu->sockets;
+
+/* multiplication of 32bit numbers fits into a 64bit variable */
+if ((tmp *= def->cpu->cores) > UINT_MAX ||
+(tmp *= def->cpu->threads) > UINT_MAX) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("cpu topology results in more than %u cpus"), 
UINT_MAX);
+return -1;
+}
+
+if (maxvcpus)
+*maxvcpus = tmp;
+
+return 0;
+}
+
+
 virDomainDiskDefPtr
 virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt)
 {
@@ -4786,6 +4822,9 @@ virDomainDefValidateInternal(const virDomainDef *def)
 if (virDomainDefCheckDuplicateDiskInfo(def) < 0)
 return -1;

+if (virDomainDefGetVcpusTopology(def, NULL) < 0)
+return -1;
+
 return 0;
 }

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a70bc21..54c9502 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2488,6 +2488,8 @@ virBitmapPtr virDomainDefGetOnlineVcpumap(const 
virDomainDef *def);
 virDomainVcpuDefPtr virDomainDefGetVcpu(virDomainDefPtr def, unsigned int vcpu)
 ATTRIBUTE_RETURN_CHECK;
 void virDomainDefVcpuOrderClear(virDomainDefPtr def);
+int  virDomainDefGetVcpusTopology(const virDomainDef *def,
+  unsigned int *maxvcpus);

 virDomainObjPtr virDomainObjNew(virDomainXMLOptionPtr caps)
 ATTRIBUTE_NONNULL(1);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 923afd1..233cf6b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -247,6 +247,7 @@ virDomainDefGetVcpu;
 virDomainDefGetVcpuPinInfoHelper;
 virDomainDefGetVcpus;
 virDomainDefGetVcpusMax;
+virDomainDefGetVcpusTopology;
 virDomainDefHasDeviceAddress;
 virDomainDefHasMemballoon;
 virDomainDefHasMemoryHotplug;
-- 
2.10.0

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

[libvirt] [PATCH 2/2] qemu: Reuse virDomainDeGetVcpusTopology to calculate total vcpu count

[Peter Krempa]

Rather than multiplying sockets, cores, and threads use the new helper
for getting the vcpu count resulting from the topology.
---
 src/qemu/qemu_domain.c | 18 --
 src/qemu/qemu_driver.c | 14 ++
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2b24c01..83ac389 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2401,7 +2401,7 @@ qemuDomainDefValidate(const virDomainDef *def,
 {
 virQEMUDriverPtr driver = opaque;
 virQEMUCapsPtr qemuCaps = NULL;
-size_t topologycpus;
+unsigned int topologycpus;
 int ret = -1;

 if (!(qemuCaps = virQEMUCapsCacheLookup(caps,
@@ -2443,15 +2443,13 @@ qemuDomainDefValidate(const virDomainDef *def,
 }

 /* qemu as of 2.5.0 rejects SMP topologies that don't match the cpu count 
*/
-if (def->cpu && def->cpu->sockets) {
-topologycpus = def->cpu->sockets * def->cpu->cores * def->cpu->threads;
-if (topologycpus != virDomainDefGetVcpusMax(def)) {
-/* presence of query-hotpluggable-cpus should be a good enough 
witness */
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("CPU topology doesn't match maximum vcpu 
count"));
-goto cleanup;
-}
+if (virDomainDefGetVcpusTopology(def, ) == 0 &&
+topologycpus != virDomainDefGetVcpusMax(def)) {
+/* presence of query-hotpluggable-cpus should be a good enough witness 
*/
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("CPU topology doesn't match maximum vcpu count"));
+goto cleanup;
 }
 }

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 29a7e3f..e6f845d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4718,6 +4718,7 @@ qemuDomainSetVcpusMax(virQEMUDriverPtr driver,
   unsigned int nvcpus)
 {
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+unsigned int topologycpus;
 int ret = -1;

 if (def) {
@@ -4733,16 +4734,13 @@ qemuDomainSetVcpusMax(virQEMUDriverPtr driver,
 goto cleanup;
 }

-if (persistentDef->cpu && persistentDef->cpu->sockets) {
+if (virDomainDefGetVcpusTopology(persistentDef, ) == 0 &&
+nvcpus != topologycpus) {
 /* allow setting a valid vcpu count for the topology so an invalid
  * setting may be corrected via this API */
-if (nvcpus != persistentDef->cpu->sockets *
-  persistentDef->cpu->cores *
-  persistentDef->cpu->threads) {
-virReportError(VIR_ERR_INVALID_ARG, "%s",
-   _("CPU topology doesn't match the desired vcpu 
count"));
-goto cleanup;
-}
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+   _("CPU topology doesn't match the desired vcpu count"));
+goto cleanup;
 }

 /* ordering information may become invalid, thus clear it */
-- 
2.10.0

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

[libvirt] [PATCH 0/2] Sanitize vcpu topology checking

[Peter Krempa]

See patch 1.

Peter Krempa (2):
  conf: Sanitize cpu topology numbers
  qemu: Reuse virDomainDeGetVcpusTopology to calculate total vcpu count

 src/conf/domain_conf.c   | 39 +++
 src/conf/domain_conf.h   |  2 ++
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_domain.c   | 18 --
 src/qemu/qemu_driver.c   | 14 ++
 5 files changed, 56 insertions(+), 18 deletions(-)

-- 
2.10.0

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

Re: [libvirt] [PATCH] libvirt-guests: Weaken dependency on libvirtd

[Martin Kletzander]

On Fri, Oct 07, 2016 at 10:19:53AM +0200, Andrea Bolognani wrote:

The Requires relationship is very strong, in that it prevents
a unit from running unless all the units it Requires are
running as well.

This turns out to be a problem because we want to be able to
restart libvirtd at any time without having libvirt-guests
suspend or shutdown running guests.

Turn the Requires relationship into a Wants relationship:
this way starting libvirt-guests will cause systemd to (attempt
to) start libvirtd as well, but stopping or restarting libvirtd
will not alter libvirt-guests' running state.


I can't figure out how exactly this works, even when looking at the
systemd.unit documentation.  You are saying that Wants= means that if
you issue 'service libvirtd stop' it will not save your guests after
this patch.  What if you stop the service and then shutdown the machine?
I would agree that it's your fault if it doesn't save your guests.  From
what I am reading, it makes sense, so ACK from me.  Basically because I
can't come up with scenarios that could be broken by this change =)

Martin


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

Re: [libvirt] [PATCH] audit: add audit information about panic devices

[Martin Kletzander]

On Mon, Oct 10, 2016 at 08:18:04PM +0800, Chen Hanxiao wrote:


At 2016-10-10 19:24:02, "Daniel P. Berrange"  wrote:

On Mon, Oct 10, 2016 at 07:19:57PM +0800, Chen Hanxiao wrote:

From: Chen Hanxiao 

This patch add audit info for panic notifier devices.


The audit code only emits audit information for cases where QEMU is
using some resource on the host. The panic device does not have any
host backend, so there's no reason to audit it.


Thanks for the clarification.
But should we doc it in auditlog.html.in?



Audit is, by definition, meant for auditing what do we allow qemu to
do.  So that later you can see what domains had access to what resources
on the system.  Doesn't make much sense stating that explicitly there,
but it's easy to get someone confused, so I wouldn't be totally against
adding one sentence to the Introduction, I guess.


Regards,
- Chen

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


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

Re: [libvirt] [PATCH 2/2 RFC] Purge marked callbacks before dispatching events

[Martin Kletzander]

On Mon, Oct 10, 2016 at 04:03:32PM +0800, Michal Privoznik wrote:

On 07.10.2016 19:52, Martin Kletzander wrote:

I don't have any justification for this except an empirical one: with
this patch the code from Bug 1363628 doesn't crash after "leaking".

I currently don't have properly working valgrind, but I'm working on it
and it might shed some light into this (although it might also not
happen due to the slowdown).

However in the meantime I attempted some analysis and I got even more
confused than before, I guess.  With this patch the code doesn't crash,
even though virObjectEventStateQueueDispatch() properly skips callbacks
marked as deleted.  What I feel is even more weird, that if I duplicate
the purgatory function (instead of moving it), it crashes again, and I
feel like even more often.

Weirdly-fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1363628

Signed-off-by: Martin Kletzander 
---
 src/conf/object_event.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index e5af4be68a7e..4066b4673b42 100644
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -821,13 +821,13 @@ virObjectEventStateFlush(virObjectEventStatePtr state)
 if (state->timer != -1)
 virEventUpdateTimeout(state->timer, -1);

+/* Purge any deleted callbacks */
+virObjectEventCallbackListPurgeMarked(state->callbacks);
+
 virObjectEventStateQueueDispatch(state,
  ,
  state->callbacks);

-/* Purge any deleted callbacks */
-virObjectEventCallbackListPurgeMarked(state->callbacks);
-
 state->isDispatching = false;
 virObjectEventStateUnlock(state);
 }


Well, I have no idea either, because virObjectEventStateQueueDispatch()
calls virObjectEventStateDispatchCallbacks() which in turn calls
virObjectEventDispatchMatchCallback() which returns false if the
callback->deleted is truem and thus the callback is skipped - just like
it is after this patch of yours.

This whole bug feels like a refcounting problem and IMO your patch is
just making the scenario more unlikely.



So I've found out what it is thanks to Luayo Huang.  And if I understand
it correctly, it won't happen with stateful drivers.  Stateless,
however, will have this problem.

The problem is that testConnectClose() clears the driver structure,
including calling virObjectEventStateFree() on the eventState.  It
clears the whole eventState, but it can happen while the eventState is
dispatching.  So there's the race.  And it would also explain what I was
seeing all the time.  qemuCleanup() is the only function in qemu driver
that would cleanup the eventState and since that gets called from the
daemon only after other stuff is cleaned up, it won't happen there.

I'll try turning virObjectEventStatePtr into virObject, we'll have
refcounting then and it will not disappear until it's stopped being
used.  I'll see if that helps or not, but it could.

Martin


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

Re: [libvirt] [PATCH 09/11] qemu_capabilities: check for existence of virtio-vga

[John Ferlan]

On 10/10/2016 06:50 AM, Pavel Hrdina wrote:
> On Sat, Oct 08, 2016 at 10:01:51AM -0400, John Ferlan wrote:
>>
>>
>> On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
>>> Commit 21373feb added support for primary virtio-vga device but it was
>>> checking for virtio-gpu.  Let's check for existence of virtio-vga if we
>>> want to use it.
>>>
>>> Signed-off-by: Pavel Hrdina 
>>> ---
>>>  src/qemu/qemu_capabilities.c  | 2 ++
>>>  src/qemu/qemu_capabilities.h  | 1 +
>>>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml  | 1 +
>>>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml  | 1 +
>>>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 1 +
>>>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml  | 1 +
>>>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml  | 1 +
>>>  7 files changed, 8 insertions(+)
>>>
>>
>> It seems this is related to your patch 6 changes or what I said should
>> be split. There's something funky there and I think everything should be
>> kept "closer" if not merged together.  It could get ugly, but getting it
>> all done and explained at once would be best IMO.
>>
>> It seems what you're indicating is that the two shouldn't have been
>> combined into one CAP.  Still that seems to be counter intuitive to the
>> first part of this series which was merging caps.  It's a bit confusing
>> to say the least ;-)!
> 
> The thing is that there is a difference in QXL video and virtio video.
> If QXL video device is compiled in QEMU there are always both models
> *qxl-vga* and *qxl*.  But this is not the same as for virtio video.
> It is currently represented by three different models *virtio-gpu-device*,
> *virtio-gpu-pci* and *virtio-vga*.  The first two models are tied together
> and if virtio video devices is compiled in they both exist.  However,
> the *virtio-vga* model doesn't have to exist on some architectures even if
> the first two models exist.  So we cannot group all three together.
> 

Cut, snip, place into commit message!

ACK - now that it's a bit clearer

John


> Pavel
> 
>>
>> Also you'll have a merge to handle with this patch...
>>
>>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>>> index 5449b07..55e5527 100644
>>> --- a/src/qemu/qemu_capabilities.c
>>> +++ b/src/qemu/qemu_capabilities.c
>>> @@ -344,6 +344,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>>>"query-hotpluggable-cpus",
>>>  
>>>"virtio-net.rx_queue_size", /* 235 */
>>> +  "virtio-vga",
>>>  );
>>>  
>>>  
>>> @@ -1570,6 +1571,7 @@ struct virQEMUCapsStringFlags 
>>> virQEMUCapsObjectTypes[] = {
>>>  { "virtio-net-device", QEMU_CAPS_DEVICE_VIRTIO_NET },
>>>  { "virtio-gpu-pci", QEMU_CAPS_DEVICE_VIRTIO_GPU },
>>>  { "virtio-gpu-device", QEMU_CAPS_DEVICE_VIRTIO_GPU },
>>> +{ "virtio-vga", QEMU_CAPS_DEVICE_VIRTIO_VGA },
>>>  { "virtio-keyboard-device", QEMU_CAPS_VIRTIO_KEYBOARD },
>>>  { "virtio-keyboard-pci", QEMU_CAPS_VIRTIO_KEYBOARD },
>>>  { "virtio-mouse-device", QEMU_CAPS_VIRTIO_MOUSE },
>>> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>>> index 1766aa6..b2c8c9f 100644
>>> --- a/src/qemu/qemu_capabilities.h
>>> +++ b/src/qemu/qemu_capabilities.h
>>> @@ -378,6 +378,7 @@ typedef enum {
>>>  
>>>  /* 235 */
>>>  QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE, /* virtio-net-*.rx_queue_size */
>>> +QEMU_CAPS_DEVICE_VIRTIO_VGA, /* virtio-vga */
>>
>> /* -device virtio-vga */
>>
>> Before an ACK - I'll be interested to understand the relationship w/
>> patch 6 and a bit of the history/reasoning for a separate cap
>> (especially in light of combining other caps).
>>
>> John
>>
>>>  
>>>  QEMU_CAPS_LAST /* this must always be the last item */
>>>  } virQEMUCapsFlags;
>>> diff --git a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml 
>>> b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml
>>> index 8d53988..17bd4bb 100644
>>> --- a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml
>>> +++ b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml
>>> @@ -180,6 +180,7 @@
>>>
>>>
>>>
>>> +  
>>>2004000
>>>0
>>>
>>> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml 
>>> b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml
>>> index a4f5663..fe058d9 100644
>>> --- a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml
>>> +++ b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml
>>> @@ -185,6 +185,7 @@
>>>
>>>
>>>
>>> +  
>>>2005000
>>>0
>>>
>>> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml 
>>> b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
>>> index 60f5392..e63e7c1 100644
>>> --- a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
>>> +++ b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
>>> @@ -152,6 +152,7 @@
>>>
>>>
>>>
>>> +  
>>>2005094
>>>0
>>>
>>> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml 
>>> 

Re: [libvirt] [PATCH 05/11] qemu_process: move video validation out of qemu_command

[John Ferlan]

On 10/10/2016 05:44 AM, Pavel Hrdina wrote:
> On Sat, Oct 08, 2016 at 09:58:15AM -0400, John Ferlan wrote:
>>
>>
>> On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
>>> Runtime validation that depend on qemu capabilities should be moved
>>> into qemuProcessStartValidateXML.
>>>
>>> Signed-off-by: Pavel Hrdina 
>>> ---
>>>  src/qemu/qemu_command.c  | 33 +---
>>>  src/qemu/qemu_process.c  | 50 ++-
>>>  tests/qemuxml2argvtest.c | 78 
>>> +---
>>>  3 files changed, 98 insertions(+), 63 deletions(-)
>>>
>>
>> Could the qemuxml2argvtest.c to add QEMU_CAPS_DEVICE_CIRRUS_VGA move to
>> earlier? Perhaps there is a relationship, but it's not clear from the
>> commit message...
> 
> The changes to qemuxml2argvtest.c can be moved to separate patch, the only
> relationship is that the movement of validation code depends on the changes
> to qemuxml2argvtest.c because this patch adds the validation for secondary
> video devices too.
> 
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 761968b..d283c67 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -4312,26 +4312,12 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
>>>  model = "virtio-gpu-pci";
>>>  }
>>>  } else {
>>> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) {
>>> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> -   "%s", _("only one video card is currently 
>>> supported"));
>>> -goto error;
>>> -}
>>> -
>>
>> Just checking - does the removal of this end up as part of the (now)
>> singular video loop in qemuProcessStartValidateVideo?  Same for the
>> removed check in qemuBuildVideoCommandLine?
>>
>> It wasn't "clear" the first pass through reading...
> 
> That exact error message is replaced by different error message:
> 
> "this QEMU does not support '%s' video device"
> 
> Currently only QXL can be used as secondary video device so if QXL device is 
> not
> supported by QEMU it also implicates that only one video card is supported.
> 
>>>  model = "qxl";
>>>  }
>>>  
>>>  virBufferAsprintf(, "%s,id=%s", model, video->info.alias);
>>>  
>>>  if (video->accel && video->accel->accel3d == VIR_TRISTATE_SWITCH_ON) {
>>> -if (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO ||
>>> -!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL)) {
>>> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> -   _("%s 3d acceleration is not supported"),
>>> -   virDomainVideoTypeToString(video->type));
>>> -goto error;
>>> -}
>>> -
>>>  virBufferAsprintf(, ",virgl=%s",
>>>
>>> virTristateSwitchTypeToString(video->accel->accel3d));
>>>  }
>>> @@ -4397,17 +4383,7 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
>>>  
>>>  primaryVideoType = def->videos[0]->type;
>>>  
>>> -if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY) &&
>>> - ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VGA &&
>>> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) ||
>>> - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_CIRRUS &&
>>> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) ||
>>> - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
>>> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) ||
>>> - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL &&
>>> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) ||
>>> - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
>>> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU {
>>> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) {
>>>  for (i = 0; i < def->nvideos; i++) {
>>>  char *str;
>>>  virCommandAddArg(cmd, "-device");
>>> @@ -4422,13 +4398,6 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
>>>  if (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_XEN) {
>>>  /* nothing - vga has no effect on Xen pvfb */
>>>  } else {
>>> -if ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL) &&
>>> -!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) {
>>> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> -   _("This QEMU does not support QXL graphics 
>>> adapters"));
>>> -return -1;
>>> -}
>>> -
>>
>> ... Is it safe to assume there can only be one video card here?
> 
> Where did you get the assumption from?
> 

Probably written before I found the gem in domain_conf about primary
being [0] and I never got back to the line in my followup pass.

I was "attempting to" match the removed code with the new code. I guess
I didn't find this as clean as it appears to 

Re: [libvirt] [PATCH v6 3/3] target-i386: Return runnability information on query-cpu-definitions

[Igor Mammedov]

On Fri,  7 Oct 2016 17:29:02 -0300
Eduardo Habkost  wrote:

> Fill the "unavailable-features" field on the x86 implementation
> of query-cpu-definitions.
> 
> Cc: Jiri Denemark 
> Cc: libvir-list@redhat.com
> Signed-off-by: Eduardo Habkost 
> ---
> Changes v5 -> v6:
> * Call x86_cpu_filter_features(), now that x86_cpu_load_features()
>   won't run it automatically
> 
> Changes v4 -> v5:
> * (none)
> 
> Changes v3 -> v4:
> * Handle missing XSAVE components cleanly, but looking up
>   the original feature that required it
> * Use x86_cpu_load_features() function
> 
> Changes v2 -> v3:
> * Create a x86_cpu_feature_name() function, to
>   isolate the code that returns the property name
> 
> Changes v1 -> v2:
> * Updated to the new schema: no @runnable field, and
>   always report @unavailable-features as present
> ---
>  target-i386/cpu.c | 76 
> +++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 23cc19b..63330ce 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1945,6 +1945,27 @@ static inline void feat2prop(char *s)
>  }
>  }
>  
> +/* Return the feature property name for a feature flag bit */
> +static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
> +{
> +/* XSAVE components are automatically enabled by other features,
> + * so return the original feature name instead
> + */
> +if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) {
> +int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr;
> +
> +if (comp < ARRAY_SIZE(x86_ext_save_areas) &&
> +x86_ext_save_areas[comp].bits) {
> +w = x86_ext_save_areas[comp].feature;
> +bitnr = ctz32(x86_ext_save_areas[comp].bits);
> +}
> +}
> +
> +assert(bitnr < 32);
> +assert(w < FEATURE_WORDS);
> +return feature_word_info[w].feat_names[bitnr];
> +}
> +
>  /* Compatibily hack to maintain legacy +-feat semantic,
>   * where +-feat overwrites any feature set by
>   * feat=on|feat even if the later is parsed after +-feat
> @@ -2030,6 +2051,59 @@ static void x86_cpu_parse_featurestr(const char 
> *typename, char *features,
>  }
>  }
>  
> +static void x86_cpu_load_features(X86CPU *cpu, Error **errp);
> +static int x86_cpu_filter_features(X86CPU *cpu);
> +
> +/* Check for missing features that may prevent the CPU class from
> + * running using the current machine and accelerator.
> + */
> +static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
> + strList **missing_feats)
> +{
> +X86CPU *xc;
> +FeatureWord w;
> +Error *err = NULL;
> +strList **next = missing_feats;
> +
> +if (xcc->kvm_required && !kvm_enabled()) {
> +strList *new = g_new0(strList, 1);
> +new->value = g_strdup("kvm");;
> +*missing_feats = new;
> +return;
> +}
> +
> +xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
> +
> +x86_cpu_load_features(xc, );
> +if (err) {
> +/* Errors at x86_cpu_load_features should never happen,
> + * but in case it does, just report the model as not
> + * runnable at all using the "type" property.
> + */
> +strList *new = g_new0(strList, 1);
> +new->value = g_strdup("type");
> +*next = new;
> +next = >next;
> +}
> +
> +x86_cpu_filter_features(xc);
> +
> +for (w = 0; w < FEATURE_WORDS; w++) {
> +uint32_t filtered = xc->filtered_features[w];
> +int i;
> +for (i = 0; i < 32; i++) {
> +if (filtered & (1UL << i)) {
> +strList *new = g_new0(strList, 1);
> +new->value = g_strdup(x86_cpu_feature_name(w, i));
> +*next = new;
> +next = >next;
> +}
> +}
> +}
Shouldn't you add 
   if (IS_AMD_CPU(env)) { 
fixup here, that realize does right after calling x86_cpu_filter_features()


> +object_unref(OBJECT(xc));
> +}
> +
>  /* Print all cpuid feature names in featureset
>   */
>  static void listflags(FILE *f, fprintf_function print, const char 
> **featureset)
> @@ -2122,6 +2196,8 @@ static void x86_cpu_definition_entry(gpointer data, 
> gpointer user_data)
>  
>  info = g_malloc0(sizeof(*info));
>  info->name = x86_cpu_class_get_model_name(cc);
> +x86_cpu_class_check_missing_features(cc, >unavailable_features);
> +info->has_unavailable_features = true;
>  
>  entry = g_malloc0(sizeof(*entry));
>  entry->value = info;

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

Re: [libvirt] [PATCH v6 2/3] target-i386: x86_cpu_load_features() function

[Igor Mammedov]

On Fri,  7 Oct 2016 17:29:01 -0300
Eduardo Habkost  wrote:

> When probing for CPU model information, we need to reuse the code
> that initializes CPUID fields, but not the remaining side-effects
> of x86_cpu_realizefn(). Move that code to a separate function
> that can be reused later.
> 
> Signed-off-by: Eduardo Habkost 
> ---
> Changes v5 -> v6:
> * Move x86_cpu_filter_features() outside x86_cpu_load_features(),
>   as the CPU model querying API won't run
>   x86_cpu_filter_features() on most cases

> 
> Changes v4 -> v5:
> * Fix typo on x86_cpu_load_features() comment
>   Reported-by: Paolo Bonzini 
> 
> Changes series v3 -> v4:
> * New patch added to series
> ---
>  target-i386/cpu.c | 67 
> +++
>  1 file changed, 43 insertions(+), 24 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1e8127b..23cc19b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2993,34 +2993,13 @@ static void x86_cpu_enable_xsave_components(X86CPU 
> *cpu)
>  env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
>  }
>  
> -#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
> -   (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
> -   (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
> -#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
> - (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
> - (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> -static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> +/* Load CPUID data based on configured features */
> +static void x86_cpu_load_features(X86CPU *cpu, Error **errp)
>  {
> -CPUState *cs = CPU(dev);
> -X86CPU *cpu = X86_CPU(dev);
> -X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
>  CPUX86State *env = >env;
> -Error *local_err = NULL;
> -static bool ht_warned;
>  FeatureWord w;
>  GList *l;
> -
> -if (xcc->kvm_required && !kvm_enabled()) {
> -char *name = x86_cpu_class_get_model_name(xcc);
> -error_setg(_err, "CPU model '%s' requires KVM", name);
> -g_free(name);
> -goto out;
> -}
> -
> -if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> -error_setg(errp, "apic-id property was not initialized properly");
> -return;
> -}
> +Error *local_err = NULL;
>  
>  /*TODO: cpu->host_features incorrectly overwrites features
>   * set using "feat=on|off". Once we fix this, we can convert
> @@ -3086,6 +3065,46 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
>  }
>  
> +out:
> +if (local_err != NULL) {
> +error_propagate(errp, local_err);
> +}
> +}
> +
> +#define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
> +   (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
> +   (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3)
> +#define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
> + (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
> + (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> +static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> +{
> +CPUState *cs = CPU(dev);
> +X86CPU *cpu = X86_CPU(dev);
> +X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> +CPUX86State *env = >env;
> +Error *local_err = NULL;
> +static bool ht_warned;
> +
> +if (xcc->kvm_required && !kvm_enabled()) {
> +char *name = x86_cpu_class_get_model_name(xcc);
> +error_setg(_err, "CPU model '%s' requires KVM", name);
> +g_free(name);
> +goto out;
> +}
> +
> +if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> +error_setg(errp, "apic-id property was not initialized properly");
> +return;
> +}
> +
> +x86_cpu_load_features(cpu, _err);
> +if (local_err) {
> +goto out;
> +}
> +
> +x86_cpu_filter_features(cpu);
that makes 2 invocations of ^^ inside realize,
see followup line 
 
[...]
>  if (x86_cpu_filter_features(cpu) &&
>  (cpu->check_cpuid || cpu->enforce_cpuid)) {
>  x86_cpu_report_filtered_features(cpu);

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

Re: [libvirt] [PATCH] audit: add audit information about panic devices

[Chen Hanxiao]

At 2016-10-10 19:24:02, "Daniel P. Berrange"  wrote:
>On Mon, Oct 10, 2016 at 07:19:57PM +0800, Chen Hanxiao wrote:
>> From: Chen Hanxiao 
>> 
>> This patch add audit info for panic notifier devices.
>
>The audit code only emits audit information for cases where QEMU is
>using some resource on the host. The panic device does not have any
>host backend, so there's no reason to audit it.

Thanks for the clarification.
But should we doc it in auditlog.html.in?

Regards,
- Chen

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

[libvirt] [PATCH] virlog: Enclose part of virLogDefineOutputs in HAVE_SYSLOG_H block

[Erik Skultety]

Commit 640b58ab broke the mingw build because it referenced 'current_ident'
as well as 'openlog' symbols both of which are declared conditionally within
HAVE_SYSLOG_H directive. This patch fixes the broken build. Additional changes
like moving all variable declarations in virLogDefineOutputs into the
conditional block were necessary to avoid 'unused variable' errors from mingw.

Signed-off-by: Erik Skultety 
---
I could've pushed it under build breaker rule but I'd rather be safe than sorry
that I missed anything else.

Erik


 src/util/virlog.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/util/virlog.c b/src/util/virlog.c
index 14ee701..f52d9d8 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -1362,16 +1362,15 @@ virLogFindOutput(virLogOutputPtr *outputs, size_t 
noutputs,
 int
 virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs)
 {
-int ret = -1;
-int id;
+if (virLogInitialize() < 0)
+return -1;
+
+virLogLock();
+virLogResetOutputs();
+
+#if HAVE_SYSLOG_H
+int id = -1;
 char *tmp = NULL;
-
-if (virLogInitialize() < 0)
-return -1;
-
-virLogLock();
-virLogResetOutputs();
-
 /* syslog needs to be special-cased, since it keeps the fd in private */
 if ((id = virLogFindOutput(outputs, noutputs, VIR_LOG_TO_SYSLOG,
current_ident)) != -1) {
@@ -1379,20 +1378,21 @@ virLogDefineOutputs(virLogOutputPtr *outputs, size_t 
noutputs)
  * holding the lock so it's safe to call openlog and change the message
  * tag
  */
-if (VIR_STRDUP_QUIET(tmp, outputs[id]->name) < 0)
-goto cleanup;
+if (VIR_STRDUP_QUIET(tmp, outputs[id]->name) < 0) {
+virLogUnlock();
+return -1;
+}
 VIR_FREE(current_ident);
 current_ident = tmp;
 openlog(current_ident, 0, 0);
 }
+#endif
 
 virLogOutputs = outputs;
 virLogNbOutputs = noutputs;
 
-ret = 0;
- cleanup:
 virLogUnlock();
-return ret;
+return 0;
 }
 
 
-- 
2.5.5

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

[libvirt] [PATCH] qemu: hotplug: support auth for scsi hotplug

[Gema Gomez]

Add any key objects to qemu when hotplugging a scsi drive with secinfo.

Support for this was added in a previous commit for normal drives, but
not for SCSI drives.
---
 src/qemu/qemu_hotplug.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 72dd93b..a450492 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -601,13 +601,16 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
 char *devstr = NULL;
 bool driveAdded = false;
 bool encobjAdded = false;
+bool secobjAdded = false;
 char *drivealias = NULL;
 int ret = -1;
 int rv;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 virJSONValuePtr encobjProps = NULL;
+virJSONValuePtr secobjProps = NULL;
 qemuDomainDiskPrivatePtr diskPriv;
 qemuDomainSecretInfoPtr encinfo;
+qemuDomainSecretInfoPtr secinfo;
 
 if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0)
 goto cleanup;
@@ -639,6 +642,12 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
 goto error;
 
 diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+secinfo = diskPriv->secinfo;
+if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
+if (qemuBuildSecretInfoProps(secinfo, ) < 0)
+goto error;
+}
+
 encinfo = diskPriv->encinfo;
 if (encinfo && qemuBuildSecretInfoProps(encinfo, ) < 0)
 goto error;
@@ -657,6 +666,15 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
 
 qemuDomainObjEnterMonitor(driver, vm);
 
+if (secobjProps) {
+rv = qemuMonitorAddObject(priv->mon, "secret", secinfo->s.aes.alias,
+  secobjProps);
+secobjProps = NULL; /* qemuMonitorAddObject consumes */
+if (rv < 0)
+goto exit_monitor;
+secobjAdded = true;
+}
+
 if (encobjProps) {
 rv = qemuMonitorAddObject(priv->mon, "secret", encinfo->s.aes.alias,
   encobjProps);
@@ -682,6 +700,7 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
 ret = 0;
 
  cleanup:
+virJSONValueFree(secobjProps);
 virJSONValueFree(encobjProps);
 qemuDomainSecretDiskDestroy(disk);
 VIR_FREE(devstr);
@@ -696,6 +715,8 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
 VIR_WARN("Unable to remove drive %s (%s) after failed "
  "qemuMonitorAddDevice", drivealias, drivestr);
 }
+if (secobjAdded)
+ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias));
 if (encobjAdded)
 ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias));
 if (orig_err) {
-- 
2.10.0

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

[libvirt] [PATCH] support auth for qemu SCSI hotplug

[Gema Gomez]

Hi all,

commit fceeeda2 added support for adding key objects on hotplug based on a 
disk's secinfo for normal drives, but missed out SCSI drives. This patch adds 
the same support for SCSI drives, so that it's possible to hotplug SCSI drives 
requiring authentication (e.g. rbd-backed drives).

Cheers,
Gema


Gema Gomez (1):
  qemu: hotplug: support auth for scsi hotplug

 src/qemu/qemu_hotplug.c | 21 +
 1 file changed, 21 insertions(+)

-- 
2.10.0

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

Re: [libvirt] [PATCH v6 1/3] target-i386: Move warning code outside x86_cpu_filter_features()

[Igor Mammedov]

On Fri,  7 Oct 2016 17:29:00 -0300
Eduardo Habkost  wrote:

> x86_cpu_filter_features() will be reused by code that shouldn't
> print any warning. Move the warning code to a new
> x86_cpu_report_filtered_features() function, and call it from
> x86_cpu_realizefn().
> 
> Signed-off-by: Eduardo Habkost 
Reviewed-by: Igor Mammedov 

> ---
> Changes v5 -> v6:
> * Recovered v3 of patch, because x86_cpu_load_features()
>   won't call x86_cpu_filter_features() anymore and we can keep
>   the previous logic in x86_cpu_realizefn() that checked
>   x86_cpu_filter_features() return value
> 
> Changes v4 -> v5:
> * (none)
> 
> Changes v3 -> v4:
> * Made x86_cpu_filter_features() void, make
>   x86_cpu_report_filtered_features() return true if
>   some features were filtered
> ---
>  target-i386/cpu.c | 28 +++-
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 61240dd..1e8127b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2177,9 +2177,6 @@ static int x86_cpu_filter_features(X86CPU *cpu)
>  env->features[w] &= host_feat;
>  cpu->filtered_features[w] = requested_features & ~env->features[w];
>  if (cpu->filtered_features[w]) {
> -if (cpu->check_cpuid || cpu->enforce_cpuid) {
> -report_unavailable_features(w, cpu->filtered_features[w]);
> -}
>  rv = 1;
>  }
>  }
> @@ -2187,6 +2184,15 @@ static int x86_cpu_filter_features(X86CPU *cpu)
>  return rv;
>  }
>  
> +static void x86_cpu_report_filtered_features(X86CPU *cpu)
> +{
> +FeatureWord w;
> +
> +for (w = 0; w < FEATURE_WORDS; w++) {
> +report_unavailable_features(w, cpu->filtered_features[w]);
> +}
> +}
> +
>  static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
>  {
>  PropValue *pv;
> @@ -3080,12 +3086,16 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
>  }
>  
> -if (x86_cpu_filter_features(cpu) && cpu->enforce_cpuid) {
> -error_setg(_err,
> -   kvm_enabled() ?
> -   "Host doesn't support requested features" :
> -   "TCG doesn't support requested features");
> -goto out;
> +if (x86_cpu_filter_features(cpu) &&
> +(cpu->check_cpuid || cpu->enforce_cpuid)) {
> +x86_cpu_report_filtered_features(cpu);
> +if (cpu->enforce_cpuid) {
> +error_setg(_err,
> +   kvm_enabled() ?
> +   "Host doesn't support requested features" :
> +   "TCG doesn't support requested features");
> +goto out;
> +}
>  }
>  
>  /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on

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

Re: [libvirt] [PATCH] audit: add audit information about panic devices

[Daniel P. Berrange]

On Mon, Oct 10, 2016 at 07:19:57PM +0800, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> This patch add audit info for panic notifier devices.

The audit code only emits audit information for cases where QEMU is
using some resource on the host. The panic device does not have any
host backend, so there's no reason to audit it.


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

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

[libvirt] [PATCH] audit: add audit information about panic devices

[Chen Hanxiao]

From: Chen Hanxiao 

This patch add audit info for panic notifier devices.

Signed-off-by: Chen Hanxiao 
---
 docs/auditlog.html.in| 15 +++
 src/conf/domain_audit.c  | 38 ++
 src/conf/domain_audit.h  |  4 
 src/libvirt_private.syms |  1 +
 4 files changed, 58 insertions(+)

diff --git a/docs/auditlog.html.in b/docs/auditlog.html.in
index 0c778aa..45464af 100644
--- a/docs/auditlog.html.in
+++ b/docs/auditlog.html.in
@@ -371,5 +371,20 @@
   Path of the backing character device for given emulated device
 
 
+
+Panic notifier
+
+  The msg field will include the following sub-fields
+
+
+
+  resrc
+  The type of resource assigned. Set to panic
+  reason
+  The reason which caused the resource to be assigned to happen
+  model
+  The model of the panic notifier device
+
+
   
 
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index fd20ace..e48a63d 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -893,6 +893,9 @@ virDomainAuditStart(virDomainObjPtr vm, const char *reason, 
bool success)
 for (i = 0; i < vm->def->nshmems; i++)
 virDomainAuditShmem(vm, vm->def->shmems[i], "start", true);
 
+for (i = 0; i < vm->def->npanics; i++)
+virDomainAuditPanic(vm, vm->def->panics[i], "start", true);
+
 virDomainAuditMemory(vm, 0, virDomainDefGetMemoryTotal(vm->def),
  "start", true);
 virDomainAuditVcpu(vm, 0, virDomainDefGetVcpus(vm->def), "start", true);
@@ -1006,3 +1009,38 @@ virDomainAuditShmem(virDomainObjPtr vm,
 VIR_FREE(shmem);
 return;
 }
+
+void
+virDomainAuditPanic(virDomainObjPtr vm,
+virDomainPanicDefPtr def,
+const char *reason,
+bool success)
+{
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+char *vmname = virAuditEncode("vm", vm->def->name);
+const char *panic_model = virDomainPanicModelTypeToString(def->model);
+char *model = virAuditEncode("model", VIR_AUDIT_STR(panic_model));
+const char *virt = virDomainVirtTypeToString(vm->def->virtType);
+
+virUUIDFormat(vm->def->uuid, uuidstr);
+
+if (!vmname || !model) {
+VIR_WARN("OOM while encoding audit message");
+goto cleanup;
+}
+
+if (!virt) {
+VIR_WARN("Unexpected virt type %d while encoding audit message",
+ vm->def->virtType);
+virt = "?";
+}
+
+VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success,
+  "virt=%s resrc=PanicNotifier reason=%s %s uuid=%s %s",
+  virt, reason, vmname, uuidstr, model);
+
+ cleanup:
+VIR_FREE(vmname);
+VIR_FREE(model);
+return;
+}
diff --git a/src/conf/domain_audit.h b/src/conf/domain_audit.h
index 8cb585d..10ecc2a 100644
--- a/src/conf/domain_audit.h
+++ b/src/conf/domain_audit.h
@@ -133,6 +133,10 @@ void virDomainAuditShmem(virDomainObjPtr vm,
  virDomainShmemDefPtr def,
  const char *reason, bool success)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+void virDomainAuditPanic(virDomainObjPtr vm,
+ virDomainPanicDefPtr def,
+ const char *reason, bool success)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
 
 
 #endif /* __VIR_DOMAIN_AUDIT_H__ */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 923afd1..94ec7cb 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -146,6 +146,7 @@ virDomainAuditIOThread;
 virDomainAuditMemory;
 virDomainAuditNet;
 virDomainAuditNetDevice;
+virDomainAuditPanic;
 virDomainAuditRedirdev;
 virDomainAuditRNG;
 virDomainAuditSecurityLabel;
-- 
1.8.3.1


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

Re: [libvirt] [PATCH 11/11] qemu_command: add support to use virtio as secondary video device

[Pavel Hrdina]

On Sat, Oct 08, 2016 at 10:02:14AM -0400, John Ferlan wrote:
> 
> Nothing else to say?!  Is this fixing an existing bug making some other
> patch/fix "more correct".

After sending the patch I've find out that there is a BZ for this and I was
planning to add it to the commit message before pushing.  Other than that the
subject explain everything form my POW.

> On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  docs/formatdomain.html.in  |  3 +-
> >  src/qemu/qemu_command.c| 32 ---
> >  src/qemu/qemu_domain.c |  3 +-
> >  .../qemuxml2argv-video-virtio-gpu-sec.args | 25 +++
> >  .../qemuxml2argv-video-virtio-gpu-sec.xml  | 36 
> > ++
> >  tests/qemuxml2argvtest.c   |  3 ++
> >  6 files changed, 89 insertions(+), 13 deletions(-)
> >  create mode 100644 
> > tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.args
> >  create mode 100644 
> > tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.xml
> > 
> 
> Adding a new .xml without also adding a qemuxml2xmltest for the same xml
> file?   Note that you can "save" space if you make the xml2xml output
> file be the same as the input file and just create a link to it (like a
> number of other more recent changes are doing).
> 
> 
> Also it seems there are two changes happening here - thus separate patches.
> 
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 1266e9d..f08cd01 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -5641,7 +5641,8 @@ qemu-kvm -net nic,model=? /dev/null
> >video device in domain xml is the primary one, but the optional
> >attribute primary (since 
> > 1.0.2)
> >with value 'yes' can be used to mark the primary in cases of 
> > multiple
> > -  video device. The non-primary must be type of "qxl".
> > +  video device. The non-primary must be type of "qxl" or
> > +  "virtio" (since 2.4.0).
> >  
> >
> >  
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index bb8128e..2314b2df 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -114,6 +114,18 @@ VIR_ENUM_IMPL(qemuDeviceVideo, 
> > VIR_DOMAIN_VIDEO_TYPE_LAST,
> >"", /* don't support parallels */
> >"virtio-vga");
> >  
> > +VIR_ENUM_DECL(qemuDeviceVideoSec)
> > +
> > +VIR_ENUM_IMPL(qemuDeviceVideoSec, VIR_DOMAIN_VIDEO_TYPE_LAST,
> > +  "", /* no secondary device for VGA*/
> > +  "", /* no secondary device for cirrus-vga*/
> > +  "", /* no secondary device for vmware-svga*/
> > +  "", /* no device for xen */
> > +  "", /* don't support vbox */
> > +  "qxl",
> > +  "", /* don't support parallels */
> > +  "virtio-gpu-pci");
> > +
> 
> I know "Sec" is shorthand for secondary, but I've also seen it used for
> "second"... being verbose isn't bad especially when you read the code
> months later.

Sure, I can change it to ..Secondary.

> 
> >  VIR_ENUM_DECL(qemuSoundCodec)
> >  
> >  VIR_ENUM_IMPL(qemuSoundCodec, VIR_DOMAIN_SOUND_CODEC_TYPE_LAST,
> > @@ -4299,18 +4311,16 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
> >  virBuffer buf = VIR_BUFFER_INITIALIZER;
> >  const char *model;
> >  
> > -if (video->primary) {
> > +if (video->primary && qemuDomainSupportVideoVga(video, qemuCaps))
> 
> This alters your logic, but the else condition is assuming secondary and
> in fact digging into that table... Shouldn't the logic be:
> 
>if (video->primary) {
>   if (qemuDomainSupportVideoVga()
>  model = qemuDeviceVideoTypeToString(video->type);
>   else
>  model = "virtio-gpu-pci";
>} else {
>   model = qemuDeviceVideoSecTypeToString(video->type);
>}

This is exactly what I was trying to avoid.  Having an extra string outside
of that two enums.  There are two sets of models for video devices, the first
one is with VGA compatibility support (qemuDeviceVideo) and the second one is
without the VGA compatibility (qemuDeviceVideoSec).  The concept of primary vs
secondary video devices is based on the fact that libvirt tries to use video
device with VGA compatibility as primary video device.

The logic is to use the best possible model that is available.

> 
> >  model = qemuDeviceVideoTypeToString(video->type);
> > -if (!model || STREQ(model, "")) {
> > -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > -   _("video type %s is not supported with QEMU"),
> > -   virDomainVideoTypeToString(video->type));
> > -goto error;
> > -}
> > -if (!qemuDomainSupportVideoVga(video, qemuCaps))
> > -model = 

Re: [libvirt] [PATCH 10/11] qemu_command: properly detect which model to use for video device

[Pavel Hrdina]

On Sat, Oct 08, 2016 at 10:02:00AM -0400, John Ferlan wrote:
> 
> 
> On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
> > This improves commit 706b5b6277 in a way that we instead use qemu
> > capabilities to detect whether we can use virto-vga or not.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/qemu/qemu_command.c|  4 +--
> >  src/qemu/qemu_domain.c | 12 
> >  src/qemu/qemu_domain.h |  3 ++
> >  .../qemuxml2argv-video-virtio-gpu-device.args  |  2 +-
> >  .../qemuxml2argv-video-virtio-gpu-spice-gl.args|  2 +-
> >  .../qemuxml2argv-video-virtio-gpu-virgl.args   |  2 +-
> >  .../qemuxml2argv-video-virtio-vga.args | 24 
> >  .../qemuxml2argv-video-virtio-vga.xml  | 33 
> > ++
> >  tests/qemuxml2argvtest.c   |  4 +++
> >  9 files changed, 80 insertions(+), 6 deletions(-)
> >  create mode 100644 
> > tests/qemuxml2argvdata/qemuxml2argv-video-virtio-vga.args
> >  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-virtio-vga.xml
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 15bb381..bb8128e 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -4307,10 +4307,8 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
> > virDomainVideoTypeToString(video->type));
> >  goto error;
> >  }
> > -if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
> > -qemuDomainMachineIsVirt(def)) {
> > +if (!qemuDomainSupportVideoVga(video, qemuCaps))
> >  model = "virtio-gpu-pci";
> > -}
> >  } else {
> >  model = "qxl";
> >  }
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 2b7e6d4..79c945a 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -6230,3 +6230,15 @@ qemuDomainCheckMonitor(virQEMUDriverPtr driver,
> >  
> >  return ret;
> >  }
> > +
> > +
> > +bool
> > +qemuDomainSupportVideoVga(virDomainVideoDefPtr video,
> > +  virQEMUCapsPtr qemuCaps)
> > +{
> > +if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
> > +!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_VGA))
> 
> Shall I assume losing the qemuDomainMachineIsVirt check is expected?
> Perhaps a quick explanation in the commit message would suffice...

Yes, because previous patch adds proper detection whether we can use
*virtio-vga* model or not, so we can use that capability instead of
listing architectures (which may change in the future).

> As in, commit id 'xxx' use MachineIsVirt, when it should have been
> checking the cap instead...

At that time it was a quick fix to resolve that bug and using the capability
was not possible because that capability did not exist.

> There's two things going on in this one patch - not only creating a
> helper, but you're altering the logic such that you have differences in
> output (.args) that should have been "that way" when the code was first
> introduced. In a way it feels like fixing a bug... Following the
> breadcrumbs is, well mind numbing!

This basically fixes a bug by improving the logic, I'll try to explain it
better in the commit message.

Pavel

> 
> ACK in principle - still just a better explanation would be good!
> 
> 
> John
> 
> 
> 
> > +return false;
> > +
> > +return true;
> > +}
> > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> > index 521531b..b968858 100644
> > --- a/src/qemu/qemu_domain.h
> > +++ b/src/qemu/qemu_domain.h
> > @@ -738,4 +738,7 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver,
> > virDomainObjPtr vm,
> > qemuDomainAsyncJob asyncJob);
> >  
> > +bool qemuDomainSupportVideoVga(virDomainVideoDefPtr video,
> > +   virQEMUCapsPtr qemuCaps);
> > +
> >  #endif /* __QEMU_DOMAIN_H__ */
> > diff --git 
> > a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-device.args 
> > b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-device.args
> > index fefa2b6..1107409 100644
> > --- a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-device.args
> > +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-device.args
> > @@ -20,5 +20,5 @@ QEMU_AUDIO_DRV=none \
> >  -drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,if=none,\
> >  id=drive-ide0-0-0,cache=none \
> >  -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
> > --device virtio-vga,id=video0,bus=pci.0,addr=0x2 \
> > +-device virtio-gpu-pci,id=video0,bus=pci.0,addr=0x2 \
> >  -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
> > diff --git 
> > a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args 
> > b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args
> > index 8844498..f1ebb62 

Re: [libvirt] [PATCH 09/11] qemu_capabilities: check for existence of virtio-vga

[Pavel Hrdina]

On Sat, Oct 08, 2016 at 10:01:51AM -0400, John Ferlan wrote:
> 
> 
> On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
> > Commit 21373feb added support for primary virtio-vga device but it was
> > checking for virtio-gpu.  Let's check for existence of virtio-vga if we
> > want to use it.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/qemu/qemu_capabilities.c  | 2 ++
> >  src/qemu/qemu_capabilities.h  | 1 +
> >  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml  | 1 +
> >  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml  | 1 +
> >  tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 1 +
> >  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml  | 1 +
> >  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml  | 1 +
> >  7 files changed, 8 insertions(+)
> > 
> 
> It seems this is related to your patch 6 changes or what I said should
> be split. There's something funky there and I think everything should be
> kept "closer" if not merged together.  It could get ugly, but getting it
> all done and explained at once would be best IMO.
> 
> It seems what you're indicating is that the two shouldn't have been
> combined into one CAP.  Still that seems to be counter intuitive to the
> first part of this series which was merging caps.  It's a bit confusing
> to say the least ;-)!

The thing is that there is a difference in QXL video and virtio video.
If QXL video device is compiled in QEMU there are always both models
*qxl-vga* and *qxl*.  But this is not the same as for virtio video.
It is currently represented by three different models *virtio-gpu-device*,
*virtio-gpu-pci* and *virtio-vga*.  The first two models are tied together
and if virtio video devices is compiled in they both exist.  However,
the *virtio-vga* model doesn't have to exist on some architectures even if
the first two models exist.  So we cannot group all three together.

Pavel

> 
> Also you'll have a merge to handle with this patch...
> 
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 5449b07..55e5527 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -344,6 +344,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
> >"query-hotpluggable-cpus",
> >  
> >"virtio-net.rx_queue_size", /* 235 */
> > +  "virtio-vga",
> >  );
> >  
> >  
> > @@ -1570,6 +1571,7 @@ struct virQEMUCapsStringFlags 
> > virQEMUCapsObjectTypes[] = {
> >  { "virtio-net-device", QEMU_CAPS_DEVICE_VIRTIO_NET },
> >  { "virtio-gpu-pci", QEMU_CAPS_DEVICE_VIRTIO_GPU },
> >  { "virtio-gpu-device", QEMU_CAPS_DEVICE_VIRTIO_GPU },
> > +{ "virtio-vga", QEMU_CAPS_DEVICE_VIRTIO_VGA },
> >  { "virtio-keyboard-device", QEMU_CAPS_VIRTIO_KEYBOARD },
> >  { "virtio-keyboard-pci", QEMU_CAPS_VIRTIO_KEYBOARD },
> >  { "virtio-mouse-device", QEMU_CAPS_VIRTIO_MOUSE },
> > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> > index 1766aa6..b2c8c9f 100644
> > --- a/src/qemu/qemu_capabilities.h
> > +++ b/src/qemu/qemu_capabilities.h
> > @@ -378,6 +378,7 @@ typedef enum {
> >  
> >  /* 235 */
> >  QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE, /* virtio-net-*.rx_queue_size */
> > +QEMU_CAPS_DEVICE_VIRTIO_VGA, /* virtio-vga */
> 
> /* -device virtio-vga */
> 
> Before an ACK - I'll be interested to understand the relationship w/
> patch 6 and a bit of the history/reasoning for a separate cap
> (especially in light of combining other caps).
> 
> John
> 
> >  
> >  QEMU_CAPS_LAST /* this must always be the last item */
> >  } virQEMUCapsFlags;
> > diff --git a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml 
> > b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml
> > index 8d53988..17bd4bb 100644
> > --- a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml
> > +++ b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml
> > @@ -180,6 +180,7 @@
> >
> >
> >
> > +  
> >2004000
> >0
> >
> > diff --git a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml 
> > b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml
> > index a4f5663..fe058d9 100644
> > --- a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml
> > +++ b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml
> > @@ -185,6 +185,7 @@
> >
> >
> >
> > +  
> >2005000
> >0
> >
> > diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml 
> > b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
> > index 60f5392..e63e7c1 100644
> > --- a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
> > +++ b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml
> > @@ -152,6 +152,7 @@
> >
> >
> >
> > +  
> >2005094
> >0
> >
> > diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml 
> > b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml
> > index 50506ba..5f93d59 100644
> > --- a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml
> > +++ 

Re: [libvirt] [PATCH 07/11] qemu_command: separate code for video device via -vga attribute

[Pavel Hrdina]

On Sat, Oct 08, 2016 at 10:01:14AM -0400, John Ferlan wrote:
> 
> 
> On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/qemu/qemu_command.c | 140 
> > ++--
> >  1 file changed, 76 insertions(+), 64 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index d283c67..aef8c79 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -4371,6 +4371,81 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
> >  
> >  
> >  static int
> > +qemuBuildVgaVideoCommand(virCommandPtr cmd,
> > + const virDomainDef *def,
> 
> Instead of *def here, you could have
> 
> const virDomainVideoDefPtr video,
> 
> and then replace all the "def->videos[0]" with video
> 
> > + virQEMUCapsPtr qemuCaps)
> > +{
> > +const char *vgastr = qemuVideoTypeToString(def->videos[0]->type);
> > +if (!vgastr || STREQ(vgastr, "")) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("video type %s is not supported with QEMU"),
> > +   virDomainVideoTypeToString(def->videos[0]->type));
> > +return -1;
> > +}
> > +
> > +virCommandAddArgList(cmd, "-vga", vgastr, NULL);
> > +
> > +/* If we cannot use --device option to specify the video device
> > + * in QEMU we will fallback to the old --vga option. To get the
> > + * correct device name for the --vga option the 'qemuVideo' is
> > + * used, but to set some device attributes we need to use the
> > + * --global option and for that we need to specify the device
> > + * name the same as for --device option and for that we need to
> > + * use 'qemuDeviceVideo'.
> > + *
> > + * See 'Graphics Devices' section in docs/qdev-device-use.txt in
> > + * QEMU repository.
> > + */
> > +const char *dev = qemuDeviceVideoTypeToString(def->videos[0]->type);
> 
> While we're at moving things around - can the "const char *dev... " be
> defined at the top and not in the middle somewhere?
> 
> > +
> > +if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
> > +(def->videos[0]->vram || def->videos[0]->ram)) {
> > +unsigned int ram = def->videos[0]->ram;
> > +unsigned int vram = def->videos[0]->vram;
> > +unsigned int vram64 = def->videos[0]->vram64;
> > +unsigned int vgamem = def->videos[0]->vgamem;
> > +
> > +if (ram) {
> > +virCommandAddArg(cmd, "-global");
> > +virCommandAddArgFormat(cmd, "%s.ram_size=%u",
> > +   dev, ram * 1024);
> > +}
> > +if (vram) {
> > +virCommandAddArg(cmd, "-global");
> > +virCommandAddArgFormat(cmd, "%s.vram_size=%u",
> > +   dev, vram * 1024);
> > +}
> > +if (vram64 &&
> > +virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VRAM64)) {
> > +virCommandAddArg(cmd, "-global");
> > +virCommandAddArgFormat(cmd, "%s.vram64_size_mb=%u",
> > +   dev, vram64 / 1024);
> > +}
> > +if (vgamem &&
> > +virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGAMEM)) {
> > +virCommandAddArg(cmd, "-global");
> > +virCommandAddArgFormat(cmd, "%s.vgamem_mb=%u",
> > +   dev, vgamem / 1024);
> > +}
> > +}
> > +
> > +if (def->videos[0]->vram &&
> > +((def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
> > +  virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) ||
> > + (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
> > +  virQEMUCapsGet(qemuCaps, QEMU_CAPS_VMWARE_SVGA_VGAMEM {
> > +unsigned int vram = def->videos[0]->vram;
> > +
> > +virCommandAddArg(cmd, "-global");
> > +virCommandAddArgFormat(cmd, "%s.vgamem_mb=%u",
> > +   dev, vram / 1024);
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +
> > +static int
> >  qemuBuildVideoCommandLine(virCommandPtr cmd,
> >const virDomainDef *def,
> >virQEMUCapsPtr qemuCaps)
> > @@ -4398,71 +4473,8 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
> >  if (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_XEN) {
> 
> I was going to suggest something regarding removing primaryVideoType,
> but I see the next patch does more overhaul...
> 
> >  /* nothing - vga has no effect on Xen pvfb */
> >  } else {
> > -const char *vgastr = qemuVideoTypeToString(primaryVideoType);
> > -if (!vgastr || STREQ(vgastr, "")) {
> > -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > -   _("video type %s is not supported with 
> > QEMU"),
> > -   

Re: [libvirt] [PATCH 06/11] qemu_capabilities: rename QEMU_CAPS_VIRTIO_GPU_VIRGL

[Pavel Hrdina]

On Sat, Oct 08, 2016 at 09:59:20AM -0400, John Ferlan wrote:
> 
> 
> On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
> > We usually uses QEMU_CASP_DEVICE_$NAME to probe for existence of some
> 
> libvirt generally uses QEMU_CAPS_DEVICE_$NAME
> 
> > device and QEMU_CAPS_$NAME_$PROP to probe for existence of some property
> > of that device.
> > 
> > This also remove wrong assumption for QEMU_CAPS_DEVICE_VIRTIO_GPU that
> > it's also 'virtio-vga' which isn't true.
> 
> This will remove the wrong ...
> 
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/qemu/qemu_capabilities.c   |  4 +-
> >  src/qemu/qemu_capabilities.h   |  4 +-
> >  src/qemu/qemu_process.c|  2 +-
> >  .../qemucapabilitiesdata/caps_1.2.2.x86_64.replies | 30 ++
> >  .../qemucapabilitiesdata/caps_1.3.1.x86_64.replies | 28 ++
> >  .../qemucapabilitiesdata/caps_1.4.2.x86_64.replies | 30 ++
> >  .../qemucapabilitiesdata/caps_1.5.3.x86_64.replies | 30 ++
> >  .../qemucapabilitiesdata/caps_1.6.0.x86_64.replies | 30 ++
> >  .../qemucapabilitiesdata/caps_1.7.0.x86_64.replies | 30 ++
> >  .../qemucapabilitiesdata/caps_2.1.1.x86_64.replies | 32 +++
> >  .../qemucapabilitiesdata/caps_2.4.0.x86_64.replies | 54 ++
> >  .../qemucapabilitiesdata/caps_2.5.0.x86_64.replies | 64 
> > ++
> >  .../caps_2.6.0-gicv2.aarch64.replies   | 35 
> >  .../caps_2.6.0-gicv3.aarch64.replies   | 35 
> >  .../caps_2.6.0.ppc64le.replies | 35 
> >  .../qemucapabilitiesdata/caps_2.6.0.x86_64.replies | 64 
> > ++
> >  .../qemucapabilitiesdata/caps_2.7.0.x86_64.replies | 64 
> > ++
> >  tests/qemuxml2argvtest.c   |  4 +-
> >  18 files changed, 444 insertions(+), 131 deletions(-)
> > 
> 
> I think there's two things going on here... A name change wouldn't
> affect the replies files...

OK, I'll stop hiding two changes into one patch.

> 
> [ahh, umm - I just read patch 9... I think there's some synergies there
> - glad I waited before sending all my comments...  But I didn't change
> anything below...]
> 
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index ae83317..5449b07 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -1676,7 +1676,7 @@ static struct virQEMUCapsStringFlags 
> > virQEMUCapsObjectPropsQxl[] = {
> >  };
> >  
> >  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioGpu[] = {
> > -{ "virgl", QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL },
> > +{ "virgl", QEMU_CAPS_VIRTIO_GPU_VIRGL },
> >  };
> >  
> >  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsICH9[] = {
> > @@ -1745,6 +1745,8 @@ static struct virQEMUCapsObjectTypeProps 
> > virQEMUCapsObjectProps[] = {
> >ARRAY_CARDINALITY(virQEMUCapsObjectPropsQxl) },
> >  { "virtio-gpu-pci", virQEMUCapsObjectPropsVirtioGpu,
> >ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioGpu) },
> > +{ "virtio-gpu-device", virQEMUCapsObjectPropsVirtioGpu,
> > +  ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioGpu) },
> 
> Should this be a separate bug fix?  Since this is less renaming and more
> adding something that wasn't there and causes all the change to the
> qemucapabilitiesdata replies files.
> 
> The "virtio-gpu" cap was added with commit id '21373feb' - it added
> "virtio-gpu-pci" and "virtio-gpu-device" to virQEMUCapsObjectTypes.
> 
> The "virtio-gcpu.virgl" cap was added with commit id '06198b9c' - it
> added the virQEMUCapsObjectPropsVirtioGpu and added just
> "virtio-gpu-pci" to virQEMUCapsObjectProps.
> 
> So it seems you are indicating it should have also added
> "virtio-gpu-device" to that table - is that what's going on here?
> 
> I think it should be separate and clearly stated.

I guess that I was just too lazy to make a separate patch for that.

> >  { "ICH9-LPC", virQEMUCapsObjectPropsICH9,
> >ARRAY_CARDINALITY(virQEMUCapsObjectPropsICH9) },
> >  { "virtio-balloon-pci", virQEMUCapsObjectPropsVirtioBalloon,
> > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> > index 2effcb7..1766aa6 100644
> > --- a/src/qemu/qemu_capabilities.h
> > +++ b/src/qemu/qemu_capabilities.h
> > @@ -328,8 +328,8 @@ typedef enum {
> >  
> >  /* 200 */
> >  QEMU_CAPS_INCOMING_DEFER, /* -incoming defer and migrate_incoming */
> > -QEMU_CAPS_DEVICE_VIRTIO_GPU, /* -device virtio-gpu-* & virtio-vga */
> > -QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL, /* -device virtio-gpu-*.virgl */
> > +QEMU_CAPS_DEVICE_VIRTIO_GPU, /* -device virtio-gpu-* */
> > +QEMU_CAPS_VIRTIO_GPU_VIRGL, /* -device virtio-gpu-*.virgl */
> 
> Any specific reason to remove the & virtio-vga comment?  As I read
> commit id '21373feb' it says "It can be used with -device virtio-vga for
> 

Re: [libvirt] [PATCH 05/11] qemu_process: move video validation out of qemu_command

[Pavel Hrdina]

On Sat, Oct 08, 2016 at 09:58:15AM -0400, John Ferlan wrote:
> 
> 
> On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
> > Runtime validation that depend on qemu capabilities should be moved
> > into qemuProcessStartValidateXML.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/qemu/qemu_command.c  | 33 +---
> >  src/qemu/qemu_process.c  | 50 ++-
> >  tests/qemuxml2argvtest.c | 78 
> > +---
> >  3 files changed, 98 insertions(+), 63 deletions(-)
> > 
> 
> Could the qemuxml2argvtest.c to add QEMU_CAPS_DEVICE_CIRRUS_VGA move to
> earlier? Perhaps there is a relationship, but it's not clear from the
> commit message...

The changes to qemuxml2argvtest.c can be moved to separate patch, the only
relationship is that the movement of validation code depends on the changes
to qemuxml2argvtest.c because this patch adds the validation for secondary
video devices too.

> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 761968b..d283c67 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -4312,26 +4312,12 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
> >  model = "virtio-gpu-pci";
> >  }
> >  } else {
> > -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) {
> > -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > -   "%s", _("only one video card is currently 
> > supported"));
> > -goto error;
> > -}
> > -
> 
> Just checking - does the removal of this end up as part of the (now)
> singular video loop in qemuProcessStartValidateVideo?  Same for the
> removed check in qemuBuildVideoCommandLine?
> 
> It wasn't "clear" the first pass through reading...

That exact error message is replaced by different error message:

"this QEMU does not support '%s' video device"

Currently only QXL can be used as secondary video device so if QXL device is not
supported by QEMU it also implicates that only one video card is supported.

> >  model = "qxl";
> >  }
> >  
> >  virBufferAsprintf(, "%s,id=%s", model, video->info.alias);
> >  
> >  if (video->accel && video->accel->accel3d == VIR_TRISTATE_SWITCH_ON) {
> > -if (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO ||
> > -!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL)) {
> > -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > -   _("%s 3d acceleration is not supported"),
> > -   virDomainVideoTypeToString(video->type));
> > -goto error;
> > -}
> > -
> >  virBufferAsprintf(, ",virgl=%s",
> >
> > virTristateSwitchTypeToString(video->accel->accel3d));
> >  }
> > @@ -4397,17 +4383,7 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
> >  
> >  primaryVideoType = def->videos[0]->type;
> >  
> > -if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY) &&
> > - ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VGA &&
> > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) ||
> > - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_CIRRUS &&
> > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) ||
> > - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
> > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) ||
> > - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL &&
> > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) ||
> > - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
> > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU {
> > +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) {
> >  for (i = 0; i < def->nvideos; i++) {
> >  char *str;
> >  virCommandAddArg(cmd, "-device");
> > @@ -4422,13 +4398,6 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
> >  if (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_XEN) {
> >  /* nothing - vga has no effect on Xen pvfb */
> >  } else {
> > -if ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL) &&
> > -!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) {
> > -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > -   _("This QEMU does not support QXL graphics 
> > adapters"));
> > -return -1;
> > -}
> > -
> 
> ... Is it safe to assume there can only be one video card here?

Where did you get the assumption from?

> >  const char *vgastr = qemuVideoTypeToString(primaryVideoType);
> >  if (!vgastr || STREQ(vgastr, "")) {
> >  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 6ddcc1f..eb3add9 100644
> > --- a/src/qemu/qemu_process.c
> > +++ 

Re: [libvirt] Analysis of the effect of adding PCIe root ports

[Andrea Bolognani]

On Fri, 2016-10-07 at 12:11 -0400, Laine Stump wrote:
> > So instead of guaranteeing that there will always be an empty
> > slot available for hotplug during a single start/destroy
> > cycle of the guest, we would be guaranteeing that there will
> > be 3/4 empty slots available for either hotplug or coldplug
> > throughout the entire life of the guest.
> 
> A better way to put it is that we guarantee there will be "N" (3 or 4 or 
> whatever) slots available when the domain is originally defined. Once 
> any change has been made, all bets are off.

Okay, your alternative reading matches with my understanding
of your proposal :)

> > Sounds like a pretty good compromise to me.
> > 
> > The only problem I can think of is that there might be
> > management applications that add eg. a pcie-root in the XML
> > when first defining a guest, and after the change such guests
> > would get zero hotpluggable ports. Then again it's probably
> > okay to expect such management applications to add the
> > necessary number of pcie-root-ports themselves.
> 
> Yeah, if they know enough to be adding a root-port, then they know 
> enough to add extras.

Note that I wrote pcie-root, not pcie-root-port.

> > Maybe we could relax the wording on A) and ignore any
> > pci{,e}-root? Even though there is really no reason for
> > either a user or a management application to add them
> > explicitly when defining a guest, I feel like they might be
> > special enough to deserve an exception.
> 
> I thought about that; I'm not sure. In the case of libguestfs, even if 
> Rich added a pcie-root, I guess he would still be manually addressing 
> his pci devices, so that would be clue enough that he knew what he was 
> doing and didn't want any extra.

Yeah, I'm not sure about that either, I just felt like it
would be good to think about it. I don't really see problems
going one way or the other, and I guess your initial
proposal is slightly more strict, so we might as well go for
it and relax it later if a compelling use case is found.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2 00/20] Split parsing and defining logic of daemon's logging

[Daniel P. Berrange]

On Mon, Oct 10, 2016 at 08:55:29AM +0200, Erik Skultety wrote:
> > Erik Skultety (20):
> >   virlog: Rename virLogParse* to virLogParseAndDefine*
> >   virlog: Introduce virLogOutputNew
> >   virlog: Introduce virLogFilterNew
> >   virlog: Introduce virLogFindOutput
> >   virlog: Introduce virLogDefineOutputs
> >   virlog: Introduce virLogDefineFilters
> >   virlog: Introduce virLogNewOutputTo* as a replacement for
> > virLogAddOutputTo*
> >   virlog: Take a special care of syslog when setting new set of log
> > outputs
> >   virlog: Introduce virLogParseOutput
> >   virlog: Introduce virLogParseFilter
> >   virlog: Introduce virLogParseOutputs
> >   virlog: Introduce virLogParseFilters
> >   virlog: Introduce virLogSetOutputs
> >   virlog: Introduce virLogSetFilters
> >   daemon: Split output parsing and output defining
> >   daemon: Split filter parsing and filter defining
> >   virlog: Remove functions that aren't used anywhere anymore
> >   virlog: Make some of the methods static
> >   virlog: Store the journald fd within the output object
> >   virlog: Split parsing and setting priority
> > 
> >  daemon/libvirtd.c |8 +-
> >  src/libvirt_private.syms  |   10 +-
> >  src/locking/lock_daemon.c |8 +-
> >  src/logging/log_daemon.c  |8 +-
> >  src/util/virlog.c | 1079 
> > ++---
> >  src/util/virlog.h |   61 +--
> >  tests/eventtest.c |3 +-
> >  tests/testutils.c |   11 +-
> >  tests/virlogtest.c|   10 +-
> >  9 files changed, 702 insertions(+), 496 deletions(-)
> > 
> > -- 
> > 2.5.5
> > 
> 
> So, I made all the requested adjustments, moved patch 19 to in between 1 and 
> 2,
> dropped patch 18 completely, kept the introduction of Set{Filters,Outputs} and
> actually splitting the logic in separate patches, replaced all the suggested
> checks for NULL for ATTRIBUTE_NONNULL, adjusted the callers appropriately and
> tested several scenarios to make sure the daemon doesn't crash and pushed the
> patches. Anyways, thanks for reviewing the series...aaand brace yourselves -
> an increased number of BZs related to logging is coming.

This has broken the build on Mingw

../../src/util/virlog.c: In function 'virLogDefineOutputs':
../../src/util/virlog.c:1377:32: error: 'current_ident' undeclared (first use 
in this function)
current_ident)) != -1) {
^
../../src/util/virlog.c:1377:32: note: each undeclared identifier is reported 
only once for each function it appears in
../../src/util/virlog.c:1386:9: error: implicit declaration of function 
'openlog' [-Werror=implicit-function-declaration]
 openlog(current_ident, 0, 0);
 ^~~
../../src/util/virlog.c:1386:9: error: nested extern declaration of 'openlog' 
[-Werror=nested-externs]
cc1: all warnings being treated as errors


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

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

Re: [libvirt] [PATCH] vz: fix vnc host address

[Mikhail Feoktistov]

On 10.10.2016 11:49, Michal Privoznik wrote:

On 08.10.2016 00:05, Mikhail Feoktistov wrote:

Empty string means 127.0.0.1 in terms of vzSDK
But in this case we should set 0.0.0.0
---
  .gnulib | 2 +-
  src/vz/vz_sdk.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/.gnulib b/.gnulib
index e89b4a7..a2a3943 16
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit e89b4a7aefce9cb02963920712ba7cdd13641644
+Subproject commit a2a39436b65f329630df4a93ec4e30aeae403c54
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index f2a5c96..7011cbf 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -2967,7 +2967,7 @@ static int prlsdkApplyGraphicsParams(PRL_HANDLE sdkdom,
  
  glisten = virDomainGraphicsGetListen(gr, 0);

  pret = PrlVmCfg_SetVNCHostName(sdkdom, glisten && glisten->address ?
-   glisten->address : "");
+   glisten->address : "0.0.0.0");
  prlsdkCheckRetGoto(pret, cleanup);
  
  ret = 0;



Is this true? I mean, in qemu by default vnc/spice listens on the
localhost and not "0.0.0.0". It's more safe that way. Only if you
configure it to listen on "0.0.0.0" you'll get a widely available socket.

I mean for the following XML:

 
   
 

I'd expect the guest to listen to 127.0.0.1. Or is this approached
differently in vz driver?

Michal

Sorry, I see that I send this patch too early.
I need to research more about vz driver behavior.
Please ignore this patch, I'll send another.

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

Re: [libvirt] libvirt-guest.sh bug fixes

[Michal Privoznik]

On 07.10.2016 15:56, Stefan Bader wrote:
> Two small changes, before I forget about submitting them...
> 
> First one affects all environments the same. The list of UIDs which
> is generated has each element on a separate line. And using quotes
> in the echo preserves those newlines. However the processing assumes
> one line per URI and all UIDs separated by spaces. So without dropping
> the quotes only one guest will get shutdown/suspended.
> 
> The second change is for Xen environments only. Domain-0 appears in
> the list of guests and it is a persistent one. So on shutdown, the
> script tries to stop Domain-0 (which is not working) and then waits
> the whole timeout for it to stop.
> 
> -Stefan
> 

Eric ACKed both of them and I've just pushed them.

Michal

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

Re: [libvirt] [PATCH 01/11] tests: fix some QXL capability combinations that doesn't make sense

[Pavel Hrdina]

On Sat, Oct 08, 2016 at 09:52:24AM -0400, John Ferlan wrote:
> s/doesn't/don't
> 
> On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
> > If one of QEMU_CAPS_DEVICE_QXL_VGA or QEMU_CAPS_DEVICE_QXL is set the
> > other one will always be set as well because both devices are tied
> > together in QEMU.
> 
> And subsequent patches are about to make this a whole lot clearer, but
> in order to do so, we need to make sure the test output displays the
> combination of both properly especially with the capability
> QEMU_CAPS_DEVICE_VIDEO_PRIMARY
> 
> > 
> > The change of args files is caused by the presence of capability
> > QEMU_CAPS_DEVICE_VIDEO_PRIMARY which means it's safe to use
> > "-device qxl-vga" instead of "-vga qxl".
> 
> IOW: The fallout for doing so when the *VIDEO_PRIMARY capability is set
> is that the command line generated changes.

Yes, this capability exists only to use -vga instead of -device because there
was a bug in qemu that the mouse cursor was missing while using
"-device qxl-vga", see commit e3f2686b.
 
> [Text added to make sure I understand]
> 
> 
> I note it only seems to change the .args output for q35 and pcie. Even
> though graphics-spice* tests added the cap, their output didn't change.
> I assume because the *VIDEO_PRIMARY wasn't set, but figured I'd ask.
> 
> I guess I'm just making sure it's nothing specific to q35/pcie...

No it's not related to q35/pcie, it's just coincident that those tests are
affected.  The reason is that there was only QEMU_CAPS_VGA_QXL set.  Since
we support qemu with "-device" option those tests capabilities was invalid
and in real world QEMU_CAPS_DEVICE_QXL_VGA and QEMU_CAPS_DEVICE_QXL would
be set as well.

I'll add it to the commit message to make it clear.

> 
> I know the world is about to change some more ;-)
> 
> Does anything need to change on formatdomain.html.in?  I know this is a
> qemu specific change, but I'm thinking more in terms of describing how
> to use the type param for qga, qxl (primary/secondary)...

No this is only internal change.

Pavel

> > Signed-off-by: Pavel Hrdina 
> > ---
> >  .../qemuxml2argv-pcie-root-port.args   |  5 ++-
> >  .../qemuxml2argv-pcie-switch-downstream-port.args  |  5 ++-
> >  .../qemuxml2argv-pcie-switch-upstream-port.args|  5 ++-
> >  .../qemuxml2argv-pcihole64-q35.args|  5 ++-
> >  .../qemuxml2argv-q35-usb2-multi.args   |  5 ++-
> >  .../qemuxml2argv-q35-usb2-reorder.args |  5 ++-
> >  tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.args  |  5 ++-
> >  tests/qemuxml2argvdata/qemuxml2argv-q35.args   |  5 ++-
> >  tests/qemuxml2argvtest.c   | 42 
> > +++---
> >  tests/qemuxml2xmltest.c| 23 +++-
> >  10 files changed, 59 insertions(+), 46 deletions(-)
> > 
> 
> ACK in principle... Now that I've read all the patches - I see that the
> absolute key in the VIDEO_PRIMARY capability - totally changes how the
> command line is built especially since we're not falling into that else
> condition which forces the specific (and I assume) sub-optimal video
> model usage.
> 
> John
> 
> > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args 
> > b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args
> > index 35c2664..27d5164 100644
> > --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args
> > +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args
> > @@ -22,6 +22,5 @@ QEMU_AUDIO_DRV=none \
> >  -device ioh3420,port=0x1a,chassis=40,id=pci.4,bus=pcie.0,addr=0x3 \
> >  -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-sata0-0-0 \
> >  -device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \
> > --vga qxl \
> > --global qxl-vga.ram_size=67108864 \
> > --global qxl-vga.vram_size=33554432
> > +-device qxl-vga,id=video0,ram_size=67108864,vram_size=33554432,bus=pcie.0,\
> > +addr=0x1
> > diff --git 
> > a/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args 
> > b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args
> > index 500b366..3b3e80d 100644
> > --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args
> > +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args
> > @@ -30,6 +30,5 @@ QEMU_AUDIO_DRV=none \
> >  -device 
> > xio3130-downstream,port=0x6,chassis=12,id=pci.12,bus=pci.4,addr=0x6 \
> >  -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-sata0-0-0 \
> >  -device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \
> > --vga qxl \
> > --global qxl-vga.ram_size=67108864 \
> > --global qxl-vga.vram_size=33554432
> > +-device qxl-vga,id=video0,ram_size=67108864,vram_size=33554432,bus=pcie.0,\
> > +addr=0x1
> > diff --git 
> > a/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args 
> > b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args
> > index 24d5f37..93d16b8 100644
> > --- 

Re: [libvirt] [PATCH] vz: fix vnc host address

[Michal Privoznik]

On 08.10.2016 00:05, Mikhail Feoktistov wrote:
> Empty string means 127.0.0.1 in terms of vzSDK
> But in this case we should set 0.0.0.0
> ---
>  .gnulib | 2 +-
>  src/vz/vz_sdk.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/.gnulib b/.gnulib
> index e89b4a7..a2a3943 16
> --- a/.gnulib
> +++ b/.gnulib
> @@ -1 +1 @@
> -Subproject commit e89b4a7aefce9cb02963920712ba7cdd13641644
> +Subproject commit a2a39436b65f329630df4a93ec4e30aeae403c54
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index f2a5c96..7011cbf 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -2967,7 +2967,7 @@ static int prlsdkApplyGraphicsParams(PRL_HANDLE sdkdom,
>  
>  glisten = virDomainGraphicsGetListen(gr, 0);
>  pret = PrlVmCfg_SetVNCHostName(sdkdom, glisten && glisten->address ?
> -   glisten->address : "");
> +   glisten->address : "0.0.0.0");
>  prlsdkCheckRetGoto(pret, cleanup);
>  
>  ret = 0;
> 

Is this true? I mean, in qemu by default vnc/spice listens on the
localhost and not "0.0.0.0". It's more safe that way. Only if you
configure it to listen on "0.0.0.0" you'll get a widely available socket.

I mean for the following XML:


  


I'd expect the guest to listen to 127.0.0.1. Or is this approached
differently in vz driver?

Michal

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

Re: [libvirt] [PATCH 2/2 RFC] Purge marked callbacks before dispatching events

[Michal Privoznik]

On 07.10.2016 19:52, Martin Kletzander wrote:
> I don't have any justification for this except an empirical one: with
> this patch the code from Bug 1363628 doesn't crash after "leaking".
> 
> I currently don't have properly working valgrind, but I'm working on it
> and it might shed some light into this (although it might also not
> happen due to the slowdown).
> 
> However in the meantime I attempted some analysis and I got even more
> confused than before, I guess.  With this patch the code doesn't crash,
> even though virObjectEventStateQueueDispatch() properly skips callbacks
> marked as deleted.  What I feel is even more weird, that if I duplicate
> the purgatory function (instead of moving it), it crashes again, and I
> feel like even more often.
> 
> Weirdly-fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1363628
> 
> Signed-off-by: Martin Kletzander 
> ---
>  src/conf/object_event.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/conf/object_event.c b/src/conf/object_event.c
> index e5af4be68a7e..4066b4673b42 100644
> --- a/src/conf/object_event.c
> +++ b/src/conf/object_event.c
> @@ -821,13 +821,13 @@ virObjectEventStateFlush(virObjectEventStatePtr state)
>  if (state->timer != -1)
>  virEventUpdateTimeout(state->timer, -1);
> 
> +/* Purge any deleted callbacks */
> +virObjectEventCallbackListPurgeMarked(state->callbacks);
> +
>  virObjectEventStateQueueDispatch(state,
>   ,
>   state->callbacks);
> 
> -/* Purge any deleted callbacks */
> -virObjectEventCallbackListPurgeMarked(state->callbacks);
> -
>  state->isDispatching = false;
>  virObjectEventStateUnlock(state);
>  }

Well, I have no idea either, because virObjectEventStateQueueDispatch()
calls virObjectEventStateDispatchCallbacks() which in turn calls
virObjectEventDispatchMatchCallback() which returns false if the
callback->deleted is truem and thus the callback is skipped - just like
it is after this patch of yours.

This whole bug feels like a refcounting problem and IMO your patch is
just making the scenario more unlikely.

Michal

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

Re: [libvirt] [PATCH 1/2] Don't update timer if there's none.

[Michal Privoznik]

On 07.10.2016 19:52, Martin Kletzander wrote:
> Sometimes virObjectEventStateFlush can be called without timer (if the
> last event was unregistered right when the timer fired).  There is a
> check for timer == -1, but that triggers warning and other log messages,
> which is unnecessary.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  src/conf/object_event.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/object_event.c b/src/conf/object_event.c
> index b5a6a81a3a04..e5af4be68a7e 100644
> --- a/src/conf/object_event.c
> +++ b/src/conf/object_event.c
> @@ -818,7 +818,8 @@ virObjectEventStateFlush(virObjectEventStatePtr state)
>  tempQueue.events = state->queue->events;
>  state->queue->count = 0;
>  state->queue->events = NULL;
> -virEventUpdateTimeout(state->timer, -1);
> +if (state->timer != -1)
> +virEventUpdateTimeout(state->timer, -1);
> 
>  virObjectEventStateQueueDispatch(state,
>   ,
> 

ACK.

Michal

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

Re: [libvirt] [PATCH] Add 2 systemtap script in EXTRA_DIST

[Luyao Huang]

Nice idea! Thanks for your help and review.

Have a nice day!
Luyao

- Original Message -
From: "Michal Privoznik" 
To: "Luyao Huang" , libvir-list@redhat.com
Sent: Monday, October 10, 2016 3:02:58 PM
Subject: Re: [libvirt] [PATCH] Add 2 systemtap script in EXTRA_DIST

On 10.10.2016 11:42, Luyao Huang wrote:
> Then we will include these 2 useful script in VPATH build.
> 
> Signed-off-by: Luyao Huang 
> ---
>  examples/Makefile.am | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/examples/Makefile.am b/examples/Makefile.am
> index bd8460d..ca6e039 100644
> --- a/examples/Makefile.am
> +++ b/examples/Makefile.am
> @@ -28,6 +28,8 @@ EXTRA_DIST = \
>   lxcconvert/virt-lxc-convert \
>   polkit/libvirt-acl.rules \
>   systemtap/events.stp \
> + systemtap/lock-debug.stp \
> + systemtap/qemu-monitor.stp \
>   systemtap/rpc-monitor.stp \
>   $(FILTERS) \
>   $(wildcard $(srcdir)/xml/storage/*.xml) \
> 

This works, byt we can also just include systemtap/*.stp and distribute
whatever systemtap scrip we ever come up with.

Therefore, I'm squashing this in:

-   systemtap/events.stp \
-   systemtap/lock-debug.stp \
-   systemtap/qemu-monitor.stp \
-   systemtap/rpc-monitor.stp \
+   $(wildcard $(srcdir)/systemtap/*.stp) \

ACKing and pushing. Thanks.

Michal

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

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

Re: [libvirt] [PATCH] Add 2 systemtap script in EXTRA_DIST

[Michal Privoznik]

On 10.10.2016 11:42, Luyao Huang wrote:
> Then we will include these 2 useful script in VPATH build.
> 
> Signed-off-by: Luyao Huang 
> ---
>  examples/Makefile.am | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/examples/Makefile.am b/examples/Makefile.am
> index bd8460d..ca6e039 100644
> --- a/examples/Makefile.am
> +++ b/examples/Makefile.am
> @@ -28,6 +28,8 @@ EXTRA_DIST = \
>   lxcconvert/virt-lxc-convert \
>   polkit/libvirt-acl.rules \
>   systemtap/events.stp \
> + systemtap/lock-debug.stp \
> + systemtap/qemu-monitor.stp \
>   systemtap/rpc-monitor.stp \
>   $(FILTERS) \
>   $(wildcard $(srcdir)/xml/storage/*.xml) \
> 

This works, byt we can also just include systemtap/*.stp and distribute
whatever systemtap scrip we ever come up with.

Therefore, I'm squashing this in:

-   systemtap/events.stp \
-   systemtap/lock-debug.stp \
-   systemtap/qemu-monitor.stp \
-   systemtap/rpc-monitor.stp \
+   $(wildcard $(srcdir)/systemtap/*.stp) \

ACKing and pushing. Thanks.

Michal

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

Re: [libvirt] [PATCH v2 00/20] Split parsing and defining logic of daemon's logging

[Erik Skultety]

> Erik Skultety (20):
>   virlog: Rename virLogParse* to virLogParseAndDefine*
>   virlog: Introduce virLogOutputNew
>   virlog: Introduce virLogFilterNew
>   virlog: Introduce virLogFindOutput
>   virlog: Introduce virLogDefineOutputs
>   virlog: Introduce virLogDefineFilters
>   virlog: Introduce virLogNewOutputTo* as a replacement for
> virLogAddOutputTo*
>   virlog: Take a special care of syslog when setting new set of log
> outputs
>   virlog: Introduce virLogParseOutput
>   virlog: Introduce virLogParseFilter
>   virlog: Introduce virLogParseOutputs
>   virlog: Introduce virLogParseFilters
>   virlog: Introduce virLogSetOutputs
>   virlog: Introduce virLogSetFilters
>   daemon: Split output parsing and output defining
>   daemon: Split filter parsing and filter defining
>   virlog: Remove functions that aren't used anywhere anymore
>   virlog: Make some of the methods static
>   virlog: Store the journald fd within the output object
>   virlog: Split parsing and setting priority
> 
>  daemon/libvirtd.c |8 +-
>  src/libvirt_private.syms  |   10 +-
>  src/locking/lock_daemon.c |8 +-
>  src/logging/log_daemon.c  |8 +-
>  src/util/virlog.c | 1079 
> ++---
>  src/util/virlog.h |   61 +--
>  tests/eventtest.c |3 +-
>  tests/testutils.c |   11 +-
>  tests/virlogtest.c|   10 +-
>  9 files changed, 702 insertions(+), 496 deletions(-)
> 
> -- 
> 2.5.5
> 

So, I made all the requested adjustments, moved patch 19 to in between 1 and 2,
dropped patch 18 completely, kept the introduction of Set{Filters,Outputs} and
actually splitting the logic in separate patches, replaced all the suggested
checks for NULL for ATTRIBUTE_NONNULL, adjusted the callers appropriately and
tested several scenarios to make sure the daemon doesn't crash and pushed the
patches. Anyways, thanks for reviewing the series...aaand brace yourselves -
an increased number of BZs related to logging is coming.

Erik


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

Re: [libvirt] [PATCH] Forbid new-line char in name of networks

[Sławek Kapłoński]

Hello,

Ok. Thx for review. I will try today make new function for it and also
will look for how arguments should be splitted in multiple lines.

-- 
Best regards / Pozdrawiam
Sławek Kapłoński
sla...@kaplonski.pl

On Mon, 10 Oct 2016, Peter Krempa wrote:

> On Mon, Oct 10, 2016 at 04:53:15 +0200, Michal Privoznik wrote:
> > On 08.10.2016 23:38, Sławek Kapłoński wrote:
> > > New line character in name of network is now forbidden because it
> > > mess virsh output and can be confusing for users.
> > > 
> > > Closes-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=818064
> > > ---
> > >  src/conf/network_conf.c | 7 +++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> > > index aa39776..f9c537f 100644
> > > --- a/src/conf/network_conf.c
> > > +++ b/src/conf/network_conf.c
> > > @@ -2123,6 +2123,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> > >  goto error;
> > >  }
> > >  
> > > +if (strchr(def->name, '\n')) {
> > > +virReportError(
> > > +VIR_ERR_XML_ERROR,
> > > +_("name %s cannot contain new-line character"), def->name);
> 
> Please note that this is not our expected style of breaking up
> arguments.
> 
> > > +goto error;
> > > +}
> > > +
> > >  /* Extract network uuid */
> > >  tmp = virXPathString("string(./uuid[1])", ctxt);
> > >  if (!tmp) {
> > > 
> > 
> > Okay, this makes sense. We can put reasonable restrictions on object
> > names. However, I think while we are at this we can fix the same problem
> > in other areas of the code. For instance, storage pools and volumes
> > suffer from the same problem. Now, instead of copying this chunk all
> > over the places, what about doing this more wisely?
> > 
> > For instance:
> > 
> > char *c;
> > ...
> > /* Parse name from XML */
> > 
> > if ((c = strpbrk(def->name, "/\n")) {
> > /* Insert your favourite character here ^^ */
> > virReportError(VIR_ERR_XML_DETAIL,
> > _("invalid char in name: %c"), *c);
> > return -1; /* goto cleanup */
> > }
> > 
> > This way, we can just add new character to the string when somebody
> > files a bug.
> > Moreover, if we'd wrap those couple of lines into a function, we could
> > just do:
> > 
> > if (!virXMLCheckName(def->name, "/\n"))
> >return -1;
> > 
> > Let me know if you have any troubles writing this, or if you disagree.
> 
> The problem with making the parser stricter is that it's prone to create
> regressions for people that might have used such configuration.
> 
> As this was previously working (except for the broken virsh interface)
> we should limit such names only when defining a network/vm/whatever.
> 
> Peter



> --
> 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