Re: [libvirt] [PATCH v4 05/14] qemu: Store pr runtime data in status XML

[John Ferlan]

On 04/10/2018 10:58 AM, Michal Privoznik wrote:
> Now that we generate pr-manager alias and socket path store them
> in status XML so that they are preserved across daemon restarts.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 64 
> ++
>  1 file changed, 64 insertions(+)
> 

So if we were to save this information (and at this point I don't think
we need to), then this is where we should be allocating and filling in
the private data (e.g. not in the previous patch).

Again as I noted previously, save the alias when printing the domain
active information and I think you're good.

AFAICT the only thing printed now (@relPath) is something generated via
qemu_driver calls (I didn't dig deep); whereas, this data is easily
regeneratable from existing domain definition data.

John


> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 361a80be84..0856f04406 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1961,13 +1961,74 @@ qemuDomainObjPrivateFree(void *data)
>  }
>  
>  
> +static int
> +qemuStorageSourcePrivateDataParsePR(xmlXPathContextPtr ctxt,
> +virStorageSourcePtr src)
> +{
> +qemuDomainStorageSourcePrivatePtr srcPriv = 
> QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +qemuDomainDiskPRDPtr prd = NULL;
> +int rc;
> +int ret = -1;
> +
> +if ((rc = virXPathBoolean("boolean(./prd)", ctxt)) == 0) {
> +return 0;
> +} else if (rc < 0) {
> +return ret;
> +}
> +
> +if (VIR_ALLOC(prd) < 0)
> +return ret;
> +
> +if (!(prd->alias = virXPathString("string(./prd/alias)", ctxt)) ||
> +!(prd->path = virXPathString("string(./prd/path)", ctxt))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("malformed "));
> +goto cleanup;
> +}
> +
> +VIR_STEAL_PTR(srcPriv->prd, prd);
> +ret = 0;
> + cleanup:
> +qemuDomainDiskPRDFree(prd);
> +return ret;
> +}
> +
> +
> +static int
> +qemuStorageSourcePrivateDataFormatPR(virStorageSourcePtr src,
> + virBufferPtr buf)
> +{
> +qemuDomainStorageSourcePrivatePtr srcPriv = 
> QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +qemuDomainDiskPRDPtr prd;
> +
> +if (!srcPriv || !srcPriv->prd)
> +return 0;
> +
> +prd = srcPriv->prd;
> +
> +virBufferAddLit(buf, "\n");
> +virBufferAdjustIndent(buf, 2);
> +virBufferAsprintf(buf, "%s\n", prd->alias);
> +virBufferEscapeString(buf, "%s\n", prd->path);
> +virBufferAdjustIndent(buf, -2);
> +virBufferAddLit(buf, "\n");
> +return 0;
> +}
> +
> +
>  static int
>  qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
>virStorageSourcePtr src)
>  {
> +if (!(src->privateData = qemuDomainStorageSourcePrivateNew()))
> +return -1;
> +
>  if (virStorageSourcePrivateDataParseRelPath(ctxt, src) < 0)
>  return -1;
>  
> +if (qemuStorageSourcePrivateDataParsePR(ctxt, src) < 0)
> +return -1;
> +
>  return 0;
>  }
>  
> @@ -1979,6 +2040,9 @@ qemuStorageSourcePrivateDataFormat(virStorageSourcePtr 
> src,
>  if (virStorageSourcePrivateDataFormatRelPath(src, buf) < 0)
>  return -1;
>  
> +if (qemuStorageSourcePrivateDataFormatPR(src, buf) < 0)
> +return -1;
> +
>  return 0;
>  }
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 04/14] qemu: Generate alias and socket path for pr-helper

[John Ferlan]

On 04/10/2018 10:58 AM, Michal Privoznik wrote:
> While we're not generating the command line just yet (look for
> the next commits), we can generate the alias for pr-manager.
> A domain can have up to one managed pr-manager (in which case
> socket path is decided by libvirt and pr-helper is spawned by
> libvirt too), but up to ndisks of unmanaged ones (one per disk
> basically). In case of the former we can generate a simple alias
> and be sure it'll not conflict. But in case of the latter to

Well if it's only one, then it had better not conflict (IOW: after the
and is unnecessary because there is check).

> avoid any conflicts let's generate alias that's based on
> something unique - like disk target.
> 

Make sure this commit message follows whatever happens in this patch...

> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt_private.syms  |  2 ++
>  src/qemu/qemu_alias.c | 11 ++
>  src/qemu/qemu_alias.h |  2 ++
>  src/qemu/qemu_domain.c| 87 
> +--
>  src/qemu/qemu_domain.h| 10 ++
>  src/util/virstoragefile.c | 15 
>  src/util/virstoragefile.h |  2 ++
>  7 files changed, 126 insertions(+), 3 deletions(-)
> 

This patch does two things - one is just the pure alias/path for the
pr-helper for the active domain. The second is copy that same
information to the private storage source, which I'm not sure (yet) why
it's necessary since both can be regenerated as needed based on fairly
static data as opposed to secinfo and encinfo which get filled in with
information only available during Prepare (the interaction with the
secret driver and the need for the @conn pointer) that wasn't available
during launch/command line building. Although some of that has changed
with more recent changes to be able to get a secret conn "on the fly".
Anyway, I digress.

The point being - please note why you're also saving something in the
private storage source area...  The 'path' is already present in the
domain XML (both active and inactive) and the 'alias' could be saved in
the active output if you tweaked virStoragePRDefFormat to match what
virDomainDeviceInfoFormat does.


> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a376e3bb0d..5b7b5514a2 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2799,7 +2799,9 @@ virStorageNetHostTransportTypeToString;
>  virStorageNetProtocolTypeToString;
>  virStoragePRDefFormat;
>  virStoragePRDefFree;
> +virStoragePRDefIsEnabled;
>  virStoragePRDefIsEqual;
> +virStoragePRDefIsManaged;
>  virStoragePRDefParseXML;
>  virStorageSourceBackingStoreClear;
>  virStorageSourceClear;
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index 95d1e0370a..6db3da27a8 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -773,3 +773,14 @@ qemuAliasChardevFromDevAlias(const char *devAlias)
>  
>  return ret;
>  }
> +
> +
> +char *
> +qemuDomainGetManagedPRAlias(void)
> +{
> +char *alias;
> +
> +ignore_value(VIR_STRDUP(alias, "pr-helper0"));
> +
> +return alias;
> +}

I don't really see a purpose for this function.  If it were to survive,
then it could take a parameter "const char *srcalias" and create/return
the alias from that based on what's in qemuDomainPrepareDiskPRD.

Eventually when the qemuProcessKillPRDaemon is created, it doesn't
necessarily distinguish between managed and unmanaged... Still having it
fail because it couldn't strdup what amounts to be a static string
doesn't make much sense.

 diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
> index 8c744138ce..91e0a9c893 100644
> --- a/src/qemu/qemu_alias.h
> +++ b/src/qemu/qemu_alias.h
> @@ -92,4 +92,6 @@ char *qemuAliasTLSObjFromSrcAlias(const char *srcAlias)
>  char *qemuAliasChardevFromDevAlias(const char *devAlias)
>  ATTRIBUTE_NONNULL(1);
>  
> +char * qemuDomainGetManagedPRAlias(void);
> +
>  #endif /* __QEMU_ALIAS_H__*/
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 5a7b5f8417..361a80be84 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -982,6 +982,18 @@ qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr 
> *secinfo)
>  }
>  
>  
> +static void
> +qemuDomainDiskPRDFree(qemuDomainDiskPRDPtr prd)
> +{
> +if (!prd)
> +return;
> +
> +VIR_FREE(prd->alias);
> +VIR_FREE(prd->path);
> +VIR_FREE(prd);
> +}
> +
> +
>  static virClassPtr qemuDomainDiskPrivateClass;
>  static void qemuDomainDiskPrivateDispose(void *obj);
>  
> @@ -1062,6 +1074,7 @@ qemuDomainStorageSourcePrivateDispose(void *obj)
>  
>  qemuDomainSecretInfoFree(>secinfo);
>  qemuDomainSecretInfoFree(>encinfo);
> +qemuDomainDiskPRDFree(priv->prd);
>  }
>  
>  
> @@ -1473,9 +1486,6 @@ 
> qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivatePtr priv,
>  if (!hasAuth && !hasEnc)
>  return 0;
>  
> -if (!(src->privateData = qemuDomainStorageSourcePrivateNew()))
> -

Re: [libvirt] [RFC v2] external (pull) backup API

[John Snow]

On 04/13/2018 05:16 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 13.04.2018 00:16, John Snow wrote:
>>
>>
>> On 04/03/2018 08:01 AM, Nikolay Shirokovskiy wrote:
>>> Hi, all.
>>>  
>>> 
>>>  
>>> This is another RFC on pull backup API. This API provides means to read 
>>> domain   
>>> disks in a snapshotted state so that client can back them up as well as 
>>> means
>>> to write domain disks to revert them to backed up state. The previous 
>>> version
>>> of RFC is [1]. I'll also describe the API implementation details to shed 
>>> light   
>>> on misc qemu dirty bitmap commands usage.   
>>>  
>>> 
>>>  
>>> This API does not use existent disks snapshots. Instead it introduces 
>>> snapshots  
>>> provided by qemu's blockdev-backup command. The reason is we need 
>>> snapshotted
>>> disk state only temporarily for duration of backup operation and newly  
>>>  
>>> introduced snapshots can be easily discarded at the end of operation 
>>> without 
>>> block commit operation. Technically difference is next. On usual snapshot 
>>> we 
>>> create new image backed by original and all new data goes to the new image 
>>> thus  
>>> original image stays in a snapshotted state. In temporary snapshots we 
>>> create
>>> new image backed by original and all new data still goes to the original 
>>> image   
>>> but before new data is written old data to be overwritten is popped out to 
>>> the new   
>>> image thus we get snapshotted state thru new image. 
>>>  
>>> 
>>>  
>>> Disks snapshots as well as disks itself are avaiable to read/write thru 
>>> qemu 
>>> NBD server. 
>>>  
>>
>> [snip!]
>>
>> Do you think it's possible to characterize this API proposal as two
>> mechanisms:
>>
>> (1) A mechanism for creating and manipulating "checkpoints" -- which are
>> book-ended by bitmap objects in QEMU -- implemented by the creation,
>> deletion, 'disabling' and 'merging' of bitmaps, and
>>
>> (2) A mechanism for the consumption of said checkpoints via NBD / the
>> "fleecing" mechanisms that allow a live export of a static view of the
>> disk at that time (block snapshots + NBD exports)
> 
> I can't share this view because checkpoints and snapshots are created
> in one transation in pull scheme so you can't not move these two to
> different mechs.
> 

That's not a problem - transactions are comprised of elementary actions,
so it's okay to make an artificial distinction between half of the
actions and half of the others if it aids in the composition of other
transaction types.

> I'll rather see 2 mechanism here at least for pull scheme.
> 
> 1. create snapshots (and optionally checkpoints)
> 2. export snapshots
> 

You're thinking more of the Libvirt API calls instead of the component
mechanisms these API manipulate, I think.

>>
>> If this is the case, do you think it is possible to consider (1) and (2)
>> somewhat orthogonal items -- in so far as it might be possible to add
>> support to libvirt directly to add push-model support for writing out
>> these checkpoints?
>>
>> i.e.
>>
>> once you have created a temporary snapshot and merged the various
>> component bitmaps into it, instead of creating an ephemeral block
>> snapshot and exporting it via NBD, we request a `blockdev-backup` with a
>> libvirt-specified target instead?
>>
>> You don't have to add support for this right away, but I would really
>> enjoy if any API we check in here has the capacity to support both
>> push-and-pull paradigms without getting too ugly.
>>
>> Does that sound like it can easily fit in with your designs so far?
>>
> 
> I think push scheme require 3rd (1st is fleece snapshots, 2nd is exporting
> snapshots) API. First push backup has nothing to do with exporting of course. 
> Second
> contrary to fleece snapshots it will require additional parameter - a 
> checkpoint
> in past in case of incremental backup. It also have quite different image 
> parameter.
> In case of fleece snapshot fleece image will only store small delta even in 
> case of
> full backups. In case of push backup image will store full disk in case of 
> full
> backups.
> 
> Nikolay
> 

That doesn't sound too crazy. As long as the idea of a "checkpoint" can
be re-used for 

Re: [libvirt] [PATCH v2 59/73] qemu: Add support for sending capabilities in migration cookie

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:49PM +0200, Jiri Denemark wrote:

Some migration capabilities may be enabled automatically, but only if
both sides of migration support them. Thus we need to be able transfer
the list of supported migration capabilities in migration cookie.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration.c|  15 ++--
src/qemu/qemu_migration_cookie.c | 136 ++-
src/qemu/qemu_migration_cookie.h |  15 
src/qemu/qemu_migration_params.c |  20 +
src/qemu/qemu_migration_params.h |   3 +
5 files changed, 183 insertions(+), 6 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/2] vmware: Fix initialization of VMware Fusion

[Rainer Müller]

On 2018-04-11 14:02, John Ferlan wrote:
> 
> 
> On 04/03/2018 05:32 AM, Rainer Müller wrote:
>> The vmware driver wants to execute vmware-vmx from the same directory in
>> which vmrun was found. However, on VMware Fusion 10 vmrun at
>> /Applications/VMware Fusion.app/Contents/Public/vmrun is a symlink
>> pointing to ../Library/vmrun. vmware-vmx cannot be found, as
>> it is not in PATH, but only in this Library directory.
>>
>> Therefore, follow the vmrun symlink and use the resulting path. Then the
>> assumption that vmware-vmx is right next to it will still work.
>>
>> Signed-off-by: Rainer Müller 
>> ---
>>  src/vmware/vmware_driver.c | 9 -
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
>> index 8b487c4a7..60e1c1abc 100644
>> --- a/src/vmware/vmware_driver.c
>> +++ b/src/vmware/vmware_driver.c
>> @@ -127,6 +127,7 @@ vmwareConnectOpen(virConnectPtr conn,
>>  struct vmware_driver *driver;
>>  size_t i;
>>  char *tmp;
>> +char *vmrun = NULL;
>>  
>>  virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
>>  
>> @@ -164,7 +165,12 @@ vmwareConnectOpen(virConnectPtr conn,
>>   * for auto detection of the backend
>>   */
>>  for (i = 0; i < ARRAY_CARDINALITY(vmrun_candidates); i++) {
>> -driver->vmrun = virFindFileInPath(vmrun_candidates[i]);
>> +vmrun = virFindFileInPath(vmrun_candidates[i]);
> 
> What if this returns NULL?
> 
>> +if (virFileResolveLink(vmrun, >vmrun) < 0) {
> 
> I doubt this will be very happy...
> 
>> +virReportSystemError(errno, _("unable to resolve symlink 
>> '%s'"), vmrun);
>> +goto cleanup;
>> +}
>> +VIR_FREE(vmrun);
> 
> Prior to this change - if @vmrun wasn't found in the path of
> vmrun_candidates, we'd try the "next one" in the list.>
> With this change if @vmrun returned from virFindFileInPath is NULL, then
> virFileResolveLink would fail, and we wouldn't try the next one in the list.

Thank you for the review. Indeed, this was sloppy testing on my end.
I will prepare a v2.

> There is also virFileIsLink which you may want to consider - as in, what
> we got back is a link, so let's resolve to save it; otherwise, use what
> was found in path.

virFileResolveLink already calls lstat(2) to check whether the given
path is actually link. If it is not, the output is a copy of the input.
So this will already work in the way you describe.

Rainer

> John
> 
>>  /* If we found one, we can stop looking */
>>  if (driver->vmrun)
>>  break;
>> @@ -215,6 +221,7 @@ vmwareConnectOpen(virConnectPtr conn,
>>  
>>   cleanup:
>>  vmwareFreeDriver(driver);
>> +VIR_FREE(vmrun);
>>  return VIR_DRV_OPEN_ERROR;
>>  };
>>  
>>
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 02/14] qemuDomainDiskChangeSupported: Deny changing reservations

[John Ferlan]

On 04/10/2018 10:58 AM, Michal Privoznik wrote:
> Couple of reasons for that:
> 
> a) there's no monitor command to change path where the pr-helper
> connects to, or
> b) there's no monitor command to introduce a new pr-helper for a
> disk that already exists.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt_private.syms  |  1 +
>  src/qemu/qemu_domain.c|  8 
>  src/util/virstoragefile.c | 18 ++
>  src/util/virstoragefile.h |  2 ++
>  4 files changed, 29 insertions(+)
> 

[...]

> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 9917837513..b017024b2f 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -2022,6 +2022,24 @@ virStoragePRDefFormat(virBufferPtr buf,
>  }
>  
>  
> +bool
> +virStoragePRDefIsEqual(virStoragePRDefPtr a,
> +   virStoragePRDefPtr b)
> +{
> +if (!a && !b)
> +return true;
> +
> +if (!a || !b)
> +return false;
> +
> +if (a->enabled != b->enabled ||
> +a->managed != b->managed ||
> +STRNEQ_NULLABLE(a->path, b->path))
> +return false;
> +
> +return true;
> +}
> +

two blank lines


[...]

Reviewed-by: John Ferlan 

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] external (pull) backup API

[John Snow]

On 04/13/2018 08:01 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 1. It looks unsafe to use nbd server + backup(sync=none) on same node,
>>> synchronization is needed, like in block/replication, which uses
>>> backup_wait_for_overlapping_requests, backup_cow_request_begin,
>>> backup_cow_request_end. We have a filter driver for this thing, not yet
>>> in upstream.
>> Is it the case that blockdev-backup sync=none can race with read
>> requests on the NBD server?
>>
>> i.e. we can get temporarily inconsistent data before the COW completes?
>> Can you elaborate?
> 
> I'm not sure but looks possible:
> 
> 1. start NBD read, find that there is a hole in temporary image, decide
> to read from active image (or even start read) and yield
> 2. guest writes to the same are (COW happens, but it doesn't help)
> 3. reduce point (1.), read invalid (already updated by 2.) data
> 
> And similar place in block/replication, which uses backup(sync=none) too
> is protected from such situation.

I'll have to look into this one -- were you seeing problems in practice
before you implemented your proprietary filter node?

--js

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 58/73] qemu: Move qemuMonitorMigrationCaps enum

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:48PM +0200, Jiri Denemark wrote:

Since the monitor code no longer needs to see this enum, we move it
to the place where migration parameters are defined and drop the
"monitor" reference from the name.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_driver.c   |  4 +-
src/qemu/qemu_migration_params.c | 65 +++-
src/qemu/qemu_migration_params.h | 15 +++-
src/qemu/qemu_monitor.c  |  5 ---
src/qemu/qemu_monitor.h  | 14 ---
tests/qemumonitorjsontest.c  |  7 ++--
6 files changed, 58 insertions(+), 52 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 57/73] qemu: Move migration capabilities JSON formatting

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:47PM +0200, Jiri Denemark wrote:

We want to have all migration capabilities parsing and formatting at one
place, i.e., in qemu_migration_params.c. The parsing is already there in
qemuMigrationCapsCheck.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration_params.c | 66 ++--
src/qemu/qemu_migration_paramspriv.h |  4 ++
src/qemu/qemu_monitor.c  | 27 +++-
src/qemu/qemu_monitor.h  |  3 +-
src/qemu/qemu_monitor_json.c | 40 ++---
src/qemu/qemu_monitor_json.h |  3 +-
tests/qemumonitorjsontest.c  | 11 +++--
7 files changed, 96 insertions(+), 58 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 55/73] qemu: Move qemuMonitorMigrationParams structure

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:45PM +0200, Jiri Denemark wrote:

It's no longer used by the monitor code so we can hide it inside
qemu_migration_params.c.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration_params.c | 36 
src/qemu/qemu_monitor.h  | 36 
2 files changed, 36 insertions(+), 36 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 54/73] qemu: Export qemuMigrationParams{To, From}JSON for tests

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:44PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---
src/qemu/Makefile.inc.am |  1 +
src/qemu/qemu_migration_params.c |  5 +++--
src/qemu/qemu_migration_paramspriv.h | 31 
3 files changed, 35 insertions(+), 2 deletions(-)
create mode 100644 src/qemu/qemu_migration_paramspriv.h



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 52/73] qemu: Move migration parameters JSON parsing

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:42PM +0200, Jiri Denemark wrote:

We want to have all migration parameters parsing and formatting at once
place, i.e., in qemu_migration_params.c.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration_params.c | 74 +---
src/qemu/qemu_monitor.c  | 13 +-
src/qemu/qemu_monitor.h  |  2 +-
src/qemu/qemu_monitor_json.c | 51 ++
src/qemu/qemu_monitor_json.h |  2 +-
5 files changed, 85 insertions(+), 57 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 51/73] util: Introduce virJSONValueObjectStealObject

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:41PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---
src/libvirt_private.syms | 1 +
src/util/virjson.c   | 8 
src/util/virjson.h   | 2 ++
3 files changed, 11 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 50/73] qemumonitorjsontest: Drop migration params test

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:40PM +0200, Jiri Denemark wrote:

The test is mostly useless and we want to refactor migration parameters
even further. The refactoring will allow us to introduce enhanced tests
for migration parameters.

Signed-off-by: Jiri Denemark 
---
tests/qemumonitorjsontest.c | 99 -
1 file changed, 99 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] external (pull) backup API

[Vladimir Sementsov-Ogievskiy]

13.04.2018 18:05, Nikolay Shirokovskiy wrote:


On 13.04.2018 14:41, Vladimir Sementsov-Ogievskiy wrote:

13.04.2018 11:51, Nikolay Shirokovskiy wrote:

On 13.04.2018 03:04, John Snow wrote:

