Re: [libvirt] How libvirt address qemu command line args

[Michal Privoznik]

On 18.10.2016 14:59, zhun...@gmail.com wrote:
> Now I want to add some args about TPM  to domain's XML,so I can start a 
> domain by virt-manager or other virsh command,and then ,I would like to use 
> sVIrt security context to label vTPM and correspondingVM,But I do not know 
> how to get these XML  args in libvirt.
> the key problem is that how can i get and recognize these args!!!
> related XML content :

Usually, grepping the code for cmd name <-> XML element/attribute
translation is sufficient (esp. if you grep tests/)

> 
> 
> 

Firstly, this is obsolete in favour of "-machine accel=kvm". In any
case,  will do the trick (libvirt will use whatever
is supported by qemu binary in your system).

> 
>  value='file=/root/nvram_2.0-jin.qcow2,if=none,id=nvram0-0-0,format=qcow2'/>

Okay, this is not supported by libvirt yet. We don't really have a way
how to specify NVRAM in anything other than a raw file. BTW: isn't qcow
too big gun for NVRAM? I mean, NVRAM has a fixed size of what ~190 KB?
QCOW header is about the same size.

> 
> 
> 
> 

I'm not sure there's a way how to put startup=clean on the cmd line. I'm
not even sure what it does.
And I have not idea what libtpms is either :-)

> 
> 
>   
> 

Michal

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

Re: [libvirt] [RFC] make virDomainQemuMonitorCommand work in any libvirt state

[Michal Privoznik]

On 17.10.2016 20:46, Nikolay Shirokovskiy wrote:
> Hi, all.
> 
> We would like to use virDomainQemuMonitorCommand to query qemu independently 
> of
> libvirt state. Currenly it is not possible. This API call takes job condition
> just like any other call and thus is unavailable on any lengthy(or stucked)
> synchronous job.
> 
> I've already posted this question in list, just failed to find the reference.
> Somebody suggested to use proxy (and even an implementation) in between qemu
> and libvirt that can inject commands to qemu and filter replies. It is not
> really convinient. This way test setups will be different from production and
> we can not investigate problems in production environment.
> 
> I'd like to drop acquiring job condition in the call as this function does not
> deal with libvirt state (except for the taint but is is ok, we will not mess
> things up here). But this is not enough, we need to make qemu monitor deal 
> with
> many qemu commands simultaneously. Looks like it is quite a big change for
> test/debug case. But I guess eventually normal user cases can get benefits too
> from this monitor changes. For example all query API calls that query qemu
> directly can be changed to not to wait for some synchronous job
> finishing.(qemuDomainGetBlockJobInfo for example).

IIRC the last time I looked into this the problem was not on libvirt
side. QEMU's monitor was unable to process multiple commands at once.
But maybe that's no longer the case, I don't know.

However, what I think we should do is to turn our jobs into sort of RW
locks. That is - we could allow multiple QUERY jobs to happen
simultaneously and leave MODIFY jobs to be exclusive. I think dropping
BeginJob() from an API is a no go as it will definitely bite us in the
future.

Unfortunately, I have no idea what my suggestion would look like in
terms of the code. How difficult it would be to implement it (and
whether monitor code is prepared for that).

Michal

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

Re: [libvirt] Qemu: create empty cdrom

[Michal Privoznik]

On 14.10.2016 22:16, Stefan Hajnoczi wrote:
> On Fri, Feb 5, 2016 at 2:56 PM, Gromak Yuriy  wrote:
>> Qemu is latest from master branch.
>> Tryingto start a domain, which is connected toa blankcdrom:
>>
>> 
>>   
>>   
>>   
>>   
>> 
>>
>> But I get an error:
>>
>> qemu-system-x86_64: -drive
>> if=none,id=drive-scsi0-0-1-0,readonly=on,format=raw: Can't use 'raw' as a
>> block driver for the protocol level.
> 
> Mathieu hit the same problem with a QEMU 2.6 backport for Debian
> jessie.  His libvirt version is based on 1.2.9.
> 
> QEMU began printing this error starting from version 2.6.0.
> 
> I think newer libvirt versions have compensated.  There is no error
> when I use Mathieu's domain XML with libvirt based on 1.3.3 (Fedora
> 24).
> 
> Looks like you need a modern libvirt to run against a modern QEMU.

Correct, Stefan's right.
This issue has been fixed in this commit:

commit d7db33bfe978c89e1302609ac91e65be3d49379f
Author: Michal Privoznik 
AuthorDate: Mon Dec 28 15:13:52 2015 +0100
Commit: Michal Privoznik 
CommitDate: Tue Jan 5 16:41:16 2016 +0100

qemu: Specify format= iff disk source is not empty

Just recently, qemu forbade specifying format for sourceless
disks (qemu commit 39c4ae941ed992a3bb5). It kind of makes sense.
If there's no file to open, why specify its format. Anyway, I
have a domain like this:


  
  
  
  


and obviously I am unable to start it. Therefore, a fix on our
side is needed too.

Signed-off-by: Michal Privoznik 

which is contained in the 1.3.1 release.

Michal

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

Re: [libvirt] [PATCH 1/2] util: Add function to check if string contains some chars

[Michal Privoznik]

