On 09/24/2014 08:53 PM, Michal Privoznik wrote:
On 24.09.2014 14:00, Jincheng Miao wrote:
On 09/24/2014 07:40 PM, Michal Privoznik wrote:
On 24.09.2014 07:45, Jincheng Miao wrote:
In nodeGetFreePages, if startCell is given by '0',
and the max node number is '0' too. The for-loop
wouldn't be executed.
So convert it to while-loop.
Before:
virsh freepages --cellno 0 --pagesize 4
error: internal error: no suitable info found
After:
virsh freepages --cellno 0 --pagesize 4
4KiB: 472637
Signed-off-by: Jincheng Miao
---
src/nodeinfo.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 2459922..1fe190a 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -2042,18 +2042,19 @@ nodeGetFreePages(unsigned int npages,
}
lastCell = MIN(lastCell, startCell + cellCount);
+cell = startCell;
-for (cell = startCell; cell < lastCell; cell++) {
+do {
for (i = 0; i < npages; i++) {
unsigned int page_size = pages[i];
unsigned int page_free;
-if (virNumaGetPageInfo(cell, page_size, 0, NULL,
&page_free) < 0)
+if (virNumaGetPageInfo(cell++, page_size, 0, NULL,
&page_free) < 0)
goto cleanup;
counts[ncounts++] = page_free;
}
-}
+} while (cell < lastCell);
if (!ncounts) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
There's no need to switch to do-while loop. All what is needed is cell
<= lastCell in the for loop. I'm rewriting the patch and pushing. ACK.
Wait, if change the for condition to 'cell <= lastCell', and pass
startCell == -1,
the for-loop will execute twice, and will overwrite counts[1] which is
not allocated.
Oh, right. Seems like I'm brainwashed today. This is the diff I've
forgot to 'git add':
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 0a7642c..05eab6c 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -2041,7 +2041,7 @@ nodeGetFreePages(unsigned int npages,
goto cleanup;
}
-lastCell = MIN(lastCell, startCell + cellCount);
+lastCell = MIN(lastCell, startCell + cellCount - 1);
Yep, this is working now with this adjustment.
for (cell = startCell; cell <= lastCell; cell++) {
for (i = 0; i < npages; i++) {
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2014/9/24 19:49, Ján Tomko wrote:
> On 09/24/2014 05:28 AM, zhang bo wrote:
>> The patch
>> http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=de0aeafe9ce3eb414c8b5d3aa8995d776a2952de
>> removes invtsc flag in the host-model CPU.
>>
>> I'm wondering, will it be better to pass args "migratable=no/yes" to qemu,
>> and let qemu complete the
>> remaining work? As that qemu has checked whether it's necessary to use
>> invtsc or not.
>
> The 'migratable' property is only for -cpu host (
> in libvirt).
>
> For mode='host-model', libvirt detects the model and features of the host CPU
> and passes it as -cpu ,+feat,+feat2,...
> so we can't leave that to QEMU.
>
>> --
>> "invtsc is available only if using: -cpu host,migratable=no,+invtsc."
>> http://git.qemu.org/?p=qemu.git;a=commit;h=120eee7d1fdb2eba15766cfff7b9bcdc902690b4
>> --
>>
>> There's another problem, if we do not pass "migratable=no" to qemu.
>> Consider if we set host mode to pass-through
>>-
>>
>>
>>-
>> then the vm->def->cpu->features contains invtsc. however, qemu will
>> automatically remove this cpu flag
>> as that "migration=no" is not passed to it. thus, the guest will not start
>> up. This problem is in fact
>> caused by the patch:
>> http://libvirt.org/git/?p=libvirt.git;a=commit;h=fba6bc47cbcabbe08d42279691efb0dff3b9c997,
>> it forbids guest domain to start up if the host has INVTSC while the
>> guest(qemu) does not.
>
> Regardless of QEMU support for invtsc, I'm only able to start the domain,
> restore or migration fails.
>
> As far as I know, only 'invtsc' is the problematic feature, because it both
> a) can appear in the host CPU (so libvirt assumes -cpu host will add it)
> b) is checked by qemuProcessVerifyGuestCPU (and libvirt complains when it's
> not there)
>
> For other features, we only add them to qemu command line and let qemu filter
> out the unsupported ones.
>
>>-
>> for (i = 0; def->cpu && i < def->cpu->nfeatures; i++) {
>> virCPUFeatureDefPtr feature = &def->cpu->features[i];
>>
>> if (feature->policy != VIR_CPU_FEATURE_REQUIRE)
>> continue;
>>
>> if (STREQ(feature->name, "invtsc") &&
>> !cpuHasFeature(guestcpu, feature->name)) {
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>_("host doesn't support invariant TSC"));
>> goto cleanup;
>> }
>> }
>> break;
>>--
>>
>>
>> In conclusion:
>> 1 Will it better to pass args "migratable=yes/no" to qemu rather than doing
>> the mask-invtsc job in libvirt?
>> 2 If the guest has "pass-through" cpu mode, then it's unable to start up,
>> because qemu removes invtsc, and
>> vm->def->cpu->features has it. It seems a BUG.
>>
>
> I think the simplest fix for host-passthrough would be to apply the same
> filter host-model has.
>
> But since using invtsc with host-passthrough requires both +invtsc and
> migratable=no, so we'd need to either add a 'migratable' option to
> host-passthrough (this would skip the filter and add migratable=on), or allow
> fine-tuning the features for host-passthrough too.
>
> Jan
>
Additional to the 2 suggestions, will that be OK to remove the codes in
qemuProcessVerifyGuestCPU that checks whether the vm->def has
invtsc flag while qemu doesn't?
- if (STREQ(feature->name, "invtsc") &&
- !cpuHasFeature(guestcpu, feature->name)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-_("host doesn't support invariant TSC"));
- goto cleanup;
- }
Removing these codes, plus with the solution that "add 'migratable' option
to host-passthrough", it seems the problem would
be gone, and invtsc would not be so 'distinctive' in libvirt any more.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/24/2014 05:50 AM, Laine Stump wrote:
> This patch fills in the functionality of
> processNicRxFilterChangedEvent(). It now checks if it is appropriate
> to respond to the NIC_RX_FILTER_CHANGED event (based on device type
> and configuration) and takes appropriate action. Currently it checks
> if the guest interface has been configured with
> trustGuestRxFilters='yes', and if the host side device is macvtap. If
> so, and the MAC address on the guest has changed, the MAC address of
> the macvtap device is changed to match.
>
> The result of this is that networking from the guest will continue to
> work if the mac address of a macvtap-connected network device is
> changed from within the guest, as long as trustGuestRxFilters='yes'
> (previously changing the MAC address in the guest would break
> networking).
> ---
> src/qemu/qemu_driver.c | 50
> ++
> 1 file changed, 50 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 64f1d45..7801d91 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4146,8 +4146,13 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
> char *devAlias)
> {
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> virDomainDeviceDef dev;
> virDomainNetDefPtr def;
> +virNetDevRxFilterPtr filter = NULL;
> +virMacAddr oldMAC;
> +char newMacStr[VIR_MAC_STRING_BUFLEN];
> +int ret;
>
> VIR_DEBUG("Received NIC_RX_FILTER_CHANGED event for device %s "
>"from domain %p %s",
> @@ -4175,11 +4180,55 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr
> driver,
> }
> def = dev.data.net;
>
> +if (!virDomainNetGetActualTrustGuestRxFilters(def)) {
> +VIR_WARN("ignore NIC_RX_FILTER_CHANGED event for network "
> + "device %s in domain %s",
> + def->info.alias, vm->def->name);
So how often could this be emitted? Do we need some sort of "filter" to
only message once per life of the domain?
> +/* not sending "query-rx-filter" will also suppress any
> + * further NIC_RX_FILTER_CHANGED events for this device
> + */
> +goto endjob;
> +}
> +
> /* handle the event - send query-rx-filter and respond to it. */
>
> VIR_DEBUG("process NIC_RX_FILTER_CHANGED event for network "
>"device %s in domain %s", def->info.alias, vm->def->name);
>
There's no capabilities check necessary? What if the command doesn't
exist in the underlying qemu?
> +qemuDomainObjEnterMonitor(driver, vm);
> +ret = qemuMonitorQueryRxFilter(priv->mon, devAlias, &filter);
> +qemuDomainObjExitMonitor(driver, vm);
> +if (ret < 0)
> +goto endjob;
You filled in a lot of data from the qemuMonitorQueryRxFilter(), but
you're only using filter->mac (e.g. main-mac).
Or am I missing something?
John
> +
> +virMacAddrFormat(&filter->mac, newMacStr);
> +
> +if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_DIRECT) {
> +
> +/* For macvtap connections, set the macvtap device's MAC
> + * address to match that of the guest device.
> + */
> +
> +if (virNetDevGetMAC(def->ifname, &oldMAC) < 0) {
> +VIR_WARN("Couldn't get current MAC address of device %s "
> + "while responding to NIC_RX_FILTER_CHANGED",
> + def->ifname);
> +goto endjob;
> +}
> +
> +if (virMacAddrCmp(&oldMAC, &filter->mac)) {
> +/* set new MAC address from guest to associated macvtap device */
> +if (virNetDevSetMAC(def->ifname, &filter->mac) < 0) {
> +VIR_WARN("Couldn't set new MAC address %s to device %s "
> + "while responding to NIC_RX_FILTER_CHANGED",
> + newMacStr, def->ifname);
> +} else {
> +VIR_DEBUG("device %s MAC address set to %s",
> + def->ifname, newMacStr);
> +}
> +}
> +}
> +
> endjob:
> /* We had an extra reference to vm before starting a new job so ending
> the
> * job is guaranteed not to remove the last reference.
> @@ -4187,6 +4236,7 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
> ignore_value(qemuDomainObjEndJob(driver, vm));
>
> cleanup:
> +virNetDevRxFilterFree(filter);
> VIR_FREE(devAlias);
> virObjectUnref(cfg);
> }
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/24/2014 05:50 AM, Laine Stump wrote:
> NIC_RX_FILTER_CHANGED is sent by qemu any time a NIC driver in the
> guest modified the NIC's RX Filter (for example, if the MAC address of
> the NIC is changed by the guest).
>
> This patch doesn't do anything useful with that event; it just sets up
> all the plumbing to get news of the event into a worker thread with
> all proper locking/reference counting, and provide an easy place to
> add in desired functionality.
>
> For easy reference the next time a handler for a qemu event is
> written, here is the sequence:
>
> The handler in qemu_json_monitor.c calls
>
>qemu_json_monitor.c:qemuMonitorJSONHandleNicRxFilterChanged()
>
> which calls
>
>qemu_monitor.c:qemuMonitorEmitNicRxFilterChanged()
>
> which uses QEMU_MONITOR_CALLBACK() to call
> mon->cb->domainNicRxFilterChanged(), ie:
>
>qemuProcessHandleNicRxFilterChanged()
>
> which will queue an event QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED for
> a worker thread to handle.
>
> When the worker thread gets the event, it calls
>
>qemuProcessEventHandler()
>
> which calls
>
>processNicRxFilterChangedEvent()
>
> and *that* is where the actual work will be done (and any
> event-specific memory allocated during qemuProcessHandleXXX() will be
> freed) - it is the middle of this function where functionality behind
> the event will be added in the next patch; for now there is just a
> VIR_DEBUG() to log the event.
This is the kind of stuff while great in a commit message - is perhaps
even better in the comments of the code somewhere. Not sure of which is
the "best place", but perhaps before the typedef enum {}
qemuProcessEventType; At least someone will get a head start on the
various places they have to change. Anyone adding an event has to start
there.
> ---
> src/qemu/qemu_domain.h | 1 +
> src/qemu/qemu_driver.c | 55
>
> src/qemu/qemu_monitor.c | 13 +++
> src/qemu/qemu_monitor.h | 7 ++
> src/qemu/qemu_monitor_json.c | 17 ++
> src/qemu/qemu_process.c | 42 +
> 6 files changed, 135 insertions(+)
>
Although not my area of expertise, things look good. Looks like all the
changes were very similar to other events added. Not the definitive ACK
you probably want, but hopefully someone else who's made changes in this
space recently can take a quick look to make sure you've covered everything.
My one question would be - is there any sort of issue if when
registering the event that the underlying qemu doesn't support/have the
NIC_RX_FILTER_CHANGED event defined? Or does that just not matter and
all this does is take events sent to libvirt after registering at some
higher level to receive "any" event. It seems to be the latter through
qemuMonitorIO, but I figured I'd ask...
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/24/2014 05:50 AM, Laine Stump wrote:
> This function can be called at any time to get the current status of a
> guest's network device rx-filter. In particular it is useful to call
> after libvirt recieves a NIC_RX_FILTER_CHANGED event - this event only
> tells you that something has changed in the rx-filter, the details are
> retrieved with the query-rx-filter monitor command (only available in
> the json monitor). The commend sent to the qemu monitor looks like this:
>
> {"execute":"query-rx-filter", "arguments": {"name":"net2"} }'
>
> and the results will look something like this:
>
> {
> "return": [
> {
> "promiscuous": false,
> "name": "net2",
> "main-mac": "52:54:00:98:2d:e3",
> "unicast": "normal",
> "vlan": "normal",
> "vlan-table": [
> 42,
> 0
> ],
> "unicast-table": [
>
> ],
> "multicast": "normal",
> "multicast-overflow": false,
> "unicast-overflow": false,
> "multicast-table": [
> "33:33:ff:98:2d:e3",
> "01:80:c2:00:00:21",
> "01:00:5e:00:00:fb",
> "33:33:ff:98:2d:e2",
> "01:00:5e:00:00:01",
> "33:33:00:00:00:01"
> ],
> "broadcast-allowed": false
> }
> ],
> "id": "libvirt-14"
> }
>
> This is all parsed from JSON into a virNetDevRxFilter object for
> easier consumption. (unicast-table is usually empty, but is also an
> array of mac addresses similar to multicast-table).
>
> (NB: LIBNL_CFLAGS was added to tests/Makefile.am because virnetdev.h
> now includes util/virnetlink.h, which includes netlink/msg.h when
> appropriate. Without LIBNL_CFLAGS, gcc can't find that file (if
> libnl/netlink isn't available, LIBNL_CFLAGS will be empty and
> virnetlink.h won't try to include netlink/msg.h anyway).)
> ---
> src/qemu/qemu_monitor.c | 26 ++
> src/qemu/qemu_monitor.h | 4 +
> src/qemu/qemu_monitor_json.c | 215
> +++
> src/qemu/qemu_monitor_json.h | 3 +
> tests/Makefile.am| 3 +
> 5 files changed, 251 insertions(+)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index c25f002..48cbe3e 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2918,6 +2918,32 @@ int qemuMonitorRemoveNetdev(qemuMonitorPtr mon,
> }
>
>
> +int
> +qemuMonitorQueryRxFilter(qemuMonitorPtr mon, const char *alias,
> + virNetDevRxFilterPtr *filter)
> +{
> +int ret = -1;
> +VIR_DEBUG("mon=%p alias=%s filter=%p",
> + mon, alias, filter);
> +
> +if (!mon) {
> +virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("monitor must not be NULL"));
> +return -1;
> +}
The "if (!mon->json)" usually goes here rather than later as an else.
Just trying to be consistent with other functions
I'm assuming at some point whomever calls this will check if the
query-rx-filter command exits (eg, the qemu capability exists)...
Again I haven't peeked ahead.
> +
> +
> +VIR_DEBUG("mon=%p, alias=%s", mon, alias);
> +
> +if (mon->json)
> +ret = qemuMonitorJSONQueryRxFilter(mon, alias, filter);
> +else
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("query-rx-filter requires JSON monitor"));
> +return ret;
> +}
> +
> +
> int qemuMonitorGetPtyPaths(qemuMonitorPtr mon,
> virHashTablePtr paths)
> {
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 6b91e29..c37e36f 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -31,6 +31,7 @@
> # include "virbitmap.h"
> # include "virhash.h"
> # include "virjson.h"
> +# include "virnetdev.h"
> # include "device_conf.h"
> # include "cpu/cpu.h"
>
> @@ -622,6 +623,9 @@ int qemuMonitorAddNetdev(qemuMonitorPtr mon,
> int qemuMonitorRemoveNetdev(qemuMonitorPtr mon,
> const char *alias);
>
> +int qemuMonitorQueryRxFilter(qemuMonitorPtr mon, const char *alias,
> + virNetDevRxFilterPtr *filter);
> +
> int qemuMonitorGetPtyPaths(qemuMonitorPtr mon,
> virHashTablePtr paths);
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index a3d7c2c..58007e6 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -3194,6 +3194,221 @@ int qemuMonitorJSONRemoveNetdev(qemuMonitorPtr mon,
> }
>
>
> +static int
> +qemuMonitorJSONQueryRxFilterParse(virJSONValuePtr msg,
> + virNetDevRxFilterPtr *filter)
> +{
> +int ret = -1;
> +const char *tmp;
> +virJSONValuePtr returnArray, entry, table, element;
> +int nTable;
> +size_t i;
> +virNetDevRxFilte
On 09/24/2014 05:50 AM, Laine Stump wrote:
> As is done with other items such as vlan, virtualport, and bandwidth,
> set the actual trustGuestRxFilters value to be used by a domain
> interface according to a merge of the same attribute in the interface,
> portgroup, and network in use. the interface setting always takes
> precedence (if specified), followed by portgroup, and finally the
> setting in the network is used if it's not specified in the interface
> or portgroup.
> ---
> src/network/bridge_driver.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 979fb13..548e354 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -3794,6 +3794,17 @@ networkAllocateActualDevice(virDomainDefPtr dom,
> if (vlan && virNetDevVlanCopy(&iface->data.network.actual->vlan, vlan) <
> 0)
> goto error;
>
> +if (iface->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT)
> + iface->data.network.actual->trustGuestRxFilters
> + = iface->trustGuestRxFilters;
> +else if (portgroup &&
> + portgroup->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT)
> + iface->data.network.actual->trustGuestRxFilters
> + = portgroup->trustGuestRxFilters;
> +else if (netdef->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT)
> + iface->data.network.actual->trustGuestRxFilters
> + = netdef->trustGuestRxFilters;
> +
Since BOOL_ABSENT is 0 - you don't "need" the comparison - it may look
cleaner too
ACK either way
John
> if ((netdef->forward.type == VIR_NETWORK_FORWARD_NONE) ||
> (netdef->forward.type == VIR_NETWORK_FORWARD_NAT) ||
> (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE)) {
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/24/2014 05:50 AM, Laine Stump wrote:
> This new attribute will control whether or not libvirt will pay
> attention to guest notifications about changes to network device mac
> addresses and receive filters. The default for this is 'no' (for
> security reasons). If it is set to 'yes' *and* the specified device
> model and connection support it (currently only macvtap+virtio) then
> libvirt will watch for NIC_RX_FILTER_CHANGED events, and when it
> receives one, it will issue a query-rx-filter command, retrieve the
> result, and modify the host-side macvtap interface's mac address and
> unicast/multicast filters accordingly.
>
> The functionality behind this attribute will be in a later patch. This
> patch merely adds the attribute to the top-level of a domain's
> as well as to and , and adds
> documentation and schema/xml2xml tests. Rather than adding even more
> test files, I've just added the net attribute in various applicable
> places of existing test files.
> ---
> docs/formatdomain.html.in | 38
> docs/formatnetwork.html.in | 28 +--
> docs/schemas/domaincommon.rng | 5 +++
> docs/schemas/network.rng | 10 ++
> src/conf/domain_conf.c | 42
> ++
> src/conf/domain_conf.h | 3 ++
> src/conf/network_conf.c| 35 ++
> src/conf/network_conf.h| 2 ++
> src/libvirt_private.syms | 1 +
> tests/networkxml2xmlin/vepa-net.xml| 4 +--
> tests/networkxml2xmlout/vepa-net.xml | 4 +--
> .../qemuxml2argv-net-virtio-network-portgroup.xml | 4 +--
> 12 files changed, 160 insertions(+), 16 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index eefdd5e..17e180d 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3343,10 +3343,9 @@
>
>...
>
> -
> -
> -
> -
> +
> +
> +
>
>
>
> @@ -3356,8 +3355,21 @@
>
>There are several possibilities for specifying a network
>interface visible to the guest. Each subsection below provides
> - more details about common setup options. Additionally,
> - each element has an
> + more details about common setup options.
> +
> +
> + libvirt allows specifying when the host should trust reports
> + from the guest of changes to the interface mac address and
> + receive filters by setting the trustGuestRxFilters
> + attribute to yesSince
> + 1.2.9. trustGuestRxFilters defaults
> + to no for security reasons, and support depends on
> + the guest network device driver as well as the type of
> + connection on the host - currently it is only supported for the
> + virtio driver, and for macvtap connections on the host.
Since 1.2.9), the interface
element property trustGuestRxFilters provides the
capability for the host to detect and trust reports from the guest
regarding changes to the interface mac address and receive filters by
setting the attribute to yes. The default setting for the
attribute is no for security reasons and support depends on
the guest network device driver as well as the type of connection on the
host - currently it is only supported for the virtio driver and for
macvtap connections on the host.
[just looks wierd starting with libvirt allows... I also changed the
"virtio driver, and for" to not have the "," since it's just joining two
phrases not a list of things...]
> +
> +
> + Each element has an
>optional sub-element that can tie
>the interface to a particular pci slot, with
>attribute type='pci'
> @@ -3589,6 +3601,18 @@
>being the default mode. The individual modes cause the delivery of
>packets to behave as follows:
>
> +
> + If the model type is set to virtio and
> + interface's trustGuestRxFilters attribute is set
> + to yes, changes made to the interface mac address,
> + unicast/multicast receive filters, and vlan settings in the
> + guest will be monitored and propogated to the associated macvtap
s/propogated/propagated/
> + device on the host. Since
> + 1.2.9. If trustGuestRxFilters is not set, or
Instead of "...on the host. Since 1.2.9. If..." - consider "...on the
host (since 1.2.9). If..."
> + is not supported for the device model in use, an attempted
> + change to the mac address origin
On 09/22/2014 05:23 PM, Eric Blake wrote:
> On 09/22/2014 02:49 PM, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1141621
>>
>> Using qemu-attach to attach to a qemu created process and then
>> attempting to virsh detach-interface caused a libvirtd crash
>> since the assumption was that device aliases were in place and
>> that assumption is not necessarily true for the qemu-attach environment.
>
> Hmm. I'm wondering whether it is better to patch qemu-attach to populate
> alias information at attach time rather than having to chase all the
> places that will later fail when the assumption is broken. While your
> patch is certainly more appealing for being smaller, it makes me wonder
> if it is the only such place bitten by the assumptions.
>
I took this approach mainly because it seemed that generating aliases in
the qemu-attach path was explicitly not done for a reason while it was
done for the qemuConnectDomainXMLToNative and qemuDomainQemuAttach
paths. For each of the other attach paths, there are the separate calls
to the specific qemuAssignDevice*Alias functions. So it does seem that
this qemu-attach path is the only current one without the aliases set.
If there is some path in the future that doesn't set the alias properly,
then at the very least we don't have a libvirtd crash at least for this
path.
FWIW: If I modify the patch as follows:
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
-detach->info.alias) {
+if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
+if (!detach->info.alias) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ ("device alias not found: cannot detach net "
+"device not properly attached"));
+qemuDomainObjExitMonitor(driver, vm);
+goto cleanup;
+}
then rerun the sequence of steps, I get:
error: Failed to detach interface
error: operation failed: device alias not found: cannot detach net
device not properly attached
If I then add qemuAssignDeviceAliases() as described below and rerun,
the error changes to:
error: Failed to detach interface
error: operation failed: detaching net0 device failed: Device 'net0' not
found
Since the original start didn't use the "-netdev/-device" syntax that
would be expected if the DRIVE capability was set. So no worse than we
were before.
Should I just post the additional patch calling qemuAssignDeviceAliases
and go from there?
>>
>> Add some more verbiage to the virsh man page.
>>
>> Signed-off-by: John Ferlan
>> ---
>> Notes:
>> * The bz also mentions a failure for the hotplug/virsh attach-interface,
>>but that has been resolved by commit id 'ba7468db'
>> * For the bz, the attempt to attach-interface/hotplug still is not
>>successful, since device aliases are not set for the PCI controller.
>>* I did try adding a call to qemuAssignDeviceAliases() during the qemu-
>> attach code; however, that failed as well (perhaps for a different
>> reason) as the attach to PCI slot 3 function 0 was not available
>> for rtl8139 since it was being used by e1000
>
> That is, I'm wondering if this approach should be used after all, and we
> have yet another problem to solve first.
>
Using virsh/libvirt, I can define/start a guest without a network and
add one... So perhaps the qemu-attach path is "unique" with respect to
how qemu defines things and perhaps there's something "hidden"/unknown
causing e1000 to grab PCI slot 3. I didn't dig into that.
John
>>* If it's desired to add the alias call in that's fine, then that could
>> be added in qemuDomainQemuAttach prior to the qemuDomainAssignAddresses
>> call similar to qemuConnectDomainXMLToNative.
>>
>> src/qemu/qemu_hotplug.c | 3 ++-
>> tools/virsh.pod | 5 +++--
>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 7bc19cd..b7514ce 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -3520,7 +3520,8 @@ qemuDomainDetachNetDevice(virConnectPtr conn,
>> qemuDomainMarkDeviceForRemoval(vm, &detach->info);
>>
>> qemuDomainObjEnterMonitor(driver, vm);
>> -if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
>> +if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
>> +detach->info.alias) {
>> if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {
>> qemuDomainObjExitMonitor(driver, vm);
>> virDomainAuditNet(vm, detach, NULL, "detach", false);
>> diff --git a/tools/virsh.pod b/tools/virsh.pod
>> index 9919f92..947adaf 100644
>> --- a/tools/virsh.pod
>> +++ b/tools/virsh.pod
>> @@ -3686,8 +3686,9 @@ using the UNIX driver. Ideally the process will also
>> have had the
>>
>> Not all functions of libvirt are expected to work reliably after
>> attaching to an externally launched QEMU process. There may be
>> -iss
On 09/24/2014 03:59 PM, Eric Blake wrote:
> On 09/24/2014 09:49 AM, Cole Robinson wrote:
>> The check for ISCSI devices was missing a check of subsys type, which
>> meant we could skip labelling of other host devices as well. This fixes
>> USB hotplug on F21
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1145968
>> ---
>> src/security/security_apparmor.c | 3 ++-
>> src/security/security_dac.c | 6 --
>> src/security/security_selinux.c | 6 --
>> 3 files changed, 10 insertions(+), 5 deletions(-)
>
> ACK.
>
> Might be worth a helper function to cut down on the repetition, but
> that's not mandatory.
>
Thanks, pushed now
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/24/2014 02:09 PM, Dr. David Alan Gilbert wrote:
>> This right here is a problem. Qemu has explicitly documented that
>> anything beginning with x- may be subject to change or going away in a
>> later release, so libvirt policy has been to delay any patches
>> targetting an x- interface until upstream qemu renames it to drop the x-
>> to signify that it is now stable. So we can review your patches, but
>> can't apply them yet.
>
> It is a shame that libvirt doesn't have a similar mechanism which could
> be used in conjunction with qemu. My reason for currently leaving the x-
> in place is that while I don't expect the QML side to change, it seemed
> fair to put a new feature into QEMU without locking it down for the
> first cut. This seems to be normal practice in QEMU but invariably
> means that libvirt support is delayed.
Libvirt DOES have the ability to issue raw monitor commands through
libvirt-qemu.so, and that might be sufficient for coding up the required
hacks. Or, if the libvirt proof of concept patches here do the job, we
can feed that back to the qemu community as proof that the feature works
enough to lose the x- prefix right away, and commit ourselves to the
interface. The benefit of a proof-of-concept patch done in parallel is
that the moment the upstream qemu feature is no longer experimental,
libvirt can apply the patches right away, and downstream distros can
backport those packages in situations where .so stability is not violated.
--
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
* Eric Blake (ebl...@redhat.com) wrote:
> On 09/23/2014 08:09 AM, Cristian Klein wrote:
> > Qemu currently implements pre-copy live migration. VM memory pages are
> > first copied from the source hypervisor to the destination, potentially
> > multiple times as pages get dirtied during transfer, then VCPU state
> > is migrated. Unfortunately, if the VM dirties memory faster than the
> > network bandwidth, then pre-copy cannot finish. `virsh` currently
> > includes an option to suspend a VM after a timeout, so that migration
> > may finish, but at the expense of downtime.
> >
> > A future version of qemu will implement post-copy live migration. The
> > VCPU state is first migrated to the destination hypervisor, then
> > memory pages are pulled from the source hypervisor. Post-copy has the
> > potential to do migration with zero-downtime, despite the VM dirtying
> > pages fast, with minimum performance impact. On the other hand, one
> > post-copy is in progress, any network failure would render the VM
> > unusable, as its memory is partitioned between the source and
> > destination hypervisor. Therefore, post-copy should only be used when
> > necessary.
>
> Thanks for tackling the proof of concept patches.
>
> >
> > Post-copy migration in qemu will work as follows:
> > (1) The `x-postcopy-ram` migration capability needs to be set.
>
> This right here is a problem. Qemu has explicitly documented that
> anything beginning with x- may be subject to change or going away in a
> later release, so libvirt policy has been to delay any patches
> targetting an x- interface until upstream qemu renames it to drop the x-
> to signify that it is now stable. So we can review your patches, but
> can't apply them yet.
It is a shame that libvirt doesn't have a similar mechanism which could
be used in conjunction with qemu. My reason for currently leaving the x-
in place is that while I don't expect the QML side to change, it seemed
fair to put a new feature into QEMU without locking it down for the
first cut. This seems to be normal practice in QEMU but invariably
means that libvirt support is delayed.
Dave
> --
> Eric Blake eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/24/2014 01:53 PM, Eric Blake wrote:
> On 09/24/2014 06:13 AM, Pavel Hrdina wrote:
>> When building on mingw the format string for long long/unsigned long
>> long have to be I64d/I64u instead of lld/llu.
>>
>> Signed-off-by: Pavel Hrdina
>> ---
>
> Yuck. This is really a problem of the examples directory not taking
> advantage of gnulib. I really don't like this solution.
Oh, another solution would be to link this example against gnulib. See
how domtop/Makefile.am pulls in libgnu.la, so that it can use %zu
without problems (%zu is in the same boat as %lld for mingw
unsupported-ness).
--
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
On 09/24/2014 09:49 AM, Cole Robinson wrote:
> The check for ISCSI devices was missing a check of subsys type, which
> meant we could skip labelling of other host devices as well. This fixes
> USB hotplug on F21
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1145968
> ---
> src/security/security_apparmor.c | 3 ++-
> src/security/security_dac.c | 6 --
> src/security/security_selinux.c | 6 --
> 3 files changed, 10 insertions(+), 5 deletions(-)
ACK.
Might be worth a helper function to cut down on the repetition, but
that's not mandatory.
--
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
On 09/23/2014 08:09 AM, Cristian Klein wrote:
> Qemu currently implements pre-copy live migration. VM memory pages are
> first copied from the source hypervisor to the destination, potentially
> multiple times as pages get dirtied during transfer, then VCPU state
> is migrated. Unfortunately, if the VM dirties memory faster than the
> network bandwidth, then pre-copy cannot finish. `virsh` currently
> includes an option to suspend a VM after a timeout, so that migration
> may finish, but at the expense of downtime.
>
> A future version of qemu will implement post-copy live migration. The
> VCPU state is first migrated to the destination hypervisor, then
> memory pages are pulled from the source hypervisor. Post-copy has the
> potential to do migration with zero-downtime, despite the VM dirtying
> pages fast, with minimum performance impact. On the other hand, one
> post-copy is in progress, any network failure would render the VM
> unusable, as its memory is partitioned between the source and
> destination hypervisor. Therefore, post-copy should only be used when
> necessary.
Thanks for tackling the proof of concept patches.
>
> Post-copy migration in qemu will work as follows:
> (1) The `x-postcopy-ram` migration capability needs to be set.
This right here is a problem. Qemu has explicitly documented that
anything beginning with x- may be subject to change or going away in a
later release, so libvirt policy has been to delay any patches
targetting an x- interface until upstream qemu renames it to drop the x-
to signify that it is now stable. So we can review your patches, but
can't apply them yet.
--
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
On 09/24/2014 06:13 AM, Pavel Hrdina wrote:
> When building on mingw the format string for long long/unsigned long
> long have to be I64d/I64u instead of lld/llu.
>
> Signed-off-by: Pavel Hrdina
> ---
Yuck. This is really a problem of the examples directory not taking
advantage of gnulib. I really don't like this solution.
> case VIR_TYPED_PARAM_LLONG:
> -printf("\t%s: %lld\n", params[i].field, params[i].value.l);
> +printf("\t%s: "LLD_FORMAT"\n", params[i].field,
> params[i].value.l);
Would it work to include , then write this as:
printf("\t%s: %" PRId64 "\n", params[i].field,
(int64_t) params[i].value.l);
If so, I much prefer that form, as it at least uses standardized
interfaces instead of reinventing a wraparound for the brain-dead
Microsoft printf.
--
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
On 09/24/2014 09:17 AM, Gene Czarcinski wrote:
> Since libvirt-daemon-1.2.8-5 fixes the problem, I just wanted to give
> everyone a heads up. The libvirt-daemon-1.2.8-1 has an error in the
> spec file scripts which causes removal failure.
>
Known issue:
https://bugzilla.redhat.com/show_bug.cgi?id=1142367
https://lists.fedoraproject.org/pipermail/virt/2014-September/004171.html
--
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
On 09/24/2014 01:48 AM, Pavel Hrdina wrote:
> It would be nice to also print a params pointer and number of params in
> the debug message and the previous limit for number of params in the rpc
> message was too large. The 2048 params will be enough for future events.
>
> Signed-off-by: Pavel Hrdina
> ---
> daemon/remote.c | 4 ++--
> src/remote/remote_protocol.x | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
ACK.
--
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
22.09.2014 23:34, Alex Bligh wrote:
> This patch series adds inbound migrate capability from qemu-kvm version
> 1.0. [...]
Isn't it quite a bit too late already? That's an old version by
now, and supporting migration from it is interesting for long-term
support distributions - like redhat for example, with several
years of release cycle. Is it really necessary at this time to
add this ability to upstream qemu?
It'd be very nice to have this capability right when qemu-kvm
tree has been merged (or even before that), but it's been some
years ago already.
Thanks,
/mjt
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/24/2014 05:13 PM, Martin Kletzander wrote:
On Wed, Sep 24, 2014 at 02:13:25PM +0200, Pavel Hrdina wrote:
When building on mingw the format string for long long/unsigned long
long have to be I64d/I64u instead of lld/llu.
Signed-off-by: Pavel Hrdina
---
I'm not sure if this is the right way to do it so sending it for review.
I think this is OK since it's still partly build-breaker. We can
change it later.
examples/object-events/event-test.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/examples/object-events/event-test.c
b/examples/object-events/event-test.c
index 9e09736..e90b590 100644
--- a/examples/object-events/event-test.c
+++ b/examples/object-events/event-test.c
@@ -476,6 +476,15 @@ myDomainEventTunableCallback(virConnectPtr conn
ATTRIBUTE_UNUSED,
printf("%s EVENT: Domain %s(%d) tunable updated:\n",
__func__, virDomainGetName(dom), virDomainGetID(dom));
+#ifdef WIN32
+/* MinGW doesn't know the lld/llu so we have to use I64f/I64u
instead. */
+# define LLD_FORMAT "%I64d"
+# define LLU_FORMAT "%I64u"
+#else /* WIN32 */
+# define LLD_FORMAT "%lld"
+# define LLU_FORMAT "%llu"
+#endif /* WIN32 */
+
for (i = 0; i < nparams; i++) {
switch (params[i].type) {
case VIR_TYPED_PARAM_INT:
@@ -485,10 +494,10 @@ myDomainEventTunableCallback(virConnectPtr conn
ATTRIBUTE_UNUSED,
printf("\t%s: %u\n", params[i].field, params[i].value.ui);
break;
case VIR_TYPED_PARAM_LLONG:
-printf("\t%s: %lld\n", params[i].field, params[i].value.l);
+printf("\t%s: "LLD_FORMAT"\n", params[i].field,
params[i].value.l);
break;
case VIR_TYPED_PARAM_ULLONG:
-printf("\t%s: %llu\n", params[i].field, params[i].value.ul);
+printf("\t%s: "LLU_FORMAT"\n", params[i].field,
params[i].value.ul);
ACK if you put space around the macros.
Martin
Updated and pushed, thanks.
Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Since libvirt-daemon-1.2.8-5 fixes the problem, I just wanted to give
everyone a heads up. The libvirt-daemon-1.2.8-1 has an error in the
spec file scripts which causes removal failure.
I had to manually rpm -e --noscripts libvirt-daemon-1.2.8-1.fc20.x86_64
and then I reinstalled libvirt-daemon-1.2.8-5 to make sure things were
"clean".
The error was:
/var/tmp/rpm-tmp.bvfDlf: line 8: libvirtd.socket: command not found
error: %preun(libvirt-daemon-1.2.8-1.fc20.x86_64) scriptlet failed, exit
status 127
Error in PREUN scriptlet in rpm package libvirt-daemon
Gene
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Sep 24, 2014 at 02:13:25PM +0200, Pavel Hrdina wrote:
When building on mingw the format string for long long/unsigned long
long have to be I64d/I64u instead of lld/llu.
Signed-off-by: Pavel Hrdina
---
I'm not sure if this is the right way to do it so sending it for review.
I think this is OK since it's still partly build-breaker. We can
change it later.
examples/object-events/event-test.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/examples/object-events/event-test.c
b/examples/object-events/event-test.c
index 9e09736..e90b590 100644
--- a/examples/object-events/event-test.c
+++ b/examples/object-events/event-test.c
@@ -476,6 +476,15 @@ myDomainEventTunableCallback(virConnectPtr conn
ATTRIBUTE_UNUSED,
printf("%s EVENT: Domain %s(%d) tunable updated:\n",
__func__, virDomainGetName(dom), virDomainGetID(dom));
+#ifdef WIN32
+/* MinGW doesn't know the lld/llu so we have to use I64f/I64u instead. */
+# define LLD_FORMAT "%I64d"
+# define LLU_FORMAT "%I64u"
+#else /* WIN32 */
+# define LLD_FORMAT "%lld"
+# define LLU_FORMAT "%llu"
+#endif /* WIN32 */
+
for (i = 0; i < nparams; i++) {
switch (params[i].type) {
case VIR_TYPED_PARAM_INT:
@@ -485,10 +494,10 @@ myDomainEventTunableCallback(virConnectPtr conn
ATTRIBUTE_UNUSED,
printf("\t%s: %u\n", params[i].field, params[i].value.ui);
break;
case VIR_TYPED_PARAM_LLONG:
-printf("\t%s: %lld\n", params[i].field, params[i].value.l);
+printf("\t%s: "LLD_FORMAT"\n", params[i].field, params[i].value.l);
break;
case VIR_TYPED_PARAM_ULLONG:
-printf("\t%s: %llu\n", params[i].field, params[i].value.ul);
+printf("\t%s: "LLU_FORMAT"\n", params[i].field,
params[i].value.ul);
ACK if you put space around the macros.
Martin
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/24/2014 09:44 AM, Ján Tomko wrote:
> On 09/24/2014 01:24 AM, John Ferlan wrote:
>>
>>
>> On 09/18/2014 10:15 AM, Ján Tomko wrote:
>>> Add options for tuning segment offloading:
>>>
>>> >> ecn='off' ufo='off'/>
>>>
>>>
>>> which control the respective host_ and guest_ properties
>>> of the virtio-net device.
>>> ---
>>> docs/formatdomain.html.in | 24 ++-
>>> docs/schemas/domaincommon.rng | 44 -
>>> src/conf/domain_conf.c | 218
>>> -
>>> src/conf/domain_conf.h | 15 ++
>>> .../qemuxml2argv-net-virtio-disable-offloads.xml | 35
>>> tests/qemuxml2xmltest.c| 1 +
>>> 6 files changed, 329 insertions(+), 8 deletions(-)
>>> create mode 100644
>>> tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.xml
>>>
>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>> index 8c03ebb..5dd70a4 100644
>>> --- a/docs/formatdomain.html.in
>>> +++ b/docs/formatdomain.html.in
>>> @@ -3868,7 +3868,11 @@ qemu-kvm -net nic,model=? /dev/null
>>>
>>>
>>>
>>> - >> event_idx='off' queues='5'/>
>>> + >> event_idx='off' queues='5'>
>>> +>> ufo='off'/>
>>> +
>>> +
>>> +
>>>
>>>
>>>...
>>> @@ -3972,6 +3976,24 @@ qemu-kvm -net nic,model=? /dev/null
>>> processor, resulting in much higher throughput.
>>> Since 1.0.6 (QEMU and KVM only)
>>>
>>> + host offloading options
>>
>> Should this be host offloading options?
>> or host TCP options (in some way to indicate that these
>> are TCP level options). Probably also want your disclaimer
>> use of these should be for only those that know what they
>> are doing...
>>
>
> Well ufo is an UDP option.
>
Yeah - good point. I guess I associated the rest/most with TCP... Of
course you could use "TCP/UDP" instead of just TCP... I'm OK without
the TCP reference though - it was a "extra" suggestion.
>>
>>> +
>>> +The csum, gso, tso4,
>>> +tso6, ecn, ufo
>>
>> Should read: ecn, and ufo
>>
>> Should you "spell out" what each translates to?
>>
>> csum (checksums), gso (generic segmentation offload),
>> tso (tcp segmentation offload v4 and v6), ecn (congestion
>> notification), and ufo (UDP fragment offloads)
>>
>
> I thought not spelling them out was the equivalent of the "only use this if
> you know what you're doing" disclaimer.
>
Yes - I do agree that by not spelling them out it may cause someone to
"pause" before adding them.
However, of course, we're engineers so we have this "desire" to try
anyway and/or know what these new knobs/things do.
I guess it's one of those things that I don't like - that is seeing an
acronym on a website or in documentation that's not spelled out.
>>
>>
>>> +attributes with possible values on
>>> +and off can be used to turn host offloads off.
>>
>> s/turn host offloads off/turn off host TCP options/
>>
>> Saying "offloads off" aloud just doesn't seem right.
>>
>>> +By default, the supported offloads are enabled by QEMU.
>>
>> s/the supported offloads/the TCP options/
>>
>>> +Since 1.2.9 (QEMU only)
>>> +
>>> + guest offloading options
>>
>> Similar in here... Does the host setting override the guest value
>> or can the host say "tso4=off" while the guest has "tso4=on"? Curious
>> mainly.
>
> It can say that, but I doubt it'll work.
>
>>
>>> +
>>> +The csum, tso4,
>>> +tso6, ecn, ufo
>>
>> same here with the "and" - although I suppose you could just
>> reference the bits "above"...
>>
>> Another way to say it is guests can use the same options except gso
>>
>>> +attributes with possible values on
>>> +and off can be used to turn guest offloads off.
>>
>> s/turn guest offloads off/turn off guest TCP options/
>
> How about 'turn off guest offload options'?
>
That's fine - it was the "offloads off" that didn't read well.
>>
>>> +By default, the supported offloads are enabled by QEMU.
>>
>> s/the supported offloads/the TCP options/
>
>>
>> And of course the usage disclaimer!
>>
>>
>>> +Since 1.2.9 (QEMU only)
>>> +
>>>
>>>
>>> Setting network backend-specific
>>> options
>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 3ccec1c..cdaafa6 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -6897,6 +6897,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>>> char *ioeventfd = NULL;
>>> char
On 09/24/2014 01:24 AM, John Ferlan wrote:
>
>
> On 09/18/2014 10:15 AM, Ján Tomko wrote:
>> Add options for tuning segment offloading:
>>
>> > ecn='off' ufo='off'/>
>>
>>
>> which control the respective host_ and guest_ properties
>> of the virtio-net device.
>> ---
>> docs/formatdomain.html.in | 24 ++-
>> docs/schemas/domaincommon.rng | 44 -
>> src/conf/domain_conf.c | 218
>> -
>> src/conf/domain_conf.h | 15 ++
>> .../qemuxml2argv-net-virtio-disable-offloads.xml | 35
>> tests/qemuxml2xmltest.c| 1 +
>> 6 files changed, 329 insertions(+), 8 deletions(-)
>> create mode 100644
>> tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.xml
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 8c03ebb..5dd70a4 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -3868,7 +3868,11 @@ qemu-kvm -net nic,model=? /dev/null
>>
>>
>>
>> - > event_idx='off' queues='5'/>
>> + > event_idx='off' queues='5'>
>> +> ufo='off'/>
>> +
>> +
>> +
>>
>>
>>...
>> @@ -3972,6 +3976,24 @@ qemu-kvm -net nic,model=? /dev/null
>> processor, resulting in much higher throughput.
>> Since 1.0.6 (QEMU and KVM only)
>>
>> + host offloading options
>
> Should this be host offloading options?
> or host TCP options (in some way to indicate that these
> are TCP level options). Probably also want your disclaimer
> use of these should be for only those that know what they
> are doing...
>
Well ufo is an UDP option.
>
>> +
>> +The csum, gso, tso4,
>> +tso6, ecn, ufo
>
> Should read: ecn, and ufo
>
> Should you "spell out" what each translates to?
>
> csum (checksums), gso (generic segmentation offload),
> tso (tcp segmentation offload v4 and v6), ecn (congestion
> notification), and ufo (UDP fragment offloads)
>
I thought not spelling them out was the equivalent of the "only use this if
you know what you're doing" disclaimer.
>
>
>> +attributes with possible values on
>> +and off can be used to turn host offloads off.
>
> s/turn host offloads off/turn off host TCP options/
>
> Saying "offloads off" aloud just doesn't seem right.
>
>> +By default, the supported offloads are enabled by QEMU.
>
> s/the supported offloads/the TCP options/
>
>> +Since 1.2.9 (QEMU only)
>> +
>> + guest offloading options
>
> Similar in here... Does the host setting override the guest value
> or can the host say "tso4=off" while the guest has "tso4=on"? Curious
> mainly.
It can say that, but I doubt it'll work.
>
>> +
>> +The csum, tso4,
>> +tso6, ecn, ufo
>
> same here with the "and" - although I suppose you could just
> reference the bits "above"...
>
> Another way to say it is guests can use the same options except gso
>
>> +attributes with possible values on
>> +and off can be used to turn guest offloads off.
>
> s/turn guest offloads off/turn off guest TCP options/
How about 'turn off guest offload options'?
>
>> +By default, the supported offloads are enabled by QEMU.
>
> s/the supported offloads/the TCP options/
>
> And of course the usage disclaimer!
>
>
>> +Since 1.2.9 (QEMU only)
>> +
>>
>>
>> Setting network backend-specific
>> options
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 3ccec1c..cdaafa6 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -6897,6 +6897,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>> char *ioeventfd = NULL;
>> char *event_idx = NULL;
>> char *queues = NULL;
>> +char *str = NULL;
>> char *filter = NULL;
>> char *internal = NULL;
>> char *devaddr = NULL;
>> @@ -7385,6 +7386,115 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>> }
>> def->driver.virtio.queues = q;
>> }
>
> How about something like this? Not that you have anything wrong, but
> it avoids the chance that some "change" in the future requires 11 similar
> changes...
I was worried it wouldn't be clear enough and since we use similar repetition
all over domain_conf, it would be better handled separately.
Jan
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/24/2014 09:11 AM, John Ferlan wrote:
> Prior patch removed the need for the virConnectPtr in the unplug
> detach host path which caused ripple effect to remove in multiple
> callers. The previous patch just left things as ATTRIBUTE_UNUSED -
> this patch will remove the variable.
>
> Signed-off-by: John Ferlan
UG Looks like I have to squash most parts of 3/3 into here...
For review/build purposes just combine the two - I'll do the right thing
for commit...
Off for another cup o joe to see if that helps :-)
John
> ---
> src/qemu/qemu_driver.c | 4 ++--
> src/qemu/qemu_hotplug.h | 6 ++
> 2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e73d4f9..c98e42f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6758,10 +6758,10 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
> ret = qemuDomainDetachLease(driver, vm, dev->data.lease);
> break;
> case VIR_DOMAIN_DEVICE_NET:
> -ret = qemuDomainDetachNetDevice(dom->conn, driver, vm, dev);
> +ret = qemuDomainDetachNetDevice(driver, vm, dev);
> break;
> case VIR_DOMAIN_DEVICE_HOSTDEV:
> -ret = qemuDomainDetachHostDevice(dom->conn, driver, vm, dev);
> +ret = qemuDomainDetachHostDevice(driver, vm, dev);
> break;
> case VIR_DOMAIN_DEVICE_CHR:
> ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr);
> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
> index 55c9333..1c9ca8f 100644
> --- a/src/qemu/qemu_hotplug.h
> +++ b/src/qemu/qemu_hotplug.h
> @@ -78,12 +78,10 @@ int qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr
> driver,
> int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virDomainDeviceDefPtr dev);
> -int qemuDomainDetachNetDevice(virConnectPtr conn,
> - virQEMUDriverPtr driver,
> +int qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
>virDomainObjPtr vm,
>virDomainDeviceDefPtr dev);
> -int qemuDomainDetachHostDevice(virConnectPtr conn,
> - virQEMUDriverPtr driver,
> +int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virDomainDeviceDefPtr dev);
> int qemuDomainAttachLease(virQEMUDriverPtr driver,
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
https://bugzilla.redhat.com/show_bug.cgi?id=1141732
Attempting to hot unplug a scsi_host device fails. The first patch
resolves the issue (and has some details in the commit message). The
second patch removes the now unnecessary virConnectPtr from various
places. The third patch resolves a potential issue if aliases weren't
defined and debugging is enabled by making the check for NULL first
before trying to message rather than the other way around.
John Ferlan (3):
qemu: Fix hot unplug of SCSI_HOST device
qemu: Remove need for virConnectPtr in hotunplug detach host, net
qemu: Remove possible NULL deref in debug output
src/qemu/qemu_driver.c | 4 +--
src/qemu/qemu_hotplug.c | 70 +++--
src/qemu/qemu_hotplug.h | 6 ++---
3 files changed, 37 insertions(+), 43 deletions(-)
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
https://bugzilla.redhat.com/show_bug.cgi?id=1141732
Introduced by commit id '8f76ad99' the logic to detach a scsi_host
device (SCSI or iSCSI) fails when attempting to remove the 'drive'
because as I found in my investigation - the DelDevice takes care of
that for us.
The investigation turned up commits to adjust the logic for the
qemuMonitorDelDevice and qemuMonitorDriveDel processing for interfaces
(commit id '81f76598'), disk bus=VIRTIO,SCSI,USB (commit id '0635785b'),
and chr devices (commit id '55b21f9b'), but nothing with the host devices.
This commit uses the model for the previous set of changes and applies
it to the hostdev path. The call to qemuDomainDetachHostSCSIDevice will
return to qemuDomainDetachThisHostDevice handling either the audit of
the failure or the wait for the removal and then call into
qemuDomainRemoveHostDevice for the event, removal from the domain hostdev
list, and audit of the removal similar to other paths.
NOTE: For now the 'conn' param to +qemuDomainDetachHostSCSIDevice is left
as ATTRIBUTE_UNUSED. Removing requires a cascade of other changes to be
left for a future patch.
Signed-off-by: John Ferlan
---
src/qemu/qemu_hotplug.c | 48
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 7bc19cd..cf1e4dc 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2623,10 +2623,24 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
virDomainNetDefPtr net = NULL;
virObjectEventPtr event;
size_t i;
+int ret = -1;
+qemuDomainObjPrivatePtr priv = vm->privateData;
+char *drivestr;
VIR_DEBUG("Removing host device %s from domain %p %s",
hostdev->info->alias, vm, vm->def->name);
+/* build the actual drive id string as generated during
+ * qemuBuildSCSIHostdevDrvStr that is passed to qemu */
+if (virAsprintf(&drivestr, "%s-%s",
+virDomainDeviceAddressTypeToString(hostdev->info->type),
+hostdev->info->alias) < 0)
+goto cleanup;
+
+qemuDomainObjEnterMonitor(driver, vm);
+qemuMonitorDriveDel(priv->mon, drivestr);
+qemuDomainObjExitMonitor(driver, vm);
+
event = virDomainEventDeviceRemovedNewFromObj(vm, hostdev->info->alias);
if (event)
qemuDomainEventQueue(driver, event);
@@ -2679,8 +2693,12 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
networkReleaseActualDevice(vm->def, net);
virDomainNetDefFree(net);
}
+ret = 0;
+
+ cleanup:
+VIR_FREE(drivestr);
virObjectUnref(cfg);
-return 0;
+return ret;
}
@@ -3305,14 +3323,12 @@ qemuDomainDetachHostUSBDevice(virQEMUDriverPtr driver,
}
static int
-qemuDomainDetachHostSCSIDevice(virConnectPtr conn,
+qemuDomainDetachHostSCSIDevice(virConnectPtr conn ATTRIBUTE_UNUSED,
virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainHostdevDefPtr detach)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
-char *drvstr = NULL;
-char *devstr = NULL;
int ret = -1;
if (!detach->info->alias) {
@@ -3327,33 +3343,17 @@ qemuDomainDetachHostSCSIDevice(virConnectPtr conn,
return -1;
}
-if (!(drvstr = qemuBuildSCSIHostdevDrvStr(conn, detach, priv->qemuCaps,
- &buildCommandLineCallbacks)))
-goto cleanup;
-if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, detach,
priv->qemuCaps)))
-goto cleanup;
-
qemuDomainMarkDeviceForRemoval(vm, detach->info);
qemuDomainObjEnterMonitor(driver, vm);
-if ((ret = qemuMonitorDelDevice(priv->mon, detach->info->alias)) == 0) {
-if ((ret = qemuMonitorDriveDel(priv->mon, drvstr)) < 0) {
-virErrorPtr orig_err = virSaveLastError();
-if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
-VIR_WARN("Unable to add device %s (%s) after failed "
- "qemuMonitorDriveDel",
- drvstr, devstr);
-if (orig_err) {
-virSetError(orig_err);
-virFreeError(orig_err);
-}
-}
+if (qemuMonitorDelDevice(priv->mon, detach->info->alias) < 0) {
+qemuDomainObjExitMonitor(driver, vm);
+goto cleanup;
}
qemuDomainObjExitMonitor(driver, vm);
+ret = 0;
cleanup:
-VIR_FREE(drvstr);
-VIR_FREE(devstr);
return ret;
}
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 24.09.2014 14:00, Jincheng Miao wrote:
On 09/24/2014 07:40 PM, Michal Privoznik wrote:
On 24.09.2014 07:45, Jincheng Miao wrote:
In nodeGetFreePages, if startCell is given by '0',
and the max node number is '0' too. The for-loop
wouldn't be executed.
So convert it to while-loop.
Before:
virsh freepages --cellno 0 --pagesize 4
error: internal error: no suitable info found
After:
virsh freepages --cellno 0 --pagesize 4
4KiB: 472637
Signed-off-by: Jincheng Miao
---
src/nodeinfo.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 2459922..1fe190a 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -2042,18 +2042,19 @@ nodeGetFreePages(unsigned int npages,
}
lastCell = MIN(lastCell, startCell + cellCount);
+cell = startCell;
-for (cell = startCell; cell < lastCell; cell++) {
+do {
for (i = 0; i < npages; i++) {
unsigned int page_size = pages[i];
unsigned int page_free;
-if (virNumaGetPageInfo(cell, page_size, 0, NULL,
&page_free) < 0)
+if (virNumaGetPageInfo(cell++, page_size, 0, NULL,
&page_free) < 0)
goto cleanup;
counts[ncounts++] = page_free;
}
-}
+} while (cell < lastCell);
if (!ncounts) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
There's no need to switch to do-while loop. All what is needed is cell
<= lastCell in the for loop. I'm rewriting the patch and pushing. ACK.
Wait, if change the for condition to 'cell <= lastCell', and pass
startCell == -1,
the for-loop will execute twice, and will overwrite counts[1] which is
not allocated.
Oh, right. Seems like I'm brainwashed today. This is the diff I've
forgot to 'git add':
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 0a7642c..05eab6c 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -2041,7 +2041,7 @@ nodeGetFreePages(unsigned int npages,
goto cleanup;
}
-lastCell = MIN(lastCell, startCell + cellCount);
+lastCell = MIN(lastCell, startCell + cellCount - 1);
for (cell = startCell; cell <= lastCell; cell++) {
for (i = 0; i < npages; i++) {
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Sep 23, 2014 at 16:09:57 +0200, Cristian Klein wrote:
> Added a new `libvirt` migration flag `VIR_MIGRATE_POSTCOPY` and the
> necessary code to pass this flag to qemu as migration capability.
>
> Signed-off-by: Cristian Klein
> ---
> include/libvirt/libvirt.h.in | 1 +
> src/libvirt.c| 7 +++
> src/qemu/qemu_migration.c| 43 +++
> src/qemu/qemu_migration.h| 3 ++-
> 4 files changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 6371b7b..bdc33c6 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -1225,6 +1225,7 @@ typedef enum {
> VIR_MIGRATE_ABORT_ON_ERROR= (1 << 12), /* abort migration on I/O
> errors happened during migration */
> VIR_MIGRATE_AUTO_CONVERGE = (1 << 13), /* force convergence */
> VIR_MIGRATE_RDMA_PIN_ALL = (1 << 14), /* RDMA memory pinning */
> +VIR_MIGRATE_POSTCOPY = (1 << 15), /* enable (but don't start)
> post-copy */
> } virDomainMigrateFlags;
>
>
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 1a285ca..33aeafa 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -5265,6 +5265,7 @@ virDomainMigrateDirect(virDomainPtr domain,
> * automatically when supported).
> * VIR_MIGRATE_UNSAFEForce migration even if it is considered unsafe.
> * VIR_MIGRATE_OFFLINE Migrate offline
> + * VIR_MIGRATE_POSTCOPY Enable (but don't start) post-copy
> *
> * VIR_MIGRATE_TUNNELLED requires that VIR_MIGRATE_PEER2PEER be set.
> * Applications using the VIR_MIGRATE_PEER2PEER flag will probably
> @@ -5291,6 +5292,12 @@ virDomainMigrateDirect(virDomainPtr domain,
> * can use either VIR_MIGRATE_NON_SHARED_DISK or
> * VIR_MIGRATE_NON_SHARED_INC as they are mutually exclusive.
> *
> + * If you want to enable post-copy migration you must set the
> + * VIR_MIGRATE_POSTCOPY flag. Once migration is active, you may
> + * start post-copy by calling virDomainMigrateStartPostCopy.
> + * When to start post-copy is entirely left to the user, libvirt
> + * only implements the necessary mechanism.
> + *
> * In either case it is typically only necessary to specify a
> * URI if the destination host has multiple interfaces and a
> * specific interface is required to transmit migration data.
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index a5bd825..bd1f2d6 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1799,6 +1799,45 @@ qemuMigrationSetOffline(virQEMUDriverPtr driver,
>
>
> static int
> +qemuMigrationSetPostCopy(virQEMUDriverPtr driver,
> +virDomainObjPtr vm,
> +qemuDomainAsyncJob job)
s/ // in the two lines above to fix indentation.
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +int ret;
> +
> +if (qemuDomainObjEnterMonitorAsync(driver, vm, job) < 0)
> +return -1;
> +
> +ret = qemuMonitorGetMigrationCapability(
> +priv->mon,
> +QEMU_MONITOR_MIGRATION_CAPS_POSTCOPY);
QEMU_MONITOR_MIGRATION_CAPS_POSTCOPY is not defined anywhere in this
patchset. It seems you forgot to update qemu_monitor.[ch].
> +
> +if (ret < 0) {
> +goto cleanup;
> +} else if (ret == 0) {
> +if (job == QEMU_ASYNC_JOB_MIGRATION_IN) {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> + _("Post-copy migration is not supported by "
> + "target QEMU binary"));
> +} else {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> + _("Post-copy migration is not supported by "
> + "source QEMU binary"));
> +}
> +ret = -1;
> +goto cleanup;
> +}
> +
> +ret = qemuMonitorSetMigrationCapability(
> +priv->mon,
> +QEMU_MONITOR_MIGRATION_CAPS_POSTCOPY);
> +
> + cleanup:
> +qemuDomainObjExitMonitor(driver, vm);
> +return ret;
> +}
> +static int
> qemuMigrationSetCompression(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> qemuDomainAsyncJob job)
> @@ -3580,6 +3619,10 @@ qemuMigrationRun(virQEMUDriverPtr driver,
> if (flags & VIR_MIGRATE_RDMA_PIN_ALL &&
> qemuMigrationSetPinAll(driver, vm,
> QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
> +
> +if (flags & VIR_MIGRATE_POSTCOPY &&
> +qemuMigrationSetPostCopy(driver, vm,
> + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
> goto cleanup;
>
> if (qemuDomainObjEnterMonitorAsync(driver, vm,
I'd expect similar thing would need to be done in the Prepare phase on
destination... However, if destination does not need to set the
capability, we at least need to check if
When building on mingw the format string for long long/unsigned long
long have to be I64d/I64u instead of lld/llu.
Signed-off-by: Pavel Hrdina
---
I'm not sure if this is the right way to do it so sending it for review.
examples/object-events/event-test.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/examples/object-events/event-test.c
b/examples/object-events/event-test.c
index 9e09736..e90b590 100644
--- a/examples/object-events/event-test.c
+++ b/examples/object-events/event-test.c
@@ -476,6 +476,15 @@ myDomainEventTunableCallback(virConnectPtr conn
ATTRIBUTE_UNUSED,
printf("%s EVENT: Domain %s(%d) tunable updated:\n",
__func__, virDomainGetName(dom), virDomainGetID(dom));
+#ifdef WIN32
+/* MinGW doesn't know the lld/llu so we have to use I64f/I64u instead. */
+# define LLD_FORMAT "%I64d"
+# define LLU_FORMAT "%I64u"
+#else /* WIN32 */
+# define LLD_FORMAT "%lld"
+# define LLU_FORMAT "%llu"
+#endif /* WIN32 */
+
for (i = 0; i < nparams; i++) {
switch (params[i].type) {
case VIR_TYPED_PARAM_INT:
@@ -485,10 +494,10 @@ myDomainEventTunableCallback(virConnectPtr conn
ATTRIBUTE_UNUSED,
printf("\t%s: %u\n", params[i].field, params[i].value.ui);
break;
case VIR_TYPED_PARAM_LLONG:
-printf("\t%s: %lld\n", params[i].field, params[i].value.l);
+printf("\t%s: "LLD_FORMAT"\n", params[i].field, params[i].value.l);
break;
case VIR_TYPED_PARAM_ULLONG:
-printf("\t%s: %llu\n", params[i].field, params[i].value.ul);
+printf("\t%s: "LLU_FORMAT"\n", params[i].field,
params[i].value.ul);
break;
case VIR_TYPED_PARAM_DOUBLE:
printf("\t%s: %g\n", params[i].field, params[i].value.d);
--
1.8.5.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/24/2014 07:40 PM, Michal Privoznik wrote:
On 24.09.2014 07:45, Jincheng Miao wrote:
In nodeGetFreePages, if startCell is given by '0',
and the max node number is '0' too. The for-loop
wouldn't be executed.
So convert it to while-loop.
Before:
virsh freepages --cellno 0 --pagesize 4
error: internal error: no suitable info found
After:
virsh freepages --cellno 0 --pagesize 4
4KiB: 472637
Signed-off-by: Jincheng Miao
---
src/nodeinfo.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 2459922..1fe190a 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -2042,18 +2042,19 @@ nodeGetFreePages(unsigned int npages,
}
lastCell = MIN(lastCell, startCell + cellCount);
+cell = startCell;
-for (cell = startCell; cell < lastCell; cell++) {
+do {
for (i = 0; i < npages; i++) {
unsigned int page_size = pages[i];
unsigned int page_free;
-if (virNumaGetPageInfo(cell, page_size, 0, NULL,
&page_free) < 0)
+if (virNumaGetPageInfo(cell++, page_size, 0, NULL,
&page_free) < 0)
goto cleanup;
counts[ncounts++] = page_free;
}
-}
+} while (cell < lastCell);
if (!ncounts) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
There's no need to switch to do-while loop. All what is needed is cell
<= lastCell in the for loop. I'm rewriting the patch and pushing. ACK.
Wait, if change the for condition to 'cell <= lastCell', and pass
startCell == -1,
the for-loop will execute twice, and will overwrite counts[1] which is
not allocated.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/24/2014 05:28 AM, zhang bo wrote:
> The patch
> http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=de0aeafe9ce3eb414c8b5d3aa8995d776a2952de
> removes invtsc flag in the host-model CPU.
>
> I'm wondering, will it be better to pass args "migratable=no/yes" to qemu,
> and let qemu complete the
> remaining work? As that qemu has checked whether it's necessary to use invtsc
> or not.
The 'migratable' property is only for -cpu host (
in libvirt).
For mode='host-model', libvirt detects the model and features of the host CPU
and passes it as -cpu ,+feat,+feat2,...
so we can't leave that to QEMU.
> --
> "invtsc is available only if using: -cpu host,migratable=no,+invtsc."
> http://git.qemu.org/?p=qemu.git;a=commit;h=120eee7d1fdb2eba15766cfff7b9bcdc902690b4
> --
>
> There's another problem, if we do not pass "migratable=no" to qemu.
> Consider if we set host mode to pass-through
>-
>
>
>-
> then the vm->def->cpu->features contains invtsc. however, qemu will
> automatically remove this cpu flag
> as that "migration=no" is not passed to it. thus, the guest will not start
> up. This problem is in fact
> caused by the patch:
> http://libvirt.org/git/?p=libvirt.git;a=commit;h=fba6bc47cbcabbe08d42279691efb0dff3b9c997,
> it forbids guest domain to start up if the host has INVTSC while the
> guest(qemu) does not.
Regardless of QEMU support for invtsc, I'm only able to start the domain,
restore or migration fails.
As far as I know, only 'invtsc' is the problematic feature, because it both
a) can appear in the host CPU (so libvirt assumes -cpu host will add it)
b) is checked by qemuProcessVerifyGuestCPU (and libvirt complains when it's
not there)
For other features, we only add them to qemu command line and let qemu filter
out the unsupported ones.
>-
> for (i = 0; def->cpu && i < def->cpu->nfeatures; i++) {
> virCPUFeatureDefPtr feature = &def->cpu->features[i];
>
> if (feature->policy != VIR_CPU_FEATURE_REQUIRE)
> continue;
>
> if (STREQ(feature->name, "invtsc") &&
> !cpuHasFeature(guestcpu, feature->name)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>_("host doesn't support invariant TSC"));
> goto cleanup;
> }
> }
> break;
>--
>
>
> In conclusion:
> 1 Will it better to pass args "migratable=yes/no" to qemu rather than doing
> the mask-invtsc job in libvirt?
> 2 If the guest has "pass-through" cpu mode, then it's unable to start up,
> because qemu removes invtsc, and
> vm->def->cpu->features has it. It seems a BUG.
>
I think the simplest fix for host-passthrough would be to apply the same
filter host-model has.
But since using invtsc with host-passthrough requires both +invtsc and
migratable=no, so we'd need to either add a 'migratable' option to
host-passthrough (this would skip the filter and add migratable=on), or allow
fine-tuning the features for host-passthrough too.
Jan
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 24.09.2014 07:45, Jincheng Miao wrote:
In nodeGetFreePages, if startCell is given by '0',
and the max node number is '0' too. The for-loop
wouldn't be executed.
So convert it to while-loop.
Before:
virsh freepages --cellno 0 --pagesize 4
error: internal error: no suitable info found
After:
virsh freepages --cellno 0 --pagesize 4
4KiB: 472637
Signed-off-by: Jincheng Miao
---
src/nodeinfo.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 2459922..1fe190a 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -2042,18 +2042,19 @@ nodeGetFreePages(unsigned int npages,
}
lastCell = MIN(lastCell, startCell + cellCount);
+cell = startCell;
-for (cell = startCell; cell < lastCell; cell++) {
+do {
for (i = 0; i < npages; i++) {
unsigned int page_size = pages[i];
unsigned int page_free;
-if (virNumaGetPageInfo(cell, page_size, 0, NULL, &page_free) < 0)
+if (virNumaGetPageInfo(cell++, page_size, 0, NULL, &page_free) < 0)
goto cleanup;
counts[ncounts++] = page_free;
}
-}
+} while (cell < lastCell);
if (!ncounts) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
There's no need to switch to do-while loop. All what is needed is cell
<= lastCell in the for loop. I'm rewriting the patch and pushing. ACK.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Sep 23, 2014 at 16:09:56 +0200, Cristian Klein wrote:
> Signed-off-by: Cristian Klein
> ---
> src/qemu/qemu_migration.c| 1 +
> src/qemu/qemu_monitor.c | 2 +-
> src/qemu/qemu_monitor.h | 1 +
> src/qemu/qemu_monitor_json.c | 3 ++-
> src/qemu/qemu_monitor_text.c | 3 ++-
> 5 files changed, 7 insertions(+), 3 deletions(-)
>
...
> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index 46d2782..a3c8aa5 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -1458,7 +1458,8 @@ int qemuMonitorTextGetMigrationStatus(qemuMonitorPtr
> mon,
> goto cleanup;
> }
>
> -if (status->status == QEMU_MONITOR_MIGRATION_STATUS_ACTIVE) {
> +if (status->status == QEMU_MONITOR_MIGRATION_STATUS_ACTIVE ||
> +status->status == QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY_ACTIVE)
> {
> tmp = end + 1;
I don't think we need to change text monitor code since we won't be
talking with HMP to any version of QEMU that supports post-copy
migration. Otherwise the patch is just fine and we just need to wait for
post-copy to land in QEMU.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
This new attribute will control whether or not libvirt will pay
attention to guest notifications about changes to network device mac
addresses and receive filters. The default for this is 'no' (for
security reasons). If it is set to 'yes' *and* the specified device
model and connection support it (currently only macvtap+virtio) then
libvirt will watch for NIC_RX_FILTER_CHANGED events, and when it
receives one, it will issue a query-rx-filter command, retrieve the
result, and modify the host-side macvtap interface's mac address and
unicast/multicast filters accordingly.
The functionality behind this attribute will be in a later patch. This
patch merely adds the attribute to the top-level of a domain's
as well as to and , and adds
documentation and schema/xml2xml tests. Rather than adding even more
test files, I've just added the net attribute in various applicable
places of existing test files.
---
docs/formatdomain.html.in | 38
docs/formatnetwork.html.in | 28 +--
docs/schemas/domaincommon.rng | 5 +++
docs/schemas/network.rng | 10 ++
src/conf/domain_conf.c | 42 ++
src/conf/domain_conf.h | 3 ++
src/conf/network_conf.c| 35 ++
src/conf/network_conf.h| 2 ++
src/libvirt_private.syms | 1 +
tests/networkxml2xmlin/vepa-net.xml| 4 +--
tests/networkxml2xmlout/vepa-net.xml | 4 +--
.../qemuxml2argv-net-virtio-network-portgroup.xml | 4 +--
12 files changed, 160 insertions(+), 16 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index eefdd5e..17e180d 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3343,10 +3343,9 @@
...
-
-
-
-
+
+
+
@@ -3356,8 +3355,21 @@
There are several possibilities for specifying a network
interface visible to the guest. Each subsection below provides
- more details about common setup options. Additionally,
- each element has an
+ more details about common setup options.
+
+
+ libvirt allows specifying when the host should trust reports
+ from the guest of changes to the interface mac address and
+ receive filters by setting the trustGuestRxFilters
+ attribute to yesSince
+ 1.2.9. trustGuestRxFilters defaults
+ to no for security reasons, and support depends on
+ the guest network device driver as well as the type of
+ connection on the host - currently it is only supported for the
+ virtio driver, and for macvtap connections on the host.
+
+
+ Each element has an
optional sub-element that can tie
the interface to a particular pci slot, with
attribute type='pci'
@@ -3589,6 +3601,18 @@
being the default mode. The individual modes cause the delivery of
packets to behave as follows:
+
+ If the model type is set to virtio and
+ interface's trustGuestRxFilters attribute is set
+ to yes, changes made to the interface mac address,
+ unicast/multicast receive filters, and vlan settings in the
+ guest will be monitored and propogated to the associated macvtap
+ device on the host. Since
+ 1.2.9. If trustGuestRxFilters is not set, or
+ is not supported for the device model in use, an attempted
+ change to the mac address originating from the guest side will
+ result in a non-working network connection.
+
vepa
@@ -3621,7 +3645,7 @@
...
...
-
+
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 1a8ad8e..ff73952 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -35,7 +35,7 @@
-
+ default3e3fce45-4f53-4fa7-bb32-11f34168b82b
...
@@ -60,6 +60,16 @@
to have guest-to-guest communications. For further information,
see the example below for the example with no gateway addresses.
Since 1.0.1
+ trustGuestRxFilters='yes'
+ The optional parameter trustGuestRxFilters can
+be used to set that attribute of the same name for e
These patches set up an event handler for qemu's NIC_RX_FILTER_CHANGED
event, which is sent whenever a guest makes a change to a network
device's unicast/multicast filter, vlan table, or MAC address.
The handler checks if it is appropriate to respond to the
NIC_RX_FILTER_CHANGED event (based on device type and configuration)
and takes appropriate action. Currently it checks if the guest
interface has been configured with trustGuestRxFilters='yes' (defaults
to 'no' for security reasons), and if the host side device is
macvtap. If so, and the MAC address on the guest has changed, the MAC
address of the macvtap device is changed to match.
The result of this is that networking from the guest will continue to
work if the mac address of a macvtap-connected network device is
changed from within the guest, as long as trustGuestRxFilters='yes'
(previously changing the MAC address in the guest would break
networking).
I still need to add code to compare the old and new unicast and
multicast lists and program the filters in the macvtap to match the
guest, and to check for a non-empty vlan table and handle that
(currently that means just setting promiscuous mode on the macvtap),
but that can come in a followup series.
Laine Stump (6):
conf: add trustGuestRxFilters attribute to network and domain
interface
network: set interface actual trustGuestRxFilters from
network/portgroup
util: define virNetDevRxFilter and basic utility functions
qemu: qemuMonitorQueryRxFilter - retrieve guest netdev rx-filter
qemu: setup infrastructure to handle NIC_RX_FILTER_CHANGED event
qemu: change macvtap device MAC address in response to
NIC_RX_FILTER_CHANGED
docs/formatdomain.html.in | 38 +++-
docs/formatnetwork.html.in | 28 ++-
docs/schemas/domaincommon.rng | 5 +
docs/schemas/network.rng | 10 +
src/conf/domain_conf.c | 42
src/conf/domain_conf.h | 3 +
src/conf/network_conf.c| 35
src/conf/network_conf.h| 2 +
src/libvirt_private.syms | 9 +
src/network/bridge_driver.c| 11 +
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_driver.c | 105 ++
src/qemu/qemu_monitor.c| 39
src/qemu/qemu_monitor.h| 11 +
src/qemu/qemu_monitor_json.c | 232 +
src/qemu/qemu_monitor_json.h | 3 +
src/qemu/qemu_process.c| 42
src/util/virnetdev.c | 40
src/util/virnetdev.h | 57 -
tests/Makefile.am | 3 +
tests/networkxml2xmlin/vepa-net.xml| 4 +-
tests/networkxml2xmlout/vepa-net.xml | 4 +-
.../qemuxml2argv-net-virtio-network-portgroup.xml | 4 +-
23 files changed, 711 insertions(+), 17 deletions(-)
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
This patch fills in the functionality of
processNicRxFilterChangedEvent(). It now checks if it is appropriate
to respond to the NIC_RX_FILTER_CHANGED event (based on device type
and configuration) and takes appropriate action. Currently it checks
if the guest interface has been configured with
trustGuestRxFilters='yes', and if the host side device is macvtap. If
so, and the MAC address on the guest has changed, the MAC address of
the macvtap device is changed to match.
The result of this is that networking from the guest will continue to
work if the mac address of a macvtap-connected network device is
changed from within the guest, as long as trustGuestRxFilters='yes'
(previously changing the MAC address in the guest would break
networking).
---
src/qemu/qemu_driver.c | 50 ++
1 file changed, 50 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 64f1d45..7801d91 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4146,8 +4146,13 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
char *devAlias)
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainDeviceDef dev;
virDomainNetDefPtr def;
+virNetDevRxFilterPtr filter = NULL;
+virMacAddr oldMAC;
+char newMacStr[VIR_MAC_STRING_BUFLEN];
+int ret;
VIR_DEBUG("Received NIC_RX_FILTER_CHANGED event for device %s "
"from domain %p %s",
@@ -4175,11 +4180,55 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
}
def = dev.data.net;
+if (!virDomainNetGetActualTrustGuestRxFilters(def)) {
+VIR_WARN("ignore NIC_RX_FILTER_CHANGED event for network "
+ "device %s in domain %s",
+ def->info.alias, vm->def->name);
+/* not sending "query-rx-filter" will also suppress any
+ * further NIC_RX_FILTER_CHANGED events for this device
+ */
+goto endjob;
+}
+
/* handle the event - send query-rx-filter and respond to it. */
VIR_DEBUG("process NIC_RX_FILTER_CHANGED event for network "
"device %s in domain %s", def->info.alias, vm->def->name);
+qemuDomainObjEnterMonitor(driver, vm);
+ret = qemuMonitorQueryRxFilter(priv->mon, devAlias, &filter);
+qemuDomainObjExitMonitor(driver, vm);
+if (ret < 0)
+goto endjob;
+
+virMacAddrFormat(&filter->mac, newMacStr);
+
+if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_DIRECT) {
+
+/* For macvtap connections, set the macvtap device's MAC
+ * address to match that of the guest device.
+ */
+
+if (virNetDevGetMAC(def->ifname, &oldMAC) < 0) {
+VIR_WARN("Couldn't get current MAC address of device %s "
+ "while responding to NIC_RX_FILTER_CHANGED",
+ def->ifname);
+goto endjob;
+}
+
+if (virMacAddrCmp(&oldMAC, &filter->mac)) {
+/* set new MAC address from guest to associated macvtap device */
+if (virNetDevSetMAC(def->ifname, &filter->mac) < 0) {
+VIR_WARN("Couldn't set new MAC address %s to device %s "
+ "while responding to NIC_RX_FILTER_CHANGED",
+ newMacStr, def->ifname);
+} else {
+VIR_DEBUG("device %s MAC address set to %s",
+ def->ifname, newMacStr);
+}
+}
+}
+
endjob:
/* We had an extra reference to vm before starting a new job so ending the
* job is guaranteed not to remove the last reference.
@@ -4187,6 +4236,7 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
ignore_value(qemuDomainObjEndJob(driver, vm));
cleanup:
+virNetDevRxFilterFree(filter);
VIR_FREE(devAlias);
virObjectUnref(cfg);
}
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
NIC_RX_FILTER_CHANGED is sent by qemu any time a NIC driver in the
guest modified the NIC's RX Filter (for example, if the MAC address of
the NIC is changed by the guest).
This patch doesn't do anything useful with that event; it just sets up
all the plumbing to get news of the event into a worker thread with
all proper locking/reference counting, and provide an easy place to
add in desired functionality.
For easy reference the next time a handler for a qemu event is
written, here is the sequence:
The handler in qemu_json_monitor.c calls
qemu_json_monitor.c:qemuMonitorJSONHandleNicRxFilterChanged()
which calls
qemu_monitor.c:qemuMonitorEmitNicRxFilterChanged()
which uses QEMU_MONITOR_CALLBACK() to call
mon->cb->domainNicRxFilterChanged(), ie:
qemuProcessHandleNicRxFilterChanged()
which will queue an event QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED for
a worker thread to handle.
When the worker thread gets the event, it calls
qemuProcessEventHandler()
which calls
processNicRxFilterChangedEvent()
and *that* is where the actual work will be done (and any
event-specific memory allocated during qemuProcessHandleXXX() will be
freed) - it is the middle of this function where functionality behind
the event will be added in the next patch; for now there is just a
VIR_DEBUG() to log the event.
---
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_driver.c | 55
src/qemu/qemu_monitor.c | 13 +++
src/qemu/qemu_monitor.h | 7 ++
src/qemu/qemu_monitor_json.c | 17 ++
src/qemu/qemu_process.c | 42 +
6 files changed, 135 insertions(+)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 845d3c7..ad45a66 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -195,6 +195,7 @@ typedef enum {
QEMU_PROCESS_EVENT_WATCHDOG = 0,
QEMU_PROCESS_EVENT_GUESTPANIC,
QEMU_PROCESS_EVENT_DEVICE_DELETED,
+QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED,
QEMU_PROCESS_EVENT_LAST
} qemuProcessEventType;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 543de79..64f1d45 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4140,6 +4140,58 @@ processDeviceDeletedEvent(virQEMUDriverPtr driver,
}
+static void
+processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ char *devAlias)
+{
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+virDomainDeviceDef dev;
+virDomainNetDefPtr def;
+
+VIR_DEBUG("Received NIC_RX_FILTER_CHANGED event for device %s "
+ "from domain %p %s",
+ devAlias, vm, vm->def->name);
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
+if (!virDomainObjIsActive(vm)) {
+VIR_DEBUG("Domain is not running");
+goto endjob;
+}
+
+if (virDomainDefFindDevice(vm->def, devAlias, &dev, true) < 0) {
+VIR_WARN("NIC_RX_FILTER_CHANGED event received for "
+ "non-existent device %s in domain %s",
+ devAlias, vm->def->name);
+goto endjob;
+}
+if (dev.type != VIR_DOMAIN_DEVICE_NET) {
+VIR_WARN("NIC_RX_FILTER_CHANGED event received for "
+ "non-network device %s in domain %s",
+ devAlias, vm->def->name);
+goto endjob;
+}
+def = dev.data.net;
+
+/* handle the event - send query-rx-filter and respond to it. */
+
+VIR_DEBUG("process NIC_RX_FILTER_CHANGED event for network "
+ "device %s in domain %s", def->info.alias, vm->def->name);
+
+ endjob:
+/* We had an extra reference to vm before starting a new job so ending the
+ * job is guaranteed not to remove the last reference.
+ */
+ignore_value(qemuDomainObjEndJob(driver, vm));
+
+ cleanup:
+VIR_FREE(devAlias);
+virObjectUnref(cfg);
+}
+
+
static void qemuProcessEventHandler(void *data, void *opaque)
{
struct qemuProcessEvent *processEvent = data;
@@ -4160,6 +4212,9 @@ static void qemuProcessEventHandler(void *data, void
*opaque)
case QEMU_PROCESS_EVENT_DEVICE_DELETED:
processDeviceDeletedEvent(driver, vm, processEvent->data);
break;
+case QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED:
+processNicRxFilterChangedEvent(driver, vm, processEvent->data);
+break;
case QEMU_PROCESS_EVENT_LAST:
break;
}
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 48cbe3e..a4661f8 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1387,6 +1387,19 @@ qemuMonitorEmitDeviceDeleted(qemuMonitorPtr mon,
}
+int
+qemuMonitorEmitNicRxFilterChanged(qemuMonitorPtr mon,
+ const char *devAlias)
+{
+int ret = -1;
+VIR_DEBUG("mon=%p", mon);
+
+QEMU_MONITOR_CALLBACK(mon, ret, domainNicRxFilterChang
This function can be called at any time to get the current status of a
guest's network device rx-filter. In particular it is useful to call
after libvirt recieves a NIC_RX_FILTER_CHANGED event - this event only
tells you that something has changed in the rx-filter, the details are
retrieved with the query-rx-filter monitor command (only available in
the json monitor). The commend sent to the qemu monitor looks like this:
{"execute":"query-rx-filter", "arguments": {"name":"net2"} }'
and the results will look something like this:
{
"return": [
{
"promiscuous": false,
"name": "net2",
"main-mac": "52:54:00:98:2d:e3",
"unicast": "normal",
"vlan": "normal",
"vlan-table": [
42,
0
],
"unicast-table": [
],
"multicast": "normal",
"multicast-overflow": false,
"unicast-overflow": false,
"multicast-table": [
"33:33:ff:98:2d:e3",
"01:80:c2:00:00:21",
"01:00:5e:00:00:fb",
"33:33:ff:98:2d:e2",
"01:00:5e:00:00:01",
"33:33:00:00:00:01"
],
"broadcast-allowed": false
}
],
"id": "libvirt-14"
}
This is all parsed from JSON into a virNetDevRxFilter object for
easier consumption. (unicast-table is usually empty, but is also an
array of mac addresses similar to multicast-table).
(NB: LIBNL_CFLAGS was added to tests/Makefile.am because virnetdev.h
now includes util/virnetlink.h, which includes netlink/msg.h when
appropriate. Without LIBNL_CFLAGS, gcc can't find that file (if
libnl/netlink isn't available, LIBNL_CFLAGS will be empty and
virnetlink.h won't try to include netlink/msg.h anyway).)
---
src/qemu/qemu_monitor.c | 26 ++
src/qemu/qemu_monitor.h | 4 +
src/qemu/qemu_monitor_json.c | 215 +++
src/qemu/qemu_monitor_json.h | 3 +
tests/Makefile.am| 3 +
5 files changed, 251 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index c25f002..48cbe3e 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2918,6 +2918,32 @@ int qemuMonitorRemoveNetdev(qemuMonitorPtr mon,
}
+int
+qemuMonitorQueryRxFilter(qemuMonitorPtr mon, const char *alias,
+ virNetDevRxFilterPtr *filter)
+{
+int ret = -1;
+VIR_DEBUG("mon=%p alias=%s filter=%p",
+ mon, alias, filter);
+
+if (!mon) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("monitor must not be NULL"));
+return -1;
+}
+
+
+VIR_DEBUG("mon=%p, alias=%s", mon, alias);
+
+if (mon->json)
+ret = qemuMonitorJSONQueryRxFilter(mon, alias, filter);
+else
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("query-rx-filter requires JSON monitor"));
+return ret;
+}
+
+
int qemuMonitorGetPtyPaths(qemuMonitorPtr mon,
virHashTablePtr paths)
{
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 6b91e29..c37e36f 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -31,6 +31,7 @@
# include "virbitmap.h"
# include "virhash.h"
# include "virjson.h"
+# include "virnetdev.h"
# include "device_conf.h"
# include "cpu/cpu.h"
@@ -622,6 +623,9 @@ int qemuMonitorAddNetdev(qemuMonitorPtr mon,
int qemuMonitorRemoveNetdev(qemuMonitorPtr mon,
const char *alias);
+int qemuMonitorQueryRxFilter(qemuMonitorPtr mon, const char *alias,
+ virNetDevRxFilterPtr *filter);
+
int qemuMonitorGetPtyPaths(qemuMonitorPtr mon,
virHashTablePtr paths);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index a3d7c2c..58007e6 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3194,6 +3194,221 @@ int qemuMonitorJSONRemoveNetdev(qemuMonitorPtr mon,
}
+static int
+qemuMonitorJSONQueryRxFilterParse(virJSONValuePtr msg,
+ virNetDevRxFilterPtr *filter)
+{
+int ret = -1;
+const char *tmp;
+virJSONValuePtr returnArray, entry, table, element;
+int nTable;
+size_t i;
+virNetDevRxFilterPtr fil = virNetDevRxFilterNew();
+
+if (!(returnArray = virJSONValueObjectGet(msg, "return"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("query-rx-filter reply was missing return data"));
+goto cleanup;
+}
+if (returnArray->type != VIR_JSON_TYPE_ARRAY) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("query-rx-filter return data was not an array"));
+goto cleanup;
+}
+if (!(entry = virJSONValueArrayGet(returnArray, 0))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+
As is done with other items such as vlan, virtualport, and bandwidth,
set the actual trustGuestRxFilters value to be used by a domain
interface according to a merge of the same attribute in the interface,
portgroup, and network in use. the interface setting always takes
precedence (if specified), followed by portgroup, and finally the
setting in the network is used if it's not specified in the interface
or portgroup.
---
src/network/bridge_driver.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 979fb13..548e354 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -3794,6 +3794,17 @@ networkAllocateActualDevice(virDomainDefPtr dom,
if (vlan && virNetDevVlanCopy(&iface->data.network.actual->vlan, vlan) < 0)
goto error;
+if (iface->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT)
+ iface->data.network.actual->trustGuestRxFilters
+ = iface->trustGuestRxFilters;
+else if (portgroup &&
+ portgroup->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT)
+ iface->data.network.actual->trustGuestRxFilters
+ = portgroup->trustGuestRxFilters;
+else if (netdef->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT)
+ iface->data.network.actual->trustGuestRxFilters
+ = netdef->trustGuestRxFilters;
+
if ((netdef->forward.type == VIR_NETWORK_FORWARD_NONE) ||
(netdef->forward.type == VIR_NETWORK_FORWARD_NAT) ||
(netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE)) {
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Am 23.09.2014 um 10:47 hat Alexey Kardashevskiy geschrieben:
> On 09/19/2014 06:47 PM, Kevin Wolf wrote:> Am 16.09.2014 um 14:59 hat Paolo
> Bonzini geschrieben:
> >> Il 16/09/2014 14:52, Kevin Wolf ha scritto:
> >>> Yes, that's true. We can't fix this problem in qcow2, though, because
> >>> it's a more general one. I think we must make sure that
> >>> bdrv_invalidate_cache() doesn't yield.
> >>>
> >>> Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
> >>> moving the problem to the caller (where and why is it even called from a
> >>> coroutine?), or possibly by creating a new coroutine for the driver
> >>> callback and running that in a nested event loop that only handles
> >>> bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
> >>> chance to process new requests in this thread.
> >>
> >> Incoming migration runs in a coroutine (the coroutine entry point is
> >> process_incoming_migration_co). But everything after qemu_fclose() can
> >> probably be moved into a separate bottom half, so that it gets out of
> >> coroutine context.
> >
> > Alexey, you should probably rather try this (and add a bdrv_drain_all()
> > in bdrv_invalidate_cache) than messing around with qcow2 locks. This
> > isn't a problem that can be completely fixed in qcow2.
>
>
> Ok. Tried :) Not very successful though. The patch is below.
>
> Is that the correct bottom half? When I did it, I started getting crashes
> in various sport on accesses to s->l1_cache which is NULL after qcow2_close.
> Normally the code would check s->l1_size and then use but they are out of
> sync.
No, that's not the place we were talking about.
What Paolo meant is that in process_incoming_migration_co(), you can
split out the final part that calls bdrv_invalidate_cache_all() into a
BH (you need to move everything until the end of the function into the
BH then). His suggestion was to move everything below the qemu_fclose().
> So I clear it in qcow2_close(). This allowed migrated guest to work and not
> to crash until I shut it down when it aborted at "HERE IT FAILS ON SHUTDOWN".
>
> Here I realized I am missing something in this picture again, what is it?
The problem with your patch seems to be that you close the image and
then let the VM access the image before it is reopened in the BH. That
can't work out well. This is why it's important that the vm_start() call
is in the BH, too.
> void bdrv_invalidate_cache_all(Error **errp)
> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> index 904f6b1..59ff48c 100644
> --- a/block/qcow2-cache.c
> +++ b/block/qcow2-cache.c
> @@ -259,7 +259,7 @@ static int qcow2_cache_find_entry_to_replace(Qcow2Cache
> *c)
> if (min_index == -1) {
> /* This can't happen in current synchronous code, but leave the check
> * here as a reminder for whoever starts using AIO with the cache */
> -abort();
> +abort(); // < HERE IT FAILS ON SHUTDOWN
> }
> return min_index;
> }
It's a weird failure mode anyway...
Kevin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/24/2014 11:45 AM, Peter Krempa wrote:
On 09/24/14 11:36, Pavel Hrdina wrote:
If you have a bridge network in running domain and libvirtd is restarted
the information about host bridge interface is lost from live xml.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140085
Signed-off-by: Pavel Hrdina
---
changes in v2:
- added error message if bridge name is missing.
src/conf/domain_conf.c | 9 +
1 file changed, 9 insertions(+)
ACK
Thanks, pushed.
Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/24/14 11:36, Pavel Hrdina wrote:
> If you have a bridge network in running domain and libvirtd is restarted
> the information about host bridge interface is lost from live xml.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140085
>
> Signed-off-by: Pavel Hrdina
> ---
>
> changes in v2:
> - added error message if bridge name is missing.
>
> src/conf/domain_conf.c | 9 +
> 1 file changed, 9 insertions(+)
>
ACK
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
If you have a bridge network in running domain and libvirtd is restarted
the information about host bridge interface is lost from live xml.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140085
Signed-off-by: Pavel Hrdina
---
changes in v2:
- added error message if bridge name is missing.
src/conf/domain_conf.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9cc118c..0a7d0b8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6850,6 +6850,15 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
goto error;
}
VIR_FREE(class_id);
+} else if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
+char *brname = virXPathString("string(./source/@bridge)", ctxt);
+if (!brname) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Missing element with bridge name in "
+ "interface's element"));
+goto error;
+}
+actual->data.bridge.brname = brname;
}
bandwidth_node = virXPathNode("./bandwidth", ctxt);
--
1.8.5.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/18/14 15:31, Pavel Hrdina wrote:
> If you have a bridge network in running domain and libvirtd is restarted
> the information about host bridge interface is lost from live xml.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140085
>
> Signed-off-by: Pavel Hrdina
> ---
> src/conf/domain_conf.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3ccec1c..fa4166c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6837,6 +6837,10 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
> goto error;
> }
> VIR_FREE(class_id);
> +} else if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> +char *brname = virXPathString("string(./source/@bridge)", ctxt);
> +if (brname)
> +actual->data.bridge.brname = brname;
> }
>
> bandwidth_node = virXPathNode("./bandwidth", ctxt);
>
Parsers for other network types error out if the requested field is not
present. Also the intermediate variable shouldn't be necessary.
I'd like to see a version with the error before I ACK it, although the
idea is ok.
Peter
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Markus,
On 24 Sep 2014, at 09:05, Markus Armbruster wrote:
> Alex Bligh writes:
>
>> This patch series adds inbound migrate capability from qemu-kvm version
>> 1.0. The main ideas are those set out in Cole Robinson's patch here:
>> http://pkgs.fedoraproject.org/cgit/qemu.git/tree/0001-Fix-migration-from-qemu-kvm.patch?h=f20
>> however, rather than patching statically (and breaking inbound
>> migration on existing machine types), I have added a new machine
>> parameter (qemu-kvm-migration) which when turned on affects the pc-1.0
>> machine type. Usage:
>>-machine pc-1.0,qemu-kvm-migration=on
>
> Forgive me if this has been discussed already: why not simply a separate
> machine type "pc-1.0-qemu-kvm"?
That's what v2 of the patch set does (and I prefer v2). However, mst
wanted it done this way.
>
>> Three aproaches are taken:
>>
>> * cirrus-vga.vgamem_mb defaults to 16 rather than 8. In order to
>> keep -global cirrus-vga.vgamem_mb working even with
>> qemu-kvm-migration=on, this is monkey-patchedinto the default
>> valueof the MachineState structure's compat_props
>> list.
>
> This part fires only for pc-1.0, because it's in
> pc_early_init_pci_1_0().
Yes, intentional. I should have noted that. That's because
qemu-kvm-migration=on the VRAM change was designed to
work only with pc-1.0. I haven't looked at trying to make
other (older) qemu-kvm machine migrations work, but (with the stuff
below), I would guess it would work with the appropriate
command line options for VRAM size.
Obviously I could put early init elsewhere.
>> * In hw/timer/i8254_common.c, the VMSTATE_UINT32_TEST macro
>> is used to test the version for the irq_disable flags,
>> allowing version 3 or more, or version 2 for an inbound
>> migrate from qemu-kvm (only).
>>
>> * In hw/acpi/piix4.c, qemu-kvm incorrectly uses version 2 for
>> a version 3 structure, causing acpi_load_old to be used.
>> acpi_load_old detects this situation based on the machine type
>> and restarts the attempt to load the vmstate using a
>> customised VMStateDescription. The above cleaner approach is
>> unavailable here.
>
> These parts apply to all machine types, don't they?
They apply only when qemu-kvm-migration=on is selected, but to
any machine type; however machine types newer than pc-1.0 will
be exporting v3 I think anyway.
>> Changes since v1:
>> * Do not use a machine type, use a machine parameter.
>
> Okay, it has been discussed already. I'd appreciate a brief recap all
> the same.
See above. I preferred the machine type. But I got to learn more about
QOM on the way :-)
--
Alex Bligh
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/23/14 22:35, John Ferlan wrote:
>
>
> On 09/18/2014 05:54 AM, Peter Krempa wrote:
>> Report also the name of the parent file and uid/gid used to access it to
>> help debugging broken storage configurations.
>> ---
>> src/storage/storage_driver.c | 20 +++-
>> 1 file changed, 15 insertions(+), 5 deletions(-)
>>
>
> ACK
>
Thanks. I've fixed the nits you've pointed out and pushed this series.
Peter
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Alex Bligh writes:
> This patch series adds inbound migrate capability from qemu-kvm version
> 1.0. The main ideas are those set out in Cole Robinson's patch here:
> http://pkgs.fedoraproject.org/cgit/qemu.git/tree/0001-Fix-migration-from-qemu-kvm.patch?h=f20
> however, rather than patching statically (and breaking inbound
> migration on existing machine types), I have added a new machine
> parameter (qemu-kvm-migration) which when turned on affects the pc-1.0
> machine type. Usage:
> -machine pc-1.0,qemu-kvm-migration=on
Forgive me if this has been discussed already: why not simply a separate
machine type "pc-1.0-qemu-kvm"?
> Three aproaches are taken:
>
> * cirrus-vga.vgamem_mb defaults to 16 rather than 8. In order to
> keep -global cirrus-vga.vgamem_mb working even with
> qemu-kvm-migration=on, this is monkey-patchedinto the default
> valueof the MachineState structure's compat_props
> list.
This part fires only for pc-1.0, because it's in
pc_early_init_pci_1_0().
> * In hw/timer/i8254_common.c, the VMSTATE_UINT32_TEST macro
> is used to test the version for the irq_disable flags,
> allowing version 3 or more, or version 2 for an inbound
> migrate from qemu-kvm (only).
>
> * In hw/acpi/piix4.c, qemu-kvm incorrectly uses version 2 for
> a version 3 structure, causing acpi_load_old to be used.
> acpi_load_old detects this situation based on the machine type
> and restarts the attempt to load the vmstate using a
> customised VMStateDescription. The above cleaner approach is
> unavailable here.
These parts apply to all machine types, don't they?
> The above monkey-patching must be done between the selection of
> the MachineClass and the processing of the machine parameters
> (on theone hand) and the processing of the compat_props list
> and theglobals on the command line. To do this I
> have added
> an earlyinit function to MachineState and QEMUMachine.
>
> I developed this on qemu 2.0 but have forward ported it (trivially)
> to master. My testing has been on a VM live-migrated-to-file from
> Ubuntu Precise qemu-kvm 1.0.
>
> I have given this a moderate degree of testing but it could do
> with more.
>
> Note that certain hardware devices (including QXL) will not
> migrate properly due to a fundamental difference in their internal
> state between versions.
>
> Also note that (as expected) migration from qemu-2.x to qemu-1.0
> will not work, even if the machine types are the same.
>
> Changes since v1:
> * Do not use a machine type, use a machine parameter.
Okay, it has been discussed already. I'd appreciate a brief recap all
the same.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/23/2014 11:50 PM, Eric Blake wrote:
On 09/23/2014 03:26 PM, Pavel Hrdina wrote:
+
+VIR_DEBUG("Relaying domain tunable event %s %d, callback %d",
+ dom->name, dom->id, callback->callbackID);
+
Might also be nice to log "%p %n", params, nparams
Yes, that would be probably nice, but since I've pushed this patch
already I can create a following patch with this small update?
Yes, a followup is fine.
+++ b/src/remote/remote_protocol.x
@@ -247,6 +247,9 @@ const REMOTE_NETWORK_DHCP_LEASES_MAX = 65536;
/* Upper limit on count of parameters returned via bulk stats API */
const REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX = 4096;
+/* Upper limit of message size for tunable event. */
+const REMOTE_DOMAIN_EVENT_TUNABLE_MAX = 8388608;
That feels excessive...
+
/* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */
typedef opaque remote_uuid[VIR_UUID_BUFLEN];
@@ -2990,6 +2993,12 @@ struct remote_domain_event_block_job_2_msg {
int status;
};
+struct remote_domain_event_callback_tunable_msg {
+int callbackID;
+remote_nonnull_domain dom;
+remote_typed_param params;
...each param in the array will occupy multiple bytes. I think that
something as low as 2048 for REMOTE_DOMAIN_EVENT_TUNABLE_MAX is still
plenty (we don't have that many tunables yet); even if each tunable
requires 64 bytes to transmit (mostly in the name of the parameter, but
also in the type and value), that's still well under a megabyte limit of
information passed on an instance of the event.
Well, yes and no :). Let's say, that we will add in the future (and I'm
planning to do it) blkiotune where you can update at the same time all
of the tunables for all host's disks where all params for now will be
only VIR_TYPED_PARAM_STRING and that could consume a lot of memory. I
know that it will probably never be that much, but I wanted to be sure
that we will have enough space for all possible tunable events.
Still, are you going to return 8 million separate strings? Or just 8
million bytes but still contained within 2000 strings? Seriously, I
think 2048 is a perfectly LARGE limit - there are not THAT many tunables
per domain. The params is not the overall size of the command,
but the number of parameters (each of which can be quite large if they
are type string)
Sigh, I should not work that late, because I've misunderstood the
meaning of the "LIMIT". I'll post a new value with patch for the
debug message.
Thanks, Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/23/2014 06:47 PM, Alexey Kardashevskiy wrote:
> On 09/19/2014 06:47 PM, Kevin Wolf wrote:> Am 16.09.2014 um 14:59 hat Paolo
> Bonzini geschrieben:
>>> Il 16/09/2014 14:52, Kevin Wolf ha scritto:
Yes, that's true. We can't fix this problem in qcow2, though, because
it's a more general one. I think we must make sure that
bdrv_invalidate_cache() doesn't yield.
Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
moving the problem to the caller (where and why is it even called from a
coroutine?), or possibly by creating a new coroutine for the driver
callback and running that in a nested event loop that only handles
bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
chance to process new requests in this thread.
>>>
>>> Incoming migration runs in a coroutine (the coroutine entry point is
>>> process_incoming_migration_co). But everything after qemu_fclose() can
>>> probably be moved into a separate bottom half, so that it gets out of
>>> coroutine context.
>>
>> Alexey, you should probably rather try this (and add a bdrv_drain_all()
>> in bdrv_invalidate_cache) than messing around with qcow2 locks. This
>> isn't a problem that can be completely fixed in qcow2.
>
>
> Ok. Tried :) Not very successful though. The patch is below.
>
> Is that the correct bottom half? When I did it, I started getting crashes
> in various sport on accesses to s->l1_cache which is NULL after qcow2_close.
> Normally the code would check s->l1_size and then use but they are out of
> sync.
>
> So I clear it in qcow2_close(). This allowed migrated guest to work and not
> to crash until I shut it down when it aborted at "HERE IT FAILS ON SHUTDOWN".
>
> Here I realized I am missing something in this picture again, what is it?
> Thanks!
To be more precise, I can remove that abort() and it seems working for a
while but when shutting migrated guest down, the disk fails:
Will now unmount local filesystems:sd 0:0:0:0: [sda]
Result: hostbyte=0x00 driverbyte=0x08
sd 0:0:0:0: [sda]
Sense Key : 0xb [current]
sd 0:0:0:0: [sda]
ASC=0x0 ASCQ=0x6
sd 0:0:0:0: [sda] CDB:
cdb[0]=0x2a: 2a 00 00 3c 10 10 00 00 08 00
end_request: I/O error, dev sda, sector 3936272
end_request: I/O error, dev sda, sector 3936272
Buffer I/O error on device sda, logical block 492034
lost page write due to I/O error on sda
JBD2: Error -5 detected when updating journal superblock for sda-8.
[...]
spapr-vscsi or virtio-scsi - does not matter. Or crash:
Program received signal SIGSEGV, Segmentation fault.
0x1050a69c in qcow2_cache_find_entry_to_replace (c=0x10038317bb0)
at /home/alexey/p/qemu/block/qcow2-cache.c:256
(gdb) l
251 min_count = c->entries[i].cache_hits;
(gdb) p i
$2 = 0xfd6
(gdb) p c->size
$3 = 0x3ffe
(gdb) p c->entries[i]
$5 = {
table = 0x804dd70210,
offset = 0x40,
dirty = 0x0,
cache_hits = 0xee498,
ref = 0x0
}
Weird things are happening, that's my point :)
>
>
> ---
> block.c | 2 ++
> block/qcow2-cache.c | 2 +-
> block/qcow2.c | 50 --
> block/qcow2.h | 4
> 4 files changed, 43 insertions(+), 15 deletions(-)
>
> diff --git a/block.c b/block.c
> index d06dd51..1e6dfd1 100644
> --- a/block.c
> +++ b/block.c
> @@ -5044,6 +5044,8 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error
> **errp)
> error_setg_errno(errp, -ret, "Could not refresh total sector count");
> return;
> }
> +
> +bdrv_drain_all();
> }
>
> void bdrv_invalidate_cache_all(Error **errp)
> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> index 904f6b1..59ff48c 100644
> --- a/block/qcow2-cache.c
> +++ b/block/qcow2-cache.c
> @@ -259,7 +259,7 @@ static int qcow2_cache_find_entry_to_replace(Qcow2Cache
> *c)
> if (min_index == -1) {
> /* This can't happen in current synchronous code, but leave the check
> * here as a reminder for whoever starts using AIO with the cache */
> -abort();
> +abort(); // < HERE IT FAILS ON SHUTDOWN
> }
> return min_index;
> }
> diff --git a/block/qcow2.c b/block/qcow2.c
> index f9e045f..2b84562 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1399,6 +1399,7 @@ static void qcow2_close(BlockDriverState *bs)
> qemu_vfree(s->l1_table);
> /* else pre-write overlap checks in cache_destroy may crash */
> s->l1_table = NULL;
> +s->l1_size = 0;
>
> if (!(bs->open_flags & BDRV_O_INCOMING)) {
> qcow2_cache_flush(bs, s->l2_table_cache);
> @@ -1419,16 +1420,11 @@ static void qcow2_close(BlockDriverState *bs)
> qcow2_free_snapshots(bs);
> }
>
> +static void qcow2_invalidate_cache_bh_cb(void *opaque);
> +
> static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
> {
> BDRVQcowState *s = bs->opaque;
> -int flags = s->flags;
> -AES_KEY aes_encrypt_key;
> -AES_KEY aes_decrypt_key;
> -uint
