Re: [libvirt] [python v2 PATCH] Add dict check for setTime and allow pass one valid parameter

[lhuang]

Thanks your reply and i have a new idea about when time = None.

On 10/28/2014 01:24 AM, Eric Blake wrote:

On 10/27/2014 12:58 AM, Luyao Huang wrote:

When pass None to time, it will set guest time to 0.
When pass an empty dictionary, it will report error.
Allow a one-element dictionary which contains 'seconds'
or 'nseconds', setting JUST seconds will do the sane
thing of passing 0 for nseconds, instead of erroring out.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  libvirt-override-virDomain.py |  2 ++
  libvirt-override.c| 26 +++---
  2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/libvirt-override-virDomain.py b/libvirt-override-virDomain.py
index a50ec0d..d788657 100644
--- a/libvirt-override-virDomain.py
+++ b/libvirt-override-virDomain.py
@@ -69,6 +69,8 @@
  def setTime(self, time=None, flags=0):
  Set guest time to the given value. @time is a dict conatining

s/conatining/containing/ while you are at it.


  'seconds' field for seconds and 'nseconds' field for nanosecons 

and s/nanosecons/nanoseconds/


+if time is None:
+time = {'nseconds': 0, 'seconds': 0L}

I'd rather that we error out for None instead of silently converting to
0. Using all 0 (aka 1970) is usually the wrong thing.

Likewise, while defaulting nseconds to 0 makes sense, I think we should
require seconds to be present.
Allow None seems work well with VIR_DOMAIN_TIME_SYNC(future for ), when 
flags=libvirt.VIR_DOMAIN_TIME_SYNC,
time will be optional.How about set time={'seconds': 
int(time.time()),'nseconds': 0} when time = None and add some

desc in Doc.If use this way the setTime will change to:
setTime() =   virsh domtime --now
setTime({'seconds':1234567}) = virsh domtime 1234567
setTime(flags=libvirt.VIR_DOMAIN_TIME_SYNC) = virsh domtime --sync




  ret = libvirtmod.virDomainSetTime(self._o, time, flags)
  if ret == -1: raise libvirtError ('virDomainSetTime() failed', 
dom=self)
  return ret
diff --git a/libvirt-override.c b/libvirt-override.c
index a53b46f..5c016f9 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -7798,8 +7798,28 @@ libvirt_virDomainSetTime(PyObject *self 
ATTRIBUTE_UNUSED, PyObject *args) {
  domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
  
  py_dict_size = PyDict_Size(py_dict);

+
+if (py_dict_size == 1) {
+PyObject *pyobj_seconds, *pyobj_nseconds;
+
+if ((pyobj_seconds = PyDict_GetItemString(py_dict, seconds)) 
+(libvirt_longlongUnwrap(pyobj_seconds, seconds)  0)) {
+PyErr_Format(PyExc_LookupError, malformed 'seconds');
+goto cleanup;
+}
  
-if (py_dict_size == 2) {

+if ((pyobj_nseconds = PyDict_GetItemString(py_dict, nseconds)) 
+(libvirt_uintUnwrap(pyobj_nseconds, nseconds)  0)) {
+PyErr_Format(PyExc_LookupError, malformed 'nseconds');
+goto cleanup;
+}
+
+if (!pyobj_nseconds  !pyobj_seconds) {
+PyErr_Format(PyExc_LookupError, Dictionary must contain 
+ 'seconds' or 'nseconds');
+goto cleanup;
+}
+} else if (py_dict_size == 2) {
  PyObject *pyobj_seconds, *pyobj_nseconds;

This feels overly complex.  I think all we really need is:

validate that py_dict is a dict (and not None, per my argument above)
if dict contains seconds:
 populate seconds
else:
 error out
if dict contains nseconds:
 populate nseconds
else:
 nseconds = 0
if dict contains anything else:
 error out

Cool!I will use this way to check the seconds and nseconds.


  
  if (!(pyobj_seconds = PyDict_GetItemString(py_dict, seconds)) ||

@@ -7813,9 +7833,9 @@ libvirt_virDomainSetTime(PyObject *self ATTRIBUTE_UNUSED, 
PyObject *args) {
  PyErr_Format(PyExc_LookupError, malformed or missing 
'nseconds');
  goto cleanup;
  }
-} else if (py_dict_size  0) {
+} else if (py_dict_size = 0) {
  PyErr_Format(PyExc_LookupError, Dictionary must contain 
- 'seconds' and 'nseconds');
+ 'seconds' or 'nseconds');

The error message needs touching up if nseconds is going to be optional.

Yes,how about change the error to

Dictionary must contain 'seconds'





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

Re: [libvirt] [PATCH] Fix memory leak in cmdNetworkDHCPLeases

[lhuang]

On 10/30/2014 02:27 PM, Peter Krempa wrote:

On 10/30/14 03:35, Luyao Huang wrote:

After use cidr_format in function virAsprintf and vshPrintExtra, need free
cidr_format.

Fix the following memory leak from valgrind, like:
  18 bytes in 1 blocks are definitely lost in loss record 41 of 192
 at 0x4C29BBD: malloc (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
 by 0x85CE36F: __vasprintf_chk (vasprintf_chk.c:80)
 by 0x4EE52D5: UnknownInlinedFun (stdio2.h:210)
 by 0x4EE52D5: virVasprintfInternal (virstring.c:459)
 by 0x4EE53CA: virAsprintfInternal (virstring.c:480)
 by 0x14FE96: cmdNetworkDHCPLeases (virsh-network.c:1378)
 by 0x13006B: vshCommandRun (virsh.c:1915)
 by 0x12A9E1: main (virsh.c:3699)

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  tools/virsh-network.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/tools/virsh-network.c b/tools/virsh-network.c
index 90392d3..8ff6fd8 100644
--- a/tools/virsh-network.c
+++ b/tools/virsh-network.c
@@ -1381,6 +1381,8 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd)
expirytime, EMPTYSTR(lease-mac),
EMPTYSTR(typestr), cidr_format,
EMPTYSTR(lease-hostname), EMPTYSTR(lease-clientid));
+
+   VIR_FREE(cidr_format);

I didn't notice at first, but you've used a TAB to indent VIR_FREE. Our
coding style enforces use of spaces instead. Please run 'make
syntax-check before sending patches.

I'll fix the problem before pushing.

Oh sorry,i forgot it, thanks your advise :)

  }
  
  ret = true;



Peter



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

Re: [libvirt] [PATCH] Fix a mismatch attribute name

[lhuang]

On 11/07/2014 05:15 PM, Eric Blake wrote:

On 11/07/2014 09:24 AM, Luyao Huang wrote:

From libvirt.org we know this attribute named:

interface_mac   MAC address of the network interface, not unique

Shouldn't we instead be fixing the docs to match the code?  Changing the
code now may break existing implementations that have used the name in
the current code.
yes, i agree with you, i give a v2 patch, but i don't know how to change 
the doc in http://libvirt.org.


https://www.redhat.com/archives/libvir-list/2014-November/msg00209.html

And this patch is for the bug:

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

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/access/viraccessdriverpolkit.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/access/viraccessdriverpolkit.c 
b/src/access/viraccessdriverpolkit.c
index 3136be7..0e07053 100644
--- a/src/access/viraccessdriverpolkit.c
+++ b/src/access/viraccessdriverpolkit.c
@@ -206,7 +206,7 @@ virAccessDriverPolkitCheckInterface(virAccessManagerPtr 
manager,
  const char *attrs[] = {
  connect_driver, driverName,
  interface_name, iface-name,
-interface_macaddr, iface-mac,
+interface_mac, iface-mac,
  NULL,
  };
  



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

Re: [libvirt] [PATCH] Fix a mismatch attribute name

[lhuang]

On 11/07/2014 05:45 PM, Eric Blake wrote:

On 11/07/2014 10:37 AM, lhuang wrote:

On 11/07/2014 05:15 PM, Eric Blake wrote:

On 11/07/2014 09:24 AM, Luyao Huang wrote:

From libvirt.org we know this attribute named:

interface_macMAC address of the network interface, not unique

Shouldn't we instead be fixing the docs to match the code?  Changing the
code now may break existing implementations that have used the name in
the current code.

yes, i agree with you, i give a v2 patch, but i don't know how to change
the doc in http://libvirt.org.

libvirt.org autogenerates from the latest libvirt.git, at least once an
hour.



https://www.redhat.com/archives/libvir-list/2014-November/msg00209.html

So once that message is committed, the web page will auto-update.

Oh, I see, thanks

And this patch is for the bug:

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

Then mention that in the commit message :)


Okay and thanks for push the patch :)

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

Re: [libvirt] [v2 PATCH] doc: Fix a mismatch attribute name

[lhuang]

On 11/07/2014 05:53 PM, Eric Blake wrote:

On 11/07/2014 10:47 AM, Eric Blake wrote:

On 11/07/2014 10:35 AM, Luyao Huang wrote:

From virAccessDriverPolkitCheckInterface function, we know this
attribute should named: interface_macaddr

Eww. 'git am' strips an initial commit line beginning with From , if
it doesn't match an email address (this is because it special cases a
leading 'From: ...' or 'From: ...' line as overriding commit authorship
instead of using the email's sender as the author).  I didn't realize
that I lost a line of your commit message until after I had pushed my
modifications.

In the future, try to avoid using From as the first word of a commit
message.  Based on the could serve as an appropriate substitute phrase
for this case.

So interesting :)
i didn't notice this before.I will change my word
next time, thanks your advice.




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

Re: [libvirt] [PATCH] security:selinux: Fix crash when tcon is NULL

[lhuang]

On 11/10/2014 03:57 PM, Martin Kletzander wrote:

On Sat, Nov 08, 2014 at 06:17:26PM +0800, Luyao Huang wrote:

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

Libvirtd will crash when parameter tcon = NULL in 
virSecuritySELinuxSetFileconHelper
function, because libvirt do not check the first parameter when use 
strcmp().
Add a check for tcon before use strcmp() and output a error in log 
when tcon is NULL.


Signed-off-by: Luyao Huang lhu...@redhat.com
---
src/security/security_selinux.c | 5 +
1 file changed, 5 insertions(+)



NACK, calling virSecuritySELinuxSetFileconHelper() with NULL doesn't
make sense at all.  If you are trying to just fix a possible future
errot, then the check for non-NULL parameters needs to be done when
the function is starting.  But I doubt it's useful.  It's another
story if you managed to cause such call to this function.

Thanks your reply and It's a real issue :) what i do :
# /usr/libexec/qemu-kvm -cdrom /tmp/test.iso  -monitor 
unix:/tmp/demo,server,nowait  -name foo -uuid 
cece4f9f-dff0-575d-0e8e-01fe380f12ea  

[1] 16636

# VNC server running on `::1:5900'

# virsh qemu-attach 16636
Domain foo attached to pid 16636

# virsh screenshot foo
error: could not take a screenshot of foo
error: Cannot recv data: Connection reset by peer
error: Failed to reconnect to the hypervisor


Maybe root cause is : vm doesn't have a image label
  seclabel type='static' model='selinux' relabel='yes'
labelsystem_u:system_r:virtd_t:s0-s0:c0.c1023/label
  /seclabel




diff --git a/src/security/security_selinux.c 
b/src/security/security_selinux.c

index f96be50..4fd09b8 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -887,6 +887,11 @@ virSecuritySELinuxSetFileconHelper(const char 
*path, char *tcon, bool optional)

int setfilecon_errno = errno;

if (getfilecon_raw(path, econ) = 0) {
+if (tcon == NULL) {
+virReportSystemError(errno,%s,
+ _(Invalid security context : NULL));
+return -1;
+}


Explaining how did you reach this point with tcon == NULL, or if you
even managed to do that, would help others understanding the issue
from higher level.  Anyway, the problem here is that this function is
called with tcon == NULL and not that it doesn't check for that (if
you managed to see the segfault).  To explain what I mean; if you
managed to see the segfault when you performed some operation, that's
bad.  But when this patch is applied, you still won't be able to
perform that operation successfully and we need to find out the root
cause of that.

The backtrace would fit nicely in the commit message as well.

Martin


Backtrace is here:

(gdb) bt
#0  0x7ff38a0c9e76 in __strcmp_sse42 () from /lib64/libc.so.6
#1  0x7ff38d08d6a9 in virSecuritySELinuxSetFileconHelper 
(path=0x7ff36c2626c0 /var/cache/libvirt/qemu/qemu.screendump.xRL8fw, 
tcon=0x0, optional=optimized out)

at security/security_selinux.c:890
#2  0x7ff38d089c63 in virSecurityManagerSetSavedStateLabel 
(mgr=0x7ff36c0f6f40, vm=vm@entry=0x7ff3680011c0,
savefile=savefile@entry=0x7ff36c2626c0 
/var/cache/libvirt/qemu/qemu.screendump.xRL8fw) at 
security/security_manager.c:547
#3  0x7ff38d086bc6 in virSecurityStackSetSavedStateLabel 
(mgr=optimized out, vm=0x7ff3680011c0, savefile=0x7ff36c2626c0 
/var/cache/libvirt/qemu/qemu.screendump.xRL8fw)

at security/security_stack.c:351
#4  0x7ff38d089c63 in virSecurityManagerSetSavedStateLabel 
(mgr=0x7ff36c106fc0, vm=0x7ff3680011c0, savefile=0x7ff36c2626c0 
/var/cache/libvirt/qemu/qemu.screendump.xRL8fw)

at security/security_manager.c:547
#5  0x7ff375e7d8b4 in qemuDomainScreenshot (dom=0x7ff36c000910, 
st=0x7ff36c262010, screen=optimized out, flags=optimized out) at 
qemu/qemu_driver.c:3854
#6  0x7ff38cfa5d60 in virDomainScreenshot 
(domain=domain@entry=0x7ff36c000910, stream=stream@entry=0x7ff36c262010, 
screen=0, flags=0) at libvirt.c:3171
#7  0x7ff38da489d3 in remoteDispatchDomainScreenshot 
(server=optimized out, ret=0x7ff36c000a20, args=0x7ff36c23ef80, 
rerr=0x7ff37d6cdc80, msg=optimized out,

client=0x7ff38fb404e0) at remote_dispatch.h:7473
#8  remoteDispatchDomainScreenshotHelper (server=optimized out, 
client=0x7ff38fb404e0, msg=optimized out, rerr=0x7ff37d6cdc80, 
args=0x7ff36c23ef80, ret=0x7ff36c000a20)

at remote_dispatch.h:7440
#9  0x7ff38d019752 in virNetServerProgramDispatchCall 
(msg=0x7ff38fb40470, client=0x7ff38fb404e0, server=0x7ff38fb3f460, 
prog=0x7ff38fb4aea0) at rpc/virnetserverprogram.c:437
#10 virNetServerProgramDispatch (prog=0x7ff38fb4aea0, 
server=server@entry=0x7ff38fb3f460, client=0x7ff38fb404e0, 
msg=0x7ff38fb40470) at rpc/virnetserverprogram.c:307
#11 0x7ff38da6820d in virNetServerProcessMsg (msg=optimized out, 
prog=optimized out, client=optimized out, srv=0x7ff38fb3f460) at 
rpc/virnetserver.c:172
#12 virNetServerHandleJob (jobOpaque=optimized out, 

Re: [libvirt] [PATCH] conf: Fix crash when src-hosts = NULL in virStorageFileBackendGlusterInit

[lhuang]

On 11/12/2014 04:50 PM, Peter Krempa wrote:

On 11/12/14 09:47, Luyao Huang wrote:

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

When do external snapshot for a gluster disk with no host name(ip) in
snapshot xml, libvirtd will crash. Because when node do not have a children
in virDomainStorageHostParse, libvirt will return 0, but donnot get hosts for
virStorageFileBackendGlusterInit.

snpahost.xml:

domainsnapshot
namesnapshot_test/name
descriptionSnapshot Test/description
disks
disk name='vda' snapshot='external' type='network'
source protocol='gluster' name='gluster-vol1/gluster.img.snap'/
/disk
/disks
/domainsnapshot

Back trace:
virsh snapshot-create r6 snapshot.xml --disk-only

0  virStorageFileBackendGlusterInit (src=0x7fc760007ca0) at 
storage/storage_backend_gluster.c:577
1  0x7fc76d678e22 in virStorageFileInitAs (src=0x7fc760007ca0, 
uid=uid@entry=4294967295, gid=gid@entry=4294967295) at 
storage/storage_driver.c:2547
2  0x7fc76d678e9c in virStorageFileInit (src=optimized out) at 
storage/storage_driver.c:2567
3  0x7fc76bc13f9c in qemuDomainSnapshotPrepareDiskExternal (reuse=false, 
active=true, snapdisk=0x7fc7600019b8, disk=0x7fc7641e4880, conn=0x7fc76426cc10)
 at qemu/qemu_driver.c:12995
4  qemuDomainSnapshotPrepare (flags=synthetic pointer, def=0x7fc760002570, 
vm=0x7fc76422b530, conn=0x7fc76426cc10) at qemu/qemu_driver.c:13156
5  qemuDomainSnapshotCreateXML (domain=0x7fc760001f30, xmlDesc=optimized out, 
flags=16) at qemu/qemu_driver.c:13896
6  0x7fc782d4de4d in virDomainSnapshotCreateXML 
(domain=domain@entry=0x7fc760001f30,
 xmlDesc=0x7fc760001b80 domainsnapshot\nnamesnapshot_test/name\ndescriptionSnapshot 
Test/description\ndisks\ndisk name='vda' snapshot='external' type='network'\nsource protocol='gluster' 
name='gluster-vol1/gluster, flags=16) at libvirt.c:18488
7  0x7fc7837cb44c in remoteDispatchDomainSnapshotCreateXML (server=optimized 
out, msg=optimized out, ret=0x7fc76a60, args=0x7fc760001f90, 
rerr=0x7fc77344dc80,

 client=optimized out) at remote_dispatch.h:8605

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/domain_conf.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2c65276..34c1c12 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4150,7 +4150,12 @@ virDomainStorageHostParse(xmlNodePtr node,
  
  memset(host, 0, sizeof(host));
  
-child = node-children;

+if ((child = node-children) == NULL) {
+virReportError(VIR_ERR_XML_ERROR, %s,
+   _(Can not find a host in xml));
+goto cleanup;
+}
+
  while (child != NULL) {
  if (child-type == XML_ELEMENT_NODE 
  xmlStrEqual(child-name, BAD_CAST host)) {


NACK,

some protocols may use implicit host names. We want to check it for
gluster only in this case.

Also Jan already sent a patch for this so there's no need to resend this.

Peter

okay, i didn't see my e-mail box before i send this :)

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

Re: [libvirt] [PATCH] qemu: output error when try to hotplug unsupport console

[lhuang]

On 11/26/2014 12:17 AM, Pavel Hrdina wrote:

On 11/17/2014 03:31 PM, Luyao Huang wrote:

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

When try to use attach-device to hotplug a qemu
unsupport sonsole, command will return success,
and add a console in vm's xml.But this doesn't work
in qemu, qemu doesn't support these console.Add
a error output when try to hotplug a unsupport console
in qemuBuildConsoleChrDeviceStr.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_command.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1399ce4..2bf4a83 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10069,13 +10069,17 @@ qemuBuildConsoleChrDeviceStr(char **deviceStr,
  break;

  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL:
+break;
  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE:
  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN:
  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_UML:
  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC:
  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_OPENVZ:
  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST:
-break;
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+  _(unsupported console target type %s),
+ NULLSTR(virDomainChrConsoleTargetTypeToString(chr-targetType)));


Wrong indentation

Thanks for pointing out



+return ret;
  }

  ret = 0;



Otherwise it seems good, bud what about the case that you will attach
unsupported device to offline domain? It is still possible but the
domain cannot be started afterwards. You should probably take care of
that too.


Eww, i thought that before, and i have a question about attach unsupported
device to a offline domain.

If we forbid attaching a unsupported device, is it means we should also 
forbid
editing or defining a guest with a unsupported device?(because domain 
cannot
start with these settings). I thinks it will be a big work. Actually, I 
just don't
know when we try to set a invalid things to a offline domain, which way 
we should

forbid and which way we can allow it.

Thanks advance for your answer.

Pavel

Thanks,
Luyao Huang

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

Re: [libvirt] [PATCH] qemu: output error when try to hotplug unsupport console

[lhuang]

On 12/02/2014 11:59 PM, Ján Tomko wrote:

On 11/26/2014 11:02 AM, lhuang wrote:

On 11/26/2014 12:17 AM, Pavel Hrdina wrote:

On 11/17/2014 03:31 PM, Luyao Huang wrote:

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

When try to use attach-device to hotplug a qemu
unsupport sonsole, command will return success,
and add a console in vm's xml.But this doesn't work
in qemu, qemu doesn't support these console.Add
a error output when try to hotplug a unsupport console
in qemuBuildConsoleChrDeviceStr.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
   src/qemu/qemu_command.c | 6 +-
   1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1399ce4..2bf4a83 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10069,13 +10069,17 @@ qemuBuildConsoleChrDeviceStr(char **deviceStr,
   break;

   case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL:
+break;
   case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE:
   case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN:
   case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_UML:
   case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC:
   case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_OPENVZ:
   case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST:
-break;
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+  _(unsupported console target type %s),
+ NULLSTR(virDomainChrConsoleTargetTypeToString(chr-targetType)));

Wrong indentation

Thanks for pointing out

+return ret;
   }

   ret = 0;


Otherwise it seems good, bud what about the case that you will attach
unsupported device to offline domain? It is still possible but the
domain cannot be started afterwards. You should probably take care of
that too.


Eww, i thought that before, and i have a question about attach unsupported
device to a offline domain.

If we forbid attaching a unsupported device, is it means we should also forbid
editing or defining a guest with a unsupported device?(because domain cannot
start with these settings). I thinks it will be a big work. Actually, I just
don't
know when we try to set a invalid things to a offline domain, which way we 
should
forbid and which way we can allow it.


If we allowed defining a guest with an unsupported device before, I think we
should only report the error when the user tries to start it.

Otherwise it would disappear from the domain list on libvirt update and the
user can't fix the problem via libvirt.
Got it and thanks a lot for your reply. I will write a v2 for forbid 
cold-plug a unsupported device.

Jan



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

Re: [libvirt] [PATCHv2 2/2] security: Add a new func use stat to get process DAC label

[lhuang]

On 12/05/2014 12:58 AM, Ján Tomko wrote:

On 12/03/2014 11:08 AM, Luyao Huang wrote:

When use qemuProcessAttach to attach a qemu process, cannot
get a right DAC label. Add a new func to get process label
via stat func. Do not remove virDomainDefGetSecurityLabelDef
before try to use stat to get process DAC label, because
There are some other func call virSecurityDACGetProcessLabel.

v2 add support freeBSD.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/security/security_dac.c | 95 -
  1 file changed, 93 insertions(+), 2 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 85253af..89cafa3 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -23,6 +23,11 @@
  #include sys/stat.h
  #include fcntl.h
  
+#ifdef  __FreeBSD__

+# include sys/sysctl.h
+# include sys/user.h
+#endif
+
  #include security_dac.h
  #include virerror.h
  #include virfile.h
@@ -1236,18 +1241,104 @@ virSecurityDACReserveLabel(virSecurityManagerPtr mgr 
ATTRIBUTE_UNUSED,
  return 0;
  }
  
+#ifdef __linux__

+static int
+virSecurityDACGetProcessLabelInternal(pid_t pid,
+  virSecurityLabelPtr seclabel)
+{
+struct stat sb;
+char *path = NULL;
+char *label = NULL;
+int ret = -1;
+
+VIR_INFO(Getting DAC user and group on process '%d', pid);
+
+if (virAsprintf(path, /proc/%d, (int) pid)  0)
+goto cleanup;
+
+if (lstat(path, sb)  0)
+goto cleanup;

A more specific error should be reported here via virReportSystemError.

Thanks, i will move error settings in this func.

+
+if (virAsprintf(label, +%u:+%u,
+(unsigned int) sb.st_uid,
+(unsigned int) sb.st_gid)  0)
+goto cleanup;
+
+if (virStrcpy(seclabel-label, label,VIR_SECURITY_LABEL_BUFLEN) == NULL)
+goto cleanup;

Using snprintf would simplify this code.

Oh, nice, i forgot this function, and thanks your pointing out.

+ret = 0;
+
+cleanup:
+VIR_FREE(path);
+VIR_FREE(label);
+return ret;
+}
+#elif defined(__FreeBSD__)
+static int
+virSecurityDACGetProcessLabelInternal(pid_t pid,
+  virSecurityLabelPtr seclabel)
+{
+struct kinfo_proc p;
+int mib[4];
+size_t len = 4;
+char *label = NULL;
+int ret = -1;
+
+sysctlnametomib(kern.proc.pid, mib, len);
+
+len = sizeof(struct kinfo_proc);
+mib[3] = pid;
+
+if (sysctl(mib, 4, p, len, NULL, 0)  0)
+goto cleanup;
+

sysctlbyname would remove the need for a separate nametomib call and get rid
of a few variables.
Actually, i tried to use this func (sysctlbyname) before, but i found i 
cannot make it

work well here...

If you have some good way to use it, please tell me, thanks a lot for 
your answer:)



+if (virAsprintf(label, +%u:+%u,
+(unsigned int) p.ki_ruid,
+(unsigned int) p.ki_rgid)  0)
+goto cleanup;
+
+if (virStrcpy(seclabel-label, label,VIR_SECURITY_LABEL_BUFLEN) == NULL)
+goto cleanup;

Same comment as above.

Thanks

+ret = 0;
+
+cleanup:
+VIR_FREE(label);
+return ret;
+}
+#else
+static int
+virSecurityDACGetProcessLabelInternal(pid_t pid,
+  virSecurityLabelPtr seclabel)
+{
+return -1;
+}
+#endif
+
  static int
  virSecurityDACGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr def,
-  pid_t pid ATTRIBUTE_UNUSED,
+  pid_t pid,
virSecurityLabelPtr seclabel)
  {
  virSecurityLabelDefPtr secdef =
  virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
  
-if (!secdef || !seclabel)

+if (!seclabel)
  return -1;

The SELinux version of this function does not check if seclabel is non-NULL, I
think we can safely remove the check here as well.

Okay
  
+if (secdef == NULL) {

+VIR_DEBUG(missing label for DAC security 
+  driver in domain %s, def-name);
+
+if (virSecurityDACGetProcessLabelInternal(pid, seclabel)  0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _(Cannot get process %d DAC label),pid);

This would overwrite the more-specific error message from the
virSecurityDACGetProcessLabelInternal function.

Yes, got it, thanks for pointing out.

+return -1;
+}
+
+return 0;
+}
+
  if (secdef-label)
  ignore_value(virStrcpy(seclabel-label, secdef-label,
 VIR_SECURITY_LABEL_BUFLEN));


Jan




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

Re: [libvirt] [PATCHv4 2/2] security: Add a new func use stat to get process DAC label

[lhuang]

On 12/11/2014 05:45 PM, Ján Tomko wrote:

On 12/09/2014 09:33 AM, Luyao Huang wrote:

