Re: [libvirt] [PATCH v2 2/4] conf: Parse virtio-crypto in the domain XML

[Longpeng (Mike)]

Hi Martin,

On 2017/2/7 20:15, Martin Kletzander wrote:

> On Wed, Jan 11, 2017 at 04:28:24PM +0800, Longpeng(Mike) wrote:
>> This patch parse the domain XML with virtio-crypto
>> support, the virtio-crypto XML looks like this:
>>
>>  
>>
>>  
>>
>> Signed-off-by: Longpeng(Mike) 
>> ---
>> src/conf/domain_conf.c | 213 
>> -
>> src/conf/domain_conf.h |  32 +++
>> src/libvirt_private.syms   |   2 +
>> src/qemu/qemu_domain.c |   2 +
>> src/qemu/qemu_domain_address.c |   1 +
>> src/qemu/qemu_driver.c |   6 ++
>> src/qemu/qemu_hotplug.c|   1 +
>> 7 files changed, 256 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 52aee2b..ef44930 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -18967,6 +19096,25 @@ virDomainRNGDefCheckABIStability(virDomainRNGDefPtr 
>> src,
>>
>>
>> static bool
>> +virDomainCryptoDefCheckABIStability(virDomainCryptoDefPtr src,
>> +virDomainCryptoDefPtr dst)
>> +{
>> +if (src->model != dst->model) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +   _("Target Crypto model '%s' does not match source 
>> '%s'"),
>> +   virDomainCryptoModelTypeToString(dst->model),
>> +   virDomainCryptoModelTypeToString(src->model));
>> +return false;
>> +}
>> +
> 
> The number of queues is not part of ABI?  That'd make sense, I'm just
> making sure.


Oh, yep! I think it's necessary to check 'queues' for future scalability,
although QEMU cryptodev only support one queue currently.

I will take all your other suggestions and rebase the patchset on master in V3.

Thanks. :)

> 
...
>> case VIR_DOMAIN_DEVICE_LAST:
>> case VIR_DOMAIN_DEVICE_NONE:
>> return 0;


-- 
Regards,
Longpeng(Mike)

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

Re: [libvirt] [resend v2 0/7] Support cache tune in libvirt

[Eli Qiao]

--  
Eli Qiao
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)


On Tuesday, 7 February 2017 at 7:56 PM, Marcelo Tosatti wrote:

> On Tue, Feb 07, 2017 at 02:43:13PM +0800, Eli Qiao wrote:
> >  
> >  
> > --  
> > Eli Qiao
> > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
> >  
> >  
> > On Tuesday, 7 February 2017 at 3:03 AM, Marcelo Tosatti wrote:
> >  
> > > On Mon, Feb 06, 2017 at 01:33:09PM -0200, Marcelo Tosatti wrote:
> > > > On Mon, Feb 06, 2017 at 10:23:35AM +0800, Eli Qiao wrote:
> > > > > This series patches are for supportting CAT featues, which also
> > > > > called cache tune in libvirt.
> > > > >  
> > > > > First to expose cache information which could be tuned in capabilites 
> > > > > XML.
> > > > > Then add new domain xml element support to add cacahe bank which will 
> > > > > apply
> > > > > on this libvirt domain.
> > > > >  
> > > > > This series patches add a util file `resctrl.c/h`, an interface to 
> > > > > talk with
> > > > > linux kernel's sys fs.
> > > > >  
> > > > > There are still some TODOs such as expose new public interface to get 
> > > > > free
> > > > > cache information.
> > > > >  
> > > > > Some discussion about this feature support can be found from:
> > > > > https://www.redhat.com/archives/libvir-list/2017-January/msg00644.html
> > > > >  
> > > > >  
> > > >  
> > > >  
> > > >  
> > > > Two comments:
> > > >  
> > > > 1) Please perform appropriate filesystem locking when accessing
> > > > resctrlfs, as described at:
> > > >  
> > > > http://www.spinics.net/lists/linux-tip-commits/msg36754.html
> > > >  
> > >  
> > >  
> >  
> >  
> > Sure.  
> > > >  
> > > > 2)  
> > > >  
> > > > 
> > > >  
> > > > [b4c270b5-e0f9-4106-a446-69032872ed7d]# cat tasks  
> > > > 8654
> > > > [b4c270b5-e0f9-4106-a446-69032872ed7d]# pstree -p | grep qemu
> > > > |-qemu-kvm(8654)-+-{qemu-kvm}(8688)
> > > > | |-{qemu-kvm}(8692)
> > > > | `-{qemu-kvm}(8693)
> > > >  
> > > > Should add individual vcpus to the "tasks" file, not the main QEMU
> > > > process.
> > > >  
> > > > The NFV usecase requires exclusive CAT allocation for the vcpu which
> > > > runs the sensitive workload.
> > > >  
> > > > Perhaps:
> > > >  
> > > >   
> > > >  
> > > > Adds all vcpus that are pinned to the socket which cachebank with
> > > > host_id=1.
> > > >  
> > > >  > > > vcpus=2,3/>  
> > > >  
> > > > Adds PID of vcpus 2 and 3 to resctrl directory created for this
> > > > allocation.
> > > >  
> > > >  
> > >  
> > >  
> > >  
> > >  
> >  
> > Hmm.. in this case, we need to figure out what’s the pid of vcpus=2 and 
> > vcpu=3 and added them to the resctrl directory.
> >  
>  
>  
> Yes and only the pids of vcpus=2 and vcpus=3, not of any other vcpus.
>  
> > currently, I create a resctrl directory(called resctrl domain) for a VM so 
> > just put all task ids to it.
> >  
> > this is my though:
> >  
> > let say the vm has vcpus=0 1 2 3, and you want to let 0, 1 benefit cache on 
> > host_id=0, and 2, 3 on host_id=1
> >  
> > you will do:
> >  
> > 1)
> > pin vcpus 0, 1 on the cpus of socket 0  
> > pin vcpus 2, 3 on the cpus of socket 1
> > this can be done in vcputune
> >  
> > 2) define cache tune like this:
> > 
> > 
> >  
> > in libvirt:
> > we create a resctrl directory naming with the VM’s uuid
> > and set schemata for each socket 0, and socket 1, put all qemu tasks ids 
> > into tasks file, this will work fine.  
> >  
>  
>  
> No, please don't do this.
>  
> > please be note that in a resctrl directory, we can define schemata for each 
> > socket id separately.
>  
> Please do not put vcpus automatically into the reservations.
> Its necessary to have certain vcpus in a reservation and some not.
>  
> For example: 2 vcpu guest, vcpu0 pinned to socket 0 cpu0,  
> vcpu1 pinned to socket 0 cpu1.
>  
>  
> 
>  
> We want _only_ vcpu1 to be part of this reservation, and not vcpu0  
> (want vcpu0 to use the default group, ie. schemata file at  
>  
> /sys/fs/resctrl/schemata
>  
> So please have the ability to add vcpus to the XML syntax:
>  
>  vcpus='1'/>
>  
> or
>  
>  vcpus='2,3'/>
>  
Yep, get your mean, make sense.

if there’s exist method can get vcpus’ pid?  
>  
> This also allows different sizes to be specified.
>  
>  
> > > 3) CDP / non-CDP convertion.
> > >  
> > > In case the size determination has been performed with non-CDP,
> > > to emulate such allocation on a CDP host,
> > > it would be good to allow both code and data allocations to share
> > > the CBM space:  
> > >  
> > >  
> >  
> > IOM, I don’t think it’s good to have this.
> > in libvirt capabilities xml, the application will get to know if the host 
> > support cdp or not.
> >  
> > >  
> > > 
> > > 
> > >  
> > > Perhaps if using the same ID?
> > >  
> >  
> > I am open to hear about what other’s say,  
> >  
> > >  
> > >  
> > > Other than that, testing looks good.  
> > >  
> >  
> > Thanks for the testing.  
> >  
>  
>  
>  


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

Re: [libvirt] [PATCH v2 1/4] docs: schema: Add basic documentation for the virtual crypto device support

[Longpeng (Mike)]

Hi Martin,

On 2017/2/7 19:11, Martin Kletzander wrote:

> On Wed, Jan 11, 2017 at 04:28:23PM +0800, Longpeng(Mike) wrote:
>> This patch documents XML elements used for support of virtual
...
>> +  type
>> +  
>> +
>> +The required type element specifies the
>> +type of the crypto device.
> 
> What types are possible?  Only builtin?  That should be specified here.
> Also "builtin" is very non-descriptive.
> 


QEMU cryptodev only support builtin currently, QEMU could adds other
types of cryptodev backend in the future.

I will specify this and describe "builtin" in V3.

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


-- 
Regards,
Longpeng(Mike)

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

Re: [libvirt] [PATCH v2 0/3] libxl: tunnelled migration support

[Joao Martins]

On 02/08/2017 12:35 AM, Joao Martins wrote:
> Hey!
> 
> Presented herewith is take 2 from tunnelled migration addressing all previous
> comments. Changelog in individual patches (patch 1 and 2 are small 
> refactorings
> suggested in v1) Despite being functional changes mostly I had a quick round 
> of
> testing too.
Argh, My apologies. It seems I chained reply all patches by mistake - I can
respin if folks prefer.

Joao

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

[libvirt] [PATCH v2 3/3] libxl: add tunnelled migration support

[Joao Martins]

From: Bob Liu 

Tunnelled migration doesn't require any extra network connections beside
the libvirt daemon.  It's capable of strong encryption and the default
option of openstack-nova.

This patch adds the tunnelled migration(Tunnel3params) support to libxl.
On the source side, the data flow is:

 * libxlDoMigrateSend() -> pipe libxlTunnel3MigrationFunc() polls pipe
 * out and then write to dest stream.

While on the destination side:
 * Stream -> pipe -> 'recvfd of libxlDomainStartRestore'

The usage is the same as p2p migration, execpt adding one extra
'--tunnelled' to the libvirt p2p migration command.

Signed-off-by: Bob Liu 
Signed-off-by: Joao Martins 
---
v2 (comments from Jim):
 * Adjust common preparetunnel function reflecting v1 suggestions
   - Using common top-level Prepare function by adding is_tunnel variable
   - Using libxl_migration PrepareAny added in patch 1
   - virHook support in PrepareTunnel3
 * Move virThreadPtr from libxlTunnelMigrationThread into libxlTunnelControl
 * Rename function local variable from tmThreadPtr to arg
 * Remove redundant assignment "tmThreadPtr = >tmThread;" always being not
 null.
---
 src/libxl/libxl_driver.c|  49 ++--
 src/libxl/libxl_migration.c | 280 +---
 src/libxl/libxl_migration.h |   9 ++
 3 files changed, 313 insertions(+), 25 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 7bc8adf..b34f120 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5938,14 +5938,14 @@ libxlDomainMigratePrepareCommon(virConnectPtr dconn,
 char **cookieout ATTRIBUTE_UNUSED,
 int *cookieoutlen ATTRIBUTE_UNUSED,
 unsigned int flags,
-void *data)
+void *data,
+bool is_tunnel)
 {
 libxlDriverPrivatePtr driver = dconn->privateData;
 virDomainDefPtr def = NULL;
 const char *dom_xml = NULL;
 const char *dname = NULL;
 const char *uri_in = NULL;
-char **uri_out = data;
 
 #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
 virReportUnsupportedError();
@@ -5971,12 +5971,25 @@ libxlDomainMigratePrepareCommon(virConnectPtr dconn,
 if (!(def = libxlDomainMigrationPrepareDef(driver, dom_xml, dname)))
 goto error;
 
-if (virDomainMigratePrepare3ParamsEnsureACL(dconn, def) < 0)
-goto error;
+if (is_tunnel) {
+virStreamPtr st = data;
 
-if (libxlDomainMigrationPrepare(dconn, , uri_in, uri_out,
-cookiein, cookieinlen, flags) < 0)
-goto error;
+if (virDomainMigratePrepareTunnel3ParamsEnsureACL(dconn, def) < 0)
+goto error;
+
+if (libxlDomainMigrationPrepareTunnel3(dconn, st, , cookiein,
+   cookieinlen, flags) < 0)
+goto error;
+} else {
+char **uri_out = data;
+
+if (virDomainMigratePrepare3ParamsEnsureACL(dconn, def) < 0)
+goto error;
+
+if (libxlDomainMigrationPrepare(dconn, , uri_in, uri_out,
+cookiein, cookieinlen, flags) < 0)
+goto error;
+}
 
 return 0;
 
@@ -5999,7 +6012,24 @@ libxlDomainMigratePrepare3Params(virConnectPtr dconn,
 return libxlDomainMigratePrepareCommon(dconn, params, nparams,
cookiein, cookieinlen,
cookieout, cookieoutlen,
-   flags, uri_out);
+   flags, uri_out, false);
+}
+
+static int
+libxlDomainMigratePrepareTunnel3Params(virConnectPtr dconn,
+   virStreamPtr st,
+   virTypedParameterPtr params,
+   int nparams,
+   const char *cookiein,
+   int cookieinlen,
+   char **cookieout ATTRIBUTE_UNUSED,
+   int *cookieoutlen ATTRIBUTE_UNUSED,
+   unsigned int flags)
+{
+return libxlDomainMigratePrepareCommon(dconn, params, nparams,
+   cookiein, cookieinlen,
+   cookieout, cookieoutlen,
+   flags, st, true);
 }
 
 static int
@@ -6047,7 +6077,7 @@ libxlDomainMigratePerform3Params(virDomainPtr dom,
 if (virDomainMigratePerform3ParamsEnsureACL(dom->conn, vm->def) < 0)
 goto cleanup;
 
-if (flags & VIR_MIGRATE_PEER2PEER) {
+if ((flags & (VIR_MIGRATE_TUNNELLED | VIR_MIGRATE_PEER2PEER))) {
 if (libxlDomainMigrationPerformP2P(driver, vm, 

[libvirt] [PATCH v2 1/3] libxl: refactor libxlDomainMigrationPrepare

[Joao Martins]

The newly introduced function libxlDomainMigrationPrepareAny
will be shared between P2P and tunnelled variations.

Signed-off-by: Joao Martins 
---
New in v2
---
 src/libxl/libxl_migration.c | 92 +++--
 1 file changed, 56 insertions(+), 36 deletions(-)

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index a471d2a..7beabd2 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -483,41 +483,26 @@ libxlDomainMigrationPrepareDef(libxlDriverPrivatePtr 
driver,
 return def;
 }
 
-int
-libxlDomainMigrationPrepare(virConnectPtr dconn,
-virDomainDefPtr *def,
-const char *uri_in,
-char **uri_out,
-const char *cookiein,
-int cookieinlen,
-unsigned int flags)
+static int
+libxlDomainMigrationPrepareAny(virConnectPtr dconn,
+   virDomainDefPtr *def,
+   const char *cookiein,
+   int cookieinlen,
+   libxlMigrationCookiePtr *mig,
+   char **xmlout,
+   bool *taint_hook)
 {
 libxlDriverPrivatePtr driver = dconn->privateData;
 libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
-libxlMigrationCookiePtr mig = NULL;
-virDomainObjPtr vm = NULL;
-char *hostname = NULL;
-char *xmlout = NULL;
-unsigned short port;
-char portstr[100];
-virURIPtr uri = NULL;
-virNetSocketPtr *socks = NULL;
-size_t nsocks = 0;
-int nsocks_listen = 0;
-libxlMigrationDstArgs *args = NULL;
-bool taint_hook = false;
-libxlDomainObjPrivatePtr priv = NULL;
-size_t i;
-int ret = -1;
 
-if (libxlMigrationEatCookie(cookiein, cookieinlen, ) < 0)
-goto error;
+if (libxlMigrationEatCookie(cookiein, cookieinlen, mig) < 0)
+return -1;
 
-if (mig->xenMigStreamVer > LIBXL_SAVE_VERSION) {
+if ((*mig)->xenMigStreamVer > LIBXL_SAVE_VERSION) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("Xen migration stream version '%d' is not supported 
on this host"),
-   mig->xenMigStreamVer);
-goto error;
+   (*mig)->xenMigStreamVer);
+return -1;
 }
 
 /* Let migration hook filter domain XML */
@@ -528,29 +513,29 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
 if (!(xml = virDomainDefFormat(*def, cfg->caps,
VIR_DOMAIN_XML_SECURE |
VIR_DOMAIN_XML_MIGRATABLE)))
-goto error;
+return -1;
 
 hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, (*def)->name,
   VIR_HOOK_LIBXL_OP_MIGRATE, VIR_HOOK_SUBOP_BEGIN,
-  NULL, xml, );
+  NULL, xml, xmlout);
 VIR_FREE(xml);
 
 if (hookret < 0) {
-goto error;
+return -1;
 } else if (hookret == 0) {
-if (virStringIsEmpty(xmlout)) {
+if (virStringIsEmpty(*xmlout)) {
 VIR_DEBUG("Migrate hook filter returned nothing; using the"
   " original XML");
 } else {
 virDomainDefPtr newdef;
 
-VIR_DEBUG("Using hook-filtered domain XML: %s", xmlout);
-newdef = virDomainDefParseString(xmlout, cfg->caps, 
driver->xmlopt,
+VIR_DEBUG("Using hook-filtered domain XML: %s", *xmlout);
+newdef = virDomainDefParseString(*xmlout, cfg->caps, 
driver->xmlopt,
  NULL,
  VIR_DOMAIN_DEF_PARSE_INACTIVE 
|
  
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE);
 if (!newdef)
-goto error;
+return -1;
 
 /* TODO At some stage we will want to have some check of what 
the user
  * did in the hook. */
@@ -560,17 +545,52 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
 /* We should taint the domain here. However, @vm and therefore
  * privateData too are still NULL, so just notice the fact and
  * taint it later. */
-taint_hook = true;
+*taint_hook = true;
 }
 }
 }
 
+return 0;
+}
+
+int
+libxlDomainMigrationPrepare(virConnectPtr dconn,
+virDomainDefPtr *def,
+const char *uri_in,
+char **uri_out,
+const char *cookiein,
+int cookieinlen,
+unsigned int 

[libvirt] [PATCH v2 2/3] libxl: streamline top-level migrate functions

[Joao Martins]

This allows us to reuse a single function for both tunnelled and
non-tunnelled variants.

Signed-off-by: Joao Martins 
---
New in v2
---
 src/libxl/libxl_driver.c | 36 +++-
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 3a69720..7bc8adf 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5930,21 +5930,22 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
 }
 
 static int
-libxlDomainMigratePrepare3Params(virConnectPtr dconn,
- virTypedParameterPtr params,
- int nparams,
- const char *cookiein,
- int cookieinlen,
- char **cookieout ATTRIBUTE_UNUSED,
- int *cookieoutlen ATTRIBUTE_UNUSED,
- char **uri_out,
- unsigned int flags)
+libxlDomainMigratePrepareCommon(virConnectPtr dconn,
+virTypedParameterPtr params,
+int nparams,
+const char *cookiein,
+int cookieinlen,
+char **cookieout ATTRIBUTE_UNUSED,
+int *cookieoutlen ATTRIBUTE_UNUSED,
+unsigned int flags,
+void *data)
 {
 libxlDriverPrivatePtr driver = dconn->privateData;
 virDomainDefPtr def = NULL;
 const char *dom_xml = NULL;
 const char *dname = NULL;
 const char *uri_in = NULL;
+char **uri_out = data;
 
 #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
 virReportUnsupportedError();
@@ -5985,6 +5986,23 @@ libxlDomainMigratePrepare3Params(virConnectPtr dconn,
 }
 
 static int
+libxlDomainMigratePrepare3Params(virConnectPtr dconn,
+ virTypedParameterPtr params,
+ int nparams,
+ const char *cookiein,
+ int cookieinlen,
+ char **cookieout ATTRIBUTE_UNUSED,
+ int *cookieoutlen ATTRIBUTE_UNUSED,
+ char **uri_out,
+ unsigned int flags)
+{
+return libxlDomainMigratePrepareCommon(dconn, params, nparams,
+   cookiein, cookieinlen,
+   cookieout, cookieoutlen,
+   flags, uri_out);
+}
+
+static int
 libxlDomainMigratePerform3Params(virDomainPtr dom,
  const char *dconnuri,
  virTypedParameterPtr params,
-- 
2.1.4

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

[libvirt] [PATCH v2 0/3] libxl: tunnelled migration support

[Joao Martins]

Hey!

Presented herewith is take 2 from tunnelled migration addressing all previous
comments. Changelog in individual patches (patch 1 and 2 are small refactorings
suggested in v1) Despite being functional changes mostly I had a quick round of
testing too.

Thanks,
Joao

Bob Liu (1):
  libxl: add tunnelled migration support

Joao Martins (2):
  libxl: refactor libxlDomainMigrationPrepare
  libxl: streamline top-level migrate functions

 src/libxl/libxl_driver.c|  79 --
 src/libxl/libxl_migration.c | 372 +---
 src/libxl/libxl_migration.h |   9 ++
 3 files changed, 393 insertions(+), 67 deletions(-)

-- 
2.1.4

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

Re: [libvirt] [PATCH] libxl: fix disk detach when not specified

[Jim Fehlig]

On 02/07/2017 08:30 AM, Jim Fehlig wrote:

On 02/07/2017 03:26 AM, Michal Privoznik wrote:

On 02/06/2017 11:08 PM, Jim Fehlig wrote:

When a user does not explicitly set a  in the disk config,
libvirt defers selection of a default to libxl. This approach works
fine when starting a domain with such configuration or attaching a
disk to a running domain. But when detaching such a disk, libxl
will fail with "unrecognized disk backend type: 0". libxl makes no
attempt to recalculate a default backend (driver) on detach and
simply fails when uninitialized.

This patch updates the libvirt disk config with the backend selected
by libxl when starting a domain or attaching a disk to a running
domain. Another benefit of this approach is that the live XML is
also updated with the backend driver selected by libxl.

Signed-off-by: Jim Fehlig 
---
 src/libxl/libxl_conf.c   | 33 +
 src/libxl/libxl_conf.h   |  4 
 src/libxl/libxl_domain.c | 25 +
 src/libxl/libxl_driver.c |  2 +-
 4 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b5186f2..6ef7570 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -851,6 +851,7 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk,
libxl_device_disk *x_disk)
  * xl-disk-configuration.txt in the xen documentation and let
  * libxl pick a suitable backend.
  */
+virDomainDiskSetFormat(l_disk, VIR_STORAGE_FILE_RAW);
 x_disk->format = LIBXL_DISK_FORMAT_RAW;
 x_disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;


This doesn't feel right. I know what do you want it here for, but this
function is meant to take 'const' disk definition and translate it to
libxl definition. Although, I don't have a better suggestion where to
put it.


How about in the UpdateDisk function introduced in this patch? E.g. update it to
raw if current format is VIR_STORAGE_FILE_NONE?


After thinking about it some more, seems the best place to set the default when 
format is VIR_STORAGE_FILE_NONE is in the device post-parse callback. I've sent 
a V2 along those lines


https://www.redhat.com/archives/libvir-list/2017-February/msg00248.html

Regards,
Jim

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

[libvirt] [PATCH V2 1/2] libxl: set default disk format in device post-parse

[Jim Fehlig]

When starting a domian, a libxl_domain_config object is created from
virDomainDef. Any virDomainDiskDef devices with a format of
VIR_STORAGE_FILE_NONE are mapped to LIBXL_DISK_FORMAT_RAW in the
corresponding libxl_disk_device, but the virDomainDiskDef format is
never updated to reflect the change.

A better place to set a default format for disk devices is the
device post-parse callback, ensuring the virDomainDiskDef object
reflects the default format.

Signed-off-by: Jim Fehlig 
---
 src/libxl/libxl_conf.c   | 10 ++
 src/libxl/libxl_domain.c |  7 ++-
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b5186f2..da63105 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -765,8 +765,6 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk 
*x_disk)
 x_disk->format = LIBXL_DISK_FORMAT_VHD;
 x_disk->backend = LIBXL_DISK_BACKEND_TAP;
 break;
-case VIR_STORAGE_FILE_NONE:
-/* No subtype specified, default to raw/tap */
 case VIR_STORAGE_FILE_RAW:
 x_disk->format = LIBXL_DISK_FORMAT_RAW;
 x_disk->backend = LIBXL_DISK_BACKEND_TAP;
@@ -802,8 +800,6 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk 
*x_disk)
 case VIR_STORAGE_FILE_VHD:
 x_disk->format = LIBXL_DISK_FORMAT_VHD;
 break;
-case VIR_STORAGE_FILE_NONE:
-/* No subtype specified, default to raw */
 case VIR_STORAGE_FILE_RAW:
 x_disk->format = LIBXL_DISK_FORMAT_RAW;
 break;
@@ -816,8 +812,7 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk 
*x_disk)
 return -1;
 }
 } else if (STREQ(driver, "file")) {
-if (format != VIR_STORAGE_FILE_NONE &&
-format != VIR_STORAGE_FILE_RAW) {
+if (format != VIR_STORAGE_FILE_RAW) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("libxenlight does not support disk format %s "
  "with disk driver %s"),
@@ -828,8 +823,7 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk 
*x_disk)
 x_disk->format = LIBXL_DISK_FORMAT_RAW;
 x_disk->backend = LIBXL_DISK_BACKEND_QDISK;
 } else if (STREQ(driver, "phy")) {
-if (format != VIR_STORAGE_FILE_NONE &&
-format != VIR_STORAGE_FILE_RAW) {
+if (format != VIR_STORAGE_FILE_RAW) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("libxenlight does not support disk format %s "
  "with disk driver %s"),
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index ed73cd2..c0ace33 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -362,16 +362,21 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
 }
 }
 
-/* for network-based disks, set 'qemu' as the default driver */
 if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
 virDomainDiskDefPtr disk = dev->data.disk;
 int actual_type = virStorageSourceGetActualType(disk->src);
+int format = virDomainDiskGetFormat(disk);
 
+/* for network-based disks, set 'qemu' as the default driver */
 if (actual_type == VIR_STORAGE_TYPE_NETWORK) {
 if (!virDomainDiskGetDriver(disk) &&
 virDomainDiskSetDriver(disk, "qemu") < 0)
 return -1;
 }
+
+/* xl.cfg default format is raw. See xl-disk-configuration(5) */
+if (format == VIR_STORAGE_FILE_NONE)
+virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW);
 }
 
 return 0;
-- 
2.9.2

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

[libvirt] [PATCH V2 0/2] libxl: fix handling of driver and format settings

[Jim Fehlig]

V2 of

https://www.redhat.com/archives/libvir-list/2017-February/msg00185.html

Patch1 is new in V2 and essentially moves default setting of disk
format from domain build time to device post-parse, which addresses
Michal's comment from V1.

Patch2 is mostly unchanged. The hunk setting format of virDomainDiskDef
in libxlMakeDisk is removed since that is now handled by patch1.

Jim Fehlig (2):
  libxl: set default disk format in device post-parse
  libxl: fix disk detach when  not specified

 src/libxl/libxl_conf.c   | 42 ++
 src/libxl/libxl_conf.h   |  4 
 src/libxl/libxl_domain.c | 32 +++-
 src/libxl/libxl_driver.c |  1 +
 4 files changed, 70 insertions(+), 9 deletions(-)

-- 
2.9.2

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

[libvirt] [PATCH V2 2/2] libxl: fix disk detach when not specified

[Jim Fehlig]

When a user does not explicitly set a  in the disk config,
libvirt defers selection of a default to libxl. This approach works
fine when starting a domain with such configuration or attaching a
disk to a running domain. But when detaching such a disk, libxl
will fail with "unrecognized disk backend type: 0". libxl makes no
attempt to recalculate a default backend (driver) on detach and
simply fails when uninitialized.

This patch updates the libvirt disk config with the backend selected
by libxl when starting a domain or attaching a disk to a running
domain. Another benefit of this approach is that the live XML is
also updated with the backend driver selected by libxl.

Signed-off-by: Jim Fehlig 
---
 src/libxl/libxl_conf.c   | 32 
 src/libxl/libxl_conf.h   |  4 
 src/libxl/libxl_domain.c | 25 +
 src/libxl/libxl_driver.c |  1 +
 4 files changed, 62 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index da63105..4e38676 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -907,6 +907,38 @@ libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config 
*d_config)
 return -1;
 }
 
+/*
+ * Update libvirt disk config with libxl disk config.
+ *
+ * This function can be used to update the libvirt disk config with default
+ * values selected by libxl. Currently only the backend type is selected by
+ * libxl when not explicitly specified by the user.
+ */
+void
+libxlUpdateDiskDef(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk)
+{
+const char *driver = NULL;
+
+if (virDomainDiskGetDriver(l_disk))
+return;
+
+switch (x_disk->backend) {
+case LIBXL_DISK_BACKEND_QDISK:
+driver = "qemu";
+break;
+case LIBXL_DISK_BACKEND_TAP:
+driver = "tap";
+break;
+case LIBXL_DISK_BACKEND_PHY:
+driver = "phy";
+break;
+case LIBXL_DISK_BACKEND_UNKNOWN:
+break;
+}
+if (driver)
+ignore_value(virDomainDiskSetDriver(l_disk, driver));
+}
+
 int
 libxlMakeNic(virDomainDefPtr def,
  virDomainNetDefPtr l_nic,
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 69d7885..b944f6c 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -175,6 +175,10 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
 
 int
 libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
+
+void
+libxlUpdateDiskDef(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
+
 int
 libxlMakeNic(virDomainDefPtr def,
  virDomainNetDefPtr l_nic,
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index c0ace33..57ec661 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1072,6 +1072,30 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, 
libxl_domain_config *d_config)
 }
 }
 
+static void
+libxlDomainUpdateDiskParams(virDomainDefPtr def, libxl_ctx *ctx)
+{
+libxl_device_disk *disks;
+int num_disks = 0;
+size_t i;
+int idx;
+
+disks = libxl_device_disk_list(ctx, def->id, _disks);
+if (!disks)
+return;
+
+for (i = 0; i < num_disks; i++) {
+if ((idx = virDomainDiskIndexByName(def, disks[i].vdev, false)) < 0)
+continue;
+
+libxlUpdateDiskDef(def->disks[idx], [i]);
+}
+
+for (i = 0; i < num_disks; i++)
+libxl_device_disk_dispose([i]);
+VIR_FREE(disks);
+}
+
 #ifdef LIBXL_HAVE_DEVICE_CHANNEL
 static void
 libxlDomainCreateChannelPTY(virDomainDefPtr def, libxl_ctx *ctx)
@@ -1315,6 +1339,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
 goto destroy_dom;
 
 libxlDomainCreateIfaceNames(vm->def, _config);
+libxlDomainUpdateDiskParams(vm->def, cfg->ctx);
 
 #ifdef LIBXL_HAVE_DEVICE_CHANNEL
 if (vm->def->nchannels > 0)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 3a69720..e7827cc 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3028,6 +3028,7 @@ libxlDomainAttachDeviceDiskLive(virDomainObjPtr vm, 
virDomainDeviceDefPtr dev)
 goto cleanup;
 }
 
+libxlUpdateDiskDef(l_disk, _disk);
 virDomainDiskInsertPreAlloced(vm->def, l_disk);
 
 } else {
-- 
2.9.2

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

Re: [libvirt] [RFC PATCH 00/16] Introduce vGPU mdev framework to libvirt

[Alex Williamson]

On Tue, 7 Feb 2017 17:26:51 +0100
Erik Skultety  wrote:

> On Mon, Feb 06, 2017 at 09:33:14AM -0700, Alex Williamson wrote:
> > On Mon,  6 Feb 2017 13:19:42 +0100
> > Erik Skultety  wrote:
> >   
> > > Finally. It's here. This is the initial suggestion on how libvirt might
> > > interract with the mdev framework, currently only focussing on the 
> > > non-managed
> > > devices, i.e. those pre-created by the user, since that will be revisited 
> > > once
> > > we all settled on how the XML should look like, given we might not want 
> > > to use
> > > the sysfs path directly as an attribute in the domain XML. My proposal on 
> > > the
> > > XML is the following:
> > > 
> > >   
> > > 
> > > 
> > > 
> > > vGPU_UUID
> > > 
> > > 
> > > 
> > > 
> > > So the mediated device is identified by the physical parent device 
> > > visible on
> > > the host and a UUID which allows us to construct the sysfs path by 
> > > ourselves,
> > > which we then put on the QEMU's command line.  
> > 
> > Based on your test code, I think you're creating something like this:
> > 
> > -device 
> > vfio-pci,sysfsdev=/sys/class/mdev_bus/:00:03.0/53764d0e-85a0-42b4-af5c-2046b460b1dc
> > 
> > That would explain the need for the parent device address, but that's
> > an entirely self inflicted requirement.  For a managed="no" scenarios,
> > we shouldn't need the parent, we can get to the mdev device
> > via /sys/bus/mdev/devices/53764d0e-85a0-42b4-af5c-2046b460b1dc.  So it  
> 
> True, for managed="no" would this path be a nice optimization.
> 
> > seems that the UUID should be the only required source element for
> > managed="no".
> > 
> > For managed="yes", it seems like the parent device is still an optional  
> 
> The reason I went with the parent address element (and purposely neglecting 
> the
> sample mtty driver) was that I assumed any modern mdev capable HW would be
> accessible through the PCI bus on the host. Also I wanted to explicitly hint
> libvirt as much as possible which parent device a vGPU device instance should
> be created on in case there are more than one of them, rather then scanning
> sysfs for a suitable parent which actually supports the given vGPU type.
> 
> > field.  The most important thing that libvirt needs to know when
> > creating a mdev device for a VM is the mdev type name.  The parent
> > device should be an optional field to help higher level management
> > tools deal with placement of the device for locality or load balancing.
> > Also, we can't assume that the parent device is a PCI device, the
> > sample mtty driver already breaks this assumption.  
> 
> Since we need to assume non-PCI devices and we still need to enable management
> to hint libvirt about the parent to utilize load balancing and stuff, I've 
> come
> up with the following adjustments/ideas on how to reflect that in the XML:
> - still use the address element but use it with the 'type' attribute [1] 
> (still
>   breaks the sample mtty driver though) while making the element truly 
> optional
>   if I'm going to be outvoted in favor of scanning the directory for a 
> suitable
>   parent device on our own, rather than requiring the user to provide that
> 
> - providing either an attribute or a standalone element for the parent device
>   name, like a string version of the PCI address or whatever form the parent
>   device comes in (doesn't break the mtty driver but I don't quite like this)
> 
> - providing a path element/attribute to sysfs pointing to the parent device
>   which I'm afraid is what Daniel is not in favor of libvirt doing
> 
> So, this is what I've so far come up with in terms of hinting libvirt about 
> the
> parent device, do you have any input on this, maybe some more ideas on how we
> should identify the parent device?

IMO, if we cannot account for the mtty sample driver, we're doing it
wrong.  I suppose we can leave it unspecified how one selects a parent
device for the mtty driver, but it should be possible to expand the
syntax to include it.  So I think that means that when the parent
address is provided, the parent address type needs to be specified as
PCI.  So...

 

This needs to encompass the device API or else the optional VM address
cannot be resolved.  Perhaps model='vfio-pci' here?  Seems similar to
how we specify the device type for PCI controllers where we have
multiple options:

 

   

For managed='no', I don't see that anything other than the mdev UUID is
useful.

 MDEV_UUID

If libvirt gets into the business of creating mdev devices and we call
that managed='yes', then the mdev type to create is required.  I don't
know whether there's anything similar we can steal syntax from:

 "nvidia-11"

That's pretty horrible, needs some xml guru love.

We need to provide for specifying a parent, but we can't assume the
type and address format of the parent.  If we say the parent is a
string, then we don't care, libvirt 

Re: [libvirt] [PATCH v3] qemu: Forbid without

[Andrea Bolognani]

On Tue, 2017-02-07 at 18:05 +0100, Pavel Hrdina wrote:
> ACK

Pushed, thanks :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [RFC PATCH 00/16] Introduce vGPU mdev framework to libvirt

[Daniel P. Berrange]

On Tue, Feb 07, 2017 at 05:48:23PM +0100, Erik Skultety wrote:
> On Mon, Feb 06, 2017 at 04:44:37PM +, Daniel P. Berrange wrote:
> > On Mon, Feb 06, 2017 at 01:19:42PM +0100, Erik Skultety wrote:
> > > Finally. It's here. This is the initial suggestion on how libvirt might
> > > interract with the mdev framework, currently only focussing on the 
> > > non-managed
> > > devices, i.e. those pre-created by the user, since that will be revisited 
> > > once
> > > we all settled on how the XML should look like, given we might not want 
> > > to use
> > > the sysfs path directly as an attribute in the domain XML. My proposal on 
> > > the
> > > XML is the following:
> > > 
> > >   
> > > 
> > > 
> > > 
> > > vGPU_UUID
> > > 
> > > 
> > > 
> > > 
> > > So the mediated device is identified by the physical parent device 
> > > visible on
> > > the host and a UUID which allows us to construct the sysfs path by 
> > > ourselves,
> > > which we then put on the QEMU's command line.
> > > 
> > > A few remarks if you actually happen to have a machine to test this on:
> > > - right now the mediated devices are one-time use only, i.e. they have to 
> > > be
> > > recreated before every machine boot
> > > - I wouldn't recommend assigning multiple vGPUs to a single domain
> > > 
> > > Once this series is sorted out, we can then continue with 'managed=yes' 
> > > where
> > > as Laine pointed out [1], we need to figure out how exactly should the
> > > management layer hint libvirt which vGPU type should be used for device
> > > instantiation.
> > 
> > You seem to be suggesting that managed=yes with mdev devices would
> > cause create / delete of a mdev device from a specified parent.
> > 
> > This is rather different semantics from what managed=yes does with
> > PCI device assignment today.  There the managed=yes flag is just
> > about controlling host device driver attachment. ie whether libvirt
> > will manually bind to vfio.ko, or expect the admin to have bound
> > it to vfio.ko before hand. I think it is important to keep that
> > concept as is for mdev too.
> 
> If the managed attribute was used with other devices beside PCI, then sure, we
> should keep the concept, however, since only PCI devices support it and now we
> have another device type that potentially might have a use for such an
> attribute I think it's perfectly reasonable to alter the logic behind that
> attribute in favor of the new possibilities to device management which mdev
> framework is providing us with which in this case is dynamic creation and
> removal of a mediated device.

No we really shouldn't use one attribute to overload completely
different semantics. As I say, we may well find we want to implement
the existing PCI semantics for mdev devices too.

If we want to auto-create, that should be a different attribute
eg 'autocreate=yes|no'

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] [PATCH v3] qemu: Forbid without

[Pavel Hrdina]

On Tue, Feb 07, 2017 at 02:16:49PM +0100, Andrea Bolognani wrote:
> In order for memory locking to work, the hard limit on memory
> locking (and usage) has to be set appropriately by the user.
> 
> The documentation mentions the requirement already: with this
> patch, it's going to be enforced by runtime checks as well,
> by forbidding a non-compliant guest from being defined as well
> as edited and started.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1316774
> ---
> Changes from [v2]
> 
>   * Address review feedback:
> - move the check from BuildCommandLine to Validate.
> 
> Changes from [v1]
> 
>   * Address review feeback:
> - check in BuildCommandLine rather than in PostParse,
>   so that non-compliant guests will merely fail to
>   start rather than disappear completely.
> 
> [v1] https://www.redhat.com/archives/libvir-list/2017-February/msg00180.html
> [v2] https://www.redhat.com/archives/libvir-list/2017-February/msg00214.html
> 
>  src/qemu/qemu_domain.c   | 10 ++
>  tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml |  3 +++
>  2 files changed, 13 insertions(+)

ACK

Pavel


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

Re: [libvirt] [PATCH v2 4/4] qemu: propagate bridge MTU into qemu "host_mtu" option

[Maxime Coquelin]

On 02/06/2017 01:57 PM, Michal Privoznik wrote:

On 03.02.2017 18:35, Laine Stump wrote:

libvirt was able to set the host_mtu option when an MTU was explicitly
given in the interface config (with ), set the MTU of a
libvirt network in the network config (with the same named
subelement), and would automatically set the MTU of any tap device to
the MTU of the network.

This patch ties that all together (for networks based on tap devices
and either Linux host bridges or OVS bridges) by learning the MTU of
the network (i.e. the bridge) during qemuInterfaceBridgeConnect(), and
returning that value so that it can be passed to qemuBuildNicDevStr();
qemuBuildNicDevStr() then sets host_mtu in the interface's commandline
options.

The result is that a higher MTU for all guests connecting to a
particular network will be plumbed top to bottom by simply changing
the MTU of the network (in libvirt's config for libvirt-managed
networks, or directly on the bridge device for simple host bridges or
OVS bridges managed outside of libvirt).

One question I have about this - it occurred to me that in the case of
migrating a guest from a host with an older libvirt to one with a
newer libvirt, the guest may have *not* had the host_mtu option on the
older machine, but *will* have it on the newer machine. I'm curious if
this could lead to incompatibilities between source and destination (I
guess it all depends on whether or not the setting of host_mtu has a
practical effect on a guest that is already running - Maxime?) (I hope
we don't have to add a "" and only set host_mtu when
that is present :-/)


This is a question for qemu folks. I know nothing about qemu internals.


Sorry for the late reply.

Setting host_mtu on a guest that is already running will have no effect.
Indeed, the VIRTIO_NET_F_MTU feature flag will be set at device
negotiation time if host_mtu is set.

So, if guest started without this host_mtu parameter, this flag won't
be set and the value won't be taken into account.

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

Re: [libvirt] [RFC PATCH 00/16] Introduce vGPU mdev framework to libvirt

[Erik Skultety]

On Mon, Feb 06, 2017 at 04:44:37PM +, Daniel P. Berrange wrote:
> On Mon, Feb 06, 2017 at 01:19:42PM +0100, Erik Skultety wrote:
> > Finally. It's here. This is the initial suggestion on how libvirt might
> > interract with the mdev framework, currently only focussing on the 
> > non-managed
> > devices, i.e. those pre-created by the user, since that will be revisited 
> > once
> > we all settled on how the XML should look like, given we might not want to 
> > use
> > the sysfs path directly as an attribute in the domain XML. My proposal on 
> > the
> > XML is the following:
> > 
> >   
> > 
> > 
> > 
> > vGPU_UUID
> > 
> > 
> > 
> > 
> > So the mediated device is identified by the physical parent device visible 
> > on
> > the host and a UUID which allows us to construct the sysfs path by 
> > ourselves,
> > which we then put on the QEMU's command line.
> > 
> > A few remarks if you actually happen to have a machine to test this on:
> > - right now the mediated devices are one-time use only, i.e. they have to be
> > recreated before every machine boot
> > - I wouldn't recommend assigning multiple vGPUs to a single domain
> > 
> > Once this series is sorted out, we can then continue with 'managed=yes' 
> > where
> > as Laine pointed out [1], we need to figure out how exactly should the
> > management layer hint libvirt which vGPU type should be used for device
> > instantiation.
> 
> You seem to be suggesting that managed=yes with mdev devices would
> cause create / delete of a mdev device from a specified parent.
> 
> This is rather different semantics from what managed=yes does with
> PCI device assignment today.  There the managed=yes flag is just
> about controlling host device driver attachment. ie whether libvirt
> will manually bind to vfio.ko, or expect the admin to have bound
> it to vfio.ko before hand. I think it is important to keep that
> concept as is for mdev too.

If the managed attribute was used with other devices beside PCI, then sure, we
should keep the concept, however, since only PCI devices support it and now we
have another device type that potentially might have a use for such an
attribute I think it's perfectly reasonable to alter the logic behind that
attribute in favor of the new possibilities to device management which mdev
framework is providing us with which in this case is dynamic creation and
removal of a mediated device.

> 
> While we're thinking of mdev purely in terms of KVM + vfio usage,
> it wouldn't suprise me if there ended up being non-KVM based
> use cases for mdev.
> 
> It isn't clear to me that auto-creation of mdev devices as a concept
> even belongs in the domain XML neccessarily.
> 
> Looking at two similar areas. For SRIOV NICs, in the domain XML
> you either specify an explicit VF to use, or you reference a
> libvirt virtual network. The latter takes care of dynamically
> providing VFs to VMs.  For NPIV, IIRC, the domain XML works
> similarly either taking an explicit vHBA, or referencing a
> storage pool to get one more dynamically.
> 
> Before we even consider auto-creation though, I think we need
> to have manual creation designed & integrated in the node device
> APIs.
> 

Yes, integrating mdev into the nodedev driver this way ^^ is definitely planned.

> So in terms of the domain XML, I think the only think we need
> to provide is the address of the pre-existing mdev device
> to be used. In this case "address" means the UUID. We should
> not need anything about the parent device AFAICT.
> 
> 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] [RFC PATCH 00/16] Introduce vGPU mdev framework to libvirt

[Erik Skultety]

On Mon, Feb 06, 2017 at 09:33:14AM -0700, Alex Williamson wrote:
> On Mon,  6 Feb 2017 13:19:42 +0100
> Erik Skultety  wrote:
> 
> > Finally. It's here. This is the initial suggestion on how libvirt might
> > interract with the mdev framework, currently only focussing on the 
> > non-managed
> > devices, i.e. those pre-created by the user, since that will be revisited 
> > once
> > we all settled on how the XML should look like, given we might not want to 
> > use
> > the sysfs path directly as an attribute in the domain XML. My proposal on 
> > the
> > XML is the following:
> > 
> >   
> > 
> > 
> > 
> > vGPU_UUID
> > 
> > 
> > 
> > 
> > So the mediated device is identified by the physical parent device visible 
> > on
> > the host and a UUID which allows us to construct the sysfs path by 
> > ourselves,
> > which we then put on the QEMU's command line.
> 
> Based on your test code, I think you're creating something like this:
> 
> -device 
> vfio-pci,sysfsdev=/sys/class/mdev_bus/:00:03.0/53764d0e-85a0-42b4-af5c-2046b460b1dc
> 
> That would explain the need for the parent device address, but that's
> an entirely self inflicted requirement.  For a managed="no" scenarios,
> we shouldn't need the parent, we can get to the mdev device
> via /sys/bus/mdev/devices/53764d0e-85a0-42b4-af5c-2046b460b1dc.  So it

True, for managed="no" would this path be a nice optimization.

> seems that the UUID should be the only required source element for
> managed="no".
> 
> For managed="yes", it seems like the parent device is still an optional

The reason I went with the parent address element (and purposely neglecting the
sample mtty driver) was that I assumed any modern mdev capable HW would be
accessible through the PCI bus on the host. Also I wanted to explicitly hint
libvirt as much as possible which parent device a vGPU device instance should
be created on in case there are more than one of them, rather then scanning
sysfs for a suitable parent which actually supports the given vGPU type.

> field.  The most important thing that libvirt needs to know when
> creating a mdev device for a VM is the mdev type name.  The parent
> device should be an optional field to help higher level management
> tools deal with placement of the device for locality or load balancing.
> Also, we can't assume that the parent device is a PCI device, the
> sample mtty driver already breaks this assumption.

Since we need to assume non-PCI devices and we still need to enable management
to hint libvirt about the parent to utilize load balancing and stuff, I've come
up with the following adjustments/ideas on how to reflect that in the XML:
- still use the address element but use it with the 'type' attribute [1] (still
  breaks the sample mtty driver though) while making the element truly optional
  if I'm going to be outvoted in favor of scanning the directory for a suitable
  parent device on our own, rather than requiring the user to provide that

- providing either an attribute or a standalone element for the parent device
  name, like a string version of the PCI address or whatever form the parent
  device comes in (doesn't break the mtty driver but I don't quite like this)

- providing a path element/attribute to sysfs pointing to the parent device
  which I'm afraid is what Daniel is not in favor of libvirt doing

So, this is what I've so far come up with in terms of hinting libvirt about the
parent device, do you have any input on this, maybe some more ideas on how we
should identify the parent device?

> 
> Also, grep'ing through the patches, I don't see that the "device_api"

Yep, this was also on purpose since as you write below, right now the only
functioning mdev devices we have to work with are vfio-pci capable only, so
with this RFC I wanted to gather some feedback on whether I'm moving the right
direction in the first place. So yeah, I thought this could be added at any 
point
later.

[1] http://libvirt.org/formatdomain.html#elementsAddress

Erik

> file is being used to test that the mdev device actually exports the
> vfio-pci API before making use of it with the QEMU vfio-pci driver.  We
> don't yet have any examples to the contrary, but non vfio-pci mdev
> devices are in development.  Just like we can't assume the parent
> device type, we can't assume the API of an mdev device to the user.
> Thanks,
> 
> Alex
> 
> > A few remarks if you actually happen to have a machine to test this on:
> > - right now the mediated devices are one-time use only, i.e. they have to be
> > recreated before every machine boot
> > - I wouldn't recommend assigning multiple vGPUs to a single domain
> > 
> > Once this series is sorted out, we can then continue with 'managed=yes' 
> > where
> > as Laine pointed out [1], we need to figure out how exactly should the
> > management layer hint libvirt which vGPU type should be used for device
> > instantiation.
> > 
> > [1] 

Re: [libvirt] [PATCH] libxl: fix disk detach when not specified

[Jim Fehlig]

On 02/07/2017 03:26 AM, Michal Privoznik wrote:

On 02/06/2017 11:08 PM, Jim Fehlig wrote:

When a user does not explicitly set a  in the disk config,
libvirt defers selection of a default to libxl. This approach works
fine when starting a domain with such configuration or attaching a
disk to a running domain. But when detaching such a disk, libxl
will fail with "unrecognized disk backend type: 0". libxl makes no
attempt to recalculate a default backend (driver) on detach and
simply fails when uninitialized.

This patch updates the libvirt disk config with the backend selected
by libxl when starting a domain or attaching a disk to a running
domain. Another benefit of this approach is that the live XML is
also updated with the backend driver selected by libxl.

Signed-off-by: Jim Fehlig 
---
 src/libxl/libxl_conf.c   | 33 +
 src/libxl/libxl_conf.h   |  4 
 src/libxl/libxl_domain.c | 25 +
 src/libxl/libxl_driver.c |  2 +-
 4 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b5186f2..6ef7570 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -851,6 +851,7 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk 
*x_disk)
  * xl-disk-configuration.txt in the xen documentation and let
  * libxl pick a suitable backend.
  */
+virDomainDiskSetFormat(l_disk, VIR_STORAGE_FILE_RAW);
 x_disk->format = LIBXL_DISK_FORMAT_RAW;
 x_disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;


This doesn't feel right. I know what do you want it here for, but this
function is meant to take 'const' disk definition and translate it to
libxl definition. Although, I don't have a better suggestion where to
put it.


How about in the UpdateDisk function introduced in this patch? E.g. update it to 
raw if current format is VIR_STORAGE_FILE_NONE?


Regards,
Jim




 }
@@ -913,6 +914,38 @@ libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config 
*d_config)
 return -1;
 }



The rest looks okay. ACK.

Michal



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

Re: [libvirt] [PATCH v2] docs: mention bhyve SATA address changes in news.xml

[Roman Bogorodskiy]

  Andrea Bolognani wrote:

> On Mon, 2017-02-06 at 21:49 +0400, Roman Bogorodskiy wrote:
> [...]
> > +  However, as this doesn't match libvirt's understanding of
> > +  disk addresses, it was changed for the bhyve driver
> 
>   "it was changed for the bhyve driver"
>   → "the bhyve driver was changed"
> 
> Sorry for not catching it the first time around :/
> 
> ACK with that fixed.

Pushed with this fix included, thanks!

Roman Bogorodskiy


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

Re: [libvirt] [PATCH] bhyve: fix virtio disk addresses

[Roman Bogorodskiy]

  Andrea Bolognani wrote:

> On Wed, 2017-02-01 at 20:25 +0400, Roman Bogorodskiy wrote:
> > Like it usually happens, I fixed one thing and broke another:
> > in 803966c76 address allocation was fixed for SATA disks, but
> > broke that for virtio disks, because it dropped disk address
> > assignment completely. It's not needed for SATA disks anymore,
> > but still needed for the virtio ones.
> > 
> > Bring that back and add a couple of tests to make sure it won't
> > happen again.
> 
> I didn't actually test this[1], but both the code and the
> tests look reasonable enough, plus 'make check' and 'make
> syntax-check' are all green[2] on FreeBSD, so:
> 
> ACK

Pushed, thanks!

> [1] Can you run bhyve guests inside a FreeBSD KVM guest?

I guess that generally you cannot because it requires hardware
virtualization support (i.e. POPCNT CPU feature and maybe something
else). Probably it's possible with KVM's nested virtualization, I've
been wanting to check that for a while but somehow always forget about
that when I have time.

> [2] Well, mostly. We really should fix qemuxml2argvtest once
> and for all, and it looks like we're sooo close now!

True.

Roman Bogorodskiy


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

[libvirt] [PATCH v3 1/3] libvirtd: add openvitch timeout value

[Boris Fiuczynski]

Provide the ability to specify a default timeout value for
successful completion of openvswitch calls in the libvirtd
configuration file.

Signed-off-by: Boris Fiuczynski 
Reviewed-by: Bjoern Walk 
---
 daemon/libvirtd-config.c| 6 ++
 daemon/libvirtd-config.h| 2 ++
 daemon/libvirtd.aug | 1 +
 daemon/libvirtd.conf| 9 +
 daemon/test_libvirtd.aug.in | 1 +
 src/util/virnetdevopenvswitch.h | 1 +
 6 files changed, 20 insertions(+)

diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
index b469189..6c0f00e 100644
--- a/daemon/libvirtd-config.c
+++ b/daemon/libvirtd-config.c
@@ -32,6 +32,7 @@
 #include "configmake.h"
 #include "remote/remote_protocol.h"
 #include "remote/remote_driver.h"
+#include "util/virnetdevopenvswitch.h"
 #include "virstring.h"
 #include "virutil.h"
 
@@ -170,6 +171,8 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED)
 data->admin_keepalive_interval = 5;
 data->admin_keepalive_count = 5;
 
+data->ovs_timeout = VIR_NETDEV_OVS_DEFAULT_TIMEOUT;
+
 localhost = virGetHostname();
 if (localhost == NULL) {
 /* we couldn't resolve the hostname; assume that we are
@@ -388,6 +391,9 @@ daemonConfigLoadOptions(struct daemonConfig *data,
 if (virConfGetValueUInt(conf, "admin_keepalive_count", 
>admin_keepalive_count) < 0)
 goto error;
 
+if (virConfGetValueUInt(conf, "ovs_timeout", >ovs_timeout) < 0)
+goto error;
+
 return 0;
 
  error:
diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h
index ad3e80a..1edf5fa 100644
--- a/daemon/libvirtd-config.h
+++ b/daemon/libvirtd-config.h
@@ -92,6 +92,8 @@ struct daemonConfig {
 
 int admin_keepalive_interval;
 unsigned int admin_keepalive_count;
+
+unsigned int ovs_timeout;
 };
 
 
diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug
index 2b8df66..24fdf44 100644
--- a/daemon/libvirtd.aug
+++ b/daemon/libvirtd.aug
@@ -88,6 +88,7 @@ module Libvirtd =
 
let misc_entry = str_entry "host_uuid"
   | str_entry "host_uuid_source"
+  | int_entry "ovs_timeout"
 
(* Each enty in the config is one of the following three ... *)
let entry = network_entry
diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
index 8466616..ac77811 100644
--- a/daemon/libvirtd.conf
+++ b/daemon/libvirtd.conf
@@ -467,3 +467,12 @@
 # Keepalive settings for the admin interface
 #admin_keepalive_interval = 5
 #admin_keepalive_count = 5
+
+###
+# Open vSwitch:
+# This allows to specify a timeout for openvswitch calls made by
+# libvirt. The ovs-vsctl utility is used for the configuration and
+# its timeout option is set by default to 5 seconds to avoid
+# potential infinite waits blocking libvirts processing.
+#
+#ovs_timeout = 5
diff --git a/daemon/test_libvirtd.aug.in b/daemon/test_libvirtd.aug.in
index 1fb182c..1200952 100644
--- a/daemon/test_libvirtd.aug.in
+++ b/daemon/test_libvirtd.aug.in
@@ -63,3 +63,4 @@ module Test_libvirtd =
 { "admin_keepalive_required" = "1" }
 { "admin_keepalive_interval" = "5" }
 { "admin_keepalive_count" = "5" }
+{ "ovs_timeout" = "5" }
diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h
index 8f5faf1..01f6233 100644
--- a/src/util/virnetdevopenvswitch.h
+++ b/src/util/virnetdevopenvswitch.h
@@ -29,6 +29,7 @@
 # include "virnetdevvportprofile.h"
 # include "virnetdevvlan.h"
 
+# define VIR_NETDEV_OVS_DEFAULT_TIMEOUT 5
 
 int virNetDevOpenvswitchAddPort(const char *brname,
 const char *ifname,
-- 
2.5.5

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

[libvirt] [PATCH v3 3/3] libvirtd: set openvswitch timeout value based on config data

[Boris Fiuczynski]

Since a successful completion of the calls to openvswitch is expected
a longer timeout should be able to be chosen to account for loaded systems.
Therefore this patch provides the ability to specify the timeout value for
openvswitch calls in the libvirtd configuration file.

Signed-off-by: Boris Fiuczynski 
Reviewed-by: Bjoern Walk 
---
 daemon/libvirtd.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index b6d76ed..5c30c9e 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -58,6 +58,7 @@
 #include "viraccessmanager.h"
 #include "virutil.h"
 #include "virgettext.h"
+#include "util/virnetdevopenvswitch.h"
 
 #ifdef WITH_DRIVER_MODULES
 # include "driver.h"
@@ -658,6 +659,16 @@ daemonSetupNetworking(virNetServerPtr srv,
 
 
 /*
+ * Set up the openvswitch timeout
+ */
+static void
+daemonSetupNetDevOpenvswitch(struct daemonConfig *config)
+{
+virNetDevOpenvswitchSetTimeout(config->ovs_timeout);
+}
+
+
+/*
  * Set up the logging environment
  * By default if daemonized all errors go to the logfile libvirtd.log,
  * but if verbose or error debugging is asked for then also output
@@ -1267,6 +1278,8 @@ int main(int argc, char **argv) {
 exit(EXIT_FAILURE);
 }
 
+daemonSetupNetDevOpenvswitch(config);
+
 if (daemonSetupAccessManager(config) < 0) {
 VIR_ERROR(_("Can't initialize access manager"));
 exit(EXIT_FAILURE);
-- 
2.5.5

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

[libvirt] [PATCH v3 0/3] network: make openvswitch call timeout configurable

[Boris Fiuczynski]

Since a successful completion of openvswitch calls is expected a longer
timeout should be able to be chosen in order to account for loaded systems.
Therefore this patch series provides the ability to specify the timeout value
for openvswitch calls in the libvirtd configuration file.


Boris Fiuczynski (3):
  libvirtd: add openvitch timeout value
  network: allow to specify timeout for openvswitch calls
  libvirtd: set openvswitch timeout value based on config data

 daemon/libvirtd-config.c|  6 
 daemon/libvirtd-config.h|  2 ++
 daemon/libvirtd.aug |  1 +
 daemon/libvirtd.c   | 13 +
 daemon/libvirtd.conf|  9 ++
 daemon/test_libvirtd.aug.in |  1 +
 src/libvirt_private.syms|  1 +
 src/util/virnetdevopenvswitch.c | 64 +++--
 src/util/virnetdevopenvswitch.h |  5 
 9 files changed, 94 insertions(+), 8 deletions(-)

-- 
2.5.5

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

[libvirt] [PATCH v3 2/3] network: allow to specify timeout for openvswitch calls

[Boris Fiuczynski]

This patchs allows to set the timeout value used for all
openvswitch calls. The default timeout value remains as
before at 5 seconds.

Signed-off-by: Boris Fiuczynski 
Reviewed-by: Bjoern Walk 
---
 src/libvirt_private.syms|  1 +
 src/util/virnetdevopenvswitch.c | 64 +++--
 src/util/virnetdevopenvswitch.h |  4 +++
 3 files changed, 61 insertions(+), 8 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d556c7d..0a7de9a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2078,6 +2078,7 @@ virNetDevOpenvswitchGetVhostuserIfname;
 virNetDevOpenvswitchInterfaceStats;
 virNetDevOpenvswitchRemovePort;
 virNetDevOpenvswitchSetMigrateData;
+virNetDevOpenvswitchSetTimeout;
 
 
 # util/virnetdevtap.h
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index e6cb096..3a11fb4 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -1,6 +1,7 @@
 /*
  * Copyright (C) 2013 Red Hat, Inc.
  * Copyright (C) 2012 Nicira, Inc.
+ * Copyright (C) 2017 IBM Corporation
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -20,6 +21,7 @@
  * Dan Wendlandt 
  * Kyle Mestery 
  * Ansis Atteka 
+ * Boris Fiuczynski 
  */
 
 #include 
@@ -38,6 +40,23 @@
 
 VIR_LOG_INIT("util.netdevopenvswitch");
 
+/*
+ * Set openvswitch default timout
+ */
+static unsigned int virNetDevOpenvswitchTimeout = 
VIR_NETDEV_OVS_DEFAULT_TIMEOUT;
+
+/**
+ * virNetDevOpenvswitchSetTimeout:
+ * @timeout: the timeout in seconds
+ *
+ * Set the openvswitch timeout
+ */
+void
+virNetDevOpenvswitchSetTimeout(unsigned int timeout)
+{
+virNetDevOpenvswitchTimeout = timeout;
+}
+
 /**
  * virNetDevOpenvswitchAddPort:
  * @brname: the bridge name
@@ -66,6 +85,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const 
char *ifname,
 char *ifaceid_ex_id = NULL;
 char *profile_ex_id = NULL;
 char *vmid_ex_id = NULL;
+char *ovs_timeout = NULL;
 virBuffer buf = VIR_BUFFER_INITIALIZER;
 
 virMacAddrFormat(macaddr, macaddrstr);
@@ -86,10 +106,12 @@ int virNetDevOpenvswitchAddPort(const char *brname, const 
char *ifname,
 ovsport->profileID) < 0)
 goto cleanup;
 }
+if (virAsprintf(_timeout, "--timeout=%d", virNetDevOpenvswitchTimeout) 
< 0)
+goto cleanup;
 
 cmd = virCommandNew(OVSVSCTL);
 
-virCommandAddArgList(cmd, "--timeout=5", "--", "--if-exists", "del-port",
+virCommandAddArgList(cmd, ovs_timeout, "--", "--if-exists", "del-port",
  ifname, "--", "add-port", brname, ifname, NULL);
 
 if (virtVlan && virtVlan->nTags > 0) {
@@ -165,6 +187,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const 
char *ifname,
 VIR_FREE(ifaceid_ex_id);
 VIR_FREE(vmid_ex_id);
 VIR_FREE(profile_ex_id);
+VIR_FREE(ovs_timeout);
 virCommandFree(cmd);
 return ret;
 }
@@ -181,9 +204,13 @@ int virNetDevOpenvswitchRemovePort(const char *brname 
ATTRIBUTE_UNUSED, const ch
 {
 int ret = -1;
 virCommandPtr cmd = NULL;
+char *ovs_timeout = NULL;
+
+if (virAsprintf(_timeout, "--timeout=%d", virNetDevOpenvswitchTimeout) 
< 0)
+goto cleanup;
 
 cmd = virCommandNew(OVSVSCTL);
-virCommandAddArgList(cmd, "--timeout=5", "--", "--if-exists", "del-port", 
ifname, NULL);
+virCommandAddArgList(cmd, ovs_timeout, "--", "--if-exists", "del-port", 
ifname, NULL);
 
 if (virCommandRun(cmd, NULL) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -194,6 +221,7 @@ int virNetDevOpenvswitchRemovePort(const char *brname 
ATTRIBUTE_UNUSED, const ch
 ret = 0;
  cleanup:
 virCommandFree(cmd);
+VIR_FREE(ovs_timeout);
 return ret;
 }
 
@@ -211,8 +239,12 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, 
const char *ifname)
 virCommandPtr cmd = NULL;
 size_t len;
 int ret = -1;
+char *ovs_timeout = NULL;
 
-cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "--if-exists", "get", 
"Interface",
+if (virAsprintf(_timeout, "--timeout=%d", virNetDevOpenvswitchTimeout) 
< 0)
+goto cleanup;
+
+cmd = virCommandNewArgList(OVSVSCTL, ovs_timeout, "--if-exists", "get", 
"Interface",
ifname, "external_ids:PortData", NULL);
 
 virCommandSetOutputBuffer(cmd, migrate);
@@ -233,6 +265,7 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, 
const char *ifname)
 ret = 0;
  cleanup:
 virCommandFree(cmd);
+VIR_FREE(ovs_timeout);
 return ret;
 }
 
@@ -249,13 +282,17 @@ int virNetDevOpenvswitchSetMigrateData(char *migrate, 
const char *ifname)
 {
 virCommandPtr cmd = NULL;
 int ret = -1;
+char *ovs_timeout = NULL;
 
 if (!migrate) {
   

Re: [libvirt] [PATCH] bhyve: fix virtio disk addresses

[Andrea Bolognani]

On Wed, 2017-02-01 at 20:25 +0400, Roman Bogorodskiy wrote:
> Like it usually happens, I fixed one thing and broke another:
> in 803966c76 address allocation was fixed for SATA disks, but
> broke that for virtio disks, because it dropped disk address
> assignment completely. It's not needed for SATA disks anymore,
> but still needed for the virtio ones.
> 
> Bring that back and add a couple of tests to make sure it won't
> happen again.

I didn't actually test this[1], but both the code and the
tests look reasonable enough, plus 'make check' and 'make
syntax-check' are all green[2] on FreeBSD, so:

ACK


[1] Can you run bhyve guests inside a FreeBSD KVM guest?
[2] Well, mostly. We really should fix qemuxml2argvtest once
and for all, and it looks like we're sooo close now!
-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH 3/3] storage: Fix checking whether source filesystem is mounted

[Erik Skultety]

Right now, we use simple string comparison both on the source paths
(mount's output vs pool's source) and the target (mount's mnt_dir vs
pool's target). The problem are symlinks and mount indeed returns
symlinks in its output, e.g. /dev/mappper/lvm_symlink. The same goes for
the pool's source/target, so in order to successfully compare these two
replace plain string comparison with virFileComparePaths which will
resolve all symlinks and canonicalize the paths prior to comparison.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1417203

Signed-off-by: Erik Skultety 
---
 src/storage/storage_backend_fs.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index fe4705b..fae1c03 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -301,6 +301,7 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr 
pool)
 FILE *mtab;
 struct mntent ent;
 char buf[1024];
+int rc1, rc2;
 
 if ((mtab = fopen(_PATH_MOUNTED, "r")) == NULL) {
 virReportSystemError(errno,
@@ -313,8 +314,15 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr 
pool)
 if (!(src = virStorageBackendFileSystemGetPoolSource(pool)))
 goto cleanup;
 
-if (STREQ(ent.mnt_dir, pool->def->target.path) &&
-STREQ(ent.mnt_fsname, src)) {
+/* compare both mount destinations and sources to be sure the mounted
+ * FS pool is really the one we're looking for
+ */
+if ((rc1 = virFileComparePaths(ent.mnt_dir,
+   pool->def->target.path)) < 0 ||
+(rc2 = virFileComparePaths(ent.mnt_fsname, src)) < 0)
+goto cleanup;
+
+if (rc1 == 0 && rc2 == 0) {
 ret = 1;
 goto cleanup;
 }
-- 
2.10.2

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

[libvirt] [PATCH 2/3] util: Introduce virFileComparePaths

[Erik Skultety]

So rather than comparing 2 paths (strings) as they are, which can very
easily lead to unnecessary errors (e.g. in storage driver) that the paths
are not the same when in fact they'd be e.g. just symlinks to the same
location, we should put our best effort into resolving any symlinks and
canonicalizing the path and only then compare the 2 paths.

Signed-off-by: Erik Skultety 
---
 src/libvirt_private.syms |  1 +
 src/util/virfile.c   | 45 +
 src/util/virfile.h   |  2 ++
 3 files changed, 48 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8e994c7..06f3737 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1575,6 +1575,7 @@ virFileActivateDirOverride;
 virFileBindMountDevice;
 virFileBuildPath;
 virFileClose;
+virFileComparePaths;
 virFileCopyACLs;
 virFileDeleteTree;
 virFileDirectFdFlag;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index bf8099e..b261632 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3737,3 +3737,48 @@ virFileCopyACLs(const char *src,
 virFileFreeACLs();
 return ret;
 }
+
+/*
+ * virFileComparePaths:
+ * @p1: source path 1
+ * @p2: source path 2
+ *
+ * Compares two paths for equality. To do so, it first canonicalizes both paths
+ * to resolve all symlinks and discard relative path components. If symlinks
+ * resolution or path canonicalization fails, plain string equality of @p1
+ * and @p2 is performed.
+ *
+ * Returns 0 if the paths are equal, 1 if they're not or -1 in case of an
+ * error.
+ */
+int
+virFileComparePaths(const char *p1, const char *p2)
+{
+int ret = -1;
+char *res1, *res2;
+
+res1 = res2 = NULL;
+
+/* Assume p1 and p2 are symlinks, so try to resolve and canonicalize them.
+ * Canonicalization fails for example on file systems names like 'proc' or
+ * 'sysfs', since they're no real paths so fallback to plain string
+ * comparison.
+ */
+ignore_value(virFileResolveLink(p1, ));
+if (!res1 && VIR_STRDUP(res1, p1) < 0)
+goto cleanup;
+
+ignore_value(virFileResolveLink(p2, ));
+if (!res2 && VIR_STRDUP(res2, p2) < 0)
+goto cleanup;
+
+if (STREQ_NULLABLE(res1, res2))
+ret = 0;
+else
+ret = 1;
+
+ cleanup:
+VIR_FREE(res1);
+VIR_FREE(res2);
+return ret;
+}
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 0343acd..5822b29 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -331,4 +331,6 @@ void virFileFreeACLs(void **acl);
 
 int virFileCopyACLs(const char *src,
 const char *dst);
+
+int virFileComparePaths(const char *p1, const char *p2);
 #endif /* __VIR_FILE_H */
-- 
2.10.2

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

[libvirt] [PATCH 0/3] storage: Fix FS pool destroy not unmounting its source

[Erik Skultety]

The issue lies in how we check whether a FS is already mounted or not, we
compare the pool's source/target with source/target in the mount's binary
output both of which can actually be just symlinks to the same location yet
failing our check on the FS being mounted, thus reporting a strange error that
the pool's source is already mounted.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1417203

Erik Skultety (3):
  storage: Fix reporting an error on an already mounted filesystem
  util: Introduce virFileComparePaths
  storage: Fix checking whether source filesystem is mounted

 src/libvirt_private.syms |  1 +
 src/storage/storage_backend_fs.c | 25 ++
 src/util/virfile.c   | 45 
 src/util/virfile.h   |  2 ++
 4 files changed, 64 insertions(+), 9 deletions(-)

-- 
2.10.2

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

[libvirt] [PATCH 1/3] storage: Fix reporting an error on an already mounted filesystem

[Erik Skultety]

When FS pool's source is already mounted on the target location instead
of just simply marking the pool as active, thus starting it we fail with
an error stating that the source is indeed already mounted on the target.

Signed-off-by: Erik Skultety 
---
 src/storage/storage_backend_fs.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 54bcc57..fe4705b 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -358,14 +358,13 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr 
pool)
 if (virStorageBackendFileSystemIsValid(pool) < 0)
 return -1;
 
+if ((rc = virStorageBackendFileSystemIsMounted(pool)) < 0)
+return -1;
+
 /* Short-circuit if already mounted */
-if ((rc = virStorageBackendFileSystemIsMounted(pool)) != 0) {
-if (rc == 1) {
-virReportError(VIR_ERR_OPERATION_INVALID,
-   _("Target '%s' is already mounted"),
-   pool->def->target.path);
-}
-return -1;
+if (rc == 1) {
+VIR_INFO("Target '%s' is already mounted", pool->def->target.path);
+return 0;
 }
 
 if (!(src = virStorageBackendFileSystemGetPoolSource(pool)))
-- 
2.10.2

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

Re: [libvirt] [PATCH v2] docs: mention bhyve SATA address changes in news.xml

[Roman Bogorodskiy]

  Andrea Bolognani wrote:

> On Mon, 2017-02-06 at 21:49 +0400, Roman Bogorodskiy wrote:
> [...]
> > +  However, as this doesn't match libvirt's understanding of
> > +  disk addresses, it was changed for the bhyve driver
> 
>   "it was changed for the bhyve driver"
>   → "the bhyve driver was changed"
> 
> Sorry for not catching it the first time around :/
> 
> ACK with that fixed.

Thanks, I've included this change as well.

I'm not sure if it's better to push it right now or wait for
https://www.redhat.com/archives/libvir-list/2017-February/msg00012.html
to get reviewed, because the bhyve driver as it is now is not completely
fixed (the patch I mentioned fixes virtio devices), so if it happens
that this patch won't get into the upcoming release, it'd be at least a
little less awkward not to claim everything's fixed...

Roman Bogorodskiy


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

[libvirt] [PATCH v3] qemu: Forbid without

[Andrea Bolognani]

In order for memory locking to work, the hard limit on memory
locking (and usage) has to be set appropriately by the user.

The documentation mentions the requirement already: with this
patch, it's going to be enforced by runtime checks as well,
by forbidding a non-compliant guest from being defined as well
as edited and started.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1316774
---
Changes from [v2]

  * Address review feedback:
- move the check from BuildCommandLine to Validate.

Changes from [v1]

  * Address review feeback:
- check in BuildCommandLine rather than in PostParse,
  so that non-compliant guests will merely fail to
  start rather than disappear completely.

[v1] https://www.redhat.com/archives/libvir-list/2017-February/msg00180.html
[v2] https://www.redhat.com/archives/libvir-list/2017-February/msg00214.html

 src/qemu/qemu_domain.c   | 10 ++
 tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c6ce090..ce3aa69 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2831,6 +2831,16 @@ qemuDomainDefValidate(const virDomainDef *def,
 }
 }
 
+/* Memory locking can only work properly if the memory locking limit
+ * for the QEMU process has been raised appropriately: the default one
+ * is extrememly low, so there's no way the guest will fit in there */
+if (def->mem.locked && !virMemoryLimitIsSet(def->mem.hard_limit)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Setting  requires "
+ " to be set as well"));
+goto cleanup;
+}
+
 if (qemuDomainDefValidateVideo(def) < 0)
 goto cleanup;
 
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml
index 20a5eaa..2046663 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml
@@ -3,6 +3,9 @@
   c7a5fdbd-edaf-9455-926a-d65c16db1809
   219136
   219136
+  
+256000
+  
   
 
   
-- 
2.7.4

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

Re: [libvirt] [PATCH] qemu: Check for maximum vcpus exceeding cpu topology

[Daniel P. Berrange]

On Tue, Feb 07, 2017 at 07:56:29AM -0500, Kothapally Madhu Pavan wrote:
> This patch will prevent guest to start when the maximum
> vcpus are greater than cpu topology limit. Currently similar
> checks do exist only during setvcpus. The patch adds the
> missing check. The c9cb35c255222 reverted similar check to
> avoid older configs from vanishing. The current patch adds
> it in domain validation part to be consistent with the
> original intent.
> 
> Signed-off-by: Kothapally Madhu Pavan 
> ---
>  src/qemu/qemu_process.c |7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 184440d..f0d42b8 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3738,6 +3738,13 @@ qemuValidateCpuCount(virDomainDefPtr def,
>  return -1;
>  }
>  
> +if (def->cpu->sockets && virDomainDefGetVcpusMax(def) >
> +def->cpu->sockets * def->cpu->cores * def->cpu->threads) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("Maximum CPUs greater than topology limit"));
> +return -1;
> +}

This looks wrong.  def->cpu->sockets is sockets-per-NUMA node.
So you're comparing the total number of VCPUS against the CPUs
permitted in a single guest NUMA node. 


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] [PATCH] qemu: Check for maximum vcpus exceeding cpu topology

[Peter Krempa]

On Tue, Feb 07, 2017 at 07:56:29 -0500, Kothapally Madhu Pavan wrote:
> This patch will prevent guest to start when the maximum
> vcpus are greater than cpu topology limit. Currently similar
> checks do exist only during setvcpus. The patch adds the
> missing check. The c9cb35c255222 reverted similar check to
> avoid older configs from vanishing. The current patch adds
> it in domain validation part to be consistent with the
> original intent.
> 
> Signed-off-by: Kothapally Madhu Pavan 
> ---
>  src/qemu/qemu_process.c |7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 184440d..f0d42b8 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3738,6 +3738,13 @@ qemuValidateCpuCount(virDomainDefPtr def,
>  return -1;
>  }
>  
> +if (def->cpu->sockets && virDomainDefGetVcpusMax(def) >
> +def->cpu->sockets * def->cpu->cores * def->cpu->threads) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("Maximum CPUs greater than topology limit"));
> +return -1;
> +}

This exact logic is done in qemuDomainDefValidate:

+if (virDomainDefGetVcpusTopology(def, ) == 0 &&
+topologycpus != virDomainDefGetVcpusMax(def)) {
+/* presence of query-hotpluggable-cpus should be a good enough witness 
*/
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("CPU topology doesn't match maximum vcpu count"));
+goto cleanup;
 }
 }

See commit 043ba4a40a4ae26cf616146d0d1c129d65b156b8. The only difference
is that the code is validated only for certain versions of qemu known to
reject such configuration.

Is that not enough?

Peter


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

[libvirt] [PATCH] qemu: Check for maximum vcpus exceeding cpu topology

[Kothapally Madhu Pavan]

This patch will prevent guest to start when the maximum
vcpus are greater than cpu topology limit. Currently similar
checks do exist only during setvcpus. The patch adds the
missing check. The c9cb35c255222 reverted similar check to
avoid older configs from vanishing. The current patch adds
it in domain validation part to be consistent with the
original intent.

Signed-off-by: Kothapally Madhu Pavan 
---
 src/qemu/qemu_process.c |7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 184440d..f0d42b8 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3738,6 +3738,13 @@ qemuValidateCpuCount(virDomainDefPtr def,
 return -1;
 }
 
+if (def->cpu->sockets && virDomainDefGetVcpusMax(def) >
+def->cpu->sockets * def->cpu->cores * def->cpu->threads) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Maximum CPUs greater than topology limit"));
+return -1;
+}
+
 if (maxCpus > 0 && virDomainDefGetVcpusMax(def) > maxCpus) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Maximum CPUs greater than specified machine type 
limit"));

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

[libvirt] [PATCH] Mention the min duration for nodesuspend explicitly

[Nitesh Konkar]

Although currently this is documented in virsh man page
and virsh help, the expicit mention in the error message
is helful for tools using the API directly.

Signed-off-by: Nitesh Konkar 
---
 src/util/virnodesuspend.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c
index 8bb8d93..71b2d4c 100644
--- a/src/util/virnodesuspend.c
+++ b/src/util/virnodesuspend.c
@@ -76,7 +76,9 @@ static int virNodeSuspendSetNodeWakeup(unsigned long long 
alarmTime)
 int ret = -1;
 
 if (alarmTime < MIN_TIME_REQ_FOR_SUSPEND) {
-virReportError(VIR_ERR_INVALID_ARG, "%s", _("Suspend duration is too 
short"));
+virReportError(VIR_ERR_INVALID_ARG,
+   _("Suspend duration is too short, must be at least %u 
seconds"),
+   MIN_TIME_REQ_FOR_SUSPEND);
 return -1;
 }
 
-- 
2.1.0

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

Re: [libvirt] [PATCH 00/15] Add more vHBA related tests and module-arize the code

[John Ferlan]

ping?

tks -

John

On 01/25/2017 03:27 PM, John Ferlan wrote:
> Don't be scared off by the quantity of patches...
> 
> There's quite a bit of code motion and function renaming going on before
> being able to more easily add tests that will ensure that from a nodedev
> perspective creation and deletion of the vHBA will work properly and it's
> possible to test the various ways to create a vHBA (nothing provide, a
> parent by name provided, a parent by wwnn/wwpn provided, and a parent
> by fabric_wwn provided).
> 
> I did run this through the coverity checking code with no errors...
> 
> John Ferlan (15):
>   tests: Alter test_driver HBA name/data to be closer to reality
>   test: Add new NPIV capable HBA and a vHBA
>   test: Add helper to create vHBA for testNodeDeviceCreateXML
>   tests: Create a more realistic vHBA
>   test: Fix fchosttest resource leak
>   util: Create a new virvhba module and move/rename API's
>   util: Move/rename virStoragePoolGetVhbaSCSIHostParent to virvhba
>   util: Reduce complexity of virVHBAGetParent
>   util: Move scsi_host specific functions from virutil
>   tests: Add new fchosttest tests for management of a vHBA
>   nodedev: Keep the node device lock longer in nodeDeviceDestroy
>   nodedev: Rework virNodeDeviceGetParentHost
>   tests: Add createVHBAByNodeDevice-no-parent to fchosttest
>   tests: Add createVHBAByNodeDevice-parent-wwn to fchosttest
>   tests: Add createVHBAByNodeDevice-parent-fabric-wwn to fchosttest
> 
>  po/POTFILES.in|   2 +
>  src/Makefile.am   |   2 +
>  src/conf/node_device_conf.c   |  76 +++-
>  src/conf/node_device_conf.h   |  19 +-
>  src/conf/storage_conf.c   | 100 ++---
>  src/conf/storage_conf.h   |   5 -
>  src/libvirt_private.syms  |  32 +-
>  src/node_device/node_device_driver.c  |  92 ++--
>  src/node_device/node_device_linux_sysfs.c |  24 +-
>  src/storage/storage_backend_scsi.c|  68 +--
>  src/test/test_driver.c| 178 ++--
>  src/util/virscsi.c|   4 +
>  src/util/virscsihost.c| 297 
>  src/util/virscsihost.h|  40 ++
>  src/util/virutil.c| 724 
> --
>  src/util/virutil.h|  47 --
>  src/util/virvhba.c| 591 
>  src/util/virvhba.h|  59 +++
>  tests/fchosttest.c| 183 ++--
>  tests/objecteventtest.c   |   6 +-
>  tests/scsihosttest.c  |  16 +-
>  tests/virrandommock.c |   9 +
>  22 files changed, 1463 insertions(+),  deletions(-)
>  create mode 100644 src/util/virscsihost.c
>  create mode 100644 src/util/virscsihost.h
>  create mode 100644 src/util/virvhba.c
>  create mode 100644 src/util/virvhba.h
> 

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

Re: [libvirt] [PATCH v2 3/4] qemu: Implement support for 'builtin' backend for virtio-crypto

[Martin Kletzander]

On Wed, Jan 11, 2017 at 04:28:25PM +0800, Longpeng(Mike) wrote:

This patch implements support for the virtio-crypto-pci device
and the builtin backend in qemu.

Two capabilities bits are added to track support for those:

QEMU_CAPS_DEVICE_VIRTIO_CRYPTO - for the device support and
QEMU_CAPS_OBJECT_CRYPTO_BUILTIN - for the backend support.

qemu is invoked with these additional parameters if the device
id enabled:

(to add the backend)
-object cryptodev-backend-builtin,id=objcrypto0,queues=1
(to add the device)
-device virtio-crypto-pci,cryptodev=objcrypto0,id=crypto0

Signed-off-by: Longpeng(Mike) 
---
src/conf/domain_conf.c |   4 +-
src/qemu/qemu_alias.c  |  20 +++
src/qemu/qemu_alias.h  |   3 +
src/qemu/qemu_capabilities.c   |   4 ++
src/qemu/qemu_capabilities.h   |   2 +
src/qemu/qemu_command.c| 132 +
src/qemu/qemu_command.h|   3 +
src/qemu/qemu_domain_address.c |  26 +++-
8 files changed, 191 insertions(+), 3 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ef44930..cf77af5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12595,7 +12595,7 @@ virDomainCryptoDefParseXML(xmlNodePtr node,
if (virDomainDeviceInfoParseXML(node, NULL, >info, flags) < 0)
goto error;

-cleanup:
+ cleanup:
VIR_FREE(model);
VIR_FREE(backend);
VIR_FREE(queues);
@@ -12603,7 +12603,7 @@ cleanup:
ctxt->node = save;
return def;

-error:
+ error:


Make sure you run make check and make syntax check after *every* patch.
Otherwise we end up with hunks like this that clearly don't belong here.


diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d459f8e..afebe69 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5787,6 +5787,135 @@ qemuBuildRNGCommandLine(virLogManagerPtr logManager,


static char *
+qemuBuildCryptoBackendStr(virDomainCryptoDefPtr crypto,
+  virQEMUCapsPtr qemuCaps)
+{
+const char *type = NULL;
+char *alias = NULL;
+char *queue = NULL;
+char *ret = NULL;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+if (virAsprintf(, "obj%s", crypto->info.alias) < 0)
+goto cleanup;
+
+if (crypto->queues > 0) {
+if (virAsprintf(, "queues=%u", crypto->queues) < 0)
+goto cleanup;
+}
+
+switch ((virDomainCryptoBackend)crypto->backend) {
+case VIR_DOMAIN_CRYPTO_BACKEND_BUILTIN:
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_CRYPTO_BUILTIN)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("this qemu doesn't support the builtin backend"));
+goto cleanup;
+}
+
+type = "cryptodev-backend-builtin";
+break;
+
+case VIR_DOMAIN_CRYPTO_BACKEND_LAST:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("unknown crypto backend"));
+goto cleanup;
+}
+
+if (queue)
+virBufferAsprintf(, "%s,id=%s,%s", type, alias, queue);
+else
+virBufferAsprintf(, "%s,id=%s", type, alias);
+
+ret = virBufferContentAndReset();


The advantage of using buffer is that you can act on it multiple times.
If you just want one asprintf, you can just do virAsprintf instead.  But
in this case I would just append every parameter as you go.


+
+ cleanup:
+VIR_FREE(alias);
+return ret;
+}
+
+
+char *
+qemuBuildCryptoDevStr(const virDomainDef *def,
+  virDomainCryptoDefPtr dev,
+  virQEMUCapsPtr qemuCaps)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+if (dev->model != VIR_DOMAIN_CRYPTO_MODEL_VIRTIO ||
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_CRYPTO)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("this qemu doesn't support crypto device model '%s'"),
+   virDomainRNGModelTypeToString(dev->model));
+goto error;
+}
+
+if (dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unsupported address type %s for virtio crypto 
device"),
+   virDomainDeviceAddressTypeToString(dev->info.type));
+goto error;
+}
+
+virBufferAsprintf(, "virtio-crypto-pci,cryptodev=obj%s,id=%s",
+  dev->info.alias, dev->info.alias);
+
+if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps) < 0)
+goto error;
+
+if (virBufferCheckError() < 0)
+goto error;
+


This ^^ is pointless since this vv will return NULL and not leak either.


+return virBufferContentAndReset();
+
+ error:
+virBufferFreeAndReset();
+return NULL;
+}
+
+
+static int
+qemuBuildCryptoCommandLine(virCommandPtr cmd,
+   const virDomainDef *def,
+   virQEMUCapsPtr qemuCaps)
+{
+size_t i;
+
+for (i = 0; i < 

Re: [libvirt] [PATCH 00/10] Another set of qemu namespace fixes

[Michal Privoznik]

On 01/20/2017 10:42 AM, Michal Privoznik wrote:
> The major problem was with symlinks. Imagine the following chain of symlinks:
> 
> /dev/my_awesome_disk -> /home/user/blaah -> /dev/disk/by-uuid/$uuid -> 
> /dev/sda
> 
> We really need to create all those /dev/* symlinks and /dev/sda device. Also,
> some other (less critical) bugs are fixed.
> 
> Michal Privoznik (10):
>   virProcessRunInMountNamespace: Report errors from child
>   util: Introduce virFileReadLink
>   qemuDomainPrepareDisk: Fix ordering
>   qemuSecurityRestoreAllLabel: Don't use transactions
>   qemu_security: Use more transactions
>   qemuDomain{Attach,Detach}Device NS helpers: Don't relabel devices
>   qemuDomainCreateDevice: Properly deal with symlinks
>   qemuDomainCreateDevice: Don't loop endlessly
>   qemuDomainAttachDeviceMknod: Deal with symlinks
>   qemuDomainAttachDeviceMknod: Don't loop endlessly
> 
>  src/libvirt_private.syms |   1 +
>  src/qemu/qemu_domain.c   | 438 
> +++
>  src/qemu/qemu_hotplug.c  |  20 +--
>  src/qemu/qemu_security.c | 137 +--
>  src/util/virfile.c   |  12 ++
>  src/util/virfile.h   |   2 +
>  src/util/virprocess.c|   8 +-
>  7 files changed, 374 insertions(+), 244 deletions(-)
> 

Thanks Martin for the review. I've pushed these.

Michal

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

Re: [libvirt] [PATCH v2 2/4] conf: Parse virtio-crypto in the domain XML

[Martin Kletzander]

On Wed, Jan 11, 2017 at 04:28:24PM +0800, Longpeng(Mike) wrote:

This patch parse the domain XML with virtio-crypto
support, the virtio-crypto XML looks like this:

 
   
 

Signed-off-by: Longpeng(Mike) 
---
src/conf/domain_conf.c | 213 -
src/conf/domain_conf.h |  32 +++
src/libvirt_private.syms   |   2 +
src/qemu/qemu_domain.c |   2 +
src/qemu/qemu_domain_address.c |   1 +
src/qemu/qemu_driver.c |   6 ++
src/qemu/qemu_hotplug.c|   1 +
7 files changed, 256 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 52aee2b..ef44930 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18967,6 +19096,25 @@ virDomainRNGDefCheckABIStability(virDomainRNGDefPtr 
src,


static bool
+virDomainCryptoDefCheckABIStability(virDomainCryptoDefPtr src,
+virDomainCryptoDefPtr dst)
+{
+if (src->model != dst->model) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Target Crypto model '%s' does not match source 
'%s'"),
+   virDomainCryptoModelTypeToString(dst->model),
+   virDomainCryptoModelTypeToString(src->model));
+return false;
+}
+


The number of queues is not part of ABI?  That'd make sense, I'm just
making sure.


diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 4d16620..cceb576 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -228,6 +228,8 @@ virDomainControllerRemove;
virDomainControllerTypeToString;
virDomainCpuPlacementModeTypeFromString;
virDomainCpuPlacementModeTypeToString;
+virDomainCryptoBackendTypeFromString;
+virDomainCryptoModelTypeFromString;


You're missing the other variants (ToString).  That will be apparent
when you will implement FormatXML of the device as well.  One xml2xml
test case and it would be caught.


diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index d2f7953..e17476a 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -784,6 +784,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_DEVICE_LEASE:
case VIR_DOMAIN_DEVICE_GRAPHICS:
case VIR_DOMAIN_DEVICE_IOMMU:
+case VIR_DOMAIN_DEVICE_CRYPTO:


Why are you adding this to the list of devices that don't hae DeviceInfo
when this device clearly has one?  You need to ensure proper allocation


case VIR_DOMAIN_DEVICE_LAST:
case VIR_DOMAIN_DEVICE_NONE:
return 0;


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: Forbid without

[Andrea Bolognani]

On Tue, 2017-02-07 at 12:07 +0100, Pavel Hrdina wrote:
> Actually there is a third option, we have domainValidateCallback
> that is what you are looking for.  This function is called only
> when defining new XML and is skipped while parsing the XML from
> disk on libvirtd startup or while restoring from snapshot and etc.
> 
> It is also called before domain is started.

Oh, neat! I'll give it a go :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [resend v2 0/7] Support cache tune in libvirt

[Marcelo Tosatti]

On Tue, Feb 07, 2017 at 10:17:37AM +, Daniel P. Berrange wrote:
> On Tue, Feb 07, 2017 at 02:43:13PM +0800, Eli Qiao wrote:
> > > 3) CDP / non-CDP convertion.
> > >  
> > > In case the size determination has been performed with non-CDP,
> > > to emulate such allocation on a CDP host,
> > > it would be good to allow both code and data allocations to share
> > > the CBM space:  
> > >  
> > IOM, I don’t think it’s good to have this.
> > in libvirt capabilities xml, the application will get to know if the host 
> > support cdp or not.
> 
> Yep, as long as the capabilities XML provide info about the different
> cache banks, IMHO it is better to have the application be explicit
> about what they want for CDP & non-CDP scenarios.  Let the higher
> level mgmt apps above libvirt apply specific policies if they desire

There is no policy being applied here. What i mean is the following:

1) User determines that the type of the CAT allocation necessary
for his application is one which shares cache and data, that is non-CDP
(either because he didnt have a CDP machine at the time, or because
he had a CDP machine but sharing data and code cache turns out 
to be efficient for the application).
Say that this measured size is M.

2) A host with CDP enabled resctrl is used for this VM. How to create
a CAT allocation with shared code and data? Have to write to the
schemata file of the VM the following:


L3data:1=0x00ff;...
L3code:1=0x00ff;...

(that is, the data and code CBM masks use the same bits).

3) How is the user going to achieve that with this patchset today?
AFAIK, he can't. What he can do is the following:

  
  

But this will allocate the following schemata file:

L3data:1=0xff00;...
L3code:1=0x00ff;...

Which is not what is wanted.

Therefore the suggestion to _share the cbm bit space_ in case
a similar "cachetune id" is used.

(maybe have a different syntax, i don't care, as long as the user
can create code / data CAT allocations that share the CBM space).

Does that make sense now?


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

Re: [libvirt] [resend v2 0/7] Support cache tune in libvirt

[Marcelo Tosatti]

On Tue, Feb 07, 2017 at 02:43:13PM +0800, Eli Qiao wrote:
> 
> 
> --  
> Eli Qiao
> Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
> 
> 
> On Tuesday, 7 February 2017 at 3:03 AM, Marcelo Tosatti wrote:
> 
> > On Mon, Feb 06, 2017 at 01:33:09PM -0200, Marcelo Tosatti wrote:
> > > On Mon, Feb 06, 2017 at 10:23:35AM +0800, Eli Qiao wrote:
> > > > This series patches are for supportting CAT featues, which also
> > > > called cache tune in libvirt.
> > > >  
> > > > First to expose cache information which could be tuned in capabilites 
> > > > XML.
> > > > Then add new domain xml element support to add cacahe bank which will 
> > > > apply
> > > > on this libvirt domain.
> > > >  
> > > > This series patches add a util file `resctrl.c/h`, an interface to talk 
> > > > with
> > > > linux kernel's sys fs.
> > > >  
> > > > There are still some TODOs such as expose new public interface to get 
> > > > free
> > > > cache information.
> > > >  
> > > > Some discussion about this feature support can be found from:
> > > > https://www.redhat.com/archives/libvir-list/2017-January/msg00644.html
> > > >  
> > >  
> > >  
> > > Two comments:
> > >  
> > > 1) Please perform appropriate filesystem locking when accessing
> > > resctrlfs, as described at:
> > >  
> > > http://www.spinics.net/lists/linux-tip-commits/msg36754.html
> 
> Sure.  
> > >  
> > > 2)  
> > >  
> > > 
> > >  
> > > [b4c270b5-e0f9-4106-a446-69032872ed7d]# cat tasks  
> > > 8654
> > > [b4c270b5-e0f9-4106-a446-69032872ed7d]# pstree -p | grep qemu
> > > |-qemu-kvm(8654)-+-{qemu-kvm}(8688)
> > > | |-{qemu-kvm}(8692)
> > > | `-{qemu-kvm}(8693)
> > >  
> > > Should add individual vcpus to the "tasks" file, not the main QEMU
> > > process.
> > >  
> > > The NFV usecase requires exclusive CAT allocation for the vcpu which
> > > runs the sensitive workload.
> > >  
> > > Perhaps:
> > >  
> > >   
> > >  
> > > Adds all vcpus that are pinned to the socket which cachebank with
> > > host_id=1.
> > >  
> > >  > > vcpus=2,3/>  
> > >  
> > > Adds PID of vcpus 2 and 3 to resctrl directory created for this
> > > allocation.
> > >  
> >  
> >  
> Hmm.. in this case, we need to figure out what’s the pid of vcpus=2 and 
> vcpu=3 and added them to the resctrl directory.