On 04/12/2018 10:08 AM, Vladimir Sementsov-Ogievskiy wrote:

I propose, not to say that bitmap represents a checkpoint. It is simpler
to say (and it reflects the reality) that bitmap is a difference between
two consecutive checkpoints. And we can say, that active state is some
kind of a checkpoint, current point in time.

So, we have checkpoints (5* is an active state) which are points in time:

1 2 3 4 5*


Oh -- the most recent checkpoint there doesn't belong to a ***specific
time*** yet. It's a floating checkpoint -- it always represents the most
current version. It's not really a checkpoint at all.

1, 2, 3, and 4 however are associated with a specific timestamp though.


And bitmaps, first three are disabled, last is enabled:

"1->2", "2->3", "3->4", "4->5*"


OK; so 1->2, 2->3 and 3->4 define deltas between two ***defined***
points in time.

4->5* however is only anchored by one specific point in time, and is
floating just like the most recent checkpoint is floating.


So, remove first checkpoint: just remove bitmap "A->B".

I assume you mean "1->2" here.

And... yes, I agree -- if you don't care about your very first
checkpoint anymore, you can just delete the first bitmap, too.


Remove any other checkpoint N: create new bitmap "(N-1)->(N+1)" =
merge("(N-1)->N", "N->(N+1)"), drop bitmaps "(N-1)->N" and "N->(N+1)".

err, okay, so let's say we want to drop checkpoint 3:

create: "2->4"
merge: "2->3", "3->4" [and presumably store in "2->4"]
drop: 2->3, 3->4

OK, that makes more sense to me. In essence;

(1) We could consider this 2->3 absorbing 3->4, or
(2) 3->4 absorbing 2->3

and in either case it's the same, really.


If the latter was active, the new one becomes active. And we cant remove
5* checkpoint, as it is an active state, not an actual checkpoint.

OK, crystal.

--js


I prefer not talking of active checkpoint. It is a kind of controversial.
Better just keep in mind that last bitmap is active one. So for checkpoints 1 2 
3 4
we have bitmaps:

1 1->2 2->3 3->4

Note the first bitmap name. When it was created name 2 was unknown so we'd 
better
have this name for the first bitmap.

so here, 1->2 is a difference between checkpoints 2 and 3? I think it's 
unnatural.. Ofcource, when we create new active bitmap, we don't know the name of 
next checkpoint, but, why not rename it when we create next checkpoint?

So,

1. have no checkpoints and bitmaps
2. create new checkpoint 1, and bitmap 1->?
3. create new checkpoint 2 and bitmap 2->?, disable bitmap 1->? and rename it to 
1->2
and so on.

this reflects the essence of things

Makes sense and I don't see any issue from implementation POV. I would only use > only 
or >> (or whatever times >)
instead of ->. This makes possible to restrict names to prohibit > only. - is 
typical for UUIDs.




in this case, I think just > is ok. Less symbols - less 
electricity/paper/time overhead)


And more. This do not look like a hack (may be a bit=)

Why not to call the bitmap, representing difference between snapshots A 
and B: A>B?


--
Best regards,
Vladimir

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 47/73] qemu: Drop qemuMigrationCompression structure

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:37PM +0200, Jiri Denemark wrote:

By merging qemuMigrationAnyCompressionParse into
qemuMigrationParamsSetCompression we can drop the useless intermediate
qemuMigrationCompression structure and parse compression related typed
parameters and flags directly into qemuMigrationParams.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_driver.c   |  63 ++-
src/qemu/qemu_migration_params.c | 181 +--
src/qemu/qemu_migration_params.h |  26 +
3 files changed, 85 insertions(+), 185 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] external (pull) backup API

[Nikolay Shirokovskiy]

On 13.04.2018 14:41, Vladimir Sementsov-Ogievskiy wrote:
> 13.04.2018 11:51, Nikolay Shirokovskiy wrote:
>>
>> On 13.04.2018 03:04, John Snow wrote:
>>>
>>> On 04/12/2018 10:08 AM, Vladimir Sementsov-Ogievskiy wrote:
 I propose, not to say that bitmap represents a checkpoint. It is simpler
 to say (and it reflects the reality) that bitmap is a difference between
 two consecutive checkpoints. And we can say, that active state is some
 kind of a checkpoint, current point in time.

 So, we have checkpoints (5* is an active state) which are points in time:

 1 2 3 4 5*

>>> Oh -- the most recent checkpoint there doesn't belong to a ***specific
>>> time*** yet. It's a floating checkpoint -- it always represents the most
>>> current version. It's not really a checkpoint at all.
>>>
>>> 1, 2, 3, and 4 however are associated with a specific timestamp though.
>>>
 And bitmaps, first three are disabled, last is enabled:

 "1->2", "2->3", "3->4", "4->5*"

>>> OK; so 1->2, 2->3 and 3->4 define deltas between two ***defined***
>>> points in time.
>>>
>>> 4->5* however is only anchored by one specific point in time, and is
>>> floating just like the most recent checkpoint is floating.
>>>
 So, remove first checkpoint: just remove bitmap "A->B".
>>> I assume you mean "1->2" here.
>>>
>>> And... yes, I agree -- if you don't care about your very first
>>> checkpoint anymore, you can just delete the first bitmap, too.
>>>
 Remove any other checkpoint N: create new bitmap "(N-1)->(N+1)" =
 merge("(N-1)->N", "N->(N+1)"), drop bitmaps "(N-1)->N" and "N->(N+1)".
>>> err, okay, so let's say we want to drop checkpoint 3:
>>>
>>> create: "2->4"
>>> merge: "2->3", "3->4" [and presumably store in "2->4"]
>>> drop: 2->3, 3->4
>>>
>>> OK, that makes more sense to me. In essence;
>>>
>>> (1) We could consider this 2->3 absorbing 3->4, or
>>> (2) 3->4 absorbing 2->3
>>>
>>> and in either case it's the same, really.
>>>
 If the latter was active, the new one becomes active. And we cant remove
 5* checkpoint, as it is an active state, not an actual checkpoint.
>>> OK, crystal.
>>>
>>> --js
>>>
>> I prefer not talking of active checkpoint. It is a kind of controversial.
>> Better just keep in mind that last bitmap is active one. So for checkpoints 
>> 1 2 3 4
>> we have bitmaps:
>>
>> 1 1->2 2->3 3->4
>>
>> Note the first bitmap name. When it was created name 2 was unknown so we'd 
>> better
>> have this name for the first bitmap.
> 
> so here, 1->2 is a difference between checkpoints 2 and 3? I think it's 
> unnatural.. Ofcource, when we create new active bitmap, we don't know the 
> name of next checkpoint, but, why not rename it when we create next 
> checkpoint?
> 
> So,
> 
> 1. have no checkpoints and bitmaps
> 2. create new checkpoint 1, and bitmap 1->?
> 3. create new checkpoint 2 and bitmap 2->?, disable bitmap 1->? and rename it 
> to 1->2
> and so on.
> 
> this reflects the essence of things

Makes sense and I don't see any issue from implementation POV. I would only use 
> only or >> (or whatever times >)
instead of ->. This makes possible to restrict names to prohibit > only. - is 
typical for UUIDs.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 41/73] qemu: Generalize macro for getting VIR_MIGRATE_* typed params

[Jiri Denemark]

On Fri, Apr 13, 2018 at 16:22:00 +0200, Ján Tomko wrote:
> On Wed, Apr 11, 2018 at 04:41:31PM +0200, Jiri Denemark wrote:
> >So far it's used only for CPU throttling parameters which are all ints,
> >but we'll soon want to use it for more parameters with different types.
> >
> >Signed-off-by: Jiri Denemark 
> >---
> > src/qemu/qemu_migration_params.c | 31 +++
> > 1 file changed, 15 insertions(+), 16 deletions(-)
> >
> >diff --git a/src/qemu/qemu_migration_params.c 
> >b/src/qemu/qemu_migration_params.c
> >index 369e560990..e0cbdb1a38 100644
> >--- a/src/qemu/qemu_migration_params.c
> >+++ b/src/qemu/qemu_migration_params.c
> >@@ -112,6 +112,17 @@ qemuMigrationParamsFree(qemuMigrationParamsPtr 
> >migParams)
> > }
> >
> >
> >+#define GET(API, PARAM, VAR) \
> >+do { \
> >+int rc; \
> >+if ((rc = API(params, nparams, VIR_MIGRATE_PARAM_ ## PARAM, \
> >+  >params.VAR)) < 0) \
> >+goto error; \
> >+ \
> >+if (rc == 1) \
> >+migParams->params.VAR ## _set = true; \
> >+} while (0)
> >+
> > qemuMigrationParamsPtr
> > qemuMigrationParamsFromFlags(virTypedParameterPtr params,
> >  int nparams,
> >@@ -135,27 +146,13 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr 
> >params,
> > }
> > }
> >
> >-#define GET(PARAM, VAR) \
> >-do { \
> >-int rc; \
> >-if ((rc = virTypedParamsGetInt(params, nparams, \
> >-   VIR_MIGRATE_PARAM_ ## PARAM, \
> >-   >params.VAR)) < 0) \
> >-goto error; \
> >- \
> >-if (rc == 1) \
> >-migParams->params.VAR ## _set = true; \
> >-} while (0)
> >-
> > if (params) {
> > if (party == QEMU_MIGRATION_SOURCE) {
> >-GET(AUTO_CONVERGE_INITIAL, cpuThrottleInitial);
> >-GET(AUTO_CONVERGE_INCREMENT, cpuThrottleIncrement);
> >+GET(virTypedParamsGetInt, AUTO_CONVERGE_INITIAL, 
> >cpuThrottleInitial);
> >+GET(virTypedParamsGetInt, AUTO_CONVERGE_INCREMENT, 
> >cpuThrottleIncrement);
> 
> If all the getter APIs start with virTypedParamsGet, you can hide the
> prefix inside the macro definition to save columns.

The macro will go away completely later in this series anyway.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt PATCH v2 00/4] Require QEMU 1.5.0

[Ján Tomko]

On Thu, Apr 05, 2018 at 02:22:51PM +0200, Ján Tomko wrote:

v2:
* Change the version from 1.3.0 to 1.5.0
* Mention this breaks RHEL 6 QEMU and list the versions
in relevant long-term releases

Some of the patches are very big, available in my repo:
https://repo.or.cz/libvirt/jtomko.git/shortlog/refs/heads/caps_cleanup_v2
git fetch git://repo.or.cz/libvirt/jtomko.git caps_cleanup_v2:



v3: rebased on top of master (with minor .args conflicts fixed)

http://repo.or.cz/libvirt/jtomko.git/shortlog/refs/heads/caps_cleanup_v3
git fetch git://repo.or.cz/libvirt/jtomko.git caps_cleanup_v3:

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 7/9] virobject: Introduce VIR_CLASS_NEW() macro

[Michal Privoznik]

So far we are repeating the following lines over and over:

  virClassNew(virClassForObject(),
  "virSomeObject",
  sizeof(virSomeObject),
  virSomeObjectDispose);

While this works, it is impossible to do some checking. Firstly,
the class name (the 2nd argument) doesn't match the name in the
code in all cases (the 3rd argument). Secondly, the current style
is needlessly verbose. This commit turns example into following:

  VIR_CLASS_NEW(virClassForObject(),
virSomeObject);

Signed-off-by: Michal Privoznik 
---
 src/access/viraccessmanager.c   |   6 +-
 src/bhyve/bhyve_conf.c  |   6 +-
 src/conf/capabilities.c |   6 +-
 src/conf/domain_capabilities.c  |  12 +--
 src/conf/domain_conf.c  |  18 ++---
 src/conf/domain_event.c | 126 +++-
 src/conf/network_event.c|  12 +--
 src/conf/node_device_event.c|  18 ++---
 src/conf/object_event.c |  12 +--
 src/conf/secret_event.c |  18 ++---
 src/conf/storage_event.c|  18 ++---
 src/conf/virdomainobjlist.c |   6 +-
 src/conf/virinterfaceobj.c  |  12 +--
 src/conf/virnetworkobj.c|  12 +--
 src/conf/virnodedeviceobj.c |  12 +--
 src/conf/virsecretobj.c |  12 +--
 src/conf/virstorageobj.c|  24 ++
 src/datatypes.c |   6 +-
 src/interface/interface_backend_netcf.c |   6 +-
 src/libvirt-admin.c |   6 +-
 src/libxl/libxl_conf.c  |   6 +-
 src/libxl/libxl_domain.c|   6 +-
 src/libxl/libxl_migration.c |   6 +-
 src/logging/log_handler.c   |   6 +-
 src/lxc/lxc_conf.c  |   6 +-
 src/lxc/lxc_monitor.c   |   6 +-
 src/node_device/node_device_udev.c  |   6 +-
 src/qemu/qemu_agent.c   |   6 +-
 src/qemu/qemu_capabilities.c|   6 +-
 src/qemu/qemu_conf.c|   6 +-
 src/qemu/qemu_domain.c  |  36 +++--
 src/qemu/qemu_monitor.c |   6 +-
 src/rpc/virkeepalive.c  |   6 +-
 src/rpc/virnetclient.c  |   6 +-
 src/rpc/virnetclientprogram.c   |   6 +-
 src/rpc/virnetclientstream.c|   6 +-
 src/rpc/virnetdaemon.c  |   6 +-
 src/rpc/virnetlibsshsession.c   |   6 +-
 src/rpc/virnetsaslcontext.c |  12 +--
 src/rpc/virnetserver.c  |   6 +-
 src/rpc/virnetserverclient.c|   6 +-
 src/rpc/virnetserverprogram.c   |   6 +-
 src/rpc/virnetserverservice.c   |   6 +-
 src/rpc/virnetsocket.c  |   6 +-
 src/rpc/virnetsshsession.c  |   6 +-
 src/rpc/virnettlscontext.c  |  12 +--
 src/security/security_manager.c |   6 +-
 src/util/virclosecallbacks.c|   6 +-
 src/util/virdnsmasq.c   |   7 +-
 src/util/virfdstream.c  |   6 +-
 src/util/virfilecache.c |   6 +-
 src/util/virhash.c  |   6 +-
 src/util/virhostdev.c   |   6 +-
 src/util/viridentity.c  |   6 +-
 src/util/virmacmap.c|   6 +-
 src/util/virmdev.c  |   6 +-
 src/util/virobject.c|  12 +--
 src/util/virobject.h|   4 +
 src/util/virpci.c   |   6 +-
 src/util/virportallocator.c |   6 +-
 src/util/virresctrl.c   |  12 +--
 src/util/virscsi.c  |   6 +-
 src/util/virscsivhost.c |   6 +-
 src/util/virusb.c   |   6 +-
 src/vbox/vbox_common.c  |   6 +-
 src/vz/vz_driver.c  |   6 +-
 tests/virfilecachetest.c|   6 +-
 67 files changed, 230 insertions(+), 453 deletions(-)

