Re: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
On 28/02/13 04:09, Eric Blake wrote:
> On 02/27/2013 07:57 PM, TJ wrote:
>> From: TJ
>>
>> A DHCP relay daemon will be started that will forward all DHCP/BOOTP
>> requests on the bridge network via the first declared forward
>> interface. The relay is broadcast rather than routed so no IP address
>> is needed on the bridge.
>>
>
>> +static int
>> +networkBuildDhcpRelayCommandLine(virNetworkObjPtr network, virCommandPtr
>> *cmdout,
>> + char *pidfile)
>> +{
>> +virCommandPtr cmd = NULL;
>> +int ret = -1;
>> +
>> +cmd = virCommandNew(DHCPRELAY);
>
> Your patches are out of order. This patch tries to use DHCPRELAY, but
> that isn't defined until patch 8/10 modifies configure.ac. Squash those
> two patches together. Each patch needs to be buildable in isolation.
>
One thing I'm not yet sure how to deal with is the difference between build and
deployment systems when
one or the other doesn't have access to 'dhcp-helper', since as I understand
it, some distributions do not
include it in their package archives (it is available in Debian/Ubuntu).
With the current implementation it must be available on the build system since
otherwise the configure.ac definition of DHCPRELAY won't happen and it won't be
declared in config.h.in, which means that
compilation of the source-code relying on it will fail. It seems messy but the
only realistic way to deal with that is to surround the relevant code block(s)
with
#ifdef DHCPRELAY
...
#endif
My preferred option is to change the way DHCPRELAY is handled entirely. I
copied its definition style from DNSMASQ which, as far as I can see, *must*
exist on the build system and in the same absolute
path as it will be found on the deployment system.
It might be better to simply declare the expected basename of the executable so
instead of:
DHCPRELAY=/usr/sbin/dhcp-helper
we have
DHCPRELAY=dhcp-helper
then allow libvirt to find that executable using the PATH at run-time (which
also accommodates user builds installed to /usr/local/sbin/). Taking that
further to abstract the DHCPRELAY code entirely
from dhcp-helper (since unlike dnsmasq it isn't required to be available), it
might make sense to add an optional 'agent' attribute to the element;
something like (which would find 'agent' on the PATH).
This would allow the default to be taken from DHCPRELAY but over-ridden by the
optional 'agent' attribute.
That then leaves the issue of different DHCP relay agents requiring different
command-line parameters. I was looking at the discussion about the
element and wondering if that could have a
dual use here, especially in the name= variety, e.g:
For the DHCP relay the names ("bridge", "interface", "pdifile") are
constants that libvirt looks for in the XML. It replaces its hard-coded
command-line switch defaults with the respective
if found.
That approach would give the DHCP relay code the flexibility to evolve based on
sysadmin needs without requiring any code modification.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/4] qemu: add dtb capability
Hi Eric,
Thanks for your comments.
> -Original Message-
> From: Eric Blake [mailto:ebl...@redhat.com]
> Sent: Thursday, February 28, 2013 12:57 AM
> To: Yin Olivia-R63875
> Cc: libvir-list@redhat.com
> Subject: Re: [libvirt] [PATCH v3 2/4] qemu: add dtb capability
>
> On 02/27/2013 01:28 AM, Olivia Yin wrote:
> > Signed-off-by: Olivia Yin
> > ---
> > src/qemu/qemu_capabilities.c |3 +++
> > src/qemu/qemu_capabilities.h |1 +
> > 2 files changed, 4 insertions(+), 0 deletions(-)
>
> [not a patch review, but a general comment]
>
> You did deep threading:
>
> 0/4
> \> 1/4
>\> 2/4
> \> 3/4
> \> 4/4
>
> But typically we prefer shallow threading:
>
> 0/4
> +> 1/4
> +> 2/4
> +> 3/4
> \> 4/4
>
> You may want to investigate the settings you are using with git format-
> patch and/or send-email.
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -210,6 +210,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
> >
> >"rng-random", /* 130 */
> >"rng-egd",
> > + "dtb",
>
> OK, so I lied - I'm also doing a patch review:
> This uses a TAB, which is forbidden by 'make syntax-check'.
I'll fix it in next version.
>
> > );
> >
> > struct _virQEMUCaps {
> > @@ -944,6 +945,8 @@ virQEMUCapsComputeCmdFlags(const char *help,
> > }
> > if (strstr(help, "-uuid"))
> > virQEMUCapsSet(qemuCaps, QEMU_CAPS_UUID);
> > +if (strstr(help, "-dtb"))
> > +virQEMUCapsSet(qemuCaps, QEMU_CAPS_DTB);
>
> This won't work. -dtb did not exist prior to qemu 1.2, and for all qemu
> newer than 1.2, we do NOT scrape '-help' output; instead, we use QMP query
> commands. You need to figure out the corresponding QMP command that we can
> use to tell when dtb support exists in qemu.
How about add QEMU_CAPS_DTB into virQEMUCapsInitQMPBasic?
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 40022c1..a5bda24 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -210,6 +210,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"rng-random", /* 130 */
"rng-egd",
+ "dtb",
);
struct _virQEMUCaps {
@@ -1177,6 +1178,9 @@ virQEMUCapsComputeCmdFlags(const char *help,
if (version >= 1002000)
virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
+
+if (version >= 11000)
+virQEMUCapsSet(qemuCaps, QEMU_CAPS_DTB);
return 0;
}
@@ -2294,6 +2298,7 @@ virQEMUCapsInitQMPBasic(virQEMUCapsPtr qemuCaps)
virQEMUCapsSet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE);
virQEMUCapsSet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX);
virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_KVM_PIT);
+virQEMUCapsSet(qemuCaps, QEMU_CAPS_DTB);
}
> --
> Eric Blake eblake 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] Remove contiguous CPU indexes assumption
On 2013年02月28日 07:13, Eric Blake wrote:
On 02/27/2013 05:13 AM, Li Zhang wrote:
Hi Eric,
This should belong to bug-fix, could it be pushed to 1.0.3?
Without any test case under 'make check' that exposes the failure, I'm a
bit worried that this might cause regressions on other setups. I'm not
seven sure I even understand the scope of the problem. Is it something
specific to running a qemu-system-ppc64 emulator (but can be reproduced
on any host architecture), or is it specific to running on a powerpc
host (but happens for any version of qemu-* target architecture), or is
it something else altogether?
So sorry to see the failure.
I tried to 'make check' with my patch on ppc64, all of these cases pass.
For x86, I tried it with on x86 server, and pulled the latest version of
libvirt.
Even without my patch, it also fails during execute ./virshtest and the
tests with cpuset.
+++ out2013-02-28 13:02:14.290794080 +0800
@@ -1,2 +1,3 @@
error: vcpupin: Invalid or missing vCPU number.
--- exp2013-02-28 13:02:14.321794080 +0800
+++ out2013-02-28 13:02:14.320794080 +0800
@@ -1,2 +1,3 @@
error: vcpupin: Invalid vCPU number.
FAIL: vcpupin
I think this problem is specific ppc64.
I did some experiment on X86.
X86 machine: 4 CPUs (0~3)
I disable CPU1 and CPU2 by writing 0 to /sys/.../cpuX/online.
Only 0 and 3 are online.
Although only 2 cpus are online, but after executing "query-cpus", it
can get all the information of them.
[{"return": [{"current": true, "CPU": 0, "pc": 4294967280, "halted":
false, "thread_id": 31831}, {"current": false, "CPU": 1, "pc":
4294967280, "halted": false, "thread_id": 31832}, {"current": false,
"CPU": 2, "pc": 4294967280, "halted": false, "thread_id": 31833},
{"current": false, "CPU": 3, "pc": 4294967280, "halted": false,
"thread_id": 31834}], "id": "libvirt-3"}]
Qemu can expose all of these CPUs offline for X86.
But for ppc64, it can't expose these offline CPUs. The following is the
return value from ppc64.
[{"current": true, "CPU": 0, "nip": 256, "halted": false, "thread_id":
25964}, {"current": false, "CPU": 4, "nip": 256, "halted": true,
"thread_id": 25965}, {"current": false, "CPU": 8, "nip": 256, "halted":
true, "thread_id": 25966}, {"current": false, "CPU": 12, "nip": 256,
"halted": true, "thread_id": 25967}], "id": "libvirt-3"}
We have test framework in place to allow replaying of a QMP JSON
response and seeing how qemu_monitor_json.c deals with it; what I'd
really like to see is a side-by-side comparison of what the QMP
'query-cpus' command returns for a guest with multiple vcpus on a setup
where you are seeing the problem, when compared to a setup that does not
have the issue. You can get at this with virsh qemu-monitor-command
$dom '{"execute":"query-cpus"}', if that helps.
Thanks a lot. Tried with ppc64, it is different from x86 even with
some CPUs disabled.
To help you out, here's what I got for a guest using 3 vcpus on my
x86_64 host machine and using the qemu-kvm binary:
# virsh qemu-monitor-command guest '{"execute":"query-cpus"}'
{"return":[{"current":true,"CPU":0,"pc":1025829,"halted":false,"thread_id":5849},{"current":false,"CPU":1,"pc":1005735,"halted":true,"thread_id":5850},{"current":false,"CPU":2,"pc":1005735,"halted":true,"thread_id":5851}],"id":"libvirt-9"}
That is, your patch might be right, but I have not yet been convinced
that it is right; and while things may currently be broken on ppc, it is
not a recent breakage, so being conservative and either proving the fix
is right (via testsuite addition) or deferring the fix until post 1.0.3
seems like safer alternatives. Or maybe someone else will chime in and
agree to take it now, without waiting for my desired burden of proof.
OK, I will tried to see the testsuite problems for x86 even without my
patch.
I couldn't think of what kind of problems will this patch cause now.
Really thank you, Eric for your review and suggestion.
I don't know whether others have suggestions about my patch.
Any suggestion is appreciated.
Thanks.
--Li
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/13] Network disk improvements (NBD & libiscsi)
On 02/25/2013 10:44 AM, Paolo Bonzini wrote: > This series improves support for NBD disks (patches 1-6), and adds > support for the libiscsi userspace initiator (patches 7-13). 1 is a definite bug fix, and deserves to be in 1.0.3. It's too late for me tonight to do any more reviewing, but I also hope to check tomorrow whether any of 2-6 also make sense during the freeze. Patches 7-13 should wait until after 1.0.3 is out, since they are definitely new material and not bug fixes. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/13] qemu: fix use-after-free when parsing NBD disk
On 02/27/2013 10:03 PM, Eric Blake wrote: > So there is definitely a use-after-free bug fixed by your patch. > However, your patch causes a double-free bug on error (if the Rather, your patch did nothing to address the pre-existing double-free bug on error. Guess I should be more careful when pinning the blame :) > strdup(port) fails, then disk->hosts->port and disk->src are the same > pointer, but we attempt to free both of them). -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/13] qemu: fix use-after-free when parsing NBD disk
On 02/25/2013 10:44 AM, Paolo Bonzini wrote: > disk->src is still used for disks->hosts->name, do not free it. > > Signed-off-by: Paolo Bonzini > --- > src/qemu/qemu_command.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index dee493f..5dccaae 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -8707,7 +8707,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr > qemuCaps, A bit more context helps: char *host, *port; host = disk->src; disk->hosts->name = host; > disk->hosts->port = strdup(port); > if (!disk->hosts->port) > goto no_memory; > -VIR_FREE(disk->src); > disk->src = NULL; > break; So there is definitely a use-after-free bug fixed by your patch. However, your patch causes a double-free bug on error (if the strdup(port) fails, then disk->hosts->port and disk->src are the same pointer, but we attempt to free both of them). I'm squashing this in before pushing, to make the transfer semantics more obvious: diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 0a7d4ec..f8f3ade 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -8832,11 +8832,11 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, if (VIR_ALLOC(disk->hosts) < 0) goto no_memory; disk->nhosts = 1; -disk->hosts->name = host; +disk->hosts->name = disk->src; +disk->src = NULL; disk->hosts->port = strdup(port); if (!disk->hosts->port) goto no_memory; -disk->src = NULL; break; case VIR_DOMAIN_DISK_PROTOCOL_RBD: /* old-style CEPH_ARGS env variable is parsed later */ -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
On 02/27/2013 08:49 PM, Eric Blake wrote: >>> Your commit message is missing some substance, such as a summary of what >>> is being added. >> >> The first line of the commit message is in the email subject, and describes >> the commit. >> >> "conf: DHCP - add state for DHCP Relay and On/Off switch" > > The summary is nice, but we also want to see a sample of the XML that > you are adding. See, for example, commit 1716e7a6. The summary in 6/10 is more what I was looking for; maybe it's worth squashing that commit and this one into a single patch? -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/10] docs: Describe the 'enable' and 'relay' attributes
On 02/27/2013 09:29 PM, TJ wrote:
>> But it didn't add any unit tests
>> (a new .xml file somewhere under tests/networkxml2argvdata or
>> networkxml2xml{in,out} to verify that we can round-trip the new XML and
>> generate the expected command line), and it failed to add the RelaxNG
>> grammar specification under docs/schemas/network.rng, so there is still
>> more to be added.
>
> OK, I'll do those. As you probably realise I'm new to libvirt coding style
> and requirements, only touching it to add needed functionality, but hopefully
> can make it suitable for give-back to the
> project. I'll work up a V2 once all comments are in.
I guess I forgot to say one thing up front - while my review may have
sounded negative, that's just because it's easier to be terse when
pointing out how things can be improved. The fact that I replied on the
same day that you posted is a GOOD sign that your work is interesting,
and likely to be accepted once it is whipped into shape. Check out
HACKING, and feel free to ask questions if you have them. You may want
to wait for Laine Stump to also review, as he is more of the expert on
networking related patches, and will have more technical comments as
opposed to my stylistic overview.
And a big THANK YOU for contributing to the community. Too often, I
forget to express gratitude for new contributors braving the unknown
waters of posting to a list where they are not sure of the conventions.
Honestly, we aren't trying to scare you off :)
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/10] docs: Describe the 'enable' and 'relay' attributes
On 28/02/13 04:16, Eric Blake wrote:
>>
>> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
>> index 41a83fa..c4c4def 100644
>> --- a/docs/formatnetwork.html.in
>> +++ b/docs/formatnetwork.html.in
>
> Yay - your series added documentation!
I commented to a student that the documentation would make someone's day :)
> But it didn't add any unit tests
> (a new .xml file somewhere under tests/networkxml2argvdata or
> networkxml2xml{in,out} to verify that we can round-trip the new XML and
> generate the expected command line), and it failed to add the RelaxNG
> grammar specification under docs/schemas/network.rng, so there is still
> more to be added.
OK, I'll do those. As you probably realise I'm new to libvirt coding style and
requirements, only touching it to add needed functionality, but hopefully can
make it suitable for give-back to the
project. I'll work up a V2 once all comments are in.
>
> Also, I like to put documentation FIRST in the series, as it then sets
> the stage for what the reviewer will be expecting in the rest of the series.
Makes sense.
>> +Since $TODO.$FIXME it can optionally contain a boolean
>> enable attribute
>
> Instead of using $TODO.$FIXME, just use Since
> 1.0.4. We can later touch that up as part of merging it in if
> you miss the 1.0.4 release window.
OK. I did that deliberately so it would be flagged since I didn't know what the
policy was for unreleased versions and didn't want it to slip through the
cracks.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] maint: fix typo in network docs
* docs/formatnetwork.html.in: Spell variation correctly. --- Pushing under the trivial rule. docs/formatnetwork.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 41a83fa..4dd0415 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -780,7 +780,7 @@ - Below is another IPv6 varition. Instead of a dhcp range being + Below is another IPv6 variation. Instead of a dhcp range being specified, this example has a couple of IPv6 host definitions. Note that most of the dhcp host definitions use an "id" (client id or DUID) since this has proven to be a more reliable way -- 1.8.1.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/10] docs: Describe the 'enable' and 'relay' attributes
On 02/27/2013 07:57 PM, TJ wrote:
> From: TJ
>
> Signed-off-by: TJ
> ---
> docs/formatnetwork.html.in | 22 +-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
> index 41a83fa..c4c4def 100644
> --- a/docs/formatnetwork.html.in
> +++ b/docs/formatnetwork.html.in
Yay - your series added documentation! But it didn't add any unit tests
(a new .xml file somewhere under tests/networkxml2argvdata or
networkxml2xml{in,out} to verify that we can round-trip the new XML and
generate the expected command line), and it failed to add the RelaxNG
grammar specification under docs/schemas/network.rng, so there is still
more to be added.
Also, I like to put documentation FIRST in the series, as it then sets
the stage for what the reviewer will be expecting in the rest of the series.
> @@ -650,12 +650,20 @@
>dhcp
>Also within the ip element there is an
> optional dhcp element. The presence of this element
> -enables DHCP services on the virtual network. It will further
> +enables DHCP services on the virtual network. It can further
> contain one or more range elements. The
> dhcp element supported for both
> IPv4 Since 0.3.0
> and IPv6 Since 1.0.1, but
> only for one IP address of each type per network.
> +Since $TODO.$FIXME it can optionally contain a boolean
> enable attribute
Instead of using $TODO.$FIXME, just use Since
1.0.4. We can later touch that up as part of merging it in if
you miss the 1.0.4 release window.
> +('yes' or 'no') where the value defaults to 'yes', and a boolean
> +relay attribute where the value defaults to 'no'.
> +When relay='yes' any settings within the
> dhcp block
> +are ignored and a DHCP relay agent is started instead of a local
> DHCP server.
> +The DHCP relay daemon will listen on the network's bridge
> interface for
> +DHCP/BOOTP traffic and relay it via broadcast from the first
> interface declared
> +in the forward element.
>
>range
>The start and end attributes on
> the
> @@ -779,6 +787,18 @@
>
> +local
> +
> +
> +
> +
>
>Below is another IPv6 varition. Instead of a dhcp range being
Not your fault, but made obvious by your patch, so I'll fix this typo in
'variation' independently.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/10] Add copyright attribution for DHCP relay functionality
On 02/27/2013 07:57 PM, TJ wrote: > From: TJ > > Signed-off-by: TJ > --- > src/conf/network_conf.c | 1 + > src/network/bridge_driver.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c > index c5eab01..43c277c 100644 > --- a/src/conf/network_conf.c > +++ b/src/conf/network_conf.c > @@ -3,6 +3,7 @@ > * > * Copyright (C) 2006-2013 Red Hat, Inc. > * Copyright (C) 2006-2008 Daniel P. Berrange > + * Copyright (C) 2013 TJ You can't add a copyright without also adding content. This patch needs to be squashed in with the patches that actually add content where you claim copyright. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
On 02/27/2013 07:57 PM, TJ wrote:
> From: TJ
>
> A DHCP relay daemon will be started that will forward all DHCP/BOOTP
> requests on the bridge network via the first declared forward
> interface. The relay is broadcast rather than routed so no IP address
> is needed on the bridge.
>
> +static int
> +networkBuildDhcpRelayCommandLine(virNetworkObjPtr network, virCommandPtr
> *cmdout,
> + char *pidfile)
> +{
> +virCommandPtr cmd = NULL;
> +int ret = -1;
> +
> +cmd = virCommandNew(DHCPRELAY);
Your patches are out of order. This patch tries to use DHCPRELAY, but
that isn't defined until patch 8/10 modifies configure.ac. Squash those
two patches together. Each patch needs to be buildable in isolation.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
On 28/02/13 03:49, Eric Blake wrote: > On 02/27/2013 08:26 PM, TJ wrote: >> On 28/02/13 03:15, Eric Blake wrote: >>> On 02/27/2013 07:18 PM, TJ wrote: Signed-off-by: TJ >>> >>> Can you use a full legal name, instead of a two-letter pseudonym? >> >> TJ is my full name. > > Whatever. It might be the pseudonym you prefer in open source, but I > seriously doubt you come from a culture where your parents did not grant > you a surname. That's a problem with your perception. It is my full legal name as per passport, driving license, etc. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
On 02/27/2013 08:26 PM, TJ wrote: > On 28/02/13 03:15, Eric Blake wrote: >> On 02/27/2013 07:18 PM, TJ wrote: >>> Signed-off-by: TJ >> >> Can you use a full legal name, instead of a two-letter pseudonym? > > TJ is my full name. Whatever. It might be the pseudonym you prefer in open source, but I seriously doubt you come from a culture where your parents did not grant you a surname. > >> >> Your commit message is missing some substance, such as a summary of what >> is being added. > > The first line of the commit message is in the email subject, and describes > the commit. > > "conf: DHCP - add state for DHCP Relay and On/Off switch" The summary is nice, but we also want to see a sample of the XML that you are adding. See, for example, commit 1716e7a6. >> Your patch is whitespace-damaged. Did you sent it with 'git send-email'? > > Yes. It barfed first time and added 9 emails into one post! Second time it > seemed to be OK. I only see 2-10 on the second attempt. I didn't see a resend of 1. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
On 28/02/13 03:38, Eric Blake wrote: > > This indentation mess with TAB is making this hard to review. > It seems vim is the culprit. Default setting seems to be that it's inserting a tab when I hit Enter on a previous line when there's a multiple of 8 spaces of indent on the new line. I had a few arguments with it and thought it was behaving. Ignore the formatting issues for now, deal with the substance. If the substance is acceptable I'll ensure a revised series doesn't choke on hidden formatting. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
On 02/27/2013 07:57 PM, TJ wrote:
> From: TJ
>
> A DHCP relay daemon will be started that will forward all DHCP/BOOTP
> requests on the bridge network via the first declared forward
> interface. The relay is broadcast rather than routed so no IP address
> is needed on the bridge.
>
> The XML element's "relay" property of the active DHCP stanza
> defaults to 'no'. Set it to 'yes' to enable the relay:
>
>
>
>
>
> The relay will not be started if the "enable" property is 'no':
>
>
>
>
At this point in the series, I'll defer to Laine for technical review.
But I still have some style review:
> +
> +/* Use the first forwarding device to broadcast to the upstream DHCP
> server */
> +if (network->def->forward.nifs > 0) {
> +virCommandAddArgList(cmd, "-b",
> network->def->forward.ifs[0].device.dev, NULL);
> + ret = 0;
No TABs in .c, ever. Run 'make syntax-check'.
> +} else
HACKING says that if you use {} on one side of 'else', you must use it
on both sides.
> + virReportSystemError(VIR_ERR_INVALID_INTERFACE,
> + _("DHCP relay requires at least one network %s\n"),
> + " or ");
Indentation is off, even when you get rid of that TAB. You have a
mixed-language sentence - translators will get the first half, but can't
translate the 'or' in the second string, and that looks gross. I would
have written:
virReportSystemError(VIR_ERR_INVALID_INTERFACE, "%s",
_("DHCP relay requires at least one network "
" or "
"");
> +static int
> +networkBuildDhcpRelayCommandLine(virNetworkObjPtr network, virCommandPtr
> *cmdout,
> + char *pidfile)
> +{
> +virCommandPtr cmd = NULL;
> +int ret = -1;
> +
> +cmd = virCommandNew(DHCPRELAY);
> +if (networkBuildDhcpRelayArgv(network, pidfile, cmd) < 0) {
> +goto cleanup;
> +}
> +
> +if (cmdout)
> +*cmdout = cmd;
> +ret = 0;
> +cleanup:
> +if (ret < 0)
> +virCommandFree(cmd);
> +return ret;
Memory leak if ret == 0 but !*cmdout.
> +}
> +
> +static int
> +networkStartDhcpRelayDaemon(struct network_driver *driver ATTRIBUTE_UNUSED,
> + virNetworkObjPtr network)
Indentation is off.
> +{
> +virCommandPtr cmd = NULL;
> +virNetworkIpDefPtr ipdef = NULL;
> +char *pidfile = NULL;
> +char *tmp = NULL;
> +int pid_len;
> +int ret = 0;
> +const char *dhcprelay = "dhcprelay_";
> +
> +ipdef = networkGetActiveDhcp(network);
> +/* Prepare for DHCP relay agent */
> +if (ipdef && ipdef->dhcp_enabled && ipdef->dhcp_relay) {
> + ret = -1;
Indentation is off.
> +
> +if (virFileMakePath(NETWORK_PID_DIR) < 0) {
> +virReportSystemError(errno,
> + _("cannot create directory %s"),
> + NETWORK_PID_DIR);
> +goto cleanup;
> +}
> +
> +pid_len = strlen(dhcprelay) + strlen(network->def->name);
> +if ( VIR_ALLOC_N(tmp, pid_len+1) >= 0) {
Spacing is off. Correct would be:
if (VIR_ALLOC_N(tmp, pid_len + 1) >= 0) {
But you shouldn't need to do VIR_ALLOC_N in the first place, since...
> + tmp = strcpy(tmp, dhcprelay);
> + tmp = strncat(tmp, network->def->name, pid_len);
...strncat() is generally the wrong interface. Use virAsprintf or
virBuffer* instead.
> + if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, tmp))) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +} else {
> + virReportOOMError();
> + goto cleanup;
> + }
This indentation mess with TAB is making this hard to review.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/10] conf: Network - add ability to read/write XML DHCP state
On 28/02/13 03:23, Eric Blake wrote:
> On 02/27/2013 07:57 PM, TJ wrote:
>> From: TJ
>>
>> Maintain backwards XML compatibility by assuming existing default values
>> and only adding the additional XML properties if settings are not
>> default.
>>
>> Signed-off-by: TJ
>> ---
>> src/conf/network_conf.c | 28
>> 1 file changed, 24 insertions(+), 4 deletions(-)
>>
>
>> +def->dhcp_enabled = true;
>> +if ((tmp = virXMLPropString(node, "enabled"))) {
>> +def->dhcp_enabled = strncmp("no", tmp, 2) == 0 ? false :
>> def->dhcp_enabled;
>
> Yuck. This lets a user pass in trailing garbage. Use STREQ, not strncmp.
>
> For that matter, assigning def->dhcp_enabled to itself looks odd. I'd
> probably write:
>
> def->dhcp_enabled = true;
> if ((tmp = virXMLPropString(node, "enabled"))) {
> if (STREQ(tmp, "no"))
> def->dhcp_enabled = false;
> VIR_FREE(tmp);
>
> so that it doesn't look so screwy.
I knew there was probably a better 'libvirt' style but couldn't find examples
when I was looking for them.
>> + !def->dhcp_enabled || def->dhcp_relay) {
>> int ii;
>> -virBufferAddLit(buf, "\n");
>> +virBufferAddLit(buf, "> +if (!def->dhcp_enabled)
>> +virBufferAddLit(buf, " enabled='no'");
>> +if (def->dhcp_relay)
>> +virBufferAddLit(buf, " relay='yes'");
>> +virBufferAddLit(buf, ">\n");
>> +
>> virBufferAdjustIndent(buf, 2);
>>
>> -for (ii = 0 ; ii < def->nranges ; ii++) {
>> +for (ii = 0 ; def->nranges && ii < def->nranges ; ii++) {
>
> This line is a spurious change.
Ahh yes. I introduced that to catch a bug found using gdb when nranges == 0 in
the mistaken thinking that the loop would do at least one iteration. Later I
realised the bug was caused by another issue
entirely but forgot to revert that change.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
On 28/02/13 03:15, Eric Blake wrote:
> On 02/27/2013 07:18 PM, TJ wrote:
>> Signed-off-by: TJ
>
> Can you use a full legal name, instead of a two-letter pseudonym?
TJ is my full name.
>
> Your commit message is missing some substance, such as a summary of what
> is being added.
The first line of the commit message is in the email subject, and describes the
commit.
"conf: DHCP - add state for DHCP Relay and On/Off switch"
>
>> ---
>> src/conf/network_conf.h | 5 +
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
>> index c509915..8400eab 100644
>> --- a/src/conf/network_conf.h
>> +++ b/src/conf/network_conf.h
>> @@ -144,6 +144,10 @@ struct _virNetworkIpDef {
>> char *bootfile;
>> virSocketAddr bootserver;
>> +/* when false no DHCP server is started */
>> +bool dhcp_enabled;
>
> Your patch is whitespace-damaged. Did you sent it with 'git send-email'?
Yes. It barfed first time and added 9 emails into one post! Second time it
seemed to be OK.
>
>> +/* when true ranges are ignored and a DHCP relay-agent started */
>> +bool dhcp_relay;
>> };
>> typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef;
>> @@ -234,6 +238,7 @@ typedef virNetworkObj *virNetworkObjPtr;
>> struct _virNetworkObj {
>> virMutex lock;
>> +pid_t dhcprelayPid;
>> pid_t dnsmasqPid;
>
> Does your series ever allow dnsmasq and dhcprelay to run at the same
> time, or can we use a single pid_t field that covers the mutually
> exclusive choice of which helper is running based on the rest of the config?
>
When local DHCP server services are disabled dnsmasq is still launched since
there are several non-DHCP settings in the generated config.
In my test-bed for these patches dnsmasq and dhcp-helper will be started
alongside each other.
If this series is accepted I was intending to propose adding in the same style used for so that dnsmasq can be
totally disabled if un-needed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/10] conf: Network - add pointers to enabled virNetworkIpDef DHCP settings
On 02/27/2013 07:57 PM, TJ wrote:
> From: TJ
>
> Having previously introduced DHCP enabled and relay state within the
> virNetworkIpDef structure - which can be one of many on each network -
> these pointers allow us to track and easily access the DHCP state for
> IPv4 and IPv6 when setting up the network without having to iterate
> every virNetworkIpDef to find the DHCP state.
This patch is useless in isolation; it's small enough that it is
probably worth squashing in with the first patch to actual use these
members.
>
> Signed-off-by: TJ
> ---
> src/conf/network_conf.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 8400eab..1889c45 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -231,6 +231,8 @@ struct _virNetworkDef {
> virPortGroupDefPtr portGroups;
> virNetDevBandwidthPtr bandwidth;
> virNetDevVlan vlan;
> +virNetworkIpDefPtr ipv4_dhcp;
> +virNetworkIpDefPtr ipv6_dhcp;
> };
>
> typedef struct _virNetworkObj virNetworkObj;
>
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/10] conf: Network - add ability to read/write XML DHCP state
On 02/27/2013 07:57 PM, TJ wrote:
> From: TJ
>
> Maintain backwards XML compatibility by assuming existing default values
> and only adding the additional XML properties if settings are not
> default.
>
> Signed-off-by: TJ
> ---
> src/conf/network_conf.c | 28
> 1 file changed, 24 insertions(+), 4 deletions(-)
>
> +def->dhcp_enabled = true;
> +if ((tmp = virXMLPropString(node, "enabled"))) {
> +def->dhcp_enabled = strncmp("no", tmp, 2) == 0 ? false :
> def->dhcp_enabled;
Yuck. This lets a user pass in trailing garbage. Use STREQ, not strncmp.
For that matter, assigning def->dhcp_enabled to itself looks odd. I'd
probably write:
def->dhcp_enabled = true;
if ((tmp = virXMLPropString(node, "enabled"))) {
if (STREQ(tmp, "no"))
def->dhcp_enabled = false;
VIR_FREE(tmp);
so that it doesn't look so screwy.
> +VIR_FREE(tmp);
> +}
> +
> +def->dhcp_relay = false;
> +if ((tmp = virXMLPropString(node, "relay"))) {
> +def->dhcp_relay = strncmp("yes", tmp, 3) == 0 ? true :
> def->dhcp_relay;
> + VIR_FREE(tmp);
Same comments.
> +}
>
> cur = node->children;
> while (cur != NULL) {
> @@ -2180,12 +2193,19 @@ virNetworkIpDefFormat(virBufferPtr buf,
> virBufferEscapeString(buf, "\n",
>def->tftproot);
> }
> -if ((def->nranges || def->nhosts)) {
> +if ((def->nranges || def->nhosts) ||
As long as you are touching this, you can drop the redundant ().
> + !def->dhcp_enabled || def->dhcp_relay) {
> int ii;
> -virBufferAddLit(buf, "\n");
> +virBufferAddLit(buf, " +if (!def->dhcp_enabled)
> + virBufferAddLit(buf, " enabled='no'");
> + if (def->dhcp_relay)
> + virBufferAddLit(buf, " relay='yes'");
> + virBufferAddLit(buf, ">\n");
> +
> virBufferAdjustIndent(buf, 2);
>
> -for (ii = 0 ; ii < def->nranges ; ii++) {
> +for (ii = 0 ; def->nranges && ii < def->nranges ; ii++) {
This line is a spurious change.
> char *saddr = virSocketAddrFormat(&def->ranges[ii].start);
> if (!saddr)
> goto error;
> @@ -2199,7 +2219,7 @@ virNetworkIpDefFormat(virBufferPtr buf,
> VIR_FREE(saddr);
> VIR_FREE(eaddr);
> }
> -for (ii = 0 ; ii < def->nhosts ; ii++) {
> +for (ii = 0 ; def->nhosts && ii < def->nhosts ; ii++) {
So is this one.
> virBufferAddLit(buf, " if (def->hosts[ii].mac)
> virBufferAsprintf(buf, "mac='%s' ", def->hosts[ii].mac);
>
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
On 02/27/2013 07:18 PM, TJ wrote:
> Signed-off-by: TJ
Can you use a full legal name, instead of a two-letter pseudonym?
Your commit message is missing some substance, such as a summary of what
is being added.
> ---
> src/conf/network_conf.h | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index c509915..8400eab 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -144,6 +144,10 @@ struct _virNetworkIpDef {
> char *bootfile;
> virSocketAddr bootserver;
> +/* when false no DHCP server is started */
> +bool dhcp_enabled;
Your patch is whitespace-damaged. Did you sent it with 'git send-email'?
> +/* when true ranges are ignored and a DHCP relay-agent started */
> +bool dhcp_relay;
> };
> typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef;
> @@ -234,6 +238,7 @@ typedef virNetworkObj *virNetworkObjPtr;
> struct _virNetworkObj {
> virMutex lock;
> +pid_t dhcprelayPid;
> pid_t dnsmasqPid;
Does your series ever allow dnsmasq and dhcprelay to run at the same
time, or can we use a single pid_t field that covers the mutually
exclusive choice of which helper is running based on the rest of the config?
> pid_t radvdPid;
> unsigned int active : 1;
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 08/10] configure: Add DHCPRELAY to the set of external program definitions
From: TJ This variable should name the path to the system's DHCP relay daemon. At this time the expected daemon is "dhcp-helper", a DHCP relay agent from Simon Kelly, author of dnsmasq. The supporting code, however, has been designed to work with any suitable DHCP relay agent. Later patches could allow configuration of the agent's command-line arguments. Currently the chosen agent must support: -b (the virtual network to listen on) -i (the interface to broadcast on) -r (the PID file of the daemon) Signed-off-by: TJ --- configure.ac | 4 1 file changed, 4 insertions(+) diff --git a/configure.ac b/configure.ac index e3a749a..b62170e 100644 --- a/configure.ac +++ b/configure.ac @@ -295,6 +295,8 @@ dnl External programs that we can use if they are available. dnl We will hard-code paths to these programs unless we cannot dnl detect them, in which case we'll search for the program dnl along the $PATH at runtime and fail if it's not there. +AC_PATH_PROG([DHCPRELAY], [dhcp-helper], [dhcp-helper], + [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([DNSMASQ], [dnsmasq], [dnsmasq], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([RADVD], [radvd], [radvd], @@ -314,6 +316,8 @@ AC_PATH_PROG([OVSVSCTL], [ovs-vsctl], [ovs-vsctl], AC_PATH_PROG([SCRUB], [scrub], [scrub], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) +AC_DEFINE_UNQUOTED([DHCPRELAY], ["$DHCPRELAY"], + [Location or name of the dhcp relay-agent program]) AC_DEFINE_UNQUOTED([DNSMASQ],["$DNSMASQ"], [Location or name of the dnsmasq program]) AC_DEFINE_UNQUOTED([RADVD],["$RADVD"], -- 1.8.1.2.433.g9808ce0.dirty -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 10/10] docs: Describe the 'enable' and 'relay' attributes
From: TJ
Signed-off-by: TJ
---
docs/formatnetwork.html.in | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 41a83fa..c4c4def 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -650,12 +650,20 @@
dhcp
Also within the ip element there is an
optional dhcp element. The presence of this element
-enables DHCP services on the virtual network. It will further
+enables DHCP services on the virtual network. It can further
contain one or more range elements. The
dhcp element supported for both
IPv4 Since 0.3.0
and IPv6 Since 1.0.1, but
only for one IP address of each type per network.
+Since $TODO.$FIXME it can optionally contain a boolean
enable attribute
+('yes' or 'no') where the value defaults to 'yes', and a boolean
+relay attribute where the value defaults to 'no'.
+When relay='yes' any settings within the
dhcp block
+are ignored and a DHCP relay agent is started instead of a local
DHCP server.
+The DHCP relay daemon will listen on the network's bridge
interface for
+DHCP/BOOTP traffic and relay it via broadcast from the first
interface declared
+in the forward element.
range
The start and end attributes on the
@@ -779,6 +787,18 @@
+local
+
+
+
+
Below is another IPv6 varition. Instead of a dhcp range being
specified, this example has a couple of IPv6 host definitions.
--
1.8.1.2.433.g9808ce0.dirty
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 07/10] network: Bridge - don't offer dnsmasq DHCP services when DHCP relay is enabled
From: TJ
When dnsmasq's DNS services are required but the network is configured
to use a DHCP relay agent (other than dnsmasq's proxy services) the
configuration generated for dnsmasq should not enable DHCP services.
Signed-off-by: TJ
---
src/network/bridge_driver.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index c02d3de..a4cd727 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -951,11 +951,12 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
ipv4def = ipv6def = NULL;
ipdef = network->def->ipv4_dhcp;
-if (ipdef && (ipdef->nranges || ipdef->nhosts))
+if (ipdef && ipdef->dhcp_enabled && !ipdef->dhcp_relay &&
+(ipdef->nranges || ipdef->nhosts))
ipv4def = ipdef;
ipdef = network->def->ipv6_dhcp;
-if (ipdef) {
+if (ipdef && ipdef->dhcp_enabled && !ipdef->dhcp_relay) {
if (ipdef->nranges || ipdef->nhosts) {
ipv6def = ipdef;
@@ -1266,8 +1267,8 @@ static int
networkRefreshDhcpDaemon(struct network_driver *driver,
virNetworkObjPtr network)
{
-int ret = -1, ii;
-virNetworkIpDefPtr ipdef, ipv4def, ipv6def;
+int ret = -1;
+virNetworkIpDefPtr ipv4def, ipv6def;
dnsmasqContext *dctx = NULL;
/* if no IP addresses specified, nothing to do */
--
1.8.1.2.433.g9808ce0.dirty
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
From: TJ
A DHCP relay daemon will be started that will forward all DHCP/BOOTP
requests on the bridge network via the first declared forward
interface. The relay is broadcast rather than routed so no IP address
is needed on the bridge.
The XML element's "relay" property of the active DHCP stanza
defaults to 'no'. Set it to 'yes' to enable the relay:
The relay will not be started if the "enable" property is 'no':
Signed-off-by: TJ
---
src/network/bridge_driver.c | 146
1 file changed, 146 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 8410c93..c02d3de 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -587,6 +587,145 @@ cleanup:
* which is later saved into a file
*/
+static virNetworkIpDefPtr
+networkGetActiveDhcp(virNetworkObjPtr network)
+{
+virNetworkIpDefPtr dhcp = NULL;
+
+if (network->def && network->def->ipv4_dhcp)
+dhcp = network->def->ipv4_dhcp;
+
+if (!dhcp &&
+network->def && network->def->ipv6_dhcp)
+dhcp = network->def->ipv6_dhcp;
+
+return dhcp;
+}
+
+static int
+networkBuildDhcpRelayArgv(virNetworkObjPtr network,
+const char *pidfile,
+virCommandPtr cmd)
+{
+int ret = -1;
+
+/* PID file */
+virCommandAddArgList(cmd, "-r", pidfile, NULL);
+
+/* Listen for DHCP requests on the bridge interface */
+virCommandAddArgList(cmd, "-i", network->def->bridge, NULL);
+
+/* Use the first forwarding device to broadcast to the upstream DHCP
server */
+if (network->def->forward.nifs > 0) {
+virCommandAddArgList(cmd, "-b",
network->def->forward.ifs[0].device.dev, NULL);
+ ret = 0;
+} else
+ virReportSystemError(VIR_ERR_INVALID_INTERFACE,
+ _("DHCP relay requires at least one network %s\n"),
+ " or ");
+
+return ret;
+}
+
+static int
+networkBuildDhcpRelayCommandLine(virNetworkObjPtr network, virCommandPtr
*cmdout,
+ char *pidfile)
+{
+virCommandPtr cmd = NULL;
+int ret = -1;
+
+cmd = virCommandNew(DHCPRELAY);
+if (networkBuildDhcpRelayArgv(network, pidfile, cmd) < 0) {
+goto cleanup;
+}
+
+if (cmdout)
+*cmdout = cmd;
+ret = 0;
+cleanup:
+if (ret < 0)
+virCommandFree(cmd);
+return ret;
+}
+
+static int
+networkStartDhcpRelayDaemon(struct network_driver *driver ATTRIBUTE_UNUSED,
+ virNetworkObjPtr network)
+{
+virCommandPtr cmd = NULL;
+virNetworkIpDefPtr ipdef = NULL;
+char *pidfile = NULL;
+char *tmp = NULL;
+int pid_len;
+int ret = 0;
+const char *dhcprelay = "dhcprelay_";
+
+ipdef = networkGetActiveDhcp(network);
+/* Prepare for DHCP relay agent */
+if (ipdef && ipdef->dhcp_enabled && ipdef->dhcp_relay) {
+ ret = -1;
+
+if (virFileMakePath(NETWORK_PID_DIR) < 0) {
+virReportSystemError(errno,
+ _("cannot create directory %s"),
+ NETWORK_PID_DIR);
+goto cleanup;
+}
+
+pid_len = strlen(dhcprelay) + strlen(network->def->name);
+if ( VIR_ALLOC_N(tmp, pid_len+1) >= 0) {
+ tmp = strcpy(tmp, dhcprelay);
+ tmp = strncat(tmp, network->def->name, pid_len);
+ if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, tmp))) {
+ virReportOOMError();
+ goto cleanup;
+ }
+} else {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ret = networkBuildDhcpRelayCommandLine(network, &cmd, pidfile);
+if (ret < 0)
+ goto cleanup;
+
+ret = virCommandRun(cmd, NULL);
+if (ret < 0)
+ goto cleanup;
+
+ret = virPidFileRead(NETWORK_PID_DIR, pidfile, &network->dhcprelayPid);
+if (ret < 0)
+ virReportSystemError(errno, _("%s is not running"), DHCPRELAY);
+
+cleanup:
+VIR_FREE(tmp);
+VIR_FREE(pidfile);
+virCommandFree(cmd);
+}
+return ret;
+}
+
+static int
+networkRestartDhcpRelayDaemon(struct network_driver *driver,
+ virNetworkObjPtr network)
+{
+/* if there is a running DHCP relay agent, kill it */
+if (network->dhcprelayPid > 0) {
+networkKillDaemon(network->dhcprelayPid, DHCPRELAY,
+ network->def->name);
+network->dhcprelayPid = -1;
+}
+/* now start the daemon if it should be started */
+return networkStartDhcpRelayDaemon(driver, network);
+}
+
+static int
+networkRefreshDhcpRelayDaemon(struct network_driver *driver,
+ virNetworkObjPtr network)
+{
+return networkRestartDhcpRelayDaemon(driver, network);
+}
+
static int
networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx,
virNetwo
[libvirt] [PATCH 09/10] Add copyright attribution for DHCP relay functionality
From: TJ Signed-off-by: TJ --- src/conf/network_conf.c | 1 + src/network/bridge_driver.c | 1 + 2 files changed, 2 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index c5eab01..43c277c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3,6 +3,7 @@ * * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange + * Copyright (C) 2013 TJ * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a4cd727..1667ae6 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4,6 +4,7 @@ * * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange + * Copyright (C) 2013 TJ * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public -- 1.8.1.2.433.g9808ce0.dirty -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 02/10] conf: Network - add ability to read/write XML DHCP state
From: TJ
Maintain backwards XML compatibility by assuming existing default values
and only adding the additional XML properties if settings are not
default.
Signed-off-by: TJ
---
src/conf/network_conf.c | 28
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 3fc01cf..259de0a 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -828,6 +828,19 @@ virNetworkDHCPDefParseXML(const char *networkName,
{
xmlNodePtr cur;
+char *tmp = NULL;
+
+def->dhcp_enabled = true;
+if ((tmp = virXMLPropString(node, "enabled"))) {
+def->dhcp_enabled = strncmp("no", tmp, 2) == 0 ? false :
def->dhcp_enabled;
+VIR_FREE(tmp);
+}
+
+def->dhcp_relay = false;
+if ((tmp = virXMLPropString(node, "relay"))) {
+def->dhcp_relay = strncmp("yes", tmp, 3) == 0 ? true : def->dhcp_relay;
+ VIR_FREE(tmp);
+}
cur = node->children;
while (cur != NULL) {
@@ -2180,12 +2193,19 @@ virNetworkIpDefFormat(virBufferPtr buf,
virBufferEscapeString(buf, "\n",
def->tftproot);
}
-if ((def->nranges || def->nhosts)) {
+if ((def->nranges || def->nhosts) ||
+ !def->dhcp_enabled || def->dhcp_relay) {
int ii;
-virBufferAddLit(buf, "\n");
+virBufferAddLit(buf, "dhcp_enabled)
+ virBufferAddLit(buf, " enabled='no'");
+ if (def->dhcp_relay)
+ virBufferAddLit(buf, " relay='yes'");
+ virBufferAddLit(buf, ">\n");
+
virBufferAdjustIndent(buf, 2);
-for (ii = 0 ; ii < def->nranges ; ii++) {
+for (ii = 0 ; def->nranges && ii < def->nranges ; ii++) {
char *saddr = virSocketAddrFormat(&def->ranges[ii].start);
if (!saddr)
goto error;
@@ -2199,7 +2219,7 @@ virNetworkIpDefFormat(virBufferPtr buf,
VIR_FREE(saddr);
VIR_FREE(eaddr);
}
-for (ii = 0 ; ii < def->nhosts ; ii++) {
+for (ii = 0 ; def->nhosts && ii < def->nhosts ; ii++) {
virBufferAddLit(buf, "hosts[ii].mac)
virBufferAsprintf(buf, "mac='%s' ", def->hosts[ii].mac);
--
1.8.1.2.433.g9808ce0.dirty
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 05/10] network: Bridge - use IPv4 and IPv6 active DHCP stanza pointers
From: TJ
Rather than iterate through virNetworkIPDef arrays multiple times
use the new virNetworkDef ipv4_dhcp and ipv6_dhcp active stanza
pointers.
Signed-off-by: TJ
---
src/network/bridge_driver.c | 63 +++--
1 file changed, 15 insertions(+), 48 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 0932cf8..8410c93 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -810,24 +810,16 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
}
}
-/* Find the first dhcp for both IPv4 and IPv6 */
-for (ii = 0, ipv4def = NULL, ipv6def = NULL, ipv6SLAAC = false;
- (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
- ii++) {
-if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) {
-if (ipdef->nranges || ipdef->nhosts) {
-if (ipv4def) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("For IPv4, multiple DHCP definitions "
- "cannot be specified."));
-goto cleanup;
-} else {
-ipv4def = ipdef;
-}
-}
-}
-if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) {
-if (ipdef->nranges || ipdef->nhosts) {
+ipv4def = ipv6def = NULL;
+ipdef = network->def->ipv4_dhcp;
+if (ipdef && (ipdef->nranges || ipdef->nhosts))
+ipv4def = ipdef;
+
+ipdef = network->def->ipv6_dhcp;
+if (ipdef) {
+if (ipdef->nranges || ipdef->nhosts) {
+ipv6def = ipdef;
+
if (!DNSMASQ_DHCPv6_SUPPORT(caps)) {
unsigned long version = dnsmasqCapsGetVersion(caps);
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -842,18 +834,9 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
DNSMASQ_DHCPv6_MINOR_REQD);
goto cleanup;
}
-if (ipv6def) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("For IPv6, multiple DHCP definitions "
- "cannot be specified."));
-goto cleanup;
-} else {
-ipv6def = ipdef;
-}
-} else {
-ipv6SLAAC = true;
-}
-}
+ }
+} else {
+ipv6SLAAC = true;
}
if (ipv6def && ipv6SLAAC) {
@@ -1160,25 +1143,9 @@ networkRefreshDhcpDaemon(struct network_driver *driver,
if (!(dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR)))
goto cleanup;
-/* Look for first IPv4 address that has dhcp defined.
- * We only support dhcp-host config on one IPv4 subnetwork
- * and on one IPv6 subnetwork.
- */
-ipv4def = NULL;
-for (ii = 0;
- (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
- ii++) {
-if (!ipv4def && (ipdef->nranges || ipdef->nhosts))
-ipv4def = ipdef;
-}
+ipv4def = network->def->ipv4_dhcp;
-ipv6def = NULL;
-for (ii = 0;
- (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, ii));
- ii++) {
-if (!ipv6def && (ipdef->nranges || ipdef->nhosts))
-ipv6def = ipdef;
-}
+ipv6def = network->def->ipv6_dhcp;
if (ipv4def && (networkBuildDnsmasqDhcpHostsList(dctx, ipv4def) < 0))
goto cleanup;
--
1.8.1.2.433.g9808ce0.dirty
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 03/10] conf: Network - add pointers to enabled virNetworkIpDef DHCP settings
From: TJ
Having previously introduced DHCP enabled and relay state within the
virNetworkIpDef structure - which can be one of many on each network -
these pointers allow us to track and easily access the DHCP state for
IPv4 and IPv6 when setting up the network without having to iterate
every virNetworkIpDef to find the DHCP state.
Signed-off-by: TJ
---
src/conf/network_conf.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 8400eab..1889c45 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -231,6 +231,8 @@ struct _virNetworkDef {
virPortGroupDefPtr portGroups;
virNetDevBandwidthPtr bandwidth;
virNetDevVlan vlan;
+virNetworkIpDefPtr ipv4_dhcp;
+virNetworkIpDefPtr ipv6_dhcp;
};
typedef struct _virNetworkObj virNetworkObj;
--
1.8.1.2.433.g9808ce0.dirty
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 04/10] conf: Network - keep track of active DHCP stanza in virNetworkDef
From: TJ To avoid iterating all virNetworkIpDef entries when determining DHCP state keep track of the first enabled DHCP stanza in the network definition itself, for both IPv4 and IPv6. A by-product of this change is it allows the XML to contain more than one IP->DHCP stanza. The active DHCP stanza is the first enabled DHCP stanza. All other stanzas are retained which adds flexibility when multiple interfaces and routes might come and go since an alternative DHCP stanza can be selected and a refresh operation performed without needing to destroy/edit/start the network. Signed-off-by: TJ --- src/conf/network_conf.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 259de0a..c5eab01 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1882,6 +1882,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) if (ret < 0) goto error; def->nips++; + /* use only the first enabled DHCP definition */ + if (!def->ipv4_dhcp && def->ips[ii].dhcp_enabled && + VIR_SOCKET_ADDR_IS_FAMILY(&def->ips[ii].address, AF_INET)) + def->ipv4_dhcp = &def->ips[ii]; + if (!def->ipv6_dhcp && def->ips[ii].dhcp_enabled && + VIR_SOCKET_ADDR_IS_FAMILY(&def->ips[ii].address, AF_INET6)) + def->ipv6_dhcp = &def->ips[ii]; } } VIR_FREE(ipNodes); -- 1.8.1.2.433.g9808ce0.dirty -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 02/10] conf: Network - add ability to read/write XML DHCP state
From: "TJ"
Maintain backwards XML compatibility by assuming existing default values
and only adding the additional XML properties if settings are not
default.
Signed-off-by: TJ
---
src/conf/network_conf.c | 28
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 3fc01cf..259de0a 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -828,6 +828,19 @@ virNetworkDHCPDefParseXML(const char *networkName,
{
xmlNodePtr cur;
+char *tmp = NULL;
+
+def->dhcp_enabled = true;
+if ((tmp = virXMLPropString(node, "enabled"))) {
+def->dhcp_enabled = strncmp("no", tmp, 2) == 0 ? false :
def->dhcp_enabled;
+VIR_FREE(tmp);
+}
+
+def->dhcp_relay = false;
+if ((tmp = virXMLPropString(node, "relay"))) {
+def->dhcp_relay = strncmp("yes", tmp, 3) == 0 ? true : def->dhcp_relay;
+ VIR_FREE(tmp);
+}
cur = node->children;
while (cur != NULL) {
@@ -2180,12 +2193,19 @@ virNetworkIpDefFormat(virBufferPtr buf,
virBufferEscapeString(buf, "\n",
def->tftproot);
}
-if ((def->nranges || def->nhosts)) {
+if ((def->nranges || def->nhosts) ||
+ !def->dhcp_enabled || def->dhcp_relay) {
int ii;
-virBufferAddLit(buf, "\n");
+virBufferAddLit(buf, "dhcp_enabled)
+ virBufferAddLit(buf, " enabled='no'");
+ if (def->dhcp_relay)
+ virBufferAddLit(buf, " relay='yes'");
+ virBufferAddLit(buf, ">\n");
+
virBufferAdjustIndent(buf, 2);
-for (ii = 0 ; ii < def->nranges ; ii++) {
+for (ii = 0 ; def->nranges && ii < def->nranges ; ii++) {
char *saddr = virSocketAddrFormat(&def->ranges[ii].start);
if (!saddr)
goto error;
@@ -2199,7 +2219,7 @@ virNetworkIpDefFormat(virBufferPtr buf,
VIR_FREE(saddr);
VIR_FREE(eaddr);
}
-for (ii = 0 ; ii < def->nhosts ; ii++) {
+for (ii = 0 ; def->nhosts && ii < def->nhosts ; ii++) {
virBufferAddLit(buf, "hosts[ii].mac)
virBufferAsprintf(buf, "mac='%s' ", def->hosts[ii].mac);
--
1.8.1.2.433.g9808ce0.dirty
>From 79a655340f1febc7c35ea4e0a7e0f30ec03e4795 Mon Sep 17 00:00:00 2001
In-Reply-To: <512e724a.10...@iam.tj>
References: <512e724a.10...@iam.tj>
From: "TJ"
Date: Tue, 26 Feb 2013 17:11:52 +
Subject: [PATCH 03/10] conf: Network - add pointers to enabled virNetworkIpDef
DHCP settings
To: libvir-list@redhat.com
Having previously introduced DHCP enabled and relay state within the
virNetworkIpDef structure - which can be one of many on each network -
these pointers allow us to track and easily access the DHCP state for
IPv4 and IPv6 when setting up the network without having to iterate
every virNetworkIpDef to find the DHCP state.
Signed-off-by: TJ
---
src/conf/network_conf.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 8400eab..1889c45 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -231,6 +231,8 @@ struct _virNetworkDef {
virPortGroupDefPtr portGroups;
virNetDevBandwidthPtr bandwidth;
virNetDevVlan vlan;
+virNetworkIpDefPtr ipv4_dhcp;
+virNetworkIpDefPtr ipv6_dhcp;
};
typedef struct _virNetworkObj virNetworkObj;
--
1.8.1.2.433.g9808ce0.dirty
>From 5ce7f49d7e0ebf0eeb9595d305c2b70895f4e4db Mon Sep 17 00:00:00 2001
In-Reply-To: <512e724a.10...@iam.tj>
References: <512e724a.10...@iam.tj>
From: "TJ"
Date: Tue, 26 Feb 2013 17:14:36 +
Subject: [PATCH 04/10] conf: Network - keep track of active DHCP stanza in
virNetworkDef
To: libvir-list@redhat.com
To avoid iterating all virNetworkIpDef entries when determining
DHCP state keep track of the first enabled DHCP stanza in the
network definition itself, for both IPv4 and IPv6.
A by-product of this change is it allows the XML to contain more
than one IP->DHCP stanza. The active DHCP stanza is the first enabled
DHCP stanza.
All other stanzas are retained which adds flexibility when multiple
interfaces and routes might come and go since an alternative DHCP
stanza can be selected and a refresh operation performed without
needing to destroy/edit/start the network.
Signed-off-by: TJ
---
src/conf/network_conf.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 259de0a..c5eab01 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1882,6 +1882,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
if (ret < 0)
goto error;
def->nips++;
+ /* use only the first enabled DHCP definition */
+ if (!def->ipv4_dhcp && def->ips[ii].dhcp_enabled &&
+ VIR_SOCKET_ADDR_IS_FAMILY(&def->ips[ii].address, AF_INET))
+ def->ipv4_dhcp = &def->ips[ii];
+
Re: [libvirt] [PATCH 1/1] Clear PIIX3/PIIX4_USB capabilities for non-X86 platforms
On Thu, Feb 28, 2013 at 10:06 AM, Li Zhang wrote:
> I also hope that QEMU capabilities depend on the binary by QMP.
> But the flags in virQEMUCapsObjectTypes are all set in virQEMUCapsInitQMP.
>
> virQEMUCapsInitQMP -> virQEMUCapsProbeQMPObjects ->
> virQEMUCapsProcessStringFlags(qemuCaps,
>
> ARRAY_CARDINALITY(virQEMUCapsObjectTypes),
> virQEMUCapsObjectTypes,
> nvalues, values);
>
>
More information from QEMU:
I tried to execute "qom-list-types" command, I get a lot of return values
including X86 and other platforms.
So this results that most flags may be set in this function.
More comments?
Thanks.
>
>
So, it is not reasonable to set all of these flags for every platform.
> This is a problem for other non-x86 platforms.
>
> I saw that capabilities changes a lot since I used 0.10.2 before.
>
> Could you help look into the code to see this problem?
>
> Thanks a lot. :)
> -Li
>
>
> On Wed, Feb 27, 2013 at 9:36 PM, Jiri Denemark wrote:
>
>> On Wed, Feb 27, 2013 at 19:52:22 +0800, Li Zhang wrote:
>> > From: Li Zhang
>> >
>> > Currently, PIIX3/PIIX4_USB capabilities are enabled for other platforms.
>> > Actually, it is only supported for X86.
>> >
>> > So this patch is to clear the capabilities for non-X86 platforms.
>> >
>> ...
>> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> > index 40022c1..ef5c69a 100644
>> > --- a/src/qemu/qemu_capabilities.c
>> > +++ b/src/qemu/qemu_capabilities.c
>> > @@ -1307,8 +1307,11 @@ struct virQEMUCapsStringFlags
>> virQEMUCapsObjectTypes[] = {
>> > { "hda-micro", QEMU_CAPS_HDA_MICRO },
>> > { "ccid-card-emulated", QEMU_CAPS_CCID_EMULATED },
>> > { "ccid-card-passthru", QEMU_CAPS_CCID_PASSTHRU },
>> > +#if defined (__x86_64__) || \
>> > +defined (__i386__)
>> > { "piix3-usb-uhci", QEMU_CAPS_PIIX3_USB_UHCI },
>> > { "piix4-usb-uhci", QEMU_CAPS_PIIX4_USB_UHCI },
>> > +#endif
>> > { "usb-ehci", QEMU_CAPS_USB_EHCI },
>> > { "ich9-usb-ehci1", QEMU_CAPS_ICH9_USB_EHCI1 },
>> > { "vt82c686b-usb-uhci", QEMU_CAPS_VT82C686B_USB_UHCI },
>>
>> NACK. QEMU capabilities depend on the binary we are going to use
>> (emulator tag in domain XML), they don't depend on host architecture.
>>
>> Jirka
>>
>
>
>
> --
>
> Best Regards
> -Li
>
--
Best Regards
-Li
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
Signed-off-by: TJ
---
src/conf/network_conf.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index c509915..8400eab 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -144,6 +144,10 @@ struct _virNetworkIpDef {
char *bootfile;
virSocketAddr bootserver;
+/* when false no DHCP server is started */
+bool dhcp_enabled;
+/* when true ranges are ignored and a DHCP relay-agent started */
+bool dhcp_relay;
};
typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef;
@@ -234,6 +238,7 @@ typedef virNetworkObj *virNetworkObjPtr;
struct _virNetworkObj {
virMutex lock;
+pid_t dhcprelayPid;
pid_t dnsmasqPid;
pid_t radvdPid;
unsigned int active : 1;
--
1.8.1.2.433.g9808ce0.dirty
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Clear PIIX3/PIIX4_USB capabilities for non-X86 platforms
I also hope that QEMU capabilities depend on the binary by QMP.
But the flags in virQEMUCapsObjectTypes are all set in virQEMUCapsInitQMP.
virQEMUCapsInitQMP -> virQEMUCapsProbeQMPObjects ->
virQEMUCapsProcessStringFlags(qemuCaps,
ARRAY_CARDINALITY(virQEMUCapsObjectTypes),
virQEMUCapsObjectTypes,
nvalues, values);
So, it is not reasonable to set all of these flags for every platform.
This is a problem for other non-x86 platforms.
I saw that capabilities changes a lot since I used 0.10.2 before.
Could you help look into the code to see this problem?
Thanks a lot. :)
-Li
On Wed, Feb 27, 2013 at 9:36 PM, Jiri Denemark wrote:
> On Wed, Feb 27, 2013 at 19:52:22 +0800, Li Zhang wrote:
> > From: Li Zhang
> >
> > Currently, PIIX3/PIIX4_USB capabilities are enabled for other platforms.
> > Actually, it is only supported for X86.
> >
> > So this patch is to clear the capabilities for non-X86 platforms.
> >
> ...
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 40022c1..ef5c69a 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -1307,8 +1307,11 @@ struct virQEMUCapsStringFlags
> virQEMUCapsObjectTypes[] = {
> > { "hda-micro", QEMU_CAPS_HDA_MICRO },
> > { "ccid-card-emulated", QEMU_CAPS_CCID_EMULATED },
> > { "ccid-card-passthru", QEMU_CAPS_CCID_PASSTHRU },
> > +#if defined (__x86_64__) || \
> > +defined (__i386__)
> > { "piix3-usb-uhci", QEMU_CAPS_PIIX3_USB_UHCI },
> > { "piix4-usb-uhci", QEMU_CAPS_PIIX4_USB_UHCI },
> > +#endif
> > { "usb-ehci", QEMU_CAPS_USB_EHCI },
> > { "ich9-usb-ehci1", QEMU_CAPS_ICH9_USB_EHCI1 },
> > { "vt82c686b-usb-uhci", QEMU_CAPS_VT82C686B_USB_UHCI },
>
> NACK. QEMU capabilities depend on the binary we are going to use
> (emulator tag in domain XML), they don't depend on host architecture.
>
> Jirka
>
--
Best Regards
-Li
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 1/3] security_dac: Remember owner prior chown() and restore on relabel
On 02/27/2013 02:25 AM, Michal Privoznik wrote: >> Are you really planning on storing a string uid:gid? Wouldn't it be >> simpler to store a uid_t and gid_t as read from struct stat, as long as >> the data is only in memory? And when storing the data to disk in XML to >> survive libvirtd restarts, it seems like storing two attributes >> user='nnn' group='nnn' is nicer than storing one attribute >> owner='+nnn:+nnn' that requires further parsing back into user and group. > > My idea is; store userName:groupName whenever possible; When one of them > is not accessible, use +NNN instead. The rationale is - whenever a user > or a gropu changes its ID, we will follow it. For instance, a file X has > owner A:B (1:1) but libvirt chowns to C:D. Meanwhile A's ID is changed > from 1 to 2. So when relabeling we should chown to 2:1 instead of 1:1 as > A's ID is 2 not 1. That means I have to do parsing and all the > virAsprintf magic. However, maybe this is not what we want and I should > really remember just numeric values of IDs which has nice tradeoff - > much simpler code. Migration and shared storage makes this problem so much tougher - the uid on shared storage is common, but the name is not necessarily common. I'd go for uid only; leave name lookups to each local machine connecting to shared storage, but don't store the name ourselves. Besides, admins tend not to change the name associated with a uid all that frequently (it's generally one-time setup). Dan has a point that you really need to involve the lock manager or use some persistent storage (extended attribute or additional file in the storage pool) alongside the file whose attributes we want to remember - if a file is on shared storage, then it is the responsibility of the last machine using the image to restore permissions, even if it was not the machine that first did a chmod(). Merely storing a hash in just a single libvirtd instance won't scale. Does our virlockd interface support attaching attributes to a file as part of locking it? >> How is this information is preserved across a libvirtd restart? You'll >> need code that outputs private XML on the save side, then parses it back >> on the load side - is that later in the series? >> > > Yes. That is what the next two patches do. Moreover, the design may look > slightly like an overkill for now, but it is just for easier > implementation for migrating the internal state of security driver. I'm not quite sure I follow your plans for migration of security driver state. We migrate domains, not drivers - but you are storing persistent attributes on a driver, not per-domain state. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Compile libvirt V1.0.1 error
On Thu, Feb 28, 2013 at 12:47 AM, Eric Blake wrote:
[...]
> What a waste of resources - that forks a shell to do an echo every time
> that you use ${JVM_DIR}. Much simpler to write as:
>
> JVM_DIR=${JAVA_HOME}
>
I am not familiar with automake so i just use shell to finish this job.
I will test the way, 'JVM_DIR=${JAVA_HOME}', you said above.
> I'm still not even sure why you need ${JVM_DIR}, and can't just use
> ${JAVA_HOME} directly, nor why you are trying to add pieces of Java into
> libvirt (libvirt itself is written in C, not Java, for a reason).
HLFS is HDFS-based Log-Structured File System in user space.
We call some interfaces from HDFS(libhdfs.so) which is written
by JAVA. See http://code.google.com/p/cloudxy/wiki/WHAT_IS_CLOUDXY
for details ;-)
>I guess I'll just have to wait until I see a patch to understand what this
> is all about. Good luck.
We will submit HLFS driver patches to Libvirt community. Actually, before
submitting HLFS driver patches to Libvirt community, we have to sumbit
HLFS driver patches to QEMU community. I split our HLFS driver patches
into two pieces which one is for Online storage stuffs and the other one is
for Offline storage stuffs.
Online patches:
http://cloudxy.googlecode.com/svn/branches/hlfs/person/harry/hlfs/patches/hlfs_driver_for_libvirt_network_disk.patch
Offline patches:
http://cloudxy.googlecode.com/svn/branches/hlfs/person/harry/hlfs/patches/hlfs_driver_for_libvirt_offline_storage.patch
Eric, could you please give me some suggestions about merging
our HLFS patches into Libvirt community. Thanks in advance ;-)
--
Thanks
Harry Wei
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Don't fail to shutdown domains with unresponsive agent
On 02/27/2013 03:30 AM, Michal Privoznik wrote: > On 27.02.2013 00:22, Eric Blake wrote: >> On 02/26/2013 04:02 AM, Michal Privoznik wrote: >>> Currently, qemuDomainShutdownFlags() chooses the agent method of >>> shutdown whenever the agent is configured. However, this >>> assumption is not enough as the guest agent may be unresponsive >>> at the moment. So unless guest agent method has been explicitly >>> requested, we should fall back to the ACPI method. >>> --- >>> >>> diff to v1: >>> - Rework some conditions as Eric suggested in v1 >>> >>> src/qemu/qemu_driver.c | 38 ++ >>> 1 file changed, 22 insertions(+), 16 deletions(-) >> >> ACK. >> > > Do you think this one is safe to push now? > It is a bug fix and I think the code is well understood. However, the > issue it's fixing doesn't seem to be so usual to hit. Otherwise there > would be a bug report much sooner than 2012-12-22, right? > > https://bugzilla.redhat.com/show_bug.cgi?id=889635 I've hit the bug myself, but never filed a BZ - it's quite annoying that the shutdown button in virt-manager fails to work if you have the guest agent wired up in the host XML, but not installed and running in the guest, at which point you can no longer shut down the guest using virt-manager. Yes, I think this is safe to push for 1.0.3. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Dnsmasq-discuss] Chaining instances?
2013/2/28 Laine Stump > On 02/25/2013 09:08 PM, Gao Yongwei wrote: > > > > I thinks it's better that if we can put dnsmasq args or options in a > > conf file, so we can do some custom through this conf file. > > I've added a Bug 913446 in redhat bugzilla,but seems no one take care > > of this bug? > > This has been discussed extensively on the list before, and we > specifically *don't* want to do it. Gene Czarcinski even submitted a > patch that would do it, and that patch was rejected (and I *think* he > agreed with our reasoning :-) > > The problem is that when you allow a user to silently subvert the config > that is shown in libvirt's XML, and the system stops working, the user > will send a plea for help to irc/mailing list (or open a ticket with > their Linux support vendor), and the people they ask for support will > say "show us the output of 'virsh net-dumpxml mynetwork'", which they > will send, and then a comedy of errors will ensue until someone finally > realizes that there is some "extra" configuration that the user isn't > telling us about. > > There are two solutions to that: > > 1) add an element for the specific option you want to control in > libvirt's network XML. Some knobs are already there, and others are > being added. > > 2) add a private "dnsmasq" namespace to libvirt's network xml, with > provisions for directly passing through dnsmasq commandline options from > the xml to the conf file. This would be similar to what has already been > done for qemu: http://libvirt.org/drvqemu.html#qemucommand > > The difference between these and the idea of simply allowing a > user-written conf file is that everything about the network's config > would then be available in "virsh net-dumpxml $netname". > > As far as the bug you've filed, it takes awhile for bugs to be triaged. > (At a first glance, it seems reasonable to add such an option, since it > is a standard part of the dhcp protocol. We might need to do something > about specifying different units for the lease time.) I 'd better like 2nd solution,because as time goes by,people may need to use tons of different options of dnsmasq to meet new requirments. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] q35 machine type and libvirt.
Alex Williamson writes: > On Wed, 2013-02-27 at 13:20 -0500, Laine Stump wrote: >> On 02/06/2013 02:13 PM, Laine Stump wrote: >> > Now that qemu is getting the q35 machine type, libvirt needs to support it. >> >> In an attempt to make sure that libvirt actually does something useful >> with qemu's new machine type, I'm revisiting this topic and trying to >> get a more thorough understanding. >> >> I've developed my own list of what I *think* are the reasons for wanting >> a new machine type in qemu (and from that it should be more apparent >> what libvirt needs to do with it), and am wondering how far off the mark >> I am: >> > > Right, somehow libvirt needs to know or qemu needs to tell it something > about the devices it's plugging in. If you were to grab your trust > 10/100Mbps Legacy PCI ethernet card and try to plug it into a > motherboard you'd quickly notice that you can only plug it into certain > slots. This is the same problem. PCI device are attached to a legacy > PCI bus, which means it needs to be behind a PCIe-to-PCI bridge. Legacy > Endpoints and Endpoints are plugin PCIe cards, so they need to be > plugged in behind a PCIe switch or root port. Integrated Endpoints are > motherboard components, per the spec, they shouldn't even really be > hotplug-able. They attach directly to the root complex. We could do this with QOM. The chipset could have a set of link properties for the integrated devices. For instance: Q35Chipset Link integrated_nic; Link integrated_vga; ... We should prevent PCI bus plugging for slots "owned" by integrated devices. libvirt has a way to probe for links that it can add including what types are allowed for it. Regards, Anthony Liguori > >>* support some new emulated chipset devices? > > -M q35 + -device ioh3420 (root port) + -device i82801b11-bridge > (pcie-to-pci bridge) > >>* Anything else specific required to passthrough pcie devices? > > I just sent out an RFC asking about VGA routing, it's possible libvirt > will need to understand some concept of a primary VGA device and specify > it somehow to QEMU. > > It's also possible (probable) that we'll make assigned devices able to > mangle PCIe types to make it more flexible where they can be attached > (for instance you may not want to have your HD audio device exposed as a > root complex device if that disables hotplug, so we may mangle it to > look like a regular endpoint. Windows is picky about root complex > devices, so we may also mangle endpoint to integrated endpoint as well). > > Also need to be aware that all PCIe "buses" except the root complex are > actually point-to-point links, so a root port connects to one PCIe > device (which may be multifunction). PCIe switches are needed to get > fan out to connect multiple devices, one device per downstream port. It > would be interesting how much we can abuse the topology in a virtual > system, but it's probably best not to confuse guests. Thanks, > > Alex -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Fix starting qemu instances when apparmor driver is enabled
With the apparmor security driver enabled, qemu instances fail
to start
# grep ^security_driver /etc/libvirt/qemu.conf
security_driver = "apparmor"
# virsh start test-kvm
error: Failed to start domain test-kvm
error: internal error security label already defined for VM
The model field of virSecurityLabelDef object is always populated
by virDomainDefGetSecurityLabelDef(), so remove the check for a
NULL model when verifying if a label is already defined for the
instance.
Checking for a NULL model and populating it later in
AppArmorGenSecurityLabel() has been left in the code to be
consistent with virSecuritySELinuxGenSecurityLabel().
---
src/security/security_apparmor.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index ddc1fe4..2e6a57f 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -436,8 +436,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
return rc;
}
-if ((secdef->label) ||
-(secdef->model) || (secdef->imagelabel)) {
+if (secdef->label || secdef->imagelabel) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s",
_("security label already defined for VM"));
@@ -461,8 +460,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
goto err;
}
-secdef->model = strdup(SECURITY_APPARMOR_NAME);
-if (!secdef->model) {
+if (!secdef->model && !(secdef->model = strdup(SECURITY_APPARMOR_NAME))) {
virReportOOMError();
goto err;
}
--
1.8.0.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] qemu: fix graphics port allocation
On 02/27/2013 04:51 AM, Ján Tomko wrote:
> Only release ports that have been allocated before.
>
> This fixes these issues:
> * trying to release ports when qemuProcessStart fails before port
> allocation
> * trying to release the SPICE TLS port if spice_tls is 0
> * failing to release SPICE port with autoport=off (when only one
> of them is -1)
> ---
> v1: https://www.redhat.com/archives/libvir-list/2013-February/msg01464.html
> Use a pair of booleans in domain private data instead of a new field
> in the domain definition.
Closer, but where is this information saved across libvirtd restarts?
Hint: qemu_domain.c:qemuDomainObjPrivateXML{Format,Parse}()
I think this counts as a bug fix, so it's worth trying to get a v3 in
time for the 1.0.3 release.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Remove contiguous CPU indexes assumption
On 02/27/2013 05:13 AM, Li Zhang wrote:
> Hi Eric,
>
> This should belong to bug-fix, could it be pushed to 1.0.3?
Without any test case under 'make check' that exposes the failure, I'm a
bit worried that this might cause regressions on other setups. I'm not
seven sure I even understand the scope of the problem. Is it something
specific to running a qemu-system-ppc64 emulator (but can be reproduced
on any host architecture), or is it specific to running on a powerpc
host (but happens for any version of qemu-* target architecture), or is
it something else altogether?
We have test framework in place to allow replaying of a QMP JSON
response and seeing how qemu_monitor_json.c deals with it; what I'd
really like to see is a side-by-side comparison of what the QMP
'query-cpus' command returns for a guest with multiple vcpus on a setup
where you are seeing the problem, when compared to a setup that does not
have the issue. You can get at this with virsh qemu-monitor-command
$dom '{"execute":"query-cpus"}', if that helps.
To help you out, here's what I got for a guest using 3 vcpus on my
x86_64 host machine and using the qemu-kvm binary:
# virsh qemu-monitor-command guest '{"execute":"query-cpus"}'
{"return":[{"current":true,"CPU":0,"pc":1025829,"halted":false,"thread_id":5849},{"current":false,"CPU":1,"pc":1005735,"halted":true,"thread_id":5850},{"current":false,"CPU":2,"pc":1005735,"halted":true,"thread_id":5851}],"id":"libvirt-9"}
That is, your patch might be right, but I have not yet been convinced
that it is right; and while things may currently be broken on ppc, it is
not a recent breakage, so being conservative and either proving the fix
is right (via testsuite addition) or deferring the fix until post 1.0.3
seems like safer alternatives. Or maybe someone else will chime in and
agree to take it now, without waiting for my desired burden of proof.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] DHCP Relay agent functionality
On 02/27/2013 01:53 PM, TJ wrote: > Recently I needed to implement functionality to operate a DHCP relay instead > of dnsmasq's DHCP server > on the virtual networks managed by libvirt. > > The primary benefit of operating a relay is that the central DHCP and DNS > servers for the physical > network can manage IP address allocation and hostname resolution for the > entire network and, potentially, expose VMs via hostname and IP address > on the public network too, as well as being able to monitor VM instance > creation for resource planning. > > I originally implemented this functionality on libvirt 0.9.8 included with > Ubuntu 12.04 as a hack to simply disable dnsmasq entirely and > use dhcp-helper[1] (the DHCP relay written by Simon Kelley, author of dnsmaq) > although I've constructed it so that another relay-agent could be used. After > proving it works well I've reworked the code > on the current libvirt master branch. If it's thought to be of use to the > project please let me know. > > [1] http://www.thekelleys.org.uk/dhcp-helper/ > > > The following changes since commit 34f1a618a5c4507f27f3f467b723c9119c1db3c7: > > conf: Avoid leaking of RNG device definition (2013-02-25 22:31:11 +0100) > > are available in the git repository at: > > git://github.com/iam-TJ/libvirt.git dhcp_relay_network > > for you to fetch changes up to dfc0609403106712b205e60b53b29dc850cad68d: > > docs: Describe the 'enable' and 'relay' attributes (2013-02-27 > 20:35:06 +) Sounds interesting. It would also help if you posted this series of patches directly to the list. While there might be people willing to download from your repository, it is hard for them to provide comments if your patches have not also appeared on the list. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] q35 machine type and libvirt.
On Wed, 2013-02-27 at 13:20 -0500, Laine Stump wrote: > On 02/06/2013 02:13 PM, Laine Stump wrote: > > Now that qemu is getting the q35 machine type, libvirt needs to support it. > > In an attempt to make sure that libvirt actually does something useful > with qemu's new machine type, I'm revisiting this topic and trying to > get a more thorough understanding. > > I've developed my own list of what I *think* are the reasons for wanting > a new machine type in qemu (and from that it should be more apparent > what libvirt needs to do with it), and am wondering how far off the mark > I am: > >* Protection against obsolescence (PIIX (pc machine type) is 17 years old > (?) > and some OSes may drop support for it). > >* Passthrough of PCIe hardware? Is this really an issue? (e.g. the > VFs of a PCIe SRIOV network card can already be passed through as > standard PCI devices) We can't expose PCIe extended capabilities without a PCI express chipset. This includes error reporting and possibly use of advanced features for iommu interaction and caching. We also can't do any kind of IGD passthrough without a GMCH-like chipset. >* Larger bus space? Is this solved even without the new machine type by > simply > supporting the existing pci-bridge device and allowing multiple buses? > >* Support for new emulated hardware. (what is important?) Emulated devices have the same extended capabilities limitation. > Are any of these misguided thinking on my part? Are there other real > advantages over using the pc-* machine type? > > As an adjunct to that, in some conversations people have mentioned the fact > that the Q35 chipset is already out of production, and that supporting > something more recent, like the X58, might be better in the long run. Would > the main advantage of that be supportability? Or are there other concrete > differences in a newer chipset that might by themselves make it worth > considering? (Conversely, would attempting to write the drivers for a newer > chipset just be busywork that only netted a more complex virtual machine but > no useful gains?) IMHO, X58 or newer Q77 chipset would only build on and swap out components of something like Q35. Anything modern will have the same basic layout; some concept of a PCIe root complex with PCIe-to-PCI bridge(s) branching out to Legacy PCI buses and root ports, possibly with PCIe switches, connecting to PCIe endpoints. All of that will be the same for Q35/X58 or newer desktop chips. > So, based on the above (and whatever other gains are to be had from the new > machine type) what is needed/expected from libvirt? Here's my rough list: > >* setup different default buses/controllers/devices based on machine > type (including possibility of using pcie.0 as the root) > >* table of fixed locations for certain devices (if present) (again, > based on machine type) > >* restrict certain *types* of devices to certain *types* of > slots? (I'm a bit fuzzy on this one, but this has to do with the > "root complex" vs. "endpoint" vs. "integrated endpoint" that Alex > mentioned). Right, somehow libvirt needs to know or qemu needs to tell it something about the devices it's plugging in. If you were to grab your trust 10/100Mbps Legacy PCI ethernet card and try to plug it into a motherboard you'd quickly notice that you can only plug it into certain slots. This is the same problem. PCI device are attached to a legacy PCI bus, which means it needs to be behind a PCIe-to-PCI bridge. Legacy Endpoints and Endpoints are plugin PCIe cards, so they need to be plugged in behind a PCIe switch or root port. Integrated Endpoints are motherboard components, per the spec, they shouldn't even really be hotplug-able. They attach directly to the root complex. >* support some new emulated chipset devices? -M q35 + -device ioh3420 (root port) + -device i82801b11-bridge (pcie-to-pci bridge) >* Anything else specific required to passthrough pcie devices? I just sent out an RFC asking about VGA routing, it's possible libvirt will need to understand some concept of a primary VGA device and specify it somehow to QEMU. It's also possible (probable) that we'll make assigned devices able to mangle PCIe types to make it more flexible where they can be attached (for instance you may not want to have your HD audio device exposed as a root complex device if that disables hotplug, so we may mangle it to look like a regular endpoint. Windows is picky about root complex devices, so we may also mangle endpoint to integrated endpoint as well). Also need to be aware that all PCIe "buses" except the root complex are actually point-to-point links, so a root port connects to one PCIe device (which may be multifunction). PCIe switches are needed to get fan out to connect multiple devices, one device per downstream port. It would be interesting how much we can abuse the topology in a virtual sys
[libvirt] [RFC] DHCP Relay agent functionality
Recently I needed to implement functionality to operate a DHCP relay instead of dnsmasq's DHCP server on the virtual networks managed by libvirt. The primary benefit of operating a relay is that the central DHCP and DNS servers for the physical network can manage IP address allocation and hostname resolution for the entire network and, potentially, expose VMs via hostname and IP address on the public network too, as well as being able to monitor VM instance creation for resource planning. I originally implemented this functionality on libvirt 0.9.8 included with Ubuntu 12.04 as a hack to simply disable dnsmasq entirely and use dhcp-helper[1] (the DHCP relay written by Simon Kelley, author of dnsmaq) although I've constructed it so that another relay-agent could be used. After proving it works well I've reworked the code on the current libvirt master branch. If it's thought to be of use to the project please let me know. [1] http://www.thekelleys.org.uk/dhcp-helper/ The following changes since commit 34f1a618a5c4507f27f3f467b723c9119c1db3c7: conf: Avoid leaking of RNG device definition (2013-02-25 22:31:11 +0100) are available in the git repository at: git://github.com/iam-TJ/libvirt.git dhcp_relay_network for you to fetch changes up to dfc0609403106712b205e60b53b29dc850cad68d: docs: Describe the 'enable' and 'relay' attributes (2013-02-27 20:35:06 +) TJ (10): conf: DHCP - add state for DHCP Relay and On/Off switch conf: Network - add ability to read/write XML DHCP state conf: Network - add pointers to enabled virNetworkIpDef DHCP settings conf: Network - keep track of active DHCP stanza in virNetworkDef network: Bridge - use IPv4 and IPv6 active DHCP stanza pointers network: Bridge - Add support for a DHCP Relay Agent network: Bridge - don't offer dnsmasq DHCP services when DHCP relay is enabled configure: Add DHCPRELAY to the set of external program definitions Add copyright attribution for DHCP relay functionality docs: Describe the 'enable' and 'relay' attributes configure.ac| 4 +++ docs/formatnetwork.html.in | 22 +- src/conf/network_conf.c | 36 +++--- src/conf/network_conf.h | 7 + src/network/bridge_driver.c | 215 +--- 5 files changed, 229 insertions(+), 55 deletions(-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RESEND] Add support for tag in network config
On 02/26/2013 03:38 PM, Pieter Hollants wrote: > [Bcc problems] Ah, indeed, I never got some of those mails, just saw > my original patch merged last Sunday and already wondered why nobody > commented :) > > For brevity, I'll summarize the points we've been discussing. Forgive > me if I oversee something important. [very thorough summary snipped] > > Am 26.02.2013 19:05, schrieb Laine Stump: >> On the other hand, I don't want to over-engineer this problem so much >> that we never push *anything*. > > Yes, unless I missed something I think the bottom line of my summary > above is that there would have to be a really good argument _against_ > "number=xxx". Implementing "name=yyy" does not automatically mean that > "number=xxx" is a bad thing to do. Most of all, it's a very pragmatic > solution whereas "name=xxx" most probably requires some design decisions. Correct. > >> Without even arriving at a decision about this, I'm now thinking that >> maybe we should revert the earlier patch until after the >> release, and re-push it after the named-option support is done >> (potentially with some changes). >> >> Any other opinions? > > I'm used to having to compile it myself anyway, so that'd be fine with > me if we want > to give it more thought. > > On the other hand, I don't see that we can do WITHOUT "number=..." > unless we want to implement only a restricted subset of DHCP options > known by name. Right. I haven't changed my mind about the "number" attribute, but since the "value" will be re-used when we have named (and more thoroughly parsed/validated) options, and since there are some options that take a *list* of values rather than a single value, if maybe we shouldn't do something like: or something similar. (on known options, the "type" would be implicit, and if specified would be required to match what was already known). Since that would create conflicts with what we'd decided / you implemented, and since we can't change anything once it's been in an official release, I figured the safest thing to do was revert the patch until after the release, giving us more time to ponder (I reverted it this morning). Definitely *something* will go in between 1.0.3 and 1.0.4, though! (BTW, thanks for posting a patch for this so quickly. It really helped to get me motivated about the issue.) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Dnsmasq-discuss] Chaining instances?
On 02/25/2013 09:08 PM, Gao Yongwei wrote: > > I thinks it's better that if we can put dnsmasq args or options in a > conf file, so we can do some custom through this conf file. > I've added a Bug 913446 in redhat bugzilla,but seems no one take care > of this bug? This has been discussed extensively on the list before, and we specifically *don't* want to do it. Gene Czarcinski even submitted a patch that would do it, and that patch was rejected (and I *think* he agreed with our reasoning :-) The problem is that when you allow a user to silently subvert the config that is shown in libvirt's XML, and the system stops working, the user will send a plea for help to irc/mailing list (or open a ticket with their Linux support vendor), and the people they ask for support will say "show us the output of 'virsh net-dumpxml mynetwork'", which they will send, and then a comedy of errors will ensue until someone finally realizes that there is some "extra" configuration that the user isn't telling us about. There are two solutions to that: 1) add an element for the specific option you want to control in libvirt's network XML. Some knobs are already there, and others are being added. 2) add a private "dnsmasq" namespace to libvirt's network xml, with provisions for directly passing through dnsmasq commandline options from the xml to the conf file. This would be similar to what has already been done for qemu: http://libvirt.org/drvqemu.html#qemucommand The difference between these and the idea of simply allowing a user-written conf file is that everything about the network's config would then be available in "virsh net-dumpxml $netname". As far as the bug you've filed, it takes awhile for bugs to be triaged. (At a first glance, it seems reasonable to add such an option, since it is a standard part of the dhcp protocol. We might need to do something about specifying different units for the lease time.) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] q35 machine type and libvirt.
On 02/06/2013 02:13 PM, Laine Stump wrote: > Now that qemu is getting the q35 machine type, libvirt needs to support it. In an attempt to make sure that libvirt actually does something useful with qemu's new machine type, I'm revisiting this topic and trying to get a more thorough understanding. I've developed my own list of what I *think* are the reasons for wanting a new machine type in qemu (and from that it should be more apparent what libvirt needs to do with it), and am wondering how far off the mark I am: * Protection against obsolescence (PIIX (pc machine type) is 17 years old (?) and some OSes may drop support for it). * Passthrough of PCIe hardware? Is this really an issue? (e.g. the VFs of a PCIe SRIOV network card can already be passed through as standard PCI devices) * Larger bus space? Is this solved even without the new machine type by simply supporting the existing pci-bridge device and allowing multiple buses? * Support for new emulated hardware. (what is important?) Are any of these misguided thinking on my part? Are there other real advantages over using the pc-* machine type? As an adjunct to that, in some conversations people have mentioned the fact that the Q35 chipset is already out of production, and that supporting something more recent, like the X58, might be better in the long run. Would the main advantage of that be supportability? Or are there other concrete differences in a newer chipset that might by themselves make it worth considering? (Conversely, would attempting to write the drivers for a newer chipset just be busywork that only netted a more complex virtual machine but no useful gains?) So, based on the above (and whatever other gains are to be had from the new machine type) what is needed/expected from libvirt? Here's my rough list: * setup different default buses/controllers/devices based on machine type (including possibility of using pcie.0 as the root) * table of fixed locations for certain devices (if present) (again, based on machine type) * restrict certain *types* of devices to certain *types* of slots? (I'm a bit fuzzy on this one, but this has to do with the "root complex" vs. "endpoint" vs. "integrated endpoint" that Alex mentioned). * support some new emulated chipset devices? * Anything else specific required to passthrough pcie devices? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] inside a //disk/source element
On Wed, Feb 27, 2013 at 05:24:26PM +, Daniel P. Berrange wrote: > On Wed, Feb 27, 2013 at 05:14:55PM +, Richard W.M. Jones wrote: > > > > According to the docs, it should be possible to do: > > > > > > > > < NB > > > > > > > > > > > > However I tried it, and it simply doesn't work. Furthermore I looked > > at the code in domain_conf.c, and I can't see how it's even supposed > > to work. It doesn't look to me as if is ever parsed in > > that context. > > > > Can anyone else confirm that this is a bug or point out my error? > > Historically this was correct, because we only supported labels for > one security driver. When we added support for multiple security > drivers it seems we caused a regression. > > > > should have been treated as equivalent to > > > > but we're not doing that :-( This works, thanks. Unfortunately it leads to an even more intractable labelling problem, but I'll follow up on the original BZ here: https://bugzilla.redhat.com/show_bug.cgi?id=912499 Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Il 27/02/2013 18:38, Anthony Liguori ha scritto: >> > The solution is "there is no way to override a previously specified >> > key". Something like "-device >> > virtio-scsi-pci,num_queues=1,num_queues=2" now works, let's make it an >> > error instead. > That breaks compatibility. The above may seem silly but consider: > > qemu -device virtio-scsi-pci,num_queues=1,id=foo \ > -set device.foo.num_queues=2 > > This is more common than you would think primarily as a way to override > options that libvirt has set either via the qemu extra args tag or a > script wrapper of qemu. "-set" could first delete a pre-existing option. We could also extend "-set" to accept multiple values (like -set device.foo.bar=baz,device.foo.qux=quux), which is useful also to set a list option. Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix autodestroy of QEMU guests
On 02/27/2013 09:26 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" > > The virQEMUCloseCallbacksRunOne method was passing a uuid string > to virDomainObjListFindByUUID, when it actually expected to get > a raw uuid buffer. This was not caught by the compiler because > the method was using a 'void *uuid' instead of first casting > it to the expected type. > > This regression was accidentally caused by refactoring in > > commit 568a6cda277f04ab9baaeb97490e548b7b608aa6 > Author: Jiri Denemark > Date: Fri Feb 15 15:11:47 2013 +0100 > > qemu: Avoid deadlock in autodestroy > > Signed-off-by: Daniel P. Berrange > --- > src/qemu/qemu_conf.c | 12 > 1 file changed, 8 insertions(+), 4 deletions(-) ACK - definite 1.0.3 material. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Paolo Bonzini writes: > Il 27/02/2013 18:08, Anthony Liguori ha scritto: >>> > >>> > No, no, no. This makes ':' special, which means you can't have lists of >>> > anything containing ':'. Your cure is worse than the disease. Let go >>> > of that syntactic high-fructose corn syrup, stick to what we have and >>> > works just fine, thank you. >> Yes, there *must* be special syntax. If we're treating something >> special, then we should indicate to the user that it's special. >> >> Specifically, a list of integers should look distinctly different than >> overriding a previously specified integer. > > The solution is "there is no way to override a previously specified > key". Something like "-device > virtio-scsi-pci,num_queues=1,num_queues=2" now works, let's make it an > error instead. That breaks compatibility. The above may seem silly but consider: qemu -device virtio-scsi-pci,num_queues=1,id=foo \ -set device.foo.num_queues=2 This is more common than you would think primarily as a way to override options that libvirt has set either via the qemu extra args tag or a script wrapper of qemu. Regards, Anthony Liguori > > Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
On 02/27/2013 10:04 AM, Anthony Liguori wrote: >> Whatever I use to implement it, I still need to know how the >> command-line syntax will look like, because we need to tell libvirt >> developers how they should write the QEMU command-line. > > Command line syntax is not committed until it appears in a release. > libvirt *should not* assume any specific syntax until the 1.5 release > ships. Libvirt 1.0.3 will already error out on any disjoint ranges, and we are just fine waiting until qemu 1.5 is released before adding any code to support disjoint ranges in the preferred syntax. Qemu should feel free to implement it correctly, rather than trying to cater to older (broken) libvirt's abuse of comma. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix typo in internal VIR_QEMU_PROCESS_START_AUTODESROY constant
On 02/27/2013 09:53 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"
>
> s/VIR_QEMU_PROCESS_START_AUTODESROY/VIR_QEMU_PROCESS_START_AUTODESTROY/
>
> Signed-off-by: Daniel P. Berrange
> ---
> +++ b/src/qemu/qemu_process.h
> @@ -47,7 +47,7 @@ int qemuProcessAssignPCIAddresses(virDomainDefPtr def);
> typedef enum {
> VIR_QEMU_PROCESS_START_COLD = 1 << 0,
> VIR_QEMU_PROCESS_START_PAUSED = 1 << 1,
> -VIR_QEMU_PROCESS_START_AUTODESROY = 1 << 2,
> +VIR_QEMU_PROCESS_START_AUTODESTROY = 1 << 2,
Alignment - drop a space if you haven't pushed already.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Avoid spamming logs with cgroups warnings
On 02/27/2013 09:52 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" > > The code for putting the emulator threads in a separate cgroup > would spam the logs with warnings > > 2013-02-27 16:08:26.731+: 29624: warning : virCgroupMoveTask:887 : no vm > cgroup in controller 3 > 2013-02-27 16:08:26.731+: 29624: warning : virCgroupMoveTask:887 : no vm > cgroup in controller 4 > 2013-02-27 16:08:26.732+: 29624: warning : virCgroupMoveTask:887 : no vm > cgroup in controller 6 Yay - I'm tired of these as well. > > This is because it has only created child cgroups for 3 of the > controllers, but was trying to move the processes from all the > controllers. The fix is to only try to move threads in the > controllers we actually created. Also remove the warning and > make it return a hard error to avoid such lazy callers in the > future. > > Signed-off-by: Daniel P. Berrange > --- > src/qemu/qemu_cgroup.c | 5 + > src/util/vircgroup.c | 3 +-- > 2 files changed, 6 insertions(+), 2 deletions(-) ACK, and definitely 1.0.3 material. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] inside a //disk/source element
On Wed, Feb 27, 2013 at 05:14:55PM +, Richard W.M. Jones wrote: > > According to the docs, it should be possible to do: > > > > < NB > > > > > > However I tried it, and it simply doesn't work. Furthermore I looked > at the code in domain_conf.c, and I can't see how it's even supposed > to work. It doesn't look to me as if is ever parsed in > that context. > > Can anyone else confirm that this is a bug or point out my error? Historically this was correct, because we only supported labels for one security driver. When we added support for multiple security drivers it seems we caused a regression. should have been treated as equivalent to but we're not doing that :-( If you explicitly add the model it'll do what you want. We should still fix this bug though Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
On Wed, Feb 27, 2013 at 11:04:08AM -0600, Anthony Liguori wrote: > Eduardo Habkost writes: > > > On Wed, Feb 27, 2013 at 04:57:15PM +0100, Paolo Bonzini wrote: > >> Il 27/02/2013 16:42, Anthony Liguori ha scritto: > >> > There's such thing as list support in QemuOpts. The only way > >> > QemuOptsVisitor was able to implement it was to expose QemuOpts publicly > >> > via options_int.h and rely on a implementation detail. > >> > > >> > There are fixed types supported by QemuOpts. It just so happens that > >> > whenever qemu_opt_set() is called, instead of replacing the last > >> > instance, the value is either prepended or appended in order to > >> > implement a replace or set-if-unset behavior. > >> > >> Fair enough. Nobody said the implementation is pretty. > >> > >> > If we want to have list syntax, we need to introduce first class support > >> > for it. Here's a simple example of how to do this. > >> > >> If it is meant as a prototype only, and the final command-line syntax > >> would be with repeated keys, that's okay. I think that Eduardo/Markus/I > >> are focusing on the user interface, you're focusing in the implementation. > >> > >> In the meanwhile, however, it seems to me that Eduardo can use > >> QemuOptsVisitor---which can also hide the details and provide type safety. > > > > Whatever I use to implement it, I still need to know how the > > command-line syntax will look like, because we need to tell libvirt > > developers how they should write the QEMU command-line. > > Command line syntax is not committed until it appears in a release. > libvirt *should not* assume any specific syntax until the 1.5 release > ships. I am just talking about communication with libvirt developers. Developers surely write code and test their work in progress using unreleased QEMU code, instead of waiting for an official release. Nobody suggested releasing a libvirt version that relies on an unreleased QEMU feature. -- Eduardo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] remove the redundant codes
On Wed, Feb 27, 2013 at 04:09:37PM +0800, Gao feng wrote: > Intend to reduce the redundant code,use virDomainSetupNumaMemoryPolicy > to replace virLXCControllerSetupNUMAPolicy and > qemuProcessInitNumaMemoryPolicy. > > Signed-off-by: Gao feng > --- > src/conf/domain_conf.c | 113 +++ > src/conf/domain_conf.h | 2 + Again, this is not a suitable place for this code. It'll want to go in src/util/virnuma.c too. NB, you'll need to move the virDomainNumatuneDef struct definition out of domain_conf.h and rename it to 'virNumaTuneParams' in the src/util/virnuma.h file Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Il 27/02/2013 18:08, Anthony Liguori ha scritto: >> > >> > No, no, no. This makes ':' special, which means you can't have lists of >> > anything containing ':'. Your cure is worse than the disease. Let go >> > of that syntactic high-fructose corn syrup, stick to what we have and >> > works just fine, thank you. > Yes, there *must* be special syntax. If we're treating something > special, then we should indicate to the user that it's special. > > Specifically, a list of integers should look distinctly different than > overriding a previously specified integer. The solution is "there is no way to override a previously specified key". Something like "-device virtio-scsi-pci,num_queues=1,num_queues=2" now works, let's make it an error instead. Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] rename qemuGetNumadAdvice to virDomainGetNumadAdvice
On Wed, Feb 27, 2013 at 05:11:06PM +, Daniel P. Berrange wrote: > On Wed, Feb 27, 2013 at 04:09:35PM +0800, Gao feng wrote: > > qemuGetNumadAdvice will be used by LXC driver,rename > > it to virDomainGetNumaAdvice and move it to domain_conf.c > > > > Signed-off-by: Gao feng > > --- > > src/conf/domain_conf.c | 31 +++ > > src/conf/domain_conf.h | 2 ++ > > src/libvirt_private.syms | 1 + > > src/qemu/qemu_process.c | 32 +--- > > 4 files changed, 35 insertions(+), 31 deletions(-) > > Ewww no. The domain_conf.c file is for XML configuration > parsing & formatting. Code dealing with numad has no > business being there. > > We don't currently have any place for general NUMA related > helper APIs, so I'd suggest you create a new pair of files > for these methods src/util/virnuma.h and src/util/virnuma.c NB, you won't be able to pass in a virDomainDef when the code is located here. Instead make the API accept 2 params, the vcpus and memory values. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] LXC: allow uses advisory nodeset from querying numad
On Wed, Feb 27, 2013 at 04:09:36PM +0800, Gao feng wrote:
> Allow lxc using the advisory nodeset from querying numad.
>
> Signed-off-by: Gao feng
> ---
> src/lxc/lxc_controller.c | 54
> +---
> 1 file changed, 47 insertions(+), 7 deletions(-)
>
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 15aa334..c6e7bbf 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -409,7 +409,8 @@ cleanup:
> }
>
> #if WITH_NUMACTL
> -static int virLXCControllerSetupNUMAPolicy(virLXCControllerPtr ctrl)
> +static int virLXCControllerSetupNUMAPolicy(virLXCControllerPtr ctrl,
> + virBitmapPtr nodemask)
> {
> nodemask_t mask;
> int mode = -1;
> @@ -418,9 +419,22 @@ static int
> virLXCControllerSetupNUMAPolicy(virLXCControllerPtr ctrl)
> int i = 0;
> int maxnode = 0;
> bool warned = false;
> -
> -if (!ctrl->def->numatune.memory.nodemask)
> +virDomainNumatuneDef numatune = ctrl->def->numatune;
> +virBitmapPtr tmp_nodemask = NULL;
> +
> +if (numatune.memory.placement_mode ==
> +VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_STATIC) {
> +if (!numatune.memory.nodemask)
> +return 0;
> +VIR_DEBUG("Set NUMA memory policy with specified nodeset");
> +tmp_nodemask = numatune.memory.nodemask;
> +} else if (numatune.memory.placement_mode ==
> + VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_AUTO) {
> +VIR_DEBUG("Set NUMA memory policy with advisory nodeset from numad");
> +tmp_nodemask = nodemask;
> +} else {
> return 0;
> +}
>
> VIR_DEBUG("Setting NUMA memory policy");
>
> @@ -435,7 +449,7 @@ static int
> virLXCControllerSetupNUMAPolicy(virLXCControllerPtr ctrl)
> /* Convert nodemask to NUMA bitmask. */
> nodemask_zero(&mask);
> i = -1;
> -while ((i = virBitmapNextSetBit(ctrl->def->numatune.memory.nodemask, i))
> >= 0) {
> +while ((i = virBitmapNextSetBit(tmp_nodemask, i)) >= 0) {
> if (i > NUMA_NUM_NODES) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("Host cannot support NUMA node %d"), i);
> @@ -488,7 +502,8 @@ cleanup:
> return ret;
> }
> #else
> -static int virLXCControllerSetupNUMAPolicy(virLXCControllerPtr ctrl)
> +static int virLXCControllerSetupNUMAPolicy(virLXCControllerPtr ctrl,
> + virBitmapPtr nodemask
> ATTRIBUTE_UNUSED)
> {
> if (ctrl->def->numatune.memory.nodemask) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -560,13 +575,38 @@ static int
> virLXCControllerSetupCpuAffinity(virLXCControllerPtr ctrl)
> */
> static int virLXCControllerSetupResourceLimits(virLXCControllerPtr ctrl)
> {
> -
> +virBitmapPtr nodemask = NULL;
> +char *nodeset;
> if (virLXCControllerSetupCpuAffinity(ctrl) < 0)
> return -1;
>
> -if (virLXCControllerSetupNUMAPolicy(ctrl) < 0)
> +/* Get the advisory nodeset from numad if 'placement' of
> + * either or is 'auto'.
> + */
> +if ((ctrl->def->placement_mode ==
> + VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) ||
> +(ctrl->def->numatune.memory.placement_mode ==
> + VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_AUTO)) {
> +nodeset = virDomainGetNumadAdvice(ctrl->def);
Currently the 'def->vcpus' data has no effect in the LXC driver,
since it has no concept of CPUs. With this change applied, the
container is now going to be bind to exactly def->vcpus number
of host CPUs. I'm not saying this is bad - it is actually a
good thing I believe. You should document this behaviour in
the GIT comment message though.
> +if (!nodeset)
> +return -1;
> +
> +VIR_DEBUG("Nodeset returned from numad: %s", nodeset);
> +
> +if (virBitmapParse(nodeset, 0, &nodemask,
> + VIR_DOMAIN_CPUMASK_LEN) < 0) {
> +VIR_FREE(nodeset);
> +return -1;
> +}
> +VIR_FREE(nodeset);
> +}
> +
> +if (virLXCControllerSetupNUMAPolicy(ctrl, nodemask) < 0) {
> +virBitmapFree(nodemask);
> return -1;
> +}
>
> +virBitmapFree(nodemask);
> return virLXCCgroupSetup(ctrl->def);
> }
This method could do with a 'cleanup:' block so you don't duplicate
the VIR_FREE(nodeset) and virBitmapFree(nodemask) calls everywhere.
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] inside a //disk/source element
According to the docs, it should be possible to do: < NB However I tried it, and it simply doesn't work. Furthermore I looked at the code in domain_conf.c, and I can't see how it's even supposed to work. It doesn't look to me as if is ever parsed in that context. Can anyone else confirm that this is a bug or point out my error? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] rename qemuGetNumadAdvice to virDomainGetNumadAdvice
On Wed, Feb 27, 2013 at 04:09:35PM +0800, Gao feng wrote: > qemuGetNumadAdvice will be used by LXC driver,rename > it to virDomainGetNumaAdvice and move it to domain_conf.c > > Signed-off-by: Gao feng > --- > src/conf/domain_conf.c | 31 +++ > src/conf/domain_conf.h | 2 ++ > src/libvirt_private.syms | 1 + > src/qemu/qemu_process.c | 32 +--- > 4 files changed, 35 insertions(+), 31 deletions(-) Ewww no. The domain_conf.c file is for XML configuration parsing & formatting. Code dealing with numad has no business being there. We don't currently have any place for general NUMA related helper APIs, so I'd suggest you create a new pair of files for these methods src/util/virnuma.h and src/util/virnuma.c Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix typo in internal VIR_QEMU_PROCESS_START_AUTODESROY constant
On 02/27/13 17:53, Daniel P. Berrange wrote: From: "Daniel P. Berrange" s/VIR_QEMU_PROCESS_START_AUTODESROY/VIR_QEMU_PROCESS_START_AUTODESTROY/ Signed-off-by: Daniel P. Berrange --- src/qemu/qemu_driver.c| 4 ++-- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_process.c | 4 ++-- src/qemu/qemu_process.h | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) ACK. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Don't try to add non-existant devices to ACL
On 02/27/2013 09:59 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" > > The QEMU driver has a list of devices nodes that are whitelisted > for all guests. The kernel has recently started returning an > error if you try to whitelist a device which does not exist. > This causes a warning in libvirt logs and an audit error for > any missing devices. eg > > 2013-02-27 16:08:26.515+: 29625: warning : virDomainAuditCgroup:451 : > success=no virt=kvm resrc=cgroup reason=allow vm="vm031714" > uuid=9d8f1de0-44f4-a0b1-7d50-e41ee6cd897b > cgroup="/sys/fs/cgroup/devices/libvirt/qemu/vm031714/" class=path > path=/dev/kqemu rdev=? acl=rw > > Signed-off-by: Daniel P. Berrange > --- > src/qemu/qemu_cgroup.c | 6 ++ > 1 file changed, 6 insertions(+) ACK. Safe for 1.0.3. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/4] QEMU: add -dtb option support
On Wed, Feb 27, 2013 at 04:28:37PM +0800, Olivia Yin wrote:
> Signed-off-by: Olivia Yin
> ---
> src/conf/domain_conf.c |4
> src/conf/domain_conf.h |1 +
> src/qemu/qemu_command.c |6 ++
> 3 files changed, 11 insertions(+), 0 deletions(-)
The split of code across your patches is sub-optimal. We prefer
to keep changes to the XML parser/formatter separate from changes
to drivers.
Also the code to test the QEMU driver should be in the same
patch as the patch which changes the command line parser.
So what you really want is 2 patches. One with domain_conf.{c,h}
changes, RNG schema addition & schema documentation, and the other
patch with all the QEMU driver changes + testing
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Eduardo Habkost writes: > On Wed, Feb 27, 2013 at 04:57:15PM +0100, Paolo Bonzini wrote: >> Il 27/02/2013 16:42, Anthony Liguori ha scritto: >> > There's such thing as list support in QemuOpts. The only way >> > QemuOptsVisitor was able to implement it was to expose QemuOpts publicly >> > via options_int.h and rely on a implementation detail. >> > >> > There are fixed types supported by QemuOpts. It just so happens that >> > whenever qemu_opt_set() is called, instead of replacing the last >> > instance, the value is either prepended or appended in order to >> > implement a replace or set-if-unset behavior. >> >> Fair enough. Nobody said the implementation is pretty. >> >> > If we want to have list syntax, we need to introduce first class support >> > for it. Here's a simple example of how to do this. >> >> If it is meant as a prototype only, and the final command-line syntax >> would be with repeated keys, that's okay. I think that Eduardo/Markus/I >> are focusing on the user interface, you're focusing in the implementation. >> >> In the meanwhile, however, it seems to me that Eduardo can use >> QemuOptsVisitor---which can also hide the details and provide type safety. > > Whatever I use to implement it, I still need to know how the > command-line syntax will look like, because we need to tell libvirt > developers how they should write the QEMU command-line. Command line syntax is not committed until it appears in a release. libvirt *should not* assume any specific syntax until the 1.5 release ships. Regards, Anthony Liguori > > -- > Eduardo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Markus Armbruster writes: > Anthony Liguori writes: > >> Which is indistinguishable from a straight string property. This means >> it's impossible to introspect because the type is context-sensitive. >> >> What's more, there is no API outside of QemuOptsVisitor that can >> actually work with "lists" of QemuOpts values. > > There is: qemu_opt_foreach() I'm not sure I believe that you wrote that with a straight face... ;-) >> opt = g_malloc0(sizeof(*opt)); >> opt->name = g_strdup(name); >> opt->opts = opts; > > No, no, no. This makes ':' special, which means you can't have lists of > anything containing ':'. Your cure is worse than the disease. Let go > of that syntactic high-fructose corn syrup, stick to what we have and > works just fine, thank you. Yes, there *must* be special syntax. If we're treating something special, then we should indicate to the user that it's special. Specifically, a list of integers should look distinctly different than overriding a previously specified integer. Regards, Anthony Liguori > > Then add suitable list accessor functions and error checks. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Paolo Bonzini writes: > Il 27/02/2013 16:42, Anthony Liguori ha scritto: >> There's such thing as list support in QemuOpts. The only way >> QemuOptsVisitor was able to implement it was to expose QemuOpts publicly >> via options_int.h and rely on a implementation detail. >> >> There are fixed types supported by QemuOpts. It just so happens that >> whenever qemu_opt_set() is called, instead of replacing the last >> instance, the value is either prepended or appended in order to >> implement a replace or set-if-unset behavior. > > Fair enough. Nobody said the implementation is pretty. > >> If we want to have list syntax, we need to introduce first class support >> for it. Here's a simple example of how to do this. > > If it is meant as a prototype only, and the final command-line syntax > would be with repeated keys, that's okay. I think that Eduardo/Markus/I > are focusing on the user interface, you're focusing in the > implementation. No, I'm primarily motivated by the UI and am assuming that ya'll are arguing based on the implementation today. Repeating keys is quite mad. Here are some examples: qemu -numa node=1,node=2,cpus=2,cpus=3 What would a user expect that that would mean. What about: [numa] node=1 cpus=2 cpus=3 qemu -readconfig numa.cfg -numa node=1,cpus=1 I'm pretty sure you won't be able to say without looking at the source code. Now what about ranges? I'm pretty sure that what a user really wants to say is: qemu -numa node=1,cpus=0-3:8-11 This is easy to do with the patch I sent. We can add range support integer lists by just adding a check for '-' before doing dispatch. That's a heck of a lot nicer than: qemu -numa node=1,cpus=0,cpus=1,cpus=2,cpus=3,cpus=8,cpus=9,cpus=10,cpus=11 With respect to escaping, for string lists (the only place where this point is even relevant), we can simply support escaping. But I'd like to hear a use-case for a string list first. Regards, Anthony Liguori > > In the meanwhile, however, it seems to me that Eduardo can use > QemuOptsVisitor---which can also hide the details and provide type safety. > > Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/4] qemu: add dtb capability
On 02/27/2013 09:56 AM, Eric Blake wrote: >> @@ -944,6 +945,8 @@ virQEMUCapsComputeCmdFlags(const char *help, >> } >> if (strstr(help, "-uuid")) >> virQEMUCapsSet(qemuCaps, QEMU_CAPS_UUID); >> +if (strstr(help, "-dtb")) >> +virQEMUCapsSet(qemuCaps, QEMU_CAPS_DTB); > > This won't work. -dtb did not exist prior to qemu 1.2, and for all qemu > newer than 1.2, we do NOT scrape '-help' output; instead, we use QMP > query commands. You need to figure out the corresponding QMP command > that we can use to tell when dtb support exists in qemu. I stand (slightly) corrected: it looks like -dtb was added in qemu 1.1 (qemu commit 379b5c7), which means we DO need -help scraping for that version of qemu. Then, since it predates when QMP queries are reliable, you can get away with blindly setting QEMU_CAPS_DTB when targetting a qemu new enough to support QMP queries without actually having to query for it. But it's still something to be fixed before this patch is ready :) -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Warnings messages in logs from DHCP snooping code
I'm noticing that VMs which have nwfilters associated with them will apparently always cause the following errors to appear in the logs upon VM shutdown: 2013-02-27 17:02:18.709+: 9669: error : virNWFilterDHCPSnoopEnd:2131 : internal error ifname "vnet0" not in key map 2013-02-27 17:02:18.715+: 9669: error : virNetDevGetIndex:653 : Unable to get index for interface vnet0: No such device Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
On Wed, Feb 27, 2013 at 05:58:39PM +0100, Paolo Bonzini wrote: > Il 27/02/2013 17:19, Eduardo Habkost ha scritto: > >> > > >> > If it is meant as a prototype only, and the final command-line syntax > >> > would be with repeated keys, that's okay. I think that Eduardo/Markus/I > >> > are focusing on the user interface, you're focusing in the > >> > implementation. > >> > > >> > In the meanwhile, however, it seems to me that Eduardo can use > >> > QemuOptsVisitor---which can also hide the details and provide type > >> > safety. > > Whatever I use to implement it, I still need to know how the > > command-line syntax will look like, because we need to tell libvirt > > developers how they should write the QEMU command-line. > > I don't think any syntax makes sense except cpus=A,cpus=B. How we > implement it is another story. I agree completely, and I still don't know why Anthony doesn't like it. -- Eduardo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Don't try to add non-existant devices to ACL
From: "Daniel P. Berrange"
The QEMU driver has a list of devices nodes that are whitelisted
for all guests. The kernel has recently started returning an
error if you try to whitelist a device which does not exist.
This causes a warning in libvirt logs and an audit error for
any missing devices. eg
2013-02-27 16:08:26.515+: 29625: warning : virDomainAuditCgroup:451 :
success=no virt=kvm resrc=cgroup reason=allow vm="vm031714"
uuid=9d8f1de0-44f4-a0b1-7d50-e41ee6cd897b
cgroup="/sys/fs/cgroup/devices/libvirt/qemu/vm031714/" class=path
path=/dev/kqemu rdev=? acl=rw
Signed-off-by: Daniel P. Berrange
---
src/qemu/qemu_cgroup.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 671d613..9d6e88b 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -265,6 +265,12 @@ int qemuSetupCgroup(virQEMUDriverPtr driver,
}
for (i = 0; deviceACL[i] != NULL ; i++) {
+if (access(deviceACL[i], F_OK) < 0) {
+VIR_DEBUG("Ignoring non-existant device %s",
+ deviceACL[i]);
+continue;
+}
+
rc = virCgroupAllowDevicePath(cgroup, deviceACL[i],
VIR_CGROUP_DEVICE_RW);
virDomainAuditCgroupPath(vm, cgroup, "allow", deviceACL[i], "rw",
rc);
--
1.7.11.7
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Il 27/02/2013 17:19, Eduardo Habkost ha scritto: >> > >> > If it is meant as a prototype only, and the final command-line syntax >> > would be with repeated keys, that's okay. I think that Eduardo/Markus/I >> > are focusing on the user interface, you're focusing in the implementation. >> > >> > In the meanwhile, however, it seems to me that Eduardo can use >> > QemuOptsVisitor---which can also hide the details and provide type safety. > Whatever I use to implement it, I still need to know how the > command-line syntax will look like, because we need to tell libvirt > developers how they should write the QEMU command-line. I don't think any syntax makes sense except cpus=A,cpus=B. How we implement it is another story. Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/4] qemu: add dtb capability
On 02/27/2013 01:28 AM, Olivia Yin wrote:
> Signed-off-by: Olivia Yin
> ---
> src/qemu/qemu_capabilities.c |3 +++
> src/qemu/qemu_capabilities.h |1 +
> 2 files changed, 4 insertions(+), 0 deletions(-)
[not a patch review, but a general comment]
You did deep threading:
0/4
\> 1/4
\> 2/4
\> 3/4
\> 4/4
But typically we prefer shallow threading:
0/4
+> 1/4
+> 2/4
+> 3/4
\> 4/4
You may want to investigate the settings you are using with git
format-patch and/or send-email.
> +++ b/src/qemu/qemu_capabilities.c
> @@ -210,6 +210,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>
>"rng-random", /* 130 */
>"rng-egd",
> + "dtb",
OK, so I lied - I'm also doing a patch review:
This uses a TAB, which is forbidden by 'make syntax-check'.
> );
>
> struct _virQEMUCaps {
> @@ -944,6 +945,8 @@ virQEMUCapsComputeCmdFlags(const char *help,
> }
> if (strstr(help, "-uuid"))
> virQEMUCapsSet(qemuCaps, QEMU_CAPS_UUID);
> +if (strstr(help, "-dtb"))
> +virQEMUCapsSet(qemuCaps, QEMU_CAPS_DTB);
This won't work. -dtb did not exist prior to qemu 1.2, and for all qemu
newer than 1.2, we do NOT scrape '-help' output; instead, we use QMP
query commands. You need to figure out the corresponding QMP command
that we can use to tell when dtb support exists in qemu.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Avoid spamming logs with cgroups warnings
From: "Daniel P. Berrange"
The code for putting the emulator threads in a separate cgroup
would spam the logs with warnings
2013-02-27 16:08:26.731+: 29624: warning : virCgroupMoveTask:887 : no vm
cgroup in controller 3
2013-02-27 16:08:26.731+: 29624: warning : virCgroupMoveTask:887 : no vm
cgroup in controller 4
2013-02-27 16:08:26.732+: 29624: warning : virCgroupMoveTask:887 : no vm
cgroup in controller 6
This is because it has only created child cgroups for 3 of the
controllers, but was trying to move the processes from all the
controllers. The fix is to only try to move threads in the
controllers we actually created. Also remove the warning and
make it return a hard error to avoid such lazy callers in the
future.
Signed-off-by: Daniel P. Berrange
---
src/qemu/qemu_cgroup.c | 5 +
src/util/vircgroup.c | 3 +--
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index e65b486..671d613 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -692,6 +692,11 @@ int qemuSetupCgroupForEmulator(virQEMUDriverPtr driver,
}
for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
+if (i != VIR_CGROUP_CONTROLLER_CPU &&
+i != VIR_CGROUP_CONTROLLER_CPUACCT &&
+i != VIR_CGROUP_CONTROLLER_CPUSET)
+continue;
+
if (!qemuCgroupControllerActive(driver, i))
continue;
rc = virCgroupMoveTask(cgroup, cgroup_emulator, i);
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 48cba93..532e704 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -884,8 +884,7 @@ int virCgroupMoveTask(virCgroupPtr src_group, virCgroupPtr
dest_group,
if (!src_group->controllers[controller].mountPoint ||
!dest_group->controllers[controller].mountPoint) {
-VIR_WARN("no vm cgroup in controller %d", controller);
-return 0;
+return -EINVAL;
}
rc = virCgroupGetValueStr(src_group, controller, "tasks", &content);
--
1.7.11.7
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Fix typo in internal VIR_QEMU_PROCESS_START_AUTODESROY constant
From: "Daniel P. Berrange"
s/VIR_QEMU_PROCESS_START_AUTODESROY/VIR_QEMU_PROCESS_START_AUTODESTROY/
Signed-off-by: Daniel P. Berrange
---
src/qemu/qemu_driver.c| 4 ++--
src/qemu/qemu_migration.c | 2 +-
src/qemu/qemu_process.c | 4 ++--
src/qemu/qemu_process.h | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8dae8f9..0f6a431 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1460,7 +1460,7 @@ static virDomainPtr qemuDomainCreate(virConnectPtr conn,
const char *xml,
if (flags & VIR_DOMAIN_START_PAUSED)
start_flags |= VIR_QEMU_PROCESS_START_PAUSED;
if (flags & VIR_DOMAIN_START_AUTODESTROY)
-start_flags |= VIR_QEMU_PROCESS_START_AUTODESROY;
+start_flags |= VIR_QEMU_PROCESS_START_AUTODESTROY;
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -5403,7 +5403,7 @@ qemuDomainObjStart(virConnectPtr conn,
unsigned int start_flags = VIR_QEMU_PROCESS_START_COLD;
start_flags |= start_paused ? VIR_QEMU_PROCESS_START_PAUSED : 0;
-start_flags |= autodestroy ? VIR_QEMU_PROCESS_START_AUTODESROY : 0;
+start_flags |= autodestroy ? VIR_QEMU_PROCESS_START_AUTODESTROY : 0;
/*
* If there is a managed saved state restore it instead of starting
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index cae58fa..a58a79d 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2124,7 +2124,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
if (qemuProcessStart(dconn, driver, vm, migrateFrom, dataFD[0], NULL, NULL,
VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START,
VIR_QEMU_PROCESS_START_PAUSED |
- VIR_QEMU_PROCESS_START_AUTODESROY) < 0) {
+ VIR_QEMU_PROCESS_START_AUTODESTROY) < 0) {
virDomainAuditStart(vm, "migrated", false);
/* Note that we don't set an error here because qemuProcessStart
* should have already done that.
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index b9fdcd2..db95d6e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3516,7 +3516,7 @@ int qemuProcessStart(virConnectPtr conn,
* but doesn't hurt to check */
virCheckFlags(VIR_QEMU_PROCESS_START_COLD |
VIR_QEMU_PROCESS_START_PAUSED |
- VIR_QEMU_PROCESS_START_AUTODESROY, -1);
+ VIR_QEMU_PROCESS_START_AUTODESTROY, -1);
cfg = virQEMUDriverGetConfig(driver);
@@ -4043,7 +4043,7 @@ int qemuProcessStart(virConnectPtr conn,
VIR_DOMAIN_PAUSED_USER);
}
-if (flags & VIR_QEMU_PROCESS_START_AUTODESROY &&
+if (flags & VIR_QEMU_PROCESS_START_AUTODESTROY &&
qemuProcessAutoDestroyAdd(driver, vm, conn) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index bc4d54d..7d0aaa8 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -47,7 +47,7 @@ int qemuProcessAssignPCIAddresses(virDomainDefPtr def);
typedef enum {
VIR_QEMU_PROCESS_START_COLD = 1 << 0,
VIR_QEMU_PROCESS_START_PAUSED = 1 << 1,
-VIR_QEMU_PROCESS_START_AUTODESROY = 1 << 2,
+VIR_QEMU_PROCESS_START_AUTODESTROY = 1 << 2,
} qemuProcessStartFlags;
int qemuProcessStart(virConnectPtr conn,
--
1.7.11.7
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: -numa doesn't (yet) support disjoint range
On 02/27/2013 01:14 AM, Michal Privoznik wrote: > On 27.02.2013 04:08, Eric Blake wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=896092 mentions that >> qemu 1.4 and earlier only accept a simple start-stop range for >> the cpu=... argument of -numa. Libvirt would attempt to use >> -numa cpu=1,3 for a disjoint range, which did not work as intended. >> >> Upstream qemu will be adding a new syntax for disjoint cpu ranges >> in 1.5; but the design for that syntax is still under discussion >> at the time of this patch. So for libvirt 1.0.3, it is safest to >> just reject attempts to build an invalid qemu command line; in the >> future, we can add a capability bit and translate to the final >> accepted design for selecting a disjoint cpu range in numa. >> >> * src/qemu/qemu_command.c (qemuBuildNumaArgStr): Reject disjoint >> ranges. >> --- >> >> Also in response to: >> https://www.redhat.com/archives/libvir-list/2013-February/msg01414.html >> >> src/qemu/qemu_command.c | 33 - >> 1 file changed, 24 insertions(+), 9 deletions(-) > > ACK I and think safe being pushed even during current freeze. Thanks; pushed. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Compile libvirt V1.0.1 error
On 02/26/2013 11:36 PM, harryxiyou wrote:
> On Wed, Feb 27, 2013 at 11:04 AM, Eric Blake wrote:
> [...]
>> You trimmed too much of your make line to tell us what was happening.
>> It might also help to run 'make V=1' to get the full command line being
>> attempted. I have to wonder if you have an unexpanded '$JAVA_HOME'
>> injected somewhere into your configure output, where make is trying to
>> compute $J (empty) followed by literal 'AVA_HOME'; maybe you should
>> figure out what in your build setup is providing that bad variable value
>> (correct would likely be '${JAVA_HOME}'). But I cannot reproduce your
>> build failure, so it is something in your environment and not in libvirt
>> itself that is causing you grief.
>>
>
> Eric, i was dizzy. Actually, i don't know why automake recognized
> $JAVA_HOME as $AVA_HOME.
Make is not like shell. In shell, {} is optional, required only if
characters after the variable name being expanded could be part of a
variable name. In make, {} (or ()) is mandatory for any variable whose
name is longer than one byte. Thus, in make, $JAVA_HOME is the same as
${J}AVA_HOME; while in shell, it is the same as ${JAVA_HOME} - quite
different. At any rate, since you didn't define $J in your makefile,
make was expanding ${J} to the empty string, which explains why your
error message then mentions AVA_HOME.
> However, i solved the problem like
> following.
>
> JVM_DIR=${shell echo ${JAVA_HOME}}
What a waste of resources - that forks a shell to do an echo every time
that you use ${JVM_DIR}. Much simpler to write as:
JVM_DIR=${JAVA_HOME}
I'm still not even sure why you need ${JVM_DIR}, and can't just use
${JAVA_HOME} directly, nor why you are trying to add pieces of Java into
libvirt (libvirt itself is written in C, not Java, for a reason). I
guess I'll just have to wait until I see a patch to understand what this
is all about. Good luck.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Fix autodestroy of QEMU guests
From: "Daniel P. Berrange"
The virQEMUCloseCallbacksRunOne method was passing a uuid string
to virDomainObjListFindByUUID, when it actually expected to get
a raw uuid buffer. This was not caught by the compiler because
the method was using a 'void *uuid' instead of first casting
it to the expected type.
This regression was accidentally caused by refactoring in
commit 568a6cda277f04ab9baaeb97490e548b7b608aa6
Author: Jiri Denemark
Date: Fri Feb 15 15:11:47 2013 +0100
qemu: Avoid deadlock in autodestroy
Signed-off-by: Daniel P. Berrange
---
src/qemu/qemu_conf.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 4f0cb18..1cd4b7c 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -805,22 +805,26 @@ struct virQEMUCloseCallbacksData {
static void
virQEMUCloseCallbacksRunOne(void *payload,
-const void *uuid,
+const void *key,
void *opaque)
{
struct virQEMUCloseCallbacksData *data = opaque;
qemuDriverCloseDefPtr closeDef = payload;
virDomainObjPtr dom;
+const char *uuidstr = key;
+unsigned char uuid[VIR_UUID_BUFLEN];
+
+if (virUUIDParse(uuidstr, uuid) < 0)
+return;
VIR_DEBUG("conn=%p, thisconn=%p, uuid=%s, cb=%p",
- closeDef->conn, data->conn, (const char *) uuid, closeDef->cb);
+ closeDef->conn, data->conn, uuidstr, closeDef->cb);
if (data->conn != closeDef->conn || !closeDef->cb)
return;
if (!(dom = virDomainObjListFindByUUID(data->driver->domains, uuid))) {
-VIR_DEBUG("No domain object with UUID %s",
- (const char *) uuid);
+VIR_DEBUG("No domain object with UUID %s", uuidstr);
return;
}
--
1.7.11.7
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Anthony Liguori writes:
> Paolo Bonzini writes:
>
>> Il 26/02/2013 20:35, Anthony Liguori ha scritto:
> >>
> >> See also discussion on multi-valued keys in command line option
> >> arguments and config files in v1 thread. Hopefully we can reach a
> >> conclusion soon, and then we'll see whether this patch is what we want.
>
> Yeah, let's drop this patch by now. I am starting to be convinced that
> "cpus=A,cpus=B,cpus=C" is the best approach. It is not pretty, but at
> least it uses generic parser code instead of yet another ad-hoc
> parser.
>>> No, we cannot rely on this behavior. We had to do it to support
>>> backwards compat with netdev but it should not be used anywhere else.
>>
>> We chose a config format (git's) because it was a good match for
>> QemuOpts, and multiple-valued operations are well supported by that
>> config format; see git-config(1). There is no reason to consider it a
>> backwards-compatibility hack.
>
> There's such thing as list support in QemuOpts. The only way
> QemuOptsVisitor was able to implement it was to expose QemuOpts publicly
> via options_int.h and rely on a implementation detail.
>
> There are fixed types supported by QemuOpts. It just so happens that
> whenever qemu_opt_set() is called, instead of replacing the last
> instance, the value is either prepended or appended in order to
> implement a replace or set-if-unset behavior.
>
> All values are treated this way. So the above "list" would only be:
>
> {
> .name = "cpus",
> .type = QEMU_OPT_STRING
> }
>
> Which is indistinguishable from a straight string property. This means
> it's impossible to introspect because the type is context-sensitive.
>
> What's more, there is no API outside of QemuOptsVisitor that can
> actually work with "lists" of QemuOpts values.
There is: qemu_opt_foreach()
If you relax your claim to "QemuOpts API for lists sucks and needs
improvement", then we're in violent agreement.
> If we want to have list syntax, we need to introduce first class support
> for it. Here's a simple example of how to do this. Obviously we would
> want to introduce some better error checking so we can catch if someone
> tries to access a list with a non-list accessor. We also would need
> list accessor functions.
>
> But it's really not hard at all.
>
> (Only compile tested)
>
>
>>From 4ee7ed36d597f0defd78baac7480ecb29e11e1c1 Mon Sep 17 00:00:00 2001
> From: Anthony Liguori
> Date: Wed, 27 Feb 2013 09:32:09 -0600
> Subject: [PATCH] qemu-opts: support lists
>
> Signed-off-by: Anthony Liguori
> ---
> include/qemu/option.h | 2 ++
> util/qemu-option.c| 12
> 2 files changed, 14 insertions(+)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index ba197cd..e4808c3 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -41,6 +41,7 @@ enum QEMUOptionParType {
> typedef struct QEMUOptionParameter {
> const char *name;
> enum QEMUOptionParType type;
> +bool list;
> union {
> uint64_t n;
> char* s;
> @@ -95,6 +96,7 @@ enum QemuOptType {
> typedef struct QemuOptDesc {
> const char *name;
> enum QemuOptType type;
> +bool list;
> const char *help;
> } QemuOptDesc;
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 5a1d03c..6b1ae6e 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -636,6 +636,18 @@ static void opt_set(QemuOpts *opts, const char *name,
> const char *value,
> return;
> }
>
> +if (desc->list && strchr(value, ':')) {
> +gchar **tokens = g_strsplit(value, ":", 0);
> +int i;
> +for (i = 0; tokens[i]; i++) {
> +opt_set(opts, name, tokens[i], prepend, errp);
> +if (error_is_set(errp)) {
> +return;
> +}
> +}
> +g_strfreev(tokens);
> +}
> +
> opt = g_malloc0(sizeof(*opt));
> opt->name = g_strdup(name);
> opt->opts = opts;
No, no, no. This makes ':' special, which means you can't have lists of
anything containing ':'. Your cure is worse than the disease. Let go
of that syntactic high-fructose corn syrup, stick to what we have and
works just fine, thank you.
Then add suitable list accessor functions and error checks.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
On Wed, Feb 27, 2013 at 04:57:15PM +0100, Paolo Bonzini wrote: > Il 27/02/2013 16:42, Anthony Liguori ha scritto: > > There's such thing as list support in QemuOpts. The only way > > QemuOptsVisitor was able to implement it was to expose QemuOpts publicly > > via options_int.h and rely on a implementation detail. > > > > There are fixed types supported by QemuOpts. It just so happens that > > whenever qemu_opt_set() is called, instead of replacing the last > > instance, the value is either prepended or appended in order to > > implement a replace or set-if-unset behavior. > > Fair enough. Nobody said the implementation is pretty. > > > If we want to have list syntax, we need to introduce first class support > > for it. Here's a simple example of how to do this. > > If it is meant as a prototype only, and the final command-line syntax > would be with repeated keys, that's okay. I think that Eduardo/Markus/I > are focusing on the user interface, you're focusing in the implementation. > > In the meanwhile, however, it seems to me that Eduardo can use > QemuOptsVisitor---which can also hide the details and provide type safety. Whatever I use to implement it, I still need to know how the command-line syntax will look like, because we need to tell libvirt developers how they should write the QEMU command-line. -- Eduardo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
On Wed, Feb 27, 2013 at 09:42:50AM -0600, Anthony Liguori wrote:
> Paolo Bonzini writes:
>
> > Il 26/02/2013 20:35, Anthony Liguori ha scritto:
> >>
> >> See also discussion on multi-valued keys in command line option
> >> arguments and config files in v1 thread. Hopefully we can reach a
> >> conclusion soon, and then we'll see whether this patch is what we
> >> want.
> >>> >
> >>> > Yeah, let's drop this patch by now. I am starting to be convinced that
> >>> > "cpus=A,cpus=B,cpus=C" is the best approach. It is not pretty, but at
> >>> > least it uses generic parser code instead of yet another ad-hoc
> >>> > parser.
> >> No, we cannot rely on this behavior. We had to do it to support
> >> backwards compat with netdev but it should not be used anywhere else.
> >
> > We chose a config format (git's) because it was a good match for
> > QemuOpts, and multiple-valued operations are well supported by that
> > config format; see git-config(1). There is no reason to consider it a
> > backwards-compatibility hack.
>
> There's such thing as list support in QemuOpts.
[skipping stuff about internal implementation details that don't matter]
>
> If we want to have list syntax, we need to introduce first class support
> for it.
Absolutely. But how the syntax for it should look like?
> Here's a simple example of how to do this. Obviously we would
> want to introduce some better error checking so we can catch if someone
> tries to access a list with a non-list accessor. We also would need
> list accessor functions.
>
> But it's really not hard at all.
Of course. Once we decide how the syntax will look like, implementing
it should be very easy. But as far as I can see, we were trying to
discuss what's the appropriate syntax, here.
I still don't see why "option=A:B:C" with no possibility of having list
items containing ":" (like your proposal below) is a better generic
syntax for lists than "option=A,option=B,option=C".
> From 4ee7ed36d597f0defd78baac7480ecb29e11e1c1 Mon Sep 17 00:00:00 2001
> From: Anthony Liguori
> Date: Wed, 27 Feb 2013 09:32:09 -0600
> Subject: [PATCH] qemu-opts: support lists
>
> Signed-off-by: Anthony Liguori
> ---
> include/qemu/option.h | 2 ++
> util/qemu-option.c| 12
> 2 files changed, 14 insertions(+)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index ba197cd..e4808c3 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -41,6 +41,7 @@ enum QEMUOptionParType {
> typedef struct QEMUOptionParameter {
> const char *name;
> enum QEMUOptionParType type;
> +bool list;
> union {
> uint64_t n;
> char* s;
> @@ -95,6 +96,7 @@ enum QemuOptType {
> typedef struct QemuOptDesc {
> const char *name;
> enum QemuOptType type;
> +bool list;
> const char *help;
> } QemuOptDesc;
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 5a1d03c..6b1ae6e 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -636,6 +636,18 @@ static void opt_set(QemuOpts *opts, const char *name,
> const char *value,
> return;
> }
>
> +if (desc->list && strchr(value, ':')) {
> +gchar **tokens = g_strsplit(value, ":", 0);
> +int i;
> +for (i = 0; tokens[i]; i++) {
> +opt_set(opts, name, tokens[i], prepend, errp);
> +if (error_is_set(errp)) {
> +return;
> +}
> +}
> +g_strfreev(tokens);
> +}
> +
> opt = g_malloc0(sizeof(*opt));
> opt->name = g_strdup(name);
> opt->opts = opts;
> --
> 1.8.0
>
>
> Regards,
>
> Anthony Liguori
>
> >
> > Paolo
--
Eduardo
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Il 27/02/2013 16:42, Anthony Liguori ha scritto: > There's such thing as list support in QemuOpts. The only way > QemuOptsVisitor was able to implement it was to expose QemuOpts publicly > via options_int.h and rely on a implementation detail. > > There are fixed types supported by QemuOpts. It just so happens that > whenever qemu_opt_set() is called, instead of replacing the last > instance, the value is either prepended or appended in order to > implement a replace or set-if-unset behavior. Fair enough. Nobody said the implementation is pretty. > If we want to have list syntax, we need to introduce first class support > for it. Here's a simple example of how to do this. If it is meant as a prototype only, and the final command-line syntax would be with repeated keys, that's okay. I think that Eduardo/Markus/I are focusing on the user interface, you're focusing in the implementation. In the meanwhile, however, it seems to me that Eduardo can use QemuOptsVisitor---which can also hide the details and provide type safety. Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Release candidate 2 of 1.0.3 is available
On Wed, Feb 27, 2013 at 03:26:24PM +0100, Michal Privoznik wrote:
> On 27.02.2013 12:20, Daniel Veillard wrote:
> >I have just tagged git and pushed the tarball. The rpms for F17
> > are at the usual place too:
> > ftp://libvirt.org/libvirt/
> >
> >I gave a try to the set of rpms and this seems to work fine for
> > relatively simple tests.
> >Please give it more testing and let's keep change to git to
> > bug fixes to minimize risks for 1.0.3.
> > If everything goes well I will push 1.0.3 on Friday,
> >
> > thanks !
> >
> > Daniel
> >
>
> Maybe we want to postpone the release a bit; I was trying to find out how
> drop of qemu driver lock speeds up parallel startup of multiple domains,
> however - it is not working.
>
> I've created small test program - test4.c and run it against current HEAD. It
> takes arguments - domain names - which it spawns a separate thread per each
> domain and repeatedly starts and destroys domains. See code for more.
>
> Then, I've created 20 copies of diskless domain (attached as well):
> test1..test20 and run my test4 program over them:
>
> ./test4 $(for ((i=1; i<21; i++)); do echo -n "test$i "; done)
>
> I had to tweak max_client_requests=20 to really run those 20 requests in
> parallel.
>
> However, what I am getting instead of nice measuring of speedup is:
>
> [9722] Starting test1 (round 20) ...[9725] Starting test4 (round 20)
> ...[9724] Starting test3 (round 20) ...[9723] Starting test2 (round 20)
> ...[9726] Starting test5 (round 20) ...[9727] Starting test6 (round 20)
> ...[9728] Starting test7 (round 20) ...[9729] Starting test8 (round 20)
> ...[9730] Starting test9 (round 20) ...[9731] Starting test10 (round 20)
> ...[9732] Starting test11 (round 20) ...[9733] Starting test12 (round 20)
> ...[9734] Starting test13 (round 20) ...[9735] Starting test14 (round 20)
> ...[9736] Starting test15 (round 20) ...[9737] Starting test16 (round 20)
> ...[9738] Starting test17 (round 20) ...[9741] Starting test20 (round 20)
> ...[9739] Starting test18 (round 20) ...[9740] Starting test19 (round 20)
> ...libvir: error : An error occurred, but the cause is unknown
> Unable to start domain test12libvir: error : An error occurred, but the
> cause is unknown
> Unable to start domain test14libvir: error : An error occurred, but the
> cause is unknown
> Unable to start domain test7libvir: error : An error occurred, but the cause
> is unknown
> Unable to start domain test1libvir: QEMU Driver error : Requested operation
> is not valid: domain is not running
> Unable to destroy domain test12[9733] Destroying test12 ...[9728] Destroying
> test7 ...[9735] Destroying test14 ...[9733] Starting test12 (round 19)
> ...libvir: QEMU Driver error : Requested operation is not valid: domain is
> not running
> Unable to destroy domain test7[9728] Starting test7 (round 19) ...libvir:
> QEMU Driver error : Requested operation is not valid: domain is not running
> Unable to destroy domain test14[9735] Starting test14 (round 19) ...libvir:
> error : An error occurred, but the cause is unknown
> Unable to start domain test18libvir: QEMU Driver error : Requested operation
> is not valid: domain is not running
> Unable to destroy domain test1[9722] Destroying test1 ...[9722] Starting
> test1 (round 19) ...libvir: error : An error occurred, but the cause is
> unknown
> Unable to start domain test8libvir: error : An error occurred, but the cause
> is unknown
> Unable to start domain test15[9725] Done
I have my own parallel start/destroy test case that I ran during development
of the threading code to test it. It caught quite a few bugs, and I had it
run 8 threads for approximately 80,000 start/stops in total. I'm not seeing
any errors running it with current GIT though.
I've also tested your demo program with 8 threads / VMs, and it succeeeded
without any problems.
Appending my demo program - you don't need to do any setup to run it,
it will auto-create the storage & use transient guests
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
#define _GNU_SOURCE
#include
#include
#include
#include
#include
#include
#include
#include
#define NUM_THREADS 8
#define MAX_VMS 1000
const char *XML = ""
" vm%06d"
" 220160"
" 6"
" 1"
" "
"hvm"
""
" "
" "
""
" "
" "
"/bin/qemu-kvm"
""
" "
" "
" "
" "
" "
""
""
" "
""
""
""
" "
"";
static pid_t gettid(void)
{
return syscall(SYS_gettid);
}
static void *one_thread(void *arg)
{
int i;
virConnectPtr conn = arg;
virConnectRef(conn);
char *cmd;
if (asprintf(&cmd, "qemu-img create -f qcow2
/home/berrange/VirtualMachines/vm%06d.qcow 1G",
Re: [libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Paolo Bonzini writes:
> Il 26/02/2013 20:35, Anthony Liguori ha scritto:
>>
>> See also discussion on multi-valued keys in command line option
>> arguments and config files in v1 thread. Hopefully we can reach a
>> conclusion soon, and then we'll see whether this patch is what we want.
>>> >
>>> > Yeah, let's drop this patch by now. I am starting to be convinced that
>>> > "cpus=A,cpus=B,cpus=C" is the best approach. It is not pretty, but at
>>> > least it uses generic parser code instead of yet another ad-hoc
>>> > parser.
>> No, we cannot rely on this behavior. We had to do it to support
>> backwards compat with netdev but it should not be used anywhere else.
>
> We chose a config format (git's) because it was a good match for
> QemuOpts, and multiple-valued operations are well supported by that
> config format; see git-config(1). There is no reason to consider it a
> backwards-compatibility hack.
There's such thing as list support in QemuOpts. The only way
QemuOptsVisitor was able to implement it was to expose QemuOpts publicly
via options_int.h and rely on a implementation detail.
There are fixed types supported by QemuOpts. It just so happens that
whenever qemu_opt_set() is called, instead of replacing the last
instance, the value is either prepended or appended in order to
implement a replace or set-if-unset behavior.
All values are treated this way. So the above "list" would only be:
{
.name = "cpus",
.type = QEMU_OPT_STRING
}
Which is indistinguishable from a straight string property. This means
it's impossible to introspect because the type is context-sensitive.
What's more, there is no API outside of QemuOptsVisitor that can
actually work with "lists" of QemuOpts values.
If we want to have list syntax, we need to introduce first class support
for it. Here's a simple example of how to do this. Obviously we would
want to introduce some better error checking so we can catch if someone
tries to access a list with a non-list accessor. We also would need
list accessor functions.
But it's really not hard at all.
(Only compile tested)
>From 4ee7ed36d597f0defd78baac7480ecb29e11e1c1 Mon Sep 17 00:00:00 2001
From: Anthony Liguori
Date: Wed, 27 Feb 2013 09:32:09 -0600
Subject: [PATCH] qemu-opts: support lists
Signed-off-by: Anthony Liguori
---
include/qemu/option.h | 2 ++
util/qemu-option.c| 12
2 files changed, 14 insertions(+)
diff --git a/include/qemu/option.h b/include/qemu/option.h
index ba197cd..e4808c3 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -41,6 +41,7 @@ enum QEMUOptionParType {
typedef struct QEMUOptionParameter {
const char *name;
enum QEMUOptionParType type;
+bool list;
union {
uint64_t n;
char* s;
@@ -95,6 +96,7 @@ enum QemuOptType {
typedef struct QemuOptDesc {
const char *name;
enum QemuOptType type;
+bool list;
const char *help;
} QemuOptDesc;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 5a1d03c..6b1ae6e 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -636,6 +636,18 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
return;
}
+if (desc->list && strchr(value, ':')) {
+gchar **tokens = g_strsplit(value, ":", 0);
+int i;
+for (i = 0; tokens[i]; i++) {
+opt_set(opts, name, tokens[i], prepend, errp);
+if (error_is_set(errp)) {
+return;
+}
+}
+g_strfreev(tokens);
+}
+
opt = g_malloc0(sizeof(*opt));
opt->name = g_strdup(name);
opt->opts = opts;
--
1.8.0
Regards,
Anthony Liguori
>
> Paolo
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Release candidate 2 of 1.0.3 is available
On 27.02.2013 15:47, Daniel Veillard wrote: > On Wed, Feb 27, 2013 at 03:26:24PM +0100, Michal Privoznik wrote: > Just a side note. Running git-bisect gets me to this: a9e97e0c3057dfaefc7217b7db7fbba001cf8f0b is the first bad commit commit a9e97e0c3057dfaefc7217b7db7fbba001cf8f0b Author: Daniel P. Berrange Date: Wed Feb 6 18:17:20 2013 + Remove qemuDriverLock from almost everywhere With the majority of fields in the virQEMUDriverPtr struct now immutable or self-locking, there is no need for practically any methods to be using the QEMU driver lock. Only a handful of helper APIs in qemu_conf.c now need it But I think it just expose the bug we had earlier since 2 startup processes are not serialized now. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Release candidate 2 of 1.0.3 is available
On Wed, Feb 27, 2013 at 03:26:24PM +0100, Michal Privoznik wrote: > On 27.02.2013 12:20, Daniel Veillard wrote: > >I have just tagged git and pushed the tarball. The rpms for F17 > > are at the usual place too: > > ftp://libvirt.org/libvirt/ > > > >I gave a try to the set of rpms and this seems to work fine for > > relatively simple tests. > >Please give it more testing and let's keep change to git to > > bug fixes to minimize risks for 1.0.3. > > If everything goes well I will push 1.0.3 on Friday, > > > > thanks ! > > > > Daniel > > > > Maybe we want to postpone the release a bit; I was trying to find out how > drop of qemu driver lock speeds up parallel startup of multiple domains, > however - it is not working. > > I've created small test program - test4.c and run it against current HEAD. It > takes arguments - domain names - which it spawns a separate thread per each > domain and repeatedly starts and destroys domains. See code for more. > > Then, I've created 20 copies of diskless domain (attached as well): > test1..test20 and run my test4 program over them: > > ./test4 $(for ((i=1; i<21; i++)); do echo -n "test$i "; done) > > I had to tweak max_client_requests=20 to really run those 20 requests in > parallel. > > However, what I am getting instead of nice measuring of speedup is: > > [9722] Starting test1 (round 20) ...[9725] Starting test4 (round 20) > ...[9724] Starting test3 (round 20) ...[9723] Starting test2 (round 20) > ...[9726] Starting test5 (round 20) ...[9727] Starting test6 (round 20) > ...[9728] Starting test7 (round 20) ...[9729] Starting test8 (round 20) > ...[9730] Starting test9 (round 20) ...[9731] Starting test10 (round 20) > ...[9732] Starting test11 (round 20) ...[9733] Starting test12 (round 20) > ...[9734] Starting test13 (round 20) ...[9735] Starting test14 (round 20) > ...[9736] Starting test15 (round 20) ...[9737] Starting test16 (round 20) > ...[9738] Starting test17 (round 20) ...[9741] Starting test20 (round 20) > ...[9739] Starting test18 (round 20) ...[9740] Starting test19 (round 20) > ...libvir: error : An error occurred, but the cause is unknown > Unable to start domain test12libvir: error : An error occurred, but the > cause is unknown > Unable to start domain test14libvir: error : An error occurred, but the > cause is unknown > Unable to start domain test7libvir: error : An error occurred, but the cause > is unknown > Unable to start domain test1libvir: QEMU Driver error : Requested operation > is not valid: domain is not running > Unable to destroy domain test12[9733] Destroying test12 ...[9728] Destroying > test7 ...[9735] Destroying test14 ...[9733] Starting test12 (round 19) > ...libvir: QEMU Driver error : Requested operation is not valid: domain is > not running > Unable to destroy domain test7[9728] Starting test7 (round 19) ...libvir: > QEMU Driver error : Requested operation is not valid: domain is not running > Unable to destroy domain test14[9735] Starting test14 (round 19) ...libvir: > error : An error occurred, but the cause is unknown > Unable to start domain test18libvir: QEMU Driver error : Requested operation > is not valid: domain is not running > Unable to destroy domain test1[9722] Destroying test1 ...[9722] Starting > test1 (round 19) ...libvir: error : An error occurred, but the cause is > unknown > Unable to start domain test8libvir: error : An error occurred, but the cause > is unknown > Unable to start domain test15[9725] Done > > > etc. > > Unfortunately, I caught this after freeze. No problem, freeze is there to find bugs, you found a serious bug, let's fix it before the release. [...] > //sleep(20); > sleep(1); does sleep(20); there avoid the problem ? There is a failure in guest start but we don't get a proper description There is no urging deadline, except well for the pending patches waiting for the end of the freeze, if we can fix this by Monday, then that would be IMHO just fine. If on Monday we still don't have a solution let's discuss the status of the analysis, okay ? Daniel -- Daniel Veillard | Open Source and Standards, Red Hat veill...@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Release candidate 2 of 1.0.3 is available
On 27.02.2013 12:20, Daniel Veillard wrote:
>I have just tagged git and pushed the tarball. The rpms for F17
> are at the usual place too:
> ftp://libvirt.org/libvirt/
>
>I gave a try to the set of rpms and this seems to work fine for
> relatively simple tests.
>Please give it more testing and let's keep change to git to
> bug fixes to minimize risks for 1.0.3.
> If everything goes well I will push 1.0.3 on Friday,
>
> thanks !
>
> Daniel
>
Maybe we want to postpone the release a bit; I was trying to find out how drop
of qemu driver lock speeds up parallel startup of multiple domains, however -
it is not working.
I've created small test program - test4.c and run it against current HEAD. It
takes arguments - domain names - which it spawns a separate thread per each
domain and repeatedly starts and destroys domains. See code for more.
Then, I've created 20 copies of diskless domain (attached as well):
test1..test20 and run my test4 program over them:
./test4 $(for ((i=1; i<21; i++)); do echo -n "test$i "; done)
I had to tweak max_client_requests=20 to really run those 20 requests in
parallel.
However, what I am getting instead of nice measuring of speedup is:
[9722] Starting test1 (round 20) ...[9725] Starting test4 (round 20) ...[9724]
Starting test3 (round 20) ...[9723] Starting test2 (round 20) ...[9726]
Starting test5 (round 20) ...[9727] Starting test6 (round 20) ...[9728]
Starting test7 (round 20) ...[9729] Starting test8 (round 20) ...[9730]
Starting test9 (round 20) ...[9731] Starting test10 (round 20) ...[9732]
Starting test11 (round 20) ...[9733] Starting test12 (round 20) ...[9734]
Starting test13 (round 20) ...[9735] Starting test14 (round 20) ...[9736]
Starting test15 (round 20) ...[9737] Starting test16 (round 20) ...[9738]
Starting test17 (round 20) ...[9741] Starting test20 (round 20) ...[9739]
Starting test18 (round 20) ...[9740] Starting test19 (round 20) ...libvir:
error : An error occurred, but the cause is unknown
Unable to start domain test12libvir: error : An error occurred, but the cause
is unknown
Unable to start domain test14libvir: error : An error occurred, but the cause
is unknown
Unable to start domain test7libvir: error : An error occurred, but the cause
is unknown
Unable to start domain test1libvir: QEMU Driver error : Requested operation is
not valid: domain is not running
Unable to destroy domain test12[9733] Destroying test12 ...[9728] Destroying
test7 ...[9735] Destroying test14 ...[9733] Starting test12 (round 19)
...libvir: QEMU Driver error : Requested operation is not valid: domain is not
running
Unable to destroy domain test7[9728] Starting test7 (round 19) ...libvir: QEMU
Driver error : Requested operation is not valid: domain is not running
Unable to destroy domain test14[9735] Starting test14 (round 19) ...libvir:
error : An error occurred, but the cause is unknown
Unable to start domain test18libvir: QEMU Driver error : Requested operation is
not valid: domain is not running
Unable to destroy domain test1[9722] Destroying test1 ...[9722] Starting test1
(round 19) ...libvir: error : An error occurred, but the cause is unknown
Unable to start domain test8libvir: error : An error occurred, but the cause
is unknown
Unable to start domain test15[9725] Done
etc.
Unfortunately, I caught this after freeze.
Michal
#define _GNU_SOURCE
#include
#include
#include
#include
#include
#include
#include
virDomainPtr *doms = NULL;
int doms_size = 0;
int rep = 20;
int quit = 0;
void *
eventLoop(void *opaque) {
while (!quit) {
if (virEventRunDefaultImpl() < 0)
fprintf(stderr, "unable to run eventloop");
}
return NULL;
}
void *
thread(void *opaque) {
int i = (int) opaque;
int repLocal = rep;
virDomainPtr dom = doms[i];
const char *domname = virDomainGetName(dom);
pid_t tid = syscall(SYS_gettid);
for (; repLocal; repLocal--) {
printf("[%lu] Starting %s (round %d) ...", (unsigned long) tid, domname, repLocal);
fflush(stdout);
if (virDomainCreate(dom) < 0) {
fprintf(stderr, "Unable to start domain %s", domname);
} else {
printf("[%lu] Done\n", (unsigned long) tid);
}
//sleep(20);
sleep(1);
printf("[%lu] Destroying %s ...", (unsigned long) tid, domname);
if (virDomainDestroy(dom) < 0) {
fprintf(stderr, "Unable to destroy domain %s", domname);
} else {
printf("[%lu] Done\n", (unsigned long) tid);
}
}
cleanup:
return NULL;
}
int main(int argc, char *argv[]) {
virConnectPtr conn = virConnectOpen("qemu:///system");
pthread_t *thr = NULL;
pthread_t eventLoopTh;
int i;
if (!conn)
return EXIT_FAILURE;
virEventRegisterDefaultImpl();
i = 1;
while (argv[i]) {
virDomainPtr dom = virDomainLookupByName(conn, argv[i]);
if (!dom) {
fprintf(stderr, "no
Re: [libvirt] [PATCH] util: Add docs for virXMLProp string
On 02/27/13 12:05, Michal Privoznik wrote: On 21.02.2013 11:13, Peter Krempa wrote: To avoid confusion about usage of this function explicitly document that this function returns copy of the attribute string. --- src/util/virxml.c | 10 ++ 1 file changed, 10 insertions(+) ... const char *name) ACK and safe to push now. Pushed. Thanks. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Clear PIIX3/PIIX4_USB capabilities for non-X86 platforms
On Wed, Feb 27, 2013 at 19:52:22 +0800, Li Zhang wrote:
> From: Li Zhang
>
> Currently, PIIX3/PIIX4_USB capabilities are enabled for other platforms.
> Actually, it is only supported for X86.
>
> So this patch is to clear the capabilities for non-X86 platforms.
>
...
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 40022c1..ef5c69a 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -1307,8 +1307,11 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[]
> = {
> { "hda-micro", QEMU_CAPS_HDA_MICRO },
> { "ccid-card-emulated", QEMU_CAPS_CCID_EMULATED },
> { "ccid-card-passthru", QEMU_CAPS_CCID_PASSTHRU },
> +#if defined (__x86_64__) || \
> +defined (__i386__)
> { "piix3-usb-uhci", QEMU_CAPS_PIIX3_USB_UHCI },
> { "piix4-usb-uhci", QEMU_CAPS_PIIX4_USB_UHCI },
> +#endif
> { "usb-ehci", QEMU_CAPS_USB_EHCI },
> { "ich9-usb-ehci1", QEMU_CAPS_ICH9_USB_EHCI1 },
> { "vt82c686b-usb-uhci", QEMU_CAPS_VT82C686B_USB_UHCI },
NACK. QEMU capabilities depend on the binary we are going to use
(emulator tag in domain XML), they don't depend on host architecture.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] ANNOUNCE: Perl binding Sys-Virt release 1.0.2
I am pleased to announce that release 1.0.2 of Sys-Virt, the libvirt Perl API binding is now available for download: http://search.cpan.org/CPAN/authors/id/D/DA/DANBERR/Sys-Virt-1.0.2.tar.gz Changed in this release: - Add all new APIs and constants in libvirt 1.0.2 Further information, including online API documentation & previous release archives, is available from the CPAN home page http://search.cpan.org/dist/Sys-Virt/ Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 9/8] hellolibvirt: Adjust some comments and condition usage
On 02/26/2013 07:54 PM, Eric Blake wrote:
> On 02/26/2013 07:20 AM, John Ferlan wrote:
>
> I would have put '---' here, since...
>
The information was only in the email not part of the git history.
In the future I'll remember to put that after the '---'.
>> examples/hellolibvirt/hellolibvirt.c | 26 ++
>> 1 file changed, 14 insertions(+), 12 deletions(-)
>>
>
>> @@ -93,7 +94,7 @@ showDomains(virConnectPtr conn)
>> char **nameList = NULL;
>>
>> numActiveDomains = virConnectNumOfDomains(conn);
>> -if (-1 == numActiveDomains) {
>> +if (numActiveDomains == -1) {
>> ret = 1;
>> printf("Failed to get number of active domains\n");
>> showError(conn);
>
> virConnectNumOfDomains is inherently racy. Wouldn't it be better to
> just drop this section of our example?
>
Considering the back and forth I decided to make "less" change because
it's just an example, but I suppose I took too much off the top.
So below are proposed adjustments.
>> @@ -101,7 +102,7 @@ showDomains(virConnectPtr conn)
>> }
>>
>> numInactiveDomains = virConnectNumOfDefinedDomains(conn);
>> -if (-1 == numInactiveDomains) {
>> +if (numInactiveDomains == -1) {
>> ret = 1;
>> printf("Failed to get number of inactive domains\n");
>> showError(conn);
>
> Same question.
>
>> @@ -113,17 +114,20 @@ showDomains(virConnectPtr conn)
>>
>> nameList = malloc(sizeof(*nameList) * numInactiveDomains);
>>
>> -if (NULL == nameList) {
>> +if (!nameList) {
>> ret = 1;
>> printf("Could not allocate memory for list of inactive domains\n");
>> goto out;
>> }
>>
>> +/* The virConnectListDomains() will return a list of the active domains.
>> + * Alternatively the virConnectListAllDomains() API would return a list
>> + * of both active and inactive domains */
>> numNames = virConnectListDefinedDomains(conn,
>> nameList,
>> numInactiveDomains);
>
> I think it would be better to update the example to JUST use
> virConnectListAllDomains(), and completely avoid
> virConnectListDefinedDomains.
>
>>
>> -if (-1 == numNames) {
>> +if (numNames == -1) {
>> ret = 1;
>> printf("Could not get list of defined domains from hypervisor\n");
>> showError(conn);
>> @@ -136,9 +140,7 @@ showDomains(virConnectPtr conn)
>>
>> for (i = 0 ; i < numNames ; i++) {
>> printf(" %s\n", *(nameList + i));
>> -/* The API documentation doesn't say so, but the names
>> - * returned by virConnectListDefinedDomains are strdup'd and
>> - * must be freed here. */
>> +/* must free the returned named per the API documentation */
>> free(*(nameList + i));
>
> Pre-existing, but '*(nameList + i)' looks odd; 'nameList[i]' looks nicer.
>
Here's the differences from the v3 to what seems to be a happy medium.
The virConnectNum* functions are still used just to "show" they exist
and how to use them. There's a comment before the usage.
diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellol
index 335a75e..26dd67f 100644
--- a/examples/hellolibvirt/hellolibvirt.c
+++ b/examples/hellolibvirt/hellolibvirt.c
@@ -91,8 +91,14 @@ static int
showDomains(virConnectPtr conn)
{
int ret = 0, i, numNames, numInactiveDomains, numActiveDomains;
-char **nameList = NULL;
-
+int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE;
+virDomainPtr *nameList = NULL;
+ * the current call. A domain could be started or stopped and any
+ * assumptions made purely on these return values could result in
+ * unexpected results */
numActiveDomains = virConnectNumOfDomains(conn);
if (numActiveDomains == -1) {
ret = 1;
@@ -112,40 +118,24 @@ showDomains(virConnectPtr conn)
printf("There are %d active and %d inactive domains\n",
numActiveDomains, numInactiveDomains);
-nameList = malloc(sizeof(*nameList) * numInactiveDomains);
-
-if (!nameList) {
-ret = 1;
-printf("Could not allocate memory for list of inactive domains\n");
-goto out;
-}
-
-/* The virConnectListDomains() will return a list of the active domains.
- * Alternatively the virConnectListAllDomains() API would return a list
- * of both active and inactive domains */
-numNames = virConnectListDefinedDomains(conn,
-nameList,
-numInactiveDomains);
-
-if (numNames == -1) {
-ret = 1;
-printf("Could not get list of defined domains from hypervisor\n");
-showError(conn);
-goto out;
-}
-
-if (numNames > 0) {
-printf("Inactive domains:\n");
-}
-
-for (i = 0 ; i < numNames ; i++) {
Re: [libvirt] [PATCH 1/1] Clear PIIX3/PIIX4_USB capabilities for non-X86 platforms
Hi Eric,
This is also one bug-fix, could you help review and push to 1.0.3?
Thanks. :-)
On Wed, Feb 27, 2013 at 7:52 PM, Li Zhang wrote:
> From: Li Zhang
>
> Currently, PIIX3/PIIX4_USB capabilities are enabled for other platforms.
> Actually, it is only supported for X86.
>
> So this patch is to clear the capabilities for non-X86 platforms.
>
> Signed-off-by: Li Zhang
> ---
> src/conf/domain_conf.c |1 +
> src/qemu/qemu_capabilities.c |3 +++
> src/qemu/qemu_command.c |2 ++
> 3 files changed, 6 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 995cf0c..d57334a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -10453,6 +10453,7 @@ virDomainDefParseXML(virCapsPtr caps,
> VIR_FREE(nodes);
>
> /* If graphics are enabled, there's an implicit PS2 mouse */
> +/* Todo: Add implicit USB mouse and keyboard for ppc64 */
> if (def->ngraphics > 0) {
> virDomainInputDefPtr input;
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 40022c1..ef5c69a 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -1307,8 +1307,11 @@ struct virQEMUCapsStringFlags
> virQEMUCapsObjectTypes[] = {
> { "hda-micro", QEMU_CAPS_HDA_MICRO },
> { "ccid-card-emulated", QEMU_CAPS_CCID_EMULATED },
> { "ccid-card-passthru", QEMU_CAPS_CCID_PASSTHRU },
> +#if defined (__x86_64__) || \
> +defined (__i386__)
> { "piix3-usb-uhci", QEMU_CAPS_PIIX3_USB_UHCI },
> { "piix4-usb-uhci", QEMU_CAPS_PIIX4_USB_UHCI },
> +#endif
> { "usb-ehci", QEMU_CAPS_USB_EHCI },
> { "ich9-usb-ehci1", QEMU_CAPS_ICH9_USB_EHCI1 },
> { "vt82c686b-usb-uhci", QEMU_CAPS_VT82C686B_USB_UHCI },
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 1c9bfc9..0b18be0 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5784,6 +5784,8 @@ qemuBuildCommandLine(virConnectPtr conn,
> } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
> cont->model == -1 &&
> !virQEMUCapsGet(qemuCaps,
> QEMU_CAPS_PIIX3_USB_UHCI)) {
> +/* usblegacy is used for ppc64 temporarily */
> +/* Todo: support -device xxx on ppc64 platform */
> if (usblegacy) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Multiple legacy USB controllers
> are "
> --
> 1.7.10.1
>
>
--
Best Regards
-Li
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Remove contiguous CPU indexes assumption
Hi Eric,
This should belong to bug-fix, could it be pushed to 1.0.3?
Thanks.
On Wed, Feb 27, 2013 at 11:18 AM, Li Zhang wrote:
> From: Li Zhang
>
> When getting CPUs' information, it assumes that CPU indexes
> are not contiguous. But for ppc64 platform, CPU indexes are not
> contiguous because SMT is needed to be disabled, so CPU information
> is not right on ppc64 and vpuinfo, vcpupin can't work corretly.
>
> This patch is to remove the assumption to be compatible with ppc64.
>
> Test:
>4 vcpus are assigned to one VM and execute vcpuinfo command.
>
>Without patch: There is only one vcpu informaion can be listed.
>With patch: All vcpus' information can be listed correctly.
>
> Signed-off-by: Li Zhang
> ---
> src/qemu/qemu_monitor_json.c |7 ---
> 1 file changed, 7 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 9991a0a..e130f8c 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1230,13 +1230,6 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply,
> goto cleanup;
> }
>
> -if (cpu != i) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unexpected cpu index %d expecting %d"),
> - i, cpu);
> -goto cleanup;
> -}
> -
> threads[i] = thread;
> }
>
> --
> 1.7.10.1
>
>
--
Best Regards
-Li
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/1] Clear PIIX3/PIIX4_USB capabilities for non-X86 platforms
From: Li Zhang
Currently, PIIX3/PIIX4_USB capabilities are enabled for other platforms.
Actually, it is only supported for X86.
So this patch is to clear the capabilities for non-X86 platforms.
Signed-off-by: Li Zhang
---
src/conf/domain_conf.c |1 +
src/qemu/qemu_capabilities.c |3 +++
src/qemu/qemu_command.c |2 ++
3 files changed, 6 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 995cf0c..d57334a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10453,6 +10453,7 @@ virDomainDefParseXML(virCapsPtr caps,
VIR_FREE(nodes);
/* If graphics are enabled, there's an implicit PS2 mouse */
+/* Todo: Add implicit USB mouse and keyboard for ppc64 */
if (def->ngraphics > 0) {
virDomainInputDefPtr input;
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 40022c1..ef5c69a 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1307,8 +1307,11 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] =
{
{ "hda-micro", QEMU_CAPS_HDA_MICRO },
{ "ccid-card-emulated", QEMU_CAPS_CCID_EMULATED },
{ "ccid-card-passthru", QEMU_CAPS_CCID_PASSTHRU },
+#if defined (__x86_64__) || \
+defined (__i386__)
{ "piix3-usb-uhci", QEMU_CAPS_PIIX3_USB_UHCI },
{ "piix4-usb-uhci", QEMU_CAPS_PIIX4_USB_UHCI },
+#endif
{ "usb-ehci", QEMU_CAPS_USB_EHCI },
{ "ich9-usb-ehci1", QEMU_CAPS_ICH9_USB_EHCI1 },
{ "vt82c686b-usb-uhci", QEMU_CAPS_VT82C686B_USB_UHCI },
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1c9bfc9..0b18be0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5784,6 +5784,8 @@ qemuBuildCommandLine(virConnectPtr conn,
} else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
cont->model == -1 &&
!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_PIIX3_USB_UHCI)) {
+/* usblegacy is used for ppc64 temporarily */
+/* Todo: support -device xxx on ppc64 platform */
if (usblegacy) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Multiple legacy USB controllers are "
--
1.7.10.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2] qemu: fix graphics port allocation
Only release ports that have been allocated before.
This fixes these issues:
* trying to release ports when qemuProcessStart fails before port
allocation
* trying to release the SPICE TLS port if spice_tls is 0
* failing to release SPICE port with autoport=off (when only one
of them is -1)
---
v1: https://www.redhat.com/archives/libvir-list/2013-February/msg01464.html
Use a pair of booleans in domain private data instead of a new field
in the domain definition.
src/qemu/qemu_domain.h | 5 +
src/qemu/qemu_process.c | 28 +++-
2 files changed, 24 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 30e6b97..bac4d8b 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -163,6 +163,11 @@ struct _qemuDomainObjPrivate {
qemuDomainCleanupCallback *cleanupCallbacks;
size_t ncleanupCallbacks;
size_t ncleanupCallbacks_max;
+
+/* Track port allocation state for SPICE
+ * (only one device is allowed per domain) */
+bool spicePortAllocated;
+bool spiceTlsPortAllocated;
};
struct qemuDomainWatchdogEvent
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index b9fdcd2..1442cc7 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3633,6 +3633,7 @@ int qemuProcessStart(virConnectPtr conn,
}
graphics->data.spice.port = port;
+priv->spicePortAllocated = true;
}
if (cfg->spiceTLS &&
(graphics->data.spice.autoport ||
@@ -3645,10 +3646,12 @@ int qemuProcessStart(virConnectPtr conn,
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("Unable to find an unused port for
SPICE TLS"));
virPortAllocatorRelease(driver->remotePorts, port);
+priv->spicePortAllocated = false;
goto cleanup;
}
graphics->data.spice.tlsPort = tlsPort;
+priv->spiceTlsPortAllocated = true;
}
}
@@ -4311,22 +4314,29 @@ retry:
qemuProcessRemoveDomainStatus(driver, vm);
-/* Remove VNC and Spice ports from port reservation bitmap, but only if
- they were reserved by the driver (autoport=yes)
+/* Remove VNC and SPICE ports from port reservation bitmap, but only if
+ they were reserved by the driver:
+ autoport = yes and non-zero port for VNC,
+ priv->spice(Tls)PortAllocated for SPICE.
*/
for (i = 0 ; i < vm->def->ngraphics; ++i) {
virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
-graphics->data.vnc.autoport) {
+graphics->data.vnc.autoport && graphics->data.vnc.port) {
ignore_value(virPortAllocatorRelease(driver->remotePorts,
graphics->data.vnc.port));
}
-if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
-graphics->data.spice.autoport) {
-ignore_value(virPortAllocatorRelease(driver->remotePorts,
- graphics->data.spice.port));
-ignore_value(virPortAllocatorRelease(driver->remotePorts,
-
graphics->data.spice.tlsPort));
+if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
+if (priv->spicePortAllocated) {
+ignore_value(virPortAllocatorRelease(driver->remotePorts,
+
graphics->data.spice.port));
+priv->spicePortAllocated = false;
+}
+if (priv->spiceTlsPortAllocated) {
+ignore_value(virPortAllocatorRelease(driver->remotePorts,
+
graphics->data.spice.tlsPort));
+priv->spiceTlsPortAllocated = false;
+}
}
}
--
1.7.12.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Release candidate 2 of 1.0.3 is available
I have just tagged git and pushed the tarball. The rpms for F17 are at the usual place too: ftp://libvirt.org/libvirt/ I gave a try to the set of rpms and this seems to work fine for relatively simple tests. Please give it more testing and let's keep change to git to bug fixes to minimize risks for 1.0.3. If everything goes well I will push 1.0.3 on Friday, thanks ! Daniel -- Daniel Veillard | Open Source and Standards, Red Hat veill...@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: Add docs for virXMLProp string
On 21.02.2013 11:13, Peter Krempa wrote: > To avoid confusion about usage of this function explicitly document that > this function returns copy of the attribute string. > --- > src/util/virxml.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/src/util/virxml.c b/src/util/virxml.c > index 74f8beb..aa55a33 100644 > --- a/src/util/virxml.c > +++ b/src/util/virxml.c > @@ -468,6 +468,16 @@ virXPathLongLong(const char *xpath, > return ret; > } > > +/** > + * virXMLPropString: > + * @node: XML dom node pointer > + * @name: Name of the property (attribute) to get > + * > + * Convenience function to return copy of an attribute value of a XML node. > + * > + * Returns the property (attribute) value as string or NULL in case of > failure. > + * The caller is responsible for freeing the returned buffer. > + */ > char * > virXMLPropString(xmlNodePtr node, > const char *name) > ACK and safe to push now. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
