Re: [libvirt] [PATCH] qemu: document snapshot-related fixes in the release notes

[Andrea Bolognani]

On Fri, 2017-01-13 at 17:34 +0100, Peter Krempa wrote:
> ACK, although I don't really think that this explains much. I don't
> really find those patches worth documenting.

Cool, let's drop it then. Sorry for the noise :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

[Eduardo Habkost]

On Fri, Jan 13, 2017 at 03:11:54PM +0100, Jiri Denemark wrote:
> On Fri, Jan 13, 2017 at 11:04:21 +0100, Jiri Denemark wrote:
> > On Thu, Jan 12, 2017 at 11:18:11 -0500, Collin L. Walling wrote:
> > > When running on s390 with a kernel that does not support cpu model 
> > > checking and
> > > with a Qemu new enough to support query-cpu-model-expansion, the 
> > > gathering of qemu
> > > capabilities will fail. Qemu responds to the query-cpu-model-expansion qmp
> > > command with an error because the needed kernel ioct does not exist. When 
> > > this
> > > happens a guest cannot even be defined due to missing qemu capabilities 
> > > data.
> > > 
> > > This patch fixes the problem by silently ignoring generic errors stemming 
> > > from
> > > calls to query-cpu-model-expansion.
> > > 
> > > Reported-by: Farhan Ali 
> > > Signed-off-by: Collin L. Walling 
> > > Signed-off-by: Jason J. Herne 
> > > ---
> > >  src/qemu/qemu_monitor_json.c | 9 +
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > > index e767437..1662749 100644
> > > --- a/src/qemu/qemu_monitor_json.c
> > > +++ b/src/qemu/qemu_monitor_json.c
> > > @@ -5041,6 +5041,15 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr 
> > > mon,
> > >  if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> > >  goto cleanup;
> > >  
> > > +/* Some QEMU architectures have the query-cpu-model-expansion
> > > + * command, but return 'GenericError' instead of simply omitting
> > > + * the command entirely.
> > > + */
> > 
> > Hmm, this comment says something a bit different than the commit
> > message. I hope the issue described by this comment was fixed in QEMU
> > within the same release the query-cpu-model-expansion command was
> > introduced. But we need to fix this nevertheless to address what the
> > commit message describes.
> > 
> > > +if (qemuMonitorJSONHasError(reply, "GenericError")) {
> > > +ret = 0;
> > > +goto cleanup;
> > > +}
> > > +
> > >  if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> > >  goto cleanup;
> > 
> > However, we need to do a little bit more than just ignoring this error.
> > I'll send a v2 soon.
> 
> No I won't send the v2. I was wrong. I thought we should clear the
> QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION capability when we detect the
> command is not actually working, but we can't do that since the
> existence of this command serves as an indicator of other CPU
> configuration capabilities which cannot be probed directly. Thus just
> ignoring non-working query-cpu-model-expansion command is the right
> thing to do.
> 
> But I suggest to change the comment to something like the following:
> 
> /* Even though query-cpu-model-expansion is advertised by
>  * query-commands it may just return GenericError if it is not
>  * implemented for the requested guest architecture or it is not
>  * supported in the host environment.
>  */

If the command is not implemented for the architecture, it should
be already omitted from query-commands. However, if the host
environment doesn't support CPU models, I expect the commands to
fail only for the "host" CPU model, but to be still possible to
expand the other CPU models.

Expansion of static CPU models, specifically, should be always
possible and should never be affected by the host environment.

-- 
Eduardo

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

Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

[Eduardo Habkost]

On Fri, Jan 13, 2017 at 09:06:52AM -0500, Jason J. Herne wrote:
> On 01/13/2017 05:04 AM, Jiri Denemark wrote:
> > On Thu, Jan 12, 2017 at 11:18:11 -0500, Collin L. Walling wrote:
> > > When running on s390 with a kernel that does not support cpu model 
> > > checking and
> > > with a Qemu new enough to support query-cpu-model-expansion, the 
> > > gathering of qemu
> > > capabilities will fail. Qemu responds to the query-cpu-model-expansion qmp
> > > command with an error because the needed kernel ioct does not exist. When 
> > > this
> > > happens a guest cannot even be defined due to missing qemu capabilities 
> > > data.
> > > 
> > > This patch fixes the problem by silently ignoring generic errors stemming 
> > > from
> > > calls to query-cpu-model-expansion.
> > > 
> > > Reported-by: Farhan Ali 
> > > Signed-off-by: Collin L. Walling 
> > > Signed-off-by: Jason J. Herne 
> > > ---
> > >  src/qemu/qemu_monitor_json.c | 9 +
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > > index e767437..1662749 100644
> > > --- a/src/qemu/qemu_monitor_json.c
> > > +++ b/src/qemu/qemu_monitor_json.c
> > > @@ -5041,6 +5041,15 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr 
> > > mon,
> > >  if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> > >  goto cleanup;
> > > 
> > > +/* Some QEMU architectures have the query-cpu-model-expansion
> > > + * command, but return 'GenericError' instead of simply omitting
> > > + * the command entirely.
> > > + */
> > 
> > Hmm, this comment says something a bit different than the commit
> > message. I hope the issue described by this comment was fixed in QEMU
> > within the same release the query-cpu-model-expansion command was
> > introduced. But we need to fix this nevertheless to address what the
> > commit message describes.
> > 
> 
> The issue still exists in Qemu. I can work up a patch to handle it. My first
> idea is to simply test that the ioctl exists and functions at Qemu
> initialization and deregister the query-cpu-model-expansion command in the
> event that the ioctl does not exist or fails. Thoughts?

I'm not sure, probably unregistering the command after KVM is
initialized is too late. Even if today QMP is available only
after the accelerator is already initialized, we might want to
delay accelerator initialization in the future (so the
accelerator could be configured using QMP commands).

Also, I'm not sure when/how exactly the query-cpu-model-expansion
call fails. If the ioctl isn't available, shouldn't
query-cpu-model-expansion fail only when querying "host", but let
the other CPU models to be queried? In this case, unregistering
the command doesn't seem to be the right solution. Probably
omitting "host" (or flagging it as unavailable?) on
query-cpu-model-definitions would be better.

-- 
Eduardo

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

Re: [libvirt] [PATCH] qemu: document snapshot-related fixes in the release notes

[Peter Krempa]

On Fri, Jan 13, 2017 at 17:22:15 +0100, Andrea Bolognani wrote:
> ---
>  docs/news.xml | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index 50c3b3e..043d1fe 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -225,6 +225,15 @@
>for x86_64 HVM domains.
>  
>
> +  
> +
> +  qemu: snapshot-related fixes
> +
> +
> +  Properly handle image locking and stop looking for a compression
> +  program when the memory image format is "raw".
> +
> +  
>  
>
>

ACK, although I don't really think that this explains much. I don't
really find those patches worth documenting.


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

Re: [libvirt] Proposal PCI/PCIe device placement on PAPR guests

[Greg Kurz]

On Fri, 13 Jan 2017 09:57:36 +1100
David Gibson  wrote:

> On Thu, Jan 12, 2017 at 11:31:35AM +0100, Andrea Bolognani wrote:
> > On Mon, 2017-01-09 at 10:46 +1100, David Gibson wrote:  
> > > > >* To allow for hotplugged devices, libvirt should also add a number
> > > > >  of additional, empty vPHBs (the PAPR spec allows for hotplug of
> > > > >  PHBs, but this is not yet implemented in qemu).  
> > > > 
> > > > "A number" here will have to mean "one", same number of
> > > > empty PCIe Root Ports libvirt will add to a newly-defined
> > > > q35 guest.  
> > > 
> > > Umm.. why?  
> > 
> > Because some applications using libvirt would inevitably
> > start relying on the fact that such spare PHBs are
> > available, locking us into providing at least the same
> > number forever. In other words, increasing the amount at
> > a later time is always possible, but decreasing it isn't.
> > We did the same when we started automatically adding PCIe
> > Root Ports to q35 machines.
> > 
> > The rationale is that having a single spare hotpluggable
> > slot is extremely convenient for basic usage, eg. a simple
> > guest created by someone who's not necessarily very
> > familiar with virtualization; on the other hand, if you
> > are actually deploying in production you ought to conduct
> > proper capacity planning and figure out in advance how
> > many devices you're likely to need to hotplug throughout
> > the guest's life.  
> 
> Hm, ok.  Well I guess the limitation is the same as on x86, so it
> shouldn't surprise people.
> 
> > Of course this all will be moot once we can hotplug PHBs :)  
> 
> Yes.  Unfortunately, nobody's actually working on that at present.
> 

Well, there might be someone now :)

Michael Roth had posted a RFC patchset back in 2015:

https://lists.gnu.org/archive/html/qemu-ppc/2015-04/msg00275.html

I'll start from here.

Cheers.

--
Greg


pgpaUP5GNOV4p.pgp
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] news: Add support for guest CPU configuration on s390

[Jiri Denemark]

Signed-off-by: Jiri Denemark 
---
 docs/news.xml | 5 +
 1 file changed, 5 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 50c3b3ea2..a076836ed 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -73,6 +73,11 @@
   volumes when building a new logical pool on target volume(s).
 
   
+  
+
+  qemu: Add support for guest CPU configuration on s390(x)
+
+  
 
 
   
-- 
2.11.0

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

[libvirt] [PATCH] qemu: document snapshot-related fixes in the release notes

[Andrea Bolognani]

---
 docs/news.xml | 9 +
 1 file changed, 9 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 50c3b3e..043d1fe 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -225,6 +225,15 @@
   for x86_64 HVM domains.
 
   
+  
+
+  qemu: snapshot-related fixes
+
+
+  Properly handle image locking and stop looking for a compression
+  program when the memory image format is "raw".
+
+  
 
   
   