Yes and only the pids of vcpus=2 and vcpus=3, not of any other vcpus.

> currently, I create a resctrl directory(called resctrl domain) for a VM so 
> just put all task ids to it.
> 
> this is my though:
> 
> let say the vm has vcpus=0 1 2 3, and you want to let 0, 1 benefit cache on 
> host_id=0, and 2, 3 on host_id=1
> 
> you will do:
> 
> 1)
> pin vcpus 0, 1 on the cpus of socket 0  
> pin vcpus 2, 3 on the cpus of socket 1
> this can be done in vcputune
> 
> 2) define cache tune like this:
> 
> 
> 
> in libvirt:
> we create a resctrl directory naming with the VM’s uuid
> and set schemata for each socket 0, and socket 1, put all qemu tasks ids into 
> tasks file, this will work fine.  

No, please don't do this.

> please be note that in a resctrl directory, we can define schemata for each 
> socket id separately.

Please do not put vcpus automatically into the reservations.
Its necessary to have certain vcpus in a reservation and some not.

For example: 2 vcpu guest, vcpu0 pinned to socket 0 cpu0, 
   vcpu1 pinned to socket 0 cpu1.




We want _only_ vcpu1 to be part of this reservation, and not vcpu0 
(want vcpu0 to use the default group, ie. schemata file at 

/sys/fs/resctrl/schemata

So please have the ability to add vcpus to the XML syntax:



or



This also allows different sizes to be specified.


> > 3) CDP / non-CDP convertion.
> >  
> > In case the size determination has been performed with non-CDP,
> > to emulate such allocation on a CDP host,
> > it would be good to allow both code and data allocations to share
> > the CBM space:  
> >  
> IOM, I don’t think it’s good to have this.
> in libvirt capabilities xml, the application will get to know if the host 
> support cdp or not.
>   
> >  
> > 
> > 
> >  
> > Perhaps if using the same ID?
> I am open to hear about what other’s say,  
>   
> >  
> >  
> > Other than that, testing looks good.  
> Thanks for the testing.  
> 
> 

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