When use qemuProcessAttach to attach a qemu process, cannot
get a right DAC label. Add a new func to get process label
via stat func. Do not remove virDomainDefGetSecurityLabelDef
before try to use stat to get process DAC label, because
There are some other func call virSecurityDACGetProcessLabel.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
v2 add support freeBSD.
v3 use snprintf instead of VirAsprintf and move the error
settings in virSecurityDACGetProcessLabelInternal.
v4 remove errno.h include and thanks Eric advice move this
version comment to this place.

  src/security/security_dac.c | 84 +++--
  1 file changed, 81 insertions(+), 3 deletions(-)


ACK and pushed. I reworded the commit message and included the bugzilla link
and a reference to the other part of the fix that has already been pushed.

Thanks for your help :)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 85253af..300b245 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -23,6 +23,11 @@
  #include sys/stat.h
  #include fcntl.h
  
+#ifdef  __FreeBSD__

+# include sys/sysctl.h
+# include sys/user.h
+#endif
+
  #include security_dac.h
  #include virerror.h
  #include virfile.h
@@ -1236,17 +1241,90 @@ virSecurityDACReserveLabel(virSecurityManagerPtr mgr 
ATTRIBUTE_UNUSED,
  return 0;
  }
  
+#ifdef __linux__

+static int
+virSecurityDACGetProcessLabelInternal(pid_t pid,
+  virSecurityLabelPtr seclabel)
+{
+struct stat sb;
+char *path = NULL;
+int ret = -1;
+
+VIR_INFO(Getting DAC user and group on process '%d', pid);
+

VIR_INFO is too noisy for this, I've changed it to VIR_DEBUG.

+if (virAsprintf(path, /proc/%d, (int) pid)  0)
+goto cleanup;
+
+if (lstat(path, sb)  0) {
+virReportSystemError(errno,
+ _(unable to get PID %d uid and gid via stat),
+ pid);
+goto cleanup;
+}
+
+snprintf(seclabel-label, VIR_SECURITY_LABEL_BUFLEN,
+ +%u:+%u, (unsigned int) sb.st_uid, (unsigned int) sb.st_gid);
+ret = 0;
+
+cleanup:
+VIR_FREE(path);
+return ret;
+}
+#elif defined(__FreeBSD__)
+static int
+virSecurityDACGetProcessLabelInternal(pid_t pid,
+  virSecurityLabelPtr seclabel)
+{
+struct kinfo_proc p;
+int mib[4];
+size_t len = 4;
+
+sysctlnametomib(kern.proc.pid, mib, len);
+
+len = sizeof(struct kinfo_proc);
+mib[3] = pid;
+
+if (sysctl(mib, 4, p, len, NULL, 0)  0) {
+virReportSystemError(errno,
+ _(unable to get PID %d uid and gid via sysctl),
+ pid);
+return -1;
+}
+
+snprintf(seclabel-label, VIR_SECURITY_LABEL_BUFLEN,
+ +%u:+%u, (unsigned int) p.ki_ruid, (unsigned int) p.ki_rgid);

This gets the real uid/gid, we want the effective ones like on Linux.

Yes, this should use effective uid/gid after i check the doc.
Thanks for pointing out

+
+return 0;
+}
+#else
+static int
+virSecurityDACGetProcessLabelInternal(pid_t pid,
+  virSecurityLabelPtr seclabel)
+{
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   Cannot get proccess DAC label for pid %d on this platform,
+   (int) pid);

This fails syntax check - the string needs to be marked for translation with
_(). I've changed it to virReportSystemError(ENOSYS, ..), to match the use in
other places.

Oh, it's my fault, thanks.

Jan


Luyao

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

Re: [libvirt] [PATCH] conf: fix cannot start a guest have a shareable network iscsi hostdev

[lhuang]

On 12/16/2014 11:46 PM, Michal Privoznik wrote:

On 16.12.2014 04:16, Luyao Huang wrote:

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

We should do nothing for the shareable network iscsi hostdev in
qemuAddSharedHostdev and qemuRemoveSharedHostdev. Shareable for
a network iscsi hostdev is not valid, so just ignore it.


If it is invalid, can't we just forbid it in the parsing phase?


Thanks for your review.

Maybe 'invalid' this words is not correct here, after i check the code 
in qemuRemoveSharedHostdev

there are some words in there:

Currently the only conflicts we have to care about for the shared disk 
and shared host device is sgio setting, which is only valid for block 
disk and scsi host device.


I think this means we should do nothing for the network iscsi hostdev 
(just like what we do for the network disk, usb hostdev...). And i don't 
think it means the shareable/ is invalid for these disk, because they 
are already 'shareable' in guests even libvirt do nothing.


But i cannot make sure i am right :)

Michal

Luyao

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

Re: [libvirt] [PATCHv2] lxc: fix show the wrong xml when guest start failed

[lhuang]

On 02/03/2015 11:01 PM, John Ferlan wrote:


On 01/12/2015 09:33 AM, Luyao Huang wrote:

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

When guest start failed, libvirt will keep the current vm-def,
this will make a issue that we cannot get a right xml after guest
start failed. And don't call the stop/release hook to do some
other clean work.

Call virLXCProcessCleanup to help us clean the source and call
the hooks if start a vm failed.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
v2: use virLXCProcessCleanup to free the source and call the hook.

  src/lxc/lxc_process.c | 69 ++-
  1 file changed, 29 insertions(+), 40 deletions(-)


There's been some changes to virLXCProcessStart since this was written -
a revisit/rework is in order.


Yes, thanks a lot for your remind :)

John

Luyao

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

Re: [libvirt] [PATCH] conf: fix forget goto error when report a unsupport error

[lhuang]

On 02/04/2015 03:29 PM, Peter Krempa wrote:

On Wed, Feb 04, 2015 at 10:33:29 +0800, Luyao Huang wrote:

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

This will cause a issue that add a unsupport input device
to a hypervisor which unsupport it, also will cause a wrong
error message when the input device is not a mouse or keyboard.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/domain_conf.c | 1 +
  1 file changed, 1 insertion(+)

ACK. Pushed with fixed commit message.


Thanks a lot for review and message fixed.

Also thanks for review another patch just like this :)

Peter


Luyao

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

Re: [libvirt] [PATCH] qemu: Properly report error when cookin uuid not match the ctxt uuid

[lhuang]

On 02/05/2015 03:14 PM, Ján Tomko wrote:

On Thu, Feb 05, 2015 at 11:42:26AM +0800, Luyao Huang wrote:

Add the missing jump to the error label when cookin uuid
does not match the ctxt uuid.


'cookie uuid' and 'ctxt uuid' don't seem clear enough to me.
I have reworded the commit message:

Add the missing jump to the error label when the uuid in the
migration cookie XML does not match the uuid of the migrated
domain.


Okay, thanks for rewording the message.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_migration.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 8a8fa63..879b1bf 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1147,6 +1147,7 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig,
  virReportError(VIR_ERR_INTERNAL_ERROR,
 _(Incoming cookie data had unexpected UUID %s vs %s),
 tmp, uuidstr);
+goto error;
  }
  VIR_FREE(tmp);
  

ACK and pushed.


Thanks.

Jan


Luyao

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

Re: [libvirt] [PATCH 2/2] qemu: add a check when cold-plug a Chr device

[lhuang]

On 01/22/2015 07:16 AM, John Ferlan wrote:


On 12/09/2014 01:48 AM, Luyao Huang wrote:

Add a func just check the base target type which
qemu support. But i still doubt this will be useful
, we already have a check when we try to start the
vm. And this check only check the target type, and
the other things will be done in virDomainChrDefParseXML.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_hotplug.c | 64 +
  1 file changed, 64 insertions(+)


I forgot to add that this didn't build without:

--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -145,6 +145,8 @@ virDomainBlockedReasonTypeFromString;
  virDomainBlockedReasonTypeToString;
  virDomainCapabilitiesPolicyTypeToString;
  virDomainCapsFeatureTypeToString;
+virDomainChrChannelTargetTypeFromString;
+virDomainChrChannelTargetTypeToString;
  virDomainChrConsoleTargetTypeFromString;
  virDomainChrConsoleTargetTypeToString;
  virDomainChrDefForeach;


Since it seems you'll be resending your patches, I'll wait for your
patches before proceeding1...


Thanks a lot for pointing out, I will add these code in next version.

John

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b9a0cee..fe91ec7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1384,10 +1384,74 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr 
driver,
  
  }
  
+static int

+qemuDomainChrCheckDefSupport(virDomainChrDefPtr chr)
+{
+int ret = -1;
+
+switch (chr-deviceType) {
+case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
+switch ((virDomainChrSerialTargetType) chr-targetType) {
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
+ret = 0;
+break;
+
+default:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _(unsupported serial target type %s for qemu),
+   
NULLSTR(virDomainChrSerialTargetTypeToString(chr-targetType)));
+break;
+}
+break;
+
+case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
+ret = 0;
+break;
+
+case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL:
+switch ((virDomainChrChannelTargetType) chr-targetType) {
+case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD:
+case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO:
+ret = 0;
+break;
+
+default:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _(unsupported channel target type %s for qemu),
+   
NULLSTR(virDomainChrChannelTargetTypeToString(chr-targetType)));
+break;
+}
+break;
+
+case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
+switch ((virDomainChrConsoleTargetType) chr-targetType) {
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL:
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO:
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLP:
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLPLM:
+ret = 0;
+break;
+
+default:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _(unsupported console target type %s for qemu),
+   
NULLSTR(virDomainChrConsoleTargetTypeToString(chr-targetType)));
+break;
+}
+break;
+}
+
+return ret;
+}
+
  int
  qemuDomainChrInsert(virDomainDefPtr vmdef,
  virDomainChrDefPtr chr)
  {
+if (qemuDomainChrCheckDefSupport(chr)  0)
+return -1;
+
  if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
  chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) {
  virReportError(VIR_ERR_OPERATION_UNSUPPORTED, %s,



Luyao

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

Re: [libvirt] [PATCH v3 2/2] qemu: add a target type check when hot/cold-plug a Chr device

[lhuang]

On 01/22/2015 03:37 PM, Peter Krempa wrote:

On Thu, Jan 22, 2015 at 10:28:19 +0800, Luyao Huang wrote:

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

Add a func just check the base target type which
qemu support. And this check will help to avoid add a qemu
unsupport target type chr device to a running guest(hotplug)
or to the guest inactive XML (coldplug, will effect next boot)

You can get exactly the same behavior adding the device by specifying a
new XML containing the device (via virsh edit for example).
Unfortunately the unsupported devices can't be checked in the post parse
callback as it would make existing VMs with broken config disappear.
Additionally even if you forbid known-bad targets you still even with
this code can get a failure by adding a SCLP console on a x86 machine:

 console type='pty'
   target type='sclp' port='0'/
 /console

Starting such VM gets you:
error: unsupported configuration: sclp console requires QEMU to support
s390-sclp

This kind of failure depends on the actual qemu process running the VM
and can be checked only when the VM is starting.


Yes, this check cannot avoid attach a unsupported chr device in all 
scene (too old qemu or a chr device not support for x86_64), because we 
cannot do a check to qemu caps in this place. As you said we should 
check if actual qemu support it when start the vm.

And this check only check the target type, and other things
have been checked in virDomainChrDefParseXML.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/libvirt_private.syms |  2 ++
  src/qemu/qemu_hotplug.c  | 67 
  2 files changed, 69 insertions(+)


Said this. I don't think it's worth adding the check to the coldplug
path.


Eww, I thought about this before and i think maybe we can have more 
check to the Chr device when we do coldplug than we define it (add new 
check in parse will make guest which have broken settings disappear when 
update libvirt), although this check cannot cover all the sence.


And thanks a lot for your opinion and review!

Peter


Luyao

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

Re: [libvirt] [PATCH] qemu: fix fail to start vm with one cell use memdev and another not

[lhuang]

On 01/16/2015 04:54 PM, Michal Privoznik wrote:

On 16.01.2015 03:12, Luyao Huang wrote:

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

We changed our build numa qemu cmd line way in f309db,
But qemu cannot accept one cell use memdev and another not,
so this cause a issue when we start a vm with one cell
have hugpage or nodemask settings and others not.

After this patch we will use memdev for all cells if there
is at least one cell need this.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_command.c| 88 +-
  .../qemuxml2argv-numatune-memnode-no-memory.args   |  3 +-
  2 files changed, 55 insertions(+), 36 deletions(-)

While I agree the bug you are trying to fix is real bug, the patch look
a bit messy. Moreover, as far as I know Peter is doing work in this area
and he's willing to fix the bug. You know, he's rewriting the area from
scratch (or something like that), so I don't really want to cause a
rebase conflict to him. Moreover, some tests fail after this patch, so I
will not merge this for now, sorry.


Nothing, I just don't know Peter is working on it.
About patch look a bit messy, i noticed this when i wrote, but i cannot 
found a good way to fix it without rewrite this place.


And thanks a lot for your reply!

Michal


Luyao

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

Re: [libvirt] [PATCH] qemu: fix fail to start vm with one cell use memdev and another not

[lhuang]

On 01/16/2015 04:48 PM, Peter Krempa wrote:

On Fri, Jan 16, 2015 at 10:12:49 +0800, Luyao Huang wrote:

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

We changed our build numa qemu cmd line way in f309db,
But qemu cannot accept one cell use memdev and another not,
so this cause a issue when we start a vm with one cell
have hugpage or nodemask settings and others not.

After this patch we will use memdev for all cells if there
is at least one cell need this.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_command.c| 88 +-
  .../qemuxml2argv-numatune-memnode-no-memory.args   |  3 +-
  2 files changed, 55 insertions(+), 36 deletions(-)


I'm currently refactoring this piece of code and this patch would be
mostly reverted and reworked in the end, thus I'm not going to commit it
and I'll fix the bug in the new code after I'm finished with the
refactors.

Thanks for the patch though.


No problem, i am looking forward your patch:)


Peter


Luyao

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

Re: [libvirt] [PATCHv2 1/2] qemu: output error when try to hotplug unsupport console

[lhuang]

On 01/21/2015 02:59 AM, John Ferlan wrote:

Looks like this one may have been lost in the holidays...


Aha, thanks a lot for 'save' this patch, I almost forget this patch ;)

On 12/09/2014 01:48 AM, Luyao Huang wrote:

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

When try to use attach-device to hotplug a qemu
unsupport sonsole, command will return success,
and add a console in vm's xml.But this doesn't work
in qemu, qemu doesn't support these console.Add
a error output when try to hotplug a unsupport console
in qemuBuildConsoleChrDeviceStr.

Needs some grammar fixups...

When using 'virsh attach-device' to hotplug an unsupported console type
into a qemu guest, the attachment will erroneously allows the
attachment. This patch will check to ensure that only the support target
types are used for a running guest. NB, Although a guest can be defined
with an improper target, startup will cause failure.


Thanks a lot for improving the description.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_command.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1399ce4..7dced5f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10069,13 +10069,17 @@ qemuBuildConsoleChrDeviceStr(char **deviceStr,
  break;
  
  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL:

+break;
  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE:
  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN:
  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_UML:
  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC:
  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_OPENVZ:
  case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST:
-break;
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _(unsupported console target type %s),
+   
NULLSTR(virDomainChrConsoleTargetTypeToString(chr-targetType)));
+return ret;

This should be a goto cleanup;

I can clean this up; however, will it be necessary given patch 2/2?


Thanks and about Patch 2/2 is not fix this bug (although patch 2/2 will 
also fix hot-plug a qemu unsupported chr device). I have a discussion 
with Jan and Pavel in v1 of this patch (please see
https://www.redhat.com/archives/libvir-list/2014-December/msg00157.html 
and 
https://www.redhat.com/archives/libvir-list/2014-November/msg00988.html), so 
write patch 2/2 for avoiding cold-plug  a qemu unsupported device to a 
qemu vm.


John


  }
  
  ret = 0;




Luyao

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

Re: [libvirt] [PATCH 2/2] qemu: add a check when cold-plug a Chr device

[lhuang]

On 01/21/2015 03:00 AM, John Ferlan wrote:


On 12/09/2014 01:48 AM, Luyao Huang wrote:

Add a func just check the base target type which
qemu support. But i still doubt this will be useful
, we already have a check when we try to start the
vm. And this check only check the target type, and
the other things will be done in virDomainChrDefParseXML.


The commit message needs some massaging.

Essentially, this patch fixes the condition where if a guest has been
started and someone uses attach-device with (or without) the --config
option, then these checks will avoid the next guest being modified,
correct?


Right

This will also cause an error earlier that patch 1/2 as
qemuDomainChrInsert is called in the path before qemuDomainAttachDeviceLive




Yes and if this patch have been pushed then the patch 1/2 will be a 
patch for improving exist code.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_hotplug.c | 64 +
  1 file changed, 64 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b9a0cee..fe91ec7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1384,10 +1384,74 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr 
driver,
  
  }
  
+static int

+qemuDomainChrCheckDefSupport(virDomainChrDefPtr chr)
+{
+int ret = -1;
+
+switch (chr-deviceType) {
+case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
+switch ((virDomainChrSerialTargetType) chr-targetType) {
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
+case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
+ret = 0;
+break;
+
+default:

Typically in switches listing other options rather than default:

 case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:


+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _(unsupported serial target type %s for qemu),
+   
NULLSTR(virDomainChrSerialTargetTypeToString(chr-targetType)));
+break;
+}
+break;
+
+case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
+ret = 0;
+break;
+
+case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL:
+switch ((virDomainChrChannelTargetType) chr-targetType) {
+case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD:
+case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO:
+ret = 0;
+break;
+
+default:

Same, but:

 case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_NONE:
 case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST:


+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _(unsupported channel target type %s for qemu),
+   
NULLSTR(virDomainChrChannelTargetTypeToString(chr-targetType)));
+break;
+}
+break;
+
+case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE:
+switch ((virDomainChrConsoleTargetType) chr-targetType) {
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL:
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO:
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLP:
+case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLPLM:
+ret = 0;
+break;
+
+default:

Same, but:

 case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE:
 case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN:
 case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_UML:
 case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC:
 case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_OPENVZ:
 case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST:


I think perhaps this one is better than 1/2, but will see if my
responses result in other opinions...


Thanks for pointing out and i forgot cc Jan and Pavel when sent this 
patch, maybe they have some other opinions.


John


+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _(unsupported console target type %s for qemu),
+   
NULLSTR(virDomainChrConsoleTargetTypeToString(chr-targetType)));
+break;
+}
+break;
+}
+
+return ret;
+}
+
  int
  qemuDomainChrInsert(virDomainDefPtr vmdef,
  virDomainChrDefPtr chr)
  {
+if (qemuDomainChrCheckDefSupport(chr)  0)
+return -1;
+
  if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
  chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) {
  virReportError(VIR_ERR_OPERATION_UNSUPPORTED, %s,



Luyao

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

Re: [libvirt] [PATCH] virsh: fix show the wrong IP address for network type listen address graphic

[lhuang]

On 02/11/2015 04:40 AM, Laine Stump wrote:

On 02/10/2015 04:35 AM, Luyao Huang wrote:

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

We try to get the IP address in /domain/devices/graphics/@listen, howerver
for the network type listen address donnot have this parameter, it will
show the address in the /domain/devices/graphics/listen/@address, running
XML like this:

  graphics type='spice' port='5901' autoport='yes' keymap='en-us'
   listen type='network' address='192.168.122.1' network='default'/
  /graphics

This patch will try to get the IP address in this path
/domain/devices/graphics/listen/@address

That will work when the libvirtd being connected to is 0.9.4 or later,
but earlier versions of libvirt don't have the listen subelement;
instead they just have a 'listen' attribute directly inside graphics
that contains the address. All newer versions of libvirt are supposed to
populate that from listen[0] for backward compatibility.


Oh, right, thanks for your catch, i forgot this thing when i wrote this 
patch, my patch will cause a issue without the patch you attached when 
use new virsh client to connect to old libvirtd(older than 0.9.4).

The real bug here is that the listen attribute in graphics isn't being
filled in in the case of type='network' when the domain is active. On
the other hand, fixing the problem there would leave it unfixed for
cases where the client is a new libvirt but the server is running
libvirt between 0.9.4 and 1.2.12. So I think what is needed is for your
patch to check @listen, and if nothing is found there, *then* check
listen/@address. I attached a patch to this mail that I propose
squashing into your patch before pushing. Let me know if it behaves
properly and looks correct.


I test with the patch you attached, it works well when use new virsh 
client connect to new libvirtd and also works well with the old libvirtd 
(older than 0.9.4 or later ).

Thanks for your patch.

Beyond that, the server side should still be fixed. I just sent a patch
that does that:

https://www.redhat.com/archives/libvir-list/2015-February/msg00332.html


Yes, if the server have been fixed, old virsh client command will work 
well with new libvirtd.

Between the two patches, we will have fixed the problem for all versions
of server, as long as the client is new enough.


Thanks for your review and i found you have wrote a patch for the client 
side, so seems no need write a new version for this patch :)


Luyao

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

Re: [libvirt] [PATCH] virsh: fix show the wrong IP address for network type listen address graphic

[lhuang]

On 02/11/2015 04:40 AM, Laine Stump wrote:

On 02/10/2015 04:35 AM, Luyao Huang wrote:

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

We try to get the IP address in /domain/devices/graphics/@listen, howerver
for the network type listen address donnot have this parameter, it will
show the address in the /domain/devices/graphics/listen/@address, running
XML like this:

  graphics type='spice' port='5901' autoport='yes' keymap='en-us'
   listen type='network' address='192.168.122.1' network='default'/
  /graphics

This patch will try to get the IP address in this path
/domain/devices/graphics/listen/@address

That will work when the libvirtd being connected to is 0.9.4 or later,
but earlier versions of libvirt don't have the listen subelement;
instead they just have a 'listen' attribute directly inside graphics
that contains the address. All newer versions of libvirt are supposed to
populate that from listen[0] for backward compatibility.

The real bug here is that the listen attribute in graphics isn't being
filled in in the case of type='network' when the domain is active. On
the other hand, fixing the problem there would leave it unfixed for
cases where the client is a new libvirt but the server is running
libvirt between 0.9.4 and 1.2.12. So I think what is needed is for your
patch to check @listen, and if nothing is found there, *then* check
listen/@address. I attached a patch to this mail that I propose
squashing into your patch before pushing. Let me know if it behaves
properly and looks correct.

Beyond that, the server side should still be fixed. I just sent a patch
that does that:

https://www.redhat.com/archives/libvir-list/2015-February/msg00332.html

Between the two patches, we will have fixed the problem for all versions
of server, as long as the client is new enough.


Oh, i forgot the vncdisplay, it should also be fixed, i attached patch 
to this mail.


Luyao
From fcc4e7194e61788a459674d8041259309b4d867b Mon Sep 17 00:00:00 2001
From: Luyao Huang lhu...@redhat.com
Date: Wed, 11 Feb 2015 10:18:10 +0800
Subject: [PATCH] virsh: fix vncdisplay get wrong ip address when listen type
 is network

Just like the fix in domdisplay.
---
 tools/virsh-domain.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 358d61c..8dd6b5e 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10237,6 +10237,18 @@ cmdVNCDisplay(vshControl *ctl, const vshCmd *cmd)
 
 listen_addr = virXPathString(string(/domain/devices/graphics
  [@type='vnc']/@listen), ctxt);
+if (!listen_addr) {
+/* The subelement address - listen address='xyz'/ -
+ * *should* have been automatically backfilled into its
+ * parent graphics listen='xyz' (which we just tried to
+ * retrieve into listen_addr above) but in some cases it
+ * isn't, so we also do an explicit check for the
+ * subelement (which, by the way, doesn't exist on libvirt
+ *  0.9.4, so we really do need to check both places)
+ */
+listen_addr = virXPathString(string(/domain/devices/graphics
+ [@type='vnc']/listen/@address), ctxt);
+}
 if (listen_addr == NULL || STREQ(listen_addr, 0.0.0.0))
 vshPrint(ctl, :%d\n, port-5900);
 else
-- 
1.8.3.1

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

Re: [libvirt] [PATCH v3] lxc: fix show the wrong xml when guest start failed

[lhuang]

On 02/13/2015 04:45 AM, John Ferlan wrote:


On 02/04/2015 08:42 AM, Luyao Huang wrote:

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

When guest start failed, libvirt will keep the current vm-def,
this will make a issue that we cannot get a right xml after guest
start failed. And don't call the stop/release hook to do some
other clean work.

Call virLXCProcessCleanup to help us clean the source and call
the hooks if start a vm failed

Signed-off-by: Luyao Huang lhu...@redhat.com
---
v2: use virLXCProcessCleanup to free the source and call the hook.
v3: rework the patch to suit the virLXCProcessStart code changed.

  src/lxc/lxc_process.c | 76 ++-
  1 file changed, 32 insertions(+), 44 deletions(-)




FWIW:

v1 review starts here:

http://www.redhat.com/archives/libvir-list/2015-January/msg00112.html


At first I was 'concerned' about the number of changes, but after doing
more investigation - I think the patch is correct in general; however, I
think it needs some adjustments as there are really two bugs here...
I'll send an update shortly for comparison/review...


Yes, i agree about these patch need more adjustment, because i think 
maybe there is a better way to fix these issue but i cannot find them ;)


and thanks for your patches.

Essentially though - I think the console check*s* could be done earlier
before we get into other setup that requires going thru cleanup: (or
what error: was). That's one bug - which is a configuration error
which will prevent startup. Since it's easier to check early, provide an
error, and return -1 - that's what I think is cleaner solution.


Looks good for me

Second, the other bug which you were trying to clean up at the same time
is that existing cleanup paths don't properly clean all things up. The
error path worked only when/once the container started and some
pre-container start cleanup was done, but a few important ones were
missing - so that's a separate patch.

I also will add a patch to add/modify the debugging to help future such
efforts...


Thanks


BTW:
It seems commit id '88a1b542' from Cédric Bosdonnat caused yet another
merge conflict and seemingly from the description might have been in the
same area, but alas a quick check shows, it wasn't the same issue.

I'll leave the notes I was developing on this patch prior to just biting
the bullet and reformatting so you can perhaps see my thought process.


Yes, commit 88a1b542 is different from the issue i try to fix.

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 01da344..1a6cfbb 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -1022,6 +1022,7 @@ int virLXCProcessStart(virConnectPtr conn,
  virCgroupPtr selfcgroup;
  int status;
  char *pidfile = NULL;
+bool need_stop = false;
  


I think the check:

 if (vm-def-nconsoles == 0) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
_(At least one PTY console is required));
 goto cleanup;
 }

Should perhaps be moved to just before this code:

 nttyFDs = vm-def-nconsoles;
 if (VIR_ALLOC_N(ttyFDs, nttyFDs)  0)
 goto cleanup;
 for (i = 0; i  vm-def-nconsoles; i++)
 ttyFDs[i] = -1;

and that would fix the bug from the bz... as well as ensuring we don't
have a if (VIR_ALLOC_N(ttdFDs, 0)  0)... In fact is there a reason
why that check cannot move much higher and be after the cgroup checks
which return -1?  While t it, why not move the following too:

 for (i = 0; i  vm-def-nconsoles; i++) {
 if (vm-def-consoles[i]-source.type != VIR_DOMAIN_CHR_TYPE_PTY) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
_(Only PTY console types are supported));
 goto cleanup;
 }
 }

