Re: [libvirt] [PATCH] network: Add bandwidth support to ethernet interface
On 11/11/2014 08:01 PM, Chen, Hanxiao wrote: > > >> -Original Message- >> From: libvir-list-boun...@redhat.com [mailto:libvir-list-boun...@redhat.com] >> On Behalf Of Eric Blake >> Sent: Tuesday, November 11, 2014 7:13 AM >> To: Anirban Chakraborty; Michal Privoznik; libvir-list@redhat.com >> Subject: Re: [libvirt] [PATCH] network: Add bandwidth support to ethernet >> interface >> >> On 11/10/2014 03:41 PM, Anirban Chakraborty wrote: >> BTW: it would be nice if you can version you patches. I mean, this is what, 4th or 5th version? Say that in subject explicitly please. You know, in the prefix: [PATCH v5] network: ... >> >> Using 'git send-email -v5' will do that for you. > > Should it be 'git format-patch -v5'? Either one. git send-email understands ALL options of git format-patch (I hate that the man page doesn't mention it, and have even reported that to upstream git developers, but so far it's fallen on deaf ears as a low-priority patch that no one has time to write). -- 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] network: Add bandwidth support to ethernet interface
> -Original Message- > From: libvir-list-boun...@redhat.com [mailto:libvir-list-boun...@redhat.com] > On Behalf Of Eric Blake > Sent: Tuesday, November 11, 2014 7:13 AM > To: Anirban Chakraborty; Michal Privoznik; libvir-list@redhat.com > Subject: Re: [libvirt] [PATCH] network: Add bandwidth support to ethernet > interface > > On 11/10/2014 03:41 PM, Anirban Chakraborty wrote: > > >> BTW: it would be nice if you can version you patches. I mean, this is > >> what, 4th or 5th version? Say that in subject explicitly please. You > >> know, in the prefix: [PATCH v5] network: ... > > Using 'git send-email -v5' will do that for you. Should it be 'git format-patch -v5'? Thanks, - Chen > > > > > I was doing it earlier and then dropped it. I¹ll resin the patch > > addressing all your comments and send it out. However, please let me know > > if I should move the above functions (virNetDevBandwidthSet etc.) in > > src/util/virnetdevbandwidth.* and add #include "conf/domain_conf.h" in > > virnetdevbandwidth.h file. > > If it needs to reference structs defined in conf/, then the logical > place for the functions is in conf/ (possibly a new file). That way, it > can still be shared between lxc and qemu. > > -- > 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] network: Add bandwidth support to ethernet interface
On 11/10/14, 3:13 PM, "Eric Blake" wrote: >On 11/10/2014 03:41 PM, Anirban Chakraborty wrote: > >>> BTW: it would be nice if you can version you patches. I mean, this is >>> what, 4th or 5th version? Say that in subject explicitly please. You >>> know, in the prefix: [PATCH v5] network: ... > >Using 'git send-email -v5' will do that for you. Thanks. > >> >> I was doing it earlier and then dropped it. I¹ll resin the patch >> addressing all your comments and send it out. However, please let me >>know >> if I should move the above functions (virNetDevBandwidthSet etc.) in >> src/util/virnetdevbandwidth.* and add #include "conf/domain_conf.h" in >> virnetdevbandwidth.h file. > >If it needs to reference structs defined in conf/, then the logical >place for the functions is in conf/ (possibly a new file). That way, it >can still be shared between lxc and qemu. I’m planning to have this function in src/conf/netdev_bandwidth_conf.*, however, an initial compilation yields following undefined reference from qemu_process.c: Making all in tests make[2]: Entering directory `/home/ubuntu/libvirt-ups/tests' CCLD domaincapstest ../src/.libs/libvirt_driver_qemu_impl.a(libvirt_driver_qemu_impl_la-qemu_pr ocess.o): In function `qemuProcessStop': /home/ubuntu/libvirt-ups/src/qemu/qemu_process.c:4847: undefined reference to `virDomainClearNetBandwidth' collect2: error: ld returned 1 exit status What am I missing here? All I did was to add virDomainClearNetBandwidth() to netdev_bandwidth_conf.c (and to .h for function prototype). I have tried moving this function to domain_conf.c as well, however, didn’t see any difference in behavior. All other functions from netdev_bandwidth_conf.c/domain_conf.c are perfectly visible during compilation. I have attached the full patch. Thanks, Anirban curr.patch Description: curr.patch -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Re-add use of locking with iptables/ip6tables/ebtables
Quoting Daniel P. Berrange (berra...@redhat.com):
> A previous commit introduced use of locking with invocation
> of iptables in the viriptables.c module
>
> commit ba95426d6f39aec1da6e069dd7222f7a8c6a5862
> Author: Serge Hallyn
> Date: Fri Nov 1 12:36:59 2013 -0500
>
> util: use -w flag when calling iptables
>
> This only ever had effect with the virtual network driver,
> as it was not wired up into the nwfilter driver. Unfortunately
> in the firewall refactoring the use of the -w flag was
> accidentally lost.
>
> This patch introduces it to the virfirewall.c module so that
> both the virtual network and nwfilter drivers will be using
> it. It also ensures that the equivalent --concurrent flag
> to ebtables is used.
> ---
Thanks, that looks very nice.
Acked-by: Serge E. Hallyn
> src/util/virfirewall.c | 67
> +++---
> src/util/viriptables.c | 2 --
> 2 files changed, 63 insertions(+), 6 deletions(-)
>
> diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> index bab1634..c83fdc6 100644
> --- a/src/util/virfirewall.c
> +++ b/src/util/virfirewall.c
> @@ -104,6 +104,44 @@ virFirewallOnceInit(void)
>
> VIR_ONCE_GLOBAL_INIT(virFirewall)
>
> +static bool iptablesUseLock;
> +static bool ip6tablesUseLock;
> +static bool ebtablesUseLock;
> +
> +static void
> +virFirewallCheckUpdateLock(bool *lockflag,
> + const char *const*args)
> +{
> +virCommandPtr cmd = virCommandNewArgs(args);
> +if (virCommandRun(cmd, NULL) < 0) {
> +VIR_INFO("locking not supported by %s", args[0]);
> +} else {
> +VIR_INFO("using locking for %s", args[0]);
> +*lockflag = true;
> +}
> +virCommandFree(cmd);
> +}
> +
> +static void
> +virFirewallCheckUpdateLocking(void)
> +{
> +const char *iptablesArgs[] = {
> +IPTABLES_PATH, "-w", "-L", "-n", NULL,
> +};
> +const char *ip6tablesArgs[] = {
> +IP6TABLES_PATH, "-w", "-L", "-n", NULL,
> +};
> +const char *ebtablesArgs[] = {
> +EBTABLES_PATH, "--concurrent", "-L", NULL,
> +};
> +virFirewallCheckUpdateLock(&iptablesUseLock,
> + iptablesArgs);
> +virFirewallCheckUpdateLock(&ip6tablesUseLock,
> + ip6tablesArgs);
> +virFirewallCheckUpdateLock(&ebtablesUseLock,
> + ebtablesArgs);
> +}
> +
> static int
> virFirewallValidateBackend(virFirewallBackend backend)
> {
> @@ -161,6 +199,9 @@ virFirewallValidateBackend(virFirewallBackend backend)
> }
>
> currentBackend = backend;
> +
> +virFirewallCheckUpdateLocking();
> +
> return 0;
> }
>
> @@ -201,6 +242,9 @@ virFirewallPtr virFirewallNew(void)
> {
> virFirewallPtr firewall;
>
> +if (virFirewallInitialize() < 0)
> +return NULL;
> +
> if (VIR_ALLOC(firewall) < 0)
> return NULL;
>
> @@ -321,6 +365,23 @@ virFirewallAddRuleFullV(virFirewallPtr firewall,
> rule->queryOpaque = opaque;
> rule->ignoreErrors = ignoreErrors;
>
> +switch (rule->layer) {
> +case VIR_FIREWALL_LAYER_ETHERNET:
> +if (ebtablesUseLock)
> +ADD_ARG(rule, "--concurrent");
> +break;
> +case VIR_FIREWALL_LAYER_IPV4:
> +if (iptablesUseLock)
> +ADD_ARG(rule, "-w");
> +break;
> +case VIR_FIREWALL_LAYER_IPV6:
> +if (ip6tablesUseLock)
> +ADD_ARG(rule, "-w");
> +break;
> +case VIR_FIREWALL_LAYER_LAST:
> +break;
> +}
> +
> while ((str = va_arg(args, char *)) != NULL) {
> ADD_ARG(rule, str);
> }
> @@ -840,8 +901,8 @@ virFirewallApplyGroup(virFirewallPtr firewall,
> bool ignoreErrors = (group->actionFlags &
> VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
> size_t i;
>
> -VIR_INFO("Starting transaction for %p flags=%x",
> - group, group->actionFlags);
> +VIR_INFO("Starting transaction for firewall=%p group=%p flags=%x",
> + firewall, group, group->actionFlags);
> firewall->currentGroup = idx;
> group->addingRollback = false;
> for (i = 0; i < group->naction; i++) {
> @@ -879,8 +940,6 @@ virFirewallApply(virFirewallPtr firewall)
> int ret = -1;
>
> virMutexLock(&ruleLock);
> -if (virFirewallInitialize() < 0)
> -goto cleanup;
>
> if (!firewall || firewall->err == ENOMEM) {
> virReportOOMError();
> diff --git a/src/util/viriptables.c b/src/util/viriptables.c
> index 4f3ac9c..46b4017 100644
> --- a/src/util/viriptables.c
> +++ b/src/util/viriptables.c
> @@ -52,8 +52,6 @@
>
> VIR_LOG_INIT("util.iptables");
>
> -bool iptables_supports_xlock = false;
> -
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> enum {
> --
> 2.1.0
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Problem with virInterfaceCreate(), IFF_UP, and NetworkManager
On 11/10/2014 11:51 AM, Daniel P. Berrange wrote: > On Mon, Nov 10, 2014 at 10:41:26AM -0500, Laine Stump wrote: >> On a seemingly unrelated note, a few months ago mprivozn pushed a patch >> that makes it an error to call virInterfaceCreate() (i.e. "ifup") for an >> interface that is already active. (the "active" state of an interface is >> determined by looking at an interface's IFF_UP flag (and also >> IFF_RUNNING, if the interface isn't a bridge device). Previously, this >> was allowed, as it is common practive to ifup an interface to make new >> config take effect. >> >> Last week, I happened to test the "virsh iface-bridge" command on a >> system with NM enabled. That command gave an error about the interface >> being already active, so I tried again, this time ifdowning the >> interface in advance - I *still* got the error. Further investigation >> and questioning of NM developers led me to the realization that when NM >> is enabled, all interfaces *always* have IFF_UP and IFF_RUNNING set, >> even if they are ifdowned. Further, if NM is active there is no way to >> determine an interface's "active" status via iotctl() or netlink; >> instead, must query to determine if NM is active, and if it is you must >> call a NM API instead (I got this much information from NM developers >> directly; haven't investigated yet exactly what the API is). >> >> NM developers say that this pinning-up of the IFF_UP flag has been done >> for a long time, and is necessary to do interface auto-config. I think >> it is violating a long-standing assumption (if not a standard) about the >> meaning of IFF_UP, and I'm not convinced that it really is a necessity >> (certainly once a config file is present for an interface, it shouldn't >> be needed), but then I haven't spent as much time in that problem space >> as they have. > Yep, I understand their motivation here - with IPv6 address auto-config, > you really want your NICs to be permanently up (or "online"), so that > if an IPv6 router advertizement arrives it "just works" without the user > needing to turn this on manually. > > IIUC, they essentially have 3 states > > - offline - IFF_UP not set - don't think this is used unless you >explicitly tell NM to disable the interface (ie airplane mode >for the wifi NIC) > > - unconfigured - IFF_UP set and no addresses present > > - configured - IFF_UP set and addresses present. > > Our API design is really looking at the transition between "offline" > and "configured". We don't have the concept of "unconfigured" really. "offline" is what I think *should* be the state for any interface that has an ifcfg file, and in that ifcfg file has the following: BOOTPROTO=none (no IPADDRx) IPV6_AUTOCONF=no (no IPV6 addresses) *and* either of ( ONBOOT=no + the device was never ifup'ed, *or* the device has been ifdowned ). In other words, in my mind running "ifdown eth0" does exactly mean that I want the interface to be "offline". That isn't what is happening though. >> In the meantime, the virInterfaceCreate() API fails 100% of the time on >> any system that has NM enabled. My dilemma now is whether to attempt to >> affect change in NM's use of IFF_UP so that it once again can be used as >> an indicator of whether or not an interface is active, or to just give >> in and 1) officially declare that virInterface*() isn't supported if NM >> is enabled until 2) we add code to netcf that detects when NM is active >> and learns how to query interface status from NM instead of the standard >> ioctl(SIOCGIFFLAGS). >> >> And if the latter is preferred, should we in the meantime perhaps revert >> the patch that made virInterfaceCreate() an error if the interface was >> active? Or just leave it completely broken? >> >> Any opinions? > It feels like when NM is present on the system, libvirt should still > honour the IFF_UP flag. ie it should always report all the NICs managed > by network manager to be "up" if IFF_UP is set. > > I think this implies we should not forbid the virInterfaceCreate API if > the state is IFF_UP. > Although I don't have a problem reverting libvirt's behavior to allow ifup'ing an interface that is already active (since I was a detractor when it was added in the fisrst place :-), I'd still get better closure if I had a good explanation of why an interface that has an associated config file and has been explicitly marked "down" (or "offline" or whatever you want to call it) by the admin needs to have IFF_UP. I've CC'ed Dan Williams from NetworkManager in hopes that he can do that. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nwfilter: fix deadlock caused updating network device and nwfilter
On 11/11/2014 11:34 AM, Pavel Hrdina wrote:
> On 11/11/2014 04:13 PM, John Ferlan wrote:
>>
>>
>> On 11/05/2014 09:02 AM, Pavel Hrdina wrote:
>>> Commit 6e5c79a1 tried to fix deadlock between nwfilter{Define,Undefine}
>>> and starting of guest, but this same deadlock is also for
>>> updating/attaching network device to domain.
>>
>> The referenced commit has a roadmap of sorts of what the lock is -
>> perhaps you could add similar details here for before and after.
>
> In that case I can do copy&paste of that roadmap as it's the same and
> this patch only extend the referenced one.
>
>>
>> While more of a pain logistically - separating out the patches into
>> single changes may make it easier to list the bad locking order followed
>> by the order the commit changes.
>>
>> The ones that are most odd are the places where you take out the lock in
>> the middle of a function. That seems to go against the original commit
>> which takes them out right after the flags check. In particular that's
>> the qemuDomainRestoreFlags and qemuMigrationPrepareAny changes.
>
> The order of the nwfilter lock and flags could be the same in all
> functions, that's a good point. My intention was to lock the
> nwfilterRead right before the vm is locked because the deadlock is
> between the vm and nwfilter objects.
>
> However I don't quiet understand why you would like to split the patch
> into a series. All the paths leads to the same nwfilter call
> "virDomainConfNWFilterInstantiate" and that's the only function callable
> outside of nwfilter that could cause the deadlock.
>
If the bad locking order is the same for each, then I suppose it's not
necessary to copy commit message and make separate patches. It was
mostly a suggestion considering you're changing 6 API's.
Also it wasn't perfectly clear about the order especially for the
Restore/Migration changes where the lock is taken out in the middle of
each function. Furthermore for those cases, we can get to the cleanup
without the lock and do an unlock, which perhaps doesn't do anything,
but it's been a while since I've thought about the guts of thread mutex
lock/unlock. May not occur in these paths, but I seem to remember if
your thread has the read lock and attempts to acquire the read lock
again, then the thread code does a refcount++ type operation. The unlock
would normally refcount-- for each and free the lock when zero. Now in
the case where you're jumping to cleanup, you could decr a refcount
which would be unexpected. My disclaimer is this was a different OS
(hp-ux) in a prior job where there was a mix of mutex locks and file
locks that caused all sorts of issues...
John
>>
>>>
>>> The deadlock was introduced by removing global QEMU driver lock because
>>> nwfilter was counting on this lock and ensure that all driver locks are
>>> locked inside of nwfilter{Define,Undefine}.
>>>
>>> This patch extends usage of virNWFilterReadLockFilterUpdates to prevent
>>> the deadlock for all possible paths in QEMU driver. LXC and UML drivers
>>> still have global lock.
>>>
>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1143780
>>>
>>> Signed-off-by: Pavel Hrdina
>>> ---
>>>
>>> This is temporary fix for the deadlock issue as I'm planning to create
>>> global
>>> libvirt jobs (similar to QEMU domain jobs) and use it for other drivers and
>>> for example for nwfilters too.
>>>
>>> src/qemu/qemu_driver.c| 12
>>> src/qemu/qemu_migration.c | 3 +++
>>> src/qemu/qemu_process.c | 4
>>> 3 files changed, 19 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 6acaea8..9e6f505 100644
>>
>> [1] Usage around virQEMUDriverGetConfig() calls isn't consistent. In
>> particular qemuDomainAttachDeviceFlags() will lock/unlock in different
>> order.
>>
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -5937,6 +5937,8 @@ qemuDomainRestoreFlags(virConnectPtr conn,
>>> def = tmp;
>>> }
>>>
>>> +virNWFilterReadLockFilterUpdates();
>>> +
>>
>> So no other locks up to this point are taken? And no need perhaps to
>> lock earlier to play follow the leader (eg, the original commit) where
>> the lock was taken after the various flags checks.
>>
>>> if (!(vm = virDomainObjListAdd(driver->domains, def,
>>> driver->xmlopt,
>>> VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
>>> @@ -5978,6 +5980,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,
>>> virFileWrapperFdFree(wrapperFd);
>>> if (vm)
>>> virObjectUnlock(vm);
>>> +virNWFilterUnlockFilterUpdates();
>>
>> So this could get called without ever having set the lock - is that
>> correct? We can get to cleanup without calling the ReadLock - probably
>> not a good thing since it's indeterminate what happens according to the
>> man page.
>>
>>> return ret;
>>> }
>>>
>>> @@ -7502,6 +7505,8 @@ static int qemuDomainAttachDeviceFlags
Re: [libvirt] [PATCH] nwfilter: fix deadlock caused updating network device and nwfilter
On 11/11/2014 04:13 PM, John Ferlan wrote:
On 11/05/2014 09:02 AM, Pavel Hrdina wrote:
Commit 6e5c79a1 tried to fix deadlock between nwfilter{Define,Undefine}
and starting of guest, but this same deadlock is also for
updating/attaching network device to domain.
The referenced commit has a roadmap of sorts of what the lock is -
perhaps you could add similar details here for before and after.
In that case I can do copy&paste of that roadmap as it's the same and
this patch only extend the referenced one.
While more of a pain logistically - separating out the patches into
single changes may make it easier to list the bad locking order followed
by the order the commit changes.
The ones that are most odd are the places where you take out the lock in
the middle of a function. That seems to go against the original commit
which takes them out right after the flags check. In particular that's
the qemuDomainRestoreFlags and qemuMigrationPrepareAny changes.
The order of the nwfilter lock and flags could be the same in all
functions, that's a good point. My intention was to lock the
nwfilterRead right before the vm is locked because the deadlock is
between the vm and nwfilter objects.
However I don't quiet understand why you would like to split the patch
into a series. All the paths leads to the same nwfilter call
"virDomainConfNWFilterInstantiate" and that's the only function callable
outside of nwfilter that could cause the deadlock.
The deadlock was introduced by removing global QEMU driver lock because
nwfilter was counting on this lock and ensure that all driver locks are
locked inside of nwfilter{Define,Undefine}.
This patch extends usage of virNWFilterReadLockFilterUpdates to prevent
the deadlock for all possible paths in QEMU driver. LXC and UML drivers
still have global lock.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1143780
Signed-off-by: Pavel Hrdina
---
This is temporary fix for the deadlock issue as I'm planning to create global
libvirt jobs (similar to QEMU domain jobs) and use it for other drivers and
for example for nwfilters too.
src/qemu/qemu_driver.c| 12
src/qemu/qemu_migration.c | 3 +++
src/qemu/qemu_process.c | 4
3 files changed, 19 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6acaea8..9e6f505 100644
[1] Usage around virQEMUDriverGetConfig() calls isn't consistent. In
particular qemuDomainAttachDeviceFlags() will lock/unlock in different
order.
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5937,6 +5937,8 @@ qemuDomainRestoreFlags(virConnectPtr conn,
def = tmp;
}
+virNWFilterReadLockFilterUpdates();
+
So no other locks up to this point are taken? And no need perhaps to
lock earlier to play follow the leader (eg, the original commit) where
the lock was taken after the various flags checks.
if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt,
VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
@@ -5978,6 +5980,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,
virFileWrapperFdFree(wrapperFd);
if (vm)
virObjectUnlock(vm);
+virNWFilterUnlockFilterUpdates();
So this could get called without ever having set the lock - is that
correct? We can get to cleanup without calling the ReadLock - probably
not a good thing since it's indeterminate what happens according to the
man page.
return ret;
}
@@ -7502,6 +7505,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom,
const char *xml,
affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
+virNWFilterReadLockFilterUpdates();
+
[1] This one is done after the GetConfig
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -7614,6 +7619,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom,
const char *xml,
virObjectUnlock(vm);
virObjectUnref(caps);
virObjectUnref(cfg);
[1] But the release is done after the cfg unref
+virNWFilterUnlockFilterUpdates();
return ret;
}
@@ -7644,6 +7650,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
VIR_DOMAIN_AFFECT_CONFIG |
VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
+virNWFilterReadLockFilterUpdates();
+
[1] This one is done before the GetConfig
cfg = virQEMUDriverGetConfig(driver);
affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
@@ -7760,6 +7768,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
virObjectUnlock(vm);
virObjectUnref(caps);
virObjectUnref(cfg);
+virNWFilterUnlockFilterUpdates();
[1] Release done after cfg unref
return ret;
}
@@ -14510,6 +14519,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
* and use of FORCE can cause multiple transitions.
*/
+virNWF
Re: [libvirt] [PATCHv8 1/7] bhyve: Support /domain/bootloader configuration for non-FreeBSD guests.
On Tue, Nov 11, 2014 at 10:40 AM, Michal Privoznik wrote:
> On 11.11.2014 16:17, Conrad Meyer wrote:
>>
>> On Tue, Nov 11, 2014 at 9:52 AM, Michal Privoznik
>> wrote:
>>> One of the things I'm worried about is, if the boot args are not
>>> specified
>>> we properly add memory configured in domain XML.
>>>
>>> However, IIUC the memory amount can be overridden with boot args. Is that
>>> expected?
>>
>>
>> Yes. If you use bootloader_args, you get exactly what you ask for and no
>> more.
>
>
> Well, if one is modifying XML to change booloader_args already he can change
> the memory amount there too. What I'm trying to say is, we should add '-m
> def->mem.max_balloon' unconditionally. Or do you intend to give users so
> much freedom? I worried it can bite us later when libvirt fails to see the
> real amount of memory that domain has.
I think also bhyve(8) itself will be unhappy. The man page specifies:
"""
-m size[K|k|M|m|G|g|T|t]
Guest physical memory size in bytes. This must be the same
size that was given to bhyveload(8).
"""
However, I think the messaging around bootloader_args for bhyve /
bhyveload is and should remain: "if you need this, you're on your own
and need to specify *everything* manually." We expect most users to
not need for bhyve at all, especially in the medium-term
future, and even fewer users to need .
IMO, as a user it is more surprising to get additional arguments
prepended to the arguments I have specified than to have them passed
through unmodified. And we would have to prepend or otherwise shove
the -m flag in the middle somewhere. I think it is probably too much
magic.
>
>
>>
+static virCommandPtr
+virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def,
+ virConnectPtr conn,
+ const char *devmap_file,
+ char **devicesmap_out)
+{
+virDomainDiskDefPtr disk, cd;
+virBuffer devicemap;
+virCommandPtr cmd;
+size_t i;
+
+if (def->os.bootloaderArgs != NULL)
+return virBhyveProcessBuildCustomLoaderCmd(def);
+
+devicemap = (virBuffer)VIR_BUFFER_INITIALIZER;
+
+/* Search disk list for CD or HDD device. */
+cd = disk = NULL;
+for (i = 0; i < def->ndisks; i++) {
+if (!virBhyveUsableDisk(conn, def->disks[i]))
+continue;
+
+if (cd == NULL &&
+def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+cd = def->disks[i];
+VIR_INFO("Picking %s as boot CD",
virDomainDiskGetSource(cd));
+}
+
+if (disk == NULL &&
+def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+disk = def->disks[i];
+VIR_INFO("Picking %s as HDD",
virDomainDiskGetSource(disk));
+}
>>>
>>>
>>>
>>> Can we utilize attribute here?
>>
>>
>> This has come up before and the answer is yes, but I'd prefer to do so
>> in a later patch series; this one is already growing unwieldy.
>
>
> Agreed. This can be addressed later, when needed.
>
> Michal
Thanks,
Conrad
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv8.1 5/7] bhyve: Probe grub-bhyve for --cons-dev capability
---
src/bhyve/bhyve_capabilities.c | 37 +
src/bhyve/bhyve_capabilities.h | 6 ++
2 files changed, 43 insertions(+)
diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
index 132ce91..6e9a943 100644
--- a/src/bhyve/bhyve_capabilities.c
+++ b/src/bhyve/bhyve_capabilities.c
@@ -23,6 +23,7 @@
#include
#include "viralloc.h"
+#include "virfile.h"
#include "virlog.h"
#include "virstring.h"
#include "cpu/cpu.h"
@@ -104,3 +105,39 @@ virBhyveCapsBuild(void)
virObjectUnref(caps);
return NULL;
}
+
+int
+virBhyveProbeGrubCaps(unsigned *caps)
+{
+char *binary, *help;
+virCommandPtr cmd;
+int ret, exit;
+
+ret = 0;
+*caps = 0;
+cmd = NULL;
+help = NULL;
+
+binary = virFindFileInPath("grub-bhyve");
+if (binary == NULL)
+goto out;
+if (!virFileIsExecutable(binary))
+goto out;
+
+cmd = virCommandNew(binary);
+virCommandAddArg(cmd, "--help");
+virCommandSetOutputBuffer(cmd, &help);
+if (virCommandRun(cmd, &exit) < 0) {
+ret = -1;
+goto out;
+}
+
+if (strstr(help, "--cons-dev") != NULL)
+*caps |= BHYVE_GRUB_CAP_CONSDEV;
+
+ out:
+VIR_FREE(help);
+virCommandFree(cmd);
+VIR_FREE(binary);
+return ret;
+}
diff --git a/src/bhyve/bhyve_capabilities.h b/src/bhyve/bhyve_capabilities.h
index c52e0d0..a559d2a 100644
--- a/src/bhyve/bhyve_capabilities.h
+++ b/src/bhyve/bhyve_capabilities.h
@@ -26,4 +26,10 @@
virCapsPtr virBhyveCapsBuild(void);
+/* These are bit flags: */
+enum {
+BHYVE_GRUB_CAP_CONSDEV = 0x0001,
+};
+int virBhyveProbeGrubCaps(unsigned *caps);
+
#endif
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv8 1/7] bhyve: Support /domain/bootloader configuration for non-FreeBSD guests.
On 11.11.2014 16:17, Conrad Meyer wrote:
On Tue, Nov 11, 2014 at 9:52 AM, Michal Privoznik wrote:
On 08.11.2014 17:48, Conrad Meyer wrote:
+static virCommandPtr
+virBhyveProcessBuildBhyveloadCmd(virDomainDefPtr def, virDomainDiskDefPtr
disk)
{
virCommandPtr cmd;
-virDomainDiskDefPtr disk;
-if (def->ndisks < 1) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("domain should have at least one disk
defined"));
+cmd = virCommandNew(BHYVELOAD);
+
+if (def->os.bootloaderArgs == NULL) {
+VIR_DEBUG("bhyveload with default arguments");
+
+/* Memory (MB) */
+virCommandAddArg(cmd, "-m");
+virCommandAddArgFormat(cmd, "%llu",
+ VIR_DIV_UP(def->mem.max_balloon, 1024));
One of the things I'm worried about is, if the boot args are not specified
we properly add memory configured in domain XML.
+
+/* Image path */
+virCommandAddArg(cmd, "-d");
+virCommandAddArg(cmd, virDomainDiskGetSource(disk));
+
+/* VM name */
+virCommandAddArg(cmd, def->name);
+} else {
+VIR_DEBUG("bhyveload with arguments");
+virAppendBootloaderArgs(cmd, def);
+}
+
However, IIUC the memory amount can be overridden with boot args. Is that
expected?
Yes. If you use bootloader_args, you get exactly what you ask for and no more.
Well, if one is modifying XML to change booloader_args already he can
change the memory amount there too. What I'm trying to say is, we should
add '-m def->mem.max_balloon' unconditionally. Or do you intend to give
users so much freedom? I worried it can bite us later when libvirt fails
to see the real amount of memory that domain has.
+static virCommandPtr
+virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def,
+ virConnectPtr conn,
+ const char *devmap_file,
+ char **devicesmap_out)
+{
+virDomainDiskDefPtr disk, cd;
+virBuffer devicemap;
+virCommandPtr cmd;
+size_t i;
+
+if (def->os.bootloaderArgs != NULL)
+return virBhyveProcessBuildCustomLoaderCmd(def);
+
+devicemap = (virBuffer)VIR_BUFFER_INITIALIZER;
+
+/* Search disk list for CD or HDD device. */
+cd = disk = NULL;
+for (i = 0; i < def->ndisks; i++) {
+if (!virBhyveUsableDisk(conn, def->disks[i]))
+continue;
+
+if (cd == NULL &&
+def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+cd = def->disks[i];
+VIR_INFO("Picking %s as boot CD",
virDomainDiskGetSource(cd));
+}
+
+if (disk == NULL &&
+def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+disk = def->disks[i];
+VIR_INFO("Picking %s as HDD", virDomainDiskGetSource(disk));
+}
Can we utilize attribute here?
This has come up before and the answer is yes, but I'd prefer to do so
in a later patch series; this one is already growing unwieldy.
Agreed. This can be addressed later, when needed.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv8 0/7] Add non-FreeBSD guest support to Bhyve driver.
On Tue, Nov 11, 2014 at 9:52 AM, Michal Privoznik wrote: > Looking good. Basically ACK to the patches. BUT, please post a follow-up > patches to 1/7 and 5/7 (as reply to individual patches or cover letter > ideally). I don't want to make you to send another version. I'll merge the > patches then. > > Michal Great. I've posted a minor follow-up to 5/7; if you think the 1/7 is completely trivial, I can do the follow up there, but I'd rather fix it up separately. Thanks, Conrad -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/5] storage: Check for valid fc_host parent at startup
On 11/11/2014 10:05 AM, Michal Privoznik wrote:
> On 11.11.2014 13:38, John Ferlan wrote:
>>
>>
>> On 11/11/2014 07:21 AM, Michal Privoznik wrote:
>>> On 10.11.2014 23:45, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1160565
If a 'parent' attribute is provided for the fchost, then at startup
time check to ensure it is a vport capable scsi_host. If the parent
is not vport capable, then disallow the startup. The following is the
expected results:
error: Failed to start pool fc_pool
error: XML error: parent 'scsi_host2' specified for vHBA is not vport
capable
where the XML for the fc_pool is:
fc_pool
>>> wwnn='5001a4aaf3ca174b' wwpn='5001a4a77192b864'/>
...
and 'scsi_host2' is not vport capable.
Providing an incorrect parent and a correct wwnn/wwpn could lead to
failures at shutdown (deleteVport) where the assumption is the parent
is for the fchost.
NOTE: If the provided wwnn/wwpn doesn't resolve to an existing scsi_host,
then we will be creating one with code (virManageVport) which
assumes the parent is vport capable.
Signed-off-by: John Ferlan
---
src/storage/storage_backend_scsi.c | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index 02160bc..549d8db 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -556,6 +556,20 @@ createVport(virStoragePoolSourceAdapter adapter)
if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
return 0;
+/* If a parent was provided, then let's make sure it's vhost capable
*/
+if (adapter.data.fchost.parent) {
+if (virGetSCSIHostNumber(adapter.data.fchost.parent,
&parent_host) < 0)
+return -1;
>>
>> ^^^ [1] ^^^
+
+if (!virIsCapableFCHost(NULL, parent_host)) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("parent '%s' specified for vHBA "
+ "is not vport capable"),
+ adapter.data.fchost.parent);
+return -1;
+}
+}
+
/* This filters either HBA or already created vHBA */
if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
adapter.data.fchost.wwpn))) {
@@ -565,14 +579,14 @@ createVport(virStoragePoolSourceAdapter adapter)
if (!adapter.data.fchost.parent &&
!(adapter.data.fchost.parent =
virFindFCHostCapableVport(NULL))) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
+virReportError(VIR_ERR_XML_ERROR, "%s",
_("'parent' for vHBA not specified, and "
"cannot find one on this host"));
return -1;
-}
-if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) <
0)
-return -1;
+if (virGetSCSIHostNumber(adapter.data.fchost.parent,
&parent_host) < 0)
+return -1;
+}
>>>
>>> This chunk seems odd. After the error is reported, the control jumps out
>>> from the function, so virGetSCSIHostNumer is not called at all. Did you
>>> forget to commit something?
>>>
>>
>> The earlier chunk of code where the parent exists, we figure the
>> parent_host value. [1]
>>
>> This chunk is - if a parent wasn't provided, find a capable vport, then
>> get the parent_host value.
>>
>> I moved it inside the if because it makes no sense to call the function
>> twice in the event we had a parent value..
>
> My point is, when the conditions are met, and the error is reported the
> control jumps out of the function right after virReportError(). That's
> because there's 'return -1' after that. However, the next line in the
> same block is yet another function call. This, however, will never be
> called so it's a dead code. Hence my question.
>
> Michal
>
Doh! Of course as you'll find in later patches the logic is adjusted
and thus where my brain was already at. I'll fix this one though to be:
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_sc
index 549d8db..88928c9 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -577,12 +577,13 @@ createVport(virStoragePoolSourceAdapter adapter)
return 0;
}
-if (!adapter.data.fchost.parent &&
-!(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("'parent' for vHBA no
Re: [libvirt] [PATCH] nwfilter: fix deadlock caused updating network device and nwfilter
On Wed, Nov 05, 2014 at 03:02:03PM +0100, Pavel Hrdina wrote:
> Commit 6e5c79a1 tried to fix deadlock between nwfilter{Define,Undefine}
> and starting of guest, but this same deadlock is also for
> updating/attaching network device to domain.
>
> The deadlock was introduced by removing global QEMU driver lock because
> nwfilter was counting on this lock and ensure that all driver locks are
> locked inside of nwfilter{Define,Undefine}.
>
> This patch extends usage of virNWFilterReadLockFilterUpdates to prevent
> the deadlock for all possible paths in QEMU driver. LXC and UML drivers
> still have global lock.
ACK, conceptually it makes sense that we need to hold the read lock
in these methods. Concurrency should still be good because it is
only a read lock, not a write lock.
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] qemu: Always set migration capabilities
On 11/10/2014 04:41 PM, Jiri Denemark wrote: > We used to set migration capabilities only when a user asked for them in > flags. This is fine when migration succeeds since the QEMU process is > killed in the end but in case migration fails or if it's cancelled, some > capabilities may remain turned on with no way to turn them off. To fix > that, migration capabilities have to be turned on if requested but > explicitly turned off in case they were not requested but QEMU supports > them. > > https://bugzilla.redhat.com/show_bug.cgi?id=1160997 > Signed-off-by: Jiri Denemark > --- > src/qemu/qemu_migration.c| 42 +- > src/qemu/qemu_monitor.c | 5 +++-- > src/qemu/qemu_monitor.h | 3 ++- > src/qemu/qemu_monitor_json.c | 5 +++-- > src/qemu/qemu_monitor_json.h | 3 ++- > tests/qemumonitorjsontest.c | 3 ++- > 6 files changed, 41 insertions(+), 20 deletions(-) ACK Don't forget to update the bug link before pushing. Jan signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv8 1/7] bhyve: Support /domain/bootloader configuration for non-FreeBSD guests.
On Tue, Nov 11, 2014 at 9:52 AM, Michal Privoznik wrote:
> On 08.11.2014 17:48, Conrad Meyer wrote:
>> +static virCommandPtr
>> +virBhyveProcessBuildBhyveloadCmd(virDomainDefPtr def, virDomainDiskDefPtr
>> disk)
>> {
>> virCommandPtr cmd;
>> -virDomainDiskDefPtr disk;
>>
>> -if (def->ndisks < 1) {
>> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> - _("domain should have at least one disk
>> defined"));
>> +cmd = virCommandNew(BHYVELOAD);
>> +
>> +if (def->os.bootloaderArgs == NULL) {
>> +VIR_DEBUG("bhyveload with default arguments");
>> +
>> +/* Memory (MB) */
>> +virCommandAddArg(cmd, "-m");
>> +virCommandAddArgFormat(cmd, "%llu",
>> + VIR_DIV_UP(def->mem.max_balloon, 1024));
>
> One of the things I'm worried about is, if the boot args are not specified
> we properly add memory configured in domain XML.
>
>> +
>> +/* Image path */
>> +virCommandAddArg(cmd, "-d");
>> +virCommandAddArg(cmd, virDomainDiskGetSource(disk));
>> +
>> +/* VM name */
>> +virCommandAddArg(cmd, def->name);
>> +} else {
>> +VIR_DEBUG("bhyveload with arguments");
>> +virAppendBootloaderArgs(cmd, def);
>> +}
>> +
>
>
> However, IIUC the memory amount can be overridden with boot args. Is that
> expected?
Yes. If you use bootloader_args, you get exactly what you ask for and no more.
>> +static virCommandPtr
>> +virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def,
>> + virConnectPtr conn,
>> + const char *devmap_file,
>> + char **devicesmap_out)
>> +{
>> +virDomainDiskDefPtr disk, cd;
>> +virBuffer devicemap;
>> +virCommandPtr cmd;
>> +size_t i;
>> +
>> +if (def->os.bootloaderArgs != NULL)
>> +return virBhyveProcessBuildCustomLoaderCmd(def);
>> +
>> +devicemap = (virBuffer)VIR_BUFFER_INITIALIZER;
>> +
>> +/* Search disk list for CD or HDD device. */
>> +cd = disk = NULL;
>> +for (i = 0; i < def->ndisks; i++) {
>> +if (!virBhyveUsableDisk(conn, def->disks[i]))
>> +continue;
>> +
>> +if (cd == NULL &&
>> +def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
>> +cd = def->disks[i];
>> +VIR_INFO("Picking %s as boot CD",
>> virDomainDiskGetSource(cd));
>> +}
>> +
>> +if (disk == NULL &&
>> +def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
>> +disk = def->disks[i];
>> +VIR_INFO("Picking %s as HDD", virDomainDiskGetSource(disk));
>> +}
>
>
> Can we utilize attribute here?
This has come up before and the answer is yes, but I'd prefer to do so
in a later patch series; this one is already growing unwieldy.
>
>
>> +}
>> +
>> +cmd = virCommandNew(def->os.bootloader);
>> +
>> +VIR_DEBUG("grub-bhyve with default arguments");
>> +
>> +if (devicesmap_out != NULL) {
>> +/* Grub device.map (just for boot) */
>> +if (disk != NULL)
>> +virBufferAsprintf(&devicemap, "(hd0) %s\n",
>> + virDomainDiskGetSource(disk));
>> +
>> +if (cd != NULL)
>> +virBufferAsprintf(&devicemap, "(cd) %s\n",
>> + virDomainDiskGetSource(cd));
>> +
>> +*devicesmap_out = virBufferContentAndReset(&devicemap);
>> +}
>> +
>> +if (cd != NULL) {
>> +virCommandAddArg(cmd, "--root");
>> +virCommandAddArg(cmd, "cd");
>> +} else {
>> +virCommandAddArg(cmd, "--root");
>> +virCommandAddArg(cmd, "hd0,msdos1");
>> +}
>> +
>> +virCommandAddArg(cmd, "--device-map");
>> +virCommandAddArg(cmd, devmap_file);
>> +
>> +/* Memory in MB */
>> +virCommandAddArg(cmd, "--memory");
>> virCommandAddArgFormat(cmd, "%llu",
>> VIR_DIV_UP(def->mem.max_balloon, 1024));
>>
>> -/* Image path */
>> -virCommandAddArg(cmd, "-d");
>> -virCommandAddArg(cmd, virDomainDiskGetSource(disk));
>> -
>> /* VM name */
>> virCommandAddArg(cmd, def->name);
>>
>> return cmd;
>> }
>
>
> Otherwise looking good.
>
> Michal
Thanks!
Conrad
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nwfilter: fix deadlock caused updating network device and nwfilter
On 11/05/2014 09:02 AM, Pavel Hrdina wrote:
> Commit 6e5c79a1 tried to fix deadlock between nwfilter{Define,Undefine}
> and starting of guest, but this same deadlock is also for
> updating/attaching network device to domain.
The referenced commit has a roadmap of sorts of what the lock is -
perhaps you could add similar details here for before and after.
While more of a pain logistically - separating out the patches into
single changes may make it easier to list the bad locking order followed
by the order the commit changes.
The ones that are most odd are the places where you take out the lock in
the middle of a function. That seems to go against the original commit
which takes them out right after the flags check. In particular that's
the qemuDomainRestoreFlags and qemuMigrationPrepareAny changes.
>
> The deadlock was introduced by removing global QEMU driver lock because
> nwfilter was counting on this lock and ensure that all driver locks are
> locked inside of nwfilter{Define,Undefine}.
>
> This patch extends usage of virNWFilterReadLockFilterUpdates to prevent
> the deadlock for all possible paths in QEMU driver. LXC and UML drivers
> still have global lock.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1143780
>
> Signed-off-by: Pavel Hrdina
> ---
>
> This is temporary fix for the deadlock issue as I'm planning to create global
> libvirt jobs (similar to QEMU domain jobs) and use it for other drivers and
> for example for nwfilters too.
>
> src/qemu/qemu_driver.c| 12
> src/qemu/qemu_migration.c | 3 +++
> src/qemu/qemu_process.c | 4
> 3 files changed, 19 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 6acaea8..9e6f505 100644
[1] Usage around virQEMUDriverGetConfig() calls isn't consistent. In
particular qemuDomainAttachDeviceFlags() will lock/unlock in different
order.
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5937,6 +5937,8 @@ qemuDomainRestoreFlags(virConnectPtr conn,
> def = tmp;
> }
>
> +virNWFilterReadLockFilterUpdates();
> +
So no other locks up to this point are taken? And no need perhaps to
lock earlier to play follow the leader (eg, the original commit) where
the lock was taken after the various flags checks.
> if (!(vm = virDomainObjListAdd(driver->domains, def,
> driver->xmlopt,
> VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
> @@ -5978,6 +5980,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,
> virFileWrapperFdFree(wrapperFd);
> if (vm)
> virObjectUnlock(vm);
> +virNWFilterUnlockFilterUpdates();
So this could get called without ever having set the lock - is that
correct? We can get to cleanup without calling the ReadLock - probably
not a good thing since it's indeterminate what happens according to the
man page.
> return ret;
> }
>
> @@ -7502,6 +7505,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr
> dom, const char *xml,
>
> affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
>
> +virNWFilterReadLockFilterUpdates();
> +
[1] This one is done after the GetConfig
> if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> goto cleanup;
>
> @@ -7614,6 +7619,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr
> dom, const char *xml,
> virObjectUnlock(vm);
> virObjectUnref(caps);
> virObjectUnref(cfg);
[1] But the release is done after the cfg unref
> +virNWFilterUnlockFilterUpdates();
> return ret;
> }
>
> @@ -7644,6 +7650,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
>VIR_DOMAIN_AFFECT_CONFIG |
>VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
>
> +virNWFilterReadLockFilterUpdates();
> +
[1] This one is done before the GetConfig
> cfg = virQEMUDriverGetConfig(driver);
>
> affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
> @@ -7760,6 +7768,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
> virObjectUnlock(vm);
> virObjectUnref(caps);
> virObjectUnref(cfg);
> +virNWFilterUnlockFilterUpdates();
[1] Release done after cfg unref
> return ret;
> }
>
> @@ -14510,6 +14519,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
> * and use of FORCE can cause multiple transitions.
> */
>
> +virNWFilterReadLockFilterUpdates();
> +
[1] This one is done before GetConfig
> if (!(vm = qemuDomObjFromSnapshot(snapshot)))
> return -1;
>
> @@ -14831,6 +14842,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
> virObjectUnlock(vm);
> virObjectUnref(caps);
> virObjectUnref(cfg);
> +virNWFilterUnlockFilterUpdates();
[1] Release done after cfg unref
>
> return ret;
> }
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 94a4cf6..18242a
Re: [libvirt] [PATCH v2 1/5] storage: Check for valid fc_host parent at startup
On 11.11.2014 13:38, John Ferlan wrote:
On 11/11/2014 07:21 AM, Michal Privoznik wrote:
On 10.11.2014 23:45, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1160565
If a 'parent' attribute is provided for the fchost, then at startup
time check to ensure it is a vport capable scsi_host. If the parent
is not vport capable, then disallow the startup. The following is the
expected results:
error: Failed to start pool fc_pool
error: XML error: parent 'scsi_host2' specified for vHBA is not vport capable
where the XML for the fc_pool is:
fc_pool
...
and 'scsi_host2' is not vport capable.
Providing an incorrect parent and a correct wwnn/wwpn could lead to
failures at shutdown (deleteVport) where the assumption is the parent
is for the fchost.
NOTE: If the provided wwnn/wwpn doesn't resolve to an existing scsi_host,
then we will be creating one with code (virManageVport) which
assumes the parent is vport capable.
Signed-off-by: John Ferlan
---
src/storage/storage_backend_scsi.c | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index 02160bc..549d8db 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -556,6 +556,20 @@ createVport(virStoragePoolSourceAdapter adapter)
if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
return 0;
+/* If a parent was provided, then let's make sure it's vhost capable */
+if (adapter.data.fchost.parent) {
+if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
+return -1;
^^^ [1] ^^^
+
+if (!virIsCapableFCHost(NULL, parent_host)) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("parent '%s' specified for vHBA "
+ "is not vport capable"),
+ adapter.data.fchost.parent);
+return -1;
+}
+}
+
/* This filters either HBA or already created vHBA */
if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
adapter.data.fchost.wwpn))) {
@@ -565,14 +579,14 @@ createVport(virStoragePoolSourceAdapter adapter)
if (!adapter.data.fchost.parent &&
!(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
+virReportError(VIR_ERR_XML_ERROR, "%s",
_("'parent' for vHBA not specified, and "
"cannot find one on this host"));
return -1;
-}
-if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
-return -1;
+if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
+return -1;
+}
This chunk seems odd. After the error is reported, the control jumps out
from the function, so virGetSCSIHostNumer is not called at all. Did you
forget to commit something?
The earlier chunk of code where the parent exists, we figure the
parent_host value. [1]
This chunk is - if a parent wasn't provided, find a capable vport, then
get the parent_host value.
I moved it inside the if because it makes no sense to call the function
twice in the event we had a parent value..
My point is, when the conditions are met, and the error is reported the
control jumps out of the function right after virReportError(). That's
because there's 'return -1' after that. However, the next line in the
same block is yet another function call. This, however, will never be
called so it's a dead code. Hence my question.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] qemu: Improve qemuMonitorJSONSendKey function
When using modifiers with send-key, then we cannot know with what keys
those modifiers should be pressed down. QEMU changed the order of the
release events few times and that caused few send-key command to work
differently than expected.
We already state in virsh man page that the keys sent will be sent to
the guest simultaneously and can be received in random order. This
patch just tries improving user experience and keeping old behaviour.
Signed-off-by: Martin Kletzander
---
src/qemu/qemu_driver.c | 4 +-
src/qemu/qemu_monitor.c | 6 +-
src/qemu/qemu_monitor.h | 3 +-
src/qemu/qemu_monitor_json.c | 142 ---
src/qemu/qemu_monitor_json.h | 3 +-
tests/qemumonitorjsontest.c | 72 --
6 files changed, 198 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 56e8430..bdf99eb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2589,7 +2589,9 @@ static int qemuDomainSendKey(virDomainPtr domain,
}
qemuDomainObjEnterMonitor(driver, vm);
-ret = qemuMonitorSendKey(priv->mon, holdtime, keycodes, nkeycodes);
+ret = qemuMonitorSendKey(priv->mon, holdtime, keycodes, nkeycodes,
+ virQEMUCapsGet(priv->qemuCaps,
+QEMU_CAPS_INPUT_EVENT));
qemuDomainObjExitMonitor(driver, vm);
endjob:
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 276649d..4f128a1 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3431,7 +3431,8 @@ int qemuMonitorInjectNMI(qemuMonitorPtr mon)
int qemuMonitorSendKey(qemuMonitorPtr mon,
unsigned int holdtime,
unsigned int *keycodes,
- unsigned int nkeycodes)
+ unsigned int nkeycodes,
+ bool events)
{
int ret;
@@ -3439,7 +3440,8 @@ int qemuMonitorSendKey(qemuMonitorPtr mon,
mon, holdtime, nkeycodes);
if (mon->json)
-ret = qemuMonitorJSONSendKey(mon, holdtime, keycodes, nkeycodes);
+ret = qemuMonitorJSONSendKey(mon, holdtime, keycodes, nkeycodes,
+ events);
else
ret = qemuMonitorTextSendKey(mon, holdtime, keycodes, nkeycodes);
return ret;
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 76c91a3..14c6347 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -740,7 +740,8 @@ int qemuMonitorScreendump(qemuMonitorPtr mon,
int qemuMonitorSendKey(qemuMonitorPtr mon,
unsigned int holdtime,
unsigned int *keycodes,
- unsigned int nkeycodes);
+ unsigned int nkeycodes,
+ bool events);
typedef enum {
BLOCK_JOB_ABORT,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 91a7aba..cd8a38b 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -44,6 +44,7 @@
#include "virprobe.h"
#include "virstring.h"
#include "cpu/cpu_x86.h"
+#include "virkeycode.h"
#ifdef WITH_DTRACE_PROBES
# include "libvirt_qemu_probes.h"
@@ -3938,52 +3939,151 @@ int qemuMonitorJSONInjectNMI(qemuMonitorPtr mon)
return ret;
}
+static int
+qemuMonitorJSONAppendKey(virJSONValuePtr list,
+ unsigned int keycode,
+ bool use_events,
+ bool down)
+{
+virJSONValuePtr data = NULL;
+virJSONValuePtr event = NULL;
+virJSONValuePtr key = NULL;
+
+if (!(key = virJSONValueNewObject()))
+goto cleanup;
+
+/* Union KeyValue has two types, use the generic one */
+if (virJSONValueObjectAppendString(key, "type", "number") < 0)
+goto cleanup;
+
+/* with the keycode */
+if (virJSONValueObjectAppendNumberInt(key, "data", keycode) < 0)
+goto cleanup;
+
+if (!use_events) {
+if (virJSONValueArrayAppend(list, key) < 0)
+goto cleanup;
+
+/* That's all if we're not using 'input-send-event'. */
+return 0;
+}
+
+if (!(event = virJSONValueNewObject()) ||
+!(data = virJSONValueNewObject()))
+goto cleanup;
+
+if (virJSONValueObjectAppendBoolean(data, "down", down) < 0)
+goto cleanup;
+
+if (virJSONValueObjectAppend(data, "key", key) < 0)
+goto cleanup;
+
+key = NULL;
+
+if (virJSONValueObjectAppendString(event, "type", "key") < 0)
+goto cleanup;
+
+if (virJSONValueObjectAppend(event, "data", data) < 0)
+goto cleanup;
+
+data = NULL;
+
+if (virJSONValueArrayAppend(list, event) < 0)
+goto cleanup;
+
+event = NULL;
+
+return 0;
+
+ cleanup:
+virJSONValueFree(data);
+virJSONValueFree(event);
+virJSONValueFree(key);
+return -1;
+}
+
int qemuMonitorJSONSendKey(qemuMonitorPtr mon,
[libvirt] [PATCH 2/3] util: Add virKeycodeValueIsModifier function
This function returns true if the value supplied is a modifier (Ctrl,
Shift, Alt or Meta).
Signed-off-by: Martin Kletzander
---
src/libvirt_private.syms | 1 +
src/util/virkeycode.c| 21 +
src/util/virkeycode.h| 2 ++
3 files changed, 24 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b8f35e8..5c3de01 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1530,6 +1530,7 @@ virJSONValueToString;
virKeycodeSetTypeFromString;
virKeycodeSetTypeToString;
virKeycodeValueFromString;
+virKeycodeValueIsModifier;
virKeycodeValueTranslate;
diff --git a/src/util/virkeycode.c b/src/util/virkeycode.c
index 7880a0a..7705ffd 100644
--- a/src/util/virkeycode.c
+++ b/src/util/virkeycode.c
@@ -1,4 +1,5 @@
/*
+ * Copyright (C) 2014 Red Hat, Inc.
* Copyright (c) 2011 Lai Jiangshan
*
* This library is free software; you can redistribute it and/or
@@ -124,3 +125,23 @@ int virKeycodeValueTranslate(virKeycodeSet from_codeset,
return -1;
}
+
+
+bool
+virKeycodeValueIsModifier(unsigned int key_value)
+{
+switch (key_value) {
+case 29: /* Left Control */
+case 157: /* Right Control */
+case 42: /* Left Shift */
+case 54: /* Right Shift */
+case 56: /* Left Alt */
+case 184: /* Right Alt */
+case 219: /* Left Meta */
+case 220: /* Right Meta */
+return true;
+
+default:
+return false;
+}
+}
diff --git a/src/util/virkeycode.h b/src/util/virkeycode.h
index 6947cfe..d04a2a4 100644
--- a/src/util/virkeycode.h
+++ b/src/util/virkeycode.h
@@ -1,6 +1,7 @@
/*
* virkeycode.h: keycodes definitions and declarations
*
+ * Copyright (C) 2014 Red Hat, Inc.
* Copyright (c) 2011 Lai Jiangshan
*
* This library is free software; you can redistribute it and/or
@@ -29,5 +30,6 @@ int virKeycodeValueFromString(virKeycodeSet codeset, const
char *keyname);
int virKeycodeValueTranslate(virKeycodeSet from_codeset,
virKeycodeSet to_offset,
int key_value);
+bool virKeycodeValueIsModifier(unsigned int key_value);
#endif
--
2.1.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] qemu: Improve send-key command using input-send-event
QEMU added a qmp command input-send-event that is more precise about what the outcome for the guest is; it can control particular events as press/release of a key, button, move of a pointer, etc. This series is still waiting for two patches [1] from Amos Kong to be applied before it can be used and will not be pushed until that change is in upstream QEMU. Martin [1] https://lists.gnu.org/archive/html/qemu-devel/2014-11/msg01187.html Martin Kletzander (3): qemu: Add capability probing for 'imput-send-event' qmp command util: Add virKeycodeValueIsModifier function qemu: Improve qemuMonitorJSONSendKey function src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 3 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_monitor.c | 6 +- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 142 --- src/qemu/qemu_monitor_json.h | 3 +- src/util/virkeycode.c| 21 +++ src/util/virkeycode.h| 2 + tests/qemumonitorjsontest.c | 72 -- 11 files changed, 226 insertions(+), 32 deletions(-) -- 2.1.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] qemu: Add capability probing for 'imput-send-event' qmp command
Signed-off-by: Martin Kletzander
---
src/qemu/qemu_capabilities.c | 3 +++
src/qemu/qemu_capabilities.h | 1 +
2 files changed, 4 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 74a3b24..9ada568 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -272,6 +272,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"migrate-rdma",
"ivshmem",
"drive-iotune-max",
+
+ "input-send-event", /* 180 */
);
@@ -1436,6 +1438,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = {
{ "nbd-server-start", QEMU_CAPS_NBD_SERVER },
{ "change-backing-file", QEMU_CAPS_CHANGE_BACKING_FILE },
{ "rtc-reset-reinjection", QEMU_CAPS_RTC_RESET_REINJECTION },
+{ "input-send-event", QEMU_CAPS_INPUT_EVENT },
};
struct virQEMUCapsStringFlags virQEMUCapsMigration[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index ffe3494..b4c5390 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -219,6 +219,7 @@ typedef enum {
QEMU_CAPS_MIGRATE_RDMA = 177, /* have rdma migration */
QEMU_CAPS_DEVICE_IVSHMEM = 178, /* -device ivshmem */
QEMU_CAPS_DRIVE_IOTUNE_MAX = 179, /* -drive bps_max= and friends */
+QEMU_CAPS_INPUT_EVENT= 180, /* input-send-event qmp command */
QEMU_CAPS_LAST, /* this must always be the last item */
} virQEMUCapsFlags;
--
2.1.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv8 0/7] Add non-FreeBSD guest support to Bhyve driver.
On 08.11.2014 17:48, Conrad Meyer wrote: Drvbhyve hardcodes bhyveload(8) as the host bootloader for guests. bhyveload(8) loader only supports FreeBSD guests. This patch series adds and handling to bhyve_command, so libvirt can boot non-FreeBSD guests in Bhyve. Additionally, support for grub-bhyve(1)'s --cons-dev argument is added so that interactive GRUB menus can be manipulated with the domain-configured serial device. See patch logs for further details. Thanks, Conrad Changes in v8: - Fix typo in virBhyveProcessStart that prevented booting bhyve VMs. Conrad Meyer (7): bhyve: Support /domain/bootloader configuration for non-FreeBSD guests. bhyvexml2argv: Add loader argv tests. domaincommon.rng: Add 'bootloader' to os=hvm schema for Bhyve bhyvexml2argv: Add tests for domain-configured bootloader, args bhyve: Probe grub-bhyve for --cons-dev capability bhyve: Add console support for grub-bhyve bootloader bhyvexml2argv: Add test for grub console support docs/drvbhyve.html.in | 100 ++- docs/formatdomain.html.in | 4 +- docs/schemas/domaincommon.rng | 17 +- src/bhyve/bhyve_capabilities.c | 37 src/bhyve/bhyve_capabilities.h | 3 + src/bhyve/bhyve_command.c | 189 +++-- src/bhyve/bhyve_command.h | 5 +- src/bhyve/bhyve_driver.c | 16 +- src/bhyve/bhyve_driver.h | 2 + src/bhyve/bhyve_process.c | 38 - src/bhyve/bhyve_utils.h| 2 + .../bhyvexml2argv-acpiapic.ldargs | 1 + tests/bhyvexml2argvdata/bhyvexml2argv-base.ldargs | 1 + .../bhyvexml2argv-bhyveload-explicitargs.args | 3 + .../bhyvexml2argv-bhyveload-explicitargs.ldargs| 1 + .../bhyvexml2argv-bhyveload-explicitargs.xml | 23 +++ .../bhyvexml2argvdata/bhyvexml2argv-console.ldargs | 1 + .../bhyvexml2argv-custom-loader.args | 3 + .../bhyvexml2argv-custom-loader.ldargs | 1 + .../bhyvexml2argv-custom-loader.xml| 24 +++ .../bhyvexml2argv-disk-cdrom-grub.args | 3 + .../bhyvexml2argv-disk-cdrom-grub.devmap | 1 + .../bhyvexml2argv-disk-cdrom-grub.ldargs | 2 + .../bhyvexml2argv-disk-cdrom-grub.xml | 23 +++ .../bhyvexml2argv-disk-cdrom.ldargs| 1 + .../bhyvexml2argv-disk-virtio.ldargs | 1 + .../bhyvexml2argv-grub-defaults.args | 3 + .../bhyvexml2argv-grub-defaults.devmap | 1 + .../bhyvexml2argv-grub-defaults.ldargs | 2 + .../bhyvexml2argv-grub-defaults.xml| 23 +++ .../bhyvexml2argvdata/bhyvexml2argv-macaddr.ldargs | 1 + .../bhyvexml2argv-serial-grub-nocons.args | 4 + .../bhyvexml2argv-serial-grub-nocons.devmap| 1 + .../bhyvexml2argv-serial-grub-nocons.ldargs| 2 + .../bhyvexml2argv-serial-grub-nocons.xml | 26 +++ .../bhyvexml2argv-serial-grub.args | 4 + .../bhyvexml2argv-serial-grub.devmap | 1 + .../bhyvexml2argv-serial-grub.ldargs | 2 + .../bhyvexml2argv-serial-grub.xml | 26 +++ .../bhyvexml2argvdata/bhyvexml2argv-serial.ldargs | 1 + tests/bhyvexml2argvtest.c | 71 +++- 41 files changed, 631 insertions(+), 39 deletions(-) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-acpiapic.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-base.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-console.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.devmap create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-virtio.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.devmap create mode 100644
Re: [libvirt] [PATCHv8 1/7] bhyve: Support /domain/bootloader configuration for non-FreeBSD guests.
On 08.11.2014 17:48, Conrad Meyer wrote:
We still default to bhyveloader(1) if no explicit bootloader
configuration is supplied in the domain.
If the /domain/bootloader looks like grub-bhyve and the user doesn't
supply /domain/bootloader_args, we make an intelligent guess and try
chainloading the first partition on the disk (or a CD if one exists,
under the assumption that for a VM a CD is likely an install source).
Caveat: Assumes the HDD boots from the msdos1 partition. I think this is
a pretty reasonable assumption for a VM. (DrvBhyve with Bhyveload
already assumes that the first disk should be booted.)
I've tested both HDD and CD boot and they seem to work.
---
docs/drvbhyve.html.in | 100 +--
docs/formatdomain.html.in | 4 +-
src/bhyve/bhyve_command.c | 173 +-
src/bhyve/bhyve_command.h | 5 +-
src/bhyve/bhyve_driver.c | 3 +-
src/bhyve/bhyve_process.c | 38 +-
6 files changed, 295 insertions(+), 28 deletions(-)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index bea4a59..203495c 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -26,6 +26,8 @@
#include
#include "bhyve_command.h"
+#include "bhyve_domain.h"
+#include "datatypes.h"
#include "viralloc.h"
#include "virfile.h"
#include "virstring.h"
@@ -294,51 +296,186 @@ virBhyveProcessBuildDestroyCmd(bhyveConnPtr driver
ATTRIBUTE_UNUSED,
return cmd;
}
-virCommandPtr
-virBhyveProcessBuildLoadCmd(virConnectPtr conn,
-virDomainDefPtr def)
+static void
+virAppendBootloaderArgs(virCommandPtr cmd, virDomainDefPtr def)
+{
+char **blargs;
+
+/* XXX: Handle quoted? */
+blargs = virStringSplit(def->os.bootloaderArgs, " ", 0);
+virCommandAddArgSet(cmd, (const char * const *)blargs);
+virStringFreeList(blargs);
+}
+
+static virCommandPtr
+virBhyveProcessBuildBhyveloadCmd(virDomainDefPtr def, virDomainDiskDefPtr disk)
{
virCommandPtr cmd;
-virDomainDiskDefPtr disk;
-if (def->ndisks < 1) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("domain should have at least one disk defined"));
+cmd = virCommandNew(BHYVELOAD);
+
+if (def->os.bootloaderArgs == NULL) {
+VIR_DEBUG("bhyveload with default arguments");
+
+/* Memory (MB) */
+virCommandAddArg(cmd, "-m");
+virCommandAddArgFormat(cmd, "%llu",
+ VIR_DIV_UP(def->mem.max_balloon, 1024));
One of the things I'm worried about is, if the boot args are not
specified we properly add memory configured in domain XML.
+
+/* Image path */
+virCommandAddArg(cmd, "-d");
+virCommandAddArg(cmd, virDomainDiskGetSource(disk));
+
+/* VM name */
+virCommandAddArg(cmd, def->name);
+} else {
+VIR_DEBUG("bhyveload with arguments");
+virAppendBootloaderArgs(cmd, def);
+}
+
However, IIUC the memory amount can be overridden with boot args. Is
that expected?
+return cmd;
+}
+
+static virCommandPtr
+virBhyveProcessBuildCustomLoaderCmd(virDomainDefPtr def)
+{
+virCommandPtr cmd;
+
+if (def->os.bootloaderArgs == NULL) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Custom loader requires explicit %s configuration"),
+ "bootloader_args");
return NULL;
}
-disk = def->disks[0];
+VIR_DEBUG("custom loader '%s' with arguments", def->os.bootloader);
+
+cmd = virCommandNew(def->os.bootloader);
+virAppendBootloaderArgs(cmd, def);
+return cmd;
+}
+
+static bool
+virBhyveUsableDisk(virConnectPtr conn, virDomainDiskDefPtr disk)
+{
if (virStorageTranslateDiskSourcePool(conn, disk) < 0)
-return NULL;
+return false;
if ((disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) &&
(disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("unsupported disk device"));
-return NULL;
+return false;
}
if ((virDomainDiskGetType(disk) != VIR_STORAGE_TYPE_FILE) &&
(virDomainDiskGetType(disk) != VIR_STORAGE_TYPE_VOLUME)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("unsupported disk type"));
-return NULL;
+return false;
}
-cmd = virCommandNew(BHYVELOAD);
+return true;
+}
-/* Memory */
-virCommandAddArg(cmd, "-m");
+static virCommandPtr
+virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def,
+ virConnectPtr conn,
+ const char *devmap_file,
+ char **devicesmap_out)
+{
+virDomainDiskDefPtr disk, cd;
+virBuffer devicemap;
+virCommandPtr cmd;
+size_t i;
+
+if (def->os.bootloaderArgs != NULL)
+return virBhyveProcessBuildCu
Re: [libvirt] [PATCHv8 5/7] bhyve: Probe grub-bhyve for --cons-dev capability
On 08.11.2014 17:48, Conrad Meyer wrote:
---
src/bhyve/bhyve_capabilities.c | 37 +
src/bhyve/bhyve_capabilities.h | 3 +++
2 files changed, 40 insertions(+)
diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
index 132ce91..6e9a943 100644
--- a/src/bhyve/bhyve_capabilities.c
+++ b/src/bhyve/bhyve_capabilities.c
@@ -23,6 +23,7 @@
#include
#include "viralloc.h"
+#include "virfile.h"
#include "virlog.h"
#include "virstring.h"
#include "cpu/cpu.h"
@@ -104,3 +105,39 @@ virBhyveCapsBuild(void)
virObjectUnref(caps);
return NULL;
}
+
+int
+virBhyveProbeGrubCaps(unsigned *caps)
+{
+char *binary, *help;
+virCommandPtr cmd;
+int ret, exit;
+
+ret = 0;
+*caps = 0;
+cmd = NULL;
+help = NULL;
+
+binary = virFindFileInPath("grub-bhyve");
+if (binary == NULL)
+goto out;
+if (!virFileIsExecutable(binary))
+goto out;
+
+cmd = virCommandNew(binary);
+virCommandAddArg(cmd, "--help");
+virCommandSetOutputBuffer(cmd, &help);
+if (virCommandRun(cmd, &exit) < 0) {
+ret = -1;
+goto out;
+}
+
+if (strstr(help, "--cons-dev") != NULL)
+*caps |= BHYVE_GRUB_CAP_CONSDEV;
+
+ out:
+VIR_FREE(help);
+virCommandFree(cmd);
+VIR_FREE(binary);
+return ret;
+}
diff --git a/src/bhyve/bhyve_capabilities.h b/src/bhyve/bhyve_capabilities.h
index c52e0d0..f4ebd90 100644
--- a/src/bhyve/bhyve_capabilities.h
+++ b/src/bhyve/bhyve_capabilities.h
@@ -26,4 +26,7 @@
virCapsPtr virBhyveCapsBuild(void);
+# define BHYVE_GRUB_CAP_CONSDEV 0x0001
This should be rather an enum.
+int virBhyveProbeGrubCaps(unsigned *caps);
+
#endif
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] Fix handling keyboard-interactive callbacks for libssh2
On 10/25/2014 07:16 PM, Cédric Bosdonnat wrote:
> SSHD calls the KI callback with no prompt after all prompts have been
> issued. Just ignore those callbacks to avoid libvirt-java (and possibly
> others) to crash while accessing invalid pointers.
> ---
> src/rpc/virnetsshsession.c | 4
> 1 file changed, 4 insertions(+)
>
This one I'm a bit less positive on - it seems you would be doing the
right thing if the input num_prompts == 0; however, when I read the man
page (libssh2_userauth_keyboard_interactive_ex(3)) of what calls this I see:
response_callback - As authentication proceeds, the host issues several
(1 or more) challenges and requires responses. This callback will be
called at this moment. The callback is responsible to obtain responses
for the challenges, fill the provided data structure and then return
control. Responses will be sent to the host. String values will be
free(3)ed by the library. The callback prototype must match this:
void response(const char *name, int name_len, const char
*instruction, int instruction_len, int num_prompts, const
LIBSSH2_USERAUTH_KBDINT_PROMPT *prompts,
LIBSSH2_USERAUTH_KBDINT_RESPONSE *responses, void **abstract);
This says to me the response_callback shouldn't be called unless there's
1 or more challenges. So are we fixing the root cause of the problem or
a side effect of someone else's bug?
>From a libvirt POV sure this works, but is it the right fix? Perhaps
someone with more libssh2 knowledge can pipe in.
John
> diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c
> index 57119f9..e9516b8 100644
> --- a/src/rpc/virnetsshsession.c
> +++ b/src/rpc/virnetsshsession.c
> @@ -217,6 +217,10 @@ virNetSSHKbIntCb(const char *name ATTRIBUTE_UNUSED,
>
> priv->authCbErr = VIR_NET_SSH_AUTHCB_OK;
>
> +/* After all prompts, sshd calls us with 0 prompts: just ignore it */
> +if (num_prompts == 0)
> +return;
> +
> /* find credential type for asking passwords */
> for (i = 0; i < priv->cred->ncredtype; i++) {
> if (priv->cred->credtype[i] == VIR_CRED_PASSPHRASE ||
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] Fix test wanting a negative size_t
On 10/25/2014 07:16 PM, Cédric Bosdonnat wrote:
> ---
> src/rpc/virnetsshsession.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
Looking through some older patches I had marked as get to some day :-)
ACK
John
> diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c
> index 7f47b29..57119f9 100644
> --- a/src/rpc/virnetsshsession.c
> +++ b/src/rpc/virnetsshsession.c
> @@ -303,6 +303,7 @@ virNetSSHCheckHostKey(virNetSSHSessionPtr sess)
> virConnectCredential askKey;
> struct libssh2_knownhost *knownHostEntry = NULL;
> size_t i;
> +bool hasEchoPrompt = false;
> char *hostnameStr = NULL;
>
> if (sess->hostKeyVerify == VIR_NET_SSH_HOSTKEY_VERIFY_IGNORE)
> @@ -345,12 +346,12 @@ virNetSSHCheckHostKey(virNetSSHSessionPtr sess)
>
> for (i = 0; i < sess->cred->ncredtype; i++) {
> if (sess->cred->credtype[i] == VIR_CRED_ECHOPROMPT) {
> -i = -1;
> +hasEchoPrompt = true;
> break;
> }
> }
>
> -if (i > 0) {
> +if (!hasEchoPrompt) {
> virReportError(VIR_ERR_SSH, "%s",
> _("no suitable method to retrieve "
> "authentication credentials"));
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Re-add use of locking with iptables/ip6tables/ebtables
A previous commit introduced use of locking with invocation
of iptables in the viriptables.c module
commit ba95426d6f39aec1da6e069dd7222f7a8c6a5862
Author: Serge Hallyn
Date: Fri Nov 1 12:36:59 2013 -0500
util: use -w flag when calling iptables
This only ever had effect with the virtual network driver,
as it was not wired up into the nwfilter driver. Unfortunately
in the firewall refactoring the use of the -w flag was
accidentally lost.
This patch introduces it to the virfirewall.c module so that
both the virtual network and nwfilter drivers will be using
it. It also ensures that the equivalent --concurrent flag
to ebtables is used.
---
src/util/virfirewall.c | 67 +++---
src/util/viriptables.c | 2 --
2 files changed, 63 insertions(+), 6 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index bab1634..c83fdc6 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -104,6 +104,44 @@ virFirewallOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virFirewall)
+static bool iptablesUseLock;
+static bool ip6tablesUseLock;
+static bool ebtablesUseLock;
+
+static void
+virFirewallCheckUpdateLock(bool *lockflag,
+ const char *const*args)
+{
+virCommandPtr cmd = virCommandNewArgs(args);
+if (virCommandRun(cmd, NULL) < 0) {
+VIR_INFO("locking not supported by %s", args[0]);
+} else {
+VIR_INFO("using locking for %s", args[0]);
+*lockflag = true;
+}
+virCommandFree(cmd);
+}
+
+static void
+virFirewallCheckUpdateLocking(void)
+{
+const char *iptablesArgs[] = {
+IPTABLES_PATH, "-w", "-L", "-n", NULL,
+};
+const char *ip6tablesArgs[] = {
+IP6TABLES_PATH, "-w", "-L", "-n", NULL,
+};
+const char *ebtablesArgs[] = {
+EBTABLES_PATH, "--concurrent", "-L", NULL,
+};
+virFirewallCheckUpdateLock(&iptablesUseLock,
+ iptablesArgs);
+virFirewallCheckUpdateLock(&ip6tablesUseLock,
+ ip6tablesArgs);
+virFirewallCheckUpdateLock(&ebtablesUseLock,
+ ebtablesArgs);
+}
+
static int
virFirewallValidateBackend(virFirewallBackend backend)
{
@@ -161,6 +199,9 @@ virFirewallValidateBackend(virFirewallBackend backend)
}
currentBackend = backend;
+
+virFirewallCheckUpdateLocking();
+
return 0;
}
@@ -201,6 +242,9 @@ virFirewallPtr virFirewallNew(void)
{
virFirewallPtr firewall;
+if (virFirewallInitialize() < 0)
+return NULL;
+
if (VIR_ALLOC(firewall) < 0)
return NULL;
@@ -321,6 +365,23 @@ virFirewallAddRuleFullV(virFirewallPtr firewall,
rule->queryOpaque = opaque;
rule->ignoreErrors = ignoreErrors;
+switch (rule->layer) {
+case VIR_FIREWALL_LAYER_ETHERNET:
+if (ebtablesUseLock)
+ADD_ARG(rule, "--concurrent");
+break;
+case VIR_FIREWALL_LAYER_IPV4:
+if (iptablesUseLock)
+ADD_ARG(rule, "-w");
+break;
+case VIR_FIREWALL_LAYER_IPV6:
+if (ip6tablesUseLock)
+ADD_ARG(rule, "-w");
+break;
+case VIR_FIREWALL_LAYER_LAST:
+break;
+}
+
while ((str = va_arg(args, char *)) != NULL) {
ADD_ARG(rule, str);
}
@@ -840,8 +901,8 @@ virFirewallApplyGroup(virFirewallPtr firewall,
bool ignoreErrors = (group->actionFlags &
VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
size_t i;
-VIR_INFO("Starting transaction for %p flags=%x",
- group, group->actionFlags);
+VIR_INFO("Starting transaction for firewall=%p group=%p flags=%x",
+ firewall, group, group->actionFlags);
firewall->currentGroup = idx;
group->addingRollback = false;
for (i = 0; i < group->naction; i++) {
@@ -879,8 +940,6 @@ virFirewallApply(virFirewallPtr firewall)
int ret = -1;
virMutexLock(&ruleLock);
-if (virFirewallInitialize() < 0)
-goto cleanup;
if (!firewall || firewall->err == ENOMEM) {
virReportOOMError();
diff --git a/src/util/viriptables.c b/src/util/viriptables.c
index 4f3ac9c..46b4017 100644
--- a/src/util/viriptables.c
+++ b/src/util/viriptables.c
@@ -52,8 +52,6 @@
VIR_LOG_INIT("util.iptables");
-bool iptables_supports_xlock = false;
-
#define VIR_FROM_THIS VIR_FROM_NONE
enum {
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/5] storage: Check for valid fc_host parent at startup
On 11/11/2014 07:21 AM, Michal Privoznik wrote:
> On 10.11.2014 23:45, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1160565
>>
>> If a 'parent' attribute is provided for the fchost, then at startup
>> time check to ensure it is a vport capable scsi_host. If the parent
>> is not vport capable, then disallow the startup. The following is the
>> expected results:
>>
>> error: Failed to start pool fc_pool
>> error: XML error: parent 'scsi_host2' specified for vHBA is not vport capable
>>
>> where the XML for the fc_pool is:
>>
>>
>>fc_pool
>>
>> > wwpn='5001a4a77192b864'/>
>>
>> ...
>>
>> and 'scsi_host2' is not vport capable.
>>
>> Providing an incorrect parent and a correct wwnn/wwpn could lead to
>> failures at shutdown (deleteVport) where the assumption is the parent
>> is for the fchost.
>>
>> NOTE: If the provided wwnn/wwpn doesn't resolve to an existing scsi_host,
>>then we will be creating one with code (virManageVport) which
>>assumes the parent is vport capable.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/storage/storage_backend_scsi.c | 22 ++
>> 1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_scsi.c
>> b/src/storage/storage_backend_scsi.c
>> index 02160bc..549d8db 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -556,6 +556,20 @@ createVport(virStoragePoolSourceAdapter adapter)
>> if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
>> return 0;
>>
>> +/* If a parent was provided, then let's make sure it's vhost capable */
>> +if (adapter.data.fchost.parent) {
>> +if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host)
>> < 0)
>> +return -1;
^^^ [1] ^^^
>> +
>> +if (!virIsCapableFCHost(NULL, parent_host)) {
>> +virReportError(VIR_ERR_XML_ERROR,
>> + _("parent '%s' specified for vHBA "
>> + "is not vport capable"),
>> + adapter.data.fchost.parent);
>> +return -1;
>> +}
>> +}
>> +
>> /* This filters either HBA or already created vHBA */
>> if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
>> adapter.data.fchost.wwpn))) {
>> @@ -565,14 +579,14 @@ createVport(virStoragePoolSourceAdapter adapter)
>>
>> if (!adapter.data.fchost.parent &&
>> !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) {
>> - virReportError(VIR_ERR_XML_ERROR, "%s",
>> +virReportError(VIR_ERR_XML_ERROR, "%s",
>> _("'parent' for vHBA not specified, and "
>>"cannot find one on this host"));
>>return -1;
>> -}
>>
>> -if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
>> -return -1;
>> +if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host)
>> < 0)
>> +return -1;
>> +}
>
> This chunk seems odd. After the error is reported, the control jumps out
> from the function, so virGetSCSIHostNumer is not called at all. Did you
> forget to commit something?
>
The earlier chunk of code where the parent exists, we figure the
parent_host value. [1]
This chunk is - if a parent wasn't provided, find a capable vport, then
get the parent_host value.
I moved it inside the if because it makes no sense to call the function
twice in the event we had a parent value..
John
>>
>> if (virManageVport(parent_host, adapter.data.fchost.wwpn,
>> adapter.data.fchost.wwnn, VPORT_CREATE) < 0)
>>
>
> Michal
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] Fix virDomainChrEquals for spicevmc
On Tue, Nov 11, 2014 at 12:23:44PM +0100, Ján Tomko wrote: virDomainChrSourceDefIsEqual should return 'true' for identical SPICEVMC chardevs, and those that have no source specification. After this change, a failed hotplug no longer leaves a stale pointer in the domain definition. https://bugzilla.redhat.com/show_bug.cgi?id=1162097 --- src/conf/domain_conf.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 54b2bfe..73b2393 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1591,13 +1591,14 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, tgt->data.spiceport.channel); break; +case VIR_DOMAIN_CHR_TYPE_SPICEVMC: +return src->data.spicevmc == tgt->data.spicevmc; + case VIR_DOMAIN_CHR_TYPE_NULL: case VIR_DOMAIN_CHR_TYPE_VC: case VIR_DOMAIN_CHR_TYPE_STDIO: -case VIR_DOMAIN_CHR_TYPE_SPICEVMC: case VIR_DOMAIN_CHR_TYPE_LAST: -/* nada */ -break; +return true; } return false; Probably a dead code and very possibly an ewww statement. Either remove it or change it to true and remove the one you added few lines up, please. Martin signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 4/7] qemu: Add bps_max and friends qemu driver
On 11/10/2014 10:19 AM, Matthias Gatto wrote:
> Add support for bps_max and friends in the driver part.
> In the part checking if a qemu is running, check if the running binary
> support bps_max, if not print an error message, if yes add it to
> "info" variable
>
> This patch follow therse patchs:
> http://www.redhat.com/archives/libvir-list/2014-November/msg00271.html
> I've fix the syntax error and the nparams detection problem
>
> Signed-off-by: Matthias Gatto
> ---
> include/libvirt/libvirt-domain.h | 56 +++
> src/qemu/qemu_driver.c | 197
> ---
> src/qemu/qemu_monitor.c | 10 +-
> src/qemu/qemu_monitor.h | 6 +-
> src/qemu/qemu_monitor_json.c | 11 ++-
> src/qemu/qemu_monitor_json.h | 6 +-
> tests/qemumonitorjsontest.c | 4 +-
> 7 files changed, 264 insertions(+), 26 deletions(-)
>
Coverity is unhappy here too - DEADCODE...
<...snip...>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 5eccacc..242b30e 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
<...snip...>
> @@ -16753,13 +16870,14 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
> {
> virQEMUDriverPtr driver = dom->conn->privateData;
> virDomainObjPtr vm = NULL;
> -qemuDomainObjPrivatePtr priv;
> +qemuDomainObjPrivatePtr priv = NULL;
> virDomainDefPtr persistentDef = NULL;
> virDomainBlockIoTuneInfo reply;
> char *device = NULL;
> int ret = -1;
> size_t i;
> virCapsPtr caps = NULL;
(1) Event assignment: Assigning: "supportMaxOptions" = "true".
Also see events:
> +bool supportMaxOptions = true;
>
> virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>VIR_DOMAIN_AFFECT_CONFIG |
> @@ -16777,13 +16895,6 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
> if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> goto cleanup;
>
> -if ((*nparams) == 0) {
> -/* Current number of parameters supported by QEMU Block I/O
> Throttling */
> -*nparams = QEMU_NB_BLOCK_IO_TUNE_PARAM;
> -ret = 0;
> -goto cleanup;
> -}
> -
> if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> goto cleanup;
>
> @@ -16791,6 +16902,18 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
> &persistentDef) < 0)
> goto endjob;
>
> +if ((*nparams) == 0) {
> +if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> +priv = vm->privateData;
> +/* If the VM is running, we can check if the current VM can use
> optional parameters or not */
> +/* We didn't made this check sooner because we need the VM data
> to do so */
> +supportMaxOptions = virQEMUCapsGet(priv->qemuCaps,
> QEMU_CAPS_DRIVE_IOTUNE_MAX);
> +}
> +*nparams = supportMaxOptions ? QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX :
> QEMU_NB_BLOCK_IO_TUNE_PARAM;
> +ret = 0;
> +goto endjob;
The only way supportMaxOptions can change (be false) is in this block;
however, the "goto endjob;" here jumps around the later check regarding
where Coverity complains about DEADCODE
Less sure how to handle in this case, but my gut says the (!supportMaxOptions &&
check below is unnecessary.
THoughts?
John
> +}
> +
> device = qemuDiskPathToAlias(vm, disk, NULL);
> if (!device) {
> goto endjob;
> @@ -16799,7 +16922,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
> if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> priv = vm->privateData;
> qemuDomainObjEnterMonitor(driver, vm);
> -ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply);
> +ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply,
> supportMaxOptions);
> qemuDomainObjExitMonitor(driver, vm);
> if (ret < 0)
> goto endjob;
> @@ -16816,7 +16939,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
> reply = persistentDef->disks[idx]->blkdeviotune;
> }
>
> -for (i = 0; i < QEMU_NB_BLOCK_IO_TUNE_PARAM && i < *nparams; i++) {
> +for (i = 0; i < QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX && i < *nparams; i++) {
> virTypedParameterPtr param = ¶ms[i];
>
> switch (i) {
> @@ -16862,14 +16985,64 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
> reply.write_iops_sec) < 0)
> goto endjob;
> break;
> +case 6:
> +if (virTypedParameterAssign(param,
> +
> VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX,
> +VIR_TYPED_PARAM_ULLONG,
> +reply.total_bytes_sec_max) < 0)
> +goto endjob;
> +break;
> +case 7:
> +if (virTypedParameterAssign(param,
> +
> VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SE
Re: [libvirt] [PATCH v6 6/7] qemu: Add bps_max and friends to qemu command generation
On 10/29/2014 08:16 AM, Matthias Gatto wrote:
> Check the arability of the options with the current qemu binary,
> add them in the varable opt if yes, print a message if not.
>
> Signed-off-by: Matthias Gatto
> ---
> src/qemu/qemu_command.c | 57
> -
> 1 file changed, 56 insertions(+), 1 deletion(-)
>
Coverity was a bit unhappy about this change...
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2e5af4f..b3dc919 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3669,12 +3669,32 @@ qemuBuildDriveStr(virConnectPtr conn,
> goto error;
> }
>
> +/* block I/O throttling 1.7 */
> +if ((disk->blkdeviotune.total_bytes_sec_max ||
> + disk->blkdeviotune.read_bytes_sec_max ||
> + disk->blkdeviotune.write_bytes_sec_max ||
> + disk->blkdeviotune.total_iops_sec_max ||
> + disk->blkdeviotune.read_iops_sec_max ||
> + disk->blkdeviotune.write_iops_sec_max) &&
> +!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("there is some block I/O throttling paramater that
> are not supported with this "
> + "QEMU binary(need QEMU 1.7 or superior)"));
> +goto error;
> +}
> +
> if (disk->blkdeviotune.total_bytes_sec > LLONG_MAX ||
> disk->blkdeviotune.read_bytes_sec > LLONG_MAX ||
> disk->blkdeviotune.write_bytes_sec > LLONG_MAX ||
> disk->blkdeviotune.total_iops_sec > LLONG_MAX ||
> disk->blkdeviotune.read_iops_sec > LLONG_MAX ||
> -disk->blkdeviotune.write_iops_sec > LLONG_MAX) {
> +disk->blkdeviotune.write_iops_sec > LLONG_MAX ||
> +disk->blkdeviotune.total_bytes_sec_max > LLONG_MAX ||
> +disk->blkdeviotune.read_bytes_sec_max > LLONG_MAX ||
> +disk->blkdeviotune.write_bytes_sec_max > LLONG_MAX ||
> +disk->blkdeviotune.total_iops_sec_max > LLONG_MAX ||
> +disk->blkdeviotune.read_iops_sec_max > LLONG_MAX ||
> +disk->blkdeviotune.write_iops_sec_max > LLONG_MAX) {
> virReportError(VIR_ERR_OVERFLOW,
>_("block I/O throttle limit must "
> "be less than %llu using QEMU"), LLONG_MAX);
> @@ -3711,6 +3731,41 @@ qemuBuildDriveStr(virConnectPtr conn,
>disk->blkdeviotune.write_iops_sec);
> }
>
> +if (disk->blkdeviotune.total_bytes_sec_max) {
> +virBufferAsprintf(&opt, ",bps_max=%llu",
> + disk->blkdeviotune.total_bytes_sec_max);
> +}
> +
> +if (disk->blkdeviotune.read_bytes_sec_max) {
> +virBufferAsprintf(&opt, ",bps_rd_max=%llu",
> + disk->blkdeviotune.read_bytes_sec_max);
> +}
> +
> +if (disk->blkdeviotune.write_bytes_sec_max) {
> +virBufferAsprintf(&opt, ",bps_wr_max=%llu",
> + disk->blkdeviotune.write_bytes_sec_max);
> +}
> +
> +if (disk->blkdeviotune.total_iops_sec_max) {
> +virBufferAsprintf(&opt, ",iops_max=%llu",
> + disk->blkdeviotune.total_iops_sec_max);
> +}
> +
> +if (disk->blkdeviotune.read_iops_sec_max) {
> +virBufferAsprintf(&opt, ",iops_rd_max=%llu",
> + disk->blkdeviotune.read_iops_sec_max);
> +}
> +
> +if (disk->blkdeviotune.write_iops_sec_max) {
> +virBufferAsprintf(&opt, ",iops_wr_max=%llu",
> + disk->blkdeviotune.write_iops_sec_max);
> +}
> +
> +if (disk->blkdeviotune.write_iops_sec_max) {
> +virBufferAsprintf(&opt, ",iops_size=%llu",
> + disk->blkdeviotune.size_iops_sec);
> +}
> +
3755if (disk->blkdeviotune.read_iops_sec_max) {
3756virBufferAsprintf(&opt, ",iops_rd_max=%llu",
3757 disk->blkdeviotune.read_iops_sec_max);
3758}
3759
(1) Event original: "disk->blkdeviotune.write_iops_sec_max" looks like the
original copy.
Also see events:[copy_paste_error][remediation]
3760if (disk->blkdeviotune.write_iops_sec_max) {
3761virBufferAsprintf(&opt, ",iops_wr_max=%llu",
3762 disk->blkdeviotune.write_iops_sec_max);
3763}
3764
(2) Event copy_paste_error: "write_iops_sec_max" in
"disk->blkdeviotune.write_iops_sec_max" looks like a copy-paste error.
(3) Event remediation: Should it say "size_iops_sec" instead?
Also see events:[original]
3765if (disk->blkdeviotune.write_iops_sec_max) {
3766virBufferAsprintf(&opt, ",iops_size=%llu",
3767 disk->blkdeviotune.size_iops_sec);
3768}
I "assume" the (2) if should be "if (disk->blkdeviotune.size_iops_sec) {",
correct?
John
> if (virBufferCheckError(&opt) < 0)
> goto error;
>
>
--
libvi
Re: [libvirt] [PATCHv2] Fix virDomainChrEquals for spicevmc
On 11/11/2014 12:23 PM, Ján Tomko wrote: virDomainChrSourceDefIsEqual should return 'true' for identical SPICEVMC chardevs, and those that have no source specification. After this change, a failed hotplug no longer leaves a stale pointer in the domain definition. https://bugzilla.redhat.com/show_bug.cgi?id=1162097 --- src/conf/domain_conf.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 54b2bfe..73b2393 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1591,13 +1591,14 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, tgt->data.spiceport.channel); break; +case VIR_DOMAIN_CHR_TYPE_SPICEVMC: +return src->data.spicevmc == tgt->data.spicevmc; + case VIR_DOMAIN_CHR_TYPE_NULL: case VIR_DOMAIN_CHR_TYPE_VC: case VIR_DOMAIN_CHR_TYPE_STDIO: -case VIR_DOMAIN_CHR_TYPE_SPICEVMC: case VIR_DOMAIN_CHR_TYPE_LAST: -/* nada */ -break; +return true; } return false; ACK, Pavel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/5] storage: Check for valid fc_host parent at startup
On 10.11.2014 23:45, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1160565
If a 'parent' attribute is provided for the fchost, then at startup
time check to ensure it is a vport capable scsi_host. If the parent
is not vport capable, then disallow the startup. The following is the
expected results:
error: Failed to start pool fc_pool
error: XML error: parent 'scsi_host2' specified for vHBA is not vport capable
where the XML for the fc_pool is:
fc_pool
...
and 'scsi_host2' is not vport capable.
Providing an incorrect parent and a correct wwnn/wwpn could lead to
failures at shutdown (deleteVport) where the assumption is the parent
is for the fchost.
NOTE: If the provided wwnn/wwpn doesn't resolve to an existing scsi_host,
then we will be creating one with code (virManageVport) which
assumes the parent is vport capable.
Signed-off-by: John Ferlan
---
src/storage/storage_backend_scsi.c | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index 02160bc..549d8db 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -556,6 +556,20 @@ createVport(virStoragePoolSourceAdapter adapter)
if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
return 0;
+/* If a parent was provided, then let's make sure it's vhost capable */
+if (adapter.data.fchost.parent) {
+if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
+return -1;
+
+if (!virIsCapableFCHost(NULL, parent_host)) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("parent '%s' specified for vHBA "
+ "is not vport capable"),
+ adapter.data.fchost.parent);
+return -1;
+}
+}
+
/* This filters either HBA or already created vHBA */
if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
adapter.data.fchost.wwpn))) {
@@ -565,14 +579,14 @@ createVport(virStoragePoolSourceAdapter adapter)
if (!adapter.data.fchost.parent &&
!(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
+virReportError(VIR_ERR_XML_ERROR, "%s",
_("'parent' for vHBA not specified, and "
"cannot find one on this host"));
return -1;
-}
-if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
-return -1;
+if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
+return -1;
+}
This chunk seems odd. After the error is reported, the control jumps out
from the function, so virGetSCSIHostNumer is not called at all. Did you
forget to commit something?
if (virManageVport(parent_host, adapter.data.fchost.wwpn,
adapter.data.fchost.wwnn, VPORT_CREATE) < 0)
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 5/5] storage: Introduce 'managed' for the fchost parent
On 11/10/2014 05:45 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1160926
>
> Introduce a 'managed' attribute to allow libvirt to decide whether to
> delete a vHBA vport created via external means such as nodedev-create.
> The code currently decides whether to delete the vHBA based solely on
> whether the parent was provided at creation time. However, that may not
> be the desired action, so rather than delete and force someone to create
> another vHBA via an additional nodedev-create allow the configuration of
> the storage pool to decide the desired action.
>
> During createVport when libvirt does the VPORT_CREATE, set the managed
> value to YES if not already set to indicate to the deleteVport code that
> it should delete the vHBA when the pool is destroyed.
>
> If libvirtd is restarted all the memory only state was lost, so for a
> persistent storage pool, use the virStoragePoolSaveConfig in order to
> write out the managed value.
>
> Because we're now saving the current configuration, we need to be sure
> to not save the parent in the output XML if it was undefined at start.
> Saving the name would cause future starts to always use the same parent
> which is not the expected result when not providing a parent. By not
> providing a parent, libvirt is expected to find the best available
> vHBA port for each subsequent (re)start.
>
> At deleteVport, use the new managed value to decide whether to execute
> the VPORT_DELETE. Since we no longer save the parent in memory or in
> XML when provided, if it was not provided, then we have to look it up.
>
> Signed-off-by: John Ferlan
> ---
> docs/formatstorage.html.in | 13 +++
> docs/schemas/basictypes.rng| 5 ++
> src/conf/storage_conf.c| 17
> src/conf/storage_conf.h| 1 +
> src/storage/storage_backend_scsi.c | 93
> +-
> .../pool-scsi-type-fc-host-managed.xml | 15
> .../pool-scsi-type-fc-host-managed.xml | 18 +
> tests/storagepoolxml2xmltest.c | 1 +
> 8 files changed, 143 insertions(+), 20 deletions(-)
> create mode 100644
> tests/storagepoolxml2xmlin/pool-scsi-type-fc-host-managed.xml
> create mode 100644
> tests/storagepoolxml2xmlout/pool-scsi-type-fc-host-managed.xml
>
<...snip...>
Consider the following squished into deleteVport as testing asked about a
condition where someone does a 'virsh nodedev-destroy' on the vHBA
we've created resulting in the virGetFCHostNameByWWN() rightfully
returning NULL (just like it would in the createVport case when the
wwnn/wwpn doesn't exist). Allowing virsh nodedev-destroy to remove an
"active" vHBA is perhaps a different issue...
The desire was to get a real error message instead of:
destroy the 'fc_host' pool
# virsh pool-destroy fc-pool
error: Failed to destroy pool fc-pool
error: An error occurred, but the cause is unknown
#
> diff --git a/src/storage/storage_backend_scsi.c
> b/src/storage/storage_backend_scsi.c
> index 41ea67a..b1602ea 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
<...snip...>
> static int
> -deleteVport(virStoragePoolSourceAdapter adapter)
> +deleteVport(virConnectPtr conn,
> +virStoragePoolSourceAdapter adapter)
> {
> unsigned int parent_host;
> char *name = NULL;
> +char *vhba_parent = NULL;
> int ret = -1;
>
> if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
> return 0;
>
> -/* It must be a HBA instead of a vHBA as long as "parent"
> - * is NULL. "createVport" guaranteed "parent" for a vHBA
> - * cannot be NULL, it's either specified in XML, or detected
> - * automatically.
> - */
> -if (!adapter.data.fchost.parent)
> +VIR_DEBUG("conn=%p parent='%s', managed='%d' wwnn='%s' wwpn='%s'",
> + conn, NULLSTR(adapter.data.fchost.parent),
> + adapter.data.fchost.managed,
> + adapter.data.fchost.wwnn,
> + adapter.data.fchost.wwpn);
> +
> +/* If we're not managing the deletion of the vHBA, then just return */
> +if (adapter.data.fchost.managed != VIR_TRISTATE_BOOL_YES)
> return 0;
>
> +/* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */
> if (!(name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
> adapter.data.fchost.wwpn)))
> -return -1;
> -
> -if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
> goto cleanup;
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_sc
index b1602ea..3f61610 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -764,8 +764,12 @@ deleteVport(virConnectPtr conn,
/* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn
[libvirt] [PATCHv2] Fix virDomainChrEquals for spicevmc
virDomainChrSourceDefIsEqual should return 'true' for identical SPICEVMC chardevs, and those that have no source specification. After this change, a failed hotplug no longer leaves a stale pointer in the domain definition. https://bugzilla.redhat.com/show_bug.cgi?id=1162097 --- src/conf/domain_conf.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 54b2bfe..73b2393 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1591,13 +1591,14 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, tgt->data.spiceport.channel); break; +case VIR_DOMAIN_CHR_TYPE_SPICEVMC: +return src->data.spicevmc == tgt->data.spicevmc; + case VIR_DOMAIN_CHR_TYPE_NULL: case VIR_DOMAIN_CHR_TYPE_VC: case VIR_DOMAIN_CHR_TYPE_STDIO: -case VIR_DOMAIN_CHR_TYPE_SPICEVMC: case VIR_DOMAIN_CHR_TYPE_LAST: -/* nada */ -break; +return true; } return false; -- 2.0.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V2 0/3] Fix some problems of numatune
On Mon, Nov 10, 2014 at 09:53:16PM +0800, Wang Rui wrote: Fix startup failing with memory mode(strict, preferred or interleave) in numatune V1: https://www.redhat.com/archives/libvir-list/2014-November/msg00057.html V2 has been revised as Martin' suggestion. ACK && Pushed. Thank you, Martin signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] virsh: Enforce proper ordering of options
Even though vshCmddefOptParse() tried returning -1 if there was an
optional option specification that preceded a required one, it failed to
check that for boolean type options and options with VSH_OFLAG_REQ_OPT
flag set. On the other hand, it makes sense that VSH_OT_ARGV is
specified at the end of the option list.
Returning -1 enforces the proper ordering thanks to virsh-synopsis test
in 'make check'.
Signed-off-by: Martin Kletzander
---
tools/virsh.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 036b517..41893bb 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1073,6 +1073,7 @@ vshCmddefOptParse(const vshCmdDef *cmd, uint32_t
*opts_need_arg,
if (i > 31)
return -1; /* too many options */
if (opt->type == VSH_OT_BOOL) {
+optional = true;
if (opt->flags & VSH_OFLAG_REQ)
return -1; /* bool options can't be mandatory */
continue;
@@ -1105,12 +1106,14 @@ vshCmddefOptParse(const vshCmdDef *cmd, uint32_t
*opts_need_arg,
if (opt->flags & VSH_OFLAG_REQ_OPT) {
if (opt->flags & VSH_OFLAG_REQ)
*opts_required |= 1 << i;
+else
+optional = true;
continue;
}
*opts_need_arg |= 1 << i;
if (opt->flags & VSH_OFLAG_REQ) {
-if (optional)
+if (optional && opt->type != VSH_OT_ARGV)
return -1; /* mandatory options must be listed first */
*opts_required |= 1 << i;
} else {
--
2.1.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] virsh: Error out if VSH_OT_STRING option has VSH_OFLAG_REQ flag
Recent commit 12bd207e217f3c5dc2272a5ea943b81067bd8034 fixed few
VSH_OT_STRING options that should've been VSH_OT_DATA. That lead me to
this commit that enforces people to check that newly added options have
proper type. Thanks to virsh erroring out with error message, this will
immediately show up in 'make check' thanks to our virsh-synopsis test.
Signed-off-by: Martin Kletzander
---
tools/virsh.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/tools/virsh.c b/tools/virsh.c
index 41893bb..e10b3de 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1386,6 +1386,12 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname)
break;
case VSH_OT_STRING:
/* OT_STRING should never be VSH_OFLAG_REQ */
+if (opt->flags & VSH_OFLAG_REQ) {
+vshError(ctl,
+ _("internal error: bad options in command:
'%s'"),
+ def->name);
+return false;
+}
snprintf(buf, sizeof(buf), _("--%s "), opt->name);
break;
case VSH_OT_DATA:
--
2.1.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] virsh: Cleanups and checks for options
Option-ordering and typing checks and a reorder so tests pass. Martin Kletzander (3): virsh: Reorder some options virsh: Enforce proper ordering of options virsh: Error out if VSH_OT_STRING option has VSH_OFLAG_REQ flag tools/virsh-domain.c | 76 ++-- tools/virsh-pool.c | 8 +++--- tools/virsh-volume.c | 8 +++--- tools/virsh.c| 11 +++- 4 files changed, 56 insertions(+), 47 deletions(-) -- 2.1.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] virsh: Reorder some options
According to comments in parsing functions, optional options should be
specified *after* required ones. It makes sense and help output looks
cleaner. The only exceptions are options with type == VSH_OT_ARGV.
Signed-off-by: Martin Kletzander
---
tools/virsh-domain.c | 76 ++--
tools/virsh-pool.c | 8 +++---
tools/virsh-volume.c | 8 +++---
3 files changed, 46 insertions(+), 46 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 8e743f1..541363d 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3222,11 +3222,6 @@ static const vshCmdOptDef opts_dom_pm_suspend[] = {
.flags = VSH_OFLAG_REQ,
.help = N_("domain name, id or uuid")
},
-{.name = "duration",
- .type = VSH_OT_INT,
- .flags = VSH_OFLAG_REQ_OPT,
- .help = N_("duration in seconds")
-},
{.name = "target",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
@@ -3234,6 +3229,11 @@ static const vshCmdOptDef opts_dom_pm_suspend[] = {
"disk(Suspend-to-Disk), "
"hybrid(Hybrid-Suspend)")
},
+{.name = "duration",
+ .type = VSH_OT_INT,
+ .flags = VSH_OFLAG_REQ_OPT,
+ .help = N_("duration in seconds")
+},
{.name = NULL}
};
@@ -3940,10 +3940,6 @@ static const vshCmdInfo info_save[] = {
};
static const vshCmdOptDef opts_save[] = {
-{.name = "bypass-cache",
- .type = VSH_OT_BOOL,
- .help = N_("avoid file system cache when saving")
-},
{.name = "domain",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
@@ -3954,6 +3950,10 @@ static const vshCmdOptDef opts_save[] = {
.flags = VSH_OFLAG_REQ,
.help = N_("where to save the data")
},
+{.name = "bypass-cache",
+ .type = VSH_OT_BOOL,
+ .help = N_("avoid file system cache when saving")
+},
{.name = "xml",
.type = VSH_OT_STRING,
.help = N_("filename containing updated XML for the target")
@@ -4408,15 +4408,15 @@ static const vshCmdInfo info_managedsave[] = {
};
static const vshCmdOptDef opts_managedsave[] = {
-{.name = "bypass-cache",
- .type = VSH_OT_BOOL,
- .help = N_("avoid file system cache when saving")
-},
{.name = "domain",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
.help = N_("domain name, id or uuid")
},
+{.name = "bypass-cache",
+ .type = VSH_OT_BOOL,
+ .help = N_("avoid file system cache when saving")
+},
{.name = "running",
.type = VSH_OT_BOOL,
.help = N_("set domain to be running on next start")
@@ -4918,6 +4918,16 @@ static const vshCmdInfo info_dump[] = {
};
static const vshCmdOptDef opts_dump[] = {
+{.name = "domain",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("domain name, id or uuid")
+},
+{.name = "file",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("where to dump the core")
+},
{.name = "live",
.type = VSH_OT_BOOL,
.help = N_("perform a live core dump if supported")
@@ -4934,16 +4944,6 @@ static const vshCmdOptDef opts_dump[] = {
.type = VSH_OT_BOOL,
.help = N_("reset the domain after core dump")
},
-{.name = "domain",
- .type = VSH_OT_DATA,
- .flags = VSH_OFLAG_REQ,
- .help = N_("domain name, id or uuid")
-},
-{.name = "file",
- .type = VSH_OT_DATA,
- .flags = VSH_OFLAG_REQ,
- .help = N_("where to dump the core")
-},
{.name = "verbose",
.type = VSH_OT_BOOL,
.help = N_("display the progress of dump")
@@ -7452,6 +7452,11 @@ static const vshCmdOptDef opts_metadata[] = {
.flags = VSH_OFLAG_REQ,
.help = N_("domain name, id or uuid")
},
+{.name = "uri",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("URI of the namespace")
+},
{.name = "live",
.type = VSH_OT_BOOL,
.help = N_("modify/get running state")
@@ -7468,11 +7473,6 @@ static const vshCmdOptDef opts_metadata[] = {
.type = VSH_OT_BOOL,
.help = N_("use an editor to change the metadata")
},
-{.name = "uri",
- .type = VSH_OT_DATA,
- .flags = VSH_OFLAG_REQ,
- .help = N_("URI of the namespace")
-},
{.name = "key",
.type = VSH_OT_DATA,
.help = N_("key to be used as a namespace identifier"),
@@ -9269,6 +9269,16 @@ static const vshCmdInfo info_migrate[] = {
};
static const vshCmdOptDef opts_migrate[] = {
+{.name = "domain",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("domain name, id or uuid")
+},
+{.name = "desturi",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("connection URI of the destination host as seen from the
client(normal migration) or source(p2p migration)")
+},
{.name = "live",
.type = VSH_OT_BOOL,
.help = N_("live migration")
@@ -9341,16 +9351,6 @@ static const vshCmdOptDef opts_migrate[] = {
.type
Re: [libvirt] [libvirt-python][PATCH] Add dict check for setTime and allow pass 'seconds' parameter
On 11/11/2014 10:50 AM, Michal Privoznik wrote:
From: Luyao Huang
When pass None or a empty dictionary to time, it will report
error. This commit allows a one-element dictionary which contains
just 'seconds' field, which results in the same as passing 0 for
'nseconds' field. Moreover, dict is checked for unknown fields.
Signed-off-by: Luyao Huang
Signed-off-by: Michal Privoznik
---
libvirt-override-virDomain.py | 4 ++--
libvirt-override.c| 39 +++
2 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/libvirt-override-virDomain.py b/libvirt-override-virDomain.py
index a50ec0d..fa5f75f 100644
--- a/libvirt-override-virDomain.py
+++ b/libvirt-override-virDomain.py
@@ -67,8 +67,8 @@
return ret
def setTime(self, time=None, flags=0):
-"""Set guest time to the given value. @time is a dict conatining
-'seconds' field for seconds and 'nseconds' field for nanosecons """
+"""Set guest time to the given value. @time is a dict containing
+'seconds' field for seconds and 'nseconds' field for nanoseconds """
ret = libvirtmod.virDomainSetTime(self._o, time, flags)
if ret == -1: raise libvirtError ('virDomainSetTime() failed',
dom=self)
return ret
diff --git a/libvirt-override.c b/libvirt-override.c
index c01e52f..57f0ccf 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -7785,12 +7785,14 @@ static PyObject *
libvirt_virDomainSetTime(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
PyObject *py_retval = NULL;
PyObject *pyobj_domain;
+PyObject *pyobj_seconds;
+PyObject *pyobj_nseconds;
PyObject *py_dict;
virDomainPtr domain;
long long seconds = 0;
unsigned int nseconds = 0;
unsigned int flags;
-ssize_t py_dict_size;
+ssize_t py_dict_size = 0;
int c_retval;
if (!PyArg_ParseTuple(args, (char*)"OOI:virDomainSetTime",
@@ -7798,26 +7800,31 @@ libvirt_virDomainSetTime(PyObject *self
ATTRIBUTE_UNUSED, PyObject *args) {
return NULL;
domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
-py_dict_size = PyDict_Size(py_dict);
-
-if (py_dict_size == 2) {
-PyObject *pyobj_seconds, *pyobj_nseconds;
-
-if (!(pyobj_seconds = PyDict_GetItemString(py_dict, "seconds")) ||
-(libvirt_longlongUnwrap(pyobj_seconds, &seconds) < 0)) {
-PyErr_Format(PyExc_LookupError, "malformed or missing 'seconds'");
+if (PyDict_Check(py_dict)) {
+py_dict_size = PyDict_Size(py_dict);
+if ((pyobj_seconds = PyDict_GetItemString(py_dict, "seconds"))) {
+if (libvirt_longlongUnwrap(pyobj_seconds, &seconds) < 0) {
+PyErr_Format(PyExc_LookupError, "malformed 'seconds'");
+goto cleanup;
+}
+} else {
+PyErr_Format(PyExc_LookupError, "Dictionary must contains
'seconds'");
goto cleanup;
}
-if (!(pyobj_nseconds = PyDict_GetItemString(py_dict, "nseconds")) ||
-(libvirt_uintUnwrap(pyobj_nseconds, &nseconds) < 0)) {
-PyErr_Format(PyExc_LookupError, "malformed or missing 'nseconds'");
+if ((pyobj_nseconds = PyDict_GetItemString(py_dict, "nseconds"))) {
+if (libvirt_uintUnwrap(pyobj_nseconds, &nseconds) < 0) {
+PyErr_Format(PyExc_LookupError, "malformed 'nseconds'");
+goto cleanup;
+}
+} else if (py_dict_size > 1) {
+PyErr_Format(PyExc_LookupError, "Dictionary contains unknown key");
goto cleanup;
}
-} else if (py_dict_size > 0) {
-PyErr_Format(PyExc_LookupError, "Dictionary must contain "
- "'seconds' and 'nseconds'");
-goto cleanup;
+} else if (py_dict != Py_None || !flags) {
+PyErr_Format(PyExc_TypeError, "time must be a dictionary "
+ "or None with flags set");
+return NULL;
You are returning NULL here which is fine and correct, but to keep the
code consistent either use "goto cleanup" here or change all the other
cases to "return NULL". I'll personally go with "return NULL" and remove
the unnecessary cleanup as it just returns NULL or py_int.
ACK with that change,
Pavel
}
LIBVIRT_BEGIN_ALLOW_THREADS;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] selinux: ignore missing imagelabel in SetSavedStateLabel
On Tue, Nov 11, 2014 at 09:07:08AM +0100, Ján Tomko wrote: If we have no imagelabel to set (e.g. the domain was created by qemu-attach where we don't generate an imagelabel) return success instead of crashing. https://bugzilla.redhat.com/show_bug.cgi?id=1161831 --- src/security/security_selinux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f96be50..db0df7d 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1970,7 +1970,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); -if (!secdef || !secdef->relabel) +if (!secdef || !secdef->relabel || !secdef->imagelabel) return 0; Are you sure this is the only function that needs this? I think more APIs will cause a crash with attached domain. For example hot-plugging a disk. I think that we should either create a unified way of getting (image)labels from secdefs or, if there is no way that selinux secdef exists without imagelabel for normal domain, we should just fill/clean labels for attached domains and document that attaching to a domain automatically implies model="none" or relabe="yes" or whatever. Martin signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix invalid log, misused option types and a typo
On 11.11.2014 03:12, Hao Liu wrote: This patch fixes the following issues. 1) When an invalid wwn is introduced, libvirt reports "Malformed wwn: %s". The template won't be replaced. 2) "target" option for dompmsuspend and "xml" option for save-image-define are required options and should use VSH_OT_DATA instead of VSH_OT_STRING as an option type. 3) A typo. Signed-off-by: Hao Liu --- src/util/virutil.c | 4 ++-- tools/virsh-domain.c | 4 ++-- tools/virsh-host.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) ACKed & pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [python v3 PATCH] Add dict check for setTime and allow pass 'seconds' parameter
On 31.10.2014 03:02, Luyao Huang wrote:
When pass None or a empty dictionary to time, it will
report error.Allow a one-element dictionary which
contains 'seconds',setting JUST seconds will do the
sane thing of passing 0 for nseconds, instead of
erroring out.If dict have a unkown key, it will report error.
Signed-off-by: Luyao Huang
---
libvirt-override-virDomain.py | 6 +++---
libvirt-override.c| 41 +
2 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/libvirt-override-virDomain.py b/libvirt-override-virDomain.py
index a50ec0d..2a4c4c9 100644
--- a/libvirt-override-virDomain.py
+++ b/libvirt-override-virDomain.py
@@ -66,9 +66,9 @@
if ret == -1: raise libvirtError ('virDomainGetTime() failed',
dom=self)
return ret
-def setTime(self, time=None, flags=0):
-"""Set guest time to the given value. @time is a dict conatining
-'seconds' field for seconds and 'nseconds' field for nanosecons """
+def setTime(self, time, flags=0):
+"""Set guest time to the given value. @time is a dict containing
+'seconds' field for seconds and 'nseconds' field for nanoseconds """
ret = libvirtmod.virDomainSetTime(self._o, time, flags)
if ret == -1: raise libvirtError ('virDomainSetTime() failed',
dom=self)
return ret
Well, C API doesn't require @seconds and @nseconds to be always set, ie
when using VIR_DOMAIN_TIME_SYNC flag. I believe python binding should
follow the design. I'll post updated patch shortly.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-python][PATCH] Add dict check for setTime and allow pass 'seconds' parameter
From: Luyao Huang
When pass None or a empty dictionary to time, it will report
error. This commit allows a one-element dictionary which contains
just 'seconds' field, which results in the same as passing 0 for
'nseconds' field. Moreover, dict is checked for unknown fields.
Signed-off-by: Luyao Huang
Signed-off-by: Michal Privoznik
---
libvirt-override-virDomain.py | 4 ++--
libvirt-override.c| 39 +++
2 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/libvirt-override-virDomain.py b/libvirt-override-virDomain.py
index a50ec0d..fa5f75f 100644
--- a/libvirt-override-virDomain.py
+++ b/libvirt-override-virDomain.py
@@ -67,8 +67,8 @@
return ret
def setTime(self, time=None, flags=0):
-"""Set guest time to the given value. @time is a dict conatining
-'seconds' field for seconds and 'nseconds' field for nanosecons """
+"""Set guest time to the given value. @time is a dict containing
+'seconds' field for seconds and 'nseconds' field for nanoseconds """
ret = libvirtmod.virDomainSetTime(self._o, time, flags)
if ret == -1: raise libvirtError ('virDomainSetTime() failed',
dom=self)
return ret
diff --git a/libvirt-override.c b/libvirt-override.c
index c01e52f..57f0ccf 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -7785,12 +7785,14 @@ static PyObject *
libvirt_virDomainSetTime(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
PyObject *py_retval = NULL;
PyObject *pyobj_domain;
+PyObject *pyobj_seconds;
+PyObject *pyobj_nseconds;
PyObject *py_dict;
virDomainPtr domain;
long long seconds = 0;
unsigned int nseconds = 0;
unsigned int flags;
-ssize_t py_dict_size;
+ssize_t py_dict_size = 0;
int c_retval;
if (!PyArg_ParseTuple(args, (char*)"OOI:virDomainSetTime",
@@ -7798,26 +7800,31 @@ libvirt_virDomainSetTime(PyObject *self
ATTRIBUTE_UNUSED, PyObject *args) {
return NULL;
domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
-py_dict_size = PyDict_Size(py_dict);
-
-if (py_dict_size == 2) {
-PyObject *pyobj_seconds, *pyobj_nseconds;
-
-if (!(pyobj_seconds = PyDict_GetItemString(py_dict, "seconds")) ||
-(libvirt_longlongUnwrap(pyobj_seconds, &seconds) < 0)) {
-PyErr_Format(PyExc_LookupError, "malformed or missing 'seconds'");
+if (PyDict_Check(py_dict)) {
+py_dict_size = PyDict_Size(py_dict);
+if ((pyobj_seconds = PyDict_GetItemString(py_dict, "seconds"))) {
+if (libvirt_longlongUnwrap(pyobj_seconds, &seconds) < 0) {
+PyErr_Format(PyExc_LookupError, "malformed 'seconds'");
+goto cleanup;
+}
+} else {
+PyErr_Format(PyExc_LookupError, "Dictionary must contains
'seconds'");
goto cleanup;
}
-if (!(pyobj_nseconds = PyDict_GetItemString(py_dict, "nseconds")) ||
-(libvirt_uintUnwrap(pyobj_nseconds, &nseconds) < 0)) {
-PyErr_Format(PyExc_LookupError, "malformed or missing 'nseconds'");
+if ((pyobj_nseconds = PyDict_GetItemString(py_dict, "nseconds"))) {
+if (libvirt_uintUnwrap(pyobj_nseconds, &nseconds) < 0) {
+PyErr_Format(PyExc_LookupError, "malformed 'nseconds'");
+goto cleanup;
+}
+} else if (py_dict_size > 1) {
+PyErr_Format(PyExc_LookupError, "Dictionary contains unknown key");
goto cleanup;
}
-} else if (py_dict_size > 0) {
-PyErr_Format(PyExc_LookupError, "Dictionary must contain "
- "'seconds' and 'nseconds'");
-goto cleanup;
+} else if (py_dict != Py_None || !flags) {
+PyErr_Format(PyExc_TypeError, "time must be a dictionary "
+ "or None with flags set");
+return NULL;
}
LIBVIRT_BEGIN_ALLOW_THREADS;
--
2.0.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] Silently ignore MAC in NetworkLoadConfig
Libvirt's RPMs have been adding it to networks which don't support it.
https://bugzilla.redhat.com/show_bug.cgi?id=1156367
---
src/conf/network_conf.c | 4
1 file changed, 4 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 3bad07d..f36be63 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -3160,6 +3160,10 @@ virNetworkObjPtr
virNetworkLoadConfig(virNetworkObjListPtr nets,
*/
if (virNetworkSetBridgeName(nets, def, 0))
goto error;
+} else {
+/* Throw away MAC address for other forward types,
+ * which could have been generated by older libvirt RPMs */
+def->mac_specified = false;
}
if (!(net = virNetworkAssignDef(nets, def, false)))
--
2.0.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] Generate a MAC when loading a config instead of package update
Partially reverts commit 5754dbd.
The code in the specfile adds a MAC address to every ,
even for for which we don't support
changing MAC addresses.
Remove it completely. For new networks, we have been adding
MAC addresses on definition/creation since the commit mentioned above.
For existing networks (pre-0.9.0), the MAC is added by the previous
commit.
https://bugzilla.redhat.com/show_bug.cgi?id=1156367
---
libvirt.spec.in | 42 --
src/conf/network_conf.c | 4
2 files changed, 4 insertions(+), 42 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 6fcaa3e..43b3899 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1606,48 +1606,6 @@ exit 0
%post daemon
-%if %{with_network}
-# All newly defined networks will have a mac address for the bridge
-# auto-generated, but networks already existing at the time of upgrade
-# will not. We need to go through all the network configs, look for
-# those that don't have a mac address, and add one.
-
-network_files=$( (cd %{_localstatedir}/lib/libvirt/network && \
- grep -L "mac address" *.xml; \
- cd %{_sysconfdir}/libvirt/qemu/networks && \
- grep -L "mac address" *.xml) 2>/dev/null \
-| sort -u)
-
-for file in $network_files
-do
- # each file exists in either the config or state directory (or both) and
- # does not have a mac address specified in either. We add the same mac
- # address to both files (or just one, if the other isn't there)
-
- mac4=`printf '%X' $(($RANDOM % 256))`
- mac5=`printf '%X' $(($RANDOM % 256))`
- mac6=`printf '%X' $(($RANDOM % 256))`
- for dir in %{_localstatedir}/lib/libvirt/network \
- %{_sysconfdir}/libvirt/qemu/networks
- do
- if test -f $dir/$file
- then
- sed -i.orig -e \
- "s|\(|" \
- $dir/$file
- if test $? != 0
- then
- echo "failed to add " \
- "to $dir/$file"
- mv -f $dir/$file.orig $dir/$file
- else
- rm -f $dir/$file.orig
- fi
- fi
- done
-done
-%endif
-
%if %{with_systemd}
%if %{with_systemd_macros}
%systemd_post virtlockd.socket libvirtd.service libvirtd.socket
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index f36be63..4c16bb4 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -3155,6 +3155,10 @@ virNetworkObjPtr
virNetworkLoadConfig(virNetworkObjListPtr nets,
def->forward.type == VIR_NETWORK_FORWARD_NAT ||
def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
+if (!def->mac_specified) {
+virNetworkSetBridgeMacAddr(def);
+virNetworkSaveConfig(configDir, def);
+}
/* Generate a bridge if none is specified, but don't check for
collisions
* if a bridge is hardcoded, so the network is at least defined.
*/
--
2.0.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Simplify MAC address generation on update
https://bugzilla.redhat.com/show_bug.cgi?id=1156367 Ján Tomko (2): Silently ignore MAC in NetworkLoadConfig Generate a MAC when loading a config instead of package update libvirt.spec.in | 42 -- src/conf/network_conf.c | 8 2 files changed, 8 insertions(+), 42 deletions(-) -- 2.0.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] selinux: ignore missing imagelabel in SetSavedStateLabel
If we have no imagelabel to set (e.g. the domain was created by qemu-attach where we don't generate an imagelabel) return success instead of crashing. https://bugzilla.redhat.com/show_bug.cgi?id=1161831 --- src/security/security_selinux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f96be50..db0df7d 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1970,7 +1970,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); -if (!secdef || !secdef->relabel) +if (!secdef || !secdef->relabel || !secdef->imagelabel) return 0; return virSecuritySELinuxSetFilecon(savefile, secdef->imagelabel); -- 2.0.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
