Re: [libvirt] [PATCH 6/6] conf: Fix build with picky GCC

[Jiri Denemark]

On Thu, Aug 25, 2016 at 18:42:50 -0400, Peter Krempa wrote:
> ../../src/conf/domain_conf.c:4425:21: error: potential null pointer 
> dereference [-Werror=null-dereference]
>  switch (vcpu->hotpluggable) {
>  ^~
> ---
>  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 ae3eb14..61f6dbb 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4422,6 +4422,10 @@ virDomainVcpuDefPostParse(virDomainDefPtr def)
>  for (i = 0; i < maxvcpus; i++) {
>  vcpu = virDomainDefGetVcpu(def, i);
> 
> +/* impossible but some compilers don't like it */
> +if (!vcpu)
> +continue;
> +
>  switch (vcpu->hotpluggable) {
>  case VIR_TRISTATE_BOOL_ABSENT:
>  if (vcpu->online)

ACK

Jirka

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

Re: [libvirt] [PATCH 2/6] qemu: driver: Fix qemuDomainHelperGetVcpus for sparse vcpu topologies

[Jiri Denemark]

On Thu, Aug 25, 2016 at 18:42:46 -0400, Peter Krempa wrote:
> ce43cca0e refactored the helper to prepare it for sparse topologies but
> forgot to fix the iterator used to fill the structures. This would
> result into a weirdly sparse populated array and possible out of bounds
> access and crash once sparse vcpu topologies were allowed.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1369988
> ---
>  src/qemu/qemu_driver.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)

ACK

Jirka

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

Re: [libvirt] [PATCH 3/6] doc: clarify documentation for vcpu order

[Jiri Denemark]

On Thu, Aug 25, 2016 at 18:42:47 -0400, Peter Krempa wrote:
> Make it clear that vcpu order is valid for online vcpus only and state
> that it has to be specified for all vcpus or not provided at all.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1370043
> ---
>  docs/formatdomain.html.in | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index f2de8e4..f1dadc6 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -564,11 +564,12 @@
>  all disabled vcpus must be hotpluggable. Valid values are
>  yes and no.
> 
> -order allows to specify the order to add the vcpus. For
> -hypervisors/platforms that require to insert multiple vcpus at once
> +order allows to specify the order to add the online 
> vcpus.
> +For hypervisors/platforms that require to insert multiple vcpus at 
> once
>  the order may be be duplicated accross all vcpus that need to be
>  enabled at once. Specifying order is not necessary, vcpus are then
> -added in an arbitrary order.
> +added in an arbitrary order. If order info is used, it must be used 
> for
> +all online vcpus.
> 
>  Note that hypervisors may create hotpluggable vcpus differently from
>  boot vcpus thus special initialization may be necessary.

ACK

Jirka

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

Re: [libvirt] [PATCH 1/6] virsh: vcpuinfo: Report vcpu number from the structure rather than it's position

[Jiri Denemark]

On Thu, Aug 25, 2016 at 18:42:45 -0400, Peter Krempa wrote:
> virVcpuInfo contains the vcpu number that the data refers to. Report
> what's returned by the daemon rather than the sequence number as with
> sparse vcpu topologies they won't match.
> ---
>  tools/virsh-domain.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index de2a22c..90d2543 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -6315,8 +6315,8 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
>  }
> 
>  for (n = 0; n < ncpus; n++) {
> -vshPrint(ctl, "%-15s %d\n", _("VCPU:"), n);
>  if (cpuinfo) {
> +vshPrint(ctl, "%-15s %d\n", _("VCPU:"), cpuinfo[n].number);
>  vshPrint(ctl, "%-15s %d\n", _("CPU:"), cpuinfo[n].cpu);
>  vshPrint(ctl, "%-15s %s\n", _("State:"),
>   virshDomainVcpuStateToString(cpuinfo[n].state));
> @@ -6328,6 +6328,7 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
>  vshPrint(ctl, "%-15s %.1lfs\n", _("CPU time:"), cpuUsed);
>  }
>  } else {
> +vshPrint(ctl, "%-15s %d\n", _("VCPU:"), n);
>  vshPrint(ctl, "%-15s %s\n", _("CPU:"), _("N/A"));
>  vshPrint(ctl, "%-15s %s\n", _("State:"), _("N/A"));
>  vshPrint(ctl, "%-15s %s\n", _("CPU time"), _("N/A"));

ACK

Jirka

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

Re: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state

[Laine Stump]

On 08/25/2016 04:55 AM, Sascha Silbe wrote:

Dear Laine,

Laine Stump  writes:


The linkstate setting of an  is only meant to change the
online status reported to the guest system by the emulated network
device driver in qemu, but when support for auto-creating tap devices
for  was added in commit 9717d6, a chunk of

Typo: the commit id is 9c17d6. Including the title of the commit would
have made locating it easier. At first I thought it wasn't even upstream
yet.


Thanks for pointing that out. I fixed it in the commit log before I pushed.

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

Re: [libvirt] [PATCH 0/3] Fix qemu's tap device online status

[Laine Stump]

On 08/25/2016 01:57 AM, Laine Stump wrote:

These patches are all closely related, but each fixes a different
problem, so they each should be in a separate patch.

These all came out of Vasiliy's report that type='ethernet tap devices
were all offline and didn't have their IP address or routes added. He
sent a patch (which was an expanded version of 1/1), and the
modification to that patch, along with the other two patches, came out
of my review of his patch.

Laine Stump (2):
   qemu: remove unnecessary setting of tap device online state
   qemu: set tap device online for type='ethernet'

Vasiliy Tolstov (1):
   qemu: fix ethernet network type ip/route assign


Okay, I've pushed these three patches so there will be working 
type='ethernet' interfaces in the next release, and I'm working on 
patched that add:


   
 
 ...
   

to allow controlling the link state of the host side of the interface 
(the tap device in qemu, or the host  side of the veth pair in lxc).


Thanks to Vasiliy for the report, patch, reviews, explanations, and idea 
to add host-side link state!


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

[libvirt] [PATCH 6/6] conf: Fix build with picky GCC

[Peter Krempa]

../../src/conf/domain_conf.c:4425:21: error: potential null pointer dereference 
[-Werror=null-dereference]
 switch (vcpu->hotpluggable) {
 ^~
---
 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 ae3eb14..61f6dbb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4422,6 +4422,10 @@ virDomainVcpuDefPostParse(virDomainDefPtr def)
 for (i = 0; i < maxvcpus; i++) {
 vcpu = virDomainDefGetVcpu(def, i);

+/* impossible but some compilers don't like it */
+if (!vcpu)
+continue;
+
 switch (vcpu->hotpluggable) {
 case VIR_TRISTATE_BOOL_ABSENT:
 if (vcpu->online)
-- 
2.8.2

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

[libvirt] [PATCH 5/6] qemu: driver: Validate configuration when setting maximum vcpu count

[Peter Krempa]

Setting vcpu count when cpu topology is specified may result into an
invalid configuration. Since the topology can't be modified, reject the
setting if it doesn't match the requested topology. This will allow
fixing the topology in case it was broken.

Partially fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1370066
---
 src/qemu/qemu_driver.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 671d1ff..5f8c11c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4730,6 +4730,17 @@ qemuDomainSetVcpusMax(virQEMUDriverPtr driver,
 goto cleanup;
 }

+if (persistentDef && persistentDef->cpu && persistentDef->cpu->sockets) {
+/* explicitly allow correcting invalid vcpu count */
+if (nvcpus != persistentDef->cpu->sockets *
+  persistentDef->cpu->cores *
+  persistentDef->cpu->threads) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+   _("CPU topology doesn't match the desired vcpu 
count"));
+goto cleanup;
+}
+}
+
 if (virDomainDefSetVcpusMax(persistentDef, nvcpus, driver->xmlopt) < 0)
 goto cleanup;

-- 
2.8.2

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

[libvirt] [PATCH 4/6] conf: Don't validate vcpu count in XML parser

[Peter Krempa]

Validating the vcpu count is more intricate and doing it in the XML
parser will make previously valid configs (with older qemus) vanish.

Now that we have the validation callbacks we can do it in a more
appropriate place.

This basically reverts commit b54de0830a.

Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1370066
---
 src/conf/domain_conf.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 17c2f53..ae3eb14 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16376,15 +16376,6 @@ virDomainDefParseXML(xmlDocPtr xml,

 if (def->cpu == NULL)
 goto error;
-
-if (def->cpu->sockets &&
-virDomainDefGetVcpusMax(def) >
-def->cpu->sockets * def->cpu->cores * def->cpu->threads) {
-virReportError(VIR_ERR_XML_DETAIL, "%s",
-   _("Maximum CPUs greater than topology limit"));
-goto error;
-}
-
 }

 if (virDomainNumaDefCPUParseXML(def->numa, ctxt) < 0)
-- 
2.8.2

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

[libvirt] [PATCH 3/6] doc: clarify documentation for vcpu order

[Peter Krempa]

Make it clear that vcpu order is valid for online vcpus only and state
that it has to be specified for all vcpus or not provided at all.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1370043
---
 docs/formatdomain.html.in | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index f2de8e4..f1dadc6 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -564,11 +564,12 @@
 all disabled vcpus must be hotpluggable. Valid values are
 yes and no.

-order allows to specify the order to add the vcpus. For
-hypervisors/platforms that require to insert multiple vcpus at once
+order allows to specify the order to add the online vcpus.
+For hypervisors/platforms that require to insert multiple vcpus at once
 the order may be be duplicated accross all vcpus that need to be
 enabled at once. Specifying order is not necessary, vcpus are then
-added in an arbitrary order.
+added in an arbitrary order. If order info is used, it must be used for
+all online vcpus.

 Note that hypervisors may create hotpluggable vcpus differently from
 boot vcpus thus special initialization may be necessary.
-- 
2.8.2

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

[libvirt] [PATCH 0/6] Fix a few issues introduced by adding vcpu hotplug

[Peter Krempa]

Peter Krempa (6):
  virsh: vcpuinfo: Report vcpu number from the structure rather than
it's position
  qemu: driver: Fix qemuDomainHelperGetVcpus for sparse vcpu topologies
  doc: clarify documentation for vcpu order
  conf: Don't validate vcpu count in XML parser
  qemu: driver: Validate configuration when setting maximum vcpu count
  conf: Fix build with picky GCC

 docs/formatdomain.html.in |  7 ---
 src/conf/domain_conf.c| 13 -
 src/qemu/qemu_driver.c| 23 ++-
 tools/virsh-domain.c  |  3 ++-
 4 files changed, 28 insertions(+), 18 deletions(-)

-- 
2.8.2

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

[libvirt] [PATCH 1/6] virsh: vcpuinfo: Report vcpu number from the structure rather than it's position

[Peter Krempa]

virVcpuInfo contains the vcpu number that the data refers to. Report
what's returned by the daemon rather than the sequence number as with
sparse vcpu topologies they won't match.
---
 tools/virsh-domain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index de2a22c..90d2543 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6315,8 +6315,8 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
 }

 for (n = 0; n < ncpus; n++) {
-vshPrint(ctl, "%-15s %d\n", _("VCPU:"), n);
 if (cpuinfo) {
+vshPrint(ctl, "%-15s %d\n", _("VCPU:"), cpuinfo[n].number);
 vshPrint(ctl, "%-15s %d\n", _("CPU:"), cpuinfo[n].cpu);
 vshPrint(ctl, "%-15s %s\n", _("State:"),
  virshDomainVcpuStateToString(cpuinfo[n].state));
@@ -6328,6 +6328,7 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
 vshPrint(ctl, "%-15s %.1lfs\n", _("CPU time:"), cpuUsed);
 }
 } else {
+vshPrint(ctl, "%-15s %d\n", _("VCPU:"), n);
 vshPrint(ctl, "%-15s %s\n", _("CPU:"), _("N/A"));
 vshPrint(ctl, "%-15s %s\n", _("State:"), _("N/A"));
 vshPrint(ctl, "%-15s %s\n", _("CPU time"), _("N/A"));
-- 
2.8.2

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

[libvirt] [PATCH 2/6] qemu: driver: Fix qemuDomainHelperGetVcpus for sparse vcpu topologies

[Peter Krempa]

ce43cca0e refactored the helper to prepare it for sparse topologies but
forgot to fix the iterator used to fill the structures. This would
result into a weirdly sparse populated array and possible out of bounds
access and crash once sparse vcpu topologies were allowed.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1369988
---
 src/qemu/qemu_driver.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 97e2ffc..671d1ff 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1477,15 +1477,17 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
 for (i = 0; i < virDomainDefGetVcpusMax(vm->def) && ncpuinfo < maxinfo; 
i++) {
 virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, i);
 pid_t vcpupid = qemuDomainGetVcpuPid(vm, i);
+virVcpuInfoPtr vcpuinfo = info + ncpuinfo;

 if (!vcpu->online)
 continue;

 if (info) {
-info[i].number = i;
-info[i].state = VIR_VCPU_RUNNING;
+vcpuinfo->number = i;
+vcpuinfo->state = VIR_VCPU_RUNNING;

-if (qemuGetProcessInfo(&(info[i].cpuTime), &(info[i].cpu), NULL,
+if (qemuGetProcessInfo(>cpuTime,
+   >cpu, NULL,
vm->pid, vcpupid) < 0) {
 virReportSystemError(errno, "%s",
  _("cannot get vCPU placement & pCPU 
time"));
@@ -1494,7 +1496,7 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
 }

 if (cpumaps) {
-unsigned char *cpumap = VIR_GET_CPUMAP(cpumaps, maplen, i);
+unsigned char *cpumap = VIR_GET_CPUMAP(cpumaps, maplen, ncpuinfo);
 virBitmapPtr map = NULL;

 if (!(map = virProcessGetAffinity(vcpupid)))
@@ -1505,7 +1507,7 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
 }

 if (cpuwait) {
-if (qemuGetSchedInfo(&(cpuwait[i]), vm->pid, vcpupid) < 0)
+if (qemuGetSchedInfo(&(cpuwait[ncpuinfo]), vm->pid, vcpupid) < 0)
 return -1;
 }

-- 
2.8.2

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

Re: [libvirt] [PATCH 3/3] qemu: set tap device online for type='ethernet'

[Vasiliy Tolstov]

2016-08-25 8:58 GMT+03:00 Laine Stump :
> When support for auto-creating tap devices was added to  type='ethernet'> in commit 9717d6, the code assumed that
> virNetDevTapCreate() would honor the *_CREATE_IFUP flag that is
> supported by virNetDevTapCreateInBridgePort(). That isn't the case -
> the latter function performs several operations, and one of them is
> setting the tap device online. But virNetDevTapCreate() *only* creates
> the tap device, and relies on the caller to do everything else, so
> qemuInterfaceEthernetConnect() needs to call virNetDevSetOnline()
> after the device is successfully created.
> ---
>  src/qemu/qemu_interface.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
> index e327133..455c2d0 100644
> --- a/src/qemu/qemu_interface.c
> +++ b/src/qemu/qemu_interface.c
> @@ -453,6 +453,9 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
>  if (virNetDevSetMAC(net->ifname, ) < 0)
>  goto cleanup;
>
> +if (virNetDevSetOnline(net->ifname, true) < 0)
> +goto cleanup;
> +
>  if (net->script &&
>  virNetDevRunEthernetScript(net->ifname, net->script) < 0)
>  goto cleanup;
> --
> 2.7.4
>

ACK

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru

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

Re: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state

[Vasiliy Tolstov]

2016-08-25 8:57 GMT+03:00 Laine Stump :
> The linkstate setting of an  is only meant to change the
> online status reported to the guest system by the emulated network
> device driver in qemu, but when support for auto-creating tap devices
> for  was added in commit 9717d6, a chunk of
> code was also added to qemuDomainChangeNetLinkState() that sets the
> online status of the tap device (i.e. the *host* side of the
> interface) for type='ethernet'. This was never done for tap devices
> used in type='bridge' or type='network' interfaces, nor was it done in
> the past for tap devices created by external scripts for
> type='ethernet', so we shouldn't be doing it now.
>
> This patch removes the bit of code in qemuDomainChangeNetLinkState()
> that modifies online status of the tap device.
> ---
>  src/qemu/qemu_hotplug.c | 15 ---
>  1 file changed, 15 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 00e4a75..5300bc1 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2324,21 +2324,6 @@ int qemuDomainChangeNetLinkState(virQEMUDriverPtr 
> driver,
>  if (ret < 0)
>  goto cleanup;
>
> -if (virDomainNetGetActualType(dev) == VIR_DOMAIN_NET_TYPE_ETHERNET) {
> -switch (linkstate) {
> -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
> -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
> -if ((ret = virNetDevSetOnline(dev->ifname, true)) < 0)
> -goto cleanup;
> -break;
> -
> -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
> -if ((ret = virNetDevSetOnline(dev->ifname, false)) < 0)
> -goto cleanup;
> -break;
> -}
> -}
> -
>  /* modify the device configuration */
>  dev->linkstate = linkstate;
>
> --
> 2.7.4
>

ACK

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru

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

Re: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state

[Laine Stump]

On 08/25/2016 03:49 PM, Vasiliy Tolstov wrote:


25 Авг 2016 г. 21:38 пользователь "Laine Stump" > написал:

>
> On 08/25/2016 02:19 PM, Vasiliy Tolstov wrote:
>>
>> 25 Авг 2016 г. 19:18 пользователь "Laine Stump" > написал:

>> >
>> > On 08/25/2016 05:42 AM, Vasiliy Tolstov wrote:
>> >>
>> >> 25 Авг 2016 г. 12:34 пользователь Vasiliy Tolstov 
> написал:

>> >> >
>> >> > 25 Авг 2016 г. 8:58 пользователь "Laine Stump" 
> написал:

>> >> > >
>> >> > > The linkstate setting of an  is only meant to 
change the
>> >> > > online status reported to the guest system by the emulated 
network

>> >> > > device driver in qemu,
>> >> >
>> >> I need to set host side status of interface. Without this live 
migration with dinamic routing software (ospf with quagga or bird) 
bring packet drops. Because on dest interface in up state and kernel 
try to forward packets to it, but guest  CPU is not running.

>> >
>> >
>> > That shouldn't be a problem, since the IPInfo isn't added to the 
tap device until immediately before the guest CPU is started on the 
destination (that's the purpose of qemuInterfaceStartDevice()).

>> >
>> >
>> >
>> >> Also host side status needed for easy blackhole traffic to guest ip.
>> >
>> >
>> > Is this something you need to do while the guest is already 
running? If not, then I think we don't need anything extra.

>> >
>> >
>> >
>> >> May be create inside the source link state attribute for host 
side link status? So it consistent with ip and route elements?

>> >
>> >
>> > If necessary, that might be the right solution, although I still 
think it's better to not set the tap device offline, in case it's 
connected to a bridge - we wouldn't want to trigger an STP forward 
delay. Maybe just delete (and later re-add) the IPInfo would be less 
disruptive? (Or it might be *more* disruptive, we'd have to try both).

>> >
>> >
>> Thanks for info.
>> Why not add ability to specify device state from host side? If 
attribute is empty think that device is up. This is reasonable 
default. I'm use link status when vm running, for example if we have 
ddos - I down tap and via ospf route deleted and traffic blackholed.

>
>
> Yeah, I can see the utility of that. And as long as the default is 
up, then nobody is surprised by the results.

>
> So what we're talking about is a new subelement of  for any 
tap-based interface type (at least):

>
> 
>
>   
>
>
> Since it also needs to be supported for qemuDomainChangeNet(), I'm 
doubtful this can be done prior to the freeze  tonight or early 
tomorrow, since DV is in China) though. So will it be okay to have the 
patches I've made in this release (which should handle proper 
operation for everything except the "need to modify the state while 
the guest is running" case)? I don't think any of that will need to be 
*un*done to support this new attribute, and it would be nice to have 
something in the next release that works at least in the default 
situation...


Thanks, I'm happy with this.



Can you ACK Patches 2/3 and 3/3?


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

Re: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state

[Vasiliy Tolstov]

25 Авг 2016 г. 21:38 пользователь "Laine Stump"  написал:
>
> On 08/25/2016 02:19 PM, Vasiliy Tolstov wrote:
>>
>> 25 Авг 2016 г. 19:18 пользователь "Laine Stump" 
написал:
>> >
>> > On 08/25/2016 05:42 AM, Vasiliy Tolstov wrote:
>> >>
>> >> 25 Авг 2016 г. 12:34 пользователь Vasiliy Tolstov 
написал:
>> >> >
>> >> > 25 Авг 2016 г. 8:58 пользователь "Laine Stump" 
написал:
>> >> > >
>> >> > > The linkstate setting of an  is only meant to change
the
>> >> > > online status reported to the guest system by the emulated network
>> >> > > device driver in qemu,
>> >> >
>> >> I need to set host side status of interface. Without this live
migration with dinamic routing software (ospf with quagga or bird) bring
packet drops. Because on dest interface in up state and kernel try to
forward packets to it, but guest  CPU is not running.
>> >
>> >
>> > That shouldn't be a problem, since the IPInfo isn't added to the tap
device until immediately before the guest CPU is started on the destination
(that's the purpose of qemuInterfaceStartDevice()).
>> >
>> >
>> >
>> >> Also host side status needed for easy blackhole traffic to guest ip.
>> >
>> >
>> > Is this something you need to do while the guest is already running?
If not, then I think we don't need anything extra.
>> >
>> >
>> >
>> >> May be create inside the source link state attribute for host side
link status? So it consistent with ip and route elements?
>> >
>> >
>> > If necessary, that might be the right solution, although I still think
it's better to not set the tap device offline, in case it's connected to a
bridge - we wouldn't want to trigger an STP forward delay. Maybe just
delete (and later re-add) the IPInfo would be less disruptive? (Or it might
be *more* disruptive, we'd have to try both).
>> >
>> >
>> Thanks for info.
>> Why not add ability to specify device state from host side? If attribute
is empty think that device is up. This is reasonable default. I'm use link
status when vm running, for example if we have ddos - I down tap and via
ospf route deleted and traffic blackholed.
>
>
> Yeah, I can see the utility of that. And as long as the default is up,
then nobody is surprised by the results.
>
> So what we're talking about is a new subelement of  for any
tap-based interface type (at least):
>
> 
>
>   
>
>
> Since it also needs to be supported for qemuDomainChangeNet(), I'm
doubtful this can be done prior to the freeze  tonight or early tomorrow,
since DV is in China) though. So will it be okay to have the patches I've
made in this release (which should handle proper operation for everything
except the "need to modify the state while the guest is running" case)? I
don't think any of that will need to be *un*done to support this new
attribute, and it would be nice to have something in the next release that
works at least in the default situation...

Thanks, I'm happy with this.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state

[Laine Stump]

On 08/25/2016 02:19 PM, Vasiliy Tolstov wrote:


25 Авг 2016 г. 19:18 пользователь "Laine Stump" > написал:

>
> On 08/25/2016 05:42 AM, Vasiliy Tolstov wrote:
>>
>> 25 Авг 2016 г. 12:34 пользователь Vasiliy Tolstov 
> написал:

>> >
>> > 25 Авг 2016 г. 8:58 пользователь "Laine Stump" > написал:

>> > >
>> > > The linkstate setting of an  is only meant to change the
>> > > online status reported to the guest system by the emulated network
>> > > device driver in qemu,
>> >
>> I need to set host side status of interface. Without this live 
migration with dinamic routing software (ospf with quagga or bird) 
bring packet drops. Because on dest interface in up state and kernel 
try to forward packets to it, but guest  CPU is not running.

>
>
> That shouldn't be a problem, since the IPInfo isn't added to the tap 
device until immediately before the guest CPU is started on the 
destination (that's the purpose of qemuInterfaceStartDevice()).

>
>
>
>> Also host side status needed for easy blackhole traffic to guest ip.
>
>
> Is this something you need to do while the guest is already running? 
If not, then I think we don't need anything extra.

>
>
>
>> May be create inside the source link state attribute for host side 
link status? So it consistent with ip and route elements?

>
>
> If necessary, that might be the right solution, although I still 
think it's better to not set the tap device offline, in case it's 
connected to a bridge - we wouldn't want to trigger an STP forward 
delay. Maybe just delete (and later re-add) the IPInfo would be less 
disruptive? (Or it might be *more* disruptive, we'd have to try both).

>
>
Thanks for info.
Why not add ability to specify device state from host side? If 
attribute is empty think that device is up. This is reasonable 
default. I'm use link status when vm running, for example if we have 
ddos - I down tap and via ospf route deleted and traffic blackholed.




Yeah, I can see the utility of that. And as long as the default is up, 
then nobody is surprised by the results.


So what we're talking about is a new subelement of  for any 
tap-based interface type (at least):



   
  
   

Since it also needs to be supported for qemuDomainChangeNet(), I'm 
doubtful this can be done prior to the freeze  tonight or early 
tomorrow, since DV is in China) though. So will it be okay to have the 
patches I've made in this release (which should handle proper operation 
for everything except the "need to modify the state while the guest is 
running" case)? I don't think any of that will need to be *un*done to 
support this new attribute, and it would be nice to have something in 
the next release that works at least in the default situation...
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state

[Vasiliy Tolstov]

25 Авг 2016 г. 19:18 пользователь "Laine Stump"  написал:
>
> On 08/25/2016 05:42 AM, Vasiliy Tolstov wrote:
>>
>> 25 Авг 2016 г. 12:34 пользователь Vasiliy Tolstov 
написал:
>> >
>> > 25 Авг 2016 г. 8:58 пользователь "Laine Stump" 
написал:
>> > >
>> > > The linkstate setting of an  is only meant to change the
>> > > online status reported to the guest system by the emulated network
>> > > device driver in qemu,
>> >
>> I need to set host side status of interface. Without this live migration
with dinamic routing software (ospf with quagga or bird) bring packet
drops. Because on dest interface in up state and kernel try to forward
packets to it, but guest  CPU is not running.
>
>
> That shouldn't be a problem, since the IPInfo isn't added to the tap
device until immediately before the guest CPU is started on the destination
(that's the purpose of qemuInterfaceStartDevice()).
>
>
>
>> Also host side status needed for easy blackhole traffic to guest ip.
>
>
> Is this something you need to do while the guest is already running? If
not, then I think we don't need anything extra.
>
>
>
>> May be create inside the source link state attribute for host side link
status? So it consistent with ip and route elements?
>
>
> If necessary, that might be the right solution, although I still think
it's better to not set the tap device offline, in case it's connected to a
bridge - we wouldn't want to trigger an STP forward delay. Maybe just
delete (and later re-add) the IPInfo would be less disruptive? (Or it might
be *more* disruptive, we'd have to try both).
>
>
Thanks for info.
Why not add ability to specify device state from host side? If attribute is
empty think that device is up. This is reasonable default. I'm use link
status when vm running, for example if we have ddos - I down tap and via
ospf route deleted and traffic blackholed.

>
>> >
>> > > but when support for auto-creating tap devices
>> > > for  was added in commit 9717d6, a chunk
of
>> > > code was also added to qemuDomainChangeNetLinkState() that sets the
>> > > online status of the tap device (i.e. the *host* side of the
>> > > interface) for type='ethernet'. This was never done for tap devices
>> > > used in type='bridge' or type='network' interfaces, nor was it done
in
>> > > the past for tap devices created by external scripts for
>> > > type='ethernet', so we shouldn't be doing it now.
>> > >
>> > > This patch removes the bit of code in qemuDomainChangeNetLinkState()
>> > > that modifies online status of the tap device.
>> > > ---
>> > >  src/qemu/qemu_hotplug.c | 15 ---
>> > >  1 file changed, 15 deletions(-)
>> > >
>> > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> > > index 00e4a75..5300bc1 100644
>> > > --- a/src/qemu/qemu_hotplug.c
>> > > +++ b/src/qemu/qemu_hotplug.c
>> > > @@ -2324,21 +2324,6 @@ int
qemuDomainChangeNetLinkState(virQEMUDriverPtr driver,
>> > >  if (ret < 0)
>> > >  goto cleanup;
>> > >
>> > > -if (virDomainNetGetActualType(dev) ==
VIR_DOMAIN_NET_TYPE_ETHERNET) {
>> > > -switch (linkstate) {
>> > > -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
>> > > -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
>> > > -if ((ret = virNetDevSetOnline(dev->ifname, true)) <
0)
>> > > -goto cleanup;
>> > > -break;
>> > > -
>> > > -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
>> > > -if ((ret = virNetDevSetOnline(dev->ifname, false))
< 0)
>> > > -goto cleanup;
>> > > -break;
>> > > -}
>> > > -}
>> > > -
>> > >  /* modify the device configuration */
>> > >  dev->linkstate = linkstate;
>> > >
>> > > --
>> > > 2.7.4
>> > >
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state

[Laine Stump]

On 08/25/2016 05:42 AM, Vasiliy Tolstov wrote:


25 Авг 2016 г. 12:34 пользователь Vasiliy Tolstov > написал:

>
> 25 Авг 2016 г. 8:58 пользователь "Laine Stump" > написал:

> >
> > The linkstate setting of an  is only meant to change the
> > online status reported to the guest system by the emulated network
> > device driver in qemu,
>
I need to set host side status of interface. Without this live 
migration with dinamic routing software (ospf with quagga or bird) 
bring packet drops. Because on dest interface in up state and kernel 
try to forward packets to it, but guest  CPU is not running.




That shouldn't be a problem, since the IPInfo isn't added to the tap 
device until immediately before the guest CPU is started on the 
destination (that's the purpose of qemuInterfaceStartDevice()).




Also host side status needed for easy blackhole traffic to guest ip.



Is this something you need to do while the guest is already running? If 
not, then I think we don't need anything extra.



May be create inside the source link state attribute for host side 
link status? So it consistent with ip and route elements?




If necessary, that might be the right solution, although I still think 
it's better to not set the tap device offline, in case it's connected to 
a bridge - we wouldn't want to trigger an STP forward delay. Maybe just 
delete (and later re-add) the IPInfo would be less disruptive? (Or it 
might be *more* disruptive, we'd have to try both).




>
> > but when support for auto-creating tap devices
> > for  was added in commit 9717d6, a chunk of
> > code was also added to qemuDomainChangeNetLinkState() that sets the
> > online status of the tap device (i.e. the *host* side of the
> > interface) for type='ethernet'. This was never done for tap devices
> > used in type='bridge' or type='network' interfaces, nor was it done in
> > the past for tap devices created by external scripts for
> > type='ethernet', so we shouldn't be doing it now.
> >
> > This patch removes the bit of code in qemuDomainChangeNetLinkState()
> > that modifies online status of the tap device.
> > ---
> >  src/qemu/qemu_hotplug.c | 15 ---
> >  1 file changed, 15 deletions(-)
> >
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index 00e4a75..5300bc1 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -2324,21 +2324,6 @@ int 
qemuDomainChangeNetLinkState(virQEMUDriverPtr driver,

> >  if (ret < 0)
> >  goto cleanup;
> >
> > -if (virDomainNetGetActualType(dev) == 
VIR_DOMAIN_NET_TYPE_ETHERNET) {

> > -switch (linkstate) {
> > -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
> > -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
> > -if ((ret = virNetDevSetOnline(dev->ifname, true)) 
< 0)

> > -goto cleanup;
> > -break;
> > -
> > -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
> > -if ((ret = virNetDevSetOnline(dev->ifname, 
false)) < 0)

> > -goto cleanup;
> > -break;
> > -}
> > -}
> > -
> >  /* modify the device configuration */
> >  dev->linkstate = linkstate;
> >
> > --
> > 2.7.4
> >



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

Re: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state

[Laine Stump]

On 08/25/2016 05:41 AM, Vasiliy Tolstov wrote:


25 Авг 2016 г. 8:58 пользователь "Laine Stump" > написал:

>
> The linkstate setting of an  is only meant to change the
> online status reported to the guest system by the emulated network
> device driver in qemu,

I need to set host side status of interface. Without this live 
migration with dinamic routing software (ospf with quagga or bird) 
bring packet drops. Because on dest interface in up state and kernel 
try to forward packets to it, but guest  CPU is not running. Also host 
side status needed for easy blackhole traffic to guest ip.


(tl;dr - it doesn't seem to me like this should be a problem, since we 
don't add the IP addresses or routes to the tap device until just before 
starting the guest CPUs)


Hmm, so a L3 analog to the problem that we have at L2 with macvtap 
interfaces (the presence of an IFF_UP interface with the same MAC 
address as the guest causes traffic for that MAC to be sent to the 
destination too soon.


When connecting a tap device to a bridge, it's important for it to be 
IFF_UP as soon as possible, because the STP forward delay timer doesn't 
start until it's IFF_UP (and since the MAC address of the tap device 
itself isn't used for forwarding any traffic, there's no harm to an L2 
forwarding tables caused by this).


Of course in the case of taps connected to bridges, we don't have any IP 
address set, and also no routes set (although I'm wondering if in the 
future we might have routes (but still no IPs)), so we never encounter 
the issue with L3 forwarding that we do for type='ethernet'.


Setting the tap device offline does have the effect of eliminating all 
IP addresses and routes in a single operation, which works properly for 
you. But if that tap happened to be connected to a bridge (outside the 
scope of libvirt), waiting to set it IFF_UP could result in a much 
longer-than-desired wait before the interface was usable.


But of course if we're not adding the routes and IPs until 
qemuInterfaceStartDevice(), the issue wouldn't exist at domain start 
time - that function isn't called until right before the CPUs are 
started, which is exactly when you want it, so there shouldn't be any 
case where either the IP address of the tap device or the routes 
associated with it are visible prior to the exact time when you want it 
to happen.


There is one issue that may still need to be addressed - there are a few 
cases where we stop the guest CPUs temporarily, and then restart them; 
qemuInterfaceStopDevice *is* called before the CPUs are stopped, but 
because we don't have anything in there to remove the routes or IPs on 
the tap device, it would still be seen as a destination for the given 
IPs during this time. I'm not sure this is really a problem though, 
because we do fully intend to start the same CPU up again and in the 
meantime there isn't any other valid destination for the traffic - 
removing and re-adding the routes during, e.g a 
qemuDomainRevertToSnapshot() would only have the effect of causing a 
mini (and single iteration) route flap. So I don't think anything needs 
to be done about this either.


Does this all make sense, or am I missing something?

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

Re: [libvirt] [PATCH] qemu: fix ethernet network type ip/route assign

[Vasiliy Tolstov]

2016-08-25 18:17 GMT+03:00 Laine Stump :
> Until now, linkstate has always been used to control the online status of
> the emulated NIC in the guest, and the tap device has always been IFF_UP
> (and for a good reason. See my reply to you in the thread for my patches for
> more details...)


Yes, i commented with proposed solution to get that i need =)

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru

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

Re: [libvirt] [Qemu-devel] [RFC] libvirt vGPU QEMU integration

[Laine Stump]

On 08/24/2016 06:29 PM, Daniel P. Berrange wrote:

On Thu, Aug 18, 2016 at 09:41:59AM -0700, Neo Jia wrote:

Hi libvirt experts,

I am starting this email thread to discuss the potential solution / proposal of
integrating vGPU support into libvirt for QEMU.

Some quick background, NVIDIA is implementing a VFIO based mediated device
framework to allow people to virtualize their devices without SR-IOV, for
example NVIDIA vGPU, and Intel KVMGT. Within this framework, we are reusing the
VFIO API to process the memory / interrupt as what QEMU does today with passthru
device.

The difference here is that we are introducing a set of new sysfs file for
virtual device discovery and life cycle management due to its virtual nature.

Here is the summary of the sysfs, when they will be created and how they should
be used:

1. Discover mediated device

As part of physical device initialization process, vendor driver will register
their physical devices, which will be used to create virtual device (mediated
device, aka mdev) to the mediated framework.

Then, the sysfs file "mdev_supported_types" will be available under the physical
device sysfs, and it will indicate the supported mdev and configuration for this
particular physical device, and the content may change dynamically based on the
system's current configurations, so libvirt needs to query this file every time
before create a mdev.

Note: different vendors might have their own specific configuration sysfs as
well, if they don't have pre-defined types.

For example, we have a NVIDIA Tesla M60 on 86:00.0 here registered, and here is
NVIDIA specific configuration on an idle system.

For example, to query the "mdev_supported_types" on this Tesla M60:

cat /sys/bus/pci/devices/:86:00.0/mdev_supported_types
# vgpu_type_id, vgpu_type, max_instance, num_heads, frl_config, framebuffer,
max_resolution
11  ,"GRID M60-0B",  16,   2,  45, 512M,2560x1600
12  ,"GRID M60-0Q",  16,   2,  60, 512M,2560x1600
13  ,"GRID M60-1B",   8,   2,  45,1024M,2560x1600
14  ,"GRID M60-1Q",   8,   2,  60,1024M,2560x1600
15  ,"GRID M60-2B",   4,   2,  45,2048M,2560x1600
16  ,"GRID M60-2Q",   4,   4,  60,2048M,2560x1600
17  ,"GRID M60-4Q",   2,   4,  60,4096M,3840x2160
18  ,"GRID M60-8Q",   1,   4,  60,8192M,3840x2160

I'm unclear on the requirements about data format for this file.
Looking at the docs:

   http://www.spinics.net/lists/kvm/msg136476.html

the format is completely unspecified.


2. Create/destroy mediated device

Two sysfs files are available under the physical device sysfs path : mdev_create
and mdev_destroy

The syntax of creating a mdev is:

 echo "$mdev_UUID:vendor_specific_argument_list" >
/sys/bus/pci/devices/.../mdev_create

I'm not really a fan of the idea of having to provide arbitrary vendor
specific arguments to the mdev_create call, as I don't really want to
have to create vendor specific code for each vendor's vGPU hardware in
libvirt.

What is the relationship between the mdev_supported_types data and
the vendor_specific_argument_list requirements ?



The syntax of destroying a mdev is:

 echo "$mdev_UUID:vendor_specific_argument_list" >
/sys/bus/pci/devices/.../mdev_destroy

The $mdev_UUID is a unique identifier for this mdev device to be created, and it
is unique per system.

For NVIDIA vGPU, we require a vGPU type identifier (shown as vgpu_type_id in
above Tesla M60 output), and a VM UUID to be passed as
"vendor_specific_argument_list".

If there is no vendor specific arguments required, either "$mdev_UUID" or
"$mdev_UUID:" will be acceptable as input syntax for the above two commands.

This raises the question of how an application discovers what
vendor specific arguments are required or not, and what they
might mean.


To create a M60-4Q device, libvirt needs to do:

 echo "$mdev_UUID:vgpu_type_id=20,vm_uuid=$VM_UUID" >
/sys/bus/pci/devices/\:86\:00.0/mdev_create

Overall it doesn't seem like the proposed kernel interfaces provide
enough vendor abstraction to be able to use this functionality without
having to create vendor specific code in libvirt, which is something
I want to avoid us doing.



Ignoring the details though, in terms of libvirt integration, I think I'd
see us primarily doing work in the node device APIs / XML. Specifically
for physical devices, we'd have to report whether they support the
mediated device feature and some way to enumerate the validate device
types that can be created. The node device creation API would have to
support create/deletion of the devices (mapping to mdev_create/destroy)


When configuring a guest VM, we'd use the  XML to point to one
or more mediated devices that have been created via the node device APIs
previously.


I'd originally thought of this as having two separate points of support 
in libvirt as 

Re: [libvirt] [PATCH] qemu: fix ethernet network type ip/route assign

[Laine Stump]

On 08/25/2016 02:59 AM, Vasiliy Tolstov wrote:


May be I miss something but in you patch you always up tap device, but 
if I create domain with link down in XML this not right.

What do you think?



Until now, linkstate has always been used to control the online status 
of the emulated NIC in the guest, and the tap device has always been 
IFF_UP (and for a good reason. See my reply to you in the thread for my 
patches for more details...)



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

[libvirt] [PATCH 1/2] vz: implicitly support additional migration flags

[Pavel Glushchak]

* Added VIR_MIGRATE_LIVE, VIR_MIGRATE_UNDEFINE_SOURCE and
  VIR_MIGRATE_PERSIST_DEST to supported migration flags

Signed-off-by: Pavel Glushchak 
---
 src/vz/vz_driver.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index b34fe33..7a12632 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -2887,8 +2887,11 @@ vzEatCookie(const char *cookiein, int cookieinlen, 
unsigned int flags)
 goto cleanup;
 }
 
-#define VZ_MIGRATION_FLAGS (VIR_MIGRATE_PAUSED |\
-VIR_MIGRATE_PEER2PEER)
+#define VZ_MIGRATION_FLAGS (VIR_MIGRATE_PAUSED |  \
+VIR_MIGRATE_PEER2PEER |   \
+VIR_MIGRATE_LIVE |\
+VIR_MIGRATE_UNDEFINE_SOURCE | \
+VIR_MIGRATE_PERSIST_DEST)
 
 #define VZ_MIGRATION_PARAMETERS \
 VIR_MIGRATE_PARAM_DEST_XML, VIR_TYPED_PARAM_STRING, \
-- 
2.7.4

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

[libvirt] [PATCH 2/2] vz: added VIR_MIGRATE_PARAM_BANDWIDTH param handling

[Pavel Glushchak]

libvirt-python passes parameter bandwidth = 0
by default. This means that bandwidth is unlimited.
VZ driver doesn't support bandwidth rate limiting,
but we still need to handle it and fail if bandwidth > 0.

Signed-off-by: Pavel Glushchak 
---
 src/vz/vz_driver.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 7a12632..4a0068c 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -2897,6 +2897,7 @@ vzEatCookie(const char *cookiein, int cookieinlen, 
unsigned int flags)
 VIR_MIGRATE_PARAM_DEST_XML, VIR_TYPED_PARAM_STRING, \
 VIR_MIGRATE_PARAM_URI,  VIR_TYPED_PARAM_STRING, \
 VIR_MIGRATE_PARAM_DEST_NAME,VIR_TYPED_PARAM_STRING, \
+VIR_MIGRATE_PARAM_BANDWIDTH,VIR_TYPED_PARAM_ULLONG, \
 NULL
 
 static char *
@@ -2938,12 +2939,23 @@ vzDomainMigrateBegin3Params(virDomainPtr domain,
 char *xml = NULL;
 virDomainObjPtr dom = NULL;
 vzConnPtr privconn = domain->conn->privateData;
+unsigned long long bandwidth = 0;
 
 virCheckFlags(VZ_MIGRATION_FLAGS, NULL);
 
 if (virTypedParamsValidate(params, nparams, VZ_MIGRATION_PARAMETERS) < 0)
 goto cleanup;
 
+if (virTypedParamsGetULLong(params, nparams, VIR_MIGRATE_PARAM_BANDWIDTH,
+) < 0)
+goto cleanup;
+
+if (bandwidth > 0) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _("Bandwidth rate limiting is not supported"));
+goto cleanup;
+}
+
 if (!(dom = vzDomObjFromDomain(domain)))
 goto cleanup;
 
-- 
2.7.4

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

[libvirt] [PATCH] Bypass caching in saving VM memory upon external memory snapshot.

[fuweiwei]

From: Fuweiwei 

Currently in qemu-kvm platform, the process of making an external memory 
snapshot is based on the "migration-to-file" sheme. It will use the system 
cache to speed up dumping. However, it will make external disk snapshots
afterwards, which must wait for the completion of flushing the dirty pages 
to the snapshot file. i.e. In virFileWrapperFdClose() after 
qemuMigrationToFile(),
it should wait until the libvirt_iohelper thread finishes fdatasync and exits.
During this time, the VM is paused (since it is suspended from the last 
iteration
of migration-to-file, to the completion of disk snapshots).

Assuming saving 4GB dirty memory at 200MB/s fdatasync speed, the VM will pause
for up to 20s, which is unfriendly to guests.

So I propose that it may be better to bypass caching upon external memory 
snapshot, via the VIR_DOMAIN_SAVE_BYPASS_CACHE flag. As a result, it may avoid
long-term fdatasync in libvirt_iohelper thread and achieve seemless VM suspend.

Signed-off-by: Fuweiwei 
---
 src/qemu/qemu_driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2089359..f954c23 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14117,7 +14117,8 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr 
conn,
 goto cleanup;
 
 if ((ret = qemuDomainSaveMemory(driver, vm, snap->def->file,
-xml, compressed, resume, 0,
+xml, compressed, resume,
+VIR_DOMAIN_SAVE_BYPASS_CACHE,
 QEMU_ASYNC_JOB_SNAPSHOT)) < 0)
 goto cleanup;
 
-- 
1.8.3.1

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

[libvirt] [PATCH 1/2] vz: implicitly support additional migration flags

[Pavel Glushchak]

* Added VIR_MIGRATE_LIVE, VIR_MIGRATE_UNDEFINE_SOURCE and
  VIR_MIGRATE_PERSIST_DEST to supported migration flags

Signed-off-by: Pavel Glushchak 
---
 src/vz/vz_driver.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index b34fe33..7a12632 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -2887,8 +2887,11 @@ vzEatCookie(const char *cookiein, int cookieinlen, 
unsigned int flags)
 goto cleanup;
 }
 
-#define VZ_MIGRATION_FLAGS (VIR_MIGRATE_PAUSED |\
-VIR_MIGRATE_PEER2PEER)
+#define VZ_MIGRATION_FLAGS (VIR_MIGRATE_PAUSED |  \
+VIR_MIGRATE_PEER2PEER |   \
+VIR_MIGRATE_LIVE |\
+VIR_MIGRATE_UNDEFINE_SOURCE | \
+VIR_MIGRATE_PERSIST_DEST)
 
 #define VZ_MIGRATION_PARAMETERS \
 VIR_MIGRATE_PARAM_DEST_XML, VIR_TYPED_PARAM_STRING, \
-- 
2.7.4

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

[libvirt] [PATCH 2/2] vz: added VIR_MIGRATE_PARAM_BANDWIDTH param handling

[Pavel Glushchak]

libvirt-python passes parameter bandwidth = 0
by default. This means that bandwidth is unlimited.
VZ driver doesn't support bandwidth rate limiting,
but we still need to handle it and fail if bandwidth > 0.

Signed-off-by: Pavel Glushchak 
---
 src/vz/vz_driver.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 7a12632..4a0068c 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -2897,6 +2897,7 @@ vzEatCookie(const char *cookiein, int cookieinlen, 
unsigned int flags)
 VIR_MIGRATE_PARAM_DEST_XML, VIR_TYPED_PARAM_STRING, \
 VIR_MIGRATE_PARAM_URI,  VIR_TYPED_PARAM_STRING, \
 VIR_MIGRATE_PARAM_DEST_NAME,VIR_TYPED_PARAM_STRING, \
+VIR_MIGRATE_PARAM_BANDWIDTH,VIR_TYPED_PARAM_ULLONG, \
 NULL
 
 static char *
@@ -2938,12 +2939,23 @@ vzDomainMigrateBegin3Params(virDomainPtr domain,
 char *xml = NULL;
 virDomainObjPtr dom = NULL;
 vzConnPtr privconn = domain->conn->privateData;
+unsigned long long bandwidth = 0;
 
 virCheckFlags(VZ_MIGRATION_FLAGS, NULL);
 
 if (virTypedParamsValidate(params, nparams, VZ_MIGRATION_PARAMETERS) < 0)
 goto cleanup;
 
+if (virTypedParamsGetULLong(params, nparams, VIR_MIGRATE_PARAM_BANDWIDTH,
+) < 0)
+goto cleanup;
+
+if (bandwidth > 0) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _("Bandwidth rate limiting is not supported"));
+goto cleanup;
+}
+
 if (!(dom = vzDomObjFromDomain(domain)))
 goto cleanup;
 
-- 
2.7.4

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

Re: [libvirt] [PATCH v1] qemu-migration: Disallow migration of read only disk

[Koniszewski, Pawel]

Oh, sorry, had this on my todo list and somehow missed it.

This looks ok, in my case read-only device is not part of migrate-disks 
parameter, so everything should be all right.

Thanks for testing it!

Kind Regards,
Pawel Koniszewski

> -Original Message-
> From: Jason J. Herne [mailto:jjhe...@linux.vnet.ibm.com]
> Sent: Thursday, August 25, 2016 3:22 PM
> To: Corey S McQuay ; Koniszewski, Pawel
> ; libvir-list@redhat.com
> Subject: Re: [libvirt] [PATCH v1] qemu-migration: Disallow migration of read
> only disk
> 
> On 08/17/2016 05:10 PM, Jason J. Herne wrote:
> > On 08/11/2016 08:57 AM, Corey S McQuay wrote:
> >> On 08/10/2016 09:16 AM, Koniszewski, Pawel wrote:
> >>
>  -Original Message-
>  From: libvir-list-boun...@redhat.com [mailto:libvir-list-
>  boun...@redhat.com] On Behalf Of Corey S. McQuay
>  Sent: Friday, August 5, 2016 8:34 PM
>  To: jjhe...@linux.vnet.ibm.com; libvir-list@redhat.com
>  Cc: Corey S. McQuay 
>  Subject: [libvirt] [PATCH v1] qemu-migration: Disallow migration of
>  read only disk
> 
>  From: "Corey S. McQuay" 
> 
>  Currently Libvirt allows attempts to migrate read only disks. Qemu
>  cannot handle this as read only disks cannot be written to on the
>  destination system.
>  The end result is a cryptic error message and a failed migration.
> 
>  This patch causes migration to fail earlier and provides a
>  meaningful error message stating that migrating read only disks is
>  not supported.
> >>> What will happen if read-only disk is copied to destination prior to
> >>> migration start? Currently such scenario works, will it still work
> >>> with this code?
> >> Based on our testing, pre-copying a read only disk image to the
> >> destination system has no effect on the outcome of attempting to
> >> migrate a non-shared read only disk. I'm not sure what scenario you
> >> are referring to but here is what we tried:
> >>
> >> Relevant guest xml:
> >>  
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>  
> >>
> >> The disk image exists at /disk-images/guest.iso on the source. Before
> >> migration we copied the image to the same path on the destination
> >> system. Then we attempted migration:
> >>
> >>  virsh migrate --live --copy-storage-all --migrate-disks sdz
> >> --verbose kvm1 qemu+ssh://dstHost/system tcp://dstHost
> >>
> >> The error message we get is:
> >>
> >> error: internal error: info migration reply was missing return status
> >>
> >> Running journalctl shows additional information:
> >>
> >> Aug 10 16:02:16 collin-kvm libvirtd[41616]: operation failed:
> >> migration of disk sdz failed.
> >>
> >> I'm pretty sure this patch does not stop the user from doing anything
> >> that works today. But if your scenario is different from ours in some
> >> way please let us know and we'll do some more testing.
> >
> > Pawel,
> >
> > Thanks for taking a look. Does Corey's reply address your concerns?
> >
> 
> Polite ping for Pawel, and anyone else who wants to review. Thanks :)
> 
> Original patch here:
> https://www.redhat.com/archives/libvir-list/2016-August/msg00378.html
> 
> --
> -- Jason J. Herne (jjhe...@linux.vnet.ibm.com)


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

Re: [libvirt] [PATCH v1] qemu-migration: Disallow migration of read only disk

[Jason J. Herne]

On 08/17/2016 05:10 PM, Jason J. Herne wrote:

On 08/11/2016 08:57 AM, Corey S McQuay wrote:

On 08/10/2016 09:16 AM, Koniszewski, Pawel wrote:


-Original Message-
From: libvir-list-boun...@redhat.com [mailto:libvir-list-
boun...@redhat.com] On Behalf Of Corey S. McQuay
Sent: Friday, August 5, 2016 8:34 PM
To: jjhe...@linux.vnet.ibm.com; libvir-list@redhat.com
Cc: Corey S. McQuay 
Subject: [libvirt] [PATCH v1] qemu-migration: Disallow migration of
read only
disk

From: "Corey S. McQuay" 

Currently Libvirt allows attempts to migrate read only disks. Qemu
cannot
handle this as read only disks cannot be written to on the
destination system.
The end result is a cryptic error message and a failed migration.

This patch causes migration to fail earlier and provides a meaningful
error
message stating that migrating read only disks is not supported.

What will happen if read-only disk is copied to destination prior to
migration start? Currently such scenario works, will it still work
with this code?

Based on our testing, pre-copying a read only disk image to the
destination system has no effect on the outcome of attempting to migrate
a non-shared read only disk. I'm not sure what scenario you are
referring to but here is what we tried:

Relevant guest xml:
 
   
   
   
   
   
   
   
 

The disk image exists at /disk-images/guest.iso on the source. Before
migration we copied the image to the same path on the destination
system. Then we attempted migration:

 virsh migrate --live --copy-storage-all --migrate-disks sdz
--verbose kvm1 qemu+ssh://dstHost/system tcp://dstHost

The error message we get is:

error: internal error: info migration reply was missing return status

Running journalctl shows additional information:

Aug 10 16:02:16 collin-kvm libvirtd[41616]: operation failed: migration
of disk sdz failed.

I'm pretty sure this patch does not stop the user from doing anything
that works today. But if your scenario is different from ours in some
way please let us know and we'll do some more testing.


Pawel,

Thanks for taking a look. Does Corey's reply address your concerns?



Polite ping for Pawel, and anyone else who wants to review. Thanks :)

Original patch here:
https://www.redhat.com/archives/libvir-list/2016-August/msg00378.html

--
-- Jason J. Herne (jjhe...@linux.vnet.ibm.com)

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

Re: [libvirt] [PATCH] Bypass caching in saving VM memory upon external memory snapshot.

[Daniel P. Berrange]

On Thu, Aug 25, 2016 at 07:46:01AM +, fuweiwei wrote:
> From: Fuweiwei 
> 
> Currently in qemu-kvm platform, the process of making an external memory 
> snapshot is based on the "migration-to-file" sheme. It will use the system 
> cache to speed up dumping. However, it will make external disk snapshots
> afterwards, which must wait for the completion of flushing the dirty pages 
> to the snapshot file. i.e. In virFileWrapperFdClose() after 
> qemuMigrationToFile(),
> it should wait until the libvirt_iohelper thread finishes fdatasync and exits.
> During this time, the VM is paused (since it is suspended from the last 
> iteration
> of migration-to-file, to the completion of disk snapshots).
> 
> Assuming saving 4GB dirty memory at 200MB/s fdatasync speed, the VM will pause
> for up to 20s, which is unfriendly to guests.
> 
> So I propose that it may be better to bypass caching upon external memory 
> snapshot, via the VIR_DOMAIN_SAVE_BYPASS_CACHE flag. As a result, it may avoid
> long-term fdatasync in libvirt_iohelper thread and achieve seemless VM 
> suspend.
> 
> Signed-off-by: Fuweiwei 
> ---
>  src/qemu/qemu_driver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2089359..f954c23 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14117,7 +14117,8 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr 
> conn,
>  goto cleanup;
>  
>  if ((ret = qemuDomainSaveMemory(driver, vm, snap->def->file,
> -xml, compressed, resume, 0,
> +xml, compressed, resume,
> +VIR_DOMAIN_SAVE_BYPASS_CACHE,
>  QEMU_ASYNC_JOB_SNAPSHOT)) < 0)
>  goto cleanup;

We should not hardcode this - some filesystems won't support this mode.
We need to wire this upto the API, so when the user invokes the snapshot
they can request the VIR_DOMAIN_SAVE_BYPASS_CACHE flag explicitly.


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

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

Re: [libvirt] [PATCH 0/7] fspool: backend directory

[Daniel P. Berrange]

On Thu, Aug 25, 2016 at 02:03:01PM +0300, Olga Krishtal wrote:
> On 25/08/16 04:14, Daniel P. Berrange wrote:
> > On Fri, Aug 19, 2016 at 06:03:28PM +0300, Olga Krishtal wrote:
> > > Hi everyone, we would like to propose the first implementation of fspool
> > > with directory backend.
> > > 
> > > Filesystem pools is a facility to manage filesystems resources similar
> > > to how storage pools manages volume resources. Furthermore new API follows
> > > storage API closely where it makes sense. Uploading/downloading operations
> > > are not defined yet as it is not obvious how to make it properly. I guess
> > > we can use some kind of tar to make a stream from a filesystem. Please 
> > > share
> > > you thoughts on this particular issue.
> > > 
> > > The patchset provides 'dir' backend which simply expose directories in 
> > > some
> > > directory in host filesystem. The virsh commands are provided too. So it 
> > > is
> > > ready to play with, just replace 'pool' in xml descriptions and virsh 
> > > commands
> > > to 'fspool' and 'volume' to 'item'.
> > > Examle and usage:
> > > Define:
> > > virsh -c qemu:///system fspool-define-as fs_pool_name dir --target 
> > > /path/on/host
> > > Build
> > > virsh -c qemu:///system fspool-build fs_pool_name
> > > Start
> > > virsh -c qemu:///system fspool-start fs_pool_name
> > > Look inside
> > > virsh -c qemu:///system fspool-list (--all) fspool_name
> > > 
> > > Fspool called POOL, on the host fs uses /fs_driver to hold items.
> > >virsh -c qemu:///system fspool-dumpxml POOL
> > >
> > >  POOL
> > >  c57c9d7c-b1d5-4c45-ba9c-67f03d4da160
> > >  733722615808
> > >  1331486720
> > >  534810800128
> > >  
> > >  
> > >  
> > >/fs_driver
> > >
> > >  0755
> > >  0
> > >  0
> > >
> > >  
> > >
> > > 
> > > virsh -c qemu:///system fspool-info POOL
> > > Name:   POOL
> > > UUID:   c57c9d7c-b1d5-4c45-ba9c-67f03d4da160
> > > State:  running
> > > Persistent: yes
> > > Autostart:  no autostart
> > > Capacity:   683.33 GiB
> > > Allocation: 1.24 GiB
> > > Available:  498.08 GiB
> > > 
> > > virsh -c qemu+unix:///system item-list POOL
> > >Name Path
> > > --
> > > item1/fs_driver/item1
> > > item10   /fs_driver/item10
> > > item11   /fs_driver/item11
> > > item12   /fs_driver/item12
> > > item15   /fs_driver/item15
> > > 
> > > Fspool of directory type is some directory on host fs that holds items 
> > > (subdirs).
> > > Example of usage for items:
> > > virsh -c vz+unix:///system item-create-as POOL item1 1g - create item
> > > virsh -c qemu+unix:///system item-dumpxml item1 POOL
> > > 
> > >  item1
> > >  /fs_driver/item1
> > >  
> > >fspoo ='POOL'
> > Is this a typo, or is it really what you intend the 
> > content to look like. It seems rather odd to me
> It should be  but afterword  xml looks like:

I don't think we actually need that info in the fsitem XML - the
pool is passed explicitly as a parameter in the API call, so its
redundant putting it in the XML too - especially as you don't
output it afterwards.

> 
>   item1
>   /home/gray_pig/dirfspool/item1
>   1073741824
>   0
>   
> 
>   0600
>   0
>   0
> 
>   
> 

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

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

Re: [libvirt] [PATCH 0/7] fspool: backend directory

[Olga Krishtal]

On 25/08/16 04:14, Daniel P. Berrange wrote:

On Fri, Aug 19, 2016 at 06:03:28PM +0300, Olga Krishtal wrote:

Hi everyone, we would like to propose the first implementation of fspool
with directory backend.

Filesystem pools is a facility to manage filesystems resources similar
to how storage pools manages volume resources. Furthermore new API follows
storage API closely where it makes sense. Uploading/downloading operations
are not defined yet as it is not obvious how to make it properly. I guess
we can use some kind of tar to make a stream from a filesystem. Please share
you thoughts on this particular issue.

The patchset provides 'dir' backend which simply expose directories in some
directory in host filesystem. The virsh commands are provided too. So it is
ready to play with, just replace 'pool' in xml descriptions and virsh commands
to 'fspool' and 'volume' to 'item'.
Examle and usage:
Define:
virsh -c qemu:///system fspool-define-as fs_pool_name dir --target /path/on/host
Build
virsh -c qemu:///system fspool-build fs_pool_name
Start
virsh -c qemu:///system fspool-start fs_pool_name
Look inside
virsh -c qemu:///system fspool-list (--all) fspool_name

Fspool called POOL, on the host fs uses /fs_driver to hold items.
   virsh -c qemu:///system fspool-dumpxml POOL
   
 POOL
 c57c9d7c-b1d5-4c45-ba9c-67f03d4da160
 733722615808
 1331486720
 534810800128
 
 
 
   /fs_driver
   
 0755
 0
 0
   
 
   

virsh -c qemu:///system fspool-info POOL
Name:   POOL
UUID:   c57c9d7c-b1d5-4c45-ba9c-67f03d4da160
State:  running
Persistent: yes
Autostart:  no autostart
Capacity:   683.33 GiB
Allocation: 1.24 GiB
Available:  498.08 GiB

virsh -c qemu+unix:///system item-list POOL
   Name Path
--
item1/fs_driver/item1
item10   /fs_driver/item10
item11   /fs_driver/item11
item12   /fs_driver/item12
item15   /fs_driver/item15

Fspool of directory type is some directory on host fs that holds items 
(subdirs).
Example of usage for items:
virsh -c vz+unix:///system item-create-as POOL item1 1g - create item
virsh -c qemu+unix:///system item-dumpxml item1 POOL

 item1
 /fs_driver/item1
 
   fspoo ='POOL'

Is this a typo, or is it really what you intend the 
content to look like. It seems rather odd to me

It should be  but afterword  xml looks like:

  item1
  /home/gray_pig/dirfspool/item1
  1073741824
  0
  

  0600
  0
  0

  



 
 0
 0
 
   
 
   
virsh -c qemu+unix:///system item-info item1 POOL
Name:   item1
Type:   dir
Capacity:   683.33 GiB
Allocation: 634.87 MiB
Autostart:  no autostart
Capacity:   683.33 GiB
Allocation: 1.24 GiB
Available:  498.08 GiB
virsh -c qemu+unix:///system item-list POOL
   Name Path
--
   item1/fs_driver/item1
   item10   /fs_driver/item10
   item11   /fs_driver/item11
   item12   /fs_driver/item12
   item15   /fs_driver/item15


Regards,
Daniel



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

Re: [libvirt] [PATCH 6/7] fspool: default implementation of filesystem pools

[Olga Krishtal]

On 25/08/16 04:12, Daniel P. Berrange wrote:

On Fri, Aug 19, 2016 at 06:03:34PM +0300, Olga Krishtal wrote:

Implementation is backend base as in case of storage pools.
This patch adds directory backend which is nothing more
than managing directories inside another one as starting
point.

Signed-off-by: Nikolay Shirokovskiy 
---
  configure.ac |   33 +
  daemon/Makefile.am   |4 +
  po/POTFILES.in   |2 +
  src/Makefile.am  |   38 +
  src/driver.h |1 +
  src/fs/fs_backend.h  |   85 ++
  src/fs/fs_backend_dir.c  |  334 +++
  src/fs/fs_backend_dir.h  |8 +
  src/fs/fs_driver.c   | 2164 ++
  src/fs/fs_driver.h   |   10 +
  src/libvirt.c|   28 +
  src/libvirt_private.syms |1 +
  12 files changed, 2708 insertions(+)
  create mode 100644 src/fs/fs_backend.h
  create mode 100644 src/fs/fs_backend_dir.c
  create mode 100644 src/fs/fs_backend_dir.h
  create mode 100644 src/fs/fs_driver.c
  create mode 100644 src/fs/fs_driver.h

diff --git a/configure.ac b/configure.ac
index 8d7d63e..435b1ec 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1647,6 +1647,35 @@ fi
  AM_CONDITIONAL([WITH_SECRETS], [test "$with_secrets" = "yes"])
  
  
+AC_ARG_WITH([fs-dir],

+  [AS_HELP_STRING([--with-fs-dir],
+[with fs backend for fs driver @<:@default=yes@:>])],
+  [],[with_fs_dir=yes])
+
+if test "$with_libvirtd" = "no"; then
+  with_fs_dir=no
+fi
+
+if test "$with_fs_dir" = "yes" ; then
+  AC_DEFINE_UNQUOTED([WITH_FS_DIR], 1, [whether directory backend for fs 
driver is enabled])
+fi
+AM_CONDITIONAL([WITH_FS_DIR], [test "$with_fs_dir" = "yes"])
+
+with_fs=no
+for backend in dir; do
+if eval test \$with_fs_$backend = yes; then
+with_fs=yes
+break
+fi
+done
+if test $with_fs = yes; then
+AC_DEFINE([WITH_FS], [1],
+  [Define to 1 if at least one fs backend is in use])
+fi
+AM_CONDITIONAL([WITH_FS], [test "$with_fs" = "yes"])
+
+


Lets have all this in a separate m4/virt-fspool.m4 file
and just call it via a macro LIBVIRT_FS_POOL_CHECK


+
  AC_ARG_WITH([storage-dir],
[AS_HELP_STRING([--with-storage-dir],
  [with directory backend for the storage driver @<:@default=yes@:>@])],
@@ -2760,6 +2789,10 @@ AC_MSG_NOTICE([Sheepdog: $with_storage_sheepdog])
  AC_MSG_NOTICE([ Gluster: $with_storage_gluster])
  AC_MSG_NOTICE([ ZFS: $with_storage_zfs])
  AC_MSG_NOTICE([])
+AC_MSG_NOTICE([Fs Drivers])
+AC_MSG_NOTICE([])
+AC_MSG_NOTICE([ Dir: $with_fs_dir])
+AC_MSG_NOTICE([])

Likewise move this and call it via LIBVIRT_FS_POOL_RESULT


  AC_MSG_NOTICE([Security Drivers])
  AC_MSG_NOTICE([])
  AC_MSG_NOTICE([ SELinux: $with_secdriver_selinux ($SELINUX_MOUNT)])


Regards,
Daniel


OK

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

Re: [libvirt] [PATCH 7/7] virsh: filesystem pools commands

[Olga Krishtal]

On 25/08/16 04:13, Daniel P. Berrange wrote:

On Fri, Aug 19, 2016 at 06:03:35PM +0300, Olga Krishtal wrote:

Signed-off-by: Nikolay Shirokovskiy 
---
  po/POTFILES.in   |2 +
  tools/Makefile.am|4 +
  tools/virsh-fspool.c | 1728 ++
  tools/virsh-fspool.h |   36 ++
  tools/virsh-item.c   | 1274 +
  tools/virsh-item.h   |   37 ++
  tools/virsh.c|4 +
  tools/virsh.h|9 +

Could you also update tools/virsh.pod with the docs


Regards,
Daniel


Ok

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

Re: [libvirt] [PATCH 3/7] fspools: configuration and internal representation

[Olga Krishtal]

On 25/08/16 04:07, Daniel P. Berrange wrote:

On Fri, Aug 19, 2016 at 06:03:31PM +0300, Olga Krishtal wrote:

Signed-off-by: Nikolay Shirokovskiy 
---
  po/POTFILES.in   |1 +
  src/Makefile.am  |5 +
  src/conf/fs_conf.c   | 1624 ++
  src/conf/fs_conf.h   |  310 +

To go along with this could you create docs/schemas/{fspool,fsitem}.rng
schema files, and corresponding docs/formatfs.html.in docs.

Finally, we should have tests/fs{pool,item}xml2xmltest.c files which checks
that we correctly round-trip the XML docs, with at least one example XML
file for each schema.

OK



diff --git a/src/conf/fs_conf.c b/src/conf/fs_conf.c
new file mode 100644
index 000..4906c86
--- /dev/null
+++ b/src/conf/fs_conf.c
@@ -0,0 +1,1624 @@
+/*
+ * fs_conf.c: config handling for fs driver
+ *
+ */

Can you add the full LGPLv2+ boilerplate we normally have for
files.


Regards,
Daniel

OK


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

Re: [libvirt] [PATCH 1/7] fspool: introduce filesystem pools API

[Olga Krishtal]

On 25/08/16 04:00, Daniel P. Berrange wrote:

On Fri, Aug 19, 2016 at 06:03:29PM +0300, Olga Krishtal wrote:

API follows the API of storage pools closely in parts which
do not differ for filesystems.

Signed-off-by: Nikolay Shirokovskiy 
---
  include/libvirt/libvirt-fs.h | 273 +++
  include/libvirt/libvirt.h|   1 +
  2 files changed, 274 insertions(+)
  create mode 100644 include/libvirt/libvirt-fs.h

diff --git a/include/libvirt/libvirt-fs.h b/include/libvirt/libvirt-fs.h
new file mode 100644
index 000..4385220
--- /dev/null
+++ b/include/libvirt/libvirt-fs.h



+typedef enum {
+VIR_FS_POOL_CREATE_NORMAL = 0,
+
+/* Create the fspool and perform fspool build without any flags */
+VIR_FS_POOL_CREATE_WITH_BUILD = 1 << 0,
+
+/* Create the fspool and perform fspool build using the
+ * exclusive to VIR_FS_POOL_CREATE_WITH_BUILD_NO_OVERWRITE */
+VIR_FS_POOL_CREATE_WITH_BUILD_OVERWRITE = 1 << 1,
+
+/* Create the pool and perform pool build using the
+ * VIR_FS_POOL_BUILD_NO_OVERWRITE flag. This is mutually
+ * exclusive to VIR_FS_POOL_CREATE_WITH_BUILD_OVERWRITE */
+VIR_FS_POOL_CREATE_WITH_BUILD_NO_OVERWRITE = 1 << 2,
+} virFsPoolCreateFlags;


nitpick, I suggest we use  virFSPool, not virFsPool, since 'fs'
is an abbreviation and thus normal to capitalize both parts.


+
+typedef enum {
+VIR_FS_POOL_BUILD_NEW  = 0,   /* Regular build from scratch */
+VIR_FS_POOL_BUILD_NO_OVERWRITE = (1 << 2),  /* Do not overwrite existing 
pool */
+VIR_FS_POOL_BUILD_OVERWRITE = (1 << 3),  /* Overwrite data */
+} virFsPoolBuildFlags;
+
+/**
+ * virFsPool:
+ *
+ * a virFsPool is a private structure representing a fspool
+ */
+typedef struct _virFsPool virFsPool;
+
+/**
+ * virFsPoolPtr:
+ *
+ * a virFSPoolPtr is pointer to a virFsPool private structure, this is the
+ * type used to reference a fspool in the API.
+ */
+typedef virFsPool *virFsPoolPtr;
+
+typedef enum {
+VIR_FS_POOL_INACTIVE = 0,
+VIR_FS_POOL_BUILDING = 1,
+VIR_FS_POOL_RUNNING = 2,
+
+# ifdef VIR_ENUM_SENTINELS
+VIR_FS_POOL_STATE_LAST
+# endif
+} virFsPoolState;
+
+typedef struct _virFsPoolInfo virFsPoolInfo;
+
+struct _virFsPoolInfo {
+int state; /* virFsPoolState flags */
+unsigned long long capacity;   /* Logical size bytes */
+unsigned long long allocation; /* Current allocation bytes */
+unsigned long long available;  /* Remaining free space bytes */
+};
+
+typedef virFsPoolInfo *virFsPoolInfoPtr;
+
+/**
+ * virFsItem:
+ *
+ * a virFsItem is a private structure representing a fspool item
+ */
+typedef struct _virFsItem virFsItem;
+
+/**
+ * virFsItemPtr:
+ *
+ * a virFsItemPtr is pointer to a virFsItem private structure, this is the
+ * type used to reference a fspool item in the API.
+ */
+typedef virFsItem *virFsItemPtr;
+
+typedef struct _virFsItemInfo virFsItemInfo;
+
+typedef enum {
+VIR_FS_ITEM_DIR = 0,
+VIR_FS_ITEM_LAST
+} virFsItemType;
+
+struct _virFsItemInfo {
+int type;  /* virFsItemType flags */
+unsigned long long capacity;   /* Logical size bytes */
+unsigned long long allocation; /* Current allocation bytes */
+};
+
+typedef virFsItemInfo *virFsItemInfoPtr;
+/*
+ * Get connection from fspool.
+ */
+virConnectPtr   virFsPoolGetConnect(virFsPoolPtr fsool);

No need to copy the crazy whitespace we have in other headers - a single
' ' between return value and function name, is fine and no space at all
between function name and '('


+
+/*
+ * List active fspools
+ */
+int virConnectNumOfFsPools (virConnectPtr conn);
+int virConnectListFsPools  (virConnectPtr conn,
+char **const names,
+int maxnames);
+
+/*
+ * List inactive fspools
+ */
+int virConnectNumOfDefinedFsPools(virConnectPtr conn);
+int virConnectListDefinedFsPools(virConnectPtr conn,
+ char **const names,
+ int maxnames);

The 4 apis follow our old model for listing objects which we discourage
apps from using since they require O(N) API calls and have a designed
in race condition.

So, I suggest just skip these 4 APis and exclusively rely on
the virConnectListAllFsPools API, as we have no need for back
compat with old apps for this new stuff.



+
+/*
+ * virConnectListAllFsPoolsFlags:
+ *
+ * Flags used to tune fspools returned by virConnectListAllFsPools().
+ * Note that these flags come in groups; if all bits from a group are 0,
+ * then that group is not used to filter results.
+ */
+typedef enum {
+VIR_CONNECT_LIST_FS_POOLS_INACTIVE  = 1 << 0,
+VIR_CONNECT_LIST_FS_POOLS_ACTIVE= 1 << 1,
+
+VIR_CONNECT_LIST_FS_POOLS_PERSISTENT= 1 << 

Re: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state

[Vasiliy Tolstov]

25 Авг 2016 г. 12:34 пользователь Vasiliy Tolstov 
написал:
>
> 25 Авг 2016 г. 8:58 пользователь "Laine Stump"  написал:
> >
> > The linkstate setting of an  is only meant to change the
> > online status reported to the guest system by the emulated network
> > device driver in qemu,
>
> I need to set host side status of interface. Without this live migration
with dinamic routing software (ospf with quagga or bird) bring packet
drops. Because on dest interface in up state and kernel try to forward
packets to it, but guest  CPU is not running. Also host side status needed
for easy blackhole traffic to guest ip.

May be create inside the source link state attribute for host side link
status? So it consistent with ip and route elements?

>
> > but when support for auto-creating tap devices
> > for  was added in commit 9717d6, a chunk of
> > code was also added to qemuDomainChangeNetLinkState() that sets the
> > online status of the tap device (i.e. the *host* side of the
> > interface) for type='ethernet'. This was never done for tap devices
> > used in type='bridge' or type='network' interfaces, nor was it done in
> > the past for tap devices created by external scripts for
> > type='ethernet', so we shouldn't be doing it now.
> >
> > This patch removes the bit of code in qemuDomainChangeNetLinkState()
> > that modifies online status of the tap device.
> > ---
> >  src/qemu/qemu_hotplug.c | 15 ---
> >  1 file changed, 15 deletions(-)
> >
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index 00e4a75..5300bc1 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -2324,21 +2324,6 @@ int
qemuDomainChangeNetLinkState(virQEMUDriverPtr driver,
> >  if (ret < 0)
> >  goto cleanup;
> >
> > -if (virDomainNetGetActualType(dev) ==
VIR_DOMAIN_NET_TYPE_ETHERNET) {
> > -switch (linkstate) {
> > -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
> > -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
> > -if ((ret = virNetDevSetOnline(dev->ifname, true)) < 0)
> > -goto cleanup;
> > -break;
> > -
> > -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
> > -if ((ret = virNetDevSetOnline(dev->ifname, false)) < 0)
> > -goto cleanup;
> > -break;
> > -}
> > -}
> > -
> >  /* modify the device configuration */
> >  dev->linkstate = linkstate;
> >
> > --
> > 2.7.4
> >
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state

[Vasiliy Tolstov]

25 Авг 2016 г. 8:58 пользователь "Laine Stump"  написал:
>
> The linkstate setting of an  is only meant to change the
> online status reported to the guest system by the emulated network
> device driver in qemu,

I need to set host side status of interface. Without this live migration
with dinamic routing software (ospf with quagga or bird) bring packet
drops. Because on dest interface in up state and kernel try to forward
packets to it, but guest  CPU is not running. Also host side status needed
for easy blackhole traffic to guest ip.

> but when support for auto-creating tap devices
> for  was added in commit 9717d6, a chunk of
> code was also added to qemuDomainChangeNetLinkState() that sets the
> online status of the tap device (i.e. the *host* side of the
> interface) for type='ethernet'. This was never done for tap devices
> used in type='bridge' or type='network' interfaces, nor was it done in
> the past for tap devices created by external scripts for
> type='ethernet', so we shouldn't be doing it now.
>
> This patch removes the bit of code in qemuDomainChangeNetLinkState()
> that modifies online status of the tap device.
> ---
>  src/qemu/qemu_hotplug.c | 15 ---
>  1 file changed, 15 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 00e4a75..5300bc1 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2324,21 +2324,6 @@ int qemuDomainChangeNetLinkState(virQEMUDriverPtr
driver,
>  if (ret < 0)
>  goto cleanup;
>
> -if (virDomainNetGetActualType(dev) == VIR_DOMAIN_NET_TYPE_ETHERNET) {
> -switch (linkstate) {
> -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
> -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
> -if ((ret = virNetDevSetOnline(dev->ifname, true)) < 0)
> -goto cleanup;
> -break;
> -
> -case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
> -if ((ret = virNetDevSetOnline(dev->ifname, false)) < 0)
> -goto cleanup;
> -break;
> -}
> -}
> -
>  /* modify the device configuration */
>  dev->linkstate = linkstate;
>
> --
> 2.7.4
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] vz: update domain cache after device updates

[Mikhail Feoktistov]

On 25.08.2016 11:33, Nikolay Shirokovskiy wrote:

---
  src/vz/vz_driver.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index b34fe33..f223794 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -1694,6 +1694,9 @@ static int vzDomainUpdateDeviceFlags(virDomainPtr domain,
  if (prlsdkUpdateDevice(driver, dom, dev) < 0)
  goto cleanup;
  
+if (prlsdkUpdateDomain(driver, dom) < 0)

+goto cleanup;
+
  ret = 0;
   cleanup:
  


Ack

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

[libvirt] [PATCH] vz: update domain cache after device updates

[Nikolay Shirokovskiy]

---
 src/vz/vz_driver.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index b34fe33..f223794 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -1694,6 +1694,9 @@ static int vzDomainUpdateDeviceFlags(virDomainPtr domain,
 if (prlsdkUpdateDevice(driver, dom, dev) < 0)
 goto cleanup;
 
+if (prlsdkUpdateDomain(driver, dom) < 0)
+goto cleanup;
+
 ret = 0;
  cleanup:
 
-- 
1.8.3.1

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

Re: [libvirt] [PATCH v2] qemu: neglect cur_balloon in supplied xml

[Nikolay Shirokovskiy]

On 30.07.2016 16:04, John Ferlan wrote:
> s/$subj/qemu: Filter cur_balloon ABI check for certain transactions
> 
> On 06/09/2016 10:32 AM, Nikolay Shirokovskiy wrote:
>> cur_balloon value can change in between preparing external config and
>> using it in operations like save and migrate. As a resutl operation will
>> fail for ABI inconsistency. cur_balloon changes can not be predicted
>> generally and thus operations will fail from time to time.
>>
>> Skip checking cur_balloon if domain lock can not be hold between
>> preparing external config outside of libvirt and checking it against active
>> config. Instead update cur_balloon value in external config from active 
>> config.
>> This way it is protected from forges and is keeped up to date too.
>>
> 
> s/$commit/
> 
> Since the domain lock is not held during preparation of an external XML
> config, it is possible that the value can change resulting in unexpected
> failures during ABI consistency checking for some save and migrate
> operations.
> 
> This patch adds a new flag to skip the checking of the cur_balloon value
> and then sets the destination value to the source value to ensure
> subsequent checks without the skip flag will succeed.
> 
>> Signed-off-by: Nikolay Shirokovskiy 
>> ---
>>  src/conf/domain_conf.c| 14 +++---
>>  src/conf/domain_conf.h|  9 +
>>  src/libvirt_private.syms  |  1 +
>>  src/qemu/qemu_domain.c| 29 -
>>  src/qemu/qemu_domain.h|  6 +++---
>>  src/qemu/qemu_driver.c|  5 +++--
>>  src/qemu/qemu_migration.c |  4 ++--
>>  7 files changed, 49 insertions(+), 19 deletions(-)
>>
> 
> This change seems reasonable to me and it doesn't seem to negate the
> primary focus of commit id 'f2fc1eee' (which added the checks).
> 
> I do have a few suggestions and can make those locally before pushing
> after the current release is cut.
> 
> 

Thanx! I argee to all the suggestions, especially to keeping the name of the 
function
that checks ABI. After all its name reflects 99% of its functionality and it is
idiomatic for 'check' functions to have side effects in this project.

Nikolay

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

Re: [libvirt] [PATCH 2/3] qemu: remove unnecessary setting of tap device online state

[Sascha Silbe]

Dear Laine,

Laine Stump  writes:

> The linkstate setting of an  is only meant to change the
> online status reported to the guest system by the emulated network
> device driver in qemu, but when support for auto-creating tap devices
> for  was added in commit 9717d6, a chunk of

Typo: the commit id is 9c17d6. Including the title of the commit would
have made locating it easier. At first I thought it wasn't even upstream
yet.

(Haven't looked at the actual changes, was mostly interested in what
released versions were affected.)

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641


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

Re: [libvirt] [PATCH] vz: getting bus type for containers

[Nikolay Shirokovskiy]

On 15.08.2016 19:02, Mikhail Feoktistov wrote:
> We should query bus type for containers too, like for VM.
> In openstack we add volume disk like SCSI, so we can't
> hardcode SATA bus.
> ---
>  src/vz/vz_sdk.c | 32 +---
>  1 file changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index f81b320..c4a1c3d 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c

ACK

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

Re: [libvirt] [PATCH] qemu: fix ethernet network type ip/route assign

[Vasiliy Tolstov]

25 Авг 2016 г. 9:00 пользователь "Laine Stump"  написал:
>
> On 08/24/2016 02:32 PM, Laine Stump wrote:
>>
>> On 08/24/2016 12:09 PM, Vasiliy Tolstov wrote:
>>>
>>> IP and routes assigenment incorrectly placed on device stop.
>>> This is fixing it, also change device state according to xml.
>>> Note that as i know in linux routes can't be created on device that does
>>> not up.
>>>
>>> Signed-off-by: Vasiliy Tolstov 
>>> ---
>>>   src/qemu/qemu_interface.c | 23 ++-
>>>   1 file changed, 18 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
>>> index e637d21fb77a..f80feff2d545 100644
>>> --- a/src/qemu/qemu_interface.c
>>> +++ b/src/qemu/qemu_interface.c
>>> @@ -108,8 +108,25 @@ qemuInterfaceStartDevice(virDomainNetDefPtr net)
>>>   break;
>>>   }
>>>   -case VIR_DOMAIN_NET_TYPE_USER:
>>>   case VIR_DOMAIN_NET_TYPE_ETHERNET:
>>> +switch (dev->linkstate) {
>>> +case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
>>> +case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
>>> +if ((ret = virNetDevSetOnline(dev->ifname, true)) < 0)
>>> +goto cleanup;
>>> +break;
>>> +
>>> +case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
>>> +if ((ret = virNetDevSetOnline(dev->ifname, false)) < 0)
>>> +goto cleanup;
>>> +break;
>>> +}
>>
>>
>> The Online/Offline handling of the tap device for ethernet devices
should be identical to that used for network/bridge network devices. If
something is necessary (and it may be), it should be in a separate patch.
>
>
> More on this - aside from code that you added in your patch 9717d6
"autocreate tap device for ethernet network type", the linkstate of a
device is only used to send qemu commands controlling the linkstate of the
*guest* device, but never to set the online status of the host side of the
tap device (several of us missed this during the review of multiple
versions of patches leading to 9717d6; heck, I didn't even realize it when
looking at this new patch until I went back and read through all the code
again)
>
> This (plus a bit of investigation about when tap devices for bridge
networks are set online) leads to the following:
>
> 1) The above code is erroneous. It shouldn't be there at all.
>
> 2) The code in qemuDomainChangeNetLinkState() that modifies the online
status of the tap device when the interface is type='ethernet' also should
not be there.
>
> 3) The reason the tap interface isn't IFF_UP is that virNetDevTapCreate()
(called from qemuInterfaceEthernetConnect() in the same patch 9717d6)
*ignores the VIR_NETDEV_TAP_CREATE_IFUP flag* (and it is documented this
way). That's never been noticed before because the only previous call to
virNetDevTapCreate() is from virNetDevTapCreateInBridgePort(), which sets
IFF_UP on the tap device itself after calling virNetDevTapCreate().
>
> My assumption is that it was done this way possibly because the tap
device needed to be offline during some part of the initialization (or
possibly because that's just the way some function was split up during a
refactoring or something).
>
> Anyway, I think there are 3 patches needed:
>
> 1) The part of your patch that just moves the virNetDevIPInfoAddToDev()
call.
>
> 2) A patch to remove the virNetDevSetOnline() from
qemuDomainChangeNetLinkState()
>
> 3) A patch to call virNetDevSetOnline() after calling virNetDevTapCreate()
>
> I just modified your patch, created the other two patches, and sent the
resulting patch series to the list.
>
> If you can try it out and verify that it works for you (it works for me),
then I'll push it in the morning ( US EST) so it's in before the freeze.

May be I miss something but in you patch you always up tap device, but if I
create domain with link down in XML this not right.
What do you think?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: fix ethernet network type ip/route assign

[Vasiliy Tolstov]

25 Авг 2016 г. 9:00 пользователь "Laine Stump"  написал:
>
> On 08/24/2016 02:32 PM, Laine Stump wrote:
>>
>> On 08/24/2016 12:09 PM, Vasiliy Tolstov wrote:
>>>
>>> IP and routes assigenment incorrectly placed on device stop.
>>> This is fixing it, also change device state according to xml.
>>> Note that as i know in linux routes can't be created on device that does
>>> not up.
>>>
>>> Signed-off-by: Vasiliy Tolstov 
>>> ---
>>>   src/qemu/qemu_interface.c | 23 ++-
>>>   1 file changed, 18 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
>>> index e637d21fb77a..f80feff2d545 100644
>>> --- a/src/qemu/qemu_interface.c
>>> +++ b/src/qemu/qemu_interface.c
>>> @@ -108,8 +108,25 @@ qemuInterfaceStartDevice(virDomainNetDefPtr net)
>>>   break;
>>>   }
>>>   -case VIR_DOMAIN_NET_TYPE_USER:
>>>   case VIR_DOMAIN_NET_TYPE_ETHERNET:
>>> +switch (dev->linkstate) {
>>> +case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
>>> +case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
>>> +if ((ret = virNetDevSetOnline(dev->ifname, true)) < 0)
>>> +goto cleanup;
>>> +break;
>>> +
>>> +case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
>>> +if ((ret = virNetDevSetOnline(dev->ifname, false)) < 0)
>>> +goto cleanup;
>>> +break;
>>> +}
>>
>>
>> The Online/Offline handling of the tap device for ethernet devices
should be identical to that used for network/bridge network devices. If
something is necessary (and it may be), it should be in a separate patch.
>
>
> More on this - aside from code that you added in your patch 9717d6
"autocreate tap device for ethernet network type", the linkstate of a
device is only used to send qemu commands controlling the linkstate of the
*guest* device, but never to set the online status of the host side of the
tap device (several of us missed this during the review of multiple
versions of patches leading to 9717d6; heck, I didn't even realize it when
looking at this new patch until I went back and read through all the code
again)
>
> This (plus a bit of investigation about when tap devices for bridge
networks are set online) leads to the following:
>
> 1) The above code is erroneous. It shouldn't be there at all.
>
> 2) The code in qemuDomainChangeNetLinkState() that modifies the online
status of the tap device when the interface is type='ethernet' also should
not be there.
>
> 3) The reason the tap interface isn't IFF_UP is that virNetDevTapCreate()
(called from qemuInterfaceEthernetConnect() in the same patch 9717d6)
*ignores the VIR_NETDEV_TAP_CREATE_IFUP flag* (and it is documented this
way). That's never been noticed before because the only previous call to
virNetDevTapCreate() is from virNetDevTapCreateInBridgePort(), which sets
IFF_UP on the tap device itself after calling virNetDevTapCreate().
>
> My assumption is that it was done this way possibly because the tap
device needed to be offline during some part of the initialization (or
possibly because that's just the way some function was split up during a
refactoring or something).
>
> Anyway, I think there are 3 patches needed:
>
> 1) The part of your patch that just moves the virNetDevIPInfoAddToDev()
call.
>
> 2) A patch to remove the virNetDevSetOnline() from
qemuDomainChangeNetLinkState()
>
> 3) A patch to call virNetDevSetOnline() after calling virNetDevTapCreate()
>
> I just modified your patch, created the other two patches, and sent the
resulting patch series to the list.
>
> If you can try it out and verify that it works for you (it works for me),
then I'll push it in the morning ( US EST) so it's in before the freeze.