-- 
2.7.4

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

Re: [libvirt] [PATCH] docs: add entry for aggregation of pcie-root-ports to news.xml

[Andrea Bolognani]

On Fri, 2017-01-13 at 10:29 -0500, Laine Stump wrote:
> ---
>  docs/news.xml | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index 50c3b3e..18006e8 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -140,6 +140,19 @@
>the storage pool XML.
>  
>
> +  
> +
> +  qemu: aggregate pcie-root-ports onto multiple functions of a slot
> +
> +
> +  When pcie-root-ports are added to pcie-root in order to
> +  provide a place to connect PCI Express endpoint devices,
> +  libvirt now aggregates multiple root-ports together onto the

Mh, I'd say either go with "root ports" or stick with
"pcie-root-ports" here, "root-ports" is neither here nor
there.

> +  same slot (up to 8 per slot) in order to conserve
> +  slots. Using this method, it's possible to connect more than
> +  200 endpoint devices to a guest that uses PCIe without
> +  requiring setup of any PCIe switches.
> +

Oops, you forgot to close the  element here!


ACK (and safe for freeze) with that fixed.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

[Jiri Denemark]

On Fri, Jan 13, 2017 at 09:20:40 -0500, Jason J. Herne wrote:
> On 01/13/2017 09:11 AM, Jiri Denemark wrote:
> > But I suggest to change the comment to something like the following:
> >
> > /* Even though query-cpu-model-expansion is advertised by
> >  * query-commands it may just return GenericError if it is not
> >  * implemented for the requested guest architecture or it is not
> >  * supported in the host environment.
> >  */
> >
> > ACK to this patch and please let me know if you agree with the modified
> > comment.
> >
> 
> Jiri, yes, I agree with the comment change. Thanks for taking a look at 
> this.

OK, I pushed this with the modified comment. Thanks.

Jirka

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

[libvirt] [PATCH] docs: add entry for aggregation of pcie-root-ports to news.xml

[Laine Stump]

---
 docs/news.xml | 13 +
 1 file changed, 13 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 50c3b3e..18006e8 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -140,6 +140,19 @@
   the storage pool XML.
 
   
+  
+
+  qemu: aggregate pcie-root-ports onto multiple functions of a slot
+
+
+  When pcie-root-ports are added to pcie-root in order to
+  provide a place to connect PCI Express endpoint devices,
+  libvirt now aggregates multiple root-ports together onto the
+  same slot (up to 8 per slot) in order to conserve
+  slots. Using this method, it's possible to connect more than
+  200 endpoint devices to a guest that uses PCIe without
+  requiring setup of any PCIe switches.
+
 
 
   
-- 
2.7.4

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

Re: [libvirt] [PATCH 1/3] storage: Alter logic when both BLKID and PARTED unavailable

[Peter Krempa]

On Fri, Jan 13, 2017 at 07:59:08 -0500, John Ferlan wrote:
> If neither BLKID or PARTED is available and we're not writing, then
> just return 0 which allows the underlying storage pool to generate
> a failur. If both are unavailable and we're writing, then generate

failure

> a more generic error message.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend.c | 20 ++--
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 18433e9..6bdfbf1 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -2842,9 +2842,6 @@ virStorageBackendBLKIDFindEmpty(const char *device 
> ATTRIBUTE_UNUSED,
>  const char *format ATTRIBUTE_UNUSED,
>  bool writelabel ATTRIBUTE_UNUSED)
>  {
> -virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> -   _("probing for filesystems is unsupported "
> - "by this build"));
>  return -2;
>  }
>  
> @@ -2868,11 +2865,10 @@ virStorageBackendPARTEDValidLabel(const char *device 
> ATTRIBUTE_UNUSED,
>const char *format ATTRIBUTE_UNUSED,
>bool writelabel ATTRIBUTE_UNUSED)
>  {
> -virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> -   _("PARTED is unsupported by this build"));
> -return -1;
> +return -2;
>  }
>  
> +
>  #endif /* #if WITH_STORAGE_DISK */
>  
>  
> @@ -2898,5 +2894,17 @@ virStorageBackendDeviceIsEmpty(const char *devpath,
> writelabel)) == -2)
>  ret = virStorageBackendPARTEDValidLabel(devpath, format, writelabel);
>  
> +/* Neither BLKID nor PARTED available, but we're not writing,
> + * so no mechanism to check, so allow a lower layer to reject. */

I think you removed too many words here when compared to the commit
message so it stopped making sense.

