Re: [libvirt] [PATCH 0/2] vnc: remove deprecated TLS related features
Hi, > doc: switch to modern syntax for VNC TLS setup > vnc: remove support for deprecated tls, x509, x509verify options Added to ui queue. thanks, Gerd -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] Add .domainGetHostname() support for QEMU driver.
On Tue, Aug 21, 2018 at 10:34 AM, Julio Faracco wrote: > This serie adds a new function into QEMU Guest Agent handlers to use > the QEMU command 'guest-get-host-name' to retrieve the domain hostname. > This approach requires QEMU-GA running inside the guest, but it is the > fastest and easiest way to get this info. > > Julio Faracco (2): > qemu: implementing qemuAgentGetHostname() function. > qemu: adding domainGetHostname support for QEMU. > How about updating improvements part of news.xml to inform users that they could use virDomainGetHostname to qemu VMs :)? > > src/qemu/qemu_agent.c | 42 ++ > src/qemu/qemu_agent.h | 4 > src/qemu/qemu_driver.c | 40 > 3 files changed, 86 insertions(+) > > -- > 2.17.1 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- Best regards, --- Han Han Quality Engineer Redhat. Email: h...@redhat.com Phone: +861065339333 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes
在 2018/8/20 下午6:35, Andrea Bolognani 写道:
On Mon, 2018-08-20 at 16:19 +0800, Yi Min Zhao wrote:
在 2018/8/16 下午10:38, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
+struct _virZPCIDeviceAddress {
+unsigned int zpci_fid;
+unsigned int zpci_uid;
+bool fid_assigned;
+bool uid_assigned;
+};
A couple of questions about the approach here, one of which I have
mentioned already and one of which I probably haven't (my bad):
* do you really need to have separate booleans tracking whether
or not either id has been assigned? Wouldn't the same approach
as virPCIDeviceAddressIsEmpty() work, eg. consider the address
absent if both are zero and present otherwise?
It's OK for uid. But for fid, zero is a valid value, so we need a bool
to track its assignment.
See virPCIDeviceAddressIsEmpty() and virPCIDeviceAddressIsValid(),
which have very similar requirement but don't use extra booleans
to keep track of state.
You could do the same thing those functions do:
* the zPCI address is empty if both uid and fid are zero;
uid=0 and fid=0 can't mean zPCI address is empty, because the user might
only define fid with 0. If fid=0 has been assigned, we should report
error. If
it is not defined by user, fid is also 0, then we should allocate a
valid value
for fid.
* the zPCI address is invalid if it's empty or uid is too large.
You already have the latter covered, so it's only a matter of
implementing the former.
If we add a boolean for fid, why not also add another one for uid to
keep consistency?
This also makes code easy to read and obvious. It doesn't waste much
memory space.
I think it actually makes more complicated, which is why I'd rather
get rid of it :)
* especially if you don't need the additional booleans, would it
be preferable to just add the two ids to the existing struct
instead of declaring a new one that you'll have to allocate
and make sure it's not NULL before accessing it? Again, I seem
to remember Laine feeling somewhat strongly about the topic.
For other platforms, is it OK to waste this unused memory (of course,
it's little) ?
I believe adding a couple of unsigned ints is worth it if we can
get rid of all the checks on addr->zpci because of it.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Add nodesevinfo in virsh
Add virsh nodesevinfo to get AMD SEV features via virNodeGetSEVInfo. Han Han (2): virsh: Implement virNodeGetSEVInfo in virsh news: Add nodesevinfo command in virsh docs/news.xml | 9 +++ tools/virsh-host.c | 66 ++ tools/virsh.pod| 5 3 files changed, 80 insertions(+) -- 2.18.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] news: Add nodesevinfo command in virsh
Signed-off-by: Han Han --- docs/news.xml | 9 + 1 file changed, 9 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index c6d03f5556..fc9db92d05 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -63,6 +63,15 @@ Support the vhost-vsock-ccw device on S390. + + + virsh: Add subcommand nodesevinfo + + + Implement virNodeGetSEVInfo in virsh nodesevinfo to get AMD SEV + features of host. + + -- 2.18.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] virsh: Implement virNodeGetSEVInfo in virsh
Add sub-command nodesevinfo to get node infomation of AMD SEV feature.
Signed-off-by: Han Han
---
tools/virsh-host.c | 66 ++
tools/virsh.pod| 5
2 files changed, 71 insertions(+)
diff --git a/tools/virsh-host.c b/tools/virsh-host.c
index 16f504bafe..0bcd71a2b8 100644
--- a/tools/virsh-host.c
+++ b/tools/virsh-host.c
@@ -952,6 +952,67 @@ cmdNodeMemStats(vshControl *ctl, const vshCmd *cmd)
return ret;
}
+/*
+ * "nodesevinfo" command
+ */
+static const vshCmdInfo info_nodesevinfo[] = {
+{.name = "help",
+ .data = N_("AMD SEV feature information.")
+},
+{.name = "desc",
+ .data = N_("Returns information of SEV feature about the node.")
+},
+{.name = NULL}
+};
+
+static bool
+cmdNodesevinfo(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
+{
+virTypedParameterPtr params = NULL;
+int nparams = 0;
+unsigned int flags = 0;
+bool ret = false;
+size_t i;
+virshControlPtr priv = ctl->privData;
+
+if (nparams == 0) {
+/* Get the number of SEV info parameters */
+if (virNodeGetSEVInfo(priv->conn, NULL, , flags) != 0) {
+vshError(ctl, "%s",
+ _("Unable to get number of SEV info parameters"));
+goto cleanup;
+}
+}
+
+if (nparams == 0) {
+ret = true;
+goto cleanup;
+}
+
+/* Now get all the SEV info parameters */
+params = vshCalloc(ctl, nparams, sizeof(params));
+if (virNodeGetSEVInfo(priv->conn, , , flags) != 0) {
+vshError(ctl, "%s", _("Unable to get SEV info parameters"));
+goto cleanup;
+}
+
+/* XXX: Need to sort the returned params once new parameter
+ * fields not of shared memory are added.
+ */
+vshPrint(ctl, _("SEV info:\n"));
+for (i = 0; i < nparams; i++) {
+char *str = vshGetTypedParamValue(ctl, [i]);
+vshPrint(ctl, "\t%-15s %s\n", params[i].field, str);
+VIR_FREE(str);
+}
+
+ret = true;
+
+ cleanup:
+virTypedParamsFree(params, nparams);
+return ret;
+}
+
/*
* "nodesuspend" command
*/
@@ -1900,6 +1961,11 @@ const vshCmdDef hostAndHypervisorCmds[] = {
.info = info_nodememstats,
.flags = 0
},
+{.name = "nodesevinfo",
+ .handler = cmdNodesevinfo,
+ .info = info_nodesevinfo,
+ .flags = 0
+},
{.name = "nodesuspend",
.handler = cmdNodeSuspend,
.opts = opts_node_suspend,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 4e118851f8..ea513c0acc 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -317,6 +317,11 @@ of cpu statistics during 1 second.
Returns memory stats of the node.
If I is specified, this will print the specified cell statistics only.
+=item B
+
+Display AMD's SEV feature of this host, including PDH, cert-chain, cbitpos
+and reduced-phys-bits.
+
=item B [I] [I]
Puts the node (host machine) into a system-wide sleep state and schedule
--
2.18.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 12/12] news: Update news for PCI address extension attributes
在 2018/8/20 下午6:17, Andrea Bolognani 写道: On Mon, 2018-08-20 at 11:39 +0800, Yi Min Zhao wrote: 在 2018/8/18 上午12:21, Andrea Bolognani 写道: On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...] + + + qemu: Added support for PCI device passthrough on S390 + + + The new zPCI attributes uid (user-defined identifier) + and fid (PCI function identifier) of the S390 platform + extend the PCI device address to support the PCI + passthrough on S390. + + This doesn't look entirely accurate: the attributes are not only used for assigned devices but for emulated devices as well, so the text should reflect that. How about: *** to support the PCI functionality on S390. Perhaps something like qemu: Added support for PCI devices on s390 PCI addresses can now include the new uid (user-defined identifier) and fid (PCI function identifier) attributes, which make the corresponding devices usable by s390 guests. Better, I will do a little update based on your words. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/12] qemu: Add hotpluging support for PCI devices on S390 guests
在 2018/8/20 下午7:34, Andrea Bolognani 写道: So, I'm very much not familiar with the hotplug code and seeing changes to stuff like qemuDomainAttachDiskGeneric() makes me a bit uncomfortable:) I can't spot anything obviously wrong in your changes, but I think perhaps you might want to enter and exit the monitor separately for the zpci device and for the virtio device? I'm not sure that's useful at all, but network devices for example seems to follow that pattern... It would be great if someone with more experience in this area could provide a review. We have to add zpci device firstly and add corresponding pci device secondly. Do you think it's redundant to call monitor twice to add two devices? Again, I'm not familiar at all with this code, but entering (and exiting) the monitor once for each device you're dealing with seems to be a pattern. So let's see other reviewers' comment. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 06/12] conf: Introduce address caching for PCI extensions
在 2018/8/20 下午6:48, Andrea Bolognani 写道: On Mon, 2018-08-20 at 16:32 +0800, Yi Min Zhao wrote: 在 2018/8/16 下午11:03, Andrea Bolognani 写道: I haven't looked into the hash table handling in detail but I wonder if it's really necessary? IIUC you're using it just to mark which uids and fids have been already used by a device, which the PCI address allocation code does by setting bits inside integer variables - having a custom hash table for the same seems like overkill, and from the maintenance point of view it would be great to have the logic for PCI address and zPCI address allocation be similar unless diverging is strictly necessary. PCI address set uses array to store pci addresses' assignment. It doesn't need too much memory because the buses are allocated dynamically, one bus only has 32 slot, and it's a tree topology. But in zpci case, fid and uid are flat. FID is 32-bit so that we need a 4294967295 sized array. Don't you think it's too large? Welp, I guess you're right. Disregard this comment then. Thanks for your comment anyway! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/2] qemu: implementing qemuAgentGetHostname() function.
This commit implements the function qemuAgentGetHostname() that uses the
QEMU command 'guest-get-host-name' to retrieve the guest hostname of
Virtual Machine. It is a possibility where QEMU-GA is running.
Signed-off-by: Julio Faracco
---
src/qemu/qemu_agent.c | 42 ++
src/qemu/qemu_agent.h | 4
2 files changed, 46 insertions(+)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index bf08871f18..2d006b8070 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1682,6 +1682,48 @@ qemuAgentUpdateCPUInfo(unsigned int nvcpus,
return 0;
}
+int
+qemuAgentGetHostname(qemuAgentPtr mon,
+ char **hostname)
+{
+int ret = -1;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+virJSONValuePtr data = NULL;
+
+cmd = qemuAgentMakeCommand("guest-get-host-name",
+ NULL);
+
+if (!cmd)
+return ret;
+
+if (qemuAgentCommand(mon, cmd, , true,
+ VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+goto cleanup;
+
+if (qemuAgentCheckError(cmd, reply) < 0)
+goto cleanup;
+
+if (!(data = virJSONValueObjectGet(reply, "return"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("malformed return value"));
+goto cleanup;
+}
+
+if (VIR_STRDUP(*hostname,
+ virJSONValueObjectGetString(data, "host-name")) <= 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("'host-name' missing in reply of
guest-get-host-name"));
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
int
qemuAgentGetTime(qemuAgentPtr mon,
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 6dd9c702dd..4354b7e0cf 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -105,6 +105,10 @@ int qemuAgentUpdateCPUInfo(unsigned int nvcpus,
qemuAgentCPUInfoPtr cpuinfo,
int ncpuinfo);
+int
+qemuAgentGetHostname(qemuAgentPtr mon,
+ char **hostname);
+
int qemuAgentGetTime(qemuAgentPtr mon,
long long *seconds,
unsigned int *nseconds);
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/2] Add .domainGetHostname() support for QEMU driver.
This serie adds a new function into QEMU Guest Agent handlers to use the QEMU command 'guest-get-host-name' to retrieve the domain hostname. This approach requires QEMU-GA running inside the guest, but it is the fastest and easiest way to get this info. Julio Faracco (2): qemu: implementing qemuAgentGetHostname() function. qemu: adding domainGetHostname support for QEMU. src/qemu/qemu_agent.c | 42 ++ src/qemu/qemu_agent.h | 4 src/qemu/qemu_driver.c | 40 3 files changed, 86 insertions(+) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/2] qemu: adding domainGetHostname support for QEMU.
This commit add the support to use the function qemuAgentGetHostname() for
obtain the domain hostname using QEMU-GA command.
Signed-off-by: Julio Faracco
---
src/qemu/qemu_driver.c | 40
1 file changed, 40 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d4a2379e48..7c4d742444 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19309,6 +19309,45 @@ qemuConnectGetCPUModelNames(virConnectPtr conn,
return virCPUGetModels(arch, models);
}
+static char *
+qemuDomainGetHostname(virDomainPtr dom,
+ unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm = NULL;
+qemuAgentPtr agent;
+char *hostname = NULL;
+
+virCheckFlags(0, NULL);
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+return NULL;
+
+if (virDomainGetHostnameEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
+goto cleanup;
+
+if (virDomainObjCheckActive(vm) < 0)
+goto endjob;
+
+if (!qemuDomainAgentAvailable(vm, true))
+goto endjob;
+
+agent = qemuDomainObjEnterAgent(vm);
+ignore_value(qemuAgentGetHostname(agent, ));
+qemuDomainObjExitAgent(vm, agent);
+
+ endjob:
+qemuDomainObjEndAgentJob(vm);
+
+ cleanup:
+virDomainObjEndAPI();
+return hostname;
+}
+
+
static int
qemuDomainGetTime(virDomainPtr dom,
long long *seconds,
@@ -21787,6 +21826,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
.connectGetCPUModelNames = qemuConnectGetCPUModelNames, /* 1.1.3 */
.domainFSFreeze = qemuDomainFSFreeze, /* 1.2.5 */
.domainFSThaw = qemuDomainFSThaw, /* 1.2.5 */
+.domainGetHostname = qemuDomainGetHostname, /* 4.6.0 */
.domainGetTime = qemuDomainGetTime, /* 1.2.5 */
.domainSetTime = qemuDomainSetTime, /* 1.2.5 */
.nodeGetFreePages = qemuNodeGetFreePages, /* 1.2.6 */
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC v3] x86 RDT Cache Monitoring Technology (CMT)
For the supporting of Intel x86 Cache Monitoring Technology (CMT) in libvirt, we have already discussed a couple of times, and I have raised two RFCs, you can find the discussion from the following links, and thanks to the people who participated in the discussion online and offline. RFCv2 links https://www.redhat.com/archives/libvir-list/2018-July/msg00409.html https://www.redhat.com/archives/libvir-list/2018-July/msg01241.html RFCv1 https://www.redhat.com/archives/libvir-list/2018-June/msg00674.html Nearly one month has passed since last online discussion, in this month, MBA (memory bandwidth allocation) feature has been introduced by Niu Bin, and some fundamental code for CMT feature has been changed. This change is not that significant, and I wrote my POC code for this RFC again upon the new virresctrl framework, I also made some changes for the CMT feature. I think it is better to summarize my thought about the enabling of CMT here before I submit my code. I'll try to keep consistent with the previous discussion. 1. What is the purpose of this RFC and later patches? Short answer: Introducing Kernel resctrlfs based x86 CMT feature to libvirt. Detail explanation: Latest kernel has removed the "cmt, mbmt, mbml" perf event, and libvirt relies on these kernel perf events to report the result of CMT/MBM through virperf interface. Libvirt will no longer support CMT/MBM feature for Linux distribution with the latest kernel, while there CPU features are vital to some software such as Openstack Nova. About the deprecation of cmt/mbm perf events, refer to the following link for details: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git /commit/?id=c39a0e2c8850f08249383f2425dbd8dbe4baad69 Latest kernel introduces the RDT feature, including CAT MBA CDP CMT and MBM, through resource control file system (resctrl fs), and libvirt already implemented the cache and memory bandwidth allocation through kernel resctrlfs interface. I'd like to add the cache and memory bandwidth monitoring feature to libvirt through kernel resctrlfs interface. Since CMT and MBM shares a very similar kernel interface, for simplicity, only discuss CMT here. MBM is the followup that will be implemented after CMT, which should be very straightforward once CMT is ready. It is planned to output the cache monitoring result through 'domstats' command, and create or delete monitoring groups with either domain's statical XML configuration file or a new 'virsh' runtime command. 2. How many stages scheduled to implement the libvirt CMT(MBM) feature? Short answer: 4 stages. Detail explanation: stage 1: Statical CMT feature. Implementing the libvirt interface for creating and deleting CMT groups for statically through domain's XML configuration file, changes only takes effect after a reboot. Stage 2: Statical MBM feature. Very similar to stage1 CMT. Stage 3: Dynamical CMT feature. In this stage, it is aimed to implement interfaces to change the CMT groups dynamically at runtime, with no requirement for a reboot. My basic thought is implementing a new 'virsh' sub-command providing all interfaces. I also hope to implement the functionality of dynamical cache allocation (CAT) with this command. Stage 4: Dynamical MBM feature. Depending on the implementation of stage3, should share many interfaces created at stage 3. This RFC mainly covers the discussion of stage1. 3. What is the interface for statical CMT feature (the output and input)? Short answer: output through command 'domstats'; input through XML file. Detail explanation: Output: Either CMT or MBM is Intel x86 CPU feature, so I put the statistical message under the result of 'cpu-total' subcommand. This is different with RFCv2, in RFCv2 a separate 'domstats' sub-command has been created, with the name 'cpu-resource', Martin and I were not very satisfied with the the naming. The output would be: virsh domstats --cpu--total Domain: 'ubuntu16.04-base' cpu.time=3143918715755 cpu.user=1532000 cpu.system=23528000 cpu.cache.monitor.count=3 cpu.cache.0.name=vcpus_2 cpu.cache.0.vcpus=2 cpu.cache.0.bank.count=2 cpu.cache.0.bank.0.bytes=5488 cpu.cache.0.bank.1.bytes =441 cpu.cache.1.name=vcpus_1 cpu.cache.1.vcpus=1 cpu.cache.1.bank.count=2 cpu.cache.1.bank.0.bytes =7839744 cpu.cache.1.bank.1.bytes =0 cpu.cache.2.name=vcpu_0,3 cpu.cache.2.vcpus=0,3 cpu.cache.2.bank.count=2 cpu.cache.2.bank.0.bytes=53796864 cpu.cache.2.bank.1.bytes=0 Comparing with RFCv2, the CMT information is slightly changed. Input: Comparing with RFC v2, this part has been changed. Now the resource monitoring group is considered as a resource monitor toward specific allocation. The main interface for
Re: [libvirt] Matching the type of mediated devices in the migration
On Sun, 19 Aug 2018 22:25:19 +0800 Zhi Wang wrote: > Share some updates of my work on this topic recently: > > Thanks for Erik's guide and advices. Now my PoC patches almost works. > Will send the RFC soon. > > Mostly the ideas are based on Alex's idea: a match between a device > state version and a minimum required version > > > "Match of versions" in Libvirt > > Initialization stage: > > - Libvirt would detect if there is any device state version in a > "mdev_type" of a mediated device when creating a mdev node in node > device tree. > - If the "mdev_type" of a mediated device *has* a device state version, > then this mediated device supports migration. > - If not, (compatibility case, mostly for old vendor drivers which > don't support migration), this mediated device doesn't support migration > > Migration stage: > > - Libvirt would put the mdev information inside cookies and send them > between src machine and dst machine. So a new type of cookie would be > added here. > > There are different versions of migration protocols in libvirt. Each of > them starts to send cookies in different sequence. The idea here is to > let the match happens as early as possible. Looks like QEMU driver in > libvirt only support V2/V3 proto. > > > V2 proto: > > - The match would happen in SRC machine after the DST machine transfers > the cookies with mdev information back to the SRC machine during the > "preparation" stage. The disadvantage is the DST virtual machine has > already been created in "preparation" stage. If the match fails, the > virtual machine in DST machine has to be killed as well, which would > waste some time. > > V3 proto: > > - The match would happen in DST machine after the SRC machine transfers > the cookies to the DST machine during the "begin" stage. As the DST > machine hasn't entered into "preparation" stage at this time, the > virtual machine hasn't been created in DST machine at this point. No > extra VM destroy is needed if the match fails. This would be the ideal > place for a match. > > "Match of version" in QEMU level > > As there are several different types of migration in libvirt. In a > migration with hypervisor native transport, the target machine could > even not have libvirtd, the migration happens between device models > directly. So we need a match in QEMU level as well. We might still need > Kirti's approach as the last level match. The kernel and vendor driver will always have a last opportunity to nak a migration, the purpose of making certain information readily available to libvirt is only to allow userspace some insight into where a migration is likely to be successful. Even if we expose these things to userspace, it's the kernel's responsibility to validate the migration data. In fact, pushing state information for a device into the kernel would seem to be a massive security target. For instance how many vulnerabilities might a malicious user be able to exploit in the code that parses the device specific state information? How do we even detect non-malicious user errors, like trying to migrate GVTg device state to an NVIDIA vGPU? The latter at least suggests that the kernel needs to perform the same set of validation that we're trying to enable userspace to do. Cornelia also mentioned that some mdev devices are more or less shells within which a device is configured, such as ccw and likely the crypto ap devices. In those cases the mdev type might not be sufficient meta data about what we're dealing with. This might suggest some sort of header within the migration region parsed by common code for basic validation. Are there any suggestions how we can deal with security issues? Allowing userspace to provide a data stream representing the internal state of a virtual device model living within the kernel seems troublesome. If we need to trust the data stream, do we need to somehow make the operation more privileged than what a vfio user might have otherwise? Does the data stream need to be somehow signed and how might we do that? How can we build in protection against an untrusted restore image? Thanks, Alex -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 5/7] lock_driver_sanlock: Handle metadata flag gracefully
On 08/20/2018 05:04 PM, Daniel P. Berrangé wrote:
> On Mon, Aug 20, 2018 at 03:59:23PM +0100, Daniel P. Berrangé wrote:
>> On Fri, Aug 17, 2018 at 10:49:12AM -0400, John Ferlan wrote:
>>>
>>>
>>> On 08/14/2018 07:19 AM, Michal Privoznik wrote:
No real support implemented here. But hey, at least we will not
fail.
Signed-off-by: Michal Privoznik
---
src/locking/lock_driver_sanlock.c | 25 ++---
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/src/locking/lock_driver_sanlock.c
b/src/locking/lock_driver_sanlock.c
index 3e5f0e37b0..c1996fb937 100644
--- a/src/locking/lock_driver_sanlock.c
+++ b/src/locking/lock_driver_sanlock.c
@@ -791,7 +791,8 @@ static int
virLockManagerSanlockAddResource(virLockManagerPtr lock,
virLockManagerSanlockPrivatePtr priv = lock->privateData;
virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY |
- VIR_LOCK_MANAGER_RESOURCE_SHARED, -1);
+ VIR_LOCK_MANAGER_RESOURCE_SHARED |
+ VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);
if (priv->res_count == SANLK_MAX_RESOURCES) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -804,6 +805,11 @@ static int
virLockManagerSanlockAddResource(virLockManagerPtr lock,
if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
return 0;
+/* No metadata locking support for now.
+ * TODO: implement it. */
+if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA)
+return 0;
+
>>>
>>> Doesn't this give someone the false impression that their resource is
>>> locked if they choose METADATA?
>>>
>>> Something doesn't feel right about that - giving the impression that
>>> it's supported and the consumer is protected, but when push comes to
>>> shove they aren't.
>>>
>>> I'd be inclined to believe that we may want to do nothing with/for
>>> sanlock allowing the virCheckFlags above take care of the consumer.
>>
>> Yeah, this doesn't feel right to me. I think we need to treat the
>> metadata locking as completely independant of the content locking.
>> This implies we should have a separate configuration for metadata
>> locking, where the only valid options are "lockd" or "nop".
>>
>> eg our available matrix looks like
>>
>> METADATA
>> | nop lockd sanlock
>> -+
>> nop | Y Y N
>> CONTENTlockd | Y Y N
>> sanlock | Y Y N
Having some troubles parsing the table. Do you mean:
| Content | Metadata
-+---
nop |Y|Y
lockd |Y|Y
sanlock |Y|N
Where Y says the respective type (content/metadata) can/cannot be locked
by lock driver in question? I.e. we would have 'metadata_lock_manager'
config value in qemu.conf and it'd accept only 'nop' and 'lockd'.
Then we can have 'metadata_lockspace_dir' in
/etc/libvirt/qemu-lockd.conf where the lockspace would be created.
Anyway, I'm gonna state the obvious because it might not be that obvious
to somebody else: we can change the RPC between libvirtd and virtlockd
safely, because we require both to be the same version. I'm saying that
just in case I need to alter the RPC a bit (found some unused procs
there, btw but they might come handy someday).
>
> Oh and even for virtlockd we need to consider the config separately
> for content vs metdata.
Do you mean like completely new config file? qemu-lockd-metadata.conf?
Okay. So far we only need one config option (metadata_lockspace_dir) but
it might turn out we need more and it would make sens to keep options
separated from content locking options.
>
> For content we have the possibility to lock out of band files as a
> proxy for the main file. This is primarily done for protecting
> shared blocks devices as locking the device node doesn't apply
> across hosts.
>
> For metadata locking, we only ever care about the local device node
> as changes to labelling/ownership don't propagate across hosts. IOW,
> we should always directly lock the file in question for metadata
> locking, and never use an out of band proxy for locking.
Oh right. From this POV locking from secdriver seems like even better
idea.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 7/7] qemu_security: Lock metadata while relabelling
On 08/20/2018 05:16 PM, Daniel P. Berrangé wrote:
> On Tue, Aug 14, 2018 at 01:19:43PM +0200, Michal Privoznik wrote:
>> Fortunately, we have qemu wrappers so it's sufficient to put
>> lock/unlock call only there.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/qemu/qemu_security.c | 107
>> +++
>> 1 file changed, 107 insertions(+)
>>
>> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
>> index af3be42854..527563947c 100644
>> --- a/src/qemu/qemu_security.c
>> +++ b/src/qemu/qemu_security.c
>> @@ -26,6 +26,7 @@
>> #include "qemu_domain.h"
>> #include "qemu_security.h"
>> #include "virlog.h"
>> +#include "locking/domain_lock.h"
>>
>> #define VIR_FROM_THIS VIR_FROM_QEMU
>>
>> @@ -39,6 +40,12 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>> {
>> int ret = -1;
>> qemuDomainObjPrivatePtr priv = vm->privateData;
>> +bool locked = false;
>> +
>> +if (virDomainLockMetadataLock(driver->lockManager, vm) < 0)
>> +goto cleanup;
>> +
>> +locked = true;
>>
>> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> virSecurityManagerTransactionStart(driver->securityManager) < 0)
>> @@ -55,9 +62,17 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>> vm->pid) < 0)
>> goto cleanup;
>>
>> +locked = false;
>> +
>> +if (virDomainLockMetadataUnlock(driver->lockManager, vm) < 0)
>> +goto cleanup;
>> +
>> ret = 0;
>> cleanup:
>> virSecurityManagerTransactionAbort(driver->securityManager);
>> +if (locked &&
>> +virDomainLockMetadataUnlock(driver->lockManager, vm) < 0)
>> +VIR_WARN("unable to release metadata lock");
>> return ret;
>> }
>
> I'm wondering if this is really the right level to plug in the metadata
> locking ? What about if we just pass a virLockManagerPtr to the security
> drivers and let them lock each resource at the time they need to modify
> its metadata. This will trivially guarantee that we always lock the exact
> set of files that we'll be changing metadata on.
>
> eg in SELinux driver the virSecuritySELinuxSetFileconHelper method
> would directly call virLockManagerAcquire & virLockManagerRelease,
> avoiding the entire virDomainLock abstraction which was really
> focused around managing the content locks around lifecycle state
> changes.
>
Yeah, I vaguely recall writing code like this (when I was trying to
solve this some time ago). Okay, let me see if that's feasible.
But boy, this is getting hairy.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/7] domain_lock: Implement metadata locking
On 08/20/2018 05:07 PM, Daniel P. Berrangé wrote: > On Tue, Aug 14, 2018 at 01:19:42PM +0200, Michal Privoznik wrote: >> In order for our drivers to lock resources for metadata change we >> need set of new APIs. Fortunately, we don't have to care about >> every possible device a domain can have. We care only about those >> which can live on a network filesystem and hence can be accessed >> by multiple daemons at the same time. These devices are covered >> in virDomainLockMetadataLock() and only a small fraction of >> those can be hotplugged (covered in the rest of the introduced >> APIs). > > I'm not sure I understand the rationale behind saying we only care > about resources on network filesystems. > > If I have 2 locally running guests, and both have a serial port > backed by a physical serial port, eg > > > > > > > we *do* care about locking /dev/ttyS0, as libvirtd isn't doing > mutual exclusion checks anywhere else for the /dev/ttyS0 device > node. Ah you mean that the system wide daemon and session daemon could clash when relabeling /dev/ttyS0? Well, we don't do relabeling for session daemons and running two system daemons is not supported (you're gonna hit more serious problems when trying that anyway). > > In general I think we need to lock every single file resource > that is labelled for a guest, regardless of whether its local > or remote. Well this might be feasible iff locking would be done from security driver. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: Parse guestfwd channel device info again
https://bugzilla.redhat.com/show_bug.cgi?id=1610072
Due to historical reasons we were not parsing device info on
guestfwd channel. Sure, it doesn't make much sense to parse
but it surely makes sense to parse its alias (which
might be an user alias).
This reverts commit 47a3dd46ead20e6fdc30bcdc1b8e707e250d33da
which fixed https://bugzilla.redhat.com/show_bug.cgi?id=1172526.
Signed-off-by: Michal Privoznik
---
src/conf/domain_conf.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3c254801cd..123abec1ba 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12807,14 +12807,8 @@ virDomainChrDefParseXML(virDomainXMLOptionPtr xmlopt,
}
}
-if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
-def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) {
-VIR_DEBUG("Ignoring device address for gustfwd channel");
-} else if (virDomainDeviceInfoParseXML(xmlopt, node,
- >info, flags) < 0) {
+if (virDomainDeviceInfoParseXML(xmlopt, node, >info, flags) < 0)
goto error;
-}
-
if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
def->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB &&
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 5/5] jobs: Don't remove non-existing archives
On Mon, 2018-08-20 at 17:00 +0200, Erik Skultety wrote:
> On Fri, Aug 10, 2018 at 03:37:45PM +0200, Andrea Bolognani wrote:
> > None of our jobs create the archives as a side-effect, so
> > trying to remove them before generating them is pointless.
>
> Who cleans them after the jobs below? Is there a chance that you could trigger
> these jobs in a sequence that would expect the archives to be removed but
> wouldn't? If there is a risk that this could happen, then I think this patch
> should be dropped.
Cleanup is performed every time new commits are fetched from git,
so stale data shouldn't be a problem in practice; that said, I
agree that there's a chance we might end up using an old archive
if jobs are manually triggered, and keeping the extra rm there is
not really hurting anyone, so let's drop this commit :)
By the way, I just noticed that...
> >{strip_buildrequires}
> > - rm -f dist/*.tar.{{ archive_format }}
> >$PYTHON ./setup.py sdist
> >rpmbuild --clean --define "_topdir `pwd`/rpmbuild" -ta
> > dist/*.tar.{{ archive_format }}
... I mistakenly used the Ansible-style variable substitution
instead of the Jenkins Job Builder-style one, so I fixed it up
before pushing the first four patches.
Thank you for the review! :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 4/5] jobs: Minimize strip_buildrequires
On Mon, 2018-08-20 at 16:11 +0200, Erik Skultety wrote: > On Fri, Aug 10, 2018 at 03:37:44PM +0200, Andrea Bolognani wrote: > >sed -i -e 's/BuildRequires: *perl(Sys::Virt).*//' *.spec* > > - sed -i -e 's/BuildRequires: *perl-Sys-Virt.*//' *.spec* > > I assume because ^these 2 are somehow identical? Well, either one can be used to express the same dependency but strictly speaking they're not identical... However, we only use the first one in practice so the second one is unnecessary at the moment - we can add it back the first time we get actual users. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/7] domain_lock: Implement metadata locking
On Mon, Aug 20, 2018 at 04:07:28PM +0100, Daniel P. Berrangé wrote: > On Tue, Aug 14, 2018 at 01:19:42PM +0200, Michal Privoznik wrote: > > In order for our drivers to lock resources for metadata change we > > need set of new APIs. Fortunately, we don't have to care about > > every possible device a domain can have. We care only about those > > which can live on a network filesystem and hence can be accessed > > by multiple daemons at the same time. These devices are covered > > in virDomainLockMetadataLock() and only a small fraction of > > those can be hotplugged (covered in the rest of the introduced > > APIs). > > I'm not sure I understand the rationale behind saying we only care > about resources on network filesystems. > > If I have 2 locally running guests, and both have a serial port > backed by a physical serial port, eg > > > > > > > we *do* care about locking /dev/ttyS0, as libvirtd isn't doing > mutual exclusion checks anywhere else for the /dev/ttyS0 device > node. > > In general I think we need to lock every single file resource > that is labelled for a guest, regardless of whether its local > or remote. In the next patch I propose integration into the security manager that would avoid the need to touch this domain lock abstraction at all. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 7/7] qemu_security: Lock metadata while relabelling
On Tue, Aug 14, 2018 at 01:19:43PM +0200, Michal Privoznik wrote:
> Fortunately, we have qemu wrappers so it's sufficient to put
> lock/unlock call only there.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_security.c | 107
> +++
> 1 file changed, 107 insertions(+)
>
> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
> index af3be42854..527563947c 100644
> --- a/src/qemu/qemu_security.c
> +++ b/src/qemu/qemu_security.c
> @@ -26,6 +26,7 @@
> #include "qemu_domain.h"
> #include "qemu_security.h"
> #include "virlog.h"
> +#include "locking/domain_lock.h"
>
> #define VIR_FROM_THIS VIR_FROM_QEMU
>
> @@ -39,6 +40,12 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
> {
> int ret = -1;
> qemuDomainObjPrivatePtr priv = vm->privateData;
> +bool locked = false;
> +
> +if (virDomainLockMetadataLock(driver->lockManager, vm) < 0)
> +goto cleanup;
> +
> +locked = true;
>
> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> virSecurityManagerTransactionStart(driver->securityManager) < 0)
> @@ -55,9 +62,17 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
> vm->pid) < 0)
> goto cleanup;
>
> +locked = false;
> +
> +if (virDomainLockMetadataUnlock(driver->lockManager, vm) < 0)
> +goto cleanup;
> +
> ret = 0;
> cleanup:
> virSecurityManagerTransactionAbort(driver->securityManager);
> +if (locked &&
> +virDomainLockMetadataUnlock(driver->lockManager, vm) < 0)
> +VIR_WARN("unable to release metadata lock");
> return ret;
> }
I'm wondering if this is really the right level to plug in the metadata
locking ? What about if we just pass a virLockManagerPtr to the security
drivers and let them lock each resource at the time they need to modify
its metadata. This will trivially guarantee that we always lock the exact
set of files that we'll be changing metadata on.
eg in SELinux driver the virSecuritySELinuxSetFileconHelper method
would directly call virLockManagerAcquire & virLockManagerRelease,
avoiding the entire virDomainLock abstraction which was really
focused around managing the content locks around lifecycle state
changes.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 1/5] jobs: Call rpmbuild explicitly for Python projects
On Mon, Aug 20, 2018 at 04:37:13PM +0200, Andrea Bolognani wrote:
> On Mon, 2018-08-20 at 16:01 +0200, Erik Skultety wrote:
> > On Fri, Aug 10, 2018 at 03:37:41PM +0200, Andrea Bolognani wrote:
> > > - $PYTHON ./setup.py rpm
> > > + rm -f dist/*.tar.{{ archive_format }}
> > > + $PYTHON ./setup.py sdist
> > > + rpmbuild --clean --define "_topdir `pwd`/rpmbuild" -ta
> > > dist/*.tar.{{ archive_format }}
> >
> > So what if you used a standard bdist_rpm command from distutils core, I
> > believe
> > $PYTHON ./setup.py bdist_rpm --bdist-base would be equal to your
> > _topdir.
> > Although, that's just what I've digested from distutils docs, so even though
> > bdist_rpm has a plethora of options you can specify there can always be one
> > we'll be missing :P
>
> I haven't been able to find any bdist_rpm documentation that is not
> filed under Python 2, which leads me to believe it might not be as
> supported (if at all) under Python 3; moreover, the current
> documentation[1] seems to point to FPM as the preferred way to
> generate RPM packages, but that process doesn't looks like it
> involves spec files at all and bundle a whole lot of other stuff
> along with your actual software, so I'd say it's not really suitable
> for our purpose.
>
> In any case, I would still prefer the two-step approach (dist plus
> rpmbuild) to building RPMs because it is consistent with what we do
> for all other build systems (autotools and Perl's Module::Build).
>
>
> [1] https://packaging.python.org/overview/#operating-system-packages
Fair enough,
Reviewed-by: Erik Skultety
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/7] domain_lock: Implement metadata locking
On Tue, Aug 14, 2018 at 01:19:42PM +0200, Michal Privoznik wrote: > In order for our drivers to lock resources for metadata change we > need set of new APIs. Fortunately, we don't have to care about > every possible device a domain can have. We care only about those > which can live on a network filesystem and hence can be accessed > by multiple daemons at the same time. These devices are covered > in virDomainLockMetadataLock() and only a small fraction of > those can be hotplugged (covered in the rest of the introduced > APIs). I'm not sure I understand the rationale behind saying we only care about resources on network filesystems. If I have 2 locally running guests, and both have a serial port backed by a physical serial port, eg we *do* care about locking /dev/ttyS0, as libvirtd isn't doing mutual exclusion checks anywhere else for the /dev/ttyS0 device node. In general I think we need to lock every single file resource that is labelled for a guest, regardless of whether its local or remote. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 5/7] lock_driver_sanlock: Handle metadata flag gracefully
On Mon, Aug 20, 2018 at 03:59:23PM +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 17, 2018 at 10:49:12AM -0400, John Ferlan wrote:
> >
> >
> > On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> > > No real support implemented here. But hey, at least we will not
> > > fail.
> > >
> > > Signed-off-by: Michal Privoznik
> > > ---
> > > src/locking/lock_driver_sanlock.c | 25 ++---
> > > 1 file changed, 18 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/locking/lock_driver_sanlock.c
> > > b/src/locking/lock_driver_sanlock.c
> > > index 3e5f0e37b0..c1996fb937 100644
> > > --- a/src/locking/lock_driver_sanlock.c
> > > +++ b/src/locking/lock_driver_sanlock.c
> > > @@ -791,7 +791,8 @@ static int
> > > virLockManagerSanlockAddResource(virLockManagerPtr lock,
> > > virLockManagerSanlockPrivatePtr priv = lock->privateData;
> > >
> > > virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY |
> > > - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1);
> > > + VIR_LOCK_MANAGER_RESOURCE_SHARED |
> > > + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);
> > >
> > > if (priv->res_count == SANLK_MAX_RESOURCES) {
> > > virReportError(VIR_ERR_INTERNAL_ERROR,
> > > @@ -804,6 +805,11 @@ static int
> > > virLockManagerSanlockAddResource(virLockManagerPtr lock,
> > > if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
> > > return 0;
> > >
> > > +/* No metadata locking support for now.
> > > + * TODO: implement it. */
> > > +if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA)
> > > +return 0;
> > > +
> >
> > Doesn't this give someone the false impression that their resource is
> > locked if they choose METADATA?
> >
> > Something doesn't feel right about that - giving the impression that
> > it's supported and the consumer is protected, but when push comes to
> > shove they aren't.
> >
> > I'd be inclined to believe that we may want to do nothing with/for
> > sanlock allowing the virCheckFlags above take care of the consumer.
>
> Yeah, this doesn't feel right to me. I think we need to treat the
> metadata locking as completely independant of the content locking.
> This implies we should have a separate configuration for metadata
> locking, where the only valid options are "lockd" or "nop".
>
> eg our available matrix looks like
>
> METADATA
> | nop lockd sanlock
> -+
> nop | Y Y N
> CONTENTlockd | Y Y N
> sanlock | Y Y N
Oh and even for virtlockd we need to consider the config separately
for content vs metdata.
For content we have the possibility to lock out of band files as a
proxy for the main file. This is primarily done for protecting
shared blocks devices as locking the device node doesn't apply
across hosts.
For metadata locking, we only ever care about the local device node
as changes to labelling/ownership don't propagate across hosts. IOW,
we should always directly lock the file in question for metadata
locking, and never use an out of band proxy for locking.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 5/5] jobs: Don't remove non-existing archives
On Fri, Aug 10, 2018 at 03:37:45PM +0200, Andrea Bolognani wrote:
> None of our jobs create the archives as a side-effect, so
> trying to remove them before generating them is pointless.
Who cleans them after the jobs below? Is there a chance that you could trigger
these jobs in a sequence that would expect the archives to be removed but
wouldn't? If there is a risk that this could happen, then I think this patch
should be dropped.
Erik
>
> Signed-off-by: Andrea Bolognani
> ---
> jobs/autotools.yaml| 1 -
> jobs/perl-modulebuild.yaml | 1 -
> jobs/python-distutils.yaml | 1 -
> 3 files changed, 3 deletions(-)
>
> diff --git a/jobs/autotools.yaml b/jobs/autotools.yaml
> index f8a7e87..4e075bd 100644
> --- a/jobs/autotools.yaml
> +++ b/jobs/autotools.yaml
> @@ -168,7 +168,6 @@
>{local_env}
>cd build
>{strip_buildrequires}
> - rm -f *.tar.{archive_format}
>$MAKE dist
>rpmbuild --clean --define "_topdir `pwd`/rpmbuild" -ta
> *.tar.{archive_format}
> publishers:
> diff --git a/jobs/perl-modulebuild.yaml b/jobs/perl-modulebuild.yaml
> index 8b30d7e..855e6ba 100644
> --- a/jobs/perl-modulebuild.yaml
> +++ b/jobs/perl-modulebuild.yaml
> @@ -122,7 +122,6 @@
>{global_env}
>{local_env}
>{strip_buildrequires}
> - rm -f *.tar.{archive_format}
>perl Build dist
>rpmbuild --clean --define "_topdir `pwd`/rpmbuild" -ta
> *.tar.{archive_format}
> publishers:
> diff --git a/jobs/python-distutils.yaml b/jobs/python-distutils.yaml
> index eae5b6f..e640434 100644
> --- a/jobs/python-distutils.yaml
> +++ b/jobs/python-distutils.yaml
> @@ -122,7 +122,6 @@
>{global_env}
>{local_env}
>{strip_buildrequires}
> - rm -f dist/*.tar.{{ archive_format }}
>$PYTHON ./setup.py sdist
>rpmbuild --clean --define "_topdir `pwd`/rpmbuild" -ta
> dist/*.tar.{{ archive_format }}
> publishers:
> --
> 2.17.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 5/7] lock_driver_sanlock: Handle metadata flag gracefully
On Fri, Aug 17, 2018 at 10:49:12AM -0400, John Ferlan wrote:
>
>
> On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> > No real support implemented here. But hey, at least we will not
> > fail.
> >
> > Signed-off-by: Michal Privoznik
> > ---
> > src/locking/lock_driver_sanlock.c | 25 ++---
> > 1 file changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/locking/lock_driver_sanlock.c
> > b/src/locking/lock_driver_sanlock.c
> > index 3e5f0e37b0..c1996fb937 100644
> > --- a/src/locking/lock_driver_sanlock.c
> > +++ b/src/locking/lock_driver_sanlock.c
> > @@ -791,7 +791,8 @@ static int
> > virLockManagerSanlockAddResource(virLockManagerPtr lock,
> > virLockManagerSanlockPrivatePtr priv = lock->privateData;
> >
> > virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY |
> > - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1);
> > + VIR_LOCK_MANAGER_RESOURCE_SHARED |
> > + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);
> >
> > if (priv->res_count == SANLK_MAX_RESOURCES) {
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > @@ -804,6 +805,11 @@ static int
> > virLockManagerSanlockAddResource(virLockManagerPtr lock,
> > if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
> > return 0;
> >
> > +/* No metadata locking support for now.
> > + * TODO: implement it. */
> > +if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA)
> > +return 0;
> > +
>
> Doesn't this give someone the false impression that their resource is
> locked if they choose METADATA?
>
> Something doesn't feel right about that - giving the impression that
> it's supported and the consumer is protected, but when push comes to
> shove they aren't.
>
> I'd be inclined to believe that we may want to do nothing with/for
> sanlock allowing the virCheckFlags above take care of the consumer.
Yeah, this doesn't feel right to me. I think we need to treat the
metadata locking as completely independant of the content locking.
This implies we should have a separate configuration for metadata
locking, where the only valid options are "lockd" or "nop".
eg our available matrix looks like
METADATA
| nop lockd sanlock
-+
nop | Y Y N
CONTENTlockd | Y Y N
sanlock | Y Y N
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/7] lockd_driver_lockd: Implement metadata flag
On Mon, Aug 20, 2018 at 09:25:13AM +0200, Michal Prívozník wrote:
> On 08/17/2018 01:53 AM, John Ferlan wrote:
> >
> >
> > On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> >> Signed-off-by: Michal Privoznik
> >> ---
> >> src/locking/lock_daemon_dispatch.c | 12 ++--
> >> src/locking/lock_driver_lockd.c| 31 +--
> >> src/locking/lock_driver_lockd.h| 1 +
> >> 3 files changed, 32 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/src/locking/lock_daemon_dispatch.c
> >> b/src/locking/lock_daemon_dispatch.c
> >> index 10248ec0b5..c24571ceac 100644
> >> --- a/src/locking/lock_daemon_dispatch.c
> >> +++ b/src/locking/lock_daemon_dispatch.c
> >> @@ -37,6 +37,9 @@ VIR_LOG_INIT("locking.lock_daemon_dispatch");
> >>
> >> #include "lock_daemon_dispatch_stubs.h"
> >>
> >> +#define DEFAULT_OFFSET 0
> >> +#define METADATA_OFFSET 1
> >> +
> >
> > Curious, does this lead to the prospect of being able to acquire/use
> > offset==0 length==1 and offset==1 length==1 at least conceptually and
> > granularity wise?
>
> Yes, this is exactly the purpose. I've taken inspiration from qemu. They
> lock bytes from 100 to 100 + N, depending on what operations they want
> the disk for (checkout raw_apply_lock_bytes() form block/file-posix.c).
> And my idea was for virtlockd to do the same. Currently, it locks 0th
> byte of given file and with metadata flag it would lock the first one.
Agreed, NFSv3 is already doomed, and this barely makes it worse because
of the short lock time. NFSv4 is the fix for anyone who really cares.
Better to have stale NFSv3 locks due to dead clients not releasing locks
than to loose all your VM disk due to corruption.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/7] lock_driver.h: Introduce metadata flag
On Mon, Aug 20, 2018 at 09:25:18AM +0200, Michal Prívozník wrote:
> On 08/17/2018 12:24 AM, John Ferlan wrote:
> >
> >
> > On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> >> Signed-off-by: Michal Privoznik
> >> ---
> >> src/locking/lock_driver.h | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
> >> index 8b7521..7c8f79520a 100644
> >> --- a/src/locking/lock_driver.h
> >> +++ b/src/locking/lock_driver.h
> >> @@ -56,6 +56,8 @@ typedef enum {
> >> VIR_LOCK_MANAGER_RESOURCE_READONLY = (1 << 0),
> >> /* The resource is assigned in shared, writable mode */
> >> VIR_LOCK_MANAGER_RESOURCE_SHARED = (1 << 1),
> >> +/* The resource is locked for metadata change */
> >> +VIR_LOCK_MANAGER_RESOURCE_METADATA = (1 << 2),
> >
> > Does this work or make sense for lease type?
>
> That's the thing. You're right, it doesn't make sense for lease type.
> But on the level of RPC of virtlockd both leases and disks kind of blend
> together. I mean, virtlockd is merely told to lock this or that file.
That's ok. The way I look at it the domain XML describes various
resources, disks, leases, etc. The virtlockd daemon provides a
generic mechanism for acquiring locks. We just happen to map leases
onto the default lock type. Here we're adding a 2nd lock type and do
not require any mapping from leases.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/7] virlockspace: Introduce VIR_LOCK_SPACE_ACQUIRE_WAIT
On Tue, Aug 14, 2018 at 01:19:38PM +0200, Michal Privoznik wrote: > This flag modifies the way the lock is acquired. It waits for the > lock to be set instead of usual set-or-fail logic that happens > without this flag. > > Signed-off-by: Michal Privoznik > --- > src/util/virlockspace.c | 14 ++ > src/util/virlockspace.h | 1 + > 2 files changed, 11 insertions(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/7] virlockspace: Allow caller to specify start and length offset in virLockSpaceAcquireResource
On Tue, Aug 14, 2018 at 01:19:37PM +0200, Michal Privoznik wrote: > So far the virLockSpaceAcquireResource() locks the first byte in > the underlying file. But caller might want to lock other range. > > Signed-off-by: Michal Privoznik > --- > src/locking/lock_daemon_dispatch.c | 3 +++ > src/util/virlockspace.c| 15 ++- > src/util/virlockspace.h| 4 > tests/virlockspacetest.c | 29 - > 4 files changed, 41 insertions(+), 10 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 1/5] jobs: Call rpmbuild explicitly for Python projects
On Mon, 2018-08-20 at 16:01 +0200, Erik Skultety wrote:
> On Fri, Aug 10, 2018 at 03:37:41PM +0200, Andrea Bolognani wrote:
> > - $PYTHON ./setup.py rpm
> > + rm -f dist/*.tar.{{ archive_format }}
> > + $PYTHON ./setup.py sdist
> > + rpmbuild --clean --define "_topdir `pwd`/rpmbuild" -ta
> > dist/*.tar.{{ archive_format }}
>
> So what if you used a standard bdist_rpm command from distutils core, I
> believe
> $PYTHON ./setup.py bdist_rpm --bdist-base would be equal to your
> _topdir.
> Although, that's just what I've digested from distutils docs, so even though
> bdist_rpm has a plethora of options you can specify there can always be one
> we'll be missing :P
I haven't been able to find any bdist_rpm documentation that is not
filed under Python 2, which leads me to believe it might not be as
supported (if at all) under Python 3; moreover, the current
documentation[1] seems to point to FPM as the preferred way to
generate RPM packages, but that process doesn't looks like it
involves spec files at all and bundle a whole lot of other stuff
along with your actual software, so I'd say it's not really suitable
for our purpose.
In any case, I would still prefer the two-step approach (dist plus
rpmbuild) to building RPMs because it is consistent with what we do
for all other build systems (autotools and Perl's Module::Build).
[1] https://packaging.python.org/overview/#operating-system-packages
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 4/5] jobs: Minimize strip_buildrequires
On Fri, Aug 10, 2018 at 03:37:44PM +0200, Andrea Bolognani wrote: > We can drop some entries and tweak some others to be > less verbose without losing in functionality. > > Signed-off-by: Andrea Bolognani > --- > jobs/defaults.yaml | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/jobs/defaults.yaml b/jobs/defaults.yaml > index e4d1f2c..bab5bc4 100644 > --- a/jobs/defaults.yaml > +++ b/jobs/defaults.yaml > @@ -24,11 +24,9 @@ > global_env: | > local_env: | > strip_buildrequires: | > - sed -i -e 's/BuildRequires: *libvirt.*devel.*//' *.spec* > - sed -i -e 's/BuildRequires: *osinfo-db-tools.*//' *.spec* > + sed -i -e 's/BuildRequires: *libvirt.*//' *.spec* >sed -i -e 's/BuildRequires: *osinfo-db.*//' *.spec* >sed -i -e 's/BuildRequires: *perl(Sys::Virt).*//' *.spec* > - sed -i -e 's/BuildRequires: *perl-Sys-Virt.*//' *.spec* I assume because ^these 2 are somehow identical? Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 3/5] jobs: Introduce strip_buildrequires
On Fri, Aug 10, 2018 at 03:37:43PM +0200, Andrea Bolognani wrote: > There's no harm in attempting to strip more BuildRequires > than are present in a spec file, so define a shell snippet > we can reuse for the the purpose without duplicating the > same lines all over the place. > > Signed-off-by: Andrea Bolognani > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 2/5] jobs: Call rpmbuild and sed consistently
On Fri, Aug 10, 2018 at 03:37:42PM +0200, Andrea Bolognani wrote: > Doing so will allow us to refactor away the common parts. > > Signed-off-by: Andrea Bolognani > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 1/5] jobs: Call rpmbuild explicitly for Python projects
On Fri, Aug 10, 2018 at 03:37:41PM +0200, Andrea Bolognani wrote:
> Instead of using the custom 'rpm' target of setup.py,
> generate a dist archive using the 'sdist' target and then
> call rpmbuild ourselves: this way we can define _topdir
> and stop artifacts from ending up in ~/rpmbuild.
>
> Signed-off-by: Andrea Bolognani
> ---
> jobs/python-distutils.yaml | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/jobs/python-distutils.yaml b/jobs/python-distutils.yaml
> index 0b20b17..5601d26 100644
> --- a/jobs/python-distutils.yaml
> +++ b/jobs/python-distutils.yaml
> @@ -122,7 +122,9 @@
>{global_env}
>{local_env}
>sed -i -e 's/BuildRequires: libvirt.*devel.*//' *.spec.in
> - $PYTHON ./setup.py rpm
> + rm -f dist/*.tar.{{ archive_format }}
> + $PYTHON ./setup.py sdist
> + rpmbuild --clean --define "_topdir `pwd`/rpmbuild" -ta
> dist/*.tar.{{ archive_format }}
So what if you used a standard bdist_rpm command from distutils core, I believe
$PYTHON ./setup.py bdist_rpm --bdist-base would be equal to your _topdir.
Although, that's just what I've digested from distutils docs, so even though
bdist_rpm has a plethora of options you can specify there can always be one
we'll be missing :P
Erik
> publishers:
>- email:
>recipients: '{obj:spam}'
> --
> 2.17.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 8/8] storage_driver: Release pool object lock for some long running jobs
As advertised in previous commit, there are three APIs that might
run for quite some time (because they read/write data from/to a
volume) and these three are: downloadVol, uploadVol, wipeVol.
Release pool object lock and reacquire it later to allow more
concurrency.
Signed-off-by: Michal Privoznik
---
src/storage/storage_backend_iscsi_direct.c | 6 +-
src/storage/storage_backend_rbd.c | 8 ++--
src/storage/storage_driver.c | 6 ++
3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_iscsi_direct.c
b/src/storage/storage_backend_iscsi_direct.c
index 0d7d6ba9c3..5c1b251a17 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -693,7 +693,11 @@ virStorageBackenISCSIDirectWipeVol(virStoragePoolObjPtr
pool,
virCheckFlags(0, -1);
-if (!(iscsi = virStorageBackendISCSIDirectSetConnection(pool, NULL)))
+virObjectLock(pool);
+iscsi = virStorageBackendISCSIDirectSetConnection(pool, NULL);
+virObjectUnlock(pool);
+
+if (!iscsi)
return -1;
switch ((virStorageVolWipeAlgorithm) algorithm) {
diff --git a/src/storage/storage_backend_rbd.c
b/src/storage/storage_backend_rbd.c
index c6fb791a81..2cba678b72 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -1200,7 +1200,7 @@ virStorageBackendRBDVolWipe(virStoragePoolObjPtr pool,
unsigned int flags)
{
virStorageBackendRBDStatePtr ptr = NULL;
-virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
+virStoragePoolDefPtr def;
rbd_image_t image = NULL;
rbd_image_info_t info;
uint64_t stripe_count;
@@ -1209,9 +1209,13 @@ virStorageBackendRBDVolWipe(virStoragePoolObjPtr pool,
virCheckFlags(0, -1);
+virObjectLock(pool);
+def = virStoragePoolObjGetDef(pool);
VIR_DEBUG("Wiping RBD image %s/%s", def->source.name, vol->name);
+ptr = virStorageBackendRBDNewState(pool);
+virObjectUnlock(pool);
-if (!(ptr = virStorageBackendRBDNewState(pool)))
+if (!ptr)
goto cleanup;
if ((r = rbd_open(ptr->ioctx, vol->name, , NULL)) < 0) {
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 9edd5df119..8943df1f84 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2178,9 +2178,11 @@ storageVolDownload(virStorageVolPtr vol,
virStoragePoolObjIncrAsyncjobs(obj);
voldef->in_use++;
+virObjectUnlock(obj);
ret = backend->downloadVol(obj, voldef, stream, offset, length, flags);
+virObjectLock(obj);
voldef->in_use--;
virStoragePoolObjDecrAsyncjobs(obj);
@@ -2378,9 +2380,11 @@ storageVolUpload(virStorageVolPtr vol,
virStoragePoolObjIncrAsyncjobs(obj);
voldef->in_use++;
+virObjectUnlock(obj);
rc = backend->uploadVol(obj, voldef, stream, offset, length, flags);
+virObjectLock(obj);
voldef->in_use--;
virStoragePoolObjDecrAsyncjobs(obj);
@@ -2554,9 +2558,11 @@ storageVolWipePattern(virStorageVolPtr vol,
virStoragePoolObjIncrAsyncjobs(obj);
voldef->in_use++;
+virObjectUnlock(obj);
rc = backend->wipeVol(obj, voldef, algorithm, flags);
+virObjectLock(obj);
voldef->in_use--;
virStoragePoolObjDecrAsyncjobs(obj);
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 5/8] virStoragePoolObjSourceFindDuplicate: Drop @conn argument
The @conn argument is needed only to do some source matching in
case of iSCSI source. Anyway, it's used just for node device
driver and as such can be replaced with virGetConnectNodeDev().
At the same time, the @conn struct member is dropped from
_virStoragePoolObjFindDuplicateData.
Signed-off-by: Michal Privoznik
---
src/conf/virstorageobj.c | 25 +++--
src/conf/virstorageobj.h | 3 +--
src/storage/storage_driver.c | 4 ++--
3 files changed, 14 insertions(+), 18 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index b710ec652d..dce45ce870 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -1169,7 +1169,6 @@ virStorageIsSameHostnum(const char *name,
/*
* matchFCHostToSCSIHost:
*
- * @conn: Connection pointer
* @fchost: fc_host adapter ptr (either def or pool->def)
* @scsi_hostnum: Already determined "scsi_pool" hostnum
*
@@ -1177,10 +1176,10 @@ virStorageIsSameHostnum(const char *name,
* fc_adapter host# and the scsi_host host#
*/
static bool
-matchFCHostToSCSIHost(virConnectPtr conn,
- virStorageAdapterFCHostPtr fchost,
+matchFCHostToSCSIHost(virStorageAdapterFCHostPtr fchost,
unsigned int scsi_hostnum)
{
+virConnectPtr conn = NULL;
bool ret = false;
char *name = NULL;
char *scsi_host_name = NULL;
@@ -1211,7 +1210,8 @@ matchFCHostToSCSIHost(virConnectPtr conn,
* If the parent fc_hostnum is the same as the scsi_hostnum, we
* have a match.
*/
-if (conn && !fchost->parent) {
+if (!fchost->parent &&
+(conn = virGetConnectNodeDev())) {
if (virAsprintf(_host_name, "scsi_%s", name) < 0)
goto cleanup;
if ((parent_name = virNodeDeviceGetParentName(conn,
@@ -1240,6 +1240,7 @@ matchFCHostToSCSIHost(virConnectPtr conn,
VIR_FREE(name);
VIR_FREE(parent_name);
VIR_FREE(scsi_host_name);
+virConnectClose(conn);
return ret;
}
@@ -1318,8 +1319,7 @@ virStoragePoolObjSourceMatchTypeDIR(virStoragePoolObjPtr
obj,
static virStoragePoolObjPtr
virStoragePoolObjSourceMatchTypeISCSI(virStoragePoolObjPtr obj,
- virStoragePoolDefPtr def,
- virConnectPtr conn)
+ virStoragePoolDefPtr def)
{
virStorageAdapterPtr pool_adapter = >def->source.adapter;
virStorageAdapterPtr def_adapter = >source.adapter;
@@ -1363,7 +1363,7 @@
virStoragePoolObjSourceMatchTypeISCSI(virStoragePoolObjPtr obj,
if (getSCSIHostNumber(def_scsi_host, _hostnum) < 0)
return NULL;
-if (matchFCHostToSCSIHost(conn, pool_fchost, scsi_hostnum))
+if (matchFCHostToSCSIHost(pool_fchost, scsi_hostnum))
return obj;
} else if (pool_adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST &&
@@ -1374,7 +1374,7 @@
virStoragePoolObjSourceMatchTypeISCSI(virStoragePoolObjPtr obj,
if (getSCSIHostNumber(pool_scsi_host, _hostnum) < 0)
return NULL;
-if (matchFCHostToSCSIHost(conn, def_fchost, scsi_hostnum))
+if (matchFCHostToSCSIHost(def_fchost, scsi_hostnum))
return obj;
}
@@ -1411,7 +1411,6 @@
virStoragePoolObjSourceMatchTypeDEVICE(virStoragePoolObjPtr obj,
struct _virStoragePoolObjFindDuplicateData {
-virConnectPtr conn;
virStoragePoolDefPtr def;
};
@@ -1439,7 +1438,7 @@ virStoragePoolObjSourceFindDuplicateCb(const void
*payload,
case VIR_STORAGE_POOL_SCSI:
if (data->def->type == obj->def->type &&
-virStoragePoolObjSourceMatchTypeISCSI(obj, data->def, data->conn))
+virStoragePoolObjSourceMatchTypeISCSI(obj, data->def))
return 1;
break;
@@ -1488,12 +1487,10 @@ virStoragePoolObjSourceFindDuplicateCb(const void
*payload,
int
-virStoragePoolObjSourceFindDuplicate(virConnectPtr conn,
- virStoragePoolObjListPtr pools,
+virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools,
virStoragePoolDefPtr def)
{
-struct _virStoragePoolObjFindDuplicateData data = { .conn = conn,
-.def = def };
+struct _virStoragePoolObjFindDuplicateData data = {.def = def};
virStoragePoolObjPtr obj = NULL;
virObjectRWLockRead(pools);
diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h
index 9fabf34e4f..bc24db1928 100644
--- a/src/conf/virstorageobj.h
+++ b/src/conf/virstorageobj.h
@@ -246,8 +246,7 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
virStoragePoolObjPtr obj);
int
-virStoragePoolObjSourceFindDuplicate(virConnectPtr conn,
- virStoragePoolObjListPtr pools,
+virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools,
[libvirt] [PATCH v2 7/8] storage_driver: Mark volume as 'in use' for some operations
There are few operations in the storage driver that read/write data onto volumes. Such operations can take very long time to finish. During that time the storage pool object is locked which has bad performance impacts (other threads can't fetch its XML for instance). This commit prepares the storage driver for releasing the lock during those operations (downloadVol, uploadVol, wipeVol). Signed-off-by: Michal Privoznik --- src/storage/storage_driver.c | 34 -- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e7085e4773..9edd5df119 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2176,8 +2176,13 @@ storageVolDownload(virStorageVolPtr vol, goto cleanup; } -ret = backend->downloadVol(obj, voldef, stream, - offset, length, flags); +virStoragePoolObjIncrAsyncjobs(obj); +voldef->in_use++; + +ret = backend->downloadVol(obj, voldef, stream, offset, length, flags); + +voldef->in_use--; +virStoragePoolObjDecrAsyncjobs(obj); cleanup: virStoragePoolObjEndAPI(); @@ -2326,6 +2331,7 @@ storageVolUpload(virStorageVolPtr vol, virStoragePoolDefPtr def; virStorageVolDefPtr voldef = NULL; virStorageVolStreamInfoPtr cbdata = NULL; +int rc; int ret = -1; virCheckFlags(VIR_STORAGE_VOL_UPLOAD_SPARSE_STREAM, -1); @@ -2370,8 +2376,15 @@ storageVolUpload(virStorageVolPtr vol, VIR_STRDUP(cbdata->vol_path, voldef->target.path) < 0) goto cleanup; -if ((ret = backend->uploadVol(obj, voldef, stream, - offset, length, flags)) < 0) +virStoragePoolObjIncrAsyncjobs(obj); +voldef->in_use++; + +rc = backend->uploadVol(obj, voldef, stream, offset, length, flags); + +voldef->in_use--; +virStoragePoolObjDecrAsyncjobs(obj); + +if (rc < 0) goto cleanup; /* Add cleanup callback - call after uploadVol since the stream @@ -2381,7 +2394,7 @@ storageVolUpload(virStorageVolPtr vol, virStorageVolFDStreamCloseCb, cbdata, NULL); cbdata = NULL; - +ret = 0; cleanup: virStoragePoolObjEndAPI(); if (cbdata) @@ -2499,6 +2512,7 @@ storageVolWipePattern(virStorageVolPtr vol, virStorageBackendPtr backend; virStoragePoolObjPtr obj = NULL; virStorageVolDefPtr voldef = NULL; +int rc; int ret = -1; virCheckFlags(0, -1); @@ -2538,7 +2552,15 @@ storageVolWipePattern(virStorageVolPtr vol, goto cleanup; } -if (backend->wipeVol(obj, voldef, algorithm, flags) < 0) +virStoragePoolObjIncrAsyncjobs(obj); +voldef->in_use++; + +rc = backend->wipeVol(obj, voldef, algorithm, flags); + +voldef->in_use--; +virStoragePoolObjDecrAsyncjobs(obj); + +if (rc < 0) goto cleanup; /* Instead of using the refreshVol, since much changes on the target -- 2.16.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/8] virstorageobj: Move virStoragePoolObjIsDuplicate up
This function is going to be made static in used in
virStoragePoolObjAssignDef(). Therefore move it a few lines up.
Signed-off-by: Michal Privoznik
---
src/conf/virstorageobj.c | 124 +++
1 file changed, 62 insertions(+), 62 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 6f1e14ed3e..185822451b 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -1047,6 +1047,68 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn,
}
+/*
+ * virStoragePoolObjIsDuplicate:
+ * @doms : virStoragePoolObjListPtr to search
+ * @def : virStoragePoolDefPtr definition of pool to lookup
+ * @check_active: If true, ensure that pool is not active
+ *
+ * Returns: -1 on error
+ * 0 if pool is new
+ * 1 if pool is a duplicate
+ */
+int
+virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
+ virStoragePoolDefPtr def,
+ bool check_active)
+{
+int ret = -1;
+virStoragePoolObjPtr obj = NULL;
+
+/* See if a Pool with matching UUID already exists */
+obj = virStoragePoolObjFindByUUID(pools, def->uuid);
+if (obj) {
+/* UUID matches, but if names don't match, refuse it */
+if (STRNEQ(obj->def->name, def->name)) {
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+virUUIDFormat(obj->def->uuid, uuidstr);
+virReportError(VIR_ERR_OPERATION_FAILED,
+ _("pool '%s' is already defined with uuid %s"),
+ obj->def->name, uuidstr);
+goto cleanup;
+}
+
+if (check_active) {
+/* UUID & name match, but if Pool is already active, refuse it */
+if (virStoragePoolObjIsActive(obj)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ _("pool is already active as '%s'"),
+ obj->def->name);
+goto cleanup;
+}
+}
+
+ret = 1;
+} else {
+/* UUID does not match, but if a name matches, refuse it */
+obj = virStoragePoolObjFindByName(pools, def->name);
+if (obj) {
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+virUUIDFormat(obj->def->uuid, uuidstr);
+virReportError(VIR_ERR_OPERATION_FAILED,
+ _("pool '%s' already exists with uuid %s"),
+ def->name, uuidstr);
+goto cleanup;
+}
+ret = 0;
+}
+
+ cleanup:
+virStoragePoolObjEndAPI();
+return ret;
+}
+
+
/**
* virStoragePoolObjAssignDef:
* @pools: Storage Pool object list pointer
@@ -1452,68 +1514,6 @@ virStoragePoolObjGetNames(virStoragePoolObjListPtr pools,
}
-/*
- * virStoragePoolObjIsDuplicate:
- * @doms : virStoragePoolObjListPtr to search
- * @def : virStoragePoolDefPtr definition of pool to lookup
- * @check_active: If true, ensure that pool is not active
- *
- * Returns: -1 on error
- * 0 if pool is new
- * 1 if pool is a duplicate
- */
-int
-virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
- virStoragePoolDefPtr def,
- bool check_active)
-{
-int ret = -1;
-virStoragePoolObjPtr obj = NULL;
-
-/* See if a Pool with matching UUID already exists */
-obj = virStoragePoolObjFindByUUID(pools, def->uuid);
-if (obj) {
-/* UUID matches, but if names don't match, refuse it */
-if (STRNEQ(obj->def->name, def->name)) {
-char uuidstr[VIR_UUID_STRING_BUFLEN];
-virUUIDFormat(obj->def->uuid, uuidstr);
-virReportError(VIR_ERR_OPERATION_FAILED,
- _("pool '%s' is already defined with uuid %s"),
- obj->def->name, uuidstr);
-goto cleanup;
-}
-
-if (check_active) {
-/* UUID & name match, but if Pool is already active, refuse it */
-if (virStoragePoolObjIsActive(obj)) {
-virReportError(VIR_ERR_OPERATION_INVALID,
- _("pool is already active as '%s'"),
- obj->def->name);
-goto cleanup;
-}
-}
-
-ret = 1;
-} else {
-/* UUID does not match, but if a name matches, refuse it */
-obj = virStoragePoolObjFindByName(pools, def->name);
-if (obj) {
-char uuidstr[VIR_UUID_STRING_BUFLEN];
-virUUIDFormat(obj->def->uuid, uuidstr);
-virReportError(VIR_ERR_OPERATION_FAILED,
- _("pool '%s' already exists with uuid %s"),
- def->name, uuidstr);
-goto cleanup;
-}
-ret = 0;
-}
-
- cleanup:
-virStoragePoolObjEndAPI();
-return ret;
-}
-
-
static int
getSCSIHostNumber(virStorageAdapterSCSIHostPtr scsi_host,
[libvirt] [PATCH v2 1/8] storage_backend_rbd: Drop ATTRIBUTE_UNUSED for arguments that are used
In two places the passed pool object argument is marked as
ATTRIBUTE_UNUSED even though it's used right away.
Signed-off-by: Michal Privoznik
---
src/storage/storage_backend_rbd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_rbd.c
b/src/storage/storage_backend_rbd.c
index 642cacb673..c6fb791a81 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -1063,7 +1063,7 @@ virStorageBackendRBDBuildVolFrom(virStoragePoolObjPtr
pool,
}
static int
-virStorageBackendRBDRefreshVol(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
+virStorageBackendRBDRefreshVol(virStoragePoolObjPtr pool,
virStorageVolDefPtr vol)
{
virStorageBackendRBDStatePtr ptr = NULL;
@@ -1083,7 +1083,7 @@ virStorageBackendRBDRefreshVol(virStoragePoolObjPtr pool
ATTRIBUTE_UNUSED,
}
static int
-virStorageBackendRBDResizeVol(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
+virStorageBackendRBDResizeVol(virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
unsigned long long capacity,
unsigned int flags)
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/8] storage: Resolve some locking issues
Technically, this is a v2 of: https://www.redhat.com/archives/libvir-list/2018-August/msg00624.html but majority of the patches are new. The review of the patch from above made me look into storage pool locking more and I have found some issues. For best view of the patches (esp. 4/8 use --histogram option). Michal Prívozník (8): storage_backend_rbd: Drop ATTRIBUTE_UNUSED for arguments that are used virstorageobj: Move virStoragePoolObjIsDuplicate up virstorageobj: Check for duplicates from virStoragePoolObjAssignDef virstorageobj: Move virStoragePoolObjSourceFindDuplicate and friends up virStoragePoolObjSourceFindDuplicate: Drop @conn argument virstorageobj: Check for source duplicates from virStoragePoolObjAssignDef storage_driver: Mark volume as 'in use' for some operations storage_driver: Release pool object lock for some long running jobs src/conf/virstorageobj.c | 937 +++-- src/conf/virstorageobj.h | 13 +- src/libvirt_private.syms | 2 - src/storage/storage_backend_iscsi_direct.c | 6 +- src/storage/storage_backend_rbd.c | 12 +- src/storage/storage_driver.c | 56 +- src/test/test_driver.c | 12 +- 7 files changed, 529 insertions(+), 509 deletions(-) -- 2.16.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/8] virstorageobj: Check for duplicates from virStoragePoolObjAssignDef
Even though we do some checking is not as thorough as it should
be. We already have virStoragePoolObjIsDuplicate but the way we
use it is a typical TOCTOU. Imagine two threads trying to define
two pools with the same name but different UUIDs. With the
current code neither of them finds a duplicate and thus proceed
to virStoragePoolObjAssignDef where only names are compared.
Therefore both threads succeed which is obviously wrong.
We should check for duplicates where we care for them.
Signed-off-by: Michal Privoznik
---
src/conf/virstorageobj.c | 39 +++
src/conf/virstorageobj.h | 8 ++--
src/libvirt_private.syms | 1 -
src/storage/storage_driver.c | 10 ++
src/test/test_driver.c | 12 +++-
5 files changed, 34 insertions(+), 36 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 185822451b..ea0ae6fd86 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -1052,22 +1052,28 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn,
* @doms : virStoragePoolObjListPtr to search
* @def : virStoragePoolDefPtr definition of pool to lookup
* @check_active: If true, ensure that pool is not active
+ * @objRet: returned pool object
*
- * Returns: -1 on error
+ * Assumes @pools is locked by caller already.
+ *
+ * Returns: -1 on error (name or UUID mismatch)
* 0 if pool is new
- * 1 if pool is a duplicate
+ * 1 if pool is a duplicate (name and UUID match)
*/
-int
+static int
virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
virStoragePoolDefPtr def,
- bool check_active)
+ bool check_active,
+ virStoragePoolObjPtr *objRet)
{
int ret = -1;
virStoragePoolObjPtr obj = NULL;
/* See if a Pool with matching UUID already exists */
-obj = virStoragePoolObjFindByUUID(pools, def->uuid);
+obj = virStoragePoolObjFindByUUIDLocked(pools, def->uuid);
if (obj) {
+virObjectLock(obj);
+
/* UUID matches, but if names don't match, refuse it */
if (STRNEQ(obj->def->name, def->name)) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -1088,11 +1094,14 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr
pools,
}
}
+VIR_STEAL_PTR(*objRet, obj);
ret = 1;
} else {
/* UUID does not match, but if a name matches, refuse it */
-obj = virStoragePoolObjFindByName(pools, def->name);
+obj = virStoragePoolObjFindByNameLocked(pools, def->name);
if (obj) {
+virObjectLock(obj);
+
char uuidstr[VIR_UUID_STRING_BUFLEN];
virUUIDFormat(obj->def->uuid, uuidstr);
virReportError(VIR_ERR_OPERATION_FAILED,
@@ -1113,6 +1122,7 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr
pools,
* virStoragePoolObjAssignDef:
* @pools: Storage Pool object list pointer
* @def: Storage pool definition to add or update
+ * @check_active: If true, ensure that pool is not active
*
* Lookup the @def to see if it already exists in the @pools in order
* to either update or add if it does not exist.
@@ -1121,15 +1131,20 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr
pools,
*/
virStoragePoolObjPtr
virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools,
- virStoragePoolDefPtr def)
+ virStoragePoolDefPtr def,
+ bool check_active)
{
-virStoragePoolObjPtr obj;
+virStoragePoolObjPtr obj = NULL;
char uuidstr[VIR_UUID_STRING_BUFLEN];
+int rc;
virObjectRWLockWrite(pools);
-if ((obj = virStoragePoolObjFindByNameLocked(pools, def->name))) {
-virObjectLock(obj);
+rc = virStoragePoolObjIsDuplicate(pools, def, check_active, );
+
+if (rc < 0)
+goto error;
+if (rc > 0) {
if (!virStoragePoolObjIsActive(obj)) {
virStoragePoolDefFree(obj->def);
obj->def = def;
@@ -1186,7 +1201,7 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools,
return NULL;
}
-if (!(obj = virStoragePoolObjAssignDef(pools, def))) {
+if (!(obj = virStoragePoolObjAssignDef(pools, def, false))) {
virStoragePoolDefFree(def);
return NULL;
}
@@ -1248,7 +1263,7 @@ virStoragePoolObjLoadState(virStoragePoolObjListPtr pools,
}
/* create the object */
-if (!(obj = virStoragePoolObjAssignDef(pools, def)))
+if (!(obj = virStoragePoolObjAssignDef(pools, def, true)))
goto error;
/* XXX: future handling of some additional useful status data,
diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h
index dd7001c4b2..9fabf34e4f 100644
--- a/src/conf/virstorageobj.h
+++ b/src/conf/virstorageobj.h
@@ -189,7 +189,8 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn,
[libvirt] [PATCH v2 6/8] virstorageobj: Check for source duplicates from virStoragePoolObjAssignDef
Just like a few commits earlier, checking for pool source
duplicates and unlocking pools list afterwards is a buggy
pattern. The check must go into virStoragePoolObjAssignDef.
Signed-off-by: Michal Privoznik
---
src/conf/virstorageobj.c | 7 ---
src/conf/virstorageobj.h | 4
src/libvirt_private.syms | 1 -
src/storage/storage_driver.c | 6 --
4 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index dce45ce870..c717176133 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -1486,17 +1486,15 @@ virStoragePoolObjSourceFindDuplicateCb(const void
*payload,
}
-int
+static int
virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools,
virStoragePoolDefPtr def)
{
struct _virStoragePoolObjFindDuplicateData data = {.def = def};
virStoragePoolObjPtr obj = NULL;
-virObjectRWLockRead(pools);
obj = virHashSearch(pools->objs, virStoragePoolObjSourceFindDuplicateCb,
, NULL);
-virObjectRWUnlock(pools);
if (obj) {
virReportError(VIR_ERR_OPERATION_FAILED,
@@ -1531,6 +1529,9 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools,
virObjectRWLockWrite(pools);
+if (virStoragePoolObjSourceFindDuplicate(pools, def) < 0)
+goto error;
+
rc = virStoragePoolObjIsDuplicate(pools, def, check_active, );
if (rc < 0)
diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h
index bc24db1928..abd6166d88 100644
--- a/src/conf/virstorageobj.h
+++ b/src/conf/virstorageobj.h
@@ -245,10 +245,6 @@ void
virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
virStoragePoolObjPtr obj);
-int
-virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools,
- virStoragePoolDefPtr def);
-
int
virStoragePoolObjListExport(virConnectPtr conn,
virStoragePoolObjListPtr poolobjs,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 572d1a1e22..e107efedcc 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1157,7 +1157,6 @@ virStoragePoolObjSetActive;
virStoragePoolObjSetAutostart;
virStoragePoolObjSetConfigFile;
virStoragePoolObjSetDef;
-virStoragePoolObjSourceFindDuplicate;
virStoragePoolObjVolumeGetNames;
virStoragePoolObjVolumeListExport;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index df4f86c4bd..e7085e4773 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -705,9 +705,6 @@ storagePoolCreateXML(virConnectPtr conn,
if (virStoragePoolCreateXMLEnsureACL(conn, newDef) < 0)
goto cleanup;
-if (virStoragePoolObjSourceFindDuplicate(driver->pools, newDef) < 0)
-goto cleanup;
-
if ((backend = virStorageBackendForType(newDef->type)) == NULL)
goto cleanup;
@@ -796,9 +793,6 @@ storagePoolDefineXML(virConnectPtr conn,
if (virStoragePoolDefineXMLEnsureACL(conn, newDef) < 0)
goto cleanup;
-if (virStoragePoolObjSourceFindDuplicate(driver->pools, newDef) < 0)
-goto cleanup;
-
if (virStorageBackendForType(newDef->type) == NULL)
goto cleanup;
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 4/8] virstorageobj: Move virStoragePoolObjSourceFindDuplicate and friends up
This function is going to be made static in used in
virStoragePoolObjAssignDef(). Therefore move it and all the
static functions it calls a few lines up.
Signed-off-by: Michal Privoznik
---
src/conf/virstorageobj.c | 788 +++
1 file changed, 394 insertions(+), 394 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index ea0ae6fd86..b710ec652d 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -1118,6 +1118,400 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr
pools,
}
+static int
+getSCSIHostNumber(virStorageAdapterSCSIHostPtr scsi_host,
+ unsigned int *hostnum)
+{
+int ret = -1;
+unsigned int num;
+char *name = NULL;
+
+if (scsi_host->has_parent) {
+virPCIDeviceAddressPtr addr = _host->parentaddr;
+unsigned int unique_id = scsi_host->unique_id;
+
+if (!(name = virSCSIHostGetNameByParentaddr(addr->domain,
+addr->bus,
+addr->slot,
+addr->function,
+unique_id)))
+goto cleanup;
+if (virSCSIHostGetNumber(name, ) < 0)
+goto cleanup;
+} else {
+if (virSCSIHostGetNumber(scsi_host->name, ) < 0)
+goto cleanup;
+}
+
+*hostnum = num;
+ret = 0;
+
+ cleanup:
+VIR_FREE(name);
+return ret;
+}
+
+
+static bool
+virStorageIsSameHostnum(const char *name,
+unsigned int scsi_hostnum)
+{
+unsigned int fc_hostnum;
+
+if (virSCSIHostGetNumber(name, _hostnum) == 0 &&
+scsi_hostnum == fc_hostnum)
+return true;
+
+return false;
+}
+
+
+/*
+ * matchFCHostToSCSIHost:
+ *
+ * @conn: Connection pointer
+ * @fchost: fc_host adapter ptr (either def or pool->def)
+ * @scsi_hostnum: Already determined "scsi_pool" hostnum
+ *
+ * Returns true/false whether there is a match between the incoming
+ * fc_adapter host# and the scsi_host host#
+ */
+static bool
+matchFCHostToSCSIHost(virConnectPtr conn,
+ virStorageAdapterFCHostPtr fchost,
+ unsigned int scsi_hostnum)
+{
+bool ret = false;
+char *name = NULL;
+char *scsi_host_name = NULL;
+char *parent_name = NULL;
+
+/* If we have a parent defined, get its hostnum, and compare to the
+ * scsi_hostnum. If they are the same, then we have a match
+ */
+if (fchost->parent &&
+virStorageIsSameHostnum(fchost->parent, scsi_hostnum))
+return true;
+
+/* If we find an fc adapter name, then either libvirt created a vHBA
+ * for this fc_host or a 'virsh nodedev-create' generated a vHBA.
+ */
+if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
+
+/* Get the scsi_hostN for the vHBA in order to see if it
+ * matches our scsi_hostnum
+ */
+if (virStorageIsSameHostnum(name, scsi_hostnum)) {
+ret = true;
+goto cleanup;
+}
+
+/* We weren't provided a parent, so we have to query the node
+ * device driver in order to ascertain the parent of the vHBA.
+ * If the parent fc_hostnum is the same as the scsi_hostnum, we
+ * have a match.
+ */
+if (conn && !fchost->parent) {
+if (virAsprintf(_host_name, "scsi_%s", name) < 0)
+goto cleanup;
+if ((parent_name = virNodeDeviceGetParentName(conn,
+ scsi_host_name))) {
+if (virStorageIsSameHostnum(parent_name, scsi_hostnum)) {
+ret = true;
+goto cleanup;
+}
+} else {
+/* Throw away the error and fall through */
+virResetLastError();
+VIR_DEBUG("Could not determine parent vHBA");
+}
+}
+}
+
+/* NB: Lack of a name means that this vHBA hasn't yet been created,
+ * which means our scsi_host cannot be using the vHBA. Furthermore,
+ * lack of a provided parent means libvirt is going to choose the
+ * "best" fc_host capable adapter based on availabilty. That could
+ * conflict with an existing scsi_host definition, but there's no
+ * way to know that now.
+ */
+
+ cleanup:
+VIR_FREE(name);
+VIR_FREE(parent_name);
+VIR_FREE(scsi_host_name);
+return ret;
+}
+
+
+static bool
+matchSCSIAdapterParent(virStorageAdapterSCSIHostPtr pool_scsi_host,
+ virStorageAdapterSCSIHostPtr def_scsi_host)
+{
+virPCIDeviceAddressPtr pooladdr = _scsi_host->parentaddr;
+virPCIDeviceAddressPtr defaddr = _scsi_host->parentaddr;
+
+if (pooladdr->domain == defaddr->domain &&
+pooladdr->bus == defaddr->bus &&
+
Re: [libvirt] [PATCH v3 10/12] qemu: Add hotpluging support for PCI devices on S390 guests
On Mon, 2018-08-20 at 16:04 +0800, Yi Min Zhao wrote: > 在 2018/8/18 上午12:10, Andrea Bolognani 写道: > > On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: > > > This commit adds hotplug support for PCI devices on S390 guests. > > > There's no need to implement hot unplug for zPCI as QEMU implements > > > an unplug callback which will unplug both PCI and zPCI device in a > > > cascaded way. > > > > It looks like you ended up implementing explicit hot unplug at > > least for controllers. I think perhaps it would be a good idea > > to implement it for all devices instead of relying on QEMU's > > own unplug cascading so that we have more control over the whole > > process. > > It's different between controller and device. And we only hot unplug pci > controller, not for > all controller types. In addition, we only could do hot-unplug one time, > either zpci device > or corresponding pci device. It's due to Qemu logic. Qemu will > hot-unplug zpci device > automatically while doing hotplug pci device, and vice versa. Ouch, that's kinda nasty... And I wonder why that doesn't work for controllers too? They're not *that* special :) > > So, I'm very much not familiar with the hotplug code and seeing > > changes to stuff like qemuDomainAttachDiskGeneric() makes me a bit > > uncomfortable :) > > > > I can't spot anything obviously wrong in your changes, but I think > > perhaps you might want to enter and exit the monitor separately > > for the zpci device and for the virtio device? I'm not sure that's > > useful at all, but network devices for example seems to follow > > that pattern... It would be great if someone with more experience > > in this area could provide a review. > > We have to add zpci device firstly and add corresponding pci device > secondly. > Do you think it's redundant to call monitor twice to add two devices? Again, I'm not familiar at all with this code, but entering (and exiting) the monitor once for each device you're dealing with seems to be a pattern. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line
On Mon, 2018-08-20 at 16:45 +0800, Yi Min Zhao wrote: > 在 2018/8/17 上午12:31, Andrea Bolognani 写道: > > On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: > > > +virBufferAddLit(, "zpci"); > > > +virBufferAsprintf(, ",uid=%u", dev->addr.pci.zpci->zpci_uid); > > > +virBufferAsprintf(, ",fid=%u", dev->addr.pci.zpci->zpci_fid); > > > +virBufferAsprintf(, ",target=%s", dev->alias); > > > +virBufferAsprintf(, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid); > > > > Just making sure: is the uid a good choice for naming the zpci > > device? I would perhaps expect the fid to be in there as well, > > but since both have to be unique (right?) I guess it doesn't > > really matter... > > Either uid or fid, the value must be unique. But uid is defined by user. > Of course, we also give the permission to define fid. But uid is more > appropriate. So which is unique: uid, fid, or the combination of the two? Could I have -device zpci,uid=1,fid=1 -device zpci,uid=1,fid=2 or would that not work? What about -device zpci,uid=1,fid=1 -device zpci,uid=2,fid=1 would that be okay? > > > +int > > > +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info) > > > > Based on the name, this is a predicate: it should return a > > boolean value rather than an int. > > 0 means it's not zpci. 1 means it's zpci. -1 means error. My point is that a funtion called fooIsBaz() should only ever check whether the foo in question is indeed a baz, without taking any other action such as reporting errors. Leave that to the caller, or give the function a different name. > > Either way, calling a predicate should never result in an error > > being reported, so you need to move this check somewhere else. > > Acutally I have found out a proper place to add this check for a long time. > So this is the best place to add, at least I think for now. qemuDomainDeviceDefValidate() looks like a reasonable entry point for checks such as "you specified a zPCI address but this is not an s390 guest" or "your configuration requires zPCI but the QEMU binary doesn't support that feature". Both of the cases above should have proper test suite coverage, by the way. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 06/12] conf: Introduce address caching for PCI extensions
On Mon, 2018-08-20 at 16:32 +0800, Yi Min Zhao wrote: > 在 2018/8/16 下午11:03, Andrea Bolognani 写道: > > I haven't looked into the hash table handling in detail but I > > wonder if it's really necessary? IIUC you're using it just to > > mark which uids and fids have been already used by a device, > > which the PCI address allocation code does by setting bits > > inside integer variables - having a custom hash table for the > > same seems like overkill, and from the maintenance point of > > view it would be great to have the logic for PCI address and > > zPCI address allocation be similar unless diverging is strictly > > necessary. > > PCI address set uses array to store pci addresses' assignment. It doesn't > need too much memory because the buses are allocated dynamically, one > bus only has 32 slot, and it's a tree topology. But in zpci case, fid > and uid > are flat. FID is 32-bit so that we need a 4294967295 sized array. > Don't you think it's too large? Welp, I guess you're right. Disregard this comment then. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes
On Mon, 2018-08-20 at 16:19 +0800, Yi Min Zhao wrote:
> 在 2018/8/16 下午10:38, Andrea Bolognani 写道:
> > On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> > > +struct _virZPCIDeviceAddress {
> > > +unsigned int zpci_fid;
> > > +unsigned int zpci_uid;
> > > +bool fid_assigned;
> > > +bool uid_assigned;
> > > +};
> >
> > A couple of questions about the approach here, one of which I have
> > mentioned already and one of which I probably haven't (my bad):
> >
> >* do you really need to have separate booleans tracking whether
> > or not either id has been assigned? Wouldn't the same approach
> > as virPCIDeviceAddressIsEmpty() work, eg. consider the address
> > absent if both are zero and present otherwise?
>
> It's OK for uid. But for fid, zero is a valid value, so we need a bool
> to track its assignment.
See virPCIDeviceAddressIsEmpty() and virPCIDeviceAddressIsValid(),
which have very similar requirement but don't use extra booleans
to keep track of state.
You could do the same thing those functions do:
* the zPCI address is empty if both uid and fid are zero;
* the zPCI address is invalid if it's empty or uid is too large.
You already have the latter covered, so it's only a matter of
implementing the former.
> If we add a boolean for fid, why not also add another one for uid to
> keep consistency?
> This also makes code easy to read and obvious. It doesn't waste much
> memory space.
I think it actually makes more complicated, which is why I'd rather
get rid of it :)
> >* especially if you don't need the additional booleans, would it
> > be preferable to just add the two ids to the existing struct
> > instead of declaring a new one that you'll have to allocate
> > and make sure it's not NULL before accessing it? Again, I seem
> > to remember Laine feeling somewhat strongly about the topic.
>
> For other platforms, is it OK to waste this unused memory (of course,
> it's little) ?
I believe adding a couple of unsigned ints is worth it if we can
get rid of all the checks on addr->zpci because of it.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 12/12] news: Update news for PCI address extension attributes
On Mon, 2018-08-20 at 11:39 +0800, Yi Min Zhao wrote: > 在 2018/8/18 上午12:21, Andrea Bolognani 写道: > > On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: > > [...] > > > + > > > + > > > + qemu: Added support for PCI device passthrough on S390 > > > + > > > + > > > + The new zPCI attributes uid (user-defined identifier) > > > + and fid (PCI function identifier) of the S390 platform > > > + extend the PCI device address to support the PCI > > > + passthrough on S390. > > > + > > > + > > > > This doesn't look entirely accurate: the attributes are not only > > used for assigned devices but for emulated devices as well, so > > the text should reflect that. > > How about: > > *** to support the PCI functionality on S390. Perhaps something like qemu: Added support for PCI devices on s390 PCI addresses can now include the new uid (user-defined identifier) and fid (PCI function identifier) attributes, which make the corresponding devices usable by s390 guests. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] spec: Add support for RISC-V (riscv64)
On Mon, 2018-08-20 at 09:59 +0100, Richard W.M. Jones wrote: > From: David Abdurachmanov We require a Signed-off-by for libvirt contributions these days, so we can't merge this patch unless it's provided. [...] > # Numactl is not available on s390[x] and ARM Comment should be updated to numactl is not available on some architectures [...] > # numad is used to manage the CPU and memory placement dynamically, > # it's not available on s390[x] and ARM. Same as above. Everything else looks good. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: domain: validate memory access during validate domain config
commit 6534b3c4 try to raise an error when there is no numa nodes but
set access='shared' in domain config. In that commit, we add a memory access
validate function for memory device, but this check is not related to memory
device and won't work if there is no memory device in guest xml.
Move the memory access related check in the domain config validate function,
and simplify the unit test xml to avoid other error.
https://bugzilla.redhat.com/show_bug.cgi?id=1448149#c14
Signed-off-by: Luyao Huang
---
src/qemu/qemu_domain.c | 55 +++---
tests/qemuxml2argvdata/hugepages-memaccess3.xml | 62 +
tests/qemuxml2argvtest.c| 6 +--
3 files changed, 32 insertions(+), 91 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f570081..f0c461b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3888,6 +3888,29 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
static int
+qemuDomainDefValidateMemory(const virDomainDef *def)
+{
+const long system_page_size = virGetSystemPageSizeKB();
+
+/* We can't guarantee any other mem.access
+ * if no guest NUMA nodes are defined. */
+if (def->mem.nhugepages != 0 &&
+def->mem.hugepages[0].size != system_page_size &&
+virDomainNumaGetNodeCount(def->numa) == 0 &&
+def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT &&
+def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("memory access mode '%s' not supported "
+ "without guest numa node"),
+ virDomainMemoryAccessTypeToString(def->mem.access));
+return -1;
+}
+
+return 0;
+}
+
+
+static int
qemuDomainDefValidate(const virDomainDef *def,
virCapsPtr caps ATTRIBUTE_UNUSED,
void *opaque)
@@ -4009,6 +4032,9 @@ qemuDomainDefValidate(const virDomainDef *def,
if (qemuDomainDefValidateFeatures(def, qemuCaps) < 0)
goto cleanup;
+if (qemuDomainDefValidateMemory(def) < 0)
+goto cleanup;
+
ret = 0;
cleanup:
@@ -5531,30 +5557,6 @@ qemuDomainDeviceDefValidateController(const
virDomainControllerDef *controller,
static int
-qemuDomainDeviceDefValidateMemory(const virDomainMemoryDef *memory
ATTRIBUTE_UNUSED,
- const virDomainDef *def)
-{
-const long system_page_size = virGetSystemPageSizeKB();
-
-/* We can't guarantee any other mem.access
- * if no guest NUMA nodes are defined. */
-if (def->mem.nhugepages != 0 &&
-def->mem.hugepages[0].size != system_page_size &&
-virDomainNumaGetNodeCount(def->numa) == 0 &&
-def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT &&
-def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("memory access mode '%s' not supported "
- "without guest numa node"),
- virDomainMemoryAccessTypeToString(def->mem.access));
-return -1;
-}
-
-return 0;
-}
-
-
-static int
qemuDomainDeviceDefValidateVsock(const virDomainVsockDef *vsock,
const virDomainDef *def,
virQEMUCapsPtr qemuCaps)
@@ -5712,10 +5714,6 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef
*dev,
qemuCaps);
break;
-case VIR_DOMAIN_DEVICE_MEMORY:
-ret = qemuDomainDeviceDefValidateMemory(dev->data.memory, def);
-break;
-
case VIR_DOMAIN_DEVICE_VSOCK:
ret = qemuDomainDeviceDefValidateVsock(dev->data.vsock, def, qemuCaps);
break;
@@ -5737,6 +5735,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
case VIR_DOMAIN_DEVICE_MEMBALLOON:
case VIR_DOMAIN_DEVICE_NVRAM:
case VIR_DOMAIN_DEVICE_SHMEM:
+case VIR_DOMAIN_DEVICE_MEMORY:
case VIR_DOMAIN_DEVICE_PANIC:
case VIR_DOMAIN_DEVICE_IOMMU:
case VIR_DOMAIN_DEVICE_NONE:
diff --git a/tests/qemuxml2argvdata/hugepages-memaccess3.xml
b/tests/qemuxml2argvdata/hugepages-memaccess3.xml
index 8ec38d8..43c3ec6 100644
--- a/tests/qemuxml2argvdata/hugepages-memaccess3.xml
+++ b/tests/qemuxml2argvdata/hugepages-memaccess3.xml
@@ -12,76 +12,18 @@
hvm
-
-
-
-
-
-
-
-
-
-
destroy
restart
destroy
-
-
-
-
/usr/bin/qemu-system-x86_64
-
-
-
-
-
-
-
-
-
-
+
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
+
diff --git
Re: [libvirt] [PATCH v3 07/12] conf: Introduce parser, formatter for uid and fid
在 2018/8/16 下午11:14, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
+static int
+virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
+{
+if (zpci->uid_assigned &&
+(zpci->zpci_uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID ||
+ zpci->zpci_uid == 0)) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid PCI address uid='0x%x', "
+ "must be > 0x0 and <= 0x%x"),
+ zpci->zpci_uid,
+ VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
+return 0;
+}
+
+if (zpci->fid_assigned) {
+/* We don't need to check fid because fid covers
+ * all range of uint32 type.
+ */
+return 1;
+}
This branch is pointless, just drop it (but leave the comment).
[...]
@@ -37,6 +37,9 @@ typedef virPCIDeviceAddress *virPCIDeviceAddressPtr;
typedef struct _virPCIDeviceList virPCIDeviceList;
typedef virPCIDeviceList *virPCIDeviceListPtr;
+# define VIR_DOMAIN_DEVICE_ZPCI_MAX_UID UINT16_MAX
+# define VIR_DOMAIN_DEVICE_ZPCI_MAX_FID UINT32_MAX
A single space between the name and the value will do.
This should be
DO_TEST("disk-virtio-s390-zpci",
QEMU_CAPS_DEVICE_ZPCI,
QEMU_CAPS_CCW,
QEMU_CAPS_VIRTIO_S390);
Same later.
The rest looks good.
OK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [sandbox PATCH 2/2] Remove all uses of g_type_class_add_private()
On Mon, Aug 20, 2018 at 10:15:06AM +0200, Andrea Bolognani wrote: > The API has been deprecated, which causes build failures > on Fedora Rawhide; use the G_DEFINE_TYPE_WITH_PRIVATE() > macro instead. > > This requires bumping the minimum GLib version to 2.38. > > Signed-off-by: Andrea Bolognani > --- > configure.ac | 2 +- > libvirt-sandbox/libvirt-sandbox-builder-container.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-builder-initrd.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-builder-machine.c| 4 +--- > libvirt-sandbox/libvirt-sandbox-builder.c| 4 +--- > libvirt-sandbox/libvirt-sandbox-config-disk.c| 4 +--- > libvirt-sandbox/libvirt-sandbox-config-initrd.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-config-interactive.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-config-mount-file.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-config-mount-guest-bind.c| 5 ++--- > libvirt-sandbox/libvirt-sandbox-config-mount-host-bind.c | 5 ++--- > libvirt-sandbox/libvirt-sandbox-config-mount-host-image.c| 4 +--- > libvirt-sandbox/libvirt-sandbox-config-mount-ram.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-config-mount.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-config-network-address.c | 4 +--- > .../libvirt-sandbox-config-network-filterref-parameter.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-config-network-filterref.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-config-network-route.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-config-network.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-config-service-generic.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-config-service-systemd.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-config-service.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-config.c | 4 +--- > libvirt-sandbox/libvirt-sandbox-console-raw.c| 4 +--- > libvirt-sandbox/libvirt-sandbox-console-rpc.c| 4 +--- > libvirt-sandbox/libvirt-sandbox-console.c| 4 +--- > libvirt-sandbox/libvirt-sandbox-context-interactive.c| 4 +--- > libvirt-sandbox/libvirt-sandbox-context-service.c| 4 +--- > libvirt-sandbox/libvirt-sandbox-context.c| 4 +--- > 29 files changed, 31 insertions(+), 85 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [sandbox PATCH 1/2] configure: Use a single required version for GLib
On Mon, Aug 20, 2018 at 10:15:05AM +0200, Andrea Bolognani wrote: > Both GObject and GIO are part of GLib, so there's no > point in requiring different versions of the two. > > Signed-off-by: Andrea Bolognani > --- > configure.ac | 7 +++ > 1 file changed, 3 insertions(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 5/7] lock_driver_sanlock: Handle metadata flag gracefully
On 08/20/2018 09:25 AM, Michal Prívozník wrote:
> On 08/17/2018 04:49 PM, John Ferlan wrote:
>>
>>
>> On 08/14/2018 07:19 AM, Michal Privoznik wrote:
>>> No real support implemented here. But hey, at least we will not
>>> fail.
>>>
>>> Signed-off-by: Michal Privoznik
>>> ---
>>> src/locking/lock_driver_sanlock.c | 25 ++---
>>> 1 file changed, 18 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/locking/lock_driver_sanlock.c
>>> b/src/locking/lock_driver_sanlock.c
>>> index 3e5f0e37b0..c1996fb937 100644
>>> --- a/src/locking/lock_driver_sanlock.c
>>> +++ b/src/locking/lock_driver_sanlock.c
>>> @@ -791,7 +791,8 @@ static int
>>> virLockManagerSanlockAddResource(virLockManagerPtr lock,
>>> virLockManagerSanlockPrivatePtr priv = lock->privateData;
>>>
>>> virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY |
>>> - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1);
>>> + VIR_LOCK_MANAGER_RESOURCE_SHARED |
>>> + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);
>>>
>>> if (priv->res_count == SANLK_MAX_RESOURCES) {
>>> virReportError(VIR_ERR_INTERNAL_ERROR,
>>> @@ -804,6 +805,11 @@ static int
>>> virLockManagerSanlockAddResource(virLockManagerPtr lock,
>>> if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
>>> return 0;
>>>
>>> +/* No metadata locking support for now.
>>> + * TODO: implement it. */
>>> +if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA)
>>> +return 0;
>>> +
>>
>> Doesn't this give someone the false impression that their resource is
>> locked if they choose METADATA?
>
> Okay, I'll provide some implementation. Even though sanlock is not
> really my cup of coffee :-)
Actually, sanlock is doomed. It allows us to set the initial offset
(with posing some weird restrictions on it) and it doesn't allow us to
specify length we want to lock (it looks like it locks whole disk
sector). Therefore we can't lock offsets 0 and 1 independently. D'oh!
Maybe we could lock offsets 0 and 1MB but I'm unsure how sanlock behaves
when offset is outside of the file (we can't safely assume that every
file we will try to lock is at least 1MB big, can we? No we can't!
OVMF_VARS is only 128KB long).
https://linux.die.net/man/8/sanlock and scroll down to offsets.
Therefore I rather stick with my dummy implementation and have somebody
else look at this. Somebody who understands how sanlock works.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] spec: Add support for RISC-V (riscv64)
From: David Abdurachmanov
---
libvirt.spec.in | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index e7196b7d3b..5cb995a6a4 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -101,7 +101,7 @@
%endif
# Numactl is not available on s390[x] and ARM
-%ifarch s390 s390x %{arm}
+%ifarch s390 s390x %{arm} riscv64
%define with_numactl 0
%endif
@@ -120,7 +120,7 @@
%endif
# zfs-fuse is not available on some architectures
-%ifarch s390 s390x aarch64
+%ifarch s390 s390x aarch64 riscv64
%define with_storage_zfs 0
%endif
@@ -195,7 +195,7 @@
%if %{with_qemu} || %{with_lxc} || %{with_uml}
# numad is used to manage the CPU and memory placement dynamically,
# it's not available on s390[x] and ARM.
-%ifnarch s390 s390x %{arm}
+%ifnarch s390 s390x %{arm} riscv64
%define with_numad0%{!?_without_numad:1}
%endif
%endif
--
2.18.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: adding domainGetHostname support for QEMU.
On Sat, Aug 18, 2018 at 02:18:17PM +0200, Peter Krempa wrote:
> On Fri, Aug 17, 2018 at 23:25:19 -0300, Julio Faracco wrote:
> > This commit add the support to use the function qemuAgentGetHostname() for
> > obtain the domain hostname using QEMU-GA command.
> >
> > Signed-off-by: Julio Faracco
> > ---
> > src/qemu/qemu_driver.c | 41 +
> > 1 file changed, 41 insertions(+)
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index d4a2379e48..9a6614fbc2 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -19309,6 +19309,46 @@ qemuConnectGetCPUModelNames(virConnectPtr conn,
> > return virCPUGetModels(arch, models);
> > }
> >
> > +static char *
> > +qemuDomainGetHostname(virDomainPtr dom,
> > + unsigned int flags)
> > +{
> > +int rv = -1;
> > +virQEMUDriverPtr driver = dom->conn->privateData;
> > +virDomainObjPtr vm = NULL;
> > +qemuAgentPtr agent;
> > +char *hostname = NULL;
> > +
> > +virCheckFlags(0, hostname);
>
> Please return NULL directly.
>
> > +
> > +if (!(vm = qemuDomObjFromDomain(dom)))
> > +return NULL;
> > +
> > +if (virDomainGetTimeEnsureACL(dom->conn, vm->def) < 0)
>
> ACL checking functions are per-API, so this should use
> virDomainGetHostnameACL.
>
> I presume it's a copy-paste error.
>
> This is the wrong ACL checking function.
'make check' should have reported this, so make sure to run that.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] FYI: libvirt package on Fedora RISC-V
On Mon, Aug 20, 2018 at 10:40:42AM +0200, Andrea Bolognani wrote: > On Sun, 2018-08-19 at 19:46 +0100, Richard W.M. Jones wrote: > > On Sun, Aug 19, 2018 at 07:44:06PM +0100, Richard W.M. Jones wrote: > > > > > > http://fedora-riscv.tranquillity.se/koji/taskinfo?taskID=95457 > > Neat! > > > > There were some minor changes to the spec file: > > > > > > https://src.fedoraproject.org/rpms/libvirt/c/18f7b8c79c2876b2d3a6ebe2279fd6526321536b?branch=master > > > > I should of course say for clarity that David has only compiled the > > libvirt package so it can be used for other packages that BR it. > > The spec file changes look like they could be merged upstream > right away, can he (or you) submit them? Right, sorry, I forgot that we're maintaining the spec file in the libvirt sources. I'll post a patch soon. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 11/12] docs: Add 'uid' and 'fid' information
在 2018/8/18 上午12:19, Andrea Bolognani 写道: On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...] +have multiple functions used. (Since 4.7.0 +), PCI address extensions depending on the architecture +are supported. E.g. for S390 guests, PCI addresses have +additional attributes: uid (a hex value between +0x1 and 0x, inclusive), and fid (a hex value +between 0x0 and 0x, inclusive), which are used by PCI +devices on S390 for User-defined Identifiers and Function +Identifiers. I would format the minimum values for uid and fid as 0x0001 and 0x respectively for consistency - that's how they will show up in the domain XML after all. Reviewed-by: Andrea Bolognani Thanks! Have revised. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line
在 2018/8/17 上午12:31, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
+char *
+qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+virBufferAddLit(, "zpci");
+virBufferAsprintf(, ",uid=%u", dev->addr.pci.zpci->zpci_uid);
+virBufferAsprintf(, ",fid=%u", dev->addr.pci.zpci->zpci_fid);
+virBufferAsprintf(, ",target=%s", dev->alias);
+virBufferAsprintf(, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid);
Just making sure: is the uid a good choice for naming the zpci
device? I would perhaps expect the fid to be in there as well,
but since both have to be unique (right?) I guess it doesn't
really matter...
Either uid or fid, the value must be unique. But uid is defined by user.
Of course, we also give the permission to define fid. But uid is more
appropriate.
+
+if (virBufferCheckError() < 0) {
+virBufferFreeAndReset();
+return NULL;
+}
+
+return virBufferContentAndReset();
+}
+
+int
+qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)
Based on the name, this is a predicate: it should return a
boolean value rather than an int.
0 means it's not zpci. 1 means it's zpci. -1 means error.
+{
+if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
+return 1;
+}
+
+if (info->addr.pci.zpci) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("This QEMU doesn't support zpci devices"));
+return -1;
+}
Not sure about this second check. I guess the logic is supposed to
be that, when passed a "proper" zPCI device, the function would
have returned with the first check, and if we find a zPCI address
after that it's an error. Makes sense, but it's kinda obfuscated
and I'm not sure the error message is accurate: can it really only
be a problem of the QEMU binary not supporting the zPCI feature,
or could we have gotten here because the user is trying to assing
zPCI addresses to devices that don't support them?
Yes, it's because user could define zpci address in xml but
there's no zpci support.
Either way, calling a predicate should never result in an error
being reported, so you need to move this check somewhere else.
Acutally I have found out a proper place to add this check for a long time.
So this is the best place to add, at least I think for now.
[...]
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml
@@ -0,0 +1,18 @@
+
+ QEMUGuest1
+ c7a5fdbd-edaf-9455-926a-d65c16db1809
+ 219100
+
+hvm
+
+
+/usr/bin/qemu-system-s390x
+
+
+
+
+
+
+
+
+
This test case would have been a great addition to the previous
commit, where you actually introduced the ability to automatically
allocate zPCI addresses...
Another thing that I forgot to ask earlier and this is as good a
time as any: once zPCI support has been merged, will users have
to opt-in to it using
or will they get zPCI devices by default? And if so, will it still
be possible for them to get CCW devices instead if wanted?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] FYI: libvirt package on Fedora RISC-V
On Sun, 2018-08-19 at 19:46 +0100, Richard W.M. Jones wrote: > On Sun, Aug 19, 2018 at 07:44:06PM +0100, Richard W.M. Jones wrote: > > > > http://fedora-riscv.tranquillity.se/koji/taskinfo?taskID=95457 Neat! > > There were some minor changes to the spec file: > > > > https://src.fedoraproject.org/rpms/libvirt/c/18f7b8c79c2876b2d3a6ebe2279fd6526321536b?branch=master > > I should of course say for clarity that David has only compiled the > libvirt package so it can be used for other packages that BR it. The spec file changes look like they could be merged upstream right away, can he (or you) submit them? > This > does not include any of the proposed patches to add support for > qemu-system-riscv64. Hopefully Lubomir will post a respin soon - the last version only had a few minor issues, so I'd expect the next one to be reviewed and merged relatively quickly. > And there's no hardware virt support on any > existing RISC-V systems. Oh well, that will come in time I guess :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 06/12] conf: Introduce address caching for PCI extensions
在 2018/8/16 下午11:03, Andrea Bolognani 写道: On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: This patch provides a caching mechanism for the device address extensions uid and fid on S390. For efficient sparse address allocation, we introduce two hash tables for uid/fid which hold the address set information per domain. Also in order to improve performance of searching available value, we introduce our own callbacks for the two hashtables. In this way, uid/fid is saved in hash key and hash value could be any non-NULL pointer due to no operation on hash value. That is also the reason why we don't introduce hash value free callback. Signed-off-by: Yi Min Zhao Reviewed-by: Boris Fiuczynski Reviewed-by: Bjoern Walk Reviewed-by: Ján Tomko --- src/conf/domain_addr.c | 85 ++ src/conf/domain_addr.h | 9 + src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 7 4 files changed, 102 insertions(+) I haven't looked into the hash table handling in detail but I wonder if it's really necessary? IIUC you're using it just to mark which uids and fids have been already used by a device, which the PCI address allocation code does by setting bits inside integer variables - having a custom hash table for the same seems like overkill, and from the maintenance point of view it would be great to have the logic for PCI address and zPCI address allocation be similar unless diverging is strictly necessary. PCI address set uses array to store pci addresses' assignment. It doesn't need too much memory because the buses are allocated dynamically, one bus only has 32 slot, and it's a tree topology. But in zpci case, fid and uid are flat. FID is 32-bit so that we need a 4294967295 sized array. Don't you think it's too large? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 03/12] conf: Introduce a new PCI address extension flag
在 2018/8/16 下午10:44, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
+qemuDomainDeviceSupportZPCI(virDomainDeviceDefPtr device)
+{
+switch ((virDomainDeviceType) device->type) {
+case VIR_DOMAIN_DEVICE_CHR:
+return false;
+
+case VIR_DOMAIN_DEVICE_CONTROLLER:
+case VIR_DOMAIN_DEVICE_DISK:
+case VIR_DOMAIN_DEVICE_LEASE:
+case VIR_DOMAIN_DEVICE_FS:
+case VIR_DOMAIN_DEVICE_NET:
+case VIR_DOMAIN_DEVICE_INPUT:
+case VIR_DOMAIN_DEVICE_SOUND:
+case VIR_DOMAIN_DEVICE_VIDEO:
+case VIR_DOMAIN_DEVICE_HOSTDEV:
+case VIR_DOMAIN_DEVICE_WATCHDOG:
+case VIR_DOMAIN_DEVICE_GRAPHICS:
+case VIR_DOMAIN_DEVICE_HUB:
+case VIR_DOMAIN_DEVICE_REDIRDEV:
+case VIR_DOMAIN_DEVICE_SMARTCARD:
+case VIR_DOMAIN_DEVICE_MEMBALLOON:
+case VIR_DOMAIN_DEVICE_NVRAM:
+case VIR_DOMAIN_DEVICE_RNG:
+case VIR_DOMAIN_DEVICE_SHMEM:
+case VIR_DOMAIN_DEVICE_TPM:
+case VIR_DOMAIN_DEVICE_PANIC:
+case VIR_DOMAIN_DEVICE_MEMORY:
+case VIR_DOMAIN_DEVICE_IOMMU:
+case VIR_DOMAIN_DEVICE_VSOCK:
+break;
+
+case VIR_DOMAIN_DEVICE_NONE:
+case VIR_DOMAIN_DEVICE_LAST:
Missing 'default' case.
+virReportEnumRangeError(virDomainDeviceType, device->type);
+return false;
+}
Add an empty line here.
+return true;
+}
[...]
+static int
+qemuDomainFillDevicePCIExtensionFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceDefPtr dev,
+ virDomainDeviceInfoPtr info,
+ void *opaque)
+{
+virQEMUCapsPtr qemuCaps = opaque;
+
+info->pciAddressExtFlags
+= qemuDomainDeviceCalculatePCIAddressExtensionFlags(qemuCaps, dev);
Add an empty line here.
+return 0;
+}
Reviewed-by: Andrea Bolognani
Thanks!
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 04/12] qemu: Enable PCI multi bus for S390 guests
在 2018/8/16 下午10:45, Andrea Bolognani 写道: On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: QEMU on s390 supports PCI multibus since forever. Signed-off-by: Yi Min Zhao Reviewed-by: Boris Fiuczynski Reviewed-by: Stefan Zimmermann Reviewed-by: Bjoern Walk Reviewed-by: Ján Tomko --- src/qemu/qemu_capabilities.c | 4 1 file changed, 4 insertions(+) Reviewed-by: Andrea Bolognani Thanks! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 05/12] qemu: Auto add pci-root for s390/s390x guests
在 2018/8/16 下午10:52, Andrea Bolognani 写道: On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: The pci-root depends on zpci capability. So autogenerate pci-root if zpci exists. Signed-off-by: Yi Min Zhao Reviewed-by: Boris Fiuczynski Reviewed-by: Stefan Zimmermann Reviewed-by: Bjoern Walk Reviewed-by: Ján Tomko --- src/qemu/qemu_domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index de056272e8..a84e5f06b2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3227,6 +3227,7 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def, case VIR_ARCH_S390X: addDefaultUSB = false; addPanicDevice = true; +addPCIRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI); break; I wonder if you might want to do this, and other stuff, only if qemuDomainIsS390CCW()... It seems like that function is not used a lot, which might mean that someone is going to have to perform some serious cleaning up if there is ever another machine type on s390, or perhaps that it just doesn't quite apply here. If the latter, Reviewed-by: Andrea Bolognani Thanks! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes
在 2018/8/16 下午10:38, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
+typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress;
+typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
+struct _virZPCIDeviceAddress {
+unsigned int zpci_fid;
+unsigned int zpci_uid;
+bool fid_assigned;
+bool uid_assigned;
+};
A couple of questions about the approach here, one of which I have
mentioned already and one of which I probably haven't (my bad):
* do you really need to have separate booleans tracking whether
or not either id has been assigned? Wouldn't the same approach
as virPCIDeviceAddressIsEmpty() work, eg. consider the address
absent if both are zero and present otherwise?
* especially if you don't need the additional booleans, would it
be preferable to just add the two ids to the existing struct
instead of declaring a new one that you'll have to allocate
and make sure it's not NULL before accessing it? Again, I seem
to remember Laine feeling somewhat strongly about the topic.
Cosmetic issues:
Sorry, forgot this comment.
* uid should be listed before fid;
Sure.
* the zpci_ prefix is unnecessary if you have a separate struct
that contains "ZPCI" in the name. But see above :)
Let's see your response to another mail.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 02/12] qemu: Introduce zPCI capability
在 2018/8/16 下午10:39, Andrea Bolognani 写道: On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: Let's introduce zPCI capability. Signed-off-by: Yi Min Zhao Reviewed-by: Boris Fiuczynski Reviewed-by: Stefan Zimmermann Reviewed-by: Bjoern Walk Reviewed-by: Ján Tomko --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + 8 files changed, 9 insertions(+) Reviewed-by: Andrea Bolognani Thanks! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes
在 2018/8/16 下午10:38, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
+typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress;
+typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
+struct _virZPCIDeviceAddress {
+unsigned int zpci_fid;
+unsigned int zpci_uid;
+bool fid_assigned;
+bool uid_assigned;
+};
A couple of questions about the approach here, one of which I have
mentioned already and one of which I probably haven't (my bad):
* do you really need to have separate booleans tracking whether
or not either id has been assigned? Wouldn't the same approach
as virPCIDeviceAddressIsEmpty() work, eg. consider the address
absent if both are zero and present otherwise?
It's OK for uid. But for fid, zero is a valid value, so we need a bool
to track its assignment.
If we add a boolean for fid, why not also add another one for uid to
keep consistency?
This also makes code easy to read and obvious. It doesn't waste much
memory space.
* especially if you don't need the additional booleans, would it
be preferable to just add the two ids to the existing struct
instead of declaring a new one that you'll have to allocate
and make sure it's not NULL before accessing it? Again, I seem
to remember Laine feeling somewhat strongly about the topic.
For other platforms, is it OK to waste this unused memory (of course,
it's little) ?
Cosmetic issues:
* uid should be listed before fid;
* the zpci_ prefix is unnecessary if you have a separate struct
that contains "ZPCI" in the name. But see above :)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [sandbox PATCH 2/2] Remove all uses of g_type_class_add_private()
The API has been deprecated, which causes build failures
on Fedora Rawhide; use the G_DEFINE_TYPE_WITH_PRIVATE()
macro instead.
This requires bumping the minimum GLib version to 2.38.
Signed-off-by: Andrea Bolognani
---
configure.ac | 2 +-
libvirt-sandbox/libvirt-sandbox-builder-container.c | 4 +---
libvirt-sandbox/libvirt-sandbox-builder-initrd.c | 4 +---
libvirt-sandbox/libvirt-sandbox-builder-machine.c| 4 +---
libvirt-sandbox/libvirt-sandbox-builder.c| 4 +---
libvirt-sandbox/libvirt-sandbox-config-disk.c| 4 +---
libvirt-sandbox/libvirt-sandbox-config-initrd.c | 4 +---
libvirt-sandbox/libvirt-sandbox-config-interactive.c | 4 +---
libvirt-sandbox/libvirt-sandbox-config-mount-file.c | 4 +---
libvirt-sandbox/libvirt-sandbox-config-mount-guest-bind.c| 5 ++---
libvirt-sandbox/libvirt-sandbox-config-mount-host-bind.c | 5 ++---
libvirt-sandbox/libvirt-sandbox-config-mount-host-image.c| 4 +---
libvirt-sandbox/libvirt-sandbox-config-mount-ram.c | 4 +---
libvirt-sandbox/libvirt-sandbox-config-mount.c | 4 +---
libvirt-sandbox/libvirt-sandbox-config-network-address.c | 4 +---
.../libvirt-sandbox-config-network-filterref-parameter.c | 4 +---
libvirt-sandbox/libvirt-sandbox-config-network-filterref.c | 4 +---
libvirt-sandbox/libvirt-sandbox-config-network-route.c | 4 +---
libvirt-sandbox/libvirt-sandbox-config-network.c | 4 +---
libvirt-sandbox/libvirt-sandbox-config-service-generic.c | 4 +---
libvirt-sandbox/libvirt-sandbox-config-service-systemd.c | 4 +---
libvirt-sandbox/libvirt-sandbox-config-service.c | 4 +---
libvirt-sandbox/libvirt-sandbox-config.c | 4 +---
libvirt-sandbox/libvirt-sandbox-console-raw.c| 4 +---
libvirt-sandbox/libvirt-sandbox-console-rpc.c| 4 +---
libvirt-sandbox/libvirt-sandbox-console.c| 4 +---
libvirt-sandbox/libvirt-sandbox-context-interactive.c| 4 +---
libvirt-sandbox/libvirt-sandbox-context-service.c| 4 +---
libvirt-sandbox/libvirt-sandbox-context.c| 4 +---
29 files changed, 31 insertions(+), 85 deletions(-)
diff --git a/configure.ac b/configure.ac
index d2e2636..42b6538 100644
--- a/configure.ac
+++ b/configure.ac
@@ -10,7 +10,7 @@ AC_CANONICAL_HOST
AM_SILENT_RULES([yes])
-GLIB_REQUIRED=2.32.0
+GLIB_REQUIRED=2.38.0
LIBVIRT_REQUIRED=1.0.2
LIBVIRT_GCONFIG_REQUIRED=0.2.1
LIBVIRT_GLIB_REQUIRED=0.2.2
diff --git a/libvirt-sandbox/libvirt-sandbox-builder-container.c
b/libvirt-sandbox/libvirt-sandbox-builder-container.c
index 66e1fc6..a4b00e3 100644
--- a/libvirt-sandbox/libvirt-sandbox-builder-container.c
+++ b/libvirt-sandbox/libvirt-sandbox-builder-container.c
@@ -48,7 +48,7 @@ struct _GVirSandboxBuilderContainerPrivate
gboolean unused;
};
-G_DEFINE_TYPE(GVirSandboxBuilderContainer, gvir_sandbox_builder_container,
GVIR_SANDBOX_TYPE_BUILDER);
+G_DEFINE_TYPE_WITH_PRIVATE(GVirSandboxBuilderContainer,
gvir_sandbox_builder_container, GVIR_SANDBOX_TYPE_BUILDER);
enum {
@@ -473,8 +473,6 @@ static void
gvir_sandbox_builder_container_class_init(GVirSandboxBuilderContaine
builder_class->construct_devices =
gvir_sandbox_builder_container_construct_devices;
builder_class->get_disk_prefix =
gvir_sandbox_builder_container_get_disk_prefix;
builder_class->get_files_to_copy =
gvir_sandbox_builder_container_get_files_to_copy;
-
-g_type_class_add_private(klass,
sizeof(GVirSandboxBuilderContainerPrivate));
}
diff --git a/libvirt-sandbox/libvirt-sandbox-builder-initrd.c
b/libvirt-sandbox/libvirt-sandbox-builder-initrd.c
index 59a03e6..a550d4c 100644
--- a/libvirt-sandbox/libvirt-sandbox-builder-initrd.c
+++ b/libvirt-sandbox/libvirt-sandbox-builder-initrd.c
@@ -51,7 +51,7 @@ struct _GVirSandboxBuilderInitrdPrivate
gboolean unused;
};
-G_DEFINE_TYPE(GVirSandboxBuilderInitrd, gvir_sandbox_builder_initrd,
G_TYPE_OBJECT);
+G_DEFINE_TYPE_WITH_PRIVATE(GVirSandboxBuilderInitrd,
gvir_sandbox_builder_initrd, G_TYPE_OBJECT);
enum {
@@ -127,8 +127,6 @@ static void
gvir_sandbox_builder_initrd_class_init(GVirSandboxBuilderInitrdClass
object_class->finalize = gvir_sandbox_builder_initrd_finalize;
object_class->get_property = gvir_sandbox_builder_initrd_get_property;
object_class->set_property = gvir_sandbox_builder_initrd_set_property;
-
-g_type_class_add_private(klass, sizeof(GVirSandboxBuilderInitrdPrivate));
}
diff --git a/libvirt-sandbox/libvirt-sandbox-builder-machine.c
b/libvirt-sandbox/libvirt-sandbox-builder-machine.c
index b6f2218..a4a5dcd 100644
--- a/libvirt-sandbox/libvirt-sandbox-builder-machine.c
+++ b/libvirt-sandbox/libvirt-sandbox-builder-machine.c
@@ -50,7 +50,7 @@ struct _GVirSandboxBuilderMachinePrivate
gboolean unused;
};
[libvirt] [sandbox PATCH 1/2] configure: Use a single required version for GLib
Both GObject and GIO are part of GLib, so there's no point in requiring different versions of the two. Signed-off-by: Andrea Bolognani --- configure.ac | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/configure.ac b/configure.ac index f32c735..d2e2636 100644 --- a/configure.ac +++ b/configure.ac @@ -10,8 +10,7 @@ AC_CANONICAL_HOST AM_SILENT_RULES([yes]) -GIO_UNIX_REQUIRED=2.28.0 -GOBJECT_REQUIRED=2.32.0 +GLIB_REQUIRED=2.32.0 LIBVIRT_REQUIRED=1.0.2 LIBVIRT_GCONFIG_REQUIRED=0.2.1 LIBVIRT_GLIB_REQUIRED=0.2.2 @@ -74,8 +73,8 @@ AC_CONFIG_LIBOBJ_DIR([libvirt-sandbox]) LIBVIRT_SANDBOX_COMPILE_WARNINGS -PKG_CHECK_MODULES(GIO_UNIX, gio-unix-2.0 >= $GIO_UNIX_REQUIRED) -PKG_CHECK_MODULES(GOBJECT, gobject-2.0 >= $GOBJECT_REQUIRED) +PKG_CHECK_MODULES(GIO_UNIX, gio-unix-2.0 >= $GLIB_REQUIRED) +PKG_CHECK_MODULES(GOBJECT, gobject-2.0 >= $GLIB_REQUIRED) PKG_CHECK_MODULES(LIBVIRT, libvirt >= $LIBVIRT_REQUIRED) PKG_CHECK_MODULES(LIBVIRT_GLIB, libvirt-glib-1.0 >= $LIBVIRT_GOBJECT_REQUIRED) PKG_CHECK_MODULES(LIBVIRT_GOBJECT, libvirt-gobject-1.0 >= $LIBVIRT_GOBJECT_REQUIRED) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [sandbox PATCH 0/2] Remove all uses of g_type_class_add_private()
This fixes build on Fedora Rawhide. Andrea Bolognani (2): configure: Use a single required version for GLib Remove all uses of g_type_class_add_private() configure.ac | 7 +++ libvirt-sandbox/libvirt-sandbox-builder-container.c| 4 +--- libvirt-sandbox/libvirt-sandbox-builder-initrd.c | 4 +--- libvirt-sandbox/libvirt-sandbox-builder-machine.c | 4 +--- libvirt-sandbox/libvirt-sandbox-builder.c | 4 +--- libvirt-sandbox/libvirt-sandbox-config-disk.c | 4 +--- libvirt-sandbox/libvirt-sandbox-config-initrd.c| 4 +--- libvirt-sandbox/libvirt-sandbox-config-interactive.c | 4 +--- libvirt-sandbox/libvirt-sandbox-config-mount-file.c| 4 +--- libvirt-sandbox/libvirt-sandbox-config-mount-guest-bind.c | 5 ++--- libvirt-sandbox/libvirt-sandbox-config-mount-host-bind.c | 5 ++--- libvirt-sandbox/libvirt-sandbox-config-mount-host-image.c | 4 +--- libvirt-sandbox/libvirt-sandbox-config-mount-ram.c | 4 +--- libvirt-sandbox/libvirt-sandbox-config-mount.c | 4 +--- libvirt-sandbox/libvirt-sandbox-config-network-address.c | 4 +--- .../libvirt-sandbox-config-network-filterref-parameter.c | 4 +--- libvirt-sandbox/libvirt-sandbox-config-network-filterref.c | 4 +--- libvirt-sandbox/libvirt-sandbox-config-network-route.c | 4 +--- libvirt-sandbox/libvirt-sandbox-config-network.c | 4 +--- libvirt-sandbox/libvirt-sandbox-config-service-generic.c | 4 +--- libvirt-sandbox/libvirt-sandbox-config-service-systemd.c | 4 +--- libvirt-sandbox/libvirt-sandbox-config-service.c | 4 +--- libvirt-sandbox/libvirt-sandbox-config.c | 4 +--- libvirt-sandbox/libvirt-sandbox-console-raw.c | 4 +--- libvirt-sandbox/libvirt-sandbox-console-rpc.c | 4 +--- libvirt-sandbox/libvirt-sandbox-console.c | 4 +--- libvirt-sandbox/libvirt-sandbox-context-interactive.c | 4 +--- libvirt-sandbox/libvirt-sandbox-context-service.c | 4 +--- libvirt-sandbox/libvirt-sandbox-context.c | 4 +--- 29 files changed, 33 insertions(+), 88 deletions(-) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/12] qemu: Add hotpluging support for PCI devices on S390 guests
在 2018/8/18 上午12:10, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
This commit adds hotplug support for PCI devices on S390 guests.
There's no need to implement hot unplug for zPCI as QEMU implements
an unplug callback which will unplug both PCI and zPCI device in a
cascaded way.
It looks like you ended up implementing explicit hot unplug at
least for controllers. I think perhaps it would be a good idea
to implement it for all devices instead of relying on QEMU's
own unplug cascading so that we have more control over the whole
process.
It's different between controller and device. And we only hot unplug pci
controller, not for
all controller types. In addition, we only could do hot-unplug one time,
either zpci device
or corresponding pci device. It's due to Qemu logic. Qemu will
hot-unplug zpci device
automatically while doing hotplug pci device, and vice versa.
@@ -669,8 +737,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0)
goto exit_monitor;
-if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
+if (qemuDomainAttachExtensionDevice(priv->mon, >info) < 0)
+goto exit_monitor;
+
+if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
+ignore_value(qemuDomainDetachExtensionDevice(priv->mon, >info));
goto exit_monitor;
+}
if (qemuDomainObjExitMonitor(driver, vm) < 0) {
ret = -2;
So, I'm very much not familiar with the hotplug code and seeing
changes to stuff like qemuDomainAttachDiskGeneric() makes me a bit
uncomfortable :)
I can't spot anything obviously wrong in your changes, but I think
perhaps you might want to enter and exit the monitor separately
for the zpci device and for the virtio device? I'm not sure that's
useful at all, but network devices for example seems to follow
that pattern... It would be great if someone with more experience
in this area could provide a review.
We have to add zpci device firstly and add corresponding pci device
secondly.
Do you think it's redundant to call monitor twice to add two devices?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 0/2] Fix detection of slow guest shutdown
Ping - as there was no other review yet for the series except a copy-paste bug spotted (thanks Bjoern) On Tue, Aug 14, 2018 at 11:27 AM Christian Ehrhardt < christian.ehrha...@canonical.com> wrote: > Hi, > after a good discussion a few days ago in > https://www.redhat.com/archives/libvir-list/2018-August/msg00122.html > and a short lived but back then untested v2 in > https://www.redhat.com/archives/libvir-list/2018-August/msg00199.html > I finally get access to the right HW again and completed the series. > > Being finally retested and working I finally feel safe to submit without > a RFC prefix. I think this would be a great addition for a better handling > of guests with plenty of host devices passed through. > > With the new code in place I can shutdown systems that have 12, 16 or > even more hostdevs attached without getting into the "zombie" mode where > libvirt will forever consider the guest as "in shutdown" as it gave up > waiting too early because the signal zero still was able to reach it. > > Scaling examples (extracted with gdb): > 16 Devices: virProcessKillPainfullyDelay (pid=67096, force=true, > extradelay=32) > 12 Devices: virProcessKillPainfullyDelay (pid=68251, force=true, > extradelay=24) > > *Updates in v3* > - fixup some issues found in testing and code checks > > *Updates in v2* > - removed the "accept the lack of /proc/ as valid process removal" > approach due to valid concerns about reusing ressources. > - added a dynamic extra wait scaling with the amount of hostdevs > > Christian Ehrhardt (2): > process: wait longer on kill per assigned Hostdev > process: wait longer 5->30s on hard shutdown > > src/libvirt_private.syms | 1 + > src/qemu/qemu_process.c | 5 +++-- > src/util/virprocess.c| 20 > src/util/virprocess.h| 1 + > 4 files changed, 21 insertions(+), 6 deletions(-) > > -- > 2.17.1 > > -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/7] lock_driver.h: Introduce metadata flag
On 08/17/2018 12:24 AM, John Ferlan wrote:
>
>
> On 08/14/2018 07:19 AM, Michal Privoznik wrote:
>> Signed-off-by: Michal Privoznik
>> ---
>> src/locking/lock_driver.h | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
>> index 8b7521..7c8f79520a 100644
>> --- a/src/locking/lock_driver.h
>> +++ b/src/locking/lock_driver.h
>> @@ -56,6 +56,8 @@ typedef enum {
>> VIR_LOCK_MANAGER_RESOURCE_READONLY = (1 << 0),
>> /* The resource is assigned in shared, writable mode */
>> VIR_LOCK_MANAGER_RESOURCE_SHARED = (1 << 1),
>> +/* The resource is locked for metadata change */
>> +VIR_LOCK_MANAGER_RESOURCE_METADATA = (1 << 2),
>
> Does this work or make sense for lease type?
That's the thing. You're right, it doesn't make sense for lease type.
But on the level of RPC of virtlockd both leases and disks kind of blend
together. I mean, virtlockd is merely told to lock this or that file.
>
> Of course this adds something that would "conceivably" be available such
> that we may want to document it on
> https://libvirt.org/internals/locking.html, but then again that's so
> sparse it would take a bit of spelunking in the code to figure it out.
>
> Anyway, since the name itself seems reasonable to me,
>
> Reviewed-by: John Ferlan
>
> if you add a few more details around it related to disk vs. lease that'd
> be nice. If you update the docs eventually with all that you've learned,
> then that'd be great too!
I'm gonna save that to a follow up patch.
Thanks!
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/7] domain_lock: Implement metadata locking
On 08/17/2018 07:58 PM, John Ferlan wrote:
>
>
> On 08/14/2018 07:19 AM, Michal Privoznik wrote:
>> In order for our drivers to lock resources for metadata change we
>> need set of new APIs. Fortunately, we don't have to care about
>> every possible device a domain can have. We care only about those
>> which can live on a network filesystem and hence can be accessed
>> by multiple daemons at the same time. These devices are covered
>> in virDomainLockMetadataLock() and only a small fraction of
>> those can be hotplugged (covered in the rest of the introduced
>> APIs).
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/libvirt_private.syms | 8 ++
>> src/locking/domain_lock.c | 304
>> ++
>> src/locking/domain_lock.h | 28 +
>> 3 files changed, 317 insertions(+), 23 deletions(-)
>>
>
> There seems to be more than just what's described going on in this patch
> which could be split out
>
> 1. Use @def in virDomainLockManagerNew
>
> 2. Add @metadataonly to virDomainLockManagerNew and
> virDomainLockManagerAddImage
>
> 3. Introduce virDomainLockManagerAddMemory and
> virDomainLockManagerAddFile along with changes to
> virDomainLockManagerNew along with perhaps a few words why those files
> were chosen and what decisions led to their selection. If someone adds
> something in the future how would they know to add them to the list?
>
> 4. Various virDomainLockMetadata* API's.
>
D'oh! Okay, I'll split this.
>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index ca4a192a4a..720ae12301 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1284,6 +1284,14 @@ virDomainLockImageAttach;
>> virDomainLockImageDetach;
>> virDomainLockLeaseAttach;
>> virDomainLockLeaseDetach;
>> +virDomainLockMetadataDiskLock;
>> +virDomainLockMetadataDiskUnlock;
>> +virDomainLockMetadataImageLock;
>> +virDomainLockMetadataImageUnlock;
>> +virDomainLockMetadataLock;
>> +virDomainLockMetadataMemLock;
>> +virDomainLockMetadataMemUnlock;
>> +virDomainLockMetadataUnlock;
>> virDomainLockProcessInquire;
>> virDomainLockProcessPause;
>> virDomainLockProcessResume;
>> diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c
>> index 705b132457..19a097fb25 100644
>> --- a/src/locking/domain_lock.c
>> +++ b/src/locking/domain_lock.c
>> @@ -69,7 +69,8 @@ static int virDomainLockManagerAddLease(virLockManagerPtr
>> lock,
>>
>>
>> static int virDomainLockManagerAddImage(virLockManagerPtr lock,
>> -virStorageSourcePtr src)
>> +virStorageSourcePtr src,
>> +bool metadataOnly)
>> {
>> unsigned int diskFlags = 0;
>> int type = virStorageSourceGetActualType(src);
>> @@ -82,10 +83,14 @@ static int
>> virDomainLockManagerAddImage(virLockManagerPtr lock,
>>type == VIR_STORAGE_TYPE_DIR))
>> return 0;
>>
>> -if (src->readonly)
>> -diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY;
>> -if (src->shared)
>> -diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED;
>> +if (metadataOnly) {
>> +diskFlags = VIR_LOCK_MANAGER_RESOURCE_METADATA;
>> +} else {
>> +if (src->readonly)
>> +diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY;
>> +if (src->shared)
>> +diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED;
>> +}
>>
>> VIR_DEBUG("Add disk %s", src->path);
>> if (virLockManagerAddResource(lock,
>> @@ -101,13 +106,68 @@ static int
>> virDomainLockManagerAddImage(virLockManagerPtr lock,
>> }
>>
>>
>> +static int
>> +virDomainLockManagerAddMemory(virLockManagerPtr lock,
>> + const virDomainMemoryDef *mem)
>> +{
>> +const char *path = NULL;
>> +
>> +switch ((virDomainMemoryModel) mem->model) {
>> +case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
>> +path = mem->nvdimmPath;
>
> Why not flip flop the order of new helpers and just call/return
> virDomainLockManagerAddFile directly with mem->nvdimmPath
>
>> +break;
>> +
>> +case VIR_DOMAIN_MEMORY_MODEL_DIMM:
>> +case VIR_DOMAIN_MEMORY_MODEL_LAST:
>> +case VIR_DOMAIN_MEMORY_MODEL_NONE:
>> +break;
>> +}
>> +
>> +if (!path)
>> +return 0;
>
> And then just return 0 here. or just call it here since it returns 0 if
> @!file anyway.
Well, I think it's just a matter of preference though. My preference was
to have this future extensible. I mean, if we ever introduce new model
that we need to lock, this switch() would just get its path and the rest
is already handled. Whereas if I'd move AddResource() call right into
the switch() the call would be duplicated then.
>
>> +
>> +VIR_DEBUG("Adding memory %s", path);
>> +if (virLockManagerAddResource(lock,
>> + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK,
>> + path,
Re: [libvirt] [PATCH v2 4/7] lockd_driver_lockd: Implement metadata flag
On 08/17/2018 01:53 AM, John Ferlan wrote:
>
>
> On 08/14/2018 07:19 AM, Michal Privoznik wrote:
>> Signed-off-by: Michal Privoznik
>> ---
>> src/locking/lock_daemon_dispatch.c | 12 ++--
>> src/locking/lock_driver_lockd.c| 31 +--
>> src/locking/lock_driver_lockd.h| 1 +
>> 3 files changed, 32 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/locking/lock_daemon_dispatch.c
>> b/src/locking/lock_daemon_dispatch.c
>> index 10248ec0b5..c24571ceac 100644
>> --- a/src/locking/lock_daemon_dispatch.c
>> +++ b/src/locking/lock_daemon_dispatch.c
>> @@ -37,6 +37,9 @@ VIR_LOG_INIT("locking.lock_daemon_dispatch");
>>
>> #include "lock_daemon_dispatch_stubs.h"
>>
>> +#define DEFAULT_OFFSET 0
>> +#define METADATA_OFFSET 1
>> +
>
> Curious, does this lead to the prospect of being able to acquire/use
> offset==0 length==1 and offset==1 length==1 at least conceptually and
> granularity wise?
Yes, this is exactly the purpose. I've taken inspiration from qemu. They
lock bytes from 100 to 100 + N, depending on what operations they want
the disk for (checkout raw_apply_lock_bytes() form block/file-posix.c).
And my idea was for virtlockd to do the same. Currently, it locks 0th
byte of given file and with metadata flag it would lock the first one.
>
> Related or not, there's a strange NFSv3 collision between QEMU and NFS
> related to some sort of fcntl locking granularity. More details that you
> possibly want to read at:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1592582
> https://bugzilla.redhat.com/show_bug.cgi?id=1547095
>
> I only note it because well it was on my 'short term history' radar and
> suffice to say it's an ugly and not fun issue dealing with these locks.
Oh yeah, NFSv3 and file locks. Well, I'm not making our situation any
worse here. Qemu is already locking disks, and it is doing so for the
while lifetime of the domain whereas my patches would lock for only a
fraction of a second (well, as long as it takes for chown() and
setfilecon_raw() to run). It is very unlikely that connection to the
NFSv3 server will be lost right when metadata lock is held.
>
>> static int
>> virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server
>> ATTRIBUTE_UNUSED,
>> virNetServerClientPtr client,
>> @@ -50,13 +53,14 @@
>> virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server
>> ATTRIBUTE_UNU
>> virNetServerClientGetPrivateData(client);
>> virLockSpacePtr lockspace;
>> unsigned int newFlags;
>> -off_t start = 0;
>> +off_t start = DEFAULT_OFFSET;
>> off_t len = 1;
>>
>> virMutexLock(>lock);
>>
>> virCheckFlagsGoto(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED |
>> - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE,
>> cleanup);
>> + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE |
>> + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA, cleanup);
>>
>> if (priv->restricted) {
>> virReportError(VIR_ERR_OPERATION_DENIED, "%s",
>> @@ -82,6 +86,10 @@
>> virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server
>> ATTRIBUTE_UNU
>> newFlags |= VIR_LOCK_SPACE_ACQUIRE_SHARED;
>> if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE)
>> newFlags |= VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE;
>> +if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA) {
>> +start = METADATA_OFFSET;
>> +newFlags |= VIR_LOCK_SPACE_ACQUIRE_WAIT;
>> +}
>
> If this is documented in more detail, then it should be noted that a
> METADATA lock forces usage of the wait API's. I can only imagine
> someone, some day will ask for a wait w/ timeout to avoid waiting too long.
That should never happen (TM). The metadata lock is acquired only for
time that seclabels are set on some domain paths (and as we will see in
6/7 we are not going to lock all the paths). As soon as they are set the
lock is released.
>
>>
>> if (virLockSpaceAcquireResource(lockspace,
>> args->name,
>> diff --git a/src/locking/lock_driver_lockd.c
>> b/src/locking/lock_driver_lockd.c
>> index 957a963a7b..bd14ed8930 100644
>> --- a/src/locking/lock_driver_lockd.c
>> +++ b/src/locking/lock_driver_lockd.c
>> @@ -475,9 +475,11 @@ static int
>> virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>> bool autoCreate = false;
>>
>> virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY |
>> - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1);
>> + VIR_LOCK_MANAGER_RESOURCE_SHARED |
>> + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);
>>
>> -if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
>> +if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY &&
>> +!(flags & VIR_LOCK_MANAGER_RESOURCE_METADATA))
>> return 0;
>
> Could someone pass READONLY & METADATA and not return 0
Re: [libvirt] [PATCH v2 5/7] lock_driver_sanlock: Handle metadata flag gracefully
On 08/17/2018 04:49 PM, John Ferlan wrote:
>
>
> On 08/14/2018 07:19 AM, Michal Privoznik wrote:
>> No real support implemented here. But hey, at least we will not
>> fail.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/locking/lock_driver_sanlock.c | 25 ++---
>> 1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/locking/lock_driver_sanlock.c
>> b/src/locking/lock_driver_sanlock.c
>> index 3e5f0e37b0..c1996fb937 100644
>> --- a/src/locking/lock_driver_sanlock.c
>> +++ b/src/locking/lock_driver_sanlock.c
>> @@ -791,7 +791,8 @@ static int
>> virLockManagerSanlockAddResource(virLockManagerPtr lock,
>> virLockManagerSanlockPrivatePtr priv = lock->privateData;
>>
>> virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY |
>> - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1);
>> + VIR_LOCK_MANAGER_RESOURCE_SHARED |
>> + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);
>>
>> if (priv->res_count == SANLK_MAX_RESOURCES) {
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> @@ -804,6 +805,11 @@ static int
>> virLockManagerSanlockAddResource(virLockManagerPtr lock,
>> if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
>> return 0;
>>
>> +/* No metadata locking support for now.
>> + * TODO: implement it. */
>> +if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA)
>> +return 0;
>> +
>
> Doesn't this give someone the false impression that their resource is
> locked if they choose METADATA?
Okay, I'll provide some implementation. Even though sanlock is not
really my cup of coffee :-)
>
> Something doesn't feel right about that - giving the impression that
> it's supported and the consumer is protected, but when push comes to
> shove they aren't.
>
> I'd be inclined to believe that we may want to do nothing with/for
> sanlock allowing the virCheckFlags above take care of the consumer.
No we must not do this. Metadata locking is not something users can turn
on or off. Okay, they can turn it off when they disable all the locking
(i.e. comment out lock_manager in qemu.conf). But if they would have
sanlock turned on and wanted to start a domain the virCheckFlags() would
be triggered right away and starting a domain would be denied.
>
>> switch (type) {
>> case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
>> if (driver->autoDiskLease) {
>> @@ -953,12 +959,17 @@ static int
>> virLockManagerSanlockAcquire(virLockManagerPtr lock,
>> virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_RESTRICT |
>>VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY, -1);
>>
>> -if (priv->res_count == 0 &&
>> -priv->hasRWDisks &&
>> -driver->requireLeaseForDisks) {
>> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> - _("Read/write, exclusive access, disks were present,
>> but no leases specified"));
>> -return -1;
>> +if (priv->res_count == 0) {
>> +if (priv->hasRWDisks &&
>> +driver->requireLeaseForDisks) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("Read/write, exclusive access, disks were
>> present, but no leases specified"));
>> +return -1;
>> +}
>> +
>> +/* We are not handling METADATA flag yet. So no resources
>> + * case is no-op for now. */
>> +return 0;
>
> Similar comment to patch4 w/r/t resource type (e.g. disk or lease), but
> now at least it's more obvious to me that hasRWDisks means lock for disk
> vs. not.
>
> Still it's odd to think that returning 0, but not actually getting the
> lock is the "right" thing to do. "Theoretically", if AddResource failed,
> then would Acquire ever be called w/ this flag?
Sure it would. Look at virDomainLockProcessStart() for instance. It
calls virDomainLockManagerNew() which adds all the resources via
AddResource() and then calls Acquire().
>
>> }
>>
>> /* We only initialize 'sock' if we are in the real
>>
>
> BTW:
>
> Now that I think about them together...
>
> - lock_driver_lockd.h - extract from patch4 and merge to patch3. I
> suppose it could be separate too to keep mgmt vs. space separate, but
> they're related enough to keep them together. Perhaps this is where a
> few more "words" about usage expectations as part of the commit message.>
> - lock_daemon_dispatch.c - extract from patch4 and whether it goes
> before or after is one of those chicken/egg type quandaries. Keeping it
> with the fcntl/lockd as opposed to the sanlock implementation doesn't
> feel right (even though sanlock doesn't use any LOCK_SPACE symbols).
I rather keep the patches as they are now. It makes more sense to me
this way.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 7/7] qemu_security: Lock metadata while relabelling
On 08/17/2018 08:38 PM, John Ferlan wrote:
>
>
> On 08/14/2018 07:19 AM, Michal Privoznik wrote:
>> Fortunately, we have qemu wrappers so it's sufficient to put
>> lock/unlock call only there.
>>
>
> Kind of sparse... Maybe a few words related to what benefit locking the
> metadata provides. There is a danger in all this too if the unlock does
> fail since metadata locks are wait type locks any subsequent lock will wait.
>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/qemu/qemu_security.c | 107
>> +++
>> 1 file changed, 107 insertions(+)
>>
>> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
>> index af3be42854..527563947c 100644
>> --- a/src/qemu/qemu_security.c
>> +++ b/src/qemu/qemu_security.c
>> @@ -26,6 +26,7 @@
>> #include "qemu_domain.h"
>> #include "qemu_security.h"
>> #include "virlog.h"
>> +#include "locking/domain_lock.h"
>>
>> #define VIR_FROM_THIS VIR_FROM_QEMU
>>
>> @@ -39,6 +40,12 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>> {
>> int ret = -1;
>> qemuDomainObjPrivatePtr priv = vm->privateData;
>> +bool locked = false;
>> +
>> +if (virDomainLockMetadataLock(driver->lockManager, vm) < 0)
>> +goto cleanup;
>> +
>> +locked = true;
>>
>> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> virSecurityManagerTransactionStart(driver->securityManager) < 0)
>> @@ -55,9 +62,17 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>> vm->pid) < 0)
>> goto cleanup;
>>
>> +locked = false;
>
> Technically not true yet. Also , I think two such attempts with the
> second eliciting the VIR_WARN is perfectly reasonable (IOW: on failure
> of the following goto cleanup with locked == true, try again, and spew
> that message).
>
> This repeats a few times, but I'll note just once.
I don't think it is safe to unlock something twice if we've locked it
just once.
>
>> +
>> +if (virDomainLockMetadataUnlock(driver->lockManager, vm) < 0)
>> +goto cleanup;
>> +
>> ret = 0;
>> cleanup:
>> virSecurityManagerTransactionAbort(driver->securityManager);
>> +if (locked &&
>> +virDomainLockMetadataUnlock(driver->lockManager, vm) < 0)
>> +VIR_WARN("unable to release metadata lock");
>
> Should we add the @stdin_path and @vm->pid and/or vm->name here? It may
> help some day. Similar comments for other new VIR_WARN's, but the
> parameters change...
Ah, good point. Okay.
>
>> return ret;
>> }
>>
>> @@ -68,6 +83,10 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
>> bool migrated)
>> {
>> qemuDomainObjPrivatePtr priv = vm->privateData;
>> +bool unlock = true;
>> +
>> +if (virDomainLockMetadataLock(driver->lockManager, vm) < 0)
>> +unlock = false;
>>
>> /* In contrast to qemuSecuritySetAllLabel, do not use
>> * secdriver transactions here. This function is called from
>> @@ -79,6 +98,10 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
>>vm->def,
>>migrated,
>>priv->chardevStdioLogd);
>> +
>> +if (unlock &&
>> +virDomainLockMetadataUnlock(driver->lockManager, vm) < 0)
>> +VIR_WARN("unable to release metadata lock");
>> }
>>
>>
>> @@ -88,6 +111,12 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver,
>> virDomainDiskDefPtr disk)
>> {
>> int ret = -1;
>> +bool locked = false;
>> +
>> +if (virDomainLockMetadataDiskLock(driver->lockManager, vm, disk) < 0)
>> +goto cleanup;
>> +
>> +locked = true;
>>
>> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> virSecurityManagerTransactionStart(driver->securityManager) < 0)
>> @@ -103,9 +132,17 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver,
>> vm->pid) < 0)
>> goto cleanup;
>>
>> +locked = false;
>> +
>> +if (virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0)
>> +goto cleanup;
>> +
>> ret = 0;
>> cleanup:
>> virSecurityManagerTransactionAbort(driver->securityManager);
>> +if (locked &&
>> +virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0)
>> +VIR_WARN("unable to release disk metadata lock");
>> return ret;
>> }
>>
>> @@ -116,6 +153,12 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
>> virDomainDiskDefPtr disk)
>> {
>> int ret = -1;
>> +bool locked = false;
>> +
>> +if (virDomainLockMetadataDiskLock(driver->lockManager, vm, disk) < 0)
>> +goto cleanup;
>> +
>> +locked = true;
>>
>> if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> virSecurityManagerTransactionStart(driver->securityManager) < 0)
[libvirt] [PATCH] storage: fix the error message when encrypted raw volume resize
The vol-dumpxml shows the volume target format type as raw for
encrypted volumes. The error message when attempting to resize
with prealloc is confusing here.
Signed-off-by: Shivaprasad G Bhat
---
src/storage/storage_util.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 42a9b6abf0..2abedb24b0 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -2343,7 +2343,7 @@ virStorageBackendVolResizeLocal(virStoragePoolObjPtr pool,
} else if (vol->target.format == VIR_STORAGE_FILE_RAW &&
vol->target.encryption) {
if (pre_allocate) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("preallocate is only supported for raw "
+ _("preallocate is only supported for unencrypted
raw "
"type volume"));
return -1;
}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