and then remove that from the loop later on.


Yes, after your words, i think console check in this place should be 
improved.

This way checks that go to cleanup are a result of some error in
processing rather than some container configuration error...

Then the remainder of the code could be perhaps patch 2 which is fixing
a different, although related problem.


  if (virCgroupNewSelf(selfcgroup)  0)
  return -1;
@@ -1259,6 +1260,7 @@ int virLXCProcessStart(virConnectPtr conn,
  goto cleanup;
  }
  


...

-VIR_FREE(vm-def-seclabels[0]-imagelabel);
-VIR_DELETE_ELEMENT(vm-def-seclabels, 0, vm-def-nseclabels);
+err = virSaveLastError();
+if (need_stop) {
+virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
+} else {
+virSecurityManagerRestoreAllLabel(driver-securityManager,
+  vm-def, false);
+virSecurityManagerReleaseLabel(driver-securityManager, vm-def);
+/* Clear out dynamically assigned labels */
+if (vm-def-nseclabels 
+vm-def-seclabels[0]-type == 

Re: [libvirt] [PATCH] virsh: fix IP address in vncdisplay for listen type='network'

[lhuang]

On 02/16/2015 06:51 PM, Pavel Hrdina wrote:

On Sun, Feb 15, 2015 at 04:49:09PM +0800, Luyao Huang wrote:

Just like the fix for domdisplay in commit 1ba815.
---
  tools/virsh-domain.c | 12 
  1 file changed, 12 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index dc4a863..2506b89 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10269,6 +10269,18 @@ cmdVNCDisplay(vshControl *ctl, const vshCmd *cmd)
  
  listen_addr = virXPathString(string(/domain/devices/graphics

   [@type='vnc']/@listen), ctxt);
+if (!listen_addr) {
+/* The subelement address - listen address='xyz'/ -
+ * *should* have been automatically backfilled into its
+ * parent graphics listen='xyz' (which we just tried to
+ * retrieve into listen_addr above) but in some cases it
+ * isn't, so we also do an explicit check for the
+ * subelement (which, by the way, doesn't exist on libvirt
+ *  0.9.4, so we really do need to check both places)
+ */
+listen_addr = virXPathString(string(/domain/devices/graphics
+ [@type='vnc']/listen/@address), ctxt);
+}
  if (listen_addr == NULL || STREQ(listen_addr, 0.0.0.0))
  vshPrint(ctl, :%d\n, port-5900);
  else
--
1.8.3.1


ACK and pushed.


Thanks for the quick review.

Pavel


--
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] qemu: fix cannot set graphic passwd via qemuDomainSaveImageDefineXML

[lhuang]

On 01/29/2015 12:25 AM, Michal Privoznik wrote:

On 20.01.2015 10:04, Luyao Huang wrote:

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

When we try to update a xml to a image file, we will clear the
graphics passwd settings, because we do not pass VIR_DOMAIN_XML_SECURE
to qemuDomainDefCopy, qemuDomainDefFormatBuf won't format the passwd.

Add VIR_DOMAIN_XML_SECURE flag when we call qemuDomainDefCopy
in qemuDomainSaveImageUpdateDef.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_driver.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5994558..abe3b9f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5613,7 +5613,8 @@ qemuDomainSaveImageUpdateDef(virQEMUDriverPtr driver,
  
  if (!(newdef_migr = qemuDomainDefCopy(driver,

newdef,
-  VIR_DOMAIN_XML_MIGRATABLE)))
+  VIR_DOMAIN_XML_MIGRATABLE |
+  VIR_DOMAIN_XML_SECURE)))
  goto cleanup;
  
  if (!virDomainDefCheckABIStability(def, newdef_migr)) {



ACKed and pushed. Thanks for catching that.


Thanks for your review and push.


Michal


Luyao

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

Re: [libvirt] [PATCH 05/12] libvirt_private: add 4 new func in libvirt_private.syms

[lhuang]

On 01/06/2015 04:11 PM, Peter Krempa wrote:

On 01/06/15 09:10, lhuang wrote:

On 01/05/2015 10:48 PM, Peter Krempa wrote:

On 01/03/15 06:06, Luyao Huang wrote:

Signed-off-by: Luyao Huang lhu...@redhat.com
---
   src/libvirt_private.syms | 5 +
   1 file changed, 5 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index aa776b4..deab4cf 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -375,7 +375,12 @@ virDomainPMSuspendedReasonTypeToString;
   virDomainRedirdevBusTypeFromString;
   virDomainRedirdevBusTypeToString;
   virDomainRNGBackendTypeToString;
+virDomainRNGDefFree;
+virDomainRNGEquals;
+virDomainRNGFind;
+virDomainRNGInsert;
   virDomainRNGModelTypeToString;
+virDomainRNGRemove;
   virDomainRunningReasonTypeFromString;
   virDomainRunningReasonTypeToString;
   virDomainSaveConfig;


These should be in the patches that add the individual functions.

Okay, i will move virDomainRNGEquals, virDomainRNGFind,
virDomainRNGEquals, virDomainRNGRemove to other
patchs, and leave virDomainRNGDefFree in this patch.

If a function exists already, then add the symbol export to the patch
that uses it in a place that requires the symbol being exported.

Got it, thanks your advise.

Thanks for review.

Peter


Peter


Luyao

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

Re: [libvirt] [PATCH 11/12] qemu: Implement RNG device hotplug on live level

[lhuang]

On 01/05/2015 11:48 PM, Peter Krempa wrote:

On 01/03/15 06:06, Luyao Huang wrote:

We have enough patches for hotplug RNG device, maybe we can
implement live hotplug of a RNG device.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_driver.c  |  8 -
  src/qemu/qemu_hotplug.c | 92 +
  src/qemu/qemu_hotplug.h |  3 ++
  3 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f9327b4..7f1e612 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1523,6 +1523,98 @@ qemuDomainRNGRemove(virDomainDefPtr vmdef,
  return ret;
  }
  
+int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,

+  virDomainObjPtr vm,
+  virDomainRNGDefPtr rng)
+{
+int ret = -1;
+qemuDomainObjPrivatePtr priv = vm-privateData;
+virDomainDefPtr vmdef = vm-def;
+char *devstr = NULL;
+char *charAlias = NULL;
+char *objAlias = NULL;
+bool need_remove = false;
+bool releaseaddr = false;
+
+if (!virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_DEVICE)) {
+virReportError(VIR_ERR_OPERATION_INVALID, %s,
+   _(qemu does not support -device));
+return ret;
+}

Additionally to the -device support, qemu also needs to support chardev
hotplug and the rng device with the appropriate backends, we already
have capability bits for them so you need to check them here.

Thanks for your review.

Yes, i forgot these check in this place and i found 
QEMU_CAPS_OBJECT_RNG_RANDOM

and QEMU_CAPS_OBJECT_RNG_EGD for random and egd backend RNG device.
But i cannot found a capability bit for support chardev hotplug, maybe 
this one? QEMU_CAPS_CHARDEV



+
+if (qemuAssignDeviceRNGAlias(vmdef, rng, -1)  0)
+return ret;
+
+if (rng-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+if (STRPREFIX(vm-def-os.machine, s390-ccw) 
+virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
+rng-info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
+} else if (virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
+rng-info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390;
+}
+}
+
+if (rng-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
+rng-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+if (virDomainPCIAddressEnsureAddr(priv-pciaddrs, rng-info)  0)
+return ret;
+} else if (rng-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
+if (virDomainCCWAddressAssign(rng-info, priv-ccwaddrs,
+  !rng-info.addr.ccw.assigned)  0)

Line above is misaligned.

Thanks



+return ret;
+}
+releaseaddr = true;
+if (!(devstr = qemuBuildRNGDevStr(vmdef, rng, priv-qemuCaps)))
+return ret;

After you set releaseaddr to true you need to jump to cleanup in case of
error so that the address gets cleared.

Yes, i have made a mistake here. I will fix it in next version.

+
+if (virAsprintf(objAlias, obj%s, rng-info.alias)  0)
+goto cleanup;
+
+if (rng-backend == VIR_DOMAIN_RNG_BACKEND_EGD) {
+if (virAsprintf(charAlias, char%s, rng-info.alias)  0)
+goto cleanup;
+
+qemuDomainObjEnterMonitor(driver, vm);
+if (qemuMonitorAttachCharDev(priv-mon, charAlias, rng-source.chardev) 
 0) {
+qemuDomainObjExitMonitor(driver, vm);
+goto audit;
+}
+need_remove = true;

This variable should be named remove_chardev so that it's clear what
it's used to.


Okay, thanks

+} else {
+qemuDomainObjEnterMonitor(driver, vm);
+}
+
+if (qemuMonitorAttachRNGDev(priv-mon, charAlias, objAlias, rng)  0) {
+if (need_remove)
+qemuMonitorDetachCharDev(priv-mon, charAlias);
+qemuDomainObjExitMonitor(driver, vm);
+goto audit;
+}
+
+if (devstr  qemuMonitorAddDevice(priv-mon, devstr)  0) {
+qemuMonitorDetachRNGDev(priv-mon, objAlias);
+if (need_remove)
+qemuMonitorDetachCharDev(priv-mon, charAlias);
+qemuDomainObjExitMonitor(driver, vm);
+goto audit;
+}
+qemuDomainObjExitMonitor(driver, vm);
+
+if (qemuDomainRNGInsert(vmdef, rng)  0)
+goto cleanup;
+
+ret = 0;
+ audit:
+virDomainAuditRNG(vm, NULL, rng, attach, ret == 0);
+ cleanup:
+if (releaseaddr)
+qemuDomainReleaseDeviceAddress(vm, rng-info, NULL);
+VIR_FREE(charAlias);
+VIR_FREE(objAlias);
+VIR_FREE(devstr);
+return ret;
+}
+
  
  static int

  qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,


The rest looks good.

Peter




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

Re: [libvirt] [PATCH 09/12] qemu_monitor: add 2 functions qemuMonitorDetachRNGDev and qemuMonitorAttachRNGDev

[lhuang]

On 01/05/2015 11:31 PM, Peter Krempa wrote:

On 01/03/15 06:06, Luyao Huang wrote:

These 2 functions just do some basic check and then call
qemuMonitorJSONAttachRNGDev and qemuMonitorDelObject to
help us.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_monitor.c | 43 +++
  src/qemu/qemu_monitor.h |  7 +++
  2 files changed, 50 insertions(+)


This patch can be folded into the previous one. And again the commit
message could be improved.

Thanks for your review.
Okay, i will merge them into patch qemu: add a functions for attach a 
rng object in json monitor

Peter



Luyao

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

Re: [libvirt] [PATCH 07/12] qemu: introduce 2 func qemuDomainRNGInsert and qemuDomainRNGRemove

[lhuang]

On 01/05/2015 11:22 PM, Peter Krempa wrote:

On 01/03/15 06:06, Luyao Huang wrote:

qemu side functions, call virDomainRNGInsert and virDomainRNGRemove
to help us.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_hotplug.c | 23 +++
  src/qemu/qemu_hotplug.h |  7 +++
  2 files changed, 30 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 7f93b9b..f9327b4 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1501,6 +1501,29 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
  return ret;
  }
  
+int

+qemuDomainRNGInsert(virDomainDefPtr vmdef,
+virDomainRNGDefPtr rng)
+{
+return virDomainRNGInsert(vmdef, rng);
+}

This wrapper doesn't seem useful.

Yes, i will remove this function in next version.



+
+virDomainRNGDefPtr
+qemuDomainRNGRemove(virDomainDefPtr vmdef,
+virDomainRNGDefPtr rng)
+{
+virDomainRNGDefPtr ret;
+
+if (!(ret = virDomainRNGRemove(vmdef, rng))) {
+virReportError(VIR_ERR_INVALID_ARG, %s,
+   _(device not present in domain configuration));
+return NULL;
+}

Given that this function is used exactly once in the series you've
posted it doesn't make much sense to have the code separate.


Okay, ...

+
+return ret;
+}
+
+
  static int
  qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index d13c532..7b838ee 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -107,6 +107,13 @@ virDomainChrDefPtr
  qemuDomainChrRemove(virDomainDefPtr vmdef,
  virDomainChrDefPtr chr);
  
+int

+qemuDomainRNGInsert(virDomainDefPtr vmdef,
+virDomainRNGDefPtr rng);
+virDomainRNGDefPtr
+qemuDomainRNGRemove(virDomainDefPtr vmdef,
+virDomainRNGDefPtr rng);
+

Both of the functions above are used only in qemu_hotplug.c. It doesn't
make sense to export them.

... i will remove them and won't export qemuDomainRNGRemove.

Thanks for your review and pointing out.

  void qemuDomainRemoveDevice(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
  virDomainDeviceDefPtr dev);


Peter




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

Re: [libvirt] [PATCH 03/12] conf: introduce a new func virDomainRNGEquals

[lhuang]

On 01/05/2015 11:00 PM, Peter Krempa wrote:

Subject: ... How about

conf: Introduce function to compare RNG devices.


On 01/03/15 06:06, Luyao Huang wrote:

virDomainRNGEquals is a func which check if two rng device
are the same.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/domain_conf.c | 34 ++
  src/conf/domain_conf.h |  3 +++
  2 files changed, 37 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index aafc05e..91c114e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11922,6 +11922,40 @@ virDomainChrRemove(virDomainDefPtr vmdef,
  return ret;
  }
  
+bool

+virDomainRNGEquals(virDomainRNGDefPtr src,
+   virDomainRNGDefPtr tgt)
+{
+if (!src || !tgt)
+return src == tgt;
+
+if (src-model != tgt-model)
+return false;
+
+if (src-rate != tgt-rate || src-period != tgt-period)
+return false;
+
+switch ((virDomainRNGModel) src-model) {
+case VIR_DOMAIN_RNG_MODEL_VIRTIO:
+switch ((virDomainRNGBackend) src-backend) {
+case VIR_DOMAIN_RNG_BACKEND_RANDOM:
+return STREQ_NULLABLE(src-source.file, tgt-source.file);
+break;
+case VIR_DOMAIN_RNG_BACKEND_EGD:
+return virDomainChrSourceDefIsEqual((virDomainChrSourceDef *) 
src-source.chardev,
+(virDomainChrSourceDef *) 
tgt-source.chardev);

No need for typecast here; both src-source.chardev and tgt ... are
already in the required type.


Got it, i will remove the typecast in next version.

+break;
+case VIR_DOMAIN_RNG_BACKEND_LAST:
+break;
+}
+break;
+
+case VIR_DOMAIN_RNG_MODEL_LAST:
+break;
+}
+return false;
+}
+
  char *
  virDomainDefGetDefaultEmulator(virDomainDefPtr def,
 virCapsPtr caps)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 57297cd..c197095 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2597,6 +2597,9 @@ virDomainChrInsert(virDomainDefPtr vmdef,
  virDomainChrDefPtr
  virDomainChrRemove(virDomainDefPtr vmdef,
 virDomainChrDefPtr chr);
+bool
+virDomainRNGEquals(virDomainRNGDefPtr src,
+   virDomainRNGDefPtr tgt);

As in the case below, please add the type to the same line as the
declaration.

OKay

Thanks for your review
  
  int virDomainSaveXML(const char *configDir,

   virDomainDefPtr def,


Peter



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

Re: [libvirt] [PATCH 06/12] qemu: add id when build RNG device and rename object id

[lhuang]

On 01/05/2015 11:45 PM, Peter Krempa wrote:

On 01/05/15 15:51, Peter Krempa wrote:

On 01/03/15 06:06, Luyao Huang wrote:

We didn't set a id when we build RNG device cmdline before.
Give a id to every RNG device and we can hotunplug it via
QMP cmd device_del.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_command.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 46e289d..a4073ee 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5761,7 +5761,7 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd,
  goto cleanup;
  }
  
-virBufferAsprintf(buf, rng-random,id=%s,filename=%s,

+virBufferAsprintf(buf, rng-random,id=obj%s,filename=%s,
dev-info.alias, dev-source.file);
  
  virCommandAddArg(cmd, -object);

@@ -5784,7 +5784,7 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd,
  virCommandAddArgList(cmd, -chardev, backend, NULL);
  
  virCommandAddArg(cmd, -object);

-virCommandAddArgFormat(cmd, rng-egd,chardev=char%s,id=%s,
+virCommandAddArgFormat(cmd, rng-egd,chardev=char%s,id=obj%s,
 dev-info.alias, dev-info.alias);
  break;
  
@@ -5816,13 +5816,13 @@ qemuBuildRNGDevStr(virDomainDefPtr def,

  }
  
  if (dev-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)

-virBufferAsprintf(buf, virtio-rng-ccw,rng=%s, dev-info.alias);
+virBufferAsprintf(buf, virtio-rng-ccw,rng=obj%s,id=%s, 
dev-info.alias, dev-info.alias);
  else if (dev-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390)
-virBufferAsprintf(buf, virtio-rng-s390,rng=%s, dev-info.alias);
+virBufferAsprintf(buf, virtio-rng-s390,rng=obj%s,id=%s, 
dev-info.alias, dev-info.alias);
  else if (dev-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO)
-virBufferAsprintf(buf, virtio-rng-device,rng=%s, dev-info.alias);
+virBufferAsprintf(buf, virtio-rng-device,rng=obj%s,id=%s, 
dev-info.alias, dev-info.alias);
  else
-virBufferAsprintf(buf, virtio-rng-pci,rng=%s, dev-info.alias);
+virBufferAsprintf(buf, virtio-rng-pci,rng=obj%s,id=%s, 
dev-info.alias, dev-info.alias);
  
  if (dev-rate  0) {

  virBufferAsprintf(buf, ,max-bytes=%u, dev-rate);



This breaks the testsuite as you didn't fix the expected outputs after
such a change. Please always run make check before posting patches.

Additional question. Is this change even necessary? The RNG device does
have it's alias already whithout the obj prefix ... thus it's just
rng0 for example.

I misread the code. This is actually necessary as otherwise the -device
would have the same ID as the backend object. That makes sense to
change, although we need to make sure then that the code will work in
case of a long running VM (with the incorrect name) and a new libvirt
instance.

At any rate ... you need to fix the tests after this commit

Thanks for your review.
Yes, I will give another commit for the tests fix in next version.

I have test with a long running VM (start in old libvirt which RNG 
device no
device name), and update to a libvirt which have these code, if we try 
to hot-unplug the

rng device, qemu will return a error like this :
internal error: unable to execute QEMU command 'device_del': Device 
'rng0' not found


maybe my qemu is too old ? vm qemu cmdline -device do not have a id 
(because i start it in the
old libvirt), so this error is correct in this place. But i don't know 
how can i hot-unplug a device without

a id.

Peter



Luyao

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

Re: [libvirt] [PATCH 01/12] qemu: introduce a new func qemuAssignDeviceRNGAlias for rng device

[lhuang]

On 01/05/2015 11:10 PM, Peter Krempa wrote:

Subject: perhaps..

qemu: Add helper to assign RNG device aliases

Thanks, good Subject

On 01/03/15 06:06, Luyao Huang wrote:

This function used to set a alias name for RNG device, usage just

This function is used to assign an alias for a RNG device. It will be
later reused when hotplugging RNGs.

Thanks a lot

like other named qemuAssignDevice***Alias functions.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_command.c | 25 -
  src/qemu/qemu_command.h |  1 +
  2 files changed, 25 insertions(+), 1 deletion(-)


ACK with the commit message changes. (Please include the change in the
next posting, as I'm already requiring a new version).

Peter




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

Re: [libvirt] [PATCH 02/12] qemu: rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr and change something

[lhuang]

On 01/05/2015 11:18 PM, Peter Krempa wrote:

Subject: change something? That's a really vague statement.

How about:

qemu: refactor qemuBuildRNGDeviceArgs to allow reuse in RNG hotplug

Good subject :)
Thanks

On 01/03/15 06:06, Luyao Huang wrote:

rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr, we need this function
to build a cmdline.

Rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr and change the
return type so that it can be reused in the device hotplug code later.


Thanks

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_command.c | 33 -
  src/qemu/qemu_command.h |  4 
  2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c1e9bca..46e289d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5800,22 +5800,19 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd,
  return ret;
  }
  
-

Don't delete the line. Functions are usually separated by two newlines.

Okay

-static int
-qemuBuildRNGDeviceArgs(virCommandPtr cmd,
-   virDomainDefPtr def,
-   virDomainRNGDefPtr dev,
-   virQEMUCapsPtr qemuCaps)
+char *
+qemuBuildRNGDevStr(virDomainDefPtr def,
+   virDomainRNGDefPtr dev,
+   virQEMUCapsPtr qemuCaps)
  {
  virBuffer buf = VIR_BUFFER_INITIALIZER;
-int ret = -1;
  
  if (dev-model != VIR_DOMAIN_RNG_MODEL_VIRTIO ||

  !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_RNG)) {
  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 _(this qemu doesn't support RNG device type '%s'),
 virDomainRNGModelTypeToString(dev-model));
-goto cleanup;
+goto error;
  }
  
  if (dev-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)

ACK, looks good except for the commit message and the spurious line
deletion.

Peter




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

Re: [libvirt] [PATCH 10/12] audit: make function virDomainAuditRNG global

[lhuang]

On 01/05/2015 11:32 PM, Peter Krempa wrote:

In subject:

audit: export virDomainAuditRNG

On 01/03/15 06:06, Luyao Huang wrote:

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/domain_audit.c  | 2 +-
  src/conf/domain_audit.h  | 7 +++
  src/libvirt_private.syms | 1 +
  3 files changed, 9 insertions(+), 1 deletion(-)


ACK,

Thanks, i will update the subject in next version.

Peter



Luyao

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

Re: [libvirt] [PATCH 08/12] qemu: introduce 2 functions for attach a rng object in json monitor

[lhuang]

On 01/05/2015 11:29 PM, Peter Krempa wrote:

On 01/03/15 06:06, Luyao Huang wrote:

We need a new function to build a RNG device object, and need a
function to build a props which will be used in qemuMonitorJSONAddObject.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_monitor_json.c | 58 
  src/qemu/qemu_monitor_json.h |  5 
  2 files changed, 63 insertions(+)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e567aa7..4430819 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6166,6 +6166,64 @@ qemuMonitorJSONDetachCharDev(qemuMonitorPtr mon,
  return ret;
  }
  
+static virJSONValuePtr

+qemuMonitorJSONRNGPropsCommand(const char *name,
+   const char *data)
+{
+virJSONValuePtr ret;
+
+if (!(ret = virJSONValueNewObject()))
+goto error;
+
+if (virJSONValueObjectAppendString(ret, name, data)  0)
+goto error;
+
+return ret;
+
+ error:
+virJSONValueFree(ret);
+return NULL;
+}

To allow adding generic properties to objects I've added the
virJSONValueObjectCreate function that allows to create generic json
value objects. Please use that func instead of the above.

Thanks for pointing out, i forgot double check the exist functions.



+
+int
+qemuMonitorJSONAttachRNGDev(qemuMonitorPtr mon,
+const char *chrID,
+const char *objID,
+virDomainRNGDefPtr rng)
+{
+const char *type = NULL;
+virJSONValuePtr props = NULL;
+
+switch ((virDomainRNGBackend) rng-backend) {
+case VIR_DOMAIN_RNG_BACKEND_RANDOM:
+type = rng-random;
+if (!(props = qemuMonitorJSONRNGPropsCommand(filename, 
rng-source.file)))

With usage of virJSONValueObjectCreate the code will look like:

if (!(props = virJSONValueObjectCreate(s:filename, rng-source.file,
NULL)))

Thanks the example, i will use them in next version.



+goto cleanup;
+break;
+
+case VIR_DOMAIN_RNG_BACKEND_EGD:
+if (!chrID) {
+virReportError(VIR_ERR_INTERNAL_ERROR,%s,
+   _(miss chardev id));
+goto cleanup;
+}

The chardev and backend object ID can (and should) be inferred from the
rng device ID as they should be the same (except for the char/obj
prefix).

Eww, i think your mean is add a check for charname and objname, if they
are different then output a error?


+type = rng-egd;
+if (!(props = qemuMonitorJSONRNGPropsCommand(chardev, chrID)))
+goto cleanup;
+break;
+
+case VIR_DOMAIN_RNG_BACKEND_LAST:
+/*shouldn't happen*/
+goto cleanup;
+}
+
+return qemuMonitorJSONAddObject(mon, type, objID, props);
+
+ cleanup:
+virJSONValueFree(props);
+return -1;
+}
+
  
  int

Peter



Luyao

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

Re: [libvirt] [PATCH 04/12] conf: introduce 3 functions for RNG device

[lhuang]

On 01/05/2015 10:56 PM, Peter Krempa wrote:

The subject is vague. Please try to condense what the patch is adding in
a way that describes the functions instead of an opaque add 3 functions


On 01/03/15 06:06, Luyao Huang wrote:

the 3 functions are:
virDomainRNGInsert: Insert a RNG device to vm-def.
virDomainRNGRemove: remove a RNG device in vm-def.
virDomainRNGFind: find a RNG device in vm-def.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/domain_conf.c | 44 
  src/conf/domain_conf.h |  9 +
  2 files changed, 53 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 91c114e..37c4569 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11956,6 +11956,50 @@ virDomainRNGEquals(virDomainRNGDefPtr src,
  return false;
  }
  
+int

+virDomainRNGInsert(virDomainDefPtr vmdef,
+   virDomainRNGDefPtr rng)
+{
+return VIR_APPEND_ELEMENT(vmdef-rngs, vmdef-nrngs, rng);
+}
+
+virDomainRNGDefPtr
+virDomainRNGRemove(virDomainDefPtr vmdef,
+   virDomainRNGDefPtr rng)
+{
+virDomainRNGDefPtr ret;
+size_t i;
+
+for (i = 0; i  vmdef-nrngs; i++) {
+ret = vmdef-rngs[i];
+
+if (virDomainRNGEquals(ret, rng))

If you add a block here and move the VIR_DELETE_ELEMENT line here.


+break;

And return ret instead of breaking here.


+}

Then you can return NULL here and get rid of the rest.

The resulting code will be more readable.


Good idea, thanks a lot.



+
+if (i == vmdef-nrngs)
+return NULL;
+
+VIR_DELETE_ELEMENT(vmdef-rngs, i, vmdef-nrngs);
+return ret;
+}
+
+virDomainRNGDefPtr
+virDomainRNGFind(virDomainDefPtr vmdef,
+ virDomainRNGDefPtr rng)
+{
+virDomainRNGDefPtr ret;
+size_t i;
+
+for (i = 0; i  vmdef-nrngs; i++) {
+ret = vmdef-rngs[i];
+
+if (virDomainRNGEquals(ret, rng))
+return ret;
+}
+return NULL;
+}
+
  char *
  virDomainDefGetDefaultEmulator(virDomainDefPtr def,
 virCapsPtr caps)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index c197095..cb87fad 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2600,6 +2600,15 @@ virDomainChrRemove(virDomainDefPtr vmdef,
  bool
  virDomainRNGEquals(virDomainRNGDefPtr src,
 virDomainRNGDefPtr tgt);
+int
+virDomainRNGInsert(virDomainDefPtr vmdef,
+   virDomainRNGDefPtr rng);

In header files we usually put the return type on the same line as the
declaration.

Okay, thanks , i will change them in next version

+virDomainRNGDefPtr
+virDomainRNGRemove(virDomainDefPtr vmdef,
+   virDomainRNGDefPtr rng);
+virDomainRNGDefPtr
+virDomainRNGFind(virDomainDefPtr vmdef,
+ virDomainRNGDefPtr rng);
  
  int virDomainSaveXML(const char *configDir,

   virDomainDefPtr def,

Like here.

Peter


Luyao

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

Re: [libvirt] [PATCH 05/12] libvirt_private: add 4 new func in libvirt_private.syms

[lhuang]

On 01/05/2015 10:48 PM, Peter Krempa wrote:

On 01/03/15 06:06, Luyao Huang wrote:

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/libvirt_private.syms | 5 +
  1 file changed, 5 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index aa776b4..deab4cf 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -375,7 +375,12 @@ virDomainPMSuspendedReasonTypeToString;
  virDomainRedirdevBusTypeFromString;
  virDomainRedirdevBusTypeToString;
  virDomainRNGBackendTypeToString;
+virDomainRNGDefFree;
+virDomainRNGEquals;
+virDomainRNGFind;
+virDomainRNGInsert;
  virDomainRNGModelTypeToString;
+virDomainRNGRemove;
  virDomainRunningReasonTypeFromString;
  virDomainRunningReasonTypeToString;
  virDomainSaveConfig;


These should be in the patches that add the individual functions.
Okay, i will move virDomainRNGEquals, virDomainRNGFind, 
virDomainRNGEquals, virDomainRNGRemove to other

patchs, and leave virDomainRNGDefFree in this patch.

Thanks for review.

Peter



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

Re: [libvirt] [PATCH 12/12] qemu: Implement RNG device hotunplug on live level

[lhuang]

On 01/05/2015 11:54 PM, Peter Krempa wrote:

On 01/03/15 06:06, Luyao Huang wrote:

We have enough patches for hotunplug RNG device, maybe we can
implement live hotunplug of a RNG device.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_driver.c  |  4 +-
  src/qemu/qemu_hotplug.c | 97 -
  src/qemu/qemu_hotplug.h |  4 +-
  3 files changed, 102 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2ad6e01..f7600f3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7017,6 +7017,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
  case VIR_DOMAIN_DEVICE_CHR:
  ret = qemuDomainDetachChrDevice(driver, vm, dev-data.chr);
  break;
+case VIR_DOMAIN_DEVICE_RNG:
+ret = qemuDomainDetachRNGDevice(driver, vm, dev-data.rng);
+break;
  case VIR_DOMAIN_DEVICE_FS:
  case VIR_DOMAIN_DEVICE_INPUT:
  case VIR_DOMAIN_DEVICE_SOUND:
@@ -7027,7 +7030,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
  case VIR_DOMAIN_DEVICE_SMARTCARD:
  case VIR_DOMAIN_DEVICE_MEMBALLOON:
  case VIR_DOMAIN_DEVICE_NVRAM:
-case VIR_DOMAIN_DEVICE_RNG:
  case VIR_DOMAIN_DEVICE_SHMEM:
  case VIR_DOMAIN_DEVICE_REDIRDEV:
  case VIR_DOMAIN_DEVICE_NONE:
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 7f1e612..d61e2a1 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2921,6 +2921,52 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
  return ret;
  }
  
+static int

+qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
+  virDomainObjPtr vm,
+  virDomainRNGDefPtr rng)
+{
+virObjectEventPtr event;
+char *charAlias = NULL;
+char *objAlias = NULL;
+qemuDomainObjPrivatePtr priv = vm-privateData;
+int ret = -1;
+int rc;
+
+VIR_DEBUG(Removing RNG device %s from domain %p %s,
+  rng-info.alias, vm, vm-def-name);
+
+if (virAsprintf(objAlias, obj%s, rng-info.alias)  0)
+goto cleanup;
+
+if (virAsprintf(charAlias, char%s, rng-info.alias)  0)
+goto cleanup;
+
+qemuDomainObjEnterMonitor(driver, vm);
+rc = qemuMonitorDetachRNGDev(priv-mon, objAlias);
+if (rc == 0  rng-backend == VIR_DOMAIN_RNG_BACKEND_EGD)
+ignore_value(qemuMonitorDetachCharDev(priv-mon, charAlias));
+qemuDomainObjExitMonitor(driver, vm);
+
+virDomainAuditRNG(vm, rng, NULL, detach, rc == 0);
+
+if (rc  0)
+goto cleanup;
+
+event = virDomainEventDeviceRemovedNewFromObj(vm, rng-info.alias);
+if (event)
+qemuDomainEventQueue(driver, event);
+
+qemuDomainRNGRemove(vm-def, rng);
+qemuDomainReleaseDeviceAddress(vm, rng-info, NULL);
+virDomainRNGDefFree(rng);
+ret = 0;
+
+ cleanup:
+VIR_FREE(charAlias);
+VIR_FREE(objAlias);
+return ret;
+}
  
  void

  qemuDomainRemoveDevice(virQEMUDriverPtr driver,
@@ -2944,6 +2990,9 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
  case VIR_DOMAIN_DEVICE_CHR:
  qemuDomainRemoveChrDevice(driver, vm, dev-data.chr);
  break;
+case VIR_DOMAIN_DEVICE_RNG:
+qemuDomainRemoveRNGDevice(driver, vm, dev-data.rng);
+break;
  
  case VIR_DOMAIN_DEVICE_NONE:

  case VIR_DOMAIN_DEVICE_LEASE:
@@ -2958,7 +3007,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
  case VIR_DOMAIN_DEVICE_SMARTCARD:
  case VIR_DOMAIN_DEVICE_MEMBALLOON:
  case VIR_DOMAIN_DEVICE_NVRAM:
-case VIR_DOMAIN_DEVICE_RNG:
  case VIR_DOMAIN_DEVICE_SHMEM:
  case VIR_DOMAIN_DEVICE_TPM:
  case VIR_DOMAIN_DEVICE_PANIC:
@@ -3830,3 +3878,50 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
  VIR_FREE(devstr);
  return ret;
  }
+
+int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
+  virDomainObjPtr vm,
+  virDomainRNGDefPtr rng)
+{
+int ret = -1;
+qemuDomainObjPrivatePtr priv = vm-privateData;
+virDomainDefPtr vmdef = vm-def;
+virDomainRNGDefPtr tmpRNG;
+int rc;
+
+if (!(tmpRNG = virDomainRNGFind(vmdef, rng))) {
+virReportError(VIR_ERR_OPERATION_INVALID, %s,
+   _(device not present in domain configuration));
+return ret;
+}
+
+if (!virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_DEVICE)) {
+virReportError(VIR_ERR_OPERATION_INVALID, %s,
+   _(qemu does not support -device));
+return ret;
+}
+
+if (!tmpRNG-info.alias  qemuAssignDeviceRNGAlias(vmdef, tmpRNG, -1)  0)
+return ret;
+
+sa_assert(tmpRNG-info.alias);

Did coverity complain here?

I guess it will, this place just like the scene in commit e7e05801.
but i am not sure (i don't use coverity because the resource is very 
limit),

maybe we can remove it first, then wait for coverity test result.

+
+qemuDomainMarkDeviceForRemoval(vm, tmpRNG-info);
+
+

Re: [libvirt] [PATCH v2 07/12] qemu: add a functions for attach a rng object in json monitor

[lhuang]

On 01/10/2015 11:29 AM, Luyao Huang wrote:

We need a new function to build a RNG device object.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_monitor_json.c | 46 
  src/qemu/qemu_monitor_json.h |  5 +
  2 files changed, 51 insertions(+)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e567aa7..598b15d 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6166,6 +6166,52 @@ qemuMonitorJSONDetachCharDev(qemuMonitorPtr mon,
  return ret;
  }
  
+int

+qemuMonitorJSONAttachRNGDev(qemuMonitorPtr mon,
+const char *chrID,
+const char *objID,
+virDomainRNGDefPtr rng)
+{
+const char *type = NULL;
+virJSONValuePtr props = NULL;
+
+switch ((virDomainRNGBackend) rng-backend) {
+case VIR_DOMAIN_RNG_BACKEND_RANDOM:
+type = rng-random;
+if (!(props = virJSONValueObjectCreate(s:filename, rng-source.file, 
NULL)))
+goto cleanup;
+break;
+
+case VIR_DOMAIN_RNG_BACKEND_EGD:
+if (!chrID) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(miss chardev id));
+goto cleanup;
+}
+/*chrID + 4 and objID + 3 pointing to the basic alias name of chrID.*/
+if (!objID || STRNEQ(chrID + 4, objID + 3)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(chardev id '%s' basic alias name is different from 
'%s',
+   chrID, objID));
+goto cleanup;
+}


I made a big mistake here, I found a better way :

+case VIR_DOMAIN_RNG_BACKEND_EGD:
+if (STRNEQ_NULLABLE(strstr(chrID, rng), strstr(objID, rng))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(chardev id '%s' basic alias name is 
different from '%s',

+   chrID, objID));
+goto cleanup;
+}
+type = rng-egd;
+if (!(props = virJSONValueObjectCreate(s:chardev, chrID, NULL)))
+goto cleanup;
+break;
+