diff --git a/src/access/viraccessmanager.c b/src/access/viraccessmanager.c
index c268ec57f7..2940692598 100644
--- a/src/access/viraccessmanager.c
+++ b/src/access/viraccessmanager.c
@@ -54,10 +54,8 @@ static void virAccessManagerDispose(void *obj);
 
 static int virAccessManagerOnceInit(void)
 {
-if (!(virAccessManagerClass = virClassNew(virClassForObjectLockable(),
-  "virAccessManagerClass",
-  sizeof(virAccessManager),
-  virAccessManagerDispose)))
+if (!(virAccessManagerClass = VIR_CLASS_NEW(virClassForObjectLockable(),
+virAccessManager)))
 return -1;
 
 return 0;
diff --git a/src/bhyve/bhyve_conf.c b/src/bhyve/bhyve_conf.c
index b0b40c5754..027311ad37 100644
--- a/src/bhyve/bhyve_conf.c
+++ b/src/bhyve/bhyve_conf.c
@@ 

[libvirt] [PATCH 8/9] virobject: Check if @parent is the first member in class

[Michal Privoznik]

Our virObject code relies heavily on the fact that the first
member of the class struct is type of virObject (or some
derivation of if). Let's check for that.

Signed-off-by: Michal Privoznik 
---
 src/util/virobject.c | 31 +--
 src/util/virobject.h |  5 -
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/src/util/virobject.c b/src/util/virobject.c
index c5a98d21cc..e184f5349e 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -77,6 +77,7 @@ virObjectOnceInit(void)
 {
 if (!(virObjectClass = virClassNew(NULL,
"virObject",
+   0,
sizeof(virObject),
NULL)))
 return -1;
@@ -159,21 +160,31 @@ virClassForObjectRWLockable(void)
 virClassPtr
 virClassNew(virClassPtr parent,
 const char *name,
+size_t off,
 size_t objectSize,
 virObjectDisposeCallback dispose)
 {
 virClassPtr klass;
 
-if (parent == NULL &&
-STRNEQ(name, "virObject")) {
-virReportInvalidNonNullArg(parent);
-return NULL;
-} else if (parent &&
-   objectSize <= parent->objectSize) {
-virReportInvalidArg(objectSize,
-_("object size %zu of %s is smaller than parent 
class %zu"),
-objectSize, name, parent->objectSize);
-return NULL;
+if (parent == NULL) {
+if (STRNEQ(name, "virObject")) {
+virReportInvalidNonNullArg(parent);
+return NULL;
+}
+} else {
+if (objectSize <= parent->objectSize) {
+virReportInvalidArg(objectSize,
+_("object size %zu of %s is smaller than 
parent class %zu"),
+objectSize, name, parent->objectSize);
+return NULL;
+}
+
+if (off) {
+virReportInvalidArg(off,
+_("struct %s doesn't have 'parent' as its 
first member"),
+name);
+return NULL;
+}
 }
 
 if (VIR_ALLOC(klass) < 0)
diff --git a/src/util/virobject.h b/src/util/virobject.h
index 92dd512399..25db3a0c22 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -76,11 +76,14 @@ virClassPtr virClassForObjectRWLockable(void);
 # endif
 
 # define VIR_CLASS_NEW(prnt, name) \
-virClassNew(prnt, #name, sizeof(name), name##Dispose)
+virClassNew(prnt, #name, \
+offsetof(name, parent), \
+sizeof(name), name##Dispose)
 
 virClassPtr
 virClassNew(virClassPtr parent,
 const char *name,
+size_t off,
 size_t objectSize,
 virObjectDisposeCallback dispose)
 VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(2);
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 6/9] Introduce virNetSASLContextDispose

[Michal Privoznik]

Strictly speaking this is not needed right now. However, next
commits will require dispose function to exist.

Signed-off-by: Michal Privoznik 
---
 src/rpc/virnetsaslcontext.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/rpc/virnetsaslcontext.c b/src/rpc/virnetsaslcontext.c
index a7b891feb6..f1f8bdc855 100644
--- a/src/rpc/virnetsaslcontext.c
+++ b/src/rpc/virnetsaslcontext.c
@@ -52,6 +52,7 @@ struct _virNetSASLSession {
 
 static virClassPtr virNetSASLContextClass;
 static virClassPtr virNetSASLSessionClass;
+static void virNetSASLContextDispose(void *obj);
 static void virNetSASLSessionDispose(void *obj);
 
 static int virNetSASLContextOnceInit(void)
@@ -59,7 +60,7 @@ static int virNetSASLContextOnceInit(void)
 if (!(virNetSASLContextClass = virClassNew(virClassForObjectLockable(),
"virNetSASLContext",
sizeof(virNetSASLContext),
-   NULL)))
+   virNetSASLContextDispose)))
 return -1;
 
 if (!(virNetSASLSessionClass = virClassNew(virClassForObjectLockable(),
@@ -686,6 +687,11 @@ ssize_t virNetSASLSessionDecode(virNetSASLSessionPtr sasl,
 return ret;
 }
 
+void virNetSASLContextDispose(void *obj ATTRIBUTE_UNUSED)
+{
+/* nada */
+}
+
 void virNetSASLSessionDispose(void *obj)
 {
 virNetSASLSessionPtr sasl = obj;
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 9/9] cfg.mk: Introduce syntax-check rule to prefer VIR_CLASS_NEW

[Michal Privoznik]

Now that we have macro that does some checks lets forbid raw
usage of virClassNew() in favor of VIR_CLASS_NEW().

Signed-off-by: Michal Privoznik 
---
 cfg.mk | 8 
 1 file changed, 8 insertions(+)

diff --git a/cfg.mk b/cfg.mk
index 4078bc2c63..5b7a9728d2 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -321,6 +321,11 @@ sc_prohibit_internal_functions:
halt='use VIR_ macros instead of internal functions' \
  $(_sc_search_regexp)
 
+sc_prohibit_raw_virclassnew:
+   @prohibit='virClassNew *\(' \
+   halt='use VIR_CLASS_NEW macro instead of virClassNew' \
+ $(_sc_search_regexp)
+
 # Avoid raw malloc and free, except in documentation comments.
 sc_prohibit_raw_allocation:
@prohibit='^.[^*].*\<((m|c|re)alloc|free) *\([^)]' \
@@ -1188,6 +1193,9 @@ exclude_file_name_regexp--sc_prohibit_gethostname = 
^src/util/vir(util|log)\.c$$
 exclude_file_name_regexp--sc_prohibit_internal_functions = \
   ^src/(util/(viralloc|virutil|virfile)\.[hc]|esx/esx_vi\.c)$$
 
+exclude_file_name_regexp--sc_prohibit_raw_virclassnew = \
+  ^src/util/virobject\.[hc]$$
+
 exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \
   ^src/rpc/gendispatch\.pl$$
 
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/9] domain_event: s/MetadataCange/MetadataChange/g

[Michal Privoznik]

There's a typo in struct name.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_event.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 7baccd5b57..fdb48a1eaa 100644
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -270,13 +270,13 @@ struct _virDomainEventDeviceRemovalFailed {
 typedef struct _virDomainEventDeviceRemovalFailed 
virDomainEventDeviceRemovalFailed;
 typedef virDomainEventDeviceRemovalFailed 
*virDomainEventDeviceRemovalFailedPtr;
 
-struct _virDomainEventMetadataCange {
+struct _virDomainEventMetadataChange {
 virDomainEvent parent;
 
 int type;
 char *nsuri;
 };
-typedef struct _virDomainEventMetadataCange virDomainEventMetadataChange;
+typedef struct _virDomainEventMetadataChange virDomainEventMetadataChange;
 typedef virDomainEventMetadataChange *virDomainEventMetadataChangePtr;
 
 struct _virDomainEventBlockThreshold {
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/9] virClassNew rework

[Michal Privoznik]

This is inspired by commit of f574e2e5214fb9. Also, the more the code is
unified the easier it is to maintain.

Michal Privoznik (9):
  domain_event: s/MetadataCange/MetadataChange/g
  util: Make structs follow our naming convention
  datatypes: Rename @parent to @parentName in virNodeDevice
  src: Unify virObject member name
  src: Unify dispose function names
  Introduce virNetSASLContextDispose
  virobject: Introduce VIR_CLASS_NEW() macro
  virobject: Check if @parent is the first member in class
  cfg.mk: Introduce syntax-check rule to prefer VIR_CLASS_NEW

 cfg.mk  |   8 ++
 src/access/viraccessmanager.c   |   6 +-
 src/bhyve/bhyve_conf.c  |   6 +-
 src/conf/capabilities.c |  10 +--
 src/conf/domain_capabilities.c  |  12 +--
 src/conf/domain_conf.c  |  22 ++
 src/conf/domain_event.c | 130 +++-
 src/conf/network_event.c|  12 +--
 src/conf/node_device_event.c|  18 ++---
 src/conf/object_event.c |  12 +--
 src/conf/secret_event.c |  18 ++---
 src/conf/storage_event.c|  18 ++---
 src/conf/virdomainobjlist.c |   6 +-
 src/conf/virinterfaceobj.c  |  12 +--
 src/conf/virnetworkobj.c|  12 +--
 src/conf/virnodedeviceobj.c |  14 ++--
 src/conf/virsecretobj.c |  12 +--
 src/conf/virstorageobj.c|  24 ++
 src/datatypes.c |   8 +-
 src/datatypes.h |  30 
 src/interface/interface_backend_netcf.c |   6 +-
 src/libvirt-admin.c |   8 +-
 src/libvirt-domain-snapshot.c   |   2 +-
 src/libvirt-domain.c|   2 +-
 src/libvirt-host.c  |   2 +-
 src/libvirt-interface.c |   2 +-
 src/libvirt-network.c   |   2 +-
 src/libvirt-nodedev.c   |   8 +-
 src/libvirt-nwfilter.c  |   2 +-
 src/libvirt-secret.c|   2 +-
 src/libvirt-storage.c   |   4 +-
 src/libvirt-stream.c|   2 +-
 src/libxl/libxl_conf.c  |   6 +-
 src/libxl/libxl_domain.c|   6 +-
 src/libxl/libxl_migration.c |   6 +-
 src/logging/log_handler.c   |   6 +-
 src/lxc/lxc_conf.c  |   6 +-
 src/lxc/lxc_monitor.c   |   6 +-
 src/node_device/node_device_driver.c|   4 +-
 src/node_device/node_device_udev.c  |   6 +-
 src/qemu/qemu_agent.c   |   6 +-
 src/qemu/qemu_capabilities.c|   8 +-
 src/qemu/qemu_conf.c|   6 +-
 src/qemu/qemu_domain.c  |  36 +++--
 src/qemu/qemu_monitor.c |   6 +-
 src/rpc/virkeepalive.c  |   6 +-
 src/rpc/virnetclient.c  |   6 +-
 src/rpc/virnetclientprogram.c   |   8 +-
 src/rpc/virnetclientstream.c|   6 +-
 src/rpc/virnetdaemon.c  |   6 +-
 src/rpc/virnetlibsshsession.c   |   6 +-
 src/rpc/virnetsaslcontext.c |  18 +++--
 src/rpc/virnetserver.c  |   6 +-
 src/rpc/virnetserverclient.c|   6 +-
 src/rpc/virnetserverprogram.c   |   8 +-
 src/rpc/virnetserverservice.c   |   8 +-
 src/rpc/virnetsocket.c  |   6 +-
 src/rpc/virnetsshsession.c  |   6 +-
 src/rpc/virnettlscontext.c  |  12 +--
 src/security/security_manager.c |   6 +-
 src/test/test_driver.c  |   6 +-
 src/util/virclosecallbacks.c|   6 +-
 src/util/virdnsmasq.c   |   9 +--
 src/util/virfdstream.c  |  10 +--
 src/util/virfilecache.c |   8 +-
 src/util/virhash.c  |   6 +-
 src/util/virhostdev.c   |   6 +-
 src/util/viridentity.c  |   6 +-
 src/util/virmacmap.c|   8 +-
 src/util/virmacmap.h|   2 +-
 src/util/virmdev.c  |   6 +-
 src/util/virobject.c|  43 ++-
 src/util/virobject.h|   7 ++
 src/util/virpci.c   |   6 +-
 src/util/virportallocator.c |   6 +-
 src/util/virresctrl.c   |  12 +--
 src/util/virscsi.c  |   6 +-
 src/util/virscsivhost.c |   6 +-
 src/util/virusb.c   |   6 +-
 src/vbox/vbox_common.c  |   6 +-
 src/vz/vz_driver.c  |   6 +-
 tests/virfilecachetest.c|   8 +-
 82 files changed, 322 insertions(+), 517 deletions(-)

-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/9] datatypes: Rename @parent to @parentName in virNodeDevice

[Michal Privoznik]

In next patches this name will be needed for a different memeber.
Also, it makes sense to rename the variable because it does not
contain reference to parent device, just its name.

Signed-off-by: Michal Privoznik 
---
 src/conf/virnodedeviceobj.c  | 2 +-
 src/datatypes.c  | 2 +-
 src/datatypes.h  | 2 +-
 src/libvirt-nodedev.c| 6 +++---
 src/node_device/node_device_driver.c | 4 ++--
 src/test/test_driver.c   | 6 +++---
 6 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index ad0f27ee47..9d2996046f 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -870,7 +870,7 @@ virNodeDeviceObjListExportCallback(void *payload,
 virNodeDeviceMatch(obj, data->flags)) {
 if (data->devices) {
 if (!(device = virGetNodeDevice(data->conn, def->name)) ||
-VIR_STRDUP(device->parent, def->parent) < 0) {
+VIR_STRDUP(device->parentName, def->parent) < 0) {
 virObjectUnref(device);
 data->error = true;
 goto cleanup;
diff --git a/src/datatypes.c b/src/datatypes.c
index f7eef24ba8..0c3c66a9ce 100644
--- a/src/datatypes.c
+++ b/src/datatypes.c
@@ -653,7 +653,7 @@ virNodeDeviceDispose(void *obj)
 VIR_DEBUG("release dev %p %s", dev, dev->name);
 
 VIR_FREE(dev->name);
-VIR_FREE(dev->parent);
+VIR_FREE(dev->parentName);
 
 virObjectUnref(dev->conn);
 }
diff --git a/src/datatypes.h b/src/datatypes.h
index 1a8ea01ba3..66733b075c 100644
--- a/src/datatypes.h
+++ b/src/datatypes.h
@@ -618,7 +618,7 @@ struct _virNodeDevice {
 virObject object;
 virConnectPtr conn; /* pointer back to the connection */
 char *name; /* device name (unique on node) */
-char *parent;   /* parent device name */
+char *parentName;   /* parent device name */
 };
 
 /**
diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
index 563ce889b9..8ced3cea0e 100644
--- a/src/libvirt-nodedev.c
+++ b/src/libvirt-nodedev.c
@@ -346,16 +346,16 @@ virNodeDeviceGetParent(virNodeDevicePtr dev)
 
 virCheckNodeDeviceReturn(dev, NULL);
 
-if (!dev->parent) {
+if (!dev->parentName) {
 if (dev->conn->nodeDeviceDriver && 
dev->conn->nodeDeviceDriver->nodeDeviceGetParent) {
-dev->parent = 
dev->conn->nodeDeviceDriver->nodeDeviceGetParent(dev);
+dev->parentName = 
dev->conn->nodeDeviceDriver->nodeDeviceGetParent(dev);
 } else {
 virReportUnsupportedError();
 virDispatchError(dev->conn);
 return NULL;
 }
 }
-return dev->parent;
+return dev->parentName;
 }
 
 
diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index 61afa1f8eb..d04a31747a 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -256,7 +256,7 @@ nodeDeviceLookupByName(virConnectPtr conn,
 goto cleanup;
 
 if ((device = virGetNodeDevice(conn, name))) {
-if (VIR_STRDUP(device->parent, def->parent) < 0) {
+if (VIR_STRDUP(device->parentName, def->parent) < 0) {
 virObjectUnref(device);
 device = NULL;
 }
@@ -290,7 +290,7 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 goto cleanup;
 
 if ((device = virGetNodeDevice(conn, def->name))) {
-if (VIR_STRDUP(device->parent, def->parent) < 0) {
+if (VIR_STRDUP(device->parentName, def->parent) < 0) {
 virObjectUnref(device);
 device = NULL;
 }
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index eec7a82924..f5b5e3ee8d 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5416,7 +5416,7 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char 
*name)
 def = virNodeDeviceObjGetDef(obj);
 
 if ((ret = virGetNodeDevice(conn, name))) {
-if (VIR_STRDUP(ret->parent, def->parent) < 0) {
+if (VIR_STRDUP(ret->parentName, def->parent) < 0) {
 virObjectUnref(ret);
 ret = NULL;
 }
@@ -5641,8 +5641,8 @@ testNodeDeviceCreateXML(virConnectPtr conn,
 if (!(dev = virGetNodeDevice(conn, objdef->name)))
 goto cleanup;
 
-VIR_FREE(dev->parent);
-if (VIR_STRDUP(dev->parent, def->parent) < 0)
+VIR_FREE(dev->parentName);
+if (VIR_STRDUP(dev->parentName, def->parent) < 0)
 goto cleanup;
 
 ret = dev;
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/9] util: Make structs follow our naming convention

[Michal Privoznik]

There are two structs virMacMap and virFDStreamData that don't
have the underscore prefix. Put it there so that they follow the
rest of the code.

Signed-off-by: Michal Privoznik 
---
 src/util/virfdstream.c | 4 ++--
 src/util/virmacmap.c   | 2 +-
 src/util/virmacmap.h   | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index be40379a92..e2d3f365cd 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -75,9 +75,9 @@ struct _virFDStreamMsg {
 
 
 /* Tunnelled migration stream support */
-typedef struct virFDStreamData virFDStreamData;
+typedef struct _virFDStreamData virFDStreamData;
 typedef virFDStreamData *virFDStreamDataPtr;
-struct virFDStreamData {
+struct _virFDStreamData {
 virObjectLockable parent;
 
 int fd;
diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c
index 42890ba2ff..d3be3066cc 100644
--- a/src/util/virmacmap.c
+++ b/src/util/virmacmap.c
@@ -43,7 +43,7 @@ VIR_LOG_INIT("util.virmacmap");
  */
 #define VIR_MAC_MAP_FILE_SIZE_MAX (32 * 1024 * 1024)
 
-struct virMacMap {
+struct _virMacMap {
 virObjectLockable parent;
 
 virHashTablePtr macs;
diff --git a/src/util/virmacmap.h b/src/util/virmacmap.h
index e6f754e247..93295c9107 100644
--- a/src/util/virmacmap.h
+++ b/src/util/virmacmap.h
@@ -24,7 +24,7 @@
 #ifndef __VIR_MACMAP_H__
 # define __VIR_MACMAP_H__
 
-typedef struct virMacMap virMacMap;
+typedef struct _virMacMap virMacMap;
 typedef virMacMap *virMacMapPtr;
 
 char *
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 43/73] qemu: Move qemuMigrationParamsSetCompression

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:33PM +0200, Jiri Denemark wrote:

The API will soon be called from qemuMigrationParamsFromFlags. Let's
move it to avoid the need to add a forward declaration.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration_params.c | 59 
1 file changed, 30 insertions(+), 29 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 42/73] qemu: Drop qemuMigrationParamsSetCapability

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:32PM +0200, Jiri Denemark wrote:

It's become only a tiny wrapper around virBitmapSetBit, which can easily
be called directly. We don't need to call virBitmapClearBit since
migParams->caps bitmap is initialized with zeros.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration_params.c | 35 +++-
1 file changed, 7 insertions(+), 28 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 41/73] qemu: Generalize macro for getting VIR_MIGRATE_* typed params

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:31PM +0200, Jiri Denemark wrote:

So far it's used only for CPU throttling parameters which are all ints,
but we'll soon want to use it for more parameters with different types.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration_params.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index 369e560990..e0cbdb1a38 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -112,6 +112,17 @@ qemuMigrationParamsFree(qemuMigrationParamsPtr migParams)
}


+#define GET(API, PARAM, VAR) \
+do { \
+int rc; \
+if ((rc = API(params, nparams, VIR_MIGRATE_PARAM_ ## PARAM, \
+  >params.VAR)) < 0) \
+goto error; \
+ \
+if (rc == 1) \
+migParams->params.VAR ## _set = true; \
+} while (0)
+
qemuMigrationParamsPtr
qemuMigrationParamsFromFlags(virTypedParameterPtr params,
 int nparams,
@@ -135,27 +146,13 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params,
}
}

-#define GET(PARAM, VAR) \
-do { \
-int rc; \
-if ((rc = virTypedParamsGetInt(params, nparams, \
-   VIR_MIGRATE_PARAM_ ## PARAM, \
-   >params.VAR)) < 0) \
-goto error; \
- \
-if (rc == 1) \
-migParams->params.VAR ## _set = true; \
-} while (0)
-
if (params) {
if (party == QEMU_MIGRATION_SOURCE) {
-GET(AUTO_CONVERGE_INITIAL, cpuThrottleInitial);
-GET(AUTO_CONVERGE_INCREMENT, cpuThrottleIncrement);
+GET(virTypedParamsGetInt, AUTO_CONVERGE_INITIAL, 
cpuThrottleInitial);
+GET(virTypedParamsGetInt, AUTO_CONVERGE_INCREMENT, 
cpuThrottleIncrement);


If all the getter APIs start with virTypedParamsGet, you can hide the
prefix inside the macro definition to save columns.

Regardless:

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 4/4] qemu: deny privilege elevation and spawn in seccomp

[John Ferlan]

On 04/10/2018 10:49 AM, Ján Tomko wrote:
> If QEMU uses a seccomp blacklist (since 2.11), -sandbox on
> no longer tries to whitelist all the calls, but uses sets
> of blacklists:
> default (always blacklisted with -sandbox on)
> obsolete (defaults to deny)
> elevateprivileges (setuid & co, default: allow)
> spawn (fork & execve, default: allow)
> resourcecontrol (setaffinity, setscheduler, default: allow)
> 
> If these are supported, default to sandbox with all four
> categories blacklisted.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1492597
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu.conf  |  7 +++---
>  src/qemu/qemu_command.c | 10 +
>  tests/qemuxml2argvdata/minimal-sandbox.args | 29 
>  tests/qemuxml2argvdata/minimal-sandbox.xml  | 34 
> +
>  tests/qemuxml2argvtest.c| 11 ++
>  5 files changed, 88 insertions(+), 3 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/minimal-sandbox.args
>  create mode 100644 tests/qemuxml2argvdata/minimal-sandbox.xml
> 
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index 07eab7eff..740129cf5 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -669,9 +669,10 @@
>  
>  
>  
> -# Use seccomp syscall whitelisting in QEMU.
> -# 1 = on, 0 = off, -1 = use QEMU default
> -# Defaults to -1.
> +# Use seccomp syscall sandbox in QEMU.
> +# 1 = on, 0 = off, -1 = use the default
> +# For QEMUs using a whitelist, the default (-1) is off.
> +# For QEMUs using a blacklist, the default (-1) is on.

Not sure it's even possible to provide any sort of details, but suffice
to say the description here is really lacking. One of those things that
if you know and care, then you use, if you don't you ignore. Maybe it's
just me being dense ;-).

Still if someone supplies 0 or 1 does that now mean the opposite of what
it did before 2.11? That is if I had this set to 1 in my qemu.conf -
does that mean that now I'm using a blacklist instead of a whitelist?

As an Admin trying to decipher this - what would each setting mean to me
and if going with the new -1 default, then that means libvirt is going
to set "on" w/ a list of 4 to deny.

With at least a few more details or shreds of information that may help
someone decipher what really changed...

Reviewed-by: John Ferlan 

John

>  #
>  #seccomp_sandbox = 1
>  
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ba279e640..fa5906d0b 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9987,6 +9987,16 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd,
>  return 0;
>  }
>  
> +/* Use blacklist by default if supported */
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_BLACKLIST)) {
> +virCommandAddArgList(cmd, "-sandbox",
> + "on,obsolete=deny,elevateprivileges=deny,"
> + "spawn=deny,resourcecontrol=deny",
> + NULL);
> +return 0;
> +}
> +
> +/* Seccomp whitelist is opt-in */
>  if (cfg->seccompSandbox > 0)
>  virCommandAddArgList(cmd, "-sandbox", "on", NULL);
>  
> diff --git a/tests/qemuxml2argvdata/minimal-sandbox.args 
> b/tests/qemuxml2argvdata/minimal-sandbox.args
> new file mode 100644
> index 0..c9d71fe8f
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/minimal-sandbox.args
> @@ -0,0 +1,29 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu-system-i686 \
> +-name QEMUGuest1 \
> +-S \
> +-machine pc,accel=tcg,usb=off,dump-guest-core=off \
> +-m 214 \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-display none \
> +-no-user-config \
> +-nodefaults \
> +-chardev 
> socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
> +server,nowait \
> +-mon chardev=charmonitor,id=monitor,mode=control \
> +-rtc base=utc \
> +-no-shutdown \
> +-no-acpi \
> +-boot c \
> +-usb \
> +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
> +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \
> +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
> +resourcecontrol=deny
> diff --git a/tests/qemuxml2argvdata/minimal-sandbox.xml 
> b/tests/qemuxml2argvdata/minimal-sandbox.xml
> new file mode 100644
> index 0..9ef92f8fe
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/minimal-sandbox.xml
> @@ -0,0 +1,34 @@
> +
> +  QEMUGuest1
> +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> +  A description of the test machine.
> +  
> +  A test of qemus minimal configuration.
> +  This test also tests the description and title elements.
> +  
> +  219100
> +  219100
> +  1
> +  
> +hvm
> +
> +  
> +  
> +  destroy
> 

Re: [libvirt] [PATCHv2 3/4] Refactor qemuBuildSeccompSandboxCommandLine

[John Ferlan]

On 04/10/2018 10:49 AM, Ján Tomko wrote:
> Exit early if possible to simplify the logic.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_command.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 

Reviewed-by: John Ferlan 

John

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index dfeba54ee..ba279e640 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9974,16 +9974,22 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd,
> virQEMUDriverConfigPtr cfg,
> virQEMUCapsPtr qemuCaps)
>  {

Could also use

   bool has_seccomp_cap = virQEMUCapsGet(qemuCaps,
QEMU_CAPS_SECCOMP_SANDBOX);

??

Then use - doesn't matter probably because the compiler will fix it.


> -if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX)) {
> -if (cfg->seccompSandbox == 0)
> -virCommandAddArgList(cmd, "-sandbox", "off", NULL);
> -else if (cfg->seccompSandbox > 0)
> -virCommandAddArgList(cmd, "-sandbox", "on", NULL);
> -} else if (cfg->seccompSandbox > 0) {
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX) &&
> +cfg->seccompSandbox > 0) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("QEMU does not support seccomp sandboxes"));
>  return -1;
>  }
> +
> +if (cfg->seccompSandbox == 0) {
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX))
> +virCommandAddArgList(cmd, "-sandbox", "off", NULL);
> +return 0;
> +}
> +
> +if (cfg->seccompSandbox > 0)
> +virCommandAddArgList(cmd, "-sandbox", "on", NULL);
> +
>  return 0;
>  
>  }
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 2/4] Introduce qemuBuildSeccompSandboxCommandLine

[John Ferlan]

On 04/10/2018 10:49 AM, Ján Tomko wrote:
> Move the building of -sandbox command line into a separate function.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_command.c | 30 +-
>  1 file changed, 21 insertions(+), 9 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 v2 38/73] qemu: Set always-on migration caps in ParamsCheck

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:28PM +0200, Jiri Denemark wrote:

Some migration capabilities are always enabled if QEMU supports them. We
can just drop the explicit code for them and let
qemuMigrationParamsCheck automatically set such capabilities.

QEMU_MONITOR_MIGRATION_CAPS_EVENTS would normally be one of the always
on features, but it is the only feature we want to enable even for other
jobs which internally use migration (such as save and snapshot). Hence
this capability is set very early after libvirtd connects to QEMU
monitor.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration.c|  6 --
src/qemu/qemu_migration_params.c | 31 +++
2 files changed, 31 insertions(+), 6 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH jenkins-ci 2/2] Enable mingw build for virt-viewer project

[Daniel P . Berrangé]

