Re: [libvirt] [PATCH 00/16] Xen: remove xend config version
On 12/16/2015 03:11 AM, Ian Campbell wrote: > On Wed, 2015-12-16 at 09:45 +, Ian Campbell wrote: >> On Tue, 2015-12-15 at 14:32 -0700, Jim Fehlig wrote: >>> Hi All, >>> >>> Ian Campbell recently attempted [1] to fix and issue around >>> MAX_VIRT_VPUS >>> on ARM that required adding a new XEND_CONFIG_VERSION. After some >>> discussion [2] we decided to drop support for all of the old xend >>> config >>> versions and go with the version supported in Xen 4.0, since the xl >>> syntax >>> was originally based on (and intended to be compatible with) xm circa >>> that >>> point in time. >>> >>> This series removes all traces of xend config version from the >>> codebase, >>> essentially removing support for Xen 3.x. Hopefully I succeeding in >>> making >>> the rather large series reviewable. The series is also available on the >>> remove-xend-config-version branch of my libvirt github clone [2]. >> Wow, thanks for offering to take this over, I had no idea it would end up >> with so much yakk hair everywhere! > I've looked through it and the transformations look good to me: > > Acked-by: Ian Campbell Thanks for taking the time to look at all of these changes! > > There were a couple of references to xendConfigVersion remaining in > comments on src/xen/xen_driver.c. I'll remove "with xendConfigVersion >= 3" from those sentences in 13/16. > There was also po/* which I presume some sort of semiautomated update will > strip eventually. I think so. It is due to removal of translated error messages in src/xen/xend_internal.c. > > I'm not sure what to make of the references under docs/api_extension/. Those are old patches that serve as an example of implementing a new API in libvirt. AFAIK, no one expects those to be kept in sync with current code. > I tested this on ARM with "Xen: support maxvcpus in xm and xl config" on > top and it worked. Good to hear. Thanks for all the help! Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 4/4] qemu: Implement shared memory device hot-unplug
On 12/15/2015 08:39 PM, John Ferlan wrote: [...] +if (rc < 0) +return -1; + I know this is a copy of the RemoveRNGDevice; however, this code doesn't remove an 'obj'. In fact, if !shmem->server.enabled, then we don't enter the monitor at all. Thus the following event probably won't happen... I am not sure what your mean is ... i guess your mean the device remove event we get from qmp monitor won't happen ? we will get that event if qemu remove shmem device success, it should always happen if qemu really remove it and there is no bugs on qemu :) While reviewing I got lazy and didn't check the non hotplug case to how shmem is added to the vm, but the point I was trying to make is that "if (shmem->server.enabled)" fails (e.g. is false), then there is no "rc = qemuMonitorDelObject(priv->mon, objAlias);" call in this API (similar to RNG code), thus how does the following event get triggered? Even if the condition was true, does detaching the char dev cause the event to be triggered? I thought the event was related to the DelObject code, but I didn't go follow that code Oh, i see, event is not related to the object delete, i guess you have checked the code and know how it works now ;-) Thanks for your quick reply ! Luyao John [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: add reporting of vCPU wait time
On 12/10/2015 09:41 AM, Daniel P. Berrange wrote:
> The VIR_DOMAIN_STATS_VCPU flag to virDomainListGetStats
> enables reporting of stats about vCPUs. Currently we
> only report the cumulative CPU running time and the
> execution state.
>
> This adds reporting of the wait time - time the vCPU
> wants to run, but the host schedular has something else
scheduler ?
> running ahead of it.
>
> The data is reported per-vCPU eg
>
> $ virsh domstats --vcpu demo
> Domain: 'demo'
>vcpu.current=4
>vcpu.maximum=4
>vcpu.0.state=1
>vcpu.0.time=142000
>vcpu.0.wait=18403928
>vcpu.1.state=1
>vcpu.1.time=13000
>vcpu.1.wait=10612111
>vcpu.2.state=1
>vcpu.2.time=11000
>vcpu.2.wait=12759501
>vcpu.3.state=1
>vcpu.3.time=9000
>vcpu.3.wait=21825087
>
> In implementing this I notice our reporting of CPU execute
> time has very poor granularity, since we are getting it
> from /proc/$PID/stat. As a future enhancement we should
> prefer to get CPU execute time from /proc/$PID/schedstat
> or /proc/$PID/sched (if either exist on the running kernel)
Probably shouldn't lose this comment ;-) Maybe lift part of this as an
XXX in the qemuGetProcessInfo? Once it's there, the algorithm to split
and read lines could be made generic for the input required in order to
process the fields from sched
>
> Signed-off-by: Daniel P. Berrange
> ---
> src/qemu/qemu_driver.c | 100
> +++--
> 1 file changed, 96 insertions(+), 4 deletions(-)
>
Because usually these types of requests lead to more requests - should
the *cpuWait be the only member of some new private structure rather
than an unsigned long long *? Of course it makes the changes here that
much more complicated. Could always leave it for future work too.
wait_sum, iowait_sum, sum_sleep_runtime...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 783a7cd..5293294 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1361,6 +1361,80 @@ static char *qemuConnectGetCapabilities(virConnectPtr
> conn) {
>
>
> static int
> +qemuGetSchedInfo(unsigned long long *cpuWait,
> + pid_t pid, pid_t tid)
> +{
> +char *proc = NULL;
> +char *data = NULL;
> +char **lines = NULL;
> +size_t i;
> +int ret = -1;
> +double val;
> +
> +*cpuWait = 0;
> +
> +/* In general, we cannot assume pid_t fits in int; but /proc parsing
> + * is specific to Linux where int works fine. */
> +if (tid)
> +ret = virAsprintf(&proc, "/proc/%d/task/%d/sched", (int)pid,
> (int)tid);
> +else
> +ret = virAsprintf(&proc, "/proc/%d/sched", (int)pid);
> +if (ret < 0)
> +goto cleanup;
> +
> +/* The file is not guaranteed to exist (needs CONFIG_SCHED_DEBUG) */
> +if (access(proc, R_OK) < 0)
> +return 0;
Leaks 'proc'. So either go with "ret = 0; goto cleanup;" or
"VIR_FREE(proc); return 0;"
> +
> +if (virFileReadAll(proc, (1<<16), &data) < 0)
> +goto cleanup;
> +
> +lines = virStringSplit(data, "\n", 0);
> +if (!lines)
> +goto cleanup;
> +
> +for (i = 0; lines[i] != NULL; i++) {
> +const char *line = lines[i];
> +
> +/* Needs CONFIG_SCHEDSTATS. The second check
> + * is the old name the kernel used in past */
> +if (STRPREFIX(line, "se.statistics.wait_sum") ||
> +STRPREFIX(line, "se.wait_sum")) {
> +line = strchr(line, ':');
I have a recollection of some code which uses virStringSplit again in
order to get the cells and then ensures there's two fields. This works,
so it feels like overkill to call virStringSplit again, but you could.
Just a suggestion.
> +if (!line) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Missing separate in sched info '%s'"),
> + lines[i]);
> +goto cleanup;
> +}
> +line++;
> +while (*line == ' ') {
> +line++;
> +}
This passes syntax-check with the single line and {}? Also, I think
virSkipSpaces() is what should be used.
> +
> +if (virStrToDouble(line, NULL, &val) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to parse sched info value '%s'"),
> + line);
> +goto cleanup;
> +}
> +
> +*cpuWait = (unsigned long long)(val * 100);
> +break;
> +}
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +VIR_FREE(data);
> +VIR_FREE(proc);
> +VIR_FREE(lines);
> +return ret;
> +}
> +
> +
> +static int
> qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss,
> pid_t pid, int tid)
> {
> @@ -1424,6 +1498,7 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int
> *lastCpu, long *
Re: [libvirt] [PATCH] build: disable vbox on cygwin
On 10/14/2015 02:08 AM, Daniel P. Berrange wrote: > On Tue, Oct 13, 2015 at 03:17:27PM -0600, Eric Blake wrote: >> Cygwin cannot build the vbox driver yet: >> >> CC vbox/libvirt_driver_vbox_impl_la-vbox_glue.lo >> In file included from vbox/vbox_glue.c:27:0: >> vblox/vbox_XPCOMCGlue.c:63:3: error: #error "Port me" >> # error "Port me" >>^ >> In file included from vbox/vbox_XPCOMCGlue.c:45:0, >> from vbox/vbox_glue.c:27: >> vbox/vbox_XPCOMCGlue.c: In function 'tryLoadOne': >> vbox/vbox_XPCOMCGlue.c:98:46: error: 'DYNLIB_NAME' undeclared (first use in >> this function) >> if (virAsprintf(&name, "%s/%s", dir, DYNLIB_NAME) < 0) >>^ >> ./util/virstring.h:245:31: note: in definition of macro 'virAsprintf' >> strp, __VA_ARGS__) >> ^ >> >> Rather than trying to figure out how to get dynamic loading of >> vbox to work under cygwin (since I don't even have a working vbox >> setup to test whether it works), I'm going to be lazy and just >> default to not even trying vbox on cygwin. >> >> Signed-off-by: Eric Blake > > ACK Just realized I let two months go by without pushing this - whoops. Done now. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] LSN-2015-0004: CVE-2015-5313: ACL bypass using ../ to access beyond storage pool
Libvirt Security Notice: LSN-2015-0004 == Summary: ACL bypass using ../ to access beyond storage pool Reported on: 20151030 Published on: 20151211 Fixed on: 20151211 Reported by: Ossi Herrala Joonas Kuorilehto Patched by: Eric Blake See also: CVE-2015-5313, FICORA bug #876194 Description --- Various virStorageVol* API operate on user-supplied volume names by concatenating the volume name to the pool location. Note that the virStoragePoolListVolumes API, when used on a storage pool backed by a directory in a file system, will only list volumes immediately in that directory (there is no traversal into subdirectories). However, other APIs such as virStorageVolCreateXML were not checking if a potential volume name represented one of the volumes that could be returned by virStoragePoolListVolumes; because they were not rejecting the use of '/' in a volume name. Impact -- Because no checking was done on volume names, a user could supply a potential volume name of something like '../../../etc/passwd' to attempt to access a file not belonging to the storage pool. When fine-grained Access Control Lists (ACL) are in effect, a user with storage_vol:create ACL permission but lacking domain:write permssion could thus abuse virStorageVolCreateXML and similar APIs to gain access to files not normally permitted to that user. Fortunately, it appears that the only APIs that could leak information or corrupt files require read-write connection to libvirtd; and when ACLs are not in use (the default without any further configuration), a user with read-write access can already be considered to have full access to the machine, and without an escalation of privilege there is no security problem. Workaround -- If fine-grained ACLs must be used, administrators must consider all of the storage_vol:* permissions as equivalent to domain:write when running an impacted version of libvirt. The easiest way to prevent untrusted users from gaining unauthorized access to volumes outside of permitted pools is by disabling the use of fine-graned ACLs, and ensuring that such users do not have read-write access to libvirtd. Affected product Name: libvirt Repository: git://libvirt.org/git/libvirt.git http://libvirt.org/git/?p=libvirt.git Branch: master Broken in: v1.1.0 Broken in: v1.1.1 Broken in: v1.1.2 Broken in: v1.1.3 Broken in: v1.1.4 Broken in: v1.2.0 Broken in: v1.2.1 Broken in: v1.2.2 Broken in: v1.2.3 Broken in: v1.2.4 Broken in: v1.2.5 Broken in: v1.2.6 Broken in: v1.2.7 Broken in: v1.2.8 Broken in: v1.2.9 Broken in: v1.2.10 Broken in: v1.2.11 Broken in: v1.2.12 Broken in: v1.2.13 Broken in: v1.2.14 Broken in: v1.2.15 Broken in: v1.2.16 Broken in: v1.2.17 Broken in: v1.2.18 Broken in: v1.2.19 Broken in: v1.2.20 Broken in: v1.2.20 Broken in: v1.3.0 Broken by: c930410bebae0a45889b992a7932c663b06cbbcd Fixed by: 034e47c338b13a95cf02106a3af912c1c5f818d7 Branch: v1.1.0-maint Broken by: c930410bebae0a45889b992a7932c663b06cbbcd Fixed by: 14828a59eadc7221326198a8d7af817a6b8b8c13 Branch: v1.1.1-maint Broken by: c930410bebae0a45889b992a7932c663b06cbbcd Fixed by: 692ce509efa0a07f2811d0fe3b7202b020c874e0 Branch: v1.1.2-maint Broken by: c930410bebae0a45889b992a7932c663b06cbbcd Fixed by: e8643ef68c99e9f5068f6ff64ea0acab94cac7f6 Branch: v1.1.3-maint Broken in: v1.1.3.1 Broken in: v1.1.3.2 Broken in: v1.1.3.3 Broken in: v1.1.3.4 Broken in: v1.1.3.5 Broken in: v1.1.3.6 Broken in: v1.1.3.7 Broken in: v1.1.3.8 Broken in: v1.1.3.9 Broken by: c930410bebae0a45889b992a7932c663b06cbbcd Fixed by: dcce665904b8ebc9ac3e5109db179a567b33e1a2 Branch: v1.1.4-maint Broken by: c930410bebae0a45889b992a7932c663b06cbbcd Fixed by: dc2db111a9ba074589c54b90c89f33c01b1e4941 Branch: v1.2.0-maint Broken by: c930410bebae0a45889b992a7932c663b06cbbcd Fixed by: d414ecb8e1714704e6515ab01ef9386d89b8051e Branch: v1.2.1-maint Broken by: c930410bebae0a45889b992a7932c663b06cbbcd Fixed by: 02d365dae595a3453fe0e438bc274ccf3c18e20d Branch: v1.2.2-maint Broken by: c930410bebae0a45889b992a7932c663b06cbbcd Fixed by: 6542e643024ca4272f14e9052b3786378f6eec62 Branch: v1.2.3-maint Broken by: c930410bebae0a45889b992a7932c663b06cbbcd Fixed by: 91898c606496b14e0891af31dfca7eb77ba9fee3 Branch: v1.2.4-maint Broken by: c930410bebae0a45889b992a7932c663b06cbbcd Fixed by: c9450f4f855736ef3024dfbab403a849110d8bb5 Branch: v1.2.5-maint Broken by: c930410bebae0a45889b992a7932c663b06cbbcd Fixed by: 890fc0f1ffcc479b08b9fd01de31b62e3d9e7427 Branch: v1.2.6-maint Broken by: c930410bebae0a45889b992a7932c663b06cbbcd Fixed by: 6ae433938377e1b7e657c34cca39e5242634
Re: [libvirt] [PATCH 3/5] storage: drop 'Extent' from virStorageBackendWipeExtentLocal
On 12/11/2015 11:36 AM, Ján Tomko wrote:
> The only caller always passes 0 for the extent start.
> Drop the 'extent_start' parameter, as well as the mention of extents
> from the function name.
> ---
> src/storage/storage_backend.c | 32 +++-
> 1 file changed, 15 insertions(+), 17 deletions(-)
>
I think 3 & 4 could be combined - since you're removing extent_start
anyway as part of the same change...
The extent_length/wipe_len is already an unsigned long long (from
target.allocation) - so that's just part of fixing the function as far
as I'm concerned.
If you want to keep them separate I'm not going to complain either.
John
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 120d654..d1276dd 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -1987,29 +1987,28 @@
> virStorageBackendVolZeroSparseFileLocal(virStorageVolDefPtr vol,
>
>
> static int
> -virStorageBackendWipeExtentLocal(virStorageVolDefPtr vol,
> - int fd,
> - off_t extent_start,
> - off_t extent_length,
> - size_t writebuf_length,
> - size_t *bytes_wiped)
> +virStorageBackendWipeLocal(virStorageVolDefPtr vol,
> + int fd,
> + off_t extent_length,
> + size_t writebuf_length,
> + size_t *bytes_wiped)
> {
> int ret = -1, written = 0;
> off_t remaining = 0;
> size_t write_size = 0;
> char *writebuf = NULL;
>
> -VIR_DEBUG("extent logical start: %ju len: %ju",
> - (uintmax_t)extent_start, (uintmax_t)extent_length);
> +VIR_DEBUG("extent logical start: 0 len: %ju",
> + (uintmax_t)extent_length);
>
> if (VIR_ALLOC_N(writebuf, writebuf_length) < 0)
> goto cleanup;
>
> -if (lseek(fd, extent_start, SEEK_SET) < 0) {
> +if (lseek(fd, 0, SEEK_SET) < 0) {
> virReportSystemError(errno,
> - _("Failed to seek to position %ju in volume "
> + _("Failed to seek to the start in volume "
> "with path '%s'"),
> - (uintmax_t)extent_start, vol->target.path);
> + vol->target.path);
> goto cleanup;
> }
>
> @@ -2126,12 +2125,11 @@ virStorageBackendVolWipeLocal(virConnectPtr conn
> ATTRIBUTE_UNUSED,
> if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) {
> ret = virStorageBackendVolZeroSparseFileLocal(vol, st.st_size,
> fd);
> } else {
> -ret = virStorageBackendWipeExtentLocal(vol,
> - fd,
> - 0,
> - vol->target.allocation,
> - st.st_blksize,
> - &bytes_wiped);
> +ret = virStorageBackendWipeLocal(vol,
> + fd,
> + vol->target.allocation,
> + st.st_blksize,
> + &bytes_wiped);
> }
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/5] virStorageBackendWipeLocal cleanups
On 12/11/2015 11:36 AM, Ján Tomko wrote:
> The first patch fixes the return values of virStorageWipe on non-sparse local
> files
> in the case of a partial wipe or a fdatasync error.
>
> The rest reduces the number of parameters of
> virStorageBackendWipe{Extent,}Local.
>
> Ján Tomko (5):
> storage: fix return values of virStorageBackendWipeExtentLocal
> storage: move buffer allocation inside
> virStorageBackendWipeExtentLocal
> storage: drop 'Extent' from virStorageBackendWipeExtentLocal
> virStorageBackendWipeLocal: use unsigned long long instead of off_t
> virStorageBackendWipeLocal: remove bytes_wiped argument
>
> src/storage/storage_backend.c | 53
> +--
> 1 file changed, 21 insertions(+), 32 deletions(-)
>
Patch 1 note: (choose to add if you wish)
The errno is printed anyway and thus unless someone overwrites your
message should be passed back to the caller...
I did make a comment in patch 3 - your call on how to handle.
ACK series
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/7] Memory locking limit improvements
On 12/11/2015 09:45 AM, Andrea Bolognani wrote: > During my testing, I've realized that even with the series applied > there's still one case in which we're unable to restore the previous > memory locking limit after removing the last PCI hostdev from the guest. > > If an x86 guest contains a PCI hostdev in its XML definition, then the > memory locking limit will be set correctly, but virCommandGetMaxMemLock() > will be used instead of virProcessGetMaxMemLock(), and the limit will be > set right before calling exec() to spawn the qemu binary. > > In this context, we have no access to the virDomainObj or even > virDomainDef instances, so I see no way of storing the original limit > for later retrieval. > > The series is still an improvement as it covers all other cases. Still, > I thought this was worth mentioning. > > Changes since v1[1]: > > * reorder commits according to John's suggestions > * don't fail if we can't retrieve the current memory locking limit > * small changes here and there as pointed out during review > > Cheers. > > > [1] https://www.redhat.com/archives/libvir-list/2015-November/msg01021.html > > Andrea Bolognani (7): > process: Allow virProcessPrLimit() to get current limit > process: Add virProcessGetMaxMemLock() > qemu: Add qemuDomainAdjustMaxMemLock() > qemu: Use qemuDomainAdjustMaxMemLock() > qemu: Reduce memlock limit after detaching PCI hostdev > qemu: Allow qemuDomainAdjustMaxMemLock() to restore previous value > qemu: Replace Mlock with MemLock in function names > > configure.ac | 2 +- > src/conf/domain_conf.h | 3 +++ > src/libvirt_private.syms | 1 + > src/qemu/qemu_command.c | 4 ++-- > src/qemu/qemu_domain.c | 53 ++--- > src/qemu/qemu_domain.h | 5 ++-- > src/qemu/qemu_hotplug.c | 45 ++- > src/util/virprocess.c| 61 > +++- > src/util/virprocess.h| 2 ++ > 9 files changed, 135 insertions(+), 41 deletions(-) > ACK series, John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1] libvirtd: Increase NL buffer size for lots of interface
On 12/16/2015 10:24 AM, Michal Privoznik wrote:
On 10.12.2015 07:34, Leno Hou wrote:
1. When switching CPUs to offline/online in a system more than 128 cpus
2. When using virsh to destroy domain in a system with more interface
All of above happens nl_recv returned with error: No buffer space available.
This patch set socket buffer size to 128K and turn on message peeking for
nl_recv,
as this would solve this problem totally and permanetly.
LTC-Bugzilla: #133359 #125768
Apparently "LTC-Bugzilla" refers to a bugzilla instance for the Linux
Technology Center at IBM, but I don't see how to get to it. References
to bug reports are always nice in a patch, because it makes it much
easier to spelunk the discussion leading up to the fix (now and later),
but they can't be included unless they point to a publicly accessible
report. Is LTC-Bugzilla (and in particular, these two reports) publicly
accessible? If so, a clickable link would be better.
Signed-off-by: Leno Hou
Cc: Wenyi Gao
---
src/util/virnetlink.c | 8
1 file changed, 8 insertions(+)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index 679b48e..c8c9fe0 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -696,6 +696,14 @@ virNetlinkEventServiceStart(unsigned int protocol,
unsigned int groups)
goto error_server;
}
+if (nl_socket_set_buffer_size(srv->netlinknh, 131702, 0) < 0) {
The above function doesn't exist in libnl 1.1 (still used in
RHEL6/CentOS6, for example), so that would cause a build failure on some
systems. In libnl 1.1 the function is called nl_set_buffer_size().
Also, how did you arrive at 128k for the default buffer size? What kind
of sizes are you seeing?
+virReportSystemError(errno,
+"%s",_("cannot set netlink socket buffer size to 128k"));
+goto error_server;
+}
+
+nl_socket_enable_msg_peek(srv->netlinknh);
+
According to a link I followed from another message on this topic last
week, libnl's message peeking can't be guaranteed to always work,
because netlink doesn't always return the proper buffer size (depending
on version).
if ((srv->eventwatch = virEventAddHandle(fd,
VIR_EVENT_HANDLE_READABLE,
virNetlinkEventCallback,
I believe this patch appears over and over again. Usually, the problem
was in libnl library we use and this was just a workaround. Can you test
with the latest libnl version (probably even GIT HEAD) and see if that
helps?
I had the same memory. So I just looked back through the history of bug
reports about this issue, and found the following:
* libnl-1.1 and libnl-3 both originally set the default message
buffersize to 4096 bytes, with MSG_PEEK turned off.
* when this problem came up in RHEL6, it was unfortunately reported as a
private BZ (a pet peeve of mine), and the result of the discussion about
it was that libnl-1.1 (the version used in RHEL6) was patched *upstream*
to set the default message buffersize to 16384 bytes (getpagesize() *
4), which would solve the problem for even very large numbers of VFs.
That was in 2013 and there have been no further reports against RHEL6.
* Although I had assumed the problem was solved, it again came up in
RHEL7 (which uses libnl-3 - a slightly different API, and maintained in
a separate git repo), this time in a public BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1040626
I asked if perhaps the change that had been made upstream in libnl-1.1
hadn't also been made to libnl-3 (this is what I assumed during the
previous incident). It hadn't. So the same change was made for libnl-3,
both upstream and as a backport to RHEL7, and everyone was happy.
I have very little detailed memory of that time (the above was all
recalled by looking at archives of discussions) but what had stuck in my
mind was "This problem has been fixed in libnl, so libvirt should NOT
put in "workarounds" for broken versions of libnl."
But if you are using a version of libnl3 with this patch (which was in
libnl-3.2.22 upstream, and is in the libnl-3.2.21 that's in RHEL7.0+), :
https://github.com/tgraf/libnl/commit/807fddc4cd9ecb12ba64e1b7fa26d86b6c2f19b0
then the change to quadruple the buffer size in libnl was insufficient,
(and also, when I looked back at the discussion now, I see that the
libnl maintainer had said "The permanent fix would be for libvirt to
enable message peeking", so I suppose it's time to "bite the bullet" and
enable netlink message peeking in libvirt (but, since there are
apparently versions of netlink that don't properly inform libnl when a
re-read is necessary, we also need to increase the default buffer size).
However, your patch is only fixing the problem in one place. There are
several places that we allocate netlink sockets, and they should all get
the same fix, implying that there should be a common fun
Re: [libvirt] vf configuration cleanup when VM is delete
On 12/16/2015 12:35 PM, Vlad Yasevich wrote: (BTW, Cisco's enic driver, on the other hand, doesn't support setting VF MAC addresses via a netlink message to the PF *at all* (so libvirt has to make special accommodations), but Looking at upstream, it looks like it offers support for setting VF mac via VFINFO data in the netlink message. May be it got fixed? Interesting. If I had one of those systems of my own to test on, I'd give it a try. The only one I have access to is running a 2.6.32 RHEL6 kernel though :-/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] vf configuration cleanup when VM is delete
On 12/16/2015 12:22 PM, Laine Stump wrote: > On 12/16/2015 07:56 AM, Moshe Levi wrote: >> >> To clean up the VF I use >> >> ip link set dev p4p2 vf 0 mac 0 and it working >> > > Now *that* is interesting... > >> 24: enp3s0f0: mtu 1500 qdisc mq master >> ovs-system >> state UP mode DEFAULT group default qlen 1000 >> >> link/ether e4:1d:2d:a5:f1:22 brd ff:ff:ff:ff:ff:ff >> >> vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state auto >> >> vf 1 MAC 00:00:00:00:00:00, spoof checking off, link-state auto >> >> vf 2 MAC 00:00:00:00:00:b1, vlan 190, spoof checking off, link-state >> enable >> >> vf 3 MAC aa:bb:cc:00:00:12, vlan 190, spoof checking off, link-state >> enable >> >> [root@r-ufm160 devstack]# ip link set dev enp3s0f0 vf 3 mac 0 >> >> [root@r-ufm160 devstack]# ip link show enp3s0f0 >> >> 24: enp3s0f0: mtu 1500 qdisc mq master >> ovs-system >> state UP mode DEFAULT group default qlen 1000 >> >> link/ether e4:1d:2d:a5:f1:22 brd ff:ff:ff:ff:ff:ff >> >> vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state auto >> >> vf 1 MAC 00:00:00:00:00:00, spoof checking off, link-state auto >> >> vf 2 MAC 00:00:00:00:00:b1, vlan 190, spoof checking off, link-state >> enable >> >> vf 3 MAC 00:00:00:00:00:b1, vlan 190, spoof checking off, link-state >> enable >> >> It just put the address 00:00:00:00:00:b1 which I don’t know why, but as I >> remember the >> same behavior is in intel cards (I think is related to iproute) >> > > I just tried this with the igb driver on both 2.6.32 and 4.1 kernels, and a > plain "0" is > successful for me too. But, as you've experienced, it doesn't actually set > the MAC address > to 00:00:00:00:00:00, but instead puts random numbers in the final two bytes > :-/ > > So I investigated further, and found that if I use: > > ip link set dev p4p2 vf 0 mac 00:00:00:00:00 <-- note 5 bytes, not 6 > > then all bytes except the *final* byte are 0, and the final byte is two > seemingly random > bytes. But if I re-run the same command many times I find that it just > rotates between 10 > or so different values; not so random (when I give "0", or "00:00:00:00" to > ip link set, > the 2nd to last byte is always the *exact same* value. > > So I looked in the source for the ip utility (in the iproute package) and I > found that the > function parsing mac addresses from the commandline just creates the buffer > on the stack, > doesn't initialize it, then parses in as many digits as you specify, leaving > the rest with > whatever happened to be sitting on the stack at the time :-O. > > In other words, it's just a happy coincidence of a bug in iproute's mac > address parser > that "ip link set mac 0" happens to be successful (and that bytes 2-4 > are 0 and 5-6 > are non-0). > > I really don't know where to start / what to do with this information. There > is obviously > a bug in iproute that should be fixed, but if it is fixed before all the > places in the > kernel are adjusted to allow an all-0 MAC, then users will be complaining > that their > script which was working for years and years (although probably not doing > exactly what > they believed) is suddenly broken. And who knows what Hell-fury will be > unleashed by some > unknown bit of code in the kernel if a 0 mac address suddenly shows up for > the first time > ever. Sigh. But they wouldn't be there for the first time as we can plainly see from above output for vf 0 and vf 1. Not sure if users depending on the stack contents of a buffer in iproute2 would ever be really working as they expected. Might just be a necessary patch. > > (BTW, Cisco's enic driver, on the other hand, doesn't support setting VF MAC > addresses via > a netlink message to the PF *at all* (so libvirt has to make special > accommodations), but Looking at upstream, it looks like it offers support for setting VF mac via VFINFO data in the netlink message. May be it got fixed? -vlad > it happily accepts requests to directly set the MAC address to > 00:00:00:00:00:00 via > ioctl(SIOCSIFHWADDR) (and the interface MAC address really does get set to > all 0's). There > is a script for ovirt that uses a MAC address of all 0's to recognize that an > interface is > unused, and can thus be included in a pool of interfaces in a libvirt > network. That won't > work with any other SRIOV drivers though, because even if they initialize > their VF macs to > 0 (e.g. mlx and *new* (3.10+) igb (but *not* 2.6.32 igb!)), they can't be set > back to 0 > when they are once again unused. Again sigh.) > >> I used fedora 2.1 with kernel 4.1.13-100. >> >> The most annoying part is that in OpenStack if I use an SR-IOV VF >> (interface hostdev) >> for VM and delete it I can’t reuse it for macvtap (interface direct) so I >> have to clean >> the mac >> >> by running ip link set dev p4p2 vf 0 mac 0 >> >> I guess I will need to workaround it in OpenStack. >> >> *From:*sendmail [mailto:justsendmai
Re: [libvirt] vf configuration cleanup when VM is delete
On 12/16/2015 07:56 AM, Moshe Levi wrote: To clean up the VF I use ip link set dev p4p2 vf 0 mac 0 and it working Now *that* is interesting... 24: enp3s0f0: mtu 1500 qdisc mq master ovs-system state UP mode DEFAULT group default qlen 1000 link/ether e4:1d:2d:a5:f1:22 brd ff:ff:ff:ff:ff:ff vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state auto vf 1 MAC 00:00:00:00:00:00, spoof checking off, link-state auto vf 2 MAC 00:00:00:00:00:b1, vlan 190, spoof checking off, link-state enable vf 3 MAC aa:bb:cc:00:00:12, vlan 190, spoof checking off, link-state enable [root@r-ufm160 devstack]# ip link set dev enp3s0f0 vf 3 mac 0 [root@r-ufm160 devstack]# ip link show enp3s0f0 24: enp3s0f0: mtu 1500 qdisc mq master ovs-system state UP mode DEFAULT group default qlen 1000 link/ether e4:1d:2d:a5:f1:22 brd ff:ff:ff:ff:ff:ff vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state auto vf 1 MAC 00:00:00:00:00:00, spoof checking off, link-state auto vf 2 MAC 00:00:00:00:00:b1, vlan 190, spoof checking off, link-state enable vf 3 MAC 00:00:00:00:00:b1, vlan 190, spoof checking off, link-state enable It just put the address 00:00:00:00:00:b1 which I don’t know why, but as I remember the same behavior is in intel cards (I think is related to iproute) I just tried this with the igb driver on both 2.6.32 and 4.1 kernels, and a plain "0" is successful for me too. But, as you've experienced, it doesn't actually set the MAC address to 00:00:00:00:00:00, but instead puts random numbers in the final two bytes :-/ So I investigated further, and found that if I use: ip link set dev p4p2 vf 0 mac 00:00:00:00:00 <-- note 5 bytes, not 6 then all bytes except the *final* byte are 0, and the final byte is two seemingly random bytes. But if I re-run the same command many times I find that it just rotates between 10 or so different values; not so random (when I give "0", or "00:00:00:00" to ip link set, the 2nd to last byte is always the *exact same* value. So I looked in the source for the ip utility (in the iproute package) and I found that the function parsing mac addresses from the commandline just creates the buffer on the stack, doesn't initialize it, then parses in as many digits as you specify, leaving the rest with whatever happened to be sitting on the stack at the time :-O. In other words, it's just a happy coincidence of a bug in iproute's mac address parser that "ip link set mac 0" happens to be successful (and that bytes 2-4 are 0 and 5-6 are non-0). I really don't know where to start / what to do with this information. There is obviously a bug in iproute that should be fixed, but if it is fixed before all the places in the kernel are adjusted to allow an all-0 MAC, then users will be complaining that their script which was working for years and years (although probably not doing exactly what they believed) is suddenly broken. And who knows what Hell-fury will be unleashed by some unknown bit of code in the kernel if a 0 mac address suddenly shows up for the first time ever. Sigh. (BTW, Cisco's enic driver, on the other hand, doesn't support setting VF MAC addresses via a netlink message to the PF *at all* (so libvirt has to make special accommodations), but it happily accepts requests to directly set the MAC address to 00:00:00:00:00:00 via ioctl(SIOCSIFHWADDR) (and the interface MAC address really does get set to all 0's). There is a script for ovirt that uses a MAC address of all 0's to recognize that an interface is unused, and can thus be included in a pool of interfaces in a libvirt network. That won't work with any other SRIOV drivers though, because even if they initialize their VF macs to 0 (e.g. mlx and *new* (3.10+) igb (but *not* 2.6.32 igb!)), they can't be set back to 0 when they are once again unused. Again sigh.) I used fedora 2.1 with kernel 4.1.13-100. The most annoying part is that in OpenStack if I use an SR-IOV VF (interface hostdev) for VM and delete it I can’t reuse it for macvtap (interface direct) so I have to clean the mac by running ip link set dev p4p2 vf 0 mac 0 I guess I will need to workaround it in OpenStack. *From:*sendmail [mailto:justsendmailnothinge...@gmail.com] *On Behalf Of *Laine Stump *Sent:* Tuesday, December 15, 2015 9:45 PM *To:* Libvirt *Cc:* Moshe Levi ; vyase...@redhat.com *Subject:* Re: [libvirt] vf configuration cleanup when VM is delete On 12/15/2015 01:34 PM, Laine Stump wrote: On 12/13/2015 10:51 AM, Moshe Levi wrote: Hi, I have a setup with libvirt 1.3.0 and OpenStack trunk. Before launched the VM ip link command show the following VF mac/vlan configuration [1] When I launch a VM with via openstack api (OpenStack direct port) I can see that the VF get the mac/vlan according to libvrit xml [2] and ip link command [3], but when I delete the VM the
[libvirt] [PATCH] storage: Fix startup issue for logical pool
Commit id '71b803ac' assumed that the storage pool source device path
was required for a 'logical' pool. This resulted in a failure to start
a pool without any device path defined.
So, adjust the virStorageBackendLogicalMatchPoolSource logic to
return success if at least the pool name matches the vgs output
when no pool source device path is/are provided.
Signed-off-by: John Ferlan
---
src/storage/storage_backend_logical.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index c52782f..f59684a 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -541,6 +541,15 @@
virStorageBackendLogicalMatchPoolSource(virStoragePoolObjPtr pool)
goto cleanup;
}
+/* If the pool has defined source device(s), then let's make sure
+ * they match as well; otherwise, matching can only occur on the
+ * pool's name.
+ */
+if (!pool->def->source.ndevice) {
+ret = true;
+goto cleanup;
+}
+
/* Let's make sure the pool's device(s) match what the pvs output has
* for volume group devices.
*/
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] storage: Add helper to compare logical pool def against pvs output
On 12/16/2015 08:44 AM, Ján Tomko wrote: > On Wed, Dec 16, 2015 at 07:46:18AM -0500, John Ferlan wrote: >> >> >> On 12/16/2015 07:02 AM, Ján Tomko wrote: >>> On Mon, Dec 07, 2015 at 03:47:58PM -0500, John Ferlan wrote: https://bugzilla.redhat.com/show_bug.cgi?id=1025230 Add a new helper virStorageBackendLogicalMatchPoolSource to compare the pool's source name against the output from a 'pvs' command to list all volume group physical volume data on the host. In addition, compare the pool's source device list against the particular volume group's device list to ensure the source device(s) listed for the pool match what the was listed for the volume group. Then for pool startup or check API's we need to call this new API in order to ensure that the pool we're about to start or declare active during checkPool has a valid definition vs. the running host. >>> >>> This patch breaks starting of logical pools with no source devices. >>> >>> Jan >>> >> >> Not enough information for me to go on... Can you provide sample XML >> that works prior to the change? From just your description I assume you >> mean: >> >> >> xxx >> >> >> > > In my case it's: > > vg > > > > /dev/vg > ... > > > (Also, if the vg_name matches the pool name, the whole element > can be omitted) > So the check should be "if provided", then also check that... I'll send a patch shortly. >> instead of having a >> >> >> >> As the source device >> >> Without a source device how would pool-build work (vgcreate)? > > It would not, and pool-build is the only time when the list of physical > volumes is required. > > Libvirt also uses the list for pool-delete, but this is IMO a bug, > considering that we do not keep this list up to date and it could end up > calling pvremove on PVs that are no longer a part of that VG. > > > Thinking about it some more, the point of LVM is to abstract from the > physical volumes, so I do not think we should burden the user with the > task of updating the pool's XML every time the underlying PVs change > (i.e. the USB hard drives were plugged in in a different order). > > > Instead of logging a warning / rejecting the pool, could we update the > source devices list on pool refresh? > I don't see why not, but that seems to be a different/separate issue - would also involve a write of the changed XML (to at least the running cache area). John > Jan > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/6] virsh: Add build flags to pool-create[-as] and pool-start
On 12/16/2015 08:42 AM, Peter Krempa wrote:
> On Wed, Nov 25, 2015 at 14:11:08 -0500, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=830056
>>
>> Utilize recently added VIR_STORAGE_POOL_CREATE_WITH_BUILD* flags in
>> order to pass the flags along to the virStoragePoolCreateXML and
>> virStoragePoolCreate API's.
>>
>> This affects the 'virsh pool-create', 'virsh pool-create-as', and
>> 'virsh pool-start' commands. While it could be argued that pool-start
>> doesn't need the flags, they could prove useful for someone trying to
>> do one command build --overwrite and start command processing or
>> essentially starting with a clean slate.
>>
>> NB:
>> This patch is loosely based upon code originally authored by Osier
>> Yang that were not reviewed and pushed, see:
>>
>> https://www.redhat.com/archives/libvir-list/2012-July/msg00497.html
>> Signed-off-by: John Ferlan
>> ---
>> tools/virsh-pool.c | 73
>> +++---
>> tools/virsh.pod| 25 ++-
>> 2 files changed, 94 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
>> index cb91cd3..1bb987d 100644
>> --- a/tools/virsh-pool.c
>> +++ b/tools/virsh-pool.c
>> @@ -47,6 +47,13 @@
>> .help = N_("file containing an XML pool description")\
>> },\
>>
>> +#define OPT_BUILD_COMMON \
>> +{.name = "build", \
>> + .type = VSH_OT_BOOL, \
>> + .flags = 0, \
>> + .help = N_("build the pool as normal") \
>> +},\
>> +
>> #define OPT_NO_OVERWRITE_COMMON \
>> {.name = "no-overwrite", \
>> .type = VSH_OT_BOOL, \
>> @@ -235,6 +242,9 @@ static const vshCmdInfo info_pool_create[] = {
>>
>> static const vshCmdOptDef opts_pool_create[] = {
>> OPT_FILE_COMMON
>> +OPT_BUILD_COMMON
>> +OPT_NO_OVERWRITE_COMMON
>> +OPT_OVERWRITE_COMMON
>>
>
> These look terrible without commas between entries.
>
OK - so all fixed and the names are now:
VSH_POOL_OPT_COMMON
VSH_POOL_FILE_OPT_COMMON
VSH_POOL_BUILD_OPT_COMMON
VSH_POOL_NO_OVERWRITE_OPT_COMMON
VSH_POOL_OVERWRITE_OPT_COMMON
VSH_POOL_X_AS_OPT_COMMON
Each would require a comma when used within an opts_pool* struct
>> {.name = NULL}
>> };
>> @@ -246,15 +256,32 @@ cmdPoolCreate(vshControl *ctl, const vshCmd *cmd)
>> const char *from = NULL;
>> bool ret = true;
>> char *buffer;
>> +bool build;
>> +bool overwrite;
>> +bool no_overwrite;
>> +unsigned int flags = 0;
>> virshControlPtr priv = ctl->privData;
>>
>> if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0)
>> return false;
>>
>> +build = vshCommandOptBool(cmd, "build");
>> +overwrite = vshCommandOptBool(cmd, "overwrite");
>> +no_overwrite = vshCommandOptBool(cmd, "no-overwrite");
>> +
>> +VSH_EXCLUSIVE_OPTIONS_VAR(overwrite, no_overwrite);
>
> This should be used only if the name of the variable matches the name of
> the option flag since the variable name is used in the error message in
> place of the option flag name.
>
Oh yeah - I remember reading that.. I think at one time I had used the
VSH_EXCLUSIVE_OPTIONS_EXPR instead, but I cannot remember what happened
to cause me to use this one.
Both instances now changed to:
VSH_EXCLUSIVE_OPTIONS_EXPR("overwrite", overwrite,
"no-overwrite", no_overwrite);
Is it preferable to see a v2 or are the edits as I've described sufficient?
John
>> +
>> +if (build)
>> +flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD;
>> +if (overwrite)
>> +flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE;
>> +if (no_overwrite)
>> +flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE;
>> +
>> if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0)
>> return false;
>>
>> -pool = virStoragePoolCreateXML(priv->conn, buffer, 0);
>> +pool = virStoragePoolCreateXML(priv->conn, buffer, flags);
>> VIR_FREE(buffer);
>>
>> if (pool != NULL) {
>> @@ -386,6 +413,9 @@ static const vshCmdInfo info_pool_create_as[] = {
>>
>> static const vshCmdOptDef opts_pool_create_as[] = {
>> OPTS_POOL_COMMON_X_AS
>> +OPT_BUILD_COMMON
>> +OPT_NO_OVERWRITE_COMMON
>> +OPT_OVERWRITE_COMMON
>>
>> {.name = NULL}
>> };
>> @@ -397,8 +427,25 @@ cmdPoolCreateAs(vshControl *ctl, const vshCmd *cmd)
>> const char *name;
>> char *xml;
>> bool printXML = vshCommandOptBool(cmd, "print-xml");
>> +bool build;
>> +bool overwrite;
>> +bool no_overwrite;
>> +unsigned int flags = 0;
>>
Re: [libvirt] [PATCH 4/6] virsh: Create macro for "overwrite" and no-overwrite" options
On 12/16/2015 10:32 AM, John Ferlan wrote: > > [...] > >>> >>> So then in your opinion and using the same logic, patch 3 is also nixed? >>> This one will get used in patch 6. >> >> With that use it will result in total 4 uses. I won't object if you fix >> the macro name and remove the comma >> > > Hmm. by name change do you mean "OPT_POOL_FILE_COMMON", > "OPT_POOL_OVERWRITE_COMMON", etc.? I guess I was thinking less globally > available macros, e.g. module specific rather than adding them to some > more generally included virsh*.h file. > nm... patch 2 comment... you'd rather see VSH_POOL_OPT_COMMON than OPT_POOL_COMMON John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] virsh: Create macro for "overwrite" and no-overwrite" options
[...] >> >> So then in your opinion and using the same logic, patch 3 is also nixed? >> This one will get used in patch 6. > > With that use it will result in total 4 uses. I won't object if you fix > the macro name and remove the comma > Hmm. by name change do you mean "OPT_POOL_FILE_COMMON", "OPT_POOL_OVERWRITE_COMMON", etc.? I guess I was thinking less globally available macros, e.g. module specific rather than adding them to some more generally included virsh*.h file. > Patch 3 attempts to create a universal macro for any "file" option, but > that can't be really done universally. See for yourself: > > git grep -A 4 '\.name = "file"' > Yeah - 'file' is one of those that isn't universal for all virsh*.c files. This started out of a desire to not copy all the opts_pool_X_as options as was done by Osier in his initial changes (see link in cover) for pool-create-as and pool-define-as. Then I thought - oh we have so many duplicated elements, why not generate module specific macros to handle. John > The name of the macro would really need to be specific for pool XML file > name. Any other attempt will create confusion and I don't think it's > worth. > >> >> Like I noted in the cover letter - I see value in having the macros and >> I also see the pain (having to go look them up). >> >> Adjusting the changes to remove/use commas is fine - just made the >> initial cut-n-paste easier >> >> John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1] libvirtd: Increase NL buffer size for lots of interface
On 10.12.2015 07:34, Leno Hou wrote:
> 1. When switching CPUs to offline/online in a system more than 128 cpus
> 2. When using virsh to destroy domain in a system with more interface
>
> All of above happens nl_recv returned with error: No buffer space available.
> This patch set socket buffer size to 128K and turn on message peeking for
> nl_recv,
> as this would solve this problem totally and permanetly.
> LTC-Bugzilla: #133359 #125768
>
> Signed-off-by: Leno Hou
> Cc: Wenyi Gao
> ---
> src/util/virnetlink.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
> index 679b48e..c8c9fe0 100644
> --- a/src/util/virnetlink.c
> +++ b/src/util/virnetlink.c
> @@ -696,6 +696,14 @@ virNetlinkEventServiceStart(unsigned int protocol,
> unsigned int groups)
> goto error_server;
> }
>
> +if (nl_socket_set_buffer_size(srv->netlinknh, 131702, 0) < 0) {
> +virReportSystemError(errno,
> +"%s",_("cannot set netlink socket buffer size to 128k"));
> +goto error_server;
> +}
> +
> +nl_socket_enable_msg_peek(srv->netlinknh);
> +
> if ((srv->eventwatch = virEventAddHandle(fd,
> VIR_EVENT_HANDLE_READABLE,
> virNetlinkEventCallback,
>
I believe this patch appears over and over again. Usually, the problem
was in libnl library we use and this was just a workaround. Can you test
with the latest libnl version (probably even GIT HEAD) and see if that
helps?
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 2/2] qemu: emit 'defined' event after reverted to snapshot
On 16.12.2015 16:26, Peter Krempa wrote: On Wed, Dec 16, 2015 at 07:28:43 -0500, John Ferlan wrote: On 12/16/2015 07:01 AM, Peter Krempa wrote: On Wed, Dec 16, 2015 at 12:09:42 +0300, Dmitry Andreev wrote: If config file was changed VIR_DOMAIN_EVENT_DEFINED should be emitted drop "file" here. The important part is the config itself being changed. The file is just a implication of that. Additionally, commit messages here or in the previous patch don't provide any justification for this change. I'd like to have a statement why is this important to change. FWIW: This is what I got from the cover letter: The cover letter is not commited to the repo so it shouldn't be the only place to put such information. I usualy don't even bother to read the cover letter. I'll try to be more verbose in commit messages next time. Lack of the event become a problem for virt-manager https://bugzilla.redhat.com/show_bug.cgi?id=1081148 Okay, that's starting to be reasonable let's say, but ... Without the event, it seems virt-manager doesn't "see" the change and presents the data prior to snapshot With internal snapshots there's quite a few state changes that can happen according to the source and target state that need to be taken into account. If you follow the snapshot code you'll see that in some cases qemu has to be restarted and in some cases not. In case when qemu is not restarted during reversion we should not emit a lifecycle event. Stop/start of qemu is based on the fact that the "ABI stability" check of the current and target config will not match. This leaves a loophole where e.g. change of a disk source would not be properly tracked. This problem will most likely happen when combining external and internal snapshots ( https://bugzilla.redhat.com/show_bug.cgi?id=880565 ), but also other legitimate changes of config (mostly change of disk source) will not be tracked properly. This is a bug in the snapshot handling code though and qemu should be restarted at that point since it needs to open different files and libvirt needs to label them prior to that. As said, this is a bug in the snapshot code and patching it by doing an event is not right. Additionally if you are going to revert a transient VM snapshot from running state to running state the DEFINED event is totaly bogus since it doesn't have a persistent definition, which is where we refer to it as "DEFINED". Thank you for the complete explanation, it helps me to understand that in most cases we have _FROM_SNAPSHOT event and don't need DEFINED. In case of offline target states this approach may be valid since the config changes without any notification which is actually what the bug is complaining about. Using the defined event should be used only in such case. And yes, this is the case that has no events. One thing to consider then is whether it's worth fixing what bz 1081148 is tracking with this (but the other cases described above should not emit the event then), or whether a different approach is desired. That's why I asked for justification. I'll describe what problem I tried to solve. We use libvirt's API and store vm config on the client side. Each time we get DEFINED event we rewrite stored config. Now I know that we should watch for _FROM_SNAPSHOT events also. But the case with two offline states has no events and we have a problem. bz 1081148 confirmed that we are not alone and virt-manager has the same problem. Any event about a snapshot will be enough to solve my problem. My mistake was that I thought there should be DEFINED event before any _FROM_SNAPSHOT. Now I think that there could be STOPPED_FROM_SNAPSHOT or DEFINED event in case of offline target states. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: Attempt to refresh volume after successful wipe volume
On Wed, Dec 16, 2015 at 09:19:07AM -0500, John Ferlan wrote: > > > On 12/16/2015 08:48 AM, Ján Tomko wrote: > > On Tue, Dec 15, 2015 at 03:03:33PM -0500, John Ferlan wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1270709 > >> > >> When a volume wipe is successful, perform a volume refresh afterwards to > >> update any volume data that may be used in future volume commands, such as > >> volume resize. For a raw file volume, a wipe could truncate the file and > >> a followup volume resize the capacity may fail because the volume target > >> allocation isn't updated to reflect the wipe activity. > >> > >> Since the documentation doesn't mention this side effect of the zero > >> algorithm for a raw file volume, update the various documentation to > >> describe the results. > >> > > > > The documentation does not belong in this patch. Also, we could > > intentionally keep it vague so that we don't have to commit to that > > behavior. > > > > Adding the documentation was a reaction to your review of v1: > > http://www.redhat.com/archives/libvir-list/2015-December/msg00464.html > > where you queried whether we should update the documentation "to reflect > that there might not be any pass over the old data". > I was thinking out loud and I did not mean that the documentation changes should be a part of this patch. Sorry if I was not clear enough. > Whether the doc changes are kept or not I suppose then becomes > "consensus based". I see value in describing what's done and I see value > in being vague enough so that we could change to do something else in > the future. > > The question thus remains, should the current truncate be considered a > bug? Or a happy consequence/feature? > I have a slight preference for treating it as a feature and not adjusting the docs. > >> Signed-off-by: John Ferlan > >> --- > >> v1: > >> http://www.redhat.com/archives/libvir-list/2015-December/msg00085.html > >> > >> Changes since v1: > >>* Use the preferred call format from review > > > >>* Cause error if refreshVol fails > > > > If my patch adjusting the return value gets pushed before this one: > > https://www.redhat.com/archives/libvir-list/2015-December/msg00467.html > > > > that change is just cosmetic. > > yep. > > > Otherwise, I don't think a patch adding refreshVol should be changing > > the return value. > > > > So again, your initial review says: > > More readable as: > if (func() < 0) > goto cleanup; > > Which is what I followed. Other than the value of ret being -errno on > fdatasync - is the objection based solely on you'd like to see a > separate patch to handle the ret differently for the wipeVol call, then > a patch for refreshVol? > The patch I linked ajdusts the return value for the only code path that did not follow the 0/-1 convention. So: ACK to the non-documenation hunk of this patch, in its entirety. I would prefer if you could wait for my patch adjusting the return values before pushing this one, but that's not a hard prerequisite. Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] storage: Add flags to allow building pool during create processing
On 12/16/2015 08:18 AM, Michal Privoznik wrote:
> On 25.11.2015 20:11, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=830056
>>
>> Add flags handling to the virStoragePoolCreate and virStoragePoolCreateXML
>> API's which will allow the caller to provide the capability for the storage
>> pool create API's to also perform a pool build during creation rather than
>> requiring the additional buildPool step. This will allow transient pools
>> to be defined, built, and started.
>>
>> The new flags are:
>>
>> * VIR_STORAGE_POOL_CREATE_WITH_BUILD
>> Perform buildPool without any flags passed.
>>
>> * VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE
>> Perform buildPool using VIR_STORAGE_POOL_BUILD_OVERWRITE flag.
>>
>> * VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE
>> Perform buildPool using VIR_STORAGE_POOL_BUILD_NO_OVERWRITE flag.
>>
>> It is up to the backend to handle the processing of build flags. The
>> overwrite and no-overwrite flags are mutually exclusive.
>>
>> NB:
>> This patch is loosely based upon code originally authored by Osier
>> Yang that were not reviewed and pushed, see:
>>
>> https://www.redhat.com/archives/libvir-list/2012-July/msg01328.html
>> Signed-off-by: John Ferlan
>> ---
>> include/libvirt/libvirt-storage.h | 16 ++-
>> src/libvirt-storage.c | 4 ++--
>> src/storage/storage_driver.c | 42
>> +--
>> 3 files changed, 57 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/libvirt/libvirt-storage.h
>> b/include/libvirt/libvirt-storage.h
>> index 9fc3c2d..996a726 100644
>> --- a/include/libvirt/libvirt-storage.h
>> +++ b/include/libvirt/libvirt-storage.h
>> @@ -57,7 +57,6 @@ typedef enum {
>> # endif
>> } virStoragePoolState;
>>
>> -
>> typedef enum {
>> VIR_STORAGE_POOL_BUILD_NEW = 0, /* Regular build from scratch */
>> VIR_STORAGE_POOL_BUILD_REPAIR = (1 << 0), /* Repair / reinitialize */
>> @@ -71,6 +70,21 @@ typedef enum {
>> VIR_STORAGE_POOL_DELETE_ZEROED = 1 << 0, /* Clear all data to zeros
>> (slow) */
>> } virStoragePoolDeleteFlags;
>>
>> +typedef enum {
>> +VIR_STORAGE_POOL_CREATE_NORMAL = 0,
>> +
>> +/* Create the pool and perform pool build without any flags */
>> +VIR_STORAGE_POOL_CREATE_WITH_BUILD = 1 << 0,
>> +
>> +/* Create the pool and perform pool build using the
>> + * VIR_STORAGE_POOL_BUILD_OVERWRITE flag */
>> +VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE = 1 << 1,
>> +
>> +/* Create the pool and perform pool build using the
>> + * VIR_STORAGE_POOL_BUILD_NO_OVERWRITE flag */
>> +VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE = 1 << 2,
>
> So _OVERWRITE and _NO_OVERWRITE flags are mutually exclusive then,
> right? Probably worth noting.
>
>> +} virStoragePoolCreateFlags;
>> +
>> typedef struct _virStoragePoolInfo virStoragePoolInfo;
>>
>> struct _virStoragePoolInfo {
>> diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c
>> index 66dd9f0..238a6cd 100644
>> --- a/src/libvirt-storage.c
>> +++ b/src/libvirt-storage.c
>> @@ -505,7 +505,7 @@ virStoragePoolLookupByVolume(virStorageVolPtr vol)
>> * virStoragePoolCreateXML:
>> * @conn: pointer to hypervisor connection
>> * @xmlDesc: XML description for new pool
>> - * @flags: extra flags; not used yet, so callers should always pass 0
>> + * @flags: bitwise-OR of virStoragePoolCreateFlags
>> *
>> * Create a new storage based on its XML description. The
>> * pool is not persistent, so its definition will disappear
>> @@ -670,7 +670,7 @@ virStoragePoolUndefine(virStoragePoolPtr pool)
>> /**
>> * virStoragePoolCreate:
>> * @pool: pointer to storage pool
>> - * @flags: extra flags; not used yet, so callers should always pass 0
>> + * @flags: bitwise-OR of virStoragePoolCreateFlags
>> *
>> * Starts an inactive storage pool
>> *
>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index bbf21f6..59a18bf 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c
>> @@ -671,8 +671,11 @@ storagePoolCreateXML(virConnectPtr conn,
>> virStoragePoolPtr ret = NULL;
>> virStorageBackendPtr backend;
>> char *stateFile = NULL;
>> +unsigned int build_flags = 0;
>>
>> -virCheckFlags(0, NULL);
>> +virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD |
>> + VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE |
>> + VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE, NULL);
>
> How about VIR_EXCLUSIVE_FLAGS_RET(_OVERWRITE, _NO_OVERWRITE, NULL);
>
The disk and fs backends do check as well, but I can add it here.
>>
>> storageDriverLock();
>> if (!(def = virStoragePoolDefParseString(xml)))
>> @@ -694,6 +697,22 @@ storagePoolCreateXML(virConnectPtr conn,
>> goto cleanup;
>> def = NULL;
>>
>> +if (backend->buildPool) {
>> +if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE)
>> +
Re: [libvirt] [PATCH 4/6] virsh: Create macro for "overwrite" and no-overwrite" options
On Wed, Dec 16, 2015 at 08:44:50 -0500, John Ferlan wrote: > > > On 12/16/2015 08:35 AM, Peter Krempa wrote: > > On Wed, Dec 16, 2015 at 14:18:22 +0100, Michal Privoznik wrote: > >> On 25.11.2015 20:11, John Ferlan wrote: > >>> Although not currently used in more than one command, it soon will be > >>> so create a common macro to be used in the new command location. > >>> > >>> Additionally, add the ".flags = 0," for both to match the expections > >>> of the structure being predefined. The compiler already does zero init for fields that are not specified explicitly. > >>> > >>> Signed-off-by: John Ferlan > >>> --- > >>> tools/virsh-pool.c | 24 > >>> 1 file changed, 16 insertions(+), 8 deletions(-) > >>> > >> > >> ACK > > > > I don't really fancy this one. It introduces the macro for a very > > uncommon option value. If it will be used more that 10 times it would > > make sense. For 2 or 3 uses it does not. > > > > Peter > > > > So then in your opinion and using the same logic, patch 3 is also nixed? > This one will get used in patch 6. With that use it will result in total 4 uses. I won't object if you fix the macro name and remove the comma Patch 3 attempts to create a universal macro for any "file" option, but that can't be really done universally. See for yourself: git grep -A 4 '\.name = "file"' The name of the macro would really need to be specific for pool XML file name. Any other attempt will create confusion and I don't think it's worth. > > Like I noted in the cover letter - I see value in having the macros and > I also see the pain (having to go look them up). > > Adjusting the changes to remove/use commas is fine - just made the > initial cut-n-paste easier > > John signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] vf configuration cleanup when VM is delete
> -Original Message- > From: sendmail [mailto:justsendmailnothinge...@gmail.com] On Behalf Of > Laine Stump > Sent: Wednesday, December 16, 2015 12:31 AM > To: Libvirt > Cc: vyase...@redhat.com; Moshe Levi > Subject: Re: [libvirt] vf configuration cleanup when > VM is delete > > On 12/15/2015 04:12 PM, Vlad Yasevich wrote: > > On 12/15/2015 02:45 PM, Laine Stump wrote: > >> On 12/15/2015 01:34 PM, Laine Stump wrote: > >>> On 12/13/2015 10:51 AM, Moshe Levi wrote: > Hi, > > I have a setup with libvirt 1.3.0 and OpenStack trunk. > > Before launched the VM ip link command show the following VF > mac/vlan configuration [1] > > When I launch a VM with via openstack > api (OpenStack direct > port) > > I can see that the VF get the mac/vlan according to libvrit xml [2] > and ip link command [3], but when I delete the VM the mac/vlan > config are still shown as in [3] and not restored to [1] > > Shouldn’t libvirt restore the mac/vlan to [1]. > > The same problem exists when using > (OpenStack macvtap port) but just for the MAC configuration of the VF. > > >>> What libvirt does is to restore the MAC address to whatever it was > >>> before we set it up for use with a guest. Although there are some > >>> sriov net drivers that (for some unfathomable reason) think it's > >>> cool to assign a random MAC address to each VF at boot time, the > "normal" thing is for the VFs to have a MAC address of all 0's to start with. > >>> So libvirt should be saving 00:00:00:00:00:00 (it will be in the > >>> file > >>> /var/run/libvirt/hostdevmgr/$ifname_vf$vfnum) then setting the MAC > >>> to use; when done, libvirt will read the 00:00:00:00:00:00 and use > >>> netlink to set the MAC address, but this is apparently failing. > >>> > >>> I checked on my Fedora 22 system with the igb driver, and found that > >>> if the MAC address was originally set to something other than 0's, > >>> it was restored properly by libvirt, but if it was set to all 0's > >>> originally, the > attempt to set it back to 0 would fail. > >>> > >>> I then tried doing the same thing with the "ip" utility: > >>> > >>> # ip link set dev p4p2 vf 0 mac 00:00:00:00:00:00 > >>> > >>> and I get the following response: > >>> > >>> RTNETLINK answers: Invalid argument > >>> > >>> So it appears that either the kernel or the NIC driver is refusing > >>> to set the MAC address to all 0's. I'm reasonably certain this is a > >>> regression in the kernel, > >> Sigh. It appears that this has "always" been the case - I just > >> checked on a 2.6.32-573 RHEL kernel, and a 3.10.x RHEL7.2 kernel, and > >> 4.1 (Fedora 22) and both of them also refuse to set the MAC address > >> to 00:00:00:00:00:00. I'm not sure if this limitation is in the NIC driver > >> or > some basic code in the kernel. > > It's considered to be an invalid address by is_valid_ether_addr() function. > > > > There appear to be different behaviour in some adapters. In current > > upstream, it looks like a quarter of the VF capable drivers (bnxt, > > enic, fm10k, sfc) permit VF mac setting of all zeros. The others > > simply use is_valid_ether_addr function without specifically testing > > for all-0. I am not sure if this is HW related or simply oversights... > > Might > want to bringing this up on netdev. > > Thanks, Vlad! > > > Moshe, > > It sounds like in your case it is caused by code in the mlx driver, so > hopefully > you can have some influence there. My path is a bit more difficult, as the > failure on mine is in the igb driver :-) Sure I will take it with Mellanox Driver team Ok, just saw the latest thread mail you can discard my previous one > > Sending a message to netdev is a good idea. It would be wonderful if all the > vendors could agree to: > > 1) initialize all VFs with a MAC address of 0 > 2) allow setting VF MAC address to 0 at any time. > > I'll put that on my to-do list :-P > > > > -vlad > > > >> > >>> although I can't say how long it's been there, as I don't normally > >>> pay attention to this (and as I said, many SRIOV NIC drivers don't > >>> default their VFs to 0 MAC addresses) > >>> > >>> What distro and kernel are you using for your tests? > >>> > >>> > [1] - 24: enp3s0f0: mtu 1500 > qdisc mq master ovs-system state UP mode DEFAULT group default qlen > 1000 > > link/ether e4:1d:2d:a5:f1:22 brd ff:ff:ff:ff:ff:ff > > vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state > auto > > vf 1 MAC 00:00:00:00:00:00, spoof checking off, link-state > auto > > vf 2 MAC 00:00:00:00:00:00, spoof checking off, link-state > auto > > vf 3 MAC 00:00:00:00:00:00, spoof checking off, link-state > auto > > [2] - > > > > > > > > function='0x7'/> > > > > >
Re: [libvirt] [PATCH v2] storage: Attempt to refresh volume after successful wipe volume
On 12/16/2015 08:48 AM, Ján Tomko wrote: > On Tue, Dec 15, 2015 at 03:03:33PM -0500, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1270709 >> >> When a volume wipe is successful, perform a volume refresh afterwards to >> update any volume data that may be used in future volume commands, such as >> volume resize. For a raw file volume, a wipe could truncate the file and >> a followup volume resize the capacity may fail because the volume target >> allocation isn't updated to reflect the wipe activity. >> >> Since the documentation doesn't mention this side effect of the zero >> algorithm for a raw file volume, update the various documentation to >> describe the results. >> > > The documentation does not belong in this patch. Also, we could > intentionally keep it vague so that we don't have to commit to that > behavior. > Adding the documentation was a reaction to your review of v1: http://www.redhat.com/archives/libvir-list/2015-December/msg00464.html where you queried whether we should update the documentation "to reflect that there might not be any pass over the old data". Whether the doc changes are kept or not I suppose then becomes "consensus based". I see value in describing what's done and I see value in being vague enough so that we could change to do something else in the future. The question thus remains, should the current truncate be considered a bug? Or a happy consequence/feature? >> Signed-off-by: John Ferlan >> --- >> v1: >> http://www.redhat.com/archives/libvir-list/2015-December/msg00085.html >> >> Changes since v1: >>* Use the preferred call format from review > >>* Cause error if refreshVol fails > > If my patch adjusting the return value gets pushed before this one: > https://www.redhat.com/archives/libvir-list/2015-December/msg00467.html > > that change is just cosmetic. yep. > Otherwise, I don't think a patch adding refreshVol should be changing > the return value. > So again, your initial review says: More readable as: if (func() < 0) goto cleanup; Which is what I followed. Other than the value of ret being -errno on fdatasync - is the objection based solely on you'd like to see a separate patch to handle the ret differently for the wipeVol call, then a patch for refreshVol? It seemed more straightforward to do it all at once, but if you want 2 patches - that's not a problem. I just want to be clear first... John > Jan > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: Attempt to refresh volume after successful wipe volume
On Tue, Dec 15, 2015 at 03:03:33PM -0500, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1270709 > > When a volume wipe is successful, perform a volume refresh afterwards to > update any volume data that may be used in future volume commands, such as > volume resize. For a raw file volume, a wipe could truncate the file and > a followup volume resize the capacity may fail because the volume target > allocation isn't updated to reflect the wipe activity. > > Since the documentation doesn't mention this side effect of the zero > algorithm for a raw file volume, update the various documentation to > describe the results. > The documentation does not belong in this patch. Also, we could intentionally keep it vague so that we don't have to commit to that behavior. > Signed-off-by: John Ferlan > --- > v1: > http://www.redhat.com/archives/libvir-list/2015-December/msg00085.html > > Changes since v1: >* Use the preferred call format from review >* Cause error if refreshVol fails If my patch adjusting the return value gets pushed before this one: https://www.redhat.com/archives/libvir-list/2015-December/msg00467.html that change is just cosmetic. Otherwise, I don't think a patch adding refreshVol should be changing the return value. Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: Attempt to refresh volume after successful wipe volume
On Wed, Dec 16, 2015 at 14:28:03 +0100, Martin Kletzander wrote: > On Tue, Dec 15, 2015 at 03:03:33PM -0500, John Ferlan wrote: > >https://bugzilla.redhat.com/show_bug.cgi?id=1270709 > > > >When a volume wipe is successful, perform a volume refresh afterwards to > >update any volume data that may be used in future volume commands, such as > >volume resize. For a raw file volume, a wipe could truncate the file and > >a followup volume resize the capacity may fail because the volume target > >allocation isn't updated to reflect the wipe activity. > > > >Since the documentation doesn't mention this side effect of the zero > >algorithm for a raw file volume, update the various documentation to > >describe the results. Since the volume format is not really considered while wiping, this will have also various efects on qcow and other storage formats. > > > > Oh yes, side effects, we have many of those regarding volume wiping. > For this one, I think you cose the best solution, so even though there > are some other hidden things, at least this is cleaned up a bit now. Similarly a sparse file will be filled up to the maximum once you use some of the different algorithms that overwrite the file. Additionally the storage format will change in that case. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] virsh: Create macro for "overwrite" and no-overwrite" options
On 12/16/2015 08:35 AM, Peter Krempa wrote: > On Wed, Dec 16, 2015 at 14:18:22 +0100, Michal Privoznik wrote: >> On 25.11.2015 20:11, John Ferlan wrote: >>> Although not currently used in more than one command, it soon will be >>> so create a common macro to be used in the new command location. >>> >>> Additionally, add the ".flags = 0," for both to match the expections >>> of the structure being predefined. >>> >>> Signed-off-by: John Ferlan >>> --- >>> tools/virsh-pool.c | 24 >>> 1 file changed, 16 insertions(+), 8 deletions(-) >>> >> >> ACK > > I don't really fancy this one. It introduces the macro for a very > uncommon option value. If it will be used more that 10 times it would > make sense. For 2 or 3 uses it does not. > > Peter > So then in your opinion and using the same logic, patch 3 is also nixed? This one will get used in patch 6. Like I noted in the cover letter - I see value in having the macros and I also see the pain (having to go look them up). Adjusting the changes to remove/use commas is fine - just made the initial cut-n-paste easier John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] storage: Add helper to compare logical pool def against pvs output
On Wed, Dec 16, 2015 at 07:46:18AM -0500, John Ferlan wrote: > > > On 12/16/2015 07:02 AM, Ján Tomko wrote: > > On Mon, Dec 07, 2015 at 03:47:58PM -0500, John Ferlan wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1025230 > >> > >> Add a new helper virStorageBackendLogicalMatchPoolSource to compare the > >> pool's source name against the output from a 'pvs' command to list all > >> volume group physical volume data on the host. In addition, compare the > >> pool's source device list against the particular volume group's device > >> list to ensure the source device(s) listed for the pool match what the > >> was listed for the volume group. > >> > >> Then for pool startup or check API's we need to call this new API in > >> order to ensure that the pool we're about to start or declare active > >> during checkPool has a valid definition vs. the running host. > >> > > > > This patch breaks starting of logical pools with no source devices. > > > > Jan > > > > Not enough information for me to go on... Can you provide sample XML > that works prior to the change? From just your description I assume you > mean: > > > xxx > > > In my case it's: vg /dev/vg ... (Also, if the vg_name matches the pool name, the whole element can be omitted) > instead of having a > > > > As the source device > > Without a source device how would pool-build work (vgcreate)? It would not, and pool-build is the only time when the list of physical volumes is required. Libvirt also uses the list for pool-delete, but this is IMO a bug, considering that we do not keep this list up to date and it could end up calling pvremove on PVs that are no longer a part of that VG. Thinking about it some more, the point of LVM is to abstract from the physical volumes, so I do not think we should burden the user with the task of updating the pool's XML every time the underlying PVs change (i.e. the USB hard drives were plugged in in a different order). Instead of logging a warning / rejecting the pool, could we update the source devices list on pool refresh? Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/6] virsh: Add build flags to pool-create[-as] and pool-start
On Wed, Nov 25, 2015 at 14:11:08 -0500, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=830056
>
> Utilize recently added VIR_STORAGE_POOL_CREATE_WITH_BUILD* flags in
> order to pass the flags along to the virStoragePoolCreateXML and
> virStoragePoolCreate API's.
>
> This affects the 'virsh pool-create', 'virsh pool-create-as', and
> 'virsh pool-start' commands. While it could be argued that pool-start
> doesn't need the flags, they could prove useful for someone trying to
> do one command build --overwrite and start command processing or
> essentially starting with a clean slate.
>
> NB:
> This patch is loosely based upon code originally authored by Osier
> Yang that were not reviewed and pushed, see:
>
> https://www.redhat.com/archives/libvir-list/2012-July/msg00497.html
> Signed-off-by: John Ferlan
> ---
> tools/virsh-pool.c | 73
> +++---
> tools/virsh.pod| 25 ++-
> 2 files changed, 94 insertions(+), 4 deletions(-)
>
> diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
> index cb91cd3..1bb987d 100644
> --- a/tools/virsh-pool.c
> +++ b/tools/virsh-pool.c
> @@ -47,6 +47,13 @@
> .help = N_("file containing an XML pool description")\
> },\
>
> +#define OPT_BUILD_COMMON \
> +{.name = "build", \
> + .type = VSH_OT_BOOL, \
> + .flags = 0, \
> + .help = N_("build the pool as normal") \
> +},\
> +
> #define OPT_NO_OVERWRITE_COMMON \
> {.name = "no-overwrite", \
> .type = VSH_OT_BOOL, \
> @@ -235,6 +242,9 @@ static const vshCmdInfo info_pool_create[] = {
>
> static const vshCmdOptDef opts_pool_create[] = {
> OPT_FILE_COMMON
> +OPT_BUILD_COMMON
> +OPT_NO_OVERWRITE_COMMON
> +OPT_OVERWRITE_COMMON
>
These look terrible without commas between entries.
> {.name = NULL}
> };
> @@ -246,15 +256,32 @@ cmdPoolCreate(vshControl *ctl, const vshCmd *cmd)
> const char *from = NULL;
> bool ret = true;
> char *buffer;
> +bool build;
> +bool overwrite;
> +bool no_overwrite;
> +unsigned int flags = 0;
> virshControlPtr priv = ctl->privData;
>
> if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0)
> return false;
>
> +build = vshCommandOptBool(cmd, "build");
> +overwrite = vshCommandOptBool(cmd, "overwrite");
> +no_overwrite = vshCommandOptBool(cmd, "no-overwrite");
> +
> +VSH_EXCLUSIVE_OPTIONS_VAR(overwrite, no_overwrite);
This should be used only if the name of the variable matches the name of
the option flag since the variable name is used in the error message in
place of the option flag name.
> +
> +if (build)
> +flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD;
> +if (overwrite)
> +flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE;
> +if (no_overwrite)
> +flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE;
> +
> if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0)
> return false;
>
> -pool = virStoragePoolCreateXML(priv->conn, buffer, 0);
> +pool = virStoragePoolCreateXML(priv->conn, buffer, flags);
> VIR_FREE(buffer);
>
> if (pool != NULL) {
> @@ -386,6 +413,9 @@ static const vshCmdInfo info_pool_create_as[] = {
>
> static const vshCmdOptDef opts_pool_create_as[] = {
> OPTS_POOL_COMMON_X_AS
> +OPT_BUILD_COMMON
> +OPT_NO_OVERWRITE_COMMON
> +OPT_OVERWRITE_COMMON
>
> {.name = NULL}
> };
> @@ -397,8 +427,25 @@ cmdPoolCreateAs(vshControl *ctl, const vshCmd *cmd)
> const char *name;
> char *xml;
> bool printXML = vshCommandOptBool(cmd, "print-xml");
> +bool build;
> +bool overwrite;
> +bool no_overwrite;
> +unsigned int flags = 0;
> virshControlPtr priv = ctl->privData;
>
> +build = vshCommandOptBool(cmd, "build");
> +overwrite = vshCommandOptBool(cmd, "overwrite");
> +no_overwrite = vshCommandOptBool(cmd, "no-overwrite");
> +
> +VSH_EXCLUSIVE_OPTIONS_VAR(overwrite, no_overwrite);
See above.
> +
> +if (build)
> +flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD;
> +if (overwrite)
> +flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE;
> +if (no_overwrite)
> +flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE;
> +
> if (!virshBuildPoolXML(ctl, cmd, &name, &xml))
> return false;
>
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] virsh: Create macro for "overwrite" and no-overwrite" options
On Wed, Dec 16, 2015 at 14:18:22 +0100, Michal Privoznik wrote: > On 25.11.2015 20:11, John Ferlan wrote: > > Although not currently used in more than one command, it soon will be > > so create a common macro to be used in the new command location. > > > > Additionally, add the ".flags = 0," for both to match the expections > > of the structure being predefined. > > > > Signed-off-by: John Ferlan > > --- > > tools/virsh-pool.c | 24 > > 1 file changed, 16 insertions(+), 8 deletions(-) > > > > ACK I don't really fancy this one. It introduces the macro for a very uncommon option value. If it will be used more that 10 times it would make sense. For 2 or 3 uses it does not. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] virsh: Create macro for "pool" option
On Wed, Dec 16, 2015 at 08:24:43 -0500, John Ferlan wrote:
>
> [...]
>
> >> static const vshCmdOptDef opts_pool_autostart[] = {
> >> -{.name = "pool",
> >> - .type = VSH_OT_DATA,
> >> - .flags = VSH_OFLAG_REQ,
> >> - .help = N_("pool name or uuid")
> >> -},
> >> +OPT_POOL_COMMON
VSH_POOL_OPT_COMMON for consistency with basically all the other macros.
Also please don't include the trailing comma in the macro, it looks
weird c-wise, and will make possible regex matches for code consistency
really weird.
> >> +
> >> {.name = "disable",
> >> .type = VSH_OT_BOOL,
> >> .help = N_("disable autostarting")
> >
> > Nice. ACK
> >
> > Should we do something similar to domain, network, ... in the rest of virsh?
> >
Hiding stuff behind macros can be confusing sometimes, but for the most
commonly used objects it would be reasonable.
>
> I think it would make certain things easier - I did remark on this in
> the cover letter. It gets a bit tedious to do and review; however, I
> think in the long run makes things more consistent. There are a few
> options with "slight" differences, but nothing that cannot be worked out.
Easier? not really, it would just introduce less code when adding new
stuff at the slight cost of having to resolve the macro when looking up
what the stuff is doing. That's why only the most common objects should
be converted.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: Attempt to refresh volume after successful wipe volume
On Tue, Dec 15, 2015 at 03:03:33PM -0500, John Ferlan wrote: https://bugzilla.redhat.com/show_bug.cgi?id=1270709 When a volume wipe is successful, perform a volume refresh afterwards to update any volume data that may be used in future volume commands, such as volume resize. For a raw file volume, a wipe could truncate the file and a followup volume resize the capacity may fail because the volume target allocation isn't updated to reflect the wipe activity. Since the documentation doesn't mention this side effect of the zero algorithm for a raw file volume, update the various documentation to describe the results. Oh yes, side effects, we have many of those regarding volume wiping. For this one, I think you cose the best solution, so even though there are some other hidden things, at least this is cleaned up a bit now. ACK Signed-off-by: John Ferlan --- v1: http://www.redhat.com/archives/libvir-list/2015-December/msg00085.html Changes since v1: * Use the preferred call format from review * Cause error if refreshVol fails * Add wording to docs regarding zero wipe algorithm and truncating the raw file. src/libvirt-storage.c| 9 +++-- src/storage/storage_driver.c | 9 - tools/virsh.pod | 5 + 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c index 66dd9f0..62bb999 100644 --- a/src/libvirt-storage.c +++ b/src/libvirt-storage.c @@ -1725,7 +1725,9 @@ virStorageVolDelete(virStorageVolPtr vol, * @vol: pointer to storage volume * @flags: extra flags; not used yet, so callers should always pass 0 * - * Ensure data previously on a volume is not accessible to future reads + * Ensure data previously on a volume is not accessible to future reads. + * Use of this function is equivalent to calling virStorageVolWipePattern + * with the VIR_STORAGE_VOL_WIPE_ALG_ZERO for the algorithm. * * Returns 0 on success, or -1 on error */ @@ -1766,7 +1768,10 @@ virStorageVolWipe(virStorageVolPtr vol, * @flags: future flags, use 0 for now * * Similar to virStorageVolWipe, but one can choose - * between different wiping algorithms. + * between different wiping algorithms. When choosing the + * zero algorithm (VIR_STORAGE_VOL_WIPE_ALG_ZERO), it is + * possible the target storage backend will truncate the + * file rather than writing a stream of zeroes. * * Returns 0 on success, or -1 on error. */ diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index bbf21f6..2531a88 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2436,7 +2436,14 @@ storageVolWipePattern(virStorageVolPtr obj, goto cleanup; } -ret = backend->wipeVol(obj->conn, pool, vol, algorithm, flags); +if (backend->wipeVol(obj->conn, pool, vol, algorithm, flags) < 0) +goto cleanup; + +if (backend->refreshVol && +backend->refreshVol(obj->conn, pool, vol) < 0) +goto cleanup; + +ret = 0; cleanup: virStoragePoolObjUnlock(pool); diff --git a/tools/virsh.pod b/tools/virsh.pod index 21ae4a3..7f6dc8d 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3509,6 +3509,11 @@ B B: The availability of algorithms may be limited by the version of the C binary installed on the host. +B: Use of the zero algorithm for some storage backends may result +in the truncation of the target file instead of writing all zeroes to the +file. The Storage Driver will refresh the volume allocation and capacity +after successful volume wipe completion. + =item B [I<--pool> I] I Output the volume information as an XML dump to stdout. -- 2.5.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 2/2] qemu: emit 'defined' event after reverted to snapshot
On Wed, Dec 16, 2015 at 07:28:43 -0500, John Ferlan wrote: > > > On 12/16/2015 07:01 AM, Peter Krempa wrote: > > On Wed, Dec 16, 2015 at 12:09:42 +0300, Dmitry Andreev wrote: > >> If config file was changed VIR_DOMAIN_EVENT_DEFINED should be emitted > > > > drop "file" here. The important part is the config itself being changed. > > The file is just a implication of that. > > > > Additionally, commit messages here or in the previous patch don't > > provide any justification for this change. I'd like to have a statement > > why is this important to change. > > > > FWIW: This is what I got from the cover letter: The cover letter is not commited to the repo so it shouldn't be the only place to put such information. I usualy don't even bother to read the cover letter. > > Lack of the event become a problem for virt-manager > https://bugzilla.redhat.com/show_bug.cgi?id=1081148 Okay, that's starting to be reasonable let's say, but ... > > > Without the event, it seems virt-manager doesn't "see" the change and > presents the data prior to snapshot With internal snapshots there's quite a few state changes that can happen according to the source and target state that need to be taken into account. If you follow the snapshot code you'll see that in some cases qemu has to be restarted and in some cases not. In case when qemu is not restarted during reversion we should not emit a lifecycle event. Stop/start of qemu is based on the fact that the "ABI stability" check of the current and target config will not match. This leaves a loophole where e.g. change of a disk source would not be properly tracked. This problem will most likely happen when combining external and internal snapshots ( https://bugzilla.redhat.com/show_bug.cgi?id=880565 ), but also other legitimate changes of config (mostly change of disk source) will not be tracked properly. This is a bug in the snapshot handling code though and qemu should be restarted at that point since it needs to open different files and libvirt needs to label them prior to that. As said, this is a bug in the snapshot code and patching it by doing an event is not right. Additionally if you are going to revert a transient VM snapshot from running state to running state the DEFINED event is totaly bogus since it doesn't have a persistent definition, which is where we refer to it as "DEFINED". In case of offline target states this approach may be valid since the config changes without any notification which is actually what the bug is complaining about. Using the defined event should be used only in such case. One thing to consider then is whether it's worth fixing what bz 1081148 is tracking with this (but the other cases described above should not emit the event then), or whether a different approach is desired. That's why I asked for justification. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] virsh: Create macro for "pool" option
[...]
>> static const vshCmdOptDef opts_pool_autostart[] = {
>> -{.name = "pool",
>> - .type = VSH_OT_DATA,
>> - .flags = VSH_OFLAG_REQ,
>> - .help = N_("pool name or uuid")
>> -},
>> +OPT_POOL_COMMON
>> +
>> {.name = "disable",
>> .type = VSH_OT_BOOL,
>> .help = N_("disable autostarting")
>
> Nice. ACK
>
> Should we do something similar to domain, network, ... in the rest of virsh?
>
I think it would make certain things easier - I did remark on this in
the cover letter. It gets a bit tedious to do and review; however, I
think in the long run makes things more consistent. There are a few
options with "slight" differences, but nothing that cannot be worked out.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Fix event generated for qemuDomainRevertToSnapshot (pause->run)
On Tue, Dec 15, 2015 at 02:18:52PM -0500, John Ferlan wrote:
A closer review of the code shows that for the transition from paused to
running which was supposed to emit the VIR_DOMAIN_EVENT_RESUMED - no event
would be generated. Rather the event is generated when going from running
to running.
Following the 'was_running' boolean shows it is set when the domain obj
is active and the domain obj state is VIR_DOMAIN_RUNNING. So rather than
using was_running to generate the RESUMED event, use !was_running
Signed-off-by: John Ferlan
---
I hope I've read the bread crumbs in the code correctly regarding state
transitions! Saw this while reviewing something else:
http://www.redhat.com/archives/libvir-list/2015-December/msg00330.html
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 783a7cd..deeffc1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15549,7 +15549,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
event = virDomainEventLifecycleNewFromObj(vm,
VIR_DOMAIN_EVENT_STARTED,
detail);
-} else if (was_running) {
+} else if (!was_running) {
/* Transition 8 */
Transition 8 really is paused->running and was_running is set to true
only if the domain state is _RUNNING. And the variable is used only
here, so if I read them correctly, this should be fixed. But I must
say, that snapshot restoring logic... is nasty.
ACK.
detail = VIR_DOMAIN_EVENT_RESUMED;
event = virDomainEventLifecycleNewFromObj(vm,
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] qemu_hotplug: dead code removal
On 12/09/2015 12:08 PM, Ján Tomko wrote: > Originally posted as a part of > https://www.redhat.com/archives/libvir-list/2015-August/msg00521.html > > Ján Tomko (3): > qemu_hotplug: remove qemuDomainAttachDeviceControllerLive > Remove dead code from qemuDomainAttachControllerDevice > mark virDomainVirtioSerialAddrSetAddController as static. > > src/conf/domain_addr.c | 23 +-- > src/conf/domain_addr.h | 8 > src/libvirt_private.syms | 2 -- > src/qemu/qemu_driver.c | 23 +-- > src/qemu/qemu_hotplug.c | 25 +++-- > 5 files changed, 9 insertions(+), 72 deletions(-) > ACK series - although a slight adjustment to commit message as I noted in reply to patch 2 would I think be beneficial. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/6] virsh: Add build flags to pool-create[-as] and pool-start
On 25.11.2015 20:11, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=830056
>
> Utilize recently added VIR_STORAGE_POOL_CREATE_WITH_BUILD* flags in
> order to pass the flags along to the virStoragePoolCreateXML and
> virStoragePoolCreate API's.
>
> This affects the 'virsh pool-create', 'virsh pool-create-as', and
> 'virsh pool-start' commands. While it could be argued that pool-start
> doesn't need the flags, they could prove useful for someone trying to
> do one command build --overwrite and start command processing or
> essentially starting with a clean slate.
>
> NB:
> This patch is loosely based upon code originally authored by Osier
> Yang that were not reviewed and pushed, see:
>
> https://www.redhat.com/archives/libvir-list/2012-July/msg00497.html
> Signed-off-by: John Ferlan
> ---
> tools/virsh-pool.c | 73
> +++---
> tools/virsh.pod| 25 ++-
> 2 files changed, 94 insertions(+), 4 deletions(-)
>
> diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
> index cb91cd3..1bb987d 100644
> --- a/tools/virsh-pool.c
> +++ b/tools/virsh-pool.c
> @@ -47,6 +47,13 @@
> .help = N_("file containing an XML pool description")\
> },\
>
> +#define OPT_BUILD_COMMON \
> +{.name = "build", \
> + .type = VSH_OT_BOOL, \
> + .flags = 0, \
> + .help = N_("build the pool as normal") \
> +},\
> +
> #define OPT_NO_OVERWRITE_COMMON \
> {.name = "no-overwrite", \
> .type = VSH_OT_BOOL, \
> @@ -235,6 +242,9 @@ static const vshCmdInfo info_pool_create[] = {
>
> static const vshCmdOptDef opts_pool_create[] = {
> OPT_FILE_COMMON
> +OPT_BUILD_COMMON
> +OPT_NO_OVERWRITE_COMMON
> +OPT_OVERWRITE_COMMON
>
> {.name = NULL}
> };
> @@ -246,15 +256,32 @@ cmdPoolCreate(vshControl *ctl, const vshCmd *cmd)
> const char *from = NULL;
> bool ret = true;
> char *buffer;
> +bool build;
> +bool overwrite;
> +bool no_overwrite;
> +unsigned int flags = 0;
> virshControlPtr priv = ctl->privData;
>
> if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0)
> return false;
>
> +build = vshCommandOptBool(cmd, "build");
> +overwrite = vshCommandOptBool(cmd, "overwrite");
> +no_overwrite = vshCommandOptBool(cmd, "no-overwrite");
> +
> +VSH_EXCLUSIVE_OPTIONS_VAR(overwrite, no_overwrite);
A-HA! I knew it! These are mutually exclusive. So please do fix 1/6.
> +
> +if (build)
> +flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD;
> +if (overwrite)
> +flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE;
> +if (no_overwrite)
> +flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE;
> +
> if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0)
> return false;
>
> -pool = virStoragePoolCreateXML(priv->conn, buffer, 0);
> +pool = virStoragePoolCreateXML(priv->conn, buffer, flags);
> VIR_FREE(buffer);
>
> if (pool != NULL) {
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] storage: Add flags to allow building pool during create processing
On 25.11.2015 20:11, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=830056
>
> Add flags handling to the virStoragePoolCreate and virStoragePoolCreateXML
> API's which will allow the caller to provide the capability for the storage
> pool create API's to also perform a pool build during creation rather than
> requiring the additional buildPool step. This will allow transient pools
> to be defined, built, and started.
>
> The new flags are:
>
> * VIR_STORAGE_POOL_CREATE_WITH_BUILD
> Perform buildPool without any flags passed.
>
> * VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE
> Perform buildPool using VIR_STORAGE_POOL_BUILD_OVERWRITE flag.
>
> * VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE
> Perform buildPool using VIR_STORAGE_POOL_BUILD_NO_OVERWRITE flag.
>
> It is up to the backend to handle the processing of build flags. The
> overwrite and no-overwrite flags are mutually exclusive.
>
> NB:
> This patch is loosely based upon code originally authored by Osier
> Yang that were not reviewed and pushed, see:
>
> https://www.redhat.com/archives/libvir-list/2012-July/msg01328.html
> Signed-off-by: John Ferlan
> ---
> include/libvirt/libvirt-storage.h | 16 ++-
> src/libvirt-storage.c | 4 ++--
> src/storage/storage_driver.c | 42
> +--
> 3 files changed, 57 insertions(+), 5 deletions(-)
>
> diff --git a/include/libvirt/libvirt-storage.h
> b/include/libvirt/libvirt-storage.h
> index 9fc3c2d..996a726 100644
> --- a/include/libvirt/libvirt-storage.h
> +++ b/include/libvirt/libvirt-storage.h
> @@ -57,7 +57,6 @@ typedef enum {
> # endif
> } virStoragePoolState;
>
> -
> typedef enum {
> VIR_STORAGE_POOL_BUILD_NEW = 0, /* Regular build from scratch */
> VIR_STORAGE_POOL_BUILD_REPAIR = (1 << 0), /* Repair / reinitialize */
> @@ -71,6 +70,21 @@ typedef enum {
> VIR_STORAGE_POOL_DELETE_ZEROED = 1 << 0, /* Clear all data to zeros
> (slow) */
> } virStoragePoolDeleteFlags;
>
> +typedef enum {
> +VIR_STORAGE_POOL_CREATE_NORMAL = 0,
> +
> +/* Create the pool and perform pool build without any flags */
> +VIR_STORAGE_POOL_CREATE_WITH_BUILD = 1 << 0,
> +
> +/* Create the pool and perform pool build using the
> + * VIR_STORAGE_POOL_BUILD_OVERWRITE flag */
> +VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE = 1 << 1,
> +
> +/* Create the pool and perform pool build using the
> + * VIR_STORAGE_POOL_BUILD_NO_OVERWRITE flag */
> +VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE = 1 << 2,
So _OVERWRITE and _NO_OVERWRITE flags are mutually exclusive then,
right? Probably worth noting.
> +} virStoragePoolCreateFlags;
> +
> typedef struct _virStoragePoolInfo virStoragePoolInfo;
>
> struct _virStoragePoolInfo {
> diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c
> index 66dd9f0..238a6cd 100644
> --- a/src/libvirt-storage.c
> +++ b/src/libvirt-storage.c
> @@ -505,7 +505,7 @@ virStoragePoolLookupByVolume(virStorageVolPtr vol)
> * virStoragePoolCreateXML:
> * @conn: pointer to hypervisor connection
> * @xmlDesc: XML description for new pool
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of virStoragePoolCreateFlags
> *
> * Create a new storage based on its XML description. The
> * pool is not persistent, so its definition will disappear
> @@ -670,7 +670,7 @@ virStoragePoolUndefine(virStoragePoolPtr pool)
> /**
> * virStoragePoolCreate:
> * @pool: pointer to storage pool
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of virStoragePoolCreateFlags
> *
> * Starts an inactive storage pool
> *
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index bbf21f6..59a18bf 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -671,8 +671,11 @@ storagePoolCreateXML(virConnectPtr conn,
> virStoragePoolPtr ret = NULL;
> virStorageBackendPtr backend;
> char *stateFile = NULL;
> +unsigned int build_flags = 0;
>
> -virCheckFlags(0, NULL);
> +virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD |
> + VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE |
> + VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE, NULL);
How about VIR_EXCLUSIVE_FLAGS_RET(_OVERWRITE, _NO_OVERWRITE, NULL);
>
> storageDriverLock();
> if (!(def = virStoragePoolDefParseString(xml)))
> @@ -694,6 +697,22 @@ storagePoolCreateXML(virConnectPtr conn,
> goto cleanup;
> def = NULL;
>
> +if (backend->buildPool) {
> +if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE)
> +build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE;
> +else if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE)
> +build_flags |= VIR_STORAGE_POOL_BUILD_NO_OVERWRITE;
> +
> +if (build_flags ||
> +
Re: [libvirt] [PATCH 2/6] virsh: Create macro for "pool" option
On 25.11.2015 20:11, John Ferlan wrote:
> Rather than continually cut/paste the "pool" option for pool command
> option structures, generate a macro which will commonly define it for
> any command. Then of course use that macro.
>
> Signed-off-by: John Ferlan
> ---
> tools/virsh-pool.c | 91
> +++---
> 1 file changed, 31 insertions(+), 60 deletions(-)
>
> diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
> index cf5a8f3..6922ad5 100644
> --- a/tools/virsh-pool.c
> +++ b/tools/virsh-pool.c
> @@ -33,6 +33,13 @@
> #include "conf/storage_conf.h"
> #include "virstring.h"
>
> +#define OPT_POOL_COMMON \
> +{.name = "pool", \
> + .type = VSH_OT_DATA, \
> + .flags = VSH_OFLAG_REQ, \
> + .help = N_("pool name or uuid") \
> +},\
> +
> virStoragePoolPtr
> virshCommandOptPoolBy(vshControl *ctl, const vshCmd *cmd, const char
> *optname,
>const char **name, unsigned int flags)
> @@ -85,11 +92,8 @@ static const vshCmdInfo info_pool_autostart[] = {
> };
>
> static const vshCmdOptDef opts_pool_autostart[] = {
> -{.name = "pool",
> - .type = VSH_OT_DATA,
> - .flags = VSH_OFLAG_REQ,
> - .help = N_("pool name or uuid")
> -},
> +OPT_POOL_COMMON
> +
> {.name = "disable",
> .type = VSH_OT_BOOL,
> .help = N_("disable autostarting")
Nice. ACK
Should we do something similar to domain, network, ... in the rest of virsh?
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] virsh: Create macro for "overwrite" and no-overwrite" options
On 25.11.2015 20:11, John Ferlan wrote: > Although not currently used in more than one command, it soon will be > so create a common macro to be used in the new command location. > > Additionally, add the ".flags = 0," for both to match the expections > of the structure being predefined. > > Signed-off-by: John Ferlan > --- > tools/virsh-pool.c | 24 > 1 file changed, 16 insertions(+), 8 deletions(-) > ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] virsh: Create a macro for pool-define-as and pool-create-as options
On 25.11.2015 20:11, John Ferlan wrote: > Although they both are the same now, a future patch will add new options > to pool-create-as. So create a common macro to capture commonality, then > use that in the command specific structure. > > Signed-off-by: John Ferlan > --- > tools/virsh-pool.c | 151 > - > 1 file changed, 79 insertions(+), 72 deletions(-) ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] virsh: Create macro for "file" option
On 25.11.2015 20:11, John Ferlan wrote: > Rather than continually cut/paste the "file" option for pool command > option structures, generate a macro which will commonly define it for > any command. Then of course use that macro. > > Signed-off-by: John Ferlan > --- > tools/virsh-pool.c | 21 +++-- > 1 file changed, 11 insertions(+), 10 deletions(-) > ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] Remove dead code from qemuDomainAttachControllerDevice
On 12/09/2015 12:08 PM, Ján Tomko wrote:
> We only support hotplugging SCSI controllers,
> USB and virtio-serial related code is useless here.
> ---
> src/conf/domain_addr.c | 21 -
> src/conf/domain_addr.h | 4
> src/libvirt_private.syms | 1 -
> src/qemu/qemu_hotplug.c | 18 --
> 4 files changed, 44 deletions(-)
Perhaps helpful to add a bit of history:
This reverts commit id 'ee0d97a77', part of '16db8d2e', and part of
commit id 'd6d54cd1'. Although for that last one, if one goes and
checks out the history - there's no way the code ever have been called
since the only way into the function where it was added is if the
controller type is VIR_DOMAIN_CONTROLLER_TYPE_SCSI
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index ca5803e..c7eab0c 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -884,27 +884,6 @@
> virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr
> addrs
> return 0;
> }
>
> -/* virDomainVirtioSerialAddrSetRemoveController
> - *
> - * Removes a virtio serial controller from the address set.
> - */
> -void
> -virDomainVirtioSerialAddrSetRemoveController(virDomainVirtioSerialAddrSetPtr
> addrs,
> - virDomainControllerDefPtr cont)
> -{
> -ssize_t pos;
> -
> -if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
> -return;
> -
> -VIR_DEBUG("Removing virtio serial controller index %u "
> - "from the address set", cont->idx);
> -
> -pos = virDomainVirtioSerialAddrFindController(addrs, cont->idx);
> -
> -if (pos >= 0)
> -VIR_DELETE_ELEMENT(addrs->controllers, pos, addrs->ncontrollers);
> -}
>
> void
> virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs)
> diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
> index 2220a79..74f414e 100644
> --- a/src/conf/domain_addr.h
> +++ b/src/conf/domain_addr.h
> @@ -209,10 +209,6 @@ int
> virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr
> addrs,
>virDomainControllerDefPtr cont)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> -void
> -virDomainVirtioSerialAddrSetRemoveController(virDomainVirtioSerialAddrSetPtr
> addrs,
> - virDomainControllerDefPtr cont)
> -ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> int
> virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr
> addrs,
> virDomainDefPtr def)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 63d8618..536acab 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -115,7 +115,6 @@ virDomainVirtioSerialAddrSetAddController;
> virDomainVirtioSerialAddrSetAddControllers;
> virDomainVirtioSerialAddrSetCreate;
> virDomainVirtioSerialAddrSetFree;
> -virDomainVirtioSerialAddrSetRemoveController;
>
>
> # conf/domain_audit.h
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index eae5418..9bd4238 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -442,7 +442,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr
> driver,
> char *devstr = NULL;
> qemuDomainObjPrivatePtr priv = vm->privateData;
> bool releaseaddr = false;
> -bool addedToAddrSet = false;
>
> if (controller->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> @@ -484,20 +483,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr
> driver,
> if (qemuAssignDeviceControllerAlias(vm->def, priv->qemuCaps,
> controller) < 0)
> goto cleanup;
>
> -if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
> -controller->model == -1 &&
> -!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("USB controller hotplug unsupported in this
> QEMU binary"));
> -goto cleanup;
> -}
> -
> -if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL &&
> -virDomainVirtioSerialAddrSetAddController(priv->vioserialaddrs,
> - controller) < 0)
> -goto cleanup;
> -addedToAddrSet = true;
> -
> if (!(devstr = qemuBuildControllerDevStr(vm->def, controller,
> priv->qemuCaps, NULL)))
> goto cleanup;
> }
> @@ -526,9 +511,6 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr
> driver,
> }
>
> cleanup:
> -if (ret != 0 && addedToAddrSet)
> -virDomainVirtioSerialAddrSetRemoveController(priv->vioserialaddrs,
> - controller);
> if (ret != 0 && releaseaddr)
> qemuDomainReleaseDevi
Re: [libvirt] vf configuration cleanup when VM is delete
To clean up the VF I use ip link set dev p4p2 vf 0 mac 0 and it working 24: enp3s0f0: mtu 1500 qdisc mq master ovs-system state UP mode DEFAULT group default qlen 1000 link/ether e4:1d:2d:a5:f1:22 brd ff:ff:ff:ff:ff:ff vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state auto vf 1 MAC 00:00:00:00:00:00, spoof checking off, link-state auto vf 2 MAC 00:00:00:00:00:b1, vlan 190, spoof checking off, link-state enable vf 3 MAC aa:bb:cc:00:00:12, vlan 190, spoof checking off, link-state enable [root@r-ufm160 devstack]# ip link set dev enp3s0f0 vf 3 mac 0 [root@r-ufm160 devstack]# ip link show enp3s0f0 24: enp3s0f0: mtu 1500 qdisc mq master ovs-system state UP mode DEFAULT group default qlen 1000 link/ether e4:1d:2d:a5:f1:22 brd ff:ff:ff:ff:ff:ff vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state auto vf 1 MAC 00:00:00:00:00:00, spoof checking off, link-state auto vf 2 MAC 00:00:00:00:00:b1, vlan 190, spoof checking off, link-state enable vf 3 MAC 00:00:00:00:00:b1, vlan 190, spoof checking off, link-state enable It just put the address 00:00:00:00:00:b1 which I don’t know why, but as I remember the same behavior is in intel cards (I think is related to iproute) I used fedora 2.1 with kernel 4.1.13-100. The most annoying part is that in OpenStack if I use an SR-IOV VF (interface hostdev) for VM and delete it I can’t reuse it for macvtap (interface direct) so I have to clean the mac by running ip link set dev p4p2 vf 0 mac 0 I guess I will need to workaround it in OpenStack. From: sendmail [mailto:justsendmailnothinge...@gmail.com] On Behalf Of Laine Stump Sent: Tuesday, December 15, 2015 9:45 PM To: Libvirt Cc: Moshe Levi ; vyase...@redhat.com Subject: Re: [libvirt] vf configuration cleanup when VM is delete On 12/15/2015 01:34 PM, Laine Stump wrote: On 12/13/2015 10:51 AM, Moshe Levi wrote: Hi, I have a setup with libvirt 1.3.0 and OpenStack trunk. Before launched the VM ip link command show the following VF mac/vlan configuration [1] When I launch a VM with via openstack api (OpenStack direct port) I can see that the VF get the mac/vlan according to libvrit xml [2] and ip link command [3], but when I delete the VM the mac/vlan config are still shown as in [3] and not restored to [1] Shouldn’t libvirt restore the mac/vlan to [1]. The same problem exists when using (OpenStack macvtap port) but just for the MAC configuration of the VF. What libvirt does is to restore the MAC address to whatever it was before we set it up for use with a guest. Although there are some sriov net drivers that (for some unfathomable reason) think it's cool to assign a random MAC address to each VF at boot time, the "normal" thing is for the VFs to have a MAC address of all 0's to start with. So libvirt should be saving 00:00:00:00:00:00 (it will be in the file /var/run/libvirt/hostdevmgr/$ifname_vf$vfnum) then setting the MAC to use; when done, libvirt will read the 00:00:00:00:00:00 and use netlink to set the MAC address, but this is apparently failing. I checked on my Fedora 22 system with the igb driver, and found that if the MAC address was originally set to something other than 0's, it was restored properly by libvirt, but if it was set to all 0's originally, the attempt to set it back to 0 would fail. I then tried doing the same thing with the "ip" utility: # ip link set dev p4p2 vf 0 mac 00:00:00:00:00:00 and I get the following response: RTNETLINK answers: Invalid argument So it appears that either the kernel or the NIC driver is refusing to set the MAC address to all 0's. I'm reasonably certain this is a regression in the kernel, Sigh. It appears that this has "always" been the case - I just checked on a 2.6.32-573 RHEL kernel, and a 3.10.x RHEL7.2 kernel, and 4.1 (Fedora 22) and both of them also refuse to set the MAC address to 00:00:00:00:00:00. I'm not sure if this limitation is in the NIC driver or some basic code in the kernel. although I can't say how long it's been there, as I don't normally pay attention to this (and as I said, many SRIOV NIC drivers don't default their VFs to 0 MAC addresses) What distro and kernel are you using for your tests? [1] - 24: enp3s0f0: mtu 1500 qdisc mq master ovs-system state UP mode DEFAULT group default qlen 1000 link/ether e4:1d:2d:a5:f1:22 brd ff:ff:ff:ff:ff:ff vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state auto vf 1 MAC 00:00:00:00:00:00, spoof checking off, link-state auto vf 2 MAC 00:00:00:00:00:00, spoof checking off, link-state auto vf 3 MAC 00:00:00:00:00:00, spoof checking off, link-state auto [2] - [3] 24: enp3s0f0: mtu 1500 qdisc mq master ovs-system state UP mode DEFAULT group default qlen 1000 link/ether e4:1d:2d:a5:f1:22 brd ff:ff:ff:ff:ff:ff vf 0 MAC 00:00:00:00:00:00, spoof checking off, link-state auto vf 1 MAC 00:00:00:00:00:00, spoof checking off, link
Re: [libvirt] [PATCH 5/5] storage: Add helper to compare logical pool def against pvs output
On 12/16/2015 07:02 AM, Ján Tomko wrote: > On Mon, Dec 07, 2015 at 03:47:58PM -0500, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1025230 >> >> Add a new helper virStorageBackendLogicalMatchPoolSource to compare the >> pool's source name against the output from a 'pvs' command to list all >> volume group physical volume data on the host. In addition, compare the >> pool's source device list against the particular volume group's device >> list to ensure the source device(s) listed for the pool match what the >> was listed for the volume group. >> >> Then for pool startup or check API's we need to call this new API in >> order to ensure that the pool we're about to start or declare active >> during checkPool has a valid definition vs. the running host. >> > > This patch breaks starting of logical pools with no source devices. > > Jan > Not enough information for me to go on... Can you provide sample XML that works prior to the change? From just your description I assume you mean: xxx instead of having a As the source device Without a source device how would pool-build work (vgcreate)? Without a volume group, then vgchange (what pool-start calls) won't work. However, instead of getting: virsh pool-start lvm_test error: Failed to start pool lvm_test error: internal error: Child process (/usr/sbin/vgchange -aly lvm_test) unexpected exit status 5: Volume group "lvm_test" not found Cannot process volume group lvm_test One would get: virsh pool-start lvm_test error: Failed to start pool lvm_test error: unsupported configuration: cannot find logical volume group name 'lvm_test' Tks - John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 2/2] qemu: emit 'defined' event after reverted to snapshot
On 12/16/2015 07:01 AM, Peter Krempa wrote:
> On Wed, Dec 16, 2015 at 12:09:42 +0300, Dmitry Andreev wrote:
>> If config file was changed VIR_DOMAIN_EVENT_DEFINED should be emitted
>
> drop "file" here. The important part is the config itself being changed.
> The file is just a implication of that.
>
> Additionally, commit messages here or in the previous patch don't
> provide any justification for this change. I'd like to have a statement
> why is this important to change.
>
FWIW: This is what I got from the cover letter:
Lack of the event become a problem for virt-manager
https://bugzilla.redhat.com/show_bug.cgi?id=1081148
Without the event, it seems virt-manager doesn't "see" the change and
presents the data prior to snapshot
John
>> ---
>> src/qemu/qemu_driver.c | 13 +++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 783a7cd..1474eaa 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -15512,7 +15512,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
>> snapshot,
>> VIR_DOMAIN_EVENT_STARTED,
>> detail);
>> if (rc < 0)
>> -goto endjob;
>> +goto defined;
>> }
>>
>> /* Touch up domain state. */
>> @@ -15534,7 +15534,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
>> snapshot,
>> if (!virDomainObjIsActive(vm)) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("guest unexpectedly quit"));
>> -goto endjob;
>> +goto defined;
>> }
>> rc = qemuProcessStartCPUs(driver, vm, snapshot->domain->conn,
>>VIR_DOMAIN_RUNNING_FROM_SNAPSHOT,
>> @@ -15636,6 +15636,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
>> snapshot,
>>
>> ret = 0;
>>
>> + defined:
>> +if (config) {
>> +virObjectEventPtr define_event;
>> +define_event = virDomainEventLifecycleNewFromObj(vm,
>> +VIR_DOMAIN_EVENT_DEFINED,
>> +VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT);
>> +qemuDomainEventQueue(driver, define_event);
>> +}
>
> This will emit the event even if the configuration itself didn't change.
> We do a similar thing when you switch off the VM. The next-start config
> replaces the current config. This is implied by the events of starting
> and stopping of the VM.
>
> Since the internal snapshot code doesn't restart qemu in some cases,
> that's when the configuration can't change (which actually might be
> implemented wrong in a few places). For snapshot state transitions that
> change domain state, the appropriate events are already used.
>
> As of the reasons above, I'd like you to provide some justification why
> this is a good thing to do.
>
> Peter
>
>
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 2/2] qemu: emit 'defined' event after reverted to snapshot
On 12/16/2015 04:09 AM, Dmitry Andreev wrote:
> If config file was changed VIR_DOMAIN_EVENT_DEFINED should be emitted
> ---
> src/qemu/qemu_driver.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
I liked v1 better with a tweak or two
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 783a7cd..1474eaa 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15512,7 +15512,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
> VIR_DOMAIN_EVENT_STARTED,
> detail);
> if (rc < 0)
> -goto endjob;
> +goto defined;
> }
>
> /* Touch up domain state. */
> @@ -15534,7 +15534,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("guest unexpectedly quit"));
> -goto endjob;
> +goto defined;
There's done more goto endjob after here which would seemingly skip the
defined event.
There's also another "if (config)" in the SHUTDOWN, SHUTOFF, CRASHED
case that would be missed due to a goto cleanup
I can fiddle with v1 slightly and have it do the right thing.
John
> }
> rc = qemuProcessStartCPUs(driver, vm, snapshot->domain->conn,
>VIR_DOMAIN_RUNNING_FROM_SNAPSHOT,
> @@ -15636,6 +15636,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
>
> ret = 0;
>
> + defined:
> +if (config) {
> +virObjectEventPtr define_event;
> +define_event = virDomainEventLifecycleNewFromObj(vm,
> +VIR_DOMAIN_EVENT_DEFINED,
> +VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT);
> +qemuDomainEventQueue(driver, define_event);
> +}
> +
> endjob:
> qemuProcessEndJob(driver, vm);
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Search all nodes for shared memory access
In commit 686eb7a24f7d, the break was not considered part of the
condition, hence breaking after first node when searching.
Signed-off-by: Martin Kletzander
---
Pushed as 'trivial' and also John ACKed it earlier.
src/qemu/qemu_process.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ba8dfebd1357..f2740687f655 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4781,9 +4781,10 @@ qemuProcessLaunch(virConnectPtr conn,
if (!shmem && vm->def->mem.nhugepages) {
for (i = 0; i < virDomainNumaGetNodeCount(vm->def->numa); i++) {
if (virDomainNumaGetNodeMemoryAccessMode(vm->def->numa, i) ==
-VIR_NUMA_MEM_ACCESS_SHARED)
+VIR_NUMA_MEM_ACCESS_SHARED) {
shmem = true;
-break;
+break;
+}
}
}
--
2.6.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] ARM KVM GICv3 Support
On Tue, Dec 15, 2015 at 05:01:39PM +, Peter Maydell wrote: On 15 December 2015 at 16:57, Andre Przywara wrote: Even that wouldn't help us, I guess, as you cannot easily check for GICv3/GICv2 compatibility with a _script_. Having access to ioctl's make this pretty easy though: Just try to call KVM_CREATE_DEVICE with the proper type and get -ENODEV if this one is not supported. This can be done without any extra userland tool by just executing some ioctls on /dev/kvm (from C or using some helper library). kvm-ok already runs a few external helper binaries for some things. (Also you can do ioctls from a script if it's a perl script ;-)) As you say the actual technical details of how to query for the host's current supported functionality are straightforward, so it's just a question of how libvirt is expecting that to be exposed to it. We currently probe the host features as well using som ioctls on /dev/kvm, etc. There is no problem with adding any other probing. If qemu can report what it supports, that's good too. So I see it from the other side. It's straightforward to implement it in libvirt, so for me it's just a question of how exactly should we do that. What should we probe fr and also the outcome: what to (dis)allow for a domain. thanks -- PMM signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] storage: Add helper to compare logical pool def against pvs output
On Mon, Dec 07, 2015 at 03:47:58PM -0500, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1025230 > > Add a new helper virStorageBackendLogicalMatchPoolSource to compare the > pool's source name against the output from a 'pvs' command to list all > volume group physical volume data on the host. In addition, compare the > pool's source device list against the particular volume group's device > list to ensure the source device(s) listed for the pool match what the > was listed for the volume group. > > Then for pool startup or check API's we need to call this new API in > order to ensure that the pool we're about to start or declare active > during checkPool has a valid definition vs. the running host. > This patch breaks starting of logical pools with no source devices. Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 2/2] qemu: emit 'defined' event after reverted to snapshot
On Wed, Dec 16, 2015 at 12:09:42 +0300, Dmitry Andreev wrote:
> If config file was changed VIR_DOMAIN_EVENT_DEFINED should be emitted
drop "file" here. The important part is the config itself being changed.
The file is just a implication of that.
Additionally, commit messages here or in the previous patch don't
provide any justification for this change. I'd like to have a statement
why is this important to change.
> ---
> src/qemu/qemu_driver.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 783a7cd..1474eaa 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15512,7 +15512,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
> VIR_DOMAIN_EVENT_STARTED,
> detail);
> if (rc < 0)
> -goto endjob;
> +goto defined;
> }
>
> /* Touch up domain state. */
> @@ -15534,7 +15534,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("guest unexpectedly quit"));
> -goto endjob;
> +goto defined;
> }
> rc = qemuProcessStartCPUs(driver, vm, snapshot->domain->conn,
>VIR_DOMAIN_RUNNING_FROM_SNAPSHOT,
> @@ -15636,6 +15636,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
>
> ret = 0;
>
> + defined:
> +if (config) {
> +virObjectEventPtr define_event;
> +define_event = virDomainEventLifecycleNewFromObj(vm,
> +VIR_DOMAIN_EVENT_DEFINED,
> +VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT);
> +qemuDomainEventQueue(driver, define_event);
> +}
This will emit the event even if the configuration itself didn't change.
We do a similar thing when you switch off the VM. The next-start config
replaces the current config. This is implied by the events of starting
and stopping of the VM.
Since the internal snapshot code doesn't restart qemu in some cases,
that's when the configuration can't change (which actually might be
implemented wrong in a few places). For snapshot state transitions that
change domain state, the appropriate events are already used.
As of the reasons above, I'd like you to provide some justification why
this is a good thing to do.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V2] Xen: support maxvcpus in xm and xl config
On Wed, 2015-12-16 at 10:11 +, Ian Campbell wrote: > On Tue, 2015-12-15 at 15:20 -0700, Jim Fehlig wrote: > > From: Ian Campbell > > > > xend prior to 4.0 understands vcpus as maxvcpus and vcpu_avail > > as a bit map of which cpus are online (default is all). > > > > xend from 4.0 onwards understands maxvcpus as maxvcpus and > > vcpus as the number which are online (from 0..N-1). The > > upstream commit (68a94cf528e6 "xm: Add maxvcpus support") > > claims that if maxvcpus is omitted then the old behaviour > > (i.e. obeying vcpu_avail) is retained, but AFAICT it was not, > > in this case vcpu==maxcpus==online cpus. This is good for us > > because handling anything else would be fiddly. > > > > This patch changes parsing of the virDomainDef maxvcpus and vcpus > > entries to use the corresponding 'maxvcpus' and 'vcpus' settings > > from xm and xl config. It also drops use of the old Xen 3.x > > 'vcpu_avail' setting. > > > > The change also removes the maxvcpus limit of MAX_VIRT_VCPUS (since > > maxvcpus is simply a count, not a bit mask), which is particularly > > crucial on ARM where MAX_VIRT_CPUS == 1 (since all guests are > > expected to support vcpu placement, and therefore only the boot > > vcpu's info lives in the shared info page). > > > > Existing tests adjusted accordingly, and new tests added for the > > 'maxvcpus' setting. > > > > Signed-off-by: Jim Fehlig > > Acked-by: Ian Campbell > Tested-by: Ian Campbell > (as far as "domxml-from-native xen-xl" goes, I seem to have another issue > actually starting a domain on ARM, which I'll investigate...) Turned out to be a mismatch between my build-time and run-time libxl versions, fixed by a clean rebuild. So that's a full Tested-by. I noticed that the newer cmdline= (inplace of root=+extra= etc) wasn't supported. I'll knock something up. Ian. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/16] Xen: remove xend config version
On Wed, 2015-12-16 at 09:45 +, Ian Campbell wrote: > On Tue, 2015-12-15 at 14:32 -0700, Jim Fehlig wrote: > > Hi All, > > > > Ian Campbell recently attempted [1] to fix and issue around > > MAX_VIRT_VPUS > > on ARM that required adding a new XEND_CONFIG_VERSION. After some > > discussion [2] we decided to drop support for all of the old xend > > config > > versions and go with the version supported in Xen 4.0, since the xl > > syntax > > was originally based on (and intended to be compatible with) xm circa > > that > > point in time. > > > > This series removes all traces of xend config version from the > > codebase, > > essentially removing support for Xen 3.x. Hopefully I succeeding in > > making > > the rather large series reviewable. The series is also available on the > > remove-xend-config-version branch of my libvirt github clone [2]. > > Wow, thanks for offering to take this over, I had no idea it would end up > with so much yakk hair everywhere! I've looked through it and the transformations look good to me: Acked-by: Ian Campbell There were a couple of references to xendConfigVersion remaining in comments on src/xen/xen_driver.c. There was also po/* which I presume some sort of semiautomated update will strip eventually. I'm not sure what to make of the references under docs/api_extension/. I tested this on ARM with "Xen: support maxvcpus in xm and xl config" on top and it worked. Ian. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V2] Xen: support maxvcpus in xm and xl config
On Tue, 2015-12-15 at 15:20 -0700, Jim Fehlig wrote: > From: Ian Campbell > > xend prior to 4.0 understands vcpus as maxvcpus and vcpu_avail > as a bit map of which cpus are online (default is all). > > xend from 4.0 onwards understands maxvcpus as maxvcpus and > vcpus as the number which are online (from 0..N-1). The > upstream commit (68a94cf528e6 "xm: Add maxvcpus support") > claims that if maxvcpus is omitted then the old behaviour > (i.e. obeying vcpu_avail) is retained, but AFAICT it was not, > in this case vcpu==maxcpus==online cpus. This is good for us > because handling anything else would be fiddly. > > This patch changes parsing of the virDomainDef maxvcpus and vcpus > entries to use the corresponding 'maxvcpus' and 'vcpus' settings > from xm and xl config. It also drops use of the old Xen 3.x > 'vcpu_avail' setting. > > The change also removes the maxvcpus limit of MAX_VIRT_VCPUS (since > maxvcpus is simply a count, not a bit mask), which is particularly > crucial on ARM where MAX_VIRT_CPUS == 1 (since all guests are > expected to support vcpu placement, and therefore only the boot > vcpu's info lives in the shared info page). > > Existing tests adjusted accordingly, and new tests added for the > 'maxvcpus' setting. > > Signed-off-by: Jim Fehlig Acked-by: Ian Campbell Tested-by: Ian Campbell (as far as "domxml-from-native xen-xl" goes, I seem to have another issue actually starting a domain on ARM, which I'll investigate...) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 01/13] virstoragefile: Add virStorageSourceGetBackingStore
On Mon, Dec 14, 2015 at 10:57 PM, John Ferlan wrote:
>
>
> On 12/03/2015 09:35 AM, Matthias Gatto wrote:
>> Create virStorageSourceGetBackingStore function in
>> preparation for quorum:
>> Actually, if we want to get a backing store inside a virStorageSource
>> we have to do it manually(src->backingStore = backingStore).
>> The problem is that with a quorum, a virStorageSource
>> can contain multiple backing stores, and src->backingStore can
>> be treated as a virStorageSourcePtr or a virStorageSourcePtrPtr.
>>
>> Due to these reason, we need to simplify the manipulation of
>> virStorageSource, and create a function to get the asked
>> backingStore in a virStorageSource
>>
>> For now, this function only return the backingStore field
>
> More simply said -
>
> Create helper virStorageSourceGetBackingStore in order to make it easier
> to access the storage backingStore pointer. Future patches will adjust
> the backingStore pointer to become a table or list of backingStorePtr's
>
>
> [Sure you're doing it because of the quorum changes, but it's
> essentially creating an accessor function]
>>
>> Signed-off-by: Matthias Gatto
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/virstoragefile.c | 10 ++
>> src/util/virstoragefile.h | 3 +++
>> 3 files changed, 14 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index dd085c3..5354a4a 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2190,6 +2190,7 @@ virStorageSourceClear;
>> virStorageSourceCopy;
>> virStorageSourceFree;
>> virStorageSourceGetActualType;
>> +virStorageSourceGetBackingStore;
>> virStorageSourceGetSecurityLabelDef;
>> virStorageSourceInitChainElement;
>> virStorageSourceIsEmpty;
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 2aa1d90..016beaa 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -1809,6 +1809,16 @@ virStorageSourcePoolDefCopy(const
>> virStorageSourcePoolDef *src)
>> }
>>
>>
>> +virStorageSourcePtr
>> +virStorageSourceGetBackingStore(const virStorageSource *src,
>> +size_t pos ATTRIBUTE_UNUSED)
>> +{
>> +if (!src)
>
> I think perhaps Peter's point from his review is
>
> if (!src || pos > 0)
>
> IOW: range checking for pos
>
> Eventually patch 5 will make a real check...
>
> In order to make some progress on this series - at least the first 5 or
> 6 patches - I can make the change if that is what Peter had in mind...
>
> John
>> +return NULL;
>> +return src->backingStore;
>> +}
>> +
>> +
>> /**
>> * virStorageSourcePtr:
>> *
>> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
>> index b98fe25..8cd4854 100644
>> --- a/src/util/virstoragefile.h
>> +++ b/src/util/virstoragefile.h
>> @@ -289,6 +289,9 @@ struct _virStorageSource {
>> # define DEV_BSIZE 512
>> # endif
>>
>> +virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource
>> *src,
>> +size_t pos);
>> +
>> int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid);
>> int virStorageFileProbeFormatFromBuf(const char *path,
>> char *buf,
>>
Thank you for the review, and your changes on v8.
Sorry for the check, I was thinking it unnecessary because we do it on patch 5.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Xen-devel] [PATCH LIBVIRT] libxl: Use libxentoollog in preference to libxenctrl if available.
On Tue, 2015-12-15 at 16:15 -0700, Jim Fehlig wrote: > On 12/14/2015 04:37 AM, Ian Campbell wrote: > > On Mon, 2015-12-14 at 11:15 +, Daniel P. Berrange wrote: > > > On Thu, Dec 10, 2015 at 11:38:36AM +, Ian Campbell wrote: > > > > Upstream Xen is in the process of splitting the (stable API) xtl_* > > > > interfaces out from the (unstable API) libxenctrl library and into > > > > a > > > > new (stable API) libxentoollog. > > > > > > > > In order to be compatible with Xen both before and after this > > > > transition check for xtl_createlogger_stdiostream in a > > > > libxentoollog > > > > library and use it if present. If it is not present assume it is in > > > > libxenctrl. > > > Ok, so there's no API changes, just move stuf from one to the other. > > Indeed, it should really have been a separate library all along and the > > API > > already setup that way. > > > > I'm working on some other library splits, which will involve API > > changes, > > but AFAIK they are all isolated from libvirt via the use of libxl, so > > there > > should be no churn for you guys other than this one patch. > > > > > > It might be nice to get this into 1.3.0 so that supports Xen 4.7 > > > > out > > > > of the box? Not sure what the libvirt stable backport policy is but > > > > it > > > > might also be good to eventually consider it for that? > > > We've missed 1.3.0 release, but I'd be ok with adding it to the > > > stable branch if that's going to be useful. > > I think it would, to allow things to build with Xen 4.7 (when it is > > released). > > I'm not sure it is necessary. libvirt is released monthly, so there will be > several releases before Xen 4.7 is released. AH, I didn't realise it was on such a fast cadence, that's ok then. > > [...] > > > > > Looks, fine from me but will let Jim push it. > > I've pushed the patch to master, but not the 1.3.0 maint branch. It will be > included in the 1.3.1 release planned for mid January. Ian, do you think that > is > sufficient? Easily, thanks. Ian. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/16] Xen: remove xend config version
On Tue, 2015-12-15 at 14:32 -0700, Jim Fehlig wrote: > Hi All, > > Ian Campbell recently attempted [1] to fix and issue around MAX_VIRT_VPUS > on ARM that required adding a new XEND_CONFIG_VERSION. After some > discussion [2] we decided to drop support for all of the old xend config > versions and go with the version supported in Xen 4.0, since the xl syntax > was originally based on (and intended to be compatible with) xm circa that > point in time. > > This series removes all traces of xend config version from the codebase, > essentially removing support for Xen 3.x. Hopefully I succeeding in making > the rather large series reviewable. The series is also available on the > remove-xend-config-version branch of my libvirt github clone [2]. Wow, thanks for offering to take this over, I had no idea it would end up with so much yakk hair everywhere! Ian. > > [1] https://www.redhat.com/archives/libvir-list/2015-November/msg01153.ht > ml > [2] https://www.redhat.com/archives/libvir-list/2015-December/msg00148.ht > ml > [3] https://github.com/jfehlig/libvirt/tree/remove-xend-config-version > > Jim Fehlig (16): > Xen: tests: remove old xm config tests > Xen: tests: remove net-ioemu xm config test > Xen: tests: remove old sexpr2xml tests > Xen: tests: remove old xml2sexpr tests > Xen: tests: use latest XEND_CONFIG_VERSION in xm/xl tests > Xen: xenconfig: remove XEND_CONFIG_VERSION in common code > Xen: xenconfig: remove use of XEND_CONFIG_VERSION in xen_xm > Xen: xenconfig: remove xendConfigVersion from public functions > Xen: tests: use latest XEND_CONFIG_VERSION in sexpr2xml tests > Xen: xenconfig: remove disks from '(image)' sexpr > Xen: tests: use latest XEND_CONFIG_VERSION in xml2sexpr tests > Xen: xenconfig: remove use of XEND_CONFIG_VERSION in xen_sxpr > Xen: xen_driver: remove use of XEND_CONFIG_VERSION > Xen: xend: remove use of XEND_CONFIG_VERSION > Xen: xenconfig: remove xendConfigVersion from public sexpr functions > Xen: remove xendConfigVersion from driver private struct > > src/libxl/libxl_driver.c | 9 +- > src/xen/xen_driver.c | 296 --- > src/xen/xen_driver.h | 2 - > src/xen/xend_internal.c| 224 ++- > src/xen/xm_internal.c | 9 +- > src/xenconfig/xen_common.c | 211 --- > src/xenconfig/xen_common.h | 7 +- > src/xenconfig/xen_sxpr.c | 411 ++- > -- > src/xenconfig/xen_sxpr.h | 21 +- > src/xenconfig/xen_xl.c | 9 +- > src/xenconfig/xen_xl.h | 7 +- > src/xenconfig/xen_xm.c | 57 +-- > src/xenconfig/xen_xm.h | 5 +- > src/xenconfig/xenxs_private.h | 8 - > tests/sexpr2xmldata/sexpr2xml-fv-autoport.xml | 2 +- > .../sexpr2xmldata/sexpr2xml-fv-empty-kernel.sexpr | 4 +- > tests/sexpr2xmldata/sexpr2xml-fv-empty-kernel.xml | 4 +- > tests/sexpr2xmldata/sexpr2xml-fv-force-hpet.sexpr | 4 +- > tests/sexpr2xmldata/sexpr2xml-fv-force-hpet.xml| 4 +- > .../sexpr2xmldata/sexpr2xml-fv-force-nohpet.sexpr | 4 +- > tests/sexpr2xmldata/sexpr2xml-fv-force-nohpet.xml | 4 +- > tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml| 2 +- > tests/sexpr2xmldata/sexpr2xml-fv-localtime.sexpr | 3 +- > tests/sexpr2xmldata/sexpr2xml-fv-localtime.xml | 4 +- > tests/sexpr2xmldata/sexpr2xml-fv-net-ioemu.sexpr | 9 - > tests/sexpr2xmldata/sexpr2xml-fv-net-ioemu.xml | 48 --- > .../sexpr2xmldata/sexpr2xml-fv-net-netfront.sexpr | 4 +- > tests/sexpr2xmldata/sexpr2xml-fv-net-netfront.xml | 4 +- > .../sexpr2xmldata/sexpr2xml-fv-parallel-tcp.sexpr | 3 +- > tests/sexpr2xmldata/sexpr2xml-fv-parallel-tcp.xml | 4 +- > .../sexpr2xml-fv-serial-dev-2-ports.sexpr | 5 +- > .../sexpr2xml-fv-serial-dev-2-ports.xml| 4 +- > .../sexpr2xml-fv-serial-dev-2nd-port.sexpr | 4 +- > .../sexpr2xml-fv-serial-dev-2nd-port.xml | 4 +- > tests/sexpr2xmldata/sexpr2xml-fv-serial-file.sexpr | 7 +- > tests/sexpr2xmldata/sexpr2xml-fv-serial-file.xml | 4 +- > tests/sexpr2xmldata/sexpr2xml-fv-serial-null.sexpr | 3 +- > tests/sexpr2xmldata/sexpr2xml-fv-serial-null.xml | 4 +- > tests/sexpr2xmldata/sexpr2xml-fv-serial-pipe.sexpr | 7 +- > tests/sexpr2xmldata/sexpr2xml-fv-serial-pipe.xml | 4 +- > tests/sexpr2xmldata/sexpr2xml-fv-serial-pty.sexpr | 4 +- > tests/sexpr2xmldata/sexpr2xml-fv-serial-pty.xml| 4 +- > .../sexpr2xmldata/sexpr2xml-fv-serial-stdio.sexpr | 3 +- > tests/sexpr2xmldata/sexpr2xml-fv-serial-stdio.xml | 4 +- > .../sexpr2xml-fv-serial-tcp-telnet.sexpr | 3 +- > .../sexpr2xml-fv-serial-tcp-telnet.xml |
Re: [libvirt] [PATCH 0/5] UEFI loader NVRAM image in Qcow2 format
On 16.12.2015 11:13, Michal Privoznik wrote: On 08.12.2015 15:17, Dmitry Andreev wrote: Found this message right after I'v sent the patch. https://www.redhat.com/archives/libvir-list/2015-January/msg00446.html Right. When I came across this patch set I recalled having some discussion about it a long time ago (in fact as it turns out merely a year ago). So I think these patches are not needed then. Yes. In this comment https://bugzilla.redhat.com/show_bug.cgi?id=1180955#c7 Laszlo mentioned the qemu specific problem. I'll discuss it with our QEMU developers and maybe rework this patch set. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vz: implementation of domainCreateXML callback
It's my fault.
Please, disregard this patch.
Thanks to Michal.
16.12.2015 11:30, Michal Privoznik пишет:
On 09.12.2015 16:48, Mikhail Feoktistov wrote:
---
diff from v1.
Remove call of vzDomainSuspend() in case of VIR_DOMAIN_START_PAUSED flag is
set.
Now we don't support to create instance in stopped state.
src/vz/vz_driver.c | 24
1 file changed, 24 insertions(+)
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index ea1090a..4498e01 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -979,6 +979,29 @@ vzDomainUndefine(virDomainPtr domain)
return vzDomainUndefineFlags(domain, 0);
}
+static virDomainPtr
+vzDomainCreateXML(virConnectPtr conn,
+ const char *xml,
+ unsigned int flags)
+{
+virDomainPtr domain;
+int ret;
This variable seem rather redundant.
+
+virCheckFlags(0, NULL);
+
+domain = vzDomainDefineXMLFlags(conn, xml, 0);
+if (domain == NULL)
+return domain;
if (!domain) return NULL;
+
+ret = vzDomainCreate(domain);
+if (ret != 0) {
if (vzDomainCreate(domain) < 0) {
+vzDomainUndefine(domain);
+return NULL;
+}
+
+return domain;
+}
+
But those are just small nits. What I find more wrong is that
virDomainCrateXML() is supposed to create a transient domain. That is -
domain that exists only as long as it is running. When shut down, its
definition is lost - unlike for defined domains which definition is kept
around until undefined. Frankly, I don't know vz that well to say if
that's possible with the driver or not. But if it isn't maybe we should
not add implement the API at all as it may confuse users.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 1/2] Introduce new VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT event
This should be emitted whenever a domain is reverted to snapshot.
---
examples/object-events/event-test.c | 2 ++
include/libvirt/libvirt-domain.h| 1 +
tools/virsh-domain.c| 3 ++-
3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/examples/object-events/event-test.c
b/examples/object-events/event-test.c
index dcae981..afac100 100644
--- a/examples/object-events/event-test.c
+++ b/examples/object-events/event-test.c
@@ -110,6 +110,8 @@ static const char *eventDetailToString(int event, int
detail) {
ret = "Updated";
else if (detail == VIR_DOMAIN_EVENT_DEFINED_RENAMED)
ret = "Renamed";
+else if (detail == VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT)
+ret = "Snapshot";
break;
case VIR_DOMAIN_EVENT_UNDEFINED:
if (detail == VIR_DOMAIN_EVENT_UNDEFINED_REMOVED)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index a1ea6a5..f3d360b 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -2330,6 +2330,7 @@ typedef enum {
VIR_DOMAIN_EVENT_DEFINED_ADDED = 0, /* Newly created config file */
VIR_DOMAIN_EVENT_DEFINED_UPDATED = 1, /* Changed config file */
VIR_DOMAIN_EVENT_DEFINED_RENAMED = 2, /* Domain was renamed */
+VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT = 3, /* Config file was restored
from a snapshot */
# ifdef VIR_ENUM_SENTINELS
VIR_DOMAIN_EVENT_DEFINED_LAST
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 7650535..69f4792 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11910,7 +11910,8 @@ VIR_ENUM_IMPL(virshDomainEventDefined,
VIR_DOMAIN_EVENT_DEFINED_LAST,
N_("Added"),
N_("Updated"),
- N_("Renamed"))
+ N_("Renamed"),
+ N_("Snapshot"))
VIR_ENUM_DECL(virshDomainEventUndefined)
VIR_ENUM_IMPL(virshDomainEventUndefined,
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 2/2] qemu: emit 'defined' event after reverted to snapshot
If config file was changed VIR_DOMAIN_EVENT_DEFINED should be emitted
---
src/qemu/qemu_driver.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 783a7cd..1474eaa 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15512,7 +15512,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
VIR_DOMAIN_EVENT_STARTED,
detail);
if (rc < 0)
-goto endjob;
+goto defined;
}
/* Touch up domain state. */
@@ -15534,7 +15534,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("guest unexpectedly quit"));
-goto endjob;
+goto defined;
}
rc = qemuProcessStartCPUs(driver, vm, snapshot->domain->conn,
VIR_DOMAIN_RUNNING_FROM_SNAPSHOT,
@@ -15636,6 +15636,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
ret = 0;
+ defined:
+if (config) {
+virObjectEventPtr define_event;
+define_event = virDomainEventLifecycleNewFromObj(vm,
+VIR_DOMAIN_EVENT_DEFINED,
+VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT);
+qemuDomainEventQueue(driver, define_event);
+}
+
endjob:
qemuProcessEndJob(driver, vm);
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 0/2] notify about reverting to snapshot
Reverting to snapshot may change domain configuration but there will be no events about that. Lack of the event become a problem for virt-manager https://bugzilla.redhat.com/show_bug.cgi?id=1081148 This patch-set introduces new event and emits it in qemuDomainRevertToSnapshot. v2: [2/2] Reworked. John noted that in some error cases I send 'defined' event without changes in configuration. Dmitry Andreev (2): Introduce new VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT event qemu: emit 'defined' event after reverted to snapshot examples/object-events/event-test.c | 2 ++ include/libvirt/libvirt-domain.h| 1 + src/qemu/qemu_driver.c | 13 +++-- tools/virsh-domain.c| 3 ++- 4 files changed, 16 insertions(+), 3 deletions(-) -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] pci: Use virPCIDeviceAddress in virPCIDevice
On Tue, 2015-12-15 at 13:53 -0500, Laine Stump wrote: > On 12/15/2015 01:23 PM, Andrea Bolognani wrote: > > Instead of replicating the information (domain, bus, slot, function) > > inside the virPCIDevice structure, use the already-existing > > virPCIDeviceAddress structure. > > > > For users of the module, this means that the object returned by > > virPCIDeviceGetAddress() can no longer be NULL and must no longer > > be freed by the caller. > > --- > > > > Changes in v3: > > > > * don't check the return value of virPCIDeviceGetAddress(), it can no > > longer be NULL > > * don't use a variable to store the device address if it's only going > > to be used a single time > > ACK. Pushed, thanks. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vz: implementation of domainCreateXML callback
On 09.12.2015 16:48, Mikhail Feoktistov wrote:
> ---
>
> diff from v1.
> Remove call of vzDomainSuspend() in case of VIR_DOMAIN_START_PAUSED flag is
> set.
> Now we don't support to create instance in stopped state.
>
> src/vz/vz_driver.c | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
> index ea1090a..4498e01 100644
> --- a/src/vz/vz_driver.c
> +++ b/src/vz/vz_driver.c
> @@ -979,6 +979,29 @@ vzDomainUndefine(virDomainPtr domain)
> return vzDomainUndefineFlags(domain, 0);
> }
>
> +static virDomainPtr
> +vzDomainCreateXML(virConnectPtr conn,
> + const char *xml,
> + unsigned int flags)
> +{
> +virDomainPtr domain;
> +int ret;
This variable seem rather redundant.
> +
> +virCheckFlags(0, NULL);
> +
> +domain = vzDomainDefineXMLFlags(conn, xml, 0);
> +if (domain == NULL)
> +return domain;
if (!domain) return NULL;
> +
> +ret = vzDomainCreate(domain);
> +if (ret != 0) {
if (vzDomainCreate(domain) < 0) {
> +vzDomainUndefine(domain);
> +return NULL;
> +}
> +
> +return domain;
> +}
> +
But those are just small nits. What I find more wrong is that
virDomainCrateXML() is supposed to create a transient domain. That is -
domain that exists only as long as it is running. When shut down, its
definition is lost - unlike for defined domains which definition is kept
around until undefined. Frankly, I don't know vz that well to say if
that's possible with the driver or not. But if it isn't maybe we should
not add implement the API at all as it may confuse users.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/5] UEFI loader NVRAM image in Qcow2 format
On 08.12.2015 15:17, Dmitry Andreev wrote: > Found this message right after I'v sent the patch. > https://www.redhat.com/archives/libvir-list/2015-January/msg00446.html Right. When I came across this patch set I recalled having some discussion about it a long time ago (in fact as it turns out merely a year ago). So I think these patches are not needed then. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