Re: [libvirt] [PATCH v2] qemu: Forbid without

[Pavel Hrdina]

On Tue, Feb 07, 2017 at 12:35:03PM +0100, Andrea Bolognani wrote:
> In order for memory locking to work, the hard limit on memory
> locking (and usage) has to be set appropriately by the user.
> 
> The documentation mentions the requirement already: with this
> patch, it's going to be enforced by runtime checks as well,
> by forbidding a non-compliant guest from starting at all.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1316774
> ---
> Changes from [v1]
> 
>   * Address review feeback:
> - check in BuildCommandLine rather than in PostParse,
>   so that non-compliant guests will merely fail to
>   start rather than disappear completely.
> 
> [v1] https://www.redhat.com/archives/libvir-list/2017-February/msg00180.html

NACK, see my review for v1.  This should be implemented in
qemuDomainDefValidate.

Pavel

> 
>  src/qemu/qemu_command.c  | 9 +
>  tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml | 3 +++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 1396661..ca3bcdc 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7340,6 +7340,15 @@ qemuBuildMemCommandLine(virCommandPtr cmd,
>  qemuBuildMemPathStr(cfg, def, qemuCaps, cmd) < 0)
>  return -1;
>  
> +/* Memory locking can only work properly if the memory locking limit
> + * for the QEMU process has been raised appropriately: the default one
> + * is extrememly low, so there's no way the guest will fit in there */
> +if (def->mem.locked && !virMemoryLimitIsSet(def->mem.hard_limit)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("Setting  requires "
> + " to be set as well"));
> +return -1;
> +}
>  if (def->mem.locked && !virQEMUCapsGet(qemuCaps, 
> QEMU_CAPS_REALTIME_MLOCK)) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("memory locking not supported by QEMU binary"));
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml 
> b/tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml
> index 20a5eaa..2046663 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml
> @@ -3,6 +3,9 @@
>c7a5fdbd-edaf-9455-926a-d65c16db1809
>219136
>219136
> +  
> +256000
> +  
>
>  
>
> -- 
> 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