i remove the check of chrID, because this should be done in the function 
which call qemuMonitorJSONAttachRNGDev.

+type = rng-egd;
+if (!(props = virJSONValueObjectCreate(s:chardev, chrID, NULL)))
+goto cleanup;
+break;
+
+case VIR_DOMAIN_RNG_BACKEND_LAST:
+/*shouldn't happen*/
+goto cleanup;
+}
+
+return qemuMonitorJSONAddObject(mon, type, objID, props);
+
+ cleanup:
+virJSONValueFree(props);
+return -1;
+}
+
  
  int

  qemuMonitorJSONGetDeviceAliases(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 222f11e..66c519d 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -459,6 +459,11 @@ int qemuMonitorJSONAttachCharDev(qemuMonitorPtr mon,
   virDomainChrSourceDefPtr chr);
  int qemuMonitorJSONDetachCharDev(qemuMonitorPtr mon,
   const char *chrID);
+int qemuMonitorJSONAttachRNGDev(qemuMonitorPtr mon,
+const char *chrID,
+const char *objID,
+virDomainRNGDefPtr rng);
+
  
  int qemuMonitorJSONGetDeviceAliases(qemuMonitorPtr mon,

  char ***aliases);


Luyao

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

Re: [libvirt] [PATCH 02/12] qemu: rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr and change something

[lhuang]

On 01/07/2015 01:16 AM, Eric Blake wrote:

On 01/05/2015 11:56 PM, lhuang wrote:

On 01/05/2015 11:18 PM, Peter Krempa wrote:

Subject: change something? That's a really vague statement.

How about:

qemu: refactor qemuBuildRNGDeviceArgs to allow reuse in RNG hotplug

Good subject :)
Thanks

On 01/03/15 06:06, Luyao Huang wrote:

rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr, we need this
function
to build a cmdline.

Rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr and change the
return type so that it can be reused in the device hotplug code later.


Thanks

Signed-off-by: Luyao Huang lhu...@redhat.com

Another hint for more efficient email: It's a bit hard to pick out your
replies from the quoted text via a visual scan, once another layer of
quoting is added.  A good rule of thumb to follow is to include a blank
line before and after every section of your new text that you
intersperse when replying to quoted text.


Thanks for your advise.
I add a blank line this time (also next times),
I think it will be more easy to find my reply.

Luyao

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

Re: [libvirt] [PATCH] conf: fix cannot get mutli value settings when parse controllers XML

[lhuang]

On 01/07/2015 05:54 PM, Peter Krempa wrote:

On 01/07/15 10:36, Luyao Huang wrote:

We will free the old parameter value settings in next loop when we
get scsi controller's driver specific options.

This description doesn't really explain what is the actual problem you
are fixing.

Please be more specific and as this is in the XML parsing code it should
be fairly easy to do a test case for the scenario.



OK, i will give a better description in next version.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/domain_conf.c | 16 +---
  1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b9858cd..f7b4a7c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6870,11 +6870,21 @@ virDomainControllerDefParseXML(xmlNodePtr node,
  
  cur = node-children;

  while (cur != NULL) {
+char *queues2 = NULL;
+char *cmd_per_lun2 = NULL;
+char *max_sectors2 = NULL;
+
  if (cur-type == XML_ELEMENT_NODE) {
  if (xmlStrEqual(cur-name, BAD_CAST driver)) {
-queues = virXMLPropString(cur, queues);
-cmd_per_lun = virXMLPropString(cur, cmd_per_lun);
-max_sectors = virXMLPropString(cur, max_sectors);
+queues2 = virXMLPropString(cur, queues);
+if (queues2)
+queues = queues2;
+cmd_per_lun2 = virXMLPropString(cur, cmd_per_lun);
+if (cmd_per_lun2)
+cmd_per_lun = cmd_per_lun2;
+max_sectors2 = virXMLPropString(cur, max_sectors);
+if (max_sectors2)
+max_sectors = max_sectors2;
  }
  }
  cur = cur-next;


Without a proper explanation it's hard to see if the fix is right.

Peter


I found another way to fix this issue, please ignore this patch and i 
will give another one.



Luyao

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

Re: [libvirt] [PATCH] lxc: fix show the wrong xml when guest start failed

[lhuang]

On 01/06/2015 11:14 PM, Laine Stump wrote:

On 01/06/2015 09:31 AM, Luyao Huang wrote:

On 01/06/2015 10:11 PM, Michal Privoznik wrote:

On 22.12.2014 08:21, Luyao Huang wrote:

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

When guest start failed, libvirt will keep the current vm-def,
this will make a issue that we cannot get a right xml after guest
start failed. Pass the newDef to def will make it work well.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
   src/lxc/lxc_process.c | 10 ++
   1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 1c0d4e5..b7171ac 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -1353,10 +1353,6 @@ int virLXCProcessStart(virConnectPtr conn,
   VIR_FREE(veths[i]);
   }
   if (rc != 0) {
-if (vm-newDef) {
-virDomainDefFree(vm-newDef);
-vm-newDef = NULL;
-}
   if (priv-monitor) {
   virObjectUnref(priv-monitor);
   priv-monitor = NULL;
@@ -1373,6 +1369,12 @@ int virLXCProcessStart(virConnectPtr conn,
   VIR_FREE(vm-def-seclabels[0]-label);
VIR_FREE(vm-def-seclabels[0]-imagelabel);
   }
+if (vm-newDef) {
+virDomainDefFree(vm-def);
+vm-def = vm-newDef;
+vm-def-id = -1;
+vm-newDef = NULL;
+}
   }
   for (i = 0; i  nttyFDs; i++)
   VIR_FORCE_CLOSE(ttyFDs[i]);


Shouldn't this be in virLXCProcessStop() like it is in other drivers,
e.g. qemu?


These code is already in virLXCProcessStop(), but if we meet some
issue and do 'goto cleanup' in
virLXCProcessStart, we won't call virLXCProcessStop.

Maybe we can merge what we do in cleanup to virLXCProcessStop() ?

I haven't looked in detail, but I'm guessing there are likely other
things done in virLXCProcessCleanup() that should be done in certain
cases of a failed virLXCProcessStart(), but aren't. One example is that
the lxc hook is called three times during virLXCProcessStart() (prepare,
start, and started), and really should be called again with stopped
and/or release if the start fails (since the start hooks may be
allocating resources).


Thanks for your review and i noticed this function before, but i forgot
the stop/release hook should be called in this place.

So I think maybe call virLXCProcessCleanup() in this place is better than
free the source one by one, or just call virLXCProcessStop() in the cleanup
and merge 'goto error' to 'goto cleanup'?(just like what we do in qemu 
drivers)


I found if we use virLXCProcessCleanup() in this place, the left things need
free is security labels (which have been done in virLXCProcessStop()). 
And another way
is call virLXCProcessStop() (maybe need change some thing in 
virLXCProcessStop

to suit this ).

Luyao


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

Re: [libvirt] [PATCH v2 07/12] qemu: add a functions for attach a rng object in json monitor

[lhuang]

On 01/13/2015 10:35 AM, Zhu Guihua wrote:

Hi Luyao,


Hi Guihua,

On Mon, 2015-01-12 at 09:38 +0800, lhuang wrote:

On 01/10/2015 11:29 AM, Luyao Huang wrote:

We need a new function to build a RNG device object.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
   src/qemu/qemu_monitor_json.c | 46 

   src/qemu/qemu_monitor_json.h |  5 +
   2 files changed, 51 insertions(+)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e567aa7..598b15d 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6166,6 +6166,52 @@ qemuMonitorJSONDetachCharDev(qemuMonitorPtr mon,
   return ret;
   }
   
+int

+qemuMonitorJSONAttachRNGDev(qemuMonitorPtr mon,
+const char *chrID,
+const char *objID,
+virDomainRNGDefPtr rng)
+{
+const char *type = NULL;
+virJSONValuePtr props = NULL;
+
+switch ((virDomainRNGBackend) rng-backend) {
+case VIR_DOMAIN_RNG_BACKEND_RANDOM:
+type = rng-random;
+if (!(props = virJSONValueObjectCreate(s:filename, rng-source.file, 
NULL)))
+goto cleanup;
+break;
+
+case VIR_DOMAIN_RNG_BACKEND_EGD:
+if (!chrID) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(miss chardev id));
+goto cleanup;
+}
+/*chrID + 4 and objID + 3 pointing to the basic alias name of chrID.*/
+if (!objID || STRNEQ(chrID + 4, objID + 3)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(chardev id '%s' basic alias name is different from 
'%s',
+   chrID, objID));
+goto cleanup;
+}

I made a big mistake here, I found a better way :

+case VIR_DOMAIN_RNG_BACKEND_EGD:
+if (STRNEQ_NULLABLE(strstr(chrID, rng), strstr(objID, rng))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(chardev id '%s' basic alias name is
different from '%s',
+   chrID, objID));
+goto cleanup;
+}
+type = rng-egd;
+if (!(props = virJSONValueObjectCreate(s:chardev, chrID, NULL)))

Are you sure the above line could pass the compilation?
The first parameter of virJSONValueObjectCreate() should be
'struct virJSONValue **'.

Regards,
Zhu


Oh, my god! thanks your pointing out :)

Maybe this will work? I cannot test it now ,i will double check these 
code this night


+const char *type = NULL;
+virJSONValuePtr props;
+
+if (!(props = virJSONValueNewObject()))
+goto cleanup;
+
+switch ((virDomainRNGBackend) rng-backend) {
+case VIR_DOMAIN_RNG_BACKEND_RANDOM:
+type = rng-random;
+if ((virJSONValueObjectCreate(props, s:filename, 
rng-source.file, NULL))  0)

+goto cleanup;
+break;
+
+case VIR_DOMAIN_RNG_BACKEND_EGD:
+if (STRNEQ_NULLABLE(strstr(chrID, rng), strstr(objID, rng))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(chardev id '%s' basic alias name is 
different from '%s',

+   chrID, objID));
+goto cleanup;
+}
+type = rng-egd;
+if ((virJSONValueObjectCreate(props, s:chardev, chrID, 
NULL))  0)

+goto cleanup;
+break;


+goto cleanup;
+break;
+

i remove the check of chrID, because this should be done in the function
which call qemuMonitorJSONAttachRNGDev.