> +if (ret == -2 && !writelabel)
> +return 0;
> +
> +if (ret == -2) {
> +virReportError(VIR_ERR_OPERATION_INVALID,
> +   _("Unable to probe '%s' for existing data, "
> + "requires create/build using overwrite"),

I'd state "forced overwrite is necessary" or something along that points
to the flag even for direct API users.

ACK


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] storage: Alter error message in probe/empty checks

[Peter Krempa]

On Fri, Jan 13, 2017 at 07:59:10 -0500, John Ferlan wrote:
> For case VIR_STORAGE_BLKID_PROBE_DIFFERENT, clean up the message to
> avoid using the virsh like --overwrite syntax. Additionally provide
> a different error message when not writing the label to avoid confusion.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend.c | 14 ++
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index eebf039..c6a08eb 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -2808,10 +2808,16 @@ virStorageBackendBLKIDFindEmpty(const char *device,
>  break;
>  
>  case VIR_STORAGE_BLKID_PROBE_DIFFERENT:
> -virReportError(VIR_ERR_STORAGE_POOL_BUILT,
> -   _("Device '%s' formatted cannot overwrite using '%s', 
> "
> - "requires build --overwrite"),
> -   device, format);
> +if (writelabel)
> +virReportError(VIR_ERR_STORAGE_POOL_BUILT,
> +   _("Device '%s' formatted cannot overwrite using "

The beginning of the error message does not make sense.

> + "'%s', requires create/build with overwrite 
> flag"),
> +   device, format);
> +else
> +virReportError(VIR_ERR_OPERATION_INVALID,
> +   _("Format of device '%s' does not match expected "
> + "format '%s', requires rebuild"),

I'd drop the "requires rebuild" part.

ACK


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

Re: [libvirt] [PATCH 2/3] storage: Clean up return value checking

[Peter Krempa]

On Fri, Jan 13, 2017 at 07:59:09 -0500, John Ferlan wrote:
> Rather than special casing the VIR_STORAGE_BLKID_PROBE_UNKNOWN after
> calling virStorageBackendBLKIDFindPart, just allow the switch statement
> handle setting ret = -2.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend.c | 10 +-
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 6bdfbf1..eebf039 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -2777,10 +2777,6 @@ virStorageBackendBLKIDFindEmpty(const char *device,
>  rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN) {
>  
>  rc = virStorageBackendBLKIDFindPart(probe, device, format);
> -if (rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN) {
> -ret = -2;
> -goto cleanup;
> -}
>  }
>  
>  switch (rc) {
> @@ -2799,10 +2795,7 @@ virStorageBackendBLKIDFindEmpty(const char *device,
>  break;
>  
>  case VIR_STORAGE_BLKID_PROBE_UNKNOWN:
> -virReportError(VIR_ERR_STORAGE_PROBE_FAILED,
> -   _("Not capable of probing for format type '%s', "
> - "requires build --overwrite"),
> -   format);
> +ret = -2;

ACK, nice to see the virshism to go.


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

Re: [libvirt] [PATCH v2 1/2] nodedev: Fabric name must not be required for fc_host capability

[John Ferlan]

On 01/13/2017 06:56 AM, Boris Fiuczynski wrote:
> fabric_name is one of many fc_host attributes in Linux that is optional
> and left to the low-level driver to decide if it is implemented.
> The zfcp device driver does not provide a fabric name for an fcp host.
> 
> This patch removes the requirement for a fabric name by making it optional.
> 
> Signed-off-by: Boris Fiuczynski 
> ---
>  docs/formatnode.html.in   |  2 +-
>  docs/news.xml | 12 +
>  docs/schemas/nodedev.rng  |  8 +++---
>  src/libvirt_private.syms  |  1 +
>  src/node_device/node_device_linux_sysfs.c |  6 ++---
>  src/util/virutil.c| 23 
>  src/util/virutil.h|  5 
>  tests/fchostdata/fc_host/host6/node_name  |  1 +
>  tests/fchostdata/fc_host/host6/port_name  |  1 +
>  tests/fchostdata/fc_host/host6/port_state |  1 +
>  tests/fchosttest.c| 44 
> ---
>  11 files changed, 94 insertions(+), 10 deletions(-)
>  create mode 100644 tests/fchostdata/fc_host/host6/node_name
>  create mode 100644 tests/fchostdata/fc_host/host6/port_name
>  create mode 100644 tests/fchostdata/fc_host/host6/port_state
> 
> diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
> index e7b1140..f8d0e12 100644
> --- a/docs/formatnode.html.in
> +++ b/docs/formatnode.html.in
> @@ -254,7 +254,7 @@
>  number of vport in use. max_vports shows the
>  maximum vports the HBA supports. "fc_host" implies following
>  sub-elements: wwnn, wwpn, and
> -fabric_wwn.
> +optionally fabric_wwn.
>
>  
>
> diff --git a/docs/news.xml b/docs/news.xml
> index 50c3b3e..a645953 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -144,6 +144,18 @@
>  
>
>  
> +  nodedev: Fabric name must not be required for fc_host capability
> +
> +
> +  fabric_name is one of many fc_host attributes in Linux that is
> +  optional and left to the low-level driver to decide if it is
> +  implemented. For example the zfcp device driver does not provide a
> +  fabric name for an fcp host. The requirement for the existence of
> +  a fabric name has been removed by making it optional.
> +
> +  
> +  
> +
>qemu: Correct GetBlockInfo values
>  
>  
> diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
> index 9c98402..b100a6e 100644
> --- a/docs/schemas/nodedev.rng
> +++ b/docs/schemas/nodedev.rng
> @@ -367,9 +367,11 @@
>
>  
>  
> -
> -  
> -
> +
> +  
> +
> +  
> +
>
>  
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 70ed87b..43f460f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2717,6 +2717,7 @@ virParseOwnershipIds;
>  virParseVersionString;
>  virPipeReadUntilEOF;
>  virReadFCHost;
> +virReadFCHostOption;

I don't think the Option; API is necessary

>  virReadSCSIUniqueId;
>  virScaleInteger;
>  virSetBlocking;
> diff --git a/src/node_device/node_device_linux_sysfs.c 
> b/src/node_device/node_device_linux_sysfs.c
> index be99c41..1bb5b74 100644
> --- a/src/node_device/node_device_linux_sysfs.c
> +++ b/src/node_device/node_device_linux_sysfs.c
> @@ -72,10 +72,10 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d)
>  VIR_FREE(d->scsi_host.wwnn);
>  VIR_STEAL_PTR(d->scsi_host.wwnn, tmp);
>  
> -if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "fabric_name"))) {
> -VIR_WARN("Failed to read fabric WWN for host%d",
> +if (!(tmp = virReadFCHostOption(NULL, d->scsi_host.host,
> +"fabric_name", false))) {
> +VIR_INFO("Optional fabric WWN for host%d not available",
>   d->scsi_host.host);
> -goto cleanup;
>  }

Just make this:

if ((tmp = virReadFCHost(NULL, d->scsi_host.host, "fabric_name"))) {
VIR_FREE(d->scsi_host.fabric_wwn);
VIR_STEAL_PTR(d->scsi_host.fabric_wwn, tmp);
}

and remove the "need" for a virReadFCHostOption function...

>  VIR_FREE(d->scsi_host.fabric_wwn);
>  VIR_STEAL_PTR(d->scsi_host.fabric_wwn, tmp);
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index aeaa7f9..5bfbf37 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -2019,6 +2019,26 @@ virReadFCHost(const char *sysfs_prefix,
>int host,
>const char *entry)
>  {
> +   return virReadFCHostOption(sysfs_prefix, host, entry, true);
> +}
> +
> +/* virReadFCHostOption:
> + * @sysfs_prefix: "fc_host" sysfs path, defaults to SYSFS_FC_HOST_PATH
> + * @host: Host number, E.g. 5 of "fc_host/host5"
> + * 

Re: [libvirt] [PATCH] qemu: Copy SELinux labels for namespace too

[Michal Privoznik]

On 01/13/2017 12:43 PM, Martin Kletzander wrote:
> On Fri, Jan 13, 2017 at 11:12:43AM +0100, Michal Privoznik wrote:
>> When creating new /dev/* for qemu, we do chown() and copy ACLs to
>> create the exact copy from the original /dev. I though that
>> copying SELinux labels is not necessary as SELinux will chose the
>> sane defaults. Surprisingly, it does not leaving namespace with
>> the following labels:
>>
>> crw-rw-rw-. root root system_u:object_r:tmpfs_t:s0 random
>> crw---. root root system_u:object_r:tmpfs_t:s0 rtc0
>> drwxrwxrwt. root root system_u:object_r:tmpfs_t:s0 shm
>> crw-rw-rw-. root root system_u:object_r:tmpfs_t:s0 urandom
>>
>> As a result, domain is unable to start:
>>
>> error: internal error: process exited while connecting to monitor:
>> Error in GnuTLS initialization: Failed to acquire random data.
>> qemu-kvm: cannot initialize crypto: Unable to initialize GNUTLS
>> library: Failed to acquire random data.
>>
>> The solution is to copy the SELinux labels as well.
>>
>> Reported-by: Andrea Bolognani 
>> Signed-off-by: Michal Privoznik 
>> ---
>> src/qemu/qemu_domain.c | 61
>> ++
>> 1 file changed, 61 insertions(+)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 1399dee0d..a29866673 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -63,6 +63,9 @@
>> #if defined(HAVE_SYS_MOUNT_H)
>> # include 
>> #endif
>> +#ifdef WITH_SELINUX
>> +# include 
>> +#endif
>>
>> #include 
>>
>> @@ -6958,6 +6961,9 @@ qemuDomainCreateDevice(const char *device,
>> char *canonDevicePath = NULL;
>> struct stat sb;
>> int ret = -1;
>> +#ifdef WITH_SELINUX
>> +char *tcon = NULL;
>> +#endif
>>
>> if (virFileResolveAllLinks(device, ) < 0) {
>> if (errno == ENOENT && allow_noent) {
>> @@ -7023,10 +7029,34 @@ qemuDomainCreateDevice(const char *device,
>> goto cleanup;
>> }
>>
>> +#ifdef WITH_SELINUX
>> +if (getfilecon_raw(canonDevicePath, ) < 0 &&
>> +(errno != ENOTSUP && errno != ENODATA)) {
>> +virReportSystemError(errno,
>> + _("Unable to get SELinux label on %s"),
>> canonDevicePath);
>> +goto cleanup;
>> +}
>> +
>> +if (tcon &&
>> +setfilecon_raw(devicePath, (VIR_SELINUX_CTX_CONST char *)
>> tcon) < 0) {
>> +VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
>> +if (errno != EOPNOTSUPP && errno != ENOTSUP) {
>> +VIR_WARNINGS_RESET
>> +virReportSystemError(errno,
>> + _("Unable to set SELinux label on %s"),
>> + devicePath);
>> +goto cleanup;
>> +}
>> +}
>> +#endif
>> +
> 
> I think, instead of all the ifdefs, this should be a security driver API
> instead of being hardcoded in places.  That way it will be processed
> properly by all the security drivers.

I don't think I see what you mean. Firstly, we want to set seclabels for
some paths that are not touched by secdrivers at all (e.g. /dev/*random,
/dev/kvm). Secondly, secdrivers should honour norelabel flag and be a
no-op if that one is set. This would clash with sysadmins handling
seclabels themselves. Thirdly, no secdriver of ours deals with ACLs. We
have to in here.

Michal

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

Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

[Jason J. Herne]

On 01/13/2017 09:11 AM, Jiri Denemark wrote:

On Fri, Jan 13, 2017 at 11:04:21 +0100, Jiri Denemark wrote:

On Thu, Jan 12, 2017 at 11:18:11 -0500, Collin L. Walling wrote:

When running on s390 with a kernel that does not support cpu model checking and
with a Qemu new enough to support query-cpu-model-expansion, the gathering of 
qemu
capabilities will fail. Qemu responds to the query-cpu-model-expansion qmp
command with an error because the needed kernel ioct does not exist. When this
happens a guest cannot even be defined due to missing qemu capabilities data.

This patch fixes the problem by silently ignoring generic errors stemming from
calls to query-cpu-model-expansion.

Reported-by: Farhan Ali 
Signed-off-by: Collin L. Walling 
Signed-off-by: Jason J. Herne 
---
 src/qemu/qemu_monitor_json.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e767437..1662749 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5041,6 +5041,15 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
 if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
 goto cleanup;

+/* Some QEMU architectures have the query-cpu-model-expansion
+ * command, but return 'GenericError' instead of simply omitting
+ * the command entirely.
+ */


Hmm, this comment says something a bit different than the commit
message. I hope the issue described by this comment was fixed in QEMU
within the same release the query-cpu-model-expansion command was
introduced. But we need to fix this nevertheless to address what the
commit message describes.


+if (qemuMonitorJSONHasError(reply, "GenericError")) {
+ret = 0;
+goto cleanup;
+}
+
 if (qemuMonitorJSONCheckError(cmd, reply) < 0)
 goto cleanup;


However, we need to do a little bit more than just ignoring this error.
I'll send a v2 soon.


No I won't send the v2. I was wrong. I thought we should clear the
QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION capability when we detect the
command is not actually working, but we can't do that since the
existence of this command serves as an indicator of other CPU
configuration capabilities which cannot be probed directly. Thus just
ignoring non-working query-cpu-model-expansion command is the right
thing to do.

But I suggest to change the comment to something like the following:

/* Even though query-cpu-model-expansion is advertised by
 * query-commands it may just return GenericError if it is not
 * implemented for the requested guest architecture or it is not
 * supported in the host environment.
 */

ACK to this patch and please let me know if you agree with the modified
comment.



Jiri, yes, I agree with the comment change. Thanks for taking a look at 
this.


--
-- Jason J. Herne (jjhe...@linux.vnet.ibm.com)

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

Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

[Jiri Denemark]

On Fri, Jan 13, 2017 at 11:04:21 +0100, Jiri Denemark wrote:
> On Thu, Jan 12, 2017 at 11:18:11 -0500, Collin L. Walling wrote:
> > When running on s390 with a kernel that does not support cpu model checking 
> > and
> > with a Qemu new enough to support query-cpu-model-expansion, the gathering 
> > of qemu
> > capabilities will fail. Qemu responds to the query-cpu-model-expansion qmp
> > command with an error because the needed kernel ioct does not exist. When 
> > this
> > happens a guest cannot even be defined due to missing qemu capabilities 
> > data.
> > 
> > This patch fixes the problem by silently ignoring generic errors stemming 
> > from
> > calls to query-cpu-model-expansion.
> > 
> > Reported-by: Farhan Ali 
> > Signed-off-by: Collin L. Walling 
> > Signed-off-by: Jason J. Herne 
> > ---
> >  src/qemu/qemu_monitor_json.c | 9 +
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index e767437..1662749 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -5041,6 +5041,15 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr 
> > mon,
> >  if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> >  goto cleanup;
> >  
> > +/* Some QEMU architectures have the query-cpu-model-expansion
> > + * command, but return 'GenericError' instead of simply omitting
> > + * the command entirely.
> > + */
> 
> Hmm, this comment says something a bit different than the commit
> message. I hope the issue described by this comment was fixed in QEMU
> within the same release the query-cpu-model-expansion command was
> introduced. But we need to fix this nevertheless to address what the
> commit message describes.
> 
> > +if (qemuMonitorJSONHasError(reply, "GenericError")) {
> > +ret = 0;
> > +goto cleanup;
> > +}
> > +
> >  if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> >  goto cleanup;
> 
> However, we need to do a little bit more than just ignoring this error.
> I'll send a v2 soon.

No I won't send the v2. I was wrong. I thought we should clear the
QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION capability when we detect the
command is not actually working, but we can't do that since the
existence of this command serves as an indicator of other CPU
configuration capabilities which cannot be probed directly. Thus just
ignoring non-working query-cpu-model-expansion command is the right
thing to do.

But I suggest to change the comment to something like the following:

/* Even though query-cpu-model-expansion is advertised by
 * query-commands it may just return GenericError if it is not
 * implemented for the requested guest architecture or it is not
 * supported in the host environment.
 */

ACK to this patch and please let me know if you agree with the modified
comment.

Jirka

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

Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

[Jason J. Herne]

On 01/13/2017 05:04 AM, Jiri Denemark wrote:

On Thu, Jan 12, 2017 at 11:18:11 -0500, Collin L. Walling wrote:

When running on s390 with a kernel that does not support cpu model checking and
with a Qemu new enough to support query-cpu-model-expansion, the gathering of 
qemu
capabilities will fail. Qemu responds to the query-cpu-model-expansion qmp
command with an error because the needed kernel ioct does not exist. When this
happens a guest cannot even be defined due to missing qemu capabilities data.

This patch fixes the problem by silently ignoring generic errors stemming from
calls to query-cpu-model-expansion.

Reported-by: Farhan Ali 
Signed-off-by: Collin L. Walling 
Signed-off-by: Jason J. Herne 
---
 src/qemu/qemu_monitor_json.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e767437..1662749 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5041,6 +5041,15 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
 if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
 goto cleanup;

+/* Some QEMU architectures have the query-cpu-model-expansion
+ * command, but return 'GenericError' instead of simply omitting
+ * the command entirely.
+ */


Hmm, this comment says something a bit different than the commit
message. I hope the issue described by this comment was fixed in QEMU
within the same release the query-cpu-model-expansion command was
introduced. But we need to fix this nevertheless to address what the
commit message describes.



The issue still exists in Qemu. I can work up a patch to handle it. My first
idea is to simply test that the ioctl exists and functions at Qemu
initialization and deregister the query-cpu-model-expansion command in the
event that the ioctl does not exist or fails. Thoughts?


However, we need to do a little bit more than just ignoring this error.
I'll send a v2 soon.


--
-- Jason J. Herne (jjhe...@linux.vnet.ibm.com)

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

Re: [libvirt] [PATCH] qemu: Copy SELinux labels for namespace too

[Michal Privoznik]

On 01/13/2017 12:26 PM, Andrea Bolognani wrote:
> On Fri, 2017-01-13 at 11:12 +0100, Michal Privoznik wrote:
> [...]
>> @@ -7023,10 +7029,34 @@ qemuDomainCreateDevice(const char *device,
>>   goto cleanup;
>>   }
>>   
>> +#ifdef WITH_SELINUX
>> +if (getfilecon_raw(canonDevicePath, ) < 0 &&
>> +(errno != ENOTSUP && errno != ENODATA)) {
>> +virReportSystemError(errno,
>> + _("Unable to get SELinux label on %s"), 
>> canonDevicePath);
> 
> s/get SELinux label on/get SELinux label from/
> 
> One more occurrence in the patch.
> 
>> +goto cleanup;
>> +}
>> +
>> +if (tcon &&
>> +setfilecon_raw(devicePath, (VIR_SELINUX_CTX_CONST char *) tcon) < 
>> 0) {
>> +VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
>> +if (errno != EOPNOTSUPP && errno != ENOTSUP) {
>> +VIR_WARNINGS_RESET
>> +virReportSystemError(errno,
>> + _("Unable to set SELinux label on %s"),
>> + devicePath);
> 
> Please decide whether you want the argument to %s on the same
> line as the format string or on the next, and stick with it :)
> 
> [...]
>> @@ -7571,6 +7617,9 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
>> ATTRIBUTE_UNUSED,
>>cleanup:
>>   if (ret < 0 && delDevice)
>>   unlink(data->file);
>> +#ifdef WITH_SELINUX
>> +freecon(data->tcon);
>> +#endif
> 
> I don't think you should free the SELinux context...
> 
>>   virFileFreeACLs(>acl);
> 
> ... or the ACLs, for that matter, on failure: the caller
> will free them already if the helper fails, which is good
> because whoever allocates the memory should be responsible
> for releasing it.

In fact I need to. This function is ran in a separate process. Therefore
there is no double free. On the other hand, with return from this
function the process ends anyway, so if we don't free it kernel will.

I'm keeping it for the time being.

Michal

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

[libvirt] [PATCH 1/3] storage: Alter logic when both BLKID and PARTED unavailable

[John Ferlan]

If neither BLKID or PARTED is available and we're not writing, then
just return 0 which allows the underlying storage pool to generate
a failur. If both are unavailable and we're writing, then generate
a more generic error message.

Signed-off-by: John Ferlan 
---
 src/storage/storage_backend.c | 20 ++--
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 18433e9..6bdfbf1 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -2842,9 +2842,6 @@ virStorageBackendBLKIDFindEmpty(const char *device 
ATTRIBUTE_UNUSED,
 const char *format ATTRIBUTE_UNUSED,
 bool writelabel ATTRIBUTE_UNUSED)
 {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-   _("probing for filesystems is unsupported "
- "by this build"));
 return -2;
 }
 
@@ -2868,11 +2865,10 @@ virStorageBackendPARTEDValidLabel(const char *device 
ATTRIBUTE_UNUSED,
   const char *format ATTRIBUTE_UNUSED,
   bool writelabel ATTRIBUTE_UNUSED)
 {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-   _("PARTED is unsupported by this build"));
-return -1;
+return -2;
 }
 
+
 #endif /* #if WITH_STORAGE_DISK */
 
 
@@ -2898,5 +2894,17 @@ virStorageBackendDeviceIsEmpty(const char *devpath,
writelabel)) == -2)
 ret = virStorageBackendPARTEDValidLabel(devpath, format, writelabel);
 
+/* Neither BLKID nor PARTED available, but we're not writing,
+ * so no mechanism to check, so allow a lower layer to reject. */
+if (ret == -2 && !writelabel)
+return 0;
+
+if (ret == -2) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("Unable to probe '%s' for existing data, "
+ "requires create/build using overwrite"),
+   devpath);
+}
+
 return ret == 0;
 }
