Re: [libvirt] mdevctl: A shoestring mediated device management and persistence utility

[Matthew Rosato]

On 6/28/19 5:06 AM, Cornelia Huck wrote:
> On Thu, 27 Jun 2019 19:57:04 -0600
> Alex Williamson  wrote:
> 
>> On Thu, 27 Jun 2019 15:15:02 -0600
>> Alex Williamson  wrote:
>>
>>> On Thu, 27 Jun 2019 09:38:32 -0600
>>> Alex Williamson  wrote:  
> On 6/27/19 8:26 AM, Cornelia Huck wrote:  
>>
>> {
>>   "foo": "1",
>>   "bar": "42",
>>   "baz": {
>> "depends": ["foo", "bar"],
>> "value": "plahh"
>>   }
>> }
>>
>> Something like that?  

 I'm not sure yet.  I think we need to look at what's feasible (and
 easy) with jq.  Thanks,
>>>
>>> I think it's not too much trouble to remove and insert into arrays, so
>>> what if we were to define the config as:
>>>
>>> {
>>>   "mdev_type":"vendor-type",
>>>   "start":"auto",
>>>   "attrs": [
>>>   {"attrX":["Xvalue1","Xvalue2"]},
>>>   {"dir/attrY": "Yvalue1"},
>>>   {"attrX": "Xvalue3"}
>>> ]
>>> }
>>>
>>> "attr" here would define sysfs attributes under the device.  The array
>>> would be processed in order, so in the above example we'd do the
>>> following:
>>>
>>>  1. echo Xvalue1 > attrX
>>>  2. echo Xvalue2 > attrX
>>>  3. echo Yvalue1 > dir/attrY
>>>  4. echo Xvalue3 > attrX
>>>
>>> When starting the device mdevctl would simply walk the array, if the
>>> attribute key exists write the value(s).  If a write fails or the
>>> attribute doesn't exist, remove the device and report error.
> 
> Yes, I think it makes sense to fail the startup of a device where we
> cannot set all attributes to the requested values.
> 
>>>
>>> I think it's easiest with jq to manipulate arrays by removing and
>>> inserting by index.  Also if we end up with something like above, it's
>>> ambiguous if we reference the "attrX" key.  So perhaps we add the
>>> following options to the modify command:
>>>
>>> --addattr=ATTRIBUTE --delattr --index=INDEX --value=VALUE1[,VALUE2]
>>>
>>> We could handle it like a stack, so if --index is not supplied, add to
>>> the end or remove from the end.  If --index is provided, delete that
>>> index or add the attribute at that index.  So if you had the above and
>>> wanted to remove Xvalue1 but keep the ordering, you'd do:
>>>
>>> --delattr --index=0
>>> --addattr --index=0 --value=Xvalue2
>>>
>>> Which should results in:
>>>
>>>   "attrs": [
>>>   {"attrX": "Xvalue2"},
>>>   {"dir/attrY": "Yvalue1"},
>>>   {"attrX": "Xvalue3"}
>>> ]
> 
> Modifying by index looks reasonable; I just sent a pull request to
> print the index of an attribute out as well, so it is easier to specify
> the right attribute to modify.
> 
>>>
>>> If we want to modify a running device, I'm thinking we probably want a
>>> new command and options --attr=ATTRIBUTE --value=VALUE might suffice.
>>>
>>> Do we need to support something like this for the 'start' command or
>>> should we leave that for simple devices and require a sequence of:
>>>
>>> # mdevctl define ...
>>> # mdevctl modify --addattr...
>>> ...
>>> # mdevctl start
>>> # mdevctl undefine
>>>
>>> This is effectively the long way to get a transient device.  Otherwise
>>> we'd need to figure out how to have --attr --value appear multiple
>>> times on the start command line.  Thanks,  
> 
> What do you think of a way to specify JSON for the attributes directly
> on the command line? Or would it be better to just edit the config
> files directly?
> 
>>
>> This is now implemented, and yes you can specify '--addattr remove
>> --value 1' and mdevctl will immediately remove the device after it's
>> created (more power to the admin).  Listing defined devices also lists
> 
> Fun ;)
> 
>> any attributes defined for easy inspection.  It is also possible to
>> override the conversion of comma separated values into an array by
>> encoding and escaping the comma.  It's a little cumbersome, but
>> possible in case a driver isn't fully on board with the one attribute,
>> one value rule of sysfs.  Does this work for vfio-ap?  I also still
> 
> I do not have ap devices to actually test this with; but defining a
> device and adding attributes seems to work.
> 

I pulled and did a quick test with vfio-ap, it's working.  I was able to
define, modify with the appropriate attributes and start, resulting in a
correctly-configured device.


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

Re: [libvirt] mdevctl: A shoestring mediated device management and persistence utility

[Matthew Rosato]

