Re: [PATCH] libxl: initialize shutdown inhibit callback
On 1/17/20 7:37 PM, Marek Marczykowski-Górecki wrote:
> The libxl driver already tries to call shutdown inhibit callback in the
> right places, but only if it's set. That last part was missing,
> resulting in premature shutdown when running libvirtd
> --timeout=...
Wow, that's been overlooked for quite some time.
>
> Signed-off-by: Marek Marczykowski-Górecki
Reviewed-by: Jim Fehlig
and pushed now.
Regards,
Jim
> ---
> src/libxl/libxl_driver.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index bece313ec5..d45e42c100 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -648,8 +648,8 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
>
> static int
> libxlStateInitialize(bool privileged,
> - virStateInhibitCallback callback G_GNUC_UNUSED,
> - void *opaque G_GNUC_UNUSED)
> + virStateInhibitCallback callback,
> + void *opaque)
> {
> libxlDriverConfigPtr cfg;
> char *driverConf = NULL;
> @@ -670,6 +670,9 @@ libxlStateInitialize(bool privileged,
> return VIR_DRV_STATE_INIT_ERROR;
> }
>
> +libxl_driver->inhibitCallback = callback;
> +libxl_driver->inhibitOpaque = opaque;
> +
> /* Allocate bitmap for vnc port reservation */
> if (!(libxl_driver->reservedGraphicsPorts =
> virPortAllocatorRangeNew(_("VNC"),
>
[PATCH] lib: Prohibit parallel connections with tunneled migration
As discussed on the developer list, parallel migration connections
are not compatible with tunneled migration
https://www.redhat.com/archives/libvir-list/2020-January/msg00463.html
Prohibit the concurrent use of parallel and tunneled migration options.
Signed-off-by: Jim Fehlig
---
I added the check to all migration entry points except virDomainMigrate3,
where the p2p and tunneled options are already prohibitied.
src/libvirt-domain.c | 20
1 file changed, 20 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 4074397b30..b910ba6b4d 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3546,6 +3546,10 @@ virDomainMigrate(virDomainPtr domain,
VIR_MIGRATE_NON_SHARED_INC,
error);
+VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_TUNNELLED,
+ VIR_MIGRATE_PARALLEL,
+ error);
+
if (flags & VIR_MIGRATE_OFFLINE) {
if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
@@ -3701,6 +3705,10 @@ virDomainMigrate2(virDomainPtr domain,
VIR_MIGRATE_NON_SHARED_INC,
error);
+VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_TUNNELLED,
+ VIR_MIGRATE_PARALLEL,
+ error);
+
if (flags & VIR_MIGRATE_OFFLINE) {
if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
@@ -4087,6 +4095,10 @@ virDomainMigrateToURI(virDomainPtr domain,
virCheckReadOnlyGoto(domain->conn->flags, error);
virCheckNonNullArgGoto(duri, error);
+VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_TUNNELLED,
+ VIR_MIGRATE_PARALLEL,
+ error);
+
if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
goto error;
@@ -4159,6 +4171,10 @@ virDomainMigrateToURI2(virDomainPtr domain,
virCheckDomainReturn(domain, -1);
virCheckReadOnlyGoto(domain->conn->flags, error);
+VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_TUNNELLED,
+ VIR_MIGRATE_PARALLEL,
+ error);
+
if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
goto error;
@@ -4232,6 +4248,10 @@ virDomainMigrateToURI3(virDomainPtr domain,
virCheckDomainReturn(domain, -1);
virCheckReadOnlyGoto(domain->conn->flags, error);
+VIR_EXCLUSIVE_FLAGS_GOTO(VIR_MIGRATE_TUNNELLED,
+ VIR_MIGRATE_PARALLEL,
+ error);
+
if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0)
goto error;
--
2.24.1
Re: [libvirt PATCH 06/12] conf: add failover attribute to subelement of
On Tue, Jan 21, 2020 at 12:48:27PM -0500, Laine Stump wrote: > On 1/21/20 10:26 AM, Daniel P. Berrangé wrote: > > On Sun, Jan 19, 2020 at 10:24:13PM -0500, Laine Stump wrote: > > > This attribute is only used for virtio-net devices, so it is stored in > > > the virtio part of the anonymous union in virDomainNetDef::driver. An > > I'm not convinced that storing it only for virtio-net is the > > right approach. This feels like a generic concept, that just > > happens to only be implemented by virtio-net in QEMU today. > > IOW, it is valid to store it in the virDOmainNetDef in a > > place that's generally applicable. Any restriction to virtio-net > > belongs in the QEMU driver, rather than the XML parser. > > > Agreed, although in light of your comment on 8/12 (suggesting we just create > an entire new subelement), I guess the point is moot :-) Or rather this point just reinforces comment 8 :-) 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 06/12] conf: add failover attribute to subelement of
On 1/21/20 10:26 AM, Daniel P. Berrangé wrote: On Sun, Jan 19, 2020 at 10:24:13PM -0500, Laine Stump wrote: This attribute is only used for virtio-net devices, so it is stored in the virtio part of the anonymous union in virDomainNetDef::driver. An I'm not convinced that storing it only for virtio-net is the right approach. This feels like a generic concept, that just happens to only be implemented by virtio-net in QEMU today. IOW, it is valid to store it in the virDOmainNetDef in a place that's generally applicable. Any restriction to virtio-net belongs in the QEMU driver, rather than the XML parser. Agreed, although in light of your comment on 8/12 (suggesting we just create an entire new subelement), I guess the point is moot :-)
Re: [libvirt-tck PATCH 1/2] lib: TCK.pm: Favour pubkey auth over passwords on SSH connections
...
> > > +if (! -e "$ssh_key_path") {
> > > +print "# generating a new SSH RSA key pair under
> > > $ssh_dir_path\n";
> >
> > I'm wondering whether I should actually use diag here^ instead, do you have
> > a
> > suggestion Dan?
>
> I guess we do use diag in the rest of the file, so it would be worth
> being consistent, even if it is functionally identical.
Got it, consider it changed.
Thanks,
Erik
Re: [libvirt-tck PATCH 1/2] lib: TCK.pm: Favour pubkey auth over passwords on SSH connections
On Tue, Jan 21, 2020 at 06:08:01PM +0100, Erik Skultety wrote:
> On Tue, Jan 21, 2020 at 05:47:16PM +0100, Erik Skultety wrote:
> > The reason for this change is our Fedora 31 test image, because starting
> > with Fedora 31, the SSH policy for root logins with password
> > authentication changed and password auth is now disabled by default.
> > Since we were relying on this, we're now unable to log in to the guest
> > as root. Let's convert to the SSH keys usage.
> >
> > Signed-off-by: Erik Skultety
> > ---
> > lib/Sys/Virt/TCK.pm | 30 +-
> > 1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
> > index a641d01..5a5c9e4 100644
> > --- a/lib/Sys/Virt/TCK.pm
> > +++ b/lib/Sys/Virt/TCK.pm
> > @@ -408,6 +408,32 @@ sub has_disk_image {
> > return -f $target
> > }
> >
> > +sub ssh_key_path {
> > +my $self = shift;
> > +my $basedir = shift;
> > +
> > +return catfile($basedir, "ssh", "id_rsa");
> > +}
> > +
> > +sub create_host_ssh_keys {
> > +my $self = shift;
> > +
> > +my $scratch = $self->scratch_dir;
> > +my $ssh_dir_path = catfile($scratch, "ssh");
> > +my $ssh_key_path = $self->ssh_key_path($scratch);
> > +
> > +if (! -d "$ssh_dir_path") {
> > +mkdir "$ssh_dir_path", 0700;
> > +}
> > +
> > +if (! -e "$ssh_key_path") {
> > +print "# generating a new SSH RSA key pair under $ssh_dir_path\n";
>
> I'm wondering whether I should actually use diag here^ instead, do you have a
> suggestion Dan?
I guess we do use diag in the rest of the file, so it would be worth
being consistent, even if it is functionally identical.
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-tck PATCH 1/2] lib: TCK.pm: Favour pubkey auth over passwords on SSH connections
On Tue, Jan 21, 2020 at 05:47:16PM +0100, Erik Skultety wrote:
> The reason for this change is our Fedora 31 test image, because starting
> with Fedora 31, the SSH policy for root logins with password
> authentication changed and password auth is now disabled by default.
> Since we were relying on this, we're now unable to log in to the guest
> as root. Let's convert to the SSH keys usage.
>
> Signed-off-by: Erik Skultety
> ---
> lib/Sys/Virt/TCK.pm | 30 +-
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
> index a641d01..5a5c9e4 100644
> --- a/lib/Sys/Virt/TCK.pm
> +++ b/lib/Sys/Virt/TCK.pm
> @@ -408,6 +408,32 @@ sub has_disk_image {
> return -f $target
> }
>
> +sub ssh_key_path {
> +my $self = shift;
> +my $basedir = shift;
> +
> +return catfile($basedir, "ssh", "id_rsa");
> +}
> +
> +sub create_host_ssh_keys {
> +my $self = shift;
> +
> +my $scratch = $self->scratch_dir;
> +my $ssh_dir_path = catfile($scratch, "ssh");
> +my $ssh_key_path = $self->ssh_key_path($scratch);
> +
> +if (! -d "$ssh_dir_path") {
> +mkdir "$ssh_dir_path", 0700;
> +}
> +
> +if (! -e "$ssh_key_path") {
> +print "# generating a new SSH RSA key pair under $ssh_dir_path\n";
I'm wondering whether I should actually use diag here^ instead, do you have a
suggestion Dan?
Re: [libvirt-tck PATCH 2/2] nwfilter: Make use of the SSH pubkey auth rather than password-based auth
On Tue, Jan 21, 2020 at 05:02:09PM +, Daniel P. Berrangé wrote: > On Tue, Jan 21, 2020 at 05:47:17PM +0100, Erik Skultety wrote: > > Not only have SSH keys been a good practice for a while, it fixes our > > SSH connections to the f31 test vm. > > > > Signed-off-by: Erik Skultety > > --- > > scripts/nwfilter/210-no-mac-spoofing.t | 2 +- > > scripts/nwfilter/220-no-ip-spoofing.t | 2 +- > > scripts/nwfilter/230-no-mac-broadcast.t | 2 +- > > scripts/nwfilter/240-no-arp-spoofing.t | 2 +- > > 4 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/scripts/nwfilter/210-no-mac-spoofing.t > > b/scripts/nwfilter/210-no-mac-spoofing.t > > index 95f003a..9798c4f 100644 > > --- a/scripts/nwfilter/210-no-mac-spoofing.t > > +++ b/scripts/nwfilter/210-no-mac-spoofing.t > > @@ -95,7 +95,7 @@ ok($ping =~ "10 received", "ping $guestip test"); > > diag "ssh'ing into $guestip"; > > my $ssh = Net::OpenSSH->new($guestip, > > user => "root", > > -password => $tck->root_password(), > > + key_path => $tck->ssh_key_path($tck->scratch_dir()), > > Tabs in indent here & the other four places. Shoot, I created the patches in a VM where I don't have all my editor settings, sorry, will change. Thanks, Erik
Re: [libvirt-tck PATCH 2/2] nwfilter: Make use of the SSH pubkey auth rather than password-based auth
On Tue, Jan 21, 2020 at 05:47:17PM +0100, Erik Skultety wrote: > Not only have SSH keys been a good practice for a while, it fixes our > SSH connections to the f31 test vm. > > Signed-off-by: Erik Skultety > --- > scripts/nwfilter/210-no-mac-spoofing.t | 2 +- > scripts/nwfilter/220-no-ip-spoofing.t | 2 +- > scripts/nwfilter/230-no-mac-broadcast.t | 2 +- > scripts/nwfilter/240-no-arp-spoofing.t | 2 +- > 4 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/scripts/nwfilter/210-no-mac-spoofing.t > b/scripts/nwfilter/210-no-mac-spoofing.t > index 95f003a..9798c4f 100644 > --- a/scripts/nwfilter/210-no-mac-spoofing.t > +++ b/scripts/nwfilter/210-no-mac-spoofing.t > @@ -95,7 +95,7 @@ ok($ping =~ "10 received", "ping $guestip test"); > diag "ssh'ing into $guestip"; > my $ssh = Net::OpenSSH->new($guestip, > user => "root", > -password => $tck->root_password(), > + key_path => $tck->ssh_key_path($tck->scratch_dir()), Tabs in indent here & the other four places. 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-tck PATCH 1/2] lib: TCK.pm: Favour pubkey auth over passwords on SSH connections
On Tue, Jan 21, 2020 at 05:47:16PM +0100, Erik Skultety wrote: > The reason for this change is our Fedora 31 test image, because starting > with Fedora 31, the SSH policy for root logins with password > authentication changed and password auth is now disabled by default. > Since we were relying on this, we're now unable to log in to the guest > as root. Let's convert to the SSH keys usage. > > Signed-off-by: Erik Skultety > --- > lib/Sys/Virt/TCK.pm | 30 +- > 1 file changed, 29 insertions(+), 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: [libvirt] [tck PATCH 3/3] nwfilter: Fix the expected output from ebtables
On Wed, Jan 15, 2020 at 03:24:00PM +0100, Erik Skultety wrote: > For some reason, some of the PTP link addresses didn't specify the > /128 prefix explicitly which fails the pattern matching in the nwfilter > tests. Odd, I wonder if this is a backcompat break in ebtables itself. > > Signed-off-by: Erik Skultety > --- > .../nwfilter/nwfilterxml2fwallout/ipv6-test.fwall| 12 ++-- > 1 file changed, 6 insertions(+), 6 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] [tck PATCH 2/3] network: Fix the dhcp range output being matched
On Wed, Jan 15, 2020 at 03:23:59PM +0100, Erik Skultety wrote: > Since libvirt commit 82fe58ff libvirt has been formatting the network > mask to the dnsmasq's dhcp-range config option which broke a few of the > networking tests. > > Signed-off-by: Erik Skultety > --- > scripts/networks/networkxml2hostout/tck-testnet-1.dat | 2 +- > scripts/networks/networkxml2hostout/tck-testnet-2.dat | 2 +- > scripts/networks/networkxml2hostout/tck-testnet-3.dat | 2 +- > 3 files changed, 3 insertions(+), 3 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] [tck PATCH 1/3] network: Fix the iptables FORWARD chain name being queried
On Wed, Jan 15, 2020 at 03:23:58PM +0100, Erik Skultety wrote: > libvirt's has been defining private chains within iptables for a while, > only putting a target labels inside the master FORWARD chain which broke > the networking test suite which wasn't adjusted accordingly. Opps, my bad :-( > > Signed-off-by: Erik Skultety > --- > .../networks/networkxml2hostout/tck-testnet-1.dat| 3 ++- > .../networks/networkxml2hostout/tck-testnet-2.dat| 3 ++- > .../networks/networkxml2hostout/tck-testnet-3.dat| 12 +++- > 3 files changed, 11 insertions(+), 7 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] [tck PATCH 0/3] A few network related fixes to get the network suite running
On Wed, Jan 15, 2020 at 03:23:57PM +0100, Erik Skultety wrote: > Erik Skultety (3): > network: Fix the iptables FORWARD chain name being queried > network: Fix the dhcp range output being matched > nwfilter: Fix the expected output from ebtables > > .../networks/networkxml2hostout/tck-testnet-1.dat | 5 +++-- > .../networks/networkxml2hostout/tck-testnet-2.dat | 5 +++-- > .../networks/networkxml2hostout/tck-testnet-3.dat | 14 -- > .../nwfilter/nwfilterxml2fwallout/ipv6-test.fwall | 12 ++-- > 4 files changed, 20 insertions(+), 16 deletions(-) ping
Re: [libvirt] [PATCH] create a thread to handle MigrationParamResetto avoid deadlock
On Fri, Jan 03, 2020 at 10:11:22AM +, Daniel P. Berrangé wrote: > On Fri, Dec 27, 2019 at 01:59:51PM +0800, wang.y...@zte.com.cn wrote: > > Hi Daniel, > > > > Thanks a lot for your review and reply! > > > > > On Mon, Dec 23, 2019 at 04:50:00PM +0100, Michal Prívozník wrote: > > > > On 12/23/19 11:12 AM, Daniel P. Berrangé wrote: > > > > > On Mon, Dec 23, 2019 at 03:13:10PM +0800, Yi Wang wrote: > > > > >> From: Li XueLei > > > > >> > > > > >> Libvirtd no longer receives external requests, after I do the > > > > >> following. > > > > >> > > > > . > > > > > > > > > > > > Libvirt allows multiple connections concurrently and changes made by > > > > > one > > > > > connection are supposed to apply globally. No per-VM state should be > > > > > tied > > > > > to a particular connection. > > > > > > > > This is generally very true, except for migration. > > > > > > Hmm, so in that case we do need to make sure this runs from a non-main > > > event thread as this patch does. I'm surprised we've not seen this > > > before though - i'd think it should be a guaranteed deadlock anytime > > > someone tests this scenario. > > > > > > > > > > > > > > > > > IOW, I don't think we need a thread. We should just stop calling > > > > > qemuMigrationSrcCleanup from the connection close callback entirely > > > > > > > > But I agree that something fishy is going on and this doesn't look like > > > > proper solution. Yi, can you please share the backtrace of other threads > > > > too? Why aren't we getting any reply on the monitor? > > > > > > qemuMonitorSend() just puts date onto the TX queue & waits for the > > > main event loop to send it. We're invoking qemuMonitorSend from > > > the main event loop in this backtrace, hence it'll wait forever > > > for the thread to send it. > > > > > > qemuMonitorSend must never be invoked from the main event thread. > > > > So do you think how to imporve this patch? Any sugesstion is appreciated :-) > > I'm facing a related problem in my patches for providing an "embedded" > QEMU driver - any libvirt API calls that the application makes from > its main event loop will deadlock if they result in a QEMU monitor > command. > > We've got another long standing scalability bug too when dealing > with shutdown takes too long, the main event loop is blocked for an > unreasonably long time. > > I think the solution to all of these problems is to stop using the main > event loop for the QEMU monitor and agent. > > Instead, we should create a new thread for each QEMU VM, and that thread > should run an event loop, handling just the monitor and agent work for > that one VM. > > We can do this quite easily now if we use GLib's GMainContext as the > event loop. > > We'll create a GMainContext and store it in qemuDomainObjPrivatePtr. > In qemuProcessLaunch and qemuProcessReconnect we'll need to spawn a > thread running this GMainContext as an event loop. > > At the end of qemuProcess we'll need to tell this event loop to quit > and stop the thread. > > Then the qemu_agent.c and qemu_monitor.c code need changing to use the > g_source_add_unix_fd () / g_source_remove_unix_fd () APIs instead of > virEventAddHandle/virEventRemoveHandle. Is this approach something you would like to work on implementing, or have already started ? If not, I'll attempt to implement it myself in the next week or so. 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 :|
[libvirt-tck PATCH 2/2] nwfilter: Make use of the SSH pubkey auth rather than password-based auth
Not only have SSH keys been a good practice for a while, it fixes our
SSH connections to the f31 test vm.
Signed-off-by: Erik Skultety
---
scripts/nwfilter/210-no-mac-spoofing.t | 2 +-
scripts/nwfilter/220-no-ip-spoofing.t | 2 +-
scripts/nwfilter/230-no-mac-broadcast.t | 2 +-
scripts/nwfilter/240-no-arp-spoofing.t | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/scripts/nwfilter/210-no-mac-spoofing.t
b/scripts/nwfilter/210-no-mac-spoofing.t
index 95f003a..9798c4f 100644
--- a/scripts/nwfilter/210-no-mac-spoofing.t
+++ b/scripts/nwfilter/210-no-mac-spoofing.t
@@ -95,7 +95,7 @@ ok($ping =~ "10 received", "ping $guestip test");
diag "ssh'ing into $guestip";
my $ssh = Net::OpenSSH->new($guestip,
user => "root",
-password => $tck->root_password(),
+ key_path => $tck->ssh_key_path($tck->scratch_dir()),
master_opts => [-o =>
"UserKnownHostsFile=/dev/null",
-o => "StrictHostKeyChecking=no"]);
diff --git a/scripts/nwfilter/220-no-ip-spoofing.t
b/scripts/nwfilter/220-no-ip-spoofing.t
index bacb861..9615d99 100644
--- a/scripts/nwfilter/220-no-ip-spoofing.t
+++ b/scripts/nwfilter/220-no-ip-spoofing.t
@@ -89,7 +89,7 @@ ok($ebtable =~ "$guestip", "check ebtables entry");
diag "ssh'ing into $guestip";
my $ssh = Net::OpenSSH->new($guestip,
user => "root",
-password => $tck->root_password(),
+ key_path => $tck->ssh_key_path($tck->scratch_dir()),
master_opts => [-o =>
"UserKnownHostsFile=/dev/null",
-o => "StrictHostKeyChecking=no"]);
diff --git a/scripts/nwfilter/230-no-mac-broadcast.t
b/scripts/nwfilter/230-no-mac-broadcast.t
index b518a81..59683fa 100644
--- a/scripts/nwfilter/230-no-mac-broadcast.t
+++ b/scripts/nwfilter/230-no-mac-broadcast.t
@@ -117,7 +117,7 @@ system("/usr/sbin/tcpdump -v -i virbr0 -n host
$networkipbroadcast and ether hos
diag "ssh'ing into $guestip";
my $ssh = Net::OpenSSH->new($guestip,
user => "root",
-password => $tck->root_password(),
+ key_path => $tck->ssh_key_path($tck->scratch_dir()),
master_opts => [-o =>
"UserKnownHostsFile=/dev/null",
-o =>
"StrictHostKeyChecking=no"]);
diff --git a/scripts/nwfilter/240-no-arp-spoofing.t
b/scripts/nwfilter/240-no-arp-spoofing.t
index 77b36d2..2c860ed 100644
--- a/scripts/nwfilter/240-no-arp-spoofing.t
+++ b/scripts/nwfilter/240-no-arp-spoofing.t
@@ -98,7 +98,7 @@ system("/usr/sbin/tcpdump -v -i virbr0 not ip >
/tmp/tcpdump.log &");
diag "ssh'ing into $guestip";
my $ssh = Net::OpenSSH->new($guestip,
user => "root",
-password => $tck->root_password(),
+ key_path => $tck->ssh_key_path($tck->scratch_dir()),
master_opts => [-o =>
"UserKnownHostsFile=/dev/null",
-o => "StrictHostKeyChecking=no"]);
--
2.24.1
[libvirt-tck PATCH 0/2] Convert to SSH pubkey auth rather than password-based auth
Most of the nwfilter tests utilize SSH connections to execute some commands to cross reference whether the requested change in libvirt took effect. However, fedora 31 disables password-based auth for root login which breaks the test suite. Erik Skultety (2): lib: TCK.pm: Favour pubkey auth over passwords on SSH connections nwfilter: Make use of the SSH pubkey auth rather than password-based auth lib/Sys/Virt/TCK.pm | 30 - scripts/nwfilter/210-no-mac-spoofing.t | 2 +- scripts/nwfilter/220-no-ip-spoofing.t | 2 +- scripts/nwfilter/230-no-mac-broadcast.t | 2 +- scripts/nwfilter/240-no-arp-spoofing.t | 2 +- 5 files changed, 33 insertions(+), 5 deletions(-) -- 2.24.1
[libvirt-tck PATCH 1/2] lib: TCK.pm: Favour pubkey auth over passwords on SSH connections
The reason for this change is our Fedora 31 test image, because starting
with Fedora 31, the SSH policy for root logins with password
authentication changed and password auth is now disabled by default.
Since we were relying on this, we're now unable to log in to the guest
as root. Let's convert to the SSH keys usage.
Signed-off-by: Erik Skultety
---
lib/Sys/Virt/TCK.pm | 30 +-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
index a641d01..5a5c9e4 100644
--- a/lib/Sys/Virt/TCK.pm
+++ b/lib/Sys/Virt/TCK.pm
@@ -408,6 +408,32 @@ sub has_disk_image {
return -f $target
}
+sub ssh_key_path {
+my $self = shift;
+my $basedir = shift;
+
+return catfile($basedir, "ssh", "id_rsa");
+}
+
+sub create_host_ssh_keys {
+my $self = shift;
+
+my $scratch = $self->scratch_dir;
+my $ssh_dir_path = catfile($scratch, "ssh");
+my $ssh_key_path = $self->ssh_key_path($scratch);
+
+if (! -d "$ssh_dir_path") {
+mkdir "$ssh_dir_path", 0700;
+}
+
+if (! -e "$ssh_key_path") {
+print "# generating a new SSH RSA key pair under $ssh_dir_path\n";
+system "ssh-keygen -q -t rsa -f $ssh_key_path -N ''";
+}
+
+return $ssh_key_path;
+}
+
sub create_virt_builder_disk {
my $self = shift;
my $bucket = shift;
@@ -424,8 +450,10 @@ sub create_virt_builder_disk {
return $target;
}
+my $ssh_key_path = $self->create_host_ssh_keys;
+
print "# running virt-builder $osname\n";
-system "virt-builder", "--install", "dsniff", "--selinux-relabel",
"--root-password", "password:$password", "--output", $target, $osname;
+system "virt-builder", "--install", "dsniff", "--selinux-relabel",
"--root-password", "password:$password", "--ssh-inject",
"root:file:$ssh_key_path.pub", "--output", $target, $osname;
die "cannot run virt-builder: $?" if $? != 0;
--
2.24.1
Re: [libvirt PATCH 06/12] conf: add failover attribute to subelement of
On Sun, Jan 19, 2020 at 10:24:13PM -0500, Laine Stump wrote:
> This attribute is only used for virtio-net devices, so it is stored in
> the virtio part of the anonymous union in virDomainNetDef::driver. An
I'm not convinced that storing it only for virtio-net is the
right approach. This feels like a generic concept, that just
happens to only be implemented by virtio-net in QEMU today.
IOW, it is valid to store it in the virDOmainNetDef in a
place that's generally applicable. Any restriction to virtio-net
belongs in the QEMU driver, rather than the XML parser.
> example of the new option:
>
>
>
>
>
>
>
>
> The corresponding qemu commandline option can only be set if the qemu
> binary supports it, so we check for the qemu capability before adding
> it.
>
> Signed-off-by: Laine Stump
> ---
> docs/schemas/domaincommon.rng | 5 ++
> src/conf/domain_conf.c| 15 ++
> src/conf/domain_conf.h| 1 +
> src/qemu/qemu_command.c | 3 ++
> src/qemu/qemu_domain.c| 12 -
> .../qemuxml2argvdata/net-virtio-failover.args | 36 +
> .../qemuxml2argvdata/net-virtio-failover.xml | 36 +
> tests/qemuxml2argvtest.c | 3 ++
> .../net-virtio-failover.xml | 50 +++
> tests/qemuxml2xmltest.c | 2 +
> 10 files changed, 161 insertions(+), 2 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/net-virtio-failover.args
> create mode 100644 tests/qemuxml2argvdata/net-virtio-failover.xml
> create mode 100644 tests/qemuxml2xmloutdata/net-virtio-failover.xml
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 76d94b156f..80aea47e36 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3050,6 +3050,11 @@
>
>
>
> +
> +
> +
> +
> +
>
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1379ae1600..29636617a2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11558,6 +11558,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> g_autofree char *txmode = NULL;
> g_autofree char *ioeventfd = NULL;
> g_autofree char *event_idx = NULL;
> +g_autofree char *failover = NULL;
> g_autofree char *queues = NULL;
> g_autofree char *rx_queue_size = NULL;
> g_autofree char *tx_queue_size = NULL;
> @@ -11729,6 +11730,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> txmode = virXMLPropString(cur, "txmode");
> ioeventfd = virXMLPropString(cur, "ioeventfd");
> event_idx = virXMLPropString(cur, "event_idx");
> +failover = virXMLPropString(cur, "failover");
> queues = virXMLPropString(cur, "queues");
> rx_queue_size = virXMLPropString(cur, "rx_queue_size");
> tx_queue_size = virXMLPropString(cur, "tx_queue_size");
> @@ -12105,6 +12107,15 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> }
> def->driver.virtio.event_idx = val;
> }
> +if (failover) {
> +if ((val = virTristateSwitchTypeFromString(failover)) <= 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown interface failover setting
> '%s'"),
> + failover);
> +goto error;
> +}
> +def->driver.virtio.failover = val;
> +}
> if (queues) {
> unsigned int q;
> if (virStrToLong_uip(queues, NULL, 10, &q) < 0) {
> @@ -25416,6 +25427,10 @@ virDomainNetDriverAttributesFormat(char **outstr,
> virBufferAsprintf(&buf, " event_idx='%s'",
>
> virTristateSwitchTypeToString(def->driver.virtio.event_idx));
> }
> +if (def->driver.virtio.failover) {
> +virBufferAsprintf(&buf, " failover='%s'",
> +
> virTristateSwitchTypeToString(def->driver.virtio.failover));
> +}
> if (def->driver.virtio.queues)
> virBufferAsprintf(&buf, " queues='%u'",
> def->driver.virtio.queues);
> if (def->driver.virtio.rx_queue_size)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 5a44113681..af9691d62b 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -934,6 +934,7 @@ struct _virDomainNetDef {
> virDomainNetVirtioTxModeType txmode;
> virTristateSwitch ioeventfd;
> virTristateSwitch event_idx;
> +virTrist
Re: [libvirt PATCH 08/12] conf: add backupAlias attribute to driver subelement
On Sun, Jan 19, 2020 at 10:24:15PM -0500, Laine Stump wrote: > For the subelement (including the > backupAlias attribute) is parsed directly into the hostdev child > object (virDomaniHostdevDef) of the interface (using > virDomainHostdevDefParseXMLSubsys()). But for type='network'> where the network is a pool of hostdevs, the hostdev > object doesn't exist until the network port is allocated at runtime, > and so virDomainHostdevDefParseXMLSubsys() can't be called during XML > parsing, and any backupAlias in the driver subelement of the XML will > be lost. I'm thinking this is a sign that we're storing the data in the wrong way / structure internally, and possibly with the XML schema too. Overall I'm not a fan of having the data scattered across three different structs for different scenarios > For this case, we need to add a backupAlias member to the interface > object (virDomainNetDef), and parse it during the parsing > that happens for all non-hostdev interfaces. Then when the network > port is allocated at runtime and the hostdev child object is created, > we can copy the backupAlias into the hostdev so it is available when > building the QEMU commandline. > > An example usage: > > > > > > > > > > > > > > I get that a big part of the problem is that the element is parsed by different bits of code depending on the type of interface being use, so it is hard to get consistent handling of this. We could perhaps better deal with this by not using at all and instead have a new element we store in virDomainNetDef separately from the driver struct that's causing us trouble. eg 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 02/12] conf: change virDomainVirtioNet*Format() to return void
On Sun, Jan 19, 2020 at 10:24:09PM -0500, Laine Stump wrote: > All three of these functions could only return 0 anyway, so just get > rid of all the extra red tape. > > Signed-off-by: Laine Stump > --- > src/conf/domain_conf.c | 20 ++-- > 1 file changed, 6 insertions(+), 14 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 03/12] conf: rename two static functions
On Sun, Jan 19, 2020 at 10:24:10PM -0500, Laine Stump wrote: > Adding Driver to the names makes them better fit their purpose. > > Signed-off-by: Laine Stump > --- > src/conf/domain_conf.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 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 :|
RFC - (re)design of PCI multifunction hot-unplug
Hi, This is a request for comments in the design of the PCI multifunction hotplug/hot-unplug feature for the QEMU driver that hopefully I'll be sending shortly for review. The feature went through code changes since [1] mostly because of Libvirt changes itself, but Shiva's 2016 original design [2] was preserved. The design consists of a new 'devices' XML element that will contain all hostdevs of the multifunction device to be hotplugged: --- hostdev function 0 XML --- --- hostdev function 1 XML --- (...) --- hostdev function N XML --- The only requirement is function 0 to be present. We'll not force all functions to be hotplugged. Internally, Libvirt will hotplug each non-zero function first, then the zero function for last, which is what QEMU expects for both x86 and Pseries (the 2 archs that supports this feature AFAIK). For unplug, the same XML is required. And this is where the archs starts to behave differently. x86 guests will hot-unplug all functions, regardless of which function is unplugged first. In the example above, hot-unplugging function 1 will trigger all functions to be unplugged. Pseries does not work like that - all non-zero functions need to be unplugged, then the function zero unplug finally triggers the unplug of the slot. And the Libvirt side was doing exactly that. Back in 2019 I interpreted this as a case of a x86 feature paying the price for a Pseries behavior - if Pseries hot-unplug worked the same way, we could hot-unplug all functions with a single call like x86 does, and the user won't need the XML for hot-unplug. I ended up tweaking the Pseries guest in QEMU to execute hot-unplug of all functions in case function 0 is hot-unplugged, emulating the x86 behavior for function 0 [3]. This change alone already eases the unplug code in Libvirt side for Pseries, which is good. But my idea was to not use the XML for hot-unplug. Instead, use regular hot-unplug of function 0 to hot-unplug the whole slot, for both archs. However, recent discussions when contributing the new address='unassigned' type made me re-think the decision I made above: - using XML for hot-unplug isn't too much of a deal, given that the user would need it for hot-plugging the slot anyway. The gain is marginal, at best. - making a single hot-unplug command 'magically' unplug more than one hostdev is not a good idea. First, there's always the chance that the user hot-unplugs the function 0 of a multifunction device, thinking that it's a regular hostdev, just to see whole slot disappearing from the guest. Granted, I can put more code in this case, warning the user about the removal of the whole slot instead of a single hostdev. But in the end, this breaks Laine's golden rule [4] of do not tamper with devices unless the user is explicitly telling to do so, which aims to avoid these cases of devices appearing/disappearing without user consent. All this said, what I'm leaning up to do is to keep Shiva's design, but simplifying the hot-unplug mechanic considering the changes I made in QEMU for Pseries guests. I'm not adamant on it though, so I'd like to hear if I'm missing something and I should go the 'single unplug removes it all' route instead. Thanks, DHB [1] https://www.redhat.com/archives/libvir-list/2018-March/msg00729.html [2] https://www.redhat.com/archives/libvir-list/2016-April/msg01057.html [3] https://lists.gnu.org/archive/html/qemu-ppc/2019-08/msg00447.html [4] this is something Laine talks about here and there in threads I happen to be involved. If someone wants to claim authorship let me know
Re: [libvirt PATCH 12/12] conf/qemu: new attribute "useBackupMAC"
On Tue, Jan 21, 2020 at 12:46:38PM +0200, Dan Kenigsberg wrote: > On Mon, Jan 20, 2020 at 8:33 PM Daniel P. Berrangé > wrote: > > > > On Sun, Jan 19, 2020 at 10:24:19PM -0500, Laine Stump wrote: > > > Current virtio-net drivers that support the failover feature match up > > > the virtio backup device with its corresponding hostdev device by > > > looking for an interface with a matching MAC address. Since libvirt > > > will assign a different random MAC address to each interface that > > > isn't given an explicit MAC address, this means that the configuration > > > for any failover pairs must include explicit matching MAC addresses. > > > > > > To make life easier, we could have libvirt populate the XML config > > > with the same auto-generated MAC address for both interfaces when it > > > detects a failover pair that have no MAC addresses provided (a > > > failover pair can be detected by matching of the > > > virtio interface with of the hostdev > > > interface), but then we would be stuck with that behavior even if the > > > virtio guest driver later eliminated the requirement that mac > > > addresses match. > > > > > > Additionally, some management software uses the MAC address as the > > > primary index for its list of network devices, and can't properly deal > > > with two interfaces having the same MAC address (oVirt). Even > > > libvirt's own virsh utility uses MAC address (combined with interface > > > type) to uniquely identify interfaces for the virsh detach-interface > > > command (in this case, fortunately the runtime interface type is used, > > > so one of the interfaces will always be of type='hostdev' and the > > > other type='something-else", so it doesn't currently cause any problem). > > > > > > In order to remove the necessity of explicitly setting interface MAC > > > addresses, as well as permit the two interfaces of a failover pair to > > > each have a unique index while still fulfilling the current guest > > > driver requirement that the MAC addresses matchin the guest, this > > > patch adds a new attribute "useBackupMAC" that is set on the hostdev > > > interface of the pair. When useBackupMAC='yes', the setup for the > > > hostdev interface will find the virtio failover interface (using > > > backupAlias) and use that interface's MAC address to initialize the > > > MAC address of the hostdev interface; the MAC address in the hostdev > > > interface config remains unchanged, it just isnt used for device > > > initialization. > > > > > > I made this patch to followup on > > > https://bugzilla.redhat.com/show_bug.cgi?id=1693587#c12 (where I > > > suggested this attribute as a possible remedy to oVirt's requirement > > > that each network device have a unique MAC address). > > > > > > Truthfully, I'm not convinced that I want it though, as it seems > > > "a bit" hackish. In particular, I've thought for a long time that the > > > "hostdev manager" code in util/virhostdev.c should really live in the > > > node device driver and be called from the hypervisors via a public API > > > (so that there is one central place on the host that maintains the > > > list of in-use PCI devices and their status), but this patch adds an > > > obstacle to that goal by adding a virDomainDefPtr to more of the APIs in > > > that library - if this was turned into a public API, then entire > > > virDomainDef would need to be serialized and sent in the API call, > > > then parsed at the other end - yuck :-/. NB: there are already > > > functions in virhostdev.h that take a virDomainDefPtr, so maybe I'm > > > being too sensitive. > > > > > > On the upside, it solves a problem, and default bevahior is unchanged. > > > > I don't believe it does solve a real problem. > > > > If a mgmt app is capable of setting useBackupMAC=yes when writing the > > XML doc, then I don't see why it cannot just as easily set the matching > > MAC address when wring the XML doc. > > > > It can still treat both NICs as having a different MAC address in its > > own internal code. All it has to do is use the matching MAC address > > when writing out the XML config it gives to libvirt. > > > > I know oVirt has a facility for hook scripts that are add-ons which > > can arbitrarily munge the XML that VDSM creates. So AFAICT this doesn't > > even need to involve changes to VDSM itself. There can be a hook > > script which looks for the config indicating a failover pair, and > > rewrites the XML to have the matching MAC addr. > > > > Such a workaround then only needs to exist for as long as the mgmt > > app has this problematic limitation without impacting libvirt's > > maint. > > > > So I don't want to take this application specific hack in libvirt. > > I see why you can see it as a hack that should not exist in libvirt. > However I would try to put out my case for it. This feature creates > something very similar to an in-guest bond between an sriov interface > to a virtio one. Currently, we ask end users to configure the bond, > set th
Re: [libvirt] [PATCH 3/4] virsh: secret: Allow setting secrets from file
On Tue, Jan 21, 2020 at 02:43:44PM +0100, Peter Krempa wrote: > On Tue, Jan 21, 2020 at 13:38:13 +, Daniel Berrange wrote: > > On Fri, Jan 10, 2020 at 04:42:43PM +0100, Peter Krempa wrote: > > > The necessity to specify the secret value as command argument is > > > insecure. Allow reading the secret from a file. > > > > > > Signed-off-by: Peter Krempa > > > --- > > > docs/manpages/virsh.rst | 5 +++-- > > > tools/virsh-secret.c| 30 +++--- > > > 2 files changed, 30 insertions(+), 5 deletions(-) > > > > > > diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst > > > index fcc8ef6758..992b1daf90 100644 > > > --- a/docs/manpages/virsh.rst > > > +++ b/docs/manpages/virsh.rst > > > @@ -6558,10 +6558,11 @@ secret-set-value > > > > > > .. code-block:: > > > > > > - secret-set-value secret base64 > > > + secret-set-value secret (--file filename | base64) > > > > > > Set the value associated with *secret* (specified by its UUID) to the > > > value > > > -Base64-encoded value *base64*. > > > +Base64-encoded value *base64* or from file named *filename*. Note that > > > *--file* > > > +and *base64* options are mutually exclusive. > > > > You added a --plain option to secret-get-value. > > > > It would naturally suggest that we do the same here, then we can > > support > > > > secret-set-value $BASE64STR > > secret-set-value --plain $RAWSTR > > I think that both of the above should not have existed in the first > place. Adding the possibility to add plain secrets via argument looks to > me as a step back. If I could do it, I'd remove the base64 via command > line arguments as well. True, we probably should forbid --plain RAWSTR. Removing existing option is not viable, but perhaps we can at least print a warning message to stderr > > secret-set-value --file FILENAME-WITH-BASE64-STR > > This seems a bit pointless to me. Bear in mind that the place you are getting the secret from may already be giving it to you in base64 format, so it is useful to write it to the file as-is, as you would do today when using set-value base64 > > secret-set-value --plain --file FILENAME-WITH-RAW-STR 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/4] virsh: secret: Allow setting secrets from file
On Fri, Jan 10, 2020 at 04:42:43PM +0100, Peter Krempa wrote: The necessity to specify the secret value as command argument is insecure. Allow reading the secret from a file. Signed-off-by: Peter Krempa --- docs/manpages/virsh.rst | 5 +++-- tools/virsh-secret.c| 30 +++--- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index fcc8ef6758..992b1daf90 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6558,10 +6558,11 @@ secret-set-value .. code-block:: - secret-set-value secret base64 + secret-set-value secret (--file filename | base64) Set the value associated with *secret* (specified by its UUID) to the value -Base64-encoded value *base64*. +Base64-encoded value *base64* or from file named *filename*. Note that *--file* +and *base64* options are mutually exclusive. secret-passwd Please include a way to read the secret from an EBCDIC-encoded file, just for completeness. Jano signature.asc Description: PGP signature
Re: [libvirt] [PATCH 3/4] virsh: secret: Allow setting secrets from file
On Tue, Jan 21, 2020 at 13:38:13 +, Daniel Berrange wrote: > On Fri, Jan 10, 2020 at 04:42:43PM +0100, Peter Krempa wrote: > > The necessity to specify the secret value as command argument is > > insecure. Allow reading the secret from a file. > > > > Signed-off-by: Peter Krempa > > --- > > docs/manpages/virsh.rst | 5 +++-- > > tools/virsh-secret.c| 30 +++--- > > 2 files changed, 30 insertions(+), 5 deletions(-) > > > > diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst > > index fcc8ef6758..992b1daf90 100644 > > --- a/docs/manpages/virsh.rst > > +++ b/docs/manpages/virsh.rst > > @@ -6558,10 +6558,11 @@ secret-set-value > > > > .. code-block:: > > > > - secret-set-value secret base64 > > + secret-set-value secret (--file filename | base64) > > > > Set the value associated with *secret* (specified by its UUID) to the value > > -Base64-encoded value *base64*. > > +Base64-encoded value *base64* or from file named *filename*. Note that > > *--file* > > +and *base64* options are mutually exclusive. > > You added a --plain option to secret-get-value. > > It would naturally suggest that we do the same here, then we can > support > > secret-set-value $BASE64STR > secret-set-value --plain $RAWSTR I think that both of the above should not have existed in the first place. Adding the possibility to add plain secrets via argument looks to me as a step back. If I could do it, I'd remove the base64 via command line arguments as well. > secret-set-value --file FILENAME-WITH-BASE64-STR This seems a bit pointless to me. > secret-set-value --plain --file FILENAME-WITH-RAW-STR
Re: [libvirt] [PATCH 4/4] docs: secret: Unify and sanitize examples on how to set secret value
On Fri, Jan 10, 2020 at 04:42:44PM +0100, Peter Krempa wrote: > Discourage passing secrets as commandline arguments. > > Signed-off-by: Peter Krempa > --- > docs/formatsecret.html.in | 86 +-- > 1 file changed, 55 insertions(+), 31 deletions(-) > > diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in > index 8f5383cf64..61a8396682 100644 > --- a/docs/formatsecret.html.in > +++ b/docs/formatsecret.html.in > @@ -76,13 +76,13 @@ > > # virsh secret-define volume-secret.xml > Secret 0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f created > -# > -# MYSECRET=`printf %s "open sesame" | base64` > -# virsh secret-set-value 0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f $MYSECRET > -Secret value set > -# > > > + > + See virsh secret-set-value on how > + to set the value of the secret. > + > + > >The volume type secret can be supplied either in volume XML during >creation of a storage > volume > @@ -103,12 +103,11 @@ Secret value set > > # virsh secret-define luks-secret.xml > Secret f52a81b2-424e-490c-823d-6bd4235bc57 created > -# > -# MYSECRET=`printf %s "letmein" | base64` > -# virsh secret-set-value f52a81b2-424e-490c-823d-6bd4235bc57 $MYSECRET > -Secret value set > -# > > + > + See virsh secret-set-value on how > + to set the value of the secret. > + > > >The volume type secret can be supplied in domain XML for a luks storage > @@ -156,13 +155,11 @@ Secret 1b40a534-8301-45d5-b1aa-11894ebb1735 created > UUID Usage > --- > 1b40a534-8301-45d5-b1aa-11894ebb1735 cephx ceph_example > -# > -# CEPHPHRASE=`printf %s "pass phrase" | base64` > -# virsh secret-set-value 1b40a534-8301-45d5-b1aa-11894ebb1735 $CEPHPHRASE > -Secret value set > - > -# > > + > + See virsh secret-set-value on how > + to set the value of the secret. > + > > >The ceph secret can then be used by UUID or by the > @@ -229,7 +226,9 @@ incominguser myname mysecret > > >Next, use virsh secret-define iscsi-secret.xml to define > - the secret and virsh secret-set-value using the generated > + the secret and > + virsh secret-set-value > + using the generated >UUID value and a base64 generated secret value in order to define the >chosen secret pass phrase. The pass phrase must match the password >used in the iSCSI authentication configuration file. > @@ -243,12 +242,13 @@ Secret c4dbe20b-b1a3-4ac1-b6e6-2ac97852ebb6 created > --- > c4dbe20b-b1a3-4ac1-b6e6-2ac97852ebb6 iscsi libvirtiscsi > > -# MYSECRET=`printf %s "mysecret" | base64` > -# virsh secret-set-value c4dbe20b-b1a3-4ac1-b6e6-2ac97852ebb6 $MYSECRET > -Secret value set > -# > > > + > + See virsh secret-set-value on how > + to set the value of the secret. > + > + > >The iSCSI secret can then be used by UUID or by the >usage name via theelement in a domain's > @@ -313,19 +313,13 @@ Secret 718c71bd-67b5-4a2b-87ec-a24e8ca200dc created >Once the secret is defined, a secret value will need to be set. The >secret would be the passphrase used to access the TLS credentials. >The following is a simple example of using > - virsh secret-set-value to set the secret value. The > + virsh secret-set-value to > set > + the secret value. The > >virSecretSetValue API may also be used to set >a more secure secret without using printable/readable characters. > > > - > -# MYSECRET=`printf %s "letmein" | base64` > -# virsh secret-set-value 718c71bd-67b5-4a2b-87ec-a24e8ca200dc $MYSECRET > -Secret value set > - > - > - > Usage type "vtpm" > > > @@ -370,17 +364,47 @@ Secret 6dd3e4a5-1d76-44ce-961f-f119f5aad935 created >Once the secret is defined, a secret value will need to be set. The >secret would be the passphrase used to decrypt the vTPM state. >The following is a simple example of using > - virsh secret-set-value to set the secret value. The > + virsh secret-set-value > + to set the secret value. The > >virSecretSetValue API may also be used to set >a more secure secret without using printable/readable characters. > > > +Setting secret values in virsh > + > + > + To set the value of the secret you can use the following virsh > commands. > + If the secret is a password-like string (printable characters, no > newline) > + you can use: > + > + > +# virsh secret-passwd 6dd3e4a5-1d76-44ce-961f-f119f5aad935 > +Enter new value for secret: > +Secret value set > + > + > + > + Another secure option is to read the secret from a file. This way the > + secret can contain any bytes (even NUL and non-
Re: [PATCH 1/2] pvpanic: introduce crashloaded for pvpanic
Paolo Bonzini writes: > On 21/01/20 09:22, Markus Armbruster wrote: >> zhenwei pi writes: >> >>> Add bit 1 for pvpanic. This bit means that guest hits a panic, but >>> guest wants to handle error by itself. Typical case: Linux guest runs >>> kdump in panic. It will help us to separate the abnormal reboot from >>> normal operation. >>> >>> Signed-off-by: zhenwei pi >>> --- >>> docs/specs/pvpanic.txt | 8 ++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt >>> index c7bbacc778..bdea68a430 100644 >>> --- a/docs/specs/pvpanic.txt >>> +++ b/docs/specs/pvpanic.txt >>> @@ -16,8 +16,12 @@ pvpanic exposes a single I/O port, by default 0x505. On >>> read, the bits >>> recognized by the device are set. Software should ignore bits it doesn't >>> recognize. On write, the bits not recognized by the device are ignored. >>> Software should set only bits both itself and the device recognize. >> >> Guest software, I presume. >> >>> -Currently, only bit 0 is recognized, setting it indicates a guest panic >>> -has happened. >>> + >>> +Bit Definition >>> +-- >>> +bit 0: setting it indicates a guest panic has happened. >>> +bit 1: named crashloaded. setting it indicates a guest panic and run >>> + kexec to handle error by guest itself. >> >> Suggest to scratch "named crashloaded." > > bit 1: a guest panic has happened and will be handled by the guest; >the host should record it or report it, but should not affect >the execution of the guest. > > ? > > Paolo Works for me, thanks!
Re: [libvirt] [PATCH 0/4] virsh: secret: Improve handling of secret value
On 1/21/20 10:03 AM, Peter Krempa wrote: On Tue, Jan 21, 2020 at 09:57:22 -0300, Daniel Henrique Barboza wrote: On 1/10/20 12:42 PM, Peter Krempa wrote: The currently existing virsh APIs for secrets are awful for human use and don't promote security. Peter Krempa (4): virsh: secret: Add 'secret-passwd' command virsh: secret: Allow getting secret's value without base64 encoding virsh: secret: Allow setting secrets from file docs: secret: Unify and sanitize examples on how to set secret value docs/formatsecret.html.in | 86 ++-- docs/manpages/virsh.rst | 26 - tools/virsh-secret.c | 116 -- 3 files changed, 189 insertions(+), 39 deletions(-) Code-wise LGTM. I have a question about the design though. Shouldn't we ask for a password confirmation when setting the secret via secret-passwd? This would be more on par with how 'passwd' works in Linux, and can also help to prevent user typos when setting a secret. I don't really think that it's necessary. When you mess up changing of your account password you won't be able to log in thus it's a good idea to make more sure that you didn't miss-type it. With libvirt secrets, it doesn't prevent you from fixing it later as you aren't even asked for the previous value of the secret. Those are fair points. I agree. All patches: Reviewed-by: Daniel Henrique Barboza
Re: [libvirt] [PATCH 1/4] virsh: secret: Add 'secret-passwd' command
On Tue, Jan 21, 2020 at 13:34:27 +, Daniel Berrange wrote: > On Fri, Jan 10, 2020 at 04:42:41PM +0100, Peter Krempa wrote: > > Add a command which allows to read a secret value from terminal. > > 'secret-passwd' is chosen as a name as the password has limitations as > > passwords do have (printable, terminated by newline which is not > > contained in the value). This makes it way more user-friendly to use the > > secret driver with virsh when setting a value. > > In a later patch you already extend secret-set-value to have a new > "--filename PATH" arg. I think we should do the same for interactive > usage, eg > > $ virsh secret-set-value --prompt > > or --interactive / -i Hmm, yeah. Originally I've added this one first for simplicity and then dealt with the --filename thing, but you are right that this is the better option.
Re: [libvirt] [PATCH 3/4] virsh: secret: Allow setting secrets from file
On Fri, Jan 10, 2020 at 04:42:43PM +0100, Peter Krempa wrote: > The necessity to specify the secret value as command argument is > insecure. Allow reading the secret from a file. > > Signed-off-by: Peter Krempa > --- > docs/manpages/virsh.rst | 5 +++-- > tools/virsh-secret.c| 30 +++--- > 2 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst > index fcc8ef6758..992b1daf90 100644 > --- a/docs/manpages/virsh.rst > +++ b/docs/manpages/virsh.rst > @@ -6558,10 +6558,11 @@ secret-set-value > > .. code-block:: > > - secret-set-value secret base64 > + secret-set-value secret (--file filename | base64) > > Set the value associated with *secret* (specified by its UUID) to the value > -Base64-encoded value *base64*. > +Base64-encoded value *base64* or from file named *filename*. Note that > *--file* > +and *base64* options are mutually exclusive. You added a --plain option to secret-get-value. It would naturally suggest that we do the same here, then we can support secret-set-value $BASE64STR secret-set-value --plain $RAWSTR secret-set-value --file FILENAME-WITH-BASE64-STR secret-set-value --plain --file FILENAME-WITH-RAW-STR 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/4] virsh: secret: Allow getting secret's value without base64 encoding
On Fri, Jan 10, 2020 at 04:42:42PM +0100, Peter Krempa wrote: > Users might want to get the raw value instead of dealing with base64 > encoding. This might be useful for redirection to file and also for > simple human-readable secrets. > > Signed-off-by: Peter Krempa > --- > docs/manpages/virsh.rst | 6 +- > tools/virsh-secret.c| 16 ++-- > 2 files changed, 19 insertions(+), 3 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 1/4] virsh: secret: Add 'secret-passwd' command
On Fri, Jan 10, 2020 at 04:42:41PM +0100, Peter Krempa wrote:
> Add a command which allows to read a secret value from terminal.
> 'secret-passwd' is chosen as a name as the password has limitations as
> passwords do have (printable, terminated by newline which is not
> contained in the value). This makes it way more user-friendly to use the
> secret driver with virsh when setting a value.
In a later patch you already extend secret-set-value to have a new
"--filename PATH" arg. I think we should do the same for interactive
usage, eg
$ virsh secret-set-value --prompt
or --interactive / -i
>
> Signed-off-by: Peter Krempa
> ---
> docs/manpages/virsh.rst | 15 +
> tools/virsh-secret.c| 70 +
> 2 files changed, 85 insertions(+)
>
> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> index 6446a903ca..03364684b5 100644
> --- a/docs/manpages/virsh.rst
> +++ b/docs/manpages/virsh.rst
> @@ -6564,6 +6564,21 @@ Set the value associated with *secret* (specified by
> its UUID) to the value
> Base64-encoded value *base64*.
>
>
> +secret-passwd
> +
> +
> +**Syntax:**
> +
> +.. code-block::
> +
> + secret-passwd secret
> +
> +Set the value associated with *secret* (specified by its UUID) to a string
> +read from stdin. Note that input is terminated by a newline and the secret
> +can't contain non-printable characters. Use *secret-set-value* for generic
> +secrets. Note that this requires a terminal associated with virsh to read
> +the password.
> +
> secret-get-value
>
>
> diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c
> index 66369a25dc..9f64be6b14 100644
> --- a/tools/virsh-secret.c
> +++ b/tools/virsh-secret.c
> @@ -219,6 +219,70 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd)
> return ret;
> }
>
> +
> +/*
> + * "secret-passwd" command
> + */
> +static const vshCmdInfo info_secret_passwd[] = {
> +{.name = "help",
> + .data = N_("set a secret value from stdin")
> +},
> +{.name = "desc",
> + .data = N_("Set a secret value from stdin")
> +},
> +{.name = NULL}
> +};
> +
> +static const vshCmdOptDef opts_secret_passwd[] = {
> +{.name = "secret",
> + .type = VSH_OT_DATA,
> + .flags = VSH_OFLAG_REQ,
> + .help = N_("secret UUID"),
> + .completer = virshSecretUUIDCompleter,
> +},
> +{.name = NULL}
> +};
> +
> +static bool
> +cmdSecretPasswd(vshControl *ctl,
> +const vshCmd *cmd)
> +{
> +virSecretPtr secret;
> +g_autofree char *value = NULL;
> +int res;
> +bool ret = false;
> +
> +if (!ctl->istty) {
> +vshError(ctl, "%s", _("secret-passwd requires a terminal"));
> +return false;
> +}
> +
> +if (!(secret = virshCommandOptSecret(ctl, cmd, NULL)))
> +return false;
> +
> +vshPrint(ctl, "%s", _("Enter new value for secret:"));
> +fflush(stdout);
> +
> +if (!(value = getpass(""))) {
> +vshError(ctl, "%s", _("Failed to read secret"));
> +goto cleanup;
> +}
> +
> +res = virSecretSetValue(secret, (unsigned char *) value, strlen(value),
> 0);
> +
> +if (res != 0) {
> +vshError(ctl, "%s", _("Failed to set secret value"));
> +goto cleanup;
> +}
> +vshPrintExtra(ctl, "%s", _("Secret value set\n"));
> +ret = true;
> +
> + cleanup:
> +virSecretFree(secret);
> +return ret;
> +}
> +
> +
> /*
> * "secret-get-value" command
> */
> @@ -805,6 +869,12 @@ const vshCmdDef secretCmds[] = {
> .info = info_secret_set_value,
> .flags = 0
> },
> +{.name = "secret-passwd",
> + .handler = cmdSecretPasswd,
> + .opts = opts_secret_passwd,
> + .info = info_secret_passwd,
> + .flags = 0
> +},
> {.name = "secret-undefine",
> .handler = cmdSecretUndefine,
> .opts = opts_secret_undefine,
> --
> 2.24.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
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/4] virsh: secret: Improve handling of secret value
On Tue, Jan 21, 2020 at 09:57:22 -0300, Daniel Henrique Barboza wrote: > > > On 1/10/20 12:42 PM, Peter Krempa wrote: > > The currently existing virsh APIs for secrets are awful for human use > > and don't promote security. > > > > Peter Krempa (4): > >virsh: secret: Add 'secret-passwd' command > >virsh: secret: Allow getting secret's value without base64 encoding > >virsh: secret: Allow setting secrets from file > >docs: secret: Unify and sanitize examples on how to set secret value > > > > docs/formatsecret.html.in | 86 ++-- > > docs/manpages/virsh.rst | 26 - > > tools/virsh-secret.c | 116 -- > > 3 files changed, 189 insertions(+), 39 deletions(-) > > > > > Code-wise LGTM. I have a question about the design though. > > Shouldn't we ask for a password confirmation when setting the secret > via secret-passwd? This would be more on par with how 'passwd' works > in Linux, and can also help to prevent user typos when setting a > secret. I don't really think that it's necessary. When you mess up changing of your account password you won't be able to log in thus it's a good idea to make more sure that you didn't miss-type it. With libvirt secrets, it doesn't prevent you from fixing it later as you aren't even asked for the previous value of the secret.
Re: [libvirt] [PATCH 0/4] virsh: secret: Improve handling of secret value
On 1/10/20 12:42 PM, Peter Krempa wrote: The currently existing virsh APIs for secrets are awful for human use and don't promote security. Peter Krempa (4): virsh: secret: Add 'secret-passwd' command virsh: secret: Allow getting secret's value without base64 encoding virsh: secret: Allow setting secrets from file docs: secret: Unify and sanitize examples on how to set secret value docs/formatsecret.html.in | 86 ++-- docs/manpages/virsh.rst | 26 - tools/virsh-secret.c | 116 -- 3 files changed, 189 insertions(+), 39 deletions(-) Code-wise LGTM. I have a question about the design though. Shouldn't we ask for a password confirmation when setting the secret via secret-passwd? This would be more on par with how 'passwd' works in Linux, and can also help to prevent user typos when setting a secret. Thanks, DHB
Re: [libvirt] [PATCH 3/3] qemu: backup: Implement support for backup disk bitmap name configuration
On Tue, Jan 14, 2020 at 08:50:55 -0600, Eric Blake wrote:
> On 1/9/20 12:31 PM, Peter Krempa wrote:
> > Use the user-configured name of the bitmap when merging the appropriate
> > bitmaps for an incremental backup so that the user can see it as
> > configured. Additionally expose the default bitmap name if nothing is
> > configured.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > src/qemu/qemu_backup.c | 9 -
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
> > index 2cc0e6ab07..23518a5d40 100644
> > --- a/src/qemu/qemu_backup.c
> > +++ b/src/qemu/qemu_backup.c
> > @@ -322,7 +322,10 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm,
> > return -1;
> >
> > if (incremental) {
> > -dd->incrementalBitmap = g_strdup_printf("backup-%s",
> > dd->domdisk->dst);
> > +if (dd->backupdisk->exportbitmap)
> > +dd->incrementalBitmap = g_strdup(dd->backupdisk->exportbitmap);
>
> Do we need to worry about the user requesting a name that would cause
> conflicts with existing bitmaps in the qcow2 file? I'm worried this can
> open the door for odd failures if the user accidentally stomps on names that
> libvirt thought were available for its own use.
This name will be passed to a transactioned 'block-dirty-bitmap-add' so
existing bitmaps should not be overwritten.
I'm not sure whether it's worth forbidding. Users actually might want to
pick a different bitmap name when they accidentally used the one libvirt
would pick for the temporary bitmap when left blank in which case the
code would fail without any way to fix it.
Re: [libvirt] [PATCH 0/4] virsh: secret: Improve handling of secret value
On Fri, Jan 10, 2020 at 16:42:40 +0100, Peter Krempa wrote: > The currently existing virsh APIs for secrets are awful for human use > and don't promote security. > > Peter Krempa (4): > virsh: secret: Add 'secret-passwd' command > virsh: secret: Allow getting secret's value without base64 encoding > virsh: secret: Allow setting secrets from file > docs: secret: Unify and sanitize examples on how to set secret value Ping
Re: [libvirt] [PATCH v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Tue, Jan 14, 2020 at 10:34:21AM +0100, Marc Hartmayer wrote: > On Fri, Dec 13, 2019 at 03:32 PM -0500, Cole Robinson > wrote: > > On 12/12/19 8:46 AM, Marc Hartmayer wrote: > >> On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson > >> wrote: > >>> On 11/14/19 12:44 PM, Marc Hartmayer wrote: > > > > danpb has your thinking changed on this? Previous discussion context is > > in this thread: > > https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html > > > > Thanks, > > Cole > > > > Polite ping. Sorry for the ridiculous delay in responding. This has been on my todo list for ages and I didn't prioritize completing it well enough. I've sent a mail with my thoughts now. 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 v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Wed, Dec 11, 2019 at 08:11:38PM -0500, Cole Robinson wrote:
> On 11/14/19 12:44 PM, Marc Hartmayer wrote:
> > The commit 'close callback: move it to driver' (88f09b75eb99) moved
> > the responsibility for the close callback to the driver. But if the
> > driver doesn't support the connectRegisterCloseCallback API this
> > function does nothing, even no unsupported error report. This behavior
> > may lead to problems, for example memory leaks, as the caller cannot
> > differentiate whether the close callback was 'really' registered or
> > not.
> >
>
>
> Full context:
> v1 subtrhead with jferlan and danpb:
> https://www.redhat.com/archives/libvir-list/2018-March/msg00906.html
> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
>
> v2 subthread with john:
> https://www.redhat.com/archives/libvir-list/2018-April/msg02534.html
>
> My first thought is, why not just make this API start raising an error
> if it isn't supported?
>
> But you tried that here:
> https://www.redhat.com/archives/libvir-list/2018-March/msg00405.html
>
> I'm not really sure I buy the argument that we can't change the
> semantics of the API here. This is the only callback API that seems to
> not raise an explicit error. It's documented to raise an error. And
> there's possible memory leak due the ambiguity.
It can raise an error because you are only permitted to register the
close callback once - a duplicated call reports an error. Also any
other invalid parameters result in an error. So this is not
inconsistent with the idea that registering a close callback is
supported for all drivers.
>
> Yeah I see that virsh needs to be updated to match. In practice virsh
> shouldn't be a problem: this issue will only hit for local drivers, and
> virsh and client library will be updated together for that case.
The very fact that we need to update virsh shows that this would
be an API regression. That we only know of virsh having a problem
is not a valid reason. It is only by luck that virt-viewer doesn't
have the same problem, because for inexplicable reasons we didn't
handling the error as an error condition.
> BUT, if we stick with this direction, then we need to extend the doc
> text here to describe all of this:
>
> * Returns -1 if the driver can support close callback, but registering
> one failed. User must free opaque?
> * Returns 0 if the driver does not support close callback. We will free
> data for you
> * Returns 0 if the driver successfully registered a close callback. When
> that callback is triggered, opaque will be free'd
>
> But that sounds pretty nutty IMO :/
This is giving apps an uncessary level of impl detail for their
needs.
* Returns -1: if a close callback was already registered, or
if one of the parameters was invalid.
* Returns 0: if the close callback was successfully registered
The driver specific caveat is described earlier in the docs, that
not all drivers will invoke the close callback, as some may not
ever be in a situation where there is a connection is closed. Not
ever invoking the callback is not a bug, as it simply means the
error condition the callback is designed to report has not
arisen.
To fix the leak of te opaque data, we need to partially revert
the change that caused this leak in the first place
88f09b75eb99415c7f2ce3d1b010600ba8e37580.
That commit introduces some per driver handling into the close
callbacks. This is conceptually fine. The mistake was that the
virConnectCloseCallbackDataPtr closeCallback;
field was moved out of virConnectPtr, and into the per-driver
private structs. This meant that we no longer kept track of
the callback in other drivers, and thus no longer free'd the
opaque data when calling Unregister.
So from the public API entry point I think we need approx
this change:
diff --git a/src/datatypes.h b/src/datatypes.h
index 2d0407f7ec..924ef8d8e9 100644
--- a/src/datatypes.h
+++ b/src/datatypes.h
@@ -546,6 +546,9 @@ struct _virConnect {
virError err; /* the last error */
virErrorFunc handler; /* associated handler */
void *userData; /* the user data */
+
+/* Per-connection close callback */
+virConnectCloseCallbackDataPtr closeCallback;
};
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virConnect, virObjectUnref);
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index bc3d1d2803..68517bae9c 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1444,9 +1444,20 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
virCheckConnectReturn(conn, -1);
virCheckNonNullArgGoto(cb, error);
+if (virConnectCloseCallbackDataGetCallback(conn->closeCallback) != cb) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("A different callback was requested"));
+goto error;
+}
+
+virConnectCloseCallbackDataRegister(conn->closeCallback, conn, cb,
+opaque, freecb);
+
if (conn->driver->connectRegisterCloseCallb
Re: [PATCH 1/2] pvpanic: introduce crashloaded for pvpanic
On 21/01/20 09:22, Markus Armbruster wrote: > zhenwei pi writes: > >> Add bit 1 for pvpanic. This bit means that guest hits a panic, but >> guest wants to handle error by itself. Typical case: Linux guest runs >> kdump in panic. It will help us to separate the abnormal reboot from >> normal operation. >> >> Signed-off-by: zhenwei pi >> --- >> docs/specs/pvpanic.txt | 8 ++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt >> index c7bbacc778..bdea68a430 100644 >> --- a/docs/specs/pvpanic.txt >> +++ b/docs/specs/pvpanic.txt >> @@ -16,8 +16,12 @@ pvpanic exposes a single I/O port, by default 0x505. On >> read, the bits >> recognized by the device are set. Software should ignore bits it doesn't >> recognize. On write, the bits not recognized by the device are ignored. >> Software should set only bits both itself and the device recognize. > > Guest software, I presume. > >> -Currently, only bit 0 is recognized, setting it indicates a guest panic >> -has happened. >> + >> +Bit Definition >> +-- >> +bit 0: setting it indicates a guest panic has happened. >> +bit 1: named crashloaded. setting it indicates a guest panic and run >> + kexec to handle error by guest itself. > > Suggest to scratch "named crashloaded." bit 1: a guest panic has happened and will be handled by the guest; the host should record it or report it, but should not affect the execution of the guest. ? Paolo
Re: [libvirt PATCH 12/12] conf/qemu: new attribute "useBackupMAC"
On Mon, Jan 20, 2020 at 8:33 PM Daniel P. Berrangé wrote: > > On Sun, Jan 19, 2020 at 10:24:19PM -0500, Laine Stump wrote: > > Current virtio-net drivers that support the failover feature match up > > the virtio backup device with its corresponding hostdev device by > > looking for an interface with a matching MAC address. Since libvirt > > will assign a different random MAC address to each interface that > > isn't given an explicit MAC address, this means that the configuration > > for any failover pairs must include explicit matching MAC addresses. > > > > To make life easier, we could have libvirt populate the XML config > > with the same auto-generated MAC address for both interfaces when it > > detects a failover pair that have no MAC addresses provided (a > > failover pair can be detected by matching of the > > virtio interface with of the hostdev > > interface), but then we would be stuck with that behavior even if the > > virtio guest driver later eliminated the requirement that mac > > addresses match. > > > > Additionally, some management software uses the MAC address as the > > primary index for its list of network devices, and can't properly deal > > with two interfaces having the same MAC address (oVirt). Even > > libvirt's own virsh utility uses MAC address (combined with interface > > type) to uniquely identify interfaces for the virsh detach-interface > > command (in this case, fortunately the runtime interface type is used, > > so one of the interfaces will always be of type='hostdev' and the > > other type='something-else", so it doesn't currently cause any problem). > > > > In order to remove the necessity of explicitly setting interface MAC > > addresses, as well as permit the two interfaces of a failover pair to > > each have a unique index while still fulfilling the current guest > > driver requirement that the MAC addresses matchin the guest, this > > patch adds a new attribute "useBackupMAC" that is set on the hostdev > > interface of the pair. When useBackupMAC='yes', the setup for the > > hostdev interface will find the virtio failover interface (using > > backupAlias) and use that interface's MAC address to initialize the > > MAC address of the hostdev interface; the MAC address in the hostdev > > interface config remains unchanged, it just isnt used for device > > initialization. > > > > I made this patch to followup on > > https://bugzilla.redhat.com/show_bug.cgi?id=1693587#c12 (where I > > suggested this attribute as a possible remedy to oVirt's requirement > > that each network device have a unique MAC address). > > > > Truthfully, I'm not convinced that I want it though, as it seems > > "a bit" hackish. In particular, I've thought for a long time that the > > "hostdev manager" code in util/virhostdev.c should really live in the > > node device driver and be called from the hypervisors via a public API > > (so that there is one central place on the host that maintains the > > list of in-use PCI devices and their status), but this patch adds an > > obstacle to that goal by adding a virDomainDefPtr to more of the APIs in > > that library - if this was turned into a public API, then entire > > virDomainDef would need to be serialized and sent in the API call, > > then parsed at the other end - yuck :-/. NB: there are already > > functions in virhostdev.h that take a virDomainDefPtr, so maybe I'm > > being too sensitive. > > > > On the upside, it solves a problem, and default bevahior is unchanged. > > I don't believe it does solve a real problem. > > If a mgmt app is capable of setting useBackupMAC=yes when writing the > XML doc, then I don't see why it cannot just as easily set the matching > MAC address when wring the XML doc. > > It can still treat both NICs as having a different MAC address in its > own internal code. All it has to do is use the matching MAC address > when writing out the XML config it gives to libvirt. > > I know oVirt has a facility for hook scripts that are add-ons which > can arbitrarily munge the XML that VDSM creates. So AFAICT this doesn't > even need to involve changes to VDSM itself. There can be a hook > script which looks for the config indicating a failover pair, and > rewrites the XML to have the matching MAC addr. > > Such a workaround then only needs to exist for as long as the mgmt > app has this problematic limitation without impacting libvirt's > maint. > > So I don't want to take this application specific hack in libvirt. I see why you can see it as a hack that should not exist in libvirt. However I would try to put out my case for it. This feature creates something very similar to an in-guest bond between an sriov interface to a virtio one. Currently, we ask end users to configure the bond, set the sriov interface as the primary interface, and allow mac-spoofing on the virio interface. To me, the purpose of this feature is to remove the need for end-user intervention. The bond device no longer need to be created in the guest, it can
Re: [PATCH 2/2] run.in: Include tools directory on $PATH.
On Thu, Jan 16, 2020 at 05:15:42PM +, Richard W.M. Jones wrote: > You normally want to run the locally compiled copy of virsh. Trying > to run the installed version with the locally compiled library is a > recipe for problems with missing symbols and so on. By adding tools > to the path we can ensure that (eg) the libguestfs test suite will use > compatible copies of the library and virsh. > > Signed-off-by: Richard W.M. Jones > --- > run.in | 13 +++-- > 1 file changed, 7 insertions(+), 6 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: [PATCH 1/2] run.in: Add intelligent prepend function.
On Thu, Jan 16, 2020 at 05:15:41PM +, Richard W.M. Jones wrote: > This has been used in libguestfs and libnbd for quite a while as it > makes the ./run script easier to read and write. > > See also: > http://stackoverflow.com/a/9631350 > > Signed-off-by: Richard W.M. Jones > --- > run.in | 20 +--- > 1 file changed, 9 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: [PATCH 2/2] pvpanic: implement crashloaded event handling
zhenwei pi writes:
> Handle bit 1 write, then post event to monitor.
>
> Suggested by Paolo, declear a new event, using GUEST_PANICKED could
> cause upper layers to react by shutting down or rebooting the guest.
>
> In advance for extention, add GuestPanicInformation in event message.
>
> Signed-off-by: zhenwei pi
> ---
> hw/misc/pvpanic.c | 11 +--
> include/sysemu/runstate.h | 1 +
> qapi/run-state.json | 22 +-
> vl.c | 12
> 4 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index d65ac86478..4ebda7872a 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -21,11 +21,13 @@
> #include "hw/qdev-properties.h"
> #include "hw/misc/pvpanic.h"
>
> -/* The bit of supported pv event */
> +/* The bit of supported pv event, TODO: include uapi header and remove this
> */
> #define PVPANIC_F_PANICKED 0
> +#define PVPANIC_F_CRASHLOADED 1
>
> /* The pv event value */
> #define PVPANIC_PANICKED(1 << PVPANIC_F_PANICKED)
> +#define PVPANIC_CRASHLOADED (1 << PVPANIC_F_CRASHLOADED)
>
> #define ISA_PVPANIC_DEVICE(obj)\
> OBJECT_CHECK(PVPanicState, (obj), TYPE_PVPANIC)
> @@ -34,7 +36,7 @@ static void handle_event(int event)
> {
> static bool logged;
>
> -if (event & ~PVPANIC_PANICKED && !logged) {
> +if (event & ~(PVPANIC_PANICKED | PVPANIC_CRASHLOADED) && !logged) {
> qemu_log_mask(LOG_GUEST_ERROR, "pvpanic: unknown event %#x.\n",
> event);
> logged = true;
> }
> @@ -43,6 +45,11 @@ static void handle_event(int event)
> qemu_system_guest_panicked(NULL);
> return;
> }
> +
> +if (event & PVPANIC_CRASHLOADED) {
> +qemu_system_guest_crashloaded(NULL);
> +return;
> +}
> }
Okay. Possible simplification:
static void handle_event(int event)
{
static bool logged;
if (event & PVPANIC_PANICKED) {
qemu_system_guest_panicked(NULL);
return;
}
if (event & PVPANIC_CRASHLOADED) {
qemu_system_guest_crashloaded(NULL);
return;
}
if (!logged) {
qemu_log_mask(LOG_GUEST_ERROR, "pvpanic: unknown event %#x.\n",
event);
logged = true;
}
}
>
> #include "hw/isa/isa.h"
> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
> index 0b41555609..f760094858 100644
> --- a/include/sysemu/runstate.h
> +++ b/include/sysemu/runstate.h
> @@ -63,6 +63,7 @@ ShutdownCause qemu_reset_requested_get(void);
> void qemu_system_killed(int signal, pid_t pid);
> void qemu_system_reset(ShutdownCause reason);
> void qemu_system_guest_panicked(GuestPanicInformation *info);
> +void qemu_system_guest_crashloaded(GuestPanicInformation *info);
>
> #endif
>
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index d7477cd715..b7a91f3125 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -357,6 +357,26 @@
##
# @GUEST_PANICKED:
#
# Emitted when guest OS panic is detected
#
# @action: action that has been taken, currently always "pause"
Not this patch's problem, but here goes anyway: 'currently always
"pause"' is wrong since 864111f422 "vl: exit qemu on guest panic if
-no-shutdown is not set". See [*] below.
#
# @info: information about a panic (since 2.9)
#
# Since: 1.5
#
# Example:
#
# <- { "event": "GUEST_PANICKED",
# "data": { "action": "pause" } }
#
##
{ 'event': 'GUEST_PANICKED',
>'data': { 'action': 'GuestPanicAction', '*info': 'GuestPanicInformation' }
> }
>
> ##
> +# @GUEST_CRASHLOADED:
> +#
> +# Emitted when guest OS crash loaded is detected
> +#
> +# @action: action that has been taken, currently always "run"
> +#
> +# @info: information about a panic (since 2.9)
> +#
> +# Since: 5.0
> +#
> +# Example:
> +#
> +# <- { "event": "GUEST_CRASHLOADED",
> +# "data": { "action": "run" } }
> +#
> +##
> +{ 'event': 'GUEST_CRASHLOADED',
> + 'data': { 'action': 'GuestPanicAction', '*info': 'GuestPanicInformation' }
> }
> +
> +##
> # @GuestPanicAction:
> #
> # An enumeration of the actions taken when guest OS panic is detected
> @@ -366,7 +386,7 @@
> # Since: 2.1 (poweroff since 2.8)
> ##
> { 'enum': 'GuestPanicAction',
> - 'data': [ 'pause', 'poweroff' ] }
> + 'data': [ 'pause', 'poweroff', 'run' ] }
We now have
event action
GUEST_PANICKED pause
GUEST_PANICKED poweroff
GUEST_CRASHLOADED run
Why have two events?
If there's a reason for two, why have their actions mixed up in a single
enum?
>
> ##
> # @GuestPanicInformationType:
> diff --git a/vl.c b/vl.c
> index 86474a55c9..5b1b2ef095 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1468,6 +1468,18 @@ void qemu_system_guest_panicked(GuestPanicInformation
> *info)
void qemu_system_guest_panicked(GuestPanicInformation *info)
{
qemu_log_mask(LOG_GUEST_
Re: [PATCH 1/2] pvpanic: introduce crashloaded for pvpanic
zhenwei pi writes: > Add bit 1 for pvpanic. This bit means that guest hits a panic, but > guest wants to handle error by itself. Typical case: Linux guest runs > kdump in panic. It will help us to separate the abnormal reboot from > normal operation. > > Signed-off-by: zhenwei pi > --- > docs/specs/pvpanic.txt | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt > index c7bbacc778..bdea68a430 100644 > --- a/docs/specs/pvpanic.txt > +++ b/docs/specs/pvpanic.txt > @@ -16,8 +16,12 @@ pvpanic exposes a single I/O port, by default 0x505. On > read, the bits > recognized by the device are set. Software should ignore bits it doesn't > recognize. On write, the bits not recognized by the device are ignored. > Software should set only bits both itself and the device recognize. Guest software, I presume. > -Currently, only bit 0 is recognized, setting it indicates a guest panic > -has happened. > + > +Bit Definition > +-- > +bit 0: setting it indicates a guest panic has happened. > +bit 1: named crashloaded. setting it indicates a guest panic and run > + kexec to handle error by guest itself. Suggest to scratch "named crashloaded." The whole file is rather terse. I figure that's okay as along as there's just "guest panicked", because "kernel panic" is obvious enough. The addition of "panicked, handling with kexec" makes it less obvious. The commit message provides a bit more guidance. Could that be worked into this file? > > ACPI Interface > --
