Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

[John Snow]

On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> To get rid of implicit filters related workarounds in future let's
> deprecate them now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  qemu-deprecated.texi  |  7 +++
>  qapi/block-core.json  |  6 --
>  include/block/block_int.h | 10 +-
>  blockdev.c| 10 ++
>  4 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 2753fafd0b..8222440148 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to sockets in 
> server mode
>  
>  Use blockdev-mirror and blockdev-backup instead.
>  
> +@subsection implicit filters (since 4.2)
> +
> +Mirror and commit jobs inserts filters, which becomes implicit if user
> +omitted filter-node-name parameter. So omitting it is deprecated, set it
> +always. Note, that drive-mirror don't have this parameter, so it will
> +create implicit filter anyway, but drive-mirror is deprecated itself too.
> +
>  @section Human Monitor Protocol (HMP) commands
>  
>  @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 
> 3.1)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4e35526634..0505ac9d8b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1596,7 +1596,8 @@
>  # @filter-node-name: the node name that should be assigned to the
>  #filter driver that the commit job inserts into the graph
>  #above @top. If this option is not given, a node name is
> -#autogenerated. (Since: 2.9)
> +#autogenerated. Omitting this option is deprecated, it 
> will
> +#be required in future. (Since: 2.9)
>  #
>  # @auto-finalize: When false, this job will wait in a PENDING state after it 
> has
>  # finished its work, waiting for @block-job-finalize before
> @@ -2249,7 +2250,8 @@
>  # @filter-node-name: the node name that should be assigned to the
>  #filter driver that the mirror job inserts into the graph
>  #above @device. If this option is not given, a node name 
> is
> -#autogenerated. (Since: 2.9)
> +#autogenerated. Omitting this option is deprecated, it 
> will
> +#be required in future. (Since: 2.9)
>  #
>  # @copy-mode: when to copy data to the destination; defaults to 'background'
>  # (Since: 3.0)
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 3aa1e832a8..624da0b4a2 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -762,7 +762,15 @@ struct BlockDriverState {
>  bool sg;/* if true, the device is a /dev/sg* */
>  bool probed;/* if true, format was probed rather than specified */
>  bool force_share; /* if true, always allow all shared permissions */
> -bool implicit;  /* if true, this filter node was automatically inserted 
> */
> +
> +/*
> + * @implicit field is deprecated, don't set it to true for new filters.
> + * If true, this filter node was automatically inserted and user don't
> + * know about it and unprepared for any effects of it. So, implicit
> + * filters are workarounded and skipped in many places of the block
> + * layer code.
> + */
> +bool implicit;
>  
>  BlockDriver *drv; /* NULL means no media */
>  void *opaque;
> diff --git a/blockdev.c b/blockdev.c
> index 36e9368e01..b3cfaccce1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char 
> *job_id, const char *device,
>  BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>  int job_flags = JOB_DEFAULT;
>  
> +if (!has_filter_node_name) {
> +warn_report("Omitting filter-node-name parameter is deprecated, it "
> +"will be required in future");
> +}
> +
>  if (!has_speed) {
>  speed = 0;
>  }
> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char 
> *job_id,
>  Error *local_err = NULL;
>  int ret;
>  
> +if (!has_filter_node_name) {
> +warn_report("Omitting filter-node-name parameter is deprecated, it "
> +"will be required in future");
> +}
> +
>  bs = qmp_get_root_bs(device, errp);
>  if (!bs) {
>  return;
> 

This might be OK to do right away, though.

I asked Markus this not too long ago; do we want to amend the QAPI
schema specification to allow commands to return with "Warning" strings,
or "Deprecated" stings to allow in-band deprecation notices for cases
like these?

example:

{ "return": {},
  "deprecated": True,
  "warning": "Omitting filter-node-name parameter is deprecated, it will
be required in the future"
}

There's no "error" key, so this should be 

Re: [libvirt] [Qemu-devel] [PATCH 1/2] qapi: deprecate drive-mirror and drive-backup

[John Snow]

On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> It's hard and not necessary to maintain outdated versions of these
> commands.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  qemu-deprecated.texi  |  4 
>  qapi/block-core.json  |  4 
>  qapi/transaction.json |  2 +-
>  blockdev.c| 10 ++
>  4 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index fff07bb2a3..2753fafd0b 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -179,6 +179,10 @@ and accurate ``query-qmp-schema'' command.
>  Character devices creating sockets in client mode should not specify
>  the 'wait' field, which is only applicable to sockets in server mode
>  
> +@subsection drive-mirror, drive-backup and drive-backup transaction action 
> (since 4.2)
> +
> +Use blockdev-mirror and blockdev-backup instead.
> +
>  @section Human Monitor Protocol (HMP) commands
>  
>  @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 
> 3.1)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0d43d4f37c..4e35526634 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1635,6 +1635,8 @@
>  ##
>  # @drive-backup:
>  #
> +# Command is deprecated, use blockdev-mirror instead.
> +#
>  # Start a point-in-time copy of a block device to a new destination.  The
>  # status of ongoing drive-backup operations can be checked with
>  # query-block-jobs where the BlockJobInfo.type field has the value 'backup'.
> @@ -1855,6 +1857,8 @@
>  ##
>  # @drive-mirror:
>  #
> +# Command is deprecated, use blockdev-mirror instead.
> +#
>  # Start mirroring a block device's writes to a new destination. target
>  # specifies the target of the new image. If the file exists, or if it
>  # is a device, it will be used as the new destination for writes. If
> diff --git a/qapi/transaction.json b/qapi/transaction.json
> index 95edb78227..a16a9ff8a6 100644
> --- a/qapi/transaction.json
> +++ b/qapi/transaction.json
> @@ -53,7 +53,7 @@
>  # - @blockdev-snapshot: since 2.5
>  # - @blockdev-snapshot-internal-sync: since 1.7
>  # - @blockdev-snapshot-sync: since 1.1
> -# - @drive-backup: since 1.6
> +# - @drive-backup: deprecated action, since 1.6
>  #
>  # Since: 1.1
>  ##
> diff --git a/blockdev.c b/blockdev.c
> index 4d141e9a1f..36e9368e01 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1771,6 +1771,9 @@ static void drive_backup_prepare(BlkActionState 
> *common, Error **errp)
>  AioContext *aio_context;
>  Error *local_err = NULL;
>  
> +warn_report("drive-backup transaction action is deprecated and will "
> +"disappear in future. Use blockdev-backup action instead");
> +
>  assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
>  backup = common->action->u.drive_backup.data;
>  
> @@ -3591,6 +3594,10 @@ void qmp_drive_backup(DriveBackup *arg, Error **errp)
>  {
>  
>  BlockJob *job;
> +
> +warn_report("drive-backup command is deprecated and will disappear in "
> +"future. Use blockdev-backup instead");
> +
>  job = do_drive_backup(arg, NULL, errp);
>  if (job) {
>  job_start(>job);
> @@ -3831,6 +3838,9 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>  const char *format = arg->format;
>  int ret;
>  
> +warn_report("drive-mirror command is deprecated and will disappear in "
> +"future. Use blockdev-mirror instead");
> +
>  bs = qmp_get_root_bs(arg->device, errp);
>  if (!bs) {
>  return;
> 

Hm!

I wonder if this is ever-so-slightly too soon for our friends over at
the libvirt project.

I don't think they have fully moved away from the non-blockdev
interfaces *just yet*, and I might encourage seeing the first full
libvirt release that does support and use it before we start the
deprecation clock.

(Jst in case.)

That's just me being very, very cautious though.

Peter Krempa, how do you feel about this?

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

[libvirt] [PATCH 6/6] qemu: Don't report some ignored errors in qemuDomainGetStatsOneBlockFallback

[Peter Krempa]

The function ignores all errors from qemuStorageLimitsRefresh by calling
virResetLastError. This still logs them. Since qemuStorageLimitsRefresh
allows suppressing some, do so.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 243a8ac4cf..f44d134b92 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -21301,7 +21301,7 @@ qemuDomainGetStatsOneBlockFallback(virQEMUDriverPtr 
driver,
 if (virStorageSourceIsEmpty(src))
 return 0;

-if (qemuStorageLimitsRefresh(driver, cfg, dom, src, false) < 0) {
+if (qemuStorageLimitsRefresh(driver, cfg, dom, src, true) <= 0) {
 virResetLastError();
 return 0;
 }
-- 
2.21.0

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

[libvirt] [PATCH 3/6] util: storagefile: Don't report errors from virStorageSourceUpdatePhysicalSize

[Peter Krempa]

virStorageSourceUpdatePhysicalSize is called only from
qemuDomainStorageUpdatePhysical and all callers of it reset the libvirt
error if -1 is returned.

Don't bother setting the error in the first place.

Signed-off-by: Peter Krempa 
---
 src/util/virstoragefile.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 4011187a7e..21bd1f00c9 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3746,7 +3746,7 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent)
  * To be called for domain storage source reporting as the volume code does
  * not set/use the 'type' field for the voldef->source.target
  *
- * Returns 0 on success, -1 on error.
+ * Returns 0 on success, -1 on error. No libvirt errors are reported.
  */
 int
 virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src,
@@ -3763,11 +3763,8 @@ virStorageSourceUpdatePhysicalSize(virStorageSourcePtr 
src,
 break;

 case VIR_STORAGE_TYPE_BLOCK:
-if ((end = lseek(fd, 0, SEEK_END)) == (off_t) -1) {
-virReportSystemError(errno, _("failed to seek to end of '%s'"),
- src->path);
+if ((end = lseek(fd, 0, SEEK_END)) == (off_t) -1)
 return -1;
-}

 src->physical = end;
 break;
@@ -3780,12 +3777,7 @@ virStorageSourceUpdatePhysicalSize(virStorageSourcePtr 
src,
 case VIR_STORAGE_TYPE_VOLUME:
 case VIR_STORAGE_TYPE_NONE:
 case VIR_STORAGE_TYPE_LAST:
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("cannot retrieve physical for path '%s' type '%s'"),
-   NULLSTR(src->path),
-   virStorageTypeToString(actual_type));
 return -1;
-break;
 }

 return 0;
-- 
2.21.0

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

[libvirt] [PATCH 5/6] qemu: Allow suppressing errors from qemuStorageLimitsRefresh

[Peter Krempa]

qemuStorageLimitsRefresh uses qemuDomainStorageOpenStat internally and
there are callers which don't care about the error. Propagate the
skipInaccessible flag so that we can log less errors.

Callers currently don't care about the return value change.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2dffef9642..243a8ac4cf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12363,6 +12363,7 @@ qemuDomainStorageUpdatePhysical(virQEMUDriverPtr driver,
  * @cfg: driver configuration data
  * @vm: domain object
  * @src: storage source data
+ * @skipInaccessible: Suppress reporting of common errors when accessing @src
  *
  * Refresh the capacity and allocation limits of a given storage source.
  *
@@ -12384,22 +12385,27 @@ qemuDomainStorageUpdatePhysical(virQEMUDriverPtr 
driver,
  * is sparse, but the amount of sparseness changes due to writes or
  * punching holes), and physical size of a non-raw file can change.
  *
- * Returns 0 on success, -1 on failure
+ * Returns 1 if @src was successfully updated, 0 if @src can't be opened and
+ * @skipInaccessible is true (no errors are reported) or -1 otherwise (errors
+ * are reported).
  */
 static int
 qemuStorageLimitsRefresh(virQEMUDriverPtr driver,
  virQEMUDriverConfigPtr cfg,
  virDomainObjPtr vm,
- virStorageSourcePtr src)
+ virStorageSourcePtr src,
+ bool skipInaccessible)
 {
+int rc;
 int ret = -1;
 int fd = -1;
 struct stat sb;
 char *buf = NULL;
 ssize_t len;

-if (qemuDomainStorageOpenStat(driver, cfg, vm, src, , , false) < 0)
-goto cleanup;
+if ((rc = qemuDomainStorageOpenStat(driver, cfg, vm, src, , ,
+skipInaccessible)) <= 0)
+return rc;

 if (virStorageSourceIsLocalStorage(src)) {
 if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, )) < 0) 
{
@@ -12427,7 +12433,7 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver,
 S_ISBLK(sb.st_mode))
 src->allocation = 0;

-ret = 0;
+ret = 1;

  cleanup:
 VIR_FREE(buf);
@@ -12477,7 +12483,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom,

 /* for inactive domains we have to peek into the files */
 if (!virDomainObjIsActive(vm)) {
-if ((qemuStorageLimitsRefresh(driver, cfg, vm, disk->src)) < 0)
+if ((qemuStorageLimitsRefresh(driver, cfg, vm, disk->src, false)) < 0)
 goto endjob;

 info->capacity = disk->src->capacity;
@@ -21295,7 +21301,7 @@ qemuDomainGetStatsOneBlockFallback(virQEMUDriverPtr 
driver,
 if (virStorageSourceIsEmpty(src))
 return 0;

-if (qemuStorageLimitsRefresh(driver, cfg, dom, src) < 0) {
+if (qemuStorageLimitsRefresh(driver, cfg, dom, src, false) < 0) {
 virResetLastError();
 return 0;
 }
-- 
2.21.0

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

[libvirt] [PATCH 4/6] qemu: driver: Improve error suppression in qemuDomainStorageUpdatePhysical

[Peter Krempa]

None of the callers of qemuDomainStorageUpdatePhysical care about
errors.

Use the new flag for qemuDomainStorageOpenStat which suppresses some
errors and move the reset of the rest of the uncommon errors into this
function. Document what is happening in a comment for the function.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 24 ++--
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cc296c1fe3..2dffef9642 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12318,6 +12318,19 @@ qemuDomainStorageCloseStat(virStorageSourcePtr src,
 }


+/**
+ * qemuDomainStorageUpdatePhysical:
+ * @driver: qemu driver
+ * @cfg: qemu driver configuration object
+ * @vm: domain object
+ * @src: storage source to update
+ *
+ * Update the physical size of the disk by reading the actual size of the image
+ * on disk.
+ *
+ * Returns 0 on successful update and -1 otherwise (some uncommon errors may be
+ * reported but are reset (thus only logged)).
+ */
 static int
 qemuDomainStorageUpdatePhysical(virQEMUDriverPtr driver,
 virQEMUDriverConfigPtr cfg,
@@ -12331,8 +12344,11 @@ qemuDomainStorageUpdatePhysical(virQEMUDriverPtr 
driver,
 if (virStorageSourceIsEmpty(src))
 return 0;

-if (qemuDomainStorageOpenStat(driver, cfg, vm, src, , , false) < 0)
+if ((ret = qemuDomainStorageOpenStat(driver, cfg, vm, src, , , 
true)) <= 0) {
+if (ret < 0)
+virResetLastError();
 return -1;
+}

 ret = virStorageSourceUpdatePhysicalSize(src, fd, );

@@ -12504,7 +12520,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
 if (qemuDomainStorageUpdatePhysical(driver, cfg, vm, disk->src) == 0) {
 info->physical = disk->src->physical;
 } else {
-virResetLastError();
 info->physical = entry->physical;
 }
 } else {
@@ -21376,12 +21391,9 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
 QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
  "physical", entry->physical);
 } else {
-if (qemuDomainStorageUpdatePhysical(driver, cfg, dom, src) == 0) {
+if (qemuDomainStorageUpdatePhysical(driver, cfg, dom, src) == 0)
 QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
  "physical", src->physical);
-} else {
-virResetLastError();
-}
 }

 ret = 0;
-- 
2.21.0

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

[libvirt] [PATCH 2/6] qemu: Allow skipping some errors in qemuDomainStorageOpenStat

[Peter Krempa]

Some callers of this function actually don't care about errors and reset
it. The message is still logged which might irritate users in this case.

Add a boolean flag which will do few checks whether it actually makes
sense to even try opening the storage file. For local files we check
whether it exists and for remote files we at first see whether we even
have a storage driver backend for it in the first place before trying to
open it.

Other problems will still report errors but these are the most common
scenarios which can happen here.

This patch changes the return value of the function so that the caller
is able to differentiate the possibilities.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 48c7b5628b..cc296c1fe3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12247,6 +12247,7 @@ qemuDomainMemoryPeek(virDomainPtr dom,
  * @src: storage source data
  * @ret_fd: pointer to return open'd file descriptor
  * @ret_sb: pointer to return stat buffer (local or remote)
+ * @skipInaccessible: Don't report error if files are not accessible
  *
  * For local storage, open the file using qemuOpenFile and then use
  * fstat() to grab the stat struct data for the caller.
@@ -12254,7 +12255,9 @@ qemuDomainMemoryPeek(virDomainPtr dom,
  * For remote storage, attempt to access the file and grab the stat
  * struct data if the remote connection supports it.
  *
- * Returns 0 on success with @ret_fd and @ret_sb populated, -1 on failure
+ * Returns 1 if @src was successfully opened (@ret_fd and @ret_sb is 
populated),
+ * 0 if @src can't be opened and @skipInaccessible is true (no errors are
+ * reported) or -1 otherwise (errors are reported).
  */
 static int
 qemuDomainStorageOpenStat(virQEMUDriverPtr driver,
@@ -12262,9 +12265,13 @@ qemuDomainStorageOpenStat(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
   virStorageSourcePtr src,
   int *ret_fd,
-  struct stat *ret_sb)
+  struct stat *ret_sb,
+  bool skipInaccessible)
 {
 if (virStorageSourceIsLocalStorage(src)) {
+if (skipInaccessible && !virFileExists(src->path))
+return 0;
+
 if ((*ret_fd = qemuOpenFile(driver, vm, src->path, O_RDONLY,
 NULL)) < 0)
 return -1;
@@ -12275,6 +12282,9 @@ qemuDomainStorageOpenStat(virQEMUDriverPtr driver,
 return -1;
 }
 } else {
+if (skipInaccessible && 
virStorageFileSupportsBackingChainTraversal(src) <= 0)
+return 0;
+
 if (virStorageFileInitAs(src, cfg->user, cfg->group) < 0)
 return -1;

@@ -12286,7 +12296,7 @@ qemuDomainStorageOpenStat(virQEMUDriverPtr driver,
 }
 }

-return 0;
+return 1;
 }


@@ -12321,7 +12331,7 @@ qemuDomainStorageUpdatePhysical(virQEMUDriverPtr driver,
 if (virStorageSourceIsEmpty(src))
 return 0;

-if (qemuDomainStorageOpenStat(driver, cfg, vm, src, , ) < 0)
+if (qemuDomainStorageOpenStat(driver, cfg, vm, src, , , false) < 0)
 return -1;

 ret = virStorageSourceUpdatePhysicalSize(src, fd, );
@@ -12372,7 +12382,7 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver,
 char *buf = NULL;
 ssize_t len;

-if (qemuDomainStorageOpenStat(driver, cfg, vm, src, , ) < 0)
+if (qemuDomainStorageOpenStat(driver, cfg, vm, src, , , false) < 0)
 goto cleanup;

 if (virStorageSourceIsLocalStorage(src)) {
-- 
2.21.0

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

[libvirt] [PATCH 0/6] qemu: Allow suppressing some errors in qemu bulk stats code

[Peter Krempa]

There is an bugreport [1] that some Openstack deployments get periodic
errors in the log. This is most probably from the bulk stats code which
in some cases calls into the storage backends to update some aspects of
storage volumes. Since none of the errors are fatal add code to allow
skipping them.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1724808

Peter Krempa (6):
  util: Export virStorageFileSupportsBackingChainTraversal
  qemu: Allow skipping some errors in qemuDomainStorageOpenStat
  util: storagefile: Don't report errors from
virStorageSourceUpdatePhysicalSize
  qemu: driver: Improve error suppression in
qemuDomainStorageUpdatePhysical
  qemu: Allow suppressing errors from qemuStorageLimitsRefresh
  qemu: Don't report some ignored errors in
qemuDomainGetStatsOneBlockFallback

 src/libvirt_private.syms  |  1 +
 src/qemu/qemu_driver.c| 60 ---
 src/util/virstoragefile.c | 16 +++
 src/util/virstoragefile.h |  1 +
 4 files changed, 50 insertions(+), 28 deletions(-)

-- 
2.21.0

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

[libvirt] [PATCH 1/6] util: Export virStorageFileSupportsBackingChainTraversal

[Peter Krempa]

The function will be reused in the qemu snapshot code. The argument is
turned into const similarly to the other virStorageFileSupports*
functions.

Signed-off-by: Peter Krempa 
---
 src/libvirt_private.syms  | 1 +
 src/util/virstoragefile.c | 4 ++--
 src/util/virstoragefile.h | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 83f40cefe6..9db4ac7933 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2994,6 +2994,7 @@ virStorageFileReportBrokenChain;
 virStorageFileResize;
 virStorageFileStat;
 virStorageFileSupportsAccess;
+virStorageFileSupportsBackingChainTraversal;
 virStorageFileSupportsCreate;
 virStorageFileSupportsSecurityDriver;
 virStorageFileUnlink;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index ba56f452e9..4011187a7e 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -4448,8 +4448,8 @@ virStorageFileGetBackendForSupportCheck(const 
virStorageSource *src,
 }


-static int
-virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src)
+int
+virStorageFileSupportsBackingChainTraversal(const virStorageSource *src)
 {
 virStorageFileBackendPtr backend;
 int rv;
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 2882bacf3e..b65cd4c382 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -533,6 +533,7 @@ int virStorageFileChown(const virStorageSource *src, uid_t 
uid, gid_t gid);
 int virStorageFileSupportsSecurityDriver(const virStorageSource *src);
 int virStorageFileSupportsAccess(const virStorageSource *src);
 int virStorageFileSupportsCreate(const virStorageSource *src);
+int virStorageFileSupportsBackingChainTraversal(const virStorageSource *src);

 int virStorageFileGetMetadata(virStorageSourcePtr src,
   uid_t uid, gid_t gid,
-- 
2.21.0

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

[libvirt] [PATCH v2 6/6] test_driver: implement virDomainUpdateDeviceFlags

[Ilias Stamatis]

Signed-off-by: Ilias Stamatis 
---
 src/test/test_driver.c | 72 ++
 1 file changed, 72 insertions(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index e271bc58f8..9bf3728654 100755
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4999,6 +4999,65 @@ testDomainDetachDeviceLiveAndConfig(virDomainDefPtr 
vmdef,
 }


+static int
+testDomainUpdateDeviceLiveAndConfig(virDomainDefPtr vmdef,
+virDomainDeviceDefPtr dev)
+{
+virDomainDiskDefPtr newDisk;
+virDomainDeviceDef oldDev = { .type = dev->type };
+virDomainNetDefPtr net;
+int idx;
+int ret = -1;
+
+switch (dev->type) {
+case VIR_DOMAIN_DEVICE_DISK:
+newDisk = dev->data.disk;
+if ((idx = virDomainDiskIndexByName(vmdef, newDisk->dst, false)) < 
0) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("target %s doesn't exist."), newDisk->dst);
+return -1;
+}
+
+oldDev.data.disk = vmdef->disks[idx];
+if (virDomainDefCompatibleDevice(vmdef, dev, ,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE,
+ false) < 0)
+return -1;
+
+virDomainDiskDefFree(vmdef->disks[idx]);
+vmdef->disks[idx] = newDisk;
+dev->data.disk = NULL;
+break;
+
+case VIR_DOMAIN_DEVICE_NET:
+net = dev->data.net;
+if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
+goto cleanup;
+
+oldDev.data.net = vmdef->nets[idx];
+if (virDomainDefCompatibleDevice(vmdef, dev, ,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE,
+ false) < 0)
+return -1;
+
+virDomainNetDefFree(vmdef->nets[idx]);
+vmdef->nets[idx] = net;
+dev->data.net = NULL;
+ret = 0;
+break;
+
+default:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("neither persistent nor live update of device is 
supported"));
+goto cleanup;
+}
+
+ret = 0;
+ cleanup:
+return ret;
+}
+
+
 typedef enum {
 TEST_DEVICE_ATTACH = 0,
 TEST_DEVICE_DETACH,
@@ -5040,6 +5099,8 @@ testDomainDeviceOperation(testDriverPtr driver,
 goto cleanup;
 break;
 case TEST_DEVICE_UPDATE:
+if (testDomainUpdateDeviceLiveAndConfig(def, dev) < 0)
+goto cleanup;
 break;
 }

@@ -5138,6 +5199,16 @@ testDomainDetachDevice(virDomainPtr dom,
 }


+static int
+testDomainUpdateDeviceFlags(virDomainPtr dom,
+const char *xml,
+unsigned int flags)
+{
+return testDomainAttachDetachUpdateDevice(dom, TEST_DEVICE_UPDATE,
+  xml, NULL, flags);
+}
+
+
 static int testDomainGetAutostart(virDomainPtr domain,
   int *autostart)
 {
@@ -9947,6 +10018,7 @@ static virHypervisorDriver testHypervisorDriver = {
 .domainDetachDevice = testDomainDetachDevice, /* 5.7.0 */
 .domainDetachDeviceFlags = testDomainDetachDeviceFlags, /* 5.7.0 */
 .domainDetachDeviceAlias = testDomainDetachDeviceAlias, /* 5.7.0 */
+.domainUpdateDeviceFlags = testDomainUpdateDeviceFlags, /* 5.7.0 */
 .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */
 .domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */
 .domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */
--
2.22.0

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

[libvirt] [PATCH v2 5/6] test_driver: implement virDomainDetachDeviceAlias

[Ilias Stamatis]

Signed-off-by: Ilias Stamatis 
---
 src/test/test_driver.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 1152ca5bed..e271bc58f8 100755
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5119,6 +5119,17 @@ testDomainDetachDeviceFlags(virDomainPtr dom,
   xml, NULL, flags);
 }

+
+static int
+testDomainDetachDeviceAlias(virDomainPtr dom,
+const char *alias,
+unsigned int flags)
+{
+return testDomainAttachDetachUpdateDevice(dom, TEST_DEVICE_DETACH,
+  NULL, alias, flags);
+}
+
+
 static int
 testDomainDetachDevice(virDomainPtr dom,
const char *xml)
@@ -9935,6 +9946,7 @@ static virHypervisorDriver testHypervisorDriver = {
 .domainAttachDeviceFlags = testDomainAttachDeviceFlags, /* 5.7.0 */
 .domainDetachDevice = testDomainDetachDevice, /* 5.7.0 */
 .domainDetachDeviceFlags = testDomainDetachDeviceFlags, /* 5.7.0 */
+.domainDetachDeviceAlias = testDomainDetachDeviceAlias, /* 5.7.0 */
 .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */
 .domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */
 .domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */
--
2.22.0

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

[libvirt] [PATCH v2 4/6] test_driver: implement virDomainDetachDevice

[Ilias Stamatis]

Signed-off-by: Ilias Stamatis 
---
 src/test/test_driver.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index ff9693ccb7..1152ca5bed 100755
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5119,6 +5119,13 @@ testDomainDetachDeviceFlags(virDomainPtr dom,
   xml, NULL, flags);
 }

+static int
+testDomainDetachDevice(virDomainPtr dom,
+   const char *xml)
+{
+return testDomainDetachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE);
+}
+

 static int testDomainGetAutostart(virDomainPtr domain,
   int *autostart)
@@ -9926,6 +9933,7 @@ static virHypervisorDriver testHypervisorDriver = {
 .domainFSTrim = testDomainFSTrim, /* 5.7.0 */
 .domainAttachDevice = testDomainAttachDevice, /* 5.7.0 */
 .domainAttachDeviceFlags = testDomainAttachDeviceFlags, /* 5.7.0 */
+.domainDetachDevice = testDomainDetachDevice, /* 5.7.0 */
 .domainDetachDeviceFlags = testDomainDetachDeviceFlags, /* 5.7.0 */
 .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */
 .domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */
--
2.22.0

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

[libvirt] [PATCH v2 3/6] test_driver: implement virDomainDetachDeviceFlags

[Ilias Stamatis]

Signed-off-by: Ilias Stamatis 
---
 src/test/test_driver.c | 178 +
 1 file changed, 178 insertions(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index abf80b97cf..ff9693ccb7 100755
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4834,6 +4834,171 @@ testDomainAttachDeviceLiveAndConfig(virDomainDefPtr 
vmdef,
 }


+static int
+testDomainDetachDeviceLiveAndConfig(virDomainDefPtr vmdef,
+virDomainDeviceDefPtr dev)
+{
+virDomainDiskDefPtr disk, detach;
+virDomainHostdevDefPtr hostdev, det_hostdev;
+virDomainControllerDefPtr cont, det_cont;
+virDomainNetDefPtr net;
+virDomainLeaseDefPtr lease, det_lease;
+virDomainFSDefPtr fs;
+virDomainMemoryDefPtr mem;
+int idx;
+
+switch (dev->type) {
+case VIR_DOMAIN_DEVICE_DISK:
+disk = dev->data.disk;
+if (!(detach = virDomainDiskRemoveByName(vmdef, disk->dst))) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("no target device %s"), disk->dst);
+return -1;
+}
+virDomainDiskDefFree(detach);
+break;
+
+case VIR_DOMAIN_DEVICE_CONTROLLER:
+cont = dev->data.controller;
+if ((idx = virDomainControllerFind(vmdef, cont->type,
+   cont->idx)) < 0) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+   _("device not present in domain 
configuration"));
+return -1;
+}
+det_cont = virDomainControllerRemove(vmdef, idx);
+virDomainControllerDefFree(det_cont);
+break;
+
+case VIR_DOMAIN_DEVICE_NET:
+net = dev->data.net;
+if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
+return -1;
+
+/* this is guaranteed to succeed */
+virDomainNetDefFree(virDomainNetRemove(vmdef, idx));
+break;
+
+case VIR_DOMAIN_DEVICE_HOSTDEV: {
+hostdev = dev->data.hostdev;
+if ((idx = virDomainHostdevFind(vmdef, hostdev, _hostdev)) < 
0) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+   _("device not present in domain 
configuration"));
+return -1;
+}
+virDomainHostdevRemove(vmdef, idx);
+virDomainHostdevDefFree(det_hostdev);
+break;
+}
+
+case VIR_DOMAIN_DEVICE_LEASE:
+lease = dev->data.lease;
+if (!(det_lease = virDomainLeaseRemove(vmdef, lease))) {
+virReportError(VIR_ERR_DEVICE_MISSING,
+   _("Lease %s in lockspace %s does not exist"),
+   lease->key, NULLSTR(lease->lockspace));
+return -1;
+}
+virDomainLeaseDefFree(det_lease);
+break;
+
+case VIR_DOMAIN_DEVICE_FS:
+fs = dev->data.fs;
+idx = virDomainFSIndexByName(vmdef, fs->dst);
+if (idx < 0) {
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+   _("no matching filesystem device was found"));
+return -1;
+}
+
+fs = virDomainFSRemove(vmdef, idx);
+virDomainFSDefFree(fs);
+break;
+
+case VIR_DOMAIN_DEVICE_RNG:
+if ((idx = virDomainRNGFind(vmdef, dev->data.rng)) < 0) {
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+   _("no matching RNG device was found"));
+return -1;
+}
+
+virDomainRNGDefFree(virDomainRNGRemove(vmdef, idx));
+break;
+
+case VIR_DOMAIN_DEVICE_MEMORY:
+if ((idx = virDomainMemoryFindInactiveByDef(vmdef,
+dev->data.memory)) < 
0) {
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+   _("matching memory device was not found"));
+return -1;
+}
+mem = virDomainMemoryRemove(vmdef, idx);
+vmdef->mem.cur_balloon -= mem->size;
+virDomainMemoryDefFree(mem);
+break;
+
+case VIR_DOMAIN_DEVICE_REDIRDEV:
+if ((idx = virDomainRedirdevDefFind(vmdef,
+dev->data.redirdev)) < 0) {
+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
+   _("no matching redirdev was not found"));
+return -1;
+}
+
+virDomainRedirdevDefFree(virDomainRedirdevDefRemove(vmdef, idx));
+break;
+
+case VIR_DOMAIN_DEVICE_SHMEM:
+if ((idx = virDomainShmemDefFind(vmdef, dev->data.shmem)) < 0) {
+virReportError(VIR_ERR_DEVICE_MISSING, 

[libvirt] [PATCH v2 2/6] test_driver: implement virDomainAttachDevice

[Ilias Stamatis]

Signed-off-by: Ilias Stamatis 
---
 src/test/test_driver.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 5f28e9017f..abf80b97cf 100755
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4935,6 +4935,14 @@ testDomainAttachDeviceFlags(virDomainPtr dom,
 }


+static int
+testDomainAttachDevice(virDomainPtr dom,
+   const char *xml)
+{
+return testDomainAttachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE);
+}
+
+
 static int testDomainGetAutostart(virDomainPtr domain,
   int *autostart)
 {
@@ -9739,6 +9747,7 @@ static virHypervisorDriver testHypervisorDriver = {
 .domainFSFreeze = testDomainFSFreeze, /* 5.7.0 */
 .domainFSThaw = testDomainFSThaw, /* 5.7.0 */
 .domainFSTrim = testDomainFSTrim, /* 5.7.0 */
+.domainAttachDevice = testDomainAttachDevice, /* 5.7.0 */
 .domainAttachDeviceFlags = testDomainAttachDeviceFlags, /* 5.7.0 */
 .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */
 .domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */
--
2.22.0

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

[libvirt] [PATCH v2 1/6] test_driver: implement virDomainAttachDeviceFlags

[Ilias Stamatis]

Signed-off-by: Ilias Stamatis 
---
 src/test/test_driver.c | 290 +
 1 file changed, 290 insertions(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index c39eef2d4b..5f28e9017f 100755
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4646,6 +4646,295 @@ testDomainFSTrim(virDomainPtr dom,
 }


+static int
+testDomainAttachDeviceLiveAndConfig(virDomainDefPtr vmdef,
+virDomainDeviceDefPtr dev)
+{
+virDomainDiskDefPtr disk;
+virDomainNetDefPtr net;
+virDomainHostdevDefPtr hostdev;
+virDomainControllerDefPtr controller;
+virDomainHostdevDefPtr found;
+virDomainLeaseDefPtr lease;
+virDomainFSDefPtr fs;
+virDomainRedirdevDefPtr redirdev;
+virDomainShmemDefPtr shmem;
+char mac[VIR_MAC_STRING_BUFLEN];
+
+switch (dev->type) {
+case VIR_DOMAIN_DEVICE_DISK:
+disk = dev->data.disk;
+if (virDomainDiskIndexByName(vmdef, disk->dst, true) >= 0) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("target %s already exists."), disk->dst);
+return -1;
+}
+
+if (virDomainDiskInsert(vmdef, disk) < 0)
+return -1;
+
+dev->data.disk = NULL;
+break;
+
+case VIR_DOMAIN_DEVICE_CONTROLLER:
+controller = dev->data.controller;
+if (controller->idx != -1 &&
+virDomainControllerFind(vmdef, controller->type,
+controller->idx) >= 0) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("Target already exists"));
+return -1;
+}
+
+if (virDomainControllerInsert(vmdef, controller) < 0)
+return -1;
+
+dev->data.controller = NULL;
+break;
+
+case VIR_DOMAIN_DEVICE_NET:
+net = dev->data.net;
+if (virDomainHasNet(vmdef, net)) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("network device with mac %s already exists"),
+   virMacAddrFormat(>mac, mac));
+return -1;
+}
+
+if (virDomainNetInsert(vmdef, net))
+return -1;
+
+dev->data.net = NULL;
+break;
+
+case VIR_DOMAIN_DEVICE_HOSTDEV:
+hostdev = dev->data.hostdev;
+if (virDomainHostdevFind(vmdef, hostdev, ) >= 0) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("device is already in the domain 
configuration"));
+return -1;
+}
+
+if (virDomainHostdevInsert(vmdef, hostdev) < 0)
+return -1;
+
+dev->data.hostdev = NULL;
+break;
+
+case VIR_DOMAIN_DEVICE_LEASE:
+lease = dev->data.lease;
+if (virDomainLeaseIndex(vmdef, lease) >= 0) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("Lease %s in lockspace %s already exists"),
+   lease->key, NULLSTR(lease->lockspace));
+return -1;
+}
+
+if (virDomainLeaseInsert(vmdef, lease) < 0)
+return -1;
+
+dev->data.lease = NULL;
+break;
+
+case VIR_DOMAIN_DEVICE_FS:
+fs = dev->data.fs;
+if (virDomainFSIndexByName(vmdef, fs->dst) >= 0) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("Target already exists"));
+return -1;
+}
+
+if (virDomainFSInsert(vmdef, fs) < 0)
+return -1;
+
+dev->data.fs = NULL;
+break;
+
+case VIR_DOMAIN_DEVICE_RNG:
+if (dev->data.rng->info.type != 
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+virDomainDefHasDeviceAddress(vmdef, >data.rng->info)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("a device with the same address already 
exists "));
+return -1;
+}
+
+if (VIR_APPEND_ELEMENT(vmdef->rngs, vmdef->nrngs, dev->data.rng) < 
0)
+return -1;
+
+dev->data.rng = NULL;
+break;
+
+case VIR_DOMAIN_DEVICE_MEMORY:
+if (vmdef->nmems == vmdef->mem.memory_slots) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("no free memory device slot available"));
+return -1;
+}
+
+vmdef->mem.cur_balloon += dev->data.memory->size;
+if (virDomainMemoryInsert(vmdef, dev->data.memory) < 0)
+return -1;
+
+dev->data.memory = NULL;
+break;
+
+case 

[libvirt] [PATCH v2 0/6] test_driver: implement device attach/detach APIs

[Ilias Stamatis]

This series needed a respin since there were conflicts when trying to
apply on the current master.

Ilias Stamatis (6):
  test_driver: implement virDomainAttachDeviceFlags
  test_driver: implement virDomainAttachDevice
  test_driver: implement virDomainDetachDeviceFlags
  test_driver: implement virDomainDetachDevice
  test_driver: implement virDomainDetachDeviceAlias
  test_driver: implement virDomainUpdateDeviceFlags

 src/test/test_driver.c | 569 +
 1 file changed, 569 insertions(+)

--
2.22.0

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

Re: [libvirt] [PATCH] util: storage: Fix parsing of 'exportname' from legacy NBD strings

[Peter Krempa]

On Wed, Jul 31, 2019 at 17:22:48 +0200, Peter Krempa wrote:
> If the nbd export name contains a colon, our parser would not parse it
> properly as we split the string by colons. Modify the code to look up
> the exportname and copy any trailing characters as the export name is
> supposed to be at the end of the string.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1733044
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/util/virstoragefile.c | 6 --
>  tests/virstoragetest.c| 8 
>  2 files changed, 12 insertions(+), 2 deletions(-)

Ping?


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

Re: [libvirt] [PATCH 15/18] virhostdev: Unify virHostdevPreparePCIDevices behaviour for KVM and VFIO cases

[Daniel Henrique Barboza]

Michal, I believe the problem you're trying to fix in this patch
is somehow the same I'm trying to fix in this patch here:


https://www.redhat.com/archives/libvir-list/2019-June/msg01270.html

Do you mind taking a look? If that's the case, I can drop that patch
from the series that implements Multifunction PCI hotplug.


Thanks,

DHB


On 8/14/19 8:57 AM, Michal Privoznik wrote:

The virHostdevPreparePCIDevices() function works in several
steps. In the very first one, it checks if devices we want to
detach from the host are not taken already by some other domain.
However, this piece of code returns different results depending
on the stub driver used (which is not wrong per se, but keep on
reading). If the stub driver is KVM then
virHostdevIsPCINodeDeviceUsed() is called which basically checks
if a PCI device from the detach list is not used by any domain
(including the one we are preparing the device for). If that is
the case, an error is reported ("device in use") and -1 is
returned.

However, that is not what happens if the stub driver is VFIO. If
the stub driver is VFIO, then we iterate over all PCI devices
from the same IOMMU group and check if they are taken by some
other domain (because a PCI device, well IOMMU group, can't be
shared between two or more qemu processes). But we fail to check,
if the device we are trying to detach from the host is not
already taken by a domain. That is, calling
virHostdevPreparePCIDevices() over a hostdev device twice
succeeds the first time and fails too late in the second run
(fortunately, virHostdevResetAllPCIDevices() will throw an error,
but this is already too late because the PCI device in question
was moved to the list of inactive PCI devices and now it appears
in both lists).

Signed-off-by: Michal Privoznik 
---
  src/util/virhostdev.c | 17 +
  1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index cb69582c21..6861b8a84e 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -53,7 +53,7 @@ struct virHostdevIsPCINodeDeviceUsedData {
  virHostdevManagerPtr mgr;
  const char *driverName;
  const char *domainName;
-const bool usesVFIO;
+bool usesVFIO;
  };
  
  /* This module makes heavy use of bookkeeping lists contained inside a

@@ -707,7 +707,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
  virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
  bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK);
  bool usesVFIO = (virPCIDeviceGetStubDriver(pci) == 
VIR_PCI_STUB_DRIVER_VFIO);
-struct virHostdevIsPCINodeDeviceUsedData data = {mgr, drv_name, 
dom_name, usesVFIO};
+struct virHostdevIsPCINodeDeviceUsedData data = {mgr, drv_name, 
dom_name, false};
  int hdrType = -1;
  
  if (virPCIGetHeaderType(pci, ) < 0)

@@ -728,18 +728,19 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
  }
  
  /* The device is in use by other active domain if

- * the dev is in list activePCIHostdevs. VFIO devices
- * belonging to same iommu group can't be shared
- * across guests.
- */
+ * the dev is in list activePCIHostdevs. */
  devAddr = virPCIDeviceGetAddress(pci);
+if (virHostdevIsPCINodeDeviceUsed(devAddr, ))
+goto cleanup;
+
+/* VFIO devices belonging to same IOMMU group can't be
+ * shared across guests. Check if that's the case. */
  if (usesVFIO) {
+data.usesVFIO = true;
  if (virPCIDeviceAddressIOMMUGroupIterate(devAddr,
   
virHostdevIsPCINodeDeviceUsed,
   ) < 0)
  goto cleanup;
-} else if (virHostdevIsPCINodeDeviceUsed(devAddr, )) {
-goto cleanup;
  }
  }
  


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

Re: [libvirt] [PATCH v2 1/2] test_driver: handle different lifecycle actions in testDomainShutdownFlags

[Erik Skultety]

On Tue, Aug 13, 2019 at 03:28:09PM +0300, Ilias Stamatis wrote:
> Signed-off-by: Ilias Stamatis 

I'll rephrase the commit subject to:
Introduce testDomainActionSetState helper

Erik

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

Re: [libvirt] [PATCH v2 0/2] test_driver: implement virDomainSetLifecycleAction

[Erik Skultety]

On Tue, Aug 13, 2019 at 03:28:08PM +0300, Ilias Stamatis wrote:
> Ilias Stamatis (2):
>   test_driver: handle different lifecycle actions in
> testDomainShutdownFlags
>   test_driver: implement virDomainSetLifecycleAction
>
>  src/test/test_driver.c | 135 +++--
>  1 file changed, 103 insertions(+), 32 deletions(-)
>

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v2 1/2] test_driver: handle different lifecycle actions in testDomainShutdownFlags

[Erik Skultety]

On Tue, Aug 13, 2019 at 03:34:02PM +0300, Ilias Stamatis wrote:
> On Tue, Aug 13, 2019 at 3:28 PM Ilias Stamatis
>  wrote:
> >
> > Signed-off-by: Ilias Stamatis 
> > ---
> >  src/test/test_driver.c | 77 --
> >  1 file changed, 45 insertions(+), 32 deletions(-)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index 5f5c512571..5a3ed45008 100755
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -1902,6 +1902,40 @@ static int testDomainSuspend(virDomainPtr domain)
> >  return ret;
> >  }
> >
> > +
> > +static void
> > +testDomainActionSetState(virDomainObjPtr dom,
> > + int lifecycle_type)
> > +{
> > +switch (lifecycle_type) {
> > +case VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY:
> > +virDomainObjSetState(dom, VIR_DOMAIN_SHUTOFF,
> > + VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> > +break;
> > +
> > +case VIR_DOMAIN_LIFECYCLE_ACTION_RESTART:
> > +virDomainObjSetState(dom, VIR_DOMAIN_RUNNING,
> > + VIR_DOMAIN_RUNNING_BOOTED);
> > +break;
> > +
> > +case VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE:
> > +virDomainObjSetState(dom, VIR_DOMAIN_SHUTOFF,
> > + VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> > +break;
> > +
> > +case VIR_DOMAIN_LIFECYCLE_ACTION_RESTART_RENAME:
> > +virDomainObjSetState(dom, VIR_DOMAIN_RUNNING,
> > + VIR_DOMAIN_RUNNING_BOOTED);
> > +break;
> > +
> > +default:
> > +virDomainObjSetState(dom, VIR_DOMAIN_SHUTOFF,
> > + VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> > +break;
> > +}
> > +}
> > +
> > +
> >  static int testDomainShutdownFlags(virDomainPtr domain,
> > unsigned int flags)
> >  {
> > @@ -1922,13 +1956,17 @@ static int testDomainShutdownFlags(virDomainPtr 
> > domain,
> >  goto cleanup;
> >  }
> >
> > -testDomainShutdownState(domain, privdom, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> > -event = virDomainEventLifecycleNewFromObj(privdom,
> > - VIR_DOMAIN_EVENT_STOPPED,
> > - VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN);
> > +testDomainActionSetState(privdom, privdom->def->onPoweroff);
> >
> > -if (!privdom->persistent)
> > -virDomainObjListRemove(privconn->domains, privdom);
> > +if (virDomainObjGetState(privdom, NULL) == VIR_DOMAIN_SHUTOFF) {
> > +testDomainShutdownState(domain, privdom, 
> > VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> > +event = virDomainEventLifecycleNewFromObj(privdom,
> > + VIR_DOMAIN_EVENT_STOPPED,
> > + 
> > VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN);

indentation...
The thing with the events like proposed is that onPoweroff "restart" would not
generate any, but I think that events are a problem for another day.

> > +
> > +if (!privdom->persistent)
> > +virDomainObjListRemove(privconn->domains, privdom);
> > +}
> >
> >  ret = 0;
> >   cleanup:
> > @@ -1967,32 +2005,7 @@ static int testDomainReboot(virDomainPtr domain,
> >  virDomainObjSetState(privdom, VIR_DOMAIN_SHUTDOWN,
> >   VIR_DOMAIN_SHUTDOWN_USER);
>
> ^ Actually probably this is not needed since it's going to get
> overridden anyways.

Yes, I'll drop that.

>
> >
> > -switch (privdom->def->onReboot) {
> > -case VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY:
> > -virDomainObjSetState(privdom, VIR_DOMAIN_SHUTOFF,
> > - VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> > -break;
> > -
> > -case VIR_DOMAIN_LIFECYCLE_ACTION_RESTART:
> > -virDomainObjSetState(privdom, VIR_DOMAIN_RUNNING,
> > - VIR_DOMAIN_RUNNING_BOOTED);
> > -break;
> > -
> > -case VIR_DOMAIN_LIFECYCLE_ACTION_PRESERVE:
> > -virDomainObjSetState(privdom, VIR_DOMAIN_SHUTOFF,
> > - VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> > -break;
> > -
> > -case VIR_DOMAIN_LIFECYCLE_ACTION_RESTART_RENAME:
> > -virDomainObjSetState(privdom, VIR_DOMAIN_RUNNING,
> > - VIR_DOMAIN_RUNNING_BOOTED);
> > -break;
> > -
> > -default:
> > -virDomainObjSetState(privdom, VIR_DOMAIN_SHUTOFF,
> > - VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> > -break;
> > -}
> > +testDomainActionSetState(privdom, privdom->def->onReboot);
> >
> >  if (virDomainObjGetState(privdom, NULL) == VIR_DOMAIN_SHUTOFF) {
> >  testDomainShutdownState(domain, privdom, 
> > VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> > --
> > 2.22.0
> >
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com

Re: [libvirt] [PATCH 00/18] Enhance virpcimock and test VFIO instead of KVM

[Daniel Henrique Barboza]

I've run make check with each individual patch, and everything
seems fine in my environment.

For all patches:

Tested-by: Daniel Henrique Barboza 


I'll see if I can drop some code reviews later on.


Thanks,


DHB

On 8/14/19 8:57 AM, Michal Privoznik wrote:

Kernel structure looks slightly different than what virpcimock creates.
This did not use to be a problem, because we are testing KVM device
assignment even though majority of systems we run on (if not all of
them) use VFIO assignment.

In order to switch our test suite (mainly virhostdevtest and virpcitest)
to test VFIO assignment, the virpcimock must be fixed. Firstly, it needs
to create symlinks under /sys/kernel/iommu_groups/... directories (patch
13/18) so that virhostdev module can iterate over them. Secondly, it
needs to create 'driver_override' file (which exists since
kernel-3.16.0) so that the virtual environment the mock creates matches
real up to date systems (patch 03/18).

Funny thing is, that enhancing the mock uncovered a bug we had (fix is
in 15/18) and also one latent bug (14/18).


As usual, these patches can be found on my github too:

   https://github.com/zippy2/libvirt/tree/virpcimock

and just for the fun of it, here's the latest travis build of that
branch:

   https://travis-ci.org/zippy2/libvirt/builds/571752953


Michal Prívozník (18):
   virpcimock: Move actions checking one level up
   Revert "virpcitest: Test virPCIDeviceDetach failure"
   virpcimock: Create driver_override file in device dirs
   virpcimock: Drop needless typecast
   virpcimock: Use VIR_AUTOFREE()
   virpcimock: Eliminate use of @fakesysfspcidir
   virpcimock: Rename @fakesysfspcidir
   virpcimock: Create devices/ and drivers/ under /sys/bus/pci upfront
   virpcimock: Introduce and use pci_device_get_path()
   virpcimock: Introduce and use pci_driver_get_path()
   virpcimock: Store PCI address as ints not string
   virpcimock: Create PCI devices under /sys/devices/pci*
   virpcimock: Create symlink in /sys/kernel/iommu_groups/N/devices dir
   virhostdev: Check driver name too in virHostdevIsPCINodeDeviceUsed()
   virhostdev: Unify virHostdevPreparePCIDevices behaviour for KVM and
 VFIO cases
   qemuxml2argvtest: Switch to modern vfio backend
   virhostdevtest: Use modern VFIO
   virpcitest: Use modern VFIO

  src/util/virhostdev.c |  26 +-
  .../hostdev-pci-address-device.args   |   2 +-
  .../qemuxml2argvdata/hostdev-pci-address.args |   2 +-
  .../net-hostdev-bootorder.args|   3 +-
  .../net-hostdev-multidomain.args  |   2 +-
  tests/qemuxml2argvdata/net-hostdev.args   |   2 +-
  tests/qemuxml2argvdata/pci-rom.args   |   4 +-
  tests/qemuxml2argvtest.c  |  14 +-
  tests/virhostdevtest.c|   4 +-
  tests/virpcimock.c| 394 --
  tests/virpcitest.c|  48 +--
  11 files changed, 304 insertions(+), 197 deletions(-)



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

[libvirt] [PATCH v3 1/2] qemu: formatting XML from domain def choosing the root name

[Maxiwell S. Garcia]

The function virDomainDefFormatInternal() has the predefined root name
"domain" to format the XML. But to save both active and inactive domain
in the snapshot XML, the new root name "inactiveDomain" was created.
So, this function was modified to be driven by the new flag
VIR_DOMAIN_DEF_FORMAT_INACTIVE_NODE to choose the correct root name.

Signed-off-by: Maxiwell S. Garcia 
---
 src/conf/domain_conf.c | 13 ++---
 src/conf/domain_conf.h |  1 +
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 03312afaaf..7d6393b9ac 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28230,6 +28230,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 unsigned char *uuid;
 char uuidstr[VIR_UUID_STRING_BUFLEN];
 const char *type = NULL;
+const char *rootname = NULL;
 int n;
 size_t i;
 char *netprefix = NULL;
@@ -28238,7 +28239,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
   VIR_DOMAIN_DEF_FORMAT_STATUS |
   VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET |
   VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES |
-  VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST,
+  VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST |
+  VIR_DOMAIN_DEF_FORMAT_INACTIVE_NODE,
   -1);
 
 if (!(type = virDomainVirtTypeToString(def->virtType))) {
@@ -28250,7 +28252,12 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 if (def->id == -1)
 flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
 
-virBufferAsprintf(buf, "id);
 if (def->namespaceData && def->ns.href)
@@ -28732,7 +28739,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 virDomainSEVDefFormat(buf, def->sev);
 
 virBufferAdjustIndent(buf, -2);
-virBufferAddLit(buf, "\n");
+virBufferAsprintf(buf, "\n", rootname);
 
 if (virBufferCheckError(buf) < 0)
 goto error;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index bce47443c8..63791a8002 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2984,6 +2984,7 @@ typedef enum {
 VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM   = 1 << 6,
 VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT  = 1 << 7,
 VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST= 1 << 8,
+VIR_DOMAIN_DEF_FORMAT_INACTIVE_NODE   = 1 << 9,
 } virDomainDefFormatFlags;
 
 /* Use these flags to skip specific domain ABI consistency checks done
-- 
2.20.1

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

[libvirt] [PATCH v3 0/2] snapshot: Store both config and live XML in the snapshot domain

[Maxiwell S. Garcia]

This patchset store both config and live XML in the snapshot XML.
To avoid nest 'config' XML one level deeper ('inactive/domain'),
it was necessary change the virDomainDefFormatInternal() to use
'domain' or 'inactiveDomain' in the root node.

Maxiwell S. Garcia (2):
  qemu: formatting XML from domain def choosing the root name
  snapshot: Store both config and live XML in the snapshot domain

 src/conf/domain_conf.c   | 13 ++---
 src/conf/domain_conf.h   |  1 +
 src/conf/moment_conf.c   |  1 +
 src/conf/moment_conf.h   | 11 +++
 src/conf/snapshot_conf.c | 23 +--
 src/qemu/qemu_driver.c   | 37 -
 6 files changed, 72 insertions(+), 14 deletions(-)

-- 
2.20.1

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

[libvirt] [PATCH v3 2/2] snapshot: Store both config and live XML in the snapshot domain

[Maxiwell S. Garcia]

The snapshot-create operation of running guests saves the live
XML and uses it to replace the active and inactive domain in
case of revert. So, the config XML is ignored by the snapshot
process. This commit changes it and adds the config XML in the
snapshot XML as the  entry.

In case of offline guest, the behavior remains the same and the
config XML is saved in the snapshot XML as  entry. The
behavior of older snapshots of running guests, that don't have
the new , remains the same too. The revert, in
this case, overrides both active and inactive domain with the
 entry. So, the  in the snapshot XML is
not required to snapshot work, but it's useful to preserve the
config XML of running guests.

Signed-off-by: Maxiwell S. Garcia 
---
 src/conf/moment_conf.c   |  1 +
 src/conf/moment_conf.h   | 11 +++
 src/conf/snapshot_conf.c | 23 +--
 src/qemu/qemu_driver.c   | 37 -
 4 files changed, 61 insertions(+), 11 deletions(-)

diff --git a/src/conf/moment_conf.c b/src/conf/moment_conf.c
index fea13f0f97..f54a44b33e 100644
--- a/src/conf/moment_conf.c
+++ b/src/conf/moment_conf.c
@@ -66,6 +66,7 @@ virDomainMomentDefDispose(void *obj)
 VIR_FREE(def->description);
 VIR_FREE(def->parent_name);
 virDomainDefFree(def->dom);
+virDomainDefFree(def->inactiveDom);
 }
 
 /* Provide defaults for creation time and moment name after parsing XML */
diff --git a/src/conf/moment_conf.h b/src/conf/moment_conf.h
index 9fdbef2172..70cc47bd70 100644
--- a/src/conf/moment_conf.h
+++ b/src/conf/moment_conf.h
@@ -36,7 +36,18 @@ struct _virDomainMomentDef {
 char *parent_name;
 long long creationTime; /* in seconds */
 
+/*
+ * Store the active domain definition in case of online
+ * guest and the inactive domain definition in case of
+ * offline guest
+ */
 virDomainDefPtr dom;
+
+/*
+ * Store the inactive domain definition in case of online
+ * guest and leave NULL in case of offline guest
+ */
+virDomainDefPtr inactiveDom;
 };
 
 virClassPtr virClassForDomainMomentDef(void);
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 7996589ad2..36435043ca 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -235,6 +235,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
 virDomainSnapshotDefPtr def = NULL;
 virDomainSnapshotDefPtr ret = NULL;
 xmlNodePtr *nodes = NULL;
+xmlNodePtr inactiveDomNode = NULL;
 size_t i;
 int n;
 char *creation = NULL, *state = NULL;
@@ -244,6 +245,8 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
 char *memoryFile = NULL;
 bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE);
 virSaveCookieCallbacksPtr saveCookie = 
virDomainXMLOptionGetSaveCookie(xmlopt);
+int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
+  VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
 
 if (!(def = virDomainSnapshotDefNew()))
 return NULL;
@@ -293,8 +296,6 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
  * clients will have to decide between best effort
  * initialization or outright failure.  */
 if ((tmp = virXPathString("string(./domain/@type)", ctxt))) {
-int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
-  VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
 xmlNodePtr domainNode = virXPathNode("./domain", ctxt);
 
 VIR_FREE(tmp);
@@ -311,6 +312,16 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
 } else {
 VIR_WARN("parsing older snapshot that lacks domain");
 }
+
+/* /inactiveDomain entry saves the config XML present in a running
+ * VM. In case of absent, leave parent.inactiveDom NULL and use
+ * parent.dom for config and live XML. */
+if ((inactiveDomNode = virXPathNode("./inactiveDomain", ctxt))) {
+def->parent.inactiveDom = virDomainDefParseNode(ctxt->node->doc, 
inactiveDomNode,
+caps, xmlopt, 
NULL, domainflags);
+if (!def->parent.inactiveDom)
+goto cleanup;
+}
 } else if (virDomainXMLOptionRunMomentPostParse(xmlopt, >parent) < 0) 
{
 goto cleanup;
 }
@@ -405,6 +416,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
 VIR_FREE(nodes);
 VIR_FREE(memorySnapshot);
 VIR_FREE(memoryFile);
+VIR_FREE(inactiveDomNode);
 virObjectUnref(def);
 
 return ret;
@@ -908,6 +920,13 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf,
 virBufferAddLit(buf, "\n");
 }
 
+if (def->parent.inactiveDom) {
+int inactivedomainflags = domainflags | 
VIR_DOMAIN_DEF_FORMAT_INACTIVE_NODE;
+if (virDomainDefFormatInternal(def->parent.inactiveDom, caps,
+   inactivedomainflags, buf, xmlopt) < 0)
+goto error;
+}
+
 if 

[libvirt] [PATCH 5/5] security_util: Remove stale XATTRs

[Michal Privoznik]

It may happen that we leave some XATTRs behind. For instance, on
a sudden power loss, the host just shuts down without calling
restore on domain paths. This creates a problem, because when the
host starts up again, the XATTRs are there but they don't reflect
the true state and this may result in libvirt denying start of a
domain.

To solve this, save a unique timestamp among with our XATTRs. The
timestamp consists of host UUID + boot timestamp.

Signed-off-by: Michal Privoznik 
---
 src/security/security_util.c | 202 ++-
 tests/qemusecuritymock.c |  12 +++
 2 files changed, 213 insertions(+), 1 deletion(-)

diff --git a/src/security/security_util.c b/src/security/security_util.c
index 365b2dd2d6..d063f526be 100644
--- a/src/security/security_util.c
+++ b/src/security/security_util.c
@@ -22,11 +22,16 @@
 #include "virfile.h"
 #include "virstring.h"
 #include "virerror.h"
+#include "virlog.h"
+#include "viruuid.h"
+#include "virhostuptime.h"
 
 #include "security_util.h"
 
 #define VIR_FROM_THIS VIR_FROM_SECURITY
 
+VIR_LOG_INIT("security.security_util");
+
 /* There are four namespaces available on Linux (xattr(7)):
  *
  *  user - can be modified by anybody,
@@ -83,6 +88,157 @@ virSecurityGetRefCountAttrName(const char *name 
ATTRIBUTE_UNUSED)
 }
 
 
+static char *
+virSecurityGetTimestampAttrName(const char *name ATTRIBUTE_UNUSED)
+{
+char *ret = NULL;
+#ifdef XATTR_NAMESPACE
+ignore_value(virAsprintf(, 
XATTR_NAMESPACE".libvirt.security.timestamp_%s", name));
+#else
+errno = ENOSYS;
+virReportSystemError(errno, "%s",
+ _("Extended attributes are not supported on this 
system"));
+#endif
+return ret;
+}
+
+
+/* This global timestamp holds combination of host UUID + boot time so that we
+ * can detect stale XATTRs. For instance, on a sudden power loss, XATTRs are
+ * not going to change (nobody will call restoreLabel()) and thus they reflect
+ * state from just before the power loss and if there was a machine running,
+ * then XATTRs there are stale and no one will ever remove them. They don't
+ * reflect the true state (most notably the ref counter).
+ */
+static char *timestamp;
+
+
+static int
+virSecurityEnsureTimestamp(void)
+{
+unsigned char uuid[VIR_UUID_BUFLEN] = {0};
+char uuidstr[VIR_UUID_STRING_BUFLEN] = {0};
+unsigned long long boottime = 0;
+
+if (timestamp)
+return 0;
+
+if (virGetHostUUID(uuid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "%s", _("cannot get the host uuid"));
+return -1;
+}
+
+virUUIDFormat(uuid, uuidstr);
+
+if (virHostGetBootTime() < 0) {
+virReportSystemError(errno, "%s",
+ _("Unable to get host boot time"));
+return -1;
+}
+
+if (virAsprintf(, "%s-%llu", uuidstr, boottime) < 0)
+return -1;
+
+return 0;
+}
+
+
+/**
+ * virSecurityValidateTimestamp:
+ * @name: security driver name
+ * @path: file name
+ *
+ * Check if remembered label on @path for security driver @name
+ * is valid, i.e. the label has been set since the last boot. If
+ * the label was set in previous runs, all XATTRs related to
+ * @name are removed so that clean slate is restored.
+ *
+ * Returns: 0 if remembered label is valid,
+ *  1 if remembered label was not valid,
+ * -1 otherwise.
+ */
+static int
+virSecurityValidateTimestamp(const char *name,
+ const char *path)
+{
+VIR_AUTOFREE(char *) timestamp_name = NULL;
+VIR_AUTOFREE(char *) value = NULL;
+
+if (virSecurityEnsureTimestamp() < 0)
+return -1;
+
+if (!(timestamp_name = virSecurityGetTimestampAttrName(name)))
+return -1;
+
+errno = 0;
+if (virFileGetXAttrQuiet(path, timestamp_name, ) < 0) {
+if (errno == ENOSYS || errno == ENOTSUP) {
+/* XATTRs are not supported. */
+return -1;
+} else if (errno != ENODATA) {
+virReportSystemError(errno,
+ _("Unable to get XATTR %s on %s"),
+ timestamp_name,
+ path);
+return -1;
+}
+
+/* Timestamp is missing. We can continue and claim a valid timestamp.
+ * But then we would never remove stale XATTRs. Therefore, claim it
+ * invalid and have the code below remove all XATTRs. This of course
+ * means that we will not restore the original owner, but the plus side
+ * is that we reset refcounter which will represent the true state.
+ */
+}
+
+if (STREQ_NULLABLE(value, timestamp)) {
+/* Hooray, XATTRs are valid. */
+VIR_DEBUG("XATTRs on %s secdriver=%s are valid", path, name);
+return 0;
+}
+
+VIR_WARN("Invalid XATTR timestamp detected on %s secdriver=%s", path, 
name);
+
+if (virSecurityMoveRememberedLabel(name, path, NULL) < 0)
+return -1;
+
+   

[libvirt] [PATCH 4/5] util: Introduce virhostuptime

[Michal Privoznik]

This module contains function to get host boot time.

Signed-off-by: Michal Privoznik 
---
 src/libvirt_private.syms |  4 +++
 src/util/Makefile.inc.am |  2 ++
 src/util/virhostuptime.c | 61 
 src/util/virhostuptime.h | 27 ++
 4 files changed, 94 insertions(+)
 create mode 100644 src/util/virhostuptime.c
 create mode 100644 src/util/virhostuptime.h

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7a3feb8efa..bed00c3cb8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2135,6 +2135,10 @@ virHostMemGetStats;
 virHostMemSetParameters;
 
 
+# util/virhostuptime.h
+virHostGetBootTime;
+
+
 # util/viridentity.h
 virIdentityGetAttr;
 virIdentityGetCurrent;
diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
index a47f333a98..46866cf213 100644
--- a/src/util/Makefile.inc.am
+++ b/src/util/Makefile.inc.am
@@ -91,6 +91,8 @@ UTIL_SOURCES = \
util/virhostdev.h \
util/virhostmem.c \
util/virhostmem.h \
+   util/virhostuptime.c \
+   util/virhostuptime.h \
util/viridentity.c \
util/viridentity.h \
util/virinitctl.c \
diff --git a/src/util/virhostuptime.c b/src/util/virhostuptime.c
new file mode 100644
index 00..f48de672e6
--- /dev/null
+++ b/src/util/virhostuptime.c
@@ -0,0 +1,61 @@
+/*
+ * virhostuptime.c: helper APIs for host uptime
+ *
+ * Copyright (C) 2019 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#if defined(__linux__) || defined(__FreeBSD__)
+# include 
+#endif
+
+#include "virhostuptime.h"
+
+
+/**
+ * virHostGetBootTime:
+ * @when: UNIX timestamp of boot time
+ *
+ * Get a UNIX timestamp of host boot time and store it at @when.
+ * Note that this function is not reentrant.
+ *
+ * Return: 0 on success,
+ *-1 otherwise.
+ */
+int
+virHostGetBootTime(unsigned long long *when ATTRIBUTE_UNUSED)
+{
+#if defined(__linux__) || defined(__FreeBSD__)
+struct utmpx id = {.ut_type = BOOT_TIME};
+struct utmpx *res = NULL;
+
+if (!(res = getutxid())) {
+int theerrno = errno;
+endutxent();
+return -theerrno;
+}
+
+*when = res->ut_tv.tv_sec;
+endutxent();
+return 0;
+#else
+/* Unfortunately, mingw lacks utmp */
+errno = ENOSYS;
+return -errno;
+#endif
+}
diff --git a/src/util/virhostuptime.h b/src/util/virhostuptime.h
new file mode 100644
index 00..03c1517a64
--- /dev/null
+++ b/src/util/virhostuptime.h
@@ -0,0 +1,27 @@
+/*
+ * virhostuptime.h: helper APIs for host uptime
+ *
+ * Copyright (C) 2019 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#pragma once
+
+#include "internal.h"
+
+int
+virHostGetBootTime(unsigned long long *when)
+ATTRIBUTE_NOINLINE;
-- 
2.21.0

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

[libvirt] [PATCH 2/5] security_util: Use more VIR_AUTOFREE()

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 src/security/security_util.c | 78 +++-
 1 file changed, 32 insertions(+), 46 deletions(-)

diff --git a/src/security/security_util.c b/src/security/security_util.c
index 9d3f483f6b..04347f51e5 100644
--- a/src/security/security_util.c
+++ b/src/security/security_util.c
@@ -113,34 +113,32 @@ virSecurityGetRememberedLabel(const char *name,
   const char *path,
   char **label)
 {
-char *ref_name = NULL;
-char *attr_name = NULL;
-char *value = NULL;
+VIR_AUTOFREE(char *) ref_name = NULL;
+VIR_AUTOFREE(char *) attr_name = NULL;
+VIR_AUTOFREE(char *) value = NULL;
 unsigned int refcount = 0;
-int ret = -1;
 
 *label = NULL;
 
 if (!(ref_name = virSecurityGetRefCountAttrName(name)))
-goto cleanup;
+return -1;
 
 if (virFileGetXAttrQuiet(path, ref_name, ) < 0) {
-if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) {
-ret = -2;
-} else {
-virReportSystemError(errno,
- _("Unable to get XATTR %s on %s"),
- ref_name,
- path);
-}
-goto cleanup;
+if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP)
+return -2;
+
+virReportSystemError(errno,
+ _("Unable to get XATTR %s on %s"),
+ ref_name,
+ path);
+return -1;
 }
 
 if (virStrToLong_ui(value, NULL, 10, ) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("malformed refcount %s on %s"),
value, path);
-goto cleanup;
+return -1;
 }
 
 VIR_FREE(value);
@@ -149,30 +147,25 @@ virSecurityGetRememberedLabel(const char *name,
 
 if (refcount > 0) {
 if (virAsprintf(, "%u", refcount) < 0)
-goto cleanup;
+return -1;
 
 if (virFileSetXAttr(path, ref_name, value) < 0)
-goto cleanup;
+return -1;
 } else {
 if (virFileRemoveXAttr(path, ref_name) < 0)
-goto cleanup;
+return -1;
 
 if (!(attr_name = virSecurityGetAttrName(name)))
-goto cleanup;
+return -1;
 
 if (virFileGetXAttr(path, attr_name, label) < 0)
-goto cleanup;
+return -1;
 
 if (virFileRemoveXAttr(path, attr_name) < 0)
-goto cleanup;
+return -1;
 }
 
-ret = 0;
- cleanup:
-VIR_FREE(value);
-VIR_FREE(attr_name);
-VIR_FREE(ref_name);
-return ret;
+return 0;
 }
 
 
@@ -201,25 +194,23 @@ virSecuritySetRememberedLabel(const char *name,
   const char *path,
   const char *label)
 {
-char *ref_name = NULL;
-char *attr_name = NULL;
-char *value = NULL;
+VIR_AUTOFREE(char *) ref_name = NULL;
+VIR_AUTOFREE(char *) attr_name = NULL;
+VIR_AUTOFREE(char *) value = NULL;
 unsigned int refcount = 0;
-int ret = -1;
 
 if (!(ref_name = virSecurityGetRefCountAttrName(name)))
-goto cleanup;
+return -1;
 
 if (virFileGetXAttrQuiet(path, ref_name, ) < 0) {
 if (errno == ENOSYS || errno == ENOTSUP) {
-ret = -2;
-goto cleanup;
+return -2;
 } else if (errno != ENODATA) {
 virReportSystemError(errno,
  _("Unable to get XATTR %s on %s"),
  ref_name,
  path);
-goto cleanup;
+return -1;
 }
 }
 
@@ -228,7 +219,7 @@ virSecuritySetRememberedLabel(const char *name,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("malformed refcount %s on %s"),
value, path);
-goto cleanup;
+return -1;
 }
 
 VIR_FREE(value);
@@ -237,24 +228,19 @@ virSecuritySetRememberedLabel(const char *name,
 
 if (refcount == 1) {
 if (!(attr_name = virSecurityGetAttrName(name)))
-goto cleanup;
+return -1;
 
 if (virFileSetXAttr(path, attr_name, label) < 0)
-goto cleanup;
+return -1;
 }
 
 if (virAsprintf(, "%u", refcount) < 0)
-goto cleanup;
+return -1;
 
 if (virFileSetXAttr(path, ref_name, value) < 0)
-goto cleanup;
+return -1;
 
-ret = refcount;
- cleanup:
-VIR_FREE(value);
-VIR_FREE(attr_name);
-VIR_FREE(ref_name);
-return ret;
+return refcount;
 }
 
 
-- 
2.21.0

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

[libvirt] [PATCH 1/5] virUUIDFormat: s/VIR_UUID_RAW_LEN/VIR_UUID_BUFLEN/ in comment

[Michal Privoznik]

The function takes raw UUID and formats it into string
representation. However, the comment mistakenly states that the
expected size of raw UUID buffer is VIR_UUID_RAW_LEN bytes. We
don't have such constant since v0.3.2~24. It should have been
VIR_UUID_BUFLEN.

Signed-off-by: Michal Privoznik 
---
 src/util/viruuid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/viruuid.c b/src/util/viruuid.c
index 0c12ddcc3e..8930a0e199 100644
--- a/src/util/viruuid.c
+++ b/src/util/viruuid.c
@@ -141,7 +141,7 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid)
 
 /**
  * virUUIDFormat:
- * @uuid: array of VIR_UUID_RAW_LEN bytes to store the raw UUID
+ * @uuid: array of VIR_UUID_BUFLEN bytes to store the raw UUID
  * @uuidstr: array of VIR_UUID_STRING_BUFLEN bytes to store the
  * string representation of the UUID in. The resulting string
  * will be NULL terminated.
-- 
2.21.0

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

[libvirt] [PATCH 0/5] security: Deal with stale XATTRs

[Michal Privoznik]

There are some ways users can end up in stale XATTRs. One is sudden
power loss, the other is stopping libvirt whilst some domains are
running and then rebooting the host. And I believe users will find other
creative ways to shut down domains without qemuProcessStop() being
called. When that happens our XATTRs will be left behind and not reflect
the real state of things (e.g. refcounter). To resolve this, record a
timestamp within XATTRs too so that host reboots can be detected.

Michal Prívozník (5):
  virUUIDFormat: s/VIR_UUID_RAW_LEN/VIR_UUID_BUFLEN/ in comment
  security_util: Use more VIR_AUTOFREE()
  security_util: Document virSecurityMoveRememberedLabel
  util: Introduce virhostuptime
  security_util: Remove stale XATTRs

 src/libvirt_private.syms |   4 +
 src/security/security_util.c | 293 +--
 src/util/Makefile.inc.am |   2 +
 src/util/virhostuptime.c |  61 
 src/util/virhostuptime.h |  27 
 src/util/viruuid.c   |   2 +-
 tests/qemusecuritymock.c |  12 ++
 7 files changed, 353 insertions(+), 48 deletions(-)
 create mode 100644 src/util/virhostuptime.c
 create mode 100644 src/util/virhostuptime.h

-- 
2.21.0

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

[libvirt] [PATCH 3/5] security_util: Document virSecurityMoveRememberedLabel

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 src/security/security_util.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/src/security/security_util.c b/src/security/security_util.c
index 04347f51e5..365b2dd2d6 100644
--- a/src/security/security_util.c
+++ b/src/security/security_util.c
@@ -244,6 +244,19 @@ virSecuritySetRememberedLabel(const char *name,
 }
 
 
+/**
+ * virSecurityMoveRememberedLabel:
+ * @name: security driver name
+ * @src: source file
+ * @dst: destination file
+ *
+ * For given security driver @name, move all XATTRs related to seclabel
+ * remembering from @src to @dst. However, if @dst is NULL, then XATTRs
+ * are just removed from @src.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise.
+ */
 int
 virSecurityMoveRememberedLabel(const char *name,
const char *src,
-- 
2.21.0

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

Re: [libvirt] [PATCH] util: mdev: support persistent devices with mdevctl

[Boris Fiuczynski]

On 8/14/19 12:02 AM, Jonathon Jongsma wrote:

When a host is rebooted, mediated devices disappear from sysfs.  mdevctl
(https://github.com/mdevctl/mdevctl) is a utility for managing and
persisting these devices. It maintains a registry of mediated devices
and can start and stop them by UUID.

when libvirt attempts to create a new mediated device object, we currently
fail if the path does not exist in sysfs. This patch tries a little bit
harder by using mdevctl to attempt to activate the device.  The approach
is fairly naive -- it just calls 'mdevctl start -u $UUID' without
checking whether this UUID is registered with mdevctl or not.

See https://bugzilla.redhat.com/show_bug.cgi?id=1699274

Signed-off-by: Jonathon Jongsma 
---
NOTES:
- an argument could be made that we should simply do nothing here. mdevctl does
   have support for automatically activating the mediated device when the parent
   device becomes available (via udev). So if the administrator set up the mdev
   to start automatically, this patch should not even be necessary. That said, I
   think this patch could still be useful.


I would actually like to use this argument since this patch 
unconditionally starts/creates a persistently defined mdev without ever 
stopping/destroying it and also not looking if it is defined as to be 
automatically started or manually started. If the mdev is specified in 
mdevctl to be started automatically I would rate this as mdevctl is in 
control of this mdevs lifecycle and libvirt should not interfere with 
it. It might be that I am over-interpreting auto and manual as start 
options in mdevctl but it feels to me that libvirt and mdevctl should 
not run into a management clash of host resources.


In addition what about a user specifiable tag in the domain xmls mdev 
definition which controls the management behavior of an mdev with 
mdevctl or another tool?




- As I said above, the approach is pretty naive. I could have checked whether
   the device was defined in mdevctl's registry and given a different error
   message ("try registering this device with mdevctl") in that scenario. But
   I chose to keep it simple.

  src/util/virmdev.c | 22 +++---
  1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index 3d5488cdae..7a2f0adedc 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -25,6 +25,7 @@
  #include "virfile.h"
  #include "virstring.h"
  #include "viralloc.h"
+#include "vircommand.h"
  
  #define VIR_FROM_THIS VIR_FROM_NONE
  
@@ -148,9 +149,24 @@ virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model)

  return NULL;
  
  if (!virFileExists(sysfspath)) {

-virReportError(VIR_ERR_DEVICE_MISSING,
-   _("mediated device '%s' not found"), uuidstr);
-return NULL;
+bool activated = false;
+/* if the mdev device path doesn't exist, we may still be able to start
+ * the device using mdevctl
+ */
+char *mdevctl = virFindFileInPath("mdevctl");
+if (mdevctl) {
+const char *runargs[] = { mdevctl, "start", "-u", uuidstr, NULL };
+if (virRun(runargs, NULL) == 0) {
+/* check to see if it the device exists now */
+activated = virFileExists(sysfspath);
+}
+}
+
+if (!activated) {
+virReportError(VIR_ERR_DEVICE_MISSING,
+   _("mediated device '%s' not found"), uuidstr);
+return NULL;
+}
  }
  
  if (VIR_ALLOC(dev) < 0)





--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[libvirt] 5.6.0 regression: "Some activation file descriptors are unclaimed"

[Allen, John]

After upgrading to v5.6.0, starting libvirtd fails with the following message
in journalctl -xe:

libvirtd[186338]: internal error: Some activation file descriptors are unclaimed

5b8569dd6e284b9159c701e8bffafb196983fc4a introduces the message. The commit
message indicates that systemd version 227 is required, but my system seems to
be running version 237.

Is this a known issue? Is there any other configuration for systemd needed to
avoid the problem?

Thanks,
John

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

[libvirt] [PATCH] remote_daemon_dispatch.c: typecast ARRAY_CARDINALITY() in remoteDispatchProbeURI()

[Michal Privoznik]

Since users can enable/disable drivers at compile time, it may
happen that @drivers array is in fact empty (in both its
occurrences within the function). This means that
ARRAY_CARDINALITY() returns 0UL which makes gcc unhappy because
of loop condition:

  i < ARRAY_CARDINALITY(drivers)

GCC complains that @i is unsigned and comparing an unsigned value
against 0 is always false. However, changing the type of @i to
ssize_t is not enough, because compiler still sees the unsigned
zero. The solution is to typecast the ARRAY_CARDINALITY().

Signed-off-by: Michal Privoznik 
---
 src/remote/remote_daemon_dispatch.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/remote/remote_daemon_dispatch.c 
b/src/remote/remote_daemon_dispatch.c
index c8e353ebd3..1bd281dd6d 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -2141,9 +2141,9 @@ remoteDispatchProbeURI(bool readonly,
 "vbox",
 # endif
 };
-size_t i;
+ssize_t i;
 
-for (i = 0; i < ARRAY_CARDINALITY(drivers) && !*probeduri; i++) {
+for (i = 0; i < (ssize_t) ARRAY_CARDINALITY(drivers) && !*probeduri; 
i++) {
 VIR_AUTOFREE(char *) daemonname = NULL;
 VIR_AUTOFREE(char *) daemonpath = NULL;
 
@@ -2187,9 +2187,9 @@ remoteDispatchProbeURI(bool readonly,
 "vz",
 # endif
 };
-size_t i;
+ssize_t i;
 
-for (i = 0; i < ARRAY_CARDINALITY(drivers) && !*probeduri; i++) {
+for (i = 0; i < (ssize_t) ARRAY_CARDINALITY(drivers) && !*probeduri; 
i++) {
 VIR_AUTOFREE(char *) sockname = NULL;
 
 if (virAsprintf(, "%s/run/libvirt/virt%sd-%s",
-- 
2.21.0

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

[libvirt] [PATCH] src: Don't check lxc_monitor_protocol-struct when LXC is disabled

[Michal Privoznik]

If LXC is disabled at build time then there is no
libvirt_driver_lxc_impl_la-*.lo to run the 'check-protocol'
against.

Signed-off-by: Michal Privoznik 
---
 src/Makefile.am | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/Makefile.am b/src/Makefile.am
index 3ae4b87abb..817a7ecf34 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -318,9 +318,11 @@ $(srcdir)/remote_protocol-struct \
 $(srcdir)/virnetprotocol-struct $(srcdir)/virkeepaliveprotocol-struct: \
$(srcdir)/%-struct: rpc/libvirt_net_rpc_la-%.lo
$(PDWTAGS)
+if WITH_LXC
 $(srcdir)/lxc_monitor_protocol-struct: \
$(srcdir)/%-struct: lxc/libvirt_driver_lxc_impl_la-%.lo
$(PDWTAGS)
+endif WITH_LXC
 $(srcdir)/lock_protocol-struct: \
$(srcdir)/%-struct: locking/lockd_la-%.lo
$(PDWTAGS)
-- 
2.21.0

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

Re: [libvirt] [PATCH 0/3] virNetDaemonCallInhibit: check for logind availability

[Michal Privoznik]

On 8/13/19 5:08 PM, Ján Tomko wrote:

* get rid of a pointless error logged when system D-Bus is not available
* do not send messages on systems without logind
* remove four extra dbus calls on libvirtd startup when checking for
   logind availability

Ján Tomko (3):
   util: introduce virSystemdHasLogind
   util: cache the result of whether logind is available
   rpc: make virNetDaemonCallInhibit a no-op with no logind

  src/libvirt_private.syms  |  2 ++
  src/rpc/virnetdaemon.c|  3 +++
  src/util/virsystemd.c | 37 -
  src/util/virsystemd.h |  2 ++
  src/util/virsystemdpriv.h |  1 +
  tests/virsystemdtest.c|  3 +++
  6 files changed, 43 insertions(+), 5 deletions(-)



Reviewed-by: Michal Privoznik 

Michal

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

Re: [libvirt] [PATCH 0/5] util: open pci config read-only for querying

[Michal Privoznik]

On 8/13/19 3:45 PM, Ján Tomko wrote:

A bunch of functions opened the PCI device config as read-write
even though they only read from it.

Ján Tomko (5):
   util: introduce virPCIDeviceConfigOpenInternal
   util: Introduce virPCIDeviceConfigOpenWrite
   util: introduce readonly attribute to virPCIDeviceConfigOpenInternal
   util: introduce virPCIDeviceConfigOpenTry
   util: default to read-only in virPCIDeviceConfigOpen

  src/util/virpci.c | 40 +---
  1 file changed, 29 insertions(+), 11 deletions(-)



Reviewed-by: Michal Privoznik 

Michal

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

[libvirt] [PATCH 13/18] virpcimock: Create symlink in /sys/kernel/iommu_groups/N/devices dir

[Michal Privoznik]

So far, we don't need to create anything under
/sys/kernel/iommu_groups/N/devices directory (which is symlinked
from /sys/bus/pci/devices/:BB:DD.F/iommu_group directory)
because virhostdevtest still tests the old KVM assignment and
thus has no notion of IOMMU groups. This will change in near
future though. And in order to discover devices belonging to the
same IOMMU group we need to do what kernel does - create symlinks
to devices.

Signed-off-by: Michal Privoznik 
---
 tests/virpcimock.c | 32 +---
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 0774bf62d9..6f315217e2 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -400,6 +400,30 @@ pci_device_get_path(const struct pciDevice *dev,
 }
 
 
+static void
+pci_device_create_iommu(const struct pciDevice *dev,
+const char *devid)
+{
+VIR_AUTOFREE(char *) iommuPath = NULL;
+char tmp[256];
+
+if (virAsprintfQuiet(, "%s/sys/kernel/iommu_groups/%d/devices/",
+ fakerootdir, dev->iommuGroup) < 0)
+ABORT_OOM();
+
+if (virFileMakePath(iommuPath) < 0)
+ABORT("Unable to create: %s", iommuPath);
+
+if (snprintf(tmp, sizeof(tmp),
+ "../../../../devices/pci%04x:%02x/%s",
+ dev->addr.domain, dev->addr.bus, devid) < 0) {
+ABORT("@tmp overflow");
+}
+
+make_symlink(iommuPath, devid, tmp);
+}
+
+
 static void
 pci_device_new_from_stub(const struct pciDevice *data)
 {
@@ -480,13 +504,7 @@ pci_device_new_from_stub(const struct pciDevice *data)
 
 make_file(devpath, "driver_override", NULL, -1);
 
-if (snprintf(tmp, sizeof(tmp),
- "%s/../../../kernel/iommu_groups/%d",
- devpath, dev->iommuGroup) < 0) {
-ABORT("@tmp overflow");
-}
-if (virFileMakePath(tmp) < 0)
-ABORT("Unable to create %s", tmp);
+pci_device_create_iommu(dev, devid);
 
 if (snprintf(tmp, sizeof(tmp),
  "../../../kernel/iommu_groups/%d", dev->iommuGroup) < 0) {
-- 
2.21.0

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

[libvirt] [PATCH 18/18] virpcitest: Use modern VFIO

[Michal Privoznik]

The pci-stub is so old school that no one uses it. All modern
systems have adapted VFIO. Switch our virpcitest too.

Signed-off-by: Michal Privoznik 
---
 tests/virpcitest.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/virpcitest.c b/tests/virpcitest.c
index 9ecd1b7d27..0bd37268ef 100644
--- a/tests/virpcitest.c
+++ b/tests/virpcitest.c
@@ -106,12 +106,12 @@ testVirPCIDeviceDetach(const void *opaque 
ATTRIBUTE_UNUSED)
 if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0)))
 goto cleanup;
 
-virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_KVM);
+virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO);
 
 if (virPCIDeviceDetach(dev[i], activeDevs, inactiveDevs) < 0)
 goto cleanup;
 
-if (testVirPCIDeviceCheckDriver(dev[i], "pci-stub") < 0)
+if (testVirPCIDeviceCheckDriver(dev[i], "vfio-pci") < 0)
 goto cleanup;
 
 CHECK_LIST_COUNT(activeDevs, 0);
@@ -147,7 +147,7 @@ testVirPCIDeviceReset(const void *opaque ATTRIBUTE_UNUSED)
 if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0)))
 goto cleanup;
 
-virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_KVM);
+virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO);
 
 if (virPCIDeviceReset(dev[i], activeDevs, inactiveDevs) < 0)
 goto cleanup;
@@ -187,7 +187,7 @@ testVirPCIDeviceReattach(const void *opaque 
ATTRIBUTE_UNUSED)
 CHECK_LIST_COUNT(activeDevs, 0);
 CHECK_LIST_COUNT(inactiveDevs, i + 1);
 
-virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_KVM);
+virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO);
 }
 
 CHECK_LIST_COUNT(activeDevs, 0);
@@ -245,7 +245,7 @@ testVirPCIDeviceDetachSingle(const void *opaque)
 if (!dev)
 goto cleanup;
 
-virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_KVM);
+virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_VFIO);
 
 if (virPCIDeviceDetach(dev, NULL, NULL) < 0)
 goto cleanup;
@@ -405,7 +405,7 @@ mymain(void)
 /* Detach an unbound device */
 DO_TEST_PCI_DRIVER(0, 0x0a, 2, 0, NULL);
 DO_TEST_PCI(testVirPCIDeviceDetachSingle, 0, 0x0a, 2, 0);
-DO_TEST_PCI_DRIVER(0, 0x0a, 2, 0, "pci-stub");
+DO_TEST_PCI_DRIVER(0, 0x0a, 2, 0, "vfio-pci");
 
 /* Reattach an unknown unbound device */
 DO_TEST_PCI_DRIVER(0, 0x0a, 3, 0, NULL);
-- 
2.21.0

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

[libvirt] [PATCH 16/18] qemuxml2argvtest: Switch to modern vfio backend

[Michal Privoznik]

The pci-assign device is so old school that no one uses it. All
modern systems have adapted VFIO. Switch our xml2argv test too.

Signed-off-by: Michal Privoznik 
---
 .../hostdev-pci-address-device.args|  2 +-
 tests/qemuxml2argvdata/hostdev-pci-address.args|  2 +-
 tests/qemuxml2argvdata/net-hostdev-bootorder.args  |  3 +--
 .../qemuxml2argvdata/net-hostdev-multidomain.args  |  2 +-
 tests/qemuxml2argvdata/net-hostdev.args|  2 +-
 tests/qemuxml2argvdata/pci-rom.args|  4 ++--
 tests/qemuxml2argvtest.c   | 14 +++---
 7 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/tests/qemuxml2argvdata/hostdev-pci-address-device.args 
b/tests/qemuxml2argvdata/hostdev-pci-address-device.args
index 5159b00ef1..79654f44bb 100644
--- a/tests/qemuxml2argvdata/hostdev-pci-address-device.args
+++ b/tests/qemuxml2argvdata/hostdev-pci-address-device.args
@@ -27,5 +27,5 @@ server,nowait \
 -usb \
 -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
--device pci-assign,host=:06:12.5,id=hostdev0,bus=pci.0,addr=0x3 \
+-device vfio-pci,host=:06:12.5,id=hostdev0,bus=pci.0,addr=0x3 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/hostdev-pci-address.args 
b/tests/qemuxml2argvdata/hostdev-pci-address.args
index fe6acaaf62..0a57a8f29e 100644
--- a/tests/qemuxml2argvdata/hostdev-pci-address.args
+++ b/tests/qemuxml2argvdata/hostdev-pci-address.args
@@ -27,4 +27,4 @@ server,nowait \
 -usb \
 -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
--device pci-assign,host=:06:12.5,id=hostdev0,bus=pci.0,addr=0x3
+-device vfio-pci,host=:06:12.5,id=hostdev0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/net-hostdev-bootorder.args 
b/tests/qemuxml2argvdata/net-hostdev-bootorder.args
index eefc247eed..515b0c58d3 100644
--- a/tests/qemuxml2argvdata/net-hostdev-bootorder.args
+++ b/tests/qemuxml2argvdata/net-hostdev-bootorder.args
@@ -27,5 +27,4 @@ server,nowait \
 -usb \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=2 \
--device pci-assign,host=:03:07.1,id=hostdev0,bootindex=1,bus=pci.0,\
-addr=0x3
+-device vfio-pci,host=:03:07.1,id=hostdev0,bootindex=1,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/net-hostdev-multidomain.args 
b/tests/qemuxml2argvdata/net-hostdev-multidomain.args
index bf7686f49c..18c5e98188 100644
--- a/tests/qemuxml2argvdata/net-hostdev-multidomain.args
+++ b/tests/qemuxml2argvdata/net-hostdev-multidomain.args
@@ -27,5 +27,5 @@ server,nowait \
 -usb \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
--device pci-assign,host=2424:21:1c.6,id=hostdev0,bus=pci.0,addr=0x3 \
+-device vfio-pci,host=2424:21:1c.6,id=hostdev0,bus=pci.0,addr=0x3 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/net-hostdev.args 
b/tests/qemuxml2argvdata/net-hostdev.args
index 94eac176f3..aa9e91db82 100644
--- a/tests/qemuxml2argvdata/net-hostdev.args
+++ b/tests/qemuxml2argvdata/net-hostdev.args
@@ -27,5 +27,5 @@ server,nowait \
 -usb \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
--device pci-assign,host=:03:07.1,id=hostdev0,bus=pci.0,addr=0x3 \
+-device vfio-pci,host=:03:07.1,id=hostdev0,bus=pci.0,addr=0x3 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/pci-rom.args 
b/tests/qemuxml2argvdata/pci-rom.args
index 7235642ee8..4a5dc4161a 100644
--- a/tests/qemuxml2argvdata/pci-rom.args
+++ b/tests/qemuxml2argvdata/pci-rom.args
@@ -33,7 +33,7 @@ addr=0x3,rombar=1 \
 -netdev user,id=hostnet1 \
 -device 
virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:24:a5:9e,bus=pci.0,\
 addr=0x4,romfile=/etc/fake/bootrom.bin \
--device pci-assign,host=:06:12.5,id=hostdev0,bus=pci.0,addr=0x5,rombar=0 \
--device pci-assign,host=:06:12.6,id=hostdev1,bus=pci.0,addr=0x6,rombar=1,\
+-device vfio-pci,host=:06:12.5,id=hostdev0,bus=pci.0,addr=0x5,rombar=0 \
+-device vfio-pci,host=:06:12.6,id=hostdev1,bus=pci.0,addr=0x6,rombar=1,\
 romfile=/etc/fake/bootrom.bin \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index c166fd18d6..26b512d246 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -496,7 +496,7 @@ testCompareXMLToArgv(const void *data)
 if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
 hostdev->source.subsys.type == 

[libvirt] [PATCH 07/18] virpcimock: Rename @fakesysfspcidir

[Michal Privoznik]

We will need to create more directories and instead of
introducing bunch of new variables to hold their actual
paths, we can have one and reuse it.

Signed-off-by: Michal Privoznik 
---
 tests/virpcimock.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 296da9b453..e97dbd81f8 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -831,7 +831,7 @@ init_syms(void)
 static void
 init_env(void)
 {
-VIR_AUTOFREE(char *) fakesysfspcidir = NULL;
+VIR_AUTOFREE(char *) tmp = NULL;
 
 if (fakerootdir)
 return;
@@ -839,14 +839,14 @@ init_env(void)
 if (!(fakerootdir = getenv("LIBVIRT_FAKE_ROOT_DIR")))
 ABORT("Missing LIBVIRT_FAKE_ROOT_DIR env variable\n");
 
-if (virAsprintfQuiet(, "%s%s",
+if (virAsprintfQuiet(, "%s%s",
  fakerootdir, SYSFS_PCI_PREFIX) < 0)
 ABORT_OOM();
 
-if (virFileMakePath(fakesysfspcidir) < 0)
-ABORT("Unable to create: %s", fakesysfspcidir);
+if (virFileMakePath(tmp) < 0)
+ABORT("Unable to create: %s", tmp);
 
-make_file(fakesysfspcidir, "drivers_probe", NULL, -1);
+make_file(tmp, "drivers_probe", NULL, -1);
 
 # define MAKE_PCI_DRIVER(name, ...) \
 pci_driver_new(name, 0, __VA_ARGS__, -1, -1)
-- 
2.21.0

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

[libvirt] [PATCH 02/18] Revert "virpcitest: Test virPCIDeviceDetach failure"

[Michal Privoznik]

This reverts commit b70c093ffa00cd87c8d39d3652b798f033a81faf.

In next commit the virpcimock is going to be extended and thus
binding a PCI device to vfio-pci driver will finally succeed.
Remove this test as it will no longer make sense.

Signed-off-by: Michal Privoznik 
---
 tests/virpcitest.c | 32 
 1 file changed, 32 deletions(-)

diff --git a/tests/virpcitest.c b/tests/virpcitest.c
index 961a7eff1a..9ecd1b7d27 100644
--- a/tests/virpcitest.c
+++ b/tests/virpcitest.c
@@ -256,36 +256,6 @@ testVirPCIDeviceDetachSingle(const void *opaque)
 return ret;
 }
 
-static int
-testVirPCIDeviceDetachFail(const void *opaque)
-{
-const struct testPCIDevData *data = opaque;
-int ret = -1;
-virPCIDevicePtr dev;
-
-dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function);
-if (!dev)
-goto cleanup;
-
-virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_VFIO);
-
-if (virPCIDeviceDetach(dev, NULL, NULL) < 0) {
-if (virTestGetVerbose() || virTestGetDebug())
-virDispatchError(NULL);
-virResetLastError();
-ret = 0;
-} else {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "Attaching device %s to %s should have failed",
-   virPCIDeviceGetName(dev),
-   virPCIStubDriverTypeToString(VIR_PCI_STUB_DRIVER_VFIO));
-}
-
- cleanup:
-virPCIDeviceFree(dev);
-return ret;
-}
-
 static int
 testVirPCIDeviceReattachSingle(const void *opaque)
 {
@@ -421,8 +391,6 @@ mymain(void)
 DO_TEST_PCI(testVirPCIDeviceIsAssignable, 5, 0x90, 1, 0);
 DO_TEST_PCI(testVirPCIDeviceIsAssignable, 1, 1, 0, 0);
 
-DO_TEST_PCI(testVirPCIDeviceDetachFail, 0, 0x0a, 1, 0);
-
 /* Reattach a device already bound to non-stub a driver */
 DO_TEST_PCI_DRIVER(0, 0x0a, 1, 0, "i915");
 DO_TEST_PCI(testVirPCIDeviceReattachSingle, 0, 0x0a, 1, 0);
-- 
2.21.0

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

[libvirt] [PATCH 14/18] virhostdev: Check driver name too in virHostdevIsPCINodeDeviceUsed()

[Michal Privoznik]

It may happen that there are two domains with the same name in
two separate drivers (e.g. qemu and lxc). That is why for PCI
devices we track both names of driver and domain combination
which has taken the device. However, when we check if given PCI
device is in use (or PCI devices from the same IOMMU group) we
compare only domain name. This means that we can mistakenly claim
device as free to use while in fact it isn't.

Signed-off-by: Michal Privoznik 
---
 src/util/virhostdev.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 75e44b5227..cb69582c21 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -51,6 +51,7 @@ static virHostdevManagerPtr virHostdevManagerNew(void);
 
 struct virHostdevIsPCINodeDeviceUsedData {
 virHostdevManagerPtr mgr;
+const char *driverName;
 const char *domainName;
 const bool usesVFIO;
 };
@@ -91,8 +92,8 @@ static int 
virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr devAddr, void *o
 virPCIDeviceGetUsedBy(actual, _drvname, _domname);
 
 if (helperData->usesVFIO &&
-(actual_domname && helperData->domainName) &&
-(STREQ(actual_domname, helperData->domainName)))
+STREQ_NULLABLE(actual_drvname, helperData->driverName) &&
+STREQ_NULLABLE(actual_domname, helperData->domainName))
 goto iommu_owner;
 
 if (actual_drvname && actual_domname)
@@ -706,7 +707,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
 virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
 bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK);
 bool usesVFIO = (virPCIDeviceGetStubDriver(pci) == 
VIR_PCI_STUB_DRIVER_VFIO);
-struct virHostdevIsPCINodeDeviceUsedData data = { mgr, dom_name, 
usesVFIO };
+struct virHostdevIsPCINodeDeviceUsedData data = {mgr, drv_name, 
dom_name, usesVFIO};
 int hdrType = -1;
 
 if (virPCIGetHeaderType(pci, ) < 0)
@@ -1995,7 +1996,7 @@ int
 virHostdevPCINodeDeviceDetach(virHostdevManagerPtr mgr,
   virPCIDevicePtr pci)
 {
-struct virHostdevIsPCINodeDeviceUsedData data = { mgr, NULL, false };
+struct virHostdevIsPCINodeDeviceUsedData data = {mgr, NULL, NULL, false};
 int ret = -1;
 
 virObjectLock(mgr->activePCIHostdevs);
@@ -2021,7 +2022,7 @@ int
 virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr,
 virPCIDevicePtr pci)
 {
-struct virHostdevIsPCINodeDeviceUsedData data = { mgr, NULL, false };
+struct virHostdevIsPCINodeDeviceUsedData data = {mgr, NULL, NULL, false};
 int ret = -1;
 
 virObjectLock(mgr->activePCIHostdevs);
-- 
2.21.0

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

[libvirt] [PATCH 05/18] virpcimock: Use VIR_AUTOFREE()

[Michal Privoznik]

It saves us couple of lines.

Signed-off-by: Michal Privoznik 
---
 tests/virpcimock.c | 109 +++--
 1 file changed, 36 insertions(+), 73 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 853ac588e9..0950f3ba00 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -172,7 +172,7 @@ make_file(const char *path,
   ssize_t len)
 {
 int fd = -1;
-char *filepath = NULL;
+VIR_AUTOFREE(char *) filepath = NULL;
 if (value && len == -1)
 len = strlen(value);
 
@@ -186,7 +186,6 @@ make_file(const char *path,
 ABORT("Unable to write: %s", filepath);
 
 VIR_FORCE_CLOSE(fd);
-VIR_FREE(filepath);
 }
 
 static void
@@ -194,15 +193,13 @@ make_symlink(const char *path,
   const char *name,
   const char *target)
 {
-char *filepath = NULL;
+VIR_AUTOFREE(char *) filepath = NULL;
 
 if (virAsprintfQuiet(, "%s/%s", path, name) < 0)
 ABORT_OOM();
 
 if (symlink(target, filepath) < 0)
 ABORT("Unable to create symlink filepath -> target");
-
-VIR_FREE(filepath);
 }
 
 static int
@@ -213,7 +210,7 @@ pci_read_file(const char *path,
 {
 int ret = -1;
 int fd = -1;
-char *newpath;
+VIR_AUTOFREE(char *) newpath = NULL;
 
 if (virAsprintfQuiet(, "%s/%s",
  fakesysfspcidir,
@@ -237,7 +234,6 @@ pci_read_file(const char *path,
 
 ret = 0;
  cleanup:
-VIR_FREE(newpath);
 real_close(fd);
 return ret;
 }
@@ -335,10 +331,10 @@ static void
 pci_device_new_from_stub(const struct pciDevice *data)
 {
 struct pciDevice *dev;
-char *devpath;
-char *id;
+VIR_AUTOFREE(char *) devpath = NULL;
+VIR_AUTOFREE(char *) id = NULL;
 char *c;
-char *configSrc;
+VIR_AUTOFREE(char *) configSrc = NULL;
 char tmp[256];
 struct stat sb;
 bool configSrcExists = false;
@@ -363,7 +359,6 @@ pci_device_new_from_stub(const struct pciDevice *data)
 virAsprintfQuiet(, "%s/devices/%s", fakesysfspcidir, data->id) 
< 0)
 ABORT_OOM();
 
-VIR_FREE(id);
 memcpy(dev, data, sizeof(*dev));
 
 if (virFileMakePath(devpath) < 0)
@@ -380,14 +375,13 @@ pci_device_new_from_stub(const struct pciDevice *data)
  * file, and parallel VPATH builds must not stomp on the
  * original; besides, 'make distcheck' requires the original
  * to be read-only */
-char *buf;
+VIR_AUTOFREE(char *) buf = NULL;
 ssize_t len;
 
 if ((len = virFileReadAll(configSrc, 4096, )) < 0)
 ABORT("Unable to read config file '%s'", configSrc);
 
 make_file(devpath, "config", buf, len);
-VIR_FREE(buf);
 } else {
 /* If there's no config data in the virpcitestdata dir, create a dummy
  * config file */
@@ -427,9 +421,6 @@ pci_device_new_from_stub(const struct pciDevice *data)
 
 if (VIR_APPEND_ELEMENT_QUIET(pciDevices, nPCIDevices, dev) < 0)
 ABORT_OOM();
-
-VIR_FREE(devpath);
-VIR_FREE(configSrc);
 }
 
 static struct pciDevice *
@@ -483,7 +474,7 @@ pci_driver_new(const char *name, int fail, ...)
 struct pciDriver *driver;
 va_list args;
 int vendor, device;
-char *driverpath;
+VIR_AUTOFREE(char *) driverpath = NULL;
 
 if (VIR_ALLOC_QUIET(driver) < 0 ||
 VIR_STRDUP_QUIET(driver->name, name) < 0 ||
@@ -519,8 +510,6 @@ pci_driver_new(const char *name, int fail, ...)
 
 if (VIR_APPEND_ELEMENT_QUIET(pciDrivers, nPCIDrivers, driver) < 0)
 ABORT_OOM();
-
-VIR_FREE(driverpath);
 }
 
 static struct pciDriver *
@@ -586,13 +575,13 @@ static int
 pci_driver_bind(struct pciDriver *driver,
 struct pciDevice *dev)
 {
-int ret = -1;
-char *devpath = NULL, *driverpath = NULL;
+VIR_AUTOFREE(char *) devpath = NULL;
+VIR_AUTOFREE(char *) driverpath = NULL;
 
 if (dev->driver) {
 /* Device already bound */
 errno = ENODEV;
-return ret;
+return -1;
 }
 
 /* Make symlink under device tree */
@@ -601,11 +590,11 @@ pci_driver_bind(struct pciDriver *driver,
 virAsprintfQuiet(, "%s/drivers/%s",
  fakesysfspcidir, driver->name) < 0) {
 errno = ENOMEM;
-goto cleanup;
+return -1;
 }
 
 if (symlink(driverpath, devpath) < 0)
-goto cleanup;
+return -1;
 
 /* Make symlink under driver tree */
 VIR_FREE(devpath);
@@ -615,31 +604,27 @@ pci_driver_bind(struct pciDriver *driver,
 virAsprintfQuiet(, "%s/drivers/%s/%s",
  fakesysfspcidir, driver->name, dev->id) < 0) {
 errno = ENOMEM;
-goto cleanup;
+return -1;
 }
 
 if (symlink(devpath, driverpath) < 0)
-goto cleanup;
+return -1;
 
 dev->driver = driver;
-ret = 0;
- cleanup:
-VIR_FREE(devpath);
-VIR_FREE(driverpath);
-return ret;
+return 0;
 }
 
 static int
 

[libvirt] [PATCH 17/18] virhostdevtest: Use modern VFIO

[Michal Privoznik]

The pci-stub is so old school that no one uses it. All modern
systems have adapted VFIO. Switch our virhostdevtest too.

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

diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c
index 20984f3442..f860426678 100644
--- a/tests/virhostdevtest.c
+++ b/tests/virhostdevtest.c
@@ -96,7 +96,7 @@ myInit(void)
 subsys.u.pci.addr.bus = 0;
 subsys.u.pci.addr.slot = i + 1;
 subsys.u.pci.addr.function = 0;
-subsys.u.pci.backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM;
+subsys.u.pci.backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;
 hostdevs[i]->source.subsys = subsys;
 }
 
@@ -104,7 +104,7 @@ myInit(void)
 if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0)))
 goto cleanup;
 
-virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_KVM);
+virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO);
 }
 
 if (VIR_ALLOC(mgr) < 0)
-- 
2.21.0

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

[libvirt] [PATCH 11/18] virpcimock: Store PCI address as ints not string

[Michal Privoznik]

In upcoming patches we will need only some portions of the PCI
address. To construct that easily, it's better if the PCI address
of a device is stored as four integers rather than one string.

Signed-off-by: Michal Privoznik 
---
 tests/virpcimock.c | 76 +-
 1 file changed, 61 insertions(+), 15 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index de365cdb78..c10764dcdd 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -118,8 +118,16 @@ struct pciDriver {
 unsigned int fail;  /* Bitwise-OR of driverActions that should fail */
 };
 
+struct pciDeviceAddress {
+unsigned int domain;
+unsigned int bus;
+unsigned int device;
+unsigned int function;
+};
+# define ADDR_STR_FMT "%04x:%02x:%02x.%d"
+
 struct pciDevice {
-const char *id;
+struct pciDeviceAddress addr;
 int vendor;
 int device;
 int klass;
@@ -145,7 +153,7 @@ static void init_env(void);
 
 static int pci_device_autobind(struct pciDevice *dev);
 static void pci_device_new_from_stub(const struct pciDevice *data);
-static struct pciDevice *pci_device_find_by_id(const char *id);
+static struct pciDevice *pci_device_find_by_id(struct pciDeviceAddress const 
*addr);
 static struct pciDevice *pci_device_find_by_content(const char *path);
 
 static void pci_driver_new(const char *name, int fail, ...);
@@ -337,6 +345,29 @@ remove_fd(int fd)
 /*
  * PCI Device functions
  */
+static char *
+pci_address_format(struct pciDeviceAddress const *addr)
+{
+char *ret;
+
+ignore_value(virAsprintfQuiet(, ADDR_STR_FMT,
+  addr->domain, addr->bus,
+  addr->device, addr->function));
+return ret;
+}
+
+static int
+pci_address_parse(struct pciDeviceAddress *addr,
+  const char *buf)
+{
+if (sscanf(buf, ADDR_STR_FMT,
+   >domain, >bus,
+   >device, >function) != 4)
+return -1;
+return 0;
+}
+
+
 static char *
 pci_device_get_path(const struct pciDevice *dev,
 const char *file,
@@ -344,16 +375,20 @@ pci_device_get_path(const struct pciDevice *dev,
 {
 char *ret = NULL;
 const char *prefix = "";
+VIR_AUTOFREE(char *) devid = NULL;
 
 if (faked)
 prefix = fakerootdir;
 
+if (!(devid = pci_address_format(>addr)))
+return NULL;
+
 if (file) {
 ignore_value(virAsprintfQuiet(, "%s" SYSFS_PCI_PREFIX 
"devices/%s/%s",
-  prefix, dev->id, file));
+  prefix, devid, file));
 } else {
 ignore_value(virAsprintfQuiet(, "%s" SYSFS_PCI_PREFIX "devices/%s",
-  prefix, dev->id));
+  prefix, devid));
 }
 
 return ret;
@@ -366,13 +401,15 @@ pci_device_new_from_stub(const struct pciDevice *data)
 struct pciDevice *dev;
 VIR_AUTOFREE(char *) devpath = NULL;
 VIR_AUTOFREE(char *) id = NULL;
+VIR_AUTOFREE(char *) devid = NULL;
 char *c;
 VIR_AUTOFREE(char *) configSrc = NULL;
 char tmp[256];
 struct stat sb;
 bool configSrcExists = false;
 
-if (VIR_STRDUP_QUIET(id, data->id) < 0)
+if (!(devid = pci_address_format(>addr)) ||
+VIR_STRDUP_QUIET(id, devid) < 0)
 ABORT_OOM();
 
 /* Replace ':' with '-' to create the config filename from the
@@ -452,20 +489,20 @@ pci_device_new_from_stub(const struct pciDevice *data)
 make_symlink(devpath, "iommu_group", tmp);
 
 if (pci_device_autobind(dev) < 0)
-ABORT("Unable to bind: %s", data->id);
+ABORT("Unable to bind: %s", devid);
 
 if (VIR_APPEND_ELEMENT_QUIET(pciDevices, nPCIDevices, dev) < 0)
 ABORT_OOM();
 }
 
 static struct pciDevice *
-pci_device_find_by_id(const char *id)
+pci_device_find_by_id(struct pciDeviceAddress const *addr)
 {
 size_t i;
 for (i = 0; i < nPCIDevices; i++) {
 struct pciDevice *dev = pciDevices[i];
 
-if (STREQ(dev->id, id))
+if (!memcmp(>addr, addr, sizeof(*addr)))
 return dev;
 }
 
@@ -476,11 +513,13 @@ static struct pciDevice *
 pci_device_find_by_content(const char *path)
 {
 char tmp[32];
+struct pciDeviceAddress addr;
 
-if (pci_read_file(path, tmp, sizeof(tmp), true) < 0)
+if (pci_read_file(path, tmp, sizeof(tmp), true) < 0 ||
+pci_address_parse(, tmp) < 0)
 return NULL;
 
-return pci_device_find_by_id(tmp);
+return pci_device_find_by_id();
 }
 
 static int
@@ -607,6 +646,7 @@ pci_driver_find_by_path(const char *path)
 static struct pciDriver *
 pci_driver_find_by_driver_override(struct pciDevice *dev)
 {
+VIR_AUTOFREE(char *) devid = NULL;
 VIR_AUTOFREE(char *) path = NULL;
 char tmp[32];
 size_t i;
@@ -631,6 +671,7 @@ static int
 pci_driver_bind(struct pciDriver *driver,
 struct pciDevice *dev)
 {
+VIR_AUTOFREE(char *) devid = NULL;
 

[libvirt] [PATCH 10/18] virpcimock: Introduce and use pci_driver_get_path()

[Michal Privoznik]

Have just one function to generate path to a PCI driver so that
when we change it in near future there's only few of the places
we need to fix.

Signed-off-by: Michal Privoznik 
---
 tests/virpcimock.c | 34 +++---
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 52a585d0cc..de365cdb78 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -503,6 +503,29 @@ pci_device_autobind(struct pciDevice *dev)
 /*
  * PCI Driver functions
  */
+static char *
+pci_driver_get_path(const struct pciDriver *driver,
+const char *file,
+bool faked)
+{
+char *ret = NULL;
+const char *prefix = "";
+
+if (faked)
+prefix = fakerootdir;
+
+if (file) {
+ignore_value(virAsprintfQuiet(, "%s" SYSFS_PCI_PREFIX 
"drivers/%s/%s",
+  prefix, driver->name, file));
+} else {
+ignore_value(virAsprintfQuiet(, "%s" SYSFS_PCI_PREFIX "drivers/%s",
+  prefix, driver->name));
+}
+
+return ret;
+}
+
+
 static void
 pci_driver_new(const char *name, int fail, ...)
 {
@@ -513,7 +536,7 @@ pci_driver_new(const char *name, int fail, ...)
 
 if (VIR_ALLOC_QUIET(driver) < 0 ||
 VIR_STRDUP_QUIET(driver->name, name) < 0 ||
-virAsprintfQuiet(, "%s/sys/bus/pci/drivers/%s", 
fakerootdir, name) < 0)
+!(driverpath = pci_driver_get_path(driver, NULL, true)))
 ABORT_OOM();
 
 driver->fail = fail;
@@ -619,8 +642,7 @@ pci_driver_bind(struct pciDriver *driver,
 
 /* Make symlink under device tree */
 if (!(devpath = pci_device_get_path(dev, "driver", true)) ||
-virAsprintfQuiet(, "%s/sys/bus/pci/drivers/%s",
- fakerootdir, driver->name) < 0) {
+!(driverpath = pci_driver_get_path(driver, NULL, true))) {
 errno = ENOMEM;
 return -1;
 }
@@ -632,8 +654,7 @@ pci_driver_bind(struct pciDriver *driver,
 VIR_FREE(devpath);
 VIR_FREE(driverpath);
 if (!(devpath = pci_device_get_path(dev, NULL, true)) ||
-virAsprintfQuiet(, "%s/sys/bus/pci/drivers/%s/%s",
- fakerootdir, driver->name, dev->id) < 0) {
+!(driverpath = pci_driver_get_path(driver, dev->id, true))) {
 errno = ENOMEM;
 return -1;
 }
@@ -660,8 +681,7 @@ pci_driver_unbind(struct pciDriver *driver,
 
 /* Make symlink under device tree */
 if (!(devpath = pci_device_get_path(dev, "driver", true)) ||
-virAsprintfQuiet(, "%s/sys/bus/pci/drivers/%s/%s",
- fakerootdir, driver->name, dev->id) < 0) {
+!(driverpath = pci_driver_get_path(driver, dev->id, true))) {
 errno = ENOMEM;
 return -1;
 }
-- 
2.21.0

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

[libvirt] [PATCH 09/18] virpcimock: Introduce and use pci_device_get_path()

[Michal Privoznik]

Have just one function to generate path to a PCI device so that
when we change it in near future there's only few of the places
we need to fix.

Signed-off-by: Michal Privoznik 
---
 tests/virpcimock.c | 42 +++---
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 213f906500..52a585d0cc 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -337,6 +337,29 @@ remove_fd(int fd)
 /*
  * PCI Device functions
  */
+static char *
+pci_device_get_path(const struct pciDevice *dev,
+const char *file,
+bool faked)
+{
+char *ret = NULL;
+const char *prefix = "";
+
+if (faked)
+prefix = fakerootdir;
+
+if (file) {
+ignore_value(virAsprintfQuiet(, "%s" SYSFS_PCI_PREFIX 
"devices/%s/%s",
+  prefix, dev->id, file));
+} else {
+ignore_value(virAsprintfQuiet(, "%s" SYSFS_PCI_PREFIX "devices/%s",
+  prefix, dev->id));
+}
+
+return ret;
+}
+
+
 static void
 pci_device_new_from_stub(const struct pciDevice *data)
 {
@@ -365,12 +388,14 @@ pci_device_new_from_stub(const struct pciDevice *data)
 
 if (VIR_ALLOC_QUIET(dev) < 0 ||
 virAsprintfQuiet(, "%s/virpcitestdata/%s.config",
- abs_srcdir, id) < 0 ||
-virAsprintfQuiet(, "%s/sys/bus/pci/devices/%s", fakerootdir, 
data->id) < 0)
+ abs_srcdir, id) < 0)
 ABORT_OOM();
 
 memcpy(dev, data, sizeof(*dev));
 
+if (!(devpath = pci_device_get_path(dev, NULL, true)))
+ABORT_OOM();
+
 if (virFileMakePath(devpath) < 0)
 ABORT("Unable to create: %s", devpath);
 
@@ -563,9 +588,7 @@ pci_driver_find_by_driver_override(struct pciDevice *dev)
 char tmp[32];
 size_t i;
 
-if (virAsprintfQuiet(,
- SYSFS_PCI_PREFIX "devices/%s/driver_override",
- dev->id) < 0)
+if (!(path = pci_device_get_path(dev, "driver_override", false)))
 return NULL;
 
 if (pci_read_file(path, tmp, sizeof(tmp), false) < 0)
@@ -595,8 +618,7 @@ pci_driver_bind(struct pciDriver *driver,
 }
 
 /* Make symlink under device tree */
-if (virAsprintfQuiet(, "%s/sys/bus/pci/devices/%s/driver",
- fakerootdir, dev->id) < 0 ||
+if (!(devpath = pci_device_get_path(dev, "driver", true)) ||
 virAsprintfQuiet(, "%s/sys/bus/pci/drivers/%s",
  fakerootdir, driver->name) < 0) {
 errno = ENOMEM;
@@ -609,8 +631,7 @@ pci_driver_bind(struct pciDriver *driver,
 /* Make symlink under driver tree */
 VIR_FREE(devpath);
 VIR_FREE(driverpath);
-if (virAsprintfQuiet(, "%s/sys/bus/pci/devices/%s",
- fakerootdir, dev->id) < 0 ||
+if (!(devpath = pci_device_get_path(dev, NULL, true)) ||
 virAsprintfQuiet(, "%s/sys/bus/pci/drivers/%s/%s",
  fakerootdir, driver->name, dev->id) < 0) {
 errno = ENOMEM;
@@ -638,8 +659,7 @@ pci_driver_unbind(struct pciDriver *driver,
 }
 
 /* Make symlink under device tree */
-if (virAsprintfQuiet(, "%s/sys/bus/pci/devices/%s/driver",
- fakerootdir, dev->id) < 0 ||
+if (!(devpath = pci_device_get_path(dev, "driver", true)) ||
 virAsprintfQuiet(, "%s/sys/bus/pci/drivers/%s/%s",
  fakerootdir, driver->name, dev->id) < 0) {
 errno = ENOMEM;
-- 
2.21.0

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

[libvirt] [PATCH 08/18] virpcimock: Create devices/ and drivers/ under /sys/bus/pci upfront

[Michal Privoznik]

In near future, we will be creating devices under different
location and just symlink them under devices/. Just like real
kernel does. But for that we need the directories to exists.

Signed-off-by: Michal Privoznik 
---
 tests/virpcimock.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index e97dbd81f8..213f906500 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -187,6 +187,19 @@ make_file(const char *path,
 VIR_FORCE_CLOSE(fd);
 }
 
+static void
+make_dir(const char *path,
+ const char *name)
+{
+VIR_AUTOFREE(char *) dirpath = NULL;
+
+if (virAsprintfQuiet(, "%s/%s", path, name) < 0)
+ABORT_OOM();
+
+if (virFileMakePath(dirpath) < 0)
+ABORT("Unable to create: %s", dirpath);
+}
+
 static void
 make_symlink(const char *path,
   const char *name,
@@ -846,6 +859,8 @@ init_env(void)
 if (virFileMakePath(tmp) < 0)
 ABORT("Unable to create: %s", tmp);
 
+make_dir(tmp, "devices");
+make_dir(tmp, "drivers");
 make_file(tmp, "drivers_probe", NULL, -1);
 
 # define MAKE_PCI_DRIVER(name, ...) \
-- 
2.21.0

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

[libvirt] [PATCH 12/18] virpcimock: Create PCI devices under /sys/devices/pci*

[Michal Privoznik]

So far, we are creating devices directly under
/sys/bus/pci/devices/*. There is not much problem with it, but if
we really want to model kernel behaviour we need to create them
under /sys/devices/pci:BB and then only symlink them from the
old location.

Signed-off-by: Michal Privoznik 
---
 tests/virpcimock.c | 25 +
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index c10764dcdd..0774bf62d9 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -383,12 +383,17 @@ pci_device_get_path(const struct pciDevice *dev,
 if (!(devid = pci_address_format(>addr)))
 return NULL;
 
+/* PCI devices really do live under /sys/devices/pci:BB
+ * and then they are just symlinked to /sys/bus/pci/devices/
+ */
 if (file) {
-ignore_value(virAsprintfQuiet(, "%s" SYSFS_PCI_PREFIX 
"devices/%s/%s",
-  prefix, devid, file));
+ignore_value(virAsprintfQuiet(, 
"%s/sys/devices/pci%04x:%02x/%s/%s",
+  prefix, dev->addr.domain, dev->addr.bus,
+  devid, file));
 } else {
-ignore_value(virAsprintfQuiet(, "%s" SYSFS_PCI_PREFIX "devices/%s",
-  prefix, devid));
+ignore_value(virAsprintfQuiet(, "%s/sys/devices/pci%04x:%02x/%s",
+  prefix, dev->addr.domain, dev->addr.bus,
+  devid));
 }
 
 return ret;
@@ -400,6 +405,7 @@ pci_device_new_from_stub(const struct pciDevice *data)
 {
 struct pciDevice *dev;
 VIR_AUTOFREE(char *) devpath = NULL;
+VIR_AUTOFREE(char *) devsympath = NULL;
 VIR_AUTOFREE(char *) id = NULL;
 VIR_AUTOFREE(char *) devid = NULL;
 char *c;
@@ -488,6 +494,17 @@ pci_device_new_from_stub(const struct pciDevice *data)
 }
 make_symlink(devpath, "iommu_group", tmp);
 
+if (snprintf(tmp, sizeof(tmp),
+ "../../../devices/pci%04x:%02x/%s",
+ dev->addr.domain, dev->addr.bus, devid) < 0) {
+ABORT("@tmp overflow");
+}
+
+if (virAsprintfQuiet(, "%s" SYSFS_PCI_PREFIX "devices", 
fakerootdir) < 0)
+ABORT_OOM();
+
+make_symlink(devsympath, devid, tmp);
+
 if (pci_device_autobind(dev) < 0)
 ABORT("Unable to bind: %s", devid);
 
-- 
2.21.0

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

[libvirt] [PATCH 04/18] virpcimock: Drop needless typecast

[Michal Privoznik]

When creating a PCI device, the pciDevice structure contains @id
member which holds device address (.BB:DD.F) and is type of
'char *'. But the structure is initialized from a const char and
in fact we never modify or free the @id.

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

diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 1c21e4e045..853ac588e9 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -120,7 +120,7 @@ struct pciDriver {
 };
 
 struct pciDevice {
-char *id;
+const char *id;
 int vendor;
 int device;
 int klass;
@@ -878,7 +878,7 @@ init_env(void)
 
 # define MAKE_PCI_DEVICE(Id, Vendor, Device, ...) \
 do { \
-struct pciDevice dev = {.id = (char *)Id, .vendor = Vendor, \
+struct pciDevice dev = {.id = Id, .vendor = Vendor, \
 .device = Device, __VA_ARGS__}; \
 pci_device_new_from_stub(); \
 } while (0)
-- 
2.21.0

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

[libvirt] [PATCH 15/18] virhostdev: Unify virHostdevPreparePCIDevices behaviour for KVM and VFIO cases

[Michal Privoznik]

The virHostdevPreparePCIDevices() function works in several
steps. In the very first one, it checks if devices we want to
detach from the host are not taken already by some other domain.
However, this piece of code returns different results depending
on the stub driver used (which is not wrong per se, but keep on
reading). If the stub driver is KVM then
virHostdevIsPCINodeDeviceUsed() is called which basically checks
if a PCI device from the detach list is not used by any domain
(including the one we are preparing the device for). If that is
the case, an error is reported ("device in use") and -1 is
returned.

However, that is not what happens if the stub driver is VFIO. If
the stub driver is VFIO, then we iterate over all PCI devices
from the same IOMMU group and check if they are taken by some
other domain (because a PCI device, well IOMMU group, can't be
shared between two or more qemu processes). But we fail to check,
if the device we are trying to detach from the host is not
already taken by a domain. That is, calling
virHostdevPreparePCIDevices() over a hostdev device twice
succeeds the first time and fails too late in the second run
(fortunately, virHostdevResetAllPCIDevices() will throw an error,
but this is already too late because the PCI device in question
was moved to the list of inactive PCI devices and now it appears
in both lists).

Signed-off-by: Michal Privoznik 
---
 src/util/virhostdev.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index cb69582c21..6861b8a84e 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -53,7 +53,7 @@ struct virHostdevIsPCINodeDeviceUsedData {
 virHostdevManagerPtr mgr;
 const char *driverName;
 const char *domainName;
-const bool usesVFIO;
+bool usesVFIO;
 };
 
 /* This module makes heavy use of bookkeeping lists contained inside a
@@ -707,7 +707,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
 virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
 bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK);
 bool usesVFIO = (virPCIDeviceGetStubDriver(pci) == 
VIR_PCI_STUB_DRIVER_VFIO);
-struct virHostdevIsPCINodeDeviceUsedData data = {mgr, drv_name, 
dom_name, usesVFIO};
+struct virHostdevIsPCINodeDeviceUsedData data = {mgr, drv_name, 
dom_name, false};
 int hdrType = -1;
 
 if (virPCIGetHeaderType(pci, ) < 0)
@@ -728,18 +728,19 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
 }
 
 /* The device is in use by other active domain if
- * the dev is in list activePCIHostdevs. VFIO devices
- * belonging to same iommu group can't be shared
- * across guests.
- */
+ * the dev is in list activePCIHostdevs. */
 devAddr = virPCIDeviceGetAddress(pci);
+if (virHostdevIsPCINodeDeviceUsed(devAddr, ))
+goto cleanup;
+
+/* VFIO devices belonging to same IOMMU group can't be
+ * shared across guests. Check if that's the case. */
 if (usesVFIO) {
+data.usesVFIO = true;
 if (virPCIDeviceAddressIOMMUGroupIterate(devAddr,
  
virHostdevIsPCINodeDeviceUsed,
  ) < 0)
 goto cleanup;
-} else if (virHostdevIsPCINodeDeviceUsed(devAddr, )) {
-goto cleanup;
 }
 }
 
-- 
2.21.0

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

[libvirt] [PATCH 06/18] virpcimock: Eliminate use of @fakesysfspcidir

[Michal Privoznik]

The @fakesysfspcidir is derived from @fakerootdir. We don't need
two global variables that contain nearly the same content,
especially when we construct the actual path anyways.

Signed-off-by: Michal Privoznik 
---
 tests/virpcimock.c | 41 -
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 0950f3ba00..296da9b453 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -41,7 +41,6 @@ static char *(*real_virFileCanonicalizePath)(const char 
*path);
  * vircgroupmock.c:462:22: error: static variable 'fakesysfsdir' is used in an 
inline function with external linkage [-Werror,-Wstatic-in-inline]
  */
 char *fakerootdir;
-char *fakesysfspcidir;
 
 # define SYSFS_PCI_PREFIX "/sys/bus/pci/"
 
@@ -212,9 +211,7 @@ pci_read_file(const char *path,
 int fd = -1;
 VIR_AUTOFREE(char *) newpath = NULL;
 
-if (virAsprintfQuiet(, "%s/%s",
- fakesysfspcidir,
- path + strlen(SYSFS_PCI_PREFIX)) < 0) {
+if (virAsprintfQuiet(, "%s/%s", fakerootdir, path) < 0) {
 errno = ENOMEM;
 goto cleanup;
 }
@@ -245,8 +242,8 @@ getrealpath(char **newpath,
 init_env();
 
 if (STRPREFIX(path, SYSFS_PCI_PREFIX)) {
-if (virAsprintfQuiet(newpath, "%s/%s",
- fakesysfspcidir,
+if (virAsprintfQuiet(newpath, "%s/sys/bus/pci/%s",
+ fakerootdir,
  path + strlen(SYSFS_PCI_PREFIX)) < 0) {
 errno = ENOMEM;
 return -1;
@@ -356,7 +353,7 @@ pci_device_new_from_stub(const struct pciDevice *data)
 if (VIR_ALLOC_QUIET(dev) < 0 ||
 virAsprintfQuiet(, "%s/virpcitestdata/%s.config",
  abs_srcdir, id) < 0 ||
-virAsprintfQuiet(, "%s/devices/%s", fakesysfspcidir, data->id) 
< 0)
+virAsprintfQuiet(, "%s/sys/bus/pci/devices/%s", fakerootdir, 
data->id) < 0)
 ABORT_OOM();
 
 memcpy(dev, data, sizeof(*dev));
@@ -478,7 +475,7 @@ pci_driver_new(const char *name, int fail, ...)
 
 if (VIR_ALLOC_QUIET(driver) < 0 ||
 VIR_STRDUP_QUIET(driver->name, name) < 0 ||
-virAsprintfQuiet(, "%s/drivers/%s", fakesysfspcidir, name) 
< 0)
+virAsprintfQuiet(, "%s/sys/bus/pci/drivers/%s", 
fakerootdir, name) < 0)
 ABORT_OOM();
 
 driver->fail = fail;
@@ -585,10 +582,10 @@ pci_driver_bind(struct pciDriver *driver,
 }
 
 /* Make symlink under device tree */
-if (virAsprintfQuiet(, "%s/devices/%s/driver",
- fakesysfspcidir, dev->id) < 0 ||
-virAsprintfQuiet(, "%s/drivers/%s",
- fakesysfspcidir, driver->name) < 0) {
+if (virAsprintfQuiet(, "%s/sys/bus/pci/devices/%s/driver",
+ fakerootdir, dev->id) < 0 ||
+virAsprintfQuiet(, "%s/sys/bus/pci/drivers/%s",
+ fakerootdir, driver->name) < 0) {
 errno = ENOMEM;
 return -1;
 }
@@ -599,10 +596,10 @@ pci_driver_bind(struct pciDriver *driver,
 /* Make symlink under driver tree */
 VIR_FREE(devpath);
 VIR_FREE(driverpath);
-if (virAsprintfQuiet(, "%s/devices/%s",
- fakesysfspcidir, dev->id) < 0 ||
-virAsprintfQuiet(, "%s/drivers/%s/%s",
- fakesysfspcidir, driver->name, dev->id) < 0) {
+if (virAsprintfQuiet(, "%s/sys/bus/pci/devices/%s",
+ fakerootdir, dev->id) < 0 ||
+virAsprintfQuiet(, "%s/sys/bus/pci/drivers/%s/%s",
+ fakerootdir, driver->name, dev->id) < 0) {
 errno = ENOMEM;
 return -1;
 }
@@ -628,10 +625,10 @@ pci_driver_unbind(struct pciDriver *driver,
 }
 
 /* Make symlink under device tree */
-if (virAsprintfQuiet(, "%s/devices/%s/driver",
- fakesysfspcidir, dev->id) < 0 ||
-virAsprintfQuiet(, "%s/drivers/%s/%s",
- fakesysfspcidir, driver->name, dev->id) < 0) {
+if (virAsprintfQuiet(, "%s/sys/bus/pci/devices/%s/driver",
+ fakerootdir, dev->id) < 0 ||
+virAsprintfQuiet(, "%s/sys/bus/pci/drivers/%s/%s",
+ fakerootdir, driver->name, dev->id) < 0) {
 errno = ENOMEM;
 return -1;
 }
@@ -834,7 +831,9 @@ init_syms(void)
 static void
 init_env(void)
 {
-if (fakerootdir && fakesysfspcidir)
+VIR_AUTOFREE(char *) fakesysfspcidir = NULL;
+
+if (fakerootdir)
 return;
 
 if (!(fakerootdir = getenv("LIBVIRT_FAKE_ROOT_DIR")))
-- 
2.21.0

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

[libvirt] [PATCH 00/18] Enhance virpcimock and test VFIO instead of KVM

[Michal Privoznik]

Kernel structure looks slightly different than what virpcimock creates.
This did not use to be a problem, because we are testing KVM device
assignment even though majority of systems we run on (if not all of
them) use VFIO assignment.

In order to switch our test suite (mainly virhostdevtest and virpcitest)
to test VFIO assignment, the virpcimock must be fixed. Firstly, it needs
to create symlinks under /sys/kernel/iommu_groups/... directories (patch
13/18) so that virhostdev module can iterate over them. Secondly, it
needs to create 'driver_override' file (which exists since
kernel-3.16.0) so that the virtual environment the mock creates matches
real up to date systems (patch 03/18).

Funny thing is, that enhancing the mock uncovered a bug we had (fix is
in 15/18) and also one latent bug (14/18).


As usual, these patches can be found on my github too:

  https://github.com/zippy2/libvirt/tree/virpcimock

and just for the fun of it, here's the latest travis build of that
branch:

  https://travis-ci.org/zippy2/libvirt/builds/571752953


Michal Prívozník (18):
  virpcimock: Move actions checking one level up
  Revert "virpcitest: Test virPCIDeviceDetach failure"
  virpcimock: Create driver_override file in device dirs
  virpcimock: Drop needless typecast
  virpcimock: Use VIR_AUTOFREE()
  virpcimock: Eliminate use of @fakesysfspcidir
  virpcimock: Rename @fakesysfspcidir
  virpcimock: Create devices/ and drivers/ under /sys/bus/pci upfront
  virpcimock: Introduce and use pci_device_get_path()
  virpcimock: Introduce and use pci_driver_get_path()
  virpcimock: Store PCI address as ints not string
  virpcimock: Create PCI devices under /sys/devices/pci*
  virpcimock: Create symlink in /sys/kernel/iommu_groups/N/devices dir
  virhostdev: Check driver name too in virHostdevIsPCINodeDeviceUsed()
  virhostdev: Unify virHostdevPreparePCIDevices behaviour for KVM and
VFIO cases
  qemuxml2argvtest: Switch to modern vfio backend
  virhostdevtest: Use modern VFIO
  virpcitest: Use modern VFIO

 src/util/virhostdev.c |  26 +-
 .../hostdev-pci-address-device.args   |   2 +-
 .../qemuxml2argvdata/hostdev-pci-address.args |   2 +-
 .../net-hostdev-bootorder.args|   3 +-
 .../net-hostdev-multidomain.args  |   2 +-
 tests/qemuxml2argvdata/net-hostdev.args   |   2 +-
 tests/qemuxml2argvdata/pci-rom.args   |   4 +-
 tests/qemuxml2argvtest.c  |  14 +-
 tests/virhostdevtest.c|   4 +-
 tests/virpcimock.c| 394 --
 tests/virpcitest.c|  48 +--
 11 files changed, 304 insertions(+), 197 deletions(-)

-- 
2.21.0

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

[libvirt] [PATCH 03/18] virpcimock: Create driver_override file in device dirs

[Michal Privoznik]

Newer kernels (v3.16-rc1~29^2~6^4) have 'driver_override' file
which simplifies way of binding a PCI device to desired driver.
Libvirt has support for this for some time too (v2.3.0-rc1~236),
but not our virpcimock. So far we did not care because our code
is designed to deal with this situation. Except for one.
hypothetical case: binding a device to the vfio-pci driver can be
successful only via driver_override. Any attempt to bind a PCI
device to vfio-pci driver using old method (new_id + unbind +
bind) will fail because of b803b29c1a5. While on vanilla kernel
I'm able to use the old method successfully, it's failing on RHEL
kernels (not sure why).

Signed-off-by: Michal Privoznik 
---
 tests/virpcimock.c | 52 --
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 6865f992dc..1c21e4e045 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -87,6 +87,11 @@ char *fakesysfspcidir;
  *   Probe for a driver that handles the specified device.
  *   Data in format ":BB:DD.F" (Domain:Bus:Device.Function).
  *
+ * /sys/bus/pci/devices//driver_override
+ *   Name of a driver that overrides preferred driver can be written
+ *   here. The device will be attached to it on drivers_probe event.
+ *   Writing an empty string (or "\n") clears the override.
+ *
  * As a little hack, we are not mocking write to these files, but close()
  * instead. The advantage is we don't need any self growing array to hold the
  * partial writes and construct them back. We can let all the writes finish,
@@ -147,6 +152,7 @@ static struct pciDevice *pci_device_find_by_content(const 
char *path);
 static void pci_driver_new(const char *name, int fail, ...);
 static struct pciDriver *pci_driver_find_by_dev(struct pciDevice *dev);
 static struct pciDriver *pci_driver_find_by_path(const char *path);
+static struct pciDriver *pci_driver_find_by_driver_override(struct pciDevice 
*dev);
 static int pci_driver_bind(struct pciDriver *driver, struct pciDevice *dev);
 static int pci_driver_unbind(struct pciDriver *driver, struct pciDevice *dev);
 static int pci_driver_handle_change(int fd, const char *path);
@@ -202,7 +208,8 @@ make_symlink(const char *path,
 static int
 pci_read_file(const char *path,
   char *buf,
-  size_t buf_size)
+  size_t buf_size,
+  bool truncate)
 {
 int ret = -1;
 int fd = -1;
@@ -224,7 +231,8 @@ pci_read_file(const char *path,
 goto cleanup;
 }
 
-if (ftruncate(fd, 0) < 0)
+if (truncate &&
+ftruncate(fd, 0) < 0)
 goto cleanup;
 
 ret = 0;
@@ -398,6 +406,8 @@ pci_device_new_from_stub(const struct pciDevice *data)
 ABORT("@tmp overflow");
 make_file(devpath, "class", tmp, -1);
 
+make_file(devpath, "driver_override", NULL, -1);
+
 if (snprintf(tmp, sizeof(tmp),
  "%s/../../../kernel/iommu_groups/%d",
  devpath, dev->iommuGroup) < 0) {
@@ -441,7 +451,7 @@ pci_device_find_by_content(const char *path)
 {
 char tmp[32];
 
-if (pci_read_file(path, tmp, sizeof(tmp)) < 0)
+if (pci_read_file(path, tmp, sizeof(tmp), true) < 0)
 return NULL;
 
 return pci_device_find_by_id(tmp);
@@ -450,7 +460,10 @@ pci_device_find_by_content(const char *path)
 static int
 pci_device_autobind(struct pciDevice *dev)
 {
-struct pciDriver *driver = pci_driver_find_by_dev(dev);
+struct pciDriver *driver = pci_driver_find_by_driver_override(dev);
+
+if (!driver)
+driver = pci_driver_find_by_dev(dev);
 
 if (!driver) {
 /* No driver found. Nothing to do */
@@ -544,6 +557,31 @@ pci_driver_find_by_path(const char *path)
 return NULL;
 }
 
+static struct pciDriver *
+pci_driver_find_by_driver_override(struct pciDevice *dev)
+{
+VIR_AUTOFREE(char *) path = NULL;
+char tmp[32];
+size_t i;
+
+if (virAsprintfQuiet(,
+ SYSFS_PCI_PREFIX "devices/%s/driver_override",
+ dev->id) < 0)
+return NULL;
+
+if (pci_read_file(path, tmp, sizeof(tmp), false) < 0)
+return NULL;
+
+for (i = 0; i < nPCIDrivers; i++) {
+struct pciDriver *driver = pciDrivers[i];
+
+if (STREQ(tmp, driver->name))
+return driver;
+}
+
+return NULL;
+}
+
 static int
 pci_driver_bind(struct pciDriver *driver,
 struct pciDevice *dev)
@@ -657,6 +695,8 @@ pci_driver_handle_change(int fd ATTRIBUTE_UNUSED, const 
char *path)
 ret = pci_driver_handle_remove_id(path);
 else if (STREQ(file, "drivers_probe"))
 ret = pci_driver_handle_drivers_probe(path);
+else if (STREQ(file, "driver_override"))
+ret = 0; /* nada */
 else
 ABORT("Not handled write to: %s", path);
 return ret;
@@ -711,7 +751,7 @@ pci_driver_handle_new_id(const char *path)
 goto cleanup;
 }
 
-if (pci_read_file(path, buf, 

[libvirt] [PATCH 01/18] virpcimock: Move actions checking one level up

[Michal Privoznik]

The pci_driver_bind() and pci_driver_unbind() functions are
"internal implementation", meaning other parts of the code should
be able to call them and get the job done. Checking for actions
(PCI_ACTION_BIND and PCI_ACTION_UNBIND) should be done in
handlers (pci_driver_handle_bind() and
pci_driver_handle_unbind()). Surprisingly, the other two actions
(PCI_ACTION_NEW_ID and PCI_ACTION_REMOVE_ID) are checked already
at this level.

Signed-off-by: Michal Privoznik 
---
 tests/virpcimock.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index beb5e1490d..6865f992dc 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -551,8 +551,8 @@ pci_driver_bind(struct pciDriver *driver,
 int ret = -1;
 char *devpath = NULL, *driverpath = NULL;
 
-if (dev->driver || PCI_ACTION_BIND & driver->fail) {
-/* Device already bound or failing driver requested */
+if (dev->driver) {
+/* Device already bound */
 errno = ENODEV;
 return ret;
 }
@@ -598,8 +598,8 @@ pci_driver_unbind(struct pciDriver *driver,
 int ret = -1;
 char *devpath = NULL, *driverpath = NULL;
 
-if (dev->driver != driver || PCI_ACTION_UNBIND & driver->fail) {
-/* Device not bound to the @driver or failing driver used */
+if (dev->driver != driver) {
+/* Device not bound to the @driver */
 errno = ENODEV;
 return ret;
 }
@@ -669,8 +669,8 @@ pci_driver_handle_bind(const char *path)
 struct pciDevice *dev = pci_device_find_by_content(path);
 struct pciDriver *driver = pci_driver_find_by_path(path);
 
-if (!driver || !dev) {
-/* This should never happen (TM) */
+if (!driver || !dev || PCI_ACTION_BIND & driver->fail) {
+/* No driver, no device or failing driver requested */
 errno = ENODEV;
 goto cleanup;
 }
@@ -686,8 +686,8 @@ pci_driver_handle_unbind(const char *path)
 int ret = -1;
 struct pciDevice *dev = pci_device_find_by_content(path);
 
-if (!dev || !dev->driver) {
-/* This should never happen (TM) */
+if (!dev || !dev->driver || PCI_ACTION_UNBIND & dev->driver->fail) {
+/* No device, device not binded or failing driver requested */
 errno = ENODEV;
 goto cleanup;
 }
-- 
2.21.0

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

[libvirt] [PATCH 1/2] qapi: deprecate drive-mirror and drive-backup

[Vladimir Sementsov-Ogievskiy]

It's hard and not necessary to maintain outdated versions of these
commands.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qemu-deprecated.texi  |  4 
 qapi/block-core.json  |  4 
 qapi/transaction.json |  2 +-
 blockdev.c| 10 ++
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index fff07bb2a3..2753fafd0b 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -179,6 +179,10 @@ and accurate ``query-qmp-schema'' command.
 Character devices creating sockets in client mode should not specify
 the 'wait' field, which is only applicable to sockets in server mode
 
+@subsection drive-mirror, drive-backup and drive-backup transaction action 
(since 4.2)
+
+Use blockdev-mirror and blockdev-backup instead.
+
 @section Human Monitor Protocol (HMP) commands
 
 @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 
3.1)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0d43d4f37c..4e35526634 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1635,6 +1635,8 @@
 ##
 # @drive-backup:
 #
+# Command is deprecated, use blockdev-mirror instead.
+#
 # Start a point-in-time copy of a block device to a new destination.  The
 # status of ongoing drive-backup operations can be checked with
 # query-block-jobs where the BlockJobInfo.type field has the value 'backup'.
@@ -1855,6 +1857,8 @@
 ##
 # @drive-mirror:
 #
+# Command is deprecated, use blockdev-mirror instead.
+#
 # Start mirroring a block device's writes to a new destination. target
 # specifies the target of the new image. If the file exists, or if it
 # is a device, it will be used as the new destination for writes. If
diff --git a/qapi/transaction.json b/qapi/transaction.json
index 95edb78227..a16a9ff8a6 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -53,7 +53,7 @@
 # - @blockdev-snapshot: since 2.5
 # - @blockdev-snapshot-internal-sync: since 1.7
 # - @blockdev-snapshot-sync: since 1.1
-# - @drive-backup: since 1.6
+# - @drive-backup: deprecated action, since 1.6
 #
 # Since: 1.1
 ##
diff --git a/blockdev.c b/blockdev.c
index 4d141e9a1f..36e9368e01 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1771,6 +1771,9 @@ static void drive_backup_prepare(BlkActionState *common, 
Error **errp)
 AioContext *aio_context;
 Error *local_err = NULL;
 
+warn_report("drive-backup transaction action is deprecated and will "
+"disappear in future. Use blockdev-backup action instead");
+
 assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
 backup = common->action->u.drive_backup.data;
 
@@ -3591,6 +3594,10 @@ void qmp_drive_backup(DriveBackup *arg, Error **errp)
 {
 
 BlockJob *job;
+
+warn_report("drive-backup command is deprecated and will disappear in "
+"future. Use blockdev-backup instead");
+
 job = do_drive_backup(arg, NULL, errp);
 if (job) {
 job_start(>job);
@@ -3831,6 +3838,9 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
 const char *format = arg->format;
 int ret;
 
+warn_report("drive-mirror command is deprecated and will disappear in "
+"future. Use blockdev-mirror instead");
+
 bs = qmp_get_root_bs(arg->device, errp);
 if (!bs) {
 return;
-- 
2.18.0

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

[libvirt] [PATCH 0/2] Deprecate implicit filters

[Vladimir Sementsov-Ogievskiy]

Hi all!

Max's series to fix some problems around filters consists of 42 patches.
I'm sure that we didn't find all bugs around filters, and that filters
would be a constant source of bugs in future, as during developing new
feature nobody will consider all possible cases of dealing with filters
(OK, somebody will, but it's hard).

So, I'm thinking about starting some deprecations which will help us to
simplify all the picture at least in not far future. So, I'd really want
to deprecate implicit filters, ->file child based filters (move all
filters to use backing child), filename based interfaces (use node-names).

Most simple thing is implicit filters, so let's start from them.
drive-mirror don't support filter-node-name, so I propose to deprecate
it at all, together with drive-backup, instead of adding support of
filter-node-name, what do you think?

Vladimir Sementsov-Ogievskiy (2):
  qapi: deprecate drive-mirror and drive-backup
  qapi: deprecate implicit filters

 qemu-deprecated.texi  | 11 +++
 qapi/block-core.json  | 10 --
 qapi/transaction.json |  2 +-
 include/block/block_int.h | 10 +-
 blockdev.c| 20 
 5 files changed, 49 insertions(+), 4 deletions(-)

-- 
2.18.0

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

[libvirt] [PATCH 2/2] qapi: deprecate implicit filters

[Vladimir Sementsov-Ogievskiy]

To get rid of implicit filters related workarounds in future let's
deprecate them now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qemu-deprecated.texi  |  7 +++
 qapi/block-core.json  |  6 --
 include/block/block_int.h | 10 +-
 blockdev.c| 10 ++
 4 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 2753fafd0b..8222440148 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to sockets in 
server mode
 
 Use blockdev-mirror and blockdev-backup instead.
 
+@subsection implicit filters (since 4.2)
+
+Mirror and commit jobs inserts filters, which becomes implicit if user
+omitted filter-node-name parameter. So omitting it is deprecated, set it
+always. Note, that drive-mirror don't have this parameter, so it will
+create implicit filter anyway, but drive-mirror is deprecated itself too.
+
 @section Human Monitor Protocol (HMP) commands
 
 @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 
3.1)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4e35526634..0505ac9d8b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1596,7 +1596,8 @@
 # @filter-node-name: the node name that should be assigned to the
 #filter driver that the commit job inserts into the graph
 #above @top. If this option is not given, a node name is
-#autogenerated. (Since: 2.9)
+#autogenerated. Omitting this option is deprecated, it will
+#be required in future. (Since: 2.9)
 #
 # @auto-finalize: When false, this job will wait in a PENDING state after it 
has
 # finished its work, waiting for @block-job-finalize before
@@ -2249,7 +2250,8 @@
 # @filter-node-name: the node name that should be assigned to the
 #filter driver that the mirror job inserts into the graph
 #above @device. If this option is not given, a node name is
-#autogenerated. (Since: 2.9)
+#autogenerated. Omitting this option is deprecated, it will
+#be required in future. (Since: 2.9)
 #
 # @copy-mode: when to copy data to the destination; defaults to 'background'
 # (Since: 3.0)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3aa1e832a8..624da0b4a2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -762,7 +762,15 @@ struct BlockDriverState {
 bool sg;/* if true, the device is a /dev/sg* */
 bool probed;/* if true, format was probed rather than specified */
 bool force_share; /* if true, always allow all shared permissions */
-bool implicit;  /* if true, this filter node was automatically inserted */
+
+/*
+ * @implicit field is deprecated, don't set it to true for new filters.
+ * If true, this filter node was automatically inserted and user don't
+ * know about it and unprepared for any effects of it. So, implicit
+ * filters are workarounded and skipped in many places of the block
+ * layer code.
+ */
+bool implicit;
 
 BlockDriver *drv; /* NULL means no media */
 void *opaque;
diff --git a/blockdev.c b/blockdev.c
index 36e9368e01..b3cfaccce1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char 
*job_id, const char *device,
 BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
 int job_flags = JOB_DEFAULT;
 
+if (!has_filter_node_name) {
+warn_report("Omitting filter-node-name parameter is deprecated, it "
+"will be required in future");
+}
+
 if (!has_speed) {
 speed = 0;
 }
@@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char 
*job_id,
 Error *local_err = NULL;
 int ret;
 
+if (!has_filter_node_name) {
+warn_report("Omitting filter-node-name parameter is deprecated, it "
+"will be required in future");
+}
+
 bs = qmp_get_root_bs(device, errp);
 if (!bs) {
 return;
-- 
2.18.0

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

Re: [libvirt] [PATCH 2/3] util: do not repeat the pm-is-supported string

[Ján Tomko]

On Wed, Aug 14, 2019 at 11:05:39AM +0200, Andrea Bolognani wrote:

On Tue, 2019-08-13 at 17:24 +0200, Ján Tomko wrote:

+++ b/src/util/virnodesuspend.c
 case VIR_NODE_SUSPEND_TARGET_HYBRID:
-cmd = virCommandNewArgList("pm-is-supported", "--suspend-hybrid", 
NULL);
+cmd = virCommandNewArgList(binary, "--suspend-hybrid", NULL);
 break;
 default:
-return ret;
+return -1;
 }


Yeah, this last hunk should have been in the first patch O:-)



Oops, --amend vs. rebase --continue bit me again.

Jano


 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


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

Re: [libvirt] [PATCH 3/3] util: be quiet when pm-is-supported is unavailable

[Andrea Bolognani]

On Tue, 2019-08-13 at 17:24 +0200, Ján Tomko wrote:
> Look up the binary name upfront to avoid the error:
> Cannot find 'pm-is-supported' in path: No such file or directory
> 
> In that case, we just assume nodesuspend is not available.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/util/virnodesuspend.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

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 2/3] util: do not repeat the pm-is-supported string

[Andrea Bolognani]

On Tue, 2019-08-13 at 17:24 +0200, Ján Tomko wrote:
> +++ b/src/util/virnodesuspend.c
>  case VIR_NODE_SUSPEND_TARGET_HYBRID:
> -cmd = virCommandNewArgList("pm-is-supported", "--suspend-hybrid", 
> NULL);
> +cmd = virCommandNewArgList(binary, "--suspend-hybrid", NULL);
>  break;
>  default:
> -return ret;
> +return -1;
>  }

Yeah, this last hunk should have been in the first patch O:-)

  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 1/3] util: use VIR_AUTOPTR virNodeSuspendSupportsTargetPMUtils

[Andrea Bolognani]

On Tue, 2019-08-13 at 17:24 +0200, Ján Tomko wrote:
> +++ b/src/util/virnodesuspend.c
> @@ -238,9 +238,8 @@ int virNodeSuspend(unsigned int target,
> /*
>  * Check return code of command == 0 for success
>  * (i.e., the PM capability is supported)
>  */
>  *supported = (status == 0);
> -ret = 0;
> -
> - cleanup:
> -virCommandFree(cmd);
> -return ret;
> +return 0;

Please leave an empty line before 'return'.

Also this doesn't build:

  util/virnodesuspend.c: In function 'virNodeSuspendSupportsTargetPMUtils':
  util/virnodesuspend.c:257:16: error: 'ret' undeclared (first use in this 
function)
257 | return ret;
|^~~

With the obvious fix (s/ret/-1/) applied and the empty line added,

  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] util: mdev: support persistent devices with mdevctl

[Fabiano Fidêncio]

On Wed, Aug 14, 2019 at 12:03 AM Jonathon Jongsma  wrote:
>
> When a host is rebooted, mediated devices disappear from sysfs.  mdevctl
> (https://github.com/mdevctl/mdevctl) is a utility for managing and
> persisting these devices. It maintains a registry of mediated devices
> and can start and stop them by UUID.
>
> when libvirt attempts to create a new mediated device object, we currently
> fail if the path does not exist in sysfs. This patch tries a little bit
> harder by using mdevctl to attempt to activate the device.  The approach
> is fairly naive -- it just calls 'mdevctl start -u $UUID' without
> checking whether this UUID is registered with mdevctl or not.
>
> See https://bugzilla.redhat.com/show_bug.cgi?id=1699274
>
> Signed-off-by: Jonathon Jongsma 
> ---
> NOTES:
> - an argument could be made that we should simply do nothing here. mdevctl 
> does
>   have support for automatically activating the mediated device when the 
> parent
>   device becomes available (via udev). So if the administrator set up the mdev
>   to start automatically, this patch should not even be necessary. That said, 
> I
>   think this patch could still be useful.
> - As I said above, the approach is pretty naive. I could have checked whether
>   the device was defined in mdevctl's registry and given a different error
>   message ("try registering this device with mdevctl") in that scenario. But
>   I chose to keep it simple.

So, I've noticed that mdevctl is not on Fedora yet, at least according
to: https://bugzilla.redhat.com/show_bug.cgi?id=1728381

In any case, would you mind to also update the spec file and add
something like: `Recommends: mdevctl`?

Recommends is a weak dependency. It shouldn't force mdevctl to be
installed, but suggests it to be installed.

Best Regards,
-- 
Fabiano Fidêncio

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

Re: [libvirt] [PATCH 0/3] qemu: Allow migration with disk cache on

[Peter Krempa]

On Tue, Aug 13, 2019 at 18:08:35 +0200, Jiri Denemark wrote:
> QEMU 4.0.0 and newer automatically drops caches at the end of migration,
> which means we can safely allow migration even if disk/driver/@cache is
> not none nor directsync.
> 
> Jiri Denemark (3):
>   qemu: Clarify error message in qemuMigrationSrcIsSafe
>   qemu: Check for drop-cache capability
>   qemu: Allow migration with disk cache on

ACK series.


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