[libvirt] [PATCH v2] qemu: Forbid without

[Andrea Bolognani]

In order for memory locking to work, the hard limit on memory
locking (and usage) has to be set appropriately by the user.

The documentation mentions the requirement already: with this
patch, it's going to be enforced by runtime checks as well,
by forbidding a non-compliant guest from starting at all.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1316774
---
Changes from [v1]

  * Address review feeback:
- check in BuildCommandLine rather than in PostParse,
  so that non-compliant guests will merely fail to
  start rather than disappear completely.

[v1] https://www.redhat.com/archives/libvir-list/2017-February/msg00180.html

 src/qemu/qemu_command.c  | 9 +
 tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml | 3 +++
 2 files changed, 12 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1396661..ca3bcdc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7340,6 +7340,15 @@ qemuBuildMemCommandLine(virCommandPtr cmd,
 qemuBuildMemPathStr(cfg, def, qemuCaps, cmd) < 0)
 return -1;
 
+/* Memory locking can only work properly if the memory locking limit
+ * for the QEMU process has been raised appropriately: the default one
+ * is extrememly low, so there's no way the guest will fit in there */
+if (def->mem.locked && !virMemoryLimitIsSet(def->mem.hard_limit)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Setting  requires "
+ " to be set as well"));
+return -1;
+}
 if (def->mem.locked && !virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_REALTIME_MLOCK)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("memory locking not supported by QEMU binary"));
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml
index 20a5eaa..2046663 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-mlock-on.xml
@@ -3,6 +3,9 @@
   c7a5fdbd-edaf-9455-926a-d65c16db1809
   219136
   219136