On 6/27/19 11:38 AM, Alex Williamson wrote:
> On Thu, 27 Jun 2019 11:00:31 -0400
> Matthew Rosato  wrote:
> 
>> On 6/27/19 8:26 AM, Cornelia Huck wrote:
>>> On Wed, 26 Jun 2019 19:53:50 -0600
>>> Alex Williamson  wrote:
>>>   
>>>> On Wed, 26 Jun 2019 08:37:20 -0600
>>>> Alex Williamson  wrote:
>>>>  
>>>>> On Wed, 26 Jun 2019 11:58:06 +0200
>>>>> Cornelia Huck  wrote:
>>>>> 
>>>>>> On Tue, 25 Jun 2019 16:52:51 -0600
>>>>>> Alex Williamson  wrote:
>>>>>>   
>>>>>>> Hi,
>>>>>>>
>>>>>>> Based on the discussions we've had, I've rewritten the bulk of
>>>>>>> mdevctl.  I think it largely does everything we want now, modulo
>>>>>>> devices that will need some sort of 1:N values per key for
>>>>>>> configuration in the config file versus the 1:1 key:value setup we
>>>>>>> currently have (so don't consider the format final just yet).
>>>>>>
>>>>>> We might want to factor out that config format handling while we're
>>>>>> trying to finalize it.
>>>>>>
>>>>>> cc:ing Matt for his awareness. I'm currently not quite sure how to
>>>>>> handle those vfio-ap "write several values to an attribute one at a
>>>>>> time" requirements. Maybe 1:N key:value is the way to go; maybe we
>>>>>> need/want JSON or something like that.  
>>>>>
>>>>> Maybe we should just do JSON for future flexibility.  I assume there
>>>>> are lots of helpers that should make it easy even from a bash script.
>>>>> I'll look at that next.
>>>>
>>>> Done.  Throw away any old mdev config files, we use JSON now.   
>>>
>>> The code changes look quite straightforward, thanks.
>>>   
>>>> The per
>>>> mdev config now looks like this:
>>>>
>>>> {
>>>>   "mdev_type": "i915-GVTg_V4_8",
>>>>   "start": "auto"
>>>> }
>>>>
>>>> My expectation, and what I've already pre-enabled support in set_key
>>>> and get_key functions, is that we'd use arrays for values, so we might
>>>> have:
>>>>
>>>>   "new_key": ["value1", "value2"]
>>>>
>>>> set_key will automatically convert a comma separated list of values
>>>> into such an array, so I'm thinking this would be specified by the user
>>>> as:
>>>>
>>>> # mdevctl modify -u UUID --key=new_key --value=value1,value2  
>>>
>>> Looks sensible.
>>>
>>> For vfio-ap, we'd probably end up with something like the following:
>>>
>>> {
>>>   "mdev_type": "vfio_ap-passthrough",
>>>   "start": "auto",
>>>   "assign_adapter": ["5", "6"],
>>>   "assign_domain": ["4", "0xab"]
>>> }
>>>
>>> (following the Guest1 example in the kernel documentation)
>>>
>>> >> ["6", "7"]? Remove 5, add 7? Remove all values, then set the new ones?  
>>
>> IMO remove 5, add 7 would make the most sense.  I'm not sure that doing
>> an unassign of all adapters (effectively removing all APQNs) followed by
>> an assign of the new ones would work nicely with Tony's vfio-ap dynamic
>> configuration patches.
> 
> Are we conflating operating on the config file versus operating on the
> device?  I was thinking that setting a new key value replaces the
> existing key, because anything else adds unnecessary complication to
> the code and command line.  So in the above example, if the user
> specified:
> 
>   mdevctl modify -u UUID --key=assign_adapter --value=6,7
> 
> The new value is simply ["6", "7"].  This would take effect the next
> time the device is started.  We haven't yet considered how to change
> running devices, but I think the semantics we have since the respin of
> mdevctl separate saved config vs running devices in order to generalize
> the support of transient devices.

Yeah, my comment was aimed specifically at changes to a running device.
 When considering only the config file I agree: the new key value can
just replace the existing key value.

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

Re: [libvirt] mdevctl: A shoestring mediated device management and persistence utility

[Matthew Rosato]

On 6/27/19 8:26 AM, Cornelia Huck wrote:
> On Wed, 26 Jun 2019 19:53:50 -0600
> Alex Williamson  wrote:
> 
>> On Wed, 26 Jun 2019 08:37:20 -0600
>> Alex Williamson  wrote:
>>
>>> On Wed, 26 Jun 2019 11:58:06 +0200
>>> Cornelia Huck  wrote:
>>>   
 On Tue, 25 Jun 2019 16:52:51 -0600
 Alex Williamson  wrote:
 
> Hi,
>
> Based on the discussions we've had, I've rewritten the bulk of
> mdevctl.  I think it largely does everything we want now, modulo
> devices that will need some sort of 1:N values per key for
> configuration in the config file versus the 1:1 key:value setup we
> currently have (so don't consider the format final just yet).  

 We might want to factor out that config format handling while we're
 trying to finalize it.

 cc:ing Matt for his awareness. I'm currently not quite sure how to
 handle those vfio-ap "write several values to an attribute one at a
 time" requirements. Maybe 1:N key:value is the way to go; maybe we
 need/want JSON or something like that.
>>>
>>> Maybe we should just do JSON for future flexibility.  I assume there
>>> are lots of helpers that should make it easy even from a bash script.
>>> I'll look at that next.  
>>
>> Done.  Throw away any old mdev config files, we use JSON now. 
> 
> The code changes look quite straightforward, thanks.
> 
>> The per
>> mdev config now looks like this:
>>
>> {
>>   "mdev_type": "i915-GVTg_V4_8",
>>   "start": "auto"
>> }
>>
>> My expectation, and what I've already pre-enabled support in set_key
>> and get_key functions, is that we'd use arrays for values, so we might
>> have:
>>
>>   "new_key": ["value1", "value2"]
>>
>> set_key will automatically convert a comma separated list of values
>> into such an array, so I'm thinking this would be specified by the user
>> as:
>>
>> # mdevctl modify -u UUID --key=new_key --value=value1,value2
> 
> Looks sensible.
> 
> For vfio-ap, we'd probably end up with something like the following:
> 
> {
>   "mdev_type": "vfio_ap-passthrough",
>   "start": "auto",
>   "assign_adapter": ["5", "6"],
>   "assign_domain": ["4", "0xab"]
> }
> 
> (following the Guest1 example in the kernel documentation)
> 
>  ["6", "7"]? Remove 5, add 7? Remove all values, then set the new ones?

IMO remove 5, add 7 would make the most sense.  I'm not sure that doing
an unassign of all adapters (effectively removing all APQNs) followed by
an assign of the new ones would work nicely with Tony's vfio-ap dynamic
configuration patches.

> Similar for deleting the "assign_adapter" key. We have an
> "unassign_adapter" attribute, but this is not something we can infer
> automatically; we need to know that we're dealing with an vfio-ap
> matrix device...>
> 
>>
>> We should think about whether ordering is important and maybe
>> incorporate that into key naming conventions or come up with some
>> syntax for specifying startup blocks.  Thanks,
>>
>> Alex
> 
> Hm...
> 
> {
>   "foo": "1",
>   "bar": "42",
>   "baz": {
> "depends": ["foo", "bar"],
> "value": "plahh"
>   }
> }
> 
> Something like that?
> 

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

Re: [libvirt] [PATCH RFC 0/1] mdevctl: further config for vfio-ap

[Matthew Rosato]

On 6/13/19 9:54 AM, Cornelia Huck wrote:
> On Fri, 7 Jun 2019 14:30:48 -0400
> Tony Krowiak  wrote:
> 
>> On 6/7/19 10:56 AM, Cornelia Huck wrote:
>>> On Thu, 6 Jun 2019 12:45:29 -0400
>>> Matthew Rosato  wrote:
>>>   
>>>> On 6/6/19 10:44 AM, Cornelia Huck wrote:  
>>>>> This patch adds a very rough implementation of additional config data
>>>>> for mdev devices. The idea is to make it possible to specify some
>>>>> type-specific key=value pairs in the config file for an mdev device.
>>>>> If a device is started automatically, the device is stopped and restarted
>>>>> after applying the config.
>>>>>
>>>>> The code has still some problems, like not doing a lot of error handling
>>>>> and being ugly in general; but most importantly, I can't really test it,
>>>>> as I don't have the needed hardware. Feedback welcome; would be good to
>>>>> know if the direction is sensible in general.  
>>>>
>>>> Hi Connie,
>>>>
>>>> This is very similar to what I was looking to do in zdev (config via
>>>> key=value pairs), so I like your general approach.
>>>>
>>>> I pulled your code and took it for a spin on an LPAR with access to
>>>> crypto cards:
>>>>
>>>> # mdevctl create-mdev `uuidgen` matrix vfio_ap-passthrough
>>>> # mdevctl set-additional-config  ap_adapters=0x4,0x5
>>>> # mdevctl set-additional-config  ap_domains=0x36
>>>> # mdevctl set-additional-config  ap_control_domains=0x37
>>>>
>>>> Assuming all valid inputs, this successfully creates the appropriate
>>>> mdev and what looks to be a valid mdevctl.d entry.  A subsequent reboot
>>>> successfully brings the same vfio_ap-passthrough device up again.  
>>>
>>> Cool, thanks for checking!  
>>
>> I also confirmed this. I realize this is a very early proof of concept,
>> if you will, but error handling could be an interesting endeavor in
>> the case of vfio_ap given the layers of configuration involved; 
> 
> I agree; that's why I mostly ignored it for now :)
> 
>> for
>> example:
>>
>> * The necessity for the vfio_ap module to be installed
>> * The necessity that the /sys/bus/ap/apmask and /sys/bus/ap/aqmask must
>>be appropriately configured
> 
> What do you think about outsourcing that configuration to some
> s390-specific tool (probably something in s390-tools)? While we can
> (and should) rely on driverctl for vfio-ccw, this does not look like
> something that can be easily served by some generic tooling.
> 

+1 mdevctl should stick to manipulation of mdevs.  I think any config
layer above that isn't directly modifying mdev parameters, like in this
case the kernel ap/aq masks, should be separate (and for vfio_ap I agree
s390-tools is the right place for this -- it's on my todo list)

>>
>>>   
>>>>
>>>> Matt
>>>>  
>>>>>
>>>>> Also available at
>>>>>
>>>>> https://github.com/cohuck/mdevctl conf-data
>>>>>
>>>>> Cornelia Huck (1):
>>>>>allow to specify additional config data
>>>>>
>>>>>   mdevctl.libexec | 25 ++
>>>>>   mdevctl.sbin| 56 -
>>>>>   2 files changed, 80 insertions(+), 1 deletion(-)
>>>>>  
>>>>  
>>>   
>>
> 

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

Re: [libvirt] [PATCH RFC 0/1] mdevctl: further config for vfio-ap

[Matthew Rosato]

On 6/6/19 10:44 AM, Cornelia Huck wrote:
> This patch adds a very rough implementation of additional config data
> for mdev devices. The idea is to make it possible to specify some
> type-specific key=value pairs in the config file for an mdev device.
> If a device is started automatically, the device is stopped and restarted
> after applying the config.
> 
> The code has still some problems, like not doing a lot of error handling
> and being ugly in general; but most importantly, I can't really test it,
> as I don't have the needed hardware. Feedback welcome; would be good to
> know if the direction is sensible in general.

Hi Connie,

This is very similar to what I was looking to do in zdev (config via
key=value pairs), so I like your general approach.

I pulled your code and took it for a spin on an LPAR with access to
crypto cards:

# mdevctl create-mdev `uuidgen` matrix vfio_ap-passthrough
# mdevctl set-additional-config  ap_adapters=0x4,0x5
# mdevctl set-additional-config  ap_domains=0x36
# mdevctl set-additional-config  ap_control_domains=0x37

Assuming all valid inputs, this successfully creates the appropriate
mdev and what looks to be a valid mdevctl.d entry.  A subsequent reboot
successfully brings the same vfio_ap-passthrough device up again.

Matt

> 
> Also available at
> 
> https://github.com/cohuck/mdevctl conf-data
> 
> Cornelia Huck (1):
>   allow to specify additional config data
> 
>  mdevctl.libexec | 25 ++
>  mdevctl.sbin| 56 -
>  2 files changed, 80 insertions(+), 1 deletion(-)
> 

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

Re: [libvirt] [PATCH v4 0/8] Virtio-crypto device support

[Matthew Rosato]

On 07/07/2017 04:07 AM, Longpeng(Mike) wrote:
> As virtio-crypto has been supported in QEMU 2.8 and the frontend
> driver has been merged in linux 4.10, so it's necessary to support
> virtio-crypto in libvirt.
> 
> ---

Hi Mike,

Seems like this topic has gone quiet..  Is there a v5 in the works?

Matt

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

Re: [libvirt] [PATCH 0/5 v2] Corrections to SCSI logical unit handling

[Matthew Rosato]

On 06/16/2015 11:29 PM, Eric Farman wrote:
 While working with the hostdev tag and SCSI LUNs, a problem was
 discovered with the XML schema (see commit message in patch 4).
 This spawned some further corrections to the handling of the
 logical unit field throughout libvirt.
 
 This series was split from a single patch, from this feedback:
 http://www.redhat.com/archives/libvir-list/2015-June/msg00489.html
 
 Eric Farman (5):
   Print SCSI logical unit as a positive integer
   Print SCSI logical unit as unsigned integer
   Convert SCSI logical unit from int to long long
   docs: Fix XML schema handling of LUN address in hostdev tag
   docs: Correct typos in scsi hostdev and address elements

I get the value of small patches  agree with the way patches 4 and 5
are split out, but patch 1 and 2 are completely replaced by patch 3 with
a different type.  These are pretty straightforward changes, so I'd
suggest squashing patches 1-3 as a single patch that just goes from
signed int -- unsigned long long and has a commit that explains why we
needed to change both size and sign...  I see now that it was a v1
comment from John, so I'll leave it to his judgment.

Regardless, code still looks good to me so you can feel free to keep my
reviewed-by tag for the set, whether patches 1-3 are squashed or not.

Reviewed-by: Matthew Rosato mjros...@linux.vnet.ibm.com

 
  docs/formatdomain.html.in | 10 +++---
  docs/schemas/domaincommon.rng | 14 --
  src/conf/domain_audit.c   |  2 +-
  src/conf/domain_conf.c|  4 ++--
  src/conf/domain_conf.h|  2 +-
  src/qemu/qemu_command.h   |  2 +-
  src/qemu/qemu_hotplug.c   |  4 ++--
  src/util/virhostdev.c |  6 +++---
  src/util/virscsi.c| 16 
  src/util/virscsi.h|  8 
  tests/testutilsqemu.c |  2 +-
  tools/virsh-domain.c  |  6 +++---
  12 files changed, 45 insertions(+), 31 deletions(-)
 

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

Re: [libvirt] [PATCH 1/3] qemu: always call qemuInterfaceStartDevices() when starting CPUs

[Matthew Rosato]

On 12/12/2014 11:34 AM, Laine Stump wrote:
 The patch that added qemuInterfaceStartDevices() (upstream commit
 82977058f5b1d143a355079900029e9cbfee2fe4) had an extra conditional to
 prevent calling it if the reason for starting the CPUs was
 VIR_DOMAIN_RUNNING_UNPAUSED or VIR_DOMAIN_RUNNING_SAVE_CANCELED.  This
 was put in by the author as the result of a reviewer asking if it was
 necessary to ifup the interfaces in *all* occasions (because these
 were the two cases where the CPU would have already been started (and
 stopped) once, so the interface would already be ifup'ed).
 
 It turns out that, as long as there is no corresponding
 qemuInterfaceStopDevices() to ifdown the interfaces anytime the CPUs
 are stopped, neglecting to ifup when reason is RUNNING_UNPAUSED or
 RUNNING_SAVE_CANCELED doesn't cause any problems (because it just
 happens that the interface will have already been ifup'ed by a prior
 call when the CPU was previously started for some other reason).
 
 However, it also doesn't *help*, and there will soon be a
 qemuInterfaceStopDevices() function which *will* ifdown these
 interfaces when the guest CPUs are stopped, and once that is done, the
 interfaces will be left down in some cases when they should be up (for
 example, if a domain is paused and then unpaused).
 
 So, this patch is removing the condition in favor of always calling
 qemuInterfaeStartDevices() when the guest CPUs are started.
 
 This patch (and the aforementioned patch) resolve:
 
   https://bugzilla.redhat.com/show_bug.cgi?id=1081461
 ---
  src/qemu/qemu_process.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)
 
 diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
 index ab4df9b..0028283 100644
 --- a/src/qemu/qemu_process.c
 +++ b/src/qemu/qemu_process.c
 @@ -3126,9 +3126,7 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, 
 virDomainObjPtr vm,
  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 
  /* Bring up netdevs before starting CPUs */
 -if (reason != VIR_DOMAIN_RUNNING_UNPAUSED 
 -reason != VIR_DOMAIN_RUNNING_SAVE_CANCELED 
 -qemuInterfaceStartDevices(vm-def)  0)
 +if (qemuInterfaceStartDevices(vm-def)  0)
 goto cleanup;
 
  VIR_DEBUG(Using lock state '%s', NULLSTR(priv-lockState));
 

I agreed to this in a separate thread  code looks good so:

Reviewed by: Matthew Rosato mjros...@linux.vnet.ibm.com

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

Re: [libvirt] [PATCH 2/3] qemu: add a qemuInterfaceStopDevices(), called when guest CPUs stop

[Matthew Rosato]

On 12/12/2014 11:34 AM, Laine Stump wrote:
 We now have a qemuInterfaceStartDevices() which does the final
 activation needed for the host-side tap/macvtap devices that are used
 for qemu network connections. It will soon make sense to have the
 converse qemuInterfaceStopDevices() which will undo whatever was done
 during qemuInterfaceStartDevices().
 
 A function to stop a single device has also been added, and is
 called from the appropriate place in qemuDomainDetachNetDevice(),
 although this is currently unnecessary - the device is going to
 immediately be deleted anyway, so any extra deactivation will be for
 naught. The call is included for completeness, though, in anticipation
 that in the future there may be some required action that *isn't*
 nullified by deleting the device.
 
 This patch is a part of a more complete fix for:
 
   https://bugzilla.redhat.com/show_bug.cgi?id=1081461
 ---
  src/qemu/qemu_hotplug.c   |  8 ++
  src/qemu/qemu_interface.c | 66 
 +++
  src/qemu/qemu_interface.h |  2 ++
  src/qemu/qemu_process.c   |  3 +++
  4 files changed, 79 insertions(+)
 
 diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
 index 8c0642e..e56cffe 100644
 --- a/src/qemu/qemu_hotplug.c
 +++ b/src/qemu/qemu_hotplug.c
 @@ -3513,6 +3513,14 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
  VIR_WARN(cannot clear bandwidth setting for device : %s,
   detach-ifname);
 
 +/* deactivate the tap/macvtap device on the host (currently this
 + * isn't necessary, as everything done in
 + * qemuInterfaceStopDevice() is made meaningless when the device
 + * is deleted anyway, but in the future it may be important, and
 + * doesn't hurt anything for now)
 + */
 +ignore_value(qemuInterfaceStopDevice(detach));
 +
  qemuDomainMarkDeviceForRemoval(vm, detach-info);
 
  qemuDomainObjEnterMonitor(driver, vm);
 diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
 index b0f0c5d..b9694c8 100644
 --- a/src/qemu/qemu_interface.c
 +++ b/src/qemu/qemu_interface.c
 @@ -98,3 +98,69 @@ qemuInterfaceStartDevices(virDomainDefPtr def)
  }
  return 0;
  }
 +
 +
 +/**
 + * qemuInterfaceStopDevice:
 + * @net: net device to stop
 + *
 + * Based upon the type of device provided, perform the appropriate
 + * work to deactivate the device so that packets aren't forwarded to
 + * it from the rest of the network.
 + */
 +int
 +qemuInterfaceStopDevice(virDomainNetDefPtr net)
 +{
 +int ret = -1;
 +
 +switch (virDomainNetGetActualType(net)) {
 +case VIR_DOMAIN_NET_TYPE_BRIDGE:
 +case VIR_DOMAIN_NET_TYPE_NETWORK:
 +break;
 +
 +case VIR_DOMAIN_NET_TYPE_DIRECT:
 +/* macvtap interfaces need to be marked !IFF_UP (ie down) to
 + * prevent any host-generated traffic sent from this interface
 + * from putting bad info into the arp caches of other machines
 + * on this network.
 + */
 +if (virNetDevSetOnline(net-ifname, false)  0)
 +goto cleanup;
 +break;
 +
 +case VIR_DOMAIN_NET_TYPE_USER:
 +case VIR_DOMAIN_NET_TYPE_ETHERNET:
 +case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
 +case VIR_DOMAIN_NET_TYPE_SERVER:
 +case VIR_DOMAIN_NET_TYPE_CLIENT:
 +case VIR_DOMAIN_NET_TYPE_MCAST:
 +case VIR_DOMAIN_NET_TYPE_INTERNAL:
 +case VIR_DOMAIN_NET_TYPE_HOSTDEV:
 +case VIR_DOMAIN_NET_TYPE_LAST:
 +/* these types all require no action */
 +break;
 +}
 +
 +ret = 0;
 + cleanup:
 +return ret;
 +}
 +
 +/**
 + * qemuInterfaceStopDevices:
 + * @def: domain definition
 + *
 + * Make all interfaces associated with this domain inaccessible from
 + * the rest of the network.
 + */
 +int
 +qemuInterfaceStopDevices(virDomainDefPtr def)
 +{
 +size_t i;
 +
 +for (i = 0; i  def-nnets; i++) {
 +if (qemuInterfaceStopDevice(def-nets[i])  0)
 +return -1;
 +}
 +return 0;
 +}
 diff --git a/src/qemu/qemu_interface.h b/src/qemu/qemu_interface.h
 index d040f52..b4c1efc 100644
 --- a/src/qemu/qemu_interface.h
 +++ b/src/qemu/qemu_interface.h
 @@ -28,5 +28,7 @@
 
  int qemuInterfaceStartDevice(virDomainNetDefPtr net);
  int qemuInterfaceStartDevices(virDomainDefPtr def);
 +int qemuInterfaceStopDevice(virDomainNetDefPtr net);
 +int qemuInterfaceStopDevices(virDomainDefPtr def);
 
  #endif /* __QEMU_INTERFACE_H__ */
 diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
 index 0028283..a19e71a 100644
 --- a/src/qemu/qemu_process.c
 +++ b/src/qemu/qemu_process.c
 @@ -3182,6 +3182,9 @@ int qemuProcessStopCPUs(virQEMUDriverPtr driver,
  if (ret  0)
  goto cleanup;
 
 +/* de-activate netdevs after stopping CPUs */
 +ignore_value(qemuInterfaceStopDevices(vm-def));
 +
  if (priv-job.current)
  ignore_value(virTimeMillisNow(priv-job.current-stopped));
 

Reviewed by: Matthew Rosato mjros...@linux.vnet.ibm.com

--
libvir-list mailing list

Re: [libvirt] [PATCH v3] network: Bring netdevs online later

[Matthew Rosato]

On 12/11/2014 01:35 PM, Laine Stump wrote:
 On 09/16/2014 04:50 PM, Matthew Rosato wrote:
  
  #include cpu/cpu.h
  #include datatypes.h
 @@ -2947,6 +2948,12 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, 
 virDomainObjPtr vm,
  qemuDomainObjPrivatePtr priv = vm-privateData;
  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
  
 +/* Bring up netdevs before starting CPUs */
 +if (reason != VIR_DOMAIN_RUNNING_UNPAUSED 
 +reason != VIR_DOMAIN_RUNNING_SAVE_CANCELED) {
 +qemuInterfaceStartDevices(vm-def);
 +}
 Matthew,
 
 
 I've already pushed your patch, but am trying to add to it for another
 related purpose and this bit is bothering me. What is the reason for not
 calling qemuInterfaceStartDevices() when reason is
 VIR_DOMAIN_RUNNING_UNPAUSED or VIR_DOMAIN_RUNNING_SAVE_CANCELED? Is it
 just to avoid setting IFF_UP on an interface that is already IFF_UP?
 
 If that's the only reason, I would prefer to have it *always* called
 when the CPUs are started (and a corresponding
 qemuInterfaceStopDevices() called when the CPUs are stopped). Otherwise,
 it looks to me like it is possible in some situations (migration error
 recovery perhaps? search for VIR_DOMAIN_RUNNING_UNPAUSED) to have the
 CPUs running, but the macvtap interfaces *not* IFF_UP.
 
 Do you see (or did you experience?) a problem calling
 qemuInterfaceStartDevices() for all reasons?
 

Hi Laine,

I did not experience any problems calling qemuInterfaceStartDevices()
unconditionally, that's actually how I originally wrote the code -- I
added these conditional statements based on a review comment to avoid
unnecessary IFF_UPs.

I'd be fine with the call being unconditional again.

Matt

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

[libvirt] [PATCH v3 repost] network: Bring netdevs online later

[Matthew Rosato]

Repost of a patch that got lost in the shuffle.  The last version 
(v3) was based on review comments from Martin Kletzander but needs 
additional review.  
Here's a link back to the v2 post, which was the last to receive 
comments:  
http://www.redhat.com/archives/libvir-list/2014-August/msg01332.html

This repost is identical in content to the previous v3 submission, 
save for retrofit needed.

Associated bugzilla: 
https://bugzilla.redhat.com/show_bug.cgi?id=1081461

Changes for v3:
 * Some minor formatting fixes.
 * in qemuNetworkIfaceConnect, set VIR_NETDEV_TAP_CREATE_IFUP 
   unconditionally.
 * in qemuDomainAttachNetDevice, call qemuInterfaceStartDevice on for 
   VIR_DOMAIN_NET_TYPE_DIRECT, _BRIDGE and _NETWORK. 
 * in qemuProcessStartCPUs, use 'reason' to determine whether or not 
   qemuInterfaceStartDevices needs to be called.  Basically, it needs 
   to be called for any reason that the system would be initializing, 
   as well as potentially after a failed migration.

Matthew Rosato (1):
  network: Bring netdevs online later

 src/Makefile.am |3 +-
 src/conf/domain_conf.h  |2 ++
 src/lxc/lxc_process.c   |4 ++-
 src/qemu/qemu_command.c |3 ++
 src/qemu/qemu_hotplug.c |8 +
 src/qemu/qemu_interface.c   |   76 +++
 src/qemu/qemu_interface.h   |   32 ++
 src/qemu/qemu_process.c |7 
 src/util/virnetdevmacvlan.c |8 +++--
 src/util/virnetdevmacvlan.h |2 ++
 10 files changed, 140 insertions(+), 5 deletions(-)
 create mode 100644 src/qemu/qemu_interface.c
 create mode 100644 src/qemu/qemu_interface.h

-- 
1.7.9.5

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

[libvirt] [PATCH v3 repost] network: Bring netdevs online later

[Matthew Rosato]

Currently, MAC registration occurs during device creation, which is
early enough that, during live migration, you end up with duplicate
MAC addresses on still-running source and target devices, even though
the target device isn't actually being used yet.
This patch proposes to defer MAC registration until right before
the guest can actually use the device -- In other words, right
before starting guest CPUs.

Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
---
 src/Makefile.am |3 +-
 src/conf/domain_conf.h  |2 ++
 src/lxc/lxc_process.c   |4 ++-
 src/qemu/qemu_command.c |3 ++
 src/qemu/qemu_hotplug.c |8 +
 src/qemu/qemu_interface.c   |   76 +++
 src/qemu/qemu_interface.h   |   32 ++
 src/qemu/qemu_process.c |7 
 src/util/virnetdevmacvlan.c |8 +++--
 src/util/virnetdevmacvlan.h |2 ++
 10 files changed, 140 insertions(+), 5 deletions(-)
 create mode 100644 src/qemu/qemu_interface.c
 create mode 100644 src/qemu/qemu_interface.h

diff --git a/src/Makefile.am b/src/Makefile.am
index d8fe624..88125c6 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -733,7 +733,8 @@ QEMU_DRIVER_SOURCES =   
\
qemu/qemu_monitor_text.h\
qemu/qemu_monitor_json.c\
qemu/qemu_monitor_json.h\
-   qemu/qemu_driver.c qemu/qemu_driver.h
+   qemu/qemu_driver.c qemu/qemu_driver.h   \
+   qemu/qemu_interface.c qemu/qemu_interface.h
 
 XENAPI_DRIVER_SOURCES =\
xenapi/xenapi_driver.c xenapi/xenapi_driver.h   \
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index dcb30bc..3603f7c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -985,6 +985,8 @@ struct _virDomainNetDef {
 virNetDevVlan vlan;
 int trustGuestRxFilters; /* enum virTristateBool */
 int linkstate;
+/* vmOp value saved if deferring interface start */
+virNetDevVPortProfileOp vmOp;
 };
 
 /* Used for prefix of ifname of any network name generated dynamically
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 9208f02..57df90c 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -297,6 +297,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 virNetDevVPortProfilePtr prof;
 virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
 const char *linkdev = virDomainNetGetActualDirectDev(net);
+unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;
 
 /* XXX how todo bandwidth controls ?
  * Since the 'net-ifname' is about to be moved to a different
@@ -332,7 +333,8 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 prof,
 res_ifname,
 VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
-cfg-stateDir, 0)  0)
+cfg-stateDir,
+macvlan_create_flags)  0)
 goto cleanup;
 
 ret = res_ifname;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4ed6506..6ccd17d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -200,6 +200,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
 net-ifname = res_ifname;
 }
 
+/* Save vport profile op for later */
+net-vmOp = vmop;
+
 virObjectUnref(cfg);
 return rc;
 }
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b00fd8f..5893ec8 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -30,6 +30,7 @@
 #include qemu_domain.h
 #include qemu_command.h
 #include qemu_hostdev.h