On 19.10.2016 03:55, Sławek Kapłoński wrote:
> Tue, 18 Oct 2016, Michal Privoznik wrote:
>> > On 14.10.2016 04:53, Sławek Kapłoński wrote:
>>> > > This new function can be used to check if e.g. name of XML node
>>> > > don't contains forbidden chars like "/" or new-line.
>>> > > ---
>>> > >  src/conf/network_conf.c  | 2 +-
>>> > >  src/libvirt_private.syms | 1 +
>>> > >  src/util/virstring.c | 9 +
>>> > >  src/util/virstring.h | 1 +
>>> > >  4 files changed, 12 insertions(+), 1 deletion(-)
>>> > > 
>>> > > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>>> > > index aa39776..949d138 100644
>>> > > --- a/src/conf/network_conf.c
>>> > > +++ b/src/conf/network_conf.c
>>> > > @@ -2117,7 +2117,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>>> > >  goto error;
>>> > >  }
>>> > >  
>>> > > -if (strchr(def->name, '/')) {
>>> > > +if (virStringHasChars(def->name, "/")) {
>>> > >  virReportError(VIR_ERR_XML_ERROR,
>>> > > _("name %s cannot contain '/'"), def->name);
>> > 
>> > Usually, in one patch we introduce a function and then in a subsequent
>> > one we switch the rest of the code into using it. But okay, I can live
>> > with this too.
>> > 
>>> > >  goto error;
>>> > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> > > index 55b6a24..6f41234 100644
>>> > > --- a/src/libvirt_private.syms
>>> > > +++ b/src/libvirt_private.syms
>>> > > @@ -2435,6 +2435,7 @@ virStringEncodeBase64;
>>> > >  virStringFreeList;
>>> > >  virStringFreeListCount;
>>> > >  virStringGetFirstWithPrefix;
>>> > > +virStringHasChars;
>>> > >  virStringHasControlChars;
>>> > >  virStringIsEmpty;
>>> > >  virStringIsPrintable;
>>> > > diff --git a/src/util/virstring.c b/src/util/virstring.c
>>> > > index 4a70620..7799d87 100644
>>> > > --- a/src/util/virstring.c
>>> > > +++ b/src/util/virstring.c
>>> > > @@ -975,6 +975,15 @@ virStringStripIPv6Brackets(char *str)
>>> > >  }
>>> > >  
>>> > >  
>>> > > +bool
>>> > > +virStringHasChars(const char *str, const char *chars)
>>> > > +{
>>> > > +if (strpbrk(str, chars))
>>> > > +return true;
>>> > > +return false;
>>> > > +}
>> > 
>> > This, however, makes not much sense. I mean, this function has no added
>> > value to pain strpbrk(). What I suggested in one of previous reviews was
>> > that this function should report an error too. In that case, it will
>> > immediately have added value and thus it will be handy to use it.
>> > Perhaps you are afraid that error message might change in some cases?
>> > That's okay, we don't make any promises about error messages.
>> > 
> I agree that in fact it don't have too much sense but I'm not sure that
> it would be good to report error here. First of all, it could be that in
> some cases it could be used to check if function have required chars so
> there is no easy way to check which result is error in fact.

Well, even if we did want to check for that, strpbrk() is no help there.
I mean, you cannot use it to check if a string contains only allowed
characters and nothing more.

> Second thing (maybe here I'm wrong) places where I wanted to use this
> function are reporting error VIR_ERR_XML_ERROR and I'm not sure it is
> good place to report such error in "string lib".

That's why I initially suggested for this function to be in virxml.c
(and thus have a slightly different name to reflect the submodule it is in).

> So maybe better solution would be just use strbprk (or strchr) in
> src/network/bridge_driver.c to check if name contains \n char and not
> introduce any new function to such check? What You think about that?

Well, then again - I don't think we should limit ourselves to network
driver really. Other drivers suffer from the same problem, don't they? I
mean, sure - we can just use strpbrk() here and copy it all over the
place to different drivers, but I think having a small function just for
that would be more convenient.
Moreover, we already have some small, one purpose functions in virxml,
for instance:  virXMLPickShellSafeComment, virXMLSaveFile,
virXMLPropString, and others.

Michal

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

Re: [libvirt] qemu-guest-agent windows

[Michael Roth]

Quoting Michal Privoznik (2016-08-29 04:54:13)
> On 29.08.2016 11:48, Umar Draz wrote:
> > Hi Michal,
> > 
> > well after the upgrade I am still getting the old version
> > 
> > {"return":{"version":"0.12.1","supported_commands":[{"enabled":true,"name":"guest-set-user-password"},{"enabled":true,"name":"guest-set-vcpus"},{"enabled":true,"name":"guest-get-vcpus"},{"enabled":true,"name":"guest-network-get-interfaces"},{"enabled":true,"name":"guest-suspend-hybrid"},{"enabled":true,"name":"guest-suspend-ram"},{"enabled":true,"name":"guest-suspend-disk"},{"enabled":true,"name":"guest-fstrim"},{"enabled":true,"name":"guest-fsfreeze-thaw"},{"enabled":true,"name":"guest-fsfreeze-freeze"},{"enabled":true,"name":"guest-fsfreeze-status"},{"enabled":true,"name":"guest-file-flush"},{"enabled":true,"name":"guest-file-seek"},{"enabled":true,"name":"guest-file-write"},{"enabled":true,"name":"guest-file-read"},{"enabled":true,"name":"guest-file-close"},{"enabled":true,"name":"guest-file-open"},{"enabled":true,"name":"guest-shutdown"},{"enabled":true,"name":"guest-info"},{"enabled":true,"name":"guest-set-time"},{"enabled":true,"name":"guest-get-time"},{"enabled":t!
 ru!
>  
> e,"name":"guest-ping"},{"enabled":true,"name":"guest-sync"},{"enabled":true,"name":"guest-sync-delimited"}]}}
> > 
> > 
> > even I had install this qemu-ga on newly installed Windows 10, but that
> > also showing the same version as above.
> 
> Then you need to build the qemu-ga from the sources.

FWIW, I recently started publishing newer win32 builds of qemu-ga here:

https://github.com/mdroth/qemu/releases

The one shipping with Fedora virtio-win is based on RHEL6 QEMU sources I think.

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


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

Re: [libvirt] [PATCH 1/1] qemu: host NUMA hugepage policy without guest NUMA

[Martin Kletzander]

On Mon, Oct 17, 2016 at 03:45:09PM +1100, Sam Bobroff wrote:

On Fri, Oct 14, 2016 at 10:19:42AM +0200, Martin Kletzander wrote:

On Fri, Oct 14, 2016 at 11:52:22AM +1100, Sam Bobroff wrote:
>I did look at the libnuma and cgroups approaches, but I was concerned they
>wouldn't work in this case, because of the way QEMU allocates memory when
>mem-prealloc is used: the memory is allocated in the main process, before the
>CPU threads are created. (This is based only on a bit of hacking and debugging
>in QEMU, but it does seem explain the behaviour I've seen so far.)
>

But we use numactl before QEMU is exec()'d.


Sorry, I jumped ahead a bit. I'll try to explain what I mean:

I think the problem with using this method would be that the NUMA policy is
applied to all allocations by QEMU, not just ones related to the memory
backing. I'm not sure if that would cause a serious problem but it seems untidy,
and it doesn't happen in other situations (i.e. with separate memory backend
objects, QEMU sets up the policy specifically for each one and other
allocations aren't affected, AFAIK).  Presumably, if memory were very
restricted it could prevent the guest from starting.



Yes, it is, that's what  does if you don't have any
other () specifics set.


>I think QEMU could be altered to move the preallocations into the VCPU
>threads but it didn't seem trivial and I suspected the QEMU community would
>point out that there was already a way to do it using backend objects.  Another
>option would be to add a -host-nodes parameter to QEMU so that the policy can
>be given without adding a memory backend object. (That seems like a more
>reasonable change to QEMU.)
>

I think upstream won't like that, mostly because there is already a
way.  And that is using memory-backend object.  I think we could just
use that and disable changing it live.  But upstream will probably want
that to be configurable or something.


Right, but isn't this already an issue in the cases where libvirt is already
using memory backend objects and NUMA policy? (Or does libvirt already disable
changing it live in those situations?)



It is.  I'm not trying to say libvirt is perfect.  There are bugs,
e.g. like this one.  The problem is that we tried to do *everything*,
but it's not currently possible.  I'm trying to explain how stuff works
now.  It definitely needs some fixing, though.


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

Re: [libvirt] [PATCH 1/2] util: Add function to check if string contains some chars

[Sławek Kapłoński]

Hello,

Thx for review. Please read my answear inline.

-- 
Best regards / Pozdrawiam
Sławek Kapłoński
sla...@kaplonski.pl

On Tue, 18 Oct 2016, Michal Privoznik wrote:

> On 14.10.2016 04:53, Sławek Kapłoński wrote:
> > This new function can be used to check if e.g. name of XML node
> > don't contains forbidden chars like "/" or new-line.
> > ---
> >  src/conf/network_conf.c  | 2 +-
> >  src/libvirt_private.syms | 1 +
> >  src/util/virstring.c | 9 +
> >  src/util/virstring.h | 1 +
> >  4 files changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> > index aa39776..949d138 100644
> > --- a/src/conf/network_conf.c
> > +++ b/src/conf/network_conf.c
> > @@ -2117,7 +2117,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> >  goto error;
> >  }
> >  
> > -if (strchr(def->name, '/')) {
> > +if (virStringHasChars(def->name, "/")) {
> >  virReportError(VIR_ERR_XML_ERROR,
> > _("name %s cannot contain '/'"), def->name);
> 
> Usually, in one patch we introduce a function and then in a subsequent
> one we switch the rest of the code into using it. But okay, I can live
> with this too.
> 
> >  goto error;
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 55b6a24..6f41234 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -2435,6 +2435,7 @@ virStringEncodeBase64;
> >  virStringFreeList;
> >  virStringFreeListCount;
> >  virStringGetFirstWithPrefix;
> > +virStringHasChars;
> >  virStringHasControlChars;
> >  virStringIsEmpty;
> >  virStringIsPrintable;
> > diff --git a/src/util/virstring.c b/src/util/virstring.c
> > index 4a70620..7799d87 100644
> > --- a/src/util/virstring.c
> > +++ b/src/util/virstring.c
> > @@ -975,6 +975,15 @@ virStringStripIPv6Brackets(char *str)
> >  }
> >  
> >  
> > +bool
> > +virStringHasChars(const char *str, const char *chars)
> > +{
> > +if (strpbrk(str, chars))
> > +return true;
> > +return false;
> > +}
> 
> This, however, makes not much sense. I mean, this function has no added
> value to pain strpbrk(). What I suggested in one of previous reviews was
> that this function should report an error too. In that case, it will
> immediately have added value and thus it will be handy to use it.
> Perhaps you are afraid that error message might change in some cases?
> That's okay, we don't make any promises about error messages.
> 

I agree that in fact it don't have too much sense but I'm not sure that
it would be good to report error here. First of all, it could be that in
some cases it could be used to check if function have required chars so
there is no easy way to check which result is error in fact.
Second thing (maybe here I'm wrong) places where I wanted to use this
function are reporting error VIR_ERR_XML_ERROR and I'm not sure it is
good place to report such error in "string lib".
So maybe better solution would be just use strbprk (or strchr) in
src/network/bridge_driver.c to check if name contains \n char and not
introduce any new function to such check? What You think about that?

> > +
> > +
> >  static const char control_chars[] =
> >  "\x01\x02\x03\x04\x05\x06\x07"
> >  "\x08" /* \t \n */ "\x0B\x0C" /* \r */ "\x0E\x0F"
> > diff --git a/src/util/virstring.h b/src/util/virstring.h
> > index 8854d5f..7f2c96d 100644
> > --- a/src/util/virstring.h
> > +++ b/src/util/virstring.h
> > @@ -272,6 +272,7 @@ char *virStringReplace(const char *haystack,
> >  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> >  
> >  void virStringStripIPv6Brackets(char *str);
> > +bool virStringHasChars(const char *str, const char *chars);
> >  bool virStringHasControlChars(const char *str);
> >  void virStringStripControlChars(char *str);
> >  
> > 
> 
> Michal


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

Re: [libvirt] [PATCH v2 0/3] fix hot(un)plug of chardev devices with TLS encryption

[John Ferlan]

On 10/18/2016 11:04 AM, Pavel Hrdina wrote:
> Pavel Hrdina (3):
>   qemu_alias: introduce qemuAliasChardevFromDevAlias helper
>   qemu_command: create prefixed alias to separate variable
>   qemu: always generate the same alias for tls-creds-x509 object
> 
>  src/qemu/qemu_alias.c  | 16 +
>  src/qemu/qemu_alias.h  |  3 ++
>  src/qemu/qemu_command.c| 41 
> --
>  src/qemu/qemu_hotplug.c| 23 +++-
>  ...xml2argv-serial-tcp-tlsx509-chardev-verify.args |  4 +--
>  .../qemuxml2argv-serial-tcp-tlsx509-chardev.args   |  4 +--
>  6 files changed, 60 insertions(+), 31 deletions(-)
> 


ACK series

John

BTW: I did check the sa_assert won't be necessary any more...

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

[libvirt] [PATCH v2] vsh: Using VSH_REQUIRE_OPTION rather than virReportError

[Kothapally Madhu Pavan]

Correcting the error reporting method by using VSH_REQUIRE_OPTION
instead of virReportError

Signed-off-by: Kothapally Madhu Pavan 
---
 tools/virsh-domain.c |7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 050e7fb..e1cb2ac 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10443,6 +10443,7 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd)
 
 VSH_EXCLUSIVE_OPTIONS("live", "offline");
 VSH_EXCLUSIVE_OPTIONS("timeout-suspend", "timeout-postcopy");
+VSH_REQUIRE_OPTION("postcopy-after-precopy", "postcopy");
 
 if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
 return false;
@@ -10474,12 +10475,6 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd)
 }
 
 if (vshCommandOptBool(cmd, "postcopy-after-precopy")) {
-if (!vshCommandOptBool(cmd, "postcopy")) {
-virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
-  _("--postcopy-after-precopy can only be used with "
-"--postcopy"));
-goto cleanup;
-}
 iterEvent = virConnectDomainEventRegisterAny(
 priv->conn, dom,
 VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION,

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

Re: [libvirt] [PATCH] vsh: Using vshError rather than virReportError

[Madhu Pavan]

On 10/18/2016 07:33 PM, Peter Krempa wrote:

On Tue, Oct 18, 2016 at 07:39:16 -0400, Kothapally Madhu Pavan wrote:

Correcting the error reporting method by using vshError
instead of virReportError

Signed-off-by: Kothapally Madhu Pavan 
---
  tools/virsh-domain.c |6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 050e7fb..c9fabf2 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10475,9 +10475,9 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd)
  
  if (vshCommandOptBool(cmd, "postcopy-after-precopy")) {

  if (!vshCommandOptBool(cmd, "postcopy")) {
-virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
-  _("--postcopy-after-precopy can only be used with "
-"--postcopy"));
+vshError(ctl, "%s",
+ _("argument unsupported: --postcopy-after-precopy can only 
"
+   "be used with --postcopy"));
  goto cleanup;

I think the above code can be avoided by simply using
VSH_REQUIRE_OPTION.

Yes, it can be avoided by using VSH_REQUIRE_OPTION. Sending a new patch.

Thanks,
Madhu Pavan.

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

[libvirt] [PATCH] vz: set localhost as vnc address

[Mikhail Feoktistov]

We should set localhost as vnc address in case of empty string.
Because Virtuozzo sets 0.0.0.0 as default vnc address.
---
 src/vz/vz_sdk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index f2a5c96..7235172 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -2967,7 +2967,7 @@ static int prlsdkApplyGraphicsParams(PRL_HANDLE sdkdom,
 
 glisten = virDomainGraphicsGetListen(gr, 0);
 pret = PrlVmCfg_SetVNCHostName(sdkdom, glisten && glisten->address ?
-   glisten->address : "");
+   glisten->address : "127.0.0.1");
 prlsdkCheckRetGoto(pret, cleanup);
 
 ret = 0;
-- 
1.8.3.1

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

Re: [libvirt] [PATCH 2/2] qemu: always generate the same alias for tls-creds-x509 object

[John Ferlan]

On 10/18/2016 10:57 AM, Pavel Hrdina wrote:
> On Tue, Oct 18, 2016 at 10:37:37AM -0400, John Ferlan wrote:
>>
>>
>> On 10/18/2016 09:58 AM, Pavel Hrdina wrote:
>>> There was inconsistency between alias used to create tls-creds-x509
>>> object and alias used to link that object to chardev while hotpluging.
>>>
>>> In XML we have for example alias "serial0", but on qemu command line we
>>> generate "charserial0".
>>>
>>> The issue was that code, that creates QMP command to hotplug chardev
>>> devices uses only the second alias "charserial0" and that alias is also
>>> used to link the tls-creds-x509 object.
>>
>> Which two objects used the same ID in hotplug?
>>
>> tlsProps would use obj%s_tls0
>>
>> and this changes it to be essentially objchar%s_tls0
>>
>> Essentially I'm trying to figure out from the commit message prior to
>> this patch what was created incorrectly.
> 
> The issue is that while hotpluging chardev device those QMP command are 
> created
> as I've described:
> 
> {
> "execute":"object-add",
> "arguments": {
> "qom-type":"tls-creds-x509",
> "id":"objchannel3_tls0",
>   
> "props": {
> "dir":"/etc/pki/libvirt-chardev",
> "endpoint":"server",
> "verify-peer":false
> }
> },
> "id":"libvirt-29"
> }
> 
> {
> "execute":"chardev-add",
> "arguments": {
> "id":"charchannel3",
> "backend": {
> "type":"socket",
> "data": {
> "addr": {
> "type":"inet",
> "data": {
> "host":"localhost",
> "port":"3344"
> }
> },
> "wait":false,
> "telnet":false,
> "server":true,
> "tls-creds":"objcharchannel3_tls0"
>  
> }
> }
> },
> "id":"libvirt-30"
> }
> 
> And as you can see the alias used to create tls-creds-x509 object is different
> than the one used while attaching chardev.  That's because function
> qemuMonitorJSONAttachCharDevCommand() takes as first argument "charchannel3"
> instead of "channel3".  So the logical solution is to use "charchannel3" as 
> base
> while creating "obj%s_tls0" alias.

Ah... OK I see...  I wasn't visualizing the chardev-add from the commit
message.

tks -

John

> 
>>> This patch unifies the aliases for tls-creds-x509 to be always generated
>>> from "charserial0".
>>>
>>> Signed-off-by: Pavel Hrdina 
>>> ---
>>>  src/qemu/qemu_command.c  | 4 ++--
>>>  src/qemu/qemu_hotplug.c  | 9 
>>> +++--
>>>  .../qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args  | 4 ++--
>>>  .../qemuxml2argv-serial-tcp-tlsx509-chardev.args | 4 ++--
>>>  4 files changed, 13 insertions(+), 8 deletions(-)
>>>
>>
>>
>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 74f65c0..8d87e69 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -4949,10 +4949,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
>>>  if (qemuBuildTLSx509CommandLine(cmd, 
>>> cfg->chardevTLSx509certdir,
>>>  dev->data.tcp.listen,
>>>  cfg->chardevTLSx509verify,
>>> -alias, qemuCaps) < 0)
>>> +charAlias, qemuCaps) < 0)
>>>  goto error;
>>>  
>>> -if (!(objalias = qemuAliasTLSObjFromChardevAlias(alias)))
>>> +if (!(objalias = qemuAliasTLSObjFromChardevAlias(charAlias)))
>>>  goto error;
>>>  virBufferAsprintf(, ",tls-creds=%s", objalias);
>>>  VIR_FREE(objalias);
>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>> index c2ba935..e39bd8b 100644
>>> --- a/src/qemu/qemu_hotplug.c
>>> +++ b/src/qemu/qemu_hotplug.c
>>> @@ -1738,7 +1738,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
>>>   ) < 0)
>>>  goto cleanup;
>>>  
>>> -if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(chr->info.alias)))
>>> +if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
>>>  goto cleanup;
>>>  dev->data.tcp.tlscreds = true;
>>>  }
>>> @@ -4387,6 +4387,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>>>  virDomainChrDefPtr tmpChr;
>>>  char *objAlias = NULL;
>>>  char *devstr = NULL;
>>> +char *charAlias = NULL;
>>>  
>>>  if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
>>>  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>>> @@ -4397,11 +4398,14 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr 
>>> driver,
>>>  if 

[libvirt] [PATCH v2 2/3] qemu_command: create prefixed alias to separate variable

[Pavel Hrdina]

Instead of typing the prefix every time we want to append parameters
to qemu command line use a variable that contains prefixed alias.

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_command.c | 35 ---
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f5ed490..848937c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4861,28 +4861,32 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 {
 virBuffer buf = VIR_BUFFER_INITIALIZER;
 bool telnet;
+char *charAlias = NULL;
+
+if (!(charAlias = qemuAliasChardevFromDevAlias(alias)))
+goto error;
 
 switch (dev->type) {
 case VIR_DOMAIN_CHR_TYPE_NULL:
-virBufferAsprintf(, "null,id=char%s", alias);
+virBufferAsprintf(, "null,id=%s", charAlias);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_VC:
-virBufferAsprintf(, "vc,id=char%s", alias);
+virBufferAsprintf(, "vc,id=%s", charAlias);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_PTY:
-virBufferAsprintf(, "pty,id=char%s", alias);
+virBufferAsprintf(, "pty,id=%s", charAlias);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_DEV:
-virBufferAsprintf(, "%s,id=char%s,path=%s",
+virBufferAsprintf(, "%s,id=%s,path=%s",
   STRPREFIX(alias, "parallel") ? "parport" : "tty",
-  alias, dev->data.file.path);
+  charAlias, dev->data.file.path);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_FILE:
-virBufferAsprintf(, "file,id=char%s", alias);
+virBufferAsprintf(, "file,id=%s", charAlias);
 
 if (dev->data.file.append != VIR_TRISTATE_SWITCH_ABSENT &&
 !virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FILE_APPEND)) {
@@ -4898,12 +4902,12 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 break;
 
 case VIR_DOMAIN_CHR_TYPE_PIPE:
-virBufferAsprintf(, "pipe,id=char%s,path=%s", alias,
+virBufferAsprintf(, "pipe,id=%s,path=%s", charAlias,
   dev->data.file.path);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_STDIO:
-virBufferAsprintf(, "stdio,id=char%s", alias);
+virBufferAsprintf(, "stdio,id=%s", charAlias);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_UDP: {
@@ -4919,9 +4923,9 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 bindService = "0";
 
 virBufferAsprintf(,
-  "udp,id=char%s,host=%s,port=%s,localaddr=%s,"
+  "udp,id=%s,host=%s,port=%s,localaddr=%s,"
   "localport=%s",
-  alias,
+  charAlias,
   connectHost,
   dev->data.udp.connectService,
   bindHost, bindService);
@@ -4930,8 +4934,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 case VIR_DOMAIN_CHR_TYPE_TCP:
 telnet = dev->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET;
 virBufferAsprintf(,
-  "socket,id=char%s,host=%s,port=%s%s",
-  alias,
+  "socket,id=%s,host=%s,port=%s%s",
+  charAlias,
   dev->data.tcp.host,
   dev->data.tcp.service,
   telnet ? ",telnet" : "");
@@ -4956,7 +4960,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 break;
 
 case VIR_DOMAIN_CHR_TYPE_UNIX:
-virBufferAsprintf(, "socket,id=char%s,path=", alias);
+virBufferAsprintf(, "socket,id=%s,path=", charAlias);
 virQEMUBuildBufferEscapeComma(, dev->data.nix.path);
 if (dev->data.nix.listen)
 virBufferAdd(, nowait ? ",server,nowait" : ",server", -1);
@@ -4968,7 +4972,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
_("spicevmc not supported in this QEMU binary"));
 goto error;
 }
-virBufferAsprintf(, "spicevmc,id=char%s,name=%s", alias,
+virBufferAsprintf(, "spicevmc,id=%s,name=%s", charAlias,
   
virDomainChrSpicevmcTypeToString(dev->data.spicevmc));
 break;
 
@@ -4978,7 +4982,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
_("spiceport not supported in this QEMU binary"));
 goto error;
 }
-virBufferAsprintf(, "spiceport,id=char%s,name=%s", alias,
+virBufferAsprintf(, "spiceport,id=%s,name=%s", charAlias,
   dev->data.spiceport.channel);
 break;
 
@@ -5007,6 +5011,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 return virBufferContentAndReset();
 
  error:
+VIR_FREE(charAlias);
 virBufferFreeAndReset();
 return NULL;
 }
-- 
2.10.1

--
libvir-list mailing list

[libvirt] [PATCH v2 1/3] qemu_alias: introduce qemuAliasChardevFromDevAlias helper

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_alias.c   | 16 
 src/qemu/qemu_alias.h   |  3 +++
 src/qemu/qemu_command.c |  2 +-
 src/qemu/qemu_hotplug.c | 14 +++---
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index cc83fec..9737158 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -606,3 +606,19 @@ qemuAliasTLSObjFromChardevAlias(const char *chardev_alias)
 
 return ret;
 }