Big thanks! I'm test it today.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: fix ethernet network type ip/route assign

[Laine Stump]

On 08/24/2016 02:32 PM, Laine Stump wrote:

On 08/24/2016 12:09 PM, Vasiliy Tolstov wrote:

IP and routes assigenment incorrectly placed on device stop.
This is fixing it, also change device state according to xml.
Note that as i know in linux routes can't be created on device that does
not up.

Signed-off-by: Vasiliy Tolstov 
---
  src/qemu/qemu_interface.c | 23 ++-
  1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index e637d21fb77a..f80feff2d545 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -108,8 +108,25 @@ qemuInterfaceStartDevice(virDomainNetDefPtr net)
  break;
  }
  -case VIR_DOMAIN_NET_TYPE_USER:
  case VIR_DOMAIN_NET_TYPE_ETHERNET:
+switch (dev->linkstate) {
+case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
+case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
+if ((ret = virNetDevSetOnline(dev->ifname, true)) < 0)
+goto cleanup;
+break;
+
+case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
+if ((ret = virNetDevSetOnline(dev->ifname, false)) < 0)
+goto cleanup;
+break;
+}


The Online/Offline handling of the tap device for ethernet devices 
should be identical to that used for network/bridge network devices. 
If something is necessary (and it may be), it should be in a separate 
patch.