On Fri, Apr 13, 2018 at 03:40:49PM +0200, Andrea Bolognani wrote:
> On Fri, 2018-04-13 at 14:25 +0100, Daniel P. Berrangé wrote:
> > On Fri, Apr 13, 2018 at 03:05:10PM +0200, Andrea Bolognani wrote:
> > > No, wait, actually its native variant is, but the same is true for
> > > the Adwaita icon theme. Weird. I don't see how we would need one of
> > > them to be native and the other to be MinGW, they should match.
> > 
> > I think that highcolor line is obsolete cruft leftover when when
> > we added dep on mingw32 adwaita theme.
> 
> I just tried and it looks like neither mingw*-adwaita-* nor
> mingw*-hicolor-* seems to be required to build virt-viewer,
> actually. I think we can safely avoid listing them altogether.

That'll be because the icon theme is only needed when we build the
MSI installer, and that's not part of the default "make" target
we're running here. That is something I ought to fix :-)


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] [RFC] passing standard input to host bootloaders

[Fabian Freyer]

Hello list,

Some host boot loaders, e.g. grub-bhyve when using the bhyve
driver, take commands on stdin. While there is the
 tag to set arbitrary command line tags,
there is no  or similar to provide standard
input to the boot loader.

Typical input could be something along the lines of e.g. the
following grub commands:

kernel (cd)/path/to/vmlinuz some-cmdline
initrd (root)/path/to/host/initrd
boot

Using e.g. the (root) device is especially useful on the bhyve
driver when creating diskless VMs.

Before I start implementing this, I’d appreciate some feedback
on the following two points:

 1) should this be an attr on the  tag, e.g.

Re: [libvirt] [PATCH jenkins-ci 2/2] Enable mingw build for virt-viewer project

[Andrea Bolognani]

On Fri, 2018-04-13 at 14:25 +0100, Daniel P. Berrangé wrote:
> On Fri, Apr 13, 2018 at 03:05:10PM +0200, Andrea Bolognani wrote:
> > No, wait, actually its native variant is, but the same is true for
> > the Adwaita icon theme. Weird. I don't see how we would need one of
> > them to be native and the other to be MinGW, they should match.
> 
> I think that highcolor line is obsolete cruft leftover when when
> we added dep on mingw32 adwaita theme.

I just tried and it looks like neither mingw*-adwaita-* nor
mingw*-hicolor-* seems to be required to build virt-viewer,
actually. I think we can safely avoid listing them altogether.

> > Please keep in mind the Ansible part is by design not tightly tied
> > to the Jenkins part, and must be usable entirely on its own for
> > development and debugging purposes.
> 
> If we really need that, then it would be better if we expressed
> dependancies between the project vars files explicitly, rather
> than duplicating information many times over

Yeah, projects depending on each other would be neat. Until that's
in place, implicit dependencies will do.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH jenkins-ci 2/2] Enable mingw build for virt-viewer project

[Andrea Bolognani]

On Fri, 2018-04-13 at 14:28 +0100, Daniel P. Berrangé wrote:
> On Fri, Apr 13, 2018 at 03:24:09PM +0200, Andrea Bolognani wrote:
> > > +  - mingw32-glib2
> > 
> > Forgot to mention: mingw*-glib2 is not necessary here based on our
> > discussion, as it's already required to build libvirt-glib :)
> 
> This falls into similar case as mingw-spice-glib you point out though.
> eg If it is directly used by virt-viewer, it is reasonable to include
> it. We just want drop the things which are indirect dependancies. eg
> mingw-gnutls as gnutls is used only by libvirt, not directly by
> virt-viewer/libvirt-glib so doesn't need listing separately.

Right. Fair enough then :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 37/73] qemu: Drop qemuMigrationParamsSetPostCopy

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:27PM +0200, Jiri Denemark wrote:

It's just a tiny wrapper around qemuMigrationParamsSetCapability and
setting priv->job.postcopyEnabled is not something qemuMigrationParams
code should be doing anyway so let the callers do it.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration.c| 16 
src/qemu/qemu_migration_params.c | 17 -
src/qemu/qemu_migration_params.h |  5 -
3 files changed, 12 insertions(+), 26 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 36/73] qemu: Hide qemuMigrationParamsNew

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:26PM +0200, Jiri Denemark wrote:

It is no longer used outside qemu_migration_params.c.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration_params.c | 2 +-
src/qemu/qemu_migration_params.h | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 35/73] qemu: Use qemuMigrationParamsFromFlags everywhere

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:25PM +0200, Jiri Denemark wrote:

Every migration entry point in qemu_driver is supposed to call
qemuMigrationParamsFromFlags to transform flags and parameters into
qemuMigrationParams structure and pass the result to qemuMigration*
APIs.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_driver.c| 64 +++
src/qemu/qemu_migration.c | 23 ++
src/qemu/qemu_migration.h |  2 ++
3 files changed, 62 insertions(+), 27 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH jenkins-ci 2/2] Enable mingw build for virt-viewer project

[Daniel P . Berrangé]

On Fri, Apr 13, 2018 at 03:24:09PM +0200, Andrea Bolognani wrote:
> On Thu, 2018-04-12 at 15:28 +0100, Daniel P. Berrangé wrote:
> > diff --git a/guests/vars/projects/virt-viewer.yml 
> > b/guests/vars/projects/virt-viewer.yml
> > index 6f3dbf9..85d1589 100644
> > --- a/guests/vars/projects/virt-viewer.yml
> > +++ b/guests/vars/projects/virt-viewer.yml
> > @@ -6,5 +6,29 @@ packages:
> >- intltool
> >- libgovirt
> >- libxml2
> > +  - mingw32-adwaita-icon-theme
> > +  - mingw32-glib2
> 
> Forgot to mention: mingw*-glib2 is not necessary here based on our
> discussion, as it's already required to build libvirt-glib :)

This falls into similar case as mingw-spice-glib you point out though.
eg If it is directly used by virt-viewer, it is reasonable to include
it. We just want drop the things which are indirect dependancies. eg
mingw-gnutls as gnutls is used only by libvirt, not directly by
virt-viewer/libvirt-glib so doesn't need listing separately.

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 jenkins-ci 2/2] Enable mingw build for virt-viewer project

[Daniel P . Berrangé]

On Fri, Apr 13, 2018 at 03:05:10PM +0200, Andrea Bolognani wrote:
> On Fri, 2018-04-13 at 13:44 +0100, Daniel P. Berrangé wrote:
> > > Because of the missing dependencies mentioned below, you also need
> > > 
> > >   mingw32-hicolor-icon-theme:
> > > FedoraRawhide: mingw32-hicolor-icon-theme
> > 
> > There's no such package AFAIK
> 
> There sure is:
> 
>   $ dnf info mingw32-hicolor-icon-theme
>   Available Packages
>   Name : mingw32-hicolor-icon-theme
>   Version  : 0.16
>   Release  : 1.fc28
>   Arch : noarch
>   Size : 45 k
>   Source   : mingw-hicolor-icon-theme-0.16-1.fc28.src.rpm
>   Repo : rawhide
>   Summary  : MinGW hicolor icon theme for MingGW
>   URL  : http://icon-theme.freedesktop.org/releases/
>   License  : GPLv2+
>   Description  : Contains the basic directories and files needed for icon 
> theme support.
>: This is the MinGW version of this package.
> 
> It's also listed as BuildRequires in mingw-virt-viewer.spec.in...
> 
> No, wait, actually its native variant is, but the same is true for
> the Adwaita icon theme. Weird. I don't see how we would need one of
> them to be native and the other to be MinGW, they should match.

I think that highcolor line is obsolete cruft leftover when when
we added dep on mingw32 adwaita theme.

> > >   mingw32-spice-glib:
> > > FedoraRawhide: mingw32-spice-glib
> > 
> > That's not required - it is a dependency of spice-gtk3
> 
> But virt-viewer includes spice-glib headers directly, so it's good
> practice not to rely on the transitive dependency and have a
> direct one instead.
>
> > > Same as the previous patch, you need to include also the packages
> > > MinGW builds for libvirt and libvirt-glib already depend on.
> > 
> > Why would we want to duplicate that ?  This job depends on tje libvirt
> > job, so that will have already pulled in all those RPMs.  Listing them
> > again just creates the opportunity for the many duplicated listings to
> > get out of date.
> 
> Please keep in mind the Ansible part is by design not tightly tied
> to the Jenkins part, and must be usable entirely on its own for
> development and debugging purposes.

If we really need that, then it would be better if we expressed
dependancies between the project vars files explicitly, rather
than duplicating information many times over

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 jenkins-ci 2/2] Enable mingw build for virt-viewer project

[Andrea Bolognani]

On Fri, 2018-04-13 at 13:44 +0100, Daniel P. Berrangé wrote:
> > Because of the missing dependencies mentioned below, you also need
> > 
> >   mingw32-hicolor-icon-theme:
> > FedoraRawhide: mingw32-hicolor-icon-theme
> 
> There's no such package AFAIK

There sure is:

  $ dnf info mingw32-hicolor-icon-theme
  Available Packages
  Name : mingw32-hicolor-icon-theme
  Version  : 0.16
  Release  : 1.fc28
  Arch : noarch
  Size : 45 k
  Source   : mingw-hicolor-icon-theme-0.16-1.fc28.src.rpm
  Repo : rawhide
  Summary  : MinGW hicolor icon theme for MingGW
  URL  : http://icon-theme.freedesktop.org/releases/
  License  : GPLv2+
  Description  : Contains the basic directories and files needed for icon theme 
support.
   : This is the MinGW version of this package.

It's also listed as BuildRequires in mingw-virt-viewer.spec.in...

No, wait, actually its native variant is, but the same is true for
the Adwaita icon theme. Weird. I don't see how we would need one of
them to be native and the other to be MinGW, they should match.

> >   mingw32-spice-glib:
> > FedoraRawhide: mingw32-spice-glib
> 
> That's not required - it is a dependency of spice-gtk3

But virt-viewer includes spice-glib headers directly, so it's good
practice not to rely on the transitive dependency and have a
direct one instead.

> > Same as the previous patch, you need to include also the packages
> > MinGW builds for libvirt and libvirt-glib already depend on.
> 
> Why would we want to duplicate that ?  This job depends on tje libvirt
> job, so that will have already pulled in all those RPMs.  Listing them
> again just creates the opportunity for the many duplicated listings to
> get out of date.

Please keep in mind the Ansible part is by design not tightly tied
to the Jenkins part, and must be usable entirely on its own for
development and debugging purposes.

That said, I've already agreed in the other mail that we don't
actually need to duplicate dependencies in this kind of scenario,
so disregard any comments to that effect.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 34/73] qemu: Introduce qemuMigrationParty enum

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:24PM +0200, Jiri Denemark wrote:

Some migration parameters and capabilities are supposed to be set on
both sides of migration while others should only be set on one side. For
example, CPU throttling parameters make no sense on the destination and
they can be used even if the destination is too old to support them.

To make qemuMigrationParamsFromFlags more general and usable on both
sides of migration, we need to tell it what side it's been called on.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_driver.c   | 3 ++-
src/qemu/qemu_migration_params.c | 9 ++---
src/qemu/qemu_migration_params.h | 8 +++-
3 files changed, 15 insertions(+), 5 deletions(-)



That does sound like a party I could vote for.

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Add function that raises error if domain is not active

[Michal Privoznik]

On 04/13/2018 02:49 PM, Clementine Hayat wrote:
> 
> I'll take a look into coccinelle. It may take a bit more time thought.
> 

Yeah, don't waste too much time on it. I merely just wanted to mention
it. It not that trivial to learn. But once you do, it's awesome tool.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Add function that raises error if domain is not active

[Clementine Hayat]

2018-04-13 8:25 GMT+00:00 Erik Skultety :
> On Fri, Apr 13, 2018 at 09:45:48AM +0200, Michal Privoznik wrote:
>> On 04/13/2018 09:31 AM, Ján Tomko wrote:
>> > On Thu, Apr 12, 2018 at 07:49:15PM +, Clementine Hayat wrote:
>> >> Add a function named virDomainObjCheckIsActive in src/conf/domain_conf.c.
>> >> It calls virDomainObjIsActive, raises error and returns.
>> >
>> > *raises error if necessary

Yes, thank you.

>> >> There is a lot of occurence of this pattern and it will save 3 lines on
>> >> each call. Knowing that there is over 100 occurences, it will remove 300
>> >> lines from the code base.
>> >
>> > False advertising.
>> >
>> > If you don't want to send them all in one patch, I suggest spliting them
>> > per-subdirectory: conf, qemu, libxl, vz, ... (and use that prefix in the
>> > commit summary)

I'll do that thank you.

>> >> Signed-off-by: Clementine Hayat 
>> >
>> > If you have any accents in your name, feel free to use them. Even danpb
>> > recently decided the world is ready for UTF-8:
>> > https://libvirt.org/git/?p=libvirt.git;a=search;s=Daniel+P.+Berrang%C3%A9;st=author
>> >
>> >
>> >> ---
>> >> Patch proposed for gsoc2018.
>> >>
>> >> src/conf/domain_conf.c   | 11 +
>> >> src/conf/domain_conf.h   |  2 +
>> >> src/libvirt_private.syms |  1 +
>> >> src/qemu/qemu_driver.c   | 96 +---
>> >> 4 files changed, 34 insertions(+), 76 deletions(-)
>> >>
>> >
>> > The changes look good but the patch feels incomplete.
>>
>> I was just looking at this patch. Yes it is incomplete but I think that
>> was the point. Give upstream taste what the patch looks like before
>> diving in and changing all those 108 occurrences (I did change them
>> locally).

It's right, it's one of the BiteSizedTasks proposed to begin with [1]
and it's asked to do it in two times.
First do a small patch and have it review and then change all the occurences.
I'm sorry I should have mentioned it.

>> The patch looks good to me, but as Jan suggests, you can break this big
>> change into smaller (=easier to review) patches. In the first you can
>> just introduce the function. And then have patch per each folder.

> I agree with the intention of the patch and the comments, I'd maybe change the
> name slightly --> virDomainObjCheckActive (instead of *IsActive) or even
> something that emphasizes a bit more that it will report error on inactive, so
> that the caller doesn't have to care about that anymore - from the top of my
> head "virDomainObjReportInactive"...

I'm going to do that. I think virDomainObjCheckActive is a good name.
For the virDomainObjReportInactive, I have the feeling that, by
reading the name,
people may expect that the function will return 0 if there was an error.

>> Alternatively, you can write a small semantic patch [1] and use that to
>> generate the diff. But this is rather advanced stuff.

I'll take a look into coccinelle. It may take a bit more time thought.

-- 
Clementine Hayat

[1] 
https://wiki.libvirt.org/page/BiteSizedTasks#Add_function_that_raises_error_if_domain_is_not_active

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH jenkins-ci 1/2] Enable mingw build for libvirt-glib project

[Andrea Bolognani]

On Fri, 2018-04-13 at 13:20 +0100, Daniel P. Berrangé wrote:
> On Fri, Apr 13, 2018 at 02:17:45PM +0200, Andrea Bolognani wrote:
> > On Thu, 2018-04-12 at 15:28 +0100, Daniel P. Berrangé wrote:
> > >- gtk-doc
> > >- intltool
> > >- libxml2
> > > +  - mingw32-glib2
> > > +  - mingw64-glib2
> > >- vala
> > 
> > You're missing a lot of packages here, probably because they are
> > already used for the libvirt MinGW build and you didn't reset your
> > test machine between builds, thus masking the issue.
> 
> No I didn't miss them, I only listed stuff that's unique to
> libvirt-glib - duplicating everything used by libvirt mingw
> builds, under libvirt-glib mingw builds is not required, as
> they're already present, and most are not even used by
> libvirt-glib

Hm, I guess I see where you're coming from.

One of the original ideas was that the dependencies listed here
would be as comprehensive as possible, so that you would be able
to skip building some parts of the stack (and installing the
corresponding build dependencies) if you were only interested in
higher-level projects such as virt-manager: in that case, you
would just install libvirt-python from the vendor repositories
and go ahead with your day.

I guess we haven't been very consistent with that, though, as
clearly shown by the fact that the list of requirements I offered
for the MinGW build is way bigger than that of the native build!

It's also unclear how useful being so strict about packages is,
and how realistic it is that we will get it right all of the
time :)

So perhaps a reasonable compromise is indeed not to list packages
again if one of the direct dependencies already needs them to
build: libvirt-glib would only list packages libvirt doesn't
already depend on, but there could be duplicates between libvirt
and libosinfo because neither depends on the other. That would
be much easier to get right.

tl;dr go ahead with your version of the patch, and I might have
  some other cleaning up to do later :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH jenkins-ci 2/2] Enable mingw build for virt-viewer project

[Andrea Bolognani]

On Thu, 2018-04-12 at 15:28 +0100, Daniel P. Berrangé wrote:
[...]
> +  mingw32-gtk-vnc:
> +FedoraRawhide: mingw32-gtk-vnc

This (and the corresponding mingw64 entry) will drag in the version
of the library based on GTK+ 2 rather than the one based on GTK+ 3,
which is not what we want. It should look like

  mingw32-gtk-vnc:
FedoraRawhide: mingw32-gtk-vnc2

Because of the missing dependencies mentioned below, you also need

  mingw32-hicolor-icon-theme:
FedoraRawhide: mingw32-hicolor-icon-theme

  mingw32-spice-glib:
FedoraRawhide: mingw32-spice-glib

plus of course the mingw64 equivalents.

[...]
> diff --git a/guests/vars/projects/virt-viewer.yml 
> b/guests/vars/projects/virt-viewer.yml
> index 6f3dbf9..85d1589 100644
> --- a/guests/vars/projects/virt-viewer.yml
> +++ b/guests/vars/projects/virt-viewer.yml
> @@ -6,5 +6,29 @@ packages:
>- intltool
>- libgovirt
>- libxml2
> +  - mingw32-adwaita-icon-theme
> +  - mingw32-glib2
> +  - mingw32-glib-networking
> +  - mingw32-gstreamer1-plugins-bad-free
> +  - mingw32-gstreamer1-plugins-good
> +  - mingw32-gtk3
> +  - mingw32-gtk-vnc
> +  - mingw32-libgovirt
> +  - mingw32-libusbx
> +  - mingw32-rest
> +  - mingw32-spice-gtk3
> +  - mingw32-usbredir
> +  - mingw64-adwaita-icon-theme
> +  - mingw64-glib2
> +  - mingw64-glib-networking
> +  - mingw64-gstreamer1-plugins-bad-free
> +  - mingw64-gstreamer1-plugins-good
> +  - mingw64-gtk3
> +  - mingw64-gtk-vnc
> +  - mingw64-libgovirt
> +  - mingw64-libusbx
> +  - mingw64-rest
> +  - mingw64-spice-gtk3
> +  - mingw64-usbredir
>- spice-gtk3
>- xmllint

Same as the previous patch, you need to include also the packages
MinGW builds for libvirt and libvirt-glib already depend on.

Additionally, a couple more (optional?) dependencies can be
discovered thanks to the mingw-virt-viewer.spec.in file included
in the repository.

The full list should look like:

  - mingw32-adwaita-icon-theme
  - mingw32-glib2
  - mingw32-curl
  - mingw32-dbus
  - mingw32-dlfcn
  - mingw32-gcc
  - mingw32-gettext
  - mingw32-glib-networking
  - mingw32-glib2
  - mingw32-gstreamer1-plugins-bad-free
  - mingw32-gstreamer1-plugins-good
  - mingw32-gtk3
  - mingw32-gtk-vnc
  - mingw32-gtk3
  - mingw32-hicolor-icon-theme
  - mingw32-libgovirt
  - mingw32-libssh2
  - mingw32-libusbx
  - mingw32-libxml2
  - mingw32-pkg-config
  - mingw32-portablexdr
  - mingw32-readline
  - mingw32-rest
  - mingw32-spice-glib
  - mingw32-spice-gtk3
  - mingw32-usbredir
  - mingw64-adwaita-icon-theme
  - mingw64-glib2
  - mingw64-curl
  - mingw64-dbus
  - mingw64-dlfcn
  - mingw64-gcc
  - mingw64-gettext
  - mingw64-glib-networking
  - mingw64-glib2
  - mingw64-gstreamer1-plugins-bad-free
  - mingw64-gstreamer1-plugins-good
  - mingw64-gtk3
  - mingw64-gtk-vnc
  - mingw64-gtk3
  - mingw64-hicolor-icon-theme
  - mingw64-libgovirt
  - mingw64-libssh2
  - mingw64-libusbx
  - mingw64-libxml2
  - mingw64-pkg-config
  - mingw64-portablexdr
  - mingw64-readline
  - mingw64-rest
  - mingw64-spice-glib
  - mingw64-spice-gtk3
  - mingw64-usbredir

The icotool command also becomes mandatory rather than optional
when building with MinGW, but I have already posted a separate
patch that takes care of the issue[1] so you should not concern
yourself with that here.

Provided you've taken care of all of the above,

  Reviewed-by: Andrea Bolognani 


[1] https://www.redhat.com/archives/libvir-list/2018-April/msg01164.html
-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 32/73] qemu: Move ParamsCheck closer to ParamsApply on Src side

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:22PM +0200, Jiri Denemark wrote:

We reached the point when qemuMigrationParamsApply is the only API which
sends migration parameters and capabilities to QEMU. Thus all but the
TLS parameters can be set before we ask QEMU for the current values of
all parameters in qemuMigrationParamsCheck.

Supported migration capabilities are queried as soon as libvirt connects
to QEMU monitor so we can check them anytime.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration.c | 67 +++
1 file changed, 32 insertions(+), 35 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH jenkins-ci 1/2] Enable mingw build for libvirt-glib project

[Andrea Bolognani]