+type = rng-egd;
+if (!(props = virJSONValueObjectCreate(s:chardev, chrID, NULL)))
+goto cleanup;
+break;
+
+case VIR_DOMAIN_RNG_BACKEND_LAST:
+/*shouldn't happen*/
+goto cleanup;
+}
+
+return qemuMonitorJSONAddObject(mon, type, objID, props);
+
+ cleanup:
+virJSONValueFree(props);
+return -1;
+}
+
   
   int

   qemuMonitorJSONGetDeviceAliases(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 222f11e..66c519d 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -459,6 +459,11 @@ int qemuMonitorJSONAttachCharDev(qemuMonitorPtr mon,
virDomainChrSourceDefPtr chr);
   int qemuMonitorJSONDetachCharDev(qemuMonitorPtr mon,
const char *chrID);
+int qemuMonitorJSONAttachRNGDev(qemuMonitorPtr mon,
+const char *chrID,
+const char *objID,
+virDomainRNGDefPtr rng);
+
   
   int qemuMonitorJSONGetDeviceAliases(qemuMonitorPtr mon,

   char ***aliases);

Luyao

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


Thanks,
Luyao




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

Re: [libvirt] [PATCH] conf: fix crash when hotplug a channel chr device with no target

[lhuang]

On 01/13/2015 05:34 PM, Ján Tomko wrote:

On 01/13/2015 09:41 AM, Luyao Huang wrote:

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

when we try to hotplug a channel chr device with no target, we
will get success(which should fail) in virDomainChrDefParseXML,
because we use goto cleanup this place and return def.then cause
a big problem in virDomainChrEquals(touch a shouldn't happend place).

The problem is that ChrEquals matches according to the target name, so after
we add the device to the domain definition, we cannot remove it after failure,
leaving a stale pointer there.


yes, thanks a lot for improving the explanation.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/domain_conf.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

ACK

I have added the explanation to the commit message and pushed the patch.

Jan



Luyao

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

Re: [libvirt] [PATCH] qemu: fix crash in migrate when migrateuri do not have a scheme

[lhuang]

On 02/11/2015 04:19 PM, Ján Tomko wrote:

On Wed, Feb 11, 2015 at 03:50:37PM +0800, Shanzhi Yu wrote:

On 02/11/2015 03:41 PM, Luyao Huang wrote:

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

When we migrate a vm with migrateuri option with a uri do not
have scheme like this:

   # virsh migrate test4 --live qemu+ssh://lhuang/system --migrateuri 127.0.0.1

target libvirtd will crashed because uri-scheme is NULL in
qemuMigrationPrepareDirect this line:

   if (STRNEQ(uri-scheme, tcp) 

There is a similar check in doNativeMigrate:

if (!(uribits = qemuMigrationParseURI(uri, NULL)))
 return -1;

 if (STREQ(uribits-scheme, rdma)) {
 if (!virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_MIGRATE_RDMA)) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED, %s,
_(outgoing RDMA migration is not supported 
  with this QEMU binary));

It seems the scheme can be NULL here only if Prepare on the remote side
returned a wrong URI, It would still be nice not to crash in that case.



Thanks for pointing out and i will send a v2 later.

add a value check before this line.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
   src/qemu/qemu_migration.c | 7 +++
   1 file changed, 7 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 879b1bf..5c3b73e 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3281,6 +3281,13 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
   if (!(uri = qemuMigrationParseURI(uri_in, well_formed_uri)))
   goto cleanup;
   
+if (uri-scheme == NULL) {

+virReportError(VIR_ERR_INVALID_ARG,
+   _(missing scheme in migration URI: %s),
+   uri_in);
+goto cleanup;
+}
+
   if (STRNEQ(uri-scheme, tcp) 
   STRNEQ(uri-scheme, rdma)) {

Why not just use STRNEQ_NULLABLE instead of STRNEQ directly?


It would report 'unsupported scheme (null) in migration URI:',
instead of saying that the scheme is missing.

Jan


Luyao

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

Re: [libvirt] [PATCH] conf: Don't use the current state in def-data.network.actual when migrate

[lhuang]

On 01/07/2015 08:16 PM, Jiri Denemark wrote:

On Thu, Dec 25, 2014 at 11:38:00 +0800, Luyao Huang wrote:

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

When migrate a vm, we will generate a xml via qemuDomainDefFormatLive and
pass this xml to target libvirtd. Libvirt will use the current network
state in def-data.network.actual to generate the xml, this will make
migrate failed when we set a network type guest interface use a macvtap
network as a source in a vm then migrate vm to another host(which has the
different macvtap network settings: different interface name, bridge name...)

Add a flag check in virDomainNetDefFormat, if we set a VIR_DOMAIN_XML_MIGRATABLE
flag when call virDomainNetDefFormat, we won't get the current vm interface
state.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/domain_conf.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index aafc05e..fffd6cd 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17436,7 +17436,9 @@ virDomainNetDefFormat(virBufferPtr buf,
  unsigned int actualType = virDomainNetGetActualType(def);
  bool publicActual
 = (def-type == VIR_DOMAIN_NET_TYPE_NETWORK  def-data.network.actual 

-  !(flags  (VIR_DOMAIN_XML_INACTIVE | 
VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET)));
+  !(flags  (VIR_DOMAIN_XML_INACTIVE |
+ VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET |
+ VIR_DOMAIN_XML_MIGRATABLE)));
  const char *typeStr;
  virDomainHostdevDefPtr hostdef = NULL;
  char macstr[VIR_MAC_STRING_BUFLEN];

ACK, however the initialization of publicActual looks ugly (not your
fault) so a changed it a bit before pushing.


Thanks for your  review!

Jirka

@@ -17705,18 +17705,23 @@ virDomainNetDefFormat(virBufferPtr buf,
virDomainNetDefPtr def,
unsigned int flags)
  {
-/* publicActual is true if we should report the current state in
- * def-data.network.actual *instead of* the config (*not* in
- * addition to)
- */
  unsigned int actualType = virDomainNetGetActualType(def);
-bool publicActual
-   = (def-type == VIR_DOMAIN_NET_TYPE_NETWORK  def-data.network.actual 

-  !(flags  (VIR_DOMAIN_XML_INACTIVE | 
VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET)));
+bool publicActual = false;
  const char *typeStr;
  virDomainHostdevDefPtr hostdef = NULL;
  char macstr[VIR_MAC_STRING_BUFLEN];
  
+/* publicActual is true if we should report the current state in

+ * def-data.network.actual *instead of* the config (*not* in
+ * addition to)
+ */
+if (def-type == VIR_DOMAIN_NET_TYPE_NETWORK 
+def-data.network.actual 
+!(flags  (VIR_DOMAIN_XML_INACTIVE |
+   VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET |
+   VIR_DOMAIN_XML_MIGRATABLE)))
+publicActual = true;
+
  if (publicActual) {
  if (!(typeStr = virDomainNetTypeToString(actualType))) {
  virReportError(VIR_ERR_INTERNAL_ERROR,


yep, the codes looks beautiful now, thanks your help!

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

Re: [libvirt] [PATCH] conf: fix use the wrong type for period

[lhuang]

On 03/13/2015 10:38 PM, Martin Kletzander wrote:

On Fri, Mar 13, 2015 at 05:15:32PM +0800, Luyao Huang wrote:

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

When we set period as unsigned int max value 4294967295 and
start the vm, qemu will report error. This becuase we define period
as a unsigned int and parse it as a unsigned int, but we use it as
a int when set it via QMP in qemuMonitorJSONSetMemoryStatsPeriod,
so 4294967295 turn to -1 when we send the QMP command.

After check the qemu's code this value type should be int, and found
a qemu commit 1f9296b for this values range.

Seems no other hypervisor vm use this so i add a check when we parse it.



Where to start.  NACK to this patch as is.  Domains that have
INT_MAX  period  UINT_MAX will disappear after libvirt is restarted
and that's not backwards compatible.



Okay, thanks for pointing out this.


I couldn't make sense of the commit message, but at least the aim is


Hmm...my English need Improvement :)


visible from the code.  Anyway, I believe Erik already worked on this
issue as I gave him few hints regarding this on one of his patches.
Did you talk together about it?  It would be a pity if the work was
blindly doubled.



Oh, Sorry, i didn't noticed this, i should talk with Erik first before 
wrote this patch.



Looking at this commit message I made a diff to squash in, so I'll
post a v2 with it.


I saw your patches, good idea to avoid vm which have big period settings 
loss track after update libvirtd, thanks a lot for your review.


Luyao




Signed-off-by: Luyao Huang lhu...@redhat.com
---
src/conf/domain_conf.c | 3 ++-
src/conf/domain_conf.h | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 127fc91..54bd5aa 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10452,7 +10452,8 @@ virDomainMemballoonDefParseXML(xmlNodePtr node,
}

ctxt-node = node;
-if (virXPathUInt(string(./stats/@period), ctxt, def-period) 
 -1) {
+if (virXPathInt(string(./stats/@period), ctxt, def-period)  
-1 ||

+def-period  0) {
virReportError(VIR_ERR_XML_ERROR, %s,
   _(invalid statistics collection period));
goto error;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ea463cb..ee0f5fd 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1556,7 +1556,7 @@ enum {
struct _virDomainMemballoonDef {
int model;
virDomainDeviceInfo info;
-unsigned int period; /* seconds between collections */
+int period; /* seconds between collections */
};

struct _virDomainNVRAMDef {
--
1.8.3.1

--
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] qemu: fix sometimes error overwrite when we fail to start/migrate/restore

[lhuang]

On 03/19/2015 05:27 PM, Ján Tomko wrote:

On Thu, Mar 19, 2015 at 11:14:39AM +0800, Luyao Huang wrote:

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


Adding a comment in the bugzilla saying you posted a patch (with a link
to libvir-list archives) can be helpful. Otherwise they might send the
exact same patch - if your was not merged yet, or it was rejected.


got it, I will add a comment next time and thank a lot for your help to 
add them before.



When start/migrate/restore a vm failed, libvirt will try to catch the log in

Libvirt only uses the error from qemu if qemu exits during the startup.
Start/migrate/restore could fail for other reasons.


/var/log/libvirt/qemu/vm.log and output them before. However we add a check in
qemuDomainObjExitMonitor after commit dc2fd51f, this will overwrite
the error set in priv-mon which has set by qemuMonitorIO (a qemu monitor
I/O event callback function).

qemuMonitorIO only stores the error in mon-lastError.
It's the functions like qemuMonitorSend and qemuMonitorClose that take
this error and set it via virSetError.


Oh, I see.


Add a check in qemuDomainObjExitMonitor, if there is an error have been
set by other function, we won't overwrite it.

Reworded as:
 When qemu exits during startup, libvirt includes the error from
 /var/log/libvirt/qemu/vm.log in the error message:
 
 $ virsh start test3

 error: Failed to start domain test3
 error: internal error: early end of file from monitor: possible problem:
 2015-02-27T03:03:16.985494Z qemu-kvm: -numa memdev is not supported by
 machine rhel6.5.0
 
 The check for domain liveness added to qemuDomainObjExitMonitor

 in commit dc2fd51f sometimes overwrites this error:
 $ virsh start test3
 error: Failed to start domain test3
 error: operation failed: domain is no longer running
 
 Fix the check to only report an error if there is none set.


Thanks a lot for the reword.


Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_domain.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)


ACK and pushed.

Jan


Thanks for your review.


diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2eacef2..41d1263 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1609,8 +1609,9 @@ int qemuDomainObjExitMonitor(virQEMUDriverPtr driver,
  {
  qemuDomainObjExitMonitorInternal(driver, obj);
  if (!virDomainObjIsActive(obj)) {
-virReportError(VIR_ERR_OPERATION_FAILED, %s,
-   _(domain is no longer running));
+if (!virGetLastError())
+virReportError(VIR_ERR_OPERATION_FAILED, %s,
+   _(domain is no longer running));
  return -1;
  }
  return 0;
--
1.8.3.1

--
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] [PATCHv2 1/4] util: introduce 2 new helpers for get interface IPv6 address

[lhuang]

On 03/10/2015 07:44 AM, John Ferlan wrote:


On 03/09/2015 03:35 AM, lhuang wrote:
...snip...


I think once you have an address you just return the first one found and
document it that way avoiding this path completely. You could also note
that if you want a specific address use the type='address'


Hmm, yes, I agree it is not a good idea to report error when we get
multiple addresses in this function (In some case, caller just want one
ip address and not care about which one we chose), so remove the check
and error output here is reasonable (maybe we can use a DEBUG or WARNING
here complain about this and set ret to 0).

However this check and error output is a result from last review, i
think maybe we can move them to a right place (should in another patch).
Because we use listen type='network' for migration in the most case, and
if a host interface has multiple address than we always chose the first
one, this will make things worse in some case. An example:

In host A, we have a happy vm which use listen type='network' and use a
spice client to connect to it (listen address is
fe80::7ae3:b5ff:fec5:189e%enp1s0 and address is get from an interface
via virNetDevGetIfaddrsAddress() ), than we migrate this vm to a another
host B, we found a network have the same name and use a host interface
in host B, we use virNetDevGetIfaddrsAddress() get the first ipv6
address and we get 2001:db8:ca2:2::1/64, unfortunately this address is
not a good address (the good one is the second one :-P ) and spice
connection will be broken, and the user will say : why libvirt chose a
wrong address and broke my connection :-(.

NB: the comment from Laine in this mail:
https://www.redhat.com/archives/libvir-list/2015-February/msg01080.html


Right and as Laine points out:

... Because it's impossible to do it right, I say we shouldn't even
try; it would be worse for it to work halfway.

Doing it right thus requires proper configuration rather than
premonition by libvirt.  I think a follow up patch could describe the
migration conundrum where if the plan is to migrate the vm, then the
target node will have to have a valid IPv{4|6} address as the first
address; otherwise, the connection will be broken.

Migration is a tricky beast. I recall doing something for a former
project regarding this kind of issue, but I cannot remember the details.
I do remember though our logic filtered out a few address types before
returning what should be a usable address. I also recall some sort of
nasty ARP test, but the details were specific to the HP-UX world I used
to live in.


yes, migration part need more patch and more intelligence :)




...snip...


+int virNetDevGetIPAddress(const char *ifname,
+  bool want_ipv6,
+  virSocketAddrPtr addr)

same


+{
+int ret;
+
+memset(addr, 0, sizeof(*addr));
+addr-data.stor.ss_family = AF_UNSPEC;
+
+ret = virNetDevGetIfaddrsAddress(ifname, want_ipv6, addr);
+if (ret != -2)
+return ret;
+
+if (!want_ipv6)
+return virNetDevGetIPv4Address(ifname, addr);

Here if we have virNetDevGetIPv4Address() return -2, then we can take
our message above and place it right here while also adjusting the !
SIOCGIFADDR to just return -2.


+
+return -1;
+}

NOTE: This does not return -2 in any

should be return -2 (and this only happen when want_ipv6 is true and the
ret is -2)


Hmm... if we return -2, then the caller's caller spits out

network-based listen not possible, network driver not present

rather than our message... Which is less than helpful.


right, i forgot this

I still have to process your follow-up email with a patch in it, but my
guess is I'm going to repost the patches I have so that we're on the
same page.


Thanks a lot for your help !


John



Luyao

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

Re: [libvirt] [PATCHv2 1/4] util: introduce 2 new helpers for get interface IPv6 address

[lhuang]

On 03/10/2015 07:50 AM, John Ferlan wrote:


On 03/09/2015 11:37 AM, Luyao Huang wrote:

On 03/09/2015 07:50 AM, John Ferlan wrote:

On 02/28/2015 04:08 AM, Luyao Huang wrote:

Introduce virNetDevGetIfaddrsAddress to help to get interface IPv6
and IPv4 address.

Introduce virNetDevGetIPAddress to use virNetDevGetIfaddrsAddress
and virNetDevGetIPv4Address to get IP address.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
v2: add a new function virNetDevGetIPAddress to call
virNetDevGetIfaddrsAddress
, and make virNetDevGetIfaddrsAddress can get ipv4 address for a
interface.
Also add a error output in virNetDevGetIfaddrsAddress when get
multiple ip
address for a host interface.

   src/libvirt_private.syms |  1 +
   src/util/virnetdev.c | 98

   src/util/virnetdev.h |  4 ++
   3 files changed, 103 insertions(+)


...

+if (ifa-ifa_addr-sa_family == (want_ipv6 ? AF_INET6 :
AF_INET)) {
+if (++nIPaddr  1)
+break;

[1]... hmm not sure about this...


+if (want_ipv6) {
+addr-len = sizeof(addr-data.inet6);
+memcpy(addr-data.inet6, ifa-ifa_addr, addr-len);
+} else {
+addr-len = sizeof(addr-data.inet4);
+memcpy(addr-data.inet4, ifa-ifa_addr, addr-len);
+}
+addr-data.stor.ss_family = ifa-ifa_addr-sa_family;
+}
+}
+
+if (nIPaddr == 1)
+ret = 0;
+else if (nIPaddr  1)
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _(Interface '%s' has multiple %s address),

s/address/addresses


+   ifname, want_ipv6 ? IPv6 : IPv4);

[1]
But is this really desired... It seems from the previous review, if
someone wants a specific address they use listen type='address'

Otherwise, they use this function.  Since you'll be returning either an
IPv4 or IPv6 address here you'd be causing an error here if the network
had more than one IPv4 address defined; whereas, the previous code just
returned the first from the ioctl(SIOCGIFADDR...).

I think once you have an address you just return the first one found and
document it that way avoiding this path completely. You could also note
that if you want a specific address use the type='address'



I had an idea but my eyes almost close ;)  so i write here without test
them.

I think it will be better if we make this function to get mutli address
and return the number of address we get, like this:

+static int
+virNetDevGetIfaddrsAddress(const char *ifname,
+   bool want_ipv6,
+   virSocketAddrPtr *addrs)

We'd need to have an naddrs to know how many we can stuff in... or you'd
need to do the *REALLOC on the fly in this module for each address found.


Yes, i forgot this and please ignore the code, it has so many mistake 
(so it is not a good idea to write a patch when you want sleep:-( )



Interesting idea, but you'd be just throwing things away.  I could
perhaps having some code periodically cache addresses... or cache
addresses, subscribe to some event to let you know when a new address is
generated so you can add it to your list (if there is one)...

But, how do you use it?


Sorry, i don't know what these words' (how do you use it ?) mean.

I guess your mean is ask me how to use the code or function you 
mentioned, i think maybe

we can avoid to use it :)

However this should be another patch which add a function to get a list 
of ip address.



John


Luyao

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

Re: [libvirt] [PATCHv2 3/4] network: rework networkGetNetworkAddress to make it can get IPv6 address

[lhuang]

On 03/09/2015 07:50 AM, John Ferlan wrote:


$SUBJ:

network: Allow networkGetNetworkAddress to return IPv6 address

On 02/28/2015 04:08 AM, Luyao Huang wrote:

Export the required helpers and rework networkGetNetworkAddress to
make it can get IPv6 address.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
v2: rework the code and make networkGetNetworkAddress
call virNetDevGetIPAddress instead of virNetDevGetIPv4Address.

  src/network/bridge_driver.c | 15 ---
  src/network/bridge_driver.h |  6 --
  src/qemu/qemu_command.c |  8 ++--
  3 files changed, 18 insertions(+), 11 deletions(-)


This one I believe had some merge conflicts with recent upstream changes..


Yes..



diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 1209609..5aeb168 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4524,6 +4524,7 @@ networkReleaseActualDevice(virDomainDefPtr dom,
   * networkGetNetworkAddress:
   * @netname: the name of a network
   * @netaddr: string representation of IP address for that network.
+ * @want_ipv6: if true will get IPv6 address, false for IPv4.
   *
   * Attempt to return an IP (v4) address associated with the named

s/(v4)/(v4 or v6)


   * network. If a libvirt virtual network, that will be provided in the
@@ -4540,12 +4541,12 @@ networkReleaseActualDevice(virDomainDefPtr dom,
   * completely unsupported.
   */
  int
-networkGetNetworkAddress(const char *netname, char **netaddr)
+networkGetNetworkAddress(const char *netname, char **netaddr, bool want_ipv6)
  {
  int ret = -1;
  virNetworkObjPtr network;
  virNetworkDefPtr netdef;
-virNetworkIpDefPtr ipdef;
+virNetworkIpDefPtr ipdef = NULL;

This is unecessary since virNetworkDefGetIpByIndex returns NULL if not
found.

Other than that the rest seems OK.


Thanks for your review.

John


  virSocketAddr addr;
  virSocketAddrPtr addrptr = NULL;
  char *dev_name = NULL;
@@ -4566,12 +4567,12 @@ networkGetNetworkAddress(const char *netname, char 
**netaddr)
  case VIR_NETWORK_FORWARD_NONE:
  case VIR_NETWORK_FORWARD_NAT:
  case VIR_NETWORK_FORWARD_ROUTE:
-/* if there's an ipv4def, get it's address */
-ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0);
+/* if there's an ipdef, get it's IPv4 or IPv6 address */
+ipdef = virNetworkDefGetIpByIndex(netdef, want_ipv6 ? AF_INET6 : 
AF_INET, 0);
  if (!ipdef) {
  virReportError(VIR_ERR_INTERNAL_ERROR,
-   _(network '%s' doesn't have an IPv4 address),
-   netdef-name);
+   _(network '%s' doesn't have an '%s' address),
+   netdef-name, want_ipv6 ? IPv6 : IPv4);
  break;
  }
  addrptr = ipdef-address;
@@ -4599,7 +4600,7 @@ networkGetNetworkAddress(const char *netname, char 
**netaddr)
  }
  
  if (dev_name) {

-if (virNetDevGetIPv4Address(dev_name, addr)  0)
+if (virNetDevGetIPAddress(dev_name, want_ipv6, addr)  0)
  goto error;
  addrptr = addr;
  }
diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
index 2f801ee..465ab18 100644
--- a/src/network/bridge_driver.h
+++ b/src/network/bridge_driver.h
@@ -44,7 +44,9 @@ int networkReleaseActualDevice(virDomainDefPtr dom,
 virDomainNetDefPtr iface)
  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
  
-int networkGetNetworkAddress(const char *netname, char **netaddr)

+int networkGetNetworkAddress(const char *netname,
+ char **netaddr,
+ bool want_ipv6)
  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
  
  int networkDnsmasqConfContents(virNetworkObjPtr network,

@@ -57,7 +59,7 @@ int networkDnsmasqConfContents(virNetworkObjPtr network,
  #  define networkAllocateActualDevice(dom, iface) 0
  #  define networkNotifyActualDevice(dom, iface) (dom=dom, iface=iface, 0)
  #  define networkReleaseActualDevice(dom, iface) (dom=dom, iface=iface, 0)
-#  define networkGetNetworkAddress(netname, netaddr) (-2)
+#  define networkGetNetworkAddress(netname, netaddr, want_ipv6) (-2)
  #  define networkDnsmasqConfContents(network, pidfile, configstr, \
  dctx, caps) 0
  # endif
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 24b2ad9..82a4ce3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7286,7 +7286,9 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr 
cfg,
  listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0);
  if (!listenNetwork)
  break;
-ret = networkGetNetworkAddress(listenNetwork, netAddr);
+ret = networkGetNetworkAddress(listenNetwork, netAddr,
+   graphics-listens-family ==
+   

Re: [libvirt] [PATCHv2 4/4] qemu: fix a error coverd issue in 2 place

[lhuang]

On 03/09/2015 07:51 AM, John Ferlan wrote:


On 02/28/2015 04:08 AM, Luyao Huang wrote:

we already set a more clearly error in networkGetNetworkAddress,
so this error will cover our error in networkGetNetworkAddress.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_command.c | 14 --
  1 file changed, 4 insertions(+), 10 deletions(-)


Change comment to:

Remove unnecessary virReportError on networkGetNetworkAddress return

More specific errors are set in the called functions when -1 is
returned, so don't overwrite those messages.

Code seems fine and I traced functions as well.


Thanks for your review and help.

New comment looks good for me :)

John


diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 82a4ce3..cd0758d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7295,12 +7295,9 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr 
cfg,
 network driver not present));
  goto error;
  }
-if (ret  0) {
-virReportError(VIR_ERR_XML_ERROR,
-   _(listen network '%s' had no usable address),
-   listenNetwork);
+if (ret  0)
  goto error;
-}
+
  listenAddr = netAddr;
  /* store the address we found in the graphics element so it will
   * show up in status. */
@@ -7461,12 +7458,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr 
cfg,
 network driver not present));
  goto error;
  }
-if (ret  0) {
-virReportError(VIR_ERR_XML_ERROR,
-   _(listen network '%s' had no usable address),
-   listenNetwork);
+if (ret  0)
  goto error;
-}
+
  listenAddr = netAddr;
  /* store the address we found in the graphics element so it will
   * show up in status. */



Luyao

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

Re: [libvirt] [PATCHv2 2/4] conf: introduce new family attribute in graphics listen for chose IP family

[lhuang]

On 03/09/2015 07:50 AM, John Ferlan wrote:


On 02/28/2015 04:08 AM, Luyao Huang wrote:

If a interface or network have both ipv6 and ipv4 address which can be used,
we do not know use which be a listen address. So introduce a new attribute
to help us chose this.

graphics XML will like this after this commit:

 graphics type='spice' port='5900' autoport='yes'
   listen type='network' address='192.168.0.1' network='vepa-net' 
family='ipv4'/
 /graphics

and this ip family can be set 2 type, and the default one is ipv4:

ipv4: check if the interface or network have a can be used ipv4 address
ipv6: check if the interface or network have a can be used ipv6 address

fix some test which will be break by this commit and add a new test.


Adjusting the commit text slightly to match the review below


Signed-off-by: Luyao Huang lhu...@redhat.com
---
v2: remove family='default' and add a test.

  docs/formatdomain.html.in  | 10 ++-
  docs/schemas/domaincommon.rng  |  8 +
  src/conf/domain_conf.c | 20 +
  src/conf/domain_conf.h |  9 ++
  .../qemuxml2argv-graphics-listen-network-ipv6.xml  | 35 ++
  .../qemuxml2argv-graphics-listen-network.xml   |  2 +-
  .../qemuxml2xmlout-graphics-listen-network2.xml|  2 +-
  tests/qemuxml2xmltest.c|  1 +
  8 files changed, 84 insertions(+), 3 deletions(-)
  create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index fb0a0d1..e8ea6fa 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4545,7 +4545,7 @@ qemu-kvm -net nic,model=? /dev/null
  lt;graphics type='rdp' autoport='yes' multiUser='yes' /gt;
  lt;graphics type='desktop' fullscreen='yes'/gt;
  lt;graphics type='spice'gt;
-  lt;listen type='network' network='rednet'/gt;
+  lt;listen type='network' network='rednet' family='ipv4'/gt;
  lt;/graphicsgt;
lt;/devicesgt;
.../pre
@@ -4785,6 +4785,14 @@ qemu-kvm -net nic,model=? /dev/null
  the first forward dev will be used.
/dd
  /dl
+dl
+  dtcodefamily/code/dt
+  ddif codetype='network'/code, the codefamily/code
+attribute will contain an IP family. This tells which IP address
+will be got for the network. IP family can be set to ipv4
+or ipv6.

Adjusted to:

   ddif codetype='network'/code, the codefamily/code
 attribute may contain the IP family. The codefamily/code
 can be set to either codeipv4/code or codeipv6/code.
 This advises the graphics device which IP address family
 to use as listen address for the network. The listen address
 used will be the first found address of the codefamily/code
 type defined for the host.
 span class=sinceSince 1.2.14/span



Looks good to me.

+  /dd
+/dl
  
  h4a name=elementsVideoVideo devices/a/h4

  p
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4e4fe9f..e84b213 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2909,6 +2909,14 @@
  ref name=addrIPorName/
/attribute
  /optional
+optional
+  attribute name=family
+choice
+  valueipv4/value
+  valueipv6/value
+/choice
+  /attribute
+/optional
/group
  /choice
/element
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9b7ae3f..97f1250 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -522,6 +522,11 @@ VIR_ENUM_IMPL(virDomainGraphicsListen, 
VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST,
address,
network)
  
+VIR_ENUM_IMPL(virDomainGraphicsListenFamily,

+  VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST,
+  ipv4,
+  ipv6)
+

Need to add a none... As with the previous review - adding an
attribute that's optional, but then generating it on output isn't good.
So we need a way to signify it didn't exist on input. If provided that's
fine, we can output it.  If not provided, then sure we're going to
eventually default to IPv4, but that's why I said to have the boolean be
if ipv6 desired...


yes, thanks for pointing out, add a none is better than auto generate 
it to ipv4.


Hmm, use family='ipv4|ipv6' in this place , another reason is : maybe 
there will be a new ip family in the future (ipv7? :-P ), so use ip 
family in this place will be a good choice in that day ( seems so far 
away ;) )



  VIR_ENUM_IMPL(virDomainGraphicsAuthConnected,
VIR_DOMAIN_GRAPHICS_AUTH_CONNECTED_LAST,
default,
@@ -9464,6 +9469,7 @@ 

Re: [libvirt] [PATCHv2 1/4] util: introduce 2 new helpers for get interface IPv6 address

[lhuang]

On 03/09/2015 07:50 AM, John Ferlan wrote:

On 02/28/2015 04:08 AM, Luyao Huang wrote:

Introduce virNetDevGetIfaddrsAddress to help to get interface IPv6
and IPv4 address.

Introduce virNetDevGetIPAddress to use virNetDevGetIfaddrsAddress
and virNetDevGetIPv4Address to get IP address.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
v2: add a new function virNetDevGetIPAddress to call virNetDevGetIfaddrsAddress
, and make virNetDevGetIfaddrsAddress can get ipv4 address for a interface.
Also add a error output in virNetDevGetIfaddrsAddress when get multiple ip
address for a host interface.

  src/libvirt_private.syms |  1 +
  src/util/virnetdev.c | 98 
  src/util/virnetdev.h |  4 ++
  3 files changed, 103 insertions(+)


Closer...  But still missing a couple of things, which I can add for
you. I'll make my comments and do the changes locally but not commit
until Mon afternoon (Eastern US) so if Laine wants to comment he can and
of course that you agree with my comments...


Thanks in advance for your help

Going to split this commit into two -

The first commit will just make the virNetDevIPAddress shim, moving the
virNetDevIPv4Address to a static function.

The second commit will add the IPv6 option to virNetDevIPAddress



No problem

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ba05cc6..f88626a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1670,6 +1670,7 @@ virNetDevClearIPAddress;
  virNetDevDelMulti;
  virNetDevExists;
  virNetDevGetIndex;
+virNetDevGetIPAddress;
  virNetDevGetIPv4Address;

We can remove the GetIPv4Address and make it static to virtnetdev.c


Yes

  virNetDevGetLinkInfo;
  virNetDevGetMAC;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 2a0eb43..318c3b6 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -33,6 +33,10 @@
  #include virstring.h
  #include virutil.h
  
+#if defined(HAVE_GETIFADDRS)

+# include ifaddrs.h
+#endif
+
  #include sys/ioctl.h
  #include net/if.h
  #include fcntl.h
@@ -1432,6 +1436,100 @@ int virNetDevGetIPv4Address(const char *ifname 
ATTRIBUTE_UNUSED,
  
  #endif /* ! SIOCGIFADDR */
  
+/**

+ * virNetDevGetIfaddrsAddress:
+ * @ifname: name of the interface whose IP address we want
+ * @want_ipv6: get IPv4 or IPv6 address
+ * @addr: filled with the IP address
+ *
+ * This function gets the IP address for the interface @ifname
+ * and stores it in @addr
+ *
+ * Returns 0 on success, -1 on failure, -2 on unsupported.
+ */
+#if defined(HAVE_GETIFADDRS)
+static int virNetDevGetIfaddrsAddress(const char *ifname,
+  bool want_ipv6,
+  virSocketAddrPtr addr)

Although not a rule - we try to make newer API's as:

static int
fname(param1,
   param2)



oh, i should noticed this

+{
+struct ifaddrs *ifap, *ifa;
+int ret = -1;
+int nIPaddr = 0;
+
+if (getifaddrs(ifap)  0) {
+virReportSystemError(errno,
+ _(Could not get interface list for '%s'),
+ ifname);
+return -1;
+}
+
+for (ifa = ifap; ifa; ifa = ifa-ifa_next) {
+if (!ifa-ifa_addr ||
+STRNEQ(ifa-ifa_name, ifname)) {
+continue;
+}

STRNEQ_NULLABLE does the trick...


+if (ifa-ifa_addr-sa_family == (want_ipv6 ? AF_INET6 : AF_INET)) {
+if (++nIPaddr  1)
+break;

[1]... hmm not sure about this...


+if (want_ipv6) {
+addr-len = sizeof(addr-data.inet6);
+memcpy(addr-data.inet6, ifa-ifa_addr, addr-len);
+} else {
+addr-len = sizeof(addr-data.inet4);
+memcpy(addr-data.inet4, ifa-ifa_addr, addr-len);
+}
+addr-data.stor.ss_family = ifa-ifa_addr-sa_family;
+}
+}
+
+if (nIPaddr == 1)
+ret = 0;
+else if (nIPaddr  1)
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _(Interface '%s' has multiple %s address),

s/address/addresses


+   ifname, want_ipv6 ? IPv6 : IPv4);

[1]
But is this really desired... It seems from the previous review, if
someone wants a specific address they use listen type='address'

Otherwise, they use this function.  Since you'll be returning either an
IPv4 or IPv6 address here you'd be causing an error here if the network
had more than one IPv4 address defined; whereas, the previous code just
returned the first from the ioctl(SIOCGIFADDR...).

I think once you have an address you just return the first one found and
document it that way avoiding this path completely. You could also note
that if you want a specific address use the type='address'



Hmm, yes, I agree it is not a good idea to report error when we get 
multiple addresses in this function (In some case, caller just want one 
ip address and not care about which one we chose), so 

Re: [libvirt] [PATCH 4/6] qemu: check if domain is really active when do setvcpus with --live

[lhuang]

On 03/24/2015 05:31 PM, Pavel Hrdina wrote:

On Tue, Mar 24, 2015 at 02:27:42PM +0800, lhuang wrote:

On 03/20/2015 10:39 PM, Pavel Hrdina wrote:

From: Luyao Huang lhu...@redhat.com

Commit e105dc9 fix setting vcpus for offline domain, but forget check
if vm is active when pass VIR_DOMAIN_AFFECT_LIVE flags.

   # virsh setvcpus test3 4 --live
   error: Failed to create controller cpu for group: No such file or directory

Add a check if we pass VIR_DOMAIN_AFFECT_LIVE flags.

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

Signed-off-by: Luyao Huang lhu...@redhat.com
Signed-off-by: Pavel Hrdina phrd...@redhat.com
---
   src/qemu/qemu_driver.c | 14 --
   1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4942712..c4d96bd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4881,6 +4881,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int 
nvcpus,
   if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY)  0)
   goto cleanup;
   
+if (flags  VIR_DOMAIN_AFFECT_LIVE) {

+if (!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID, %s,
+   _(domain is not running));
+goto endjob;
+}
+}
+
   if (flags  VIR_DOMAIN_AFFECT_LIVE  !(flags  VIR_DOMAIN_VCPU_GUEST)) {
   if (virCgroupNewEmulator(priv-cgroup, false, cgroup_temp)  0)
   goto endjob;
@@ -4947,12 +4955,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int 
nvcpus,
   if (ncpuinfo  0)
   goto endjob;
   
-if (!virDomainObjIsActive(vm)) {

-virReportError(VIR_ERR_OPERATION_INVALID, %s,
-   _(domain is not running));
-goto endjob;
-}
-
   if (qemuAgentUpdateCPUInfo(nvcpus, cpuinfo, ncpuinfo)  0)
   goto endjob;

Sorry, i forgot there is a function virDomainLiveConfigHelperMethod(),
move this function just after qemuDomainObjBeginJob() maybe a good way
to fix this issue. Also virDomainLiveConfigHelperMethod() may change
flags, so it should be done more early (as soon as possible after set a
lock to vm).

I've already sent a patch 7/6 to move that function.  I realized that right
after I've sent this series to mailing list.


Pavel


Okay, I think i missed patch 7/6 when i looked these patches :)