More on this - aside from code that you added in your patch 9717d6 
"autocreate tap device for ethernet network type", the linkstate of a 
device is only used to send qemu commands controlling the linkstate of 
the *guest* device, but never to set the online status of the host side 
of the tap device (several of us missed this during the review of 
multiple versions of patches leading to 9717d6; heck, I didn't even 
realize it when looking at this new patch until I went back and read 
through all the code again)


This (plus a bit of investigation about when tap devices for bridge 
networks are set online) leads to the following:


1) The above code is erroneous. It shouldn't be there at all.

2) The code in qemuDomainChangeNetLinkState() that modifies the online 
status of the tap device when the interface is type='ethernet' also 
should not be there.


3) The reason the tap interface isn't IFF_UP is that 
virNetDevTapCreate() (called from qemuInterfaceEthernetConnect() in the 
same patch 9717d6) *ignores the VIR_NETDEV_TAP_CREATE_IFUP flag* (and it 
is documented this way). That's never been noticed before because the 
only previous call to virNetDevTapCreate() is from 
virNetDevTapCreateInBridgePort(), which sets IFF_UP on the tap device 
itself after calling virNetDevTapCreate().


My assumption is that it was done this way possibly because the tap 
device needed to be offline during some part of the initialization (or 
possibly because that's just the way some function was split up during a 
refactoring or something).


Anyway, I think there are 3 patches needed:

1) The part of your patch that just moves the virNetDevIPInfoAddToDev() 
call.


2) A patch to remove the virNetDevSetOnline() from 
qemuDomainChangeNetLinkState()


3) A patch to call virNetDevSetOnline() after calling virNetDevTapCreate()

I just modified your patch, created the other two patches, and sent the 
resulting patch series to the list.


If you can try it out and verify that it works for you (it works for 
me), then I'll push it in the morning ( US EST) so it's in before the 
freeze.


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