On Fri, 2018-04-13 at 13:20 +0100, Daniel P. Berrangé wrote:
> On Fri, Apr 13, 2018 at 02:17:45PM +0200, Andrea Bolognani wrote:
> > On Thu, 2018-04-12 at 15:28 +0100, Daniel P. Berrangé wrote:
> > > diff --git a/guests/vars/projects/libvirt-glib.yml 
> > > b/guests/vars/projects/libvirt-glib.yml
> > > index 13a5128..bb21ae0 100644
> > > --- a/guests/vars/projects/libvirt-glib.yml
> > > +++ b/guests/vars/projects/libvirt-glib.yml
> > > @@ -5,4 +5,6 @@ packages:
> > >- gtk-doc
> > >- intltool
> > >- libxml2
> > > +  - mingw32-glib2
> > > +  - mingw64-glib2
> > >- vala
> > 
> > You're missing a lot of packages here, probably because they are
> > already used for the libvirt MinGW build and you didn't reset your
> > test machine between builds, thus masking the issue.
> 
> No I didn't miss them, I only listed stuff that's unique to
> libvirt-glib - duplicating everything used by libvirt mingw
> builds, under libvirt-glib mingw builds is not required, as
> they're already present, and most are not even used by
> libvirt-glib
> 
> > 
> > The list of additional packages should look like:
> > 
> >   - mingw32-curl
> >   - mingw32-dbus
> >   - mingw32-dlfcn
> >   - mingw32-gcc
> >   - mingw32-gettext
> >   - mingw32-glib2
> >   - mingw32-gnutls
> >   - mingw32-libssh2
> >   - mingw32-libxml2
> >   - mingw32-pkg-config
> >   - mingw32-portablexdr
> >   - mingw64-curl
> >   - mingw64-dbus
> >   - mingw64-dlfcn
> >   - mingw64-gcc
> >   - mingw64-gettext
> >   - mingw64-glib2
> >   - mingw64-gnutls
> >   - mingw64-libssh2
> >   - mingw64-libxml2
> >   - mingw64-pkg-config
> >   - mingw64-portablexdr
> 
> 
> 
> > 
> > With that fixed,
> > 
> >   Reviewed-by: Andrea Bolognani 
> > 
> > -- 
> > Andrea Bolognani / Red Hat / Virtualization
> 
> Regards,
> Daniel
-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 31/73] qemu: Move ParamsCheck closer to ParamsApply on Dst side

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:21PM +0200, Jiri Denemark wrote:

We reached the point when qemuMigrationParamsApply is the only API which
sends migration parameters and capabilities to QEMU. Thus all but the
TLS parameters can be set before we ask QEMU for the current values of
all parameters in qemuMigrationParamsCheck.

Supported migration capabilities are queried as soon as libvirt connects
to QEMU monitor so we can check them anytime.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration.c | 32 
1 file changed, 16 insertions(+), 16 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v3 06/20] Implement GetJobInfo method for Domain interface

[Pavel Hrdina]

On Fri, Apr 13, 2018 at 01:15:17PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
>  data/org.libvirt.Domain.xml |  5 +
>  src/domain.c| 49 
> +
>  2 files changed, 54 insertions(+)
> 
> diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
> index 5c310ad..c672053 100644
> --- a/data/org.libvirt.Domain.xml
> +++ b/data/org.libvirt.Domain.xml
> @@ -62,6 +62,11 @@
>
>
>  
> +
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetJobInfo"/>
> +  
> +
>  
>  value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainListGetStats"/>
> diff --git a/src/domain.c b/src/domain.c
> index 2c3174b..47bd585 100644
> --- a/src/domain.c
> +++ b/src/domain.c
> @@ -3,6 +3,16 @@
>  
>  #include 
>  
> +VIRT_DBUS_ENUM_DECL(virtDBusDomainJob)
> +VIRT_DBUS_ENUM_IMPL(virtDBusDomainJob,
> +VIR_DOMAIN_JOB_LAST,
> +"none",
> +"bounded",
> +"unbounded",
> +"completed",
> +"failed",
> +"canceled")
> +
>  VIRT_DBUS_ENUM_DECL(virtDBusDomainMemoryStat)
>  VIRT_DBUS_ENUM_IMPL(virtDBusDomainMemoryStat,
>  VIR_DOMAIN_MEMORY_STAT_LAST,
> @@ -390,6 +400,44 @@ virtDBusDomainDetachDevice(GVariant *inArgs,
>  virtDBusUtilSetLastVirtError(error);
>  }
>  
> +static void
> +virtDBusDomainGetJobInfo(GVariant *inArgs G_GNUC_UNUSED,
> + GUnixFDList *inFDs G_GNUC_UNUSED,
> + const gchar *objectPath,
> + gpointer userData,
> + GVariant **outArgs,
> + GUnixFDList **outFDs G_GNUC_UNUSED,
> + GError **error)
> +{
> +virtDBusConnect *connect = userData;
> +g_autoptr(virDomain) domain = NULL;
> +g_autofree virDomainJobInfoPtr jobInfo = NULL;
> +const gchar *jobTypeStr;
> +
> +domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
> +if (!domain)
> +return;
> +
> +jobInfo = g_new0(virDomainJobInfo, 1);
> +if (virDomainGetJobInfo(domain, jobInfo) < 0)
> +return virtDBusUtilSetLastVirtError(error);
> +
> +jobTypeStr = virtDBusDomainJobTypeToString(jobInfo->type);
> +if (!jobTypeStr) {
> +g_set_error(error, VIRT_DBUS_ERROR, VIRT_DBUS_ERROR_LIBVIRT,
> +"Can't translate virDomainJobType to string.");

In this case setting the error makes sense since the other two options
are not to translate the type to string or returning job type 'none'
with all values as 0.

How about this error message:

"Can't format virDomainJobType '%d' to string.", jobInfo->type ?

> +virtDBusUtilSetLastVirtError(error);

This would overwrite the previous error so it should not be used here.
The purpose of this function is to get an libvirt error if some libvirt
API fails.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH jenkins-ci 1/2] Enable mingw build for libvirt-glib project

[Andrea Bolognani]

On Thu, 2018-04-12 at 15:28 +0100, Daniel P. Berrangé wrote:
> diff --git a/guests/vars/projects/libvirt-glib.yml 
> b/guests/vars/projects/libvirt-glib.yml
> index 13a5128..bb21ae0 100644
> --- a/guests/vars/projects/libvirt-glib.yml
> +++ b/guests/vars/projects/libvirt-glib.yml
> @@ -5,4 +5,6 @@ packages:
>- gtk-doc
>- intltool
>- libxml2
> +  - mingw32-glib2
> +  - mingw64-glib2
>- vala

You're missing a lot of packages here, probably because they are
already used for the libvirt MinGW build and you didn't reset your
test machine between builds, thus masking the issue.

The list of additional packages should look like:

  - mingw32-curl
  - mingw32-dbus
  - mingw32-dlfcn
  - mingw32-gcc
  - mingw32-gettext
  - mingw32-glib2
  - mingw32-gnutls
  - mingw32-libssh2
  - mingw32-libxml2
  - mingw32-pkg-config
  - mingw32-portablexdr
  - mingw64-curl
  - mingw64-dbus
  - mingw64-dlfcn
  - mingw64-gcc
  - mingw64-gettext
  - mingw64-glib2
  - mingw64-gnutls
  - mingw64-libssh2
  - mingw64-libxml2
  - mingw64-pkg-config
  - mingw64-portablexdr

With that fixed,

  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 v2 28/73] qemu: Drop unused qemuMonitorSetMigrationCapability

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:18PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_monitor.c  | 14 --
src/qemu/qemu_monitor.h  |  3 --
src/qemu/qemu_monitor_json.c | 53 
src/qemu/qemu_monitor_json.h |  3 --
4 files changed, 73 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] external (pull) backup API

[Daniel P . Berrangé]

On Fri, Apr 13, 2018 at 03:02:07PM +0300, Nikolay Shirokovskiy wrote:
> 
> 
> On 13.04.2018 12:07, Daniel P. Berrangé wrote:
> > On Tue, Apr 03, 2018 at 03:01:22PM +0300, Nikolay Shirokovskiy wrote:
> >> *Temporary snapshot API*
> >>
> >> In previous version it is called 'Fleece API' after qemu terms and I'll 
> >> still
> >> use BlockSnapshot prefix for commands as in previous RFC instead of
> >> TmpSnapshots which I inclined more now.
> >>
> >> virDomainBlockSnapshotPtr
> >> virDomainBlockSnapshotCreateXML(virDomainPtr domain,
> >> const char *xmlDesc,
> >> unsigned int flags);
> >>
> >> virDomainBlockSnapshotDelete(virDomainBlockSnapshotPtr snapshot,
> >>  unsigned int flags);
> >>
> >> virDomainBlockSnapshotList(virDomainPtr domain,
> >>virDomainBlockSnapshotPtr **snaps,
> >>unsigned int flags);
> >>
> >> virDomainBlockSnapshotGetXMLDesc(virDomainBlockSnapshotPtr snapshot,
> >>  unsigned int flags);
> >>
> >> virDomainBlockSnapshotPtr
> >> virDomainBlockSnapshotLookupByName(virDomainPtr domain,
> >>const char *name,
> >>unsigned int flags);
> >>
> >> Here is an example of snapshot xml description:
> >>
> >> 
> >> d068765e-8b50-4d74-9b72-1e55c663cbf8
> >> 
> >> 
> > 
> > Can we just call this which is how we name
> > things in normal  elements.  'fleece' in particular is an awful
> > name giving no indication of what is being talked about unless you've
> > already read the QEMU low levels, so I'd rather we don't use the word
> > "fleece" anywhere in API or XML or docs at the libvirt level.
> 
> It would be easiest thing to do) Let me explain.
> 
> "source" in plain external snapshots for example feels awkward to me. It is 
> read like "make a
> snapshot of disk sda which source file is like that". IMHO it would be better 
> if xml is read 
> like "make a snapshot of disk sda and put it into dest|target file. Then for 
> block snapshot 
> xml would read like "make a snapshot 
> of disk sda and put fleece there". Fleece may be a new term but it only costs 
> one-two 
> sentences to define it. And it is better to have this definition so that user 
> knows what the nature of this image,
> so that user have correct assumptions on image size, lifetime... If fleece 
> word itself unfortunate
> then we can coin another one.
> 
> Looks like "source" takes root in domain xml where it reads well.

It is the "source" of the data for the snapshot, in the same way
that is is the "source" of the data for the original disk.


> >> *Block export API*
> >>
> >> I guess it is natural to treat qemu NBD server as a domain device. So
> > 
> > A domain device is normally something that is related to the guest
> > machine ABI. This is entirely invisible to the guest, just a backend
> > concept, so this isn't really a natural fit as a domain device.
> 
> I have VNC in mind as a precedent.

Replace "precedent" with "historical mistake" and it would more
accurately reflect feelings about VNC.


> >> And this is how this NBD server will be exposed in domain xml:
> >>
> >> 
> >> ...
> >> 
> >> 
> >>  >>  checkpoint="d068765e-8b50-4d74-9b72-1e55c663cbf8"
> >>  
> >> exportname="sda-0044757e-1a2d-4c2c-b92f-bb403309bb17"/>
> >>  >>  checkpoint="d068765e-8b50-4d74-9b72-1e55c663cbf8
> >>  
> >> exportname="sdb-0044757e-1a2d-4c2c-b92f-bb403309bb17"/>
> >> 
> > 
> > This feels pretty awkward to me - as mentioned above this is not really
> > guest ABI related to having it under  is not a good fit.
> > 
> > I question whether the NBD server address should be exposed in the XML
> > at all. This is a transient thing that is started/stopped on demand via
> 
> Such xml resembles VNC/serial ports to me. These two are not guest ABI.
> On the other hand they connected to guest devices and nbd server is not ...
> 
> > the APIs you show above. So I'd suggest we just have an API to query
> > the listening address of the NBD server.
> 
> Such API looks like having very little function to have it...
> 
> > 
> > At most in the XML we could have a element under each respective
> > existing  element to say whether it is exported or not, instead
> > of adding new  elements in a separate place.
> > 
> 
> Just as in case of graphical framebuffer I thought we can have multiple
> NBD servers (qemu limited to just one now). So if we put export info
> under disks we need to refer to NBD server which is basically specifying
> its address. So having xml with NBD servers each providing info on
> what it exports looks more simple.

If we ever have multiple NBD servers, then we can just assign each one a
name, and under the  reference that name https://berrange.com  

Re: [libvirt] [PATCH v2 30/73] qemu: Set XBZRLE cache size via migration parameters

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:20PM +0200, Jiri Denemark wrote:

Prefer xbzrle-cache-size migration parameter over the special
migrate-set-cache-size QMP command.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration.c|  6 ++---
src/qemu/qemu_migration_params.c | 42 +---
src/qemu/qemu_migration_params.h |  4 +--
3 files changed, 25 insertions(+), 27 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [jenkins-ci PATCH] guests: Add icoutils dependency to virt-viewer project

[Andrea Bolognani]

The icotool command is used to manipulate Windows icon files.

Signed-off-by: Andrea Bolognani 
---
 guests/vars/mappings.yml | 4 
 guests/vars/projects/virt-viewer.yml | 1 +
 2 files changed, 5 insertions(+)

diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml
index e074648..6af5065 100644
--- a/guests/vars/mappings.yml
+++ b/guests/vars/mappings.yml
@@ -173,6 +173,10 @@ mappings:
   hal:
 FreeBSD: hal
 
+  icoutils:
+default: icoutils
+CentOS6:
+
   intltool:
 default: intltool
 
diff --git a/guests/vars/projects/virt-viewer.yml 
b/guests/vars/projects/virt-viewer.yml
index 6f3dbf9..54c96fe 100644
--- a/guests/vars/projects/virt-viewer.yml
+++ b/guests/vars/projects/virt-viewer.yml
@@ -3,6 +3,7 @@ packages:
   - glib2
   - gtk-vnc
   - gtk3
+  - icoutils
   - intltool
   - libgovirt
   - libxml2
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH jenkins-ci 0/2] Enable virt-viewer mingw build

[Andrea Bolognani]

On Thu, 2018-04-12 at 15:28 +0100, Daniel P. Berrangé wrote:
> Daniel P. Berrangé (2):
>   Enable mingw build for libvirt-glib project
>   Enable mingw build for virt-viewer project
> 
>  guests/vars/mappings.yml  | 72 
> +++
>  guests/vars/projects/libvirt-glib.yml |  2 +
>  guests/vars/projects/virt-viewer.yml  | 24 
>  projects/libvirt-glib.yaml|  4 ++
>  projects/virt-viewer.yaml |  4 ++
>  5 files changed, 106 insertions(+)

So, yesterday I posted a couple of series[1][2] that introduce the
concept of "project variants" through the project+variant notation,
which is meant to be used for cases where you can build a project
in a way that is different enough from the usual native build case
that an almost entirely different toolchain might be required.
MinGW builds are obviously a prime example of this.

You can still push your patches after you've addressed a few issues
that I'm going to point out separately: I have follow-up patches
to convert them to the new notation ready. I just wanted to give
you a heads-up :)


[1] https://www.redhat.com/archives/libvir-list/2018-April/msg01058.html
[2] https://www.redhat.com/archives/libvir-list/2018-April/msg01051.html
-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] external (pull) backup API

[Vladimir Sementsov-Ogievskiy]

13.04.2018 00:35, John Snow wrote:


On 04/12/2018 08:26 AM, Vladimir Sementsov-Ogievskiy wrote:

1. It looks unsafe to use nbd server + backup(sync=none) on same node,
synchronization is needed, like in block/replication, which uses
backup_wait_for_overlapping_requests, backup_cow_request_begin,
backup_cow_request_end. We have a filter driver for this thing, not yet
in upstream.

Is it the case that blockdev-backup sync=none can race with read
requests on the NBD server?

i.e. we can get temporarily inconsistent data before the COW completes?
Can you elaborate?


I'm not sure but looks possible:

1. start NBD read, find that there is a hole in temporary image, decide 
to read from active image (or even start read) and yield

2. guest writes to the same are (COW happens, but it doesn't help)
3. reduce point (1.), read invalid (already updated by 2.) data

And similar place in block/replication, which uses backup(sync=none) too 
is protected from such situation.





2. If we use filter driver anyway, it may be better to not use backup at
all, and do all needed things in a filter driver.

if blockdev-backup sync=none isn't sufficient to get the semantics we
want, it may indeed be more appropriate to just leave the entire task to
a new filter node.


3. It may be interesting to implement something like READ_ONCE for NBD,
which means, that we will never read these clusters again. And after
such command, we don't need to copy corresponding clusters to temporary
image, if guests decides to write them (as we know, that client already
read them and don't going to read again).

That would be a very interesting optimization indeed; but I don't think
we have any kind of infrastructure for such things currently. It's
almost like a TRIM on which regions need to perform COW for the
BlockSnapshot.


Hmm, READ+TRIM may be used too. And trim may be naturally implemented in 
special filter driver.


--
Best regards,
Vladimir

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 29/73] qemu: Add support for xbzrle-cache-size migration parameter

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:19PM +0200, Jiri Denemark wrote:

Originally QEMU provided query-migrate-cache-size and
migrate-set-cache-size QMP commands for querying/setting XBZRLE cache
size. In version 2.11 QEMU added support for XBZRLE cache size to the
general migration paramaters commands.

This patch adds support for this parameter to libvirt to make sure it is
properly restored to its original value after a failed or aborted
migration.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_monitor.c  | 5 +++--
src/qemu/qemu_monitor.h  | 3 +++
src/qemu/qemu_monitor_json.c | 2 ++
tests/qemumonitorjsontest.c  | 4 +++-
4 files changed, 11 insertions(+), 3 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 27/73] qemu: Do not use qemuMonitorSetMigrationCapability

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:17PM +0200, Jiri Denemark wrote:

Rework all remaining callers of qemuMonitorSetMigrationCapability to use
the new qemuMonitorSetMigrationCapabilities API.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration_params.c | 11 ---
tests/qemumonitorjsontest.c  | 16 +++-
2 files changed, 19 insertions(+), 8 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v3 03/20] Implement MemoryStats for Domain Interface

[Pavel Hrdina]

On Fri, Apr 13, 2018 at 01:15:14PM +0200, Katerina Koukiou wrote:
> This method is not tested for now since the test driver
> doesn't support this API.
> 
> Signed-off-by: Katerina Koukiou 
> ---
>  data/org.libvirt.Domain.xml |  6 
>  src/domain.c| 72 
> +
>  2 files changed, 78 insertions(+)
> 
> diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
> index df9fbf9..7174769 100644
> --- a/data/org.libvirt.Domain.xml
> +++ b/data/org.libvirt.Domain.xml
> @@ -69,6 +69,12 @@
>
>
>  
> +
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainMemoryStats"/>
> +  
> +  
> +
>  
>  value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainReboot"/>
> diff --git a/src/domain.c b/src/domain.c
> index f775fd4..5f87572 100644
> --- a/src/domain.c
> +++ b/src/domain.c
> @@ -3,6 +3,42 @@
>  
>  #include 
>  
> +VIRT_DBUS_ENUM_DECL(virtDBusDomainMemoryStat)
> +VIRT_DBUS_ENUM_IMPL(virtDBusDomainMemoryStat,
> +VIR_DOMAIN_MEMORY_STAT_LAST,
> +"swap_in",
> +"swap_out",
> +"major_fault",
> +"minor_fault",
> +"unused",
> +"available",
> +"actual_baloon",
> +"rss",
> +"usable",
> +"last_update")

I was looking at libvirt-python code to get some inspiration how to
handle the error message later in this code and find out that the last
two memory stats "usable" and "last_update" were introduced in libvirt
2.1.0 but we currently require only 1.2.8.

You need to bump it in configure.ac.

> +
> +static GVariant *
> +virtDBusDomainMemoryStatsToGVariant(virDomainMemoryStatPtr stats,
> +gint nr_stats,
> +GError **error)
> +{
> +GVariantBuilder builder;
> +
> +g_variant_builder_init(, G_VARIANT_TYPE("a{st}"));
> +
> +for (gint i = 0; i < nr_stats; i++) {
> +const gchar *memoryStat = 
> virtDBusDomainMemoryStatTypeToString(stats[i].tag);
> +if (!memoryStat) {
> +g_set_error(error, VIRT_DBUS_ERROR, VIRT_DBUS_ERROR_LIBVIRT,
> +"Can't translate virDomainMemoryStatTags to 
> string.");
> +return NULL;
> +}

Now that I think about it a little bit more, we should probably skip it
in case that we don't know how to translate it.  Sorry about the noise.
The reason is that if libvirt-dbus is used with newer libvirt that has
some new stats this API would stop working.

So instead of setting an error simple 'continue' should be good enough.
You can remove the '**error' parameter.

> +g_variant_builder_add(, "{st}", memoryStat, stats[i].val);
> +}
> +
> +return g_variant_builder_end();
> +}
> +
>  static virDomainPtr
>  virtDBusDomainGetVirDomain(virtDBusConnect *connect,
> const gchar *objectPath,
> @@ -398,6 +434,41 @@ virtDBusDomainGetXMLDesc(GVariant *inArgs,
>  *outArgs = g_variant_new("(s)", xml);
>  }
>  
> +static void
> +virtDBusDomainMemoryStats(GVariant *inArgs,
> +  GUnixFDList *inFDs G_GNUC_UNUSED,
> +  const gchar *objectPath,
> +  gpointer userData,
> +  GVariant **outArgs,
> +  GUnixFDList **outFDs G_GNUC_UNUSED,
> +  GError **error)
> +{
> +virtDBusConnect *connect = userData;
> +g_autoptr(virDomain) domain = NULL;
> +virDomainMemoryStatStruct stats[VIR_DOMAIN_MEMORY_STAT_NR];
> +gint nr_stats;
> +guint flags;
> +GVariant *gstats;
> +
> +g_variant_get(inArgs, "(u)", );
> +
> +domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
> +if (!domain)
> +return;
> +
> +nr_stats = virDomainMemoryStats(domain, stats, 
> VIR_DOMAIN_MEMORY_STAT_NR, flags);
> +if (nr_stats < 0)
> +return virtDBusUtilSetLastVirtError(error);
> +
> +gstats = virtDBusDomainMemoryStatsToGVariant(stats, nr_stats, error);
> +if (!gstats) {
> +virtDBusUtilSetLastVirtError(error);
> +return;
> +}

The whole if part can be dropped now.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 05/10] qemu: add support to launch SEV guest

[Erik Skultety]

On Mon, Apr 02, 2018 at 07:04:25PM -0400, John Ferlan wrote:
>
>
> On 04/02/2018 10:18 AM, Brijesh Singh wrote:
> > QEMU >= 2.12 provides 'sev-guest' object which is used to launch encrypted
> > VMs on AMD platform using SEV feature. The various inputs required to
> > launch SEV guest is provided through the  tag. A typical
> > SEV guest launch command line looks like this:
> >
> > # $QEMU ...\
> >   -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 ...\
> >   -machine memory-encryption=sev0 \
> >
> > Signed-off-by: Brijesh Singh 
> > ---
> >  src/qemu/qemu_command.c | 35 +
> >  src/qemu/qemu_process.c | 58 
> > +
> >  2 files changed, 93 insertions(+)
> >
>
> (slight delay for next part of review - today was rocket launch day and
> then we headed out for a bit ;-))
>
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 682d714..55bbfa2 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -7405,6 +7405,9 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
> >  virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
> >  qemuAppendLoadparmMachineParm(, def);
> >
> > +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)
>
> Since we already checked sev-guest at prepare host storage (mostly
> unconditionally), I don't think we have to make the check here as well -
> although I could be wrong...