Thanks for your reply

   

Luyao


Luyao

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

Re: [libvirt] [PATCH] qemu: fix can use setmaxmem to change the maximum memory biger than max memory

[lhuang]

On 03/25/2015 04:48 PM, Peter Krempa wrote:

On Tue, Mar 24, 2015 at 22:12:37 +0800, Luyao Huang wrote:

When call qemuDomainSetMemoryFlags() to change maximum memory size, we
do not check if the maximum memory is biger than the max memory, this will
make vm disappear after libvirtd restart if we set a big maximum memory.

Add a check for this, also fix a typos issue.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/domain_conf.c | 2 +-
  src/qemu/qemu_driver.c | 7 +++
  2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d633f93..0d4b165 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16646,7 +16646,7 @@ virDomainDefCheckABIStability(virDomainDefPtr src,
  
  if (src-mem.memory_slots != dst-mem.memory_slots) {

  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _(Target domain memory slots count '%u' doesn't match 
source '%u),
+   _(Target domain memory slots count '%u' doesn't match 
source '%u'),
 dst-mem.memory_slots, src-mem.memory_slots);
  goto error;
  }

The hunk above is not at all related to the code change below and should
be separate. I'll split the patch for you.



Thanks a lot for your help.


diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3518dec..86b87af 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2324,6 +2324,13 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, 
unsigned long newmem,
   nodes cannot be modified with this API));
  goto endjob;
  }
+if (persistentDef-mem.max_memory 
+persistentDef-mem.max_memory  newmem) {
+virReportError(VIR_ERR_OPERATION_INVALID, %s,
+   _(cannot set maximum memory size biger than 

higher than or greather than. Bigger can't be used in this context.


+ the max memory size));
+goto endjob;
+}

Technically this code requires that the VM does not have numa configured
but has the memory hotplug code enabled. While you can set such
configuration and use the qemuDomainSetMemoryFlags() API on it you won't
be able to actually use that as QEMU requires NUMA to be enabled.


Yes, it just a corner issue.



Strictly speaking, this patch makes sense as it allows to induce a
invalid configuration that would vapourize the guest.

ACK. I'll split the patch and tweak the error message before pushing.


Thanks for your review.



Peter


Luyao

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

Re: [libvirt] [PATCH] qemu: fix cannot start a vm with memory device with address

[lhuang]

On 03/26/2015 04:24 PM, Peter Krempa wrote:

On Thu, Mar 26, 2015 at 14:30:56 +0800, Luyao Huang wrote:

When start a vm which have a memory device with address, the error
like this :

error: Failed to start domain test3
error: internal error: early end of file from monitor: possible problem:
2015-03-26T03:45:52.338891Z qemu-kvm: -device pc-dimm,node=0,memdev=memdimm0,
id=dimm0,slot=0,base=4294967296: Property '.base' not found

After check the qemu code i think this 'base' should named as 'addr',
you can see this mail:

http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg00129.html

Or check the include/hw/mem/pc-dimm.h in qemu source.
Also add a tests for this.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
I didn't change this parameter's name ('base'), because i am not sure
if there is some special mean in it, or it is a result after
discuss.

You are right, the base address has to be formatted as addr.


  src/qemu/qemu_command.c|  2 +-
  .../qemuxml2argv-memory-hotplug-dimm-addr.args |  9 +
  .../qemuxml2argv-memory-hotplug-dimm-addr.xml  | 45 ++
  tests/qemuxml2argvtest.c   |  2 +
  4 files changed, 57 insertions(+), 1 deletion(-)
  create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args
  create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.xml

I've reworded the commit message and pushed this patch.


Thanks for your quick review.


Peter


Luyao

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

Re: [libvirt] [PATCH] qemu: add a value check for granularity

[lhuang]

On 04/01/2015 04:12 PM, Michal Privoznik wrote:

On 27.03.2015 10:56, Luyao Huang wrote:

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

From our manual of virsh and qemu side code, we know this
value must be power of 2, so instead of let qemu output error,
we can add a check when we file this value in qemuDomainBlockCopy.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_driver.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2c55fb0..6d63317 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16871,6 +16871,11 @@ qemuDomainBlockCopy(virDomainPtr dom, const char 
*disk, const char *destxml,
  }
  bandwidth = param-value.ul;
  } else if (STREQ(param-field, VIR_DOMAIN_BLOCK_COPY_GRANULARITY)) {
+if (param-value.ui != VIR_ROUND_UP_POWER_OF_TWO(param-value.ui)) 
{
+virReportError(VIR_ERR_INVALID_ARG, %s,
+   _(granularity must be power of 2));
+goto cleanup;
+}
  granularity = param-value.ui;
  } else if (STREQ(param-field, VIR_DOMAIN_BLOCK_COPY_BUF_SIZE)) {
  buf_size = param-value.ul;


In fact, the virsh man page is not as precise as it could be either:

diff --git a/tools/virsh.pod b/tools/virsh.pod
index 63325ff..5d52761 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1039,10 +1039,10 @@ unlimited, but more likely would overflow; it is safer 
to use 0 for that
purpose.  Specifying Igranularity allows fine-tuning of the granularity that
will be copied when a dirty region is detected; larger values trigger less
I/O overhead but may end up copying more data overall (the default value is
usually correct); {+hypervisors may restrict+} this [-value must-]{+to+} be a 
power of [-two.-]{+two or fall+}
{+within a certain range.+} Specifying Ibuf-size will control how much data 
can
be simultaneously in-flight during the copy; larger values use more memory but
may allow faster completion (the default value is usually correct).

=item Bblockpull Idomain Ipath [Ibandwidth] [Ibase]
[I--wait [I--verbose] [I--timeout Bseconds] [I--async]]

I'm fixing the man page too, rewording the commit message a bit and ACKing. 
Will push after the release.


Oh, thanks a lot for your remind and your fix for man page looks good 
for me.


Thanks for your review.


Michal


Luyao

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

Re: [libvirt] [PATCH] tools: fix the wrong check when use virsh setvcpus --maximum

[lhuang]

On 03/20/2015 11:01 PM, Pavel Hrdina wrote:

On Fri, Mar 20, 2015 at 04:22:24PM +0800, Luyao Huang wrote:

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

We will ignore --maximum option when only use setvcpus with
this option, like this (this error is another issue):

  # virsh setvcpus test3 --maximum 10
  error: Failed to create controller cpu for group: No such file or directory

this is because we do not set it in flags before we check if there is
a flags set.

Refactor these code and fix the logic.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  tools/virsh-domain.c | 33 -
  1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1d8225c..6ab7b05 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6742,9 +6742,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  flags |= VIR_DOMAIN_AFFECT_LIVE;
  if (guest)
  flags |= VIR_DOMAIN_VCPU_GUEST;
-/* none of the options were specified */
-if (!current  flags == 0)
-flags = -1;
+if (maximum)
+flags |= VIR_DOMAIN_VCPU_MAXIMUM;
  
  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))

  return false;
@@ -6754,27 +6753,19 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  goto cleanup;
  }
  
-if (flags == -1) {

+/* none of the options were specified */
+if (!current  flags == 0) {
  if (virDomainSetVcpus(dom, count) != 0)
  goto cleanup;
  } else {
-/* If the --maximum flag was given, we need to ensure only the
-   --config flag is in effect as well */
-if (maximum) {
-vshDebug(ctl, VSH_ERR_DEBUG, --maximum flag was given\n);
-
-flags |= VIR_DOMAIN_VCPU_MAXIMUM;
-
-/* If neither the --config nor --live flags were given, OR
-   if just the --live flag was given, we need to error out
-   warning the user that the --maximum flag can only be used
-   with the --config flag */
-if (live || !config) {
-
-/* Warn the user about the invalid flag combination */
-vshError(ctl, _(--maximum must be used with --config only));
-goto cleanup;
-}
+/* If neither the --config nor --live flags were given, OR
+   if just the --live flag was given, we need to error out
+   warning the user that the --maximum flag can only be used
+   with the --config flag */
+if (maximum  (live || !config)) {
+/* Warn the user about the invalid flag combination */
+vshError(ctl, _(--maximum must be used with --config only));
+goto cleanup;

Instead of this ugly code you could've used VSH_EXCLUSIVE_OPTIONS_VAR to set
that maximum and live are mutually exclusive.  I've updated your patch and send
it together with some other cleanups.

See https://www.redhat.com/archives/libvir-list/2015-March/msg01073.html.


Okay, i have saw your patches, it is a good idea and will make codes 
clean, thanks for your review.



Pavel




Luyao

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

Re: [libvirt] [PATCH] qemu: check if domain is really active when do setvcpus with --live

[lhuang]

On 03/20/2015 10:52 PM, Pavel Hrdina wrote:

On Fri, Mar 20, 2015 at 03:07:09PM +0800, Luyao Huang wrote:

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

Commit e105dc9 fix setting vcpus for offline domain, but forget check
if vm is active when pass VIR_DOMAIN_AFFECT_LIVE flags.

  # virsh setvcpus test3 4 --live
  error: Failed to create controller cpu for group: No such file or directory

add a check if we pass VIR_DOMAIN_AFFECT_LIVE flags.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_driver.c | 6 ++
  1 file changed, 6 insertions(+)


I've updated the patch to error out for every case we are trying to update live
domain either by using VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CURRENT.

See https://www.redhat.com/archives/libvir-list/2015-March/msg01072.html.


Good, thanks for your help and review.


Pavel


Luyao

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

Re: [libvirt] [PATCH] docs: add a mention for start a vm with rawio = 'yes'

[lhuang]

On 03/02/2015 06:43 PM, Daniel P. Berrange wrote:

On Mon, Mar 02, 2015 at 06:04:44PM +0800, Luyao Huang wrote:

When we start a vm which have rawio = 'yes' settings without
any file caps settings for qemu, qemu process still cannot use
this caps (CAP_SYS_RAWIO) and the /proc/pidofqemu/status like
this:

   CapInh: 0002
   CapPrm: 
   CapEff: 
   CapBnd: 001f

this is because we do not set file caps for qemu (see man 7
capabilities), although laine have mentioned this in commit
e11451, i think it will be good if we add this in docs.

This is only true if you are starting the guest under the
qemu:///session URI. In such a case I think it is expected
that the QEMU lacks rawio capabilities, because the whole
point of qemu:///session is that the VM has no elevated
privileges.

In the case of qemu:///system libvirt should ensure that
it does the right thing with passing on raw io capability
flag. If it does not, then we must fix that in the code,
not the docs.


Hmm, what i show is the test result in qemu:///system, and we already 
set the right cap flag before we do execv() or execve(), however we run 
qemu process in qemu(107) not root(0) in most case, so only set this cap 
flags cannot make qemu to use this flag, because from capabilities(7):


   Transformation of capabilities during execve()
   During an execve(2), the kernel calculates the new capabilities 
of the process using the following algorithm:


   P'(permitted) = (P(inheritable)  F(inheritable)) |
   (F(permitted)  cap_bset)

   P'(effective) = F(effective) ? P'(permitted) : 0

   P'(inheritable) = P(inheritable)[i.e., unchanged]

   where:

   P denotes the value of a thread capability set 
before the execve(2)


   P'denotes the value of a capability set after the 
execve(2)


   F denotes a file capability set

   cap_bset  is the value of the capability bounding set 
(described below).


So if not set any file cap to qemu program (/usr/libexec/qemu-kvm), the 
qemu process will get this cap flags:


Uid:107107107107
Gid:107107107107
...
CapInh:0002
CapPrm:
CapEff:
CapBnd:001f

and qemu process do not have this cap as the CapEff is for kernel do 
permission check.


I think libvirt already do the right things here although running qemu 
process do not have rawio capability
flag in this case, because i think it is not a good idea for libvirt to 
set file cap to qemu program, libvirt is not the only user which use or 
call qemu program, set a file cap to qemu program will affect other 
callers (although set a small file cap will not be a big deal :) ), so i 
guess maybe it is good to make the users to set this instead of libvirt 
use cap_set_file() to do this.


BTW, if we make qemu process run with root(0) uid and gid, the cap flags 
will like this:

...
Uid:0000
Gid:0000
...
CapInh:0002
CapPrm:0002
CapEff:0002
CapBnd:0002



Regards,
Daniel


Thanks,
Luyao

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

Re: [libvirt] [PATCH] qemu: fix not remove the pidfile when close a vm after restart libvirtd

[lhuang]

On 03/03/2015 06:57 PM, Michal Privoznik wrote:

On 02.03.2015 10:37, Luyao Huang wrote:

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

when create a happy vm and then restart libvirtd, we will loss
priv-pidfile, because we don't check if it is there is a pidfile.
However we only use this pidfile when we start the vm, and won't use
it after it start, so this is not a big deal.

But it is strange when vm is offline but pidfile still exist, so
remove vmname.pid in state dir (maybe /run/libvirt/qemu/)when
priv-pidfile is NULL.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_process.c | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 515402e..46b93b3 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -92,6 +92,7 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver,
  {
  char ebuf[1024];
  char *file = NULL;
+char *pidfile = NULL;
  qemuDomainObjPrivatePtr priv = vm-privateData;
  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
  int ret = -1;
@@ -99,16 +100,19 @@ qemuProcessRemoveDomainStatus(virQEMUDriverPtr driver,
  if (virAsprintf(file, %s/%s.xml, cfg-stateDir, vm-def-name)  0)
  goto cleanup;
  
+if (virAsprintf(pidfile, %s/%s.pid, cfg-stateDir, vm-def-name)  0)

+goto cleanup;

If this fails, @file is leaked. And there's a helper function to
generate pid file path: virPidFileBuildPath(). I know it does exactly
the same, but lets use that, so whenever we decide on changing the path,
we need to change it at one place only, instead of digging through
source code just to check if somebody has not used virAsprintf() directly.


Yes, i forgot check if file will be leaked, thanks for pointing out that.
About the virPidFileBuildPath(), i should say i missed this function 
from the code :) and use virPidFileBuildPath() is better than 
virAsprintf() in every sense.

+
  if (unlink(file)  0  errno != ENOENT  errno != ENOTDIR)
  VIR_WARN(Failed to remove domain XML for %s: %s,
   vm-def-name, virStrerror(errno, ebuf, sizeof(ebuf)));
  VIR_FREE(file);
  
-if (priv-pidfile 

-unlink(priv-pidfile)  0 
+if (unlink(priv-pidfile ? priv-pidfile : pidfile)  0 
  errno != ENOENT)
  VIR_WARN(Failed to remove PID file for %s: %s,
   vm-def-name, virStrerror(errno, ebuf, sizeof(ebuf)));
+VIR_FREE(pidfile);
  
  ret = 0;

   cleanup:


While this works, I think we need a different approach:

https://www.redhat.com/archives/libvir-list/2015-March/msg00047.html


Good approach :)

Michal


Luyao

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

Re: [libvirt] [PATCH 1/4] util: introduce a new helper for get interface IPv6 address

[lhuang]

On 02/25/2015 11:55 PM, Laine Stump wrote:

On 02/25/2015 04:50 AM, lhuang wrote:


And i also thought about another issue after your reminding: An
interface can have more than one IPv6 address. But i still couldn't
find a good way until now to chose which IPv6 address if we find more
than one IPv6 address in one interface (maybe users should use listen
type=address instead of listen type=network in this case ;)).

There are some special addresses anddefault address selection for IPv6
address, i don't know if it is a good idea to guess which address is
the caller really desired if we get some IPv6 address in one interface.

If they are using listen type='network', then either they will have
taken care to have the network only have a single IP, or they won't
care. Otherwise, as you say, they should use listen type='address'.

BTW, some history for listen type='network' - this feature was added to
allow a way to remove the explicit listen address from the domain
config, thus allowing the domain to be migrated from one host to
another, as long as there is a network with the same name on both hosts.
Often the network used for this is created only for this purpose. If the
physical interface named in the network has multiple IP addresses, there
really isn't a good way for libvirt to indicate preference for any one
of the addresses over another (you can't specify it with an index,
because the index of the desired IP address may be different between the
hosts. Because it's impossible to do it right, I say we shouldn't even
try; it would be worse for it to work halfway.


After i read some doc and self check , i agree with your opinion: output 
error instead of chose one of them.


i found it is not a good idea for libvirt to chose use which IP address 
if the hosts interface have multiple IP address (both ipv6 and ipv4), 
because if the caller won't want this spice open to public, he will want 
to use fe80 address in this scene. However if the caller want to use a 
address for public, he will want to use 2XXX

address.

Thanks a lot for your help.




Luyao

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

Re: [libvirt] [PATCH] conf: fix not jump to cleanup when parse a host id is invalid

[lhuang]

On 02/26/2015 03:56 PM, Ján Tomko wrote:

On Thu, Feb 26, 2015 at 02:14:20PM +0800, Luyao Huang wrote:

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

We already check whether the host id is valid or not, add a jump
to forbid invalid host id.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/network_conf.c | 1 +
  1 file changed, 1 insertion(+)

ACK


Thanks for your quick review.

Introduced by v1.0.3-rc1~7, I will push it to v1.1.3-maint and
v1.2.9-maint as well.

Jan


Luyao

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

Re: [libvirt] [PATCH 1/4] util: introduce a new helper for get interface IPv6 address

[lhuang]

On 02/25/2015 11:55 PM, Laine Stump wrote:

On 02/25/2015 04:50 AM, lhuang wrote:


And i also thought about another issue after your reminding: An
interface can have more than one IPv6 address. But i still couldn't
find a good way until now to chose which IPv6 address if we find more
than one IPv6 address in one interface (maybe users should use listen
type=address instead of listen type=network in this case ;)).

There are some special addresses anddefault address selection for IPv6
address, i don't know if it is a good idea to guess which address is
the caller really desired if we get some IPv6 address in one interface.

If they are using listen type='network', then either they will have
taken care to have the network only have a single IP, or they won't
care. Otherwise, as you say, they should use listen type='address'.


Yes, this is what i thought at the first time, thanks for your help.

BTW, some history for listen type='network' - this feature was added to
allow a way to remove the explicit listen address from the domain
config, thus allowing the domain to be migrated from one host to
another, as long as there is a network with the same name on both hosts.
Often the network used for this is created only for this purpose. If the
physical interface named in the network has multiple IP addresses, there
really isn't a good way for libvirt to indicate preference for any one
of the addresses over another (you can't specify it with an index,
because the index of the desired IP address may be different between the
hosts. Because it's impossible to do it right, I say we shouldn't even
try; it would be worse for it to work halfway.



Okay, got it, so maybe report error if host physical interface has 
multiple IP addresses will be a good choice in this scene, but i found a 
host physical interface will have 2 Ipv6 address, one is 2001...or 
2002... for public and another fe80... address for internal.


just like this:
inet6 2001:db8:ca2:2::1/64 scope global
   valid_lft forever preferred_lft forever
inet6 fe80::5054:ff:fe7f:f047/64 scope link
   valid_lft forever preferred_lft forever

i think maybe we can use 2001... ipv6 address instead of output error in 
this scene ?



Luyao

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

Re: [libvirt] [PATCH 1/4] util: introduce a new helper for get interface IPv6 address

[lhuang]

On 02/25/2015 12:12 AM, John Ferlan wrote:


On 02/13/2015 02:17 AM, Luyao Huang wrote:

Introduce a new function to help to get interface IPv6 address.


s/Introduce a new function to help to/Introduce virNetDevGetIPv6Address/



Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/libvirt_private.syms |  1 +
  src/util/virnetdev.c | 70 
  src/util/virnetdev.h |  2 ++
  3 files changed, 73 insertions(+)


Hmm... maybe rather than introducing a new IPv6 specific routine and
forcing the caller to handle the logic of knowing how/whether to return
an IPv4 or IPv6 address...

Why not change the existing GetIPv4Address into a shim
virNetDevGetIPAddress which then makes the decisions regarding returning
only one family or allowing a failed fetch of IPv4 to use the IPv6
routine...

This way you hide the details.  Your first patch would just change the
IPv4 into an GetIPAddress API and that would just call the now
local/static IPv4 API. You won't have a #if #else #endif for the new API
- it would return 0 or -1.

Check out how safezero has multiple ways in order to zero out a file
based on what's present. I suspect your new API could follow that
methodology.

In the long run since getifaddrs() can return an IPv4 or IPv6 address it
could be theoretically called first. If it returns nothing, fall back to
the IPv4 API

Your new API could be something like:

virNetDevGetIPAddress(const char *ifname,
   bool want_ipv6,
   virSocketAddrPtr addr)

{
 int ret;

 memset(addr, 0, sizeof(*addr));
 addr-data.stor.ss_family = AF_UNSPEC;

 ret = virNetDevGetIfaddrsAddress(ifname, want_ipv6, addr);
 if (ret != -2)
 return ret;

 if (!want_ipv6)
 return virNetDevGetIPv4Address(ifname, addr);

 return -1;
}


The virNetDevGetIfaddrsAddress would follow safezero_posix_fallocate
returning -2 in the #else of #if defined(HAVE_GETIFADDRS).  The logic in
the function would return the first found address of IPv6 if that's
desired or IPv4 otherwise.


Good idea! I just thought add a new functions for ipv6 address but 
forgot getifaddrs() can also get the IPv4 address :)


Thanks for pointing out and i will rework this patch in next version.

The virNetDevGetIPv4Address() wouldn't need the two stolen lines to
clear addr, but would otherwise function as it does today.

Hopefully this makes sense - you'll be adding more patches, but I think
in the long run based on the following patches it will make it easier
on the caller to just get an address and force it to be IPv6 only if
that's what's desired.


Yes, i had thought about this before, maybe we can add a new function to 
get(or chose) the IPv6 address we desired, because one interface can 
have many IPv6 address and sometimes we just need one of them.

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 645aef1..f60911c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1659,6 +1659,7 @@ virNetDevDelMulti;
  virNetDevExists;
  virNetDevGetIndex;
  virNetDevGetIPv4Address;
+virNetDevGetIPv6Address;
  virNetDevGetLinkInfo;
  virNetDevGetMAC;
  virNetDevGetMTU;



diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 2a0eb43..c1a588e 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -33,6 +33,10 @@
  #include virstring.h
  #include virutil.h
  
+#if defined(HAVE_GETIFADDRS)

+# include ifaddrs.h
+#endif
+
  #include sys/ioctl.h
  #include net/if.h
  #include fcntl.h
@@ -1432,6 +1436,72 @@ int virNetDevGetIPv4Address(const char *ifname 
ATTRIBUTE_UNUSED,
  
  #endif /* ! SIOCGIFADDR */
  
+/**

+ *virNetDevGetIPv6Address:
+ *@ifname: name of the interface whose IP address we want

s/IP/IPv6


thanks, but seems this function will be removed (or renamed) in next 
version :)

+ *@addr: filled with the IPv6 address
+ *
+ *This function gets the IPv6 address for the interface @ifname
+ *and stores it in @addr
+ *
+ *Returns 0 on success, -1 on failure.
+ */

Each of the lines above needs s/*/* /


Sorry for my careless.

+#if defined(HAVE_GETIFADDRS)
+int virNetDevGetIPv6Address(const char *ifname,
+virSocketAddrPtr addr)
+{
+struct ifaddrs *ifap, *ifa;
+int ret = -1;
+bool found_one = false;
+
+if (getifaddrs(ifap)  0) {
+virReportSystemError(errno, %s,
+ _(Could not get interface list));

s/list$/list for '%s'/  and of course an ifname parameter ...


Hmm, seems it is not the interface issue when getifaddrs cannot get 
interface list, however it will give a more nice error. Thanks your 
advise and i will change it in next version.

+return -1;
+}
+
+for (ifa = ifap; ifa; ifa = ifa-ifa_next) {
+if (STRNEQ(ifa-ifa_name, ifname))
+continue;
+found_one = true;
+if (ifa-ifa_addr-sa_family == AF_INET6)
+break;

 From the getifaddrs(3) man page:

The 

Re: [libvirt] [PATCH 1/4] util: introduce a new helper for get interface IPv6 address

[lhuang]

On 02/25/2015 01:42 AM, John Ferlan wrote:


On 02/24/2015 11:12 AM, John Ferlan wrote:

...snip...

As I'm reading the next patch I'm thinking I forgot something in the
previous one...

The default has been IPv4 and would seem to be the right thing to
return once we find an address; however, if we wanted an IPv6 address
and returned an IPv4 one, that wouldn't be good...



Thanks your review and help and i have changed some place for this part, 
please see the email i replied before(just now).

Leaving the following - including the capability to get either IPv6 or
IPv4 address depending upon what's desired/required:

 for (ifa = ifap; ifa; ifa = ifa-ifa_next) {
 if (!ifa-ifa_addr ||
 STRNEQ(ifa-ifa_name, ifname))
 continue;
 found_one = true;
 if (want_ipv6  ifa-ifa_addr-sa_family != AF_INET6)
 continue;

 /* Found an address to return */
 addr-data.stor.ss_family = ifa-ifa_addr-sa_family;
 if (ifa-ifa_addr-sa_family == AF_INET6)
 addr-len = sizeof(addr-data.inet6);
 memcpy(addr-data.inet6, ifa-ifa_addr, addr-len);
 } else {

} else if (!want_ipv6  ifa-ifa_addr-sa_family == AF_INET) {


 addr-len = sizeof(addr-data.inet4);
 memcpy(addr-data.inet4, ifa-ifa_addr, addr-len);
 }
 ret = 0;
 goto cleanup;
 }



Luyao

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

Re: [libvirt] [PATCH 3/4] network: rework networkGetNetworkAddress to make it can get IPv6 address

[lhuang]

On 02/25/2015 02:22 AM, John Ferlan wrote:


On 02/13/2015 02:17 AM, Luyao Huang wrote:

Export the required helpers and rework networkGetNetworkAddress to
make it can get IPv6 address.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/network_conf.c |  2 +-
  src/conf/network_conf.h |  1 +
  src/libvirt_private.syms|  1 +
  src/network/bridge_driver.c | 50 -
  src/network/bridge_driver.h |  6 --
  src/qemu/qemu_command.c |  4 ++--
  6 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index f947d89..9eb640b 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -148,7 +148,7 @@ virNetworkDHCPHostDefClear(virNetworkDHCPHostDefPtr def)
  VIR_FREE(def-name);
  }
  
-static void

+void

I believe this is unnecessary


  virNetworkIpDefClear(virNetworkIpDefPtr def)
  {
  VIR_FREE(def-family);
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 484522e..0c4b559 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -310,6 +310,7 @@ virNetworkObjPtr virNetworkFindByName(virNetworkObjListPtr 
nets,
  void virNetworkDefFree(virNetworkDefPtr def);
  void virNetworkObjFree(virNetworkObjPtr net);
  void virNetworkObjListFree(virNetworkObjListPtr vms);
+void virNetworkIpDefClear(virNetworkIpDefPtr def);

Same unnecessary
  
  
  typedef bool (*virNetworkObjListFilter)(virConnectPtr conn,

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f60911c..ff942d8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -559,6 +559,7 @@ virNetworkDeleteConfig;
  virNetworkFindByName;
  virNetworkFindByUUID;
  virNetworkForwardTypeToString;
+virNetworkIpDefClear;

Unnecessary


Yes, thanks for pointing out these place, i forgot check the code in 
virNetworkDefGetIpByIndex.



  virNetworkIpDefNetmask;
  virNetworkIpDefPrefix;
  virNetworkLoadAllConfigs;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 2798010..d1afd16 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4487,6 +4487,7 @@ networkReleaseActualDevice(virDomainDefPtr dom,
   * networkGetNetworkAddress:
   * @netname: the name of a network
   * @netaddr: string representation of IP address for that network.
+ * @family: IP family

@want_ipv6: boolean to force return of IPv6 address for that network


   *
   * Attempt to return an IP (v4) address associated with the named
   * network. If a libvirt virtual network, that will be provided in the
@@ -4503,12 +4504,13 @@ networkReleaseActualDevice(virDomainDefPtr dom,
   * completely unsupported.
   */
  int
-networkGetNetworkAddress(const char *netname, char **netaddr)
+networkGetNetworkAddress(const char *netname, char **netaddr, int family)

s/int family/bool want_ipv6/


  {
  int ret = -1;
  virNetworkObjPtr network;
  virNetworkDefPtr netdef;
-virNetworkIpDefPtr ipdef;
+virNetworkIpDefPtr ipdef = NULL;
+virNetworkIpDefPtr ipdef6 = NULL;

The ipdef6 is unnecessary


  virSocketAddr addr;
  virSocketAddrPtr addrptr = NULL;
  char *dev_name = NULL;
@@ -4529,15 +4531,9 @@ networkGetNetworkAddress(const char *netname, char 
**netaddr)
  case VIR_NETWORK_FORWARD_NONE:
  case VIR_NETWORK_FORWARD_NAT:
  case VIR_NETWORK_FORWARD_ROUTE:
-/* if there's an ipv4def, get it's address */
+/* if there's an ipv4def or ipv6def, get it's address */
  ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0);
-if (!ipdef) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _(network '%s' doesn't have an IPv4 address),
-   netdef-name);
-break;
-}
-addrptr = ipdef-address;
+ipdef6 = virNetworkDefGetIpByIndex(netdef, AF_INET6, 0);

Not sure I follow why we're losing the error reporting here...

I'd change this to:


 if (want_ipv6)
 ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET6, 0);
 else
 ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0);
 if (!ipdef) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
   _(network '%s' doesn't have an '%s' address),
   netdef-name, want_ipv6 ? IPv6 : IPv4);
 break;
 }
 addrptr = ipdef-address;

NB: virNetworkDefGetIpByIndex() doesn't return a COPY that we must FREE,
it returns a pointer to something owned elsewhere



Thanks for your advise and i will changed this in next version:

ipdef = virNetworkDefGetIpByIndex(netdef, want_ipv6 ? AF_INET6 
: AF_INET, 0);

if (!ipdef) {
virReportError(VIR_ERR_INTERNAL_ERROR,
   _(network '%s' doesn't have an '%s' address),
   netdef-name, want_ipv6 ? IPv6 : IPv4);
break;
}
addrptr = ipdef-address;


  break;
  
  

Re: [libvirt] [PATCH 4/4] qemu: fix a error coverd issue in 2 place

[lhuang]

On 02/25/2015 02:52 AM, John Ferlan wrote:


On 02/13/2015 02:17 AM, Luyao Huang wrote:

we already set a more clearly error in networkGetNetworkAddress,
so this error will cover our error in networkGetNetworkAddress.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_command.c | 14 --
  1 file changed, 4 insertions(+), 10 deletions(-)


I didn't check every exit patch from networkGetNetworkAddress, but
perhaps these should change to :

 if (ret  0) {
 if (virGetLastError() == NULL)
 virReportError(VIR_ERR_XML_ERROR,...);
 goto error;
 }

Just to be sure we don't leave without setting some sort of error.


Okay, thanks for your advise, i will change them in next version.


John

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6bd8f69..b42e0f4 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7284,12 +7284,9 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr 
cfg,
 network driver not present));
  goto error;
  }