+#include qemu_interface.h
 #include domain_audit.h
 #include netdev_bandwidth_conf.h
 #include domain_nwfilter.h
@@ -922,6 +923,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
 priv-qemuCaps, tapfd, tapfdSize)  0)
 goto cleanup;
 iface_connected = true;
+/* Set device online immediately */
+qemuInterfaceStartDevice(net);
 if (qemuOpenVhostNet(vm-def, net, priv-qemuCaps, vhostfd, 
vhostfdSize)  0)
 goto cleanup;
 } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
@@ -937,6 +940,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
  
VIR_NETDEV_VPORT_PROFILE_OP_CREATE))  0)
 goto cleanup;
 iface_connected = true;
+/* Set device online immediately */
+qemuInterfaceStartDevice(net);
 if (qemuOpenVhostNet(vm-def, net, priv-qemuCaps, vhostfd, 
vhostfdSize)  0)
 goto cleanup;
 } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
@@ -2087,6 +2092,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
 goto cleanup;
 }
 
+/* Set device online immediately

Re: [libvirt] [PATCH v3] network: Bring netdevs online later

[Matthew Rosato]

On 09/16/2014 04:50 PM, Matthew Rosato wrote:
 Currently, MAC registration occurs during device creation, which is
 early enough that, during live migration, you end up with duplicate
 MAC addresses on still-running source and target devices, even though
 the target device isn't actually being used yet.
 This patch proposes to defer MAC registration until right before
 the guest can actually use the device -- In other words, right
 before starting guest CPUs.
 
 Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
 ---

Ping

 
 Associated BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1081461
 
 Changes for v3:
  * Some minor formatting fixes.
  * in qemuNetworkIfaceConnect, set VIR_NETDEV_TAP_CREATE_IFUP 
unconditionally.
  * in qemuDomainAttachNetDevice, call qemuInterfaceStartDevice only for 
VIR_DOMAIN_NET_TYPE_DIRECT, _BRIDGE and _NETWORK. 
  * in qemuProcessStartCPUs, use 'reason' to determine whether or not 
qemuInterfaceStartDevices needs to be called.  Basically, it needs 
to be called for any reason that the system would be initializing 
(or re-initializing).
 
  src/Makefile.am |3 +-
  src/conf/domain_conf.h  |2 ++
  src/lxc/lxc_process.c   |4 ++-
  src/qemu/qemu_command.c |3 ++
  src/qemu/qemu_hotplug.c |8 +
  src/qemu/qemu_interface.c   |   78 
 +++
  src/qemu/qemu_interface.h   |   32 ++
  src/qemu/qemu_process.c |7 
  src/util/virnetdevmacvlan.c |8 +++--
  src/util/virnetdevmacvlan.h |2 ++
  10 files changed, 142 insertions(+), 5 deletions(-)
  create mode 100644 src/qemu/qemu_interface.c
  create mode 100644 src/qemu/qemu_interface.h
 
 diff --git a/src/Makefile.am b/src/Makefile.am
 index fa741a8..035120e 100644
 --- a/src/Makefile.am
 +++ b/src/Makefile.am
 @@ -703,7 +703,8 @@ QEMU_DRIVER_SOURCES = 
 \
   qemu/qemu_monitor_text.h\
   qemu/qemu_monitor_json.c\
   qemu/qemu_monitor_json.h\
 - qemu/qemu_driver.c qemu/qemu_driver.h
 + qemu/qemu_driver.c qemu/qemu_driver.h   \
 + qemu/qemu_interface.c qemu/qemu_interface.h
 
  XENAPI_DRIVER_SOURCES =  \
   xenapi/xenapi_driver.c xenapi/xenapi_driver.h   \
 diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
 index 0862bd7..5f328cf 100644
 --- a/src/conf/domain_conf.h
 +++ b/src/conf/domain_conf.h
 @@ -951,6 +951,8 @@ struct _virDomainNetDef {
  virNetDevBandwidthPtr bandwidth;
  virNetDevVlan vlan;
  int linkstate;
 +/* vmOp value saved if deferring interface start */
 +virNetDevVPortProfileOp vmOp;
  };
 
  /* Used for prefix of ifname of any network name generated dynamically
 diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
 index ed30c37..b2256c0 100644
 --- a/src/lxc/lxc_process.c
 +++ b/src/lxc/lxc_process.c
 @@ -300,6 +300,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr 
 conn,
  virNetDevBandwidthPtr bw;
  virNetDevVPortProfilePtr prof;
  virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
 +unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;
 
  /* XXX how todo bandwidth controls ?
   * Since the 'net-ifname' is about to be moved to a different
 @@ -336,7 +337,8 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr 
 conn,
  res_ifname,
  VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
  cfg-stateDir,
 -virDomainNetGetActualBandwidth(net), 0)  0)
 +virDomainNetGetActualBandwidth(net),
 +macvlan_create_flags)  0)
  goto cleanup;
 
  ret = res_ifname;
 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
 index e5270bd..229dff4 100644
 --- a/src/qemu/qemu_command.c
 +++ b/src/qemu/qemu_command.c
 @@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
  net-ifname = res_ifname;
  }
 
 +/* Save vport profile op for later */
 +net-vmOp = vmop;
 +
  virObjectUnref(cfg);
  return rc;
  }
 diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
 index 7bc19cd..530e6da 100644
 --- a/src/qemu/qemu_hotplug.c
 +++ b/src/qemu/qemu_hotplug.c
 @@ -30,6 +30,7 @@
  #include qemu_domain.h
  #include qemu_command.h
  #include qemu_hostdev.h
 +#include qemu_interface.h
  #include domain_audit.h
  #include domain_nwfilter.h
  #include virlog.h
 @@ -922,6 +923,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
  priv-qemuCaps, tapfd, tapfdSize)  0)
  goto cleanup;
  iface_connected = true;
 +/* Set device online immediately */
 +qemuInterfaceStartDevice(net);
  if (qemuOpenVhostNet(vm-def, net, priv-qemuCaps, vhostfd

[libvirt] [PATCH v3] network: Bring netdevs online later

[Matthew Rosato]

Currently, MAC registration occurs during device creation, which is
early enough that, during live migration, you end up with duplicate
MAC addresses on still-running source and target devices, even though
the target device isn't actually being used yet.
This patch proposes to defer MAC registration until right before
the guest can actually use the device -- In other words, right
before starting guest CPUs.

Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
---

Associated BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1081461

Changes for v3:
 * Some minor formatting fixes.
 * in qemuNetworkIfaceConnect, set VIR_NETDEV_TAP_CREATE_IFUP 
   unconditionally.
 * in qemuDomainAttachNetDevice, call qemuInterfaceStartDevice only for 
   VIR_DOMAIN_NET_TYPE_DIRECT, _BRIDGE and _NETWORK. 
 * in qemuProcessStartCPUs, use 'reason' to determine whether or not 
   qemuInterfaceStartDevices needs to be called.  Basically, it needs 
   to be called for any reason that the system would be initializing 
   (or re-initializing).

 src/Makefile.am |3 +-
 src/conf/domain_conf.h  |2 ++
 src/lxc/lxc_process.c   |4 ++-
 src/qemu/qemu_command.c |3 ++
 src/qemu/qemu_hotplug.c |8 +
 src/qemu/qemu_interface.c   |   78 +++
 src/qemu/qemu_interface.h   |   32 ++
 src/qemu/qemu_process.c |7 
 src/util/virnetdevmacvlan.c |8 +++--
 src/util/virnetdevmacvlan.h |2 ++
 10 files changed, 142 insertions(+), 5 deletions(-)
 create mode 100644 src/qemu/qemu_interface.c
 create mode 100644 src/qemu/qemu_interface.h

diff --git a/src/Makefile.am b/src/Makefile.am
index fa741a8..035120e 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -703,7 +703,8 @@ QEMU_DRIVER_SOURCES =   
\
qemu/qemu_monitor_text.h\
qemu/qemu_monitor_json.c\
qemu/qemu_monitor_json.h\
-   qemu/qemu_driver.c qemu/qemu_driver.h
+   qemu/qemu_driver.c qemu/qemu_driver.h   \
+   qemu/qemu_interface.c qemu/qemu_interface.h
 
 XENAPI_DRIVER_SOURCES =\
xenapi/xenapi_driver.c xenapi/xenapi_driver.h   \
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0862bd7..5f328cf 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -951,6 +951,8 @@ struct _virDomainNetDef {
 virNetDevBandwidthPtr bandwidth;
 virNetDevVlan vlan;
 int linkstate;
+/* vmOp value saved if deferring interface start */
+virNetDevVPortProfileOp vmOp;
 };
 
 /* Used for prefix of ifname of any network name generated dynamically
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index ed30c37..b2256c0 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -300,6 +300,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 virNetDevBandwidthPtr bw;
 virNetDevVPortProfilePtr prof;
 virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
+unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;
 
 /* XXX how todo bandwidth controls ?
  * Since the 'net-ifname' is about to be moved to a different
@@ -336,7 +337,8 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 res_ifname,
 VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
 cfg-stateDir,
-virDomainNetGetActualBandwidth(net), 0)  0)
+virDomainNetGetActualBandwidth(net),
+macvlan_create_flags)  0)
 goto cleanup;
 
 ret = res_ifname;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index e5270bd..229dff4 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
 net-ifname = res_ifname;
 }
 
+/* Save vport profile op for later */
+net-vmOp = vmop;
+
 virObjectUnref(cfg);
 return rc;
 }
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 7bc19cd..530e6da 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -30,6 +30,7 @@
 #include qemu_domain.h
 #include qemu_command.h
 #include qemu_hostdev.h
+#include qemu_interface.h
 #include domain_audit.h
 #include domain_nwfilter.h
 #include virlog.h
@@ -922,6 +923,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
 priv-qemuCaps, tapfd, tapfdSize)  0)
 goto cleanup;
 iface_connected = true;