-- 
2.7.4

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

[libvirt] [PATCH 2/3] storage: Clean up return value checking

[John Ferlan]

Rather than special casing the VIR_STORAGE_BLKID_PROBE_UNKNOWN after
calling virStorageBackendBLKIDFindPart, just allow the switch statement
handle setting ret = -2.

Signed-off-by: John Ferlan 
---
 src/storage/storage_backend.c | 10 +-
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 6bdfbf1..eebf039 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -2777,10 +2777,6 @@ virStorageBackendBLKIDFindEmpty(const char *device,
 rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN) {
 
 rc = virStorageBackendBLKIDFindPart(probe, device, format);
-if (rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN) {
-ret = -2;
-goto cleanup;
-}
 }
 
 switch (rc) {
@@ -2799,10 +2795,7 @@ virStorageBackendBLKIDFindEmpty(const char *device,
 break;
 
 case VIR_STORAGE_BLKID_PROBE_UNKNOWN:
-virReportError(VIR_ERR_STORAGE_PROBE_FAILED,
-   _("Not capable of probing for format type '%s', "
- "requires build --overwrite"),
-   format);
+ret = -2;
 break;
 
 case VIR_STORAGE_BLKID_PROBE_MATCH:
@@ -2829,7 +2822,6 @@ virStorageBackendBLKIDFindEmpty(const char *device,
 ret = -1;
 }
 
- cleanup:
 blkid_free_probe(probe);
 
 return ret;
-- 
2.7.4

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

[libvirt] [PATCH 3/3] storage: Alter error message in probe/empty checks

[John Ferlan]

For case VIR_STORAGE_BLKID_PROBE_DIFFERENT, clean up the message to
avoid using the virsh like --overwrite syntax. Additionally provide
a different error message when not writing the label to avoid confusion.

Signed-off-by: John Ferlan 
---
 src/storage/storage_backend.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index eebf039..c6a08eb 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -2808,10 +2808,16 @@ virStorageBackendBLKIDFindEmpty(const char *device,
 break;
 
 case VIR_STORAGE_BLKID_PROBE_DIFFERENT:
-virReportError(VIR_ERR_STORAGE_POOL_BUILT,
-   _("Device '%s' formatted cannot overwrite using '%s', "
- "requires build --overwrite"),
-   device, format);
+if (writelabel)
+virReportError(VIR_ERR_STORAGE_POOL_BUILT,
+   _("Device '%s' formatted cannot overwrite using "
+ "'%s', requires create/build with overwrite 
flag"),
+   device, format);
+else
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("Format of device '%s' does not match expected "
+ "format '%s', requires rebuild"),
+   device, format);
 break;
 }
 
-- 
2.7.4

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

[libvirt] [PATCH 0/3] More adjustments for recent storage probe logic

[John Ferlan]

Adjustments based on recent activity... 

The !writelabel path of patch 3 is probably a moot point since patches
to check at FS and Logical startup for a valid data on the device(s)
were reverted.

John Ferlan (3):
  storage: Alter logic when both BLKID and PARTED unavailable
  storage: Clean up return value checking
  storage: Alter error message in probe/empty checks

 src/storage/storage_backend.c | 44 ---
 1 file changed, 25 insertions(+), 19 deletions(-)

-- 
2.7.4

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

[libvirt] [PATCH v2 2/2] util: Minor correction in documenting comment

[Boris Fiuczynski]

Signed-off-by: Boris Fiuczynski 
---
 src/util/virutil.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virutil.c b/src/util/virutil.c
index 5bfbf37..d79775d 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -2011,7 +2011,7 @@ virGetSCSIHostNameByParentaddr(unsigned int domain,
  *
  * Read the value of sysfs "fc_host" entry.
  *
- * Returns result as a stringon success, caller must free @result after
+ * Returns result as a string on success, caller must free @result after
  * Otherwise returns NULL.
  */
 char *
-- 
2.5.5

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

[libvirt] [PATCH v2 1/2] nodedev: Fabric name must not be required for fc_host capability

[Boris Fiuczynski]

fabric_name is one of many fc_host attributes in Linux that is optional
and left to the low-level driver to decide if it is implemented.
The zfcp device driver does not provide a fabric name for an fcp host.

This patch removes the requirement for a fabric name by making it optional.

Signed-off-by: Boris Fiuczynski 
---
 docs/formatnode.html.in   |  2 +-
 docs/news.xml | 12 +
 docs/schemas/nodedev.rng  |  8 +++---
 src/libvirt_private.syms  |  1 +
 src/node_device/node_device_linux_sysfs.c |  6 ++---
 src/util/virutil.c| 23 
 src/util/virutil.h|  5 
 tests/fchostdata/fc_host/host6/node_name  |  1 +
 tests/fchostdata/fc_host/host6/port_name  |  1 +
 tests/fchostdata/fc_host/host6/port_state |  1 +
 tests/fchosttest.c| 44 ---
 11 files changed, 94 insertions(+), 10 deletions(-)
 create mode 100644 tests/fchostdata/fc_host/host6/node_name
 create mode 100644 tests/fchostdata/fc_host/host6/port_name
 create mode 100644 tests/fchostdata/fc_host/host6/port_state

diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
index e7b1140..f8d0e12 100644
--- a/docs/formatnode.html.in
+++ b/docs/formatnode.html.in
@@ -254,7 +254,7 @@
 number of vport in use. max_vports shows the
 maximum vports the HBA supports. "fc_host" implies following
 sub-elements: wwnn, wwpn, and
-fabric_wwn.
+optionally fabric_wwn.
   
 
   
diff --git a/docs/news.xml b/docs/news.xml
index 50c3b3e..a645953 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -144,6 +144,18 @@
 
   
 
+  nodedev: Fabric name must not be required for fc_host capability
+
+
+  fabric_name is one of many fc_host attributes in Linux that is
+  optional and left to the low-level driver to decide if it is
+  implemented. For example the zfcp device driver does not provide a
+  fabric name for an fcp host. The requirement for the existence of
+  a fabric name has been removed by making it optional.
+
+  
+  
+
   qemu: Correct GetBlockInfo values
 
 
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
index 9c98402..b100a6e 100644
--- a/docs/schemas/nodedev.rng
+++ b/docs/schemas/nodedev.rng
@@ -367,9 +367,11 @@
   
 
 
-
-  
-
+
+  
+
+  
+
   
 
   
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 70ed87b..43f460f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2717,6 +2717,7 @@ virParseOwnershipIds;
 virParseVersionString;
 virPipeReadUntilEOF;
 virReadFCHost;
+virReadFCHostOption;
 virReadSCSIUniqueId;
 virScaleInteger;
 virSetBlocking;
diff --git a/src/node_device/node_device_linux_sysfs.c 
b/src/node_device/node_device_linux_sysfs.c
index be99c41..1bb5b74 100644
--- a/src/node_device/node_device_linux_sysfs.c
+++ b/src/node_device/node_device_linux_sysfs.c
@@ -72,10 +72,10 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d)
 VIR_FREE(d->scsi_host.wwnn);
 VIR_STEAL_PTR(d->scsi_host.wwnn, tmp);
 
-if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "fabric_name"))) {
-VIR_WARN("Failed to read fabric WWN for host%d",
+if (!(tmp = virReadFCHostOption(NULL, d->scsi_host.host,
+"fabric_name", false))) {
+VIR_INFO("Optional fabric WWN for host%d not available",
  d->scsi_host.host);
-goto cleanup;
 }
 VIR_FREE(d->scsi_host.fabric_wwn);
 VIR_STEAL_PTR(d->scsi_host.fabric_wwn, tmp);
diff --git a/src/util/virutil.c b/src/util/virutil.c
index aeaa7f9..5bfbf37 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -2019,6 +2019,26 @@ virReadFCHost(const char *sysfs_prefix,
   int host,
   const char *entry)
 {
+   return virReadFCHostOption(sysfs_prefix, host, entry, true);
+}
+
+/* virReadFCHostOption:
+ * @sysfs_prefix: "fc_host" sysfs path, defaults to SYSFS_FC_HOST_PATH
+ * @host: Host number, E.g. 5 of "fc_host/host5"
+ * @entry: Name of the sysfs entry to read
+ * @required: If true reports error when entry not found else no error
+ *
+ * Read the value of sysfs "fc_host" entry.
+ *
+ * Returns result as a string on success, caller must free @result after
+ * Otherwise returns NULL.
+ */
+char *
+virReadFCHostOption(const char *sysfs_prefix,
+int host,
+const char *entry,
+bool required)
+{
 char *sysfs_path = NULL;
 char *p = NULL;
 char *buf = NULL;
@@ -2029,6 +2049,9 @@ virReadFCHost(const char *sysfs_prefix,
 host, entry) < 

[libvirt] [PATCH v2 0/2] Fabric name must not be required for fc_host capability

[Boris Fiuczynski]

fabric_name is one of many fc_host attributes in Linux that is optional
and left to the low-level driver to decide if it is implemented.

---
Changes since v1:
  - split of minor correction in documenting comment
  - added news entry...
  - added new virReadFCHostOption method
  - added check for file existence when optional is set
  - rearranged fchost tests to fit the numbering

---
Boris Fiuczynski (2):
  nodedev: Fabric name must not be required for fc_host capability
  util: Minor correction in documenting comment

 docs/formatnode.html.in   |  2 +-
 docs/news.xml | 12 +
 docs/schemas/nodedev.rng  |  8 +++---
 src/libvirt_private.syms  |  1 +
 src/node_device/node_device_linux_sysfs.c |  6 ++---
 src/util/virutil.c| 25 +-
 src/util/virutil.h|  5 
 tests/fchostdata/fc_host/host6/node_name  |  1 +
 tests/fchostdata/fc_host/host6/port_name  |  1 +
 tests/fchostdata/fc_host/host6/port_state |  1 +
 tests/fchosttest.c| 44 ---
 11 files changed, 95 insertions(+), 11 deletions(-)
 create mode 100644 tests/fchostdata/fc_host/host6/node_name
 create mode 100644 tests/fchostdata/fc_host/host6/port_name
 create mode 100644 tests/fchostdata/fc_host/host6/port_state

-- 
2.5.5

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

Re: [libvirt] [PATCH] qemu: Copy SELinux labels for namespace too

[Martin Kletzander]

On Fri, Jan 13, 2017 at 11:12:43AM +0100, Michal Privoznik wrote:

When creating new /dev/* for qemu, we do chown() and copy ACLs to
create the exact copy from the original /dev. I though that
copying SELinux labels is not necessary as SELinux will chose the
sane defaults. Surprisingly, it does not leaving namespace with
the following labels:

crw-rw-rw-. root root system_u:object_r:tmpfs_t:s0 random
crw---. root root system_u:object_r:tmpfs_t:s0 rtc0
drwxrwxrwt. root root system_u:object_r:tmpfs_t:s0 shm
crw-rw-rw-. root root system_u:object_r:tmpfs_t:s0 urandom

As a result, domain is unable to start:

error: internal error: process exited while connecting to monitor: Error in 
GnuTLS initialization: Failed to acquire random data.
qemu-kvm: cannot initialize crypto: Unable to initialize GNUTLS library: Failed 
to acquire random data.

The solution is to copy the SELinux labels as well.

Reported-by: Andrea Bolognani 
Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_domain.c | 61 ++
1 file changed, 61 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1399dee0d..a29866673 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -63,6 +63,9 @@
#if defined(HAVE_SYS_MOUNT_H)
# include 
#endif
+#ifdef WITH_SELINUX
+# include 
+#endif

#include 

@@ -6958,6 +6961,9 @@ qemuDomainCreateDevice(const char *device,
char *canonDevicePath = NULL;
struct stat sb;
int ret = -1;
+#ifdef WITH_SELINUX
+char *tcon = NULL;
+#endif

if (virFileResolveAllLinks(device, ) < 0) {
if (errno == ENOENT && allow_noent) {
@@ -7023,10 +7029,34 @@ qemuDomainCreateDevice(const char *device,
goto cleanup;
}

+#ifdef WITH_SELINUX
+if (getfilecon_raw(canonDevicePath, ) < 0 &&
+(errno != ENOTSUP && errno != ENODATA)) {
+virReportSystemError(errno,
+ _("Unable to get SELinux label on %s"), 
canonDevicePath);
+goto cleanup;
+}
+
+if (tcon &&
+setfilecon_raw(devicePath, (VIR_SELINUX_CTX_CONST char *) tcon) < 0) {
+VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
+if (errno != EOPNOTSUPP && errno != ENOTSUP) {
+VIR_WARNINGS_RESET
+virReportSystemError(errno,
+ _("Unable to set SELinux label on %s"),
+ devicePath);
+goto cleanup;
+}
+}
+#endif
+


I think, instead of all the ifdefs, this should be a security driver API
instead of being hardcoded in places.  That way it will be processed
properly by all the security drivers.

For the purpose of this release, let's say I noticed this patch right
after the release and hence didn't have time to NACK it properly on
time.

Martin


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

Re: [libvirt] [PATCH] qemu: Copy SELinux labels for namespace too

[Andrea Bolognani]

On Fri, 2017-01-13 at 11:12 +0100, Michal Privoznik wrote:
[...]
> @@ -7023,10 +7029,34 @@ qemuDomainCreateDevice(const char *device,
>  goto cleanup;
>  }
>  
> +#ifdef WITH_SELINUX
> +if (getfilecon_raw(canonDevicePath, ) < 0 &&
> +(errno != ENOTSUP && errno != ENODATA)) {
> +virReportSystemError(errno,
> + _("Unable to get SELinux label on %s"), 
> canonDevicePath);

s/get SELinux label on/get SELinux label from/

One more occurrence in the patch.

> +goto cleanup;
> +}
> +
> +if (tcon &&
> +setfilecon_raw(devicePath, (VIR_SELINUX_CTX_CONST char *) tcon) < 0) 
> {
> +VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
> +if (errno != EOPNOTSUPP && errno != ENOTSUP) {
> +VIR_WARNINGS_RESET
> +virReportSystemError(errno,
> + _("Unable to set SELinux label on %s"),
> + devicePath);

Please decide whether you want the argument to %s on the same
line as the format string or on the next, and stick with it :)

[...]
> @@ -7571,6 +7617,9 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
> ATTRIBUTE_UNUSED,
>   cleanup:
>  if (ret < 0 && delDevice)
>  unlink(data->file);
> +#ifdef WITH_SELINUX
> +freecon(data->tcon);
> +#endif

I don't think you should free the SELinux context...

>  virFileFreeACLs(>acl);

... or the ACLs, for that matter, on failure: the caller
will free them already if the helper fails, which is good
because whoever allocates the memory should be responsible
for releasing it.

[...]
> @@ -7619,6 +7677,9 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
>  
>  ret = 0;
>   cleanup:
> +#ifdef WITH_SELINUX
> +freecon(data.tcon);
> +#endif
>  virFileFreeACLs();
>  return 0;

Existing, but I'm pretty sure you want to return 'ret'
rather than 0 here.


ACK once you deal with the issues mentioned above, and we
definitely want to have this in as soon as possible.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH] docs: Fix libvirt_guest nss module name

[Michal Privoznik]

In the documentation we are mixing libvirt-guest and
libvirt_guest module name. The correct name is the latter.

Signed-off-by: Michal Privoznik 
---

Pushed as trivial.

 docs/news.xml|  4 ++--
 docs/nss.html.in | 12 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/docs/news.xml b/docs/news.xml
index 8a876e998..50c3b3ea2 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -29,10 +29,10 @@
   
   
 
-  nss: Introduce libvirt-guest
+  nss: Introduce libvirt_guest
 
 
-  New libvirt-guest nss module that translates libvirt
+  New libvirt_guest nss module that translates libvirt
   guest names into IP addresses.
 
   
diff --git a/docs/nss.html.in b/docs/nss.html.in
index e07f9b774..2a5a46cd1 100644
--- a/docs/nss.html.in
+++ b/docs/nss.html.in
@@ -72,34 +72,34 @@ hosts:   files libvirt dns
 records. Therefore this is dependent on hostname provided by guests. Thing
 is, not all the guests out there provide one in DHCP transactions, or not
 every sysadmin out there believes all the guests. Hence libvirt implements
-second method in libvirt-guest module which does libvirt guest
+second method in libvirt_guest module which does libvirt guest
 name to IP address translation (regardless of hostname set in the guest).
 
 
 
 To enable either of the modules put their name into the
 nsswitch.conf file. For instance, to enable
-libvirt-guest module:
+libvirt_guest module:
 
 
 
 $ cat /etc/nsswitch.conf
 # /etc/nsswitch.conf:
-hosts:   files libvirt-guest dns
+hosts:   files libvirt_guest dns
 # ...
 
 Or users can enable both at the same time:
 
 $ cat /etc/nsswitch.conf
 # /etc/nsswitch.conf:
-hosts:   files libvirt libvirt-guest dns
+hosts:   files libvirt libvirt_guest dns
 # ...
 
 
 
 This configuration will mean that if hostname is not found by the
 libvirt module (e.g. because a guest did not sent hostname
-during DHCP transaction), the libvirt-guest module is
+during DHCP transaction), the libvirt_guest module is
 consulted (and if the hostname matches libvirt guest name it will be
 resolved).
 
@@ -181,7 +181,7 @@ virsh domifaddr --source lease $domain
 
 If there's no record for either of the aforementioned commands, it's
 very likely that NSS module won't find anything and vice versa.
-As of v3.0.0 libvirt provides libvirt-guest NSS
+As of v3.0.0 libvirt provides libvirt_guest NSS
 module that doesn't have this limitation. However, the statement is still
 true for the libvirt NSS module.
 
-- 
2.11.0

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

Re: [libvirt] [PATCH v2 10/10] nss: Introduce libvirt-guest module

[Michal Privoznik]

On 01/13/2017 11:27 AM, Andrea Bolognani wrote:
> On Mon, 2016-12-05 at 11:31 +0100, Michal Privoznik wrote:
> [...]
>> +
>> +$ cat /etc/nsswitch.conf
>> +# /etc/nsswitch.conf:
>> +hosts:   files libvirt-guest dns
>> +# ...
>> +
>> +Or users can enable both at the same time:
>> +
>> +$ cat /etc/nsswitch.conf
>> +# /etc/nsswitch.conf:
>> +hosts:   files libvirt libvirt-guest dns
>> +# ...
>> +
> 
> I just realized that, while the documentation consistently
> refers to the new module as "libvirt-guest", the module is
> actually called "libvirt_guest". In particular, the examples
> above won't work at all.
> 
> We should change one or the other before release.

Ah, good catch.

Unfortunately, due to way that glibc works, we cannot use libvirt-guest
and have to stick with libvirt_guest. The problem is: when gethost*() is
called, glibc loads all the plugins listed in nsswitch.conf and tries to
see if they implement the name translation function. This function,
however, has to be unique otherwise symbol clash would occur. Therefore
glibc developers decided to go with:

_nss_$(pluginName)_$(implemntedFunction)_r

Now, if $pluginName was "libvirt-guest" you could hardly declare the
function in C as its name would have to be:

_nss_libvirt-guest_gethostbyname_r

C compiler sees two tokens there. Therefore we have to stick with a bit
more ugly "libvirt_guest" name.

Patch on its way.

Michal

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

Re: [libvirt] [PATCH v2 10/10] nss: Introduce libvirt-guest module

[Andrea Bolognani]

On Mon, 2016-12-05 at 11:31 +0100, Michal Privoznik wrote:
[...]
> +
> +$ cat /etc/nsswitch.conf
> +# /etc/nsswitch.conf:
> +hosts:   files libvirt-guest dns
> +# ...
> +
> +Or users can enable both at the same time:
> +
> +$ cat /etc/nsswitch.conf
> +# /etc/nsswitch.conf:
> +hosts:   files libvirt libvirt-guest dns
> +# ...
> +

I just realized that, while the documentation consistently
refers to the new module as "libvirt-guest", the module is
actually called "libvirt_guest". In particular, the examples
above won't work at all.

We should change one or the other before release.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH] qemu: Copy SELinux labels for namespace too

[Michal Privoznik]

When creating new /dev/* for qemu, we do chown() and copy ACLs to
create the exact copy from the original /dev. I though that
copying SELinux labels is not necessary as SELinux will chose the
sane defaults. Surprisingly, it does not leaving namespace with
the following labels:

crw-rw-rw-. root root system_u:object_r:tmpfs_t:s0 random
crw---. root root system_u:object_r:tmpfs_t:s0 rtc0
drwxrwxrwt. root root system_u:object_r:tmpfs_t:s0 shm
crw-rw-rw-. root root system_u:object_r:tmpfs_t:s0 urandom

As a result, domain is unable to start:

error: internal error: process exited while connecting to monitor: Error in 
GnuTLS initialization: Failed to acquire random data.
qemu-kvm: cannot initialize crypto: Unable to initialize GNUTLS library: Failed 
to acquire random data.

The solution is to copy the SELinux labels as well.

Reported-by: Andrea Bolognani 
Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 61 ++
 1 file changed, 61 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1399dee0d..a29866673 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -63,6 +63,9 @@
 #if defined(HAVE_SYS_MOUNT_H)
 # include 
 #endif
+#ifdef WITH_SELINUX
+# include 
+#endif
 
 #include 
 
@@ -6958,6 +6961,9 @@ qemuDomainCreateDevice(const char *device,
 char *canonDevicePath = NULL;
 struct stat sb;
 int ret = -1;
+#ifdef WITH_SELINUX
+char *tcon = NULL;
+#endif
 
 if (virFileResolveAllLinks(device, ) < 0) {
 if (errno == ENOENT && allow_noent) {
@@ -7023,10 +7029,34 @@ qemuDomainCreateDevice(const char *device,
 goto cleanup;
 }
 
+#ifdef WITH_SELINUX
+if (getfilecon_raw(canonDevicePath, ) < 0 &&
+(errno != ENOTSUP && errno != ENODATA)) {
+virReportSystemError(errno,
+ _("Unable to get SELinux label on %s"), 
canonDevicePath);
+goto cleanup;
+}
+
+if (tcon &&
+setfilecon_raw(devicePath, (VIR_SELINUX_CTX_CONST char *) tcon) < 0) {
+VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
+if (errno != EOPNOTSUPP && errno != ENOTSUP) {
+VIR_WARNINGS_RESET
+virReportSystemError(errno,
+ _("Unable to set SELinux label on %s"),
+ devicePath);
+goto cleanup;
+}
+}
+#endif
+
 ret = 0;
  cleanup:
 VIR_FREE(canonDevicePath);
 VIR_FREE(devicePath);
+#ifdef WITH_SELINUX
+freecon(tcon);
+#endif
 return ret;
 }
 
@@ -7472,6 +7502,9 @@ struct qemuDomainAttachDeviceMknodData {
 const char *file;
 struct stat sb;
 void *acl;
+#ifdef WITH_SELINUX
+char *tcon;
+#endif
 };
 
 
@@ -7515,6 +7548,19 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
ATTRIBUTE_UNUSED,
 goto cleanup;
 }
 
+#ifdef WITH_SELINUX
+if (setfilecon_raw(data->file, (VIR_SELINUX_CTX_CONST char *) data->tcon) 
< 0) {
+VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
+if (errno != EOPNOTSUPP && errno != ENOTSUP) {
+VIR_WARNINGS_RESET
+virReportSystemError(errno,
+ _("Unable to set SELinux label on %s"),
+ data->file);
+goto cleanup;
+}
+}
+#endif
+
 switch ((virDomainDeviceType) data->devDef->type) {
 case VIR_DOMAIN_DEVICE_DISK: {
 virDomainDiskDefPtr def = data->devDef->data.disk;
@@ -7571,6 +7617,9 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
ATTRIBUTE_UNUSED,
  cleanup:
 if (ret < 0 && delDevice)
 unlink(data->file);
+#ifdef WITH_SELINUX
+freecon(data->tcon);
+#endif
 virFileFreeACLs(>acl);
 return ret;
 }
@@ -7605,6 +7654,15 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
 return ret;
 }
 
+#ifdef WITH_SELINUX
+if (getfilecon_raw(file, ) < 0 &&
+(errno != ENOTSUP && errno != ENODATA)) {
+virReportSystemError(errno,
+ _("Unable to get SELinux label on %s"), file);
+goto cleanup;
+}
+#endif
+
 if (virSecurityManagerPreFork(driver->securityManager) < 0)
 goto cleanup;
 
@@ -7619,6 +7677,9 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
 
 ret = 0;
  cleanup:
+#ifdef WITH_SELINUX
+freecon(data.tcon);
+#endif
 virFileFreeACLs();
 return 0;
 }
-- 
2.11.0

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

Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

[Jiri Denemark]

On Thu, Jan 12, 2017 at 11:18:11 -0500, Collin L. Walling wrote:
> When running on s390 with a kernel that does not support cpu model checking 
> and
> with a Qemu new enough to support query-cpu-model-expansion, the gathering of 
> qemu
> capabilities will fail. Qemu responds to the query-cpu-model-expansion qmp
> command with an error because the needed kernel ioct does not exist. When this
> happens a guest cannot even be defined due to missing qemu capabilities data.
> 
> This patch fixes the problem by silently ignoring generic errors stemming from
> calls to query-cpu-model-expansion.
> 
> Reported-by: Farhan Ali 
> Signed-off-by: Collin L. Walling 
> Signed-off-by: Jason J. Herne 
> ---
>  src/qemu/qemu_monitor_json.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index e767437..1662749 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5041,6 +5041,15 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>  if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
>  goto cleanup;
>  
> +/* Some QEMU architectures have the query-cpu-model-expansion
> + * command, but return 'GenericError' instead of simply omitting
> + * the command entirely.
> + */

Hmm, this comment says something a bit different than the commit
message. I hope the issue described by this comment was fixed in QEMU
within the same release the query-cpu-model-expansion command was
introduced. But we need to fix this nevertheless to address what the
commit message describes.

> +if (qemuMonitorJSONHasError(reply, "GenericError")) {
> +ret = 0;
> +goto cleanup;
> +}
> +
>  if (qemuMonitorJSONCheckError(cmd, reply) < 0)
>  goto cleanup;

However, we need to do a little bit more than just ignoring this error.
I'll send a v2 soon.

Jirka

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

Re: [libvirt] [V4] RFC for support cache tune(CAT) in libvirt

[Daniel P. Berrange]

On Fri, Jan 13, 2017 at 09:38:44AM +0800, 乔立勇(Eli Qiao) wrote:
> 
> virsh capabilities
> 
> 
> 
> 
> 
>   
>  <- level 3 cache is per socket, so group them by
> socket id
> 
>
> 
>cpus="3,4,5,9,10,11"/>
> 
>   
> 
>   
> 
>   
> 
>   
> 
>   
> 
> ...
> 
>  
> 
> 
> 
> Opens
> 
> 1.  how about add socket id to bank for bank type = l3 ?

This isn't needed - with the 'cpu' IDs here, the application can
look at the topology info in the capabilities to find out what
socket the logical CPU is part of.

> 2.  do we really want to expose l2/l3 cache for now , they are per core
> resource and linux kernel don't support l2 yet (depend no hardware)?

We dont't need to report all levels of cache - we just need the XML
schema to allow it by design.

> 3.  if enable CDP in resctrl, for bank type=l3 , it will be split to
> l3data l3code, should expose this ability.
> 
>   
>  <- level 3 cache is per socket, so group them by
> socket id
> 
>

'cdp' is intel specific terminology. We need to use some more generic
description. Perhaps we want this when CDP is enabled:




and when its disabled just



If we have this scope option, then we'll need it when reporting too...

> ## Provide a new API to get the avail cache on each bank, such as the
> output are:
> 
> 
> 
>id=0
> 
>type=l3

...eg

  scope=data

>avail=56320


> 
>total = ?? <- do we need this?

That info is static and available from capabilities, so we
don't need to repeat it here IMHO.

> 
> 
> 
>id=1
> 
>type=l3
> 
>avail=56320
> 
> 
> 
>id=3
> 
>type=l2
> 
>avail=256
> 
> 
> 
> Opens:
> 
> · Don't expose the avail cache information if the host can not do
> the allocation of that type cache(eg, for l2 currently) ?

This api should only report info about cache banks that support allocation/.

> · We can not make all of the cache , the reservation amount is the
> min_cbm_len (=1) * min_unit .

If there is some minimum amount which is reserved and cannot be
allocated, we should report that in the capabilities XML too.
eg



> · do we need to expose total?

No, that's available in capabilities XML

> ##  enable CAT for a domain
> 
> 
> 
> 1 Domain XML changes
> 
> 
> 
>
> 
>
> 
>
> 
> 
> 
>
> 
>
> 
>
> 
>
> 
>
> 
> 2. Extend cputune command ?

Do we need the ability to change cache allocation for a running
guest ?  If so, then we need to extend cputune command, if not
we can ignore it.

> Opens:
> 
> 
> 
> 1. Do we accept to extend existed API ? or using new API/virsh?
> 
> 2. How to calculate cache size -> CBM bit?
> 
> 
> 
> eg:
> 
> 5632/ 2816 = 2 bits
> 
> 5733/ 2816 = 2 bits or 3 bits?

In the capabilities XML we report the min unit granularity:



So in the XML, we should report an error if the requested
size is *not* a multiple of the reported unit granulirty

> ## Restriction for using cache tune on multiple sockets' host.
> 
> 
> 
> The l3 cache is per socket resource, kernel need to know about what's
> affinity looks like, so for a VM which running on a multiple socket's host,
> it should have NUMA setting or vcpuset pin setting. Or cache tune will fail.

Yep, we need to report an error if cache allocation is requested
without CPU pinning being requested for the VM.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] Proposal PCI/PCIe device placement on PAPR guests

[Greg Kurz]

On Fri, 13 Jan 2017 15:48:31 +1100
David Gibson  wrote:

> On Thu, Jan 12, 2017 at 10:09:03AM +0100, Greg Kurz wrote:
> > On Thu, 12 Jan 2017 17:19:40 +1100
> > Alexey Kardashevskiy  wrote:
> >   
> > > On 12/01/17 14:52, David Gibson wrote:  
> > > > On Fri, Jan 06, 2017 at 12:57:58PM +0100, Greg Kurz wrote:
> > > >> On Thu, 5 Jan 2017 16:46:18 +1100
> > > >> David Gibson  wrote:
> > > >>
> > > >>> There was a discussion back in November on the qemu list which spilled
> > > >>> onto the libvirt list about how to add support for PCIe devices to
> > > >>> POWER VMs, specifically 'pseries' machine type PAPR guests.
> > > >>>
> > > >>> Here's a more concrete proposal for how to handle part of this in
> > > >>> future from the libvirt side.  Strictly speaking what I'm suggesting
> > > >>> here isn't intrinsically linked to PCIe: it will make adding PCIe
> > > >>> support sanely easier, as well as having a number of advantages for
> > > >>> both PCIe and plain-PCI devices on PAPR guests.
> > > >>>
> > > >>> Background:
> > > >>>
> > > >>>  * Currently the pseries machine type only supports vanilla PCI
> > > >>>buses.
> > > >>> * This is a qemu limitation, not something inherent - PAPR guests
> > > >>>   running under PowerVM (the IBM hypervisor) can use passthrough
> > > >>>   PCIe devices (PowerVM doesn't emulate devices though).
> > > >>> * In fact the way PCI access is para-virtalized in PAPR makes the
> > > >>>   usual distinctions between PCI and PCIe largely disappear
> > > >>>  * Presentation of PCIe devices to PAPR guests is unusual
> > > >>> * Unlike x86 - and other "bare metal" platforms, root ports are
> > > >>>   not made visible to the guest. i.e. all devices (typically)
> > > >>>   appear as though they were integrated devices on x86
> > > >>> * In terms of topology all devices will appear in a way similar to
> > > >>>   a vanilla PCI bus, even PCIe devices
> > > >>>* However PCIe extended config space is accessible
> > > >>> * This means libvirt's usual placement of PCIe devices is not
> > > >>>   suitable for PAPR guests
> > > >>>  * PAPR has its own hotplug mechanism
> > > >>> * This is used instead of standard PCIe hotplug
> > > >>> * This mechanism works for both PCIe and vanilla-PCI devices
> > > >>> * This can hotplug/unplug devices even without a root port P2P
> > > >>>   bridge between it and the root "bus
> > > >>>  * Multiple independent host bridges are routine on PAPR
> > > >>> * Unlike PC (where all host bridges have multiplexed access to
> > > >>>   configuration space) PCI host bridges (PHBs) are truly
> > > >>>   independent for PAPR guests (disjoint MMIO regions in system
> > > >>>   address space)
> > > >>> * PowerVM typically presents a separate PHB to the guest for each
> > > >>>   host slot passed through
> > > >>>
> > > >>> The Proposal:
> > > >>>
> > > >>> I suggest that libvirt implement a new default algorithm for placing
> > > >>> (i.e. assigning addresses to) both PCI and PCIe devices for (only)
> > > >>> PAPR guests.
> > > >>>
> > > >>> The short summary is that by default it should assign each device to a
> > > >>> separate vPHB, creating vPHBs as necessary.
> > > >>>
> > > >>>   * For passthrough sometimes a group of host devices can't be safely
> > > >>> isolated from each other - this is known as a (host) Partitionable
> > > >>> Endpoint (PE).  In this case, if any device in the PE is passed
> > > >>> through to a guest, the whole PE must be passed through to the
> > > >>> same vPHB in the guest.  From the guest POV, each vPHB has exactly
> > > >>> one (guest) PE.
> > > >>>   * To allow for hotplugged devices, libvirt should also add a number
> > > >>> of additional, empty vPHBs (the PAPR spec allows for hotplug of
> > > >>> PHBs, but this is not yet implemented in qemu).  When hotplugging
> > > >>> a new device (or PE) libvirt should locate a vPHB which doesn't
> > > >>> currently contain anything.
> > > >>>   * libvirt should only (automatically) add PHBs - never root ports or
> > > >>> other PCI to PCI bridges
> > > >>>
> > > >>> In order to handle migration, the vPHBs will need to be represented in
> > > >>> the domain XML, which will also allow the user to override this
> > > >>> topology if they want.
> > > >>>
> > > >>> Advantages:
> > > >>>
> > > >>> There are still some details I need to figure out w.r.t. handling PCIe
> > > >>> devices (on both the qemu and libvirt sides).  However the fact that  
> > > >>>   
> > > >>
> > > >> One such detail may be that PCIe devices should have the
> > > >> "ibm,pci-config-space-type" property set to 1 in the DT,
> > > >> for the driver to be able to access the extended config
> > > >> space.
> > > > 
> > > > So, we have a bit of an oddity here.  It looks like we currently set
> > > > 

Re: [libvirt] [PATCH 0/4] Doc update changes for my recent commits

[Martin Kletzander]

On Wed, Jan 11, 2017 at 07:25:51AM -0500, John Ferlan wrote:

These are all trivial and will push in a bit - I was essentially waiting
for the news.html.in or news.xml to shake out before adjusting.

John Ferlan (4):
 docs: Document the new vHBA/NPIV params for storage
 docs: Add NPIV/vHBA change description to news.xml
 docs: Add file system pool overwrite change description to news.xml
 docs: Add logical storage pool overwrite change description to
 news.xml



The last two could be one entry, since it's similar thing.  Also "Add
... description to news.xml" could be just "news:; prefix ;)

ACK series (with tiny comments), safe for freeze


docs/formatstorage.html.in | 17 +
docs/news.xml  | 34 ++
2 files changed, 51 insertions(+)

--
2.7.4

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


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

Re: [libvirt] [PATCH 3/4] docs: Add file system pool overwrite change description to news.xml

[Martin Kletzander]

On Wed, Jan 11, 2017 at 07:25:54AM -0500, John Ferlan wrote:

Add bug fixes description of overwrite changes for a file system storage pool

Signed-off-by: John Ferlan 
---
docs/news.xml | 12 
1 file changed, 12 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 13f6965..ec65766 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -162,6 +162,18 @@
  /dev.

  
+  
+
+  storage: Fix implementation of no-overwrite for file system backend
+
+
+  Fix file system storage backend implementation of the OVERWRITE
+  flags to be consistent between code and documentation. Add checks
+  to ensure that when building a new file system on a target volume
+  that there is not something already on the disk in a format that
+  libvirt can recognize.


This is pretty long and confusing sentence, especially without any
punctuation.  ACK to this, but I'd be also fine with it being reworde
slightly.


+
+  

  
  
--
2.7.4

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


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

Re: [libvirt] [PATCH 1/4] docs: Document the new vHBA/NPIV params for storage

[Martin Kletzander]

On Wed, Jan 11, 2017 at 07:25:52AM -0500, John Ferlan wrote:

Commit id 'bb74a7ffe' forgot to adjust the storage docs to describe the
new fields.

Signed-off-by: John Ferlan 
---
docs/formatstorage.html.in | 17 +
1 file changed, 17 insertions(+)

diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index a7273ed..f6887ae 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -237,6 +237,23 @@
the scsi_hostN of some existing storage pool.
Since 1.0.4
  
+  parent_wwnn and parent_wwpn
+  Instead of the parent to specify which scsi_host
+to use by name, it's possible to provide the wwnn and wwpn of
+the parent to be used for the vHBA in order to ensure that
+between reboots or after a hardware configuration change that
+the scsi_host parent name doesn't change. Both the parent_wwnn
+and parent_wwpn must be provided.
+Since 3.0.0
+  
+  parent_fabric_wwn
+  Instead of the parent to specify which scsi_host
+to use by name, it's possible to provide the fabric_wwn on which
+the scsi_host exists. This provides flexibility for choosing
+a scsi_host that may be available on the fabric rather than
+requiring a specific parent by wwnn or wwpn to be available.
+Since 3.0.0
+  
  managed
  An optional attribute to instruct the SCSI storage backend to
manage destroying the vHBA when the pool is destroyed. For
--
2.7.4



ACK, safe for freeze (since it's docs).  Have you seen this [1] patch?
I think I understand it righ, but since you implemented it, it'd be nice
to have your input on that as well.

[1] https://www.redhat.com/archives/libvir-list/2017-January/msg00519.html


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


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

Re: [libvirt] [PATCH 1/3] Revert "storage: For FS pool check for properly formatted target volume"

[Peter Krempa]

On Thu, Jan 12, 2017 at 19:45:52 -0500, John Ferlan wrote:
> 
> 
> On 01/12/2017 09:41 AM, Peter Krempa wrote:
> > On Thu, Jan 12, 2017 at 09:36:01 -0500, John Ferlan wrote:
> >>
> >>
> >> On 01/12/2017 09:24 AM, Peter Krempa wrote:
> >>> The check does not work properly (crashes) with netfs filesystems and
> >>> also checking that a device is not empty when attempting to mount a
> >>> filesystem is not very usefull since the mount will fail anyways.
> >>>
> >>> As the code would improve only a very minor corner case I don't really
> >>> see a reason to have this code at all.
> >>>
> >>> This code would also fail if libvirt is compiled without support for
> >>> blkid and without parted.
> >>>
> >>> This reverts commit a11fd69735e6951cda9bf256d8e423696a441aa4.
> >>> ---
> >>>  src/storage/storage_backend_fs.c | 13 ++---
> >>>  1 file changed, 2 insertions(+), 11 deletions(-)
> >>>
> >>
> >> Instead of reverting why not just fix the issue.
> > 
> > Because I think the whole check is pointless. It's valid only when
> > mounting a local filesystem, and in that case basically the same check
> > is done when mounting the filesystem by the kernel.
> > 
> 
> Here's the difference between the check and removing the check (not
> withstanding the no PARTED and no BLKID available)...
> 
> Create a pool using ext4, start it - life is happy.  Destroy the pool.
> Define a pool using xfs.  Start it
> 
> w/ my change:
> 
> # virsh pool-start fs
> error: Failed to start pool fs
> error: Storage pool already built: Device '/dev/sde' formatted cannot
> overwrite using 'xfs', requires build --overwrite

I think that the error code and the message are in fact misleading here.

You are starting the pool not building it, so both the message comming
from the error code and the free form string state nonsensical facts.

> 
> w/o my change (e.g. if the change is reverted):
> 
> # virsh pool-start fs
> error: Failed to start pool fs
> error: internal error: Child process (/usr/bin/mount -t xfs /dev/sde
> /home/vm-images/fs) unexpected exit status 32: mount: wrong fs type, bad
> option, bad superblock on /dev/sde,
>missing codepage or helper program, or other error
> 
>In some cases useful info is found in syslog - try
>dmesg | tail or so.
> 

While this is a bit too verbose, but in my opinion describes the problem
better.

Peter


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