Re: [libvirt] [PATCHv3] Configure native vlan modes on Open vSwitch ports
On May 10, 2013, at 1:10 PM, Laine Stump wrote: > On 04/30/2013 02:12 PM, james robson wrote: >> This patch adds functionality to allow libvirt to configure the >> 'native-tagged' and 'native-untagged' modes on openvswitch networks. >> >> v2 changes: >> Fix problems reported by Eric Blake >> >> v3 changes: >> Re work patch to address review comments >> >> --- >> >> >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >> index 888c005..5278534 100644 >> --- a/docs/formatdomain.html.in >> +++ b/docs/formatdomain.html.in >> @@ -3290,6 +3290,13 @@ qemu-kvm -net nic,model=? /dev/null >>> interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> >> >> >> + >> + >>>> + >> + ... >> +>> + >> + >> ... >> >> @@ -3316,6 +3323,15 @@ qemu-kvm -net nic,model=? /dev/null >> vlan element. >> >> >> + >> + For network connections using openvswitch it is possible to >> + configure the 'native-tagged' and 'native-untagged' vlan modes >> + (Since 1.0.6). This uses the optional >> + nativeMode attribute on the >> + element: nativeMode may be set to 'tagged' or >> + 'untagged'. The id atribute of the element sets the native vlan. >> + >> + >> Modifying virtual link state >> >> ... >> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in >> index 4dd0415..e065ca4 100644 >> --- a/docs/formatnetwork.html.in >> +++ b/docs/formatnetwork.html.in >> @@ -414,6 +414,7 @@ >> Since 0.9.4 >> >> >> + >> Setting VLAN tag (on supported network >> types only) >> >> >> @@ -429,6 +430,13 @@ >> > interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> >> >> >> + >> + >>>> + >> + ... >> +>> + >> + >> ... >> >> @@ -452,6 +460,14 @@ >> be added to the vlan element. >> >> >> + For network connections using openvswitch it is possible to >> + configure the 'native-tagged' and 'native-untagged' vlan modes >> + (Since 1.0.6). This uses the optional >> + nativeMode attribute on the >> + element: nativeMode may be set to 'tagged' or >> + 'untagged'. The id atribute of the element sets the native vlan. >> + >> + >> elements can also be specified in >> a element, as well as directly in >> a domain's element. In the case >> diff --git a/docs/schemas/networkcommon.rng >> b/docs/schemas/networkcommon.rng >> index 51ff759..e60f1fc 100644 >> --- a/docs/schemas/networkcommon.rng >> +++ b/docs/schemas/networkcommon.rng >> @@ -204,6 +204,14 @@ >> 4095 >> >> >> + >> + >> + >> +tagged >> +untagged >> + >> + >> + >> >> >> >> diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c >> index 13ba8c6..dd5b64e 100644 >> --- a/src/conf/netdev_vlan_conf.c >> +++ b/src/conf/netdev_vlan_conf.c >> @@ -17,6 +17,7 @@ >> * >> * Authors: >> * Laine Stump >> + * James Robson >> */ >> >> #include >> @@ -33,6 +34,7 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr >> ctxt, virNetDevVlanPtr de >> int ret = -1; >> xmlNodePtr save = ctxt->node; >> const char *trunk = NULL; >> +const char *nativeMode = NULL; >> xmlNodePtr *tagNodes = NULL; >> int nTags, ii; >> >> @@ -54,6 +56,8 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr >> ctxt, virNetDevVlanPtr de >> goto error; >> } >> >> +def->nativeMode = 0; >> +def->nativeTag = 0; >> for (ii = 0; ii < nTags; ii++) { >> unsigned long id; >> >> @@ -68,6 +72,21 @@ virNetDevVlanParse(xmlNodePtr node, >> xmlXPathContextPtr ctxt, virNetDevVlanPtr de >>_("vlan tag id %lu too large (maximum >> 4095)"), id); >> goto error; >> } >> +if ((nativeMode = virXPathString("string(./@nativeMode)", >> ctxt)) != NULL) { >> +if (STRCASENEQ(nativeMode, "tagged") && >> STRCASENEQ(nativeMode, "untagged")) { >> +virReportError(VIR_ERR_XML_ERROR, >> + _("invalid \"nativeMode='%s'\" in >> - " >> + "native_mode must be 'tagged' or >> 'untagged'"), nativeMode); >> +goto error; >> +} >> +if (def->nativeMode != 0) { >> +virReportError(VIR_ERR_XML_ERROR, "%s", >> + _("duplicate native vlan setting")); >> +
Re: [libvirt] [PATCH] util: do a better job of matching up pids with their binaries
On Oct 30, 2012, at 12:49 PM, Laine Stump wrote:
> On 10/30/2012 09:13 AM, Kyle Mestery (kmestery) wrote:
>> ACK.
>
> Thanks. I noticed during my final check that it was still failing make
> syntax-check due to the syntax-check rule that prohibits calling
> readlink not being specific enough (it prohibited calling any function
> that ended with "readlink"), so I squashed in the following change
> before pushing:
>
>
> diff --git a/cfg.mk b/cfg.mk
> index d3c96ba..50e6a50 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -386,7 +386,7 @@ sc_prohibit_sprintf:
> $(_sc_search_regexp)
>
> sc_prohibit_readlink:
> - @prohibit='readlink *\('\
> + @prohibit='\halt='use virFileResolveLink, not readlink' \
> $(_sc_search_regexp)
ACK to that change as well.
Thanks,
Kyle
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: do a better job of matching up pids with their binaries
On Oct 29, 2012, at 5:50 PM, Laine Stump wrote: > This patch resolves: https://bugzilla.redhat.com/show_bug.cgi?id=871201 > > If libvirt is restarted after updating the dnsmasq or radvd packages, > a subsequent "virsh net-destroy" will fail to kill the dnsmasq/radvd > process. > > The problem is that when libvirtd restarts, it re-reads the dnsmasq > and radvd pidfiles, then does a sanity check on each pid it finds, > including checking that the symbolic link in /proc/$pid/exe actually > points to the same file as the path used by libvirt to execute the > binary in the first place. If this fails, libvirt assumes that the > process is no longer alive. > > But if the original binary has been replaced, the link in /proc is set > to "$binarypath (deleted)" (it literally has the string " (deleted)" > appended to the link text stored in the filesystem), so even if a new > binary exists in the same location, attempts to resolve the link will > fail. > > In the end, not only is the old dnsmasq/radvd not terminated when the > network is stopped, but a new dnsmasq can't be started when the > network is later restarted (because the original process is still > listening on the ports that the new process wants). > > The solution is, when the initial "use stat to check for identical > inodes" check for identity between /proc/$pid/exe and $binpath fails, > to check /proc/$pid/exe for a link ending with " (deleted)" and if so, > truncate that part of the link and compare what's left with the > original binarypath. > > A twist to this problem is that on systems with "merged" /sbin and > /usr/sbin (i.e. /sbin is really just a symlink to /usr/sbin; Fedora > 17+ is an example of this), libvirt may have started the process using > one path, but /proc/$pid/exe lists a different path (indeed, on F17 > this is the case - libvirtd uses /sbin/dnsmasq, but /proc/$pid/exe > shows "/usr/sbin/dnsmasq"). The further bit of code to resolve this is > to call virFileResolveAllLinks() on both the original binarypath and > on the truncated link we read from /proc/$pid/exe, and compare the > results. > > The resulting code still succeeds in all the same cases it did before, > but also succeeds if the binary was deleted or replaced after it was > started. ACK. Thanks, Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv5 3/3] qemu_migration: Transport OVS per-port data during live migration
On Oct 23, 2012, at 12:25 PM, Laine Stump wrote: > From: Kyle Mestery > > Transport Open vSwitch per-port data during live > migration by using the utility functions > virNetDevOpenvswitchGetMigrateData() and > virNetDevOpenvswitchSetMigrateData(). > > Signed-off-by: Kyle Mestery Looks good, and passed all my tests! ACK. Acked-by: Kyle Mestery -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv5 1/3] qemu_migration: Add hooks to transport network data during migration
On Oct 23, 2012, at 12:25 PM, Laine Stump wrote: > From: Kyle Mestery > > Add the ability for the Qemu V3 migration protocol to > include transporting network configuration. A generic > framework is proposed with this patch to allow for the > transfer of opaque data. > > Signed-off-by: Kyle Mestery > Signed-off-by: Laine Stump Looks good, and passed all my tests! ACK. Acked-by: Kyle Mestery -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv5 2/3] openvswitch: Add utility functions for getting and setting Open vSwitch per-port data
On Oct 23, 2012, at 12:25 PM, Laine Stump wrote: > From: Kyle Mestery > > Add utility functions for Open vSwitch to both save > per-port data before a live migration, and restore the > per-port data after a live migration. > > Signed-off-by: Kyle Mestery Looks good, and passed all my tests! ACK. Acked-by: Kyle Mestery -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4 0/3] Transport Open vSwitch per-port data during live migration
On Oct 23, 2012, at 12:37 PM, Laine Stump wrote: > On 10/22/2012 05:30 PM, Laine Stump wrote: >> This is an update of Kyle Mestery's patch series v3 by the same name: >> >> https://www.redhat.com/archives/libvir-list/2012-October/msg00014.html > > And I've just posted a new spin based on Michal and Eric's comments. > Please test that version instead: > > https://www.redhat.com/archives/libvir-list/2012-October/msg01334.html > Just pulled those patches, building now. The good news is, the previous version you posted passed all my tests! Once this builds, I'll test it out and give you a likely ACK on all of them. Thanks for keeping up with these Laine! Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemu_migration: Add hooks to transport network data during migration
On Oct 22, 2012, at 4:21 PM, Laine Stump wrote:
> On 10/01/2012 11:18 AM, Kyle Mestery wrote:
>> Add the ability for the Qemu V3 migration protocol to
>> include transporting network configuration. A generic
>> framework is proposed with this patch to allow for the
>> transfer of opaque data.
>
> I changed this patch to 1) put the portdata in a separate element rather
> than having an extremely long attribute, 2) escape the portdata to avoid
> "surprises" if the data happened to have a character that has a
> syntactical meaning for xml, and 3) clean up miscellaneous other
> problems I've noted below.
>
> Because I'm unable to test these changes myself (and they are more than
> trivial), I'm reposting the series. Please test it and report back
> whether or not it works (ie ACK or NACK). If ACK, then I'll push the
> updated patches.
>
This is great, thanks Laine! I just now see your updated patches, I will pull
those down,
apply, rebuild and let you know how my testing goes with a ACK/NACK reply.
Thanks!
Kyle
>>
>> Signed-off-by: Kyle Mestery
>> ---
>> src/qemu/qemu_migration.c | 246
>> +-
>> 1 file changed, 244 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index db69a0a..a17ccbd 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -70,6 +70,7 @@ enum qemuMigrationCookieFlags {
>> QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS,
>> QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE,
>> QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT,
>> +QEMU_MIGRATION_COOKIE_FLAG_NETWORK,
>>
>> QEMU_MIGRATION_COOKIE_FLAG_LAST
>> };
>> @@ -77,12 +78,13 @@ enum qemuMigrationCookieFlags {
>> VIR_ENUM_DECL(qemuMigrationCookieFlag);
>> VIR_ENUM_IMPL(qemuMigrationCookieFlag,
>> QEMU_MIGRATION_COOKIE_FLAG_LAST,
>> - "graphics", "lockstate", "persistent");
>> + "graphics", "lockstate", "persistent", "network");
>>
>> enum qemuMigrationCookieFeatures {
>> QEMU_MIGRATION_COOKIE_GRAPHICS = (1 <<
>> QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS),
>> QEMU_MIGRATION_COOKIE_LOCKSTATE = (1 <<
>> QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE),
>> QEMU_MIGRATION_COOKIE_PERSISTENT = (1 <<
>> QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT),
>> +QEMU_MIGRATION_COOKIE_NETWORK = (1 <<
>> QEMU_MIGRATION_COOKIE_FLAG_NETWORK),
>> };
>>
>> typedef struct _qemuMigrationCookieGraphics qemuMigrationCookieGraphics;
>> @@ -95,6 +97,27 @@ struct _qemuMigrationCookieGraphics {
>> char *tlsSubject;
>> };
>>
>> +typedef struct _qemuMigrationCookieNetdata qemuMigrationCookieNetdata;
>> +typedef qemuMigrationCookieNetdata *qemuMigrationCookieNetdataPtr;
>> +struct _qemuMigrationCookieNetdata {
>> +int vporttype; /* enum virNetDevVPortProfile */
>> +
>> +/*
>> + * Array of pointers to saved data. Each VIF will have it's own
>> + * data to transfer.
>> + */
>> +char *portdata;
>> +};
>> +
>> +typedef struct _qemuMigrationCookieNetwork qemuMigrationCookieNetwork;
>> +typedef qemuMigrationCookieNetwork *qemuMigrationCookieNetworkPtr;
>> +struct _qemuMigrationCookieNetwork {
>> +/* How many virtual NICs are we saving data for? */
>> +int nnets;
>> +
>> +qemuMigrationCookieNetdataPtr *net;
>
> I changed this to "qemuMigratiponCookieNetdataPtr net;" to reduce the
> levels of indirection. The effect is that this is now points to an array
> of qemuMigrationCookieNetdata, rather than pointing to an array of
> pointers to qemu.Netdata. All the rest of the code (in this patch
> and in 3/3) was adjusted accordingly.
>
>> +};
>> +
>> typedef struct _qemuMigrationCookie qemuMigrationCookie;
>> typedef qemuMigrationCookie *qemuMigrationCookiePtr;
>> struct _qemuMigrationCookie {
>> @@ -120,6 +143,9 @@ struct _qemuMigrationCookie {
>>
>> /* If (flags & QEMU_MIGRATION_COOKIE_PERSISTENT) */
>> virDomainDefPtr persistent;
>> +
>> +/* If (flags & QEMU_MIGRATION_COOKIE_NETWORK) */
>> +qemuMigrationCookieNetworkPtr network;
>> };
>>
>> static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr
>> grap)
>> @@ -132,6 +158,25 @@ static void
>> qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr grap)
>> }
>>
>>
>> +static void qemuMigrationCookieNetworkFree(qemuMigrationCookieNetworkPtr
>> + network)
>
> Here and in a few other places, I changed the formatting to this:
>
> static void
> functionName( xxx,
> xxx)
>
> (note the return type and function name on separate lines, and the first
> characters of the arg types lining up rather than being offset).
>
>> +{
>> +int i;
>> +
>> +if (!network)
>> +return;
>> +
>> +for (i = 0; i < network->nnets; i++) {
>> +if (network->net[i]) {
>> +VIR_FREE(network->net[i]->portdata);
>> +VIR_FREE(network->net[i]);
>> +}
>> +}
>
> This was adjusted for the one l
Re: [libvirt] [PATCH 0/3] Transport Open vSwitch per-port data during live migration
On Oct 22, 2012, at 9:16 AM, Laine Stump wrote: > On 10/22/2012 09:41 AM, Kyle Mestery (kmestery) wrote: >> On Oct 11, 2012, at 4:36 PM, Kyle Mestery (kmestery) >> wrote: >>> On Oct 11, 2012, at 4:25 PM, Laine Stump wrote: >>>> On 10/11/2012 05:06 PM, Kyle Mestery (kmestery) wrote: >>>>> On Oct 1, 2012, at 10:18 AM, Kyle Mestery wrote: >>>>>> This series of commits has the end goal of allowing per-port data stored >>>>>> in the Open vSwitch DB to be transported during live migration. This is >>>>>> done by first providing a generic infrastructure for transporting network >>>>>> data, adding some utility functions specific to Open vSwitch, and hooking >>>>>> the two together. >>>>>> >>>>>> The framework provided is generic in that other networking data could be >>>>>> transferred as well by simply adding in additional hooks as needed. >>>>>> >>>>>> Kyle Mestery (3): >>>>>> Add the ability for the Qemu V3 migration protocol to include >>>>>> transporting network configuration. A generic framework is proposed >>>>>> with this patch to allow for the transfer of opaque data. >>>>>> Add utility functions for Open vSwitch to both save per-port data >>>>>> before a live migration, and restore the per-port data after a >>>>>> live migration. >>>>>> Transport Open vSwitch per-port data during live migration by >>>>>> using the utility functions >>>>>> virNetDevOpenvswitchGetMigrateData() and >>>>>> virNetDevOpenvswitchSetMigrateData(). >>>>>> >>>>>> src/libvirt_private.syms| 2 + >>>>>> src/qemu/qemu_migration.c | 263 >>>>>> +++- >>>>>> src/util/virnetdevopenvswitch.c | 70 +++ >>>>>> src/util/virnetdevopenvswitch.h | 6 + >>>>>> 4 files changed, 339 insertions(+), 2 deletions(-) >>>>> Just curious if anyone has had a chance to review this series yet? I >>>>> believe I've addressed all the comments >>>>> I've received so far. I may need to rebase and send it out again since >>>>> it's been almost 2 weeks since I sent it >>>>> out. >>>> Sorry I haven't responded. Lately it's been one deadline after another >>>> (actually I'm working on the latest as I type). >>>> >>> Thanks Laine! >>> >>>> One thought I've had about this - since you're just taking the data >>>> directly from ovs-vsctl and printf-ing it into a buffer, there is a >>>> potential that the data could contain xml meta characters (either by >>>> design / on purpose, or perhaps if an attacker takes over ovs-vsctl or >>>> the database in some way). This could leave the system that's the target >>>> of the migration open to attacks based on injecting "something else" >>>> into the migration cookie. >>>> >>>> Is virBufferEscapeString() enough to both guarantee nothing like that >>>> happens, as well as getting the exact same string at the destination? I >>>> *think* so, but am not sure. >>>> >>>> Also, I'm still not so sure about having the data as an attribute when >>>> it could potentially been very long. DV, what's your opinion about this? >>>> Is it okay to have very long strings as attributes, or is it considered >>>> in better taste to put something that is, say, 1000 characters long as a >>>> separate element? >>>> >>>> Anyway, at this point I don't think you need to rebase/resend. By the >>>> beginning of next week hopefully someone more wise than me about XML >>>> will have answered these two questions, and I can just squash in the >>>> minor changes and push it. >>> Thanks Laine! Let me know if you need anything else on my end. >>> >>> Thanks, >>> Kyle >> >> Just pinging again on this patch set Laine. I don't think anyone responded on >> your XML questions yet, so I'm unsure what the current status of this patch >> set >> is. I assume it's in the same state? > > Yes. That was to be my first task this morning, but I arrived at the > keyboard to a string of questions on IRC. I'll hopefully post something > for you to test before noon (EST). If it works properly, and you approve > of the changes, then I'll push. Thanks again for the update Laine! Will test things out once you shoot the patches out. Thanks, Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Transport Open vSwitch per-port data during live migration
On Oct 11, 2012, at 4:36 PM, Kyle Mestery (kmestery) wrote: > On Oct 11, 2012, at 4:25 PM, Laine Stump wrote: >> On 10/11/2012 05:06 PM, Kyle Mestery (kmestery) wrote: >>> On Oct 1, 2012, at 10:18 AM, Kyle Mestery wrote: >>>> This series of commits has the end goal of allowing per-port data stored >>>> in the Open vSwitch DB to be transported during live migration. This is >>>> done by first providing a generic infrastructure for transporting network >>>> data, adding some utility functions specific to Open vSwitch, and hooking >>>> the two together. >>>> >>>> The framework provided is generic in that other networking data could be >>>> transferred as well by simply adding in additional hooks as needed. >>>> >>>> Kyle Mestery (3): >>>> Add the ability for the Qemu V3 migration protocol to include >>>> transporting network configuration. A generic framework is proposed >>>> with this patch to allow for the transfer of opaque data. >>>> Add utility functions for Open vSwitch to both save per-port data >>>> before a live migration, and restore the per-port data after a >>>> live migration. >>>> Transport Open vSwitch per-port data during live migration by >>>> using the utility functions >>>> virNetDevOpenvswitchGetMigrateData() and >>>> virNetDevOpenvswitchSetMigrateData(). >>>> >>>> src/libvirt_private.syms| 2 + >>>> src/qemu/qemu_migration.c | 263 >>>> +++- >>>> src/util/virnetdevopenvswitch.c | 70 +++ >>>> src/util/virnetdevopenvswitch.h | 6 + >>>> 4 files changed, 339 insertions(+), 2 deletions(-) >>> >>> Just curious if anyone has had a chance to review this series yet? I >>> believe I've addressed all the comments >>> I've received so far. I may need to rebase and send it out again since it's >>> been almost 2 weeks since I sent it >>> out. >> >> Sorry I haven't responded. Lately it's been one deadline after another >> (actually I'm working on the latest as I type). >> > Thanks Laine! > >> One thought I've had about this - since you're just taking the data >> directly from ovs-vsctl and printf-ing it into a buffer, there is a >> potential that the data could contain xml meta characters (either by >> design / on purpose, or perhaps if an attacker takes over ovs-vsctl or >> the database in some way). This could leave the system that's the target >> of the migration open to attacks based on injecting "something else" >> into the migration cookie. >> >> Is virBufferEscapeString() enough to both guarantee nothing like that >> happens, as well as getting the exact same string at the destination? I >> *think* so, but am not sure. >> >> Also, I'm still not so sure about having the data as an attribute when >> it could potentially been very long. DV, what's your opinion about this? >> Is it okay to have very long strings as attributes, or is it considered >> in better taste to put something that is, say, 1000 characters long as a >> separate element? >> >> Anyway, at this point I don't think you need to rebase/resend. By the >> beginning of next week hopefully someone more wise than me about XML >> will have answered these two questions, and I can just squash in the >> minor changes and push it. > > Thanks Laine! Let me know if you need anything else on my end. > > Thanks, > Kyle Just pinging again on this patch set Laine. I don't think anyone responded on your XML questions yet, so I'm unsure what the current status of this patch set is. I assume it's in the same state? Thanks! Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Transport Open vSwitch per-port data during live migration
On Oct 11, 2012, at 4:25 PM, Laine Stump wrote: > On 10/11/2012 05:06 PM, Kyle Mestery (kmestery) wrote: >> On Oct 1, 2012, at 10:18 AM, Kyle Mestery wrote: >>> This series of commits has the end goal of allowing per-port data stored >>> in the Open vSwitch DB to be transported during live migration. This is >>> done by first providing a generic infrastructure for transporting network >>> data, adding some utility functions specific to Open vSwitch, and hooking >>> the two together. >>> >>> The framework provided is generic in that other networking data could be >>> transferred as well by simply adding in additional hooks as needed. >>> >>> Kyle Mestery (3): >>> Add the ability for the Qemu V3 migration protocol to include >>> transporting network configuration. A generic framework is proposed >>> with this patch to allow for the transfer of opaque data. >>> Add utility functions for Open vSwitch to both save per-port data >>> before a live migration, and restore the per-port data after a >>> live migration. >>> Transport Open vSwitch per-port data during live migration by >>> using the utility functions >>> virNetDevOpenvswitchGetMigrateData() and >>> virNetDevOpenvswitchSetMigrateData(). >>> >>> src/libvirt_private.syms| 2 + >>> src/qemu/qemu_migration.c | 263 >>> +++- >>> src/util/virnetdevopenvswitch.c | 70 +++ >>> src/util/virnetdevopenvswitch.h | 6 + >>> 4 files changed, 339 insertions(+), 2 deletions(-) >> >> Just curious if anyone has had a chance to review this series yet? I believe >> I've addressed all the comments >> I've received so far. I may need to rebase and send it out again since it's >> been almost 2 weeks since I sent it >> out. > > Sorry I haven't responded. Lately it's been one deadline after another > (actually I'm working on the latest as I type). > Thanks Laine! > One thought I've had about this - since you're just taking the data > directly from ovs-vsctl and printf-ing it into a buffer, there is a > potential that the data could contain xml meta characters (either by > design / on purpose, or perhaps if an attacker takes over ovs-vsctl or > the database in some way). This could leave the system that's the target > of the migration open to attacks based on injecting "something else" > into the migration cookie. > > Is virBufferEscapeString() enough to both guarantee nothing like that > happens, as well as getting the exact same string at the destination? I > *think* so, but am not sure. > > Also, I'm still not so sure about having the data as an attribute when > it could potentially been very long. DV, what's your opinion about this? > Is it okay to have very long strings as attributes, or is it considered > in better taste to put something that is, say, 1000 characters long as a > separate element? > > Anyway, at this point I don't think you need to rebase/resend. By the > beginning of next week hopefully someone more wise than me about XML > will have answered these two questions, and I can just squash in the > minor changes and push it. Thanks Laine! Let me know if you need anything else on my end. Thanks, Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Transport Open vSwitch per-port data during live migration
On Oct 1, 2012, at 10:18 AM, Kyle Mestery wrote: > This series of commits has the end goal of allowing per-port data stored > in the Open vSwitch DB to be transported during live migration. This is > done by first providing a generic infrastructure for transporting network > data, adding some utility functions specific to Open vSwitch, and hooking > the two together. > > The framework provided is generic in that other networking data could be > transferred as well by simply adding in additional hooks as needed. > > Kyle Mestery (3): > Add the ability for the Qemu V3 migration protocol to include >transporting network configuration. A generic framework is proposed >with this patch to allow for the transfer of opaque data. > Add utility functions for Open vSwitch to both save per-port data >before a live migration, and restore the per-port data after a >live migration. > Transport Open vSwitch per-port data during live migration by >using the utility functions >virNetDevOpenvswitchGetMigrateData() and >virNetDevOpenvswitchSetMigrateData(). > > src/libvirt_private.syms| 2 + > src/qemu/qemu_migration.c | 263 +++- > src/util/virnetdevopenvswitch.c | 70 +++ > src/util/virnetdevopenvswitch.h | 6 + > 4 files changed, 339 insertions(+), 2 deletions(-) Just curious if anyone has had a chance to review this series yet? I believe I've addressed all the comments I've received so far. I may need to rebase and send it out again since it's been almost 2 weeks since I sent it out. Thanks, Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Transport network port data during live migration
On Oct 1, 2012, at 9:40 AM, Kyle Mestery (kmestery) wrote: > On Oct 1, 2012, at 2:34 AM, Daniel P. Berrange wrote: >> On Mon, Oct 01, 2012 at 03:48:32AM +0000, Kyle Mestery (kmestery) wrote: >>> On Sep 30, 2012, at 9:44 AM, Laine Stump wrote: >>>> On 09/28/2012 03:58 PM, Kyle Mestery (kmestery) wrote: >>>>> As an example, an OpenFlow controller may have certain information about >>>>> the >>>>> port, specific to this controller, which it may want to store with the >>>>> port itself on the >>>>> host. This especially true if an agent exists on the host which needs to >>>>> read this data, >>>>> update it, and use it to perform some tasks. It's convenient to have this >>>>> data stored >>>>> as close to the port itself, which in this case is the OVS DB, and having >>>>> it transferred >>>>> as part of the migration protocol is also very handy. >>>>> >>>> >>>> But how big is it, and what does it look like? (I assume it's all >>>> printable ASCII, since you're getting it as the output of a shell command) >>>> >>>> If it's *really* large, possibly it would go better as a subelement of >>>> , rather than an attribute, i.e.: >>>> >>>> >>>> >>>> blah blah blah blah >>>> >>>> >>> >>> >>> Yes. it's all printable ASCII. I think at the largest, it's possible for it >>> to be up to a few K (e.g. 2-4K >>> or so). So perhaps making it a subelement would be the way to go. >>> >>> As for an example, let me talk to some controller people and see if I can >>> scrounge one up. >> >> The other reason why I want to see indicitive size is to make sure we >> don't risk hitting any limits on the size of the migration cookie. >> >> Daniel > > > For the most part, this data will be well below 4K per port. However, if a > feature such as > port security is utilized, it could be a list of allowed MAC addresses for > the port, which could > explode the length of the data, but still be below 4K per port. > > I've added all of Laine's comments into the patch set, I'm testing now. Once > that is done, I will > reformulate the patch one more time to make "portdata" a subelement of > rather than > an attribute. When I complete that, I will resubmit the patches again. > Actually, I'm going to leave this as an attribute for now, since the patches work just fine as is, unless someone has a comment about specifically refactoring. I'm resending the patches now. > Thanks, > Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Transport network port data during live migration
On Oct 1, 2012, at 2:34 AM, Daniel P. Berrange wrote: > On Mon, Oct 01, 2012 at 03:48:32AM +0000, Kyle Mestery (kmestery) wrote: >> On Sep 30, 2012, at 9:44 AM, Laine Stump wrote: >>> On 09/28/2012 03:58 PM, Kyle Mestery (kmestery) wrote: >>>> As an example, an OpenFlow controller may have certain information about >>>> the >>>> port, specific to this controller, which it may want to store with the >>>> port itself on the >>>> host. This especially true if an agent exists on the host which needs to >>>> read this data, >>>> update it, and use it to perform some tasks. It's convenient to have this >>>> data stored >>>> as close to the port itself, which in this case is the OVS DB, and having >>>> it transferred >>>> as part of the migration protocol is also very handy. >>>> >>> >>> But how big is it, and what does it look like? (I assume it's all >>> printable ASCII, since you're getting it as the output of a shell command) >>> >>> If it's *really* large, possibly it would go better as a subelement of >>> , rather than an attribute, i.e.: >>> >>> >>> >>>blah blah blah blah >>> >>> >> >> >> Yes. it's all printable ASCII. I think at the largest, it's possible for it >> to be up to a few K (e.g. 2-4K >> or so). So perhaps making it a subelement would be the way to go. >> >> As for an example, let me talk to some controller people and see if I can >> scrounge one up. > > The other reason why I want to see indicitive size is to make sure we > don't risk hitting any limits on the size of the migration cookie. > > Daniel For the most part, this data will be well below 4K per port. However, if a feature such as port security is utilized, it could be a list of allowed MAC addresses for the port, which could explode the length of the data, but still be below 4K per port. I've added all of Laine's comments into the patch set, I'm testing now. Once that is done, I will reformulate the patch one more time to make "portdata" a subelement of rather than an attribute. When I complete that, I will resubmit the patches again. Thanks, Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Transport network port data during live migration
On Sep 30, 2012, at 9:44 AM, Laine Stump wrote: > On 09/28/2012 03:58 PM, Kyle Mestery (kmestery) wrote: >> As an example, an OpenFlow controller may have certain information about the >> port, specific to this controller, which it may want to store with the port >> itself on the >> host. This especially true if an agent exists on the host which needs to >> read this data, >> update it, and use it to perform some tasks. It's convenient to have this >> data stored >> as close to the port itself, which in this case is the OVS DB, and having it >> transferred >> as part of the migration protocol is also very handy. >> > > But how big is it, and what does it look like? (I assume it's all > printable ASCII, since you're getting it as the output of a shell command) > > If it's *really* large, possibly it would go better as a subelement of > , rather than an attribute, i.e.: > > > > blah blah blah blah > > Yes. it's all printable ASCII. I think at the largest, it's possible for it to be up to a few K (e.g. 2-4K or so). So perhaps making it a subelement would be the way to go. As for an example, let me talk to some controller people and see if I can scrounge one up. Thanks, Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Transport network port data during live migration
On Sep 28, 2012, at 11:42 AM, Daniel P. Berrange wrote: > On Fri, Sep 21, 2012 at 05:16:45PM -0400, Kyle Mestery wrote: >> This series of commits has the end goal of allowing per-port data stored >> in the Open vSwitch DB to be transported during live migration. This is >> done by first providing a generic infrastructure for transporting network >> data, adding some utility functions specific to Open vSwitch, and hooking >> the two together. >> >> The framework provided is generic in that other networking data could be >> transferred as well by simply adding in additional hooks as needed. >> >> An example of the XML being transported is shown here: >> >> >> >> >> > > Could you give an example of what goes in the netdata='blah blah blah' > part of the XML ? I'm interesting in seeing the type of data, and its > indicitive size > > > Daniel Hi Daniel: >From a hypothetical angle: As an example, an OpenFlow controller may have certain information about the port, specific to this controller, which it may want to store with the port itself on the host. This especially true if an agent exists on the host which needs to read this data, update it, and use it to perform some tasks. It's convenient to have this data stored as close to the port itself, which in this case is the OVS DB, and having it transferred as part of the migration protocol is also very handy. Thanks, Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemu_migration: Add hooks to transport network port data during migration
On Sep 28, 2012, at 11:14 AM, Laine Stump wrote:
> On 09/21/2012 05:16 PM, Kyle Mestery wrote:
>> Add the ability for the Qemu V3 migration protocol to
>> include transporting network configuration. A generic
>> framework is proposed with this patch to allow for the
>> transfer of opaque data.
>
> Functionally this all looks good (and sounds like it lives up to that in
> practice :-). There are some code style issues, though…
>
Thanks for all the comments Laine. I'll address them all and
send out new versions of the patches ASAP.
Kyle
>>
>> Signed-off-by: Kyle Mestery
>> ---
>> src/qemu/qemu_migration.c | 278
>> +-
>> 1 file changed, 276 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 8e85875..06981db 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -70,6 +70,7 @@ enum qemuMigrationCookieFlags {
>> QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS,
>> QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE,
>> QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT,
>> +QEMU_MIGRATION_COOKIE_FLAG_NETWORK,
>>
>> QEMU_MIGRATION_COOKIE_FLAG_LAST
>> };
>> @@ -77,12 +78,13 @@ enum qemuMigrationCookieFlags {
>> VIR_ENUM_DECL(qemuMigrationCookieFlag);
>> VIR_ENUM_IMPL(qemuMigrationCookieFlag,
>> QEMU_MIGRATION_COOKIE_FLAG_LAST,
>> - "graphics", "lockstate", "persistent");
>> + "graphics", "lockstate", "persistent", "network");
>>
>> enum qemuMigrationCookieFeatures {
>> QEMU_MIGRATION_COOKIE_GRAPHICS = (1 <<
>> QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS),
>> QEMU_MIGRATION_COOKIE_LOCKSTATE = (1 <<
>> QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE),
>> QEMU_MIGRATION_COOKIE_PERSISTENT = (1 <<
>> QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT),
>> +QEMU_MIGRATION_COOKIE_NETWORK = (1 <<
>> QEMU_MIGRATION_COOKIE_FLAG_NETWORK),
>> };
>>
>> typedef struct _qemuMigrationCookieGraphics qemuMigrationCookieGraphics;
>> @@ -95,6 +97,35 @@ struct _qemuMigrationCookieGraphics {
>> char *tlsSubject;
>> };
>>
>> +VIR_ENUM_DECL(virInterface);
>> +VIR_ENUM_IMPL(virInterface, VIR_NETDEV_VPORT_PROFILE_LAST,
>> + "none",
>> + "802.1QBg",
>> + "802.1QBh",
>> + "openvswitch");
>
> (I'd never before considered using VIR_ENUM_*() and giving an enum type
> identifier that didn't match the enum being translated)
>
> Since you're using the enum virNetDevVPortProfile for these values
> anyway, you should just use the already-existing functions:
>
> virNetDevVPortTypeFromString
> virNetDevVPortTypeToString
>
> (these are declared/defined in virnetdevvportprofile.h, and included in
> libvirt_private.syms).
>
>
>> +
>> +typedef struct _qemuMigrationCookieNetdata qemuMigrationCookieNetdata;
>> +typedef qemuMigrationCookieNetdata *qemuMigrationCookieNetdataPtr;
>> +struct _qemuMigrationCookieNetdata {
>> +/* What type is each interace ? virInterface above ... */
>> +char *interfacetype;
>
> 1) Since the xml attribute is called "vporttype", you should give the
> variable in the struct the same name, to avoid confusion.
>
>
> 2) Internally, we usually store the enum value rather than the string,
> and translate to/from string form during XML parse/format. (For some
> reason, normal usage is to specify it in the struct as an int, with a
> comment giving the actual type:
>
>
>int vporttype; /* enum virNetDevVPortProfile */
>
>
> I'm not sure why that is, to be truthful - Eric or Dan?)
>
>
>> +
>> +/*
>> + * Array of pointers to saved data. Each VIF will have it's own
>> + * data to transfer.
>> + */
>> +char *portdata;
>> +};
>> +
>> +typedef struct _qemuMigrationCookieNetwork qemuMigrationCookieNetwork;
>> +typedef qemuMigrationCookieNetwork *qemuMigrationCookieNetworkPtr;
>> +struct _qemuMigrationCookieNetwork {
>> +/* How many virtual NICs are we saving data for? */
>> +int nnets;
>> +
>> +qemuMigrationCookieNetdataPtr *net;
>> +};
>> +
>> typedef struct _qemuMigrationCookie qemuMigrationCookie;
>> typedef qemuMigrationCookie *qemuMigrationCookiePtr;
>> struct _qemuMigrationCookie {
>> @@ -120,6 +151,9 @@ struct _qemuMigrationCookie {
>>
>> /* If (flags & QEMU_MIGRATION_COOKIE_PERSISTENT) */
>> virDomainDefPtr persistent;
>> +
>> +/* If (flags & QEMU_MIGRATION_COOKIE_NETWORK) */
>> +qemuMigrationCookieNetworkPtr network;
>> };
>>
>> static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr
>> grap)
>> @@ -132,6 +166,28 @@ static void
>> qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr grap)
>> }
>>
>>
>> +static void qemuMigrationCookieNetworkFree(qemuMigrationCookieNetworkPtr
>> + network)
>> +{
>> +int i;
>> +
>> +if (!network)
>> +return;
>> +
>> +for (i = 0; i < network->nnets; i++) {
>> +if (network->net[i]) {
>> +if (network
Re: [libvirt] [V2 PATCH 0/3] Transport Open vSwitch per-port data during live migration
On Sep 14, 2012, at 9:04 PM, Laine Stump wrote: > On 09/14/2012 03:01 PM, Kyle Mestery wrote: >> This series of commits has the end goal of allowing per-port data stored >> in the Open vSwitch DB to be transported during live migration. This is >> done by first providing a generic infrastructure for transporting network >> data, adding some utility functions specific to Open vSwitch, and hooking >> the two together. >> >> The framework provided is generic in that other networking data could be >> transferred as well by simply adding in additional hooks as needed. >> >> V2 of this patch series fixes some issues found when migrating VMs on >> standard Linux bridges. > > Oops. I just made some comments on one of the V1 patches before I > scrolled down and saw these v2 patches. It looks like the same comments > apply. > Thanks Laine, I see your comments on the V1 patch set. I'll try to get these updated, but I'm traveling next week, and will have to fit it into that schedule. Stay tuned and thanks! Kyle > This is definitely looking better - it will make it much simpler for the > next interface type that needs to send migration data to do so, and it > gets the running of ovs-vsctl commands isolated into virnetdevopenvswitch.c. > >> >> Kyle Mestery (3): >> Add the ability for the Qemu V3 migration protocol to include >>transporting network configuration. A generic framework is proposed >>with this patch to allow for the transfer of opaque data. >> Add utility functions for Open vSwitch to both save per-port data >>before a live migration, and restore the per-port data after a >>live migration. >> Transport Open vSwitch per-port data during live migration by >>using the utility functions virNetDevOpenvswitchGetMigrateData() >> and virNetDevOpenvswitchSetMigrateData(). >> >> src/libvirt_private.syms| 2 + >> src/qemu/qemu_migration.c | 291 >> +++- >> src/util/virnetdevopenvswitch.c | 71 ++ >> src/util/virnetdevopenvswitch.h | 6 + >> 4 files changed, 368 insertions(+), 2 deletions(-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Transport Open vSwitch per-port data during live migration
I found an issue with this patch when migrating VMs without OVS interfaces. I'm fixing that issue now, and will be posting V2 of this series soon. On Sep 14, 2012, at 11:38 AM, Kyle Mestery wrote: > This series of commits has the end goal of allowing per-port data stored > in the Open vSwitch DB to be transported during live migration. This is > done by first providing a generic infrastructure for transporting network > data, adding some utility functions specific to Open vSwitch, and hooking > the two together. > > The framework provided is generic in that other networking data could be > transferred as well by simply adding in additional hooks as needed. > > Kyle Mestery (3): > Add the ability for the Qemu V3 migration protocol to include >transporting network configuration. A generic framework is proposed >with this patch to allow for the transfer of opaque data. > Add utility functions for Open vSwitch to both save per-port data >before a live migration, and restore the per-port data after a >live migration. > Transport Open vSwitch per-port data during live migration by >using the utility functions virNetDevOpenvswitchGetMigrateData() > and virNetDevOpenvswitchSetMigrateData(). > > cfg.mk | 9 + > configure.ac | 36 +- > docs/formatdomain.html.in | 45 +- > docs/schemas/domaincommon.rng | 75 ++- > src/conf/domain_conf.c | 394 +++- > src/conf/domain_conf.h | 32 + > src/esx/esx_util.c | 7 +- > src/libvirt.c | 20 +- > src/libvirt_private.syms | 5 + > src/parallels/parallels_driver.c | 133 +++- > src/qemu/qemu_capabilities.c | 679 +++-- > src/qemu/qemu_capabilities.h | 49 +- > src/qemu/qemu_command.c| 660 +++- > src/qemu/qemu_command.h| 56 +- > src/qemu/qemu_domain.c | 16 +- > src/qemu/qemu_domain.h | 4 +- > src/qemu/qemu_driver.c | 50 +- > src/qemu/qemu_hotplug.c| 141 ++--- > src/qemu/qemu_migration.c | 295 - > src/qemu/qemu_monitor.c| 114 ++-- > src/qemu/qemu_monitor.h| 10 +- > src/qemu/qemu_monitor_json.c | 77 ++- > src/qemu/qemu_monitor_json.h | 5 +- > src/qemu/qemu_monitor_text.c | 335 +++--- > src/qemu/qemu_process.c| 62 +- > src/rpc/virnetserverprogram.c | 11 +- > src/rpc/virnettlscontext.c | 6 +- > src/util/bitmap.c | 19 + > src/util/bitmap.h | 6 + > src/util/virnetdevopenvswitch.c| 71 +++ > src/util/virnetdevopenvswitch.h| 6 + > src/vbox/vbox_tmpl.c | 14 +- > src/vmware/vmware_driver.c | 4 +- > tests/Makefile.am | 10 +- > .../domain-parallels-ct-simple.xml | 27 + > tests/qemuhelptest.c | 16 +- > tests/qemumonitortestutils.c | 12 +- > .../qemuxml2argv-cpu-eoi-disabled.args | 4 + > .../qemuxml2argv-cpu-eoi-disabled.xml | 28 + > .../qemuxml2argv-cpu-eoi-enabled.args | 4 + > .../qemuxml2argv-cpu-eoi-enabled.xml | 28 + > .../qemuxml2argv-eoi-disabled.args | 4 + > .../qemuxml2argvdata/qemuxml2argv-eoi-disabled.xml | 25 + > .../qemuxml2argvdata/qemuxml2argv-eoi-enabled.args | 4 + > .../qemuxml2argvdata/qemuxml2argv-eoi-enabled.xml | 25 + > .../qemuxml2argv-usb-redir-filter.args | 10 + > .../qemuxml2argv-usb-redir-filter.xml | 45 ++ > tests/qemuxml2argvtest.c | 23 +- > tests/qemuxml2xmltest.c| 6 + > tests/qemuxmlnstest.c | 6 +- > tests/undefine | 72 --- > tests/virsh-undefine | 72 +++ > tools/virsh-domain.c | 4 +- > tools/virsh-network.c | 8 +- > tools/virsh-pool.c | 8 +- > tools/virsh-volume.c | 8 +- > 56 files changed, 2678 insertions(+), 1217 deletions(-) > create mode 100644 tests/domainschemadata/domain-parallels-ct-simple.xml > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-eoi-disabled.args
Re: [libvirt] [PATCH 1/1 V2] Migrate per-port data for Open vSwitch ports during Qemu Live Migration
On Sep 13, 2012, at 3:09 PM, Laine Stump wrote: > On 09/12/2012 11:08 PM, Kyle Mestery (kmestery) wrote: >> Hi Laine: >> >> I'm in the process of reworking this patch along the lines you and Daniel >> have >> provided input towards. I defined some helper functions in >> virnetdevopenvswitch.c, >> but when calling them from qemu_migration.c, the build is failing during >> linking. >> I suspect I need to add whatever gets built out of src/util to the qemu >> stuff, but before >> going down that path, wanted to run this by the list. Here is the error I >> see: >> >> make[3]: Leaving directory `/production/git/local/libvirt/libvirt/python' >> Making all in tests >> make[3]: Entering directory >> `/production/git/local/libvirt/libvirt/python/tests' >> make[3]: Nothing to be done for `all'. >> make[3]: Leaving directory >> `/production/git/local/libvirt/libvirt/python/tests' >> make[2]: Leaving directory `/production/git/local/libvirt/libvirt/python' >> Making all in tests >> make[2]: Entering directory `/production/git/local/libvirt/libvirt/tests' >> CCLD qemuxml2argvtest >> ../src/.libs/libvirt_driver_qemu_impl.a(libvirt_driver_qemu_impl_la-qemu_migration.o): >> In function `qemuMigrationCookieNetworkAlloc': >> /production/git/local/libvirt/libvirt/src/qemu/qemu_migration.c:343: >> undefined reference to `virNetDevOpenvswitchGetMigrateData' >> ../src/.libs/libvirt_driver_qemu_impl.a(libvirt_driver_qemu_impl_la-qemu_migration.o): >> In function `qemuDomainMigrateOPDRelocate': >> /production/git/local/libvirt/libvirt/src/qemu/qemu_migration.c:1302: >> undefined reference to `virNetDevOpenvswitchSetMigrateData' > > Usually this means that you haven't added the new utility functions to > src/libvirt_private.syms. Find the section for the source file the new > function was added to, then add in the name of your new function with a > semicolon at the end of the line. Be sure to keep the names in > alphabetic order (within each file) lest you incur the wrath of Eric! :-) > Awesome, that was the issue, thanks for the help Laine! Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1 V2] Migrate per-port data for Open vSwitch ports during Qemu Live Migration
On Sep 6, 2012, at 10:58 AM, Laine Stump wrote:
> On 09/04/2012 04:35 PM, Kyle Mestery wrote:
>> Add the ability to migrate per-port data on Open vSwitch
>> ports during qemu live migration. A controller can use this
>> to store data relating to each port, and have it migrated
>> with the virtual machine and populated on the destination
>> host.
>
> Good job in figuring this out!
>
>>
>> Signed-off-by: Kyle Mestery
>> Cc: Laine Stump
>> ---
>> src/qemu/qemu_migration.c | 246
>> +-
>> 1 file changed, 244 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 1b21ef6..8c1a8f1 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -70,6 +70,7 @@ enum qemuMigrationCookieFlags {
>> QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS,
>> QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE,
>> QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT,
>> +QEMU_MIGRATION_COOKIE_FLAG_OVS_PORT_DATA,
>>
>> QEMU_MIGRATION_COOKIE_FLAG_LAST
>> };
>> @@ -77,12 +78,13 @@ enum qemuMigrationCookieFlags {
>> VIR_ENUM_DECL(qemuMigrationCookieFlag);
>> VIR_ENUM_IMPL(qemuMigrationCookieFlag,
>> QEMU_MIGRATION_COOKIE_FLAG_LAST,
>> - "graphics", "lockstate", "persistent");
>> + "graphics", "lockstate", "persistent", "ovsportdata");
>>
>> enum qemuMigrationCookieFeatures {
>> QEMU_MIGRATION_COOKIE_GRAPHICS = (1 <<
>> QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS),
>> QEMU_MIGRATION_COOKIE_LOCKSTATE = (1 <<
>> QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE),
>> QEMU_MIGRATION_COOKIE_PERSISTENT = (1 <<
>> QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT),
>> +QEMU_MIGRATION_COOKIE_OVS_PORT_DATA = (1 <<
>> QEMU_MIGRATION_COOKIE_FLAG_OVS_PORT_DATA),
>> };
>>
>> typedef struct _qemuMigrationCookieGraphics qemuMigrationCookieGraphics;
>> @@ -95,6 +97,19 @@ struct _qemuMigrationCookieGraphics {
>> char *tlsSubject;
>> };
>>
>> +typedef struct _qemuMigrationCookieOvsPortData
>> qemuMigrationCookieOvsPortData;
>> +typedef qemuMigrationCookieOvsPortData *qemuMigrationCookieOvsPortDataPtr;
>> +struct _qemuMigrationCookieOvsPortData {
>> +/* How many virtual NICs are we saving data for? */
>> +int nnets;
>> +
>> +/*
>> + * Array of pointers to saved data. Each VIF will have it's own
>> + * data to transfer.
>> + */
>> +char **portdata;
>> +};
>> +
>> typedef struct _qemuMigrationCookie qemuMigrationCookie;
>> typedef qemuMigrationCookie *qemuMigrationCookiePtr;
>> struct _qemuMigrationCookie {
>> @@ -120,6 +135,9 @@ struct _qemuMigrationCookie {
>>
>> /* If (flags & QEMU_MIGRATION_COOKIE_PERSISTENT) */
>> virDomainDefPtr persistent;
>> +
>> +/* If (flags & QEMU_MIGRATION_COOKIE_OVS_PORT_DATA) */
>> +qemuMigrationCookieOvsPortDataPtr ovsportdata;
>> };
>>
>> static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr
>> grap)
>> @@ -132,6 +150,24 @@ static void
>> qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr grap)
>> }
>>
>>
>> +static void
>> qemuMigrationCookieOvsPortDataFree(qemuMigrationCookieOvsPortDataPtr
>> + ovsportdata)
>> +{
>> +int i;
>> +
>> +if (!ovsportdata)
>> +return;
>> +
>> +for (i = 0; i < ovsportdata->nnets; i++) {
>> +VIR_FREE(ovsportdata->portdata[i]);
>> +}
>> +
>> +VIR_FREE(ovsportdata->portdata);
>> +
>> +VIR_FREE(ovsportdata);
>> +}
>> +
>> +
>> static void qemuMigrationCookieFree(qemuMigrationCookiePtr mig)
>> {
>> if (!mig)
>> @@ -140,6 +176,10 @@ static void
>> qemuMigrationCookieFree(qemuMigrationCookiePtr mig)
>> if (mig->flags & QEMU_MIGRATION_COOKIE_GRAPHICS)
>> qemuMigrationCookieGraphicsFree(mig->graphics);
>>
>> +if (mig->flags & QEMU_MIGRATION_COOKIE_OVS_PORT_DATA) {
>> +qemuMigrationCookieOvsPortDataFree(mig->ovsportdata);
>> +}
>> +
>> VIR_FREE(mig->localHostname);
>> VIR_FREE(mig->remoteHostname);
>> VIR_FREE(mig->name);
>> @@ -256,6 +296,60 @@ error:
>> }
>>
>>
>> +static qemuMigrationCookieOvsPortDataPtr
>> +qemuMigrationCookieOvsPortDataAlloc(struct qemud_driver *driver
>> ATTRIBUTE_UNUSED,
>> +virDomainDefPtr def)
>> +{
>> +qemuMigrationCookieOvsPortDataPtr mig;
>> +int i;
>> +virCommandPtr cmd = NULL;
>> +virDomainNetDefPtr netptr;
>> +
>> +if (VIR_ALLOC(mig) < 0)
>> +goto no_memory;
>> +
>> +mig->nnets = def->nnets;
>> +
>> +if (VIR_ALLOC_N(mig->portdata, def->nnets) < 0)
>> +goto no_memory;
>> +
>> +for (i = 0; i < def->nnets; i++) {
>> +virNetDevVPortProfilePtr vport =
>> virDomainNetGetActualVirtPortProfile(def->nets[i]);
>> +netptr = def->nets[i];
>> +
>> +if (vport && vport->virtPortType ==
>> VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {
>> +if (VIR_ALLOC(mig->portdata[i]) < 0)
>> +goto no_memory;
>> +
>> +
Re: [libvirt] [PATCH 1/1 V2] Migrate per-port data for Open vSwitch ports during Qemu Live Migration
On Sep 6, 2012, at 12:35 AM, Daniel Veillard wrote:
> On Tue, Sep 04, 2012 at 04:35:24PM -0400, Kyle Mestery wrote:
>> Add the ability to migrate per-port data on Open vSwitch
>> ports during qemu live migration. A controller can use this
>> to store data relating to each port, and have it migrated
>> with the virtual machine and populated on the destination
>> host.
>>
>> Signed-off-by: Kyle Mestery
>> Cc: Laine Stump
> []
>> +static void qemuMigrationCookieOvsPortDataXMLFormat(virBufferPtr buf,
>> +
>> qemuMigrationCookieOvsPortDataPtr optr)
>> +{
>> +int i;
>> +
>> +virBufferAsprintf(buf, " \n", optr->nnets);
>> +if (optr->nnets > 0)
>> +virBufferAsprintf(buf, "\n");
>> +for (i = 0; i < optr->nnets; i++) {
>> +virBufferAsprintf(buf, " \n",
>> + i, optr->portdata[i]);
>> +}
>> +if (optr->nnets > 0)
>> +virBufferAsprintf(buf, "\n");
>> +
>> +virBufferAddLit(buf, " \n");
>> +}
>
> I'm not specialist of the networking layer, but it looks to me that
> this should work (but can someone explain the error scenario if the
> receiving end don't understand the extra XML passed in the migration
> data ?). My main concern is tying that generic mechanism to OVS, seems
> to me this should be split in 2 patches:
> - one patch adding the generic framework to adding XML networking
> related data to the XML passed along the domain in the migration
> - one patch using that generic mechanism to carry the OpenVSwitch
> per port data
> The first part defining the data extension etc ought to be generic
> (but probably network related) and the second part specific to OVS
>
> Laine, am I off track ?
>
> Daniel
Thanks for the review Daniel. Laine and I spoke offline, and he had similar
comments to you. I will rework the patch as you describe above, and incorporate
any additional comments Laine has as well.
Thanks!
Kyle
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: prevent infinite hang if ovs-vswitchd isn't running
On Sep 5, 2012, at 1:24 PM, Laine Stump wrote: > This fixes https://bugzilla.redhat.com/show_bug.cgi?id=852984 > > If a network or interface is configured to use Open vSwitch, but > ovs-vswitchd (the Open vSwitch database service) isn't running, the > ovs-vsctl add-port/del-port commands will hang indefinitely rather > than returning an error. There is a --nowait option, but that appears > to have no effect on add-port and del-port commands, so instead we add > a --timeout=5 to the commands - they will retry for up to 5 seconds, > then fail if there is no response. This looks good. I agree with your comments here around the 5 second timeout not being ideal, but being workable. Acked-by: Kyle Mestery -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix adding ports to OVS bridges without VLAN tags
On Aug 31, 2012, at 9:09 AM, Daniel Veillard wrote: > On Fri, Aug 31, 2012 at 01:32:34PM +0000, Kyle Mestery (kmestery) wrote: >> On Aug 30, 2012, at 10:23 PM, Daniel Veillard wrote: >>> On Thu, Aug 30, 2012 at 04:38:06PM -0400, Kyle Mestery wrote: > [...] >>> Still there is something which looks wrong, if we don't have a profileID >>> why do we end up with "" instead of NULL ? I'm seeing various tests for >>> profileID[0] over conf/*.c and util/*.c , and that sounds wrong to me. >>> if there is no data, store NULL ! Then test for profileID instead of >>> profileID[0]. Then there is no risk of a crash because abscence of data >>> led to NULL instead of an empty string, the code is more resilient ! >>> >>> I expect a followup patch cleaning this up, but after 0.10.1 ... >>> thanks ! >>> >> Thanks Daniel, I'll work on the followup patch today. > > No hurry, because I just rolled 0.10.1 out so that won't make it > (and it's not urgent). Giving 0.10.1 a try would be nice too, > > thanks ! > > Daniel Hi Daniel: Picking this back up. struct _virNetDevVPortProfile contains the following: /* this member is used when virtPortType == 802.1Qbh|openvswitch */ /* this is a null-terminated character string */ char profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX]; To address your comments around checking for profileID[0], we could make profileID here a pointer, and allocate it when we allocate a struct _virNetDevVPortProfile object. But to me, having a fixed length string in this structure doesn't seem wrong. Copying Laine here as well for his comments, but I'm inclined to leave things as they are. Thanks, Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix adding ports to OVS bridges without VLAN tags
On Aug 30, 2012, at 10:23 PM, Daniel Veillard wrote:
> On Thu, Aug 30, 2012 at 04:38:06PM -0400, Kyle Mestery wrote:
>> The introduction of the new VLAN code, along with the fix
>> from 5e465df6be8bcb00f0b4bff831e91f4042fae272, caused the
>> addition of OVS ports to fail with the following message:
>>
>> ovs-vsctl: 2|vsctl|ERR|: missing column name
>>
>> This fix takes into account the VLAN arguments are optional,
>> and correctly sets up the command line to run the "ovs-vsctl"
>> command to add ports to the OVS bridge.
>>
>> Signed-off-by: Kyle Mestery
>> CC: Eric Blake
>> ---
>> V2:
>> - Use virBufferUse() to check if a buffer is in use. Found
>>by Eric Blake.
>> ---
>> src/util/virnetdevopenvswitch.c | 13 +
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/util/virnetdevopenvswitch.c
>> b/src/util/virnetdevopenvswitch.c
>> index 00271a0..764f478 100644
>> --- a/src/util/virnetdevopenvswitch.c
>> +++ b/src/util/virnetdevopenvswitch.c
>> @@ -104,9 +104,15 @@ int virNetDevOpenvswitchAddPort(const char *brname,
>> const char *ifname,
>> }
>>
>> cmd = virCommandNew(OVSVSCTL);
>> +
>> +virCommandAddArgList(cmd, "--", "--may-exist", "add-port",
>> +brname, ifname, NULL);
>> +
>> +if (virBufferUse(&buf) != 0)
>> +virCommandAddArgList(cmd, virBufferCurrentContent(&buf), NULL);
>> +
>> if (ovsport->profileID[0] == '\0') {
>> -virCommandAddArgList(cmd, "--", "--may-exist", "add-port",
>> -brname, ifname, virBufferCurrentContent(&buf),
>> +virCommandAddArgList(cmd,
>> "--", "set", "Interface", ifname, attachedmac_ex_id,
>> "--", "set", "Interface", ifname, ifaceid_ex_id,
>> "--", "set", "Interface", ifname, vmid_ex_id,
>> @@ -114,8 +120,7 @@ int virNetDevOpenvswitchAddPort(const char *brname,
>> const char *ifname,
>> "external-ids:iface-status=active",
>> NULL);
>> } else {
>> -virCommandAddArgList(cmd, "--", "--may-exist", "add-port",
>> -brname, ifname, virBufferCurrentContent(&buf),
>> +virCommandAddArgList(cmd,
>> "--", "set", "Interface", ifname, attachedmac_ex_id,
>> "--", "set", "Interface", ifname, ifaceid_ex_id,
>> "--", "set", "Interface", ifname, vmid_ex_id,
>
> Okay, ACK,
>
> pushed, thanks !
>
> Still there is something which looks wrong, if we don't have a profileID
> why do we end up with "" instead of NULL ? I'm seeing various tests for
> profileID[0] over conf/*.c and util/*.c , and that sounds wrong to me.
> if there is no data, store NULL ! Then test for profileID instead of
> profileID[0]. Then there is no risk of a crash because abscence of data
> led to NULL instead of an empty string, the code is more resilient !
>
> I expect a followup patch cleaning this up, but after 0.10.1 ...
> thanks !
>
Thanks Daniel, I'll work on the followup patch today.
Kyle
> Daniel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Fix adding ports to OVS bridges without VLAN tags
On Aug 30, 2012, at 3:31 PM, Eric Blake wrote: > On 08/30/2012 11:53 AM, Kyle Mestery wrote: >> The introduction of the new VLAN code, along with the fix >> from 5e465df6be8bcb00f0b4bff831e91f4042fae272, caused the >> addition of OVS ports to fail with the following message: >> >> ovs-vsctl: 2|vsctl|ERR|: missing column name >> >> This fix takes into account the VLAN arguments are optional, >> and correctly sets up the command line to run the "ovs-vsctl" >> command to add ports to the OVS bridge. >> >> Signed-off-by: Kyle Mestery >> --- >> src/util/virnetdevopenvswitch.c | 13 + >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/src/util/virnetdevopenvswitch.c >> b/src/util/virnetdevopenvswitch.c >> index 00271a0..fcf6d91 100644 >> --- a/src/util/virnetdevopenvswitch.c >> +++ b/src/util/virnetdevopenvswitch.c >> @@ -104,9 +104,15 @@ int virNetDevOpenvswitchAddPort(const char *brname, >> const char *ifname, >> } >> > >> +if (virBufferCurrentContent(&buf) != "") > > Won't work. The address of "" is not constant. Instead, you want to > probe virBufferUse() for being non-zero. > Thanks Eric, I'll generate a new patch now and send it out. Kyle > -- > Eric Blake ebl...@redhat.com+1-919-301-3266 > Libvirt virtualization library http://libvirt.org > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Fix adding ports to OVS bridges without VLAN tags
This fix is critical for the release tomorrow (0.10.1 I believe), as without
it, adding
ports to OVS bridges without a VLAN setup will fail.
Thanks,
Kyle
On Aug 30, 2012, at 1:53 PM, Kyle Mestery wrote:
> The introduction of the new VLAN code, along with the fix
> from 5e465df6be8bcb00f0b4bff831e91f4042fae272, caused the
> addition of OVS ports to fail with the following message:
>
> ovs-vsctl: 2|vsctl|ERR|: missing column name
>
> This fix takes into account the VLAN arguments are optional,
> and correctly sets up the command line to run the "ovs-vsctl"
> command to add ports to the OVS bridge.
>
> Signed-off-by: Kyle Mestery
> ---
> src/util/virnetdevopenvswitch.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index 00271a0..fcf6d91 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -104,9 +104,15 @@ int virNetDevOpenvswitchAddPort(const char *brname,
> const char *ifname,
> }
>
> cmd = virCommandNew(OVSVSCTL);
> +
> +virCommandAddArgList(cmd, "--", "--may-exist", "add-port",
> +brname, ifname, NULL);
> +
> +if (virBufferCurrentContent(&buf) != "")
> +virCommandAddArgList(cmd, virBufferCurrentContent(&buf), NULL);
> +
> if (ovsport->profileID[0] == '\0') {
> -virCommandAddArgList(cmd, "--", "--may-exist", "add-port",
> -brname, ifname, virBufferCurrentContent(&buf),
> +virCommandAddArgList(cmd,
> "--", "set", "Interface", ifname, attachedmac_ex_id,
> "--", "set", "Interface", ifname, ifaceid_ex_id,
> "--", "set", "Interface", ifname, vmid_ex_id,
> @@ -114,8 +120,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const
> char *ifname,
> "external-ids:iface-status=active",
> NULL);
> } else {
> -virCommandAddArgList(cmd, "--", "--may-exist", "add-port",
> -brname, ifname, virBufferCurrentContent(&buf),
> +virCommandAddArgList(cmd,
> "--", "set", "Interface", ifname, attachedmac_ex_id,
> "--", "set", "Interface", ifname, ifaceid_ex_id,
> "--", "set", "Interface", ifname, vmid_ex_id,
> --
> 1.7.11.4
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix a crash when using Open vSwitch virtual ports
On Aug 30, 2012, at 3:52 AM, Laine Stump wrote:
> On 08/30/2012 01:32 AM, Eric Blake wrote:
>> [...]
>> That is, I think the real patch here would be simply this (untested)
>> version:
>>
>> diff --git i/src/util/virnetdevopenvswitch.c
>> w/src/util/virnetdevopenvswitch.c
>> index b903ae4..e8b9f4a 100644
>> --- i/src/util/virnetdevopenvswitch.c
>> +++ w/src/util/virnetdevopenvswitch.c
>> @@ -59,7 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname,
>> const char *ifname,
>> char *ifaceid_ex_id = NULL;
>> char *profile_ex_id = NULL;
>> char *vmid_ex_id = NULL;
>> -virBufferPtr buf;
>> +virBufferPtr buf = NULL;
>>
>> virMacAddrFormat(macaddr, macaddrstr);
>> virUUIDFormat(ovsport->interfaceID, ifuuidstr);
>> @@ -79,7 +79,7 @@ int virNetDevOpenvswitchAddPort(const char *brname,
>> const char *ifname,
>> ovsport->profileID) < 0)
>> goto out_of_memory;
>> }
>> -if (virtVlan) {
>> +if (virtVlan && virtVlan->tag[0]) {
>
> If you were to believe that virtVlan might be sent with no tags (which
> isn't the case with my patch applied...), then checking for non-0
> virtVLan->tag[0] without checking for non-NULL virtVlan->tag would
> itself lead to a crash.
Just saw all the cleanups to this patch and the fact it was pushed. Thanks for
all the
comments and cleanups to it Erik and Laine! And thanks for pushing it up.
Kyle
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: get vlan info for Open vSwitch interfaces from proper source
On Aug 29, 2012, at 9:49 AM, Kyle Mestery (kmestery) wrote:
> On Aug 29, 2012, at 6:19 AM, Alex Jia wrote:
>> On 08/29/2012 06:43 PM, Laine Stump wrote:
>>> This bug was revealed by the crash described in
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=852383
>>>
>>> The vlan info pointer sent to virNetDevOpenvswitchAddPort should never
>>> be non-NULL unless there is at least one tag. The factthat such a vlan
>>> info pointer was receveid pointed out that a caller was passing the
>>> wrong pointer. Instead of sending&net->vlan, the result of
>>> virDomainNetGetActualVlan(net) should be sent - that function will
>>> look for vlan info in net->data.network.actual->vlan, and in cany case
>>> return NULL instead of a pointer if the vlan info it finds has no
>>> tags.
>>>
>>> Aside from causing the crash, sending a hardcoded&net->vlan has the
>>> effect of ignoring vlan info from a or config.
>>> ---
>>>
>>> Since I'm not online in a regular fashion for the next few days (too
>>> bad I wasn't online in the 12 hours or so *before* the 0.10.0 release
>>> instead of after :-/), I would appreciate if whoever ACKs this could
>>> push it. Thanks!
>>
>> Laine, unfortunately, the libvirtd still is crash without my patch after
>> applying your patch :(
>>
> Hi Alex and Laine:
>
> What I notice is, that with Laine's patch, for virtualport of type
> openvswitch, if I do NOT define
> a VLAN, it crashes. If I do, then I can start my virtual machines. Without
> Laine's patch, it's the
> same, which leads me to believe Laine's patch only fixes a symptom of this
> issue.
>
> I'm looking into this now, will see how far I get with it today. But it
> appears to me to be a bug
> in the logic which is handling VLANs at a higher layer.
>
> Thanks,
> Kyle
>
I just sent a patch, which combined with Laine's patches, stop the crashes in
the latest libvirt code
when using Open vSwitch virtual ports. I think this should go into the 0.10.1
release for this
Friday, because without it libvirt crashes consistently with Open vSwitch
virtualports.
Thanks,
Kyle
>>> src/qemu/qemu_command.c | 2 +-
>>> src/uml/uml_conf.c | 2 +-
>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 8c32a4d..25f2451 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -258,7 +258,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
>>> err = virNetDevTapCreateInBridgePort(brname,&net->ifname,&net->mac,
>>> def->uuid,&tapfd,
>>>
>>> virDomainNetGetActualVirtPortProfile(net),
>>> -&net->vlan,
>>> + virDomainNetGetActualVlan(net),
>>> tap_create_flags);
>>> virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd>= 0);
>>> if (err< 0) {
>>> diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
>>> index 5461b42..410f3e2 100644
>>> --- a/src/uml/uml_conf.c
>>> +++ b/src/uml/uml_conf.c
>>> @@ -141,7 +141,7 @@ umlConnectTapDevice(virConnectPtr conn,
>>> if (virNetDevTapCreateInBridgePort(bridge,&net->ifname,&net->mac,
>>>vm->uuid, NULL,
>>>
>>> virDomainNetGetActualVirtPortProfile(net),
>>> -&net->vlan,
>>> + virDomainNetGetActualVlan(net),
>>>VIR_NETDEV_TAP_CREATE_IFUP)< 0) {
>>> if (template_ifname)
>>> VIR_FREE(net->ifname);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: get vlan info for Open vSwitch interfaces from proper source
On Aug 29, 2012, at 6:19 AM, Alex Jia wrote:
> On 08/29/2012 06:43 PM, Laine Stump wrote:
>> This bug was revealed by the crash described in
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=852383
>>
>> The vlan info pointer sent to virNetDevOpenvswitchAddPort should never
>> be non-NULL unless there is at least one tag. The factthat such a vlan
>> info pointer was receveid pointed out that a caller was passing the
>> wrong pointer. Instead of sending&net->vlan, the result of
>> virDomainNetGetActualVlan(net) should be sent - that function will
>> look for vlan info in net->data.network.actual->vlan, and in cany case
>> return NULL instead of a pointer if the vlan info it finds has no
>> tags.
>>
>> Aside from causing the crash, sending a hardcoded&net->vlan has the
>> effect of ignoring vlan info from a or config.
>> ---
>>
>> Since I'm not online in a regular fashion for the next few days (too
>> bad I wasn't online in the 12 hours or so *before* the 0.10.0 release
>> instead of after :-/), I would appreciate if whoever ACKs this could
>> push it. Thanks!
>
> Laine, unfortunately, the libvirtd still is crash without my patch after
> applying your patch :(
>
Hi Alex and Laine:
What I notice is, that with Laine's patch, for virtualport of type openvswitch,
if I do NOT define
a VLAN, it crashes. If I do, then I can start my virtual machines. Without
Laine's patch, it's the
same, which leads me to believe Laine's patch only fixes a symptom of this
issue.
I'm looking into this now, will see how far I get with it today. But it appears
to me to be a bug
in the logic which is handling VLANs at a higher layer.
Thanks,
Kyle
>> src/qemu/qemu_command.c | 2 +-
>> src/uml/uml_conf.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 8c32a4d..25f2451 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -258,7 +258,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
>> err = virNetDevTapCreateInBridgePort(brname,&net->ifname,&net->mac,
>> def->uuid,&tapfd,
>>
>> virDomainNetGetActualVirtPortProfile(net),
>> -&net->vlan,
>> + virDomainNetGetActualVlan(net),
>> tap_create_flags);
>> virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd>= 0);
>> if (err< 0) {
>> diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
>> index 5461b42..410f3e2 100644
>> --- a/src/uml/uml_conf.c
>> +++ b/src/uml/uml_conf.c
>> @@ -141,7 +141,7 @@ umlConnectTapDevice(virConnectPtr conn,
>> if (virNetDevTapCreateInBridgePort(bridge,&net->ifname,&net->mac,
>> vm->uuid, NULL,
>>
>> virDomainNetGetActualVirtPortProfile(net),
>> -&net->vlan,
>> + virDomainNetGetActualVlan(net),
>> VIR_NETDEV_TAP_CREATE_IFUP)< 0) {
>> if (template_ifname)
>> VIR_FREE(net->ifname);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Add support for setting VLANs on Open vSwitch ports
On Aug 17, 2012, at 1:18 PM, Laine Stump wrote: > On 08/17/2012 12:04 AM, Kyle Mestery wrote: >> Add the ability to support VLAN tags for Open vSwitch >> virtual port types. To accomplish this, modify >> virNetDevOpenvswitchAddPort and >> virNetDevTapCreateInBridgePort to take a virNetDevVlanPtr >> argument. When adding the port to the OVS bridge, setup >> either a single VLAN or a trunk port based on the >> configuration from the virNetDevVlanPtr. >> >> Signed-off-by: Kyle Mestery >> --- >> .gnulib | 2 +- >> src/lxc/lxc_process.c | 2 +- >> src/network/bridge_driver.c | 2 +- >> src/qemu/qemu_command.c | 1 + >> src/uml/uml_conf.c | 1 + >> src/util/virnetdevopenvswitch.c | 34 +++--- >> src/util/virnetdevopenvswitch.h | 4 +++- >> src/util/virnetdevtap.c | 3 ++- >> src/util/virnetdevtap.h | 2 ++ >> 9 files changed, 43 insertions(+), 8 deletions(-) >> >> diff --git a/.gnulib b/.gnulib >> index 271dd74..dbd9144 16 >> --- a/.gnulib >> +++ b/.gnulib >> @@ -1 +1 @@ >> -Subproject commit 271dd74fdf54ec2a03e73a5173b0b5697f6088f1 >> +Subproject commit dbd914496c99c52220e5f5ba4121d6cb55fb3beb > > BTW, I meant to point out that somehow you had gotten a gnulib submodule > update added onto your patch. I removed that before I pushed it. Actually, I noticed this too. For some reason, when I build RPMs from the libvirt git repo, this always happens. I assume this is not to be expected, but it's unclear to me how it's happening. Thanks for pulling that piece out before committing! Thanks, Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Add support for setting VLANs on Open vSwitch ports
On Aug 17, 2012, at 10:12 AM, Laine Stump wrote:
> On 08/17/2012 12:04 AM, Kyle Mestery wrote:
>> Add the ability to support VLAN tags for Open vSwitch
>> virtual port types. To accomplish this, modify
>> virNetDevOpenvswitchAddPort and
>> virNetDevTapCreateInBridgePort to take a virNetDevVlanPtr
>> argument. When adding the port to the OVS bridge, setup
>> either a single VLAN or a trunk port based on the
>> configuration from the virNetDevVlanPtr.
>>
>> Signed-off-by: Kyle Mestery
>> ---
>> .gnulib | 2 +-
>> src/lxc/lxc_process.c | 2 +-
>> src/network/bridge_driver.c | 2 +-
>> src/qemu/qemu_command.c | 1 +
>> src/uml/uml_conf.c | 1 +
>> src/util/virnetdevopenvswitch.c | 34 +++---
>> src/util/virnetdevopenvswitch.h | 4 +++-
>> src/util/virnetdevtap.c | 3 ++-
>> src/util/virnetdevtap.h | 2 ++
>> 9 files changed, 43 insertions(+), 8 deletions(-)
>>
>> diff --git a/.gnulib b/.gnulib
>> index 271dd74..dbd9144 16
>> --- a/.gnulib
>> +++ b/.gnulib
>> @@ -1 +1 @@
>> -Subproject commit 271dd74fdf54ec2a03e73a5173b0b5697f6088f1
>> +Subproject commit dbd914496c99c52220e5f5ba4121d6cb55fb3beb
>> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
>> index 046ed86..dc34bef 100644
>> --- a/src/lxc/lxc_process.c
>> +++ b/src/lxc/lxc_process.c
>> @@ -325,7 +325,7 @@ static int
>> virLXCProcessSetupInterfaceBridged(virConnectPtr conn,
>>
>> if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
>> ret = virNetDevOpenvswitchAddPort(brname, parentVeth, &net->mac,
>> - vm->uuid, vport);
>> + vm->uuid, vport, &net->vlan);
>> else
>> ret = virNetDevBridgeAddPort(brname, parentVeth);
>> if (ret < 0)
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index 474bbfa..a78e3b6 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -1763,7 +1763,7 @@ networkStartNetworkVirtual(struct network_driver
>> *driver,
>> }
>> if (virNetDevTapCreateInBridgePort(network->def->bridge,
>>&macTapIfName, &network->def->mac,
>> - NULL, NULL, NULL,
>> + NULL, NULL, NULL, NULL,
>>
>> VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE) < 0) {
>> VIR_FREE(macTapIfName);
>> goto err0;
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 9383530..e0062a1 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -255,6 +255,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
>> err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac,
>> def->uuid, &tapfd,
>>
>> virDomainNetGetActualVirtPortProfile(net),
>> + &net->vlan,
>> tap_create_flags);
>> virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0);
>> if (err < 0) {
>> diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
>> index 4c299d8..5461b42 100644
>> --- a/src/uml/uml_conf.c
>> +++ b/src/uml/uml_conf.c
>> @@ -141,6 +141,7 @@ umlConnectTapDevice(virConnectPtr conn,
>> if (virNetDevTapCreateInBridgePort(bridge, &net->ifname, &net->mac,
>>vm->uuid, NULL,
>>
>> virDomainNetGetActualVirtPortProfile(net),
>> + &net->vlan,
>>VIR_NETDEV_TAP_CREATE_IFUP) < 0) {
>> if (template_ifname)
>> VIR_FREE(net->ifname);
>> diff --git a/src/util/virnetdevopenvswitch.c
>> b/src/util/virnetdevopenvswitch.c
>> index b57532b..8a31e77 100644
>> --- a/src/util/virnetdevopenvswitch.c
>> +++ b/src/util/virnetdevopenvswitch.c
>> @@ -46,9 +46,11 @@
>> int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
>>const virMacAddrPtr macaddr,
>>const unsigned char *vmuuid,
>> - virNetDevVPortProfilePtr ovsport)
>> + virNetDevVPortProfilePtr ovsport,
>> + virNetDevVlanPtr virtVlan)
>> {
>> int ret = -1;
>> +int i = 0;
>> virCommandPtr cmd = NULL;
>> char macaddrstr[VIR_MAC_STRING_BUFLEN];
>> char ifuuidstr[VIR_UUID_STRING_BUFLEN];
>> @@ -57,6 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const
>> char *ifname,
>> char *ifaceid_ex_id = NULL;
>> char *profile_ex_id = NULL;
>> char *vmid_ex_id = NULL;
>> +virBufferPtr buf;
>>
>> virMacAddrFormat(macaddr, macaddrstr);
>> vir
Re: [libvirt] [PATCHv2 0/4] support element for interfaces and networks
On Aug 15, 2012, at 12:47 PM, Laine Stump wrote: > On 08/14/2012 03:15 AM, Laine Stump wrote: >> The purpose of these patches is to define a data type that can >> properly describe an interface's vlan configuration (including >> multiple vlans for trunking), provide XML parser and formatter to >> translate to and from the new data type, add the new element to >> both domain and to in appropriate places, and to >> provide the new vlan data to the hypervisor driver. >> > > Thanks to everyone for their reviews and comments. I've pushed these 4 > patches, so now Kyle can create a (hopefully much simplified) patch that > looks for virDomainNetGetActualVlan(netdef) and setups up the vlan > appropriately for guests connecting to an Open vSwtich network. > > I also have a (very small, because almost everything was already there) > patch to support setting the vlan tag on SR-IOV PCI passthrough devices. > I still have to get a successful build on my SR-IOV-capable hardware so > I can test it, though :-) > Thanks for your work on these patches Laine! I'll go ahead and reformat my patch to add VLAN support to Open vSwitch virtual port types and resubmit. Thanks, Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 5/5] network: add connections counter to networks
Looks good to me.
Acked-by: Kyle Mestery
On Aug 14, 2012, at 2:10 AM, Laine Stump wrote:
> Just as each physical device used by a network has a connections
> counter, now each network has a connections counter which is
> incremented once for each guest interface that connects using this
> network.
>
> The count is output in the live network XML, like this:
>
>
> ...
>
>
> It is read-only, and for informational purposes only - it isn't used
> internally anywhere by libvirt.
> ---
> src/conf/network_conf.c | 6 +-
> src/conf/network_conf.h | 1 +
> src/network/bridge_driver.c | 10 ++
> 3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index ca5b759..45f8e69 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1463,7 +1463,11 @@ char *virNetworkDefFormat(const virNetworkDefPtr def,
> unsigned int flags)
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> int ii;
>
> -virBufferAddLit(&buf, "\n");
> +virBufferAddLit(&buf, " +if (!(flags & VIR_NETWORK_XML_INACTIVE) && (def->connections > 0)) {
> +virBufferAsprintf(&buf, " connections='%d'", def->connections);
> +}
> +virBufferAddLit(&buf, ">\n");
> virBufferEscapeString(&buf, " %s\n", def->name);
>
> uuid = def->uuid;
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 23f1632..32fc765 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -156,6 +156,7 @@ struct _virNetworkDef {
> unsigned char uuid[VIR_UUID_BUFLEN];
> bool uuid_specified;
> char *name;
> +int connections; /* # of guest interfaces connected to this network */
>
> char *bridge; /* Name of bridge device */
> char *domain;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 680b3f3..b784eb4 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -3005,6 +3005,10 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> VIR_DEBUG("Using physical device %s, %d connections",
> dev->dev, dev->connections);
> }
> +
> +netdef->connections++;
> +VIR_DEBUG("Using network %s, %d connections",
> + netdef->name, netdef->connections);
> ret = 0;
> error:
> for (ii = 0; ii < num_virt_fns; ii++)
> @@ -3114,6 +3118,9 @@ networkNotifyActualDevice(virDomainNetDefPtr iface)
> }
>
> cleanup:
> +netdef->connections++;
> +VIR_DEBUG("Using network %s, %d connections",
> + netdef->name, netdef->connections);
> ret = 0;
> error:
> if (network)
> @@ -3199,6 +3206,9 @@ networkReleaseActualDevice(virDomainNetDefPtr iface)
> }
>
> cleanup:
> +netdef->connections--;
> +VIR_DEBUG("Releasing network %s, %d connections",
> + netdef->name, netdef->connections);
> ret = 0;
> error:
> if (network)
> --
> 1.7.11.2
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 3/5] conf: output forward device connections count in network XML
Looks good to me.
Acked-by: Kyle Mestery
On Aug 14, 2012, at 2:10 AM, Laine Stump wrote:
> It may be useful for management applications to know which physical
> network devices are in use by guests. This information is already
> available in the network objects, but wasn't output in the XML. This
> patch outputs it whan the INACTIVE flag isn't set (and if it's non-0).
> ---
> src/conf/network_conf.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 905c644..ca5b759 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1495,8 +1495,14 @@ char *virNetworkDefFormat(const virNetworkDefPtr def,
> unsigned int flags)
> if (def->nForwardIfs &&
> (!def->nForwardPfs || !(flags & VIR_NETWORK_XML_INACTIVE))) {
> for (ii = 0; ii < def->nForwardIfs; ii++) {
> -virBufferEscapeString(&buf, "\n",
> +virBufferEscapeString(&buf, " def->forwardIfs[ii].dev);
> +if (!(flags & VIR_NETWORK_XML_INACTIVE) &&
> +(def->forwardIfs[ii].connections > 0)) {
> +virBufferAsprintf(&buf, " connections='%d'",
> + def->forwardIfs[ii].connections);
> +}
> +virBufferAddLit(&buf, "/>\n");
> }
> }
> if (def->nForwardPfs || def->nForwardIfs)
> --
> 1.7.11.2
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 1/5] conf: use a unique data type for PF array in virDomainNetDef
Looks good to me.
Acked-by: Kyle Mestery
On Aug 14, 2012, at 2:10 AM, Laine Stump wrote:
> This array was originally defined using the existing
> virNetworkForwardIfDef, but that struct has a UsageCount field that
> isn't used in the case of PFs. This patch just copies that struct and
> removes UsageCount. It ends up being a struct with a single field, but
> I left it as a struct in case we need to add other fields to it in the
> future.
> ---
> src/conf/network_conf.c | 9 +++--
> src/conf/network_conf.h | 8 +++-
> 2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 783c388..666118c 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -97,6 +97,12 @@ virNetworkForwardIfDefClear(virNetworkForwardIfDefPtr def)
> VIR_FREE(def->dev);
> }
>
> +static void
> +virNetworkForwardPfDefClear(virNetworkForwardPfDefPtr def)
> +{
> +VIR_FREE(def->dev);
> +}
> +
> static void virNetworkIpDefClear(virNetworkIpDefPtr def)
> {
> int ii;
> @@ -157,7 +163,7 @@ void virNetworkDefFree(virNetworkDefPtr def)
> VIR_FREE(def->domain);
>
> for (ii = 0 ; ii < def->nForwardPfs && def->forwardPfs ; ii++) {
> -virNetworkForwardIfDefClear(&def->forwardPfs[ii]);
> +virNetworkForwardPfDefClear(&def->forwardPfs[ii]);
> }
> VIR_FREE(def->forwardPfs);
>
> @@ -1113,7 +1119,6 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> goto error;
> }
>
> -def->forwardPfs->usageCount = 0;
> def->forwardPfs->dev = forwardDev;
> forwardDev = NULL;
> def->nForwardPfs++;
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index a95b382..040d912 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -135,6 +135,12 @@ struct _virNetworkForwardIfDef {
> int usageCount; /* how many guest interfaces are bound to this device? */
> };
>
> +typedef struct _virNetworkForwardPfDef virNetworkForwardPfDef;
> +typedef virNetworkForwardPfDef *virNetworkForwardPfDefPtr;
> +struct _virNetworkForwardPfDef {
> +char *dev; /* name of device */
> +};
> +
> typedef struct _virPortGroupDef virPortGroupDef;
> typedef virPortGroupDef *virPortGroupDefPtr;
> struct _virPortGroupDef {
> @@ -164,7 +170,7 @@ struct _virNetworkDef {
> * interfaces), they will be listed here.
> */
> size_t nForwardPfs;
> -virNetworkForwardIfDefPtr forwardPfs;
> +virNetworkForwardPfDefPtr forwardPfs;
>
> size_t nForwardIfs;
> virNetworkForwardIfDefPtr forwardIfs;
> --
> 1.7.11.2
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 4/5] network: change cleanup: to error: in network*() functions
Looks good.
Acked-by: Kyle Mestery
On Aug 14, 2012, at 2:10 AM, Laine Stump wrote:
> A later patch will be adding a counter that will be
> incremented/decremented each time an guest interface starts/stops
> using a particular network. For this to work, all types of networks
> need to go through a common return sequence rather than returning
> early. To setup for this, the existing cleanup: label is renamed to
> error:, a new cleanup: label is added (when necessary), and early
> returns are changed to goto cleanup (except the case where the
> interface type != NETWORK).
> ---
> src/network/bridge_driver.c | 106 ++--
> 1 file changed, 53 insertions(+), 53 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 77b38d2..680b3f3 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2767,9 +2767,8 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> virReportError(VIR_ERR_NO_NETWORK,
>_("no network with matching name '%s'"),
>iface->data.network.name);
> -goto cleanup;
> +goto error;
> }
> -
> netdef = network->def;
>
> /* portgroup can be present for any type of network, in particular
> @@ -2786,12 +2785,12 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> if (!iface->data.network.actual
> && (VIR_ALLOC(iface->data.network.actual) < 0)) {
> virReportOOMError();
> -goto cleanup;
> +goto error;
> }
>
> if (virNetDevBandwidthCopy(&iface->data.network.actual->bandwidth,
>portgroup->bandwidth) < 0)
> -goto cleanup;
> +goto error;
> }
>
> if ((netdef->forwardType == VIR_NETWORK_FORWARD_NONE) ||
> @@ -2813,14 +2812,14 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> if (!iface->data.network.actual
> && (VIR_ALLOC(iface->data.network.actual) < 0)) {
> virReportOOMError();
> -goto cleanup;
> +goto error;
> }
>
> iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
> iface->data.network.actual->data.bridge.brname =
> strdup(netdef->bridge);
> if (!iface->data.network.actual->data.bridge.brname) {
> virReportOOMError();
> -goto cleanup;
> +goto error;
> }
>
> /* merge virtualports from interface, network, and portgroup to
> @@ -2831,7 +2830,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> netdef->virtPortProfile,
> portgroup
> ? portgroup->virtPortProfile : NULL)
> < 0) {
> -goto cleanup;
> +goto error;
> }
> virtport = iface->data.network.actual->virtPortProfile;
> if (virtport) {
> @@ -2842,7 +2841,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> "'%s' which uses a bridge device"),
>
> virNetDevVPortTypeToString(virtport->virtPortType),
>netdef->name);
> -goto cleanup;
> +goto error;
> }
> }
>
> @@ -2858,7 +2857,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> if (!iface->data.network.actual
> && (VIR_ALLOC(iface->data.network.actual) < 0)) {
> virReportOOMError();
> -goto cleanup;
> +goto error;
> }
>
> /* Set type=direct and appropriate */
> @@ -2886,7 +2885,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> netdef->virtPortProfile,
> portgroup
> ? portgroup->virtPortProfile : NULL)
> < 0) {
> -goto cleanup;
> +goto error;
> }
> virtport = iface->data.network.actual->virtPortProfile;
> if (virtport) {
> @@ -2898,7 +2897,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> "'%s' which uses a macvtap device"),
>
> virNetDevVPortTypeToString(virtport->virtPortType),
>netdef->name);
> -goto cleanup;
> +goto error;
> }
> }
>
> @@ -2910,7 +2909,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
>_("network '%s' uses a direct mode, but "
> "has no forward dev and no interface pool"),
>netdef->name);
> -goto cleanup;
> +goto error;
> } else {
> /* pick an interface from the pool */
>
> @@ -2927,19 +2926,19 @@ networkAllocateAct
Re: [libvirt] [PATCHv2 2/5] conf: rename interface "usageCount" to "connections"
Looks good to me.
Acked-by: Kyle Mestery
On Aug 14, 2012, at 2:10 AM, Laine Stump wrote:
> I want to include this count in the xml output of networks, but
> calling it "connections" in the XML sounds better than "usageCount", and it
> would be better if the name in the XML matched the variable name.
>
> In a few places, usageCount was being initialized to 0, but this is
> unnecessary, because VIR_ALLOC_N zero-fills everything anyway.
> ---
> src/conf/network_conf.c | 2 --
> src/conf/network_conf.h | 2 +-
> src/network/bridge_driver.c | 46 ++---
> 3 files changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 666118c..905c644 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1137,7 +1137,6 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> }
>
> if (forwardDev) {
> -def->forwardIfs[0].usageCount = 0;
> def->forwardIfs[0].dev = forwardDev;
> forwardDev = NULL;
> def->nForwardIfs++;
> @@ -1169,7 +1168,6 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>
> def->forwardIfs[ii].dev = forwardDev;
> forwardDev = NULL;
> -def->forwardIfs[ii].usageCount = 0;
> def->nForwardIfs++;
> }
> }
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 040d912..23f1632 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -132,7 +132,7 @@ typedef struct _virNetworkForwardIfDef
> virNetworkForwardIfDef;
> typedef virNetworkForwardIfDef *virNetworkForwardIfDefPtr;
> struct _virNetworkForwardIfDef {
> char *dev; /* name of device */
> -int usageCount; /* how many guest interfaces are bound to this device? */
> +int connections; /* how many guest interfaces are connected to this
> device? */
> };
>
> typedef struct _virNetworkForwardPfDef virNetworkForwardPfDef;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index ec99e4d..77b38d2 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2914,10 +2914,11 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> } else {
> /* pick an interface from the pool */
>
> -/* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require
> - * exclusive access to a device, so current usageCount must be
> - * 0. Other modes can share, so just search for the one with
> - * the lowest usageCount.
> +/* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both
> + * require exclusive access to a device, so current
> + * connections count must be 0. Other modes can share, so
> + * just search for the one with the lowest number of
> + * connections.
> */
> if (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) {
> if ((netdef->nForwardPfs > 0) && (netdef->nForwardIfs <= 0)) {
> @@ -2949,14 +2950,13 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> virReportOOMError();
> goto cleanup;
> }
> -netdef->forwardIfs[ii].usageCount = 0;
> }
> }
>
> -/* pick first dev with 0 usageCount */
> +/* pick first dev with 0 connections */
>
> for (ii = 0; ii < netdef->nForwardIfs; ii++) {
> -if (netdef->forwardIfs[ii].usageCount == 0) {
> +if (netdef->forwardIfs[ii].connections == 0) {
> dev = &netdef->forwardIfs[ii];
> break;
> }
> @@ -2966,9 +2966,9 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
>
> (iface->data.network.actual->virtPortProfile->virtPortType
> == VIR_NETDEV_VPORT_PROFILE_8021QBH)) {
>
> -/* pick first dev with 0 usageCount */
> +/* pick first dev with 0 connections */
> for (ii = 0; ii < netdef->nForwardIfs; ii++) {
> -if (netdef->forwardIfs[ii].usageCount == 0) {
> +if (netdef->forwardIfs[ii].connections == 0) {
> dev = &netdef->forwardIfs[ii];
> break;
> }
> @@ -2977,7 +2977,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> /* pick least used dev */
> dev = &netdef->forwardIfs[0];
> for (ii = 1; ii < netdef->nForwardIfs; ii++) {
> -if (netdef->forwardIfs[ii].usageCount < dev->usageCount)
> +if (netdef->forwardIfs[ii].connections <
> dev->connections)
> dev = &netde
Re: [libvirt] [PATCHv2 9/9] network: merge relevant virtualports rather than choosing one
This looks good to me, with some minor nits below. Acked-by: Kyle Mestery On Aug 14, 2012, at 2:04 AM, Laine Stump wrote: > One of the original ideas behind allowing a in an > interface definition as well as in the definition *and*one > or more s within the network, was that guest-specific > parameteres (like instanceid and interfaceid) could be given in the > interface's virtualport, and more general things (portid, managerid, > etc) could be given in the network and/or portgroup, with all the bits > brought together at guest startup time and combined into a single > virtualport to be used by the guest. This was somehow overlooked in > the implementation, though - it simply picks the "most specific" > virtualport, and uses the entire thing, with no attempt to merge in > details from the others. > > This patch uses virNetDevVPortProfileMerge3() to combine the three > possible virtualports into one, then uses > virNetDevVPortProfileCheck*() to verify that the resulting virtualport > type is appropriate for the type of network, and that all the required > attributes for that type are present. > > An example of usage is this: assuming a definitions on host > ABC of: > > >testA >... > >... > > > > > > > > > > > > > and the same on host DEF of: > > >testA >... > > > >... > > > > > > > > > > > > > and a guest definition of: > > > > >interfaceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f"\> > >... > > > If the guest was started on host ABC, the used would be: > > >profileid='sales'/> > > > but if that guest was started on host DEF, the would be: > > >typeid="1193047" typeidversion="2" > managerid="55"/> > > > Additionally, if none of the involved s had a specified type > (this includes cases where no virtualport is given at all), > --- > docs/formatdomain.html.in | 72 --- > docs/formatnetwork.html.in | 27 - > src/network/bridge_driver.c | 74 - > 3 files changed, 134 insertions(+), 39 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index f97c630..f3b3fa8 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -2230,11 +2230,40 @@ > the network; one network may have multiple portgroups defined, > with each portgroup containing slightly different configuration > information for different classes of network > - connections. Since 0.9.4). Also, > - similar to direct network connections (described > - below), a connection of type network may specify > - a virtportprofile element, with configuration data > - to be forwarded to a vepa or 802.1Qbh compliant switch. > + connections. Since 0.9.4. > + > + > + Also, similar to direct network connections > + (described below), a connection of type network may > + specify a virtualport element, with configuration > + data to be forwarded to a vepa (802.1Qbg) or 802.1Qbh compliant > + switch (Since 0.8.2), or to an > + OpenvSwitch virtual switch (Since If you look at http://openvswitch.org/, you can see they use "Open vSwitch" instead of "OpenvSwitch" as the name. Can you adjust the term in this patch to match that? > + 0.9.11). > + > + > + Since the actual type of switch may vary depending on the > + configuration in theon the host, > + it is acceptable to omit the virtualport type > + attribute, and specify attributes from multiple different > + virtualport types (and also to leave out certain attributes); at > + domain startup time, a complete > + element will be constructed by merging together the type and > + attributes found in the which will be filled in from the network > + or portgroup ) > + (Since 0.10.0). For example, in order > + to work properly with both an 802.1Qbh switch and an OpenvSwitch Same "Open vSwitch" adjustment here. > + switch, you may choose to specify no type, but both > + an instanceid (in case the switch is 802.1Qbh) and > + an interfaceid (in case the switch is OpenvSwitch) Same "Open vSwitch" adjustment here. > + (you may also omit the other attributes, such as managerid, > + typeid, or profileid, to be filled in from the > + network's ). If you want to > + limit a guest to connecting only to certain types of switches, > + you can specify the virtualport type, but still omit some/all of > + the parameters - in this case if the host's network has a > + different type of virtualport, connection of the interface will > + fail. > >
Re: [libvirt] [PATCHv2 8/9] conf: support partially-specified in parser and formatter
This looks good to me. Acked-by: Kyle Mestery On Aug 14, 2012, at 2:04 AM, Laine Stump wrote: > Until now, all attributes in a parameter list that were > acceptable for a particular type, were also required. There were no > optional attributes. > > One of the aims of supporting in libvirt's virtual > networks and portgroups is to allow specifying the group-wide > parameters in the network's virtualport, and merge that with the > interface's virtualport, which will have the instance-specific info > (i.e. the interfaceid or instanceid). > > Additionally, the guest's interface XML shouldn't need to know what > type of network connection will be used prior to runtime - it could be > openvswitch, 802.1Qbh, 802.1Qbg, or none of the above - but should > still be able to specify instance-specific info just in case it turns > out to be applicable. > > Finally, up to now, the parser for virtualport has always generated a > random instanceid/interfaceid when appropriate, making it impossible > to leave it blank (which is what's required for virtualports within a > network/portprofile definition). > > This patch modifies the parser and formatter of the > element in the following ways: > > * because most of the attributes in a virNetDevVPortProfile are fixed > size binary data with no reserved values, there is no way to embed a > "this value wasn't specified" sentinel into the existing data. To > solve this problem, the new *_specified fields in the > virNetDevVPortProfile object that were added in a previous patch of > this series are now set when the corresponding attribute is present > during the parse. > > * allow parsing/formatting a that has no type set. In > this case, all fields are settable, but all are also optional. > > * add a GENERATE_MISSING_DEFAULTS flag to the parser - if this flag is > set and an instanceid/interfaceid is expected but not provided, a > random one will be generated. This was previously the default > behavior, but is now done only for virtualports inside an > definition, not for those in or . > > * add a REQUIRE_ALL_ATTRIBUTES flag to the parser - if this flag is > set the parser will call the new > virNetDevVPortProfileCheckComplete() functions at the end of the > parser to check for any missing attributes (based on type), and > return failure if anything is missing. This used to be default > behavior. Now it is only used for the virtualport defined inside an > interface's element (by the time you've figured out the > contents of , you should have all the necessary data to fill > in the entire virtualport) > > * add a REQUIRE_TYPE flag to the parser - if this flag is set, the > parser will return an error if the virtualport has no type > attribute. This also was previously the default behavior, but isn't > needed in the case of the virtualport for a type='network' interface > (i.e. the exact type isn't yet known), or the virtualport of a > portgroup (i.e. the portgroup just has modifiers for the network's > virtualport, which *does* require a type) - in those cases, the > check will be done at domain startup, once the final virtualport is > assembled (this is handled in the next patch). > --- > docs/schemas/networkcommon.rng | 114 ++-- > src/conf/domain_conf.c | 25 +- > src/conf/netdev_vport_profile_conf.c | 308 +++-- > src/conf/netdev_vport_profile_conf.h | 17 +- > src/conf/network_conf.c| 7 +- > tests/networkxml2xmlin/openvswitch-net.xml | 16 ++ > tests/networkxml2xmlin/vepa-net.xml| 6 +- > tests/networkxml2xmlout/openvswitch-net.xml| 16 ++ > tests/networkxml2xmlout/vepa-net.xml | 6 +- > tests/networkxml2xmltest.c | 1 + > .../qemuxml2argv-net-virtio-network-portgroup.xml | 14 + > 11 files changed, 344 insertions(+), 186 deletions(-) > create mode 100644 tests/networkxml2xmlin/openvswitch-net.xml > create mode 100644 tests/networkxml2xmlout/openvswitch-net.xml > > diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng > index 2328892..f2c3330 100644 > --- a/docs/schemas/networkcommon.rng > +++ b/docs/schemas/networkcommon.rng > @@ -15,22 +15,30 @@ > > 802.1Qbg > > - > - > - > - > - > - > - > - > - > - > - > - > - > - > - > - > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + >
Re: [libvirt] [PATCHv2 7/9] conf: simplify Buffer Indentation in virDomainNetDefFormat
This looks good to me.
Acked-by: Kyle Mestery
On Aug 14, 2012, at 2:04 AM, Laine Stump wrote:
> This function has several calls to increase the buffer indent by 6,
> then decrease it again, then increase, then decrease. Additionally,
> there were several printfs that had 6 spaces at the beginning of the
> line.
>
> virDomainActualNetDefFormat, which is called by virDomainNetDefFormat,
> had similar ugliness.
>
> This patch changes both functions to just increase the indent at the
> beginning, decrease it at (well, just before*) the end, and remove all
> of the occurences of 6/8 spaces at the beginning of lines.
>
> *The indent had to be reset before the end of the function because
> virDomainDeviceInfoFormat assumes a 0 indent and is called from many
> other places, and I didn't want to do an overhaul of every caller of
> that function. A separate patch to switch all of domain_conf.c would
> be a useful exercise, but my current goal is unrelated to that, so
> I'll leave it for another day.
> ---
> src/conf/domain_conf.c | 73 +++---
> 1 file changed, 33 insertions(+), 40 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5990634..8f1f244 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11560,21 +11560,22 @@ virDomainActualNetDefFormat(virBufferPtr buf,
> return -1;
> }
>
> -virBufferAsprintf(buf, " +virBufferAsprintf(buf, " if (def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> def->data.hostdev.def.managed) {
> virBufferAddLit(buf, " managed='yes'");
> }
> virBufferAddLit(buf, ">\n");
>
> +virBufferAdjustIndent(buf, 2);
> switch (def->type) {
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> -virBufferEscapeString(buf, "\n",
> +virBufferEscapeString(buf, "\n",
> def->data.bridge.brname);
> break;
>
> case VIR_DOMAIN_NET_TYPE_DIRECT:
> -virBufferAddLit(buf, " +virBufferAddLit(buf, " if (def->data.direct.linkdev)
> virBufferEscapeString(buf, " dev='%s'",
> def->data.direct.linkdev);
> @@ -11590,12 +11591,10 @@ virDomainActualNetDefFormat(virBufferPtr buf,
> break;
>
> case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> -virBufferAdjustIndent(buf, 8);
> if (virDomainHostdevSourceFormat(buf, &def->data.hostdev.def,
> flags, true) < 0) {
> return -1;
> }
> -virBufferAdjustIndent(buf, -8);
> break;
>
> case VIR_DOMAIN_NET_TYPE_NETWORK:
> @@ -11606,14 +11605,13 @@ virDomainActualNetDefFormat(virBufferPtr buf,
> return -1;
> }
>
> -virBufferAdjustIndent(buf, 8);
> if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0)
>return -1;
> if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0)
> return -1;
> -virBufferAdjustIndent(buf, -8);
>
> -virBufferAddLit(buf, " \n");
> +virBufferAdjustIndent(buf, -2);
> +virBufferAddLit(buf, "\n");
> return 0;
> }
>
> @@ -11637,14 +11635,15 @@ virDomainNetDefFormat(virBufferPtr buf,
> }
> virBufferAddLit(buf, ">\n");
>
> +virBufferAdjustIndent(buf, 6);
> virBufferAsprintf(buf,
> - " address='%02x:%02x:%02x:%02x:%02x:%02x'/>\n",
> + "\n",
> def->mac.addr[0], def->mac.addr[1], def->mac.addr[2],
> def->mac.addr[3], def->mac.addr[4], def->mac.addr[5]);
>
> switch (def->type) {
> case VIR_DOMAIN_NET_TYPE_NETWORK:
> -virBufferEscapeString(buf, " +virBufferEscapeString(buf, " def->data.network.name);
> virBufferEscapeString(buf, " portgroup='%s'",
> def->data.network.portgroup);
> @@ -11655,39 +11654,41 @@ virDomainNetDefFormat(virBufferPtr buf,
> break;
>
> case VIR_DOMAIN_NET_TYPE_ETHERNET:
> -virBufferEscapeString(buf, " \n",
> +virBufferEscapeString(buf, "\n",
> def->data.ethernet.dev);
> if (def->data.ethernet.ipaddr)
> -virBufferAsprintf(buf, " \n",
> +virBufferAsprintf(buf, "\n",
> def->data.ethernet.ipaddr);
> break;
>
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> -virBufferEscapeString(buf, " \n",
> +virBufferEscapeString(buf, "\n",
> def->data.bridge.brname);
> -if (def->data.bridge.ipaddr)
> -virBufferAsprintf(buf, " \n",
> +if (def->data.bridge.ipaddr) {
> +virBufferAsprintf(buf, "\n",
> def->data.bridge.ipaddr);
> +}
> break;
>
> case VIR_DOMAIN_NET_TYPE_SERVER:
> case VIR_DOMAIN_NET_TYPE_CLIENT:
> case VIR_DOMAIN_NE
Re: [libvirt] [PATCHv2 4/9] util: utility functions for virNetDevVPortProfile
This looks good to me.
Acked-by: Kyle Mestery
On Aug 14, 2012, at 2:04 AM, Laine Stump wrote:
> This patch adds three utility functions that operate on
> virNetDevVPortProfile objects.
>
> * virNetDevVPortProfileCheckComplete() - verifies that all attributes
>required for the type of the given virtport are specified.
>
> * virNetDevVPortProfileCheckNoExtras() - verifies that there are no
>attributes specified which are inappropriate for the type of the
>given virtport.
>
> * virNetDevVPortProfileMerge3() - merges 3 virtports into a single,
>newly allocated virtport. If any attributes are specified in
>more than one of the three sources, and do not exactly match,
>an error is logged and the function fails.
>
> These new functions depend on new fields in the virNetDevVPortProfile
> object that keep track of whether or not each attribute was
> specified. Since the higher level parse function doesn't yet set those
> fields, these functions are not actually usable yet (but that's okay,
> because they also aren't yet used - all of that functionality comes in
> a later patch.)
>
> Note that these three functions return 0 on success and -1 on
> failure. This may seem odd for the first two Check functions, since
> they could also easily return true/false, but since they actually log
> an error when the requested condition isn't met (and should result in
> a failure of the calling function), I thought 0/-1 was more
> appropriate.
> ---
> src/libvirt_private.syms | 3 +
> src/util/virnetdevvportprofile.c | 313 +++
> src/util/virnetdevvportprofile.h | 17 ++-
> 3 files changed, 332 insertions(+), 1 deletion(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c023dbf..89fb1f4 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1427,8 +1427,11 @@ virNetDevVethDelete;
>
> # virnetdevvportprofile.h
> virNetDevVPortProfileAssociate;
> +virNetDevVPortProfileCheckComplete;
> +virNetDevVPortProfileCheckNoExtras;
> virNetDevVPortProfileDisassociate;
> virNetDevVPortProfileEqual;
> +virNetDevVPortProfileMerge3;
> virNetDevVPortProfileOpTypeFromString;
> virNetDevVPortProfileOpTypeToString;
>
> diff --git a/src/util/virnetdevvportprofile.c
> b/src/util/virnetdevvportprofile.c
> index 6db04f7..e686fd9 100644
> --- a/src/util/virnetdevvportprofile.c
> +++ b/src/util/virnetdevvportprofile.c
> @@ -120,6 +120,319 @@ virNetDevVPortProfileEqual(virNetDevVPortProfilePtr a,
> virNetDevVPortProfilePtr
> return true;
> }
>
> +/* virNetDevVPortProfileCheckComplete() checks that all attributes
> + * required for the type of virtport are specified. When
> + * generateMissing is true, any missing attribute that can be
> + * autogenerated, will be (instanceid, interfaceid). If virtport ==
> + * NULL or virtPortType == NONE, then the result is always 0
> + * (success). If a required attribute is missing, an error is logged
> + * and -1 is returned.
> + */
> +int
> +virNetDevVPortProfileCheckComplete(virNetDevVPortProfilePtr virtport,
> + bool generateMissing)
> +{
> +const char *missing = NULL;
> +
> +if (!virtport || virtport->virtPortType == VIR_NETDEV_VPORT_PROFILE_NONE)
> +return 0;
> +
> +switch (virtport->virtPortType) {
> +case VIR_NETDEV_VPORT_PROFILE_8021QBG:
> +if (!virtport->managerID_specified) {
> +missing = "managerid";
> +} else if (!virtport->typeID_specified) {
> +missing = "typeid";
> +} else if (!virtport->typeIDVersion_specified) {
> +missing = "typeidversion";
> +} else if (!virtport->instanceID_specified) {
> +if (generateMissing) {
> +if (virUUIDGenerate(virtport->instanceID) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot generate a random uuid for
> instanceid"));
> +return -1;
> +}
> +virtport->instanceID_specified = true;
> +} else {
> +missing = "instanceid";
> +}
> +}
> +break;
> +
> +case VIR_NETDEV_VPORT_PROFILE_8021QBH:
> +if (!virtport->profileID[0])
> +missing = "profileid";
> +break;
> +
> +case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
> +/* profileid is optional for openvswitch */
> +if (!virtport->interfaceID_specified) {
> +if (generateMissing) {
> +if (virUUIDGenerate(virtport->interfaceID) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot generate a random uuid for
> interfaceid"));
> +return -1;
> +}
> +virtport->interfaceID_specified = true;
> +} else {
> +missing = "interfaceid";
> +}
> +
Re: [libvirt] [PATCHv2 6/9] conf: make error returns from virDomainActualNetDefFormat consistent
Nice cleanup here, this looks good to me.
Acked-by: Kyle Mestery
On Aug 14, 2012, at 2:04 AM, Laine Stump wrote:
> There was an error: label that simply did "return ret", but ret was
> defaulted to -1, and was never used other than setting it manually to
> 0 just before a non-error return. Aside from this, some of the error
> return paths used "goto error" and others used "return ret".
>
> This patch removes ret and the error: label, and makes all error
> returns just consistently do "return -1".
> ---
> src/conf/domain_conf.c | 14 +-
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e239909..5990634 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11547,7 +11547,6 @@ virDomainActualNetDefFormat(virBufferPtr buf,
> virDomainActualNetDefPtr def,
> unsigned int flags)
> {
> -int ret = -1;
> const char *type;
> const char *mode;
>
> @@ -11558,7 +11557,7 @@ virDomainActualNetDefFormat(virBufferPtr buf,
> if (!type) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
>_("unexpected net type %d"), def->type);
> -return ret;
> +return -1;
> }
>
> virBufferAsprintf(buf, " @@ -11585,7 +11584,7 @@ virDomainActualNetDefFormat(virBufferPtr buf,
> virReportError(VIR_ERR_INTERNAL_ERROR,
>_("unexpected source mode %d"),
>def->data.direct.mode);
> -return ret;
> +return -1;
> }
> virBufferAsprintf(buf, " mode='%s'/>\n", mode);
> break;
> @@ -11604,21 +11603,18 @@ virDomainActualNetDefFormat(virBufferPtr buf,
> default:
> virReportError(VIR_ERR_INTERNAL_ERROR,
>_("unexpected net type %s"), type);
> -goto error;
> +return -1;
> }
>
> virBufferAdjustIndent(buf, 8);
> if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0)
>return -1;
> if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0)
> -goto error;
> +return -1;
> virBufferAdjustIndent(buf, -8);
>
> virBufferAddLit(buf, " \n");
> -
> -ret = 0;
> -error:
> -return ret;
> +return 0;
> }
>
> static int
> --
> 1.7.11.2
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 5/9] conf: move virtPortProfile out of unions in virDomainNetDef
This looks good to me.
Acked-by: Kyle Mestery
On Aug 14, 2012, at 2:04 AM, Laine Stump wrote:
> virtPortProfile is now used by 4 different types of network devices
> (NETWORK, BRIDGE, DIRECT, and HOSTDEV), and it's getting cumbersome to
> replicate so much code in 4 different places just because each type
> has the virtPortProfile in a slightly different place. This patch puts
> a single virtPortProfile in a common place (outside the type-specific
> union) in both virDomainNetDef and virDomainActualNetDef, and adjusts
> the parse and format code (and the few other places where it is used)
> accordingly.
>
> Note that when a element is found, the parse functions
> verify that the interface is of a type that supports one, otherwise an
> error is generated (CONFIG_UNSUPPORTED in the case of , and
> INTERNAL in the case of , since the contents of are
> always generated by libvirt itself).
> ---
> src/conf/domain_conf.c | 125 +++-
> src/conf/domain_conf.h | 10 ++--
> src/network/bridge_driver.c | 16 +++---
> src/qemu/qemu_driver.c | 2 +-
> src/qemu/qemu_hotplug.c | 19 +++
> 5 files changed, 63 insertions(+), 109 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d8c0969..e239909 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1018,20 +1018,18 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr
> def)
> switch (def->type) {
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> VIR_FREE(def->data.bridge.brname);
> -VIR_FREE(def->data.bridge.virtPortProfile);
> break;
> case VIR_DOMAIN_NET_TYPE_DIRECT:
> VIR_FREE(def->data.direct.linkdev);
> -VIR_FREE(def->data.direct.virtPortProfile);
> break;
> case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> virDomainHostdevDefClear(&def->data.hostdev.def);
> -VIR_FREE(def->data.hostdev.virtPortProfile);
> break;
> default:
> break;
> }
>
> +VIR_FREE(def->virtPortProfile);
> virNetDevBandwidthFree(def->bandwidth);
>
> VIR_FREE(def);
> @@ -1059,14 +1057,12 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
> case VIR_DOMAIN_NET_TYPE_NETWORK:
> VIR_FREE(def->data.network.name);
> VIR_FREE(def->data.network.portgroup);
> -VIR_FREE(def->data.network.virtPortProfile);
> virDomainActualNetDefFree(def->data.network.actual);
> break;
>
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> VIR_FREE(def->data.bridge.brname);
> VIR_FREE(def->data.bridge.ipaddr);
> -VIR_FREE(def->data.bridge.virtPortProfile);
> break;
>
> case VIR_DOMAIN_NET_TYPE_INTERNAL:
> @@ -1075,12 +1071,10 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
>
> case VIR_DOMAIN_NET_TYPE_DIRECT:
> VIR_FREE(def->data.direct.linkdev);
> -VIR_FREE(def->data.direct.virtPortProfile);
> break;
>
> case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> virDomainHostdevDefClear(&def->data.hostdev.def);
> -VIR_FREE(def->data.hostdev.virtPortProfile);
> break;
>
> case VIR_DOMAIN_NET_TYPE_USER:
> @@ -1088,6 +1082,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
> break;
> }
>
> +VIR_FREE(def->virtPortProfile);
> VIR_FREE(def->script);
> VIR_FREE(def->ifname);
>
> @@ -4371,6 +4366,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
> int ret = -1;
> xmlNodePtr save_ctxt = ctxt->node;
> xmlNodePtr bandwidth_node = NULL;
> +xmlNodePtr virtPortNode;
> char *type = NULL;
> char *mode = NULL;
> char *addrtype = NULL;
> @@ -4403,15 +4399,24 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
> goto error;
> }
>
> -if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> -xmlNodePtr virtPortNode = virXPathNode("./virtualport", ctxt);
> -if (virtPortNode &&
> -(!(actual->data.bridge.virtPortProfile =
> - virNetDevVPortProfileParse(virtPortNode
> +virtPortNode = virXPathNode("./virtualport", ctxt);
> +if (virtPortNode) {
> +if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> +actual->type == VIR_DOMAIN_NET_TYPE_DIRECT ||
> +actual->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> +if (!(actual->virtPortProfile
> + = virNetDevVPortProfileParse(virtPortNode))) {
> +goto error;
> +}
> +} else {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _(" element unsupported for
> type='%s'"
> + " in interface's element"), type);
> goto error;
> -} else if (actual->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
> -xmlNodePtr virtPortNode;
> +}
> +}
>
> +if (actual->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
> actual->data.direct.linkdev =
> virXPathString("string(./source[1]/@dev)", ctxt);
>
> mode
Re: [libvirt] [PATCHv2 2/9] util: eliminate union in virNetDevVPortProfile
This looks good to me.
Acked-by: Kyle Mestery
On Aug 14, 2012, at 2:04 AM, Laine Stump wrote:
> virNetDevVPortProfile has (had) a type field that can be set to one of
> several values, and a union of several structs, one for each
> type. When a domain's interface object is of type "network", the
> domain config may not know beforehand which type of virtualport is
> going to be provided in the actual device handed down from the network
> driver at runtime, but may want to set some values in the virtualport
> that may or may not be used, depending on the type. To support this
> usage, this patch replaces the union of structs with toplevel fields
> in the struct, making it possible for all of the fields to be set at
> the same time.
> ---
> src/conf/netdev_vport_profile_conf.c | 38 ++--
> src/util/virnetdevopenvswitch.c | 8
> src/util/virnetdevvportprofile.c | 24 +++
> src/util/virnetdevvportprofile.h | 27 -
> 4 files changed, 47 insertions(+), 50 deletions(-)
>
> diff --git a/src/conf/netdev_vport_profile_conf.c
> b/src/conf/netdev_vport_profile_conf.c
> index d008042..31ee9b4 100644
> --- a/src/conf/netdev_vport_profile_conf.c
> +++ b/src/conf/netdev_vport_profile_conf.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2009-2011 Red Hat, Inc.
> + * Copyright (C) 2009-2012 Red Hat, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -100,7 +100,7 @@ virNetDevVPortProfileParse(xmlNodePtr node)
> goto error;
> }
>
> -virtPort->u.virtPort8021Qbg.managerID = (uint8_t)val;
> +virtPort->managerID = (uint8_t)val;
>
> if (virStrToLong_ui(virtPortTypeID, NULL, 0, &val)) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> @@ -114,7 +114,7 @@ virNetDevVPortProfileParse(xmlNodePtr node)
> goto error;
> }
>
> -virtPort->u.virtPort8021Qbg.typeID = (uint32_t)val;
> +virtPort->typeID = (uint32_t)val;
>
> if (virStrToLong_ui(virtPortTypeIDVersion, NULL, 0, &val)) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> @@ -128,17 +128,17 @@ virNetDevVPortProfileParse(xmlNodePtr node)
> goto error;
> }
>
> -virtPort->u.virtPort8021Qbg.typeIDVersion = (uint8_t)val;
> +virtPort->typeIDVersion = (uint8_t)val;
>
> if (virtPortInstanceID != NULL) {
> if (virUUIDParse(virtPortInstanceID,
> - virtPort->u.virtPort8021Qbg.instanceID)) {
> + virtPort->instanceID)) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("cannot parse instanceid parameter
> as a uuid"));
> goto error;
> }
> } else {
> -if (virUUIDGenerate(virtPort->u.virtPort8021Qbg.instanceID))
> {
> +if (virUUIDGenerate(virtPort->instanceID)) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("cannot generate a random uuid for
> instanceid"));
> goto error;
> @@ -156,7 +156,7 @@ virNetDevVPortProfileParse(xmlNodePtr node)
>
> case VIR_NETDEV_VPORT_PROFILE_8021QBH:
> if (virtPortProfileID != NULL) {
> -if (virStrcpyStatic(virtPort->u.virtPort8021Qbh.profileID,
> +if (virStrcpyStatic(virtPort->profileID,
> virtPortProfileID) != NULL) {
> virtPort->virtPortType = VIR_NETDEV_VPORT_PROFILE_8021QBH;
> } else {
> @@ -173,13 +173,13 @@ virNetDevVPortProfileParse(xmlNodePtr node)
> case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
> if (virtPortInterfaceID != NULL) {
> if (virUUIDParse(virtPortInterfaceID,
> - virtPort->u.openvswitch.interfaceID)) {
> + virtPort->interfaceID)) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
>_("cannot parse interfaceid parameter as a
> uuid"));
> goto error;
> }
> } else {
> -if (virUUIDGenerate(virtPort->u.openvswitch.interfaceID)) {
> +if (virUUIDGenerate(virtPort->interfaceID)) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
>_("cannot generate a random uuid for
> interfaceid"));
> goto error;
> @@ -187,14 +187,14 @@ virNetDevVPortProfileParse(xmlNodePtr node)
> }
> /* profileid is not mandatory for Open vSwitch */
> if (virtPortProfileID != NULL) {
> -if (virStrcpyStatic(virtPort->u.openvswitch.profileID,
> +if (virStrcpyStatic(virtPort->profileID,
>
Re: [libvirt] [PATCHv2 3/9] util: add openvswitch case to virNetDevVPortProfileEqual
This looks good to me. Acked-by: Kyle Mestery On Aug 14, 2012, at 2:04 AM, Laine Stump wrote: > This function was overlooked when openvswitch support was > added. Fortunately it's only use for update-device, which is > relatively new and seldom-used. > --- > src/util/virnetdevvportprofile.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/src/util/virnetdevvportprofile.c > b/src/util/virnetdevvportprofile.c > index af9151e..6db04f7 100644 > --- a/src/util/virnetdevvportprofile.c > +++ b/src/util/virnetdevvportprofile.c > @@ -107,6 +107,12 @@ virNetDevVPortProfileEqual(virNetDevVPortProfilePtr a, > virNetDevVPortProfilePtr > return false; > break; > > +case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: > +if (STRNEQ(a->profileID, b->profileID) || > +memcmp(a->interfaceID, b->interfaceID, VIR_UUID_BUFLEN) != 0) > +return false; > +break; > + > default: > break; > } > -- > 1.7.11.2 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 1/9] util: make return value of virUUIDFormat and virMacAddrFormat useful
This looks good to me.
Acked-by: Kyle Mestery
On Aug 14, 2012, at 2:04 AM, Laine Stump wrote:
> Both of these functions returned void, but it's convenient for them to
> return a const char* of the char* that is passed in. This was you can
> call the function and use the result in the same expression/arg.
> ---
> src/util/uuid.c | 6 --
> src/util/uuid.h | 6 +++---
> src/util/virmacaddr.c | 13 +++--
> src/util/virmacaddr.h | 4 ++--
> 4 files changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/src/util/uuid.c b/src/util/uuid.c
> index 1efde69..54e7bb6 100644
> --- a/src/util/uuid.c
> +++ b/src/util/uuid.c
> @@ -182,9 +182,10 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid) {
> *
> * Converts the raw UUID into printable format, with embedded '-'
> *
> - * Returns 0 in case of success and -1 in case of error.
> + * Returns a pointer to the resulting character string.
> */
> -void virUUIDFormat(const unsigned char *uuid, char *uuidstr)
> +const char *
> +virUUIDFormat(const unsigned char *uuid, char *uuidstr)
> {
> snprintf(uuidstr, VIR_UUID_STRING_BUFLEN,
>
> "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
> @@ -193,6 +194,7 @@ void virUUIDFormat(const unsigned char *uuid, char
> *uuidstr)
> uuid[8], uuid[9], uuid[10], uuid[11],
> uuid[12], uuid[13], uuid[14], uuid[15]);
> uuidstr[VIR_UUID_STRING_BUFLEN-1] = '\0';
> +return uuidstr;
> }
>
>
> diff --git a/src/util/uuid.h b/src/util/uuid.h
> index da56a92..54e0573 100644
> --- a/src/util/uuid.h
> +++ b/src/util/uuid.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2007, 2011 Red Hat, Inc.
> + * Copyright (C) 2007, 2011, 2012 Red Hat, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -35,7 +35,7 @@ int virUUIDParse(const char *uuidstr,
> unsigned char *uuid)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>
> -void virUUIDFormat(const unsigned char *uuid,
> - char *uuidstr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +const char *virUUIDFormat(const unsigned char *uuid,
> + char *uuidstr) ATTRIBUTE_NONNULL(1)
> ATTRIBUTE_NONNULL(2);
>
> #endif /* __VIR_UUID_H__ */
> diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
> index c07976b..e207927 100644
> --- a/src/util/virmacaddr.c
> +++ b/src/util/virmacaddr.c
> @@ -176,14 +176,23 @@ virMacAddrParse(const char* str, virMacAddrPtr addr)
> return -1;
> }
>
> -void virMacAddrFormat(const virMacAddrPtr addr,
> - char *str)
> +/* virMacAddrFormat
> + * Converts the binary mac address in addr into a NULL-terminated
> + * character string in str. It is assumed that the memory pointed to
> + * by str is at least VIR_MAC_STRING_BUFLEN bytes long.
> + *
> + * Returns a pointer to the resulting character string.
> + */
> +const char *
> +virMacAddrFormat(const virMacAddrPtr addr,
> + char *str)
> {
> snprintf(str, VIR_MAC_STRING_BUFLEN,
> "%02X:%02X:%02X:%02X:%02X:%02X",
> addr->addr[0], addr->addr[1], addr->addr[2],
> addr->addr[3], addr->addr[4], addr->addr[5]);
> str[VIR_MAC_STRING_BUFLEN-1] = '\0';
> +return str;
> }
>
> void virMacAddrGenerate(const unsigned char prefix[VIR_MAC_PREFIX_BUFLEN],
> diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h
> index fdac362..4c5074c 100644
> --- a/src/util/virmacaddr.h
> +++ b/src/util/virmacaddr.h
> @@ -44,8 +44,8 @@ int virMacAddrCmpRaw(const virMacAddrPtr mac1,
> void virMacAddrSet(virMacAddrPtr dst, const virMacAddrPtr src);
> void virMacAddrSetRaw(virMacAddrPtr dst, const unsigned char
> s[VIR_MAC_BUFLEN]);
> void virMacAddrGetRaw(virMacAddrPtr src, unsigned char dst[VIR_MAC_BUFLEN]);
> -void virMacAddrFormat(const virMacAddrPtr addr,
> - char *str);
> +const char *virMacAddrFormat(const virMacAddrPtr addr,
> + char *str);
> void virMacAddrGenerate(const unsigned char prefix[VIR_MAC_PREFIX_BUFLEN],
> virMacAddrPtr addr);
> int virMacAddrParse(const char* str,
> --
> 1.7.11.2
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Add vlantag parameter for openvswitch ports
On Aug 13, 2012, at 12:04 AM, Laine Stump wrote: > On 08/10/2012 05:44 PM, Laine Stump wrote: >> How about allowing multiple at the >> toplevel? Then you could have: >> >> >> >> in the simplest case, or: >> >> >> >> if you wanted a trunk with a single tag, or: >> >> >> >> >> >> if you want a trunk with multiple tags (trunk='yes' would be implicit). >> >> For either of these cases, the data definition can remain the same (as >> long as we don't require that superfluous "trunk='yes|no'" attributes be >> preserved across an iteration of parse/format.) > > I've implemented the data structure, parser, and formatter for the above > proposal, and tied it into the domain and network config, as well as > modifying the network driver to grab the most appropriate vlan object > from the interface / portgroup / network configuration and make it > available via a single function call: > > https://www.redhat.com/archives/libvir-list/2012-August/msg00790.html > > I haven't yet implemented actually *setting* the vlan for PCI passthrough > with , but all of the xml2xml tests run properly, > so hopefully it's all functional enough to hook into the openvswitch backend. > Thanks Laine, I see your patches, I'll review them now and see if I can test out hooking up Open vSwitch support. If it all works, I'll send out a new patch to allow setting this for Open vSwitch networks. Thanks! Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Add vlantag parameter for openvswitch ports
On Aug 9, 2012, at 3:14 PM, Laine Stump wrote: > On 08/08/2012 03:47 PM, Kyle Mestery wrote: >> Add the ability to specify a "vlantag" parameter for bridge >> networks with a virtualport type of openvswitch. This >> allows for specifying the port is on a single VLAN, and >> should receive untagged traffic (e.g. vlantag=10), or that >> a port should receive tagged traffic and is a trunk port >> (e.g. vlantag=10,11). > > Ha! We must have a telepathic link! > Well, we're at least thinking in the same general direction. :) > I'm just about to add vlan support for SR-IOV VFs in PCI passthrough > (aka "hostdev") and macvtap passthrough modes (the only thing that's > kept me from action is indecision about where to put the vlan tag in the > xml), and since at least one of those modes doesn't use at > all, I was planning to add a element at the toplevel of > interface. There would also be a similar element at the toplevel of > , and another within . > > Something like this: > > > > > ... > > > In this case, there is no involved. Here's a case that uses a > network: > > > > > > ... > > > > net42 > > > > > > > > > In this second case, when the interface was brought up, it would > allocate a physical device from the pool, and inherit the vlan tag, > which would be set in the SR-IOV card's hardware as it's assigned to the > guest. Note that hostdev passthrough would behave similarly (except > forward mode would be 'hosted') > Those cases both look good. I think the formatting works just fine for virtualport type=openvswitch as well, something like this: Single VLAN (no trunk): Single VLAN (trunk): Multiple VLANs (trunk): > Finally, there may be a network with multiple vlans. portgroups could be > used for that: > > > > > ... > > > > net42 > > > > > > > > > .. > > > > > > .. > > > > > Selecting a different portgroup would put you on a difference vlan of > the same switch. We would need to make a decision about which would take > precedence if the vlan tag was given in multiple places - should the > domain's request be honored, in order to allow it to made individual > modifications to the general config in the ? Or should the > network, as an entity of higher authority, be allowed to override what > the domain asks for? > I think this is the most elegant solution, and dovetails nicely with the fact that virtualport type='openvswitch' ports already support port-profiles. I think in general, my feeling here is the domain XML should override the network element. This is similar to an interface override of a port-profile on a Cisco switch, for example, where you define configuration in a port-profile, but allow configuration directly on the interface itself to override the port-profile configuration. > I've never used vlan trunk groups, but I'm guessing whatever information > you needed could be added as attributes to the element. BTW, in > general we don't like to have multiple pieces of data in a single > element. We would prefer that they be in separate elements. So maybe if > you wanted a single vlan tag, you could do it as above, but when you > wanted a trunk group, you could do something like: > > > > > > > > (you might allow omitting the "trunk='yes'") Or possibly put them all at > the top level: > > > > > > and if you wanted a vlan trunk with only a single vlan in it: > > > > (actually, having multiple toplevel elements could get messy if we > started talking about merging the elements from > network/portgroup/interface together, so maybe that's not such a good idea). > > *Still another* possibility would be to put the vlan element as > described above inside , then allow with no > type to contain a element. This would require a change to my > recent merging patches, but they haven't been pushed yet. > I'm not convinced I like that option, though. > > I think this needs a day or so to stew... So I guess the question is, are you going to role this up into your patches? If so, you can likely make use of some pieces of the patches I posted. Let me know, and I can also work with you on this as well. Thanks, Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] sending openvswitch "port db" info during libvirt migration
On Aug 9, 2012, at 4:23 AM, Daniel P. Berrange wrote: > On Wed, Aug 08, 2012 at 10:58:28PM -0400, Laine Stump wrote: >> On 08/08/2012 12:30 PM, Kyle Mestery (kmestery) wrote: >>> On Aug 8, 2012, at 9:49 AM, Daniel P. Berrange wrote: >>>> On Tue, Aug 07, 2012 at 03:50:20PM -0400, Laine Stump wrote: >>>>> Someone asked on IRC the other day about sending openvswitch per-port >>>>> data (normally stored in the switch) to the destination host during a >>>>> migration. I suggested maybe this could be handled by encoding the >>>>> information into the interface's prior to migration, and >>>>> then writing it back out to openvswitch on the destination when the >>>>> interface was reconnected there. >>>>> >>>>> I think there's a problem with that, though - they don't want to save >>>>> the port data on the source until the instant that the interface is >>>>> disconnected (since it is constantly changing), but by then the domain >>>>> XML has already long ago been formatted and sent to the destination. >>>>> >>>>> So is there some other way within the confines of the current migration >>>>> protocol that this information can be sent from migration source to >>>>> destination? >>>> I'd be interested to know more about just what sort of data it is that >>>> needs to be passed around. From what you describe though, the only >>>> way to pass that info would be to store it in the migration cookie >>>> when the 'Perform' step completes (ie QEMU now paused, no network >>>> I/O taking place), whereupon it will be passed into the 'Finish' >>>> step on the dest (which can set it and then resume QEMU) >>>> >>> This is per-port data, tied to the specific virtual port associated with >>> the VM >>> itself. This data is stored in the OVS DB, and is used/could be used by an >>> entity controlling Open vSwitch on the host. For example, stats may be kept >>> here. >>> >>>> From what you indicate above, can you point me at the code which handles >>>> the >>> migration cookie? >> >> Look in qemu_migration.c. I notice there is also code there that calls >> the 802.1QbX-oriented virtualport associate function (search for >> qemuMigrationVPAssociatePortProfiles), and it's likely there where the >> port data should be unloaded from the cookie into the destination host. >> As for when/where to load the port data *into* the cookie on the source, >> I'm new to that code too, and didn't see it right away :-) >> >> I do see that the entire persistent definition can be sent in the >> cookie. It seems to me that "last minute (last instant) updates" to the >> live XML could be an issue not just for interfaces connected to ovs >> switches, but for other circumstances as well. Does it maybe make sense >> to solve this in a more generalized fashion, by resending the complete >> live XML? (I'm assuming that during migration libvirt is preventing any >> modification to the live XML that would result in invalidating the vm >> state contained in the memory image being transferred). > > No, at that point the VM is already started on the dest host, so you > can't just re-send the live XML there. You can put any other data into > the cookie though, it doesn't have to be passed in the persistent XML > directly. > > > Daniel Thanks guys, I think I see how to do this now. I'll work on this and send some patches. Thanks, Kyle -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/9] network: properly support openvswitch in
On Aug 8, 2012, at 7:06 PM, Ansis Atteka wrote: > > On Wed, Aug 8, 2012 at 2:18 PM, Laine Stump wrote: > On 08/08/2012 03:43 PM, Ansis Atteka wrote: >> >> >> On Sat, Aug 4, 2012 at 10:16 PM, Laine Stump wrote: >> Although it's been possible (ever since openvswitch was added to >> libvirt in 0.9.11) for a libvirt network to use an openvswitch bridge >> (by adding ), the virtualport in the >> network would always have a default random interfaceid included, which >> would be re-used for all interfaces using that network, which doesn't >> really work at all. The alternative was to not specify openvswitch in >> the definition, but to do it in the guest's >> definition instead - this of course goes against the principle of not >> having host-specific config embedded in guest config. >> >> This patch series enhances the functionality of >> elements, to allow omitting some attributes (and even the type), and >> to merge the interface, network, and portgroup virtualports rather >> than simply picking one. This not only makes openvswitch s >> more practical (because the network can specify type='openvswitch' >> without also specifying an interfaceid), but also makes >> in networks and portgroups more useful in general - for example, an >> interface can specify an interfaceid (used only by openvswitch) *and* >> an instanceid (used only by 802.1Qbh), while the network's virtualport >> specifies only the type, and the portgroups specify the managerid, >> typeid, profileid, or whatever is appropriate for the type of switch >> used by the network. >> >> The result is that the guest config can be completely devoid of >> knowledge about the type of switch being used on the hardware, but can >> still enjoy full configurability for whatever switch ends up being >> used. >> If I understand correctly, then these patch series should enable >> following configuration to work: >> >> The domain XML: >> ... >> >> >> >> > function='0x0'/> >> >> ... >> >> The network XML: >> >> ovsnet >> ad68ae68-ee26-5c65-8cff-e6c182ff3593 >> >> >> >> >> >> >> >> And then I would expect that libvirt would auto generate the >> interface IDs for each interface that gets added to this ovsnet >> network, > > That was (at some point anyway) my intent - if the interface has an interface > id use it, if not then generate one, but... > > >> but instead I am seeing the following error: >> >> 2012-08-08 19:22:16.149+: 10840: error : >> virNetDevVPortProfileCheckComplete:165 : XML error: missing interfaceid in >> > > Urg. In the end I forgot that part, so when it checks for completeness, it > fails. I'll make a patch to fix that. > > Thanks for catching that bug. > > (one issue about this - since it's not known until runtime that the network > is ovs, the interfaceid won't be generated until then, and at that time it's > not reasonable to update the interfaceid in the guest's persistent config. > So, if you're configured this way, the guest will get a different new > interfaceid every time it is restarted.) > That will not work well from the OpenFlow Controller > perspective. > > Interface ID must not change across guest restarts, > otherwise OpenFlow Controller will loose the track > on which interface was which. > I agree with Ansis here. If this UUID changes each time the VM is started, it's effectively useless for something external to libvirt/OVS to utilize to recognize the interface. It needs to remain the same for the life of the interface on the VM. > > >> >> I guess, it would be desirable to auto-generate interface >> IDs, if the network was of type openvswitch? Otherwise >> domain XML still needs to know what kind of type is >> the underlying bridge in the network XML. > > Agreed. It's okay if it *optionally* puts in that info (just in case the > network is ovs), but it shouldn't require it. > > >> >> Though, how would this work in Actual Config, once the >> network type gets switched back from OVS to Linux >> Bridge? Would the interface ID be automatically removed >> from all relevant Domain XMLs? > > Kind of. More exactly, the interface ID would be ignored when not relevant. > Similarly, you could also have an instanceid (used by 802.1Qbg), and if the > actual network type was ovs, the instanceid would be ignored. The only thing > you *can't* do is to specify in the interface if the > network is actually some other type. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/1] Add VLAN capability to openvswitch virtualport types
On Aug 8, 2012, at 3:53 PM, dennis jenkins wrote: > How would one specify VLAN trunk mode with only one vlan tag present? > Actually, this is a deficiency with the model below, it isn't possible. I had thought of using "vlantag=" for access ports, and "vlantrunk=" for trunks, but was hoping to simplify things. Would the 2 different parameter approach be better? What do others think? Thanks, Kyle > On Wed, Aug 8, 2012 at 2:47 PM, Kyle Mestery wrote: >> With this change, it is now possible to support VLANs (both access and >> trunks) >> for openvswitch ports in libvirt. This also takes into account the profileid >> parameter, as the vlantag parameter is also optional. >> >> Examples of this configuration are below. >> >> Setup the port as an access port: >> >> >> >> >> >>> vlantag='70'/> >> >> >> >> >> >> Setup the port as an trunk port: >> >> >> >> >> >>> vlantag='70,71,72'/> >> >> >> >> >> >> src/conf/netdev_vport_profile_conf.c | 34 ++ >> src/util/virnetdevopenvswitch.c | 23 +-- >> src/util/virnetdevvportprofile.h | 2 ++ >> 3 files changed, 53 insertions(+), 6 deletions(-) >> >> -- >> 1.7.11.2 >> >> -- >> 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] sending openvswitch "port db" info during libvirt migration
On Aug 8, 2012, at 9:49 AM, Daniel P. Berrange wrote: > On Tue, Aug 07, 2012 at 03:50:20PM -0400, Laine Stump wrote: >> Someone asked on IRC the other day about sending openvswitch per-port >> data (normally stored in the switch) to the destination host during a >> migration. I suggested maybe this could be handled by encoding the >> information into the interface's prior to migration, and >> then writing it back out to openvswitch on the destination when the >> interface was reconnected there. >> >> I think there's a problem with that, though - they don't want to save >> the port data on the source until the instant that the interface is >> disconnected (since it is constantly changing), but by then the domain >> XML has already long ago been formatted and sent to the destination. >> >> So is there some other way within the confines of the current migration >> protocol that this information can be sent from migration source to >> destination? > > I'd be interested to know more about just what sort of data it is that > needs to be passed around. From what you describe though, the only > way to pass that info would be to store it in the migration cookie > when the 'Perform' step completes (ie QEMU now paused, no network > I/O taking place), whereupon it will be passed into the 'Finish' > step on the dest (which can set it and then resume QEMU) > This is per-port data, tied to the specific virtual port associated with the VM itself. This data is stored in the OVS DB, and is used/could be used by an entity controlling Open vSwitch on the host. For example, stats may be kept here. >From what you indicate above, can you point me at the code which handles the migration cookie? Thanks! Kyle > > Daniel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/9] network: properly support openvswitch in
I just looked at this series and it looks great to me Laine. I think the new flexibility this series introduces will be really nice to have, and it will make starting guests across hosts with differing network types easier as well. Thanks! Kyle On Aug 5, 2012, at 12:16 AM, Laine Stump wrote: > Although it's been possible (ever since openvswitch was added to > libvirt in 0.9.11) for a libvirt network to use an openvswitch bridge > (by adding ), the virtualport in the > network would always have a default random interfaceid included, which > would be re-used for all interfaces using that network, which doesn't > really work at all. The alternative was to not specify openvswitch in > the definition, but to do it in the guest's > definition instead - this of course goes against the principle of not > having host-specific config embedded in guest config. > > This patch series enhances the functionality of > elements, to allow omitting some attributes (and even the type), and > to merge the interface, network, and portgroup virtualports rather > than simply picking one. This not only makes openvswitch s > more practical (because the network can specify type='openvswitch' > without also specifying an interfaceid), but also makes > in networks and portgroups more useful in general - for example, an > interface can specify an interfaceid (used only by openvswitch) *and* > an instanceid (used only by 802.1Qbh), while the network's virtualport > specifies only the type, and the portgroups specify the managerid, > typeid, profileid, or whatever is appropriate for the type of switch > used by the network. > > The result is that the guest config can be completely devoid of > knowledge about the type of switch being used on the hardware, but can > still enjoy full configurability for whatever switch ends up being > used. > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Attach vm-uuid to Open vSwitch interfaces.
On Mar 1, 2012, at 11:51 PM, Ansis Atteka wrote:
> This patch will allow OpenFlow controllers to identify which interface
> belongs to a particular VM by using the Domain UUID.
>
> ovs-vsctl get Interface vnet0 external_ids
> {attached-mac="52:54:00:8C:55:2C",
> iface-id="83ce45d6-3639-096e-ab3c-21f66a05f7fa", iface-status=active,
> vm-uuid="142a90a7-0acc-ab92-511c-586f12da8851"}
This patch looks good to me Ansis, and it will be handy to have the VM UUID
available in the OVS DB for connected ports. Nice work!
Thanks,
Kyle
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