I guess you surely meant qemuProcessPrepareSevGuestInput, but you're right, we
don't need it.

...
> >  static int
> >  qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd,
> > @@ -10195,6 +10227,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
> >  if (qemuBuildVMCoreInfoCommandLine(cmd, def, qemuCaps) < 0)
> >  goto error;
> >
> > +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)
> > +qemuBuildSevCommandLine(vm, cmd, def->sev);
> > +
>
> I think we're save to change this to:

Yep.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] external (pull) backup API

[Vladimir Sementsov-Ogievskiy]

13.04.2018 11:51, Nikolay Shirokovskiy wrote:


On 13.04.2018 03:04, John Snow wrote:


On 04/12/2018 10:08 AM, Vladimir Sementsov-Ogievskiy wrote:

I propose, not to say that bitmap represents a checkpoint. It is simpler
to say (and it reflects the reality) that bitmap is a difference between
two consecutive checkpoints. And we can say, that active state is some
kind of a checkpoint, current point in time.

So, we have checkpoints (5* is an active state) which are points in time:

1 2 3 4 5*


Oh -- the most recent checkpoint there doesn't belong to a ***specific
time*** yet. It's a floating checkpoint -- it always represents the most
current version. It's not really a checkpoint at all.

1, 2, 3, and 4 however are associated with a specific timestamp though.


And bitmaps, first three are disabled, last is enabled:

"1->2", "2->3", "3->4", "4->5*"


OK; so 1->2, 2->3 and 3->4 define deltas between two ***defined***
points in time.

4->5* however is only anchored by one specific point in time, and is
floating just like the most recent checkpoint is floating.


So, remove first checkpoint: just remove bitmap "A->B".

I assume you mean "1->2" here.

And... yes, I agree -- if you don't care about your very first
checkpoint anymore, you can just delete the first bitmap, too.


Remove any other checkpoint N: create new bitmap "(N-1)->(N+1)" =
merge("(N-1)->N", "N->(N+1)"), drop bitmaps "(N-1)->N" and "N->(N+1)".

err, okay, so let's say we want to drop checkpoint 3:

create: "2->4"
merge: "2->3", "3->4" [and presumably store in "2->4"]
drop: 2->3, 3->4

OK, that makes more sense to me. In essence;

(1) We could consider this 2->3 absorbing 3->4, or
(2) 3->4 absorbing 2->3

and in either case it's the same, really.


If the latter was active, the new one becomes active. And we cant remove
5* checkpoint, as it is an active state, not an actual checkpoint.

OK, crystal.

--js


I prefer not talking of active checkpoint. It is a kind of controversial.
Better just keep in mind that last bitmap is active one. So for checkpoints 1 2 
3 4
we have bitmaps:

1 1->2 2->3 3->4

Note the first bitmap name. When it was created name 2 was unknown so we'd 
better
have this name for the first bitmap.


so here, 1->2 is a difference between checkpoints 2 and 3? I think it's 
unnatural.. Ofcource, when we create new active bitmap, we don't know 
the name of next checkpoint, but, why not rename it when we create next 
checkpoint?


So,

1. have no checkpoints and bitmaps
2. create new checkpoint 1, and bitmap 1->?
3. create new checkpoint 2 and bitmap 2->?, disable bitmap 1->? and 
rename it to 1->2

and so on.

this reflects the essence of things



Checkpoint 4 can not be used without checkpoint 5 by design to it is not a 
problem
that 3->4 is active.

Nikolay



--
Best regards,
Vladimir

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 24/73] qemu: Hide internals of qemuMigrationParams struct

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:14PM +0200, Jiri Denemark wrote:

All users of migration parameters are supposed to use APIs provided by
qemu_migration_params.c without having to worry about the internals.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration_params.c | 5 +
src/qemu/qemu_migration_params.h | 3 ---
2 files changed, 5 insertions(+), 3 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v3 01/20] Implement Setter for Autostart property for Domain interface

[Pavel Hrdina]

On Fri, Apr 13, 2018 at 01:15:12PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
>  data/org.libvirt.Domain.xml |  5 +++--
>  src/domain.c| 22 +-
>  test/test_domain.py |  7 +++
>  3 files changed, 31 insertions(+), 3 deletions(-)

[...]

> +static void
> +virtDBusDomainSetAutostart(GVariant *value,
> +   const gchar *objectPath,
> +   gpointer userData,
> +   GError **error)
> +{
> +virtDBusConnect *connect = userData;
> +g_autoptr(virDomain) domain = NULL;
> +gboolean autostart;
> +
> +g_variant_get(value, "b", );
> +
> +domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
> +if (!domain)
> +return;
> +
> +if (virDomainSetAutostart(domain, autostart) < 0)
> +return virtDBusUtilSetLastVirtError(error);

You missed one 'return'.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 23/73] qemu: Rename qemuMigrationParamsSet

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:13PM +0200, Jiri Denemark wrote:

The new name is qemuMigrationParamsApply and it will soon become the
only API which will send all requested migration parameters and
capabilities to QEMU. All other qemuMigrationParams* APIs will just
operate on the qemuMigrationParams structure.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration.c|  8 
src/qemu/qemu_migration_params.c | 21 -
src/qemu/qemu_migration_params.h |  8 
3 files changed, 24 insertions(+), 13 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v3 18/20] Implement MigrateSetMaxSpeed method for Domain Interface

[Katerina Koukiou]

Signed-off-by: Katerina Koukiou 
---
 data/org.libvirt.Domain.xml |  6 ++
 src/domain.c| 25 +
 2 files changed, 31 insertions(+)

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
index eec9dcb..838ebed 100644
--- a/data/org.libvirt.Domain.xml
+++ b/data/org.libvirt.Domain.xml
@@ -146,6 +146,12 @@
   
   
 
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainMigrateSetMaxSpeed"/>
+  
+  
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainReboot"/>
diff --git a/src/domain.c b/src/domain.c
index 46a8c03..b7bc10c 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -831,6 +831,30 @@ virtDBusDomainMigrateSetMaxDowntime(GVariant *inArgs,
 virtDBusUtilSetLastVirtError(error);
 }
 