+
+
+/* qemuAliasChardevFromDevAlias:
+ * @devAlias: pointer do device alias
+ *
+ * Generate and return a string to be used as chardev alias.
+ */
+char *
+qemuAliasChardevFromDevAlias(const char *devAlias)
+{
+char *ret;
+
+ignore_value(virAsprintf(, "char%s", devAlias));
+
+return ret;
+}
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
index 11d9fde..d298a4d 100644
--- a/src/qemu/qemu_alias.h
+++ b/src/qemu/qemu_alias.h
@@ -83,4 +83,7 @@ char *qemuDomainGetSecretAESAlias(const char *srcalias,
 char *qemuAliasTLSObjFromChardevAlias(const char *chardev_alias)
 ATTRIBUTE_NONNULL(1);
 
+char *qemuAliasChardevFromDevAlias(const char *devAlias)
+ATTRIBUTE_NONNULL(1);
+
 #endif /* __QEMU_ALIAS_H__*/
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 21fd85c..f5ed490 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5484,7 +5484,7 @@ qemuBuildRNGBackendProps(virDomainRNGDefPtr rng,
 
 *type = "rng-egd";
 
-if (virAsprintf(, "char%s", rng->info.alias) < 0)
+if (!(charBackendAlias = 
qemuAliasChardevFromDevAlias(rng->info.alias)))
 goto cleanup;
 
 if (virJSONValueObjectCreate(props, "s:chardev", charBackendAlias,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c2ba935..af87581 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1058,7 +1058,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 goto cleanup;
 }
 
-if (virAsprintf(, "char%s", net->info.alias) < 0)
+if (!(charDevAlias = qemuAliasChardevFromDevAlias(net->info.alias)))
 goto cleanup;
 break;
 
@@ -1487,7 +1487,7 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr 
driver,
 if (qemuAssignDeviceRedirdevAlias(def, redirdev, -1) < 0)
 goto cleanup;
 
-if (virAsprintf(, "char%s", redirdev->info.alias) < 0)
+if (!(charAlias = qemuAliasChardevFromDevAlias(redirdev->info.alias)))
 goto cleanup;
 
 if (!(devstr = qemuBuildRedirdevDevStr(def, redirdev, priv->qemuCaps)))
@@ -1723,7 +1723,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
 if (qemuBuildChrDeviceStr(, vmdef, chr, priv->qemuCaps) < 0)
 goto cleanup;
 
-if (virAsprintf(, "char%s", chr->info.alias) < 0)
+if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias)))
 goto cleanup;
 
 if (qemuDomainChrPreInsert(vmdef, chr) < 0)
@@ -1858,7 +1858,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
 if (virAsprintf(, "obj%s", rng->info.alias) < 0)
 goto cleanup;
 
-if (virAsprintf(, "char%s", rng->info.alias) < 0)
+if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias)))
 goto cleanup;
 
 qemuDomainObjEnterMonitor(driver, vm);
@@ -3370,7 +3370,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
   net->info.alias, vm, vm->def->name);
 
 if (virAsprintf(_name, "host%s", net->info.alias) < 0 ||
-virAsprintf(, "char%s", net->info.alias) < 0)
+!(charDevAlias = qemuAliasChardevFromDevAlias(net->info.alias)))
 goto cleanup;
 
 
@@ -3477,7 +3477,7 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
 VIR_DEBUG("Removing character device %s from domain %p %s",
   chr->info.alias, vm, vm->def->name);
 
-if (virAsprintf(, "char%s", chr->info.alias) < 0)
+if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias)))
 goto cleanup;
 
 qemuDomainObjEnterMonitor(driver, vm);
@@ -3522,7 +3522,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
 if (virAsprintf(, "obj%s", rng->info.alias) < 0)
 goto cleanup;
 
-if (virAsprintf(, "char%s", rng->info.alias) < 0)
+if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias)))
 goto cleanup;
 
 qemuDomainObjEnterMonitor(driver, vm);
-- 
2.10.1

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

[libvirt] [PATCH v2 3/3] qemu: always generate the same alias for tls-creds-x509 object

[Pavel Hrdina]

There was inconsistency between alias used to create tls-creds-x509
object and alias used to link that object to chardev while hotpluging.
Hotplug ends with this error:

  error: Failed to detach device from channel-tcp.xml
  error: internal error: unable to execute QEMU command 'chardev-add':
  No TLS credentials with id 'objcharchannel3_tls0'

In XML we have for example alias "serial0", but on qemu command line we
generate "charserial0".

The issue was that code, that creates QMP command to hotplug chardev
devices uses only the second alias "charserial0" and that alias is also
used to link the tls-creds-x509 object.

This patch unifies the aliases for tls-creds-x509 to be always generated
from "charserial0".

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_command.c  | 4 ++--
 src/qemu/qemu_hotplug.c  | 9 +++--
 .../qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args  | 4 ++--
 .../qemuxml2argv-serial-tcp-tlsx509-chardev.args | 4 ++--
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 848937c..8282162 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4949,10 +4949,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir,
 dev->data.tcp.listen,
 cfg->chardevTLSx509verify,
-alias, qemuCaps) < 0)
+charAlias, qemuCaps) < 0)
 goto error;
 
-if (!(objalias = qemuAliasTLSObjFromChardevAlias(alias)))
+if (!(objalias = qemuAliasTLSObjFromChardevAlias(charAlias)))
 goto error;
 virBufferAsprintf(, ",tls-creds=%s", objalias);
 VIR_FREE(objalias);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index af87581..2cb2267 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1738,7 +1738,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
  ) < 0)
 goto cleanup;
 
-if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(chr->info.alias)))
+if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
 goto cleanup;
 dev->data.tcp.tlscreds = true;
 }
@@ -4387,6 +4387,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
 virDomainChrDefPtr tmpChr;
 char *objAlias = NULL;
 char *devstr = NULL;
+char *charAlias = NULL;
 
 if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -4399,9 +4400,12 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
 
 sa_assert(tmpChr->info.alias);
 
+if (!(charAlias = qemuAliasChardevFromDevAlias(tmpChr->info.alias)))
+goto cleanup;
+
 if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP &&
 cfg->chardevTLS &&
-!(objAlias = qemuAliasTLSObjFromChardevAlias(tmpChr->info.alias)))
+!(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
 goto cleanup;
 
 if (qemuBuildChrDeviceStr(, vmdef, chr, priv->qemuCaps) < 0)
@@ -4427,6 +4431,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
  cleanup:
 qemuDomainResetDeviceRemoval(vm);
 VIR_FREE(devstr);
+VIR_FREE(charAlias);
 virObjectUnref(cfg);
 return ret;
 
diff --git 
a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args 
b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
index f521e33..003d11d 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
@@ -25,9 +25,9 @@ server,nowait \
 -chardev udp,id=charserial0,host=127.0.0.1,port=,localaddr=127.0.0.1,\
 localport= \
 -device isa-serial,chardev=charserial0,id=serial0 \
--object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\
+-object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\
 endpoint=client,verify-peer=yes \
 -chardev socket,id=charserial1,host=127.0.0.1,port=,\
-tls-creds=objserial1_tls0 \
+tls-creds=objcharserial1_tls0 \
 -device isa-serial,chardev=charserial1,id=serial1 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git 
a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args 
b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args
index 4c8c23e..b456cce 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args
@@ -25,9 +25,9 @@ server,nowait \
 -chardev 

[libvirt] [PATCH v2 0/3] fix hot(un)plug of chardev devices with TLS encryption

[Pavel Hrdina]

Pavel Hrdina (3):
  qemu_alias: introduce qemuAliasChardevFromDevAlias helper
  qemu_command: create prefixed alias to separate variable
  qemu: always generate the same alias for tls-creds-x509 object

 src/qemu/qemu_alias.c  | 16 +
 src/qemu/qemu_alias.h  |  3 ++
 src/qemu/qemu_command.c| 41 --
 src/qemu/qemu_hotplug.c| 23 +++-
 ...xml2argv-serial-tcp-tlsx509-chardev-verify.args |  4 +--
 .../qemuxml2argv-serial-tcp-tlsx509-chardev.args   |  4 +--
 6 files changed, 60 insertions(+), 31 deletions(-)

-- 
2.10.1

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

Re: [libvirt] [PATCH 2/2] qemu: always generate the same alias for tls-creds-x509 object

[Pavel Hrdina]

On Tue, Oct 18, 2016 at 10:37:37AM -0400, John Ferlan wrote:
> 
> 
> On 10/18/2016 09:58 AM, Pavel Hrdina wrote:
> > There was inconsistency between alias used to create tls-creds-x509
> > object and alias used to link that object to chardev while hotpluging.
> > 
> > In XML we have for example alias "serial0", but on qemu command line we
> > generate "charserial0".
> > 
> > The issue was that code, that creates QMP command to hotplug chardev
> > devices uses only the second alias "charserial0" and that alias is also
> > used to link the tls-creds-x509 object.
> 
> Which two objects used the same ID in hotplug?
> 
> tlsProps would use obj%s_tls0
> 
> and this changes it to be essentially objchar%s_tls0
> 
> Essentially I'm trying to figure out from the commit message prior to
> this patch what was created incorrectly.

The issue is that while hotpluging chardev device those QMP command are created
as I've described:

{
"execute":"object-add",
"arguments": {
"qom-type":"tls-creds-x509",
"id":"objchannel3_tls0",
  
"props": {
"dir":"/etc/pki/libvirt-chardev",
"endpoint":"server",
"verify-peer":false
}
},
"id":"libvirt-29"
}

{
"execute":"chardev-add",
"arguments": {
"id":"charchannel3",
"backend": {
"type":"socket",
"data": {
"addr": {
"type":"inet",
"data": {
"host":"localhost",
"port":"3344"
}
},
"wait":false,
"telnet":false,
"server":true,
"tls-creds":"objcharchannel3_tls0"
 
}
}
},
"id":"libvirt-30"
}

And as you can see the alias used to create tls-creds-x509 object is different
than the one used while attaching chardev.  That's because function
qemuMonitorJSONAttachCharDevCommand() takes as first argument "charchannel3"
instead of "channel3".  So the logical solution is to use "charchannel3" as base
while creating "obj%s_tls0" alias.

> > This patch unifies the aliases for tls-creds-x509 to be always generated
> > from "charserial0".
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/qemu/qemu_command.c  | 4 ++--
> >  src/qemu/qemu_hotplug.c  | 9 
> > +++--
> >  .../qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args  | 4 ++--
> >  .../qemuxml2argv-serial-tcp-tlsx509-chardev.args | 4 ++--
> >  4 files changed, 13 insertions(+), 8 deletions(-)
> > 
> 
> 
> 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 74f65c0..8d87e69 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -4949,10 +4949,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
> >  if (qemuBuildTLSx509CommandLine(cmd, 
> > cfg->chardevTLSx509certdir,
> >  dev->data.tcp.listen,
> >  cfg->chardevTLSx509verify,
> > -alias, qemuCaps) < 0)
> > +charAlias, qemuCaps) < 0)
> >  goto error;
> >  
> > -if (!(objalias = qemuAliasTLSObjFromChardevAlias(alias)))
> > +if (!(objalias = qemuAliasTLSObjFromChardevAlias(charAlias)))
> >  goto error;
> >  virBufferAsprintf(, ",tls-creds=%s", objalias);
> >  VIR_FREE(objalias);
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index c2ba935..e39bd8b 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -1738,7 +1738,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
> >   ) < 0)
> >  goto cleanup;
> >  
> > -if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(chr->info.alias)))
> > +if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
> >  goto cleanup;
> >  dev->data.tcp.tlscreds = true;
> >  }
> > @@ -4387,6 +4387,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
> >  virDomainChrDefPtr tmpChr;
> >  char *objAlias = NULL;
> >  char *devstr = NULL;
> > +char *charAlias = NULL;
> >  
> >  if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
> >  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > @@ -4397,11 +4398,14 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr 
> > driver,
> >  if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) 
> > < 0)
> >  goto cleanup;
> >  
> > +if (virAsprintf(, "char%s", tmpChr->info.alias) < 0)
> > +goto cleanup;
> > +
> 
> This would seemingly need to go after the 