+  
+256000
+  
   
 
   
-- 
2.7.4

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

Re: [libvirt] [PATCH 08/10] qemuDomainCreateDevice: Don't loop endlessly

[Michal Privoznik]

On 02/07/2017 11:34 AM, Martin Kletzander wrote:
> On Fri, Jan 20, 2017 at 10:42:48AM +0100, Michal Privoznik wrote:
>> When working with symlinks it is fairly easy to get into a loop.
>> Don't.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>> src/qemu/qemu_domain.c | 28 
>> 1 file changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 8cbfb2d16..448583313 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -6954,9 +6954,10 @@ qemuDomainGetPreservedMounts(virQEMUDriverPtr
>> driver,
>>
>>
>> static int
>> -qemuDomainCreateDevice(const char *device,
>> -   const char *path,
>> -   bool allow_noent)
>> +qemuDomainCreateDeviceRecursive(const char *device,
>> +const char *path,
>> +bool allow_noent,
>> +unsigned int ttl)
>> {
>> char *devicePath = NULL;
>> char *target = NULL;
>> @@ -6968,6 +6969,13 @@ qemuDomainCreateDevice(const char *device,
>> char *tcon = NULL;
>> #endif
>>
>> +if (!ttl) {
>> +virReportSystemError(ELOOP,
>> + _("Too many levels of symbolic links: %s"),
>> + device);
>> +return ret;
>> +}
>> +
>> if (lstat(device, ) < 0) {
>> if (errno == ENOENT && allow_noent) {
>> /* Ignore non-existent device. */
>> @@ -7057,7 +7065,8 @@ qemuDomainCreateDevice(const char *device,
>> tmp = NULL;
>> }
>>
>> -if (qemuDomainCreateDevice(target, path, allow_noent) < 0)
>> +if (qemuDomainCreateDeviceRecursive(target, path,
>> +allow_noent, ttl - 1) < 0)
>> goto cleanup;
>> } else {
>> if (create &&
>> @@ -7128,6 +7137,17 @@ qemuDomainCreateDevice(const char *device,
>> }
>>
>>
>> +static int
>> +qemuDomainCreateDevice(const char *device,
>> +   const char *path,
>> +   bool allow_noent)
>> +{
>> +long symloop_max = sysconf(_SC_SYMLOOP_MAX);
>> +
>> +return qemuDomainCreateDeviceRecursive(device, path,
>> +   allow_noent, symloop_max);
> 
> So you are taking 'long' that can, officially, be '-1' and pass it to a
> function as unsigned int.  Having a maximum is nice, I have no idea why
> on my system there is no limit apparently (sysconf() returns -1 with no
> errno set), but if the system doesn't report any (like my case above) it
> effectively makes the SYMLOOP_MAX = UINT_MAX in this codepath.  Should
> we avoid that or just let it be?  This way it's still better than
> unlimited, so ACK to this version, but I just wanted a (short)
> discussion about this.

Sure. I think it's safe to assume if the level of symlinks reaches
UINT_MAX your system is terribly broken.

Michal

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

Re: [libvirt] [PATCH 09/10] qemuDomainAttachDeviceMknod: Deal with symlinks

[Michal Privoznik]

On 02/07/2017 11:57 AM, Martin Kletzander wrote:
> On Fri, Jan 20, 2017 at 10:42:49AM +0100, Michal Privoznik wrote:
>> Similarly to one of the previous commits, we need to deal
>> properly with symlinks in hotplug case too.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>> src/qemu/qemu_domain.c | 120
>> ++---
>> 1 file changed, 94 insertions(+), 26 deletions(-)
>>
> 
> ACK to this, but ...
> 
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 448583313..bcfb2446f 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -7701,17 +7763,22 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr
>> driver,
>> }
>> #endif
>>
>> -if (virSecurityManagerPreFork(driver->securityManager) < 0)
>> -goto cleanup;
>> +if (STRPREFIX(file, DEVPREFIX)) {
>> +if (virSecurityManagerPreFork(driver->securityManager) < 0)
>> +goto cleanup;
>>
>> -if (virProcessRunInMountNamespace(vm->pid,
>> -  qemuDomainAttachDeviceMknodHelper,
>> -  ) < 0) {
>> +if (virProcessRunInMountNamespace(vm->pid,
>> + 
>> qemuDomainAttachDeviceMknodHelper,
>> +  ) < 0) {
> 
> ... I'm sure you have patches for this somewhere that are not posted or
> something =D However now we actually fork for every level of the
> symlink.  Even when everyone is scared of every single fork().  Can't we
> use transactions for this as well?  If not, could we enhance them so
> that we can use them?

Transactions are security driver specific. But we can imitate them here
too. Instead of direct fork() we would have a list to which we append
all the symlinks we want to create and then fork() once and execute the
list. Good point. I will work on that.

Michal

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

Re: [libvirt] [resend v2 0/7] Support cache tune in libvirt

[Martin Kletzander]

On Mon, Feb 06, 2017 at 10:23:35AM +0800, Eli Qiao wrote:

This series patches are for supportting CAT featues, which also
called cache tune in libvirt.

First to expose cache information which could be tuned in capabilites XML.
Then add new domain xml element support to add cacahe bank which will apply
on this libvirt domain.

This series patches add a util file `resctrl.c/h`, an interface to talk with
linux kernel's sys fs.

There are still some TODOs such as expose new public interface to get free
cache information.

Some discussion about this feature support can be found from:
https://www.redhat.com/archives/libvir-list/2017-January/msg00644.html

Eli Qiao (7):
 Resctrl: Add some utils functions
 Resctrl: expose cache information to capabilities
 Resctrl: Add new xml element to support cache tune
 Resctrl: Add private interface to set cachebanks
 Qemu: Set cache banks
 Resctrl: enable l3code/l3data
 Resctrl: Make sure l3data/l3code are pairs

docs/schemas/domaincommon.rng |   41 ++
include/libvirt/virterror.h   |1 +
src/Makefile.am   |1 +
src/conf/capabilities.c   |   56 +++
src/conf/capabilities.h   |   23 +
src/conf/domain_conf.c|  154 +++
src/conf/domain_conf.h|   18 +
src/libvirt_private.syms  |   10 +
src/qemu/qemu_capabilities.c  |   68 +++
src/qemu/qemu_driver.c|4 +
src/qemu/qemu_process.c   |   11 +
src/util/virerror.c   |1 +
src/util/virresctrl.c | 1019 +
src/util/virresctrl.h |  135 ++
14 files changed, 1542 insertions(+)
create mode 100644 src/util/virresctrl.c
create mode 100644 src/util/virresctrl.h



I see that Daniel didn't mention this, so just on a side note, we tend
to add documentation in the same patch as the parsing/formatting code,
so that it's consistent.  Also there could be at least xml2xml test case
added.


--
1.9.1

--
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 v2 1/4] docs: schema: Add basic documentation for the virtual crypto device support

[Martin Kletzander]

On Wed, Jan 11, 2017 at 04:28:23PM +0800, Longpeng(Mike) wrote:

This patch documents XML elements used for support of virtual
crypto devices.

In the devices section in the domain XML users may specify:
 
   
 
to enable the crypto device for guests.

Signed-off-by: Longpeng(Mike) 
---
docs/formatdomain.html.in | 60 +++
docs/schemas/domaincommon.rng | 27 +++
2 files changed, 87 insertions(+)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 39f5a88..1ad666c 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -7081,6 +7081,66 @@ qemu-kvm -net nic,model=? /dev/null
  


+Crypto device
+
+
+  The virtual crypto device is a kind of virtual hardware for
+  virtual machines and it can be added to the guest via the
+  crypto element.


"kind of a virtual hardware" doesn't tell me anything about it.


+  Since 3.0.0, QEMU and KVM only
+
+
+
+  Example: usage of the Crypto device:
+
+
+  ...
+  devices
+crypto model='virtio'
+  backend type='builtin' queues='1'/
+/crypto
+  /devices
+  ...
+
+
+  model
+  
+
+  The required model attribute specifies what
+  type of crypto device is provide. Currently the valid values





+  are:
+
+
+  'virtio'  needs virtio-crypto guest driver


list of values with one item, just throw away the list and jspecify it
inline.


+
+  
+  backend
+  
+
+  The backend element specifies the type and
+  number of queues of the crypto device to be used for the
+  domain.
+
+
+  type
+  
+
+The required type element specifies the
+type of the crypto device.


What types are possible?  Only builtin?  That should be specified here.
Also "builtin" is very non-descriptive.


+
+  
+  queues
+  
+
+The optional queues element specifies the
+number of queues of the crypto device, the default number
+of queues is 1.
+
+  
+
+  
+
+
Security label


diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index be0a609..0878245 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4320,6 +4320,7 @@



+
  


@@ -4804,6 +4805,32 @@

  

+  
+
+  
+
+  virtio
+
+  
+  
+
+  
+
+  
+
+  
+
+  builtin
+
+  
+  
+
+  
+
+  
+
+  
+
  

  
--
1.8.3.1


--
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] qemu: Forbid without

[Pavel Hrdina]

On Tue, Feb 07, 2017 at 08:43:56AM +0100, Jiri Denemark wrote:
> On Mon, Feb 06, 2017 at 18:29:59 +0100, Andrea Bolognani wrote:
> > In order for memory locking to work, the hard limit on memory
> > locking (and usage) has to be set appropriately by the user.
> > 
> > The documentation mentions the requirement already: with this
> > patch, it's going to be enforced by runtime checks as well.
> > 
> > Note that this will make existing guests that don't satisfy
> > the requirement disappear; that said, such guests have never
> > been able to start in the first place.
> 
> Yes, could not start or if they started they would just crash
> afterwards. But with your patch they would just disappear and the user
> would not even be able to fix the XML. I think the check should be done
> prior to starting a domain to avoid this unfortunate behaviour.
> 
> Jirka

Actually there is a third option, we have domainValidateCallback
that is what you are looking for.  This function is called only
when defining new XML and is skipped while parsing the XML from
disk on libvirtd startup or while restoring from snapshot and etc.

It is also called before domain is started.

Pavel

> 
> --
> 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 05/10] qemu_security: Use more transactions

[Martin Kletzander]

On Tue, Feb 07, 2017 at 11:02:09AM +0100, Michal Privoznik wrote:

On 02/02/2017 04:03 PM, Martin Kletzander wrote:

On Fri, Jan 20, 2017 at 10:42:45AM +0100, Michal Privoznik wrote:

The idea is to move all the seclabel setting to security driver.
Having the relabel code spread all over the place looks very
messy.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_security.c | 112
+--
1 file changed, 80 insertions(+), 32 deletions(-)

diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 13d99cdbd..9d1a87971 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -90,14 +90,26 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
 virDomainDiskDefPtr disk)
{
-if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) {
-/* Already handled by namespace code. */
-return 0;
-}
+int ret = -1;

-return virSecurityManagerSetDiskLabel(driver->securityManager,
+if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+virSecurityManagerTransactionStart(driver->securityManager) < 0)
+goto cleanup;
+
+if (virSecurityManagerSetDiskLabel(driver->securityManager,
  vm->def,
-  disk);
+  disk) < 0)


indentation


+goto cleanup;
+
+if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+virSecurityManagerTransactionCommit(driver->securityManager,
+vm->pid) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+virSecurityManagerTransactionAbort(driver->securityManager);
+return ret;
}



So much code duplication.


I have a patch ready that kills that and replaces it with a macro.


 Wasn't there an idea to have a callback in
the security manager that would be called before/after the labelling?


The problem is that security driver sees just virDomainDef while all the
namespace stuff (e.g. pid, enabled namespace bitmap) lives in a domain
object. Therefore security driver can't make such decision on its own.
Thus transactions were born.

Having a chown callback that would enter the namespace and change
ownership proved to be very suboptimal: entering a namespace requires a
fork(). Therefore, instead of forking like crazy, a list of all the
desired chown()-s is constructed, one fork() is called and then all the
operations from the list are performed.


Since this is only for a disk and hostdev, let's leave it like this, I
guess, but I'm expecting this much code duplication to bite us back in a
while.  None of my ideas for how to make this better are finalized, but
I have some, if you want to discuss.


Sure. I'm up for making this better.



When thinking about them after a while none of them are very clean.
Let's see how it looks with your macros for now.


Michal


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

Re: [libvirt] [PATCH 10/10] qemuDomainAttachDeviceMknod: Don't loop endlessly

[Martin Kletzander]

On Fri, Jan 20, 2017 at 10:42:50AM +0100, Michal Privoznik wrote:

When working with symlinks it is fairly easy to get into a loop.
Don't.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_domain.c | 32 +++-
1 file changed, 27 insertions(+), 5 deletions(-)



ACK, same worries as before, but no better soluton exists in my mind.


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

Re: [libvirt] [PATCH 09/10] qemuDomainAttachDeviceMknod: Deal with symlinks

[Martin Kletzander]

On Fri, Jan 20, 2017 at 10:42:49AM +0100, Michal Privoznik wrote:

Similarly to one of the previous commits, we need to deal
properly with symlinks in hotplug case too.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_domain.c | 120 ++---
1 file changed, 94 insertions(+), 26 deletions(-)



ACK to this, but ...


diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 448583313..bcfb2446f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7701,17 +7763,22 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
}
#endif

-if (virSecurityManagerPreFork(driver->securityManager) < 0)
-goto cleanup;
+if (STRPREFIX(file, DEVPREFIX)) {
+if (virSecurityManagerPreFork(driver->securityManager) < 0)
+goto cleanup;

-if (virProcessRunInMountNamespace(vm->pid,
-  qemuDomainAttachDeviceMknodHelper,
-  ) < 0) {
+if (virProcessRunInMountNamespace(vm->pid,
+  qemuDomainAttachDeviceMknodHelper,
+  ) < 0) {


... I'm sure you have patches for this somewhere that are not posted or
something =D However now we actually fork for every level of the
symlink.  Even when everyone is scared of every single fork().  Can't we
use transactions for this as well?  If not, could we enhance them so
that we can use them?


+virSecurityManagerPostFork(driver->securityManager);
+goto cleanup;
+}
virSecurityManagerPostFork(driver->securityManager);
-goto cleanup;
}

-virSecurityManagerPostFork(driver->securityManager);
+if (isLink &&
+qemuDomainAttachDeviceMknod(driver, vm, devDef, target) < 0)
+goto cleanup;

ret = 0;
 cleanup:


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

Re: [libvirt] [resend v2 4/7] Resctrl: Add private interface to set cachebanks

[Eli Qiao]

--  
Eli Qiao
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)


On Tuesday, 7 February 2017 at 6:15 PM, Daniel P. Berrange wrote:

> On Tue, Feb 07, 2017 at 02:43:04PM +0800, Eli Qiao wrote:
> > On Tuesday, 7 February 2017 at 12:17 AM, Daniel P. Berrange wrote:
> >  
> > > On Mon, Feb 06, 2017 at 10:23:39AM +0800, Eli Qiao wrote:
> > > > virResCtrlSetCacheBanks: Set cache banks of a libvirt domain. It will
> > > > create new resource domain under `/sys/fs/resctrl` and fill the
> > > > schemata according the cache banks configration.
> > > >  
> > > > virResCtrlUpdate: Update the schemata after libvirt domain destroy.
> > > >  
> > > > Signed-off-by: Eli Qiao  > > > (mailto:liyong.q...@intel.com)>
> > > > ---
> > > > src/libvirt_private.syms | 2 +
> > > > src/util/virresctrl.c | 644 
> > > > ++-
> > > > src/util/virresctrl.h | 47 +++-
> > > > 3 files changed, 691 insertions(+), 2 deletions(-)
> > > >  
> > >  
> > >  
> > >  
> > > > diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> > > > index 3cc41da..11f43d8 100644
> > > > --- a/src/util/virresctrl.h
> > > > +++ b/src/util/virresctrl.h
> > > > @@ -26,6 +26,7 @@
> > > >  
> > > > # include "virutil.h"
> > > > # include "virbitmap.h"
> > > > +# include "domain_conf.h"
> > > >  
> > > > #define RESCTRL_DIR "/sys/fs/resctrl"
> > > > #define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"
> > > > @@ -82,9 +83,53 @@ struct _virResCtrl {
> > > > virResCacheBankPtr cache_banks;
> > > > };
> > > >  
> > > > +/**
> > > > + * a virResSchemata represents a schemata object under a resource 
> > > > control
> > > > + * domain.
> > > > + */
> > > > +typedef struct _virResSchemataItem virResSchemataItem;
> > > > +typedef virResSchemataItem *virResSchemataItemPtr;
> > > > +struct _virResSchemataItem {
> > > > + unsigned int socket_no;
> > > > + unsigned schemata;
> > > > +};
> > > > +
> > > > +typedef struct _virResSchemata virResSchemata;
> > > > +typedef virResSchemata *virResSchemataPtr;
> > > > +struct _virResSchemata {
> > > > + unsigned int n_schemata_items;
> > > > + virResSchemataItemPtr schemata_items;
> > > > +};
> > > > +
> > > > +/**
> > > > + * a virResDomain represents a resource control domain. It's a double 
> > > > linked
> > > > + * list.
> > > > + */
> > > > +
> > > > +typedef struct _virResDomain virResDomain;
> > > > +typedef virResDomain *virResDomainPtr;
> > > > +
> > > > +struct _virResDomain {
> > > > + char* name;
> > > > + virResSchemataPtr schematas[RDT_NUM_RESOURCES];
> > > > + char* tasks;
> > > > + int n_sockets;
> > > > + virResDomainPtr pre;
> > > > + virResDomainPtr next;
> > > > +};
> > > > +
> > > > +/* All resource control domains on this host*/
> > > > +
> > > > +typedef struct _virResCtrlDomain virResCtrlDomain;
> > > > +typedef virResCtrlDomain *virResCtrlDomainPtr;
> > > > +struct _virResCtrlDomain {
> > > > + unsigned int num_domains;
> > > > + virResDomainPtr domains;
> > > > +};
> > > >  
> > >  
> > >  
> > >  
> > > I don't think any of these need to be in the header file - they're
> > > all private structs used only by the .c file.
> > >  
> >  
> > Yep.  
> > > The biggest issue I see here is a complete lack of any kind of
> > > mutex locking. Libvirt is multi-threaded, so you must expect to
> > > have virResCtrlSetCacheBanks() and virResCtrlUpdate called
> > > concurrently and thus use mutexes to ensure safety.
> > >  
> >  
> > okay.  
> > > With the data structure design of using linked list of virResDomain
> > > though, doing good locking is going to be hard. It'll force you to
> > > have a single global mutex across the whole set of data structures
> > > which will really harm concurrency for VM startup.
> > >  
> > > Really each virResDomain needs to be completely standalone, with
> > > its own dedicate mutex. A global mutex for virResCtrlDomain can
> > > be acquired whle you lookup the virResDomain object to use. Thereafter
> > > the global mutex should be released and only the mutex for the specific
> > > domain used.
> > >  
> >  
> >  
> > oh, yes, but lock is really a hard thing, I need to be careful to avoid 
> > dead lock.  
> > >  
> > > > bool virResCtrlAvailable(void);
> > > > int virResCtrlInit(void);
> > > > virResCtrlPtr virResCtrlGet(int);
> > > > -
> > > > +int virResCtrlSetCacheBanks(virDomainCachetunePtr, unsigned char*, 
> > > > pid_t);
> > > > +int virResCtrlUpdate(void);
> > > >  
> > >  
> > >  
> > >  
> > > This API design doesn't really make locking very easy - since you are not
> > > passing any info into the virResCtrlUpdate() method you've created the
> > > need to do global rescans when updating.
> > >  
> >  
> >  
> >  
> > yes, what if I use a global mutex variable in virresctrl.c?
>  
> I'd like to see finer grained locking if possible. We try really hard to
> make guest startup be highly parallizeable, so any global locks that will
> be required by all VMs starting hurts that.
>  
hi Daniel, thanks for 

Re: [libvirt] [PATCH 08/10] qemuDomainCreateDevice: Don't loop endlessly

[Martin Kletzander]

On Fri, Jan 20, 2017 at 10:42:48AM +0100, Michal Privoznik wrote:

When working with symlinks it is fairly easy to get into a loop.
Don't.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_domain.c | 28 
1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8cbfb2d16..448583313 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6954,9 +6954,10 @@ qemuDomainGetPreservedMounts(virQEMUDriverPtr driver,


static int
-qemuDomainCreateDevice(const char *device,
-   const char *path,
-   bool allow_noent)
+qemuDomainCreateDeviceRecursive(const char *device,
+const char *path,
+bool allow_noent,
+unsigned int ttl)
{
char *devicePath = NULL;
char *target = NULL;
@@ -6968,6 +6969,13 @@ qemuDomainCreateDevice(const char *device,
char *tcon = NULL;
#endif

+if (!ttl) {
+virReportSystemError(ELOOP,
+ _("Too many levels of symbolic links: %s"),
+ device);
+return ret;
+}
+
if (lstat(device, ) < 0) {
if (errno == ENOENT && allow_noent) {
/* Ignore non-existent device. */
@@ -7057,7 +7065,8 @@ qemuDomainCreateDevice(const char *device,
tmp = NULL;
}

-if (qemuDomainCreateDevice(target, path, allow_noent) < 0)
+if (qemuDomainCreateDeviceRecursive(target, path,
+allow_noent, ttl - 1) < 0)
goto cleanup;
} else {
if (create &&
@@ -7128,6 +7137,17 @@ qemuDomainCreateDevice(const char *device,
}


+static int
+qemuDomainCreateDevice(const char *device,
+   const char *path,
+   bool allow_noent)
+{
+long symloop_max = sysconf(_SC_SYMLOOP_MAX);
+
+return qemuDomainCreateDeviceRecursive(device, path,
+   allow_noent, symloop_max);


So you are taking 'long' that can, officially, be '-1' and pass it to a
function as unsigned int.  Having a maximum is nice, I have no idea why
on my system there is no limit apparently (sysconf() returns -1 with no
errno set), but if the system doesn't report any (like my case above) it
effectively makes the SYMLOOP_MAX = UINT_MAX in this codepath.  Should
we avoid that or just let it be?  This way it's still better than
unlimited, so ACK to this version, but I just wanted a (short)
discussion about this.


+}
+

static int
qemuDomainPopulateDevices(virQEMUDriverPtr driver,
--
2.11.0

--
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] libxl: fix disk detach when not specified

[Michal Privoznik]

On 02/06/2017 11:08 PM, Jim Fehlig wrote:
> When a user does not explicitly set a  in the disk config,
> libvirt defers selection of a default to libxl. This approach works
> fine when starting a domain with such configuration or attaching a
> disk to a running domain. But when detaching such a disk, libxl
> will fail with "unrecognized disk backend type: 0". libxl makes no
> attempt to recalculate a default backend (driver) on detach and
> simply fails when uninitialized.
> 
> This patch updates the libvirt disk config with the backend selected
> by libxl when starting a domain or attaching a disk to a running
> domain. Another benefit of this approach is that the live XML is
> also updated with the backend driver selected by libxl.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  src/libxl/libxl_conf.c   | 33 +
>  src/libxl/libxl_conf.h   |  4 
>  src/libxl/libxl_domain.c | 25 +
>  src/libxl/libxl_driver.c |  2 +-
>  4 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index b5186f2..6ef7570 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -851,6 +851,7 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, 
> libxl_device_disk *x_disk)
>   * xl-disk-configuration.txt in the xen documentation and let
>   * libxl pick a suitable backend.
>   */
> +virDomainDiskSetFormat(l_disk, VIR_STORAGE_FILE_RAW);
>  x_disk->format = LIBXL_DISK_FORMAT_RAW;
>  x_disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;

This doesn't feel right. I know what do you want it here for, but this
function is meant to take 'const' disk definition and translate it to
libxl definition. Although, I don't have a better suggestion where to
put it.

>  }
> @@ -913,6 +914,38 @@ libxlMakeDiskList(virDomainDefPtr def, 
> libxl_domain_config *d_config)
>  return -1;
>  }
>  

The rest looks okay. ACK.

Michal

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

Re: [libvirt] [PATCH v2] docs: mention bhyve SATA address changes in news.xml

[Andrea Bolognani]

On Mon, 2017-02-06 at 21:49 +0400, Roman Bogorodskiy wrote:
[...]
> +  However, as this doesn't match libvirt's understanding of
> +  disk addresses, it was changed for the bhyve driver

  "it was changed for the bhyve driver"
  → "the bhyve driver was changed"

Sorry for not catching it the first time around :/

ACK 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 07/10] qemuDomainCreateDevice: Properly deal with symlinks

[Martin Kletzander]

On Fri, Jan 20, 2017 at 10:42:47AM +0100, Michal Privoznik wrote:

Imagine you have a disk with the following source set up:

/dev/disk/by-uuid/$uuid (symlink to) -> /dev/sda

After cbc45525cb21 the transitive end of the symlink chain is
created (/dev/sda), but we need to create any item in chain too.
Others might rely on that.
In this case, /dev/disk/by-uuid/$uuid comes from domain XML thus
it is this path that secdriver tries to relabel. Not the resolved
one.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_domain.c | 150 +++--
1 file changed, 108 insertions(+), 42 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0f45f753e..8cbfb2d16 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -68,6 +68,7 @@
#endif

#include 
+#include "dosname.h"

#define VIR_FROM_THIS VIR_FROM_QEMU

@@ -6958,70 +6959,135 @@ qemuDomainCreateDevice(const char *device,
   bool allow_noent)
{
char *devicePath = NULL;
-char *canonDevicePath = NULL;
+char *target = NULL;
struct stat sb;
int ret = -1;
+bool isLink = false;
+bool create = false;
#ifdef WITH_SELINUX
char *tcon = NULL;
#endif

-if (virFileResolveAllLinks(device, ) < 0) {
+if (lstat(device, ) < 0) {
if (errno == ENOENT && allow_noent) {
/* Ignore non-existent device. */
-ret = 0;
-goto cleanup;
+return 0;
}
-
-virReportError(errno, _("Unable to canonicalize %s"), device);
-goto cleanup;
-}
-
-if (!STRPREFIX(canonDevicePath, DEVPREFIX)) {
-ret = 0;
-goto cleanup;
+virReportSystemError(errno, _("Unable to stat %s"), device);
+return ret;
}

-if (virAsprintf(, "%s/%s",
-path, canonDevicePath + strlen(DEVPREFIX)) < 0)
-goto cleanup;
+isLink = S_ISLNK(sb.st_mode);

-if (stat(canonDevicePath, ) < 0) {
-if (errno == ENOENT && allow_noent) {
-/* Ignore non-existent device. */
-ret = 0;
+/* Here, @device might be whatever path in the system. We
+ * should create the path in the namespace iff its "/dev"


s/its/it's/

ACK


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

Re: [libvirt] [resend v2 0/7] Support cache tune in libvirt

[Daniel P. Berrange]

On Tue, Feb 07, 2017 at 02:43:13PM +0800, Eli Qiao wrote:
> > 3) CDP / non-CDP convertion.
> >  
> > In case the size determination has been performed with non-CDP,
> > to emulate such allocation on a CDP host,
> > it would be good to allow both code and data allocations to share
> > the CBM space:  
> >  
> IOM, I don’t think it’s good to have this.
> in libvirt capabilities xml, the application will get to know if the host 
> support cdp or not.

Yep, as long as the capabilities XML provide info about the different
cache banks, IMHO it is better to have the application be explicit
about what they want for CDP & non-CDP scenarios.  Let the higher
level mgmt apps above libvirt apply specific policies if they desire


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] [resend v2 4/7] Resctrl: Add private interface to set cachebanks

[Daniel P. Berrange]

On Tue, Feb 07, 2017 at 02:43:04PM +0800, Eli Qiao wrote:
> On Tuesday, 7 February 2017 at 12:17 AM, Daniel P. Berrange wrote:
> 
> > On Mon, Feb 06, 2017 at 10:23:39AM +0800, Eli Qiao wrote:
> > > virResCtrlSetCacheBanks: Set cache banks of a libvirt domain. It will
> > > create new resource domain under `/sys/fs/resctrl` and fill the
> > > schemata according the cache banks configration.
> > > 
> > > virResCtrlUpdate: Update the schemata after libvirt domain destroy.
> > > 
> > > Signed-off-by: Eli Qiao  > > (mailto:liyong.q...@intel.com)>
> > > ---
> > > src/libvirt_private.syms | 2 +
> > > src/util/virresctrl.c | 644 
> > > ++-
> > > src/util/virresctrl.h | 47 +++-
> > > 3 files changed, 691 insertions(+), 2 deletions(-)
> > > 
> > 
> > 
> > > diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> > > index 3cc41da..11f43d8 100644
> > > --- a/src/util/virresctrl.h
> > > +++ b/src/util/virresctrl.h
> > > @@ -26,6 +26,7 @@
> > > 
> > > # include "virutil.h"
> > > # include "virbitmap.h"
> > > +# include "domain_conf.h"
> > > 
> > > #define RESCTRL_DIR "/sys/fs/resctrl"
> > > #define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"
> > > @@ -82,9 +83,53 @@ struct _virResCtrl {
> > > virResCacheBankPtr cache_banks;
> > > };
> > > 
> > > +/**
> > > + * a virResSchemata represents a schemata object under a resource control
> > > + * domain.
> > > + */
> > > +typedef struct _virResSchemataItem virResSchemataItem;
> > > +typedef virResSchemataItem *virResSchemataItemPtr;
> > > +struct _virResSchemataItem {
> > > + unsigned int socket_no;
> > > + unsigned schemata;
> > > +};
> > > +
> > > +typedef struct _virResSchemata virResSchemata;
> > > +typedef virResSchemata *virResSchemataPtr;
> > > +struct _virResSchemata {
> > > + unsigned int n_schemata_items;
> > > + virResSchemataItemPtr schemata_items;
> > > +};
> > > +
> > > +/**
> > > + * a virResDomain represents a resource control domain. It's a double 
> > > linked
> > > + * list.
> > > + */
> > > +
> > > +typedef struct _virResDomain virResDomain;
> > > +typedef virResDomain *virResDomainPtr;
> > > +
> > > +struct _virResDomain {
> > > + char* name;
> > > + virResSchemataPtr schematas[RDT_NUM_RESOURCES];
> > > + char* tasks;
> > > + int n_sockets;
> > > + virResDomainPtr pre;
> > > + virResDomainPtr next;
> > > +};
> > > +
> > > +/* All resource control domains on this host*/
> > > +
> > > +typedef struct _virResCtrlDomain virResCtrlDomain;
> > > +typedef virResCtrlDomain *virResCtrlDomainPtr;
> > > +struct _virResCtrlDomain {
> > > + unsigned int num_domains;
> > > + virResDomainPtr domains;
> > > +};
> > > 
> > 
> > 
> > I don't think any of these need to be in the header file - they're
> > all private structs used only by the .c file.
> > 
> Yep. 
> > The biggest issue I see here is a complete lack of any kind of
> > mutex locking. Libvirt is multi-threaded, so you must expect to
> > have virResCtrlSetCacheBanks() and virResCtrlUpdate called
> > concurrently and thus use mutexes to ensure safety.
> > 
> okay. 
> > With the data structure design of using linked list of virResDomain
> > though, doing good locking is going to be hard. It'll force you to
> > have a single global mutex across the whole set of data structures
> > which will really harm concurrency for VM startup.
> > 
> > Really each virResDomain needs to be completely standalone, with
> > its own dedicate mutex. A global mutex for virResCtrlDomain can
> > be acquired whle you lookup the virResDomain object to use. Thereafter
> > the global mutex should be released and only the mutex for the specific
> > domain used.
> > 
> > 
> > 
> 
> oh, yes, but lock is really a hard thing, I need to be careful to avoid dead 
> lock. 
> > 
> > > bool virResCtrlAvailable(void);
> > > int virResCtrlInit(void);
> > > virResCtrlPtr virResCtrlGet(int);
> > > -
> > > +int virResCtrlSetCacheBanks(virDomainCachetunePtr, unsigned char*, 
> > > pid_t);
> > > +int virResCtrlUpdate(void);
> > > 
> > 
> > 
> > This API design doesn't really make locking very easy - since you are not
> > passing any info into the virResCtrlUpdate() method you've created the
> > need to do global rescans when updating.
> > 
> > 
> > 
> 
> 
> yes, what if I use a global mutex variable in virresctrl.c?

I'd like to see finer grained locking if possible. We try really hard to
make guest startup be highly parallizeable, so any global locks that will
be required by all VMs starting hurts that.

> > IMHO virResCtrlSetCacheBanks needs to return an object that represents the
> > config for that particular VM. This can be passed back in, when the guest
> > is shutdown to release resources once again, avoiding any global scans.
> 
> Hmm.. what object should virResCtrlSetCacheBanks return? schemata setting?

Well it might not need to return an object neccessarily. Perhaps you can
just pass the VM PID into the method when your shutdown instead, and have
a hash table keyed off PID to 

Re: [libvirt] [resend v2 1/7] Resctrl: Add some utils functions

[Daniel P. Berrange]

On Tue, Feb 07, 2017 at 02:43:17PM +0800, Eli Qiao wrote:
> On Tuesday, 7 February 2017 at 12:11 AM, Daniel P. Berrange wrote:
> 
> > On Mon, Feb 06, 2017 at 10:23:36AM +0800, Eli Qiao wrote:
> > > This patch adds some utils struct and functions to expose resctrl
> > > information.
> > > 
> > > virResCtrlAvailable: If resctrl interface exist on host
> > > virResCtrlGet: get specify type resource contral information
> > > virResCtrlInit: initialize resctrl struct from the host's sys fs.
> > > ResCtrlAll[]: an array to maintain resource control information.
> > > 
> > > Signed-off-by: Eli Qiao  > > (mailto:liyong.q...@intel.com)>
> > > ---
> > > +};
> > > +
> > > +
> > > +typedef struct _virResCacheBank virResCacheBank;
> > > +typedef virResCacheBank *virResCacheBankPtr;
> > > +struct _virResCacheBank {
> > > + unsigned int host_id;
> > > + unsigned long long cache_size;
> > > + unsigned long long cache_left;
> > > + unsigned long long cache_min;
> > > + virBitmapPtr cpu_mask;
> > > +};
> > > 
> > 
> > 
> > > +typedef struct _virResCtrl virResCtrl;
> > > +typedef virResCtrl *virResCtrlPtr;
> > > +struct _virResCtrl {
> > > + bool enabled;
> > > + const char *name;
> > > + int num_closid;
> > > + int cbm_len;
> > > + int min_cbm_bits;
> > > + const char* cache_level;
> > > + int num_banks;
> > > + virResCacheBankPtr cache_banks;
> > > +};
> > > 
> > 
> > 
> > Can any of these fields change at runtime, or are they all
> > immutable once populated.
> 
> Only cache_banks will be change at runtime, such as cache_left
> on each socket will be updated if libvirt allocates cache to domains.

Ok, so the API is returning a direct point to the virResCtrlPtr
struct and there's no locking here. So if cache_banks struct
contents changes while some caller is reading the virResCtrlPtr
struct we have race problems.

I'd suggest that best option is probably to take the mutable
cache_left field *out* of the struct. That lets the callers
treat the entire struct as immutable.

Then add a separate API to explicitly query the cache_left
value - only that api needs to do locking to return a consistent
point-in-time view of the cache_left value.


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] [PATCH 06/10] qemuDomain{Attach, Detach}Device NS helpers: Don't relabel devices

[Michal Privoznik]

On 02/07/2017 10:59 AM, Martin Kletzander wrote:
> On Fri, Jan 20, 2017 at 10:42:46AM +0100, Michal Privoznik wrote:
>> After previous commit this has become redundant step.
>> Also setting up devices in namespace and setting their label
>> later on are two different steps and should be not done at once.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>> src/qemu/qemu_domain.c | 112
>> +
>> 1 file changed, 2 insertions(+), 110 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index c67604222..0f45f753e 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -7707,67 +7655,11 @@ qemuDomainDetachDeviceUnlinkHelper(pid_t pid
>> ATTRIBUTE_UNUSED,
>>
>>
>> static int
>> -qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver,
>> +qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>>  virDomainObjPtr vm,
>> - virDomainDeviceDefPtr dev,
>> + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
> 
> Any reason for keeping these unused attributes here?  Your call on
> removing them (from caller as well, of course).  ACK either way.

As mentioned in previous e-mail, I have another patch set ready that
goes on the top of this and removes couple of unused variables/arguments
(mostly because I'm fixing another bug by calling
qemuDomainNamespaceSetupDisk() from a place where just
virStorageSourcePtr is available. So stay tuned :-)

Michal

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

Re: [libvirt] [PATCH 05/10] qemu_security: Use more transactions

[Michal Privoznik]

On 02/02/2017 04:03 PM, Martin Kletzander wrote:
> On Fri, Jan 20, 2017 at 10:42:45AM +0100, Michal Privoznik wrote:
>> The idea is to move all the seclabel setting to security driver.
>> Having the relabel code spread all over the place looks very
>> messy.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>> src/qemu/qemu_security.c | 112
>> +--
>> 1 file changed, 80 insertions(+), 32 deletions(-)
>>
>> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
>> index 13d99cdbd..9d1a87971 100644
>> --- a/src/qemu/qemu_security.c
>> +++ b/src/qemu/qemu_security.c
>> @@ -90,14 +90,26 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver,
>>  virDomainObjPtr vm,
>>  virDomainDiskDefPtr disk)
>> {
>> -if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) {
>> -/* Already handled by namespace code. */
>> -return 0;
>> -}
>> +int ret = -1;
>>
>> -return virSecurityManagerSetDiskLabel(driver->securityManager,
>> +if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> +virSecurityManagerTransactionStart(driver->securityManager) < 0)
>> +goto cleanup;
>> +
>> +if (virSecurityManagerSetDiskLabel(driver->securityManager,
>>   vm->def,
>> -  disk);
>> +  disk) < 0)
> 
> indentation
> 
>> +goto cleanup;
>> +
>> +if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> +virSecurityManagerTransactionCommit(driver->securityManager,
>> +vm->pid) < 0)
>> +goto cleanup;
>> +
>> +ret = 0;
>> + cleanup:
>> +virSecurityManagerTransactionAbort(driver->securityManager);
>> +return ret;
>> }
>>
> 
> So much code duplication.

I have a patch ready that kills that and replaces it with a macro.

>  Wasn't there an idea to have a callback in
> the security manager that would be called before/after the labelling?

The problem is that security driver sees just virDomainDef while all the
namespace stuff (e.g. pid, enabled namespace bitmap) lives in a domain
object. Therefore security driver can't make such decision on its own.
Thus transactions were born.

Having a chown callback that would enter the namespace and change
ownership proved to be very suboptimal: entering a namespace requires a
fork(). Therefore, instead of forking like crazy, a list of all the
desired chown()-s is constructed, one fork() is called and then all the
operations from the list are performed.

> Since this is only for a disk and hostdev, let's leave it like this, I
> guess, but I'm expecting this much code duplication to bite us back in a
> while.  None of my ideas for how to make this better are finalized, but
> I have some, if you want to discuss.

Sure. I'm up for making this better.

Michal

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

Re: [libvirt] [PATCH 06/10] qemuDomain{Attach, Detach}Device NS helpers: Don't relabel devices

[Martin Kletzander]

On Fri, Jan 20, 2017 at 10:42:46AM +0100, Michal Privoznik wrote:

After previous commit this has become redundant step.
Also setting up devices in namespace and setting their label
later on are two different steps and should be not done at once.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_domain.c | 112 +
1 file changed, 2 insertions(+), 110 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c67604222..0f45f753e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7707,67 +7655,11 @@ qemuDomainDetachDeviceUnlinkHelper(pid_t pid 
ATTRIBUTE_UNUSED,


static int
-qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver,
+qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
 virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
+ virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,


Any reason for keeping these unused attributes here?  Your call on
removing them (from caller as well, of course).  ACK either way.


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

Re: [libvirt] [PATCH 02/10] util: Introduce virFileReadLink

[Martin Kletzander]

On Thu, Feb 02, 2017 at 08:57:53PM +0100, Martin Kletzander wrote:

On Thu, Feb 02, 2017 at 03:09:15PM +, Daniel P. Berrange wrote:

On Thu, Feb 02, 2017 at 03:11:56PM +0100, Martin Kletzander wrote:

On Fri, Jan 20, 2017 at 10:42:42AM +0100, Michal Privoznik wrote:
> We will need to traverse the symlinks one step at the time.
> Therefore we need to see where a symlink is pointing to.
>
> Signed-off-by: Michal Privoznik 
> ---
> src/libvirt_private.syms |  1 +
> src/util/virfile.c   | 12 
> src/util/virfile.h   |  2 ++
> 3 files changed, 15 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index cfeb43cf0..7f7dcfe44 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1615,6 +1615,7 @@ virFileReadAllQuiet;
> virFileReadBufQuiet;
> virFileReadHeaderFD;
> virFileReadLimFD;
> +virFileReadLink;
> virFileRelLinkPointsTo;
> virFileRemove;
> virFileRemoveLastComponent;
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index bf8099e34..49ea1d1f0 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -76,6 +76,7 @@
> #include "virutil.h"
>
> #include "c-ctype.h"
> +#include "areadlink.h"
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> @@ -1614,6 +1615,17 @@ virFileIsLink(const char *linkpath)
> return S_ISLNK(st.st_mode) != 0;
> }
>
> +/*
> + * Read where symlink is pointing to.
> + *
> + * Returns 0 on success (@linkpath is a successfully read link),
> + *-1 with errno set upon error.
> + */
> +int
> +virFileReadLink(const char *linkpath, char **resultpath)
> +{
> +return (*resultpath = areadlink(linkpath)) ? 0 : -1;
> +}
>

I don't really see the benefit of this function, are you trying to
obfuscate it?  You essentially double the return information for no
reason and try to push it all into one line.


It would be useful if you have an ATTRIBUTE_RETURN_CHECK annotation
on it, as it would allow compiler time verification of error checking,
which is not possible when you overload the return value to contain
both the data and error indicator.



More like if there was another logic, e.g. checking that it is a link
and if it is not, returning the same path or similar.

ATTRIBUTE_RETURN_CHECK doesn't make much sense to me here since you can
get the same information from the second parameter and you are basically
making the caller use two values that have the same information.
Moreover you can set ATTRIBUTE_RETURN_CHECK for the function and just
directly return the output of areadlink(), essentially just renaming
it.  I don't see the point in that.

It would be different if the function guaranteed non-modification of
that parameter when it errors out, for example.  Or set an call
virReportSystemError(errno, ...); so that the error reporting is done
consistently and in one place.  I haven't checked yet how Michal uses
it, though, so I can't say what's the best solution for now.

But I now see where it comes from.  It's the virFileResolveAllLinks()
that does the same thing.  That doesn't mean it's right, though.

My question is this.  If returning integer, which does not contain any
other value then just the error, is desired, should we mandate that for
non-simple functions and rewrite, for example, qemuBuildCommandLine()?


> /*
>  * Finds a requested executable file in the PATH env. e.g.:
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 0343acd5b..981a9e07d 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -166,6 +166,8 @@ int virFileResolveAllLinks(const char *linkpath,
> int virFileIsLink(const char *linkpath)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>
> +int virFileReadLink(const char *linkpath, char **resultpath);


eg add ATTRIBUTE_RETURN_CHECK here



So it seems like Michal is not participating in the discussion about his
own patch.  But I talked to him personally and he admitted that the
function prototype was meant to look like it does simply to be just like
the other functions, e.g. virFileResolveAllLinks().

So for now I'm OK with it, we can change how all the functions look
later on, but that's a discussion to be had.  So ACK with the
ATTRIBUTE_RETURN_CHECK added here.


> +
> char *virFindFileInPath(const char *file);
>
> char *virFileFindResource(const char *filename,



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


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

Re: [libvirt] [PATCH 5/5] xenFoxenFormatXLDisk: Don't leak @target

[Michal Privoznik]

On 02/06/2017 08:18 PM, Jim Fehlig wrote:
> Michal Privoznik wrote:
>> ==11260== 1,006 bytes in 1 blocks are definitely lost in loss record 106 of 
>> 111
>> ==11260==at 0x4C2AE5F: malloc (vg_replace_malloc.c:297)
>> ==11260==by 0x4C2BDFF: realloc (vg_replace_malloc.c:693)
>> ==11260==by 0x4EA430B: virReallocN (viralloc.c:245)
>> ==11260==by 0x4EA7C52: virBufferGrow (virbuffer.c:130)
>> ==11260==by 0x4EA7D28: virBufferAdd (virbuffer.c:165)
>> ==11260==by 0x4EA8E10: virBufferStrcat (virbuffer.c:718)
>> ==11260==by 0x42D263: xenFormatXLDiskSrcNet (xen_xl.c:960)
>> ==11260==by 0x42D4EB: xenFormatXLDiskSrc (xen_xl.c:1015)
>> ==11260==by 0x42D870: xenFormatXLDisk (xen_xl.c:1101)
>> ==11260==by 0x42DA89: xenFormatXLDomainDisks (xen_xl.c:1148)
>> ==11260==by 0x42EAF8: xenFormatXL (xen_xl.c:1558)
>> ==11260==by 0x40E85F: testCompareParseXML (xlconfigtest.c:105)
>>
>> Signed-off-by: Michal Privoznik 
> 
> Strange function name in $subject. s/xenFoxenFormatXLDisk/xenFormatXLDisk/

Ah, indeed. I've started writing the function name and then copy-pasted
it. I am a giddy goat :-)

> 
> ACK.

Thanks, I've pushed this one.

Michal

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

Re: [libvirt] [PATCH v2 0/4] Virtio-crypto device support

[Longpeng (Mike)]

Hi guys,

Does anyone have any comments ? :)

-- 
Regards,
Longpeng(Mike)

On 2017/1/11 16:28, Longpeng(Mike) wrote:

> As virtio-crypto has been supported in QEMU 2.8 and the frontend
> driver has been merged in linux 4.10, so it's necessary to support
> virtio-crypto in libvirt.
> 
> ---
> Changes since v1:
>   - split patch [Martin]
>   - rebase on master [Martin]
>   - add docs/tests/schema [Martin]
>   - fix typos [Gonglei]
> 
> ---
> Longpeng(Mike) (4):
>   docs: schema: Add basic documentation for the virtual crypto device
> support
>   conf: Parse virtio-crypto in the domain XML
>   qemu: Implement support for 'builtin' backend for virtio-crypto
>   tests: Add testcase for virtio-crypto XML parsing
> 
>  docs/formatdomain.html.in  |  60 ++
>  docs/schemas/domaincommon.rng  |  27 +++
>  src/conf/domain_conf.c | 213 
> -
>  src/conf/domain_conf.h |  32 
>  src/libvirt_private.syms   |   2 +
>  src/qemu/qemu_alias.c  |  20 ++
>  src/qemu/qemu_alias.h  |   3 +
>  src/qemu/qemu_capabilities.c   |   4 +
>  src/qemu/qemu_capabilities.h   |   2 +
>  src/qemu/qemu_command.c| 132 +
>  src/qemu/qemu_command.h|   3 +
>  src/qemu/qemu_domain.c |   2 +
>  src/qemu/qemu_domain_address.c |  25 +++
>  src/qemu/qemu_driver.c |   6 +
>  src/qemu/qemu_hotplug.c|   1 +
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml|   2 +
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   |   2 +
>  .../qemuxml2argv-virtio-crypto-builtin.xml |  26 +++
>  .../qemuxml2argv-virtio-crypto.args|  22 +++
>  .../qemuxml2xmlout-virtio-crypto-builtin.xml   |  31 +++
>  tests/qemuxml2xmltest.c|   2 +
>  21 files changed, 616 insertions(+), 1 deletion(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-virtio-crypto-builtin.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-crypto.args
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-crypto-builtin.xml
> 


-- 
Regards,
Longpeng(Mike)

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

[libvirt] [PATCH] qemu_capabilities: introduce QEMU_CAPS_SD_CARD to probe sd-card drivers

[Chen Hanxiao]

From: Chen Hanxiao 

This patch introduces QEMU_CAPS_SD_CARD for probing
whether qemu support SD card by:

{"execute": "device-list-properties",
"arguments":{"typename":"sd-card"}}

It will be helpful for apps which used
cmd 'virsh domcaps` etc.

Also helpful for:
https://bugzilla.redhat.com/show_bug.cgi?id=1387218

Signed-off-by: Chen Hanxiao 
---
 src/qemu/qemu_capabilities.c | 9 +++--
 src/qemu/qemu_capabilities.h | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 3247d25..b200b37 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -357,6 +357,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
 
   "query-cpu-model-expansion", /* 245 */
   "virtio-net.host_mtu",
+  "sd-card",
 );
 
 
@@ -1624,6 +1625,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
 { "ivshmem-plain", QEMU_CAPS_DEVICE_IVSHMEM_PLAIN },
 { "ivshmem-doorbell", QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL },
 { "vhost-scsi", QEMU_CAPS_DEVICE_VHOST_SCSI },
+{ "sd-card", QEMU_CAPS_SD_CARD },
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = {
@@ -5193,8 +5195,7 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr 
qemuCaps,
 VIR_DOMAIN_CAPS_ENUM_SET(disk->bus,
  VIR_DOMAIN_DISK_BUS_IDE,
  VIR_DOMAIN_DISK_BUS_SCSI,
- VIR_DOMAIN_DISK_BUS_VIRTIO,
- /* VIR_DOMAIN_DISK_BUS_SD */);
+ VIR_DOMAIN_DISK_BUS_VIRTIO);
 
 /* PowerPC pseries based VMs do not support floppy device */
 if (!ARCH_IS_PPC64(qemuCaps->arch) ||
@@ -5203,6 +5204,10 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr 
qemuCaps,
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE))
 VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_USB);
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SD_CARD))
+VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_SD);
+
 return 0;
 }
 
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 95bb67d..85e7959 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -393,6 +393,7 @@ typedef enum {
 /* 245 */
 QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION, /* qmp query-cpu-model-expansion */
 QEMU_CAPS_VIRTIO_NET_HOST_MTU, /* virtio-net-*.host_mtu */
+QEMU_CAPS_SD_CARD, /* -sd abc.img */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
-- 
2.7.4


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