-if (ret  0) {
-virReportError(VIR_ERR_XML_ERROR,
-   _(listen network '%s' had no usable address),
-   listenNetwork);
+if (ret  0)
  goto error;
-}
+
  listenAddr = netAddr;
  /* store the address we found in the graphics element so it will
   * show up in status. */
@@ -7448,12 +7445,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr 
cfg,
 network driver not present));
  goto error;
  }
-if (ret  0) {
-virReportError(VIR_ERR_XML_ERROR,
-   _(listen network '%s' had no usable address),
-   listenNetwork);
+if (ret  0)
  goto error;
-}
+
  listenAddr = netAddr;
  /* store the address we found in the graphics element so it will
   * show up in status. */



Luyao

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

Re: [libvirt] [PATCH 4/4] qemu: fix a error coverd issue in 2 place

[lhuang]

On 02/25/2015 10:51 PM, Ján Tomko wrote:

On Tue, Feb 24, 2015 at 01:52:08PM -0500, John Ferlan wrote:


On 02/13/2015 02:17 AM, Luyao Huang wrote:

we already set a more clearly error in networkGetNetworkAddress,
so this error will cover our error in networkGetNetworkAddress.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_command.c | 14 --
  1 file changed, 4 insertions(+), 10 deletions(-)


I didn't check every exit patch from networkGetNetworkAddress, but
perhaps these should change to :

 if (ret  0) {
 if (virGetLastError() == NULL)
 virReportError(VIR_ERR_XML_ERROR,...);
 goto error;
 }

Just to be sure we don't leave without setting some sort of error.

No.

All the exit paths should be checked to make sure an error was reported
in all cases where we return -1.


Okay, i have checked the code and think we will return -1 with an error 
setting.


So i will keep the code like this:
  if (ret  0)
  goto error;

Thanks for your review

Jan


Luyao

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

Re: [libvirt] [PATCH 4/6] qemu: check if domain is really active when do setvcpus with --live

[lhuang]

On 03/20/2015 10:39 PM, Pavel Hrdina wrote:

From: Luyao Huang lhu...@redhat.com

Commit e105dc9 fix setting vcpus for offline domain, but forget check
if vm is active when pass VIR_DOMAIN_AFFECT_LIVE flags.

  # virsh setvcpus test3 4 --live
  error: Failed to create controller cpu for group: No such file or directory

Add a check if we pass VIR_DOMAIN_AFFECT_LIVE flags.

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

Signed-off-by: Luyao Huang lhu...@redhat.com
Signed-off-by: Pavel Hrdina phrd...@redhat.com
---
  src/qemu/qemu_driver.c | 14 --
  1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4942712..c4d96bd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4881,6 +4881,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int 
nvcpus,
  if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY)  0)
  goto cleanup;
  
+if (flags  VIR_DOMAIN_AFFECT_LIVE) {

+if (!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID, %s,
+   _(domain is not running));
+goto endjob;
+}
+}
+
  if (flags  VIR_DOMAIN_AFFECT_LIVE  !(flags  VIR_DOMAIN_VCPU_GUEST)) {
  if (virCgroupNewEmulator(priv-cgroup, false, cgroup_temp)  0)
  goto endjob;
@@ -4947,12 +4955,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int 
nvcpus,
  if (ncpuinfo  0)
  goto endjob;
  
-if (!virDomainObjIsActive(vm)) {

-virReportError(VIR_ERR_OPERATION_INVALID, %s,
-   _(domain is not running));
-goto endjob;
-}
-
  if (qemuAgentUpdateCPUInfo(nvcpus, cpuinfo, ncpuinfo)  0)
  goto endjob;


Sorry, i forgot there is a function virDomainLiveConfigHelperMethod(), 
move this function just after qemuDomainObjBeginJob() maybe a good way 
to fix this issue. Also virDomainLiveConfigHelperMethod() may change 
flags, so it should be done more early (as soon as possible after set a 
lock to vm).


  


Luyao

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

Re: [libvirt] [PATCH] lxc: fix 2 issue around cpuset

[lhuang]

On 04/20/2015 10:04 PM, Ján Tomko wrote:

On Fri, Apr 03, 2015 at 06:11:15PM +0800, Luyao Huang wrote:

There are two bugs in this function:

1. cannot start a vm with cpuset but without numatune settings

  # virsh -c lxc:/// start helloworld
  error: Failed to start domain helloworld
  error: internal error: guest failed to start: Invalid value '1-3' for 
'cpuset.mems': Invalid argument

we don't free mask after use it for virCgroupSetCpusetCpus() and
then virDomainNumatuneMaybeFormatNodeset() do not get a new mask,
then we use it in virCgroupSetCpusetMems().

2. when start a lxc with numatune memory mode not strict

I think these two fixes would be better pushed separately.


  # virsh -c lxc:/// start helloworld
  error: Failed to start domain helloworld
  error: internal error: guest failed to start: Unknown failure in libvirt_lxc 
startup

We shouldn't set anything in cpuset.mems for these mode.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/lxc/lxc_cgroup.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)


ACK, I've split the commit into two and pushed both.


Thanks a lot for your review and split.



Jan


Luyao

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

Re: [libvirt] [PATCH 0/2] rework the check for the numa cpus

[lhuang]

On 04/20/2015 10:39 PM, Michal Privoznik wrote:

On 02.04.2015 10:02, Luyao Huang wrote:

We introduce a check for numa cpu total count in 5f7b71,
But seems this check cannot work well. There are some scenarioes:

1. one of cpu id is out of maxvcpus, can set success(cpu count = 5  10):

vcpu placement='static'10/vcpu
cell id='0' cpus='0-3,100' memory='512000' unit='KiB'/

the cpus '100' exceed maxvcpus, this setting is not valid (although qemu
do not output error).

2. use the same cpu in 2 cell, can set success(cpu count = 8  10):
vcpu placement='static'10/vcpu
cell id='0' cpus='0-3' memory='512000' unit='KiB'/
cell id='1' cpus='0-3' memory='512000' unit='KiB'/

I guess nobody will use this setting if he really want use numa in his
vm. (qemu not output error, but we can find some interesting things in
vm, all cpus have bind in one numa node)

3. use the same cpu in 2 cell, cannot set success(cpu count = 11  10):
vcpu placement='static'10/vcpu
cell id='0' cpus='0-6' memory='512000' unit='KiB'/
cell id='1' cpus='0-3' memory='512000' unit='KiB'/

No need forbid this scenario if scenario 2 is a 'valid' setting.

However add new check during parse xml will make vm has broken settings
disappear after update libvirtd, so possible solutions:

1. add new check when parse xml, and find a way to avoid vm evaporate.
I chose this way and i don't think just drop the invalid settings is a good
idea for numa cpus, so i add a new function.

2. add new check when start vm.
I think this is a configuration issue, so no need report it so late.

3. just remove this check and do not add new check... :)

Luyao Huang (2):
   conf: introduce a new function to add check avoid losing track
   conf: rework the cpu check for vm numa settings

  src/bhyve/bhyve_driver.c |  4 ++--
  src/conf/domain_conf.c   | 21 ++---
  src/conf/domain_conf.h   |  1 +
  src/conf/numa_conf.c | 37 ++---
  src/conf/numa_conf.h |  2 +-
  src/esx/esx_driver.c |  2 +-
  src/libxl/libxl_driver.c |  4 ++--
  src/lxc/lxc_driver.c |  4 ++--
  src/openvz/openvz_driver.c   |  4 ++--
  src/parallels/parallels_driver.c |  2 +-
  src/phyp/phyp_driver.c   |  2 +-
  src/qemu/qemu_driver.c   |  4 ++--
  src/test/test_driver.c   |  4 ++--
  src/uml/uml_driver.c |  4 ++--
  src/vbox/vbox_common.c   |  2 +-
  src/vmware/vmware_driver.c   |  4 ++--
  src/xen/xen_driver.c |  4 ++--
  src/xenapi/xenapi_driver.c   |  4 ++--
  18 files changed, 70 insertions(+), 39 deletions(-)


I agree with renaming and extending the virDomainNumaGetCPUCountTotal().
Even the diff in 2/2 shows the function is utterly broken. However, I
think this function can be called unconditionally - even when the flag
is not set. Having said that, the flag is redundant.


Oh, good news to me :)

So if this function can be called unconditionally, there is no reason to 
introduce this new flag.




Moreover, when sending a patchset, the sources should compile cleanly
after each patch. This is not the case with this one.


Get it, i will pay more attention for this things next time, thanks for 
pointing out that.



Looking forward to v2.


Thanks for your review and advise, i will propose a new version these days.


Michal


Luyao

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

Re: [libvirt] [PATCH] docs: add dimm address document in docs

[lhuang]

On 05/14/2015 07:57 PM, Peter Krempa wrote:

On Thu, May 14, 2015 at 17:08:53 +0800, Luyao Huang wrote:

As we have a new device address type 'dimm', add
some document and an example to our docs.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  docs/formatdomain.html.in | 8 
  1 file changed, 8 insertions(+)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index e0b6ba7..0e908a1 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2876,6 +2876,13 @@
  attributes: codeiobase/code and codeirq/code.
  span class=sinceSince 1.2.1/span
/dd
+  dtcodetype='dimm'/code/dt
+  ddDIMM addresses, for memory device, have the following additional
+attributes: codeslot/code (a value between 0 and 4294967295,
+inclusive), and codebase/code (a hex value between 0 and
+0x, inclusive).
+span class=sinceSince 1.2.14/span

The values for the DIMM address shouldn't really be documented. I'd
rather state that the values are determined automatically and the user
should not need to touch them.


Oh, i see, i will remove the value for DIMM address and add some mention 
about the values.


Thanks for your quick review.



Peter


Luyao

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

Re: [libvirt] [PATCHv2] virsh: fix no error output when parse cpulist fail

[lhuang]

On 05/14/2015 08:33 PM, Michal Privoznik wrote:

On 11.05.2015 10:25, Luyao Huang wrote:

When we pass a invalid cpulist or the lastcpu in the
cpulist exceed the maxcpu, we cannot get any error.
like this:

  # virsh vcpupin test3 1 aaa

  # virsh vcpupin test3 1 1000

Because virBitmapParse() use virReportError() to set
the error message, vshCommandRun would output the error
in vshReportError, but in the meantime it is overwriten
by the virResetLastError in virDomainFree. If we want use
the error which set by virReportError(), we need vshSaveLibvirtError
to help us. However the error from virBitmap is not clearly
enough, i chose use vshError to output error when parse failed.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
v2:
  Add the check in vshParseCPUList, because this will make
  get last cpu more easier when the cpulist is a bitmap.

  tools/virsh-domain.c | 21 +++--
  1 file changed, 15 insertions(+), 6 deletions(-)

Reworded the commit message a bit, ACKed and pushed.


Thanks for your review and help.



Michal


Luyao

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

Re: [libvirt] [PATCHv2] conf: fix no error when set an unsupport string in ./devices/shmem/msi[@ioeventfd]

[lhuang]

On 05/11/2015 10:08 PM, Martin Kletzander wrote:

On Mon, May 11, 2015 at 08:59:37PM +0800, Luyao Huang wrote:

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

Pass the return value to an enum directly is not safe.
When pass a invalid @ioeventfd and virTristateSwitchTypeFromString
return -1 to def-msi.ioeventfd, and this value transform to
4294967295, so no error when the parse failed.

To fix this issue, folter the value using int and then assign it
to virTristateSwitch as it's done.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
v2:
 a new way to fix this issue.



ACK, I reworded the commit message and pushed and I believe there is
no need for a unit test.


Yes, thanks for your quick review.



I wonder why we don't rework out FromString() helpers to parse the
value into a parameter passed as a pointer and return 0/-1 properly.
Probably nobody wanted to mess with half of libvirt code, I guess...



Maybe... :)


Luyao


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

Re: [libvirt] [PATCH] qemu: fix double free when fail to cold-plug a rng device

[lhuang]

On 05/12/2015 11:14 PM, Michal Privoznik wrote:

On 12.05.2015 15:55, Luyao Huang wrote:

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

When cold-plug a rng device and get failed in qemuDomainAssignAddresses,
we will double free the rng device. Free the pointer after we Insert the
device success to fix this issue.

...
5  0x7fb7d180ac8a in virFree at util/viralloc.c:582
6  0x7fb7d1895cdd in virDomainRNGDefFree at conf/domain_conf.c:19786
7  0x7fb7d1895d99 in virDomainDeviceDefFree at conf/domain_conf.c:2022
8  0x7fb7b92b8baf in qemuDomainAttachDeviceFlags at qemu/qemu_driver.c:8785
9  0x7fb7d190c5d7 in virDomainAttachDeviceFlags at libvirt-domain.c:8488
10 0x7fb7d23af9d2 in remoteDispatchDomainAttachDeviceFlags at 
remote_dispatch.h:2842
...

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_driver.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 33c1cfd..f922a28 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8359,11 +8359,10 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
  
  if (virDomainRNGInsert(vmdef, dev-data.rng, false)  0)

  return -1;
+dev-data.rng = NULL;
  
  if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL)  0)

  return -1;
-
-dev-data.rng = NULL;
  break;
  
  case VIR_DOMAIN_DEVICE_MEMORY:



I've reworded the commit message a bit, ACKed and pushed.


Thanks for quick review.


Michal


Luyao

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

Re: [libvirt] [PATCH] conf: fix memleak in qemuRestoreCgroupState

[lhuang]

On 04/08/2015 06:00 PM, Ján Tomko wrote:

The subject prefix is incorrect - this is not in src/conf.

I just removed it completely, there's still a reference to qemu driver
in the function name.


Oh, that is a mistake, i can't remember why i chose conf as this 
commit subject prefix (maybe i was so hungry at that time :-P ).


Thanks a lot for your review and help.


On Wed, Apr 08, 2015 at 02:25:59PM +0800, Luyao Huang wrote:

  131,088 bytes in 16 blocks are definitely lost in loss record 2,174 of 2,176
 at 0x4C29BFD: malloc (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
 by 0x4C2BACB: realloc (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
 by 0x52A026F: virReallocN (viralloc.c:245)
 by 0x52BFCB5: saferead_lim (virfile.c:1268)
 by 0x52C00EF: virFileReadLimFD (virfile.c:1328)
 by 0x52C019A: virFileReadAll (virfile.c:1351)
 by 0x52A5D4F: virCgroupGetValueStr (vircgroup.c:763)
 by 0x1DDA0DA3: qemuRestoreCgroupState (qemu_cgroup.c:805)
 by 0x1DDA0DA3: qemuConnectCgroup (qemu_cgroup.c:857)
 by 0x1DDB7BA1: qemuProcessReconnect (qemu_process.c:3694)
 by 0x52FD171: virThreadHelper (virthread.c:206)
 by 0x82B8DF4: start_thread (pthread_create.c:308)
 by 0x85C31AC: clone (clone.S:113)

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_cgroup.c | 2 ++
  1 file changed, 2 insertions(+)

ACK and pushed.

Jan


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

Re: [libvirt] [PATCHv4 0/3] Support IPv6 addresses for graphics listening on networks

[lhuang]

On 04/08/2015 11:35 PM, Ján Tomko wrote:

v1: https://www.redhat.com/archives/libvir-list/2015-February/msg00442.html
v2: https://www.redhat.com/archives/libvir-list/2015-February/msg01228.html
v3: https://www.redhat.com/archives/libvir-list/2015-March/msg00423.html
changes in v4:
* remove the family attribute and just return the first IP address
* rename virNetDevGetIPv4Address to virNetDevGetIPv4AddressIoctl

John Ferlan (1):
   util: Replace virNetDevGetIPv4Address with virNetDevGetIPAddress

Ján Tomko (1):
   Support IPv6 in networkGetNetworkAddress

Luyao Huang (1):
   util: Update virNetDevGetIPAddress to get IPv6 addresses


Oh, thanks a lot for your help, i was hesitating if i need give a new 
version :)




  src/libvirt_private.syms|   2 +-
  src/network/bridge_driver.c |  11 ++--
  src/util/virnetdev.c| 119 +++-
  src/util/virnetdev.h|   2 +-
  4 files changed, 114 insertions(+), 20 deletions(-)



Luyao

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

Re: [libvirt] [PATCH] qemu: fix wrong call addressrelease function when attach RNG device success

[lhuang]

On 06/03/2015 02:04 AM, John Ferlan wrote:


On 05/31/2015 07:29 AM, Luyao Huang wrote:

Add a return value check to avoid the wrong address release.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_hotplug.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


ACK - although I'll make an adjustment to the commit message before pushing


Thanks a lot for your help and review



John


Luyao

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

Re: [libvirt] [PATCH] conf:audit: fix no audit log when start a vm with iothread

[lhuang]

On 06/03/2015 02:06 AM, John Ferlan wrote:


On 05/31/2015 10:07 AM, Luyao Huang wrote:

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/domain_audit.c | 2 ++
  1 file changed, 2 insertions(+)


ACK -

Will adjust commit message slightly before pushing...


Thanks again for your review and help... ;)



John


Luyao

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

Re: [libvirt] [PATCH] qemu: fix forget pass the return value to variable @ret

[lhuang]

On 06/03/2015 02:05 AM, John Ferlan wrote:


On 05/31/2015 09:27 AM, Luyao Huang wrote:

If we do not pass the return value from qemuDomainRemoveRNGDevice()
function to variable @ret, qemuDomainRemoveDevice() functiuon will
always fail when recive rng device unplug event from qemu monitor.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_hotplug.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


ACK -

Will adjust commit message slightly before pushing


Thank your help John



John


Luyao

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

Re: [libvirt] [PATCH] conf: fix not format priority when it is zero

[lhuang]

On 06/26/2015 05:26 AM, Martin Kletzander wrote:

On Wed, Jun 24, 2015 at 12:00:36PM +0800, Luyao Huang wrote:

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

We do not format the priority if it is 0, but this will
be a broken settings in guest. Change the condition of
format priority element to always format priority if
scheduler is 'fifo' or 'rr'.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
I haven't intorduce a new bool parameter to mark if we
set the priority value just like the other place we avoid
this issue, because i think this looks unnecessary in this
place.

src/conf/domain_conf.c |  6 ++--


The part for domain_conf.c didn't apply properly, but it's easy enough
to fix.

I modified the commit message as follows and pushed the patch:

   conf: Format scheduler priority when it is zero

   According to our XML definition, zero is as valid as any other value.
   Mainly because it should be kernel-agnostic.


Thanks a lot for your help and quick review.


Martin


Luyao

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

Re: [libvirt] [PATCH] qemu:lxc:xml:libxl:xen: improve the error in openconsole/channel

[lhuang]

On 06/24/2015 04:12 PM, Martin Kletzander wrote:

On Mon, Jun 15, 2015 at 09:58:36PM +0800, Luyao Huang wrote:

We allow do not pass the dev_name to openconsole() and openchannel()
function, but the error message is not good when we do not specified
the console/channel name.

the error message after this patch:
error: internal error: character device serial0 is not using a PTY

Signed-off-by: Luyao Huang lhu...@redhat.com
---
src/libxl/libxl_driver.c | 4 ++--
src/lxc/lxc_driver.c | 3 ++-
src/qemu/qemu_driver.c   | 8 
src/uml/uml_driver.c | 3 ++-
src/xen/xen_driver.c | 3 ++-
5 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index a7be745..c64d9be 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -4292,14 +4292,14 @@ libxlDomainOpenConsole(virDomainPtr dom,
if (!chr) {
virReportError(VIR_ERR_INTERNAL_ERROR,
   _(cannot find character device %s),
-   NULLSTR(dev_name));
+   dev_name ? dev_name : to open);
goto cleanup;
}



NACK to this hunk as that would be untranslatable.  And not just
because to open would stay as-is.



Got it, thanks for pointing out that.


if (chr-source.type != VIR_DOMAIN_CHR_TYPE_PTY) {
virReportError(VIR_ERR_INTERNAL_ERROR,
   _(character device %s is not using a PTY),
-   NULLSTR(dev_name));
+   dev_name ? dev_name : NULLSTR(chr-info.alias));
goto cleanup;
}



The rest look pretty fine, though, so I'll push that part in a while.

With fixed-up commit message...



Thanks a lot for your review and help !


Martin


Luyao

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

Re: [libvirt] [PATCH] conf: improve the way we format blkiotune and cputune

[lhuang]

On 06/24/2015 05:32 PM, Martin Kletzander wrote:

On Tue, Jun 23, 2015 at 09:24:25PM +0800, Luyao Huang wrote:

Just refactor existing code to use a child buf instead of
check all element before format blkiotune and cputune.
This will avoid the more and more bigger element check during
we introduce new elements in blkiotune and cputune in the
future.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
src/conf/domain_conf.c | 168 
+++--

1 file changed, 65 insertions(+), 103 deletions(-)



Nice cleanup, ACK  Pushed.


Thanks for your quick review

Luyao

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

Re: [libvirt] [PATCH] qemu:conf: introduce a function to delete vcpu sched

[lhuang]

On 06/24/2015 08:51 PM, Peter Krempa wrote:

On Wed, Jun 24, 2015 at 16:44:22 +0800, Luyao Huang wrote:

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

We have API allow vpu to be deleted, but an vcpu may be
included in some domain vcpu sched, so add a new API to
allow removing an iothread from some entry.

Split the virDomainIOThreadSchedDelId to reuse some code.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/domain_conf.c   | 48 ++--
  src/conf/domain_conf.h   |  1 +
  src/libvirt_private.syms |  1 +
  src/qemu/qemu_driver.c   |  6 --
  4 files changed, 40 insertions(+), 16 deletions(-)


I'm going to refactor the data structures that are storing the thread
scheduler data which will inherently fix this issue so even if we apply
this patch it will be basically reverted very soon.


Yes, i see your comment in this bug (unfortunately this is after i send 
this patch :) ), i agree no need apply this patch if you will fix that 
issue together during refactor.


Thanks for your quick review.


Peter


Luyao

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

Re: [libvirt] [PATCH] qemu: fix wrong remove guest cfg if migrate fail

[lhuang]

On 06/25/2015 04:25 PM, Jiri Denemark wrote:

On Thu, Jun 25, 2015 at 09:38:57 +0800, Luyao Huang wrote:

If we get fail in qemuMigrationPrepareAny, we forget
check if the vm is persistent then always call
qemuDomainRemoveInactive to clean the inactive settings.
Add a check to avoid this. This issue was introduce in
commit 540c339.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_migration.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 47d49cd..a57a177 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3432,7 +3432,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
  VIR_FREE(priv-origname);
  virPortAllocatorRelease(driver-migrationPorts, priv-nbdPort);
  priv-nbdPort = 0;
-qemuDomainRemoveInactive(driver, vm);
+if (!vm-persistent)
+qemuDomainRemoveInactive(driver, vm);
  }
  virDomainObjEndAPI(vm);
  if (event)

ACK.

I rewrote the commit message and pushed this patch, thanks.


You are welcome,  thanks for your quick review.


Jirka


Luyao

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

Re: [libvirt] [PATCHv1.5 1/2] cpu:x86: fix specified features will disappear after migrate/restore

[lhuang]

On 06/18/2015 09:29 PM, Jiri Denemark wrote:

On Wed, Jun 17, 2015 at 22:22:44 +0800, Luyao Huang wrote:

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

We allow set the feature with the host-passthrough cpu,
but won't save them in the migration xml, the features we
specified will disappear after migrate/restore. This is because
we skip the virCPUDefUpdateFeature if cpu mode is host-passthrough.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  v1.5: just update the description.

  src/cpu/cpu_x86.c | 9 -
  1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 2a14705..26601b6 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -2087,8 +2087,7 @@ x86UpdateCustom(virCPUDefPtr guest,
  
  static int

  x86UpdateHostModel(virCPUDefPtr guest,
-   const virCPUDef *host,
-   bool passthrough)
+   const virCPUDef *host)
  {
  virCPUDefPtr oldguest = NULL;
  const struct x86_map *map;
@@ -2124,7 +2123,7 @@ x86UpdateHostModel(virCPUDefPtr guest,
  }
  }
  }
-for (i = 0; !passthrough  i  oldguest-nfeatures; i++) {
+for (i = 0; i  oldguest-nfeatures; i++) {
  if (virCPUDefUpdateFeature(guest,
 oldguest-features[i].name,
 oldguest-features[i].policy)  0)

While this looks correct, save/restore (and likely migration too) with
-cpu host has even more bugs (e.g., -cpu host turns into -cpu host,+...
after save/restore) and I'd like to fix all that at once.


Yes, there are more bugs here, seems i didn't notice that at that time.

Thanks a lot for your review



Jirka


Luyao

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

Re: [libvirt] [PATCHv3] qemu: add a check for slot and base when build dimm address

[lhuang]

On 06/18/2015 08:12 PM, John Ferlan wrote:


On 06/15/2015 08:33 AM, Luyao Huang wrote:

When hot-plug a memory device, we don't check if there
is a memory device have the same address with the memory device
we want hot-pluged. Qemu forbid use/hot-plug 2 memory device
with same slot or the same base(qemu side this elemnt named addr).

Introduce a address check when build memory device qemu command line.

Signed-off-by: Luyao Huang lhu...@redhat.com
---

NOTE: Used the following for commit message:

qemu: Add a check for slot and base dimm address conflicts

When hotplugging a memory device, there wasn't a check to determine
if there is a conflict with the address space being used by the to
be added memory device and any existing device which is disallowed by qemu.

This patch adds a check to ensure the new device address doesn't
conflict with any existing device.


Thanks for the message improvement




  v3:
   rename qemuBuildMemoryDeviceAddr to qemuCheckMemoryDimmConflict and
   remove the refactor.

  src/qemu/qemu_command.c | 37 +
  1 file changed, 37 insertions(+)


ACK

Adjusted patch as shown below and pushed.


Thanks a lot for your review and help



John




Luyao

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

Re: [libvirt] [PATCH 2/2] qemu: fix a document issue as we support host-passthrough with features

[lhuang]

On 06/18/2015 08:42 PM, Jiri Denemark wrote:

On Wed, Jun 17, 2015 at 22:22:45 +0800, Luyao Huang wrote:

From the documentation :With this mode, the CPU visible to
the guest should be exactly the same as the host CPU even in
the aspects that libvirt does not understand.

So this place document need fix.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_process.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 64ee049..3cd0ff4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4024,8 +4024,10 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver,
  bool ret = false;
  size_t i;
  
-/* no features are passed to QEMU with -cpu host

- * so it makes no sense to verify them */
+/* Although we allow set features with host-passthrough
+ * cpu mode, we allow user require/disable the feature
+ * that libvirt does not understand, so it makes no sense
+ * to verify them */
  if (def-cpu  def-cpu-mode == VIR_CPU_MODE_HOST_PASSTHROUGH)
  return true;
  

NACK. We certainly don't want features to be just passed through without
any validation if used with host-passthrough. We rather need to fix the
code check the features used in a guest cpu element are all known to
libvirt.


Okay, get it

Thanks a lot for your review.



Jirka


Luyao

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

Re: [libvirt] [PATCH] qemu: fix cannot attach a virtio channel

[lhuang]

On 06/15/2015 04:25 PM, Ján Tomko wrote:

I'd rather be more specific in the commit summary, e.g.:
fix address allocation on chardev hotplug


good summary, thanks for you help


On Wed, Jun 10, 2015 at 10:49:37PM +0800, Luyao Huang wrote:

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

When attach a channel which target type is virtio, we will
get an error:

error: Failed to attach device from channel-file.xml
error: internal error: virtio serial device has invalid address type

This issue was introduced in commit 9807c4.
We didn't check the chr device type is serial then check
if the device target type is pci-serial, but not all the
Chr device is a serial type so we should check the device
type before check the target type to avoid assign wrong
address to other device type chr device.

Also most of chr device do not need {pci, virtio-serial} address
in qemu, we just get the address for the device which needed.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_hotplug.c | 72 +++--
  1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 3562de6..4d60513 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1531,6 +1531,48 @@ qemuDomainChrRemove(virDomainDefPtr vmdef,
  return ret;
  }
  
+static int

+qemuDomainAttachChrDeviceAssignAddr(qemuDomainObjPrivatePtr priv,
+virDomainChrDefPtr chr)
+{
+if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
+chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO 
+virDomainVirtioSerialAddrAutoAssign(NULL, priv-vioserialaddrs,
+chr-info, true)  0)
+return -1;
+
+if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL 
+chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI 
+(chr-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
+ chr-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) 

Do we need to check the address type here? pci-serial should always have
a PCI address.


Yes, pci-serial should always have a PCI address, and add this check
is avoid always assign a pci address even we already specified the address
which type is not pci, in that case i think output an error will be better.
(although it is a corner case, and i notice when we start a guest with wrong
address type pci-serial will get the error,  but cannot get the error 
when attach

it, so i guess QE will complain about this ;) )

and after add this check the error will be:

# virsh attach-device rhel7.0  pci-serial.xml
error: Failed to attach device from pci-serial.xml
error: unsupported configuration: pci-serial requires address of pci type


+virDomainPCIAddressEnsureAddr(priv-pciaddrs, chr-info)  0)
+return -1;
+
+if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL 
+chr-targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO 
+virDomainVirtioSerialAddrAutoAssign(NULL, priv-vioserialaddrs,
+chr-info, false)  0)
+return -1;
+
+return 0;
+}
+
+static void
+qemuDomainAttachChrDeviceReleaseAddr(qemuDomainObjPrivatePtr priv,
+ virDomainObjPtr vm,
+ virDomainChrDefPtr chr)
+{
+if ((chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 
+ chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) ||
+(chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL 
+ chr-targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO))
+virDomainVirtioSerialAddrRelease(priv-vioserialaddrs, chr-info);
+
+if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL 
+chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI)
+qemuDomainReleaseDeviceAddress(vm, chr-info, NULL);
+}
+
  int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainChrDefPtr chr)
@@ -1541,7 +1583,6 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
  char *devstr = NULL;
  char *charAlias = NULL;
  bool need_release = false;
-bool allowZero = false;
  
  if (!virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_DEVICE)) {

  virReportError(VIR_ERR_OPERATION_INVALID, %s,
@@ -1552,22 +1593,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
  if (qemuAssignDeviceChrAlias(vmdef, chr, -1)  0)
  goto cleanup;
  
-if (chr-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE 

-chr-targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO)
-allowZero = true;
-
-if (chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) {
-if (virDomainPCIAddressEnsureAddr(priv-pciaddrs, chr-info)  0)
-goto cleanup;
-} else if (chr-targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) {
-

Re: [libvirt] [PATCHv3] qemu: add a check for slot and base when build dimm address

[lhuang]

On 06/16/2015 08:27 AM, zhang bo wrote:

On 2015/6/15 20:33, Luyao Huang wrote:


When hot-plug a memory device, we don't check if there
is a memory device have the same address with the memory device
we want hot-pluged. Qemu forbid use/hot-plug 2 memory device
with same slot or the same base(qemu side this elemnt named addr).

+for (i = 0; i  def-nmems; i++) {
+ virDomainMemoryDefPtr tmp = def-mems[i];
+
+ if (tmp == mem ||
+ tmp-info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM)
+ continue;
+
+ if (mem-info.addr.dimm.slot == tmp-info.addr.dimm.slot) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+_(memory device slot '%u' is already being
+   used by another memory device),
+mem-info.addr.dimm.slot);
+ return true;
+ }
+
+ if (mem-info.addr.dimm.base != 0  tmp-info.addr.dimm.base != 0 
+ mem-info.addr.dimm.base == tmp-info.addr.dimm.base) {


Equal the memory device base with 0 means: make sure the 2 memory device 
base is specified (not let qemu auto assign the base).


So the logic here is :
1. make sure memory device A and B base is not auto assign mode.
2. then equal the base address


The logic here equals:
  if (mem-info.addr.dimm.base != 0 
  mem-info.addr.dimm.base == tmp-info.addr.dimm.base) {
will it be better like this?



Indeed, your code looks better.

Thanks a lot for your review.


+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+_(memory device base '0x%llx' is already being
+   used by another memory device),
+mem-info.addr.dimm.base);
+ return true;
+ }
+}
+
+return false;
+}
+
  char *
  qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
   virDomainDefPtr def,
@@ -4993,6 +5027,9 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
mem-targetNode, mem-info.alias, mem-info.alias);
  
  if (mem-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) {

+if (qemuCheckMemoryDimmConflict(def, mem))
+return NULL;
+
  virBufferAsprintf(buf, ,slot=%d, mem-info.addr.dimm.slot);
  virBufferAsprintf(buf, ,addr=%llu, mem-info.addr.dimm.base);
  }





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

Re: [libvirt] [PATCHv2] qemu: add a check for slot and base when build dimm address

[lhuang]

On 06/11/2015 03:12 AM, John Ferlan wrote:


On 05/27/2015 05:50 AM, Luyao Huang wrote:

When hot-plug a memory device, we don't check if there
is a memory device have the same address with the memory device
we want hot-pluged. Qemu forbid use/hot-plug 2 memory device
with same slot or the same base(qemu side this elemnt named addr).

Introduce a address check when build memory device qemu command line.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
v2:
  move the check to qemuBuildMemoryDeviceStr() to check the dimm address
  when start/hot-plug a memory device.

  src/qemu/qemu_command.c | 77 -
  1 file changed, 57 insertions(+), 20 deletions(-)


Perhaps a bit easier to review if this was split into two patches the
first being purely code motion and the second being fixing the
problem...  That said, I don't think the refactor is necessary. I've
attached an alternative and some notes inline below...


Yes, i should split these changes in two patches, however i think you 
are right

i will remove the unnecessary refactor in next version, so no need split ;)



John


diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d8ce511..0380a3b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4955,6 +4955,61 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem,
  }
  
  
+static int

+qemuBuildMemoryDeviceAddr(virBuffer *buf,
+  virDomainDefPtr def,
+  virDomainMemoryDefPtr mem)

static bool
qemuCheckMemoryDimmConflict(virDomainDefPtr def,
 virDomainMemoryDefPtr mem)


+{
+ssize_t i;

size_t usually, then keep the following checks in the caller


Okay




+
+if (mem-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
+return 0;
+
+if (mem-info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+   _(only 'dimm' addresses are supported for the 
+ pc-dimm device));
+return -1;
+}
+
+if (mem-info.addr.dimm.slot = def-mem.memory_slots) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _(memory device slot '%u' exceeds slots count '%u'),
+   mem-info.addr.dimm.slot, def-mem.memory_slots);
+return -1;
+}
+

Thru here... Since it seems the following is your bugfix correct?


Yes, this is bugfix part




+for (i = 0; i  def-nmems; i++) {
+ virDomainMemoryDefPtr tmp = def-mems[i];
+
+ if (tmp == mem ||
+ tmp-info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM)
+ continue;
+
+ if (mem-info.addr.dimm.slot == tmp-info.addr.dimm.slot) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+_(memory device slot '%u' already been
+   used by other memory device),

...is already being used by another


+mem-info.addr.dimm.slot);
+ return -1;

return true;


+ }
+
+ if (mem-info.addr.dimm.base != 0  tmp-info.addr.dimm.base != 0 
+ mem-info.addr.dimm.base == tmp-info.addr.dimm.base) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+_(memory device base '0x%llx' already been

...is already being used by another...


Thanks for pointing out this.




+   used by other memory device),
+mem-info.addr.dimm.base);
+ return -1;

return true


+ }
+}

return false;

Keep remainder in caller


+
+virBufferAsprintf(buf, ,slot=%d, mem-info.addr.dimm.slot);
+virBufferAsprintf(buf, ,addr=%llu, mem-info.addr.dimm.base);
+
+return 0;
+}
+
  char *
  qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
   virDomainDefPtr def,
@@ -4976,29 +5031,11 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
  return NULL;
  }
  
-if (mem-info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM 

-mem-info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
-   _(only 'dimm' addresses are supported for the 
- pc-dimm device));

Your refactor adjusts this test, so if the type was something else then
the virBufferAsprintf happens before the error... it's a nit, but future
adjustments could do something unexpected...


Right, i forgot this :)


-return NULL;
-}
-
-if (mem-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM 
-mem-info.addr.dimm.slot = def-mem.memory_slots) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _(memory device slot '%u' exceeds slots count 
'%u'),
-   mem-info.addr.dimm.slot, def-mem.memory_slots);
-return 

Re: [libvirt] [PATCH] conf: improve the address check for dimm type

[lhuang]

On 05/27/2015 03:03 PM, Peter Krempa wrote:

On Wed, May 27, 2015 at 14:39:58 +0800, Luyao Huang wrote:

When hot-plug/cold-plug a memory device, we use
memcmp() function to check if there is a memory device
have the same address with the memory device we want
hot-pluged. But qemu forbid use/hot-plug 2 memory device
with same slot *or* the same base(qemu side this elemnt
named addr).

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/domain_conf.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6e57425..413f839 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3089,7 +3089,10 @@ virDomainDeviceInfoAddressIsEqual(const 
virDomainDeviceInfo *a,
  break;
  
  case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM:

-if (memcmp(a-addr.dimm, b-addr.dimm, sizeof(a-addr.dimm)))
+if (a-addr.dimm.slot != b-addr.dimm.slot 
+(a-addr.dimm.base == 0 ||
+ b-addr.dimm.base == 0 ||
+ a-addr.dimm.base != b-addr.dimm.base))
  return false;

This function is designed to check if the address is equal not if it is
not conflicting for a particular hypervisor.

If you are going to enforce that both the address and base are
different, this function is not the right place.


Okay, reasonable to me, i will found a place for the dimm address check 
in src/qemu/*


Thanks you advise and quick review.



Peter


Luyao

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

Re: [libvirt] [PATCH] util: improve the sysinfo element XML format

[lhuang]

On 05/27/2015 07:59 AM, John Ferlan wrote:


On 05/22/2015 05:26 AM, Luyao Huang wrote:

When set sysinfo element without sub-elements,
libvirt will format it as:

   sysinfo type='smbios'
   /sysinfo

After improve the format:

   sysinfo type='smbios'/

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/util/virsysinfo.c | 33 ++---
  1 file changed, 22 insertions(+), 11 deletions(-)


ACK - I'll slightly adjust commit message before pushing


Thanks for your quick review



John


Luyao

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

Re: [libvirt] [PATCH] conf: do not format redirfilter element if it do not have sub-element

[lhuang]

On 05/27/2015 08:00 AM, John Ferlan wrote:


On 05/22/2015 05:26 AM, Luyao Huang wrote:

When set a redirfilter element without sub-element, libvirt will
format it like this:

 redirfilter
 /redirfilter

Just drop this element if it do not have any sub-element.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/conf/domain_conf.c | 4 
  1 file changed, 4 insertions(+)


ACK - I'll slightly adjust commit message before pushing


Thank you John :)



John


Luyao

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

Re: [libvirt] [PATCH] qemu: fix not end the job after use OpenGraphics(FD) and get fail when exit monitor

[lhuang]

On 07/01/2015 02:15 PM, Martin Kletzander wrote:

On Tue, Jun 30, 2015 at 11:35:13AM +0800, Luyao Huang wrote:

If guest unexpect exit(qemu process be killed) and get failed when
exit the monitor, guest job still handled by old function, this will
make guest cannot start later.
Need call qemuDomainObjEndJob to release job status before unref vm.

Signed-off-by: Luyao Huang lhu...@redhat.com
---


ACK and safe for freeze.  I'll push it in a while with modified commit
message.



Thanks a lot for your quick review and help.

Luyao

src/qemu/qemu_driver.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2b530c8..b1c9f08 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17255,10 +17255,8 @@ qemuDomainOpenGraphics(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorOpenGraphics(priv-mon, protocol, fd, graphicsfd,
  (flags  
VIR_DOMAIN_OPEN_GRAPHICS_SKIPAUTH) != 0);

-if (qemuDomainObjExitMonitor(driver, vm)  0) {
+if (qemuDomainObjExitMonitor(driver, vm)  0)
ret = -1;
-goto cleanup;
-}
qemuDomainObjEndJob(driver, vm);

 cleanup:
@@ -17327,10 +17325,8 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorOpenGraphics(priv-mon, protocol, pair[1], 
graphicsfd,
  (flags  
VIR_DOMAIN_OPEN_GRAPHICS_SKIPAUTH));

-if (qemuDomainObjExitMonitor(driver, vm)  0) {
+if (qemuDomainObjExitMonitor(driver, vm)  0)
ret = -1;
-goto cleanup;
-}
qemuDomainObjEndJob(driver, vm);
if (ret  0)
goto cleanup;
--
1.8.3.1

--
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 03/10] qemu: Refactor creation of shared memory device commandline

[lhuang]

On 07/06/2015 01:38 PM, Martin Kletzander wrote:

On Mon, Jul 06, 2015 at 10:23:59AM +0800, lhuang wrote:


On 07/03/2015 08:56 PM, Martin Kletzander wrote:

On Wed, Jun 17, 2015 at 11:56:14AM +0800, Luyao Huang wrote:

Rename qemuBuildShmemDevCmd to qemuBuildShmemDevStr and change the
return type so that it can be reused in the device hotplug code later.

And split the chardev creation part in a new function
qemuBuildShmemBackendStr for reused in the device hotplug code later.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
src/qemu/qemu_command.c | 70
+++--
src/qemu/qemu_command.h |  7 +
2 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 636e040..0414f77 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8433,9 +8433,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
   return ret;
}

-static int
-qemuBuildShmemDevCmd(virCommandPtr cmd,
- virDomainDefPtr def,
+char *
+qemuBuildShmemDevStr(virDomainDefPtr def,
virDomainShmemDefPtr shmem,
virQEMUCapsPtr qemuCaps)
{
@@ -8489,14 +8488,38 @@ qemuBuildShmemDevCmd(virCommandPtr cmd,
   if (virBufferCheckError(buf)  0)
   goto error;

-virCommandAddArg(cmd, -device);
-virCommandAddArgBuffer(cmd, buf);
-
-return 0;
+return virBufferContentAndReset(buf);



You should be able to just unconditionally do
return virBufferContentAndReset() here since it returns NULL if
there's a buf-error.

ACK with that changed.



Right, i forgot that, thanks a lot for your review



Sorry, you cannot, there's a goto error; from some part of the code
where there is no buf-error set, so no change to this, you were
right.

No need to resend these first three, I'll push whatever is OK to go in
after I finish the review, and let you know what needs work.



Okay, got it, thanks a lot for your helps.

BR,
Luyao


Thanks,
Martin


Luyao


error:
   virBufferFreeAndReset(buf);
-return -1;
+return NULL;
+}
+
+char *
+qemuBuildShmemBackendStr(virDomainShmemDefPtr shmem,
+ virQEMUCapsPtr qemuCaps)
+{
+char *devstr = NULL;
+virDomainChrSourceDef source = {
+.type = VIR_DOMAIN_CHR_TYPE_UNIX,
+.data.nix = {
+.path = shmem-server.path,
+.listen = false,
+}
+};
+
+if (!shmem-server.path 
+virAsprintf(source.data.nix.path,
+/var/lib/libvirt/shmem-%s-sock,
+shmem-name)  0)
+return NULL;
+
+devstr = qemuBuildChrChardevStr(source, shmem-info.alias,
qemuCaps);
+
+if (!shmem-server.path)
+VIR_FREE(source.data.nix.path);
+
+return devstr;
}

static int
@@ -8505,35 +8528,18 @@ qemuBuildShmemCommandLine(virCommandPtr cmd,
 virDomainShmemDefPtr shmem,
 virQEMUCapsPtr qemuCaps)
{
-if (qemuBuildShmemDevCmd(cmd, def, shmem, qemuCaps)  0)
+char *devstr = NULL;
+
+if (!(devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps)))
   return -1;
+virCommandAddArgList(cmd, -device, devstr, NULL);
+VIR_FREE(devstr);

   if (shmem-server.enabled) {
-char *devstr = NULL;
-virDomainChrSourceDef source = {
-.type = VIR_DOMAIN_CHR_TYPE_UNIX,
-.data.nix = {
-.path = shmem-server.path,
-.listen = false,
-}
-};
-
-if (!shmem-server.path 
-virAsprintf(source.data.nix.path,
-/var/lib/libvirt/shmem-%s-sock,
-shmem-name)  0)
+if (!(devstr = qemuBuildShmemBackendStr(shmem, qemuCaps)))
   return -1;

-devstr = qemuBuildChrChardevStr(source,
shmem-info.alias, qemuCaps);
-
-if (!shmem-server.path)
-VIR_FREE(source.data.nix.path);
-
-if (!devstr)
-return -1;
-
-virCommandAddArg(cmd, -chardev);
-virCommandAddArg(cmd, devstr);
+virCommandAddArgList(cmd, -chardev, devstr, NULL);
   VIR_FREE(devstr);
   }

diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 0fc59a8..73f24dc 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -194,6 +194,13 @@ int
qemuBuildRNGBackendProps(virDomainRNGDefPtr rng,
const char **type,
virJSONValuePtr *props);

+char *qemuBuildShmemDevStr(virDomainDefPtr def,
+   virDomainShmemDefPtr shmem,
+   virQEMUCapsPtr qemuCaps);
+char *qemuBuildShmemBackendStr(virDomainShmemDefPtr shmem,
+   virQEMUCapsPtr qemuCaps);
+
+
int qemuOpenPCIConfig(virDomainHostdevDefPtr dev);

/* Legacy, pre device support */
--
1.8.3.1

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




--
libvir-list mailing list

Re: [libvirt] [PATCH] virsh: report error if vcpu number exceed the guest maxvcpu number

[lhuang]

On 07/02/2015 04:29 AM, John Ferlan wrote:


On 06/28/2015 10:10 PM, Luyao Huang wrote:

If usr pass a vcpu which exceed guest maxvcpu number, virsh client
will only output an header like this:

  # virsh vcpupin test3 1000
  VCPU: CPU Affinity
  --

After this patch:

  # virsh vcpupin test3 1000
  error: vcpu 1000 is out of range of cpu count 2

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  tools/virsh-domain.c | 5 +
  1 file changed, 5 insertions(+)


Seemed odd that this check wasn't there - so I did some digging...

Pavel's commit id '81dd81e' removed a check that seems to be what is
desired in this path (or was there before his change):

 if (vcpu = info.nrVirtCpu) {
 vshError(ctl, %s, _(vcpupin: vCPU index out of range.));
 goto cleanup;
 return false;
 }

As part of this commit, you'll note there was a test change in
tests/vcpupin:

  # An out-of-range vCPU number deserves a diagnostic, too.
  $abs_top_builddir/tools/virsh --connect test:///default vcpupin test
100 0,1  out 21
  test $? = 1 || fail=1
  cat \EOF  exp || fail=1
-error: vcpupin: vCPU index out of range.
+error: invalid argument: requested vcpu is higher than allocated vcpus


Searching on their error message lands one in test_driver.c/
testDomainPinVcpu().  So something specific for a set path, but the path
you're hitting is the get path.


Yes, i noticed this and checked if need introduce a test or change the 
old test, but

i found test driver not support get vcpupin.



FWIW: If a similar test was run on my system I get:
# virsh vcpupin $dom 1000 0,1
error: invalid argument: vcpu 1000 is out of range of live cpu count 2

#


So, if I understand everything that was done - then while your change is
mostly correct, I think you could perhaps message the error similar to
the vshCPUCountCollect failure (see the attached patch)


I saw the attached patch, but there is some problem about check the flag 
(actually

i had a try with check flags and output a better error before).

If check flags like vshCPUCountCollect failure, there will be a problem 
when do not
pass flag or just pass current flag to vcpupin, we will get error like 
this (pass a too big vcpu

number):

# virsh list;virsh vcpupin rhel7.0 1000 --current
 IdName   State

 3 rhel7.0running

error: vcpu 1000 is out of range of persistent cpu count 4

In this case, we output persistent instead of live, this is because 
vshCPUCountCollect()
cannot return certain flags (although there is a description say 
Returns the count of vCPUs for a
domain and certain flags). So we need more check for current flags, 
maybe like this :


diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 27d62e9..334fd3a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6497,6 +6497,19 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 }

+if (got_vcpu  vcpu = ncpus) {
+if (flags  VIR_DOMAIN_AFFECT_LIVE ||
+(flags  VIR_DOMAIN_AFFECT_CURRENT  
virDomainIsActive(dom) == 1))

+vshError(ctl,
+ _(vcpu %d is out of range of live cpu count %d),
+ vcpu, ncpus);
+else
+vshError(ctl,
+ _(vcpu %d is out of range of persistent cpu 
count %d),

+ vcpu, ncpus);
+goto cleanup;
+}
+
 cpumaplen = VIR_CPU_MAPLEN(maxcpu);
 cpumap = vshMalloc(ctl, ncpus * cpumaplen);
 if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,



Before I make that change for you - hopefully Pavel can take a look as
well to be sure I haven't missed something.

With any luck we this could be addressed before the 1.2.17 release, but
if not since it's been a regression since 1.2.13 and no one's noticed,
then another release probably won't hurt.


Right, if we can fix it in 1.2.17, it will be better :)

Thanks a lot for your help and review.


John



Luyao

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