Re: [libvirt] [PATCH v2 2/2] libssh_transport: add new libssh-based transport

[Daniel P. Berrange]

On Tue, Oct 18, 2016 at 04:46:53PM +0200, Pino Toscano wrote:
> On Tuesday, 18 October 2016 14:19:41 CEST Daniel P. Berrange wrote:
> > On Mon, Oct 17, 2016 at 04:24:53PM +0200, Pino Toscano wrote:
> > > Implement a new libssh transport, which uses libssh to communicate with
> > > remote hosts, and use it in virNetSockets.
> > > 
> > > This new transport supports all the common ssh authentication methods,
> > > making use of libvirt's auth callbacks for interaction with the user.
> > > 
> > > Most of the functionalities and implementation are based on the libssh2
> > > transport.
> > > ---
> > >  config-post.h |2 +
> > >  configure.ac  |3 +
> > >  include/libvirt/virterror.h   |2 +
> > >  m4/virt-libssh.m4 |   26 +
> > >  src/Makefile.am   |   21 +-
> > >  src/libvirt_libssh.syms   |   22 +
> > >  src/remote/remote_driver.c|   41 ++
> > >  src/rpc/virnetclient.c|  123 
> > >  src/rpc/virnetclient.h|   13 +
> > >  src/rpc/virnetlibsshsession.c | 1424 
> > > +
> > >  src/rpc/virnetlibsshsession.h |   80 +++
> > >  src/rpc/virnetsocket.c|  179 ++
> > >  src/rpc/virnetsocket.h|   13 +
> > >  src/util/virerror.c   |9 +-
> > >  14 files changed, 1955 insertions(+), 3 deletions(-)
> > >  create mode 100644 m4/virt-libssh.m4
> > >  create mode 100644 src/libvirt_libssh.syms
> > >  create mode 100644 src/rpc/virnetlibsshsession.c
> > >  create mode 100644 src/rpc/virnetlibsshsession.h
> > 
> > libvirt.spec.in and mingw-libvirt.spec.in need updating too, as well
> > as docs/remote.html.in
> 
> OK for libvirt.spec.in and docs/remote.html.in, but libssh for mingw is
> not available in Fedora.

Is it possible to build libssh for mingw ?  If so we'll need to look at
getting that into Fedora.

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 v4 09/25] [ACKED] qemu: use virDomainPCIAddressReserveNextAddr in qemuDomainAssignDevicePCISlots

[Andrea Bolognani]

On Fri, 2016-10-14 at 15:54 -0400, Laine Stump wrote:
> instead of calling virDomainPCIAddressGetNextSlot() (which I want to
> turn into a local static in domain_addr.c).
> ---
> 
> Change: fixed line length
> 
>  src/qemu/qemu_domain_address.c | 33 +++--
>  1 file changed, 15 insertions(+), 18 deletions(-)

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2 2/2] libssh_transport: add new libssh-based transport

[Pino Toscano]

On Tuesday, 18 October 2016 14:19:41 CEST Daniel P. Berrange wrote:
> On Mon, Oct 17, 2016 at 04:24:53PM +0200, Pino Toscano wrote:
> > Implement a new libssh transport, which uses libssh to communicate with
> > remote hosts, and use it in virNetSockets.
> > 
> > This new transport supports all the common ssh authentication methods,
> > making use of libvirt's auth callbacks for interaction with the user.
> > 
> > Most of the functionalities and implementation are based on the libssh2
> > transport.
> > ---
> >  config-post.h |2 +
> >  configure.ac  |3 +
> >  include/libvirt/virterror.h   |2 +
> >  m4/virt-libssh.m4 |   26 +
> >  src/Makefile.am   |   21 +-
> >  src/libvirt_libssh.syms   |   22 +
> >  src/remote/remote_driver.c|   41 ++
> >  src/rpc/virnetclient.c|  123 
> >  src/rpc/virnetclient.h|   13 +
> >  src/rpc/virnetlibsshsession.c | 1424 
> > +
> >  src/rpc/virnetlibsshsession.h |   80 +++
> >  src/rpc/virnetsocket.c|  179 ++
> >  src/rpc/virnetsocket.h|   13 +
> >  src/util/virerror.c   |9 +-
> >  14 files changed, 1955 insertions(+), 3 deletions(-)
> >  create mode 100644 m4/virt-libssh.m4
> >  create mode 100644 src/libvirt_libssh.syms
> >  create mode 100644 src/rpc/virnetlibsshsession.c
> >  create mode 100644 src/rpc/virnetlibsshsession.h
> 
> libvirt.spec.in and mingw-libvirt.spec.in need updating too, as well
> as docs/remote.html.in

OK for libvirt.spec.in and docs/remote.html.in, but libssh for mingw is
not available in Fedora.

Thanks,
-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 10/25] [ACKED] conf: make virDomainPCIAddressGetNextSlot() a local static function

[Andrea Bolognani]

On Fri, 2016-10-14 at 15:54 -0400, Laine Stump wrote:
> This function is no longer needed outside of domain_addr.c.
> ---
>  src/conf/domain_addr.c   | 2 +-
>  src/conf/domain_addr.h   | 5 -
>  src/libvirt_private.syms | 1 -
>  3 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 1710220..3a9e474 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -591,7 +591,7 @@ virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr 
> addrs)
>  }
>  
>  
> -int
> +static int
>  virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,
> virPCIDeviceAddressPtr next_addr,
> virDomainPCIConnectFlags flags)
> diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
> index 904d060..4d6d12a 100644
> --- a/src/conf/domain_addr.h
> +++ b/src/conf/domain_addr.h
> @@ -155,11 +155,6 @@ int 
> virDomainPCIAddressReleaseSlot(virDomainPCIAddressSetPtr addrs,
> virPCIDeviceAddressPtr addr)
>  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  
> -int virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,
> -   virPCIDeviceAddressPtr next_addr,
> -   virDomainPCIConnectFlags flags)
> -ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

As noted while reviewing v3, you're losing both
ATTRIBUTE_NONNULL() with this commit. I was probably not
clear enough last time around, sorry about that :(

ACK with the attributes properly carried over.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 2/2] qemu: always generate the same alias for tls-creds-x509 object

[John Ferlan]

On 10/18/2016 09:58 AM, Pavel Hrdina wrote:
> There was inconsistency between alias used to create tls-creds-x509
> object and alias used to link that object to chardev while hotpluging.
> 
> In XML we have for example alias "serial0", but on qemu command line we
> generate "charserial0".
> 
> The issue was that code, that creates QMP command to hotplug chardev
> devices uses only the second alias "charserial0" and that alias is also
> used to link the tls-creds-x509 object.

Which two objects used the same ID in hotplug?

tlsProps would use obj%s_tls0

and this changes it to be essentially objchar%s_tls0

Essentially I'm trying to figure out from the commit message prior to
this patch what was created incorrectly.

John

> 
> This patch unifies the aliases for tls-creds-x509 to be always generated
> from "charserial0".
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_command.c  | 4 ++--
>  src/qemu/qemu_hotplug.c  | 9 
> +++--
>  .../qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args  | 4 ++--
>  .../qemuxml2argv-serial-tcp-tlsx509-chardev.args | 4 ++--
>  4 files changed, 13 insertions(+), 8 deletions(-)
> 



> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 74f65c0..8d87e69 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4949,10 +4949,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
>  if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir,
>  dev->data.tcp.listen,
>  cfg->chardevTLSx509verify,
> -alias, qemuCaps) < 0)
> +charAlias, qemuCaps) < 0)
>  goto error;
>  
> -if (!(objalias = qemuAliasTLSObjFromChardevAlias(alias)))
> +if (!(objalias = qemuAliasTLSObjFromChardevAlias(charAlias)))
>  goto error;
>  virBufferAsprintf(, ",tls-creds=%s", objalias);
>  VIR_FREE(objalias);
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index c2ba935..e39bd8b 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1738,7 +1738,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
>   ) < 0)
>  goto cleanup;
>  
> -if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(chr->info.alias)))
> +if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
>  goto cleanup;
>  dev->data.tcp.tlscreds = true;
>  }
> @@ -4387,6 +4387,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>  virDomainChrDefPtr tmpChr;
>  char *objAlias = NULL;
>  char *devstr = NULL;
> +char *charAlias = NULL;
>  
>  if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
>  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> @@ -4397,11 +4398,14 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>  if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 
> 0)
>  goto cleanup;
>  
> +if (virAsprintf(, "char%s", tmpChr->info.alias) < 0)
> +goto cleanup;
> +

This would seemingly need to go after the subsequent line...  Although I
think the subsequent line gets removed if use a qemu_alias.c helper.

>  sa_assert(tmpChr->info.alias);
>  
>  if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP &&
>  cfg->chardevTLS &&
> -!(objAlias = qemuAliasTLSObjFromChardevAlias(tmpChr->info.alias)))
> +!(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
>  goto cleanup;
>  
>  if (qemuBuildChrDeviceStr(, vmdef, chr, priv->qemuCaps) < 0)
> @@ -4427,6 +4431,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>   cleanup:
>  qemuDomainResetDeviceRemoval(vm);
>  VIR_FREE(devstr);
> +VIR_FREE(charAlias);
>  virObjectUnref(cfg);
>  return ret;
>  
> diff --git 
> a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args 
> b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
> index f521e33..003d11d 100644
> --- 
> a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
> +++ 
> b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
> @@ -25,9 +25,9 @@ server,nowait \
>  -chardev udp,id=charserial0,host=127.0.0.1,port=,localaddr=127.0.0.1,\
>  localport= \
>  -device isa-serial,chardev=charserial0,id=serial0 \
> --object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\
> +-object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\
>  endpoint=client,verify-peer=yes \
>  -chardev socket,id=charserial1,host=127.0.0.1,port=,\
> -tls-creds=objserial1_tls0 \
> +tls-creds=objcharserial1_tls0 \
>  

Re: [libvirt] [PATCH v2 2/2] libssh_transport: add new libssh-based transport

[Pino Toscano]

On Tuesday, 18 October 2016 15:15:07 CEST Peter Krempa wrote:
> On Mon, Oct 17, 2016 at 16:24:53 +0200, Pino Toscano wrote:
> > Implement a new libssh transport, which uses libssh to communicate with
> > remote hosts, and use it in virNetSockets.
> > 
> > This new transport supports all the common ssh authentication methods,
> > making use of libvirt's auth callbacks for interaction with the user.
> > 
> > Most of the functionalities and implementation are based on the libssh2
> > transport.
> > ---
> >  config-post.h |2 +
> >  configure.ac  |3 +
> >  include/libvirt/virterror.h   |2 +
> >  m4/virt-libssh.m4 |   26 +
> >  src/Makefile.am   |   21 +-
> >  src/libvirt_libssh.syms   |   22 +
> >  src/remote/remote_driver.c|   41 ++
> >  src/rpc/virnetclient.c|  123 
> >  src/rpc/virnetclient.h|   13 +
> >  src/rpc/virnetlibsshsession.c | 1424 
> > +
> >  src/rpc/virnetlibsshsession.h |   80 +++
> >  src/rpc/virnetsocket.c|  179 ++
> >  src/rpc/virnetsocket.h|   13 +
> >  src/util/virerror.c   |9 +-
> >  14 files changed, 1955 insertions(+), 3 deletions(-)
> >  create mode 100644 m4/virt-libssh.m4
> >  create mode 100644 src/libvirt_libssh.syms
> >  create mode 100644 src/rpc/virnetlibsshsession.c
> >  create mode 100644 src/rpc/virnetlibsshsession.h
> 
> Is it possible to split out the src/rpc/virnetlibsshsession.(ch) changes
> (plus the ones necessary to compile it) from adding all the bits that
> actually make use of it? This patch is very big.

Yes, it is -- I was not sure how much should the changes be split,
especially in cases like this when adding a new "thing" which is used
only in a single place later on.

Just to be sure, a reasonable split for this patch would be:
1) add libssh bits in virerror
2) add virnetlibsshsession.(ch) + autofoo stuff for libssh
3) add glue code in virNetSocket + virNetClient
or were you thinking about something else? (no problems on my side)

> > diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> > index 361dc1a..b296aac 100644
> > --- a/src/rpc/virnetclient.c
> > +++ b/src/rpc/virnetclient.c
> > @@ -505,6 +505,129 @@ virNetClientPtr virNetClientNewLibSSH2(const char 
> > *host,
> >  }
> >  #undef DEFAULT_VALUE
> >  
> > +#define DEFAULT_VALUE(VAR, VAL) \
> > +if (!VAR)   \
> > +VAR = VAL;
> > +virNetClientPtr virNetClientNewLibssh(const char *host,
> > +  const char *port,
> > +  int family,
> > +  const char *username,
> > +  const char *privkeyPath,
> > +  const char *knownHostsPath,
> > +  const char *knownHostsVerify,
> > +  const char *authMethods,
> > +  const char *netcatPath,
> > +  const char *socketPath,
> > +  virConnectAuthPtr authPtr,
> > +  virURIPtr uri)
> > +{
> > +virNetSocketPtr sock = NULL;
> > +virNetClientPtr ret = NULL;
> > +
> > +virBuffer buf = VIR_BUFFER_INITIALIZER;
> > +char *nc = NULL;
> > +char *command = NULL;
> > +
> > +char *homedir = virGetUserDirectory();
> > +char *confdir = virGetUserConfigDirectory();
> > +char *knownhosts = NULL;
> > +char *privkey = NULL;
> > +
> > +/* Use default paths for known hosts an public keys if not provided */
> 
> So is libssh able to handle e.g. ECDSA keys in known hosts? Libssh2 was
> not and truncated the known hosts file which was not acceptable.

Yes, it is. For example in my tests I'm passing
  known_hosts=$HOME/.ssh/known_hosts
as additional query item to the qemu+libssh URLs, and it works well.

> 
> > +if (confdir) {
> > +if (!knownHostsPath) {
> > +if (virFileExists(confdir)) {
> > +virBufferAsprintf(, "%s/known_hosts", confdir);
> > +if (!(knownhosts = virBufferContentAndReset()))
> 
> Use virAsprintf instead of the two lines above.

OK.

Small side note: all the glue code in virNetSocket, virNetClient and
remote_driver.c was basically a copy from the libssh2
implementation, so all these fixes should be done there too.

> > +memset(_passphrase, 0, sizeof(virConnectCredential));
> > +retr_passphrase.type = virCredTypeForPrompt(sess->cred, echo);
> > +retr_passphrase.prompt = virBufferCurrentContent();
> 
> This shouldn't really be used. Please get the content into a variable.

Not a problem to change this, but could you please explain a bit more
on the reasons? (So I can avoid doing the same again.)

> > +p = virStrncpy(buf, retr_passphrase.result,
> > +   

Re: [libvirt] [PATCH 1/2] qemu_command: create prefixed alias to separate variable

[Pavel Hrdina]

On Tue, Oct 18, 2016 at 10:22:50AM -0400, John Ferlan wrote:
> 
> 
> On 10/18/2016 09:58 AM, Pavel Hrdina wrote:
> > Instead of typing the prefix every time we want to append parameters
> > to qemu command line use a variable that contains prefixed alias.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/qemu/qemu_command.c | 35 ---
> >  1 file changed, 20 insertions(+), 15 deletions(-)
> > 
> 
> Why not create a qemu_alias.c helper that then can also be used in your
> followup patch?

That's a good point, I did not realize that we have qemu_alias.c.

I'll send v2, thanks.

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 v4 08/25] conf: new function virDomainPCIAddressReserveNextAddr()

[Andrea Bolognani]

On Fri, 2016-10-14 at 15:54 -0400, Laine Stump wrote:
> There is an existing virDomainPCIAddressReserveNextSlot() which will
> reserve all functions of the next available PCI slot. One place in the
> qemu PCI address assignment code requires reserving a *single*
> function of the next available PCI slot. This patch modifies and
> renames virDomainPCIAddressReserveNextSlot() so that it can fulfill
> both the original purpose and the need to reserve a single function.
> 
> (This is being done so that the abovementioned code in qemu can have
> its "kind of open coded" solution replaced with a call to this new
> function).
> ---
> 
> Changes: Fixed indentation and comments, and removed unnecessary
>  setting of lastaddr.function and lastaddr.multi.
> 
>  src/conf/domain_addr.c   | 49 
>+++-
>  src/conf/domain_addr.h   |  7 +++
>  src/libvirt_private.syms |  1 +
>  3 files changed, 52 insertions(+), 5 deletions(-)
> 
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 080d882..1710220 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -691,29 +691,68 @@ 
> virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,
>  return 0;
>  }
>  
> +
> +/**
> + * virDomainPCIAddressReserveNextAddr:
> + *
> + * find the next *completely unreserved* slot with compatible
> + * connection @flags, mark either one function or the entire
> + * slot as in-use (according to @function and @reserveEntireSlot),
> + * and set @dev->addr.pci with this newly reserved address.

This looks much much better...

> + * @addrs: a set of PCI addresses.
> + *
> + * @dev:   virDomainDeviceInfo that should get the new address.
> + *
> + * @flags: CONNECT_TYPE flags for the device that needs an address.
> + *
> + * @function:  which function on the slot to mark as reserved
> + * (if @reserveEntireSlot is false)
> + *
> + * @reserveEntireSlot: true to reserve all functions on the new slot,
> + *  false to reserve just @function

... but these are still weird. Please take a look at
include/libvirt/libvirt-domain.h to see what I mean.

Interestingly, in that file the arguments are documented
*before* describing the function itself. We should probably
stick to that order here as well.

> + *

Extra line here, didn't notice it the first time around.

> + *
> + * returns 0 on success, or -1 on failure.
> + */
>  int
> -virDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs,
> +virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,

ACK after you tweak indentation for the comment :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 1/2] qemu_command: create prefixed alias to separate variable

[John Ferlan]

On 10/18/2016 09:58 AM, Pavel Hrdina wrote:
> Instead of typing the prefix every time we want to append parameters
> to qemu command line use a variable that contains prefixed alias.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_command.c | 35 ---
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 

Why not create a qemu_alias.c helper that then can also be used in your
followup patch?

John

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 21fd85c..74f65c0 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4861,28 +4861,32 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
>  {
>  virBuffer buf = VIR_BUFFER_INITIALIZER;
>  bool telnet;
> +char *charAlias = NULL;
> +
> +if (virAsprintf(, "char%s", alias) < 0)
> +goto error;
>  
>  switch (dev->type) {
>  case VIR_DOMAIN_CHR_TYPE_NULL:
> -virBufferAsprintf(, "null,id=char%s", alias);
> +virBufferAsprintf(, "null,id=%s", charAlias);
>  break;
>  
>  case VIR_DOMAIN_CHR_TYPE_VC:
> -virBufferAsprintf(, "vc,id=char%s", alias);
> +virBufferAsprintf(, "vc,id=%s", charAlias);
>  break;
>  
>  case VIR_DOMAIN_CHR_TYPE_PTY:
> -virBufferAsprintf(, "pty,id=char%s", alias);
> +virBufferAsprintf(, "pty,id=%s", charAlias);
>  break;
>  
>  case VIR_DOMAIN_CHR_TYPE_DEV:
> -virBufferAsprintf(, "%s,id=char%s,path=%s",
> +virBufferAsprintf(, "%s,id=%s,path=%s",
>STRPREFIX(alias, "parallel") ? "parport" : "tty",
> -  alias, dev->data.file.path);
> +  charAlias, dev->data.file.path);
>  break;
>  
>  case VIR_DOMAIN_CHR_TYPE_FILE:
> -virBufferAsprintf(, "file,id=char%s", alias);
> +virBufferAsprintf(, "file,id=%s", charAlias);
>  
>  if (dev->data.file.append != VIR_TRISTATE_SWITCH_ABSENT &&
>  !virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FILE_APPEND)) {
> @@ -4898,12 +4902,12 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
>  break;
>  
>  case VIR_DOMAIN_CHR_TYPE_PIPE:
> -virBufferAsprintf(, "pipe,id=char%s,path=%s", alias,
> +virBufferAsprintf(, "pipe,id=%s,path=%s", charAlias,
>dev->data.file.path);
>  break;
>  
>  case VIR_DOMAIN_CHR_TYPE_STDIO:
> -virBufferAsprintf(, "stdio,id=char%s", alias);
> +virBufferAsprintf(, "stdio,id=%s", charAlias);
>  break;
>  
>  case VIR_DOMAIN_CHR_TYPE_UDP: {
> @@ -4919,9 +4923,9 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
>  bindService = "0";
>  
>  virBufferAsprintf(,
> -  "udp,id=char%s,host=%s,port=%s,localaddr=%s,"
> +  "udp,id=%s,host=%s,port=%s,localaddr=%s,"
>"localport=%s",
> -  alias,
> +  charAlias,
>connectHost,
>dev->data.udp.connectService,
>bindHost, bindService);
> @@ -4930,8 +4934,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
>  case VIR_DOMAIN_CHR_TYPE_TCP:
>  telnet = dev->data.tcp.protocol == 
> VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET;
>  virBufferAsprintf(,
> -  "socket,id=char%s,host=%s,port=%s%s",
> -  alias,
> +  "socket,id=%s,host=%s,port=%s%s",
> +  charAlias,
>dev->data.tcp.host,
>dev->data.tcp.service,
>telnet ? ",telnet" : "");
> @@ -4956,7 +4960,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
>  break;
>  
>  case VIR_DOMAIN_CHR_TYPE_UNIX:
> -virBufferAsprintf(, "socket,id=char%s,path=", alias);
> +virBufferAsprintf(, "socket,id=%s,path=", charAlias);
>  virQEMUBuildBufferEscapeComma(, dev->data.nix.path);
>  if (dev->data.nix.listen)
>  virBufferAdd(, nowait ? ",server,nowait" : ",server", -1);
> @@ -4968,7 +4972,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
> _("spicevmc not supported in this QEMU binary"));
>  goto error;
>  }
> -virBufferAsprintf(, "spicevmc,id=char%s,name=%s", alias,
> +virBufferAsprintf(, "spicevmc,id=%s,name=%s", charAlias,
>
> virDomainChrSpicevmcTypeToString(dev->data.spicevmc));
>  break;
>  
> @@ -4978,7 +4982,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
> _("spiceport not supported in this QEMU binary"));
>  goto error;
>  }
> -virBufferAsprintf(, "spiceport,id=char%s,name=%s", alias,
> +virBufferAsprintf(, 

[libvirt] [PATCH 0/2] fix hot(un)plug of chardev devices with TLS encryption

[Pavel Hrdina]

Pavel Hrdina (2):
  qemu_command: create prefixed alias to separate variable
  qemu: always generate the same alias for tls-creds-x509 object

 src/qemu/qemu_command.c| 39 --
 src/qemu/qemu_hotplug.c|  9 +++--
 ...xml2argv-serial-tcp-tlsx509-chardev-verify.args |  4 +--
 .../qemuxml2argv-serial-tcp-tlsx509-chardev.args   |  4 +--
 4 files changed, 33 insertions(+), 23 deletions(-)

-- 
2.10.1

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

[libvirt] How libvirt address qemu command line args

[zhun...@gmail.com]

Now I want to add some args about TPM  to domain's XML,so I can start a domain 
by virt-manager or other virsh command,and then ,I would like to use sVIrt 
security context to label vTPM and correspondingVM,But I do not know how to get 
these XML  args in libvirt.
the key problem is that how can i get and recognize these args!!!
related XML content :











  




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

Re: [libvirt] [PATCH] vsh: Using vshError rather than virReportError

[Peter Krempa]

On Tue, Oct 18, 2016 at 07:39:16 -0400, Kothapally Madhu Pavan wrote:
> Correcting the error reporting method by using vshError
> instead of virReportError
> 
> Signed-off-by: Kothapally Madhu Pavan 
> ---
>  tools/virsh-domain.c |6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 050e7fb..c9fabf2 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -10475,9 +10475,9 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd)
>  
>  if (vshCommandOptBool(cmd, "postcopy-after-precopy")) {
>  if (!vshCommandOptBool(cmd, "postcopy")) {
> -virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> -  _("--postcopy-after-precopy can only be used with "
> -"--postcopy"));
> +vshError(ctl, "%s",
> + _("argument unsupported: --postcopy-after-precopy can 
> only "
> +   "be used with --postcopy"));
>  goto cleanup;

I think the above code can be avoided by simply using
VSH_REQUIRE_OPTION.


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

[libvirt] [PATCH 2/2] qemu: always generate the same alias for tls-creds-x509 object

[Pavel Hrdina]

There was inconsistency between alias used to create tls-creds-x509
object and alias used to link that object to chardev while hotpluging.

In XML we have for example alias "serial0", but on qemu command line we
generate "charserial0".

The issue was that code, that creates QMP command to hotplug chardev
devices uses only the second alias "charserial0" and that alias is also
used to link the tls-creds-x509 object.

This patch unifies the aliases for tls-creds-x509 to be always generated
from "charserial0".

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_command.c  | 4 ++--
 src/qemu/qemu_hotplug.c  | 9 +++--
 .../qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args  | 4 ++--
 .../qemuxml2argv-serial-tcp-tlsx509-chardev.args | 4 ++--
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 74f65c0..8d87e69 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4949,10 +4949,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir,
 dev->data.tcp.listen,
 cfg->chardevTLSx509verify,
-alias, qemuCaps) < 0)
+charAlias, qemuCaps) < 0)
 goto error;
 
-if (!(objalias = qemuAliasTLSObjFromChardevAlias(alias)))
+if (!(objalias = qemuAliasTLSObjFromChardevAlias(charAlias)))
 goto error;
 virBufferAsprintf(, ",tls-creds=%s", objalias);
 VIR_FREE(objalias);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c2ba935..e39bd8b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1738,7 +1738,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
  ) < 0)
 goto cleanup;
 
-if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(chr->info.alias)))
+if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
 goto cleanup;
 dev->data.tcp.tlscreds = true;
 }
@@ -4387,6 +4387,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
 virDomainChrDefPtr tmpChr;
 char *objAlias = NULL;
 char *devstr = NULL;
+char *charAlias = NULL;
 
 if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -4397,11 +4398,14 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
 if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0)
 goto cleanup;
 
+if (virAsprintf(, "char%s", tmpChr->info.alias) < 0)
+goto cleanup;
+
 sa_assert(tmpChr->info.alias);
 
 if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP &&
 cfg->chardevTLS &&
