Re: Questions about using qemuProcess API within a libvirt test
On 2/10/20 10:01 PM, Collin Walling wrote: Hello, Apart from what Laine said, this looks like your test binary is missing event loop. The way sending QMP commands work is that you have one thread, that wants to send a QMP command. It will construct the command, format JSON string out of it and notifies the other thread (where the event loop is running) and goes to sleep. The other thread waits until QMP socket becomes writable (poll()) and then writes the JSON string into it. Then it waits until the socket becomes readable and when it does so, it reads incoming data (which is a reply to the command). Once the full reply was received it will wake up the original thread [1]. Adding an event loop to your program should be easy. Just see what tests/qemucapsprobe.c is doing. Important bits are eventLoop() function and virEventRegisterDefaultImpl() and virThreadCreate() calls. However, as Laine says, we don't want our test suite to spawn any qemu process. Not only it would create a non-reproducible environment for the tests (developers might run different version of qemu), it might also not work at all - when building libvirt we don't require qemu to be installed (as we don't link with it, there is no library to link with anyways). But we want our test suite to run successfully. When we want to test something that talks on QMP monitor (e.g. qemumonitorjsontest), we create two threads and provide an alternative implementation for the event loop - instead of actually touching any real QMP monitor socket, it discards all writes, and provides well defined replies (crafted before). Look into the source file and you will understand. Michal 1: In fact, the event loop always poll()-s for socket to be readable, because QEMU can send us an event asynchronously, but let's not complicate things right now.
Re: Questions about using qemuProcess API within a libvirt test
On 2/10/20 4:01 PM, Collin Walling wrote:
Hello,
I am working on implementing libvirt test cases for the hypervisor-cpu-compare
and -baseline commands. Ideally, I would like to take advantage of the
qemuProcess API to spin up a QEMU instance and run these commands to test real
data queried from the hypervisor.
The tests in libvirt's tests directory are all unit tests, and don't run
external binaries like qemu - they attempt to test various pieces of
code within libvirt in a limited, controlled environment, not requiring
any special privileges, and without being subject to the vagaries of
external programs. qemuxml2argvtest, for example, calls the code that
converts a libvirt domain xml into a qemu commandline, but rather than
starting up a qemu process with that commandline, it then compares the
generated commandline string to the "known correct" commandline that is
stored in a file in a subdirectory of the tests directory (for this
example, look at tests/qemuxml2argvdata/*.xml vs
tests/qemuxml2argvdata/*.args); this way the success of the test doesn't
depend on qemu being installed, the user having the proper privileges to
perform all the other things that qemu needs to run, or be subject to a
failure due to a bug in the version of qemu running on the build system.
Other examples of tests are *xml2xmltest, which parse a test input file,
then format the resulting object back to xml, and compare that result
with known-correct XML that, again, is stored in a file.
Sometimes the code that is being tested calls a function that can't be
realistically called from a unit test that is run on random hardware by
an unprivileged user (e.g. unbinding a PCI device from its host driver
and re-binding it to the vfio-pci driver). In that case, various library
functions are "mocked" (by linking in the .o created from *mock.c files
- virpcimock.c in the case of PCI device driver binding) to return
reasonable values that will allow the libvirt code to be tested without
actually needing to perform the "impossible" operation.
Anyway, the short answer to your questions is that libvirt's tests
directory isn't the proper location (nor is "make check" the proper
time) for a test that starts up a qemu process and sends commands to it.
Something like that would be better suited for an integration test
suite, like the ones that are in the libvirt-tck package (which is a bit
dated, but still very useful, as long as you don't mind writing perl
scripts :-P)
For a unit test in the tests directory, you would want to setup a test
harness (blahtest.c) that links in the files containing your code, calls
those functions with a fixed set of inputs, then compares the results to
fixed known outputs. Along the way you may need to "stub-out" or "mock"
various functions that are called by the code under test but can't
actually be called in the environment of a unit test.
However, I am having issues with my libvirt
tests communicating with a QEMU instance. The API can successfully spin an
instance, but no commands can be sent to QEMU -- not even the qmp_capabilities
handshake. The test case hangs forever with no indication that something went
wrong.
The hang occurs specifically within the qemuProcessQMPLaunch phase of the
qemuProcessQMPStart function. Eventually the libvirt API will get to
qemuMonitorSend, and at this loop...
while (!mon->msg->finished) {
if (virCondWait(>notify, >parent.lock) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Unable to wait on monitor condition"));
goto cleanup;
}
}
...the program will hang at conditional wait. I kept the QEMU instance alive
after killing my test and connected to the monitor socket that was still
lingering. The qmp_capabilities command was never sent, thus leading me to
believe that the libvirt tests cannot communicate with a QEMU instance.
As s390x is currently the only arch to have implemented these commands, I
believe it would be beneficial to have an easy way to test both the expected
QEMU response and libvirt parsing within a single test case if/when other archs
decide to implement these commands.
I'd like to ask two questions:
1: does it make sense for libvirt tests to communicate with an external binary
(specifically QEMU)?
2: if so, is there some sort of conflict between a libvirt test and a QEMU
binary? I afraid to say that I am at a loss how to repair this or perhaps how
to use the API properly.
I appreciate anyone's help with looking into this.
Note: in case I am not clear, by "libvirt test" I am referring to a test
implemented in the tests directory of the libvirt project.
Re: [PATCH v1 01/14] vircgroup: add virCgroupSetupBlkioTune()
On 2/11/20 4:21 PM, Daniel Henrique Barboza wrote: > > > On 2/11/20 11:35 AM, Cole Robinson wrote: >> On 2/11/20 7:28 AM, Daniel Henrique Barboza wrote: >>> > > [...] > >> >> I asked about this case in December, danpb suggested creating a new >> src/hypervisor/ directory for containing shared driver code like this: >> >> https://www.redhat.com/archives/libvir-list/2019-December/msg00817.html > > Thanks, I'm going for it. > > Do you have any suggestions for the file names? I see that you mentioned > "domain_cgroup" in that thread, thus I'm thinking about creating a new > src/hypervisor/domain_cgroup.c for the shared code between lxc_cgroup.c and > qemu_cgroup.c. > > I'm also considering a src/hypervisor/domain_driver.c for the driver common > code between lxc_driver.c and qemu_driver.c that I ended up finding in this > work. > Those both sound reasonable to me. Thanks, Cole
Re: [PATCH v1 01/14] vircgroup: add virCgroupSetupBlkioTune()
On 2/11/20 11:35 AM, Cole Robinson wrote: On 2/11/20 7:28 AM, Daniel Henrique Barboza wrote: [...] I asked about this case in December, danpb suggested creating a new src/hypervisor/ directory for containing shared driver code like this: https://www.redhat.com/archives/libvir-list/2019-December/msg00817.html Thanks, I'm going for it. Do you have any suggestions for the file names? I see that you mentioned "domain_cgroup" in that thread, thus I'm thinking about creating a new src/hypervisor/domain_cgroup.c for the shared code between lxc_cgroup.c and qemu_cgroup.c. I'm also considering a src/hypervisor/domain_driver.c for the driver common code between lxc_driver.c and qemu_driver.c that I ended up finding in this work. DHB - Cole
Re: [libvirt PATCH] docs: Improve documentation for and
On Tue, Feb 11, 2020 at 03:44:40PM +0100, Andrea Bolognani wrote: Users expect to be able to configure the element and see that configuration reflected into the element or at least sticking, however due to our crazy back-compat code that doesn't always happen. There's really not much we can do to make this kind of corner cases work as the user would expect, especially not without introducing additional complexity in a part of libvirt that already has more than a fair share of it; we can, however, improve the documentation so that it will nudge said users in the right direction. https://bugzilla.redhat.com/show_bug.cgi?id=1770725 Signed-off-by: Andrea Bolognani --- docs/formatdomain.html.in | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
[libvirt PATCH] docs: Improve documentation for and
Users expect to be able to configure the element and see that configuration reflected into the element or at least sticking, however due to our crazy back-compat code that doesn't always happen. There's really not much we can do to make this kind of corner cases work as the user would expect, especially not without introducing additional complexity in a part of libvirt that already has more than a fair share of it; we can, however, improve the documentation so that it will nudge said users in the right direction. https://bugzilla.redhat.com/show_bug.cgi?id=1770725 Signed-off-by: Andrea Bolognani --- docs/formatdomain.html.in | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 44e2062d01..5ccf39abd1 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -7510,7 +7510,10 @@ qemu-kvm -net nic,model=? /dev/null since 4.7.0, 16550a (usable with the system-serial target type); sclpconsole and sclplmconsole (usable with - the sclp-serial target type). + the sclp-serial target type). Providing a target model is + usually unnecessary: libvirt will automatically pick one that's suitable + for the chosen target type, and overriding that value is generally not + recommended. @@ -7656,7 +7659,8 @@ qemu-kvm -net nic,model=? /dev/null for early boot logging / interactive / recovery use, and one paravirtualized serial console to be used eg. as a side channel. Most people will be fine with having just the first console - element in their configuration. + element in their configuration, but if a specific configuration is + desired then both elements should be specified. -- 2.24.1
Re: [PATCH v1 01/14] vircgroup: add virCgroupSetupBlkioTune()
On 2/11/20 7:28 AM, Daniel Henrique Barboza wrote: > > > On 2/10/20 10:39 PM, Ján Tomko wrote: >> On Mon, Feb 10, 2020 at 07:05:07PM -0300, Daniel Henrique Barboza wrote: >>> This new util function puts the duplicated code from >>> qemu_cgroup.c:qemuSetupBlkioCgroup() and >>> lxc_cgroup.c:virLXCCgroupSetupBlkioTune() in a single >>> function to be used in both places. >>> >>> Signed-off-by: Daniel Henrique Barboza >>> --- >>> src/libvirt_private.syms | 1 + >>> src/lxc/lxc_cgroup.c | 49 +-- >>> src/qemu/qemu_cgroup.c | 47 +- >>> src/util/vircgroup.c | 55 >>> src/util/vircgroup.h | 3 +++ >>> 5 files changed, 61 insertions(+), 94 deletions(-) >>> >>> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c >>> index 0680ff7c24..0d83e2094f 100644 >>> --- a/src/util/vircgroup.c >>> +++ b/src/util/vircgroup.c >>> @@ -35,6 +35,7 @@ >>> #define LIBVIRT_VIRCGROUPPRIV_H_ALLOW >>> #include "vircgrouppriv.h" >>> >>> +#include "conf/domain_conf.h" >>> #include "virutil.h" >>> #include "viralloc.h" >>> #include "vircgroupbackend.h" >>> diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h >>> index 15263f534a..d2d7e7ab51 100644 >>> --- a/src/util/vircgroup.h >>> +++ b/src/util/vircgroup.h >>> @@ -24,6 +24,7 @@ >>> #include "virutil.h" >>> #include "virbitmap.h" >>> #include "virenum.h" >>> +#include "conf/virconftypes.h" >>> >> >> src/util should be below src/conf and not including stuff from it >> (even though we do have some debt there at the moment) >> >> The last time this came up: >> https://www.redhat.com/archives/libvir-list/2018-June/msg00381.html >> >> And there should've been a syntax-check rule yelling about this, >> but I see the out-of-srcdir build broke it: >> https://www.redhat.com/archives/libvir-list/2020-February/msg00442.html > > > Just saw that you fixed this check on newest master. syntax-check is not > pleased with these patches anymore. > > I'll send a v2. I asked about this case in December, danpb suggested creating a new src/hypervisor/ directory for containing shared driver code like this: https://www.redhat.com/archives/libvir-list/2019-December/msg00817.html - Cole
Re: [libvirt PATCH 0/6] Introduce Local Migration Support in Libvirt
On Mon, Feb 10, 2020 at 08:45:28AM -0700, Jim Fehlig wrote: > On 2/3/20 5:43 AM, Daniel P. Berrangé wrote: > > I'm (re-)sending this patch series on behalf of Shaju Abraham > > who has tried to send this several times > > already. > > > > Red Hat's email infrastructure is broken, accepting the mails and then > > failing to deliver them to mailman, or any other Red Hat address. > > Unfortunately it means that while we can send comments back to Shaju > > on this thread, subscribers will then probably fail to see any responses > > Shaju tries to give :-( To say this is bad is an understatement. I have > > yet another ticket open tracking & escalating this awful problem but > > can't give any ETA on a fix :-( > > > > Anyway, with that out of the way, here's Shaju's original cover letter > > below > > > > 1) What is this patch series about? > > > > Local live migration of a VM is about Live migrating a VM instance with in > > the > > same node. Traditional libvirt live migration involves migrating the VM > > from a > > source node to a remote node. The local migrations are forbidden in Libvirt > > for > > a myriad of reasons. This patch series is to enable local migration in > > Libvirt. > > > > 2) Why Local Migration is important? > > > > The ability to Live migrate a VM locally paves the way for hypervisor > > upgrades > > without shutting down the VM. For example to upgrade qemu after a security > > upgrade, we can locally migrate the VM to the new qemu instance. By > > utilising > > capabilities like "bypass-shared-memory" in qemu, the hypervisor upgrades > > are > > faster. > > > > 3) Why is local migration difficult in Libvirt? > > > > Libvirt always assumes that the name/UUID pair is unique with in a node. > > During > > local migration there will be two different VMs with the same UUID/name pair > > which will confuse the management stack. There are other path variables like > > monitor path, config paths etc which assumes that the name/UUID pair is > > unique. > > So during migration the same monitor will be used by both the source and the > > target. We cannot assign a temporary UUID to the target VM, since UUID is a > > part > > of the machine ABI which is immutable. > > To decouple the dependecy on UUID/name, a new field (the domain id) is > > included > > in all the PATHs that Libvirt uses. This will ensure that all instances of > > the > > VM gets a unique PATH. > > Since localhost migration is difficult, and there will likely be some > growing pains until the feature is fully baked, perhaps it is best to have a > knob for enabling/disabling it. The namespace feature suffered similar > growing pains and having the ability to disable it in qemu.conf proved quite > handy at times. Probably an API flag VIR_MIGRATE_SAME_HOST is sufficient, as that shows an opt-in on the part of the person/thing that initiates it. I think we'd want this flag forever, regardless of whether its experimental or production quality, because there are special concerns about clashing host files/resources. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 5/6] Make PATHs unique for a VM object instance
On Tue, Feb 11, 2020 at 10:05:53AM +0100, Martin Kletzander wrote: > On Wed, Feb 05, 2020 at 05:32:50PM +, Daniel P. Berrangé wrote: > > On Mon, Feb 03, 2020 at 12:43:32PM +, Daniel P. Berrangé wrote: > > > From: Shaju Abraham > > > > > > There are various config paths that a VM uses. The monitor paths and > > > other lib paths are examples. These paths are tied to the VM name or > > > UUID. The local migration breaks the assumption that there will be only > > > one VM with a unique UUID and name. During local migrations there can be > > > multiple VMs with same name and UUID in the same host. Append the > > > domain-id field to the path so that there is no duplication of path > > > names. > > > > This is the really critical problem with localhost migration. > > > > Appending the domain-id looks "simple" but this is a significant > > behavioural / functional change for applications, and I don't think > > it can fully solve the problem either. > > > > This is changing thue paths used in various places where libvirt > > internally generates unique paths (eg QMP socket, huge page or > > file based memory paths, and defaults used for auto-filling > > device paths ( if not specified). > > > > Some of these paths are functionally important to external > > applications and cannot be changed in this way. eg stuff > > integrating with QEMU can be expecting certain memory backing > > file paths, or certain paths & is liable to break > > if we change the naming convention. > > > > For sake of argument, lets assume we can changing the naming > > convention without breaking anything... > > > > This was already done in (I would say) most places as they use > virDomainDefGetShortName() to get a short, unique name of a directory -- it > uses > the domain ID as a prefix. > > > This only applies to paths libvirt generates at VM startup. > > > > There are plenty of configuration elements in the guest XML > > that are end user / mgmt app defined, and reference paths in > > the host OS. > > > > For example , , , , > > all support UNIX domain sockets and TCP sockets. A UNIX > > domain socket cannot be listened on by multiple VMs > > at once. If the UNIX socket is in client mode, we cannot > > assume the thing QEMU is connecting to allows multiple > > concurrent connections. eg 2 QEMU's could have their > > connected together over a UNIX socket pair. > > Similarly if automatic TCP port assignment is not used > > we cannot have multiple QEMU's listening on the same > > host. > > > > One answer is to say that localhost migration is just > > not supported for such VMs, but I don't find that very > > convincing because the UNIX domain socket configs > > affected are in common use. > > > > I would be okay with saying that these either need to be changed in a provided > destination XML or the migration will probably break. I do not think it is > unreasonable to say that if users are trying to shoot themselves, we should > not > spend a ridiculous time just so we can prevent that. Otherwise we will get to > the same point as we are now -- there might be a case where a local migration > would work, but users cannot execute it even if they were very cautious and > went > through all the things that can prevent it from the technical point of view, > libvirt will still disallow that. If there are clashing resources, we can't rely on QEMU reporting an error. For example with a UNIX domain socket, the first thing QEMU does is unlink(/socket/path), which will blow away the UNIX domain socket belonging to the original QEMU. As a result if migration fails, and we rollback, the original QEMU will be broken. Preventing users from shooting themselves in the foot is a core part of the value that libvirt is adding for QEMU migration. We do this with stable device ABI, and controlled locking / disk labelling, and CPU compatibility checking, and so on. We're not perfect by any means, but the one thing we've tried very hard to ensure is that if the destination QEMU fails for any reason during migration, the user should always be able to rollback to use the original source QEMU without problems. The localhost migration support makes it harder to guarantee the the source QEMU is not broken, so I do think we need to make extra affect to protect users, if we're going to try to allow this. This series has taken the approach of trying to make the localhost migration work as if it were just a normal remote migration, with just the minimum change to alter some auto-generated paths on disk, and keeping the second list of domains. So we still the same begin/prepare/perform/finish/confirm phases fully separated. IOW, essentially the migration code has very little, almost no, knowlege of the fact that this is a localhost migration. This is understandaable as a way to minimize the invasiveness of any changes, but I think it misses the point that localhost migration needs more than just changing a few paths on disk. > Adding
Re: [PATCH 4/6] admin: support server cert update mode
On Sun, Feb 09, 2020 at 10:03:14PM +0800, Zhang Bo wrote:
> virAdmServerUpdateTlsFiles:
> @flags specifies how to update server cert/key in tls service.
> Two modes are currently supported: append mode and clear mode, means
> whether to clear the original cert then add the new one, or just append
> to the original one.
> ---
> include/libvirt/libvirt-admin.h | 14 ++
> src/admin/admin_server.c| 7 +--
> src/admin/libvirt-admin.c | 7 ++-
> src/rpc/virnetserver.c | 17 +
> src/rpc/virnetserver.h | 3 ++-
> src/rpc/virnettlscontext.c | 7 +--
> src/rpc/virnettlscontext.h | 3 ++-
> 7 files changed, 43 insertions(+), 15 deletions(-)
>
> diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
> index 6e38261129..dfdd81ae83 100644
> --- a/include/libvirt/libvirt-admin.h
> +++ b/include/libvirt/libvirt-admin.h
> @@ -392,6 +392,20 @@ int virAdmClientClose(virAdmClientPtr client, unsigned
> int flags);
>
> # define VIR_SERVER_CLIENTS_UNAUTH_CURRENT "nclients_unauth"
>
> +typedef enum {
> +/* free old credentials and then set new tls context.
> + */
> +VIR_TLS_UPDATE_CLEAR = 0,
> +
> +/* do not clear original certificates and keys.
> + */
> +VIR_TLS_UPDATE_APPEND = 1,
I don't think we should we supporting this operation. In order to achieve
reliability of the TLS reload, we need to re-create the credentials object
and swap out the original credentails on success. This precludes updating
the original credentials
> @@ -1165,7 +1166,9 @@ int virNetTLSContextReload(virNetTLSContextPtr ctxt,
> }
>
> if (filetypes & VIR_TLS_FILE_TYPE_SERVER_CERT) {
> -gnutls_certificate_free_keys(ctxt->x509cred);
> +if (flags == VIR_TLS_UPDATE_CLEAR)
> +gnutls_certificate_free_keys(ctxt->x509cred);
> +
> if (virNetTLSContextSetCertAndKey(ctxt, cert, key, false))
> goto cleanup;
...because this code still has the dangerous behaviour that it leaves the
server without any valid cert/key on failure.
The second bad thing is that the state in-memory does not match the
state on disk. This puts the old & new cert+key in memory, but only
the new cert+key is on disk. So if libvirtd is restarted for any
reason you'll get a change in certs again. The reload operation
should just be updating libvirtd's in-memory state to exactly match
the on-disk state, in the same way that a restart does.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 2/6] virnetserver: Introduce virNetServerUpdateTlsFiles
On Sun, Feb 09, 2020 at 10:03:12PM +0800, Zhang Bo wrote:
> Add an API to update server's tls context before admin method can be
> introduced.
> ---
> include/libvirt/libvirt-admin.h | 8
> src/libvirt_remote.syms | 1 +
> src/rpc/virnetserver.c | 72 +
> src/rpc/virnetserver.h | 3 ++
> src/rpc/virnetserverclient.c| 4 ++
> src/rpc/virnettlscontext.c | 41 +++
> src/rpc/virnettlscontext.h | 2 +
> 7 files changed, 131 insertions(+)
>
> diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
> index abf2792926..3edc044490 100644
> --- a/include/libvirt/libvirt-admin.h
> +++ b/include/libvirt/libvirt-admin.h
> @@ -392,6 +392,14 @@ int virAdmClientClose(virAdmClientPtr client, unsigned
> int flags);
>
> # define VIR_SERVER_CLIENTS_UNAUTH_CURRENT "nclients_unauth"
>
> +/* tls related filetype flags. */
> +typedef enum {
> +VIR_TLS_FILE_TYPE_CA_CERT = (1U << 0),
> +VIR_TLS_FILE_TYPE_CA_CRL = (1U << 1),
> +VIR_TLS_FILE_TYPE_SERVER_CERT = (1U << 2),
> +VIR_TLS_FILE_TYPE_SERVER_KEY = (1U << 3),
> +} virServerTlsFiletype;
[snip]
> diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
> index 12811bed78..8baa6a15b2 100644
> --- a/src/rpc/virnettlscontext.c
> +++ b/src/rpc/virnettlscontext.c
> @@ -1139,6 +1139,47 @@ void virNetTLSContextDispose(void *obj)
> gnutls_certificate_free_credentials(ctxt->x509cred);
> }
>
> +int virNetTLSContextReload(virNetTLSContextPtr ctxt,
> + unsigned int filetypes)
> +{
> +int ret = -1;
> +char *cacert = NULL;
> +char *cacrl = NULL;
> +char *cert = NULL;
> +char *key = NULL;
> +
> +virObjectLock(ctxt);
> +
> +if (virNetTLSContextLocateCredentials(NULL, false, true,
> + , , , ) < 0)
> +goto cleanup;
> +
> +if (filetypes & VIR_TLS_FILE_TYPE_CA_CERT) {
> +if (virNetTLSContextSetCACert(ctxt, cacert, false))
> +goto cleanup;
> +}
> +
> +if (filetypes & VIR_TLS_FILE_TYPE_CA_CRL) {
> +if (virNetTLSContextSetCACRL(ctxt, cacrl, false))
> +goto cleanup;
> +}
> +
> +if (filetypes & VIR_TLS_FILE_TYPE_SERVER_CERT) {
> +gnutls_certificate_free_keys(ctxt->x509cred);
> +if (virNetTLSContextSetCertAndKey(ctxt, cert, key, false))
> +goto cleanup;
> +}
I'm not convinced we should be doing a selective update of only
a subset of files here.
I feel like an oneline update should always have the exact same
result as you would get by doing a restart of libvirtd.
Consider if you update the CA cert, CA CRL and Server Cert
on disk, but then tell libvirtd to only reload Server Cert.
The state of libvirtd is now out of sync with state on disk,
and when libvirtd gets restarted due to a RPM software upgrade
later, its will load different content again.
This can lead to hard to diagnose problems, or delayed discovery
of problems.
The second is is that we're modifying the existing "x509cred"
object in virNetTLSContext.
gnutls_certificate_free_keys(ctxt->x509cred);
if (virNetTLSContextSetCertAndKey(ctxt, cert, key, false))
goto cleanup;
Consider if virNetTLSContextSetCertAndKey() here fails - now libvirtd
is missing the original TLS cert/key, and also missing the new TLS
cert/key.
When we're reloading certs, I think we need to create an entirely
new gnutls_certificate_credentials_t object, and populate it from
all the files on disk.
Only once that is succesful, should we then replace the "x509cred"
object in the virNetTLSContext. This gives us an atomic update
path, so we're guaranteed to always have valid credentials
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 7/7] testutils: remove now unused virTestCaptureProgramOutput
On Sun, Feb 09, 2020 at 02:32:37AM +0100, Ján Tomko wrote: > Signed-off-by: Ján Tomko > --- > tests/testutils.c | 84 --- > tests/testutils.h | 2 -- > 2 files changed, 86 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 6/7] virshtest: use virCommand instead of custom impl
On Sun, Feb 09, 2020 at 02:32:36AM +0100, Ján Tomko wrote:
> Our virCommand helper API already has the ability to capture
> program output, there's no need to open-code it.
>
> Apart from simplifying the code, the test is marginally faster
> due to recent improvements in virCommandMassClose.
>
> Signed-off-by: Ján Tomko
> ---
> tests/virshtest.c | 19 ++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/tests/virshtest.c b/tests/virshtest.c
> index 83675710ea..add33215b7 100644
> --- a/tests/virshtest.c
> +++ b/tests/virshtest.c
> @@ -5,6 +5,7 @@
> #include "internal.h"
> #include "virxml.h"
> #include "testutils.h"
> +#include "vircommand.h"
> #include "virstring.h"
>
> #define VIR_FROM_THIS VIR_FROM_NONE
> @@ -61,10 +62,26 @@ testCompareOutputLit(const char *expectData,
> const char *filter, const char *const argv[])
> {
> g_autofree char *actualData = NULL;
> +const char *empty = "";
> +g_autoptr(virCommand) cmd = NULL;
> +g_autofree char *errbuf = NULL;
>
> -if (virTestCaptureProgramOutput(argv, , 4096) < 0)
> +if (!(cmd = virCommandNewArgs(argv)))
> return -1;
>
> +virCommandAddEnvString(cmd, "LANG=C");
> +virCommandSetInputBuffer(cmd, empty);
> +virCommandSetOutputBuffer(cmd, );
> +virCommandSetErrorBuffer(cmd, );
> +
> +if (virCommandRun(cmd, NULL) < 0)
> +return -1;
> +
> +if (STRNEQ(errbuf, "")) {
> +fprintf(stderr, "Command reported error: %s", errbuf);
> +return -1;
> +}
The current method merges stdout and stderr into the same 'actualData'
buffer, so this impl is not functionally the same.
That said, assuming any commands we run don't write to stderr, I think
this impl is better.
Can you just mention that this is an intended change in the commit
message to remind us, in case we have a surprise later.
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 5/7] virshtest: refactor testCompareOutputLit
On Sun, Feb 09, 2020 at 02:32:35AM +0100, Ján Tomko wrote: > Use g_autofree and get rid of the cleanup label. > > Signed-off-by: Ján Tomko > --- > tests/virshtest.c | 16 +--- > 1 file changed, 5 insertions(+), 11 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 3/7] testutils: use g_autoptr
On Sun, Feb 09, 2020 at 02:32:33AM +0100, Ján Tomko wrote: > Use g_autoptr where possible. > > virTestCapsBuildNUMATopology is not converted completely, > because while the VIR_FREE call on cell_cpus is technically > wrong, neither VIR_ALLOC_N nor virBitmapNew can return > an allocation error now so it is effectively dead code. > > Signed-off-by: Ján Tomko > --- > tests/testutils.c | 13 + > 1 file changed, 5 insertions(+), 8 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 4/7] testutils: remove unnecessary labels
On Sun, Feb 09, 2020 at 02:32:34AM +0100, Ján Tomko wrote: > The cleanups made some labels redundant. > > Signed-off-by: Ján Tomko > --- > tests/testutils.c | 44 > 1 file changed, 16 insertions(+), 28 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 2/7] testutils: use g_autofree
On Sun, Feb 09, 2020 at 02:32:32AM +0100, Ján Tomko wrote: > Signed-off-by: Ján Tomko > --- > tests/testutils.c | 28 +--- > 1 file changed, 9 insertions(+), 19 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 0/7] virshtest: remove virTestCaptureProgramOutput
On Sun, Feb 09, 2020 at 02:32:30AM +0100, Ján Tomko wrote: > Use virCommand instead of open-coding it and do some other cleanups > found along the way. Ohh, great, I thought about this when I did the FreeBSD mass close patch, but didn't fancy tackling it myself yet. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 1/7] testutils: check return value of g_setenv
On Sun, Feb 09, 2020 at 02:32:31AM +0100, Ján Tomko wrote: > The function returns gboolean. > Compare against the FALSE value from GLib. > > Signed-off-by: Ján Tomko > Fixes: 2c3353242337bb50fe5abc9454fd5fc98236d4ef > --- > tests/testutils.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v1 01/14] vircgroup: add virCgroupSetupBlkioTune()
On 2/10/20 10:39 PM, Ján Tomko wrote: On Mon, Feb 10, 2020 at 07:05:07PM -0300, Daniel Henrique Barboza wrote: This new util function puts the duplicated code from qemu_cgroup.c:qemuSetupBlkioCgroup() and lxc_cgroup.c:virLXCCgroupSetupBlkioTune() in a single function to be used in both places. Signed-off-by: Daniel Henrique Barboza --- src/libvirt_private.syms | 1 + src/lxc/lxc_cgroup.c | 49 +-- src/qemu/qemu_cgroup.c | 47 +- src/util/vircgroup.c | 55 src/util/vircgroup.h | 3 +++ 5 files changed, 61 insertions(+), 94 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 0680ff7c24..0d83e2094f 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -35,6 +35,7 @@ #define LIBVIRT_VIRCGROUPPRIV_H_ALLOW #include "vircgrouppriv.h" +#include "conf/domain_conf.h" #include "virutil.h" #include "viralloc.h" #include "vircgroupbackend.h" diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 15263f534a..d2d7e7ab51 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -24,6 +24,7 @@ #include "virutil.h" #include "virbitmap.h" #include "virenum.h" +#include "conf/virconftypes.h" src/util should be below src/conf and not including stuff from it (even though we do have some debt there at the moment) The last time this came up: https://www.redhat.com/archives/libvir-list/2018-June/msg00381.html And there should've been a syntax-check rule yelling about this, but I see the out-of-srcdir build broke it: https://www.redhat.com/archives/libvir-list/2020-February/msg00442.html Just saw that you fixed this check on newest master. syntax-check is not pleased with these patches anymore. I'll send a v2. Thanks, DHB Jano
Re: [libvirt PATCH 5/6] Make PATHs unique for a VM object instance
On Wed, Feb 05, 2020 at 05:32:50PM +, Daniel P. Berrangé wrote:
On Mon, Feb 03, 2020 at 12:43:32PM +, Daniel P. Berrangé wrote:
From: Shaju Abraham
There are various config paths that a VM uses. The monitor paths and
other lib paths are examples. These paths are tied to the VM name or
UUID. The local migration breaks the assumption that there will be only
one VM with a unique UUID and name. During local migrations there can be
multiple VMs with same name and UUID in the same host. Append the
domain-id field to the path so that there is no duplication of path
names.
This is the really critical problem with localhost migration.
Appending the domain-id looks "simple" but this is a significant
behavioural / functional change for applications, and I don't think
it can fully solve the problem either.
This is changing thue paths used in various places where libvirt
internally generates unique paths (eg QMP socket, huge page or
file based memory paths, and defaults used for auto-filling
device paths ( if not specified).
Some of these paths are functionally important to external
applications and cannot be changed in this way. eg stuff
integrating with QEMU can be expecting certain memory backing
file paths, or certain paths & is liable to break
if we change the naming convention.
For sake of argument, lets assume we can changing the naming
convention without breaking anything...
This was already done in (I would say) most places as they use
virDomainDefGetShortName() to get a short, unique name of a directory -- it uses
the domain ID as a prefix.
This only applies to paths libvirt generates at VM startup.
There are plenty of configuration elements in the guest XML
that are end user / mgmt app defined, and reference paths in
the host OS.
For example , , , ,
all support UNIX domain sockets and TCP sockets. A UNIX
domain socket cannot be listened on by multiple VMs
at once. If the UNIX socket is in client mode, we cannot
assume the thing QEMU is connecting to allows multiple
concurrent connections. eg 2 QEMU's could have their
connected together over a UNIX socket pair.
Similarly if automatic TCP port assignment is not used
we cannot have multiple QEMU's listening on the same
host.
One answer is to say that localhost migration is just
not supported for such VMs, but I don't find that very
convincing because the UNIX domain socket configs
affected are in common use.
I would be okay with saying that these either need to be changed in a provided
destination XML or the migration will probably break. I do not think it is
unreasonable to say that if users are trying to shoot themselves, we should not
spend a ridiculous time just so we can prevent that. Otherwise we will get to
the same point as we are now -- there might be a case where a local migration
would work, but users cannot execute it even if they were very cautious and went
through all the things that can prevent it from the technical point of view,
libvirt will still disallow that.
Adding at least partial support where we could say we rely on QEMU failing
reasonably would allow couple of mgmt apps to do more than they can do now. And
they might have taken care of the path collisions (e.g. when running libvirtd in
containers or so).
If localhost migration is only usable in a small subset
scenarios, I'm not convinced it is worth the support
burden. Rarely used & tested features are liable to
bitrot, and migration is already a serious point of
instability in QEMU in general.
Signed-off-by: Shaju Abraham
---
src/qemu/qemu_conf.c | 4 ++--
src/qemu/qemu_domain.c | 16 ++--
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 00801ef01b..6769736d58 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1894,7 +1894,7 @@ qemuGetDomainHugepagePath(const virDomainDef *def,
char *ret = NULL;
if (base && domPath)
-ret = g_strdup_printf("%s/%s", base, domPath);
+ret = g_strdup_printf("%s/%s-%d", base, domPath, def->id);
return ret;
}
@@ -1962,7 +1962,7 @@ qemuGetMemoryBackingDomainPath(const virDomainDef *def,
return -1;
qemuGetMemoryBackingBasePath(cfg, );
-*path = g_strdup_printf("%s/%s", base, shortName);
+*path = g_strdup_printf("%s/%s-%d", base, shortName, def->id);
return 0;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b0c0e1a19b..002c092cf8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2127,11 +2127,13 @@ qemuDomainSetPrivatePathsOld(virQEMUDriverPtr driver,
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
if (!priv->libDir)
-priv->libDir = g_strdup_printf("%s/domain-%s", cfg->libDir,
vm->def->name);
+priv->libDir = g_strdup_printf("%s/domain-%s-%d", cfg->libDir,
+ vm->def->name, vm->def->id);
if (!priv->channelTargetDir)
-
Re: [libvirt PATCH 0/9] syntax-check: fix sc-prohibit-cross-inclusion
On 2/11/20 2:37 AM, Ján Tomko wrote: And drop some legacy stuff, since I already had to open the file. Ján Tomko (9): syntax-check: do not enforce ChangeLog syntax syntax-check: fix sc-prohibit-cross-inclusion syntax-check: drop vulnerable Makefile checks syntax-check: drop CVS keyword expansion check syntax-check: drop update-NEWS-hash syntax-check: exclude: remove deleted files syntax-check: exclude: remove virstring syntax-check: remove README syntax-check: remove some exception mechanisms build-aux/syntax-check.mk | 73 +++ 1 file changed, 5 insertions(+), 68 deletions(-) Reviewed-by: Michal Privoznik Michal