+/* Set device online immediately */
+qemuInterfaceStartDevice(net);
 if (qemuOpenVhostNet(vm-def, net, priv-qemuCaps, vhostfd, 
vhostfdSize)  0)
 goto cleanup;
 } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
@@ -937,6 +940,8 @@ int qemuDomainAttachNetDevice(virConnectPtr

Re: [libvirt] [PATCH v2 1/2] util: Introduce flags field for macvtap creation

[Matthew Rosato]

On 08/28/2014 08:45 AM, Martin Kletzander wrote:
 On Wed, Aug 27, 2014 at 10:34:13AM -0400, Matthew Rosato wrote:
 Currently, there is one flag passed in during macvtap creation
 (withTap) -- Let's convert this field to an unsigned int flag
 field for future expansion.

 Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
 ---
 src/lxc/lxc_process.c   |4 ++--
 src/qemu/qemu_command.c |6 --
 src/util/virnetdevmacvlan.c |   28 +---
 src/util/virnetdevmacvlan.h |   14 ++
 4 files changed, 33 insertions(+), 19 deletions(-)

 diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
 index 3353dc1..ed30c37 100644
 --- a/src/lxc/lxc_process.c
 +++ b/src/lxc/lxc_process.c
 @@ -331,12 +331,12 @@ char
 *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 net-ifname, net-mac,
 virDomainNetGetActualDirectDev(net),
 virDomainNetGetActualDirectMode(net),
 -false, false, def-uuid,
 +false, def-uuid,
 virDomainNetGetActualVirtPortProfile(net),
 res_ifname,
 VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
 cfg-stateDir,
 -virDomainNetGetActualBandwidth(net))  0)
 +virDomainNetGetActualBandwidth(net), 0)  0)
 goto cleanup;

 ret = res_ifname;
 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
 index 9241f57..1f71fa3 100644
 --- a/src/qemu/qemu_command.c
 +++ b/src/qemu/qemu_command.c
 @@ -177,6 +177,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
 char *res_ifname = NULL;
 int vnet_hdr = 0;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 +unsigned int macvlan_create_flags =
 VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;

 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNET_HDR) 
 net-model  STREQ(net-model, virtio))
 @@ -186,11 +187,12 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
 net-ifname, net-mac,
 virDomainNetGetActualDirectDev(net),
 virDomainNetGetActualDirectMode(net),
 -true, vnet_hdr, def-uuid,
 +vnet_hdr, def-uuid,
 virDomainNetGetActualVirtPortProfile(net),
 res_ifname,
 vmop, cfg-stateDir,
 -virDomainNetGetActualBandwidth(net));
 +virDomainNetGetActualBandwidth(net),
 +macvlan_create_flags);
 if (rc = 0) {
 virDomainAuditNetDevice(def, net, res_ifname, true);
 VIR_FREE(net-ifname);
 diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
 index 054194b..9ddeca4 100644
 --- a/src/util/virnetdevmacvlan.c
 +++ b/src/util/virnetdevmacvlan.c
 @@ -790,6 +790,7 @@ virNetDevMacVLanVPortProfileRegisterCallback(const
 char *ifname,
  * @macaddress: The MAC address for the macvtap device
  * @linkdev: The interface name of the NIC to connect to the external
 bridge
  * @mode: int describing the mode for 'bridge', 'vepa', 'private' or
 'passthru'.
 + * @flags: OR of virNetDevMacVLanCreateFlags.
 
 I took the liberty of moving this as a last parameter as well.
 
  * @vnet_hdr: 1 to enable IFF_VNET_HDR, 0 to disable it
  * @vmuuid: The UUID of the VM the macvtap belongs to
  * @virtPortProfile: pointer to object holding the virtual port
 profile data
 @@ -797,25 +798,29 @@
 virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
  * interface will be stored into if everything succeeded. It is up
  * to the caller to free the string.
  *
 - * Returns file descriptor of the tap device in case of success with
 @withTap,
 - * otherwise returns 0; returns -1 on error.
 + * Returns file descriptor of the tap device in case of success with
 + * @flags  VIR_NETDEV_MACVLAN_CREATE_WITH_TAP, otherwise returns 0;
 returns
 + * -1 on error.
  */
 int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
const virMacAddr *macaddress,
const char *linkdev,
virNetDevMacVLanMode mode,
 -   bool withTap,
int vnet_hdr,
const unsigned char *vmuuid,
virNetDevVPortProfilePtr
 virtPortProfile,
char **res_ifname,
virNetDevVPortProfileOp vmOp,
char *stateDir,
 -   virNetDevBandwidthPtr
 bandwidth)
 +   virNetDevBandwidthPtr
 bandwidth,
 +   unsigned int flags)
 {
 -const char *type = withTap ? macvtap : macvlan;
 -const char *prefix = withTap ? MACVTAP_NAME_PREFIX :
 MACVLAN_NAME_PREFIX;
 -const char *pattern = withTap ? MACVTAP_NAME_PATTERN :
 MACVLAN_NAME_PATTERN;
 +const char *type = (flags

Re: [libvirt] [PATCH v2 2/2] network: Bring netdevs online later

[Matthew Rosato]

On 08/28/2014 09:56 AM, Martin Kletzander wrote:
 On Wed, Aug 27, 2014 at 10:34:14AM -0400, Matthew Rosato wrote:
 Currently, MAC registration occurs during device creation, which is
 early enough that, during live migration, you end up with duplicate
 MAC addresses on still-running source and target devices, even though
 the target device isn't actually being used yet.
 This patch proposes to defer MAC registration until right before
 the guest can actually use the device -- In other words, right
 before starting guest CPUs.

 Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
 ---
 src/Makefile.am |3 +-
 src/conf/domain_conf.h  |2 ++
 src/lxc/lxc_process.c   |4 ++-
 src/qemu/qemu_command.c |6 +++-
 src/qemu/qemu_hotplug.c |7 
 src/qemu/qemu_interface.c   |   78
 +++
 src/qemu/qemu_interface.h   |   32 ++
 src/qemu/qemu_process.c |4 +++
 src/util/virnetdevmacvlan.c |8 +++--
 src/util/virnetdevmacvlan.h |2 ++
 10 files changed, 140 insertions(+), 6 deletions(-)
 create mode 100644 src/qemu/qemu_interface.c
 create mode 100644 src/qemu/qemu_interface.h

 diff --git a/src/Makefile.am b/src/Makefile.am
 index 46e411e..842573f 100644
 --- a/src/Makefile.am
 +++ b/src/Makefile.am
 @@ -703,7 +703,8 @@ QEMU_DRIVER_SOURCES =\
 qemu/qemu_monitor_text.h\
 qemu/qemu_monitor_json.c\
 qemu/qemu_monitor_json.h\
 -qemu/qemu_driver.c qemu/qemu_driver.h
 +qemu/qemu_driver.c qemu/qemu_driver.h   \
 
 I don't know if this got mixed by the mail client or not, but I'm
 adjusting it (the backslash) to match the others.
 
 +qemu/qemu_interface.c qemu/qemu_interface.h

 
 I still don't see anything qemu-specific inthose files, but for now
 separate files are fine, we can move the code around any time later if
 it's needed.

I mentioned this in the cover letter comments, but you may have missed it:

 * I left the contents of qemu_interface as-is, rather than collapsing
them into non-qemu-specific functions, in order to keep Makefile linkage
consistent  happy (needs to be part of QEMU_DRIVER_SOURCES).  Instead,
incorporated copyright suggestions from previous comments.  

Basically, you're right in that there is nothing inherently
qemu-specific in this code, just that it's currently only being written
with qemu in mind, and it sticks with the current Makefile convention
(everything in QEMU_DRIVER_SOURCES is part of src/qemu/qemu_*).

 
 XENAPI_DRIVER_SOURCES =\
 xenapi/xenapi_driver.c xenapi/xenapi_driver.h\
 diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
 index aead903..6268690 100644
 --- a/src/conf/domain_conf.h
 +++ b/src/conf/domain_conf.h
 @@ -950,6 +950,8 @@ struct _virDomainNetDef {
 virNetDevBandwidthPtr bandwidth;
 virNetDevVlan vlan;
 int linkstate;
 +/* vmOp value saved if deferring interface start */
 +virNetDevVPortProfileOp vmOp;
 };

 /* Used for prefix of ifname of any network name generated dynamically
 diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
 index ed30c37..b2256c0 100644
 --- a/src/lxc/lxc_process.c
 +++ b/src/lxc/lxc_process.c
 @@ -300,6 +300,7 @@ char
 *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 virNetDevBandwidthPtr bw;
 virNetDevVPortProfilePtr prof;
 virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
 +unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;

 /* XXX how todo bandwidth controls ?
  * Since the 'net-ifname' is about to be moved to a different
 @@ -336,7 +337,8 @@ char
 *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 res_ifname,
 VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
 cfg-stateDir,
 -virDomainNetGetActualBandwidth(net), 0)  0)
 +virDomainNetGetActualBandwidth(net),
 +macvlan_create_flags)  0)
 goto cleanup;

 ret = res_ifname;
 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
 index 1f71fa3..43a8b63 100644
 --- a/src/qemu/qemu_command.c
 +++ b/src/qemu/qemu_command.c
 @@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
 net-ifname = res_ifname;
 }

 +/* Save vport profile op for later */
 +net-vmOp = vmop;
 +
 virObjectUnref(cfg);
 return rc;
 }
 @@ -286,7 +289,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
 {
 char *brname = NULL;
 int ret = -1;
 -unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
 +unsigned int tap_create_flags = 0;
 bool template_ifname = false;
 int actualType = virDomainNetGetActualType(net);
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 @@ -346,6 +349,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
 goto cleanup;
 }
 } else {
 +tap_create_flags

[libvirt] [PATCH v2 1/2] util: Introduce flags field for macvtap creation

[Matthew Rosato]

Currently, there is one flag passed in during macvtap creation
(withTap) -- Let's convert this field to an unsigned int flag
field for future expansion.

Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
---
 src/lxc/lxc_process.c   |4 ++--
 src/qemu/qemu_command.c |6 --
 src/util/virnetdevmacvlan.c |   28 +---
 src/util/virnetdevmacvlan.h |   14 ++
 4 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 3353dc1..ed30c37 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -331,12 +331,12 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr 
conn,
 net-ifname, net-mac,
 virDomainNetGetActualDirectDev(net),
 virDomainNetGetActualDirectMode(net),
-false, false, def-uuid,
+false, def-uuid,
 virDomainNetGetActualVirtPortProfile(net),
 res_ifname,
 VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
 cfg-stateDir,
-virDomainNetGetActualBandwidth(net))  0)
+virDomainNetGetActualBandwidth(net), 0)  0)
 goto cleanup;
 
 ret = res_ifname;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9241f57..1f71fa3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -177,6 +177,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
 char *res_ifname = NULL;
 int vnet_hdr = 0;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNET_HDR) 
 net-model  STREQ(net-model, virtio))