+static void
+virtDBusDomainMigrateSetMaxSpeed(GVariant *inArgs,
+ GUnixFDList *inFDs G_GNUC_UNUSED,
+ const gchar *objectPath,
+ gpointer userData,
+ GVariant **outArgs G_GNUC_UNUSED,
+ GUnixFDList **outFDs G_GNUC_UNUSED,
+ GError **error)
+{
+virtDBusConnect *connect = userData;
+g_autoptr(virDomain) domain = NULL;
+gulong bandwidth;
+guint flags;
+
+g_variant_get(inArgs, "(tu)", , );
+
+domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
+if (!domain)
+return;
+
+if (virDomainMigrateSetMaxSpeed(domain, bandwidth, flags) < 0)
+return virtDBusUtilSetLastVirtError(error);
+}
+
 static void
 virtDBusDomainReboot(GVariant *inArgs,
  GUnixFDList *inFDs G_GNUC_UNUSED,
@@ -1023,6 +1047,7 @@ static virtDBusGDBusMethodTable 
virtDBusDomainMethodTable[] = {
 { "MigrateGetMaxDowntime", virtDBusDomainMigrateGetMaxDowntime },
 { "MigrateGetMaxSpeed", virtDBusDomainMigrateGetMaxSpeed },
 { "MigrateSetMaxDowntime", virtDBusDomainMigrateSetMaxDowntime },
+{ "MigrateSetMaxSpeed", virtDBusDomainMigrateSetMaxSpeed },
 { "Reboot", virtDBusDomainReboot },
 { "Reset", virtDBusDomainReset },
 { "Resume", virtDBusDomainResume },
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v3 11/20] Implement HasManagedSaveImage method for Domain Interface

[Katerina Koukiou]

Signed-off-by: Katerina Koukiou 
---
 data/org.libvirt.Domain.xml |  6 ++
 src/domain.c| 29 +
 test/test_domain.py |  2 +-
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
index 91e2558..e29cd4c 100644
--- a/data/org.libvirt.Domain.xml
+++ b/data/org.libvirt.Domain.xml
@@ -90,6 +90,12 @@
   
   
 
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainHasManagedSaveImage"/>
+  
+  
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainManagedSave"/>
diff --git a/src/domain.c b/src/domain.c
index 4d42d28..05904d7 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -550,6 +550,34 @@ virtDBusDomainGetXMLDesc(GVariant *inArgs,
 *outArgs = g_variant_new("(s)", xml);
 }
 
+static void
+virtDBusDomainHasManagedSaveImage(GVariant *inArgs,
+  GUnixFDList *inFDs G_GNUC_UNUSED,
+  const gchar *objectPath,
+  gpointer userData,
+  GVariant **outArgs,
+  GUnixFDList **outFDs G_GNUC_UNUSED,
+  GError **error)
+
+{
+virtDBusConnect *connect = userData;
+g_autoptr(virDomain) domain = NULL;
+gint managedSaveImage;
+guint flags;
+
+g_variant_get(inArgs, "(u)", );
+
+domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
+if (!domain)
+return;
+
+managedSaveImage = virDomainHasManagedSaveImage(domain, flags);
+if (managedSaveImage < 0)
+return virtDBusUtilSetLastVirtError(error);
+
+*outArgs = g_variant_new("(b)", managedSaveImage);
+}
+
 static void
 virtDBusDomainManagedSave(GVariant *inArgs,
   GUnixFDList *inFDs G_GNUC_UNUSED,
@@ -819,6 +847,7 @@ static virtDBusGDBusMethodTable virtDBusDomainMethodTable[] 
= {
 { "GetStats", virtDBusDomainGetStats },
 { "GetVcpus", virtDBusDomainGetVcpus },
 { "GetXMLDesc", virtDBusDomainGetXMLDesc },
+{ "HasManagedSaveImage", virtDBusDomainHasManagedSaveImage },
 { "ManagedSave", virtDBusDomainManagedSave },
 { "MemoryStats", virtDBusDomainMemoryStats },
 { "MigrateGetMaxDowntime", virtDBusDomainMigrateGetMaxDowntime },
diff --git a/test/test_domain.py b/test/test_domain.py
index 31348ce..13f27f7 100755
--- a/test/test_domain.py
+++ b/test/test_domain.py
@@ -56,7 +56,7 @@ class TestDomain(libvirttest.BaseTestClass):
 
 obj, domain = self.domain()
 domain.ManagedSave(0)
-
+assert domain.HasManagedSaveImage(0) == dbus.Boolean(True)
 state = obj.Get('org.libvirt.Domain', 'State', 
dbus_interface=dbus.PROPERTIES_IFACE)
 assert state == 'shutoff'
 
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v3 07/20] Implement AbortJob for Domain interface

[Katerina Koukiou]

Signed-off-by: Katerina Koukiou 
---
 data/org.libvirt.Domain.xml |  4 
 src/domain.c| 21 +
 2 files changed, 25 insertions(+)

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
index c672053..074dd62 100644
--- a/data/org.libvirt.Domain.xml
+++ b/data/org.libvirt.Domain.xml
@@ -40,6 +40,10 @@
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetUUIDString"/>
 
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainAbortJob"/>
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainAttachDeviceFlags"/>
diff --git a/src/domain.c b/src/domain.c
index 47bd585..b55a615 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -306,6 +306,26 @@ virtDBusDomainSetAutostart(GVariant *value,
 return virtDBusUtilSetLastVirtError(error);
 }
 
+static void
+virtDBusDomainAbortJob(GVariant *inArgs G_GNUC_UNUSED,
+   GUnixFDList *inFDs G_GNUC_UNUSED,
+   const gchar *objectPath,
+   gpointer userData,
+   GVariant **outArgs G_GNUC_UNUSED,
+   GUnixFDList **outFDs G_GNUC_UNUSED,
+   GError **error)
+{
+virtDBusConnect *connect = userData;
+g_autoptr(virDomain) domain = NULL;
+
+domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
+if (!domain)
+return;
+
+if (virDomainAbortJob(domain) < 0)
+virtDBusUtilSetLastVirtError(error);
+}
+
 static void
 virtDBusDomainAttachDevice(GVariant *inArgs,
GUnixFDList *inFDs G_GNUC_UNUSED,
@@ -713,6 +733,7 @@ static virtDBusGDBusPropertyTable 
virtDBusDomainPropertyTable[] = {
 };
 
 static virtDBusGDBusMethodTable virtDBusDomainMethodTable[] = {
+{ "AbortJob", virtDBusDomainAbortJob },
 { "AttachDevice", virtDBusDomainAttachDevice },
 { "Create", virtDBusDomainCreate },
 { "Destroy", virtDBusDomainDestroy },
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v3 14/20] Implement GetMemoryParameters method for Domain Interface

[Katerina Koukiou]

Signed-off-by: Katerina Koukiou 
---
 data/org.libvirt.Domain.xml |  6 ++
 src/domain.c| 36 
 2 files changed, 42 insertions(+)

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
index 41814c7..36167d8 100644
--- a/data/org.libvirt.Domain.xml
+++ b/data/org.libvirt.Domain.xml
@@ -71,6 +71,12 @@
 value="See 
https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetJobInfo"/>
   
 
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetMemoryParameters"/>
+  
+  
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainListGetStats"/>
diff --git a/src/domain.c b/src/domain.c
index 9b8779b..7ab832b 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -458,6 +458,41 @@ virtDBusDomainGetJobInfo(GVariant *inArgs G_GNUC_UNUSED,
  jobInfo->fileRemaining);
 }
 
+static void
+virtDBusDomainGetMemoryParameters(GVariant *inArgs,
+  GUnixFDList *inFDs G_GNUC_UNUSED,
+  const gchar *objectPath,
+  gpointer userData,
+  GVariant **outArgs,
+  GUnixFDList **outFDs G_GNUC_UNUSED,
+  GError **error)
+{
+virtDBusConnect *connect = userData;
+g_autoptr(virDomain) domain = NULL;
+g_autofree virTypedParameterPtr params = NULL;
+gint nparams = 0;
+guint flags;
+gint ret;
+GVariant *grecords;
+
+g_variant_get(inArgs, "(u)", );
+
+domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
+if (!domain)
+return;
+
+ret = virDomainGetMemoryParameters(domain, NULL, , flags);
+if (ret == 0 && nparams != 0) {
+params = g_new0(virTypedParameter, nparams);
+if (virDomainGetMemoryParameters(domain, params, , flags) < 0)
+return virtDBusUtilSetLastVirtError(error);
+}
+
+grecords = virtDBusUtilTypedParamsToGVariant(params, nparams);
+
+*outArgs = g_variant_new_tuple(, 1);
+}
+
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainStatsRecordPtr, 
virDomainStatsRecordListFree);
 
 static void
@@ -892,6 +927,7 @@ static virtDBusGDBusMethodTable virtDBusDomainMethodTable[] 
= {
 { "Destroy", virtDBusDomainDestroy },
 { "DetachDevice", virtDBusDomainDetachDevice },
 { "GetJobInfo", virtDBusDomainGetJobInfo },
+{ "GetMemoryParameters", virtDBusDomainGetMemoryParameters },
 { "GetStats", virtDBusDomainGetStats },
 { "GetVcpus", virtDBusDomainGetVcpus },
 { "GetXMLDesc", virtDBusDomainGetXMLDesc },
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v3 10/20] Implement ManagedSave method for Domain Interface

[Katerina Koukiou]

Signed-off-by: Katerina Koukiou 
---
 data/org.libvirt.Domain.xml |  5 +
 src/domain.c| 24 
 test/test_domain.py | 15 +++
 3 files changed, 44 insertions(+)

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
index 10c4da8..91e2558 100644
--- a/data/org.libvirt.Domain.xml
+++ b/data/org.libvirt.Domain.xml
@@ -90,6 +90,11 @@
   
   
 
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainManagedSave"/>
+  
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainMemoryStats"/>
diff --git a/src/domain.c b/src/domain.c
index 522fedb..4d42d28 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -550,6 +550,29 @@ virtDBusDomainGetXMLDesc(GVariant *inArgs,
 *outArgs = g_variant_new("(s)", xml);
 }
 
+static void
+virtDBusDomainManagedSave(GVariant *inArgs,
+  GUnixFDList *inFDs G_GNUC_UNUSED,
+  const gchar *objectPath,
+  gpointer userData,
+  GVariant **outArgs G_GNUC_UNUSED,
+  GUnixFDList **outFDs G_GNUC_UNUSED,
+  GError **error)
+{
+virtDBusConnect *connect = userData;
+g_autoptr(virDomain) domain = NULL;
+guint flags;
+
+g_variant_get(inArgs, "(u)", );
+
+domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
+if (!domain)
+return;
+
+if (virDomainManagedSave(domain, flags) < 0)
+virtDBusUtilSetLastVirtError(error);
+}
+
 static void
 virtDBusDomainMemoryStats(GVariant *inArgs,
   GUnixFDList *inFDs G_GNUC_UNUSED,
@@ -796,6 +819,7 @@ static virtDBusGDBusMethodTable virtDBusDomainMethodTable[] 
= {
 { "GetStats", virtDBusDomainGetStats },
 { "GetVcpus", virtDBusDomainGetVcpus },
 { "GetXMLDesc", virtDBusDomainGetXMLDesc },
+{ "ManagedSave", virtDBusDomainManagedSave },
 { "MemoryStats", virtDBusDomainMemoryStats },
 { "MigrateGetMaxDowntime", virtDBusDomainMigrateGetMaxDowntime },
 { "MigrateSetMaxDowntime", virtDBusDomainMigrateSetMaxDowntime },
diff --git a/test/test_domain.py b/test/test_domain.py
index a7cf962..31348ce 100755
--- a/test/test_domain.py
+++ b/test/test_domain.py
@@ -47,6 +47,21 @@ class TestDomain(libvirttest.BaseTestClass):
 autostart_current = domain.Get('org.libvirt.Domain', 'Autostart', 
dbus_interface=dbus.PROPERTIES_IFACE)
 assert autostart_current == dbus.Boolean(autostart_expected)
 
+def test_domain_managed_save(self):
+def domain_stopped(path, _event):
+assert isinstance(path, dbus.ObjectPath)
+self.loop.quit()
+
+self.connect.connect_to_signal('DomainEvent', domain_stopped, 
arg1='Stopped')
+
+obj, domain = self.domain()
+domain.ManagedSave(0)
+
+state = obj.Get('org.libvirt.Domain', 'State', 
dbus_interface=dbus.PROPERTIES_IFACE)
+assert state == 'shutoff'
+
+self.main_loop()
+
 def test_resume(self):
 def domain_resumed(path, _event):
 assert isinstance(path, dbus.ObjectPath)
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v3 06/20] Implement GetJobInfo method for Domain interface

[Katerina Koukiou]

Signed-off-by: Katerina Koukiou 
---
 data/org.libvirt.Domain.xml |  5 +
 src/domain.c| 49 +
 2 files changed, 54 insertions(+)

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
index 5c310ad..c672053 100644
--- a/data/org.libvirt.Domain.xml
+++ b/data/org.libvirt.Domain.xml
@@ -62,6 +62,11 @@
   
   
 
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetJobInfo"/>
+  
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainListGetStats"/>
diff --git a/src/domain.c b/src/domain.c
index 2c3174b..47bd585 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -3,6 +3,16 @@
 
 #include 
 
+VIRT_DBUS_ENUM_DECL(virtDBusDomainJob)
+VIRT_DBUS_ENUM_IMPL(virtDBusDomainJob,
+VIR_DOMAIN_JOB_LAST,
+"none",
+"bounded",
+"unbounded",
+"completed",
+"failed",
+"canceled")
+
 VIRT_DBUS_ENUM_DECL(virtDBusDomainMemoryStat)
 VIRT_DBUS_ENUM_IMPL(virtDBusDomainMemoryStat,
 VIR_DOMAIN_MEMORY_STAT_LAST,
@@ -390,6 +400,44 @@ virtDBusDomainDetachDevice(GVariant *inArgs,
 virtDBusUtilSetLastVirtError(error);
 }
 
+static void
+virtDBusDomainGetJobInfo(GVariant *inArgs G_GNUC_UNUSED,
+ GUnixFDList *inFDs G_GNUC_UNUSED,
+ const gchar *objectPath,
+ gpointer userData,
+ GVariant **outArgs,
+ GUnixFDList **outFDs G_GNUC_UNUSED,
+ GError **error)
+{
+virtDBusConnect *connect = userData;
+g_autoptr(virDomain) domain = NULL;
+g_autofree virDomainJobInfoPtr jobInfo = NULL;
+const gchar *jobTypeStr;
+
+domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
+if (!domain)
+return;
+
+jobInfo = g_new0(virDomainJobInfo, 1);
+if (virDomainGetJobInfo(domain, jobInfo) < 0)
+return virtDBusUtilSetLastVirtError(error);
+
+jobTypeStr = virtDBusDomainJobTypeToString(jobInfo->type);
+if (!jobTypeStr) {
+g_set_error(error, VIRT_DBUS_ERROR, VIRT_DBUS_ERROR_LIBVIRT,
+"Can't translate virDomainJobType to string.");
+virtDBusUtilSetLastVirtError(error);
+return;
+}
+*outArgs = g_variant_new("((sttt))", jobTypeStr,
+ jobInfo->timeElapsed, jobInfo->timeRemaining,
+ jobInfo->dataTotal, jobInfo->dataProcessed,
+ jobInfo->dataRemaining, jobInfo->memTotal,
+ jobInfo->memProcessed, jobInfo->memRemaining,
+ jobInfo->fileTotal, jobInfo->fileProcessed,
+ jobInfo->fileRemaining);
+}
+
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainStatsRecordPtr, 
virDomainStatsRecordListFree);
 
 static void
@@ -669,6 +717,7 @@ static virtDBusGDBusMethodTable virtDBusDomainMethodTable[] 
= {
 { "Create", virtDBusDomainCreate },
 { "Destroy", virtDBusDomainDestroy },
 { "DetachDevice", virtDBusDomainDetachDevice },
+{ "GetJobInfo", virtDBusDomainGetJobInfo },
 { "GetStats", virtDBusDomainGetStats },
 { "GetVcpus", virtDBusDomainGetVcpus },
 { "GetXMLDesc", virtDBusDomainGetXMLDesc },
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v3 04/20] Implement AttachDevice method for Domain Interface

[Katerina Koukiou]

This method is not tested for now since the test driver
doesn't suport this API.

Signed-off-by: Katerina Koukiou 
---
 data/org.libvirt.Domain.xml |  6 ++
 src/domain.c| 25 +
 2 files changed, 31 insertions(+)

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
index 7174769..9360cbf 100644
--- a/data/org.libvirt.Domain.xml
+++ b/data/org.libvirt.Domain.xml
@@ -40,6 +40,12 @@
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetUUIDString"/>
 
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainAttachDeviceFlags"/>
+  
+  
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainCreateWithFlags"/>
diff --git a/src/domain.c b/src/domain.c
index 5f87572..dccba94 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -296,6 +296,30 @@ virtDBusDomainSetAutostart(GVariant *value,
 return virtDBusUtilSetLastVirtError(error);
 }
 
+static void
+virtDBusDomainAttachDevice(GVariant *inArgs,
+   GUnixFDList *inFDs G_GNUC_UNUSED,
+   const gchar *objectPath,
+   gpointer userData,
+   GVariant **outArgs G_GNUC_UNUSED,
+   GUnixFDList **outFDs G_GNUC_UNUSED,
+   GError **error)
+{
+virtDBusConnect *connect = userData;
+g_autoptr(virDomain) domain = NULL;
+const gchar *xml;
+guint flags;
+
+g_variant_get(inArgs, "()", , );
+
+domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
+if (!domain)
+return;
+
+if (virDomainAttachDeviceFlags(domain, xml, flags) < 0)
+virtDBusUtilSetLastVirtError(error);
+}
+
 static void
 virtDBusDomainCreate(GVariant *inArgs,
  GUnixFDList *inFDs G_GNUC_UNUSED,
@@ -617,6 +641,7 @@ static virtDBusGDBusPropertyTable 
virtDBusDomainPropertyTable[] = {
 };
 
 static virtDBusGDBusMethodTable virtDBusDomainMethodTable[] = {
+{ "AttachDevice", virtDBusDomainAttachDevice },
 { "Create", virtDBusDomainCreate },
 { "Destroy", virtDBusDomainDestroy },
 { "GetStats", virtDBusDomainGetStats },
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v3 03/20] Implement MemoryStats for Domain Interface

[Katerina Koukiou]

This method is not tested for now since the test driver
doesn't support this API.

Signed-off-by: Katerina Koukiou 
---
 data/org.libvirt.Domain.xml |  6 
 src/domain.c| 72 +
 2 files changed, 78 insertions(+)

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
index df9fbf9..7174769 100644
--- a/data/org.libvirt.Domain.xml
+++ b/data/org.libvirt.Domain.xml
@@ -69,6 +69,12 @@
   
   
 
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainMemoryStats"/>
+  
+  
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainReboot"/>
diff --git a/src/domain.c b/src/domain.c
index f775fd4..5f87572 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -3,6 +3,42 @@
 
 #include 
 
+VIRT_DBUS_ENUM_DECL(virtDBusDomainMemoryStat)
+VIRT_DBUS_ENUM_IMPL(virtDBusDomainMemoryStat,
+VIR_DOMAIN_MEMORY_STAT_LAST,
+"swap_in",
+"swap_out",
+"major_fault",
+"minor_fault",
+"unused",
+"available",
+"actual_baloon",
+"rss",
+"usable",
+"last_update")
+
+static GVariant *
+virtDBusDomainMemoryStatsToGVariant(virDomainMemoryStatPtr stats,
+gint nr_stats,
+GError **error)
+{
+GVariantBuilder builder;
+
+g_variant_builder_init(, G_VARIANT_TYPE("a{st}"));
+
+for (gint i = 0; i < nr_stats; i++) {
+const gchar *memoryStat = 
virtDBusDomainMemoryStatTypeToString(stats[i].tag);
+if (!memoryStat) {
+g_set_error(error, VIRT_DBUS_ERROR, VIRT_DBUS_ERROR_LIBVIRT,
+"Can't translate virDomainMemoryStatTags to string.");
+return NULL;
+}
+g_variant_builder_add(, "{st}", memoryStat, stats[i].val);
+}
+
+return g_variant_builder_end();
+}
+
 static virDomainPtr
 virtDBusDomainGetVirDomain(virtDBusConnect *connect,
const gchar *objectPath,
@@ -398,6 +434,41 @@ virtDBusDomainGetXMLDesc(GVariant *inArgs,
 *outArgs = g_variant_new("(s)", xml);
 }
 
+static void
+virtDBusDomainMemoryStats(GVariant *inArgs,
+  GUnixFDList *inFDs G_GNUC_UNUSED,
+  const gchar *objectPath,
+  gpointer userData,
+  GVariant **outArgs,
+  GUnixFDList **outFDs G_GNUC_UNUSED,
+  GError **error)
+{
+virtDBusConnect *connect = userData;
+g_autoptr(virDomain) domain = NULL;
+virDomainMemoryStatStruct stats[VIR_DOMAIN_MEMORY_STAT_NR];
+gint nr_stats;
+guint flags;
+GVariant *gstats;
+
+g_variant_get(inArgs, "(u)", );
+
+domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
+if (!domain)
+return;
+
+nr_stats = virDomainMemoryStats(domain, stats, VIR_DOMAIN_MEMORY_STAT_NR, 
flags);
+if (nr_stats < 0)
+return virtDBusUtilSetLastVirtError(error);
+
+gstats = virtDBusDomainMemoryStatsToGVariant(stats, nr_stats, error);
+if (!gstats) {
+virtDBusUtilSetLastVirtError(error);
+return;
+}
+
+*outArgs = g_variant_new_tuple(, 1);
+}
+
 static void
 virtDBusDomainReboot(GVariant *inArgs,
  GUnixFDList *inFDs G_GNUC_UNUSED,
@@ -551,6 +622,7 @@ static virtDBusGDBusMethodTable virtDBusDomainMethodTable[] 
= {
 { "GetStats", virtDBusDomainGetStats },
 { "GetVcpus", virtDBusDomainGetVcpus },
 { "GetXMLDesc", virtDBusDomainGetXMLDesc },
+{ "MemoryStats", virtDBusDomainMemoryStats },
 { "Reboot", virtDBusDomainReboot },
 { "Reset", virtDBusDomainReset },
 { "Resume", virtDBusDomainResume },
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v3 09/20] Implement MigrateGetMaxDowntime method for Domain Interface

[Katerina Koukiou]

Signed-off-by: Katerina Koukiou 
---
 data/org.libvirt.Domain.xml |  6 ++
 src/domain.c| 31 +++
 2 files changed, 37 insertions(+)

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
index c3c64fa..10c4da8 100644
--- a/data/org.libvirt.Domain.xml
+++ b/data/org.libvirt.Domain.xml
@@ -96,6 +96,12 @@
   
   
 
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainMigrateGetMaxDowntime"/>
+  
+  
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainMigrateSetMaxDowntime"/>
diff --git a/src/domain.c b/src/domain.c
index 88e6aaa..522fedb 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -585,6 +585,36 @@ virtDBusDomainMemoryStats(GVariant *inArgs,
 *outArgs = g_variant_new_tuple(, 1);
 }
 
+static void
+virtDBusDomainMigrateGetMaxDowntime(GVariant *inArgs,
+GUnixFDList *inFDs G_GNUC_UNUSED,
+const gchar *objectPath,
+gpointer userData,
+GVariant **outArgs,
+GUnixFDList **outFDs G_GNUC_UNUSED,
+GError **error)
+{
+virtDBusConnect *connect = userData;
+g_autoptr(virDomain) domain = NULL;
+gulong downtime;
+guint flags;
+gint ret;
+
+g_variant_get(inArgs, "(u)", );
+
+domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
+if (!domain)
+return;
+
+ret = virDomainMigrateGetMaxDowntime(domain,
+ (unsigned long long *),
+ flags);
+if (ret < 0)
+return virtDBusUtilSetLastVirtError(error);
+
+*outArgs = g_variant_new("(t)", downtime);
+}
+
 static void
 virtDBusDomainMigrateSetMaxDowntime(GVariant *inArgs,
 GUnixFDList *inFDs G_GNUC_UNUSED,
@@ -767,6 +797,7 @@ static virtDBusGDBusMethodTable virtDBusDomainMethodTable[] 
= {
 { "GetVcpus", virtDBusDomainGetVcpus },
 { "GetXMLDesc", virtDBusDomainGetXMLDesc },
 { "MemoryStats", virtDBusDomainMemoryStats },
+{ "MigrateGetMaxDowntime", virtDBusDomainMigrateGetMaxDowntime },
 { "MigrateSetMaxDowntime", virtDBusDomainMigrateSetMaxDowntime },
 { "Reboot", virtDBusDomainReboot },
 { "Reset", virtDBusDomainReset },
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v3 00/20] More APIs for Domain Interface

[Katerina Koukiou]

Changes from v2:
Adjusted to reviews.

Katerina Koukiou (20):
  Implement Setter for Autostart property for Domain interface
  Implement SchedulerType property for Domain Interface
  Implement MemoryStats for Domain Interface
  Implement AttachDevice method for Domain Interface
  Implement DetachDevice method for Domain Interface
  Implement GetJobInfo method for Domain interface
  Implement AbortJob for Domain interface
  Implement MigrateSetMaxDowntime method for Domain interface
  Implement MigrateGetMaxDowntime method for Domain Interface
  Implement ManagedSave method for Domain Interface
  Implement HasManagedSaveImage method for Domain Interface
  Implement ManagedSaveRemove method for Domain Interface
  Implement SetVcpus method for Domain Interface
  Implement GetMemoryParameters method for Domain Interface
  Implement GetBlkioParameters method for domain Interface
  Implement Updated property for Domain Interface
  Implement MigrateGetMaxSpeed method for Domain Interface
  Implement MigrateSetMaxSpeed method for Domain Interface
  Implement SetMemory method for Domain Interface
  Implement GetSchedulerParameters method for Domain Interface

 data/org.libvirt.Domain.xml | 110 +++-
 src/domain.c| 605 +++-
 test/test_domain.py |  37 ++-
 3 files changed, 747 insertions(+), 5 deletions(-)

-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v3 01/20] Implement Setter for Autostart property for Domain interface

[Katerina Koukiou]

Signed-off-by: Katerina Koukiou 
---
 data/org.libvirt.Domain.xml |  5 +++--
 src/domain.c| 22 +-
 test/test_domain.py |  7 +++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
index 78378bb..b8ee5b8 100644
--- a/data/org.libvirt.Domain.xml
+++ b/data/org.libvirt.Domain.xml
@@ -7,9 +7,10 @@
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainIsActive"/>
 
-
+
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetAutostart"/>
+value="See 
https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetAutostart and
+   
https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainSetAutostart"/>
 
 
   https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v3 12/20] Implement ManagedSaveRemove method for Domain Interface

[Katerina Koukiou]

Signed-off-by: Katerina Koukiou 
---
 data/org.libvirt.Domain.xml |  5 +
 src/domain.c| 24 
 test/test_domain.py |  2 ++
 3 files changed, 31 insertions(+)

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
index e29cd4c..f300b59 100644
--- a/data/org.libvirt.Domain.xml
+++ b/data/org.libvirt.Domain.xml
@@ -101,6 +101,11 @@
 value="See 
https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainManagedSave"/>
   
 
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainManagedSaveRemove"/>
+  
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainMemoryStats"/>
diff --git a/src/domain.c b/src/domain.c
index 05904d7..5f83b33 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -601,6 +601,29 @@ virtDBusDomainManagedSave(GVariant *inArgs,
 virtDBusUtilSetLastVirtError(error);
 }
 
+static void
+virtDBusDomainManagedSaveRemove(GVariant *inArgs,
+GUnixFDList *inFDs G_GNUC_UNUSED,
+const gchar *objectPath,
+gpointer userData,
+GVariant **outArgs G_GNUC_UNUSED,
+GUnixFDList **outFDs G_GNUC_UNUSED,
+GError **error)
+{
+virtDBusConnect *connect = userData;
+g_autoptr(virDomain) domain = NULL;
+guint flags;
+
+g_variant_get(inArgs, "(u)", );
+
+domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
+if (!domain)
+return;
+
+if (virDomainManagedSaveRemove(domain, flags) < 0)
+virtDBusUtilSetLastVirtError(error);
+}
+
 static void
 virtDBusDomainMemoryStats(GVariant *inArgs,
   GUnixFDList *inFDs G_GNUC_UNUSED,
@@ -849,6 +872,7 @@ static virtDBusGDBusMethodTable virtDBusDomainMethodTable[] 
= {
 { "GetXMLDesc", virtDBusDomainGetXMLDesc },
 { "HasManagedSaveImage", virtDBusDomainHasManagedSaveImage },
 { "ManagedSave", virtDBusDomainManagedSave },
+{ "ManagedSaveRemove", virtDBusDomainManagedSaveRemove },
 { "MemoryStats", virtDBusDomainMemoryStats },
 { "MigrateGetMaxDowntime", virtDBusDomainMigrateGetMaxDowntime },
 { "MigrateSetMaxDowntime", virtDBusDomainMigrateSetMaxDowntime },
diff --git a/test/test_domain.py b/test/test_domain.py
index 13f27f7..45a99f9 100755
--- a/test/test_domain.py
+++ b/test/test_domain.py
@@ -59,6 +59,8 @@ class TestDomain(libvirttest.BaseTestClass):
 assert domain.HasManagedSaveImage(0) == dbus.Boolean(True)
 state = obj.Get('org.libvirt.Domain', 'State', 
dbus_interface=dbus.PROPERTIES_IFACE)
 assert state == 'shutoff'
+domain.ManagedSaveRemove(0)
+assert domain.HasManagedSaveImage(0) == dbus.Boolean(False)
 
 self.main_loop()
 
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v3 02/20] Implement SchedulerType property for Domain Interface

[Katerina Koukiou]

Signed-off-by: Katerina Koukiou 
---
 data/org.libvirt.Domain.xml |  4 
 src/domain.c| 23 +++
 test/test_domain.py |  3 +++
 3 files changed, 30 insertions(+)

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
index b8ee5b8..df9fbf9 100644
--- a/data/org.libvirt.Domain.xml
+++ b/data/org.libvirt.Domain.xml
@@ -28,6 +28,10 @@
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainIsPersistent"/>
 
+
+  https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetSchedulerType"/>
+
 
   https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetState"/>
diff --git a/src/domain.c b/src/domain.c
index 82682ef..f775fd4 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -149,6 +149,28 @@ virtDBusDomainGetPersistent(const gchar *objectPath,
 *value = g_variant_new("b", !!persistent);
 }
 
+static void
+virtDBusDomainGetSchedulerType(const gchar *objectPath,
+   gpointer userData,
+   GVariant **value,
+   GError **error)
+{
+virtDBusConnect *connect = userData;
+g_autoptr(virDomain) domain = NULL;
+g_autofree gchar *schedtype = NULL;
+gint nparams;
+
+domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
+if (!domain)
+return;
+
+schedtype = virDomainGetSchedulerType(domain, );
+if (!schedtype)
+return virtDBusUtilSetLastVirtError(error);
+
+*value = g_variant_new("(si)", schedtype, nparams);
+}
+
 static void
 virtDBusDomainGetState(const gchar *objectPath,
gpointer userData,
@@ -517,6 +539,7 @@ static virtDBusGDBusPropertyTable 
virtDBusDomainPropertyTable[] = {
 { "Name", virtDBusDomainGetName, NULL },
 { "OSType", virtDBusDomainGetOsType, NULL },
 { "Persistent", virtDBusDomainGetPersistent, NULL },
+{ "SchedulerType", virtDBusDomainGetSchedulerType, NULL},
 { "State", virtDBusDomainGetState, NULL },
 { "UUID", virtDBusDomainGetUUID, NULL },
 { 0 }
diff --git a/test/test_domain.py b/test/test_domain.py
index 7fa9aad..a7cf962 100755
--- a/test/test_domain.py
+++ b/test/test_domain.py
@@ -16,6 +16,9 @@ class TestDomain(libvirttest.BaseTestClass):
 assert isinstance(props['Name'], dbus.String)
 assert isinstance(props['OSType'], dbus.String)
 assert isinstance(props['Persistent'], dbus.Boolean)
+assert any([isinstance(props['SchedulerType'], dbus.Struct),
+isinstance(props['SchedulerType'][0], dbus.String),
+isinstance(props['SchedulerType'][1], dbus.Int32)])
 assert isinstance(props['State'], dbus.String)
 assert isinstance(props['UUID'], dbus.String)
 
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 20/22] Implement MigrateSetMaxSpeed method for Domain Interface

[Pavel Hrdina]

On Thu, Apr 12, 2018 at 04:32:59PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
>  data/org.libvirt.Domain.xml |  6 ++
>  src/domain.c| 25 +
>  2 files changed, 31 insertions(+)

[...]

> +static void
> +virtDBusDomainMigrateSetMaxSpeed(GVariant *inArgs,
> + GUnixFDList *inFDs G_GNUC_UNUSED,
> + const gchar *objectPath,
> + gpointer userData,
> + GVariant **outArgs G_GNUC_UNUSED,
> + GUnixFDList **outFDs G_GNUC_UNUSED,
> + GError **error)
> +{
> +virtDBusConnect *connect = userData;
> +g_autoptr(virDomain) domain = NULL;
> +guint64 bandwidth;

gulong

> +guint flags;
> +
> +g_variant_get(inArgs, "(tu)", , );
> +
> +domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
> +if (!domain)
> +return;
> +
> +if (virDomainMigrateSetMaxSpeed(domain, bandwidth, flags) < 0)
> +return virtDBusUtilSetLastVirtError(error);

No need to use 'return'.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 19/22] Implement MigrateGetMaxSpeed method for Domain Interface

[Pavel Hrdina]

On Thu, Apr 12, 2018 at 04:32:58PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
>  data/org.libvirt.Domain.xml |  6 ++
>  src/domain.c| 28 
>  2 files changed, 34 insertions(+)

[...]

> +static void
> +virtDBusDomainMigrateGetMaxSpeed(GVariant *inArgs,
> + GUnixFDList *inFDs G_GNUC_UNUSED,
> + const gchar *objectPath,
> + gpointer userData,
> + GVariant **outArgs,
> + GUnixFDList **outFDs G_GNUC_UNUSED,
> + GError **error)
> +
> +{
> +virtDBusConnect *connect = userData;
> +g_autoptr(virDomain) domain = NULL;
> +guint64 bandwidth;

gulong

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 17/22] Implement GetBlkioParameters method for domain Interface

[Pavel Hrdina]

On Thu, Apr 12, 2018 at 04:32:56PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
>  data/org.libvirt.Domain.xml |  6 ++
>  src/domain.c| 37 +
>  2 files changed, 43 insertions(+)

[...]

> +static void
> +virtDBusDomainGetBlkioParameters(GVariant *inArgs,
> + GUnixFDList *inFDs G_GNUC_UNUSED,
> + const gchar *objectPath,
> + gpointer userData,
> + GVariant **outArgs,
> + GUnixFDList **outFDs G_GNUC_UNUSED,
> + GError **error)
> +{
> +virtDBusConnect *connect = userData;
> +g_autoptr(virDomain) domain = NULL;
> +g_autofree virTypedParameterPtr params = NULL;
> +gint nparams = 0;
> +guint flags;
> +GVariant *grecords;
> +
> +g_variant_get(inArgs, "(u)", );
> +
> +domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
> +if (!domain)
> +return;
> +
> +if (virDomainGetBlkioParameters(domain, NULL, , flags) == 0 &&
> +nparams != 0) {
> +if ((params = g_new0(virTypedParameter, nparams)) == NULL)
> +return;
> +if (virDomainGetBlkioParameters(domain, params, , flags))
> +return virtDBusUtilSetLastVirtError(error);
> +}
> +
> +grecords = virtDBusUtilTypedParamsToGVariant(params,
> + nparams);

Same comments as for the previous patch.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 21/73] qemu: Set tlsHostname inside qemuMigrationParamsEnableTLS

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:11PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration.c| 31 ---
src/qemu/qemu_migration_params.c |  9 +++--
src/qemu/qemu_migration_params.h |  1 +
3 files changed, 20 insertions(+), 21 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 20/73] qemu: Rename qemuMigrationParamsAddTLSObjects

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:10PM +0200, Jiri Denemark wrote:

The new name is qemuMigrationParamsEnableTLS.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration.c| 12 ++--
src/qemu/qemu_migration_params.c | 18 +-
src/qemu/qemu_migration_params.h | 16 
3 files changed, 23 insertions(+), 23 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 17/73] qemu: Drop qemuMigrationParamsCheckSetupTLS

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:07PM +0200, Jiri Denemark wrote:

The code can be merged directly in qemuMigrationParamsAddTLSObjects.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration.c| 15 ---
src/qemu/qemu_migration_params.c | 73 ++--
src/qemu/qemu_migration_params.h |  6 ---
3 files changed, 23 insertions(+), 71 deletions(-)



So we trade not catching some errors in the Begin phase for more
readable code.

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 16/73] qemu: Reset all migration parameters

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:06PM +0200, Jiri Denemark wrote:

Restore the original values of all migration parameters we store in
qemuDomainJobObj instead of explicitly resting only a limited set of
them.

The result is not strictly equivalent to the previous code wrt reseting
TLS state because the previous code would only reset it if we changed it
before while the new code will reset it always if QEMU supports TLS
migration. This is not a problem for the parameters themselves, but it
can cause spurious errors about missing TLS objects being logged at the
end of non-TLS migration. This issue will be fixed ~50 patches later.


It better be.



Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration.c| 20 +
src/qemu/qemu_migration_params.c | 48 +++-
src/qemu/qemu_migration_params.h |  3 +-
src/qemu/qemu_process.c  |  4 +--
4 files changed, 34 insertions(+), 41 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 16/22] Implement GetMemoryParameters method for Domain Interface

[Pavel Hrdina]

On Thu, Apr 12, 2018 at 04:32:55PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
>  data/org.libvirt.Domain.xml |  6 ++
>  src/domain.c| 37 +
>  2 files changed, 43 insertions(+)
> 
> diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
> index 7f58cbd..db9a93e 100644
> --- a/data/org.libvirt.Domain.xml
> +++ b/data/org.libvirt.Domain.xml
> @@ -71,6 +71,12 @@
>  value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetJobInfo"/>
>
>  
> +
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetMemoryParameters"/>
> +  
> +  
> +
>  
>  value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainListGetStats"/>
> diff --git a/src/domain.c b/src/domain.c
> index 1896590..1f907e0 100644
> --- a/src/domain.c
> +++ b/src/domain.c
> @@ -450,6 +450,42 @@ virtDBusDomainGetJobInfo(GVariant *inArgs G_GNUC_UNUSED,
>   jobInfo->fileRemaining);
>  }
>  
> +static void
> +virtDBusDomainGetMemoryParameters(GVariant *inArgs,
> +  GUnixFDList *inFDs G_GNUC_UNUSED,
> +  const gchar *objectPath,
> +  gpointer userData,
> +  GVariant **outArgs,
> +  GUnixFDList **outFDs G_GNUC_UNUSED,
> +  GError **error)
> +{
> +virtDBusConnect *connect = userData;
> +g_autoptr(virDomain) domain = NULL;
> +g_autofree virTypedParameterPtr params = NULL;
> +gint nparams = 0;
> +guint flags;
> +GVariant *grecords;
> +
> +g_variant_get(inArgs, "(u)", );
> +
> +domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
> +if (!domain)
> +return;
> +
> +if (virDomainGetMemoryParameters(domain, NULL, , flags) == 0 &&
> +nparams != 0) {

Using temporary 'ret' variable for virDomainGetMemoryParameters will
make the code nicer:

ret = virDomainGetMemoryParameters(domain, NULL, , flags);
if (ret == 0 && nparams != 0) {
...
}

> +if ((params = g_new0(virTypedParameter, nparams)) == NULL)
> +return;

There is no need to check 'params == NULL', g_new0 aborts a program if
the allocation fails.

> +if (virDomainGetMemoryParameters(domain, params, , flags))

Explicit check for return value ' < 0' is preferred.

> +return virtDBusUtilSetLastVirtError(error);
> +}
> +
> +grecords = virtDBusUtilTypedParamsToGVariant(params,
> + nparams);

This can be on a single line.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Add function that raises error if domain is not active

[Jiri Denemark]

On Thu, Apr 12, 2018 at 19:49:15 +, Clementine Hayat wrote:
> Add a function named virDomainObjCheckIsActive in src/conf/domain_conf.c.
> It calls virDomainObjIsActive, raises error and returns.
> 
> There is a lot of occurence of this pattern and it will save 3 lines on
> each call. Knowing that there is over 100 occurences, it will remove 300
> lines from the code base.
> 
> Signed-off-by: Clementine Hayat 
> ---
> Patch proposed for gsoc2018.
> 
>  src/conf/domain_conf.c   | 11 +
>  src/conf/domain_conf.h   |  2 +
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_driver.c   | 96 +---
>  4 files changed, 34 insertions(+), 76 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d23182f18..86d28c26a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6003,6 +6003,17 @@ virDomainDefValidate(virDomainDefPtr def,
>  return 0;
>  }
>  
> +int
> +virDomainObjCheckIsActive(virDomainObjPtr dom)
> +{
> +if (!virDomainObjIsActive(dom)) {
> +   virReportError(VIR_ERR_OPERATION_INVALID,
> +  "%s", _("domain is not running"));
> +   return -1;

You need to add one more space of indentation in the three lines above.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 14/73] qemu: Typedef struct qemuDomainJobObj

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:04PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_domain.c  | 6 +++---
src/qemu/qemu_domain.h  | 8 +---
src/qemu/qemu_process.c | 4 ++--
3 files changed, 10 insertions(+), 8 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 13/22] Implement HasManagedSaveImage method for Domain Interface

[Pavel Hrdina]

On Thu, Apr 12, 2018 at 04:32:52PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
>  data/org.libvirt.Domain.xml |  6 ++
>  src/domain.c| 29 +
>  test/test_domain.py |  2 +-
>  3 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml
> index 382b062..57e5ea4 100644
> --- a/data/org.libvirt.Domain.xml
> +++ b/data/org.libvirt.Domain.xml
> @@ -90,6 +90,12 @@
>
>
>  
> +
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainHasManagedSaveImage"/>
> +  
> +  

I would change the type to 'b', the return value is 0 or 1.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 11/22] Implement MigrateGetMaxDowntime method for Domain Interface

[Pavel Hrdina]

On Thu, Apr 12, 2018 at 04:32:50PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
>  data/org.libvirt.Domain.xml |  6 ++
>  src/domain.c| 31 +++
>  2 files changed, 37 insertions(+)

[...]

> +static void
> +virtDBusDomainMigrateGetMaxDowntime(GVariant *inArgs,
> +GUnixFDList *inFDs G_GNUC_UNUSED,
> +const gchar *objectPath,
> +gpointer userData,
> +GVariant **outArgs G_GNUC_UNUSED,

outArgs is used so remove G_GNUC_UNUSED

> +GUnixFDList **outFDs G_GNUC_UNUSED,
> +GError **error)
> +{
> +virtDBusConnect *connect = userData;
> +g_autoptr(virDomain) domain = NULL;
> +guint64 downtime;

For consistency this could be gulong and also in the previous patch.
Both are 'unsigned long'.

> +guint flags;
> +gint ret;
> +
> +g_variant_get(inArgs, "(u)", );
> +
> +domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
> +if (!domain)
> +return;
> +
> +ret = virDomainMigrateGetMaxDowntime(domain,
> + (long long unsigned int *),

The 'int' is redundant, you can remove it, and usually we put the
'unsigned' keyword as first one.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 11/73] qemu: Introduce qemuMigrationParams struct

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:41:01PM +0200, Jiri Denemark wrote:

Currently migration parameters are stored in a structure which mimics
the QEMU migration parameters handled by query-migrate-parameters and
migrate-set-parameters. The new structure will become a libvirt's
abstraction on top of QEMU migration parameters, capabilities, and
related stuff.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_driver.c   |  6 ++--
src/qemu/qemu_migration.c| 26 +++---
src/qemu/qemu_migration.h|  2 +-
src/qemu/qemu_migration_params.c | 62 
src/qemu/qemu_migration_params.h | 21 +++
5 files changed, 62 insertions(+), 55 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] external (pull) backup API

[Nikolay Shirokovskiy]

On 13.04.2018 01:10, John Snow wrote:
> 
> On 04/12/2018 04:58 AM, Nikolay Shirokovskiy wrote:
>> On 11.04.2018 19:32, Eric Blake wrote:
>>> On 04/03/2018 07:01 AM, Nikolay Shirokovskiy wrote:
> 
> 
> [snip]
> 
> 
>>>
>>> I'm trying to figure out how BlockCheckpoint and BlockSnapshots relate.
>>> Maybe it will be more clear when I read the implementation section
>>> below.  Is the idea that I can't create a BlockSnapshot without first
>>> having a checkpoint available?  If so, where does that fit in the
>>>  XML?
>>
>> No, you can create snapshot without available checkpoints. Actually the 
>> first snapshot
>> is like that.
>>
>> Now if you create a snapshot with checkpoint and then delete the snapshot
>> the checkpoint remains, so we need an API to delete them if we wish.
>>
> 
> Hmm - OK, you are being careful to label three notions separately:
> 
> (1) Checkpoints (which are metadata objects in libvirt supported by
> bitmaps in QEMU, roughly)
> (2) BlockSnapshots (which expose data using checkpoints as metadata)
> (3) Backups (which are made by a 3rd party client using a snapshot)
> 
> In this case, though, if a snapshot is requested it probably ought to be
> *prepared* to create a checkpoint in case that snapshot is used by the
> third party client to make a backup, right?

Block snapshots can be used without checkpoint altogether. Imagine we always
make full backups. It looks like next. Create block snapshot without checkpoint,
export it, make a backup, delete from export and delete snapshot. No any
checkpoints appear. On next full backup steps are the same.

> 
> IOW, a snapshot -- though ignorant of how it is used -- can be and often
> will be used to accomplish an incremental backup and as such must be
> prepared to manipulate the checkpoints/bitmaps/etc in such a way to be
> able to make a new checkpoint.
> 
> Right?

I don't unrestand what these preparations are. If we want to make incremental
backups we need to create every snapshot with checkpoint so that later we
can export CBT between current snapshot and snapshot in past.

If we create snapshot with checkpoint then internally it means we create bitmap
which starts at this point in time. Later we can use this bitmap (and
probably other bitmaps corresponding to checkpoints in the middle) to calculate
the CBT we interested in.

> 
> 
> [snip]
> 
> 
>>> Earlier, you said that the new virDomainBlockSnapshotPtr are
>>> independent, with no relations between them.  But here, you are wanting
>>> to keep incremental backups related to one another.
>>
>> Yes, but backups are not snapshots. All backup relation management are on
>> client. In pull backup scheme libvirt is only here to export a snapshotted
>> disk state with optionally a CBT from some point in time. Client itself
>> makes backups and track their relationships.
>>
>> However as we use chain of disabled bitmaps with one active bitmap on tip
>> of the chain and qemu does not track their order we need to do it in
>> libvirt.
>>
> 
> Well, you seem to be tracking it in *qemu*, by using the name field.
> Should we not make a commitment to whether or not we store this lineage
> information in either qemu OR libvirt, but not distributed across both...?
> 

I don't know actual use cases to decide. A commitment that this meta is stored
in disks like proposed can be useful IMHO so that mgmt can expect that dumb 
reinserting
disks to a different domain (recreated for example) keep all checkpoints.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] crash in libxlDoMigrateDstReceive in error case

