Re: [libvirt] [PATCH v4 00/20] Incremental Backup API additions
On Mon, Feb 18, 2019 at 15:50:52 -0600, Eric Blake wrote: > On 2/18/19 10:34 AM, Peter Krempa wrote: [...] > > In the ideal world of snapshots when deletion/revertion was implemented > > we'd never expose the virDomainBlockCommit and virDomainBlockPull APIs > > including the multi-use backdoor virDomainBlockRebase() which should > > have never existed and users would do equivalent operations using the > > snapshot APIs. > > > > virDomainBlockCopy is useful on it's own though but badly combines with > > snapshots. This will need some fixing. > > Indeed, and that's true regardless of whether the backup API goes in > (although the backup API probably compounds the issue on the number of > corner cases we have to think about; the conservative approach is that > at least in the beginning, you won't be able to run a BlockCopy and a > BackupBegin job at the same time). We currently do this kind of interlocking between blockjobs and snapshots as well, so that will not be a problem. We don't even allow two blockjobs on non-conflicting parts of the backing chain while qemu allows that now. Allowing that will require some API design considerations/compromises as our job tracking and manipulation APIs and events identify the job by the disk. Given that a guest can detach a disk frontend without qemu interaction we will somehow need to be able to display jobs which don't have a disk assigned any more though which will have the same implications basically. > > With the new checkpoint APIs the situation is even more "fun" as > > modification of the backing chain involves in some cases changes to the > > bitmaps. Ideally these would do "the right thing (TM)" during snapshot > > deletion/reversion. Given that at this time we don't support snapshot > > deletion/reversion for external snapshots we can use the excuse that > > snapshot management is not implemented so that checkpoints don't need > > to be modified. > > Somewhat correct - but we DO have to think about how we plan for the API > to grow in the future when we eventually DO fix snapshot > deletion/reversion. Hence my question - would we rather have the > creation of a checkpoint at the same time as the creation of an external > snapshot (which we DO know we will want) to occur via the existing API > (by extending the snapshot XML to include the checkpoint XML as a > sub-element), or via a new API (by passing the checkpoint XML as a > second parameter)? Once we've answered that question, it then > determines what signature we want for virDomainBackupBegin() (either two > separate XML parameters, one for the backup job and one for the > checkpoint creation, as presented in v4 of the series, OR as one single > XML call where the checkpoint XML is a sub-element of the backup XML). This is an interesting point. Given that the snapshot creates new files (speaking of external snapshots obviously) you get an implicit "checkpoint" at that moment as basically all blocks changed since that moment are recorded in the overlay file. For any other snapshot you get the same if you apply the overlay file on top of it. Now the question is whether we need to be able to track that checkpoint explicitly or whether it needs an explicit bitmap, but I'm not that familiar with qemu implementation. If we don't require any explicit bitmap or marking in qemu to be able to track a checkpoint from a snapshot point we can e.g. allow the backup API to take an external checkpoint as a reference for the backup itself. Creating both a snapshot and a checkpoint is certainly possible but I fear that making the snapshot more complex than it is will be an unexpected can of worms. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 5/6] block/dirty-bitmaps: unify qmp_locked and user_locked calls
On 2/18/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
> 14.02.2019 2:23, John Snow wrote:
>> These mean the same thing now. Unify them and rename the merged call
>> bdrv_dirty_bitmap_busy to indicate semantically what we are describing,
>> as well as help disambiguate from the various _locked and _unlocked
>> versions of bitmap helpers that refer to mutex locks.
>>
>> Signed-off-by: John Snow
>> Reviewed-by: Eric Blake
>> ---
>> block/dirty-bitmap.c | 41 +++---
>> blockdev.c | 18 +++
>> include/block/dirty-bitmap.h | 5 ++---
>> migration/block-dirty-bitmap.c | 6 ++---
>> nbd/server.c | 6 ++---
>> 5 files changed, 35 insertions(+), 41 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 2042c62602..8ab048385a 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -48,9 +48,9 @@ struct BdrvDirtyBitmap {
>> QemuMutex *mutex;
>> HBitmap *bitmap;/* Dirty bitmap implementation */
>> HBitmap *meta; /* Meta dirty bitmap */
>> -bool qmp_locked;/* Bitmap is locked, it can't be modified
>> - through QMP */
>> -BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked
>> state */
>> +bool busy; /* Bitmap is busy, it can't be modified
>> through
>> + QMP */
>
> better not "modified" but "used".. for example, export through NBD is not a
> modification.
>
True.
>> +BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
>
> hm this comment change about successor relates more to previous patch, but I
> don't really care.
>
Oh, true.
>> char *name; /* Optional non-empty unique ID */
>> int64_t size; /* Size of the bitmap, in bytes */
>> bool disabled; /* Bitmap is disabled. It ignores all
>> writes to
>> @@ -188,22 +188,17 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap
>> *bitmap)
>> return bitmap->successor;
>> }
>>
>
>
> In comment for bdrv_dirty_bitmap_create_successor, there is "locked" word,
> which you forget to fix to "busy"
> with at least this fixed:
Good spot. Too many occurrences of the word "lock" to have looked for
them all.
> Reviewed-by: Vladimir Sementsov-Ogievskiy
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 2/2] network: add netmask to dhcp range of dnsmasq conf file for IPv4
dnsmasq documentation says that the *IPv4* prefix/network
address/broadcast address sent to dhcp clients will be automatically
determined by dnsmasq by looking at the interface it's listening on,
so the original libvirt code did not add a netmask to the dnsmasq
commandline (or later, the dnsmasq conf file).
For *IPv6* however, dnsmasq apparently cannot automatically determine
the prefix (functionally the same as a netmask), and it must be
explicitly provided in the conf file (as a part of the dhcp-range
option). So many years after IPv4 DHCP support had been added, when
IPv6 dhcp support was added the prefix was included at the end of the
dhcp-range setting, but only for IPv6.
Recently a user reported (privately, because they suspected a possible
security implication, which turned out to be unfounded) a bug on a
host where one of the interfaces was a superset of the libvirt network
where dhcp is needed (e.g., the host's ethernet is 10.0.0.20/8, and
the libvirt network is 10.10.0.1/24). For some reason dnsmasq was
supplying the netmask for the /8 network to clients requesting an
address on the /24 interface.
This seems like a bug in dnsmasq, but even if/when it gets fixed
there, it looks like there is no harm in just always adding the
netmask to all IPv4 dhcp-range options similar to how prefix is added
to all IPv6 dhcp-range options.
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 27 +++
.../dhcp6-nat-network.conf| 2 +-
.../networkxml2confdata/isolated-network.conf | 2 +-
.../nat-network-dns-srv-record-minimal.conf | 2 +-
.../nat-network-dns-srv-record.conf | 2 +-
.../nat-network-dns-txt-record.conf | 2 +-
.../networkxml2confdata/nat-network-mtu.conf | 2 +-
.../nat-network-name-with-quotes.conf | 2 +-
tests/networkxml2confdata/nat-network.conf| 2 +-
.../networkxml2confdata/netboot-network.conf | 2 +-
.../netboot-proxy-network.conf| 2 +-
.../networkxml2confdata/ptr-domains-auto.conf | 2 +-
12 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 6d80818e40..9fa902896b 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1320,11 +1320,28 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
!(eaddr = virSocketAddrFormat(>ranges[r].end)))
goto cleanup;
-virBufferAsprintf(, "dhcp-range=%s,%s",
- saddr, eaddr);
-if (VIR_SOCKET_ADDR_IS_FAMILY(>address, AF_INET6))
-virBufferAsprintf(, ",%d", prefix);
-virBufferAddLit(, "\n");
+if (VIR_SOCKET_ADDR_IS_FAMILY(>address, AF_INET6)) {
+ virBufferAsprintf(, "dhcp-range=%s,%s,%d\n",
+ saddr, eaddr, prefix);
+} else {
+/* IPv4 - dnsmasq requires a netmask rather than prefix */
+virSocketAddr netmask;
+char *netmaskStr;
+
+if (virSocketAddrPrefixToNetmask(prefix, , AF_INET) <
0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to translate bridge '%s' "
+ "prefix %d to netmask"),
+ def->bridge, prefix);
+goto cleanup;
+}
+
+if (!(netmaskStr = virSocketAddrFormat()))
+goto cleanup;
+virBufferAsprintf(, "dhcp-range=%s,%s,%s\n",
+ saddr, eaddr, netmaskStr);
+VIR_FREE(netmaskStr);
+}
VIR_FREE(saddr);
VIR_FREE(eaddr);
diff --git a/tests/networkxml2confdata/dhcp6-nat-network.conf
b/tests/networkxml2confdata/dhcp6-nat-network.conf
index d1058df3b6..536974e508 100644
--- a/tests/networkxml2confdata/dhcp6-nat-network.conf
+++ b/tests/networkxml2confdata/dhcp6-nat-network.conf
@@ -8,7 +8,7 @@ strict-order
except-interface=lo
bind-dynamic
interface=virbr0
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64
diff --git a/tests/networkxml2confdata/isolated-network.conf
b/tests/networkxml2confdata/isolated-network.conf
index ce4a59f6c1..693a83d9a0 100644
--- a/tests/networkxml2confdata/isolated-network.conf
+++ b/tests/networkxml2confdata/isolated-network.conf
@@ -10,7 +10,7 @@ bind-interfaces
listen-address=192.168.152.1
dhcp-option=3
no-resolv
-dhcp-range=192.168.152.2,192.168.152.254
+dhcp-range=192.168.152.2,192.168.152.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
[libvirt] [PATCHv2 0/2] network: add netmask to dhcp range of dnsmasq conf file for IPv4
I sent V1 of this patch way back in October: https://www.redhat.com/archives/libvir-list/2018-October/msg00889.html but then self-NACKed it because I had written the patch to add the network prefix to dhcp-range, but IPv4 requires the netmask rather than range. Unfortunately, when I modified the patch accordingly the getnameinfo() function began failing, I couldn't immediately see why, and I had to pack everything up and go to KVM Forum. After that, I promptly forgot to look back at it. Today I finally revisited the problem (after the person who originally found it sent a reminder), and discovered the failure of getnameinfo() was due to a bug in a function that I added to the virSocketAddr library in 2010! Laine Stump (2): util: set missing data length in virSocketAddrPrefixToNetmask() network: add netmask to dhcp range of dnsmasq conf file for IPv4 src/network/bridge_driver.c | 27 +++ src/util/virsocketaddr.c | 2 ++ .../dhcp6-nat-network.conf| 2 +- .../networkxml2confdata/isolated-network.conf | 2 +- .../nat-network-dns-srv-record-minimal.conf | 2 +- .../nat-network-dns-srv-record.conf | 2 +- .../nat-network-dns-txt-record.conf | 2 +- .../networkxml2confdata/nat-network-mtu.conf | 2 +- .../nat-network-name-with-quotes.conf | 2 +- tests/networkxml2confdata/nat-network.conf| 2 +- .../networkxml2confdata/netboot-network.conf | 2 +- .../netboot-proxy-network.conf| 2 +- .../networkxml2confdata/ptr-domains-auto.conf | 2 +- 13 files changed, 35 insertions(+), 16 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 1/2] util: set missing data length in virSocketAddrPrefixToNetmask()
This fixes a bug that has been present since the original version of
the function was pushed in commit 1ab80f3 on Nov. 26 2010 (by me). The
virSocketAddr::len was not being set.
Apparently until now we were always calling
virSocketAddrPrefixToNetmask() with a virSocketAddr object that was
already (coincidentally) initialized for the proper address family,
but the bug became apparent when trying to use it to fill in an
otherwise uninitialized object.
Signed-off-by: Laine Stump
---
src/util/virsocketaddr.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index 4bc14bbd15..ccfaeabe13 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -1032,6 +1032,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix,
ip = prefix ? ~((1 << (32 - prefix)) - 1) : 0;
netmask->data.inet4.sin_addr.s_addr = htonl(ip);
netmask->data.stor.ss_family = AF_INET;
+netmask->len = sizeof(struct sockaddr_in);
result = 0;
} else if (family == AF_INET6) {
@@ -1055,6 +1056,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix,
netmask->data.inet6.sin6_addr.s6_addr[i++] = 0;
}
netmask->data.stor.ss_family = AF_INET6;
+netmask->len = sizeof(struct sockaddr_in6);
result = 0;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/6] block/dirty-bitmap: change semantics of enabled predicate
On 2/18/19 11:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.02.2019 2:23, John Snow wrote:
>> Currently, enabled means something like "the status of the bitmap
>> is ACTIVE." After this patch, it should mean exclusively: "This
>> bitmap is recording guest writes, and is allowed to do so."
>>
>> In many places, this is how this predicate was already used.
>> We'll allow users to call user_locked if they're really curious about
>> finding out if the bitmap is in use by an operation.
>>
>> To accommodate this, modify the create_successor routine to now
>> explicitly disable the parent bitmap at creation time.
>>
>>
>> Justifications:
>>
>> 1. bdrv_dirty_bitmap_status suffers no change from the lack of
>> 1:1 parity with the new predicates because of the order in which
>> the predicates are checked. This is now only for compatibility.
>>
>> 2. bdrv_set_dirty_bitmap is only used by mirror, which does not use
>> disabled bitmaps -- all of these writes are internal usages.
>> Therefore, we should allow writes even in the disabled state.
>> The condition is removed.
>>
>> 3. bdrv_reset_dirty_bitmap Similarly, this is only used internally by
>> mirror and migration. In these contexts it is always enabled anyway,
>> but our API does not need to enforce this.
>>
>> 4. bdrv_set_dirty() is unchanged: pre-patch, it was skipping bitmaps that
>> were
>> disabled or had a successor, while post-patch it is only skipping bitmaps
>> that are disabled. To accommodate this, create_successor now ensures that
>> any bitmap with a successor is explicitly disabled.
>>
>
> 5-8 are examples of "this is how this predicate was already used"
>
>> 5. qcow2_store_persistent_dirty_bitmaps: This only ever wanted to check if
>> the
>> bitmap was enabled or not. Theoretically if we save during an operation,
>> this now gets set as enabled instead of disabled,
>
> No, as you explicitly disable bitmap in create_successor, so bitmaps with
> successor
> will be disabled anyway.
>
Well, yeah. There's no way it happens in practice currently. It's just
"theoretically" from the viewpoint of the API call itself. There's
nothing stopping a developer from making that call, and this is a
potential change in behavior that we don't expect to observe. Just
noting it down.
> Hmm, and this shows, that actually, you don't need this big description for
> all calls,
> as actually nothing changed and all calls may be described like (4.). Except
> (2. and 3.),
> as these calls are removed (so, is it worth to split them into separate
> previous patch?)
>
I could, to at least have its own justification in a commit message
apart from these -- but at this point it's primarily a benefit for Eric,
You, and myself.
> but this cannot happen
>> in practice because jobs must be finished before we close the disk.
>>
>> 6. block_dirty_bitmap_enable_prepare only ever cared about the
>> literal bit, and already checked for user_locked beforehand.
>>
>> 7. block_dirty_bitmap_disable_prepare ditto as above.
>>
>> 8. init_dirty_bitmap_migration also already checks user_locked,
>> so this call can be a simple enabled/disabled check.
>
>
> hmmm
> 9. nbd_export_new, which too checks bdrv_dirty_bitmap_user_locked but _after_
> call to bdrv_dirty_bitmap_enabled. Anyway it's not changed as described
> in (4.),
> I think it is better to check _user_locked first.
>
You're right, and Eric left a similar feedback elsewhere. user_locked is
the more obvious disqualifier. I think this ought to be its own small
patch because it has nothing much to do with this one.
>
>>
>> Signed-off-by: John Snow
>> Reviewed-by: Eric Blake
>> ---
>> block/dirty-bitmap.c | 7 ---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 639ebc0645..bb2e19e0d8 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -209,7 +209,7 @@ bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap
>> *bitmap)
>> /* Called with BQL taken. */
>> bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
>> {
>> -return !(bitmap->disabled || bitmap->successor);
>> +return !bitmap->disabled;
>> }
>>
>> /* Called with BQL taken. */
>> @@ -264,6 +264,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState
>> *bs,
>>
>> /* Successor will be on or off based on our current state. */
>> child->disabled = bitmap->disabled;
>> +bitmap->disabled = true;
>>
>> /* Install the successor and freeze the parent */
>> bitmap->successor = child;
>> @@ -346,6 +347,8 @@ BdrvDirtyBitmap
>> *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>> error_setg(errp, "Merging of parent and successor bitmap failed");
>> return NULL;
>> }
>> +
>> +parent->disabled = successor->disabled;
>
> at this point comment to the function
> "The merged parent will not be user_locked, nor
Re: [libvirt] [jenkins-ci PATCH 0/5] Tidy up top-level directory and fix a few small issues
On 2/8/19 11:51 AM, Andrea Bolognani wrote:
> Andrea Bolognani (5):
> jenkins: Move all Jenkins-related files
> README: Add short overview of the repository
> guests: Fix a small issue in README
> jenkins: Fix a couple small issues in README
> jenkins: Add YAML start of document marker
>
> README.markdown | 56 ---
> guests/README.markdown| 2 +-
> README.markdown => jenkins/README.markdown| 18 +++---
> {jobs => jenkins/jobs}/autotools.yaml | 2 +-
> {jobs => jenkins/jobs}/defaults.yaml | 2 +-
> {jobs => jenkins/jobs}/generic.yaml | 2 +-
> {jobs => jenkins/jobs}/go.yaml| 2 +-
> {jobs => jenkins/jobs}/perl-modulebuild.yaml | 2 +-
> {jobs => jenkins/jobs}/python-distutils.yaml | 2 +-
> .../projects}/libosinfo+mingw32.yaml | 2 +-
> .../projects}/libosinfo+mingw64.yaml | 2 +-
> {projects => jenkins/projects}/libosinfo.yaml | 2 +-
> .../projects}/libvirt+mingw32.yaml| 2 +-
> .../projects}/libvirt+mingw64.yaml| 2 +-
> .../projects}/libvirt-cim.yaml| 2 +-
> .../projects}/libvirt-dbus.yaml | 2 +-
> .../projects}/libvirt-glib+mingw32.yaml | 2 +-
> .../projects}/libvirt-glib+mingw64.yaml | 2 +-
> .../projects}/libvirt-glib.yaml | 2 +-
> .../projects}/libvirt-go-xml.yaml | 2 +-
> .../projects}/libvirt-go.yaml | 2 +-
> .../projects}/libvirt-ocaml.yaml | 2 +-
> .../projects}/libvirt-perl.yaml | 2 +-
> .../projects}/libvirt-python.yaml | 2 +-
> .../projects}/libvirt-sandbox.yaml| 2 +-
> .../projects}/libvirt-tck.yaml| 2 +-
> {projects => jenkins/projects}/libvirt.yaml | 2 +-
> .../projects}/osinfo-db-tools+mingw32.yaml| 2 +-
> .../projects}/osinfo-db-tools+mingw64.yaml| 2 +-
> .../projects}/osinfo-db-tools.yaml| 2 +-
> {projects => jenkins/projects}/osinfo-db.yaml | 2 +-
> .../projects}/virt-manager.yaml | 2 +-
> .../projects}/virt-viewer+mingw32.yaml| 2 +-
> .../projects}/virt-viewer+mingw64.yaml| 2 +-
> .../projects}/virt-viewer.yaml| 2 +-
> 35 files changed, 51 insertions(+), 89 deletions(-)
> copy README.markdown => jenkins/README.markdown (65%)
> rename {jobs => jenkins/jobs}/autotools.yaml (99%)
> rename {jobs => jenkins/jobs}/defaults.yaml (99%)
> rename {jobs => jenkins/jobs}/generic.yaml (99%)
> rename {jobs => jenkins/jobs}/go.yaml (99%)
> rename {jobs => jenkins/jobs}/perl-modulebuild.yaml (99%)
> rename {jobs => jenkins/jobs}/python-distutils.yaml (99%)
> rename {projects => jenkins/projects}/libosinfo+mingw32.yaml (98%)
> rename {projects => jenkins/projects}/libosinfo+mingw64.yaml (98%)
> rename {projects => jenkins/projects}/libosinfo.yaml (99%)
> rename {projects => jenkins/projects}/libvirt+mingw32.yaml (98%)
> rename {projects => jenkins/projects}/libvirt+mingw64.yaml (98%)
> rename {projects => jenkins/projects}/libvirt-cim.yaml (98%)
> rename {projects => jenkins/projects}/libvirt-dbus.yaml (99%)
> rename {projects => jenkins/projects}/libvirt-glib+mingw32.yaml (98%)
> rename {projects => jenkins/projects}/libvirt-glib+mingw64.yaml (98%)
> rename {projects => jenkins/projects}/libvirt-glib.yaml (99%)
> rename {projects => jenkins/projects}/libvirt-go-xml.yaml (99%)
> rename {projects => jenkins/projects}/libvirt-go.yaml (99%)
> rename {projects => jenkins/projects}/libvirt-ocaml.yaml (99%)
> rename {projects => jenkins/projects}/libvirt-perl.yaml (99%)
> rename {projects => jenkins/projects}/libvirt-python.yaml (99%)
> rename {projects => jenkins/projects}/libvirt-sandbox.yaml (99%)
> rename {projects => jenkins/projects}/libvirt-tck.yaml (99%)
> rename {projects => jenkins/projects}/libvirt.yaml (99%)
> rename {projects => jenkins/projects}/osinfo-db-tools+mingw32.yaml (98%)
> rename {projects => jenkins/projects}/osinfo-db-tools+mingw64.yaml (98%)
> rename {projects => jenkins/projects}/osinfo-db-tools.yaml (99%)
> rename {projects => jenkins/projects}/osinfo-db.yaml (99%)
> rename {projects => jenkins/projects}/virt-manager.yaml (99%)
> rename {projects => jenkins/projects}/virt-viewer+mingw32.yaml (98%)
> rename {projects => jenkins/projects}/virt-viewer+mingw64.yaml (98%)
> rename {projects => jenkins/projects}/virt-viewer.yaml (99%)
>
Reviewed-by: Yash Mankad
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/6] block/dirty-bitmaps: rename frozen predicate helper
On 2/18/19 8:57 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.02.2019 2:23, John Snow wrote:
>> "Frozen" was a good description a long time ago, but it isn't adequate now.
>> Rename the frozen predicate to has_successor to make the semantics of the
>> predicate more clear to outside callers.
>>
>> In the process, remove some calls to frozen() that no longer semantically
>> make sense. For enabled and disabled in particular, it's actually okay for
>> the internals to do this but only forbidden for users to invoke them, and
>
> I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom?
> I think, it would be simpler for me to read patch itself :)
>
Touched this up. I meant enable and disable, not enabled and disabled.
>> all of the QMP entry uses already check against qmp_locked.
>>
>> Several other assertions really want to check that the bitmap isn't in-use
>> by another operation -- use the qmp_locked function for this instead, which
>> presently also checks for has_successor.
>
> hm, you mean user_locked, not qmp_locked.
>
Yes.
[...]
>> /**
>>* Create a successor bitmap destined to replace this bitmap after an
>> operation.
>> - * Requires that the bitmap is not frozen and has no successor.
>> + * Requires that the bitmap is not locked and has no successor.
>
> I think, user_locked, to not interfere with bitmaps mutex. And you use
> user_locked in
> other comments in this patch.
>
You're right. It gets changed again later, but I didn't make this easy
to read.
>>* Called with BQL taken.
>>*/
>> int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>> @@ -244,12 +244,16 @@ int
>> bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>> uint64_t granularity;
>> BdrvDirtyBitmap *child;
>>
>> -if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> -error_setg(errp, "Cannot create a successor for a bitmap that is "
>> - "currently frozen");
>> +if (bdrv_dirty_bitmap_user_locked(bitmap)) {
>> +error_setg(errp, "Cannot create a successor for a bitmap that is
>> in-use "
>> + "by an operation");
>> +return -1;
>> +}
>> +if (bdrv_dirty_bitmap_has_successor(bitmap)) {
>> +error_setg(errp, "Cannot create a successor for a bitmap that
>> already "
>> + "has one");
>
>
> Amm, dead code? _user_locked() implies no successor, so we instead can keep
> an assertion..
>
It gets changed later in the series, but I didn't do a great job of
explaining that in advance. I'll amend the commit message to explain
what I'm trying to do.
I tried to hint at this with: "which presently also checks for
has_successor" as an admission that it was redundant, but I need to call
it out in stronger language.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/6] block/dirty-bitmaps: move comment block
On 2/18/19 12:39 PM, Vladimir Sementsov-Ogievskiy wrote: > 14.02.2019 2:23, John Snow wrote: >> Simply move the big status enum comment block to above the status >> function, and document it as being deprecated. The whole confusing >> block can get deleted in three releases time. >> >> Signed-off-by: John Snow > > > Reviewed-by: Vladimir Sementsov-Ogievskiy > > But I think, it's OK to remove it now. It mostly unrelated to code now, so, > is it needed? And it is unrelated to deprecation. As I understand, user-seen > information is DirtyBitmapStatus declaration in qapi, and it is deprecated > and to be removed in three releases. > I decided to keep it because I wanted to document the particulars of what the fields meant internally before we finally remove it some future release, in case we need to change this function around to maintain backwards compatibility. It'll get removed at that point in time. --js -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/6] lxc: Create a method to initialize network data outisde.
This method has the same idea of the method to parse IPv{4,6} data.
The method lxcNetworkParseDataInit() is responsible to initialize
network settings outside handler.
Signed-off-by: Julio Faracco
---
src/lxc/lxc_native.c | 56 +---
1 file changed, 32 insertions(+), 24 deletions(-)
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index 95e08c18f4..25e35e93dd 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -552,6 +552,37 @@ lxcAddNetworkDefinition(lxcNetworkParseData *data)
return -1;
}
+static int
+lxcNetworkParseDataInit(virConfValuePtr value, lxcNetworkParseData *parseData)
+{
+virDomainDefPtr def = parseData->def;
+size_t networks = parseData->networks;
+bool privnet = parseData->privnet;
+int status;
+
+/* Store the previous NIC */
+status = lxcAddNetworkDefinition(parseData);
+
+if (status < 0)
+return -1;
+else if (status > 0)
+networks++;
+else if (parseData->type != NULL && STREQ(parseData->type, "none"))
+privnet = false;
+
+/* clean NIC to store a new one */
+memset(parseData, 0, sizeof(*parseData));
+
+parseData->def = def;
+parseData->networks = networks;
+parseData->privnet = privnet;
+
+/* Keep the new value */
+parseData->type = value->str;
+
+return 0;
+}
+
static int
lxcNetworkParseDataIPs(const char *name, virConfValuePtr value,
lxcNetworkParseData *parseData)
{
@@ -591,32 +622,9 @@ lxcNetworkParseDataIPs(const char *name, virConfValuePtr
value, lxcNetworkParseD
static int
lxcNetworkParseDataSuffix(const char *name, virConfValuePtr value,
lxcNetworkParseData *parseData)
{
-int status;
-
if (STREQ(name, "type")) {
-virDomainDefPtr def = parseData->def;
-size_t networks = parseData->networks;
-bool privnet = parseData->privnet;
-
-/* Store the previous NIC */
-status = lxcAddNetworkDefinition(parseData);
-
-if (status < 0)
+if (lxcNetworkParseDataInit(value, parseData) < 0)
return -1;
-else if (status > 0)
-networks++;
-else if (parseData->type != NULL && STREQ(parseData->type, "none"))
-privnet = false;
-
-/* clean NIC to store a new one */
-memset(parseData, 0, sizeof(*parseData));
-
-parseData->def = def;
-parseData->networks = networks;
-parseData->privnet = privnet;
-
-/* Keep the new value */
-parseData->type = value->str;
}
else if (STREQ(name, "link"))
parseData->link = value->str;
--
2.19.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/6] lxc: Create a separate method to handle IPv{4, 6} outside parser.
The new method called lxcNetworkParseDataIPs() is responsible to handle
IPv{4,6} settings now. The idea is let lxcNetworkWalkCallback() method
handle all entries related to network definition only.
Signed-off-by: Julio Faracco
---
src/lxc/lxc_native.c | 65 +---
1 file changed, 37 insertions(+), 28 deletions(-)
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index 1eee3fc2bb..5f132c 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -552,6 +552,42 @@ lxcAddNetworkDefinition(lxcNetworkParseData *data)
return -1;
}
+static int
+lxcNetworkParseDataIPs(const char *name, virConfValuePtr value,
lxcNetworkParseData *parseData)
+{
+int family = AF_INET;
+char **ipparts = NULL;
+virNetDevIPAddrPtr ip = NULL;
+
+if (VIR_ALLOC(ip) < 0)
+return -1;
+
+if (STREQ(name, "lxc.network.ipv6"))
+family = AF_INET6;
+
+ipparts = virStringSplit(value->str, "/", 2);
+if (virStringListLength((const char * const *)ipparts) != 2 ||
+virSocketAddrParse(>address, ipparts[0], family) < 0 ||
+virStrToLong_ui(ipparts[1], NULL, 10, >prefix) < 0) {
+
+virReportError(VIR_ERR_INVALID_ARG,
+ _("Invalid CIDR address: '%s'"), value->str);
+
+virStringListFree(ipparts);
+VIR_FREE(ip);
+return -1;
+}
+
+virStringListFree(ipparts);
+
+if (VIR_APPEND_ELEMENT(parseData->ips, parseData->nips, ip) < 0) {
+VIR_FREE(ip);
+return -1;
+}
+
+return 0;
+}
+
static int
lxcNetworkWalkCallback(const char *name, virConfValuePtr value, void *data)
{
@@ -597,35 +633,8 @@ lxcNetworkWalkCallback(const char *name, virConfValuePtr
value, void *data)
parseData->name = value->str;
else if (STREQ(name, "lxc.network.ipv4") ||
STREQ(name, "lxc.network.ipv6")) {
-int family = AF_INET;
-char **ipparts = NULL;
-virNetDevIPAddrPtr ip = NULL;
-
-if (VIR_ALLOC(ip) < 0)
+if (lxcNetworkParseDataIPs(name, value, parseData) < 0)
return -1;
-
-if (STREQ(name, "lxc.network.ipv6"))
-family = AF_INET6;
-
-ipparts = virStringSplit(value->str, "/", 2);
-if (virStringListLength((const char * const *)ipparts) != 2 ||
-virSocketAddrParse(>address, ipparts[0], family) < 0 ||
-virStrToLong_ui(ipparts[1], NULL, 10, >prefix) < 0) {
-
-virReportError(VIR_ERR_INVALID_ARG,
- _("Invalid CIDR address: '%s'"), value->str);
-
-virStringListFree(ipparts);
-VIR_FREE(ip);
-return -1;
-}
-
-virStringListFree(ipparts);
-
-if (VIR_APPEND_ELEMENT(parseData->ips, parseData->nips, ip) < 0) {
-VIR_FREE(ip);
-return -1;
-}
} else if (STREQ(name, "lxc.network.ipv4.gateway")) {
parseData->gateway_ipv4 = value->str;
} else if (STREQ(name, "lxc.network.ipv6.gateway")) {
--
2.19.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/6] lxc: Clean up lxcNetworkWalkCallback() method.
This series has a lots of cleanups to prepare lxc to handle network
settings for verion 3.0 or higher. The new version is considering
network indexes to define network interfaces. i.e.: "lxc.net.X.".
As we need modular structures to support both versions, this method
needs a cleanup before accepting new features for version 3.
Obs: this version does not include any new feature for LXC 3.0 network.
Julio Faracco (6):
lxc: Create a separate method to handle IPv{4,6} outside parser.
lxc: Create a new method called lxcNetworkParseDataEntry().
lxc: Converting full string entries in types only.
lxc: Refactoring lxcNetworkWalkCallback() to a simple method.
lxc: Create a method to initialize network data outisde.
lxc: Converting 'if,else' logic into a 'switch,case'.
src/lxc/lxc_native.c | 184 +--
src/lxc/lxc_native.h | 17
2 files changed, 141 insertions(+), 60 deletions(-)
--
2.19.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/6] lxc: Converting full string entries in types only.
This commit removes the full network entry setting: "lxc.network.X" to
type only. Like "type", "name", "flags", etc. So, here no matter if the
settings is "lxc.network.X" or "lxc.net.X.Y".
Signed-off-by: Julio Faracco
---
src/lxc/lxc_native.c | 30 +++---
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index c144f3c52e..ed50415a93 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -562,7 +562,7 @@ lxcNetworkParseDataIPs(const char *name, virConfValuePtr
value, lxcNetworkParseD
if (VIR_ALLOC(ip) < 0)
return -1;
-if (STREQ(name, "lxc.network.ipv6"))
+if (STREQ(name, "ipv6"))
family = AF_INET6;
ipparts = virStringSplit(value->str, "/", 2);
@@ -589,12 +589,11 @@ lxcNetworkParseDataIPs(const char *name, virConfValuePtr
value, lxcNetworkParseD
}
static int
-lxcNetworkWalkCallback(const char *name, virConfValuePtr value, void *data)
+lxcNetworkParseDataSuffix(const char *name, virConfValuePtr value,
lxcNetworkParseData *parseData)
{
-lxcNetworkParseData *parseData = data;
int status;
-if (STREQ(name, "lxc.network.type")) {
+if (STREQ(name, "type")) {
virDomainDefPtr def = parseData->def;
size_t networks = parseData->networks;
bool privnet = parseData->privnet;
@@ -619,30 +618,31 @@ lxcNetworkWalkCallback(const char *name, virConfValuePtr
value, void *data)
/* Keep the new value */
parseData->type = value->str;
}
-else if (STREQ(name, "lxc.network.link"))
+else if (STREQ(name, "link"))
parseData->link = value->str;
-else if (STREQ(name, "lxc.network.hwaddr"))
+else if (STREQ(name, "hwaddr"))
parseData->mac = value->str;
-else if (STREQ(name, "lxc.network.flags"))
+else if (STREQ(name, "flags"))
parseData->flag = value->str;
-else if (STREQ(name, "lxc.network.macvlan.mode"))
+else if (STREQ(name, "macvlan.mode"))
parseData->macvlanmode = value->str;
-else if (STREQ(name, "lxc.network.vlan.id"))
+else if (STREQ(name, "vlan.id"))
parseData->vlanid = value->str;
-else if (STREQ(name, "lxc.network.name"))
+else if (STREQ(name, "name"))
parseData->name = value->str;
-else if (STREQ(name, "lxc.network.ipv4") ||
- STREQ(name, "lxc.network.ipv6")) {
+else if (STREQ(name, "ipv4") ||
+ STREQ(name, "ipv6")) {
if (lxcNetworkParseDataIPs(name, value, parseData) < 0)
return -1;
-} else if (STREQ(name, "lxc.network.ipv4.gateway")) {
+} else if (STREQ(name, "ipv4.gateway")) {
parseData->gateway_ipv4 = value->str;
-} else if (STREQ(name, "lxc.network.ipv6.gateway")) {
+} else if (STREQ(name, "ipv6.gateway")) {
parseData->gateway_ipv6 = value->str;
-} else if (STRPREFIX(name, "lxc.network")) {
+} else {
VIR_WARN("Unhandled network property: %s = %s",
name,
value->str);
+return -1;
}
return 0;
--
2.19.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/6] lxc: Create a new method called lxcNetworkParseDataEntry().
This new method is responsible to verify is the settings correspond to
network entry. Right now, it is only verifying "lxc.network.", but in
the future, it can be used to verify "lxc.net.X." too. Any other case
would be rejected.
On the other hand, the idea here is working only with types. If we know
that entry is part of network settings, after we just need to know which
type is. It keeps the hanlder simple.
Signed-off-by: Julio Faracco
---
src/lxc/lxc_native.c | 8
1 file changed, 8 insertions(+)
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index 5f132c..c144f3c52e 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -648,6 +648,14 @@ lxcNetworkWalkCallback(const char *name, virConfValuePtr
value, void *data)
return 0;
}
+static int
+lxcNetworkParseDataEntry(const char *name, virConfValuePtr value,
lxcNetworkParseData *parseData)
+{
+const char *suffix = STRSKIP(name, "lxc.network.");
+
+return lxcNetworkParseDataSuffix(suffix, value, parseData);
+}
+
static int
lxcConvertNetworkSettings(virDomainDefPtr def, virConfPtr properties)
{
--
2.19.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 6/6] block/dirty-bitmaps: move comment block
14.02.2019 2:23, John Snow wrote:
> Simply move the big status enum comment block to above the status
> function, and document it as being deprecated. The whole confusing
> block can get deleted in three releases time.
>
> Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
But I think, it's OK to remove it now. It mostly unrelated to code now, so,
is it needed? And it is unrelated to deprecation. As I understand, user-seen
information is DirtyBitmapStatus declaration in qapi, and it is deprecated
and to be removed in three releases.
> ---
> block/dirty-bitmap.c | 36 +++-
> 1 file changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 8ab048385a..fc390cae94 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -28,22 +28,6 @@
> #include "block/block_int.h"
> #include "block/blockjob.h"
>
> -/**
> - * A BdrvDirtyBitmap can be in four possible user-visible states:
> - * (1) Active: successor is NULL, and disabled is false: full r/w mode
> - * (2) Disabled: successor is NULL, and disabled is true: qualified r/w mode,
> - * guest writes are dropped, but monitor writes are possible,
> - * through commands like merge and clear.
> - * (3) Frozen: successor is not NULL.
> - * A frozen bitmap cannot be renamed, deleted, cleared, set,
> - * enabled, merged to, etc. A frozen bitmap can only abdicate()
> - * or reclaim().
> - * In this state, the anonymous successor bitmap may be either
> - * Active and recording writes from the guest (e.g. backup
> jobs),
> - * but it can be Disabled and not recording writes.
> - * (4) Locked: Whether Active or Disabled, the user cannot modify this
> bitmap
> - * in any way from the monitor.
> - */
> struct BdrvDirtyBitmap {
> QemuMutex *mutex;
> HBitmap *bitmap;/* Dirty bitmap implementation */
> @@ -205,7 +189,25 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
> return !bitmap->disabled;
> }
>
> -/* Called with BQL taken. */
> +/**
> + * bdrv_dirty_bitmap_status: This API is now deprecated.
> + * Called with BQL taken.
> + *
> + * A BdrvDirtyBitmap can be in four possible user-visible states:
> + * (1) Active: successor is NULL, and disabled is false: full r/w mode
> + * (2) Disabled: successor is NULL, and disabled is true: qualified r/w mode,
> + * guest writes are dropped, but monitor writes are possible,
> + * through commands like merge and clear.
> + * (3) Frozen: successor is not NULL.
> + * A frozen bitmap cannot be renamed, deleted, cleared, set,
> + * enabled, merged to, etc. A frozen bitmap can only abdicate()
> + * or reclaim().
> + * In this state, the anonymous successor bitmap may be either
> + * Active and recording writes from the guest (e.g. backup
> jobs),
> + * but it can be Disabled and not recording writes.
> + * (4) Locked: Whether Active or Disabled, the user cannot modify this
> bitmap
> + * in any way from the monitor.
> + */
> DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
> {
> if (bdrv_dirty_bitmap_has_successor(bitmap)) {
>
--
Best regards,
Vladimir
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 5/6] block/dirty-bitmaps: unify qmp_locked and user_locked calls
14.02.2019 2:23, John Snow wrote:
> These mean the same thing now. Unify them and rename the merged call
> bdrv_dirty_bitmap_busy to indicate semantically what we are describing,
> as well as help disambiguate from the various _locked and _unlocked
> versions of bitmap helpers that refer to mutex locks.
>
> Signed-off-by: John Snow
> Reviewed-by: Eric Blake
> ---
> block/dirty-bitmap.c | 41 +++---
> blockdev.c | 18 +++
> include/block/dirty-bitmap.h | 5 ++---
> migration/block-dirty-bitmap.c | 6 ++---
> nbd/server.c | 6 ++---
> 5 files changed, 35 insertions(+), 41 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 2042c62602..8ab048385a 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -48,9 +48,9 @@ struct BdrvDirtyBitmap {
> QemuMutex *mutex;
> HBitmap *bitmap;/* Dirty bitmap implementation */
> HBitmap *meta; /* Meta dirty bitmap */
> -bool qmp_locked;/* Bitmap is locked, it can't be modified
> - through QMP */
> -BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked
> state */
> +bool busy; /* Bitmap is busy, it can't be modified
> through
> + QMP */
better not "modified" but "used".. for example, export through NBD is not a
modification.
> +BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
hm this comment change about successor relates more to previous patch, but I
don't really care.
> char *name; /* Optional non-empty unique ID */
> int64_t size; /* Size of the bitmap, in bytes */
> bool disabled; /* Bitmap is disabled. It ignores all
> writes to
> @@ -188,22 +188,17 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap
> *bitmap)
> return bitmap->successor;
> }
>
In comment for bdrv_dirty_bitmap_create_successor, there is "locked" word,
which you forget to fix to "busy"
with at least this fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy
--
Best regards,
Vladimir
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] security: apparmor: make vhost-net access a static rule
So far we were detecting at guest start if any devices needed vhost net
and only if that was true added a rule for /dev/vhost-net.
It turns out that it is an absolutely valid case to start a guest
without any vhost-net networking but later on wanting to hotplug such a
device which then would be denied by apparmor.
Unfortunately there also is no security labeling callback involved other
than the one to /dev/net/tun. But on the other hand vhost-net is no more
new and considered rather safe. Therefore drop the old detection and
just add it as a static rule.
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1815910
Signed-off-by: Christian Ehrhardt
---
src/security/apparmor/libvirt-qemu | 1 +
src/security/virt-aa-helper.c | 17 +
2 files changed, 2 insertions(+), 16 deletions(-)
diff --git a/src/security/apparmor/libvirt-qemu
b/src/security/apparmor/libvirt-qemu
index eaa5167525..a71f34c175 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -21,6 +21,7 @@
signal (receive) peer=/usr/sbin/libvirtd,
/dev/net/tun rw,
+ /dev/vhost-net rw,
/dev/kvm rw,
/dev/ptmx rw,
/dev/kqemu rw,
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 8e22e9978a..ebc4feac77 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -937,7 +937,7 @@ get_files(vahControl * ctl)
size_t i;
char *uuid;
char uuidstr[VIR_UUID_STRING_BUFLEN];
-bool needsVfio = false, needsvhost = false;
+bool needsVfio = false;
/* verify uuid is same as what we were given on the command line */
virUUIDFormat(ctl->def->uuid, uuidstr);
@@ -1248,21 +1248,6 @@ get_files(vahControl * ctl)
}
}
-if (ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) {
-for (i = 0; i < ctl->def->nnets; i++) {
-virDomainNetDefPtr net = ctl->def->nets[i];
-if (net && net->model) {
-if (net->driver.virtio.name ==
VIR_DOMAIN_NET_BACKEND_TYPE_QEMU)
-continue;
-if (!virDomainNetIsVirtioModel(net))
-continue;
-}
-needsvhost = true;
-}
-}
-if (needsvhost)
-virBufferAddLit(, " \"/dev/vhost-net\" rw,\n");
-
if (needsVfio) {
virBufferAddLit(, " \"/dev/vfio/vfio\" rw,\n");
virBufferAddLit(, " \"/dev/vfio/[0-9]*\" rw,\n");
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/2] security: aa-helper: generate more rules for gl devices
Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled
graphics devices" implemented the detection for gl enabled
devices in virt-aa-helper. But further testing showed
that it will need much more access for the full gl stack
to work.
Upstream apparmor just recently split those things out and now
has two related abstractions at
https://gitlab.com/apparmor/apparmor/blob/master:
- dri-common at /profiles/apparmor.d/abstractions/dri-common
- mesa: at /profiles/apparmor.d/abstractions/mesa
If would be great to just include that for the majority of
rules, but they are not yet in any distribution so we need
to add rules inspired by them based on the testing that we
can do.
Furthermore qemu with opengl will also probe the backing device
of the rendernode for attributes which should be safe as
read-only wildcard rules.
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1815452
Signed-off-by: Christian Ehrhardt
---
src/security/virt-aa-helper.c | 21 -
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 8e22e9978a..784ee49e61 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -937,7 +937,7 @@ get_files(vahControl * ctl)
size_t i;
char *uuid;
char uuidstr[VIR_UUID_STRING_BUFLEN];
-bool needsVfio = false, needsvhost = false;
+bool needsVfio = false, needsvhost = false, needsgl = false;
/* verify uuid is same as what we were given on the command line */
virUUIDFormat(ctl->def->uuid, uuidstr);
@@ -1065,9 +1065,11 @@ get_files(vahControl * ctl)
if (rendernode) {
vah_add_file(, rendernode, "rw");
+needsgl = true;
} else {
if (virDomainGraphicsNeedsAutoRenderNode(graphics)) {
char *defaultRenderNode = virHostGetDRMRenderNode();
+needsgl = true;
if (defaultRenderNode) {
vah_add_file(, defaultRenderNode, "rw");
@@ -1267,6 +1269,23 @@ get_files(vahControl * ctl)
virBufferAddLit(, " \"/dev/vfio/vfio\" rw,\n");
virBufferAddLit(, " \"/dev/vfio/[0-9]*\" rw,\n");
}
+if (needsgl) {
+/* if using gl all sorts of further dri related paths will be needed */
+virBufferAddLit(, " # DRI/Mesa/(e)GL config and driver paths\n");
+virBufferAddLit(, " \"/usr/lib{,32,64}/dri/**.so\" mr,\n");
+virBufferAddLit(, " \"/usr/lib/@{multiarch}/dri/**.so\" mr,\n");
+virBufferAddLit(, " \"/usr/lib/fglrx/dri/**.so\" mr,\n");
+virBufferAddLit(, " \"/etc/drirc\" r,\n");
+virBufferAddLit(, " \"/usr/share/drirc.d/{,*.conf}\" r,\n");
+virBufferAddLit(, " \"/etc/glvnd/egl_vendor.d/{,*}\" r,\n");
+virBufferAddLit(, " \"/usr/share/glvnd/egl_vendor.d/{,*}\" r,\n");
+virBufferAddLit(, " # Probe DRI device attributes\n");
+virBufferAddLit(, " \"/dev/dri/\" r,\n");
+virBufferAddLit(, "
\"/sys/devices/*/*/{uevent,vendor,device,subsystem_vendor,subsystem_device}\"
r,\n");
+virBufferAddLit(, "
\"/sys/devices/*/*/drm/*/{uevent,vendor,device,subsystem_vendor,subsystem_device}\"
r,\n");
+virBufferAddLit(, " # dri libs will trigger that, but t is not
requited and DAC would deny it anyway\n");
+virBufferAddLit(, " deny \"/var/lib/libvirt/.cache/\" w,\n");
+}
if (ctl->newfile)
if (vah_add_file(, ctl->newfile, "rwk") != 0)
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/2] security: aa-helper: allow virt-aa-helper to read /dev/dri
Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled
graphics devices" implemented the detection for gl enabled
devices in virt-aa-helper. But it will in certain cases e.g. if
no rendernode was explicitly specified need to read /dev/dri
which it currently isn't allowed.
Add a rule to the apparmor profile of virt-aa-helper itself to
be able to do that.
Acked-by: Jamie Strandboge
Signed-off-by: Christian Ehrhardt
---
src/security/apparmor/usr.lib.libvirt.virt-aa-helper | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
index de9436872c..78994bcda6 100644
--- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
+++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper
@@ -19,6 +19,9 @@ profile virt-aa-helper
/usr/{lib,lib64}/libvirt/virt-aa-helper {
/etc/libnl-3/classid r,
+ # for gl enabled graphics
+ /dev/dri/{,*} r,
+
# for hostdev
/sys/devices/ r,
/sys/devices/** r,
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/2] further apparmor handling of opengl
Further testing with opengl enabled graphics showed that we need much more rules than we initially added. Upstream apparmor has abstractions [1][2] for the majority of what we'd need, but those are in no Distribution yet so we can't rely on them. But we can add rules "like those known ones" matching what our testing shows as needed when we add a gl enabled device. There is no overly critical access opened by this, but still we continue to only add those to the guests that have gl enabled. The most "discussion worthy" part of it most likely are the wildcards into /dev/devices/... but they are rather specific and read only - furthermore retracing those in advance starting from the rendernode most likely is rather error prone, so I went with the wildcards. Example apparmor denials can be found in the launchpad bug [3] Updates in V2: - add jamies ack to patch #1 - limit the mapping rules to .so files - change the .changes rule to a deny as DAC would block it anyway [1]: https://gitlab.com/apparmor/apparmor/blob/master/profiles/apparmor.d/abstractions/mesa [2]: https://gitlab.com/apparmor/apparmor/blob/master/profiles/apparmor.d/abstractions/dri-common [3]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1815452 Christian Ehrhardt (2): security: aa-helper: allow virt-aa-helper to read /dev/dri security: aa-helper: generate more rules for gl devices .../apparmor/usr.lib.libvirt.virt-aa-helper | 3 +++ src/security/virt-aa-helper.c | 21 ++- 2 files changed, 23 insertions(+), 1 deletion(-) -- 2.17.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/6] block/dirty-bitmap: explicitly lock bitmaps with successors
14.02.2019 2:23, John Snow wrote:
> Instead of implying a locked status, make it explicit.
locked interferes with bitmap mutex, so may be better "qmp_locked state", but
not sure.
> Now, bitmaps in use by migration, NBD or backup operations
> are all treated the same way with the same code paths.
>
> Signed-off-by: John Snow
> Reviewed-by: Eric Blake
Reviewed-by: Vladimir Sementsov-Ogievskiy
Hmm. Isn't it better to make successor-related staff unrelated to locking at
all?
So, backup will call set_qmp_locked like others? And then do create_successor,
abdicate, reclaim, whatever it wants, and finally set_qmp_locked(false) ?
To make it work even more in the same path. But it may be done separately, if we
want.
> ---
> block/dirty-bitmap.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index bb2e19e0d8..2042c62602 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -188,10 +188,8 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap
> *bitmap)
> return bitmap->successor;
> }
>
> -/* Both conditions disallow user-modification via QMP. */
> bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
> -return bdrv_dirty_bitmap_has_successor(bitmap) ||
> - bdrv_dirty_bitmap_qmp_locked(bitmap);
> +return bdrv_dirty_bitmap_qmp_locked(bitmap);
> }
>
> void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool
> qmp_locked)
> @@ -266,8 +264,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState
> *bs,
> child->disabled = bitmap->disabled;
> bitmap->disabled = true;
>
> -/* Install the successor and freeze the parent */
> +/* Install the successor and lock the parent */
> bitmap->successor = child;
> +bitmap->qmp_locked = true;
> return 0;
> }
>
> @@ -321,6 +320,7 @@ BdrvDirtyBitmap
> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> bitmap->successor = NULL;
> successor->persistent = bitmap->persistent;
> bitmap->persistent = false;
> +bitmap->qmp_locked = false;
> bdrv_release_dirty_bitmap(bs, bitmap);
>
> return successor;
> @@ -349,6 +349,7 @@ BdrvDirtyBitmap
> *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> }
>
> parent->disabled = successor->disabled;
> +parent->qmp_locked = false;
> bdrv_release_dirty_bitmap_locked(successor);
> parent->successor = NULL;
>
>
--
Best regards,
Vladimir
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/6] block/dirty-bitmap: change semantics of enabled predicate
14.02.2019 2:23, John Snow wrote:
> Currently, enabled means something like "the status of the bitmap
> is ACTIVE." After this patch, it should mean exclusively: "This
> bitmap is recording guest writes, and is allowed to do so."
>
> In many places, this is how this predicate was already used.
> We'll allow users to call user_locked if they're really curious about
> finding out if the bitmap is in use by an operation.
>
> To accommodate this, modify the create_successor routine to now
> explicitly disable the parent bitmap at creation time.
>
>
> Justifications:
>
> 1. bdrv_dirty_bitmap_status suffers no change from the lack of
> 1:1 parity with the new predicates because of the order in which
> the predicates are checked. This is now only for compatibility.
>
> 2. bdrv_set_dirty_bitmap is only used by mirror, which does not use
> disabled bitmaps -- all of these writes are internal usages.
> Therefore, we should allow writes even in the disabled state.
> The condition is removed.
>
> 3. bdrv_reset_dirty_bitmap Similarly, this is only used internally by
> mirror and migration. In these contexts it is always enabled anyway,
> but our API does not need to enforce this.
>
> 4. bdrv_set_dirty() is unchanged: pre-patch, it was skipping bitmaps that were
> disabled or had a successor, while post-patch it is only skipping bitmaps
> that are disabled. To accommodate this, create_successor now ensures that
> any bitmap with a successor is explicitly disabled.
>
5-8 are examples of "this is how this predicate was already used"
> 5. qcow2_store_persistent_dirty_bitmaps: This only ever wanted to check if the
> bitmap was enabled or not. Theoretically if we save during an operation,
> this now gets set as enabled instead of disabled,
No, as you explicitly disable bitmap in create_successor, so bitmaps with
successor
will be disabled anyway.
Hmm, and this shows, that actually, you don't need this big description for all
calls,
as actually nothing changed and all calls may be described like (4.). Except
(2. and 3.),
as these calls are removed (so, is it worth to split them into separate
previous patch?)
but this cannot happen
> in practice because jobs must be finished before we close the disk.
>
> 6. block_dirty_bitmap_enable_prepare only ever cared about the
> literal bit, and already checked for user_locked beforehand.
>
> 7. block_dirty_bitmap_disable_prepare ditto as above.
>
> 8. init_dirty_bitmap_migration also already checks user_locked,
> so this call can be a simple enabled/disabled check.
hmmm
9. nbd_export_new, which too checks bdrv_dirty_bitmap_user_locked but _after_
call to bdrv_dirty_bitmap_enabled. Anyway it's not changed as described in
(4.),
I think it is better to check _user_locked first.
>
> Signed-off-by: John Snow
> Reviewed-by: Eric Blake
> ---
> block/dirty-bitmap.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 639ebc0645..bb2e19e0d8 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -209,7 +209,7 @@ bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap)
> /* Called with BQL taken. */
> bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
> {
> -return !(bitmap->disabled || bitmap->successor);
> +return !bitmap->disabled;
> }
>
> /* Called with BQL taken. */
> @@ -264,6 +264,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState
> *bs,
>
> /* Successor will be on or off based on our current state. */
> child->disabled = bitmap->disabled;
> +bitmap->disabled = true;
>
> /* Install the successor and freeze the parent */
> bitmap->successor = child;
> @@ -346,6 +347,8 @@ BdrvDirtyBitmap
> *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> error_setg(errp, "Merging of parent and successor bitmap failed");
> return NULL;
> }
> +
> +parent->disabled = successor->disabled;
at this point comment to the function
"The merged parent will not be user_locked, nor explicitly re-enabled."
becomes really weird. Need to reword it somehow..
> bdrv_release_dirty_bitmap_locked(successor);
> parent->successor = NULL;
>
> @@ -542,7 +545,6 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
> void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
> int64_t offset, int64_t bytes)
> {
> -assert(bdrv_dirty_bitmap_enabled(bitmap));
> assert(!bdrv_dirty_bitmap_readonly(bitmap));
> hbitmap_set(bitmap->bitmap, offset, bytes);
> }
> @@ -559,7 +561,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
> int64_t offset, int64_t bytes)
> {
> -assert(bdrv_dirty_bitmap_enabled(bitmap));
>
Re: [libvirt] [PATCH v4 00/20] Incremental Backup API additions
On Wed, Feb 06, 2019 at 01:17:58PM -0600, Eric Blake wrote:
> The following is the latest version of my API proposal for
> incremental backups, and follows along from the demo I presented
> as part of my KVM Forum 2018 talk:
> https://kvmforum2018.sched.com/event/FzuB/facilitating-incremental-backup-eric-blake-red-hat
>
> The patches are also available via https://repo.or.cz/libvirt/ericb.git
> at the tag backup-v4.
>
> Integration with external snapshots is still not included in these
> patches, although I have been playing more with that locally, and
> I still haven't gotten a working demonstration of a push-mode
> incremental backup.
>
> At this point, I'm posting mainly because there have been enough
> changes in qemu (the backup APIs are now stable! and in qemu.git
> in time for 4.0) and libvirt (several releases and code cleanups
> in between, such that the v3 no longer applies easily), while I
> continue to work locally on more features, addressing the review
> comments I got on v3, and remove the 'wip' tag on several of the
> later patches.
>
> Among other things, I still haven't determined how we want to
> integrate checkpoints with external snapshots; we could either have:
>
> virDomainSnapshotCreateXML("...") # existing
> virDomainBackupBegin("...", "...") # this
> series
> virDomainSnapshotCheckpointCreateXML("...",
> "...") # new
A slightly related question: when these new APIs (thanks for working on
them!) are merged, am I right in assuming that they should be able to
"replace" the existing (and provide additional):
virDomainBlockRebase(); and
virDomainBlockCopy()
... _provided_ that an application is adjusted to using libvirt that is
new enough to drive QEMU's '-blockdev', QMP `blockdev-add` et al?
Or is that (the new APIs being backward-compatible with blockRebase()
and blockCopy()) an explicit non-goal?
I'm only this out of curiosity.
> or to make checkpoint creation part of the snapshot and backup XML, as in:
>
> virDomainSnapshotCreateXML("...")
> # extension of existing
> virDomainBackupBegin("...") #
> tweak to this series
>
> I'm also planning to add an API to query which job ids are currently
> running (right now, the code only supports one job at a time, and
> always calls it job 1, but that is not future-friendly) and an update
> to the redefine XML command to make it easier to redefine multiple
> checkpoints in one API call.
[...]
--
/kashyap
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remove remains of kqemu
On Mon, Feb 18, 2019 at 04:15:21PM +0100, Ján Tomko wrote: > We dropped support in commit 8e91a40 (November 2015), but some > occurrences still remained, even in live code. > > Signed-off-by: Ján Tomko > Reported-by: Thomas Huth > --- Can we at least get rid of X_QEMU_CAPS_KQEMU and X_QEMU_CAPS_ENABLE_KQEMU enums as per: "When a flag is no longer needed we temporarily give it an X_ prefix to indicate it should no longer be used in code. Periodically we can then purge all the X_ flags..." No other comments: Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] iohelper: Don't include newlines in error messages
On 2/13/19 7:04 AM, Andrea Bolognani wrote:
> The newline was pretty arbitrary, and we're better off
> without it.
>
> Signed-off-by: Andrea Bolognani
> ---
> src/util/iohelper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
I'm mostly ambivalent about this one; however, since
virGetLastErrorMessage could return a string without a "\n", then
perhaps it's best to keep the \n since it really doesn't hurt.
John
> diff --git a/src/util/iohelper.c b/src/util/iohelper.c
> index 1ff4a7b314..aed7ef3184 100644
> --- a/src/util/iohelper.c
> +++ b/src/util/iohelper.c
> @@ -236,7 +236,7 @@ main(int argc, char **argv)
> return 0;
>
> error:
> -fprintf(stderr, _("%s: failure with %s\n: %s"),
> +fprintf(stderr, _("%s: failure with %s: %s"),
> program_name, path, virGetLastErrorMessage());
> exit(EXIT_FAILURE);
> }
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/3] virfile: Report error in virFileWrapperFdFree()
On 2/13/19 7:04 AM, Andrea Bolognani wrote:
> Logging the error is fine and all, but getting the information
> to the user directly is even better.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1578741
> Signed-off-by: Andrea Bolognani
> ---
> src/util/virfile.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
Shall I assume this works because qemuProcessHandleDumpCompleted will
move this message now as opposed to some other message related to EPIPE
as noted in the bz response?
I think it's good to fill in pieces of the commit message at least so if
someone else ends up in this same code they have a few hints where to start.
My other throughts would be to note commit 1f25194ad and 34e8f63a3 as
the genesis of at least printing the message "somewhere"... Then commit
b0c3e931 at least made sure the message got printed in the event that
the *FdClose never occurred. Just saying "is fine and all" hand waves a
bit too much for me compared to what you got to figure out here ;-).
Reviewed-by: John Ferlan
John
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 271bf5e796..30cad87df9 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -351,8 +351,12 @@ virFileWrapperFdFree(virFileWrapperFdPtr wfd)
> if (!wfd)
> return;
>
> +/* If the command used to process IO has produced errors, it's fair
> + * to assume those will be more relevant to the user than whatever
> + * eg. QEMU can figure out on its own, so it's okay if we end up
> + * discarding an existing error */
> if (wfd->err_msg && *wfd->err_msg)
> -VIR_WARN("iohelper reports: %s", wfd->err_msg);
> +virReportError(VIR_ERR_OPERATION_FAILED, "%s", wfd->err_msg);
>
> virCommandAbort(wfd->cmd);
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: Use VIR_AUTOUNREF for virstoragefile
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan
---
BTW: I did try this with a linked travis-ci and github branch, and it
worked for me. I'm avoiding altering virStorageFileMetadataNew...
src/util/virstoragefile.c | 130 +++---
1 file changed, 52 insertions(+), 78 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index b2e308d81d..003183bf76 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1205,11 +1205,11 @@ virStorageFileGetMetadataFromFD(const char *path,
{
virStorageSourcePtr ret = NULL;
-virStorageSourcePtr meta = NULL;
ssize_t len = VIR_STORAGE_MAX_HEADER;
struct stat sb;
int dummy;
VIR_AUTOFREE(char *) buf = NULL;
+VIR_AUTOUNREF(virStorageSourcePtr) meta = NULL;
if (!backingFormat)
backingFormat =
@@ -1231,21 +1231,21 @@ virStorageFileGetMetadataFromFD(const char *path,
meta->type = VIR_STORAGE_TYPE_DIR;
meta->format = VIR_STORAGE_FILE_DIR;
VIR_STEAL_PTR(ret, meta);
-goto cleanup;
+return ret;
}
if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
virReportSystemError(errno, _("cannot seek to start of '%s'"),
meta->path);
-goto cleanup;
+return NULL;
}
if ((len = virFileReadHeaderFD(fd, len, )) < 0) {
virReportSystemError(errno, _("cannot read header '%s'"), meta->path);
-goto cleanup;
+return NULL;
}
if (virStorageFileGetMetadataInternal(meta, buf, len, backingFormat) < 0)
-goto cleanup;
+return NULL;
if (S_ISREG(sb.st_mode))
meta->type = VIR_STORAGE_TYPE_FILE;
@@ -1253,9 +1253,6 @@ virStorageFileGetMetadataFromFD(const char *path,
meta->type = VIR_STORAGE_TYPE_BLOCK;
VIR_STEAL_PTR(ret, meta);
-
- cleanup:
-virObjectUnref(meta);
return ret;
}
@@ -2243,7 +2240,8 @@ virStorageSourcePtr
virStorageSourceCopy(const virStorageSource *src,
bool backingChain)
{
-virStorageSourcePtr def = NULL;
+virStorageSourcePtr ret = NULL;
+VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
if (!(def = virStorageSourceNew()))
return NULL;
@@ -2282,60 +2280,57 @@ virStorageSourceCopy(const virStorageSource *src,
VIR_STRDUP(def->compat, src->compat) < 0 ||
VIR_STRDUP(def->tlsAlias, src->tlsAlias) < 0 ||
VIR_STRDUP(def->tlsCertdir, src->tlsCertdir) < 0)
-goto error;
+return NULL;
if (src->nhosts) {
if (!(def->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts)))
-goto error;
+return NULL;
def->nhosts = src->nhosts;
}
if (src->srcpool &&
!(def->srcpool = virStorageSourcePoolDefCopy(src->srcpool)))
-goto error;
+return NULL;
if (src->features &&
!(def->features = virBitmapNewCopy(src->features)))
-goto error;
+return NULL;
if (src->encryption &&
!(def->encryption = virStorageEncryptionCopy(src->encryption)))
-goto error;
+return NULL;
if (src->perms &&
!(def->perms = virStoragePermsCopy(src->perms)))
-goto error;
+return NULL;
if (src->timestamps &&
!(def->timestamps = virStorageTimestampsCopy(src->timestamps)))
-goto error;
+return NULL;
if (virStorageSourceSeclabelsCopy(def, src) < 0)
-goto error;
+return NULL;
if (src->auth &&
!(def->auth = virStorageAuthDefCopy(src->auth)))
-goto error;
+return NULL;
if (src->pr &&
!(def->pr = virStoragePRDefCopy(src->pr)))
-goto error;
+return NULL;
if (virStorageSourceInitiatorCopy(>initiator, >initiator))
-goto error;
+return NULL;
if (backingChain && src->backingStore) {
if (!(def->backingStore = virStorageSourceCopy(src->backingStore,
true)))
-goto error;
+return NULL;
}
-return def;
-
- error:
-virObjectUnref(def);
-return NULL;
+VIR_STEAL_PTR(ret, def);
+return ret;
}
@@ -2601,27 +2596,28 @@ static virStorageSourcePtr
virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent,
const char *rel)
{
-virStorageSourcePtr def;
+virStorageSourcePtr ret = NULL;
VIR_AUTOFREE(char *) dirname = NULL;
+VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
if (!(def = virStorageSourceNew()))
return NULL;
/* store relative name */
if (VIR_STRDUP(def->relPath, parent->backingStoreRaw) < 0)
-goto error;
+return NULL;
if (!(dirname = mdir_name(parent->path))) {
virReportOOMError();
-goto error;
+return NULL;
}
if
[libvirt] [PATCH] remove remains of kqemu
We dropped support in commit 8e91a40 (November 2015), but some
occurrences still remained, even in live code.
Signed-off-by: Ján Tomko
Reported-by: Thomas Huth
---
docs/drvqemu.html.in | 2 +-
docs/formatdomain.html.in | 2 +-
src/qemu/qemu.conf | 2 +-
src/qemu/qemu_cgroup.c | 2 +-
src/qemu/qemu_driver.c | 3 ---
src/qemu/test_libvirtd_qemu.aug.in | 5 ++---
src/security/apparmor/libvirt-qemu | 1 -
7 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/docs/drvqemu.html.in b/docs/drvqemu.html.in
index 5ad956740f..0218b5a7a9 100644
--- a/docs/drvqemu.html.in
+++ b/docs/drvqemu.html.in
@@ -395,7 +395,7 @@ chmod o+x /path/to/directory
/dev/null, /dev/full, /dev/zero,
/dev/random, /dev/urandom,
-/dev/ptmx, /dev/kvm, /dev/kqemu,
+/dev/ptmx, /dev/kvm,
/dev/rtc, /dev/hpet
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 2ae5006849..7882c54bc1 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -22,7 +22,7 @@
type
specifies the hypervisor used for running
the domain. The allowed values are driver specific, but
- include "xen", "kvm", "qemu", "lxc" and "kqemu". The
+ include "xen", "kvm", "qemu" and "lxc". The
second attribute is id which is a unique
integer identifier for the running guest machine. Inactive
machines have no id value.
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 7820e72dd8..334b4cd4ee 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -489,7 +489,7 @@
#cgroup_device_acl = [
#"/dev/null", "/dev/full", "/dev/zero",
#"/dev/random", "/dev/urandom",
-#"/dev/ptmx", "/dev/kvm", "/dev/kqemu",
+#"/dev/ptmx", "/dev/kvm",
#"/dev/rtc","/dev/hpet"
#]
#
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index e88cb8c45f..c23f0af2aa 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -45,7 +45,7 @@ VIR_LOG_INIT("qemu.qemu_cgroup");
const char *const defaultDeviceACL[] = {
"/dev/null", "/dev/full", "/dev/zero",
"/dev/random", "/dev/urandom",
-"/dev/ptmx", "/dev/kvm", "/dev/kqemu",
+"/dev/ptmx", "/dev/kvm",
"/dev/rtc", "/dev/hpet",
NULL,
};
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 024ee5b62d..5118f4ad42 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1260,9 +1260,6 @@ qemuConnectGetMaxVcpus(virConnectPtr conn
ATTRIBUTE_UNUSED, const char *type)
if (STRCASEEQ(type, "kvm"))
return virHostCPUGetKVMMaxVCPUs();
-if (STRCASEEQ(type, "kqemu"))
-return 1;
-
virReportError(VIR_ERR_INVALID_ARG,
_("unknown type '%s'"), type);
return -1;
diff --git a/src/qemu/test_libvirtd_qemu.aug.in
b/src/qemu/test_libvirtd_qemu.aug.in
index 51a7ad5892..fea1d308b7 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -60,9 +60,8 @@ module Test_libvirtd_qemu =
{ "5" = "/dev/urandom" }
{ "6" = "/dev/ptmx" }
{ "7" = "/dev/kvm" }
-{ "8" = "/dev/kqemu" }
-{ "9" = "/dev/rtc" }
-{ "10" = "/dev/hpet" }
+{ "8" = "/dev/rtc" }
+{ "9" = "/dev/hpet" }
}
{ "save_image_format" = "raw" }
{ "dump_image_format" = "raw" }
diff --git a/src/security/apparmor/libvirt-qemu
b/src/security/apparmor/libvirt-qemu
index eaa5167525..7d28faa163 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -23,7 +23,6 @@
/dev/net/tun rw,
/dev/kvm rw,
/dev/ptmx rw,
- /dev/kqemu rw,
@{PROC}/*/status r,
# When qemu is signaled to terminate, it will read cmdline of signaling
# process for reporting purposes. Allowing read access to a process
--
2.19.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/6] block/dirty-bitmaps: rename frozen predicate helper
14.02.2019 2:23, John Snow wrote:
> "Frozen" was a good description a long time ago, but it isn't adequate now.
> Rename the frozen predicate to has_successor to make the semantics of the
> predicate more clear to outside callers.
>
> In the process, remove some calls to frozen() that no longer semantically
> make sense. For enabled and disabled in particular, it's actually okay for
> the internals to do this but only forbidden for users to invoke them, and
I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom?
I think, it would be simpler for me to read patch itself :)
> all of the QMP entry uses already check against qmp_locked.
>
> Several other assertions really want to check that the bitmap isn't in-use
> by another operation -- use the qmp_locked function for this instead, which
> presently also checks for has_successor.
hm, you mean user_locked, not qmp_locked.
>
> Signed-off-by: John Snow
> ---
> block/dirty-bitmap.c | 32 +---
> include/block/dirty-bitmap.h | 2 +-
> migration/block-dirty-bitmap.c | 2 +-
> 3 files changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 101383b3af..639ebc0645 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -50,7 +50,7 @@ struct BdrvDirtyBitmap {
> HBitmap *meta; /* Meta dirty bitmap */
> bool qmp_locked;/* Bitmap is locked, it can't be modified
> through QMP */
> -BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
> +BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked
> state */
aha, looks like a good moment to fix preexisting misalignment of the comment,
but new line is exactly 80 characters length, so, not a good moment)
> char *name; /* Optional non-empty unique ID */
> int64_t size; /* Size of the bitmap, in bytes */
> bool disabled; /* Bitmap is disabled. It ignores all
> writes to
> @@ -183,14 +183,14 @@ const char *bdrv_dirty_bitmap_name(const
> BdrvDirtyBitmap *bitmap)
> }
>
> /* Called with BQL taken. */
> -bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
> +bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap)
> {
> return bitmap->successor;
> }
>
> /* Both conditions disallow user-modification via QMP. */
> bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
> -return bdrv_dirty_bitmap_frozen(bitmap) ||
> +return bdrv_dirty_bitmap_has_successor(bitmap) ||
> bdrv_dirty_bitmap_qmp_locked(bitmap);
> }
>
> @@ -215,7 +215,7 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
> /* Called with BQL taken. */
> DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
> {
> -if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +if (bdrv_dirty_bitmap_has_successor(bitmap)) {
> return DIRTY_BITMAP_STATUS_FROZEN;
> } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
> return DIRTY_BITMAP_STATUS_LOCKED;
> @@ -235,7 +235,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap
> *bitmap)
>
> /**
>* Create a successor bitmap destined to replace this bitmap after an
> operation.
> - * Requires that the bitmap is not frozen and has no successor.
> + * Requires that the bitmap is not locked and has no successor.
I think, user_locked, to not interfere with bitmaps mutex. And you use
user_locked in
other comments in this patch.
>* Called with BQL taken.
>*/
> int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> @@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState
> *bs,
> uint64_t granularity;
> BdrvDirtyBitmap *child;
>
> -if (bdrv_dirty_bitmap_frozen(bitmap)) {
> -error_setg(errp, "Cannot create a successor for a bitmap that is "
> - "currently frozen");
> +if (bdrv_dirty_bitmap_user_locked(bitmap)) {
> +error_setg(errp, "Cannot create a successor for a bitmap that is
> in-use "
> + "by an operation");
> +return -1;
> +}
> +if (bdrv_dirty_bitmap_has_successor(bitmap)) {
> +error_setg(errp, "Cannot create a successor for a bitmap that
> already "
> + "has one");
Amm, dead code? _user_locked() implies no successor, so we instead can keep an
assertion..
> return -1;
> }
> -assert(!bitmap->successor);
>
> /* Create an anonymous successor */
> granularity = bdrv_dirty_bitmap_granularity(bitmap);
--
Best regards,
Vladimir
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: Use virStorageSourceNew in virStorageFileMetadataNew
On Mon, Feb 18, 2019 at 08:28:54AM -0500, John Ferlan wrote:
>
>
> On 2/18/19 7:51 AM, Peter Krempa wrote:
> > On Mon, Feb 18, 2019 at 07:38:34 -0500, John Ferlan wrote:
> >>
> >>
> >> On 2/18/19 7:27 AM, Peter Krempa wrote:
> >>> Commit dcda2bf4c110 forgot to fix this one instance.
> >>>
> >>> Signed-off-by: Peter Krempa
> >>> ---
> >>> src/util/virstoragefile.c | 14 ++
> >>> 1 file changed, 6 insertions(+), 8 deletions(-)
> >>>
> >>
> >> Right fix as far as I'm concerned; however, I am curious to know whether
> >> this passes the MinGW build test because this is exactly where things
> >> started to break down for VIR_AUTOPTR usage.
> >
> > We'll see after I push it. I don't have mingw deployed and don't care
> > enough about that platform to do so.
> >
>
> Cannot disagree about the relevance of the importance of MinGW. Still I
> note it because it was something that caused previous changes to add
> VIR_AUTOPTR for this module to not be pushed. I was pointed in the
> direction of Andrea and the lcitool, but TBH that didn't help me much.
> Eventually I noted that Erik had run a build via some link between
> travis-ci.org and a github repo. I was able to do something similar and
> found a similar failure.
This actually passes the build on MinGW:
https://travis-ci.org/eskultety/libvirt
>
> >>
> >> VIR_AUTOUNREF could also be used more liberally in this module...
> >
> > I'll not pursue this refactor.
> >
> >>
> >>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> >>> index 5927140a66..b2e308d81d 100644
> >>> --- a/src/util/virstoragefile.c
> >>> +++ b/src/util/virstoragefile.c
> >>> @@ -1119,22 +1119,20 @@ static virStorageSourcePtr
> >>> virStorageFileMetadataNew(const char *path,
> >>>int format)
> >>> {
> >>> -virStorageSourcePtr def = NULL;
> >>> +VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
> >>> +virStorageSourcePtr ret = NULL;
> >>
> >> Erik prefers the usage of VIR_AUTO* defs last (IOW, swap these).
> >
> > Well I prefer if the returned variable is last and if the longer lines
> > are first.
> >
>
> Picking and choosing which review comments to follow is an interesting
> decision - hopefully it's not contagious.
>
> Consistency wise, VIR_AUTO* defs have been last. If it's that important
> I suppose per usual someone can come in afterwards and propose another
> patch as well as either a rule in/for make check or adjustment to the
> hacking guide.
I am obviously in favour of consistency and I'd like to us to follow it, but of'
course as a reviewer you can't really force the author to do that :/..
well, wechnically we could abuse perl once again for "the rescue" and create a
syntax-check rule, but HELL no...I agree that we might want to mention this in
the HACKING guide.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: Use virStorageSourceNew in virStorageFileMetadataNew
On Mon, Feb 18, 2019 at 08:28:54 -0500, John Ferlan wrote:
>
>
> On 2/18/19 7:51 AM, Peter Krempa wrote:
> > On Mon, Feb 18, 2019 at 07:38:34 -0500, John Ferlan wrote:
> >>
> >>
> >> On 2/18/19 7:27 AM, Peter Krempa wrote:
> >>> Commit dcda2bf4c110 forgot to fix this one instance.
> >>>
> >>> Signed-off-by: Peter Krempa
> >>> ---
> >>> src/util/virstoragefile.c | 14 ++
> >>> 1 file changed, 6 insertions(+), 8 deletions(-)
> >>>
> >>
> >> Right fix as far as I'm concerned; however, I am curious to know whether
> >> this passes the MinGW build test because this is exactly where things
> >> started to break down for VIR_AUTOPTR usage.
> >
> > We'll see after I push it. I don't have mingw deployed and don't care
> > enough about that platform to do so.
> >
>
> Cannot disagree about the relevance of the importance of MinGW. Still I
> note it because it was something that caused previous changes to add
> VIR_AUTOPTR for this module to not be pushed. I was pointed in the
> direction of Andrea and the lcitool, but TBH that didn't help me much.
> Eventually I noted that Erik had run a build via some link between
> travis-ci.org and a github repo. I was able to do something similar and
> found a similar failure.
I've looked at the ci.centos.org page and the tests were aborted after
90 mins without any error:
This is the last output:
CC virnettlshelpers.o
CC pkix_asn1_tab.o
CC virnettlssessiontest.o
CC virdbustest-virdbustest.o
CC virdbustest-testutils.o
Build timed out (after 90 minutes). Marking the build as aborted.
Build was aborted
Finished: ABORTED
Seems to me that it's more an environment or compiler problem.
> >> VIR_AUTOUNREF could also be used more liberally in this module...
> >
> > I'll not pursue this refactor.
> >
> >>
> >>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> >>> index 5927140a66..b2e308d81d 100644
> >>> --- a/src/util/virstoragefile.c
> >>> +++ b/src/util/virstoragefile.c
> >>> @@ -1119,22 +1119,20 @@ static virStorageSourcePtr
> >>> virStorageFileMetadataNew(const char *path,
> >>>int format)
> >>> {
> >>> -virStorageSourcePtr def = NULL;
> >>> +VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
> >>> +virStorageSourcePtr ret = NULL;
> >>
> >> Erik prefers the usage of VIR_AUTO* defs last (IOW, swap these).
> >
> > Well I prefer if the returned variable is last and if the longer lines
> > are first.
> >
>
> Picking and choosing which review comments to follow is an interesting
> decision - hopefully it's not contagious.
>
> Consistency wise, VIR_AUTO* defs have been last. If it's that important
> I suppose per usual someone can come in afterwards and propose another
> patch as well as either a rule in/for make check or adjustment to the
> hacking guide.
Any other variable addition after the block of the VIR_AUTO*stuff will
be forgotten sooner or later during review, so if isn't enforced by
a syntax check rule, it's pointless to even think to open the editor
and change it.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: Use virStorageSourceNew in virStorageFileMetadataNew
On 2/18/19 7:51 AM, Peter Krempa wrote:
> On Mon, Feb 18, 2019 at 07:38:34 -0500, John Ferlan wrote:
>>
>>
>> On 2/18/19 7:27 AM, Peter Krempa wrote:
>>> Commit dcda2bf4c110 forgot to fix this one instance.
>>>
>>> Signed-off-by: Peter Krempa
>>> ---
>>> src/util/virstoragefile.c | 14 ++
>>> 1 file changed, 6 insertions(+), 8 deletions(-)
>>>
>>
>> Right fix as far as I'm concerned; however, I am curious to know whether
>> this passes the MinGW build test because this is exactly where things
>> started to break down for VIR_AUTOPTR usage.
>
> We'll see after I push it. I don't have mingw deployed and don't care
> enough about that platform to do so.
>
Cannot disagree about the relevance of the importance of MinGW. Still I
note it because it was something that caused previous changes to add
VIR_AUTOPTR for this module to not be pushed. I was pointed in the
direction of Andrea and the lcitool, but TBH that didn't help me much.
Eventually I noted that Erik had run a build via some link between
travis-ci.org and a github repo. I was able to do something similar and
found a similar failure.
>>
>> VIR_AUTOUNREF could also be used more liberally in this module...
>
> I'll not pursue this refactor.
>
>>
>>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>>> index 5927140a66..b2e308d81d 100644
>>> --- a/src/util/virstoragefile.c
>>> +++ b/src/util/virstoragefile.c
>>> @@ -1119,22 +1119,20 @@ static virStorageSourcePtr
>>> virStorageFileMetadataNew(const char *path,
>>>int format)
>>> {
>>> -virStorageSourcePtr def = NULL;
>>> +VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
>>> +virStorageSourcePtr ret = NULL;
>>
>> Erik prefers the usage of VIR_AUTO* defs last (IOW, swap these).
>
> Well I prefer if the returned variable is last and if the longer lines
> are first.
>
Picking and choosing which review comments to follow is an interesting
decision - hopefully it's not contagious.
Consistency wise, VIR_AUTO* defs have been last. If it's that important
I suppose per usual someone can come in afterwards and propose another
patch as well as either a rule in/for make check or adjustment to the
hacking guide.
John
>> As long as this gets through the MinGW build test, then
>>
>> Reviewed-by: John Ferlan
>>
>> John
>>
>> Altering other virStorageSource refs here to use VIR_AUTOUNREF would be
>> the next step (and of course getting them through MinGW build test).
>>
>>>
>>> -if (VIR_ALLOC(def) < 0)
>>> +if (!(def = virStorageSourceNew()))
>>> return NULL;
>>>
>>> def->format = format;
>>> def->type = VIR_STORAGE_TYPE_FILE;
>>>
>>> if (VIR_STRDUP(def->path, path) < 0)
>>> -goto error;
>>> -
>>> -return def;
>>> +return NULL;
>>>
>>> - error:
>>> -virObjectUnref(def);
>>> -return NULL;
>>> +VIR_STEAL_PTR(ret, def);
>>> +return ret;
>>> }
>>>
>>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/6] block/dirty-bitmap: add recording and busy properties
14.02.2019 2:23, John Snow wrote:
> The current API allows us to report a single status, which we've defined as:
>
> Frozen: has a successor, treated as qmp_locked, may or may not be enabled.
> Locked: no successor, qmp_locked. may or may not be enabled.
> Disabled: Not frozen or locked, disabled.
> Active: Not frozen, locked, or disabled.
>
> The problem is that both "Frozen" and "Locked" mean nearly the same thing,
> and that both of them do not intuit whether they are recording guest writes
> or not.
>
> This patch deprecates that status field and introduces two orthogonal
> properties instead to replace it.
>
> Signed-off-by: John Snow
> ---
> block/dirty-bitmap.c | 9 +
> qapi/block-core.json | 10 +-
> qemu-deprecated.texi | 6 ++
> tests/qemu-iotests/236.out | 28
> 4 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index c6d4acebfa..101383b3af 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -226,6 +226,13 @@ DirtyBitmapStatus
> bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
> }
> }
>
> +/* Called with BQL taken. */
> +static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
> +{
> +return !bitmap->disabled || (bitmap->successor &&
> + !bitmap->successor->disabled);
> +}
> +
> /**
>* Create a successor bitmap destined to replace this bitmap after an
> operation.
>* Requires that the bitmap is not frozen and has no successor.
> @@ -448,6 +455,8 @@ BlockDirtyInfoList
> *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
> info->has_name = !!bm->name;
> info->name = g_strdup(bm->name);
> info->status = bdrv_dirty_bitmap_status(bm);
> +info->recording = bdrv_dirty_bitmap_recording(bm);
> +info->busy = bdrv_dirty_bitmap_user_locked(bm);
> info->persistent = bm->persistent;
> entry->value = info;
> *plist = entry;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8f23f2ebb8..5d1d182447 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -458,7 +458,14 @@
> #
> # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
> #
> -# @status: current status of the dirty bitmap (since 2.4)
> +# @status: Deprecated in favor of @recording and @locked. (since 2.4)
> +#
> +# @recording: true if the bitmap is recording new writes from the guest.
> +# Replaces `active` and `disabled` statuses. (since 4.0)
> +#
> +# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
> +#and cannot be modified via QMP or used by another operation.
> +#Replaces `locked` and `frozen` statuses. (since 4.0)
> #
> # @persistent: true if the bitmap will eventually be flushed to persistent
> # storage (since 4.0)
> @@ -467,6 +474,7 @@
> ##
> { 'struct': 'BlockDirtyInfo',
> 'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
> + 'recording': 'bool', 'busy': 'bool',
> 'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
>
> ##
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 80b0702ad5..f7c9f4c101 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -84,6 +84,12 @@ topologies described with -smp include all possible cpus,
> i.e.
> "autoload" parameter is now ignored. All bitmaps are automatically loaded
> from qcow2 images.
>
> +@subsection query-block dirty-bitmaps.status parameter (since 4.0)
It is not a parameter. It's a field of result structure..
maybe
@subsection query-block result field 'dirty-bitmaps[i].status' (since 4.0)
with or without it:
Reviewed-by: Vladimir Sementsov-Ogievskiy
> +
> +The ``status'' field of the ``BlockDirtyInfo'' structure, returned by
> +the query-block command is deprecated. Two new boolean fields,
> +``recording'' and ``busy'' effectively replace it.
Hm, preexisting, but do we have some good alternative to `` '' pair? Maybe a
kind
of @var{} or something?
> +
> @subsection query-cpus (since 2.12.0)
>
> The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
--
Best regards,
Vladimir
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: Use virStorageSourceNew in virStorageFileMetadataNew
On Mon, Feb 18, 2019 at 07:38:34 -0500, John Ferlan wrote:
>
>
> On 2/18/19 7:27 AM, Peter Krempa wrote:
> > Commit dcda2bf4c110 forgot to fix this one instance.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > src/util/virstoragefile.c | 14 ++
> > 1 file changed, 6 insertions(+), 8 deletions(-)
> >
>
> Right fix as far as I'm concerned; however, I am curious to know whether
> this passes the MinGW build test because this is exactly where things
> started to break down for VIR_AUTOPTR usage.
We'll see after I push it. I don't have mingw deployed and don't care
enough about that platform to do so.
>
> VIR_AUTOUNREF could also be used more liberally in this module...
I'll not pursue this refactor.
>
> > diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> > index 5927140a66..b2e308d81d 100644
> > --- a/src/util/virstoragefile.c
> > +++ b/src/util/virstoragefile.c
> > @@ -1119,22 +1119,20 @@ static virStorageSourcePtr
> > virStorageFileMetadataNew(const char *path,
> >int format)
> > {
> > -virStorageSourcePtr def = NULL;
> > +VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
> > +virStorageSourcePtr ret = NULL;
>
> Erik prefers the usage of VIR_AUTO* defs last (IOW, swap these).
Well I prefer if the returned variable is last and if the longer lines
are first.
> As long as this gets through the MinGW build test, then
>
> Reviewed-by: John Ferlan
>
> John
>
> Altering other virStorageSource refs here to use VIR_AUTOUNREF would be
> the next step (and of course getting them through MinGW build test).
>
> >
> > -if (VIR_ALLOC(def) < 0)
> > +if (!(def = virStorageSourceNew()))
> > return NULL;
> >
> > def->format = format;
> > def->type = VIR_STORAGE_TYPE_FILE;
> >
> > if (VIR_STRDUP(def->path, path) < 0)
> > -goto error;
> > -
> > -return def;
> > +return NULL;
> >
> > - error:
> > -virObjectUnref(def);
> > -return NULL;
> > +VIR_STEAL_PTR(ret, def);
> > +return ret;
> > }
> >
> >
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: Use virStorageSourceNew in virStorageFileMetadataNew
On 2/18/19 7:27 AM, Peter Krempa wrote:
> Commit dcda2bf4c110 forgot to fix this one instance.
>
> Signed-off-by: Peter Krempa
> ---
> src/util/virstoragefile.c | 14 ++
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
Right fix as far as I'm concerned; however, I am curious to know whether
this passes the MinGW build test because this is exactly where things
started to break down for VIR_AUTOPTR usage.
VIR_AUTOUNREF could also be used more liberally in this module...
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 5927140a66..b2e308d81d 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1119,22 +1119,20 @@ static virStorageSourcePtr
> virStorageFileMetadataNew(const char *path,
>int format)
> {
> -virStorageSourcePtr def = NULL;
> +VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
> +virStorageSourcePtr ret = NULL;
Erik prefers the usage of VIR_AUTO* defs last (IOW, swap these).
As long as this gets through the MinGW build test, then
Reviewed-by: John Ferlan
John
Altering other virStorageSource refs here to use VIR_AUTOUNREF would be
the next step (and of course getting them through MinGW build test).
>
> -if (VIR_ALLOC(def) < 0)
> +if (!(def = virStorageSourceNew()))
> return NULL;
>
> def->format = format;
> def->type = VIR_STORAGE_TYPE_FILE;
>
> if (VIR_STRDUP(def->path, path) < 0)
> -goto error;
> -
> -return def;
> +return NULL;
>
> - error:
> -virObjectUnref(def);
> -return NULL;
> +VIR_STEAL_PTR(ret, def);
> +return ret;
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: Use virStorageSourceNew in virStorageFileMetadataNew
Commit dcda2bf4c110 forgot to fix this one instance.
Signed-off-by: Peter Krempa
---
src/util/virstoragefile.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 5927140a66..b2e308d81d 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1119,22 +1119,20 @@ static virStorageSourcePtr
virStorageFileMetadataNew(const char *path,
int format)
{
-virStorageSourcePtr def = NULL;
+VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
+virStorageSourcePtr ret = NULL;
-if (VIR_ALLOC(def) < 0)
+if (!(def = virStorageSourceNew()))
return NULL;
def->format = format;
def->type = VIR_STORAGE_TYPE_FILE;
if (VIR_STRDUP(def->path, path) < 0)
-goto error;
-
-return def;
+return NULL;
- error:
-virObjectUnref(def);
-return NULL;
+VIR_STEAL_PTR(ret, def);
+return ret;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] util: Replace virStorageSourceFree with virObjectUnref
On Mon, Feb 18, 2019 at 07:15:38 -0500, John Ferlan wrote: > > > On 2/15/19 7:42 AM, Peter Krempa wrote: > > Now that virStorageSource is a subclass of virObject we can use > > virObjectUnref and remove virStorageSourceFree which was a thin wrapper. > > > > Signed-off-by: Peter Krempa > > --- > > src/conf/domain_conf.c | 8 > > src/conf/snapshot_conf.c | 2 +- > > src/libvirt_private.syms | 1 - > > src/qemu/qemu_blockjob.c | 10 +- > > src/qemu/qemu_domain.c | 2 +- > > src/qemu/qemu_driver.c | 6 +++--- > > src/qemu/qemu_hotplug.c| 2 +- > > src/qemu/qemu_migration.c | 2 +- > > src/storage/storage_util.c | 2 +- > > src/util/virstoragefile.c | 35 ++- > > src/util/virstoragefile.h | 1 - > > tests/virstoragetest.c | 4 ++-- > > 12 files changed, 33 insertions(+), 42 deletions(-) > > > > [...] > > > diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c > > index 56c6510c5e..9dd27b5ca6 100644 > > --- a/src/util/virstoragefile.c > > +++ b/src/util/virstoragefile.c > > @@ -1133,7 +1133,7 @@ virStorageFileMetadataNew(const char *path, > > return def; > > > > error: > > -virStorageSourceFree(def); > > +virObjectUnref(def); > > Is this right? @def is VIR_ALLOC'd and not virStorageSourceNew alloc'd. > Also anything that calls this would have the same problem I would think. No. virStorageSource now must be allocated using virStorageSourceNew. The first patch missed fixing this instance. I'll post a patch soon. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] util: Replace virStorageSourceFree with virObjectUnref
On 2/15/19 7:42 AM, Peter Krempa wrote:
> Now that virStorageSource is a subclass of virObject we can use
> virObjectUnref and remove virStorageSourceFree which was a thin wrapper.
>
> Signed-off-by: Peter Krempa
> ---
> src/conf/domain_conf.c | 8
> src/conf/snapshot_conf.c | 2 +-
> src/libvirt_private.syms | 1 -
> src/qemu/qemu_blockjob.c | 10 +-
> src/qemu/qemu_domain.c | 2 +-
> src/qemu/qemu_driver.c | 6 +++---
> src/qemu/qemu_hotplug.c| 2 +-
> src/qemu/qemu_migration.c | 2 +-
> src/storage/storage_util.c | 2 +-
> src/util/virstoragefile.c | 35 ++-
> src/util/virstoragefile.h | 1 -
> tests/virstoragetest.c | 4 ++--
> 12 files changed, 33 insertions(+), 42 deletions(-)
>
[...]
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 56c6510c5e..9dd27b5ca6 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1133,7 +1133,7 @@ virStorageFileMetadataNew(const char *path,
> return def;
>
> error:
> -virStorageSourceFree(def);
> +virObjectUnref(def);
Is this right? @def is VIR_ALLOC'd and not virStorageSourceNew alloc'd.
Also anything that calls this would have the same problem I would think.
FWIW: This module is the one I ran afoul of the MinGW builds when trying
to use VIR_AUTOPTR. I see no change in here to use VIR_AUTOUNREF, so I
assume this was the way around it, albeit without the object alloc.
John
> return NULL;
> }
>
> @@ -1158,7 +1158,7 @@ virStorageFileMetadataNew(const char *path,
> * image didn't specify an explicit format for its backing store. Callers are
> * advised against probing for the backing store format in this case.
> *
> - * Caller MUST free the result after use via virStorageSourceFree.
> + * Caller MUST free the result after use via virObjectUnref.
> */
> virStorageSourcePtr
> virStorageFileGetMetadataFromBuf(const char *path,
> @@ -1178,7 +1178,7 @@ virStorageFileGetMetadataFromBuf(const char *path,
>
> if (virStorageFileGetMetadataInternal(ret, buf, len,
>backingFormat) < 0) {
> -virStorageSourceFree(ret);
> +virObjectUnref(ret);
> return NULL;
> }
>
> @@ -1197,7 +1197,7 @@ virStorageFileGetMetadataFromBuf(const char *path,
> * format, since a malicious guest can turn a raw file into any
> * other non-raw format at will.
> *
> - * Caller MUST free the result after use via virStorageSourceFree.
> + * Caller MUST free the result after use via virObjectUnref.
> */
> virStorageSourcePtr
> virStorageFileGetMetadataFromFD(const char *path,
> @@ -1257,7 +1257,7 @@ virStorageFileGetMetadataFromFD(const char *path,
> VIR_STEAL_PTR(ret, meta);
>
> cleanup:
> -virStorageSourceFree(meta);
> +virObjectUnref(meta);
> return ret;
> }
>
> @@ -2336,7 +2336,7 @@ virStorageSourceCopy(const virStorageSource *src,
> return def;
>
> error:
> -virStorageSourceFree(def);
> +virObjectUnref(def);
> return NULL;
> }
>
> @@ -2522,7 +2522,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr
> def)
> VIR_FREE(def->backingStoreRaw);
>
> /* recursively free backing chain */
> -virStorageSourceFree(def->backingStore);
> +virObjectUnref(def->backingStore);
> def->backingStore = NULL;
> }
>
> @@ -2598,13 +2598,6 @@ virStorageSourceNew(void)
> }
>
>
> -void
> -virStorageSourceFree(virStorageSourcePtr def)
> -{
> -virObjectUnref(def);
> -}
> -
> -
> static virStorageSourcePtr
> virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent,
> const char *rel)
> @@ -2656,7 +2649,7 @@
> virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent,
> return def;
>
> error:
> -virStorageSourceFree(def);
> +virObjectUnref(def);
> def = NULL;
> goto cleanup;
> }
> @@ -3697,7 +3690,7 @@ virStorageSourceNewFromBackingAbsolute(const char *path)
> return def;
>
> error:
> -virStorageSourceFree(def);
> +virObjectUnref(def);
> return NULL;
> }
>
> @@ -3738,7 +3731,7 @@ virStorageSourceNewFromBacking(virStorageSourcePtr
> parent)
> return def;
>
> error:
> -virStorageSourceFree(def);
> +virObjectUnref(def);
> return NULL;
> }
>
> @@ -3919,7 +3912,7 @@ virStorageSourceUpdateCapacity(virStorageSourcePtr src,
> ret = 0;
>
> cleanup:
> -virStorageSourceFree(meta);
> +virObjectUnref(meta);
> return ret;
> }
>
> @@ -4943,7 +4936,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr
> src,
> if (virStorageSourceHasBacking(src))
> src->backingStore->id = depth;
> virStorageFileDeinit(src);
> -virStorageSourceFree(backingStore);
> +virObjectUnref(backingStore);
> return ret;
> }
>
> @@ -4966,7 +4959,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr
> src,
> * If @report_broken
Re: [libvirt] [PATCH 2/2] security: aa-helper: generate more rules for gl devices
On Fri, Feb 15, 2019 at 11:29 PM Jamie Strandboge wrote:
>
> On Tue, 12 Feb 2019, Christian Ehrhardt wrote:
>
> > Change fb01e1a44 "virt-aa-helper: generate rules for gl enabled
> > graphics devices" implemented the detection for gl enabled
> > devices in virt-aa-helper. But further testing showed
> > that it will need much more access for the full gl stack
> > to work.
> >
> > Upstream apparmor just recently split those things out and now
> > has two related abstractions at
> > https://gitlab.com/apparmor/apparmor/blob/master:
> > - dri-common at /profiles/apparmor.d/abstractions/dri-common
> > - mesa: at /profiles/apparmor.d/abstractions/mesa
> >
> > If would be great to just include that for the majority of
> > rules, but they are not yet in any distribution so we need
> > to add rules inspired by them based on the testing that we
> > can do.
> >
> > Furthermore qemu with opengl will also probe the backing device
> > of the rendernode for attributes which should be safe as
> > read-only wildcard rules.
> >
> > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1815452
> >
> > Signed-off-by: Christian Ehrhardt
> > ---
> > src/security/virt-aa-helper.c | 20 +++-
> > 1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> > index 8e22e9978a..46c1914f58 100644
> > --- a/src/security/virt-aa-helper.c
> > +++ b/src/security/virt-aa-helper.c
> > @@ -937,7 +937,7 @@ get_files(vahControl * ctl)
> > size_t i;
> > char *uuid;
> > char uuidstr[VIR_UUID_STRING_BUFLEN];
> > -bool needsVfio = false, needsvhost = false;
> > +bool needsVfio = false, needsvhost = false, needsgl = false;
> >
> > /* verify uuid is same as what we were given on the command line */
> > virUUIDFormat(ctl->def->uuid, uuidstr);
> > @@ -1065,9 +1065,11 @@ get_files(vahControl * ctl)
> >
> > if (rendernode) {
> > vah_add_file(, rendernode, "rw");
> > +needsgl = true;
> > } else {
> > if (virDomainGraphicsNeedsAutoRenderNode(graphics)) {
> > char *defaultRenderNode = virHostGetDRMRenderNode();
> > +needsgl = true;
> >
> > if (defaultRenderNode) {
> > vah_add_file(, defaultRenderNode, "rw");
> > @@ -1267,6 +1269,22 @@ get_files(vahControl * ctl)
> > virBufferAddLit(, " \"/dev/vfio/vfio\" rw,\n");
> > virBufferAddLit(, " \"/dev/vfio/[0-9]*\" rw,\n");
> > }
> > +if (needsgl) {
> > +/* if using gl all sorts of further dri related paths will be
> > needed */
> > +virBufferAddLit(, " # DRI/Mesa/(e)GL config and driver
> > paths\n");
> > +virBufferAddLit(, " \"/usr/lib{,32,64}/dri/**\" mr,\n");
> > +virBufferAddLit(, " \"/usr/lib/@{multiarch}/dri/**\" mr,\n");
> > +virBufferAddLit(, " \"/usr/lib/fglrx/dri/**\" mr,\n");
> > +virBufferAddLit(, " \"/etc/drirc\" r,\n");
> > +virBufferAddLit(, " \"/usr/share/drirc.d/{,*.conf}\" r,\n");
> > +virBufferAddLit(, " \"/etc/glvnd/egl_vendor.d/{,*}\" r,\n");
> > +virBufferAddLit(, " \"/usr/share/glvnd/egl_vendor.d/{,*}\"
> > r,\n");
>
> The above rules are fine. It would be nice to me slightly more specific in the
> mmap rules to mmap only .so files, but not a blocker.
Ok, will do *.so for the mmaps in a V2 thanks for the hint
> > +virBufferAddLit(, " owner \"/var/lib/libvirt/.cache/\" w,\n");
>
> This doesn't seem to belong here and DAC is usually going to prevent it since
> VMs run as non-root and /var/lib/libvirt is 755. Perhaps get rid of owner and
> make this an explicit denial rule to silence the denial (with a code comment)?
I was just following the denials that I saw, I also couldn't fully see
the reason.
I can follow your DAC argument which is correct, and in addition every
instance would use the same cache which I'm not sure would be right.
I'll remove the line in a v2 and replace it with the suggested denial.
> > +virBufferAddLit(, " # Probe DRI device attributes\n");
> > +virBufferAddLit(, " \"/dev/dri/\" r,\n");
> > +virBufferAddLit(, "
> > \"/sys/devices/*/*/{uevent,vendor,device,subsystem_vendor,subsystem_device}\"
> > r,\n");
> > +virBufferAddLit(, "
> > \"/sys/devices/*/*/drm/*/{uevent,vendor,device,subsystem_vendor,subsystem_device}\"
> > r,\n");
>
> These are fine.
>
> --
> Jamie Strandboge | http://www.canonical.com
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] util: Introduce function for allocating virStorageSource
On Mon, Feb 18, 2019 at 10:16:25AM +0100, Peter Krempa wrote: > On Mon, Feb 18, 2019 at 08:29:14 +0100, Erik Skultety wrote: > > On Fri, Feb 15, 2019 at 01:42:09PM +0100, Peter Krempa wrote: > > > Add virStorageSourceNew and refactor places allocating that structure to > > > use the helper. > > > > > > Signed-off-by: Peter Krempa > > > --- > > Looks like you caught all the occurrences but 1 in bhyve: bhyveParsePCIDisk > > With that addressed too: > > Instead of changing that in this patch I propose we fix the bhyve parser > properly: > > https://www.redhat.com/archives/libvir-list/2019-February/msg00956.html Yep, makes sense. Thanks, Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] bhyve: use virDomainDiskDefNew to instead of VIR_ALLOC
On Mon, Feb 18, 2019 at 10:15:35AM +0100, Peter Krempa wrote: > Use the proper function to allocate a disk definition. > > Signed-off-by: Peter Krempa > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] util: Introduce function for allocating virStorageSource
On Mon, Feb 18, 2019 at 08:29:14 +0100, Erik Skultety wrote: > On Fri, Feb 15, 2019 at 01:42:09PM +0100, Peter Krempa wrote: > > Add virStorageSourceNew and refactor places allocating that structure to > > use the helper. > > > > Signed-off-by: Peter Krempa > > --- > Looks like you caught all the occurrences but 1 in bhyve: bhyveParsePCIDisk > With that addressed too: Instead of changing that in this patch I propose we fix the bhyve parser properly: https://www.redhat.com/archives/libvir-list/2019-February/msg00956.html signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] bhyve: use virDomainDiskDefNew to instead of VIR_ALLOC
Use the proper function to allocate a disk definition. Signed-off-by: Peter Krempa --- src/bhyve/bhyve_parse_command.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c index 1c9191fb96..bd93070dfb 100644 --- a/src/bhyve/bhyve_parse_command.c +++ b/src/bhyve/bhyve_parse_command.c @@ -430,10 +430,8 @@ bhyveParsePCIDisk(virDomainDefPtr def, int idx = -1; virDomainDiskDefPtr disk = NULL; -if (VIR_ALLOC(disk) < 0) +if (!(disk = virDomainDiskDefNew(NULL))) goto cleanup; -if (VIR_ALLOC(disk->src) < 0) -goto error; disk->bus = bus; disk->device = device; -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/7] conf: Rework virDomainDeviceDefPostParseCommon()
On Mon, 2019-02-18 at 09:46 +0100, Bjoern Walk wrote:
> Andrea Bolognani [2019-02-15, 12:55PM +0100]:
> > +case VIR_DOMAIN_DEVICE_NONE:
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("unexpected VIR_DOMAIN_DEVICE_NONE"));
>
> Can we find a better error message here?
Feel free to post a patch changing this (and the one in
qemuDomainDeviceDefPostParse() that I've copied it from :), but it's
one of those error messages that will only show up if we've made some
pretty serious mistake somewhere leading up to the function being
called, so I don't feel like it's particularly important for it to
be user friendly.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/7] conf: Rework virDomainDeviceDefPostParseCommon()
Andrea Bolognani [2019-02-15, 12:55PM +0100]:
> +case VIR_DOMAIN_DEVICE_NONE:
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("unexpected VIR_DOMAIN_DEVICE_NONE"));
Can we find a better error message here?
> +break;
> +
> +case VIR_DOMAIN_DEVICE_LAST:
> +default:
> +virReportEnumRangeError(virDomainDeviceType, dev->type);
> +break;
> +}
> +
> +return ret;
> }
>
>
> --
> 2.20.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
IBM Systems
Linux on Z & Virtualization Development
--
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
--
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] util: Replace virStorageSourceFree with virObjectUnref
On Fri, Feb 15, 2019 at 01:42:13PM +0100, Peter Krempa wrote: > Now that virStorageSource is a subclass of virObject we can use > virObjectUnref and remove virStorageSourceFree which was a thin wrapper. > > Signed-off-by: Peter Krempa > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] util: Remove the AUTOPTR func for virStorageSource
On Fri, Feb 15, 2019 at 01:42:12PM +0100, Peter Krempa wrote: > Since virStorageSource is now a subclass of virObject, we can use > VIR_AUTOUNREF instead. > > Signed-off-by: Peter Krempa > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] util: Remove the AUTOPTR func for virStorageSource
On Fri, Feb 15, 2019 at 01:42:12PM +0100, Peter Krempa wrote: > Since virStorageSource is now a subclass of virObject, we can use > VIR_AUTOUNREF instead. > > Signed-off-by: Peter Krempa > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] util: alloc: Introduce VIR_AUTOUNREF macro
On Fri, Feb 15, 2019 at 01:42:11PM +0100, Peter Krempa wrote: > Add helper for utilizing __attribute__(cleanup())) for unref-ing > instances of sublasses of virObject. > > Signed-off-by: Peter Krempa > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
