[libvirt] [PATCH v2] qemu: fix vcpupin fail as no vcpu tid set
vcpupin will fail when maxvcpus is larger than current
vcpu:
virsh vcpupin win7 --vcpu 0 --cpulist 5-6
error: Requested operation is not valid: cpu affinity is not supported
win7 xml in the command above is like below:
...
8
...
The reason is vcpu[3] and vcpu[4] have zero tids and should not been
compared as valid situation in qemuDomainRefreshVcpuInfo().
This issue is introduced by commit 34f7743, which fix recording of vCPU
pids for MTTCG.
Signed-off-by: Yi Wang
---
v2: stop considering zero TIDs as duplicate. Thanks to Jano.
---
src/qemu/qemu_domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ac01e86..c70d3f3 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10708,7 +10708,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
}
for (j = 0; j < i; j++) {
-if (info[i].tid == info[j].tid) {
+if (0 != info[i].tid && info[i].tid == info[j].tid) {
VIR_DEBUG("vCPU[%zu] PID %llu duplicates vCPU[%zu]",
i, (unsigned long long)info[i].tid, j);
validTIDs = false;
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix vcpupin fail as no vcpu tid set
Hi Jano,
Thanks for your reply.
> On Mon, Feb 18, 2019 at 05:21:31PM +0800, Wen Yang wrote:
> >From: Yi Wang
> >
> >vcpupin will fail when maxvcpus is larger than current
> >vcpu:
> >
...
>
> This for loop was introduced by:
> commit 34f77437da884f9cf0e2450f25f373b17cf527e2
> qemu: fix recording of vCPU pids for MTTCG
>
> which started recording TIDs for TCG as well.
> Skipping it for VIRT_QEMU reverts the benefits for multithreaded TCG,
> while leaving the loop broken for VIRT_KVM when not all vCPUs are
> enabled.
>
> The proper fix here would be to stop considering zero TIDs as duplicate.
Ok, I will send a v2 patch. Thanks.
>
> Jano
>
> > if (info[i].tid == vm->pid) {
> > VIR_DEBUG("vCPU[%zu] PID %llu duplicates process",
> > i, (unsigned long long)info[i].tid);
> >--
> >1.8.3.1
> >
> >--
> >libvir-list mailing list
> >libvir-list@redhat.com
>
> >https://www.redhat.com/mailman/listinfo/libvir-list
---
Best wishes
Yi Wang--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Fix snapshot redefine vs. domain state bug
On 2/23/19 3:00 PM, Eric Blake wrote:
> The existing qemu snapshot code has a slight bug: if the domain
> is currently pmsuspended, you can't use the _REDEFINE flag even
> though the current domain state should have no bearing on being
> able to recreate metadata state; and conversely, you can use the
> _REDEFINE flag to create snapshot metadata claiming to be
> pmsuspended as a bypass to the normal restrictions that you can't
> create an original qemu snapshot in that state (the restriction
> against pmsuspend is specific to qemu, rather than part of the
> driver-agnostic snapshot_conf code).
>
> Fix this by factoring out the snapshot validation into a helper
> routine (which will also be useful in a later patch adding bulk
> redefine), and by adjusting the state being checked to the one
> supplied by the snapshot XML for redefinition, vs. the current
> domain state for original creation. However, since snapshots
> have one additional state beyond virDomainState (namely, the
> "disk-snapshot" state), and given the way virDomainSnapshotState
> was defined as an extension enum on top of virDomainState, we
> lose out on gcc's ability to force type-based validation that
> all switch branches are covered.
Would it be worth adding some sort of compiler warning that
VIR_DOMAIN_SNAPSHOT_STATE_LAST must be VIR_DOMAIN_DISK_SNAPSHOT + 1.
That would cause future additions to virDomainSnapshotState to rework
the math - verify seems to be used for some places. Perhaps something
worthwhile for any other one of these overlaid structs.
>
> In the process, observe that the qemu code already rejects _LIVE
> with _REDEFINE, but that this rejection occurs after parsing, and
> with a rather confusing message. Hoist that particular exclusive
> flag check earlier, so it gets a better error message, and is not
> inadvertently skipped when bulk redefine is added later (as that
> addition will occur prior to parsing).
>
> Fixes the second problem mentioned in https://bugzilla.redhat.com/1680304
>
> Signed-off-by: Eric Blake
> ---
> src/qemu/qemu_driver.c | 117 +
> 1 file changed, 71 insertions(+), 46 deletions(-)
>
It seems there are multiple things going on, some code refactoring and
then a couple bug fixes. IDC if they're combined - separating for easier
backporting is always nice...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index fe1b7801e9..1f37107a20 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15669,6 +15669,70 @@
> qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
> }
>
>
> +/* Validate that a snapshot object does not violate any qemu-specific
> + * constraints. @state is virDomainState if flags implies creation, or
> + * virDomainSnapshotState if flags includes _REDEFINE (the latter
> + * enum is a superset of the former). */
> +static int
> +qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def, int state,
> + unsigned int flags)
> +{
> +/* reject snapshot names containing slashes or starting with dot as
> + * snapshot definitions are saved in files named by the snapshot name */
> +if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
> +if (strchr(def->name, '/')) {
> +virReportError(VIR_ERR_XML_DETAIL,
> + _("invalid snapshot name '%s': "
> + "name can't contain '/'"),
> + def->name);
> +return -1;
> +}
> +
> +if (def->name[0] == '.') {
> +virReportError(VIR_ERR_XML_DETAIL,
> + _("invalid snapshot name '%s': "
> + "name can't start with '.'"),
> + def->name);
> +return -1;
> +}
> +}
> +
> +/* allow snapshots only in certain states */
> +switch (state) {
> +/* valid states */
> +case VIR_DOMAIN_RUNNING:
> +case VIR_DOMAIN_PAUSED:
> +case VIR_DOMAIN_SHUTDOWN:
> +case VIR_DOMAIN_SHUTOFF:
> +case VIR_DOMAIN_CRASHED:
> +break;
> +
> +case VIR_DOMAIN_DISK_SNAPSHOT:
> +if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state
> %s"),
> + virDomainSnapshotStateTypeToString(state));
I assume this is part of the fix - is there perhaps a way or a desire to
distinguish between this and the invalid states below? That is a
slightly different message to more easily know what went wrong without
having to dig into the code.
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_PMSUSPENDED:
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("qemu doesn't support taking snapshots of "
> + "PMSUSPENDED guests"));
> +return -1;
> +
> +/* invalid states */
> +case
Re: [libvirt] [PATCH 1/2] snapshot: Permit redefine of offline external snapshot
On 2/23/19 3:00 PM, Eric Blake wrote: > Due to historical back-compat, 'virsh snapshot-create-as' favors > internal snapshots (but can't be used on domains with raw storage), > while 'virsh snapshot-create-as --disk-only) favors external s/)/' > snapshots. What's more, snapshots created with --disk-only while > the domain was running are marked as snapshot state 'disk-snapshot', > while snapshots created while the domain was offline are marked as > snapshot state 'shutdown' (a 'disk-snapshot' image might not be > quiescent, while a 'shutdown' snapshot always is). > > But this leads to some interesting problems: if we create a > --disk-only snapshot of an offline guest, and then immediately try > to 'virsh snapshot-create --redefine' using the resulting XML to > overwrite the existing snapashot in place, things silently succeed, > but 'virsh snapshot-create --redefine --disk-only' fails because > the snapshot state is not 'disk-only'. Worse, if we delete the > snapshot metadata first and then try to recreate things, omitting > --disk-only fails because the verification code wants to force the > default of an internal snapshot (which doesn't work with raw disks), > and using --disk-only fails because the verification code doesn't > see the 'disk-only' state in the snapshot xml - making it impossible > to recreate the snapshot metadata. Ideally, the presence or absence > of the --disk-only flag, and the presence or absence of an existing > snapshot being overwritten, shouldn't matter; if the XML is valid > for one situation, it should always be valid to redefine the > metadata for that snapshot. > > Fix things by uniformly declaring that a redefined snapshot in the > 'shutdown' state can permit a disk-only external snapshot (we got > it right in only one out of three places in the function). > > See also https://bugzilla.redhat.com/1680304; this fixes the > domain-agnostic problems mentioned there, but another patch is needed > to fix further oddities with the qemu driver. I did not check for > sure, but this has probably been broken even prior to when commit > 670e86bf (1.1.4) factored redefine prep out of qemu code into the > common snapshot_conf code. > Perhaps 709b0f37c (1.0.2)? or older e260e401a5 (1.0.0) The 1.0.2 change seems to be the source of hunk 2 and 3 below, while 1.0.0 is the source of hunk 1. > Signed-off-by: Eric Blake > --- > src/conf/snapshot_conf.c | 11 +-- > 1 file changed, 5 insertions(+), 6 deletions(-) > Your explanation seems reasonable and although I don't think I could begin to (re) explain all the various states, conditions, [in]consistencies, etc... Still, consider it Reviewed-by: John Ferlan John [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Entering freeze for libvirt-5.1.0
As planned I just tagged it in git, and pushed the signed tarball and rpms to the usual place: ftp://libvirt.org/libvirt/ I'm travelling this week so my testing is even more limited than usual, but that seems fine with me and https://ci.centos.org/view/libvirt/ seems to be all green so I'm hopeful :-) I will push rc2 on Wednesday, and possibly GA on Friday if everything looks fine, worse case I will push it over the week-end. Please give it some testing, thanks ! Daniel -- Daniel Veillard | Red Hat Developers Tools http://developer.redhat.com/ veill...@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 00/10] dirty-bitmaps: deprecate @status field
On 2/22/19 7:06 PM, John Snow wrote: > The current internal meanings of "locked", "user_locked", > "qmp_locked", "frozen", "enabled", and "disabled" are all > a little muddled. > > Deprecate the @status field in favor of two new booleans > that carry very specific meanings. Then, rename and rework > some of the internal semantics to help make the API a bit > more clear and easier to read. > > Well, in my opinion. > > Based on my current bitmaps branch. > > V3: > > [] : patches are identical > [] : number of functional differences between upstream/downstream patch > [down] : patch is downstream-only > The flags [FC] indicate (F)unctional and (C)ontextual differences, > respectively > > 001/10:[0002] [FC] 'block/dirty-bitmap: add recording and busy properties' > 002/10:[0002] [FC] 'block/dirty-bitmaps: rename frozen predicate helper' > 003/10:[down] 'block/dirty-bitmap: remove set/reset assertions against > enabled bit' > 004/10:[0006] [FC] 'block/dirty-bitmap: change semantics of enabled predicate' > 005/10:[down] 'nbd: change error checking order for bitmaps' > 006/10:[0002] [FC] 'block/dirty-bitmap: explicitly lock bitmaps with > successors' > 007/10:[0011] [FC] 'block/dirty-bitmaps: unify qmp_locked and user_locked > calls' > 008/10:[0002] [FC] 'block/dirty-bitmaps: move comment block' > 009/10:[down] 'blockdev: remove unused paio parameter documentation' > 010/10:[down] 'iotests: add busy/recording bit test to 124' > > V3: (Following Vladimir's feedback) > > 001: Changed texi phrasing, parameter --> field > 002: Commit message adjustment > comment edit: "not locked" --> "not user_locked" > 003: pulled forward out of patch 004 > 004: Commit message rewrite > create_successor and abdicate doc adjustments > 005: New. > 006: Change successor doc comment > Commit message adjustment > 007: BdrvDirtyBitmap struct comment adjustments > Comment change to create_successor (lock --> busy) > Incidental changes from 004's changes > 008: Grammar fix (Eric) > 009: New, trivial, unrelated fix tagging along. > 010: iotest 124 added. > > John Snow (10): > block/dirty-bitmap: add recording and busy properties > block/dirty-bitmaps: rename frozen predicate helper > block/dirty-bitmap: remove set/reset assertions against enabled bit > block/dirty-bitmap: change semantics of enabled predicate > nbd: change error checking order for bitmaps > block/dirty-bitmap: explicitly lock bitmaps with successors > block/dirty-bitmaps: unify qmp_locked and user_locked calls > block/dirty-bitmaps: move comment block > blockdev: remove unused paio parameter documentation > iotests: add busy/recording bit test to 124 > > block/dirty-bitmap.c | 111 ++--- > blockdev.c | 19 +++--- > include/block/dirty-bitmap.h | 7 +-- > migration/block-dirty-bitmap.c | 8 +-- > nbd/server.c | 14 ++--- > qapi/block-core.json | 10 ++- > qemu-deprecated.texi | 6 ++ > tests/qemu-iotests/124 | 110 > tests/qemu-iotests/124.out | 4 +- > tests/qemu-iotests/236.out | 28 + > 10 files changed, 239 insertions(+), 78 deletions(-) > Pushed to my bitmaps branch with the following minor changes: 001/10:[0002] [FC] 'block/dirty-bitmap: add recording and busy properties' 002/10:[0002] [FC] 'block/dirty-bitmaps: rename frozen predicate helper' 003/10:[] [--] 'block/dirty-bitmap: remove set/reset assertions against enabled bit' 004/10:[] [--] 'block/dirty-bitmap: change semantics of enabled predicate' 005/10:[] [--] 'nbd: change error checking order for bitmaps' 006/10:[] [--] 'block/dirty-bitmap: explicitly lock bitmaps with successors' 007/10:[0002] [FC] 'block/dirty-bitmaps: unify qmp_locked and user_locked calls' 008/10:[] [--] 'block/dirty-bitmaps: move comment block' 009/10:[] [--] 'blockdev: remove unused paio parameter documentation' 010/10:[0003] [FC] 'iotests: add busy/recording bit test to 124' 001: Texi doc spelling (Eric) 002: Commit message spelling (Eric) Additional asserts (Vladimir) 007: Comment phrasing (Vladimir) 010: Extra test comment -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 13/20] backup: Implement virsh support for checkpoints
On 2/12/19 1:15 PM, John Ferlan wrote:
>
>
> On 2/6/19 2:18 PM, Eric Blake wrote:
>> Introduce a bunch of new virsh commands for managing checkpoints
>> in isolation. More commands are needed for performing incremental
>> backups, but these commands were easy to implement by modeling
>> heavily after virsh-snapshot.c (no need for checkpoint-revert,
>> and checkpoint-list was a lot easier since we don't have to cater
>> to older libvirt API).
>>
>> Signed-off-by: Eric Blake
>> ---
>> tools/virsh-checkpoint.h | 29 +
>> tools/virsh-completer.h |4 +
>> tools/virsh-util.h |3 +
>> tools/virsh.h|1 +
>> po/POTFILES |1 +
>> tools/Makefile.am|3 +-
>> tools/virsh-checkpoint.c | 1329 ++
>> tools/virsh-completer.c | 52 +-
>> tools/virsh-util.c | 11 +
>> tools/virsh.c|2 +
>> 10 files changed, 1433 insertions(+), 2 deletions(-)
>> create mode 100644 tools/virsh-checkpoint.h
>> create mode 100644 tools/virsh-checkpoint.c
>
> virsh.pod would be useful in order to figure out what everything means.
>
Gah. I completely forgot to copy-and-paste that portion of the snapshot
code. Will rectify.
> This is just too much being added here to really give this a proper
> review. This really is a case where too much has been built up. Start
> with the basics and build up from there.
>
> I have similar things as previously w/r/t 2 blank lines between
> functions and one argument per line per method - all newer norms than
> when -snapshot was created.
>
> Also may as well use the VIR_AUTOFREE type functions for (char *)
> VIR_FREE's. And VIR_AUTOPTR(virString) as approriate
Yep, on my list of things to clean up where I spot them, to avoid
regressing to the older state-of-the-art when I first copied this code
from snapshots.
>> +static bool
>> +virshCheckpointCreate(vshControl *ctl, virDomainPtr dom, const char *buffer,
>> + unsigned int flags, const char *from)
>> +{
>> +bool ret = false;
>> +virDomainCheckpointPtr checkpoint;
>> +const char *name = NULL;
>> +
>> +checkpoint = virDomainCheckpointCreateXML(dom, buffer, flags);
>> +
>> +if (checkpoint == NULL)
>> +goto cleanup;
>> +
>> +name = virDomainCheckpointGetName(checkpoint);
>> +if (!name) {
>> +vshError(ctl, "%s", _("Could not get snapshot name"));
>
> Could not find domain checkpoint '%s'
>
> would be better
Particularly since it's not a snapshot. (Too much copy-and-paste...)
>> +/* TODO - worth adding this flag?
>> +{.name = "quiesce",
>> + .type = VSH_OT_BOOL,
>> + .help = N_("quiesce guest's file systems")
>> +},
>> +*/
>
> More to do
At least adding it here is easy. Adding it in qemu is harder.
>> +/*
>> + * "checkpoint-edit" command
>> + */
>
> Oh and this is *really* a scary thing to allow! Especially since IIRC
> the Assign function would just create a new one if a name didn't
> exist... There seems to be a few things that could happen without locks
> that could really mess things up.
Modeled after "snapshot-edit" - but yes, it is a scary-enough command
that splitting it to a separate commit doesn't hurt my feelings (nor
hold up the initial API).
>> +static bool
>> +cmdCheckpointCurrent(vshControl *ctl, const vshCmd *cmd)
>> +{
>> +virDomainPtr dom = NULL;
>> +bool ret = false;
>> +int current;
>> +virDomainCheckpointPtr checkpoint = NULL;
>> +char *xml = NULL;
>> +const char *checkpointname = NULL;
>> +unsigned int flags = 0;
>> +const char *domname;
>> +
>> +if (vshCommandOptBool(cmd, "security-info"))
>> +flags |= VIR_DOMAIN_CHECKPOINT_XML_SECURE;
>> +if (vshCommandOptBool(cmd, "no-domain"))
>> +flags |= VIR_DOMAIN_CHECKPOINT_XML_NO_DOMAIN;
>> +if (vshCommandOptBool(cmd, "size"))
>> +flags |= VIR_DOMAIN_CHECKPOINT_XML_SIZE;
>> +
>> +VSH_EXCLUSIVE_OPTIONS("name", "checkpointname");
>> +
>> +if (!(dom = virshCommandOptDomain(ctl, cmd, )))
>> +return false;
>> +
>> +if (vshCommandOptStringReq(ctl, cmd, "checkpointname", )
>> < 0)
>> +goto cleanup;
>> +
>> +if (checkpointname) {
>> +virDomainCheckpointPtr checkpoint2 = NULL;
>> +flags = (VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE |
>> + VIR_DOMAIN_CHECKPOINT_CREATE_CURRENT);
>> +
>> +if (!(checkpoint = virDomainCheckpointLookupByName(dom,
>> + checkpointname,
>> 0)))
>> +goto cleanup;
>> +
>> +xml = virDomainCheckpointGetXMLDesc(checkpoint,
>> +
>> VIR_DOMAIN_CHECKPOINT_XML_SECURE);
>> +if (!xml)
>> +goto cleanup;
>> +
>> +if (!(checkpoint2 = virDomainCheckpointCreateXML(dom, xml, flags)))
>> +goto cleanup;
>
> I need a better map. When creating a checkpoint a couple patches
Re: [libvirt] [PATCH v4 12/20] wip: backup: Parse and output backup XML
On 2/12/19 12:08 PM, John Ferlan wrote:
>
>
> On 2/6/19 2:18 PM, Eric Blake wrote:
>> Accept XML describing a generic block job, and output it again
>> as needed. At the moment, it has some qemu-specific hacks, such
>> as storing internal XML for a node name, that might be cleaner
>> once full-tree node-name support goes in.
>>
>> Still not done: decent tests
>>
>> Signed-off-by: Eric Blake
>> ---
>> src/conf/checkpoint_conf.h | 65 +
>> src/conf/domain_conf.h | 3 +
>> src/conf/checkpoint_conf.c | 503 +
>> src/libvirt_private.syms | 8 +-
>> 4 files changed, 578 insertions(+), 1 deletion(-)
>>
>
> I think this should be it's own module src/conf/backup_conf.{c,h}
That one makes sense to me. Even if the public API lives directly in
libvirt-domain.c (instead of my current placement in
libvirt-domain-checkpoint.c), the backend for this XML is sufficient
enough for its own file. Will split.
>> +
>> +/* Needed? A way for users to list a disk and explicitly mark it
>> + * as not participating, and then output shows all disks rather
>> + * than just active disks */
>> +#if 0
>> +backup = virXMLPropString(node, "backup");
>> +if (backup) {
>> +def->type = virDomainCheckpointTypeFromString(checkpoint);
>> +if (def->type <= 0) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("unknown disk checkpoint setting '%s'"),
>> + checkpoint);
>> +goto cleanup;
>> +}
>> +}
>> +#endif
>
> Still need to decide...
Yes, I do.
>
>> +
>> +if ((type = virXMLPropString(node, "type"))) {
>> +if ((def->store->type = virStorageTypeFromString(type)) <= 0 ||
>> +def->store->type == VIR_STORAGE_TYPE_VOLUME ||
>> +def->store->type == VIR_STORAGE_TYPE_DIR) {
>> +virReportError(VIR_ERR_XML_ERROR,
>> + _("unknown disk backup type '%s'"), type);
>> +goto cleanup;
>> +}
>> +} else {
>> +def->store->type = VIR_STORAGE_TYPE_FILE;
>> +}
>> +
>> +if ((cur = virXPathNode(push ? "./target" : "./scratch", ctxt)) &&
>> +virDomainDiskSourceParse(cur, ctxt, def->store, 0, xmlopt) < 0)
>> +goto cleanup;
>> +
>> +if (internal) {
>
> Is this another way of using FLAGS to determine the parse mode? IOW, is
> this only needed when parsing the status/running XML.
>
Yes, it is for internal-use XML, and I already know I need to fix things
to use a single 'flags' argument with sane enum values (matching what I
already cleaned up for snapshots).
>> +
>> +if (flags & VIR_DOMAIN_BACKUP_PARSE_INTERNAL) {
>> +char *tmp = virXMLPropString(ctxt->node, "id");
>
> You can use VIR_AUTOFREE(char *) tmp too...
>
> Ahh... so this is where it's referenced... I recall this from RNG...
Which also raises the question of whether the RNG has to call it out, if
the only thing using it is internal code, or if the user will ever see
it. We have the interesting problem of outputting some things as
user-visible which are output-only, but then having to validate them in
the RNG on reparse even if we are going to ignore them, so that the user
doesn't have to trim them out. I'll have to double-check my intent
here, and will leave an appropriate comment (probably along the lines of
"id is valid in output and hence part of the RNG, but ignored on input
except when parsing internal XML for reconstructing job state across
libvirtd restarts").
>> +if (node) {
>> +if (def->type != VIR_DOMAIN_BACKUP_TYPE_PULL) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("use of requires pull mode backup"));
>> +goto cleanup;
>> +}
>> +if (VIR_ALLOC(def->server) < 0)
>> +goto cleanup;
>> +if (virDomainStorageNetworkParseHost(node, def->server) < 0)
>> +goto cleanup;
>> +if (def->server->transport == VIR_STORAGE_NET_HOST_TRANS_RDMA) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("transport rdma is not supported for
>> "));
>
> If transport == FOO is ever added and not supported, then you're in
> trouble... Go with
>
> if (def->server->transport != VIR_STORAGE_NET_HOST_TRANS_...)
> {TCP|UNIX}
>
> and use the virStorageHostNetTransportTypeToString to translate what is
> not supported...
Good call.
>> +static int
>> +virDomainBackupDiskDefFormat(virBufferPtr buf,
>> + virDomainBackupDiskDefPtr disk,
>> + bool push, bool internal)
>
> one arg per line
>
> Should @internal make use of flags instead?
Yes, especially since I just fixed that in snapshots.
>
>> +{
>> +int type = disk->store->type;
>> +virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
>> +virBuffer childBuf = VIR_BUFFER_INITIALIZER;
>> +int ret
Re: [libvirt] [PATCH v4 11/20] wip: backup: Parse and output checkpoint XML
On 2/12/19 11:23 AM, John Ferlan wrote:
>
>
> On 2/6/19 2:18 PM, Eric Blake wrote:
>> Work in progress - the checkpoint code is not quite passing
>> tests (part of that is figuring out the minimal XML that is
>> still valid as a element, or just use --no-domain flag).
>>
>> Signed-off-by: Eric Blake
>> ---
>> src/conf/checkpoint_conf.h | 150
>> src/conf/domain_conf.h | 11 +-
>> po/POTFILES |1 +
>> src/conf/Makefile.inc.am|2 +
>> src/conf/checkpoint_conf.c | 1030 +++
>> src/conf/domain_conf.c |7 +-
>> src/libvirt_private.syms| 21 +
>> tests/Makefile.am |9 +-
>> tests/domaincheckpointxml2xmltest.c | 231 ++
>> 9 files changed, 1458 insertions(+), 4 deletions(-)
>> create mode 100644 src/conf/checkpoint_conf.h
>> create mode 100644 src/conf/checkpoint_conf.c
>> create mode 100644 tests/domaincheckpointxml2xmltest.c
>>
>
> Starting to lose some steam - seeing wip means I don't want to spend too
> much time on some algorithms for fear they'll change, but then again I
> know you're looking for feedback...
I understand. The 'wip' tags should be gone for v5.
>
>> diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h
>> new file mode 100644
>> index 00..994a8bd083
>> --- /dev/null
>> +++ b/src/conf/checkpoint_conf.h
>> @@ -0,0 +1,150 @@
>> +/*
>> + * checkpoint_conf.h: domain checkpoint XML processing
>> + *
>> + * Copyright (C) 2006-2019 Red Hat, Inc.
>> + * Copyright (C) 2006-2008 Daniel P. Berrange
>
> This is new, right? Not a copy of...
New file, but so heavily copied from snapshot_conf.h that I felt safer
preserving all existing copyrights.
>> +struct _virDomainCheckpointDef {
>> +/* Public XML. */
>> +char *name;
>> +char *description;
>> +char *parent;
>> +long long creationTime; /* in seconds */
>> +
>> +size_t ndisks; /* should not exceed dom->ndisks */
>> +virDomainCheckpointDiskDef *disks;
>> +
>> +virDomainDefPtr dom;
>> +
>> +/* Internal use. */
>> +bool current; /* At most one checkpoint in the list should have this
>> set */
>
> Typically these are then in the *Obj...
Copy-and-paste from virDomainSnapshotDef, but my recent work there has
made me want to reconsider where the current object is tracked.
The problem is that the current way we store things on disk is via a
separate XML per snapshot, with no other obvious place
for the internal representation of a to track which snapshot is
current. BUT, as I have just posted patches to add a bulk dump/redefine
with VIR_DOMAIN_XML_SNAPSHOTS, we COULD just ditch our current
separate-file storage of snapshots, and instead store the snapshots
directly with the (for all new saves; when loading libvirtd,
we'd still have to keep the code to load from older separate snapshot
files for back-compat reasons, even if we no longer track separate files
after the one-time conversion). And if I make snapshots cleaner like
that, then checkpoints can just start life with the saner approach,
rather than being stuck with copying the older one-file-per-checkpoint
approach that I implemented using copy-and-paste.
>
>> +};
>> +
>> +struct _virDomainCheckpointObj {
>> +virDomainCheckpointDefPtr def; /* non-NULL except for metaroot */
>> +
>> +virDomainCheckpointObjPtr parent; /* non-NULL except for metaroot,
>> before
>> +
>> virDomainCheckpointUpdateRelations, or
>> + after
>> virDomainCheckpointDropParent */
>> +virDomainCheckpointObjPtr sibling; /* NULL if last child of parent */
>> +size_t nchildren;
>> +virDomainCheckpointObjPtr first_child; /* NULL if no children */
>
> The whole relationship thing is I think overly complex and a bit
> difficult to follow. Having @sibling and @first_child seems to make the
> code even more complex. I would think you have a parent and maybe a
> child. If this is the "first" checkpoint, then there is no parent. I
> would think that means it's the root.
>
> There's just much more to be see when it comes to how these
> relationships play out as checkpoints are made and removed. I guess I
> just have a very linear model in mind, but reading the code seems to go
> beyond linearality.
>
> The use of hash tables just makes it easier to ensure no def->name is
> reused. Not having locks in the object means some parent lock is
> managing to make sure two checkpoint operations cannot occur at the same
> time from different threads (e.g. a domain object lock), but that is not
> 100% clear.
Again, heavy copy-and-paste from snapshots, which DO lend themselves to
non-linear DAGs of related snapshots. (If you create snapshot A, then
run the guest, then create B, then revert to A, then run the guest and
create C, then both B and C are children of A). I don't readily know if
checkpoints
Re: [libvirt] [PATCH v3 07/10] block/dirty-bitmaps: unify qmp_locked and user_locked calls
On 2/25/19 7:03 AM, Vladimir Sementsov-Ogievskiy wrote: > 23.02.2019 3:06, 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 > > Hmm, and here, you directly fixed what I've noticed, so my r-b, > of course, still applies. > Sorry for not adding them. I didn't plan to merge the series until you got a chance to review it, so I wasn't worried about missing it in the long run. Sorry if it makes it harder to see which ones you'd like to look at. > Ha, but I noticed funny thing: > >> --- >> block/dirty-bitmap.c | 40 +++--- >> blockdev.c | 18 +++ >> include/block/dirty-bitmap.h | 5 ++--- >> migration/block-dirty-bitmap.c | 6 ++--- >> nbd/server.c | 6 ++--- >> 5 files changed, 34 insertions(+), 41 deletions(-) >> >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index d92a269753..b3627b0d8c 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c > > [..] > >> @@ -265,9 +259,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState >> *bs, >> child->disabled = bitmap->disabled; >> bitmap->disabled = true; >> >> -/* Install the successor and lock the parent */ >> +/* Install the successor and mark the parent as busy */ > > I asked you to fix comment for bdrv_dirty_bitmap_create_successor(), but I > didn't > noticed this one, I meant the comment to the whole function, placed above it, > which > now have > "Requires that the bitmap is not user_locked and has no successor." > > So, it should be updated too. > > and with it: > Reviewed-by: Vladimir Sementsov-Ogievskiy > Ha. OK, updated to "is not marked busy" Thanks for the reviews! >> bitmap->successor = child; >> -bitmap->qmp_locked = true; >> +bitmap->busy = true; >> return 0; >> } >> > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/10] iotests: add busy/recording bit test to 124
On 2/23/19 5:06 PM, Eric Blake wrote:
> On 2/22/19 6:06 PM, John Snow wrote:
>> This adds a simple test that ensures the busy bit works for push backups,
>> as well as doubling as bonus test for incremental backups that get
>> interrupted
>> by EIO errors.
>
> This makes 124 longer to run, but it is not in the 'quick' group, so
> that's okay.
>
> The easy part: I compiled the series, and validated that ./check still
> passes with this applied, so:
>
> Tested-by: Eric Blake
>
> Now for the fun part (I know from IRC that you had some interesting
> challenges coming up with scenarios to even provoke the states you
> wanted shown in the output):
>
>>
>> Recording bit tests are already handled sufficiently by 236.
>>
>> Signed-off-by: John Snow
>> ---
>> tests/qemu-iotests/124 | 110 +
>> tests/qemu-iotests/124.out | 4 +-
>> 2 files changed, 112 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
>> index 5aa1bf1bd6..30f12a2202 100755
>> --- a/tests/qemu-iotests/124
>> +++ b/tests/qemu-iotests/124
>> @@ -634,6 +634,116 @@ class
>> TestIncrementalBackupBlkdebug(TestIncrementalBackupBase):
>> self.vm.shutdown()
>> self.check_backups()
>>
>> +def test_incremental_pause(self):
>> +"""
>> +Test an incremental backup that errors into a pause and is resumed.
>> +"""
>> +
>> +drive0 = self.drives[0]
>> +result = self.vm.qmp('blockdev-add',
>> + node_name=drive0['id'],
>> + driver=drive0['fmt'],
>> + file={
>> + 'driver': 'blkdebug',
>> + 'image': {
>> + 'driver': 'file',
>> + 'filename': drive0['file']
>> + },
>> + 'set-state': [{
>> + 'event': 'flush_to_disk',
>> + 'state': 1,
>> + 'new_state': 2
>> + },{
>> + 'event': 'read_aio',
>> + 'state': 2,
>> + 'new_state': 3
>> + }],
>> + 'inject-error': [{
>> + 'event': 'read_aio',
>> + 'errno': 5,
>> + 'state': 3,
>> + 'immediately': False,
>> + 'once': True
>> + }],
>
> Yeah, it's hairy to come up with sequences that will pause at the right
> place, and this may be sensitive enough that we have to revisit it in
> the future, but for now it works and I don't have any better suggestions.
>
The problem in this case was that the drive_add itself provokes a
read_aio. I didn't dig too far to see why, but I needed an extra
read_aio state in the sequence there so that the error would occur
during the actual backup job.
I think I'll document this in the test, with at least a comment.
I wonder if there's a syntactical sugar we could write that makes this
easier to read, like:
'inject-error': [{ 'event': 'flush_to_disk->read_aio->read_aio' }] or
some such, but that's probably at odds with the existing interface and
not worth my time to go on a crusade about.
>> + })
>> +self.assert_qmp(result, 'return', {})
>> +self.create_anchor_backup(drive0)
>> +bitmap = self.add_bitmap('bitmap0', drive0)
>> +
>> +# Emulate guest activity
>> +self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
>> + ('0xfe', '16M', '256k'),
>> + ('0x64', '32736k', '64k')))
>> +
>> +# For the purposes of query-block visibility of bitmaps, add a drive
>> +# frontend after we've written data; otherwise we can't use hmp-io
>> +result = self.vm.qmp("device_add",
>> + id="device0",
>> + drive=drive0['id'],
>> + driver="virtio-blk")
>> +self.assert_qmp(result, 'return', {})
>> +
>> +# Bitmap Status Check
>> +query = self.vm.qmp('query-block')
>> +ret = [bmap for bmap in query['return'][0]['dirty-bitmaps']
>> + if bmap.get('name') == bitmap.name][0]
>> +self.assert_qmp(ret, 'count', 458752)
>> +self.assert_qmp(ret, 'granularity', 65536)
>> +self.assert_qmp(ret, 'status', 'active')
>> +self.assert_qmp(ret, 'busy', False)
>> +self.assert_qmp(ret, 'recording', True)
>
> So far, nothing too different from we've seen elsewhere,
Re: [libvirt] [PATCH v4 06/20] backup: Document new XML for backups
On 2/9/19 9:56 AM, John Ferlan wrote: >> + > > I haven't looked ahead to conf processing, but I assume this is the same > type as whatever this gets read into. Whenever I see size in reference > to disks I'm thinking ULL's. It comes from docs/schemas/basictypes.rng. We don't document it very well, but in the RNG, 'int' is 32-bit, 'unsignedLong' is 64-bit, and we don't really do much to enforce things. Your question in later patches about using 'unsigned long long' C type to match 'unsignedLong' in the RNG is not unexpected, but this is not the first time we've had that sort of mismatch in naming (as the 'unsignedLong' as 64-bit type is a common idiom in the RNG). >> +++ b/tests/domainbackupxml2xmlin/empty.xml >> @@ -0,0 +1 @@ >> + > > Should add a 'block' and 'network' example too for completeness - of > course that depends on the matching between docs and rng. > > Should add target w/ shallow=on for completeness. > > Yes, I recall the commit message indicating essentially minimal. Yes, I need to beef up my examples, and make sure the testsuite passes. >> +++ b/tests/domaincheckpointxml2xmlout/empty.xml >> @@ -0,0 +1,10 @@ >> + >> + 1525889631 >> + 1525889631 >> + >> + >> + >> + >> +9d37b878-a7cc-9f9a-b78f-49b3abad25a8 > > This looks strange without the name... and the domain type like the > sample.xml output has Here's part of my reason for still having 'wip' on later patches - the snapshot code USED to just take a single UUID, until we released a couple of releases later that you can't revert to a domain state unless you capture the entire at the point in time you plan to revert to (as hot-plug actions after that time must be rolled back for the reversion to be correct). So the snapshot code has a horrible hack of taking either a UUID or a full sub-element. I don't want to repeat that hack in - I want a full element every time. But coming up with the absolute minimum that does not make the test balloon into a super-long demonstration while still passing the RNG as a valid XML was where I got stuck, especially since the user does NOT pass in a sublement except when using the _REDEFINE flag (the rest of the time, the sublement is computed at creation time from the virDomainPtr that is gaining a new checkpoint). >> +++ b/tests/domaincheckpointxml2xmlout/sample.xml >> @@ -0,0 +1,16 @@ >> + >> + 1525889631 >> + Completion of updates after OS install >> + 1525889631 >> + >> +1525111885 >> + >> + >> + > > Maybe this one should show the size attribute too... No, I want size to be an output-only element, and one you have to pass in an additional flag to get (because it requires runtime computation, rather than a static output). Oh, and that reminds me of another long-standing yak hair - we introduced the ability to validate some of our XML against the RNG, but we have not finished wiring it up to all of the APIs that take XML. Domain snapshots don't have a validation flag, and hence my copy-and-paste code for checkpoints don't have one, but both need one. (Otherwise, the presence or absence of an ignored element won't really matter whether the RNG permits or rejects it) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 02/10] block/dirty-bitmaps: rename frozen predicate helper
On 2/25/19 2:01 AM, Vladimir Sementsov-Ogievskiy wrote:
> 23.02.2019 3:06, 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 bdrv_enable_dirty_bitmap_locked and
>> bdrv_disable_dirty_bitmap_locked, it doesn't make sense to prohibit QEMU
>> internals from performing this action when we only wished to prohibit QMP
>> users from issuing these commands. All of the QMP API commands for bitmap
>> manipulation already check against user_locked() to prohibit these actions.
>>
>> Several other assertions really want to check that the bitmap isn't in-use
>> by another operation -- use the bitmap_user_locked function for this instead,
>> which presently also checks for has_successor. This leaves some redundant
>> checks of has_sucessor through different helpers that are addressed in
>> forthcoming patches.
>>
>> 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..aa3f86bb73 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>
> [..]
>
>> @@ -285,7 +288,7 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap
>> *bitmap)
>> static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
>> {
>> assert(!bitmap->active_iterators);
>> -assert(!bdrv_dirty_bitmap_frozen(bitmap));
>> +assert(!bdrv_dirty_bitmap_user_locked(bitmap));
>
> hmm. I'd prefere to keep a separate assertion for successor: we don't free it
> here,
> so we really depend on absence of this object.
>
Reasonable.
>> assert(!bitmap->meta);
>> QLIST_REMOVE(bitmap, list);
>> hbitmap_free(bitmap->bitmap);
>> @@ -325,7 +328,7 @@ BdrvDirtyBitmap
>> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>> /**
>>* In cases of failure where we can no longer safely delete the parent,
>>* we may wish to re-join the parent and child/successor.
>> - * The merged parent will be un-frozen, but not explicitly re-enabled.
>> + * The merged parent will not be user_locked, nor explicitly re-enabled.
>>* Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
>>*/
>> BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>> @@ -373,7 +376,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs,
>> int64_t bytes)
>>
>> bdrv_dirty_bitmaps_lock(bs);
>> QLIST_FOREACH(bitmap, >dirty_bitmaps, list) {
>> -assert(!bdrv_dirty_bitmap_frozen(bitmap));
>> +assert(!bdrv_dirty_bitmap_user_locked(bitmap));
>
> and here too. As we don't truncate successors.
>
>> assert(!bitmap->active_iterators);
>> hbitmap_truncate(bitmap->bitmap, bytes);
>> bitmap->size = bytes;
>> @@ -391,7 +394,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs,
>> BdrvDirtyBitmap *bitmap)
>>
>> /**
>>* Release all named dirty bitmaps attached to a BDS (for use in
>> bdrv_close()).
>> - * There must not be any frozen bitmaps attached.
>> + * There must not be any user_locked bitmaps attached.
>>* This function does not remove persistent bitmaps from the storage.
>>* Called with BQL taken.
>>*/
>> @@ -428,7 +431,6 @@ void
>> bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>> {
>> bdrv_dirty_bitmap_lock(bitmap);
>> -assert(!bdrv_dirty_bitmap_frozen(bitmap));
>> bitmap->disabled = true;
>> bdrv_dirty_bitmap_unlock(bitmap);
>> }
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index 04a117fc81..cdbb4dfefd 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>
> Could you please add scripts/git.orderfile to your .git/config, like
>
> [diff]
> orderFile = /path/to/scripts/git.orderfile
>
> ?
>
Hm, we should add this to the .gitpublish profile, but yes, let me
re-add this.
>> @@ -36,7 +36,7 @@ BlockDirtyInfoList
>> *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
>> uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
>> uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
>> bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
>> -bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
>> +bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap);
>> const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
>> int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
>> DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
>> diff
Re: [libvirt] [PATCH v4 08/20] backup: Introduce virDomainBackup APIs
On 2/11/19 3:21 PM, John Ferlan wrote:
>
>
> On 2/6/19 2:18 PM, Eric Blake wrote:
>> Introduce a few more new public APIs related to incremental backups.
>> This builds on the previous notion of a checkpoint (without an
>> existing checkpoint, the new API is a full backup, differing only
>> from virDomainCopy in the point of time chosen); and also allows
>> creation of a new checkpoint at the same time as starting the backup
>> (after all, an incremental backup is only useful if it covers the
>> state since the previous backup). It also enhances event reporting
>> for signaling when a push model backup completes (where the
>> hypervisor creates the backup); note that the pull model does not
>> have an event (starting the backup lets a third party access the
>> data, and only the third party knows when it is finished).
>>
>> The full list of new API:
>> virDomainBackupBegin;
>> virDomainBackupEnd;
>> virDomainBackupGetXMLDesc;
>>
>> +typedef enum {
>> +VIR_DOMAIN_BACKUP_BEGIN_NO_METADATA = (1 << 0), /* Make checkpoint
>> without
>> + remembering it */
>> +/* TODO: VIR_DOMAIN_BACKUP_BEGIN_QUIESCE */
>
> Implement or drop TODO:
Indeed. And if I get it working for virDomainCheckpointCreateXML, it's
trivially supported here as well.
>> +++ b/src/libvirt-domain-checkpoint.c
>> @@ -721,3 +721,205 @@ virDomainCheckpointFree(virDomainCheckpointPtr
>> checkpoint)
>> virObjectUnref(checkpoint);
>> return 0;
>> }
>
> Dirtying the namespace of libvirt-domain-checkpoint w/ Backup. Why not a
> separate libvirt-domain-backup - beyond the obvious need to alter more
> stuff of course. If kept here would seem to need to alter line #2 way
> back up at the top to include virDomainBackupPtr API's too.
Or even place the backup APIs directly in libvirt-domain.c, as they are
domain-based operations with no separate object being created, and are
more similar to other job-based APIs like virDomainMigrate().
>> + * This operation returns quickly, such that a user can choose to
>> + * start a backup job between virDomainFSFreeze() and
>> + * virDomainFSThaw() in order to create the backup while guest I/O is
>> + * quiesced.
>> + */
>> +/* FIXME: Do we need a specific API for listing all current backup
>> + * jobs (which, at the moment, is at most one job), or is it better to
>> + * refactor other existing job APIs in libvirt-domain.c to have job-id
>> + * counterparts along with a generic listing of all jobs (with flags
>> + * for filtering to specific job types)?
>> + */
>
> Or do we wait until some consumer asks for this? Do you mean
> GetBlockJobInfo? GetJobStats? GetJobInfo?
>
> Is it required for initial implementation? Not sure I'm expert enough
> to answer that questions!
I've already had Nir asking for something; he also requested an ability
to name jobs (he may use a UUID, but any name would work). My initial
implementation was hard-coded to always create job id '1' (and what's
the point of listing something, if the answer is either 'no job running'
or 'job 1 is running'). But where things get tricky is that if we do
NOT have an API for listing all jobs (even if there is just one possible
job in the initial implementation), then it gets much harder to backport
that API later. So yes, I _do_ need to get this implemented. I haven't
got it up yet (my v5 posting will probably still have it lacking), but
if I've missed 5.1, then I have enough time before 5.2 to definitely get
that API tackled as part of everything else related to incremental backups.
>> +/**
>> + * virDomainBackupGetXMLDesc:
>> + * @domain: a domain object
>> + * @id: the id of an active backup job previously started with
>> + * virDomainBackupBegin()
>> + * @flags: extra flags; not used yet, so callers should always pass 0
>> + *
>> + * In some cases, a user can start a backup job without supplying all
>> + * details, and rely on libvirt to fill in the rest (for example,
>
> s/, and/ and/
>
>> + * selecting the port used for an NBD export). This API can then be
>> + * used to learn what default values were chosen.
>> + *
>> + * Returns a NUL-terminated UTF-8 encoded XML instance, or NULL in
>
> s/, or/ or/
>
>> + * case of error. The caller must free() the returned value.
>> + */
>
> Do we have the same security concerns as Checkpoint? IOW: One doesn't
> necessarily have to use a conn that was used for virDomainBackupBegin or
> do they? Should we just check for a non read-only connection (which I
> assume has implications later in remote.x defs).
The security concern for CheckpointGetXMLDesc is that the checkpoint XML
includes a sub-element, which must NOT expose any passwords in
the except on a read-write connection where the _SECURE flag is
passed.
In the patches as posted so far, I separated the Checkpoint XML (with
its subelement) to be completely distinct from the Backup XML
(which is very minimal, and just describes the
Re: [libvirt] [PATCH v4 07/20] backup: Introduce virDomainCheckpoint APIs
On 2/11/19 3:19 PM, John Ferlan wrote:
>
>
> On 2/6/19 2:18 PM, Eric Blake wrote:
>> Introduce a bunch of new public APIs related to backup checkpoints.
>> Checkpoints are modeled heavily after virDomainSnapshotPtr (both
>> represent a point in time of the guest), although a snapshot exists
>> with the intent of rolling back to that state, while a checkpoint
>> exists to make it possible to create an incremental backup at a later
>> time.
>>
>> The full list of new API:
>> virDomainCheckpointCreateXML;
>> virDomainCheckpointCurrent;
>> virDomainCheckpointDelete;
>> virDomainCheckpointFree;
>> virDomainCheckpointGetConnect;
>> virDomainCheckpointGetDomain;
>> virDomainCheckpointGetParent;
>> virDomainCheckpointGetXMLDesc;
>> virDomainCheckpointHasMetadata;
>> virDomainCheckpointIsCurrent;
>> virDomainCheckpointListChildren;
>> virDomainCheckpointLookupByName;
>> virDomainCheckpointRef;
>> virDomainHasCurrentCheckpoint;
>> virDomainListCheckpoints;
>> virDomainCheckpointGetName;
>
> After reading and commenting further I came back here and started
> wondering if we should just name this 'libvirt-domain-backup' since
> checkpoint is part of backup... Then of course adjust various comments
> accordingly to indicate checkpoint and backup.
>
> Of course there's also something to be said for separating out the
> domain-backup API's into their own module too.
>
> So while you may see comments later about splitting just keep this
> secondary thought in mind.
Long-term, I'd like to have both virDomainBackupBegin() and some form of
virDomainSnapshotCreateXML() be able to kick off the creation of a
checkpoint object. Object-wise, we have virDomainSnapshotPtr and
virDomainCheckpointPtr (two distinct objects that are long-term XML
entities present regardless of what else you do), but no
virDomainBackupPtr (a backup job is just that - a job, that goes away
when it is complete). I'm actually thinking that it is better to have
virDomainBackupBegin() and friends live directly in libvirt-domain.h,
alongside other job-like APIs (virDomainMigrate(), virDomainBlockCopy(),
...), especially if I fix snapshots to refer to checkpoints.
So there's that to think about as well when considering where to place
things.
>
>> diff --git a/include/libvirt/libvirt-domain-checkpoint.h
>> b/include/libvirt/libvirt-domain-checkpoint.h
>> new file mode 100644
>> index 00..9006a46c6e
>> --- /dev/null
>> +++ b/include/libvirt/libvirt-domain-checkpoint.h
>> @@ -0,0 +1,155 @@
>> +/*
>> + * libvirt-domain-checkpoint.h
>> + * Summary: APIs for management of domain checkpoints
>> + * Description: Provides APIs for the management of domain checkpoints
>> + *
>> + * Copyright (C) 2006-2019 Red Hat, Inc.
>
> New file right, so should this just be 2019? There's varying opinions to
> follow on this...
Heavily copied from an existing file, so I copied the copyright as part
of that.
>> +typedef enum {
>> +VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE= (1 << 0), /* Restore or alter
>> +metadata */
>> +VIR_DOMAIN_CHECKPOINT_CREATE_CURRENT = (1 << 1), /* With redefine,
>> make
>> +checkpoint
>> current */
>> +VIR_DOMAIN_CHECKPOINT_CREATE_NO_METADATA = (1 << 2), /* Make checkpoint
>> without
>> +remembering it
>> */
>> +/* TODO: VIR_DOMAIN_CHECKPOINT_CREATE_QUIESCE */
>
> Need to remove the TODO or add the flag of course...
I'll see if I can implement it (it probably missed 5.1, but then again,
this whole series needs a respin with freeze today; but should be easy
enough to get by 5.2)
>> @@ -1580,6 +1627,17 @@ struct _virHypervisorDriver {
>> virDrvConnectBaselineHypervisorCPU connectBaselineHypervisorCPU;
>> virDrvNodeGetSEVInfo nodeGetSEVInfo;
>> virDrvDomainGetLaunchSecurityInfo domainGetLaunchSecurityInfo;
>> +virDrvDomainCheckpointCreateXML domainCheckpointCreateXML;
>> +virDrvDomainCheckpointGetXMLDesc domainCheckpointGetXMLDesc;
>> +virDrvDomainListCheckpoints domainListCheckpoints;
>> +virDrvDomainCheckpointListChildren domainCheckpointListChildren;
>> +virDrvDomainCheckpointLookupByName domainCheckpointLookupByName;
>> +virDrvDomainHasCurrentCheckpoint domainHasCurrentCheckpoint;
>> +virDrvDomainCheckpointGetParent domainCheckpointGetParent;
>> +virDrvDomainCheckpointCurrent domainCheckpointCurrent;
>> +virDrvDomainCheckpointIsCurrent domainCheckpointIsCurrent;
>> +virDrvDomainCheckpointHasMetadata domainCheckpointHasMetadata;
>> +virDrvDomainCheckpointDelete domainCheckpointDelete;
>> };
>
> Since we're adding new I think we should be consistent w/ naming
>
> virDrvDomainCheckpoint* and domainCheckpoint*
>
> DomainListCheckpoints and
[libvirt] [PATCH] virfile: added GPFS as shared fs
From: dmichelotto
Added GPFS as shared file system recognized during live migration
security checks.
GPFS is 'IBM General Parallel File System' also called
'IBM Spectrum Scale'
BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528
Signed-off-by: Diego Michelotto
---
src/util/virfile.c| 9 -
src/util/virfile.h| 1 +
tests/virfiledata/mounts3.txt | 1 +
tests/virfilemock.c | 5 +
tests/virfiletest.c | 1 +
5 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 271bf5e796..615c287104 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3468,6 +3468,9 @@ int virFilePrintf(FILE *fp, const char *msg, ...)
# ifndef CEPH_SUPER_MAGIC
# define CEPH_SUPER_MAGIC 0x00C36400
# endif
+# ifndef GPFS_SUPER_MAGIC
+# define GPFS_SUPER_MAGIC 0x47504653
+# endif
# define PROC_MOUNTS "/proc/mounts"
@@ -3613,6 +3616,9 @@ virFileIsSharedFSType(const char *path,
if ((fstypes & VIR_FILE_SHFS_CEPH) &&
(f_type == CEPH_SUPER_MAGIC))
return 1;
+if ((fstypes & VIR_FILE_SHFS_GPFS) &&
+(f_type == GPFS_SUPER_MAGIC))
+return 1;
return 0;
}
@@ -3776,7 +3782,8 @@ int virFileIsSharedFS(const char *path)
VIR_FILE_SHFS_AFS |
VIR_FILE_SHFS_SMB |
VIR_FILE_SHFS_CIFS |
- VIR_FILE_SHFS_CEPH);
+ VIR_FILE_SHFS_CEPH |
+ VIR_FILE_SHFS_GPFS);
}
diff --git a/src/util/virfile.h b/src/util/virfile.h
index be612af770..0b946fe1ca 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -221,6 +221,7 @@ enum {
VIR_FILE_SHFS_SMB = (1 << 4),
VIR_FILE_SHFS_CIFS = (1 << 5),
VIR_FILE_SHFS_CEPH = (1 << 6),
+VIR_FILE_SHFS_GPFS = (1 << 7),
};
int virFileIsSharedFSType(const char *path, int fstypes) ATTRIBUTE_NONNULL(1);
diff --git a/tests/virfiledata/mounts3.txt b/tests/virfiledata/mounts3.txt
index 68eded048c..4377e5d471 100644
--- a/tests/virfiledata/mounts3.txt
+++ b/tests/virfiledata/mounts3.txt
@@ -35,3 +35,4 @@ host:/gv0 /gluster fuse.glusterfs rw 0 0
root@host:/tmp/mkdir /gluster/sshfs fuse.sshfs rw 0 0
192.168.0.1:/ceph/data /ceph ceph
rw,noatime,name=cephfs,secret=,acl,wsize=16777216 0 0
192.168.0.1,192.168.0.2,192.168.0.3:/ceph/data2 /ceph/multi ceph
rw,noatime,name=cephfs,secret=,acl,wsize=16777216 0 0
+gpfs_data /gpfs/data gpfs rw,relatime 0 0
diff --git a/tests/virfilemock.c b/tests/virfilemock.c
index 499135d773..89e14c5b67 100644
--- a/tests/virfilemock.c
+++ b/tests/virfilemock.c
@@ -89,6 +89,9 @@ setmntent(const char *filename, const char *type)
#ifndef CEPH_SUPER_MAGIC
# define CEPH_SUPER_MAGIC 0x00c36400
#endif
+#ifndef GPFS_SUPER_MAGIC
+# define GPFS_SUPER_MAGIC 0x47504653
+#endif
static int
@@ -137,6 +140,8 @@ statfs_mock(const char *mtab,
ftype = FUSE_SUPER_MAGIC;
} else if (STRPREFIX(mb.mnt_type, "ceph")) {
ftype = CEPH_SUPER_MAGIC;
+} else if (STRPREFIX(mb.mnt_type, "gpfs")) {
+ftype = GPFS_SUPER_MAGIC;
} else {
/* Everything else is EXT4. We don't care really for other paths.
*/
ftype = EXT4_SUPER_MAGIC;
diff --git a/tests/virfiletest.c b/tests/virfiletest.c
index f90c611ac4..e2bd4953ed 100644
--- a/tests/virfiletest.c
+++ b/tests/virfiletest.c
@@ -457,6 +457,7 @@ mymain(void)
DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/some/symlink/file", true);
DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/ceph/file", true);
DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/ceph/multi/file", true);
+DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/gpfs/data", true);
return ret != 0 ? EXIT_FAILURE : EXIT_SUCCESS;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/1] update copyright notice
On Mon, Feb 25, 2019 at 8:04 PM Jim Fehlig wrote:
> On 2/22/19 2:06 AM, David Kiarie wrote:
> > > i did this as part of GSoC. there was no contract.
> >
> > I'm not entirely sure, but we might want to ask the SuSE folks
> whether
> > they agree.
> >
> >
> > i guess you can do that. my mentor was jim, he's around libvirt a lot.
>
> David is correct. There was no contract between him and SUSE. His work in
> the
> xl<->xml converter was done under a broader GSoC project.
>
> It sounds like the main grievance is the SUSE copyright in
> src/xenconfig/xen_xl.{ch} and tests/xlconfigtest.c. I suppose that came by
> way
> of copy-and-paste from other files. I'd be more than happy to send a patch
> removing the SUSE copyright from those files if that will put closure to
> this issue.
>
issue long closed (or at least as far as i am concerned :-)) and anyways
removing SUSE copyright from the files wasn't the right solution to the
issue.
>
> Regards,
> Jim
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH v4 3/5] lcitool: avoid intermediate list of packages
On Thu, 2019-02-21 at 16:33 +, Daniel P. Berrangé wrote: > We can purge any packages which expand to None straight away, and > simply convert to a set to get rid of duplicates. > > Signed-off-by: Daniel P. Berrangé > --- > guests/lcitool | 17 + > 1 file changed, 5 insertions(+), 12 deletions(-) This doesn't work: $ ./lcitool dockerfile libvirt-centos-7 libvirt Traceback (most recent call last): File "./lcitool", line 601, in Application().run() File "./lcitool", line 596, in run args.func(args) File "./lcitool", line 560, in _action_dockerfile if pkgs[package] is None: KeyError: 'apparmor' That's because... > @@ -557,21 +556,15 @@ class Application: > for package in self._projects.get_packages(project): > for key in keys: > if key in mappings[package]: > -temp[package] = mappings[package][key] > +pkgs[package] = mappings[package][key] ... the hunk if package not in pkgs: continue from the following patch should have been included right about here. It would be nice if the Ansible part would be converted to a similar logic, which should be possible using the appropriate Jinja2 filters, but that can wait until later. If you include the hunk mentioned above, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/1] update copyright notice
On 2/22/19 2:06 AM, David Kiarie wrote:
> i did this as part of GSoC. there was no contract.
I'm not entirely sure, but we might want to ask the SuSE folks whether
they agree.
i guess you can do that. my mentor was jim, he's around libvirt a lot.
David is correct. There was no contract between him and SUSE. His work in the
xl<->xml converter was done under a broader GSoC project.
It sounds like the main grievance is the SUSE copyright in
src/xenconfig/xen_xl.{ch} and tests/xlconfigtest.c. I suppose that came by way
of copy-and-paste from other files. I'd be more than happy to send a patch
removing the SUSE copyright from those files if that will put closure to this issue.
Regards,
Jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/1] virsh: Test batch mode parser improvements
On 2/21/19 3:20 PM, Eric Blake wrote: > No good feature is complete without tests ;) > > Signed-off-by: Eric Blake > --- > > Maybe I should squash all three patches into one? I like splitting the first two. Obviously parts of this could be split into the first one with the second one obtaining the \\\ eyesore. I leave that fun up to you though ;-)... End result of 2 patches I think. > > tests/virshtest.c | 7 +++ > 1 file changed, 7 insertions(+) > Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] virfile: added GPFS as shared fs
On Mon, Feb 25, 2019 at 17:39:46 +0100, Diego Michelotto wrote: > Hi Peter > > > Il giorno 25 feb 2019, alle ore 13:28, Peter Krempa ha > > scritto: > > > >> On Mon, Feb 25, 2019 at 09:39:39 +0100, dmichelotto wrote: > >> Added GPFS as shared file system recognized during live migration > >> security checks. > > > > You could mention that GPFS stands for 'IBM General Parallel File System > > (GPFS)'. > > OK, I’ll change it. > > > > >> > >> This becomes from upstream code on https://libvirt.org/git from master > >> branch > > > > This should not be part of the commit message. > > Ok, I’ll remove it. > > > >> > >> BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528 > >> --- > > > > Please note that the libvirt project requires that you comply with the > > developer certificate of origin. See > > https://libvirt.org/hacking.html#patches > > part 6 and https://developercertificate.org/. > > Is it enough if I add --signoff option and my full name at the last commit? Yes, that's exactly what we require. I just prefer wording it carefully so that I don't say "you need to add a Signed-off-by: ... line" where it might not be clear that it has a formal meaning. > > > > > Also we prefer if you use full name as the author when sending the > > patch. > > > > > >> src/util/virfile.c| 9 - > >> src/util/virfile.h| 1 + > >> tests/virfiledata/mounts3.txt | 1 + > >> tests/virfilemock.c | 5 + > >> tests/virfiletest.c | 1 + > >> 5 files changed, 16 insertions(+), 1 deletion(-) > > > > The changes look good to me but I'd prefer if you send the patch again > > including the sign-off and proper name. > > > > I’m very sorry for my mistakes, it’s the first libvirt patch for me. That's all right, you did good in the code department, just a few formal improvements needed. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/1] virsh: Elide backslash-newline in batch mode
On 2/21/19 3:20 PM, Eric Blake wrote: > The previous patch made it possible to split multiple commands by > adding newline, but not to split a long single command. The sequence > backslash-newline was being used as if it were a quoted newline > character, rather than completely elided the way the shell does. > > Signed-off-by: Eric Blake > --- > tools/virsh.pod | 4 +++- > tools/virt-admin.pod | 3 ++- > tools/vsh.c | 8 ++-- > 3 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/tools/virsh.pod b/tools/virsh.pod > index 2bf1ee77bb..4057edf3a7 100644 > --- a/tools/virsh.pod > +++ b/tools/virsh.pod > @@ -41,7 +41,9 @@ The B program can be used either to run one > I by giving the > command and its arguments on the shell command line, or a I > which is a single shell argument consisting of multiple I actions > and their arguments joined with whitespace, and separated by semicolons or I think you meant to chop the above line too... > -newlines between commands. Within I, virsh understands the > +and their arguments joined with whitespace, and separated by semicolons or > +newlines between commands, and where unquoted backslash-newline pairs are s/, and where/, where/ > +elided. Within I, virsh understands the > same single, double, and backslash escapes as the shell, although you must > add another layer of shell escaping in creating the single shell argument. > If no command is given in the command line, B will then start a > minimal > diff --git a/tools/virt-admin.pod b/tools/virt-admin.pod > index b2c9a1db6d..e1513e27e5 100644 > --- a/tools/virt-admin.pod > +++ b/tools/virt-admin.pod > @@ -24,7 +24,8 @@ The B program can be used either to run one > I by giving the > command and its arguments on the shell command line, or a I > which is a single shell argument consisting of multiple I actions > and their arguments joined with whitespace, and separated by semicolons or Same here. > -newlines between commands. Within I, virt-admin understands > the > +newlines between commands, and where unquoted backslash-newline pairs are s/, and where/, where/ Reviewed-by: John Ferlan John > +elided. Within I, virt-admin understands the > same single, double, and backslash escapes as the shell, although you must > add another layer of shell escaping in creating the single shell argument. > If no command is given in the command line, B will then start a > minimal [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] snapshot: Improve message for VIR_ERR_INVALID_DOMAIN_SNAPSHOT
On Mon, Feb 25, 2019 at 10:40:00AM -0600, Eric Blake wrote: > For consistency with other error messages, and the fact that > the object is always called a virDomainSnapshot rather than > a mere virSnapshot, include the word "domain" in the error > message. > > Suggested-by: John Ferlan > Signed-off-by: Eric Blake > --- > src/util/virerror.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Treat \n like ; in batch mode
On 2/21/19 1:55 PM, Eric Blake wrote: > I wanted to do a demonstration with virsh batch mode, which > takes multiple commands all packed into a single argument: > > $ virsh -c test:///default 'echo a; echo b;' > a > b > > but that produced a really long line, so I tried to make it > more legible: > > $ virsh -c test:///default ' >echo a; >echo b; > ' > error: unknown command: ' > ' > > Let's be more like the shell, and treat unquoted newline as a > command separator just as we do for semicolon. In fact, with > that, I can even now mix styles: > > $ virsh -c test:///default ' >echo a; echo b >echo c > ' > a > b > c > > Fix the grammer in a nearby comment while at it. > > Signed-off-by: Eric Blake > --- > tools/virsh.pod | 4 ++-- > tools/virt-admin.pod | 4 ++-- > tools/vsh.c | 6 +++--- > 3 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/tools/virsh.pod b/tools/virsh.pod > index 67edb57b14..2bf1ee77bb 100644 > --- a/tools/virsh.pod > +++ b/tools/virsh.pod > @@ -40,8 +40,8 @@ as a name. > The B program can be used either to run one I by giving the > command and its arguments on the shell command line, or a I > which is a single shell argument consisting of multiple I actions > -and their arguments joined with whitespace, and separated by semicolons > -between commands. Within I, virsh understands the > +and their arguments joined with whitespace, and separated by semicolons or s/whitespace, and/whitespace and/ > +newlines between commands. Within I, virsh understands the > same single, double, and backslash escapes as the shell, although you must > add another layer of shell escaping in creating the single shell argument. > If no command is given in the command line, B will then start a > minimal > diff --git a/tools/virt-admin.pod b/tools/virt-admin.pod > index 3ddbbff934..b2c9a1db6d 100644 > --- a/tools/virt-admin.pod > +++ b/tools/virt-admin.pod > @@ -23,8 +23,8 @@ Where I is one of the commands listed below. > The B program can be used either to run one I by giving > the > command and its arguments on the shell command line, or a I > which is a single shell argument consisting of multiple I actions > -and their arguments joined with whitespace, and separated by semicolons > -between commands. Within I, virt-admin understands the > +and their arguments joined with whitespace, and separated by semicolons or s/whitespace, and/whitespace and/ Reviewed-by: John Ferlan John > +newlines between commands. Within I, virt-admin understands > the > same single, double, and backslash escapes as the shell, although you must > add another layer of shell escaping in creating the single shell argument. > If no command is given in the command line, B will then start a > minimal [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] snapshot: Improve message for VIR_ERR_INVALID_DOMAIN_SNAPSHOT
For consistency with other error messages, and the fact that
the object is always called a virDomainSnapshot rather than
a mere virSnapshot, include the word "domain" in the error
message.
Suggested-by: John Ferlan
Signed-off-by: Eric Blake
---
src/util/virerror.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virerror.c b/src/util/virerror.c
index 9e3975fe76..05e535d859 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -1123,8 +1123,8 @@ const virErrorMsgTuple
virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = {
N_("Hook script execution failed"),
N_("Hook script execution failed: %s") },
[VIR_ERR_INVALID_DOMAIN_SNAPSHOT] = {
-N_("Invalid snapshot"),
-N_("Invalid snapshot: %s") },
+N_("Invalid domain snapshot"),
+N_("Invalid domain snapshot: %s") },
[VIR_ERR_NO_DOMAIN_SNAPSHOT] = {
N_("Domain snapshot not found"),
N_("Domain snapshot not found: %s") },
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] virfile: added GPFS as shared fs
Hi Peter > Il giorno 25 feb 2019, alle ore 13:28, Peter Krempa ha > scritto: > >> On Mon, Feb 25, 2019 at 09:39:39 +0100, dmichelotto wrote: >> Added GPFS as shared file system recognized during live migration >> security checks. > > You could mention that GPFS stands for 'IBM General Parallel File System > (GPFS)'. OK, I’ll change it. > >> >> This becomes from upstream code on https://libvirt.org/git from master branch > > This should not be part of the commit message. Ok, I’ll remove it. > >> >> BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528 >> --- > > Please note that the libvirt project requires that you comply with the > developer certificate of origin. See https://libvirt.org/hacking.html#patches > part 6 and https://developercertificate.org/. Is it enough if I add --signoff option and my full name at the last commit? > > Also we prefer if you use full name as the author when sending the > patch. > > >> src/util/virfile.c| 9 - >> src/util/virfile.h| 1 + >> tests/virfiledata/mounts3.txt | 1 + >> tests/virfilemock.c | 5 + >> tests/virfiletest.c | 1 + >> 5 files changed, 16 insertions(+), 1 deletion(-) > > The changes look good to me but I'd prefer if you send the patch again > including the sign-off and proper name. > I’m very sorry for my mistakes, it’s the first libvirt patch for me. I’ll summit the new patch soon. All the best Diego -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] domcaps: use virTristateBool
On 2/19/19 3:09 PM, Cole Robinson wrote: > Extending domaincapabilities with new XML schema is currently a bit of > a maintenance pain. Consider the case of adding a new enum for listing > models. I want to output this info for the qemu driver. > > Internally in the domaincapabilities plumbing, whether a is > supported= is tracked with boolean true/false. If I extend that > pattern for devices and fill in data for the qemu driver, the > other domcaps implementations will now automatically output a new XML > element: > > > > Now, for bhyve I can 'git grep' confirm that it doesn't have any > support, but for xen/libxl it _is_ supported. So if I don't > fill in accurate support in the xen driver, I've just made their > domcaps report blatantly incorrect info. > > Ideally I would make these changes and the other drivers output > would _not_ change. xen output would now be incomplete, but not > obviously wrong, which is easier on me the developer, and safer for the > API consumer. > > This moves domcaps plumbing in that direction. It switches most > internal 'supported' fields to virTristateBool so we can track an > ABSENT state and map that to outputting no XML. Explicit supported='no' > values are filled in where needed to ensure existing driver XML doesn't > change. cpu and sev supported= values are left unconverted, but they > require semi-special handling anyways so aren't really affected by the > problem I laid out above. > > Cole Robinson (10): > tests: domcaps: Add a default 'empty' test > tests: domcaps: Remove unused typedef > tests: domcaps: Remove 'full' test > conf: domcaps: Add single line formatting macro > conf: domcaps: use virTristateBool for 'supported' > qemu: domcaps: fill in explicit supported BOOL_NO > libxl: domcaps: fill in explicit supported BOOL_NO > bhyve: domcaps: fill in explicit supported BOOL_NO > schemas: domcaps: Make more elements optional > conf: domcaps: Don't output XML on tristate ABSENT > > docs/schemas/domaincaps.rng | 20 +-- > src/bhyve/bhyve_capabilities.c | 20 +-- > src/conf/domain_capabilities.c | 26 ++ > src/conf/domain_capabilities.h | 20 +++ > src/libxl/libxl_capabilities.c | 18 --- > src/qemu/qemu_capabilities.c | 26 ++ > tests/domaincapsschemadata/empty.xml | 16 ++ > tests/domaincapstest.c | 78 ++-- > 8 files changed, 103 insertions(+), 121 deletions(-) > create mode 100644 tests/domaincapsschemadata/empty.xml > I think in patch3, you probably should remove the full.xml file too. Logically, it seems things work; however, I am curious what happens if/when this is applied to bhvye and libxl environments. I think it would be good to get someone to apply this there and be sure there's no unexpected (for you) failures. Interesting "view" for on "empty.xml" - digging deeper shows that "mode" could be optional (at least that's how I read the formatdomain text). So rather than "no" for all 3, there would be nothing. Similarly for SEV the "no" just is some default/optional value matching your RNG changes. Also, even though it wouldn't necessarily be a "feature" of perhaps Intel, someone running on AMD could I assume get a different result than you got. IOW: Same problem I ran into with that 2 patch series trying to "fake" SEV output. If cpu, devices, and features have no subelements - should they even be displayed? Leaving purely just the path, domain, machine, and arch output? Even with all that - is there any thought that perhaps some application has made use of the existing "functionality" to spit out "no" for those undefined/missing (e.g. ABSENT) values. IOW: It seems via the RNG we would have made claims that certain output exists that we're now making optional. I conceptually don't have an issue with it, I'm just thinking terms of what API output contract we may have made. Yes, those applications should be able to handle missing fields, but at the very least we probably would need to update the news.xml to let the consumer know. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 05/20] backup: Introduce virDomainCheckpointPtr
On 2/9/19 9:48 AM, John Ferlan wrote:
>
>
> On 2/6/19 2:18 PM, Eric Blake wrote:
>> Prepare for introducing a bunch of new public APIs related to
>> backup checkpoints by first introducing a new internal type
>> and errors associated with that type. Checkpoints are modeled
>> heavily after virDomainSnapshotPtr (both represent a point in
>> time of the guest), although a snapshot exists with the intent
>> of rolling back to that state, while a checkpoint exists to
>> make it possible to create an incremental backup at a later
>> time.
>>
>> Signed-off-by: Eric Blake
>>
>> @@ -1214,6 +1215,15 @@ const virErrorMsgTuple
>> virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = {
>> [VIR_ERR_NO_NWFILTER_BINDING] = {
>> N_("Network filter binding not found"),
>> N_("Network filter binding not found: %s") },
>> +[VIR_ERR_INVALID_DOMAIN_CHECKPOINT] = {
>> +N_("Invalid checkpoint"),
>> +N_("Invalid checkpoint: %s") },
>
> Invalid domain checkpoint
Copy and paste from VIR_ERR_INVALID_DOMAIN_SNAPSHOT; will do separate
patch to fix that wording.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] virfile: added GPFS as shared fs
Hi Peter, > Il giorno 25 feb 2019, alle ore 13:20, Peter Krempa ha > scritto: > >> On Mon, Feb 25, 2019 at 09:37:55 +0100, dmichelotto wrote: >> Added GPFS as shared file system recognized during live migration >> security checks. >> >> This patch is based on libvirt-4.5.0 distributed by CentOS 7.6.1810 >> via source rpm. > > This is the libvirt project upstream mailing list. We don't take patches > for centos here. > > Also I'm sure that the centos maintainer will accept this patch only > after the upstream version is merged and the downstream backport is > referencing it. I’ll summit a new patch soon as you request. If my patch will be accepted upstream, what should I do to get the patch backported? All the best Diego. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 05/20] backup: Introduce virDomainCheckpointPtr
On 2/9/19 9:48 AM, John Ferlan wrote:
>
>
> On 2/6/19 2:18 PM, Eric Blake wrote:
>> Prepare for introducing a bunch of new public APIs related to
>> backup checkpoints by first introducing a new internal type
>> and errors associated with that type. Checkpoints are modeled
>> heavily after virDomainSnapshotPtr (both represent a point in
>> time of the guest), although a snapshot exists with the intent
>> of rolling back to that state, while a checkpoint exists to
>> make it possible to create an incremental backup at a later
>> time.
>>
>> Signed-off-by: Eric Blake
>>
>> ---
>> v2: fix copy-and-paste issue in virerror.c [John]
>> ---
>> include/libvirt/virterror.h | 7 +++--
>> src/util/virerror.c | 12 ++-
>> include/libvirt/libvirt.h | 2 ++
>> src/datatypes.h | 31 ++-
>> src/datatypes.c | 62 -
>> src/libvirt_private.syms| 2 ++
>> 6 files changed, 111 insertions(+), 5 deletions(-)
>>
>
> Naturally something changed recently in datatypes.h... so you will have
> a merge conflict here.
Yeah, a long-running issue. v5 will be rebased yet again.
>
>> diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
>> index 3c19ff5e2e..acbf03d0ea 100644
>> --- a/include/libvirt/virterror.h
>> +++ b/include/libvirt/virterror.h
>> @@ -4,7 +4,7 @@
>> * Description: Provides the interfaces of the libvirt library to handle
>> * errors raised while using the library.
>> *
>> - * Copyright (C) 2006-2016 Red Hat, Inc.
>> + * Copyright (C) 2006-2019 Red Hat, Inc.
>
> I'll let someone else complain about your emacs macro ;-)
>> --- a/include/libvirt/libvirt.h
>> +++ b/include/libvirt/libvirt.h
>> @@ -34,6 +34,8 @@ extern "C" {
>> # include
>> # include
>> # include
>> +typedef struct _virDomainCheckpoint virDomainCheckpoint;
>> +typedef virDomainCheckpoint *virDomainCheckpointPtr;
>
> This is a weird construct for this file...
>
> I think this should be in a libvirt-domain-checkpoint.h type file which
> I see is generated a few patches later and these lines moved. In the
> long run I guess as long as it's in a file it doesn't matter.
I'll add a comment here making it obvious that this is a temporary hack
to avoid even more invasive changes in this patch, and that it gets
cleaned up later.
>> +++ b/src/datatypes.h
>> @@ -1,7 +1,7 @@
>> /*
>> * datatypes.h: management of structs for public data types
>> *
>> - * Copyright (C) 2006-2015 Red Hat, Inc.
>> + * Copyright (C) 2006-2018 Red Hat, Inc.
>
> haha 2018 - I know when you last touched this ;-)
D'oh - my emacs macro may be nice, but it isn't consistent unless I
retouch the file. :-)
>> +/**
>> + * virGetDomainCheckpoint:
>> + * @domain: the domain to checkpoint
>> + * @name: pointer to the domain checkpoint name
>> + *
>> + * Allocates a new domain checkpoint object. When the object is no longer
>> needed,
>> + * virObjectUnref() must be called in order to not leak data.
>> + *
>> + * Returns a pointer to the domain checkpoint object, or NULL on error.
>> + */
>> +virDomainCheckpointPtr
>> +virGetDomainCheckpoint(virDomainPtr domain, const char *name)
>
> More recently we or I have been trying to enforce one line per argument
> for new code or changed methods.
Can do. Old habits die hard, but I can make the effort to avoid them in
favor of one-argument-per-line.
>
>> +{
>> +virDomainCheckpointPtr ret = NULL;
>> +
>> +if (virDataTypesInitialize() < 0)
>> +return NULL;
>> +
>> +virCheckDomainGoto(domain, error);
>> +virCheckNonNullArgGoto(name, error);
>> +
>> +if (!(ret = virObjectNew(virDomainCheckpointClass)))
>> +goto error;
>
> Could return NULL too, but it is a copy of virGetDomainSnapshot
>
> Reviewed-by: John Ferlan
>
And as you've seen in my other recent patches, I've been cleaning up
some snapshot oddities; any cleanups I do there will also need to be
folded into v5 here.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 03/20] qemu: Allow optional export name during NBD export
On 2/25/19 9:53 AM, Eric Blake wrote: > On 2/9/19 9:41 AM, John Ferlan wrote: >> >> >> On 2/6/19 2:18 PM, Eric Blake wrote: >>> Right now, we only use NBD exports during storage migration, >>> where we control the NBD client and therefore don't care about >>> the export name (qemu's default of naming the export after the >>> device is fine). But upcoming patches for exposing backups over >>> NBD wants to use a different export name (matching the libvirt >>> domain XML rather than the qemu device name); so enhance the >>> QMP glue code to allow this flexibility. >>> >>> Signed-off-by: Eric Blake >>> --- >>> src/qemu/qemu_monitor.h | 1 + >>> src/qemu/qemu_monitor_json.h | 1 + >>> src/qemu/qemu_migration.c| 2 +- >>> src/qemu/qemu_monitor.c | 5 +++-- >>> src/qemu/qemu_monitor_json.c | 2 ++ >>> tests/qemumonitorjsontest.c | 2 +- >>> 6 files changed, 9 insertions(+), 4 deletions(-) >>> >> >> I assume/hope some day a test will be added that will show what the >> @export variable does. I haven't read ahead at all - I'm trying to just >> go in order and get some feedback sooner so you can start pushing stuff. >> >> Reviewed-by: John Ferlan > > Yes, I need to do better at in-tree tests (and not just stuff I've > mentioned in the commit messages) for my v5. The export name gets used > during pull-style backups, where I purposefully went with naming the > exports 'vda' and 'vdb' for a guest with two disks, rather than based on > any file name or block node name; 'qemu-nbd --list' connected to the NBD > server during a pull backup is an easy way of testing the export names > made it through the stack. > For that matter, I'm probably just going to squash this patch in with 15/20, and do all the QMP monitor changes in one patch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH v4 2/5] lcitool: avoid repetition when expanding package mappings
On Thu, 2019-02-21 at 16:33 +, Daniel P. Berrangé wrote: [...] > +keys = ["default", package_format, os_name, os_full] > # We need to add the base project manually here: the standard > # machinery hides it because it's an implementation detail > for project in projects + ["base"]: > for package in self._projects.get_packages(project): > -if "default" in mappings[package]: > -temp[package] = mappings[package]["default"] > -if package_format in mappings[package]: > -temp[package] = mappings[package][package_format] > -if os_name in mappings[package]: > -temp[package] = mappings[package][os_name] > -if os_full in mappings[package]: > -temp[package] = mappings[package][os_full] > +for key in keys: > +if key in mappings[package]: > +temp[package] = mappings[package][key] Little historical note: the reason why the code looked like this in the first place[1] is that its structure was supposed to mirror that of playbooks/update/tasks/packages.yml as closely as possible - the idea being that, if the Ansible implementation was correct, then the Python one would most likely be as well. Of course that's no longer the case as of dcded110e102, so it makes perfect sense to go further down this road and make the code more compact. Reviewed-by: Andrea Bolognani [1] In addition to Python being admittedly not my forte :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 03/20] qemu: Allow optional export name during NBD export
On 2/9/19 9:41 AM, John Ferlan wrote: > > > On 2/6/19 2:18 PM, Eric Blake wrote: >> Right now, we only use NBD exports during storage migration, >> where we control the NBD client and therefore don't care about >> the export name (qemu's default of naming the export after the >> device is fine). But upcoming patches for exposing backups over >> NBD wants to use a different export name (matching the libvirt >> domain XML rather than the qemu device name); so enhance the >> QMP glue code to allow this flexibility. >> >> Signed-off-by: Eric Blake >> --- >> src/qemu/qemu_monitor.h | 1 + >> src/qemu/qemu_monitor_json.h | 1 + >> src/qemu/qemu_migration.c| 2 +- >> src/qemu/qemu_monitor.c | 5 +++-- >> src/qemu/qemu_monitor_json.c | 2 ++ >> tests/qemumonitorjsontest.c | 2 +- >> 6 files changed, 9 insertions(+), 4 deletions(-) >> > > I assume/hope some day a test will be added that will show what the > @export variable does. I haven't read ahead at all - I'm trying to just > go in order and get some feedback sooner so you can start pushing stuff. > > Reviewed-by: John Ferlan Yes, I need to do better at in-tree tests (and not just stuff I've mentioned in the commit messages) for my v5. The export name gets used during pull-style backups, where I purposefully went with naming the exports 'vda' and 'vdb' for a guest with two disks, rather than based on any file name or block node name; 'qemu-nbd --list' connected to the NBD server during a pull backup is an easy way of testing the export names made it through the stack. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] util: alloc: Introduce macro for self-freeing NULL-terminated lists
On Fri, Feb 22, 2019 at 05:04:38PM +0100, Peter Krempa wrote:
> VIR_AUTOLISTPTR allows you to define a pointer to a NULL-terminated list
> of elements which will be auto-freed when destroying the scope.
>
> This is done so that we can avoid using VIR_AUTOPTR in those cases as it
> worked only for virStringList-related stuff.
>
> Signed-off-by: Peter Krempa
> ---
> src/util/viralloc.h | 61 +
> 1 file changed, 61 insertions(+)
>
> diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> index 50a07d4fa3..983a6e83d1 100644
> --- a/src/util/viralloc.h
> +++ b/src/util/viralloc.h
> @@ -631,6 +631,54 @@ void virAllocTestHook(void (*func)(int, void*), void
> *data);
> (func)(_ptr); \
> }
>
> +
> +# define VIR_AUTOLISTPTR_FUNC_NAME(type) type##AutoListPtrFree
> +
> +/**
> + * VIR_DEFINE_AUTOLISTPTR_FUNC:
> + * @type: type of the element of the list
> + * @func: freeing function for a single element of the list
> + *
> + * This macro defines the freeing function used by VIR_AUTOLISTPTR for @type.
> + * @func must free one of the elements of @type.
> + *
> + * Note that the function is not inline due to size and thus
> + * VIR_DECLARE_AUTOLISTPTR_FUNC needs to be used in the appropriate header
> file.
> + *
> + * VIR_DEFINE_AUTOLISTPTR_FUNC is mutually exclusive with
> + * VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT.
> + */
> +# define VIR_DECLARE_AUTOLISTPTR_FUNC(type) \
> +void VIR_AUTOLISTPTR_FUNC_NAME(type)(type **_ptr);
> +# define VIR_DEFINE_AUTOLISTPTR_FUNC(type, func) \
> +void VIR_AUTOLISTPTR_FUNC_NAME(type)(type **_ptr) \
> +{ \
> +type *n;\
> +for (n = *_ptr; n && *n; n++) \
> +(func)(n); \
> +VIR_FREE(*_ptr);\
> +}
So, I believe that ^this should be unnecessary and it should always be handled
by the function. I don't have a good argument yet, I need to think about this
some more, but at the time being, I think that if we have a type from which we
create a NULL-terminated list, we should have dedicated type for that too and a
destructor as well, ergo VIR_AUTOPTR suffices. I appreciate and understand the
effort, solely because it was virString driving it which is confusing on its
own because we can't make AUTOPTR work properly because virString introduces an
additional pointer and everything would go haywire if we had virStringList
type. Maybe it would be worth saying that virString is indeed special and
therefore needs to be handled on its own with its own macro. That way, the whole
AUTOLIST wouldn't be needed, at least not in the current form.
I can be convinced otherwise with good arguments though.
> +
> +/**
> + * VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT:
> + * @type: type of the element of the list
> + * @func: freeing function for the whole NULL-terminated list of @type
> + *
> + * This macro defines the freeing function used by VIR_AUTOLISTPTR for @type.
> + * @func must free a NULL terminated dense list of @type and also the list
> + * itself. This is a convenience option if @type has already such function.
> + *
> + * VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT is mutually exclusive with
> + * VIR_DEFINE_AUTOLISTPTR_FUNC.
> + */
> +# define VIR_DEFINE_AUTOLISTPTR_FUNC_DIRECT(type, func) \
> +static inline void VIR_AUTOLISTPTR_FUNC_NAME(type)(type **_ptr) \
> +{ \
> +if (*_ptr) \
> +(func)(*_ptr); \
> +}
So the only difference between ^this and VIR_AUTOPTR is the description.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 01/20] snapshots: Avoid term 'checkpoint' for full system snapshot
On 2/9/19 9:40 AM, John Ferlan wrote: > > > On 2/6/19 2:17 PM, Eric Blake wrote: >> Upcoming patches plan to introduce virDomainCheckpointPtr as a new >> object for use in incremental backups, along with documentation on >> how incremental backups differ from snapshots. But first, we need >> to rename any existing mention of a 'system checkpoint' to instead >> be a 'full system snapshot', so that we aren't overloading >> the term checkpoint. >> >> Signed-off-by: Eric Blake >> >> --- >> v2: wording improvements based on review >> --- >> include/libvirt/libvirt-domain-snapshot.h | 2 +- >> docs/formatsnapshot.html.in | 31 +++ >> src/conf/snapshot_conf.c | 4 +-- >> src/libvirt-domain-snapshot.c | 7 ++--- >> src/qemu/qemu_driver.c| 12 - >> tools/virsh-snapshot.c| 2 +- >> tools/virsh.pod | 14 +- >> 7 files changed, 36 insertions(+), 36 deletions(-) >> > > Figured I'd start this and continue on Monday > > In my "scan" of this I found it "interesting" that eventually as we get > closer to the user docs there's an emphasis to know the difference > between external and internal snapshots... > > Reviewed-by: John Ferlan I think I'll go ahead and push this one now, regardless of the work remaining on the rest of the series, as it is a nice cleanup to have in the release. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/6] lxc: Converting 'if, else' logic into a 'switch, case'.
On 2/18/19 2:09 PM, Julio Faracco wrote:
> The structure used to handle network entries was based on 'if,else'
> conditions. This commit converts this ugly structure into a switch to
> clearify each option of the handler.
>
> Signed-off-by: Julio Faracco
> ---
> src/lxc/lxc_native.c | 58
> src/lxc/lxc_native.h | 17 +
> 2 files changed, 59 insertions(+), 16 deletions(-)
>
> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index 25e35e93dd..c746c443da 100644
> --- a/src/lxc/lxc_native.c
> +++ b/src/lxc/lxc_native.c
> @@ -35,6 +35,20 @@
>
> VIR_LOG_INIT("lxc.lxc_native");
>
> +VIR_ENUM_IMPL(virLXCNetworkConfigEntry, VIR_LXC_NETWORK_CONFIG_LAST,
> + "name",
> + "type",
> + "link",
> + "hwaddr",
> + "flags",
> + "macvlan.mode",
> + "vlan.id",
> + "ipv4",
> + "ipv4.gateway",
> + "ipv6",
> + "ipv6.gateway"
> +);
> +
> static virDomainFSDefPtr
> lxcCreateFSDef(int type,
> const char *src,
> @@ -620,35 +634,47 @@ lxcNetworkParseDataIPs(const char *name,
> virConfValuePtr value, lxcNetworkParseD
> }
>
> static int
> -lxcNetworkParseDataSuffix(const char *name, virConfValuePtr value,
> lxcNetworkParseData *parseData)
> +lxcNetworkParseDataSuffix(const char *entry, virConfValuePtr value,
> lxcNetworkParseData *parseData)
same w/r/t empty lines and argument on each line.
> {
> -if (STREQ(name, "type")) {
> +int type = virLXCNetworkConfigEntryTypeFromString(entry);
s/int type/int elem/
@entry was another option, but @elem I think is better than @type since
"type" is a field.
Reviewed-by: John Ferlan
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] lxc: Create a method to initialize network data outisde.
$SUBJ:
lxc: Introduce lxcNetworkParseDataType
[see below for name change]
On 2/18/19 2:09 PM, Julio Faracco wrote:
> 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.
Extract out the network "type" processing into it's own method
rather than inline within lxcNetworkParseDataSuffix.
>
> 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)
Similar w/r/t blank lines and 2 lines per argument
I think this is better named lxcNetworkParseDataType
Reviewed-by: John Ferlan
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] lxc: Create a separate method to handle IPv{4, 6} outside parser.
FWIW: No need for stop (e.g. '.') at end of each comment message...
On 2/18/19 2:09 PM, Julio Faracco wrote:
> 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)
When adding new functions we like to see 2 blank lines before/after each
method and each argument for the method on it's own line.
I've fixed all of this for you...
Reviewed-by: John Ferlan
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] lxc: Converting full string entries in types only.
$SUBJ: lxc: Introduce lxcNetworkParseDataSuffix On 2/18/19 2:09 PM, Julio Faracco wrote: > 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". Replace last sentence w/ This will handle entries regardless of whether they are prefixed by "lxc.network." (today) or "lxc.net.X" (the future). > > Signed-off-by: Julio Faracco > --- > src/lxc/lxc_native.c | 30 +++--- > 1 file changed, 15 insertions(+), 15 deletions(-) > Reviewed-by: John Ferlan John [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] lxc: Refactoring lxcNetworkWalkCallback() to a simple method.
On 2/18/19 2:09 PM, Julio Faracco wrote: > The method lxcNetworkWalkCallback() needs to be simple enough to handle > both possible network settings with indexes or the simple one. It is > better the decouple the whole algorithm to parse data to only parse > which entry type libvirt is handling. > > Signed-off-by: Julio Faracco > --- > src/lxc/lxc_native.c | 13 + > 1 file changed, 13 insertions(+) > This merges w/ patch2 as discussed there. John [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] lxc: Create a new method called lxcNetworkParseDataEntry().
$SUBJ: lxc: Introduce lxcNetworkParseDataEntry On 2/18/19 2:09 PM, Julio Faracco wrote: > 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. *handler > > 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) Same thoughts as before w/r/t method headers. However, because we require each patch to be able to compile separately, things won't work in this order as lxcNetworkParseDataEntry is a static method with no callers. What needs to be done is this move this above lxcNetworkWalkCallback and then merge in patch 4 to here and alter patch3 slightly. That means patch4 gets dropped. I can do this and alter the commit messages appropriately. Reviewed-by: John Ferlan John [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/10] block/dirty-bitmap: add recording and busy properties
On 2/25/19 9:01 AM, Vladimir Sementsov-Ogievskiy wrote: > 23.02.2019 3:06, 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 >> --- >> +++ 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) > > > Don't we want instead an array of flags? Which will include also persistent > and > inconsistent? No, I don't think an array of flags is worth the extra complications in generation and parsing of the JSON. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/10] block/dirty-bitmap: add recording and busy properties
23.02.2019 3:06, 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 2b8afbb924..6e543594b3 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)
Don't we want instead an array of flags? Which will include also persistent and
inconsistent?
> #
> # @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 45c57952da..4c7ae8180f 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -67,6 +67,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 result field(s) dirty-bitmaps[i].status (since 4.0)
> +
> +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.
> +
> @subsection query-cpus (since 2.12.0)
>
> The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
> diff --git a/tests/qemu-iotests/236.out b/tests/qemu-iotests/236.out
> index 5006f7bca1..815cd053f0 100644
> --- a/tests/qemu-iotests/236.out
> +++ b/tests/qemu-iotests/236.out
> @@ -22,17 +22,21 @@ write -P0xcd 0x3ff 64k
> "bitmaps": {
> "drive0": [
> {
> +"busy": false,
> "count": 262144,
> "granularity": 65536,
> "name": "bitmapB",
> "persistent": false,
> +"recording": true,
> "status": "active"
> },
> {
> +"busy": false,
> "count": 262144,
> "granularity": 65536,
> "name": "bitmapA",
> "persistent": false,
> +"recording": true,
> "status": "active"
> }
> ]
> @@ -84,17 +88,21 @@ write -P0xcd 0x3ff 64k
> "bitmaps": {
> "drive0": [
> {
> +"busy": false,
> "count": 262144,
>
Re: [libvirt] [PATCH 1/4] util: alloc: Remove pointless clearing of variable in AUTOPTR_FUNCs
On Fri, Feb 22, 2019 at 05:04:37PM +0100, Peter Krempa wrote: > VIR_DEFINE_AUTOPTR_FUNC defines a function which is supposed to free the > resources for the given VIR_AUTOPTR variable. Given that the cleanup > function is executed when the stack frame is being destroyed it does not > make much sense to set the pointer to NULL. Although correct, the current code is correct too and I really don't see any benefit behind removing a single line where you reset a variable to NULL, if anything, this adds more safety regardless of scenario (defensive programming in general). Originally, I wanted to argue, that this might introduce an issue in loops if you pass the temporary variable to a function by reference and then exiting on failure would free some garbage, but that's irrelevant since Sukrit introduced a syntax check to ensure, VIR_AUTO variables are always initialized. > > Making the inline function contain less code also decreases the > possibility that for a given type the inlining will be declined by the > compiler due to increasing code size. I honestly doubt that re-setting a variable to NULL will have any impact on this and it's such a trivial operation that the compiler might optimize this for you in some way. I think this patch should be dropped from the series and left as is. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH v4 1/5] lcitool: use subparsers for commands
On Thu, 2019-02-21 at 16:33 +, Daniel P. Berrangé wrote:
> Currently only a single global parser is used for all commands. This
> means that every command accepts every argument which is undesirable as
> users don't know what to pass. It also prevents the parser from
> generating useful help information.
>
> Python's argparse module supports multi-command binaries in a very easy
> way by adding subparsers, each of which has their own distinct
> arguments. It can then generate suitable help text on a per command
> basis. This also means commands can use positional arguments for data
> items that are always passed.
>
> $ ./guests/lcitool -h
> usage: lcitool [-h] {install,update,build,hosts,projects,dockerfile} ...
>
> libvirt CI guest management tool
>
> positional arguments:
> {install,update,build,hosts,projects,dockerfile}
> commands
> install perform unattended host installation
> update prepare hosts and keep them updated
> build build projects on hosts
> hosts list all known hosts
> projectslist all known projects
> dockerfile generate a host docker file
>
> optional arguments:
> -h, --helpshow this help message and exit
>
> $ ./guests/lcitool install -h
> usage: lcitool install [-h] hosts
>
> positional arguments:
> hosts list of hosts to act on (accepts globs)
>
> optional arguments:
> -h, --help show this help message and exit
>
> $ ./guests/lcitool dockerfile -h
> usage: lcitool dockerfile [-h] hosts projects
>
> positional arguments:
> hosts list of hosts to act on (accepts globs)
> projectslist of projects to consider (accepts globs)
>
> optional arguments:
> -h, --help show this help message and exit
I feel like the usage examples are not really adding much to the
commit message, so I'd just leave them out.
On the other hand, all the examples in README.markdown are now
invalid, so you should make sure that file is updated along with
the script.
[...]
> +++ b/guests/lcitool
> @@ -307,43 +307,72 @@ class Application:
> conflict_handler="resolve",
> formatter_class=argparse.RawDescriptionHelpFormatter,
Since we're no longer providing our own, pre-formatted help text
and we're leaving its generation up to argparse instead, setting
formatter_class is no longer needed.
The same applies to all callers you introduce later.
[...]
> +subparser = self._parser.add_subparsers(help="commands")
Please rename the variable to 'subparsers'. At the same time, drop
use of the 'help' parameter and set 'metavar' to 'ACTION' instead:
that will change the main help output from
usage: lcitool [-h] {install,update,build,hosts,projects,dockerfile} ...
libvirt CI guest management tool
positional arguments:
{install,update,build,hosts,projects,dockerfile}
commands
install perform unattended host installation
...
to a less unwieldy
usage: lcitool [-h] ACTION ...
libvirt CI guest management tool
positional arguments:
ACTION
install perform unattended host installation
...
As an aside, I don't much like how argparse takes over the '-h'
option as a shorthand for '--help', especially since up until now
it has been used to specify hosts, but I haven't found a way to
disable that behavior. I guess I can live with it :)
> +subparser.required = True
> +subparser.dest = "command"
Setting the 'dest' parameter is not necessary - you're not using
it anywhere.
[...]
> +installparser.set_defaults(func=self._action_install)
Well, isn't this clever :)
[...]
> +dockerfileparser = subparser.add_parser(
> +"dockerfile", help="generate a host docker file",
I'd prefer if you kept the original help text here.
Everything else looks good.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Replace virDomainChrSourceDefFree with virObjectUnref
On Wed, Feb 20, 2019 at 09:51:07AM +0100, Marc Hartmayer wrote: Replace virDomainChrSourceDefFree with virObjectUnref. Signed-off-by: Marc Hartmayer Reviewed-by: Boris Fiuczynski --- cfg.mk| 1 - src/conf/domain_conf.c| 17 + src/conf/domain_conf.h| 1 - src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c| 4 ++-- src/qemu/qemu_parse_command.c | 4 ++-- src/qemu/qemu_process.c | 5 ++--- 7 files changed, 11 insertions(+), 22 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Use refcounting for priv->monConfig
On Wed, Feb 20, 2019 at 09:51:06AM +0100, Marc Hartmayer wrote: Use refcounting for priv->monConfig instead of asymmetric freeing. Signed-off-by: Marc Hartmayer Reviewed-by: Boris Fiuczynski --- src/qemu/qemu_driver.c | 5 + src/qemu/qemu_process.c | 6 +++--- 2 files changed, 4 insertions(+), 7 deletions(-) Reviewed-by: Ján Tomko Jano 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: fix heap-buffer-overflow in virFileWrapperFdFree
On Tue, Feb 19, 2019 at 03:40:31PM +0800, Jay Zhou wrote: From: Jing Wu Some functions like doCoreDump call virFileWrapperFdNew to execute async cmds, if a step after virFileWrapperFdNew failed, the func may skip virFileWrapperFdClose and jump to cleanup label to call virFileWrapperFdFree directly. Wouldn't the proper fix be to always call virFileWrapperFdClose? See this series by Andrea: https://www.redhat.com/archives/libvir-list/2019-February/msg01155.html If the child process of the cmd is running and asyncioThread is polling, cmd->errbuf have been alloced at least one byte but not yet operate (*cmd->errbuf)[errlen] = '\0', access of wfd->err_msg in virFileWrapperFdFree at this time will cause risk of heap-buffer-overflow. So, we need to put VIR_WARN(wfd->err_msg) after VIR_FREE(wfd->err_msg). This sentence does not make sense - why would we access the error after freeing it? Jano Besides, since virCommandFree has included virCommandAbort, there is no need to call virCommandAbort extraly. Signed-off-by: Jing Wu Signed-off-by: Jay Zhou --- src/util/virfile.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] rpc: client: fix race on stream error and stream creation
On 22.02.2019 18:15, John Ferlan wrote:
>
>
> On 2/15/19 9:10 AM, Nikolay Shirokovskiy wrote:
>> Message of API call that creates stream and stream itself have
>> same rpc serial. This can lead to issues. If stream got error
>> immediately after creation then notification can be delivered
>> before API call reply arrived. This is possible because the
>> reply and the error message are sent from different threads
>> (rpc worker thread and event loop thread respectively). As
>> we don't check for message type in virNetClientCallCompleteAllWaitingReply
>> we complete the API call which leads to API call error [1]
>> as there is no actual reply. Later when reply arrives connection
>> will be closed due to protocol error (see check in
>> virNetClientCallDispatchReply).
>> Let's fix virNetClientCallCompleteAllWaitingReply.
>>
>> Queue inspection on arriving VIR_NET_CONTINUE message in
>> virNetClientCallDispatchStream is safe because there we check for
>> status field also. Queue inspection on arriving VIR_NET_OK is safe
>> too as this message can not arrive before we call virFinishAbort(Finish)
>> which is not possible before API call that creates streams returns.
>> But just to be sure let's add checking message type in these places too.
>>
>> [1] error: internal error: Unexpected message type 0
>>
>> Signed-off-by: Nikolay Shirokovskiy
>> ---
>> src/rpc/virnetclient.c | 9 ++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>
> The commit message was hard to read, but I know it's an english is not
> the first language type thing.
>
>> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
>> index 64855fb..0393587 100644
>> --- a/src/rpc/virnetclient.c
>> +++ b/src/rpc/virnetclient.c
>> @@ -1163,7 +1163,8 @@ static void
>> virNetClientCallCompleteAllWaitingReply(virNetClientPtr client)
>> virNetClientCallPtr call;
>>
>> for (call = client->waitDispatch; call; call = call->next) {
>> -if (call->msg->header.prog == client->msg.header.prog &&
>> +if (call->msg->header.type == VIR_NET_STREAM &&
>
> What about VIR_NET_STREAM_HOLE ?
Here and in the other places we care only on "dummy recv"/finish/abort messages
(only such messages have expectReply flag set) and they all have type
VIR_NET_STREAM.
>
> Hopefully someone with greater working knowledge of the processing of
> this code can help out here. I guess I'm just concerned about some odd
> processing case that may have expected to be called, but now wouldn't be
> because of this change.
>
> BTW: any thoughts to perhaps extracting out the common parts between the
> three places changed into it's own helper/method? e.g.:
>
> thecall->msg->header.prog == client->msg.header.prog &&
> thecall->msg->header.vers == client->msg.header.vers &&
> thecall->msg->header.serial == client->msg.header.serial &&
> thecall->expectReply
>
> and then the follow-up patch to add the additional '.type' checks?
>
I'm ok.
Nikolay
>
>> +call->msg->header.prog == client->msg.header.prog &&
>> call->msg->header.vers == client->msg.header.vers &&
>> call->msg->header.serial == client->msg.header.serial &&
>> call->expectReply)
>> @@ -1207,7 +1208,8 @@ static int
>> virNetClientCallDispatchStream(virNetClientPtr client)
>>
>> /* Find oldest dummy message waiting for incoming data. */
>> for (thecall = client->waitDispatch; thecall; thecall =
>> thecall->next) {
>> -if (thecall->msg->header.prog == client->msg.header.prog &&
>> +if (thecall->msg->header.type == VIR_NET_STREAM &&
>> +thecall->msg->header.prog == client->msg.header.prog &&
>> thecall->msg->header.vers == client->msg.header.vers &&
>> thecall->msg->header.serial == client->msg.header.serial &&
>> thecall->expectReply &&
>> @@ -1225,7 +1227,8 @@ static int
>> virNetClientCallDispatchStream(virNetClientPtr client)
>> case VIR_NET_OK:
>> /* Find oldest abort/finish message. */
>> for (thecall = client->waitDispatch; thecall; thecall =
>> thecall->next) {
>> -if (thecall->msg->header.prog == client->msg.header.prog &&
>> +if (thecall->msg->header.type == VIR_NET_STREAM &&
>> +thecall->msg->header.prog == client->msg.header.prog &&
>> thecall->msg->header.vers == client->msg.header.vers &&
>> thecall->msg->header.serial == client->msg.header.serial &&
>> thecall->expectReply &&
>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 0/4] qemu: Report better error on dump/migrate failure
On Wed, Feb 20, 2019 at 03:24:56PM +0100, Andrea Bolognani wrote: Changes from [v3]: * Make sure errors from virFileWrapperFdClose() result in an overall failure being bubbled up; * don't call virReportError() unless the function is going to report the failure through its return code as well; * revert b0c3e931804a more thoroughly. Changes from [v2]: * Move error reporting from virFileWrapperFdFree() to virFileWrapperFdClose(). Changes from [v1]: * Use VIR_FREE() followed by VIR_ALLOC_N() instead of manually setting the last (and only) byte of the array returned by VIR_REALLOC_N() to zero. [v3] https://www.redhat.com/archives/libvir-list/2019-February/msg01069.html [v2] https://www.redhat.com/archives/libvir-list/2019-February/msg00782.html [v1] https://www.redhat.com/archives/libvir-list/2019-February/msg00156.html Andrea Bolognani (4): util: Make it safe to call virFileWrapperFdClose() multiple times qemu: Always call virFileWrapperFdClose() util: Move error reporting back to virFileWrapperFdClose() util: Report error in virFileWrapperFdClose() src/qemu/qemu_driver.c | 16 ++-- src/util/virfile.c | 22 -- 2 files changed, 26 insertions(+), 12 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] tools: console: cleanup console on errors in main thread
On 22.02.2019 17:55, John Ferlan wrote:
>
>
> On 2/15/19 9:39 AM, Nikolay Shirokovskiy wrote:
>> We only check now for virObjectWait failures in virshRunConsole but
>> we'd better check and for other failures too. Anyway if failure
>> happened we need to shutdown console to stop delivering events
>> from event loop thread or we are in trouble as console is freed
>> on virshRunConsole exit.
>>
>> And we need to add refcounter to console object because event
>> can be delivered after we remove fd handle/stream callback.
>>
>> Signed-off-by: Nikolay Shirokovskiy
>> ---
>> tools/virsh-console.c | 161
>> +-
>> 1 file changed, 119 insertions(+), 42 deletions(-)
>>
>
> There's three things going on in this patch, let's split into convert
> virConsole to virObjectLockable, the usage of locks in the event
> callback methods, and finally handling the late event.
>
> FWIW: I could see the order being
>
> 1. Add the virMutex{Lock|Unlock} into the functions and use cleanup path
> logic since right now @con is being altered without the lock.
>
> 2. Convert to using virObjectLockable
>
> 3. Add logic to handle late event
>
>
>> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
>> index 045a636..c0c3f90 100644
>> --- a/tools/virsh-console.c
>> +++ b/tools/virsh-console.c
>> @@ -60,9 +60,10 @@ struct virConsoleBuffer {
>> typedef struct virConsole virConsole;
>> typedef virConsole *virConsolePtr;
>> struct virConsole {
>> +virObjectLockable parent;
>> +
>> virStreamPtr st;
>> bool quit;
>> -virMutex lock;
>> virCond cond;
>>
>> int stdinWatch;
>> @@ -74,6 +75,19 @@ struct virConsole {
>> char escapeChar;
>> };
>>
>> +static virClassPtr virConsoleClass;
>> +static void virConsoleDispose(void *obj);
>> +
>> +static int
>> +virConsoleOnceInit(void)
>> +{
>> +if (!VIR_CLASS_NEW(virConsole, virClassForObjectLockable()))
>> +return -1;
>> +
>> +return 0;
>> +}
>> +
>> +VIR_ONCE_GLOBAL_INIT(virConsole);
>>
>> static void
>> virConsoleHandleSignal(int sig ATTRIBUTE_UNUSED)
>> @@ -104,16 +118,14 @@ virConsoleShutdown(virConsolePtr con)
>>
>>
>> static void
>> -virConsoleFree(virConsolePtr con)
>> +virConsoleDispose(void *obj)
>> {
>> -if (!con)
>> -return;
>> +virConsolePtr con = obj;
>>
>> if (con->st)
>> virStreamFree(con->st);
>> -virMutexDestroy(>lock);
>> +
>> virCondDestroy(>cond);
>> -VIR_FREE(con);
>> }
>>
>>
>> @@ -123,6 +135,12 @@ virConsoleEventOnStream(virStreamPtr st,
>> {
>> virConsolePtr con = opaque;
>>
>> +virObjectLock(con);
>> +
>> +/* we got late event after console shutdowned */
>
> s/shutdowned/was shutdown/
>
> (repeats)
>
>> +if (!con->st)
>> +goto cleanup;
>> +
>> if (events & VIR_STREAM_EVENT_READABLE) {
>> size_t avail = con->streamToTerminal.length -
>> con->streamToTerminal.offset;
>> @@ -132,7 +150,7 @@ virConsoleEventOnStream(virStreamPtr st,
>> if (VIR_REALLOC_N(con->streamToTerminal.data,
>>con->streamToTerminal.length + 1024) < 0) {
>> virConsoleShutdown(con);
>> -return;
>> +goto cleanup;
>> }
>> con->streamToTerminal.length += 1024;
>> avail += 1024;
>> @@ -143,10 +161,10 @@ virConsoleEventOnStream(virStreamPtr st,
>> con->streamToTerminal.offset,
>> avail);
>> if (got == -2)
>> -return; /* blocking */
>> +goto cleanup; /* blocking */
>> if (got <= 0) {
>> virConsoleShutdown(con);
>> -return;
>> +goto cleanup;
>> }
>> con->streamToTerminal.offset += got;
>> if (con->streamToTerminal.offset)
>> @@ -162,10 +180,10 @@ virConsoleEventOnStream(virStreamPtr st,
>> con->terminalToStream.data,
>> con->terminalToStream.offset);
>> if (done == -2)
>> -return; /* blocking */
>> +goto cleanup; /* blocking */
>> if (done < 0) {
>> virConsoleShutdown(con);
>> -return;
>> +goto cleanup;
>> }
>> memmove(con->terminalToStream.data,
>> con->terminalToStream.data + done,
>> @@ -187,6 +205,9 @@ virConsoleEventOnStream(virStreamPtr st,
>> events & VIR_STREAM_EVENT_HANGUP) {
>> virConsoleShutdown(con);
>> }
>> +
>> + cleanup:
>> +virObjectUnlock(con);
>> }
>>
>>
>> @@ -198,6 +219,12 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>> {
>> virConsolePtr con = opaque;
>>
>> +virObjectLock(con);
>> +
>> +/* we got late event after console shutdowned */
>> +if (!con->st)
>> +goto cleanup;
>> +
>> if (events & VIR_EVENT_HANDLE_READABLE) {
>>
Re: [libvirt] [PATCH 2/2] tools: console: pass stream/fd errors to user
On 22.02.2019 18:07, John Ferlan wrote:
>
>
> On 2/15/19 9:39 AM, Nikolay Shirokovskiy wrote:
>> If console disconnected due to connection problem or problem on server
>
> If the console was disconnected due to a connection problem or a problem
> on the server
>
>> side for example it is convinient to provide the cause to the user.
>
> side it is convenient
>
>> If error comes from API then error is saved in virsh global variable
>
> If the error come from the API, then the error is save in a virsh global
> variable.
>
>> but as we return success from virshRunConsole if we reach waiting
>
> However, since success is returned from virshRunConsole and we reach the
> waiting
>
>> stage then error is never reported. Let's track for error in event loop!
>
> stage, then the error is never reported. Let's track the error in the
> event loop.
>>
>> Next after failure we do a cleanup and this cleanup can overwrite
>> root cause. Let's save root cause and then set it to virsh error
>> after all cleanup is done.
>
> [I think this paragraph could be dropped in favor of below]
I would like to keep it. It explains next construction in patch.
+
+if (ret < 0) {
+vshResetLibvirtError();
+virSetError(>error);
+vshSaveLibvirtHelperError();
+}
+
Nikolay
>
>>
>> Let's also add missing error reports in code.
>
> Written this way it feels like an extra patch; however, I know it's
> not... How about...
>
> Since we'll be sending the error to the consumer, each failure path from
> the event handlers needs to be augmented to provide what error generated
> the failure so that can be reported properly during cleanup processing
> from virshRunConsole.
>
>
>>
>> Signed-off-by: Nikolay Shirokovskiy
>> ---
>> tools/virsh-console.c | 59
>> +++
>> 1 file changed, 50 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
>> index c0c3f90..f1ad076 100644
>> --- a/tools/virsh-console.c
>> +++ b/tools/virsh-console.c
>> @@ -73,6 +73,7 @@ struct virConsole {
>> struct virConsoleBuffer terminalToStream;
>>
>> char escapeChar;
>> +virError error;
>> };
>>
>> static virClassPtr virConsoleClass;
>> @@ -98,6 +99,11 @@ virConsoleHandleSignal(int sig ATTRIBUTE_UNUSED)
>> static void
>> virConsoleShutdown(virConsolePtr con)
>> {
>> +virErrorPtr err = virGetLastError();
>> +
>> +if (con->error.code == VIR_ERR_OK && err && err->code != VIR_ERR_OK)
>> +virCopyLastError(>error);
>> +
>> if (con->st) {
>> virStreamEventRemoveCallback(con->st);
>> virStreamAbort(con->st);
>> @@ -126,6 +132,7 @@ virConsoleDispose(void *obj)
>> virStreamFree(con->st);
>>
>> virCondDestroy(>cond);
>> +virResetError(>error);
>> }
>>
>>
>> @@ -162,7 +169,13 @@ virConsoleEventOnStream(virStreamPtr st,
>> avail);
>> if (got == -2)
>> goto cleanup; /* blocking */
>> -if (got <= 0) {
>> +if (got == 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("console stream EOF"));
>> +virConsoleShutdown(con);
>> +goto cleanup;
>> +}
>> +if (got < 0) {
>
> if (got <= 0) {
> if (got == 0)
> virReportError(...)
> virConsoleShutdown
> goto cleanup
> }
>
>> virConsoleShutdown(con);
>> goto cleanup;
>> }
>> @@ -245,11 +258,14 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>> con->terminalToStream.offset,
>> avail);
>> if (got < 0) {
>> -if (errno != EAGAIN)
>> +if (errno != EAGAIN) {
>> +virReportSystemError(errno, "%s", _("can not read from
>> stdin"));
>
> "cannot" is fine (similar below)
>
> John
>
>> virConsoleShutdown(con);
>> +}
>> goto cleanup;
>> }
>> if (got == 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdin"));
>> virConsoleShutdown(con);
>> goto cleanup;
>> }
>> @@ -265,9 +281,16 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>> VIR_STREAM_EVENT_WRITABLE);
>> }
>>
>> -if (events & VIR_EVENT_HANDLE_ERROR ||
>> -events & VIR_EVENT_HANDLE_HANGUP) {
>> +if (events & VIR_EVENT_HANDLE_ERROR) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("IO error on
>> stdin"));
>> virConsoleShutdown(con);
>> +goto cleanup;
>> +}
>> +
>> +if (events & VIR_EVENT_HANDLE_HANGUP) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdin"));
>> +virConsoleShutdown(con);
>> +goto cleanup;
>> }
>>
>> cleanup:
>> @@ -297,8 +320,10 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
>>
Re: [libvirt] [PATCH] qemu: fix vcpupin fail as no vcpu tid set
On Mon, Feb 18, 2019 at 05:21:31PM +0800, Wen Yang wrote:
From: Yi Wang
vcpupin will fail when maxvcpus is larger than current
vcpu:
virsh vcpupin win7 --vcpu 0 --cpulist 5-6
error: Requested operation is not valid: cpu affinity is not supported
win7 xml in the command above is like below:
...
8
...
This issue is caused by qemuDomainRefreshVcpuInfo(), which mistake to
make validTIDs false and not set vcpu tid because vcpu[3] and vcpu[4]
have duplicate zero tids.
This problem description does not match the attempted fix.
This patch fix this.
Signed-off-by: Yi Wang
---
src/qemu/qemu_domain.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ac01e86..6987550 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10701,6 +10701,9 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
* impl which we can support.
*/
for (i = 0; i < maxvcpus && validTIDs; i++) {
+if (vm->def->virtType != VIR_DOMAIN_VIRT_QEMU)
+break;
+
This for loop was introduced by:
commit 34f77437da884f9cf0e2450f25f373b17cf527e2
qemu: fix recording of vCPU pids for MTTCG
which started recording TIDs for TCG as well.
Skipping it for VIRT_QEMU reverts the benefits for multithreaded TCG,
while leaving the loop broken for VIRT_KVM when not all vCPUs are
enabled.
The proper fix here would be to stop considering zero TIDs as duplicate.
Jano
if (info[i].tid == vm->pid) {
VIR_DEBUG("vCPU[%zu] PID %llu duplicates process",
i, (unsigned long long)info[i].tid);
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/10] blockdev: remove unused paio parameter documentation
23.02.2019 3:06, John Snow wrote: > This field isn't present anymore. > > Signed-off-by: John Snow > --- > blockdev.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/blockdev.c b/blockdev.c > index 1aaadb6128..cbce44877d 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1255,7 +1255,6 @@ out_aio_context: >* @node: The name of the BDS node to search for bitmaps >* @name: The name of the bitmap to search for >* @pbs: Output pointer for BDS lookup, if desired. Can be NULL. > - * @paio: Output pointer for aio_context acquisition, if desired. Can be > NULL. >* @errp: Output pointer for error information. Can be NULL. >* >* @return: A bitmap object on success, or NULL on failure. > Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] virfile: added GPFS as shared fs
On Mon, Feb 25, 2019 at 09:39:39 +0100, dmichelotto wrote: > Added GPFS as shared file system recognized during live migration > security checks. You could mention that GPFS stands for 'IBM General Parallel File System (GPFS)'. > > This becomes from upstream code on https://libvirt.org/git from master branch This should not be part of the commit message. > > BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528 > --- Please note that the libvirt project requires that you comply with the developer certificate of origin. See https://libvirt.org/hacking.html#patches part 6 and https://developercertificate.org/. Also we prefer if you use full name as the author when sending the patch. > src/util/virfile.c| 9 - > src/util/virfile.h| 1 + > tests/virfiledata/mounts3.txt | 1 + > tests/virfilemock.c | 5 + > tests/virfiletest.c | 1 + > 5 files changed, 16 insertions(+), 1 deletion(-) The changes look good to me but I'd prefer if you send the patch again including the sign-off and proper name. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] virfile: added GPFS as shared fs
On Mon, Feb 25, 2019 at 09:37:55 +0100, dmichelotto wrote: > Added GPFS as shared file system recognized during live migration > security checks. > > This patch is based on libvirt-4.5.0 distributed by CentOS 7.6.1810 > via source rpm. This is the libvirt project upstream mailing list. We don't take patches for centos here. Also I'm sure that the centos maintainer will accept this patch only after the upstream version is merged and the downstream backport is referencing 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 v3 07/10] block/dirty-bitmaps: unify qmp_locked and user_locked calls
23.02.2019 3:06, 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 Hmm, and here, you directly fixed what I've noticed, so my r-b, of course, still applies. Ha, but I noticed funny thing: > --- > block/dirty-bitmap.c | 40 +++--- > blockdev.c | 18 +++ > include/block/dirty-bitmap.h | 5 ++--- > migration/block-dirty-bitmap.c | 6 ++--- > nbd/server.c | 6 ++--- > 5 files changed, 34 insertions(+), 41 deletions(-) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index d92a269753..b3627b0d8c 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c [..] > @@ -265,9 +259,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState > *bs, > child->disabled = bitmap->disabled; > bitmap->disabled = true; > > -/* Install the successor and lock the parent */ > +/* Install the successor and mark the parent as busy */ I asked you to fix comment for bdrv_dirty_bitmap_create_successor(), but I didn't noticed this one, I meant the comment to the whole function, placed above it, which now have "Requires that the bitmap is not user_locked and has no successor." So, it should be updated too. and with it: Reviewed-by: Vladimir Sementsov-Ogievskiy > bitmap->successor = child; > -bitmap->qmp_locked = true; > +bitmap->busy = true; > return 0; > } > -- Best regards, Vladimir -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 08/10] block/dirty-bitmaps: move comment block
23.02.2019 3:06, 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 -- Best regards, Vladimir -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-go PATCH 0/3] Introduce recent DomainSnapshotXML and DomainSaveImageXML flags
On Mon, Feb 25, 2019 at 12:42:58PM +0100, Erik Skultety wrote: > On Mon, Feb 25, 2019 at 11:14:41AM +, Daniel P. Berrangé wrote: > > On Mon, Feb 25, 2019 at 12:09:41PM +0100, Erik Skultety wrote: > > > On Fri, Feb 22, 2019 at 04:43:25PM +, Daniel P. Berrangé wrote: > > > > On Fri, Feb 22, 2019 at 04:32:27PM +0100, Erik Skultety wrote: > > > > > Unfortunately, in order to support the new flags, the last patch > > > > > introduces an > > > > > API breakage as the convention we use for the bindings is to also > > > > > enforce types > > > > > for flags. > > > > > > > > Yes, unfortunately this need to break ABI is fallout resulting from > > > > my decision to make the Go bindings do stricter enum validation that > > > > we have had at the C level. > > > > > > How would this affect the ABI? Both the old and the new enum have the > > > same int > > > value 0x01, with the only difference that you could have fed a few more > > > enums > > > into the API even though we documented that they were unsupported for that > > > usage. > > > > Previously an application would have done > > > > dom.ManagedSaveGetXMLDesc(libvirt.DOMAIN_XML_SECURE) > > > > With your change pushed, this becomes a compile error > > > >demo.go:20:27 cannot use DOMAIN_XML_SECURE (type DomainXMLFlags) as type > > DomainSaveImageXMLFlags in argument to dom.ManagedSaveGetXMLDesc > > > > because the DomainXMLFlags enum type doesn't match the new > > DomainSaveImageXMLFlags enum type. > > Yes, but if it's a compile time error, it's static type checking, isn't that > still API breakage? I'd understand if e.g. it was a public structure and the > project we'd expand the structure or added padding where the type is still the > same, so compilation is okay, but from binary POV it would not... Oh, i missed that you wrote 'ABI' rather than 'API'. ABI is probably ok unless function param types result in symbol mangling like C++ does, but I've not checked that. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [python PATCH] Fix virDomainPinIOThread typed params check
The VIR_DOMAIN_IOTHREAD_POLL_SHRINK is unsigned int.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1680546
Signed-off-by: Pavel Hrdina
---
Pushed under trivial rule.
libvirt-override.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libvirt-override.c b/libvirt-override.c
index f7b2f6b..857c018 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -1684,7 +1684,7 @@ libvirt_virDomainPinIOThread(PyObject *self
ATTRIBUTE_UNUSED,
static virPyTypedParamsHint virPyDomainSetIOThreadParams[] = {
{ VIR_DOMAIN_IOTHREAD_POLL_MAX_NS, VIR_TYPED_PARAM_ULLONG },
{ VIR_DOMAIN_IOTHREAD_POLL_GROW, VIR_TYPED_PARAM_UINT },
-{ VIR_DOMAIN_IOTHREAD_POLL_SHRINK, VIR_TYPED_PARAM_ULLONG },
+{ VIR_DOMAIN_IOTHREAD_POLL_SHRINK, VIR_TYPED_PARAM_UINT },
};
static PyObject *
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/1] virfile: added GPFS as shared fs
Added GPFS as shared file system recognized during live migration security checks. This becomes from upstream code on https://libvirt.org/git from master branch BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528 dmichelotto (1): virfile: added GPFS as shared fs src/util/virfile.c| 9 - src/util/virfile.h| 1 + tests/virfiledata/mounts3.txt | 1 + tests/virfilemock.c | 5 + tests/virfiletest.c | 1 + 5 files changed, 16 insertions(+), 1 deletion(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/1] virfile: added GPFS as shared fs
Added GPFS as shared file system recognized during live migration security checks. This patch is based on libvirt-4.5.0 distributed by CentOS 7.6.1810 via source rpm. The rpms builded fix our bug with GPFS. BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528 dmichelotto (1): virfile: added GPFS as shared fs src/util/virfile.c| 9 - src/util/virfile.h| 1 + tests/virfiledata/mounts3.txt | 1 + tests/virfilemock.c | 5 + tests/virfiletest.c | 1 + 5 files changed, 16 insertions(+), 1 deletion(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/1] virfile: added GPFS as shared fs
Added GPFS as shared file system recognized during live migration
security checks.
This patch is based on libvirt-4.5.0 distributed by CentOS 7.6.1810
via source rpm.
The rpms builded fix our bug with GPFS.
BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528
---
src/util/virfile.c| 9 -
src/util/virfile.h| 1 +
tests/virfiledata/mounts3.txt | 1 +
tests/virfilemock.c | 5 +
tests/virfiletest.c | 1 +
5 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 716b55d..59bf619 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3537,6 +3537,9 @@ int virFilePrintf(FILE *fp, const char *msg, ...)
# ifndef FUSE_SUPER_MAGIC
# define FUSE_SUPER_MAGIC 0x65735546
# endif
+# ifndef GPFS_SUPER_MAGIC
+# define GPFS_SUPER_MAGIC 0x47504653
+# endif
# define PROC_MOUNTS "/proc/mounts"
@@ -3682,6 +3685,9 @@ virFileIsSharedFSType(const char *path,
if ((fstypes & VIR_FILE_SHFS_CIFS) &&
(f_type == CIFS_SUPER_MAGIC))
return 1;
+if ((fstypes & VIR_FILE_SHFS_GPFS) &&
+(f_type == GPFS_SUPER_MAGIC))
+return 1;
return 0;
}
@@ -3845,7 +3851,8 @@ int virFileIsSharedFS(const char *path)
VIR_FILE_SHFS_OCFS |
VIR_FILE_SHFS_AFS |
VIR_FILE_SHFS_SMB |
- VIR_FILE_SHFS_CIFS);
+ VIR_FILE_SHFS_CIFS |
+ VIR_FILE_SHFS_GPFS);
}
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 6f1e802..a26be1e 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -205,6 +205,7 @@ enum {
VIR_FILE_SHFS_AFS = (1 << 3),
VIR_FILE_SHFS_SMB = (1 << 4),
VIR_FILE_SHFS_CIFS = (1 << 5),
+VIR_FILE_SHFS_GPFS = (1 << 7),
};
int virFileIsSharedFSType(const char *path, int fstypes) ATTRIBUTE_NONNULL(1);
diff --git a/tests/virfiledata/mounts3.txt b/tests/virfiledata/mounts3.txt
index 134c6e8..a6cea9d 100644
--- a/tests/virfiledata/mounts3.txt
+++ b/tests/virfiledata/mounts3.txt
@@ -33,3 +33,4 @@ host:/nfs /nfs nfs4
rw,relatime,vers=4.1,rsize=1048576,wsize=1048576,namlen=255,
dev /nfs/blah devtmpfs
rw,nosuid,relatime,size=10240k,nr_inodes=4093060,mode=755 0 0
host:/gv0 /gluster fuse.glusterfs rw 0 0
root@host:/tmp/mkdir /gluster/sshfs fuse.sshfs rw 0 0
+gpfs_data /gpfs/data gpfs rw,relatime 0 0
diff --git a/tests/virfilemock.c b/tests/virfilemock.c
index ae5c8d0..bf721cf 100644
--- a/tests/virfilemock.c
+++ b/tests/virfilemock.c
@@ -88,6 +88,9 @@ setmntent(const char *filename, const char *type)
#ifndef FUSE_SUPER_MAGIC
# define FUSE_SUPER_MAGIC 0x65735546
#endif
+#ifndef GPFS_SUPER_MAGIC
+# define GPFS_SUPER_MAGIC 0x47504653
+#endif
static int
@@ -134,6 +137,8 @@ statfs_mock(const char *mtab,
ftype = CIFS_SUPER_MAGIC;
} else if (STRPREFIX(mb.mnt_type, "fuse")) {
ftype = FUSE_SUPER_MAGIC;
+} else if (STRPREFIX(mb.mnt_type, "gpfs")) {
+ftype = GPFS_SUPER_MAGIC;
} else {
/* Everything else is EXT4. We don't care really for other paths.
*/
ftype = EXT4_SUPER_MAGIC;
diff --git a/tests/virfiletest.c b/tests/virfiletest.c
index a246d60..9d9ab2f 100644
--- a/tests/virfiletest.c
+++ b/tests/virfiletest.c
@@ -458,6 +458,7 @@ mymain(void)
DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/gluster/file", true);
DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/gluster/sshfs/file",
false);
DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/some/symlink/file", true);
+DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/gpfs/data", true);
return ret != 0 ? EXIT_FAILURE : EXIT_SUCCESS;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/1] virfile: added GPFS as shared fs
Added GPFS as shared file system recognized during live migration
security checks.
This becomes from upstream code on https://libvirt.org/git from master branch
BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528
---
src/util/virfile.c| 9 -
src/util/virfile.h| 1 +
tests/virfiledata/mounts3.txt | 1 +
tests/virfilemock.c | 5 +
tests/virfiletest.c | 1 +
5 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 271bf5e796..615c287104 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3468,6 +3468,9 @@ int virFilePrintf(FILE *fp, const char *msg, ...)
# ifndef CEPH_SUPER_MAGIC
# define CEPH_SUPER_MAGIC 0x00C36400
# endif
+# ifndef GPFS_SUPER_MAGIC
+# define GPFS_SUPER_MAGIC 0x47504653
+# endif
# define PROC_MOUNTS "/proc/mounts"
@@ -3613,6 +3616,9 @@ virFileIsSharedFSType(const char *path,
if ((fstypes & VIR_FILE_SHFS_CEPH) &&
(f_type == CEPH_SUPER_MAGIC))
return 1;
+if ((fstypes & VIR_FILE_SHFS_GPFS) &&
+(f_type == GPFS_SUPER_MAGIC))
+return 1;
return 0;
}
@@ -3776,7 +3782,8 @@ int virFileIsSharedFS(const char *path)
VIR_FILE_SHFS_AFS |
VIR_FILE_SHFS_SMB |
VIR_FILE_SHFS_CIFS |
- VIR_FILE_SHFS_CEPH);
+ VIR_FILE_SHFS_CEPH |
+ VIR_FILE_SHFS_GPFS);
}
diff --git a/src/util/virfile.h b/src/util/virfile.h
index be612af770..0b946fe1ca 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -221,6 +221,7 @@ enum {
VIR_FILE_SHFS_SMB = (1 << 4),
VIR_FILE_SHFS_CIFS = (1 << 5),
VIR_FILE_SHFS_CEPH = (1 << 6),
+VIR_FILE_SHFS_GPFS = (1 << 7),
};
int virFileIsSharedFSType(const char *path, int fstypes) ATTRIBUTE_NONNULL(1);
diff --git a/tests/virfiledata/mounts3.txt b/tests/virfiledata/mounts3.txt
index 68eded048c..4377e5d471 100644
--- a/tests/virfiledata/mounts3.txt
+++ b/tests/virfiledata/mounts3.txt
@@ -35,3 +35,4 @@ host:/gv0 /gluster fuse.glusterfs rw 0 0
root@host:/tmp/mkdir /gluster/sshfs fuse.sshfs rw 0 0
192.168.0.1:/ceph/data /ceph ceph
rw,noatime,name=cephfs,secret=,acl,wsize=16777216 0 0
192.168.0.1,192.168.0.2,192.168.0.3:/ceph/data2 /ceph/multi ceph
rw,noatime,name=cephfs,secret=,acl,wsize=16777216 0 0
+gpfs_data /gpfs/data gpfs rw,relatime 0 0
diff --git a/tests/virfilemock.c b/tests/virfilemock.c
index 499135d773..89e14c5b67 100644
--- a/tests/virfilemock.c
+++ b/tests/virfilemock.c
@@ -89,6 +89,9 @@ setmntent(const char *filename, const char *type)
#ifndef CEPH_SUPER_MAGIC
# define CEPH_SUPER_MAGIC 0x00c36400
#endif
+#ifndef GPFS_SUPER_MAGIC
+# define GPFS_SUPER_MAGIC 0x47504653
+#endif
static int
@@ -137,6 +140,8 @@ statfs_mock(const char *mtab,
ftype = FUSE_SUPER_MAGIC;
} else if (STRPREFIX(mb.mnt_type, "ceph")) {
ftype = CEPH_SUPER_MAGIC;
+} else if (STRPREFIX(mb.mnt_type, "gpfs")) {
+ftype = GPFS_SUPER_MAGIC;
} else {
/* Everything else is EXT4. We don't care really for other paths.
*/
ftype = EXT4_SUPER_MAGIC;
diff --git a/tests/virfiletest.c b/tests/virfiletest.c
index f90c611ac4..e2bd4953ed 100644
--- a/tests/virfiletest.c
+++ b/tests/virfiletest.c
@@ -457,6 +457,7 @@ mymain(void)
DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/some/symlink/file", true);
DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/ceph/file", true);
DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/ceph/multi/file", true);
+DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/gpfs/data", true);
return ret != 0 ? EXIT_FAILURE : EXIT_SUCCESS;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-go PATCH 0/3] Introduce recent DomainSnapshotXML and DomainSaveImageXML flags
On Mon, Feb 25, 2019 at 11:14:41AM +, Daniel P. Berrangé wrote: > On Mon, Feb 25, 2019 at 12:09:41PM +0100, Erik Skultety wrote: > > On Fri, Feb 22, 2019 at 04:43:25PM +, Daniel P. Berrangé wrote: > > > On Fri, Feb 22, 2019 at 04:32:27PM +0100, Erik Skultety wrote: > > > > Unfortunately, in order to support the new flags, the last patch > > > > introduces an > > > > API breakage as the convention we use for the bindings is to also > > > > enforce types > > > > for flags. > > > > > > Yes, unfortunately this need to break ABI is fallout resulting from > > > my decision to make the Go bindings do stricter enum validation that > > > we have had at the C level. > > > > How would this affect the ABI? Both the old and the new enum have the same > > int > > value 0x01, with the only difference that you could have fed a few more > > enums > > into the API even though we documented that they were unsupported for that > > usage. > > Previously an application would have done > > dom.ManagedSaveGetXMLDesc(libvirt.DOMAIN_XML_SECURE) > > With your change pushed, this becomes a compile error > >demo.go:20:27 cannot use DOMAIN_XML_SECURE (type DomainXMLFlags) as type > DomainSaveImageXMLFlags in argument to dom.ManagedSaveGetXMLDesc > > because the DomainXMLFlags enum type doesn't match the new > DomainSaveImageXMLFlags enum type. Yes, but if it's a compile time error, it's static type checking, isn't that still API breakage? I'd understand if e.g. it was a public structure and the project we'd expand the structure or added padding where the type is still the same, so compilation is okay, but from binary POV it would not... Thanks for clarifying, Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-go PATCH 0/3] Introduce recent DomainSnapshotXML and DomainSaveImageXML flags
On Mon, Feb 25, 2019 at 12:09:41PM +0100, Erik Skultety wrote: > On Fri, Feb 22, 2019 at 04:43:25PM +, Daniel P. Berrangé wrote: > > On Fri, Feb 22, 2019 at 04:32:27PM +0100, Erik Skultety wrote: > > > Unfortunately, in order to support the new flags, the last patch > > > introduces an > > > API breakage as the convention we use for the bindings is to also enforce > > > types > > > for flags. > > > > Yes, unfortunately this need to break ABI is fallout resulting from > > my decision to make the Go bindings do stricter enum validation that > > we have had at the C level. > > How would this affect the ABI? Both the old and the new enum have the same int > value 0x01, with the only difference that you could have fed a few more enums > into the API even though we documented that they were unsupported for that > usage. Previously an application would have done dom.ManagedSaveGetXMLDesc(libvirt.DOMAIN_XML_SECURE) With your change pushed, this becomes a compile error demo.go:20:27 cannot use DOMAIN_XML_SECURE (type DomainXMLFlags) as type DomainSaveImageXMLFlags in argument to dom.ManagedSaveGetXMLDesc because the DomainXMLFlags enum type doesn't match the new DomainSaveImageXMLFlags enum type. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-go PATCH 0/3] Introduce recent DomainSnapshotXML and DomainSaveImageXML flags
On Fri, Feb 22, 2019 at 04:43:25PM +, Daniel P. Berrangé wrote: > On Fri, Feb 22, 2019 at 04:32:27PM +0100, Erik Skultety wrote: > > Unfortunately, in order to support the new flags, the last patch introduces > > an > > API breakage as the convention we use for the bindings is to also enforce > > types > > for flags. > > Yes, unfortunately this need to break ABI is fallout resulting from > my decision to make the Go bindings do stricter enum validation that > we have had at the C level. How would this affect the ABI? Both the old and the new enum have the same int value 0x01, with the only difference that you could have fed a few more enums into the API even though we documented that they were unsupported for that usage. > > On balance I think that is still the right tradeoff to have stronger > type checking as it catches real errors in app code. > > Due to the widespread use of "vendoring" in the Go community where > apps fixate on specific git commit hashes of their dependandancies, > the fallout is more limited and will mostly only impact devs at the > time they decide to explicitly sync to newer git. > > So for all three > > Reviewed-by: Daniel P. Berrangé Pushed. Thanks, Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/2] security: aa-helper: generate more rules for gl devices
On Fri, Feb 22, 2019 at 2:42 PM Jamie Strandboge wrote:
>
> On Mon, 18 Feb 2019, Christian Ehrhardt wrote:
> > +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");
>
> I'm sorry I think I wasn't clear on how to add in the .so files. I suggest:
At least I didn't make it up - I asked on apparmor channels and this
is what I got :-)
> 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");
> This is slightly futureproofed with the trailing '*'. On my system, the '**'
> wasn't needed, but if you observe systems where it is, feel free to keep it.
I checked through all of Debian/Ubuntu with apt-file and found no
cases that really need the **.
Thereby I'll take your suggestion and push it (after another round of
safety builds) with your ack (as all else was already fine).
> The other parts of this patch looked 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