[Olaf Hering]

Jim,

while fixing and debugging virNetSocketNewListenTCP in master, I had
breakpoints in virNetSocketNewListenTCP and virNetSocketNew. After
starting a migration the receiving libvirtd went into both functions. I
hit continue a 4 times, but forgot to do it for the fifth call to let it
proceed. libvirtd was stuck there for a long time.

As a result, when leaving virNetSocketNew, I got a crash in
libxlDoMigrateDstReceive -> libxlDomainStartRestore -> libxlDomainStart
because args->conn->privateData is NULL. This means 'driver' becomes
NULL and things fall apart.

I have no checked where privateData is set. I think some place should
catch the broken connection and stop the incoming migration.

Olaf


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] external (pull) backup API

[Nikolay Shirokovskiy]

On 13.04.2018 00:16, John Snow wrote:
> 
> 
> On 04/03/2018 08:01 AM, Nikolay Shirokovskiy wrote:
>> Hi, all. 
>> 
>>  
>> 
>> This is another RFC on pull backup API. This API provides means to read 
>> domain   
>> disks in a snapshotted state so that client can back them up as well as 
>> means
>> to write domain disks to revert them to backed up state. The previous 
>> version
>> of RFC is [1]. I'll also describe the API implementation details to shed 
>> light   
>> on misc qemu dirty bitmap commands usage.
>> 
>>  
>> 
>> This API does not use existent disks snapshots. Instead it introduces 
>> snapshots  
>> provided by qemu's blockdev-backup command. The reason is we need 
>> snapshotted
>> disk state only temporarily for duration of backup operation and newly   
>> 
>> introduced snapshots can be easily discarded at the end of operation without 
>> 
>> block commit operation. Technically difference is next. On usual snapshot we 
>> 
>> create new image backed by original and all new data goes to the new image 
>> thus  
>> original image stays in a snapshotted state. In temporary snapshots we 
>> create
>> new image backed by original and all new data still goes to the original 
>> image   
>> but before new data is written old data to be overwritten is popped out to 
>> the new   
>> image thus we get snapshotted state thru new image.  
>> 
>>  
>> 
>> Disks snapshots as well as disks itself are avaiable to read/write thru qemu 
>> 
>> NBD server.  
>> 
> 
> [snip!]
> 
> Do you think it's possible to characterize this API proposal as two
> mechanisms:
> 
> (1) A mechanism for creating and manipulating "checkpoints" -- which are
> book-ended by bitmap objects in QEMU -- implemented by the creation,
> deletion, 'disabling' and 'merging' of bitmaps, and
> 
> (2) A mechanism for the consumption of said checkpoints via NBD / the
> "fleecing" mechanisms that allow a live export of a static view of the
> disk at that time (block snapshots + NBD exports)

I can't share this view because checkpoints and snapshots are created
in one transation in pull scheme so you can't not move these two to
different mechs.

I'll rather see 2 mechanism here at least for pull scheme.

1. create snapshots (and optionally checkpoints)
2. export snapshots

> 
> If this is the case, do you think it is possible to consider (1) and (2)
> somewhat orthogonal items -- in so far as it might be possible to add
> support to libvirt directly to add push-model support for writing out
> these checkpoints?
> 
> i.e.
> 
> once you have created a temporary snapshot and merged the various
> component bitmaps into it, instead of creating an ephemeral block
> snapshot and exporting it via NBD, we request a `blockdev-backup` with a
> libvirt-specified target instead?
> 
> You don't have to add support for this right away, but I would really
> enjoy if any API we check in here has the capacity to support both
> push-and-pull paradigms without getting too ugly.
> 
> Does that sound like it can easily fit in with your designs so far?
> 

I think push scheme require 3rd (1st is fleece snapshots, 2nd is exporting
snapshots) API. First push backup has nothing to do with exporting of course. 
Second
contrary to fleece snapshots it will require additional parameter - a checkpoint
in past in case of incremental backup. It also have quite different image 
parameter.
In case of fleece snapshot fleece image will only store small delta even in 
case of
full backups. In case of push backup image will store full disk in case of full
backups.

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC v2] external (pull) backup API

[Daniel P . Berrangé]

On Tue, Apr 03, 2018 at 03:01:22PM +0300, Nikolay Shirokovskiy wrote:
> *Temporary snapshot API*
> 
> In previous version it is called 'Fleece API' after qemu terms and I'll still
> use BlockSnapshot prefix for commands as in previous RFC instead of
> TmpSnapshots which I inclined more now.
> 
> virDomainBlockSnapshotPtr
> virDomainBlockSnapshotCreateXML(virDomainPtr domain,
> const char *xmlDesc,
> unsigned int flags);
> 
> virDomainBlockSnapshotDelete(virDomainBlockSnapshotPtr snapshot,
>  unsigned int flags);
> 
> virDomainBlockSnapshotList(virDomainPtr domain,
>virDomainBlockSnapshotPtr **snaps,
>unsigned int flags);
> 
> virDomainBlockSnapshotGetXMLDesc(virDomainBlockSnapshotPtr snapshot,
>  unsigned int flags);
> 
> virDomainBlockSnapshotPtr
> virDomainBlockSnapshotLookupByName(virDomainPtr domain,
>const char *name,
>unsigned int flags);
> 
> Here is an example of snapshot xml description:
> 
> 
> d068765e-8b50-4d74-9b72-1e55c663cbf8
> 
> 

Can we just call this which is how we name
things in normal  elements.  'fleece' in particular is an awful
name giving no indication of what is being talked about unless you've
already read the QEMU low levels, so I'd rather we don't use the word
"fleece" anywhere in API or XML or docs at the libvirt level.

> 
> 
> 
> 
> 

> 
> Temporary snapshots are indepentent thus they are not organized in tree 
> structure
> as usual snapshots, so the 'list snapshots' and 'lookup' function will 
> suffice.
> 
> Qemu can track what disk's blocks are changed from snapshotted state so on 
> next
> backup client can backup only changed blocks. virDomainBlockSnapshotCreateXML
> accepts VIR_DOMAIN_BLOCK_SNAPSHOT_CREATE_CHECKPOINT flag to turn this option
> for snapshot which means to track changes from this particular snapshot. I 
> used
> checkpoint term and not [dirty] bitmap because many qemu dirty bitmaps are 
> used
> to provide changed blocks from the given checkpoint to current snapshot in
> current implementation (see *Implemenation* section for more details). Also
> bitmap keeps block changes and thus itself changes in time and checkpoint is
> a more statical terms means you can query changes from that moment in time.
> 
> Checkpoints are visible in active domain xml:
> 
> 
>   ..
>   
>   
>   
>   

How are these checkpoints recorded / associated to actual storage
on disk ? What happens across restarts of the VM if this is only
in the live XML.

>   ..
> 


> *Block export API*
> 
> I guess it is natural to treat qemu NBD server as a domain device. So

A domain device is normally something that is related to the guest
machine ABI. This is entirely invisible to the guest, just a backend
concept, so this isn't really a natural fit as a domain device.

> we can use virDomainAttachDeviceFlags/virDomainDetachDeviceFlags API to 
> start/stop NBD
> server and virDomainUpdateDeviceFlags to add/delete disks to be exported.
> While I'm have no doubts about start/stop operations using 
> virDomainUpdateDeviceFlags 
> looks a bit inconvinient so I decided to add a pair of API functions just
> to add/delete disks to be exported:
> 
> int
> virDomainBlockExportStart(virDomainPtr domain,
>   const char *xmlDesc,
>   unsigned int flags);
> 
> int
> virDomainBlockExportStop(virDomainPtr domain,
>  const char *xmlDesc,
>  unsigned int flags);
> 
> I guess more appropriate names are virDomainBlockExportAdd and
> virDomainBlockExportRemove but as I already have a patch series implementing 
> pull
> backups with these names I would like to keep these names now.
> 
> These names also reflect that in the implementation I decided to start/stop 
> NBD
> server in a lazy manner. While it is a bit innovative for libvirt API I guess
> it is convinient because to refer NBD server to add/remove disks to we need to
> identify it thru it's parameters like type, address etc until we introduce 
> some
> device id (which does not looks consistent with current libvirt design). So it
> looks like we have all parameters to start/stop server in the frame of these
> calls so why have extra API calls just to start/stop server manually. If we
> later need to have NBD server without disks we can perfectly support
> virDomainAttachDeviceFlags/virDomainDetachDeviceFlags.
> 
> Here is example of xml to add/remove disks (specifying checkpoint
> attribute is not needed for removing disks of course):
> 
> 
> 
>   checkpoint="d068765e-8b50-4d74-9b72-1e55c663cbf8"/>
>   checkpoint="d068765e-8b50-4d74-9b72-1e55c663cbf8"/>

What do all these UUIDs refer to 

Re: [libvirt] [PATCH v2 07/73] qemu: Make qemuMigrationParamsFree follow common pattern

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:40:57PM +0200, Jiri Denemark wrote:

Our *Free functions usually do not take a double pointer and the caller
has to make sure it doesn't use the stale pointer after the *Free
function returns.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_driver.c   |  2 +-
src/qemu/qemu_migration_params.c | 10 +-
src/qemu/qemu_migration_params.h |  2 +-
3 files changed, 7 insertions(+), 7 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 06/73] qemu: Reindent qemuMigrationParamsSetEmptyTLS

[Ján Tomko]

On Wed, Apr 11, 2018 at 04:40:56PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_migration_params.c | 16 
1 file changed, 8 insertions(+), 8 deletions(-)



Thank you.

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list