-!(objAlias = qemuAliasTLSObjFromChardevAlias(tmpChr->info.alias)))
+!(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
 goto cleanup;
 
 if (qemuBuildChrDeviceStr(, vmdef, chr, priv->qemuCaps) < 0)
@@ -4427,6 +4431,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
  cleanup:
 qemuDomainResetDeviceRemoval(vm);
 VIR_FREE(devstr);
+VIR_FREE(charAlias);
 virObjectUnref(cfg);
 return ret;
 
diff --git 
a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args 
b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
index f521e33..003d11d 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
@@ -25,9 +25,9 @@ server,nowait \
 -chardev udp,id=charserial0,host=127.0.0.1,port=,localaddr=127.0.0.1,\
 localport= \
 -device isa-serial,chardev=charserial0,id=serial0 \
--object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\
+-object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\
 endpoint=client,verify-peer=yes \
 -chardev socket,id=charserial1,host=127.0.0.1,port=,\
-tls-creds=objserial1_tls0 \
+tls-creds=objcharserial1_tls0 \
 -device isa-serial,chardev=charserial1,id=serial1 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git 
a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args 
b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args
index 4c8c23e..b456cce 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args
@@ -25,9 +25,9 @@ server,nowait \
 -chardev udp,id=charserial0,host=127.0.0.1,port=,localaddr=127.0.0.1,\
 localport= \
 -device isa-serial,chardev=charserial0,id=serial0 \
--object 

[libvirt] [PATCH 1/2] qemu_command: create prefixed alias to separate variable

[Pavel Hrdina]

Instead of typing the prefix every time we want to append parameters
to qemu command line use a variable that contains prefixed alias.

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_command.c | 35 ---
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 21fd85c..74f65c0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4861,28 +4861,32 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 {
 virBuffer buf = VIR_BUFFER_INITIALIZER;
 bool telnet;
+char *charAlias = NULL;
+
+if (virAsprintf(, "char%s", alias) < 0)
+goto error;
 
 switch (dev->type) {
 case VIR_DOMAIN_CHR_TYPE_NULL:
-virBufferAsprintf(, "null,id=char%s", alias);
+virBufferAsprintf(, "null,id=%s", charAlias);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_VC:
-virBufferAsprintf(, "vc,id=char%s", alias);
+virBufferAsprintf(, "vc,id=%s", charAlias);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_PTY:
-virBufferAsprintf(, "pty,id=char%s", alias);
+virBufferAsprintf(, "pty,id=%s", charAlias);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_DEV:
-virBufferAsprintf(, "%s,id=char%s,path=%s",
+virBufferAsprintf(, "%s,id=%s,path=%s",
   STRPREFIX(alias, "parallel") ? "parport" : "tty",
-  alias, dev->data.file.path);
+  charAlias, dev->data.file.path);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_FILE:
-virBufferAsprintf(, "file,id=char%s", alias);
+virBufferAsprintf(, "file,id=%s", charAlias);
 
 if (dev->data.file.append != VIR_TRISTATE_SWITCH_ABSENT &&
 !virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FILE_APPEND)) {
@@ -4898,12 +4902,12 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 break;
 
 case VIR_DOMAIN_CHR_TYPE_PIPE:
-virBufferAsprintf(, "pipe,id=char%s,path=%s", alias,
+virBufferAsprintf(, "pipe,id=%s,path=%s", charAlias,
   dev->data.file.path);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_STDIO:
-virBufferAsprintf(, "stdio,id=char%s", alias);
+virBufferAsprintf(, "stdio,id=%s", charAlias);
 break;
 
 case VIR_DOMAIN_CHR_TYPE_UDP: {
@@ -4919,9 +4923,9 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 bindService = "0";
 
 virBufferAsprintf(,
-  "udp,id=char%s,host=%s,port=%s,localaddr=%s,"
+  "udp,id=%s,host=%s,port=%s,localaddr=%s,"
   "localport=%s",
-  alias,
+  charAlias,
   connectHost,
   dev->data.udp.connectService,
   bindHost, bindService);
@@ -4930,8 +4934,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 case VIR_DOMAIN_CHR_TYPE_TCP:
 telnet = dev->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET;
 virBufferAsprintf(,
-  "socket,id=char%s,host=%s,port=%s%s",
-  alias,
+  "socket,id=%s,host=%s,port=%s%s",
+  charAlias,
   dev->data.tcp.host,
   dev->data.tcp.service,
   telnet ? ",telnet" : "");
@@ -4956,7 +4960,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 break;
 
 case VIR_DOMAIN_CHR_TYPE_UNIX:
-virBufferAsprintf(, "socket,id=char%s,path=", alias);
+virBufferAsprintf(, "socket,id=%s,path=", charAlias);
 virQEMUBuildBufferEscapeComma(, dev->data.nix.path);
 if (dev->data.nix.listen)
 virBufferAdd(, nowait ? ",server,nowait" : ",server", -1);
@@ -4968,7 +4972,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
_("spicevmc not supported in this QEMU binary"));
 goto error;
 }
-virBufferAsprintf(, "spicevmc,id=char%s,name=%s", alias,
+virBufferAsprintf(, "spicevmc,id=%s,name=%s", charAlias,
   
virDomainChrSpicevmcTypeToString(dev->data.spicevmc));
 break;
 
@@ -4978,7 +4982,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
_("spiceport not supported in this QEMU binary"));
 goto error;
 }
-virBufferAsprintf(, "spiceport,id=char%s,name=%s", alias,
+virBufferAsprintf(, "spiceport,id=%s,name=%s", charAlias,
   dev->data.spiceport.channel);
 break;
 
@@ -5007,6 +5011,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 return virBufferContentAndReset();
 
  error:
+VIR_FREE(charAlias);
 virBufferFreeAndReset();
 return NULL;
 }
-- 
2.10.1

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

Re: [libvirt] [PATCH] cpu_conf: add comments about sockets in cpu_conf

[Peter Krempa]

On Tue, Oct 18, 2016 at 16:18:10 +0800, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> 'sockets' in output of `virsh capabilities' means

You are talking here about virsh capabilities, while the code is about
the cpu definition in the domain configuration.

> the sockets per NUMA node,
> which is a special case.

Indeed it is a special case in the capabilities XML. So why are you
changing the code relevant to domain definition, where the sockets
count is actually the total sockets count?

> discuss in:
> https://www.redhat.com/archives/libvir-list/2012-March/msg01123.html

https://www.redhat.com/archives/libvir-list/2012-March/msg01133.html
describes why sockets per NUMA node is not really what we want.

> Signed-off-by: Chen Hanxiao 
> ---


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

Re: [libvirt] [PATCH v4 07/25] [NEW] qemu: change first arg of qemuDomainAttachChrDeviceAssignAddr()

[Andrea Bolognani]

On Fri, 2016-10-14 at 15:54 -0400, Laine Stump wrote:
> from virDomainDefPtr to virDomainObjPtr so that the function has
> access to the other parts of the virDomainObjPtr. Take advantage of
> this by removing the "priv" arg and retrieving it from the
> virDomainObjPtr instead.
> 
> No functional change.
> ---
>  src/qemu/qemu_hotplug.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 14af4e1..dc0faa6 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1636,10 +1636,11 @@ qemuDomainChrRemove(virDomainDefPtr vmdef,
>  }
>  
>  static int
> -qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def,
> -qemuDomainObjPrivatePtr priv,
> +qemuDomainAttachChrDeviceAssignAddr(virDomainObjPtr vm,
>  virDomainChrDefPtr chr)
>  {
> +virDomainDefPtr def = vm->def;
> +qemuDomainObjPrivatePtr priv = vm->privateData;
>  int ret = -1;
>  virDomainVirtioSerialAddrSetPtr vioaddrs = NULL;
>  
> @@ -1715,7 +1716,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
>  if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0)
>  goto cleanup;
>  
> -if ((rc = qemuDomainAttachChrDeviceAssignAddr(vm->def, priv, chr)) < 0)
> +if ((rc = qemuDomainAttachChrDeviceAssignAddr(vm, chr)) < 0)
>  goto cleanup;
>  if (rc == 1)
>  need_release = true;

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v4 06/25] [NEW] qemu: make error message in qemuDomainPCIAddressSetCreate more clear.

[Andrea Bolognani]

On Fri, 2016-10-14 at 15:54 -0400, Laine Stump wrote:
> This error should only ever be seen by a developer anyway, but the
> existing message made even less sense that this new version.
> ---
>  src/qemu/qemu_domain_address.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 3926b18..8628dc1 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -583,7 +583,7 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
>  if (idx >= addrs->nbuses) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Inappropriate new pci controller index %zu "
> - "not found in addrs"), idx);
> + "exceeds addrs array length"), idx);
>  goto error;
>  }

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2 2/2] libssh_transport: add new libssh-based transport

[Daniel P. Berrange]

On Mon, Oct 17, 2016 at 04:24:53PM +0200, Pino Toscano wrote:
> Implement a new libssh transport, which uses libssh to communicate with
> remote hosts, and use it in virNetSockets.
> 
> This new transport supports all the common ssh authentication methods,
> making use of libvirt's auth callbacks for interaction with the user.
> 
> Most of the functionalities and implementation are based on the libssh2
> transport.
> ---
>  config-post.h |2 +
>  configure.ac  |3 +
>  include/libvirt/virterror.h   |2 +
>  m4/virt-libssh.m4 |   26 +
>  src/Makefile.am   |   21 +-
>  src/libvirt_libssh.syms   |   22 +
>  src/remote/remote_driver.c|   41 ++
>  src/rpc/virnetclient.c|  123 
>  src/rpc/virnetclient.h|   13 +
>  src/rpc/virnetlibsshsession.c | 1424 
> +
>  src/rpc/virnetlibsshsession.h |   80 +++
>  src/rpc/virnetsocket.c|  179 ++
>  src/rpc/virnetsocket.h|   13 +
>  src/util/virerror.c   |9 +-
>  14 files changed, 1955 insertions(+), 3 deletions(-)
>  create mode 100644 m4/virt-libssh.m4
>  create mode 100644 src/libvirt_libssh.syms
>  create mode 100644 src/rpc/virnetlibsshsession.c
>  create mode 100644 src/rpc/virnetlibsshsession.h

libvirt.spec.in and mingw-libvirt.spec.in need updating too, as well
as docs/remote.html.in


> +static virClassPtr virNetLibsshSessionClass;
> +static int
> +virNetLibsshSessionOnceInit(void)
> +{
> +const char *dbgLevelStr;
> +
> +if (!(virNetLibsshSessionClass = virClassNew(virClassForObjectLockable(),
> + "virNetLibsshSession",
> + sizeof(virNetLibsshSession),
> + 
> virNetLibsshSessionDispose)))
> +return -1;
> +
> +if (ssh_init() < 0)
> +return -1;

You need to report a libvirt error here.


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 v4 05/25] [NEW] qemu: remove superfluous setting of addrs->nbuses

[Andrea Bolognani]

On Fri, 2016-10-14 at 15:54 -0400, Laine Stump wrote:
> This is already set by virDomainPCIAddressSetAlloc().
> ---
>  src/qemu/qemu_domain_address.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index d2a3237..3926b18 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -556,7 +556,6 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
>  if ((addrs = virDomainPCIAddressSetAlloc(nbuses)) == NULL)
>  return NULL;
>  
> -addrs->nbuses = nbuses;
>  addrs->dryRun = dryRun;
>  
>  /* As a safety measure, set default model='pci-root' for first pci

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2 2/2] libssh_transport: add new libssh-based transport

[Peter Krempa]

On Mon, Oct 17, 2016 at 16:24:53 +0200, Pino Toscano wrote:
> Implement a new libssh transport, which uses libssh to communicate with
> remote hosts, and use it in virNetSockets.
> 
> This new transport supports all the common ssh authentication methods,
> making use of libvirt's auth callbacks for interaction with the user.
> 
> Most of the functionalities and implementation are based on the libssh2
> transport.
> ---
>  config-post.h |2 +
>  configure.ac  |3 +
>  include/libvirt/virterror.h   |2 +
>  m4/virt-libssh.m4 |   26 +
>  src/Makefile.am   |   21 +-
>  src/libvirt_libssh.syms   |   22 +
>  src/remote/remote_driver.c|   41 ++
>  src/rpc/virnetclient.c|  123 
>  src/rpc/virnetclient.h|   13 +
>  src/rpc/virnetlibsshsession.c | 1424 
> +
>  src/rpc/virnetlibsshsession.h |   80 +++
>  src/rpc/virnetsocket.c|  179 ++
>  src/rpc/virnetsocket.h|   13 +
>  src/util/virerror.c   |9 +-
>  14 files changed, 1955 insertions(+), 3 deletions(-)
>  create mode 100644 m4/virt-libssh.m4
>  create mode 100644 src/libvirt_libssh.syms
>  create mode 100644 src/rpc/virnetlibsshsession.c
>  create mode 100644 src/rpc/virnetlibsshsession.h

Is it possible to split out the src/rpc/virnetlibsshsession.(ch) changes
(plus the ones necessary to compile it) from adding all the bits that
actually make use of it? This patch is very big.

> diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
> index efe83aa..2efee8f 100644
> --- a/include/libvirt/virterror.h
> +++ b/include/libvirt/virterror.h
> @@ -131,6 +131,7 @@ typedef enum {
>  VIR_FROM_XENXL = 64,/* Error from Xen xl config code */
>  
>  VIR_FROM_PERF = 65, /* Error from perf */
> +VIR_FROM_LIBSSH = 66,   /* Error from libssh connection transport */
>  
>  # ifdef VIR_ENUM_SENTINELS
>  VIR_ERR_DOMAIN_LAST
> @@ -317,6 +318,7 @@ typedef enum {
>  VIR_ERR_NO_CLIENT = 96, /* Client was not found */
>  VIR_ERR_AGENT_UNSYNCED = 97,/* guest agent replies with wrong id
> to guest-sync command */
> +VIR_ERR_LIBSSH = 98,/* error in libssh transport driver 
> */

These error handling changes can be split to a separate patch as well.

>  } virErrorNumber;
>  
>  /**

[...]

> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index a3cd7cd..db2bdd4 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -673,6 +673,7 @@ remoteConnectSupportsFeatureUnlocked(virConnectPtr conn,
>   *   - xxx:///-> UNIX domain socket
>   *   - xxx+ssh:///-> SSH connection (legacy)
>   *   - xxx+libssh2:///-> SSH connection (using libssh2)
> + *   - xxx+libssh:/// -> SSH connection (using libssh)
>   */
>  static int
>  doRemoteOpen(virConnectPtr conn,
> @@ -689,6 +690,7 @@ doRemoteOpen(virConnectPtr conn,
>  trans_libssh2,
>  trans_ext,
>  trans_tcp,
> +trans_libssh,
>  } transport;
>  #ifndef WIN32
>  char *daemonPath = NULL;
> @@ -736,6 +738,8 @@ doRemoteOpen(virConnectPtr conn,
>  transport = trans_ext;
>  } else if (STRCASEEQ(transport_str, "tcp")) {
>  transport = trans_tcp;
> +} else if (STRCASEEQ(transport_str, "libssh")) {
> +transport = trans_libssh;
>  } else {
>  virReportError(VIR_ERR_INVALID_ARG, "%s",
> _("remote_open: transport in URL not 
> recognised "
> @@ -959,6 +963,43 @@ doRemoteOpen(virConnectPtr conn,
>  priv->is_secure = 1;
>  break;
>  
> +case trans_libssh:
> +if (!sockname) {
> +/* Right now we don't support default session connections */
> +if (STREQ_NULLABLE(conn->uri->path, "/session")) {
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +   _("Connecting to session instance without "
> + "socket path is not supported by the libssh 
> "
> + "connection driver"));
> +goto failed;
> +}
> +
> +if (VIR_STRDUP(sockname,
> +   flags & VIR_DRV_OPEN_REMOTE_RO ?
> +   LIBVIRTD_PRIV_UNIX_SOCKET_RO : 
> LIBVIRTD_PRIV_UNIX_SOCKET) < 0)
> +goto failed;
> +}
> +
> +VIR_DEBUG("Starting libssh session");
> +
> +priv->client = virNetClientNewLibssh(priv->hostname,
> + port,
> + AF_UNSPEC,
> + username,
> + keyfile,
> +

Re: [libvirt] [PATCH v4 04/25] [NEW] conf: add typedef for anonymous enum used for memballoon device model

[Andrea Bolognani]

On Fri, 2016-10-14 at 15:54 -0400, Laine Stump wrote:
> For some reason the values of memballoon model are set using an
> anonymous enum, making it impossible to perform nice tricks like
> demanding there are cases for all possible values in a switch. This
> patch turns the anonymous enum into virDomainMemBalloonModel.
> ---
>  src/conf/domain_conf.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 3e85ae4..d415d2e 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1536,13 +1536,13 @@ struct _virDomainRedirFilterDef {
>  virDomainRedirFilterUSBDevDefPtr *usbdevs;
>  };
>  
> -enum {
> +typedef enum {
>  VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO,
>  VIR_DOMAIN_MEMBALLOON_MODEL_XEN,
>  VIR_DOMAIN_MEMBALLOON_MODEL_NONE,
>  
>  VIR_DOMAIN_MEMBALLOON_MODEL_LAST
> -};
> +} virDomainMemBalloonModel;
>  
>  struct _virDomainMemballoonDef {
>  int model;

Great catch!

However, you'll want to

  s/MemBalloon/Memballoon/

because further down the file you can find

  VIR_ENUM_DECL(virDomainMemballoonModel)

I like your name better, by the way, so if you feel like
renameing every instance in a separate patch I'll gladly
review and ACK it :)

ACK with the above taken care of.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v9 1/5] domain: Add optional 'tls' attribute for TCP chardev

[John Ferlan]

On 10/18/2016 07:21 AM, Daniel P. Berrange wrote:
> On Tue, Oct 18, 2016 at 06:59:57AM -0400, John Ferlan wrote:
>>
>>
>> On 10/18/2016 02:27 AM, Pavel Hrdina wrote:
>> [...]
>>

 "As default behaviour I think it is desirable that we can turn TLS on
 for every VM at once - I tend to view it as a host network integration
 task, rather than a VM configuration task. Same rationale that we use
 for TLS wth VNC/SPICE."
>>>
>>> Don't forget this part of the same review:
>>>
>>> "There's no reason we can't have a tri-state TLS flag against the chardev
>>> in the XML too, to override the default behaviour of cfg->chardevTLS"
>>>
>>> That also means to override chardev_tls = "0" by "tls='yes'".
>>
>> If the default cfg behaviour is "1", then that tells us "someone" has
>> set up the TLS environment and thus the domain/chardev override would be
>> "no".
>>
>> If the default cfg behaviour is "0", then that means we cannot guarantee
>> the environment necessary has been set up and we cannot allow the
>> domain/chardev setting to enable TLS.
> 
> We have two separate tasks at the host level
> 
>  - Setup of TLS certificates (ie put the PEM files in the right places)
>  - Global default for use of TLS by chardevs
> 
> We only have a config option in qemu.conf for the latter. ie if
> chardev_tls=1, this is implicitly saying that TLS certs are deployed
> in right place.  IIUC, you're saying that if chardev_tls=0, then we
> should interpret that to meant TLS certs are *not* deployed.
> 

If someone has gone through the trouble of setting up their environment
because they found in qemu.conf support was added, why would they set
chardev_tls=0 afterwards? IOW: Are we assuming something or are we over
engineering things?

Why would someone set up and define chardevTLSx509certdir and then have
chardevTLS=0? Maybe, they believe this will disable TLS for every
domain. After all, why would disabling a global setting allow some
localized setting override that.

Just because there's a "default" environment, why should we assume that
because they added "tls='yes'" into their domain that that is the
environment they want?

Maybe they do have a chardev specific environment, but also decided to
comment it out when they commented out chardev_tls=1? So we're assuming
that by finding that "tls='yes'" in the domain that the domain should
use that environment? How can we be so sure that's what's desired?

This is the problem with two triggers - which one do you pull and which
one has a bullet in the chamber? We're having a hard enough time
agreeing on the implementation. Now the customer has to figure out all
the permutations and possibilities.

Personally, if I disable a global variable/value, then I wouldn't expect
some other definition to override that. For example, if I set my SELinux
to be "permissive" or "disabled", then I'm probably not very happy if
something ignores that and decides to be "enforcing".

> Pavel is saying that if chardev_tls=0 in qemu.conf, and tls=1 in the
> XML, then we should assume that TLS certs *are* deployed on the host
> in the right place. I think this is not unreasonable - we can easily
> check to see if the certs exist on disk in this case.

FWIW: Remember how qemu_conf.c sets things up...

During qemuStateInitialize, we call virQEMUDriverConfigNew which will
set a default directory path for "cfg->defaultTLSx509certdir" and then
will use that as a default value for vnc, spice, and chardev. There is
no check "if" that directory structure exists.

Once the defaults are set up, then if "chardev_tls_x509_cert_dir" is
defined, it will override the default (similar for others).

So chardevTLSx509certdir always has some default value, but is that the
certificate environment we want to use for chardev's? How do we know?

> 
> IOW, I agree that we should have a tri-state at the XML level
> 
>  - no tls attribute in XML - honour  chardev_tls from qemu.conf
>  - tls=1 in XML, then turn on TLS
>  - tls=0 in XML, then don't use TLS
> 

So if we get this far, then a qemu_domain.c function, such as

bool
qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg,
   const virDomainChrSourceDef *dev)
{
if (cfg->chardevTLS && dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO)
return true;
if (!cfg->chardevTLS && dev->data.tcp.haveTLS ==
VIR_TRISTATE_BOOL_YES &&
virFileExists(cfg->chardevTLSx509certdir))
return true;
return false;
}

where I suppose we could also compare chardevTLSx509certdir and
defaultTLSx509certdir... Still how do we know that by merely having the
environment there and "tls='yes'" in the domain chardev config that this
is what's desired.  It might well be, but it may also be that the
clearing of the global chardev_tls was meant to disable regardless of
what the domain setting was.

John

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

Re: [libvirt] [PATCH v4 03/25] [NEW] qemu: replace "def->nets[i]" with "net" and "def->sounds[i]" with "sound"

[Andrea Bolognani]

On Fri, 2016-10-14 at 15:53 -0400, Laine Stump wrote:
> More occurences of repeatedly dereferencing the same pointer stored in
> an array are replaced with the definition of a temporary pointer that
> is then used directly. No functional change.
> ---
>  src/qemu/qemu_domain_address.c | 41 +++--
>  1 file changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index e6abadf..d2a3237 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -211,11 +211,12 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def,
>  /* Default values match QEMU. See spapr_(llan|vscsi|vty).c */
>  
>  for (i = 0; i < def->nnets; i++) {
> -if (def->nets[i]->model &&
> -STREQ(def->nets[i]->model, "spapr-vlan"))
> -def->nets[i]->info.type = 
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
> -if (qemuDomainAssignSpaprVIOAddress(def, >nets[i]->info,
> -VIO_ADDR_NET) < 0)
> +virDomainNetDefPtr net = def->nets[i];
> +
> +if (net->model &&
> +STREQ(net->model, "spapr-vlan"))
> +net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;

Our coding style requires brackets here, please add them.

[...]
>  /* Sound cards */
>  for (i = 0; i < def->nsounds; i++) {
> -if (!virDeviceInfoPCIAddressWanted(>sounds[i]->info))
> +virDomainSoundDefPtr sound = def->sounds[i];
> +
> +if (!virDeviceInfoPCIAddressWanted(>info))
>  continue;
>  /* Skip ISA sound card, PCSPK and usb-audio */
> -if (def->sounds[i]->model == VIR_DOMAIN_SOUND_MODEL_SB16 ||
> -def->sounds[i]->model == VIR_DOMAIN_SOUND_MODEL_PCSPK ||
> -def->sounds[i]->model == VIR_DOMAIN_SOUND_MODEL_USB)
> +if (sound->model == VIR_DOMAIN_SOUND_MODEL_SB16 ||
> +sound->model == VIR_DOMAIN_SOUND_MODEL_PCSPK ||
> +sound->model == VIR_DOMAIN_SOUND_MODEL_USB)
>  continue;

This one needs brackets as well.

ACK with those nits fixed.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH] vsh: Using vshError rather than virReportError

[Kothapally Madhu Pavan]

Correcting the error reporting method by using vshError
instead of virReportError

Signed-off-by: Kothapally Madhu Pavan 
---
 tools/virsh-domain.c |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 050e7fb..c9fabf2 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10475,9 +10475,9 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd)
 
 if (vshCommandOptBool(cmd, "postcopy-after-precopy")) {
 if (!vshCommandOptBool(cmd, "postcopy")) {
-virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
-  _("--postcopy-after-precopy can only be used with "
-"--postcopy"));
+vshError(ctl, "%s",
+ _("argument unsupported: --postcopy-after-precopy can 
only "
+   "be used with --postcopy"));
 goto cleanup;
 }
 iterEvent = virConnectDomainEventRegisterAny(

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

Re: [libvirt] [PATCH] qemu_hotplug: fix crash in hot(un)plugging chardev devices

[Pavel Hrdina]

On Tue, Oct 18, 2016 at 01:26:55PM +0200, Peter Krempa wrote:
> On Tue, Oct 18, 2016 at 13:20:01 +0200, Pavel Hrdina wrote:
> > We need to make sure that the chardev is serial and TCP.
> 
> Note that it corrupts pointers in a different part of the union.
> 
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/qemu/qemu_hotplug.c | 8 ++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index 14af4e1..1003d50 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -1729,7 +1729,9 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
> >  if (qemuDomainChrPreInsert(vmdef, chr) < 0)
> >  goto cleanup;
> >  
> > -if (cfg->chardevTLS) {
> > +if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
> 
> This is relevant for all chardevs that have TCP transport not just the
> serial ones, thus only the actual backend dev should matter.

Right, I didn't realize that, thanks, I'll push it shortly.


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_hotplug: fix crash in hot(un)plugging chardev devices

[John Ferlan]

On 10/18/2016 07:20 AM, Pavel Hrdina wrote:
> We need to make sure that the chardev is serial and TCP.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_hotplug.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 

ACK - although you could put a few more details in the commit message
and perhaps a short stack trace...

John

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

Re: [libvirt] [PATCH] qemu_hotplug: fix crash in hot(un)plugging chardev devices

[Peter Krempa]

On Tue, Oct 18, 2016 at 13:20:01 +0200, Pavel Hrdina wrote:
> We need to make sure that the chardev is serial and TCP.

Note that it corrupts pointers in a different part of the union.

> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_hotplug.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 14af4e1..1003d50 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1729,7 +1729,9 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
>  if (qemuDomainChrPreInsert(vmdef, chr) < 0)
>  goto cleanup;
>  
> -if (cfg->chardevTLS) {
> +if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&

This is relevant for all chardevs that have TCP transport not just the
serial ones, thus only the actual backend dev should matter.

ACK with that



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

Re: [libvirt] [PATCH v4 02/25] [ACKED but has additions] qemu: replace a lot of "def->controllers[i]" with equivalent "cont"

[Andrea Bolognani]

On Fri, 2016-10-14 at 15:53 -0400, Laine Stump wrote:
> There's no functional change here. This pointer was just used so many
> times that the extra long lines became annoying.
> ---
> 
> Change: added more instances of the same change.
> 
>  src/qemu/qemu_domain_address.c | 208 
>++---
>  1 file changed, 111 insertions(+), 97 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index dc67d51..e6abadf 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -220,18 +220,22 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def,
>  }
>  
>  for (i = 0; i < def->ncontrollers; i++) {
> -model = def->controllers[i]->model;
> -if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> +virDomainControllerDefPtr cont = def->controllers[i];
> +
> +model = cont->model;
> +if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
>  if (qemuDomainSetSCSIControllerModel(def, qemuCaps, ) < 0)

Definitely not something that should be touched by this patch,
but shouldn't we pass >model here?

I mean, if the value stored in model will be different than
the one that was already in cont->model, it means that the
default controller model was not set properly earlier...

On the other hand, the default model should really have been
set in PostParse() or something like that.

> @@ -743,36 +749,38 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
>  virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPE_PCIE_DEVICE;
>  
>  for (i = 0; i < def->ncontrollers; i++) {
> -switch (def->controllers[i]->type) {
> +virDomainControllerDefPtr cont = def->controllers[i];
> +
> +switch (cont->type) {
>  case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
>  /* Verify that the first SATA controller is at 00:1F.2 the
>   * q35 machine type *always* has a SATA controller at this
>   * address.
>   */
> -if (def->controllers[i]->idx == 0) {
> -if 
> (virDeviceInfoPCIAddressPresent(>controllers[i]->info)) {
> -if (def->controllers[i]->info.addr.pci.domain != 0 ||
> -def->controllers[i]->info.addr.pci.bus != 0 ||
> -def->controllers[i]->info.addr.pci.slot != 0x1F ||
> -def->controllers[i]->info.addr.pci.function != 2) {
> +if (cont->idx == 0) {
> +if (virDeviceInfoPCIAddressPresent(>info)) {
> +if (cont->info.addr.pci.domain != 0 ||
> +cont->info.addr.pci.bus != 0 ||
> +cont->info.addr.pci.slot != 0x1F ||
> +cont->info.addr.pci.function != 2) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Primary SATA controller must have 
>PCI address 0:0:1f.2"));
>  goto cleanup;
>  }
>  } else {
> -def->controllers[i]->info.type = 
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> -def->controllers[i]->info.addr.pci.domain = 0;
> -def->controllers[i]->info.addr.pci.bus = 0;
> -def->controllers[i]->info.addr.pci.slot = 0x1F;
> -def->controllers[i]->info.addr.pci.function = 2;
> +cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> +cont->info.addr.pci.domain = 0;
> +cont->info.addr.pci.bus = 0;
> +cont->info.addr.pci.slot = 0x1F;
> +cont->info.addr.pci.function = 2;
>  }
>  }
>  break;
>  
>  case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> -if ((def->controllers[i]->model
> +if ((cont->model
>   == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1) &&

You can now join these two lines...

> -(def->controllers[i]->info.type
> +(cont->info.type
>   == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)) {

... and these two.

ACK with that nit 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 v9 1/5] domain: Add optional 'tls' attribute for TCP chardev

[Daniel P. Berrange]

On Tue, Oct 18, 2016 at 06:59:57AM -0400, John Ferlan wrote:
> 
> 
> On 10/18/2016 02:27 AM, Pavel Hrdina wrote:
> [...]
> 
> >>
> >> "As default behaviour I think it is desirable that we can turn TLS on
> >> for every VM at once - I tend to view it as a host network integration
> >> task, rather than a VM configuration task. Same rationale that we use
> >> for TLS wth VNC/SPICE."
> > 
> > Don't forget this part of the same review:
> > 
> > "There's no reason we can't have a tri-state TLS flag against the chardev
> > in the XML too, to override the default behaviour of cfg->chardevTLS"
> > 
> > That also means to override chardev_tls = "0" by "tls='yes'".
> 
> If the default cfg behaviour is "1", then that tells us "someone" has
> set up the TLS environment and thus the domain/chardev override would be
> "no".
> 
> If the default cfg behaviour is "0", then that means we cannot guarantee
> the environment necessary has been set up and we cannot allow the
> domain/chardev setting to enable TLS.

We have two separate tasks at the host level

 - Setup of TLS certificates (ie put the PEM files in the right places)
 - Global default for use of TLS by chardevs

We only have a config option in qemu.conf for the latter. ie if
chardev_tls=1, this is implicitly saying that TLS certs are deployed
in right place.  IIUC, you're saying that if chardev_tls=0, then we
should interpret that to meant TLS certs are *not* deployed.

Pavel is saying that if chardev_tls=0 in qemu.conf, and tls=1 in the
XML, then we should assume that TLS certs *are* deployed on the host
in the right place. I think this is not unreasonable - we can easily
check to see if the certs exist on disk in this case.

IOW, I agree that we should have a tri-state at the XML level

 - no tls attribute in XML - honour  chardev_tls from qemu.conf
 - tls=1 in XML, then turn on TLS
 - tls=0 in XML, then don't use TLS

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

[libvirt] [PATCH] qemu_hotplug: fix crash in hot(un)plugging chardev devices

[Pavel Hrdina]

We need to make sure that the chardev is serial and TCP.

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_hotplug.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 14af4e1..1003d50 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1729,7 +1729,9 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
 if (qemuDomainChrPreInsert(vmdef, chr) < 0)
 goto cleanup;
 
-if (cfg->chardevTLS) {
+if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
+dev->type == VIR_DOMAIN_CHR_TYPE_TCP &&
+cfg->chardevTLS) {
 if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir,
  dev->data.tcp.listen,
  cfg->chardevTLSx509verify,
@@ -4398,7 +4400,9 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
 
 sa_assert(tmpChr->info.alias);
 
-if (cfg->chardevTLS &&
+if (tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
+tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP &&
+cfg->chardevTLS &&
 !(objAlias = qemuAliasTLSObjFromChardevAlias(tmpChr->info.alias)))
 goto cleanup;
 
-- 
2.10.1

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

Re: [libvirt] [PATCH v9 1/5] domain: Add optional 'tls' attribute for TCP chardev

[Pavel Hrdina]

On Tue, Oct 18, 2016 at 06:59:57AM -0400, John Ferlan wrote:
> 
> 
> On 10/18/2016 02:27 AM, Pavel Hrdina wrote:
> [...]
> 
> >>
> >> "As default behaviour I think it is desirable that we can turn TLS on
> >> for every VM at once - I tend to view it as a host network integration
> >> task, rather than a VM configuration task. Same rationale that we use
> >> for TLS wth VNC/SPICE."
> > 
> > Don't forget this part of the same review:
> > 
> > "There's no reason we can't have a tri-state TLS flag against the chardev
> > in the XML too, to override the default behaviour of cfg->chardevTLS"
> > 
> > That also means to override chardev_tls = "0" by "tls='yes'".
> 
> If the default cfg behaviour is "1", then that tells us "someone" has
> set up the TLS environment and thus the domain/chardev override would be
> "no".
> 
> If the default cfg behaviour is "0", then that means we cannot guarantee
> the environment necessary has been set up and we cannot allow the
> domain/chardev setting to enable TLS. 

Likewise someone can modify only qemu.conf without preparing certificates and
the domain would fail to start.  Libvirt cannot guarantee that the environment
is configured in any case.  So if someone doesn't setup the environment and
uses "tls='yes'" libvirt will print sane error message from qemu that the
certificate files doesn't exist.

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 v9 1/5] domain: Add optional 'tls' attribute for TCP chardev

[John Ferlan]

On 10/18/2016 02:27 AM, Pavel Hrdina wrote:
[...]

>>
>> "As default behaviour I think it is desirable that we can turn TLS on
>> for every VM at once - I tend to view it as a host network integration
>> task, rather than a VM configuration task. Same rationale that we use
>> for TLS wth VNC/SPICE."
> 
> Don't forget this part of the same review:
> 
> "There's no reason we can't have a tri-state TLS flag against the chardev
> in the XML too, to override the default behaviour of cfg->chardevTLS"
> 
> That also means to override chardev_tls = "0" by "tls='yes'".

If the default cfg behaviour is "1", then that tells us "someone" has
set up the TLS environment and thus the domain/chardev override would be
"no".

If the default cfg behaviour is "0", then that means we cannot guarantee
the environment necessary has been set up and we cannot allow the
domain/chardev setting to enable TLS.



John


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

Re: [libvirt] [PATCH v4 01/25] qemu: new functions qemuDomainMachineHasPCI[e]Root()

[Andrea Bolognani]

On Fri, 2016-10-14 at 15:53 -0400, Laine Stump wrote:
> These functions provide a simple one line method of learning if the
> current domain has a pci-root or pcie-root bus.
> ---
> 
> Changes: "reversed polarity" of 2nd if clause as suggested by Andrea.
> 
>  src/qemu/qemu_domain.c | 30 ++
>  src/qemu/qemu_domain.h |  2 ++
>  2 files changed, 32 insertions(+)

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH] cpu_conf: add comments about sockets in cpu_conf

[Chen Hanxiao]

From: Chen Hanxiao 

'sockets' in output of `virsh capabilities' means
the sockets per NUMA node,
which is a special case.

discuss in:
https://www.redhat.com/archives/libvir-list/2012-March/msg01123.html

Signed-off-by: Chen Hanxiao 
---
 src/conf/cpu_conf.h | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h
index e084392..95d6b3e 100644
--- a/src/conf/cpu_conf.h
+++ b/src/conf/cpu_conf.h
@@ -95,15 +95,15 @@ struct _virCPUFeatureDef {
 typedef struct _virCPUDef virCPUDef;
 typedef virCPUDef *virCPUDefPtr;
 struct _virCPUDef {
-int type;   /* enum virCPUType */
-int mode;   /* enum virCPUMode */
-int match;  /* enum virCPUMatch */
+int type;   /* enum virCPUType */
+int mode;   /* enum virCPUMode */
+int match;  /* enum virCPUMatch */
 virArch arch;
 char *model;
-char *vendor_id;/* vendor id returned by CPUID in the guest */
-int fallback;   /* enum virCPUFallback */
+char *vendor_id;/* vendor id returned by CPUID in the guest */
+int fallback;   /* enum virCPUFallback */
 char *vendor;
-unsigned int sockets;
+unsigned int sockets;   /* sockets per NUMA node */
 unsigned int cores;
 unsigned int threads;
 size_t nfeatures;
-- 
1.8.3.1


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

Re: [libvirt] [PATCH 0/3] Misc adjustments from recent code review

[Michal Privoznik]

On 18.10.2016 03:48, John Ferlan wrote:
> The following were all part of the review of the TCP chardev TLS series which
> were outside the realm of the specific changes for the series...
> 
> http://www.redhat.com/archives/libvir-list/2016-October/msg00742.html
> 
> 1. Removal of cfg from qemuProcessPrepareDomain should be separate patch
> 2. Setting chardevTLSx509verify should have been it's own patch...
> 3. !conn in qemuDomainSecretChardevPrepare not necessary - so that's
>true for the SecretDiskPrepare and SecretHostdevPrepare too
> 
> John Ferlan (3):
>   qemu: Remove unnecessary cfg fetch/unref
>   qemu: Add 'verify-peer=yes' test for chardev TCP TLS
>   qemu: Remove unnecessary NULL arg check
> 
>  src/qemu/qemu_domain.c |  5 +--
>  src/qemu/qemu_process.c|  2 --
>  ...xml2argv-serial-tcp-tlsx509-chardev-verify.args | 33 +
>  ...uxml2argv-serial-tcp-tlsx509-chardev-verify.xml | 41 
> ++
>  tests/qemuxml2argvtest.c   |  5 +++
>  5 files changed, 80 insertions(+), 6 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.xml
> 

ACK series

Michal

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

Re: [libvirt] [PATCH v9 1/5] domain: Add optional 'tls' attribute for TCP chardev

[Pavel Hrdina]

On Mon, Oct 17, 2016 at 11:24:58AM -0400, John Ferlan wrote:
> 
> 
> On 10/17/2016 10:37 AM, Pavel Hrdina wrote:
> > On Mon, Oct 17, 2016 at 09:54:46AM -0400, John Ferlan wrote:
> >>
> >>
> >> On 10/17/2016 04:09 AM, Pavel Hrdina wrote:
> >>> On Fri, Oct 14, 2016 at 04:23:04PM -0400, John Ferlan wrote:
>  Add an optional "tls='yes|no'" attribute for a TCP chardev for the
>  express purpose to disable setting up TLS for the specific chardev in
>  the event the qemu.conf settings have enabled hypervisor wide TLS for
>  serial TCP chardevs.
> 
>  Signed-off-by: John Ferlan 
>  ---
>   docs/formatdomain.html.in  | 21 +
>   docs/schemas/domaincommon.rng  |  5 +++
>   src/conf/domain_conf.c | 22 +-
>   src/conf/domain_conf.h |  1 +
>   src/qemu/qemu_command.c|  3 +-
>   src/qemu/qemu_hotplug.c|  3 +-
>   ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +
>   ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 
>  ++
>   .../qemuxml2argv-serial-tcp-tlsx509-chardev.xml|  2 +-
>   tests/qemuxml2argvtest.c   |  3 ++
>   ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml |  1 +
>   .../qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml  |  2 +-
>   tests/qemuxml2xmltest.c|  1 +
>   13 files changed, 139 insertions(+), 5 deletions(-)
>   create mode 100644 
>  tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args
>   create mode 100644 
>  tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml
>   create mode 12 
>  tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml
> 
>  diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>  index 9051178..6145d65 100644
>  --- a/docs/formatdomain.html.in
>  +++ b/docs/formatdomain.html.in
>  @@ -6204,6 +6204,27 @@ qemu-kvm -net nic,model=? /dev/null
> /devices
> ...
>   
>  +
>  +  Since 2.4.0, the optional attribute
>  +  tls can be used to control whether a serial chardev
>  +  TCP communication channel would utilize a hypervisor configured
>  +  TLS X.509 certificate environment in order to encrypt the data
>  +  channel. For the QEMU hypervisor usage of TLS is controlled by the
>  +  chardev_tls setting in file /etc/libvirt/qemu.conf. 
>  If
>  +  enabled, then by setting this attribute to "no" will disable usage
>  +  of the TLS environment for this particular serial TCP data 
>  channel.
>  +
> >>>
> >>> Previous discussion:
> >>>
> >>> http://www.redhat.com/archives/libvir-list/2016-October/msg00734.html
> >>>
> >>> So now there is no functional issue if we agree that this design is what 
> >>> we
> >>> want.  I personally thing that there could be also use-case where you 
> >>> want to
> >>> configure certificates in qemu.conf and use 'tls' attribute to explicitly
> >>> enable TLS encryption only for some VMs and only for some chardev and 
> >>> this is
> >>> not currently possible whit this design.  Now user can enable the TLS 
> >>> encryption
> >>> for every chardev for every domain and explicitly disable for some 
> >>> chardevs.
> >>>
> >>> This would require to add all the extra code from that discussion to 
> >>> handle
> >>> migration properly and that's why I've started the discussion.
> >>>
> >>
> >> w/r/t: design - re-read what I pulled from Dan's v5 review:
> >>
> >> http://www.redhat.com/archives/libvir-list/2016-October/msg00698.html
> >>
> >> While forcing to set "tls='yes'" is certainly a mechanism that could be
> >> used and adding extra code is possible, is that something that we want
> >> to require of everyone that's using TLS chardev's? If you go through the
> >> trouble of configuring for your host, then how would you feel about
> >> having to update all your domains (for those that have more than 1 or 2)
> >> to set the property in order to be able to use it? Again, I really don't
> > 
> > I have a feeling that we are having trouble to understand each other.  I'm 
> > not
> > saying that you will have to set "tls='yes'" for every domain and every 
> > chardev.
> > Forcing to set "tls='no'" for every domain and every chardevs if you want 
> > to use
> > TLS only for one domain is not a good mechanism.
> 
> Clearly ;-)  Setting "tls='yes'" wasn't my implication either. The way
> this patch is written 'YES' or 'ABSENT' have the same meaning to take
> the host default.
> 
> So "forcing" setting of "tls='no'"
> > 
> > What I would like to achieve is this:
> > 
> > 1. Currently there is already existing "chardev_tls" config option that 
> > allows
> > you 

Re: [libvirt] [PATCH 0/3] Misc adjustments from recent code review

[Pavel Hrdina]

On Mon, Oct 17, 2016 at 03:48:53PM -0400, John Ferlan wrote:
> The following were all part of the review of the TCP chardev TLS series which
> were outside the realm of the specific changes for the series...
> 
> http://www.redhat.com/archives/libvir-list/2016-October/msg00742.html
> 
> 1. Removal of cfg from qemuProcessPrepareDomain should be separate patch
> 2. Setting chardevTLSx509verify should have been it's own patch...
> 3. !conn in qemuDomainSecretChardevPrepare not necessary - so that's
>true for the SecretDiskPrepare and SecretHostdevPrepare too

ACK series

Pavel


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