@@ -186,11 +187,12 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
 net-ifname, net-mac,
 virDomainNetGetActualDirectDev(net),
 virDomainNetGetActualDirectMode(net),
-true, vnet_hdr, def-uuid,
+vnet_hdr, def-uuid,
 virDomainNetGetActualVirtPortProfile(net),
 res_ifname,
 vmop, cfg-stateDir,
-virDomainNetGetActualBandwidth(net));
+virDomainNetGetActualBandwidth(net),
+macvlan_create_flags);
 if (rc = 0) {
 virDomainAuditNetDevice(def, net, res_ifname, true);
 VIR_FREE(net-ifname);
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 054194b..9ddeca4 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -790,6 +790,7 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char 
*ifname,
  * @macaddress: The MAC address for the macvtap device
  * @linkdev: The interface name of the NIC to connect to the external bridge
  * @mode: int describing the mode for 'bridge', 'vepa', 'private' or 
'passthru'.
+ * @flags: OR of virNetDevMacVLanCreateFlags.
  * @vnet_hdr: 1 to enable IFF_VNET_HDR, 0 to disable it
  * @vmuuid: The UUID of the VM the macvtap belongs to
  * @virtPortProfile: pointer to object holding the virtual port profile data
@@ -797,25 +798,29 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char 
*ifname,
  * interface will be stored into if everything succeeded. It is up
  * to the caller to free the string.
  *
- * Returns file descriptor of the tap device in case of success with @withTap,
- * otherwise returns 0; returns -1 on error.
+ * Returns file descriptor of the tap device in case of success with
+ * @flags  VIR_NETDEV_MACVLAN_CREATE_WITH_TAP, otherwise returns 0; returns
+ * -1 on error.
  */
 int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
const virMacAddr *macaddress,
const char *linkdev,
virNetDevMacVLanMode mode,
-   bool withTap,
int vnet_hdr,
const unsigned char *vmuuid,
virNetDevVPortProfilePtr 
virtPortProfile,
char **res_ifname,
virNetDevVPortProfileOp vmOp,
char *stateDir,
-   virNetDevBandwidthPtr bandwidth)
+   virNetDevBandwidthPtr bandwidth,
+   unsigned int flags)
 {
-const char *type = withTap ? macvtap : macvlan;
-const char *prefix = withTap ? MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX;
-const char *pattern = withTap ? MACVTAP_NAME_PATTERN : 
MACVLAN_NAME_PATTERN;
+const char *type = (flags  VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
+macvtap : macvlan;
+const char *prefix = (flags  VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
+MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX;
+const char *pattern = (flags

[libvirt] [PATCH v2 2/2] network: Bring netdevs online later

[Matthew Rosato]

Currently, MAC registration occurs during device creation, which is
early enough that, during live migration, you end up with duplicate
MAC addresses on still-running source and target devices, even though
the target device isn't actually being used yet.  
This patch proposes to defer MAC registration until right before
the guest can actually use the device -- In other words, right
before starting guest CPUs.

Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
---
 src/Makefile.am |3 +-
 src/conf/domain_conf.h  |2 ++
 src/lxc/lxc_process.c   |4 ++-
 src/qemu/qemu_command.c |6 +++-
 src/qemu/qemu_hotplug.c |7 
 src/qemu/qemu_interface.c   |   78 +++
 src/qemu/qemu_interface.h   |   32 ++
 src/qemu/qemu_process.c |4 +++
 src/util/virnetdevmacvlan.c |8 +++--
 src/util/virnetdevmacvlan.h |2 ++
 10 files changed, 140 insertions(+), 6 deletions(-)
 create mode 100644 src/qemu/qemu_interface.c
 create mode 100644 src/qemu/qemu_interface.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 46e411e..842573f 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -703,7 +703,8 @@ QEMU_DRIVER_SOURCES =   
\
qemu/qemu_monitor_text.h\
qemu/qemu_monitor_json.c\
qemu/qemu_monitor_json.h\
-   qemu/qemu_driver.c qemu/qemu_driver.h
+   qemu/qemu_driver.c qemu/qemu_driver.h   \
+   qemu/qemu_interface.c qemu/qemu_interface.h
 
 XENAPI_DRIVER_SOURCES =\
xenapi/xenapi_driver.c xenapi/xenapi_driver.h   \
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index aead903..6268690 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -950,6 +950,8 @@ struct _virDomainNetDef {
 virNetDevBandwidthPtr bandwidth;
 virNetDevVlan vlan;
 int linkstate;
+/* vmOp value saved if deferring interface start */
+virNetDevVPortProfileOp vmOp;
 };
 
 /* Used for prefix of ifname of any network name generated dynamically
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index ed30c37..b2256c0 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -300,6 +300,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 virNetDevBandwidthPtr bw;
 virNetDevVPortProfilePtr prof;
 virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
+unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;
 
 /* XXX how todo bandwidth controls ?
  * Since the 'net-ifname' is about to be moved to a different
@@ -336,7 +337,8 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 res_ifname,
 VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
 cfg-stateDir,
-virDomainNetGetActualBandwidth(net), 0)  0)
+virDomainNetGetActualBandwidth(net),
+macvlan_create_flags)  0)
 goto cleanup;
 
 ret = res_ifname;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1f71fa3..43a8b63 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
 net-ifname = res_ifname;
 }
 
+/* Save vport profile op for later */
+net-vmOp = vmop;
+
 virObjectUnref(cfg);
 return rc;
 }
@@ -286,7 +289,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
 {
 char *brname = NULL;
 int ret = -1;
-unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
+unsigned int tap_create_flags = 0;
 bool template_ifname = false;
 int actualType = virDomainNetGetActualType(net);
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
@@ -346,6 +349,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
 goto cleanup;
 }
 } else {
+tap_create_flags |= VIR_NETDEV_TAP_CREATE_IFUP;
 if (qemuCreateInBridgePortWithHelper(cfg, brname,
  net-ifname,
  tapfd, tap_create_flags)  0) {
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a364c52..633eda5 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -30,6 +30,7 @@
 #include qemu_domain.h
 #include qemu_command.h
 #include qemu_hostdev.h
+#include qemu_interface.h
 #include domain_audit.h
 #include domain_nwfilter.h
 #include virlog.h
@@ -878,6 +879,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
 if (networkAllocateActualDevice(vm-def, net)  0)
 goto cleanup;
 
+/* Set device online immediately */
+qemuInterfaceStartDevice(net);
+
 actualType = virDomainNetGetActualType(net);
 
 if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
@@ -2069,6

[libvirt] [PATCH v2 0/2] network: Bring netdevs online later

[Matthew Rosato]

The following patchset introduces code to defer setting netdevs online
(and therefore registering MACs) until right before beginning guest 
CPU execution.  The first patch introduces some infrastructure changes
in preparation of the actual function added in the 2nd patch.  

Associated BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1081461

Changes for v2:
 * Ping for comments, esp. on patch #2.
 * Moved @flags operand of virNetDevMacVLanCreateWithVPortProfile to the 
   end of the operand list.
 * Minor changes based on comments by Martin Kletzander.
 * Added detail to patch #2 commit message.  
 * Martin suggested that I replace various ? operations with if/else 
   statements, but I ended up leaving them alone as they were being used 
   to conditionally assign values to constant fields.
 * I left the contents of qemu_interface as-is, rather than collapsing 
   them into non-qemu-specific functions, in order to keep Makefile linkage
   consistent  happy (needs to be part of QEMU_DRIVER_SOURCES).  Instead,
   incorporated copyright suggestions from previous comments.  Martin, if
   you feel strongly about not having these new functions in a qemu-specific
   part, feel free to comment.

Changes since RFC:
 * Add a separate patch to introduce a flags field for macvlan/macvtap 
   creation.
 * Use macvlan/tap IFUP flags to skip virNetDevSetOnline (for qemu only).  
 * Add hotplug support.
 * For macvlan, save the current virNetDevVPortProfileOp in virDomainNetDef
   during qemuPhysIfaceConnect.  As Laine mentioned, could use this field in
   a future patch to eliminate passing virNetDevVPortProfileOp everywhere.  
 * Add qemu_interface.c and qemu_interface.h to encapsulate new functions. 

Matthew Rosato (2):
  util: Introduce flags field for macvtap creation
  network: Bring netdevs online later

 src/Makefile.am |3 +-
 src/conf/domain_conf.h  |2 ++
 src/lxc/lxc_process.c   |6 ++--
 src/qemu/qemu_command.c |   12 +--
 src/qemu/qemu_hotplug.c |7 
 src/qemu/qemu_interface.c   |   78 +++
 src/qemu/qemu_interface.h   |   32 ++
 src/qemu/qemu_process.c |4 +++
 src/util/virnetdevmacvlan.c |   36 
 src/util/virnetdevmacvlan.h |   16 ++---
 10 files changed, 172 insertions(+), 24 deletions(-)
 create mode 100644 src/qemu/qemu_interface.c
 create mode 100644 src/qemu/qemu_interface.h

-- 
1.7.9.5

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

Re: [libvirt] [PATCH 1/2] util: Introduce flags field for macvtap creation

[Matthew Rosato]

On 07/23/2014 08:56 AM, Martin Kletzander wrote:
 On Tue, Jul 01, 2014 at 02:00:56PM -0400, Matthew Rosato wrote:
 Currently, there is one flag passed in during macvtap creation
 (withTap) -- Let's convert this field to an unsigned int flag
 field for future expansion.

 Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
 ---
 src/lxc/lxc_process.c   |2 +-
 src/qemu/qemu_command.c |3 ++-
 src/util/virnetdevmacvlan.c |   24 +++-
 src/util/virnetdevmacvlan.h |8 +++-
 4 files changed, 25 insertions(+), 12 deletions(-)

 diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
 index cb85b74..bfbecbf 100644
 --- a/src/util/virnetdevmacvlan.c
 +++ b/src/util/virnetdevmacvlan.c
 @@ -790,6 +790,7 @@ virNetDevMacVLanVPortProfileRegisterCallback(const
 char *ifname,
  * @macaddress: The MAC address for the macvtap device
  * @linkdev: The interface name of the NIC to connect to the external
 bridge
  * @mode: int describing the mode for 'bridge', 'vepa', 'private' or
 'passthru'.
 + * @flags: OR of virNetDevMacVLanCreateFlags.
  * @vnet_hdr: 1 to enable IFF_VNET_HDR, 0 to disable it
  * @vmuuid: The UUID of the VM the macvtap belongs to
  * @virtPortProfile: pointer to object holding the virtual port
 profile data
 @@ -797,14 +798,15 @@
 virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
  * interface will be stored into if everything succeeded. It is up
  * to the caller to free the string.
  *
 - * Returns file descriptor of the tap device in case of success with
 @withTap,
 - * otherwise returns 0; returns -1 on error.
 + * Returns file descriptor of the tap device in case of success with
 + * flags  VIR_NETDEV_MACVLAN_CREATE_WITH_TAP, otherwise returns 0;
 returns
 
 s/flags/@flags/

Thanks.

 
 + * -1 on error.
  */
 int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
const virMacAddr *macaddress,
const char *linkdev,
virNetDevMacVLanMode mode,
 -   bool withTap,
 +   unsigned int flags,
int vnet_hdr,
const unsigned char *vmuuid,
virNetDevVPortProfilePtr
 virtPortProfile,
 
 Having @flags as some middle parameter feels itchy, could you move it
 to the end?
 

Sure.

 [...]
 @@ -813,9 +815,12 @@ int virNetDevMacVLanCreateWithVPortProfile(const
 char *tgifname,
char *stateDir,
virNetDevBandwidthPtr
 bandwidth)
 {
 -const char *type = withTap ? macvtap : macvlan;
 -const char *prefix = withTap ? MACVTAP_NAME_PREFIX :
 MACVLAN_NAME_PREFIX;
 -const char *pattern = withTap ? MACVTAP_NAME_PATTERN :
 MACVLAN_NAME_PATTERN;
 +const char *type = (flags  VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
 +macvtap : macvlan;
 +const char *prefix = (flags  VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
 +MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX;
 +const char *pattern = (flags  VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
 +MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN;
 
 Since these are now multiline anyway, it would be nice to have them
 set in an if for more readability.

Sure.

Thanks for the comments - Sorry for the delayed reply, just returning
from vacation.

Matt

 
 Martin

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

Re: [libvirt] [PATCH 2/2] network: Bring netdevs online later

[Matthew Rosato]

On 07/23/2014 10:08 AM, Eric Blake wrote:
 On 07/23/2014 07:49 AM, Martin Kletzander wrote:
 On Tue, Jul 01, 2014 at 02:00:57PM -0400, Matthew Rosato wrote:
 Defer MAC registration until net devices are actually going
 to be used by the guest.  This patch does so by setting the
 devices online just before starting guest CPUs.


 Does this have some upside/downside?  Are you trying to fix some
 problem?  It would be nice to describe it in the commit message, so I
 know what to focus on or why it's needed.  Depending on the answer
 there might be a way how to unit-test it.

 
 +++ b/src/qemu/qemu_interface.c
 @@ -0,0 +1,65 @@
 +/*
 + * qemu_interface.c: QEMU interface management
 + *
 + * Copyright (C) 2014 Red Hat, Inc.
 + * Copyright IBM Corp. 2014
 + *

 I don't understand this double copyright here, copy-paste mistake?
 
 If this file is copied from a pre-existing file with double copyright,
 and substantially borrows from that content rather than being fresh
 material, then keeping double copyright is ideal.  If this is something
 you mostly wrote yourself, and then just copied in boilerplate, then
 it's simpler to just put one copyright line for yourself (in this case,
 IBM); then later, if other contributors make contributions, a second
 line can be added then (I tend to add Red Hat copyright whenever I touch
 files, since I'm doing my work on Red Hat time).  I don't see it as a
 show-stopper either way, because these days, git log is more reliable
 for telling who contributed what portions of a file.

Copied for boilerplate, so I'll strike the Red Hat line for now --
Thanks for the detailed info!

 

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

Re: [libvirt] [PATCH 2/2] network: Bring netdevs online later

[Matthew Rosato]

On 07/23/2014 09:49 AM, Martin Kletzander wrote:
 On Tue, Jul 01, 2014 at 02:00:57PM -0400, Matthew Rosato wrote:
 Defer MAC registration until net devices are actually going
 to be used by the guest.  This patch does so by setting the
 devices online just before starting guest CPUs.

 
 Does this have some upside/downside?  Are you trying to fix some
 problem?  It would be nice to describe it in the commit message, so I
 know what to focus on or why it's needed.  Depending on the answer
 there might be a way how to unit-test it.

(sorry for the late reply, just returning from vacation)

Well, it's attempting to fix the problem linked in the cover letter:
https://bugzilla.redhat.com/show_bug.cgi?id=1081461
as well as a problem Wangrui (on CC) reported separately:
https://www.redhat.com/archives/libvir-list/2014-March/msg01054.html

In short, MAC registration currently occurs during target device
creation, early enough that you end up with a duplicate MAC address on
the still-running source and the target device -- But the target isn't
really even using the device yet (not running yet).  The longer the live
migration takes, the wider the window where this disparity exists, and
could cause packets intended for the still-running source to get routed
to the not-yet-running target.  The patch proposes to shrink this window
by not upping those netdevs until right before CPUs are started (ie,
immediately before the guest will actually start using them).

I will try to make the problem statement a little clearer in the commit
message for the next version.

 
 Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
 ---
 src/Makefile.am |1 +
 src/conf/domain_conf.h  |2 ++
 src/lxc/lxc_process.c   |3 +-
 src/qemu/qemu_command.c |6 +++-
 src/qemu/qemu_hotplug.c |7 +
 src/qemu/qemu_interface.c   |   65
 +++
 src/qemu/qemu_interface.h   |   33 ++
 src/qemu/qemu_process.c |4 +++
 src/util/virnetdevmacvlan.c |8 --
 src/util/virnetdevmacvlan.h |2 ++
 10 files changed, 126 insertions(+), 5 deletions(-)
 create mode 100644 src/qemu/qemu_interface.c
 create mode 100644 src/qemu/qemu_interface.h

 diff --git a/src/Makefile.am b/src/Makefile.am
 index 35720be..e3d2e36 100644
 --- a/src/Makefile.am
 +++ b/src/Makefile.am
 @@ -687,6 +687,7 @@ QEMU_DRIVER_SOURCES =\
 qemu/qemu_domain.c qemu/qemu_domain.h\
 qemu/qemu_cgroup.c qemu/qemu_cgroup.h\
 qemu/qemu_hostdev.c qemu/qemu_hostdev.h\
 +qemu/qemu_interface.c qemu/qemu_interface.h\
 qemu/qemu_hotplug.c qemu/qemu_hotplug.h\
 qemu/qemu_hotplugpriv.h\
 qemu/qemu_conf.c qemu/qemu_conf.h\
 diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
 index 1122eb2..8375106 100644
 --- a/src/conf/domain_conf.h
 +++ b/src/conf/domain_conf.h
 @@ -921,6 +921,8 @@ struct _virDomainNetDef {
 virNetDevBandwidthPtr bandwidth;
 virNetDevVlan vlan;
 int linkstate;
 +/* vmOp value saved if deferring interface start */
 +virNetDevVPortProfileOp vmOp;
 };

 /* Used for prefix of ifname of any network name generated dynamically
 diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
 index ab0c5d0..3083ed3 100644
 --- a/src/lxc/lxc_process.c
 +++ b/src/lxc/lxc_process.c
 @@ -302,6 +302,7 @@ char
 *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 virNetDevBandwidthPtr bw;
 virNetDevVPortProfilePtr prof;
 virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
 +unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;

 /* XXX how todo bandwidth controls ?
  * Since the 'net-ifname' is about to be moved to a different
 @@ -333,7 +334,7 @@ char
 *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 net-ifname, net-mac,
 virDomainNetGetActualDirectDev(net),
 virDomainNetGetActualDirectMode(net),
 -0, false, def-uuid,
 +macvlan_create_flags, false, def-uuid,
 virDomainNetGetActualVirtPortProfile(net),
 res_ifname,
 VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
 index 314d8a3..a5662f5 100644
 --- a/src/qemu/qemu_command.c
 +++ b/src/qemu/qemu_command.c
 @@ -196,6 +196,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
 net-ifname = res_ifname;
 }

 +/* Save vport profile op for later */
 +net-vmOp = vmop;
 +
 virObjectUnref(cfg);
 return rc;

 @@ -294,7 +297,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
 {
 char *brname = NULL;
 int ret = -1;
 -unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
 +unsigned int tap_create_flags = 0;
 bool template_ifname = false;
 int actualType = virDomainNetGetActualType(net

Re: [libvirt] [PATCH 0/2] (for 1.2.7) network: Bring netdevs online later

[Matthew Rosato]

On 07/01/2014 02:00 PM, Matthew Rosato wrote:
 The following patchset introduces code to defer setting netdevs online
 (and therefore registering MACs) until right before beginning guest 
 CPU execution.  The first patch introduces some infrastructure changes
 in preparation of the actual function added in the 2nd patch.  
 
 Associated BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1081461

Ping...

 
 Changes since RFC:
  * Add a separate patch to introduce a flags field for macvlan/macvtap 
creation.
  * Use macvlan/tap IFUP flags to skip virNetDevSetOnline (for qemu only).  
  * Add hotplug support.
  * For macvlan, save the current virNetDevVPortProfileOp in virDomainNetDef
during qemuPhysIfaceConnect.  As Laine mentioned, could use this field in
a future patch to eliminate passing virNetDevVPortProfileOp everywhere.  
  * Add qemu_interface.c and qemu_interface.h to encapsulate new functions.
 
 Matthew Rosato (2):
   util: Introduce flags field for macvtap creation
   network: Bring netdevs online later
 
  src/Makefile.am |1 +
  src/conf/domain_conf.h  |2 ++
  src/lxc/lxc_process.c   |3 +-
  src/qemu/qemu_command.c |9 --
  src/qemu/qemu_hotplug.c |7 +
  src/qemu/qemu_interface.c   |   65 
 +++
  src/qemu/qemu_interface.h   |   33 ++
  src/qemu/qemu_process.c |4 +++
  src/util/virnetdevmacvlan.c |   32 +
  src/util/virnetdevmacvlan.h |   10 ++-
  10 files changed, 150 insertions(+), 16 deletions(-)
  create mode 100644 src/qemu/qemu_interface.c
  create mode 100644 src/qemu/qemu_interface.h
 

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

[libvirt] [PATCH 1/2] util: Introduce flags field for macvtap creation

[Matthew Rosato]

Currently, there is one flag passed in during macvtap creation
(withTap) -- Let's convert this field to an unsigned int flag
field for future expansion.

Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
---
 src/lxc/lxc_process.c   |2 +-
 src/qemu/qemu_command.c |3 ++-
 src/util/virnetdevmacvlan.c |   24 +++-
 src/util/virnetdevmacvlan.h |8 +++-
 4 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 0aef13a..ab0c5d0 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -333,7 +333,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 net-ifname, net-mac,
 virDomainNetGetActualDirectDev(net),
 virDomainNetGetActualDirectMode(net),
-false, false, def-uuid,
+0, false, def-uuid,
 virDomainNetGetActualVirtPortProfile(net),
 res_ifname,
 VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 63f322a..314d8a3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -171,6 +171,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
 char *res_ifname = NULL;
 int vnet_hdr = 0;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNET_HDR) 
 net-model  STREQ(net-model, virtio))
@@ -180,7 +181,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
 net-ifname, net-mac,
 virDomainNetGetActualDirectDev(net),
 virDomainNetGetActualDirectMode(net),
-true, vnet_hdr, def-uuid,
+macvlan_create_flags, vnet_hdr, def-uuid,
 virDomainNetGetActualVirtPortProfile(net),
 res_ifname,
 vmop, cfg-stateDir,
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index cb85b74..bfbecbf 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -790,6 +790,7 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char 
*ifname,
  * @macaddress: The MAC address for the macvtap device
  * @linkdev: The interface name of the NIC to connect to the external bridge
  * @mode: int describing the mode for 'bridge', 'vepa', 'private' or 
'passthru'.
+ * @flags: OR of virNetDevMacVLanCreateFlags.
  * @vnet_hdr: 1 to enable IFF_VNET_HDR, 0 to disable it
  * @vmuuid: The UUID of the VM the macvtap belongs to
  * @virtPortProfile: pointer to object holding the virtual port profile data
@@ -797,14 +798,15 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char 
*ifname,
  * interface will be stored into if everything succeeded. It is up
  * to the caller to free the string.
  *
- * Returns file descriptor of the tap device in case of success with @withTap,
- * otherwise returns 0; returns -1 on error.
+ * Returns file descriptor of the tap device in case of success with
+ * flags  VIR_NETDEV_MACVLAN_CREATE_WITH_TAP, otherwise returns 0; returns
+ * -1 on error.
  */
 int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
const virMacAddr *macaddress,
const char *linkdev,
virNetDevMacVLanMode mode,
-   bool withTap,
+   unsigned int flags,
int vnet_hdr,
const unsigned char *vmuuid,
virNetDevVPortProfilePtr 
virtPortProfile,
@@ -813,9 +815,12 @@ int virNetDevMacVLanCreateWithVPortProfile(const char 
*tgifname,
char *stateDir,
virNetDevBandwidthPtr bandwidth)
 {
-const char *type = withTap ? macvtap : macvlan;
-const char *prefix = withTap ? MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX;
-const char *pattern = withTap ? MACVTAP_NAME_PATTERN : 
MACVLAN_NAME_PATTERN;
+const char *type = (flags  VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
+macvtap : macvlan;
+const char *prefix = (flags  VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
+MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX;
+const char *pattern = (flags  VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
+MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN;
 int c, rc;
 char ifname[IFNAMSIZ];
 int retries, do_retry = 0;
@@ -903,7 +908,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char 
*tgifname,
 goto disassociate_exit;
 }
 
-if (withTap) {
+if (flags  VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) {
 if ((rc = virNetDevMacVLanTapOpen(cr_ifname, 10))  0)
 goto disassociate_exit;
 
@@ -925,7 +930,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char 
*tgifname

[libvirt] [PATCH 0/2] (for 1.2.7) network: Bring netdevs online later

[Matthew Rosato]

The following patchset introduces code to defer setting netdevs online
(and therefore registering MACs) until right before beginning guest 
CPU execution.  The first patch introduces some infrastructure changes
in preparation of the actual function added in the 2nd patch.  

Associated BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1081461

Changes since RFC:
 * Add a separate patch to introduce a flags field for macvlan/macvtap 
   creation.
 * Use macvlan/tap IFUP flags to skip virNetDevSetOnline (for qemu only).  
 * Add hotplug support.
 * For macvlan, save the current virNetDevVPortProfileOp in virDomainNetDef
   during qemuPhysIfaceConnect.  As Laine mentioned, could use this field in
   a future patch to eliminate passing virNetDevVPortProfileOp everywhere.  
 * Add qemu_interface.c and qemu_interface.h to encapsulate new functions.

Matthew Rosato (2):
  util: Introduce flags field for macvtap creation
  network: Bring netdevs online later

 src/Makefile.am |1 +
 src/conf/domain_conf.h  |2 ++
 src/lxc/lxc_process.c   |3 +-
 src/qemu/qemu_command.c |9 --
 src/qemu/qemu_hotplug.c |7 +
 src/qemu/qemu_interface.c   |   65 +++
 src/qemu/qemu_interface.h   |   33 ++
 src/qemu/qemu_process.c |4 +++
 src/util/virnetdevmacvlan.c |   32 +
 src/util/virnetdevmacvlan.h |   10 ++-
 10 files changed, 150 insertions(+), 16 deletions(-)
 create mode 100644 src/qemu/qemu_interface.c
 create mode 100644 src/qemu/qemu_interface.h

-- 
1.7.9.5

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

[libvirt] [PATCH 2/2] network: Bring netdevs online later

[Matthew Rosato]

Defer MAC registration until net devices are actually going
to be used by the guest.  This patch does so by setting the
devices online just before starting guest CPUs.

Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
---
 src/Makefile.am |1 +
 src/conf/domain_conf.h  |2 ++
 src/lxc/lxc_process.c   |3 +-
 src/qemu/qemu_command.c |6 +++-
 src/qemu/qemu_hotplug.c |7 +
 src/qemu/qemu_interface.c   |   65 +++
 src/qemu/qemu_interface.h   |   33 ++
 src/qemu/qemu_process.c |4 +++
 src/util/virnetdevmacvlan.c |8 --
 src/util/virnetdevmacvlan.h |2 ++
 10 files changed, 126 insertions(+), 5 deletions(-)
 create mode 100644 src/qemu/qemu_interface.c
 create mode 100644 src/qemu/qemu_interface.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 35720be..e3d2e36 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -687,6 +687,7 @@ QEMU_DRIVER_SOURCES =   
\
qemu/qemu_domain.c qemu/qemu_domain.h   \
qemu/qemu_cgroup.c qemu/qemu_cgroup.h   \
qemu/qemu_hostdev.c qemu/qemu_hostdev.h \
+   qemu/qemu_interface.c qemu/qemu_interface.h \
qemu/qemu_hotplug.c qemu/qemu_hotplug.h \
qemu/qemu_hotplugpriv.h \
qemu/qemu_conf.c qemu/qemu_conf.h   \
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 1122eb2..8375106 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -921,6 +921,8 @@ struct _virDomainNetDef {
 virNetDevBandwidthPtr bandwidth;
 virNetDevVlan vlan;
 int linkstate;
+/* vmOp value saved if deferring interface start */
+virNetDevVPortProfileOp vmOp;
 };
 
 /* Used for prefix of ifname of any network name generated dynamically
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index ab0c5d0..3083ed3 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -302,6 +302,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 virNetDevBandwidthPtr bw;
 virNetDevVPortProfilePtr prof;
 virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
+unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;
 
 /* XXX how todo bandwidth controls ?
  * Since the 'net-ifname' is about to be moved to a different
@@ -333,7 +334,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
 net-ifname, net-mac,
 virDomainNetGetActualDirectDev(net),
 virDomainNetGetActualDirectMode(net),
-0, false, def-uuid,
+macvlan_create_flags, false, def-uuid,
 virDomainNetGetActualVirtPortProfile(net),
 res_ifname,
 VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 314d8a3..a5662f5 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -196,6 +196,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
 net-ifname = res_ifname;
 }
 
+/* Save vport profile op for later */
+net-vmOp = vmop;
+
 virObjectUnref(cfg);
 return rc;
 
@@ -294,7 +297,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
 {
 char *brname = NULL;
 int ret = -1;
-unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
+unsigned int tap_create_flags = 0;
 bool template_ifname = false;
 int actualType = virDomainNetGetActualType(net);
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
@@ -354,6 +357,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
 goto cleanup;
 }
 } else {
+tap_create_flags |= VIR_NETDEV_TAP_CREATE_IFUP;
 if (qemuCreateInBridgePortWithHelper(cfg, brname,
  net-ifname,
  tapfd, tap_create_flags)  0) {
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 5e8aa4e..98e7764 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -49,6 +49,7 @@
 #include virstoragefile.h
 #include virstring.h
 #include virtime.h
+#include qemu_interface.h
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
@@ -854,6 +855,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
 if (networkAllocateActualDevice(vm-def, net)  0)
 goto cleanup;
 
+/* Set device online immediately */
+qemuInterfaceStartDevice(net);
+
 actualType = virDomainNetGetActualType(net);
 
 if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
@@ -2030,6 +2034,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
 goto cleanup;
 }
 
+/* Set device online immediately */
+qemuInterfaceStartDevice(newdev);
+
 newType = virDomainNetGetActualType(newdev);
 
 if (newType

Re: [libvirt] [PATCH RFC] network: Bring netdevs online later

[Matthew Rosato]

On 06/09/2014 08:55 AM, Laine Stump wrote:
 On 06/04/2014 05:55 PM, Matthew Rosato wrote:
 Defer MAC registration until net devices are actually going
 to be used by the guest.  This patch does so by setting the
 devices online just before starting guest CPUs.

 This approach is an alternative to my previously proposed
 'network: Defer online of macvtap during qemu migration'
 Laine/Wangrui, is this the sort of thing you had in mind?
 
 Yes and no. It is basically what I was thinking - *always* wait to bring
 up the devices right before turning on the CPU of the guest. However, it
 doesn't account for hotplug - In that case, the CPUs have already been
 started, and the single device that has just been hotplugged needs to be
 started. This patch doesn't do anything about that. And there are a few
 other problems I saw from reading through it as well (this is without
 compiling/testing it):
 

Thanks very much for your detailed comments, this is exactly what I was
looking for and why I marked as RFC -- Wanted to make sure I was even in
the right ballpark with this.


 Previous thread:
 https://www.redhat.com/archives/libvir-list/2014-May/msg00427.html
 Associated BZ:
 https://bugzilla.redhat.com/show_bug.cgi?id=1081461

 Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
 ---
  src/qemu/qemu_command.c |   45 
 +++
  src/qemu/qemu_command.h |3 +++
  src/qemu/qemu_process.c |3 +++
  src/util/virnetdevmacvlan.c |5 -
  src/util/virnetdevtap.c |3 ---
  5 files changed, 51 insertions(+), 8 deletions(-)

 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
 index e6acced..c161d73 100644
 --- a/src/qemu/qemu_command.c
 +++ b/src/qemu/qemu_command.c
 @@ -571,6 +571,51 @@ qemuNetworkPrepareDevices(virDomainDefPtr def)
  return ret;
  }
  
 +void
 +qemuNetworkIfaceUp(virDomainNetDefPtr net)
 +{
 +if (virNetDevSetOnline(net-ifname, true)  0) {
 +ignore_value(virNetDevTapDelete(net-ifname));
 +}
 +return;
 +}
 +
 +void
 +qemuPhysIfaceUp(virDomainNetDefPtr net)
 +{
 +if (virNetDevSetOnline(net-ifname, true)  0) {
 +ignore_value(virNetDevVPortProfileDisassociate(net-ifname,
 +   
 virDomainNetGetActualVirtPortProfile(net),
 +   net-mac,
 +   
 virDomainNetGetActualDirectDev(net),
 +   -1,
 +   
 VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH));
 
 Is this always the proper vmop (MIGRATE_IN_FINISH) no matter what the
 situation is? (remember it could now be happening not as the result of a
 failure during migration, but also as the result of a failure during the
 initial start of a domain, or during a hotplug).
 

D'oh.  Good catch, I forgot this was even being passed in (as you
probably guessed, it was copied directly from my migration-specific patch).

 (I *really* dislike the way that this vmop (which is only used by low
 level macvtap functions) must be passed around through multiple layers
 of the callstack in higher level functions (existing problem), and
 wish/hope that there is a way to make it more localized, perhaps by
 storing the current state in the NetworkDef object as status somehow.
 But that's a separate issue, so for now we have to just continue passing
 it around.)
 
 +ignore_value(virNetDevMacVLanDelete(net-ifname));
 +}
 +return;
 +}
 
 I think it would be better to have a single function that takes a
 virDomainNetPtr and contains the switch statement. This way 1) a call to
 it can easily be added in the proper place to support hotplug, and 2)
 that one function can later be moved to the same final location as what
 is currently called networkAllocateActualDevice() and those two
 functions can become part of an API that will allow non-privileged
 libvirt instances to use these network types.


OK, sure.

 +
 +void
 +qemuNetworkInitializeDevices(virDomainDefPtr def)
 
 I think the verb Start would be better than Initialize in this case
 - that one is too easily confused with the already existing Prepare.
 

Yeah, I went back-and-forth on the naming, Start sounds fine.

 Also, I think we should create a qemu_interface.c to contain all of
 these functions, similar to how we currently have a qemu_hostdev.c for
 functions related to hostdev.
 
 

OK.

 +{
 +size_t i;
 +
 +for (i = 0; i  def-nnets; i++) {
 +virDomainNetDefPtr net = def-nets[i];
 +switch(virDomainNetGetActualType(net)) {
 +case VIR_DOMAIN_NET_TYPE_BRIDGE:
 +case VIR_DOMAIN_NET_TYPE_NETWORK:
 +qemuNetworkIfaceUp(net);
 +break;
 +case VIR_DOMAIN_NET_TYPE_DIRECT:
 +qemuPhysIfaceUp(net);
 +break

[libvirt] [PATCH RFC] network: Bring netdevs online later

[Matthew Rosato]

Defer MAC registration until net devices are actually going
to be used by the guest.  This patch does so by setting the
devices online just before starting guest CPUs.

This approach is an alternative to my previously proposed
'network: Defer online of macvtap during qemu migration'
Laine/Wangrui, is this the sort of thing you had in mind?

Previous thread:
https://www.redhat.com/archives/libvir-list/2014-May/msg00427.html
Associated BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1081461

Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
---
 src/qemu/qemu_command.c |   45 +++
 src/qemu/qemu_command.h |3 +++
 src/qemu/qemu_process.c |3 +++
 src/util/virnetdevmacvlan.c |5 -
 src/util/virnetdevtap.c |3 ---
 5 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index e6acced..c161d73 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -571,6 +571,51 @@ qemuNetworkPrepareDevices(virDomainDefPtr def)
 return ret;
 }
 
+void
+qemuNetworkIfaceUp(virDomainNetDefPtr net)
+{
+if (virNetDevSetOnline(net-ifname, true)  0) {
+ignore_value(virNetDevTapDelete(net-ifname));
+}
+return;
+}
+
+void
+qemuPhysIfaceUp(virDomainNetDefPtr net)
+{
+if (virNetDevSetOnline(net-ifname, true)  0) {
+ignore_value(virNetDevVPortProfileDisassociate(net-ifname,
+   
virDomainNetGetActualVirtPortProfile(net),
+   net-mac,
+   
virDomainNetGetActualDirectDev(net),
+   -1,
+   
VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH));
+ignore_value(virNetDevMacVLanDelete(net-ifname));
+}
+return;
+}
+
+void
+qemuNetworkInitializeDevices(virDomainDefPtr def)
+{
+size_t i;
+
+for (i = 0; i  def-nnets; i++) {
+virDomainNetDefPtr net = def-nets[i];
+switch(virDomainNetGetActualType(net)) {
+case VIR_DOMAIN_NET_TYPE_BRIDGE:
+case VIR_DOMAIN_NET_TYPE_NETWORK:
+qemuNetworkIfaceUp(net);
+break;
+case VIR_DOMAIN_NET_TYPE_DIRECT:
+qemuPhysIfaceUp(net);
+break;
+}
+}
+
+return;
+}
+
 static int qemuDomainDeviceAliasIndex(const virDomainDeviceInfo *info,
   const char *prefix)
 {
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index afbd6ff..4a44464 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -206,6 +206,9 @@ int qemuOpenVhostNet(virDomainDefPtr def,
  int *vhostfdSize);
 
 int qemuNetworkPrepareDevices(virDomainDefPtr def);
+void qemuNetworkIfaceUp(virDomainNetDefPtr net);
+void qemuPhysIfaceUp(virDomainNetDefPtr net);
+void qemuNetworkInitializeDevices(virDomainDefPtr def);
 
 /*
  * NB: def-name can be NULL upon return and the caller
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d719716..bbc11f3 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2765,6 +2765,9 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, 
virDomainObjPtr vm,
 qemuDomainObjPrivatePtr priv = vm-privateData;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 
+/* Bring up netdevs before starting CPUs */
+qemuNetworkInitializeDevices(vm-def);
+
 VIR_DEBUG(Using lock state '%s', NULLSTR(priv-lockState));
 if (virDomainLockProcessResume(driver-lockManager, cfg-uri,
vm, priv-lockState)  0) {
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index cb85b74..3748527 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -898,11 +898,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const char 
*tgifname,
 goto link_del_exit;
 }
 
-if (virNetDevSetOnline(cr_ifname, true)  0) {
-rc = -1;
-goto disassociate_exit;
-}
-
 if (withTap) {
 if ((rc = virNetDevMacVLanTapOpen(cr_ifname, 10))  0)
 goto disassociate_exit;
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index 0b444fa..09b9c12 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -574,9 +574,6 @@ int virNetDevTapCreateInBridgePort(const char *brname,
 goto error;
 }
 
-if (virNetDevSetOnline(*ifname, !!(flags  VIR_NETDEV_TAP_CREATE_IFUP))  
0)
-goto error;
-
 return 0;
 
  error:
-- 
1.7.9.5

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

Re: [libvirt] [PATCH] network: Defer online of macvtap during qemu migration

[Matthew Rosato]

On 05/05/2014 12:33 PM, Matthew Rosato wrote:
 On 05/05/2014 12:26 PM, Matthew Rosato wrote:
 When generating macvtaps via virNetDevMacVLanCreateWithVPortProfile,
 the macvtap device is unconditionally set to the up state.  However,
 during migration, this results in a case where both the source and
 target system are simultaneously up with the same MAC address.  This
 patch defers bringing the target macvtap up until later in the
 migration to shrink this window.

 Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
 
 Forgot to mention that this patch is associated with what Wangrui
 reported here:
 
 http://www.redhat.com/archives/libvir-list/2014-March/msg01054.html
 
 and follows Viktor's suggested solution mentioned here:
 
 http://www.redhat.com/archives/libvir-list/2014-March/msg01654.html
 
 

Ping.

 ---
  src/qemu/qemu_migration.c   |   18 ++
  src/util/virnetdevmacvlan.c |   11 ---
  2 files changed, 26 insertions(+), 3 deletions(-)

 diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
 index a9f7fea..aee803a 100644
 --- a/src/qemu/qemu_migration.c
 +++ b/src/qemu/qemu_migration.c
 @@ -56,6 +56,7 @@
  #include virhook.h
  #include virstring.h
  #include virtypedparam.h
 +#include virnetdev.h

  #define VIR_FROM_THIS VIR_FROM_QEMU

 @@ -4468,6 +4469,8 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
  virCapsPtr caps = NULL;
  unsigned short port;
 +virDomainNetDefPtr net;
 +size_t i;

  VIR_DEBUG(driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, 
cookieout=%p, cookieoutlen=%p, flags=%lx, retcode=%d,
 @@ -4574,6 +4577,21 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
  }

  if (!(flags  VIR_MIGRATE_PAUSED)  !(flags  
 VIR_MIGRATE_OFFLINE)) {
 +/* Macvtaps were previously left offline, bring them online now 
 */
 +for (i = 0; i  vm-def-nnets; i++) {
 +net = vm-def-nets[i];
 +if (virDomainNetGetActualType(net) == 
 VIR_DOMAIN_NET_TYPE_DIRECT) {
 +if (virNetDevSetOnline(net-ifname, true)  0) {
 +
 ignore_value(virNetDevVPortProfileDisassociate(net-ifname,
 +   
 virDomainNetGetActualVirtPortProfile(net),
 +   
 net-mac,
 +   
 virDomainNetGetActualDirectDev(net),
 +   -1,
 +   
 VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH));
 +ignore_value(virNetDevMacVLanDelete(net-ifname));
 +}
 +}
 +}
  /* run 'cont' on the destination, which allows migration on qemu
   * = 0.10.6 to work properly.  This isn't strictly necessary on
   * older qemu's, but it also doesn't hurt anything there
 diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
 index 7bbf540..3da845b 100644
 --- a/src/util/virnetdevmacvlan.c
 +++ b/src/util/virnetdevmacvlan.c
 @@ -898,9 +898,14 @@ int virNetDevMacVLanCreateWithVPortProfile(const char 
 *tgifname,
  goto link_del_exit;
  }

 -if (virNetDevSetOnline(cr_ifname, true)  0) {
 -rc = -1;
 -goto disassociate_exit;
 +/* If this device is being created as part of an inbound
 + * migration, leave the device offline for now.
 + */
 +if (vmOp != VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) {
 +if (virNetDevSetOnline(cr_ifname, true)  0) {
 +rc = -1;
 +goto disassociate_exit;
 +}
  }

  if (withTap) {

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

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

[libvirt] [PATCH] network: Defer online of macvtap during qemu migration

[Matthew Rosato]

When generating macvtaps via virNetDevMacVLanCreateWithVPortProfile,
the macvtap device is unconditionally set to the up state.  However,
during migration, this results in a case where both the source and
target system are simultaneously up with the same MAC address.  This
patch defers bringing the target macvtap up until later in the
migration to shrink this window.

Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
---
 src/qemu/qemu_migration.c   |   18 ++
 src/util/virnetdevmacvlan.c |   11 ---
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index a9f7fea..aee803a 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -56,6 +56,7 @@
 #include virhook.h
 #include virstring.h
 #include virtypedparam.h
+#include virnetdev.h
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
@@ -4468,6 +4469,8 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 virCapsPtr caps = NULL;
 unsigned short port;
+virDomainNetDefPtr net;
+size_t i;
 
 VIR_DEBUG(driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, 
   cookieout=%p, cookieoutlen=%p, flags=%lx, retcode=%d,
@@ -4574,6 +4577,21 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
 }
 
 if (!(flags  VIR_MIGRATE_PAUSED)  !(flags  VIR_MIGRATE_OFFLINE)) {
+/* Macvtaps were previously left offline, bring them online now */
+for (i = 0; i  vm-def-nnets; i++) {
+net = vm-def-nets[i];
+if (virDomainNetGetActualType(net) == 
VIR_DOMAIN_NET_TYPE_DIRECT) {
+if (virNetDevSetOnline(net-ifname, true)  0) {
+
ignore_value(virNetDevVPortProfileDisassociate(net-ifname,
+   
virDomainNetGetActualVirtPortProfile(net),
+   
net-mac,
+   
virDomainNetGetActualDirectDev(net),
+   -1,
+   
VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH));
+ignore_value(virNetDevMacVLanDelete(net-ifname));
+}
+}
+}
 /* run 'cont' on the destination, which allows migration on qemu
  * = 0.10.6 to work properly.  This isn't strictly necessary on
  * older qemu's, but it also doesn't hurt anything there
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 7bbf540..3da845b 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -898,9 +898,14 @@ int virNetDevMacVLanCreateWithVPortProfile(const char 
*tgifname,
 goto link_del_exit;
 }
 
-if (virNetDevSetOnline(cr_ifname, true)  0) {
-rc = -1;
-goto disassociate_exit;
+/* If this device is being created as part of an inbound
+ * migration, leave the device offline for now.
+ */
+if (vmOp != VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) {
+if (virNetDevSetOnline(cr_ifname, true)  0) {
+rc = -1;
+goto disassociate_exit;
+}
 }
 
 if (withTap) {
-- 
1.7.9.5

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

Re: [libvirt] [PATCH] network: Defer online of macvtap during qemu migration

[Matthew Rosato]

On 05/05/2014 12:26 PM, Matthew Rosato wrote:
 When generating macvtaps via virNetDevMacVLanCreateWithVPortProfile,
 the macvtap device is unconditionally set to the up state.  However,
 during migration, this results in a case where both the source and
 target system are simultaneously up with the same MAC address.  This
 patch defers bringing the target macvtap up until later in the
 migration to shrink this window.
 
 Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com

Forgot to mention that this patch is associated with what Wangrui
reported here:

http://www.redhat.com/archives/libvir-list/2014-March/msg01054.html

and follows Viktor's suggested solution mentioned here:

http://www.redhat.com/archives/libvir-list/2014-March/msg01654.html


 ---
  src/qemu/qemu_migration.c   |   18 ++
  src/util/virnetdevmacvlan.c |   11 ---
  2 files changed, 26 insertions(+), 3 deletions(-)
 
 diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
 index a9f7fea..aee803a 100644
 --- a/src/qemu/qemu_migration.c
 +++ b/src/qemu/qemu_migration.c
 @@ -56,6 +56,7 @@
  #include virhook.h
  #include virstring.h
  #include virtypedparam.h
 +#include virnetdev.h
 
  #define VIR_FROM_THIS VIR_FROM_QEMU
 
 @@ -4468,6 +4469,8 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
  virCapsPtr caps = NULL;
  unsigned short port;
 +virDomainNetDefPtr net;
 +size_t i;
 
  VIR_DEBUG(driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, 
cookieout=%p, cookieoutlen=%p, flags=%lx, retcode=%d,
 @@ -4574,6 +4577,21 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
  }
 
  if (!(flags  VIR_MIGRATE_PAUSED)  !(flags  VIR_MIGRATE_OFFLINE)) 
 {
 +/* Macvtaps were previously left offline, bring them online now 
 */
 +for (i = 0; i  vm-def-nnets; i++) {
 +net = vm-def-nets[i];
 +if (virDomainNetGetActualType(net) == 
 VIR_DOMAIN_NET_TYPE_DIRECT) {
 +if (virNetDevSetOnline(net-ifname, true)  0) {
 +
 ignore_value(virNetDevVPortProfileDisassociate(net-ifname,
 +   
 virDomainNetGetActualVirtPortProfile(net),
 +   
 net-mac,
 +   
 virDomainNetGetActualDirectDev(net),
 +   -1,
 +   
 VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH));
 +ignore_value(virNetDevMacVLanDelete(net-ifname));
 +}
 +}
 +}
  /* run 'cont' on the destination, which allows migration on qemu
   * = 0.10.6 to work properly.  This isn't strictly necessary on
   * older qemu's, but it also doesn't hurt anything there
 diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
 index 7bbf540..3da845b 100644
 --- a/src/util/virnetdevmacvlan.c
 +++ b/src/util/virnetdevmacvlan.c
 @@ -898,9 +898,14 @@ int virNetDevMacVLanCreateWithVPortProfile(const char 
 *tgifname,
  goto link_del_exit;
  }
 
 -if (virNetDevSetOnline(cr_ifname, true)  0) {
 -rc = -1;
 -goto disassociate_exit;
 +/* If this device is being created as part of an inbound
 + * migration, leave the device offline for now.
 + */
 +if (vmOp != VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) {
 +if (virNetDevSetOnline(cr_ifname, true)  0) {
 +rc = -1;
 +goto disassociate_exit;
 +}
  }
 
  if (withTap) {
 

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

Re: [libvirt] [PATCH] qemu: add macvlan delete to qemuDomainAttachNetDevice cleanup

[Matthew Rosato]

On 07/01/2013 06:42 PM, Laine Stump wrote:

On 07/01/2013 11:04 AM, Viktor Mihajlovski wrote:

From: Matthew Rosato mjros...@linux.vnet.ibm.com

If an error occurs during qemuDomainAttachNetDevice after the macvtap
was created in qemuPhysIfaceConnect, the macvtap device gets left behind.
This patch adds code to the cleanup routine to delete the macvtap.

Signed-off-by: Matthew Rosato mjros...@linux.vnet.ibm.com
Reviewed-by: Viktor Mihajlovski mihaj...@linux.vnet.ibm.com
---
  src/qemu/qemu_hotplug.c |   10 ++
  1 file changed, 10 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 46875ad..c6045a0 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -965,6 +965,16 @@ cleanup:
  if (iface_connected) {
  virDomainConfNWFilterTeardown(net);

+if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) {
+ignore_value(virNetDevMacVLanDeleteWithVPortProfile(
+ net-ifname, net-mac,
+ virDomainNetGetActualDirectDev(net),
+ virDomainNetGetActualDirectMode(net),
+ virDomainNetGetActualVirtPortProfile(net),
+ cfg-stateDir));
+VIR_FREE(net-ifname);
+}
+


This is a good catch, but incomplete. If you search for other
occurrences of virDomainConfNWFilterTeardown() and
qemuPhysIfaceConnect(), you will find the same problem exists in two
other places in the code:

qemuBuildInterfaceCommandLine (during error cleanup, needs to be called
   for the one interface that was partially
   created)
qemuBuildCommandLine  (during error cleanup, needs to be called
   for all interfaces that were completely
   created (up to last_good_net))

We really should fix them all in one patch, since they are all the same
problem.


Thank you for your comments.  I tested the two cases that you mentioned 
by forcing errors; in both, the macvtap will be released by code in 
qemuProcessStop(), which releases any macvtap in the domain's nets list. 
 Is this sufficient, or did you still want something changed?




(Ideally, *all* guest interface setup for each interface should be
handled in a single function, and that function should be in the network
driver (networkReleaseActualDevice() seems properly situated). That way
it could be put behind an RPC, and the non-privileged libvirtd could
call it too (with proper credentials). That is a larger problem, though.)

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




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