Re: [libvirt] [PATCH] virsh: Fix vcpupin command output wrong vcpu pinning info

[lhuang]

On 12/19/2018 05:27 PM, Michal Privoznik wrote:

On 12/19/18 4:17 AM, Luyao Huang wrote:

Commit 3072ded3 changed the waya to format the vcpu pinning info
and forget to get cpumap for each vcpu during the loop, that cause
vcpupin command will display vcpu 0 info for other vcpus.

Signed-off-by: Luyao Huang 
---
  tools/virsh-domain.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 4d9f065..24f7852 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6954,7 +6954,8 @@ virshVcpuPinQuery(vshControl *ctl,
  if (got_vcpu && i != vcpu)
  continue;
  
-if (!(pinInfo = virBitmapDataFormat(cpumap, cpumaplen)))

+if (!(pinInfo = virBitmapDataFormat(VIR_GET_CPUMAP(cpumap, 
cpumaplen, i),
+cpumaplen)))
  goto cleanup;
  
  if (virAsprintf(, "%zu", i) < 0)




ACKed and pushed.


Thanks a lot for your quick review !

Luyao


Michal


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

Re: [libvirt] [PATCH] qemu: domain: validate memory access during validate domain config

[lhuang]

On 08/28/2018 06:01 AM, John Ferlan wrote:


On 08/20/2018 05:48 AM, Luyao Huang wrote:

commit 6534b3c4 try to raise an error when there is no numa nodes but
set access='shared' in domain config. In that commit, we add a memory access
validate function for memory device, but this check is not related to memory
device and won't work if there is no memory device in guest xml.

Move the memory access related check in the domain config validate function,
and simplify the unit test xml to avoid other error.

https://bugzilla.redhat.com/show_bug.cgi?id=1448149#c14

Signed-off-by: Luyao Huang 
---
  src/qemu/qemu_domain.c  | 55 +++---
  tests/qemuxml2argvdata/hugepages-memaccess3.xml | 62 +
  tests/qemuxml2argvtest.c|  6 +--
  3 files changed, 32 insertions(+), 91 deletions(-)


Hmm... I have a recollection of reviewing this... Let's see, yes:

https://www.redhat.com/archives/libvir-list/2017-December/msg00387.html

and follow-ups...  I think I agree with you the validation should be
done in qemuDomainDefValidate instead of qemuDomainDeviceDefValidate;
however, you're making multiple changes at one time - so I went to
dissect...  This response got lengthy and I'm kind of at a hmm? wtf
moment that hopefully Michal can look at since he was the original
author.  Maybe he can recall 8 months ago and whether he got the
expected error message or not on output.

My concern/point was changing hugepages-memaccess3.xml and
qemuxml2argvtest.c to remove some things while also changing from
DO_TEST_FAILURE to DO_TEST_PARSE_ERROR is doing two different things in
one patch, which we generally try to avoid, but in this case it may just
be necessary.


yes, i was trying to fix the hugepages-memaccess3.xml since my changes 
will break the unit test.
Actually the code changes for the hugepages-memaccess3.xml and 
qemuxml2argvtest.c included 2 different fix, one for fixing the exist 
unit test and another for fixing the failure introduced in this patch.
And i merged them in one patch, maybe i should keep them split in next 
time ?




When I tried to separate the changes in such a way that the unnecessary
lines were removed from hugepages-memaccess3.xml first - the test ended
up succeeding! So that was strange, but I guess not totally unexpected
since the device mems don't exist and the wrong check was being done.

So I went back to whence we started and note that qemuxml2argvtest fails
the hugepages-memaccess3 without any of these changes with (as seen with
VIR_TEST_DEBUG=1 VIR_TEST_RANGE=169 tests/qemuxml2argvtest):

169) QEMU XML-2-ARGV hugepages-memaccess3
... Got expected error:
2018-08-27 21:24:04.934+: 5816: info : libvirt version: 4.7.0
2018-08-27 21:24:04.934+: 5816: info : hostname: localhost.localdomain
2018-08-27 21:24:04.934+: 5816: error :
qemuProcessStartValidateVideo:5033 : unsupported configuration: this
QEMU does not support 'virtio' video device

So, going a bit slower... and removing things to see if I can get that
"memory access mode '%s' not supported..." error message...

1. Remove  and  entries, but get:

169) QEMU XML-2-ARGV hugepages-memaccess3
... Got expected error:
2018-08-27 21:26:28.754+: 5907: info : libvirt version: 4.7.0
2018-08-27 21:26:28.754+: 5907: info : hostname: localhost.localdomain
2018-08-27 21:26:28.754+: 5907: error : qemuBuildPMCommandLine:6523
: unsupported configuration: setting ACPI S3 not supported

2. Remove  and get:

169) QEMU XML-2-ARGV hugepages-memaccess3
... Got expected error:
2018-08-27 21:27:48.000+: 5972: info : libvirt version: 4.7.0
2018-08-27 21:27:48.000+: 5972: info : hostname: localhost.localdomain
2018-08-27 21:27:48.000+: 5972: error :
qemuBuildUSBControllerDevStr:2681 : unsupported configuration:
piix3-usb-uhci not supported in this QEMU binary

3. Remove "model='piix3-uhci'" from the  and get:

169) QEMU XML-2-ARGV hugepages-memaccess3
... Got expected error:
2018-08-27 21:30:21.989+: 6360: info : libvirt version: 4.7.0
2018-08-27 21:30:21.989+: 6360: info : hostname: localhost.localdomain
2018-08-27 21:30:21.989+: 6360: error :
qemuBuildSerialChrDeviceStr:10576 : unsupported configuration:
'isa-serial' is not supported in this QEMU binary

5. Remove , , and  and get passed instead of
failure.


You really have a good patience ;) i deleted all the unrelated elements 
when i saw the error come from qemuProcessStartValidateVideo() function.


BTW: I think maybe we can add some function like 
DO_TEST_FAILURE_CHECK_ERROR which use regex to check if the error is 
expected.




So at first I was a bit stumped as to what happened... Could there be
"conflict" or no error message now due to changes Pavel Hrdina made
dealing with hugepages and parsing. So, in any case, I'd like to wait
for Michal's input before proceeding further on this one.


Sure, and i have checked that there is no conflict with the Pavel 
Hrdina's "Fix and 

Re: [libvirt] [PATCH] qemu: Make sure shmem memory is shared

[lhuang]

I have test this patch, and it works well.

After this patch, Libvirt can generate share=yes in ivshmem-plain memory 
backend command line:


# ps aux|grep r7
...
-object 
memory-backend-file,id=shmmem-shmem0,mem-path=/dev/shm/my_shmem1,size=4194304,share=yes 
-device ivshmem-plain,id=shmem0,memdev=shmmem-shmem0,bus=pci.0,addr=0x7

...

BR,
Luyao

On 11/10/2016 03:32 PM, Martin Kletzander wrote:

Even though using /dev/shm/asdf as the backend, we still need to make
the mapping shared.  The original patch forgot to add that parameter.

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

Signed-off-by: Martin Kletzander 
---
  src/qemu/qemu_command.c   | 1 +
  tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args | 6 +++---
  2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index caa80e74c26a..d3f99d34c67f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8565,6 +8565,7 @@ qemuBuildShmemBackendMemProps(virDomainShmemDefPtr shmem)
  virJSONValueObjectCreate(,
   "s:mem-path", mem_path,
   "U:size", shmem->size,
+ "b:share", true,
   NULL);

  VIR_FREE(mem_path);
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args 
b/tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args
index 7abc7f8c4be5..688b7c7f63e2 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args
@@ -18,13 +18,13 @@ QEMU_AUDIO_DRV=none \
  -boot c \
  -usb \
  -object memory-backend-file,id=shmmem-shmem0,mem-path=/dev/shm/shmem0,\
-size=4194304 \
+size=4194304,share=yes \
  -device ivshmem-plain,id=shmem0,memdev=shmmem-shmem0,bus=pci.0,addr=0x3 \
  -object memory-backend-file,id=shmmem-shmem1,mem-path=/dev/shm/shmem1,\
-size=134217728 \
+size=134217728,share=yes \
  -device ivshmem-plain,id=shmem1,memdev=shmmem-shmem1,bus=pci.0,addr=0x5 \
  -object memory-backend-file,id=shmmem-shmem2,mem-path=/dev/shm/shmem2,\
-size=268435456 \
+size=268435456,share=yes \
  -device ivshmem-plain,id=shmem2,memdev=shmmem-shmem2,bus=pci.0,addr=0x4 \
  -device ivshmem-doorbell,id=shmem3,chardev=charshmem3,ioeventfd=on,bus=pci.0,\
  addr=0x6 \


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

Re: [libvirt] [PATCH] tests: add missed directories in EXTRA_DIST

[lhuang]

On 07/12/2016 05:41 PM, Martin Kletzander wrote:

On Tue, Jul 12, 2016 at 04:28:22PM +0800, Luyao Huang wrote:

In commit ec5dcf2a and b0b4a35c, we have moved some xml to
new directories, but forget fix the Makefile. Add 2 directories
in EXTRA_DIST to fix broken vpath build.

Signed-off-by: Luyao Huang 
---
tests/Makefile.am | 2 ++
1 file changed, 2 insertions(+)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index fb2380d..010e348 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -122,6 +122,8 @@ EXTRA_DIST =\
qemucaps2xmldata \
qemuhelpdata \
qemuhotplugtestdata \


This ^^ should be removed as well, there are two unused files in it only
that I mistakenly created.  I'll add this to the commit, adjust the
commit message and push it in a while.  Thanks for noticing.



Oh, you are right, i didn't deep check these code when i wrote this patch.

Thanks a lot for your review.

Have a nice day !
Luyao


+qemuhotplugtestdevices \
+qemuhotplugtestdomains \
qemumonitorjsondata \
qemuxml2argvdata \
qemuxml2xmloutdata \
--
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] virsh: fix cpu-stats command output format issue

[lhuang]

On 01/26/2016 04:24 PM, Peter Krempa wrote:

On Tue, Jan 26, 2016 at 10:25:05 +0800, Luyao Huang wrote:

After commit 57177f1, the cpu-stats command format change to:

CPU0:
 cpu_time 14401.507878990 seconds
 vcpu_time14378732785511

vcpu_time is not user friendly. After this patch, it will
change back:
CPU0:
 cpu_time 14401.507878990 seconds
 vcpu_time14378.732785511 seconds

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

Signed-off-by: Luyao Huang 
---
  tools/virsh-domain.c | 1 +
  1 file changed, 1 insertion(+)

ACK, I'll push it shortly.


Thanks a lot for your quick review,

Luyao


Peter


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

Re: [libvirt] [PATCHv2 4/4] qemu: Implement shared memory device hot-unplug

[lhuang]

On 12/21/2015 10:24 AM, lhuang wrote:



On 12/17/2015 08:26 PM, John Ferlan wrote:


On 12/16/2015 09:43 PM, lhuang wrote:


On 12/15/2015 08:39 PM, John Ferlan wrote:

[...]


+if (rc < 0)
+return -1;
+

I know this is a copy of the RemoveRNGDevice; however, this code
doesn't
remove an 'obj'. In fact, if !shmem->server.enabled, then we don't
enter
the monitor at all.

Thus the following event probably won't happen...
I am not sure what your mean is ... i guess your mean the device 
remove
event we get from qmp monitor won't happen ? we will get that 
event if
qemu remove shmem device success, it should always happen if qemu 
really

remove it and there is no bugs on qemu :)

While reviewing I got lazy and didn't check the non hotplug case to 
how
shmem is added to the vm, but the point I was trying to make is 
that "if

(shmem->server.enabled)" fails (e.g. is false), then there is no "rc =
qemuMonitorDelObject(priv->mon, objAlias);" call in this API 
(similar to
RNG code), thus how does the following event get triggered? Even if 
the

condition was true, does detaching the char dev cause the event to be
triggered?
I thought the event was related to the DelObject code, but I didn't go
follow that code

Oh, i see, event is not related to the object delete, i guess you have
checked the code and know how it works now ;-)

Maybe you misunderstood - maybe I misunderstood your response... I
didn't go searching through the code, but something just didn't seem
right when looking so I wanted to note it.


Looks like i misunderstood :)


Since I figured your model was RNG my thought process was how does RNG
do it and what is different...

AttachRNG has 3 steps AttachCharDev, AddObject, and AddDevice
DetachRNG has 1 step DelDevice
RemoveRNG has 2 steps DelObject and DetachCharDev


AttachShmem has 2 steps AttachCharDev and AddDevice
DetachShmem has 1 step DelDevice
RemoveShmem has 1 step DetachCharDev

I think my concern was more related to the difference where an RNG has
an Object that gets removed, but Shmem doesn't.


ivshmem device doesn't have a QOM object, so no need to add/del a QOM 
object.


and you can check this doc:

http://wiki.qemu.org/TexiDemo

and see "Inter-VM Shared Memory device" part



ivshmem device could have a object when it use x-memdev, so seems we 
will add DelObject in RemoveShmem in future :) (maybe when someone 
implement ivshmem with hugepage support)



Luyao


After closer inspection now it seems that this event generation needs to
stay. The RemoveShmem code is generating the event not waiting on it
which perhaps is where my thoughts were when I first went through the
changes.


yes, virDomainEventDeviceRemovedNewFromObj() is generating a event and 
it can trigger the callback functions which registered in qemu driver.


Thanks for your help and reply

Luayo


John


Thanks for your quick reply !

Luyao


John

[...]



--
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 4/4] qemu: Implement shared memory device hot-unplug

[lhuang]

On 12/17/2015 08:26 PM, John Ferlan wrote:


On 12/16/2015 09:43 PM, lhuang wrote:


On 12/15/2015 08:39 PM, John Ferlan wrote:

[...]


+if (rc < 0)
+return -1;
+

I know this is a copy of the RemoveRNGDevice; however, this code
doesn't
remove an 'obj'. In fact, if !shmem->server.enabled, then we don't
enter
the monitor at all.

Thus the following event probably won't happen...

I am not sure what your mean is ... i guess your mean the device remove
event we get from qmp monitor won't happen ? we will get that event if
qemu remove shmem device success, it should always happen if qemu really
remove it and there is no bugs on qemu :)


While reviewing I got lazy and didn't check the non hotplug case to how
shmem is added to the vm, but the point I was trying to make is that "if
(shmem->server.enabled)" fails (e.g. is false), then there is no "rc =
qemuMonitorDelObject(priv->mon, objAlias);" call in this API (similar to
RNG code), thus how does the following event get triggered?  Even if the
condition was true, does detaching the char dev cause the event to be
triggered?
I thought the event was related to the DelObject code, but I didn't go
follow that code

Oh, i see, event is not related to the object delete, i guess you have
checked the code and know how it works now ;-)

Maybe you misunderstood - maybe I misunderstood your response... I
didn't go searching through the code, but something just didn't seem
right when looking so I wanted to note it.


Looks like i misunderstood :)


Since I figured your model was RNG my thought process was how does RNG
do it and what is different...

AttachRNG has 3 steps AttachCharDev, AddObject, and AddDevice
DetachRNG has 1 step DelDevice
RemoveRNG has 2 steps DelObject and DetachCharDev


AttachShmem has 2 steps AttachCharDev and AddDevice
DetachShmem has 1 step DelDevice
RemoveShmem has 1 step DetachCharDev

I think my concern was more related to the difference where an RNG has
an Object that gets removed, but Shmem doesn't.


ivshmem device doesn't have a QOM object, so no need to add/del a QOM 
object.


and you can check this doc:

http://wiki.qemu.org/TexiDemo

and see "Inter-VM Shared Memory device" part


After closer inspection now it seems that this event generation needs to
stay. The RemoveShmem code is generating the event not waiting on it
which perhaps is where my thoughts were when I first went through the
changes.


yes, virDomainEventDeviceRemovedNewFromObj() is generating a event and 
it can trigger the callback functions which registered in qemu driver.


Thanks for your help and reply

Luayo


John


Thanks for your quick reply !

Luyao


John

[...]



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

Re: [libvirt] [PATCHv2 4/4] qemu: Implement shared memory device hot-unplug

[lhuang]

On 12/15/2015 08:39 PM, John Ferlan wrote:

[...]


+if (rc < 0)
+return -1;
+

I know this is a copy of the RemoveRNGDevice; however, this code doesn't
remove an 'obj'. In fact, if !shmem->server.enabled, then we don't enter
the monitor at all.

Thus the following event probably won't happen...

I am not sure what your mean is ... i guess your mean the device remove
event we get from qmp monitor won't happen ? we will get that event if
qemu remove shmem device success, it should always happen if qemu really
remove it and there is no bugs on qemu :)


While reviewing I got lazy and didn't check the non hotplug case to how
shmem is added to the vm, but the point I was trying to make is that "if
(shmem->server.enabled)" fails (e.g. is false), then there is no "rc =
qemuMonitorDelObject(priv->mon, objAlias);" call in this API (similar to
RNG code), thus how does the following event get triggered?  Even if the
condition was true, does detaching the char dev cause the event to be
triggered?
I thought the event was related to the DelObject code, but I didn't go
follow that code


Oh, i see, event is not related to the object delete, i guess you have 
checked the code and know how it works now ;-)


Thanks for your quick reply !

Luyao


John

[...]



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

Re: [libvirt] [PATCHv2 1/4] conf: Add helpers to insert/remove/find shmem devices in domain def

[lhuang]

On 12/10/2015 08:50 AM, John Ferlan wrote:


On 11/26/2015 04:06 AM, Luyao Huang wrote:

The helpers will be useful when implementing hotplug and coldplug of
shared memory devices.

Signed-off-by: Luyao Huang 
---
  src/conf/domain_conf.c   | 66 
  src/conf/domain_conf.h   |  7 +
  src/libvirt_private.syms |  3 +++
  3 files changed, 76 insertions(+)


Although there is a v1, it doesn't seem patches 6-10 of that series
(e.g. these patches) were reviewed.  Is that correct?  Just want to make
sure I'm not missing something from someone else's review...

anyway...


here is the review of patch 10:

https://www.redhat.com/archives/libvir-list/2015-July/msg00338.html

patch 6:

https://www.redhat.com/archives/libvir-list/2015-July/msg00319.html

And patch 7-9 were not reviewed, i posted a new version since i 
introduced a new parameter in virDomainShmemFind() and removed audit 
part (I remember Martin will help finish that part, but i am sorry that 
i forgot mention this in some place)



diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 616bf80..cc2a767 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13950,6 +13950,72 @@ virDomainMemoryRemove(virDomainDefPtr def,
  }
  
  
+int

+virDomainShmemInsert(virDomainDefPtr def,
+ virDomainShmemDefPtr shmem)

I see you've followed the virDomainHostdevInsert model...


+{
+return VIR_APPEND_ELEMENT(def->shmems, def->nshmems, shmem);
+}
+
+

Would be nice to have an intro comment explaining the params (for other
two as well), but this one is a bit more "interesting" because of arg3.
  This includes adding return value.


Okay, i will add them in next version.


+ssize_t
+virDomainShmemFind(virDomainDefPtr def,

More or less follows virDomainHostdevFind, but could also be more
explicit using "FindIndex" or "FindIdx"... Not that important.


+   virDomainShmemDefPtr shmem,
+   bool fullmatch)

I think this is more "match name only".  It could also be an "unsigned
int flags" where the flags is an enum that could dictate the level of
match required by the caller (perhaps overkill for what is necessary,
but flags just seems to be a better option in my opinion.  A passed
value of 0 for flags would indicate to match everything


interesting idea, i will have a try with your option, you will see it in 
next version :)



Beyond that, following virDomainHostdevFind - you could perhaps return
an int. We know size_t will be 0 to def->nshmems...


got it.


+{
+size_t i;
+
+for (i = 0; i < def->nshmems; i++) {
+ virDomainShmemDefPtr tmpshmem = def->shmems[i];
+
+ if (STREQ(shmem->name, tmpshmem->name)) {
+ if (!fullmatch)
+ return i;
+ } else {
+ continue;
+ }

Perhaps cleaner as:

 if (STRNEQ(shmem->name, tmpshmem->name))
 continue;

 if (!fullmatch)
 return i;

You could also use matchnameonly or (flags &
VIR_DOMAIN_SHMEM_NAME_MATCH_ONLY)


Okay


+
+ if (shmem->size != tmpshmem->size)
+ continue;
+

Eventually someone could add other flags such as

  -> match name and size only...
  -> match name, size, and server
  -> match name, size, server, and msi
  -> match name, size, server, msi, and info/address



I see.


+ if (shmem->server.enabled != tmpshmem->server.enabled ||
+ (shmem->server.enabled &&
+  STRNEQ_NULLABLE(shmem->server.chr.data.nix.path,
+  tmpshmem->server.chr.data.nix.path)))
+ continue;
+
+ if (shmem->msi.enabled != tmpshmem->msi.enabled ||
+ (shmem->msi.enabled &&
+  (shmem->msi.vectors != tmpshmem->msi.vectors ||
+   shmem->msi.ioeventfd != tmpshmem->msi.ioeventfd)))
+ continue;
+
+if (shmem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+!virDomainDeviceInfoAddressIsEqual(>info, >info))
+continue;

Matching address is such a touchy thing, I know why it's here to be
complete, but since the remove device doesn't have to add it to the XML
passed in, is there a need?   I guess we can leave it for now unless
someone else offers up a different opinion.



Indeed, i will remove this check in next version.


+
+break;

Why not just return i; here?


You are right, i will fix this and ...


+}
+
+if (i < def->nshmems)
+return i;

Removing need for this check


... this in next version

Thanks a lot for your reviews and suggestions.

Luyao


+
+return -1;
+}
+
+
+virDomainShmemDefPtr
+virDomainShmemRemove(virDomainDefPtr def,
+ size_t idx)
+{
+virDomainShmemDefPtr ret = def->shmems[idx];
+
+VIR_DELETE_ELEMENT(def->shmems, idx, def->nshmems);
+
+return ret;
+}
+
+
  char *
  virDomainDefGetDefaultEmulator(virDomainDefPtr def,
 

Re: [libvirt] [PATCHv2 3/4] qemu: Implement share memory device hot-plug

[lhuang]

On 12/10/2015 09:23 AM, John Ferlan wrote:

$SUBJ

s/share/shared


thanks for pointing out this



On 11/26/2015 04:06 AM, Luyao Huang wrote:

Signed-off-by: Luyao Huang 
---
  src/qemu/qemu_driver.c  | 10 -
  src/qemu/qemu_hotplug.c | 58 +
  src/qemu/qemu_hotplug.h |  3 +++
  3 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5ded9ef..3c25c07 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7784,6 +7784,15 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
  dev->data.memory = NULL;
  break;
  
+case VIR_DOMAIN_DEVICE_SHMEM:

+ret = qemuDomainAttachShmemDevice(driver, vm,
+  dev->data.shmem);
+if (!ret) {
+alias = dev->data.shmem->info.alias;
+dev->data.shmem = NULL;
+}
+break;
+
  case VIR_DOMAIN_DEVICE_NONE:
  case VIR_DOMAIN_DEVICE_FS:
  case VIR_DOMAIN_DEVICE_INPUT:
@@ -7795,7 +7804,6 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
  case VIR_DOMAIN_DEVICE_SMARTCARD:
  case VIR_DOMAIN_DEVICE_MEMBALLOON:
  case VIR_DOMAIN_DEVICE_NVRAM:
-case VIR_DOMAIN_DEVICE_SHMEM:
  case VIR_DOMAIN_DEVICE_TPM:
  case VIR_DOMAIN_DEVICE_PANIC:
  case VIR_DOMAIN_DEVICE_LAST:
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 8804d3d..c5e544d 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1882,6 +1882,64 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
  }
  
  

It seems this is modeled after qemuDomainAttachRNGDevice, right?


Right :)


+int
+qemuDomainAttachShmemDevice(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+virDomainShmemDefPtr shmem)
+{
+int ret = -1;
+qemuDomainObjPrivatePtr priv = vm->privateData;
+char *devstr = NULL;
+char *charAlias = NULL;
+
+if (virAsprintf(>info.alias, "shmem%zu", vm->def->nshmems) < 0)
+return -1;
+
+if (VIR_REALLOC_N(vm->def->shmems, vm->def->nshmems + 1) < 0)
+return -1;
+
+if ((shmem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
+ shmem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
+ (virDomainPCIAddressEnsureAddr(priv->pciaddrs, >info) < 0))
+return -1;
+
+if (!(devstr = qemuBuildShmemDevStr(vm->def, shmem, priv->qemuCaps)))
+goto cleanup;
+



+if (virAsprintf(, "char%s", shmem->info.alias) < 0)
+goto cleanup;
+

This seems to be up to a 2 stage process "if" server.enabled" is true"


Indeed, i will fix it in next version




+qemuDomainObjEnterMonitor(driver, vm);
+
+if (shmem->server.enabled &&
+qemuMonitorAttachCharDev(priv->mon, charAlias,
+ >server.chr) < 0) {

Instead of the following change to:

goto failchardev;

and the {} won't be necessary


...

Instead of the following change to

goto failadddevice;

and the {} won't be necessary

...

Again following RNG model - this should have a

 if (*Exit*() < 0) {
 vm = NULL;
 goto cleanup
 }

...

And of course the auditing change as well.

...
Following RNG the && vm would be used here... See your patch commits 
'0ed3b3353' and '980b265d0'

...

  failadddevice:
 if (shmem->server.enabled)
 ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));

  failchardev:
 ignore_value(qemuDomainObjExitMonitor(driver, vm));
 goto cleanup;



Nice improve, i must forgot a old problem that the  will be changed 
after qemu unexpected exit during enter the monitor.



Hope this all makes sense (it's been a long day ;-))


Thanks a lot for your review and suggestion, and i am sorry that i 
didn't reply your mails when i saw them (I'm kind of overwhelmed lately :) )


Have a nice day !

Luyao



John

+}
+
+
  static int
  qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index 4140da3..60137a6 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -109,6 +109,9 @@ int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
  int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainRNGDefPtr rng);
+int qemuDomainAttachShmemDevice(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+virDomainShmemDefPtr shmem);
  
  int

  qemuDomainChrInsert(virDomainDefPtr vmdef,



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

Re: [libvirt] [PATCHv2 4/4] qemu: Implement shared memory device hot-unplug

[lhuang]

On 12/10/2015 09:43 AM, John Ferlan wrote:


On 11/26/2015 04:07 AM, Luyao Huang wrote:

Signed-off-by: Luyao Huang 
---
  src/qemu/qemu_driver.c  |  4 ++-
  src/qemu/qemu_hotplug.c | 94 -
  src/qemu/qemu_hotplug.h |  3 ++
  3 files changed, 99 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3c25c07..9e7429a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7881,6 +7881,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
  case VIR_DOMAIN_DEVICE_MEMORY:
  ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory);
  break;
+case VIR_DOMAIN_DEVICE_SHMEM:
+ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem);
+break;
  
  case VIR_DOMAIN_DEVICE_FS:

  case VIR_DOMAIN_DEVICE_INPUT:
@@ -7892,7 +7895,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
  case VIR_DOMAIN_DEVICE_SMARTCARD:
  case VIR_DOMAIN_DEVICE_MEMBALLOON:
  case VIR_DOMAIN_DEVICE_NVRAM:
-case VIR_DOMAIN_DEVICE_SHMEM:
  case VIR_DOMAIN_DEVICE_REDIRDEV:
  case VIR_DOMAIN_DEVICE_NONE:
  case VIR_DOMAIN_DEVICE_TPM:
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c5e544d..4ab05f3 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3347,6 +3347,45 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
  }
  
  
+static int

+qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+virDomainShmemDefPtr shmem)
+{
+virObjectEventPtr event;
+char *charAlias = NULL;
+qemuDomainObjPrivatePtr priv = vm->privateData;
+ssize_t idx;
+int rc = 0;
+
+VIR_DEBUG("Removing shared memory device %s from domain %p %s",
+  shmem->info.alias, vm, vm->def->name);
+
+if (shmem->server.enabled) {
+if (virAsprintf(, "char%s", shmem->info.alias) < 0)
+return -1;
+
+qemuDomainObjEnterMonitor(driver, vm);
+rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
+VIR_FREE(charAlias);
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+return -1;
+}
+

Auditing code?


I have removed them since i remember @Martin will finish the audit part.


+if (rc < 0)
+return -1;
+

I know this is a copy of the RemoveRNGDevice; however, this code doesn't
remove an 'obj'. In fact, if !shmem->server.enabled, then we don't enter
the monitor at all.

Thus the following event probably won't happen...


I am not sure what your mean is ... i guess your mean the device remove 
event we get from qmp monitor won't happen ? we will get that event if 
qemu remove shmem device success, it should always happen if qemu really 
remove it and there is no bugs on qemu :)



+if ((event = virDomainEventDeviceRemovedNewFromObj(vm, shmem->info.alias)))
+qemuDomainEventQueue(driver, event);
+
+if ((idx = virDomainShmemFind(vm->def, shmem, true)) >= 0)
+virDomainShmemRemove(vm->def, idx);
+qemuDomainReleaseDeviceAddress(vm, >info, NULL);
+virDomainShmemDefFree(shmem);
+return 0;
+}
+
+
  int
  qemuDomainRemoveDevice(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
@@ -3378,6 +3417,10 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
  ret = qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory);
  break;
  
+case VIR_DOMAIN_DEVICE_SHMEM:

+ret = qemuDomainRemoveShmemDevice(driver, vm, dev->data.shmem);
+break;
+
  case VIR_DOMAIN_DEVICE_NONE:
  case VIR_DOMAIN_DEVICE_LEASE:
  case VIR_DOMAIN_DEVICE_FS:
@@ -3391,7 +3434,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
  case VIR_DOMAIN_DEVICE_SMARTCARD:
  case VIR_DOMAIN_DEVICE_MEMBALLOON:
  case VIR_DOMAIN_DEVICE_NVRAM:
-case VIR_DOMAIN_DEVICE_SHMEM:
  case VIR_DOMAIN_DEVICE_TPM:
  case VIR_DOMAIN_DEVICE_PANIC:
  case VIR_DOMAIN_DEVICE_LAST:
@@ -4378,3 +4420,53 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
  qemuDomainResetDeviceRemoval(vm);
  return ret;
  }
+
+
+int
+qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+virDomainShmemDefPtr shmem)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+ssize_t idx;
+virDomainShmemDefPtr tmpshmem;
+int rc;
+int ret = -1;
+
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("qemu does not support -device"));
+return -1;
+}

Well it's here, but not in AttachShmemDevice...


I must forgot  add it in the AttachShmemDevice, thanks for pointing out.


+
+if ((idx = virDomainShmemFind(vm->def, shmem, true)) < 0) {

Again we could use a 'flags' argument instead...  Of course there isn't
a "false" argument to any 

Re: [libvirt] [PATCHv2 2/4] qemu: Implement shared memory device cold (un)plug

[lhuang]

On 12/10/2015 08:53 AM, John Ferlan wrote:


On 11/26/2015 04:06 AM, Luyao Huang wrote:

Add support for using the attach/detach device APIs on the inactive
configuration to add/del shared memory devices.

Signed-off-by: Luyao Huang 
---
  src/libvirt_private.syms |  1 +
  src/qemu/qemu_driver.c   | 21 +++--
  2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 88c2c53..c89d27a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -451,6 +451,7 @@ virDomainSaveStatus;
  virDomainSaveXML;
  virDomainSeclabelTypeFromString;
  virDomainSeclabelTypeToString;
+virDomainShmemDefFree;
  virDomainShmemFind;
  virDomainShmemInsert;
  virDomainShmemRemove;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 65ccf99..5ded9ef 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8174,6 +8174,15 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
  dev->data.memory = NULL;
  break;
  
+case VIR_DOMAIN_DEVICE_SHMEM:

+if (virDomainShmemInsert(vmdef, dev->data.shmem) < 0)
+return -1;
+dev->data.shmem = NULL;
+
+if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
+return -1;
+break;
+
  case VIR_DOMAIN_DEVICE_INPUT:
  case VIR_DOMAIN_DEVICE_SOUND:
  case VIR_DOMAIN_DEVICE_VIDEO:
@@ -8183,7 +8192,6 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
  case VIR_DOMAIN_DEVICE_SMARTCARD:
  case VIR_DOMAIN_DEVICE_MEMBALLOON:
  case VIR_DOMAIN_DEVICE_NVRAM:
-case VIR_DOMAIN_DEVICE_SHMEM:
  case VIR_DOMAIN_DEVICE_REDIRDEV:
  case VIR_DOMAIN_DEVICE_NONE:
  case VIR_DOMAIN_DEVICE_TPM:
@@ -8310,6 +8318,16 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
  virDomainMemoryDefFree(virDomainMemoryRemove(vmdef, idx));
  break;
  
+case VIR_DOMAIN_DEVICE_SHMEM:

+if ((idx = virDomainShmemFind(vmdef, dev->data.shmem, true)) < 0) {

Here rather than 'true' which to me only has meaning if I go read the
code... Of course same thing holds true when passing a 0 (zero) flags value.

BTW: My idea around flags matches what I recently added for
VolReadErrorMode in storage_backend.h... But for this it'd be something
like the following in (I assume) domain_conf.h near _virDomainShmemDef.

 /* DomainShmemMatchFlags
  * Flags to dictate the level of matching when searching for a
  * shmem object in the domain 'nshmems' list.
  */
 enum {
 VIR_DOMAIN_SHMEM_MATCH_NAME_ONLY = 1 << 0,
 }


In general though it seems fine.


Okay, i have saw your comment mentioned this in another mail, and i will 
post a new version include your idea.


Thanks a lot for your review and suggestion.

Luyao


John


+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("no matching shared memory device was found"));
+return -1;
+}
+
+virDomainShmemDefFree(virDomainShmemRemove(vmdef, idx));
+break;
+
  case VIR_DOMAIN_DEVICE_INPUT:
  case VIR_DOMAIN_DEVICE_SOUND:
  case VIR_DOMAIN_DEVICE_VIDEO:
@@ -8319,7 +8337,6 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
  case VIR_DOMAIN_DEVICE_SMARTCARD:
  case VIR_DOMAIN_DEVICE_MEMBALLOON:
  case VIR_DOMAIN_DEVICE_NVRAM:
-case VIR_DOMAIN_DEVICE_SHMEM:
  case VIR_DOMAIN_DEVICE_REDIRDEV:
  case VIR_DOMAIN_DEVICE_NONE:
  case VIR_DOMAIN_DEVICE_TPM:



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

Re: [libvirt] [PATCH v2 00/10] Make loading domains with invalid XML possible

[lhuang]

On 12/02/2015 01:35 AM, Martin Kletzander wrote:

We always had to deal with new parsing errors in a weird way.  All of
them needed to go into functions starting the domains.  That messes up
the code, it's confusing to newcomers and so on.

I once had an idea that we can handle this stuff.  We know what
failed, we have the XML that failed parsing and if the problem is not
in the domain name nor UUID, we can create a domain object out of that
as well.  This way we are able to do something we weren't with this
series applied.  Example follows.

Assume "dummy" is a domain with invalid XML (I just modified the
file).  Now, just for the purpose of this silly example, let's say we
allowed domains with archtecture x86_*, but now we've realized that
x86_64 is the only one we want to allow, but there already is a domain
defined that has .  With the current master,
the domain would be lost, so we would need to modify the funstion
starting the domain (e.g. qemuProcessStart for qemu domain).  However,
with this series, it behaves like this:

   # virsh list --all --reason
  IdName   State  Reason
 ---
  - dummy  shut off   invalid XML

   # virsh domstate --reason dummy
 shut off (invalid XML)

   # virsh start dummy
   error: Failed to start domain dummy
   error: XML error: domain 'dummy' was not loaded due to an XML error
   (unsupported configuration: Unknown architecture x86_256), please
   redefine it

   # VISUAL='sed -i s/x86_256/x86_64/' virsh edit dummy
   Domain dummy XML configuration edited.

   # virsh domstate --reason dummy
   shut off (unknown)

   # virsh start dummy
   Domain dummy started

This is a logical next step for us to clean and separate parsing and
starting, getting rid of some old code without sacrifying compatibility
and maybe generating parser code in the future.

v2:
  - rebased on top of current master (with virdomainobjlist.c)
  - only disallow starting domains with invalid definitions (change
done in v1), but allow re-defining them
  - add support for "virsh list --reason" to better excercise the
feature added in this patchset

v1:
  - rebase
  - don't allow starting domains with invalid state
  - https://www.redhat.com/archives/libvir-list/2015-November/msg01127.html

RFC: https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html


Martin Kletzander (10):
   conf, virsh: Add new domain shutoff reason
   virsh: Refactor the table output of the list command
   virsh: Add support for list -reason
   qemu: Few whitespace cleanups
   conf: Extract name-parsing into its own function
   conf: Extract UUID parsing into its own function
   conf: Optionally keep domains with invalid XML, but don't allow
 starting them
   qemu: Don't lookup invalid domains unless specified otherwise
   qemu: Prepare basic APIs to handle invalid defs
   qemu: Load domains with invalid XML on start

  include/libvirt/libvirt-domain.h |   2 +
  src/bhyve/bhyve_driver.c |   2 +
  src/conf/domain_conf.c   | 121 +++
  src/conf/domain_conf.h   |   7 +++
  src/conf/virdomainobjlist.c  |  71 +--
  src/conf/virdomainobjlist.h  |   1 +
  src/libvirt_private.syms |   1 +
  src/libxl/libxl_driver.c |   3 +
  src/lxc/lxc_driver.c |   3 +
  src/qemu/qemu_driver.c   |  64 +
  src/uml/uml_driver.c |   2 +
  tests/virshtest.c|  30 +++---
  tools/virsh-domain-monitor.c |  60 ---
  tools/virsh.pod  |   5 +-
  14 files changed, 303 insertions(+), 69 deletions(-)




I am glad to see these patches you told me before.

And i have test your patches and found some problem until now:

1. if guest xml have private info, libvirt will output it if xml is invalid:

# virsh list --all --reason
 IdName   State  Reason


 - test3  shut off   invalid XML

# virsh -r dumpxml test3
...
     graphics[i]);

Thread 1 (Thread 0x7fcbe835c8c0 (LWP 21589)):
#0  0x7fcbe773af87 in virDomainDefFree (def=0x7fcbc423d980) at 
conf/domain_conf.c:2327
#1  0x7fcbe773b903 in virDomainObjDispose (obj=0x7fcbc4235fe0) at 
conf/domain_conf.c:2487
#2  0x7fcbe76f182b in virObjectUnref (anyobj=) at 
util/virobject.c:265
#3  0x7fcbe76d0ac9 in 

Re: [libvirt] [PATCH v2 00/10] Make loading domains with invalid XML possible

[lhuang]

On 12/02/2015 05:13 PM, Martin Kletzander wrote:

On Wed, Dec 02, 2015 at 04:46:20PM +0800, lhuang wrote:



On 12/02/2015 01:35 AM, Martin Kletzander wrote:

We always had to deal with new parsing errors in a weird way.  All of
them needed to go into functions starting the domains.  That messes up
the code, it's confusing to newcomers and so on.

I once had an idea that we can handle this stuff.  We know what
failed, we have the XML that failed parsing and if the problem is not
in the domain name nor UUID, we can create a domain object out of that
as well.  This way we are able to do something we weren't with this
series applied.  Example follows.

Assume "dummy" is a domain with invalid XML (I just modified the
file).  Now, just for the purpose of this silly example, let's say we
allowed domains with archtecture x86_*, but now we've realized that
x86_64 is the only one we want to allow, but there already is a domain
defined that has .  With the current master,
the domain would be lost, so we would need to modify the funstion
starting the domain (e.g. qemuProcessStart for qemu domain). However,
with this series, it behaves like this:

  # virsh list --all --reason
 IdName   State  Reason
---
 - dummy  shut off   invalid XML

  # virsh domstate --reason dummy
shut off (invalid XML)

  # virsh start dummy
  error: Failed to start domain dummy
  error: XML error: domain 'dummy' was not loaded due to an XML error
  (unsupported configuration: Unknown architecture x86_256), please
  redefine it

  # VISUAL='sed -i s/x86_256/x86_64/' virsh edit dummy
  Domain dummy XML configuration edited.

  # virsh domstate --reason dummy
  shut off (unknown)

  # virsh start dummy
  Domain dummy started

This is a logical next step for us to clean and separate parsing and
starting, getting rid of some old code without sacrifying compatibility
and maybe generating parser code in the future.

v2:
 - rebased on top of current master (with virdomainobjlist.c)
 - only disallow starting domains with invalid definitions (change
   done in v1), but allow re-defining them
 - add support for "virsh list --reason" to better excercise the
   feature added in this patchset

v1:
 - rebase
 - don't allow starting domains with invalid state
 - 
https://www.redhat.com/archives/libvir-list/2015-November/msg01127.html


RFC: 
https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html 




Martin Kletzander (10):
  conf, virsh: Add new domain shutoff reason
  virsh: Refactor the table output of the list command
  virsh: Add support for list -reason
  qemu: Few whitespace cleanups
  conf: Extract name-parsing into its own function
  conf: Extract UUID parsing into its own function
  conf: Optionally keep domains with invalid XML, but don't allow
starting them
  qemu: Don't lookup invalid domains unless specified otherwise
  qemu: Prepare basic APIs to handle invalid defs
  qemu: Load domains with invalid XML on start

 include/libvirt/libvirt-domain.h |   2 +
 src/bhyve/bhyve_driver.c |   2 +
 src/conf/domain_conf.c   | 121 
+++

 src/conf/domain_conf.h   |   7 +++
 src/conf/virdomainobjlist.c  |  71 +--
 src/conf/virdomainobjlist.h  |   1 +
 src/libvirt_private.syms |   1 +
 src/libxl/libxl_driver.c |   3 +
 src/lxc/lxc_driver.c |   3 +
 src/qemu/qemu_driver.c   |  64 +
 src/uml/uml_driver.c |   2 +
 tests/virshtest.c|  30 +++---
 tools/virsh-domain-monitor.c |  60 ---
 tools/virsh.pod  |   5 +-
 14 files changed, 303 insertions(+), 69 deletions(-)




I am glad to see these patches you told me before.

And i have test your patches and found some problem until now:

1. if guest xml have private info, libvirt will output it if xml is 
invalid:


# virsh list --all --reason
IdName   State  Reason


- test3  shut off   invalid XML

# virsh -r dumpxml test3
...
  <wrong place


It should be in the same place as it was saved on the disk.


oh, seems you misunderstood, i mean there is the invalid place which 
make xml invalid :)





 <show passwd



Oh, that's definitely what's not suppose to happen, but since we cannot
parse it, I will just disable the dumpxml for read-only connections.



okay, sounds good.


2. vcpucount command output looks strange (small problem :) ):

# virsh vcpucount test3



Oh, so my guess was that this happens because virsh calls dumpxml, then
searches for the info.  But I checked and it calls an API that should be
blocked.  I'm guessing that happened because of the uuid parsing has
gone wrong (be

Re: [libvirt] Ten years of libvirt

[lhuang]

Wow !! so cool !

Luyao

On 11/03/2015 04:07 PM, Cedric Bosdonnat wrote:

Hi all,

This is my present for this 10 years anniversary: enjoy watching how you
made libvirt grow fantastically in this gource video:

https://vimeo.com/144404779

Happy anniversary (with delay)!

--
Cedric

On Mon, 2015-11-02 at 09:57 +0100, Michal Privoznik wrote:

Dear list,

join me on this big day in congratulations to libvirt that has just
turned 10 and is starting new decade of its life. At the same time big
thanks to all who contributed in any way.

Cheers!

Michal

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



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


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

Re: [libvirt] [PATCH] util: set error if DAD is not finished

[lhuang]

On 10/30/2015 02:15 AM, Laine Stump wrote:

On 10/29/2015 08:32 AM, Maxim Perevedentsev wrote:

On 10/29/2015 12:47 PM, Luyao Huang wrote:

If DAD not finished in 5 seconds, user will get an
unknown error like this:

  # virsh net-start ipv6
  error: Failed to start network ipv6
  error: An error occurred, but the cause is unknown

Call virReportError to set an error.

Signed-off-by: Luyao Huang 
---
I found the DAD will take 7 seconds
on my machine, and i cannot create a network which
use ipv6 now :( . Can we offer a way allow user to change this
timeout ? maybe add a configuration file option in
libvirtd.conf.


Could you please send the related records of your sysctl -a?
Max DAD timeout should be
rand() % net.ipv6.conf.default.router_solicitation_delay + 
net.ipv6.conf.default.dad_transmits * 
net.ipv6.neigh.default.retrans_time_ms
I wonder whether my calculations were faulty or it is your specific 
configuration.




On my system, these are set to 1, 1, and 1000, and I've found that DAD 
takes something between 5.7 and 6.8 seconds to complete (it was at the 
lower end with a single IP address, and at the high end with 70 IP 
addresses (or also with 20 IPs, so I don't think it's going to get 
substantially higher). (Too bad I didn't do this *before* I pushed, 
rather than relying on a successful build and reports of proper 
operation on your system :-/)


Based on that, I think it makes sense to push a patch that sets the 
timeout to 20 seconds (and push Luyao's patch ragardless). Since the 
timeout is meant to catch an "infinite" wait, I think 20 seconds is 
okay; I don't want to add yet another tunable parameter unless we 
really need to.




yes, change the timeout to 20 seconds is a good way to avoid this issue. 
I was trying to figure out why DAD take so long time on my machine.


And thanks a lot for your quick review.

Luyao

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

Re: [libvirt] [PATCH] util: set error if DAD is not finished

[lhuang]

On 10/29/2015 08:32 PM, Maxim Perevedentsev wrote:

On 10/29/2015 12:47 PM, Luyao Huang wrote:

If DAD not finished in 5 seconds, user will get an
unknown error like this:

  # virsh net-start ipv6
  error: Failed to start network ipv6
  error: An error occurred, but the cause is unknown

Call virReportError to set an error.

Signed-off-by: Luyao Huang 
---
I found the DAD will take 7 seconds
on my machine, and i cannot create a network which
use ipv6 now :( . Can we offer a way allow user to change this
timeout ? maybe add a configuration file option in
libvirtd.conf.


Could you please send the related records of your sysctl -a?
Max DAD timeout should be
rand() % net.ipv6.conf.default.router_solicitation_delay + 
net.ipv6.conf.default.dad_transmits * 
net.ipv6.neigh.default.retrans_time_ms
I wonder whether my calculations were faulty or it is your specific 
configuration.




I haven't change them before, and they are:

net.ipv6.conf.default.router_solicitation_delay = 1
net.ipv6.conf.default.dad_transmits = 1
net.ipv6.neigh.default.retrans_time_ms = 1000

Thanks your quick reply.

Luyao

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

Re: [libvirt] [PATCH] qemu: fix wrong nodeset info when numatune placement is auto

[lhuang]

On 10/29/2015 04:54 AM, John Ferlan wrote:


On 10/12/2015 05:28 AM, Luyao Huang wrote:

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

Since commit 9deb96f remove check the nodeset from cgroup for
a running vm, the numatune nodeset reporting for those guest
numatune placement is auto is not right. Pass the autonodeset
to virDomainNumatuneFormatNodeset() to fix this.

Signed-off-by: Luyao Huang 
---
  src/qemu/qemu_driver.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)


Seems reasonable - although perhaps the bug was introduced by commit id
'43b67f2e7' which added the call to virDomainNumatuneFormatNodeset if
the CpusetMems call fails.  There's also some more history I put into
the commit message before pushing.


Nice commit message, thanks a lot for your help and review.

Luyao


John



diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8cd5ee3..8ca55dd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10177,6 +10177,7 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
  size_t i;
  virDomainObjPtr vm = NULL;
  virDomainNumatuneMemMode tmpmode = VIR_DOMAIN_NUMATUNE_MEM_STRICT;
+qemuDomainObjPrivatePtr priv;
  char *nodeset = NULL;
  int ret = -1;
  virDomainDefPtr def = NULL;
@@ -10187,6 +10188,7 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
  
  if (!(vm = qemuDomObjFromDomain(dom)))

  return -1;
+priv = vm->privateData;
  
  if (virDomainGetNumaParametersEnsureACL(dom->conn, vm->def) < 0)

  goto cleanup;
@@ -10214,7 +10216,8 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
  break;
  
  case 1: /* fill numa nodeset here */

-nodeset = virDomainNumatuneFormatNodeset(def->numa, NULL, -1);
+nodeset = virDomainNumatuneFormatNodeset(def->numa,
+ priv->autoNodeset, -1);
  if (!nodeset ||
  virTypedParameterAssign(param, VIR_DOMAIN_NUMA_NODESET,
  VIR_TYPED_PARAM_STRING, nodeset) < 0)



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

Re: [libvirt] [PATCH] util: remove unnecessary code to avoid error overwrite

[lhuang]

On 10/29/2015 05:21 AM, John Ferlan wrote:


On 10/28/2015 03:06 AM, Luyao Huang wrote:

virCgroupRemove use VIR_ERROR to log the error and won't
change the virLastErr in the thread, so no need do this.

Signed-off-by: Luyao Huang 
---
  src/util/vircgroup.c | 10 --
  1 file changed, 10 deletions(-)


Although virCgroupRemove calls virCgroupRemoveRecursively to remove the
cgroups and as noted in the recursive function the usage of VIR_ERROR
instead of virReportError is true.

However, virCgroupRemove also calls virCgroupPathOfController which
calls virReport[System]Error so I don't think we can 'remove' these
overwrite protections... Consider the case when the recursive function
is never called because we keep continuing.


You're right, i just check virCgroupRemoveRecursively() , and 
virCgroupPathOfController() calls virReport[System]Error. Then i think 
we need avoid  the error overwrite when call virCgroupPathOfController() 
in virCgroupRemove(). Like this:


diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 0379c2e..3ba6da3 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3524,15 +3524,20 @@ virCgroupRemove(virCgroupPtr group)
 if (STREQ(group->controllers[i].placement, "/"))
 continue;

+virErrorPtr saved = virSaveLastError();
 if (virCgroupPathOfController(group,
   i,
   NULL,
-  ) != 0)
+  ) != 0) {
+virSetError(saved);
+virFreeError(saved);
 continue;
+}

 VIR_DEBUG("Removing cgroup %s and all child cgroups", grppath);
 rc = virCgroupRemoveRecursively(grppath);
 VIR_FREE(grppath);
+virFreeError(saved);
 }
 VIR_DEBUG("Done removing cgroup %s", group->path);



Since we just skip remove the current cgroup if 
virCgroupPathOfController get failure, and return rc at last.So i think 
we really don't care about the error in virCgroupPathOfController in 
this place , and we should add some codes to avoid the error overwrite here.


Thanks a lot for your review and help.

Luyao


John

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 0379c2e..a120a15 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1670,13 +1670,8 @@ virCgroupNewMachineSystemd(const char *name,
  }
  
  if (virCgroupAddTask(*group, pidleader) < 0) {

-virErrorPtr saved = virSaveLastError();
  virCgroupRemove(*group);
  virCgroupFree(group);
-if (saved) {
-virSetError(saved);
-virFreeError(saved);
-}
  }
  
  ret = 0;

@@ -1728,13 +1723,8 @@ virCgroupNewMachineManual(const char *name,
  goto cleanup;
  
  if (virCgroupAddTask(*group, pidleader) < 0) {

-virErrorPtr saved = virSaveLastError();
  virCgroupRemove(*group);
  virCgroupFree(group);
-if (saved) {
-virSetError(saved);
-virFreeError(saved);
-}
  }
  
   done:




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

Re: [libvirt] [PATCH] conf: Add serial target type to ABI stability check

[lhuang]

On 10/27/2015 05:45 PM, Martin Kletzander wrote:

On Wed, Oct 21, 2015 at 03:14:03PM +0800, Luyao Huang wrote:

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

There is no ABI check for serial target type attribute, just
add it.

Signed-off-by: Luyao Huang 
---
src/conf/domain_conf.c | 8 
1 file changed, 8 insertions(+)



ACK && Pushed


Thanks a lot for your review.

Luyao




diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 02cc8ad..a31bc05 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17225,6 +17225,14 @@ static bool
virDomainSerialDefCheckABIStability(virDomainChrDefPtr src,
virDomainChrDefPtr dst)
{
+if (src->targetType != dst->targetType) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Target serial type %s does not match 
source %s"),

+ virDomainChrSerialTargetTypeToString(dst->targetType),
+ virDomainChrSerialTargetTypeToString(src->targetType));
+return false;
+}
+
if (src->target.port != dst->target.port) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
   _("Target serial port %d does not match source 
%d"),

--
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 migration flags undefinesource cannot work

[lhuang]

On 10/27/2015 05:45 PM, Martin Kletzander wrote:

On Tue, Oct 27, 2015 at 04:53:59PM +0800, Luyao Huang wrote:

In commit f41be296, we moved vm->persistent check into
qemuDomainRemoveInactive, but we didn't change the vm->persistent
before call qemuDomainRemoveInactive in some place before and just
call it to remove the inactive vm.

Signed-off-by: Luyao Huang 
---
src/qemu/qemu_migration.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)



ACK to all three, pushed now.


Thanks a lot for your review.

Luyao




diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index b53491a..2abf2ee 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3912,8 +3912,10 @@ qemuMigrationConfirm(virConnectPtr conn,

qemuMigrationJobFinish(driver, vm);
if (!virDomainObjIsActive(vm)) {
-if (flags & VIR_MIGRATE_UNDEFINE_SOURCE)
+if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) {
virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, 
vm);

+vm->persistent = 0;
+}
qemuDomainRemoveInactive(driver, vm);
}

@@ -5405,8 +5407,10 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver,

qemuMigrationJobFinish(driver, vm);
if (!virDomainObjIsActive(vm) && ret == 0) {
-if (flags & VIR_MIGRATE_UNDEFINE_SOURCE)
+if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) {
virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, 
vm);

+vm->persistent = 0;
+}
qemuDomainRemoveInactive(driver, vm);
}

--
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] virsh: fix no error when pass a count <= 0 to setvcpus

[lhuang]

On 10/22/2015 03:32 PM, Andrea Bolognani wrote:

On Thu, 2015-10-22 at 11:27 +0800, Luyao Huang wrote:

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

When count <= 0, the client exit without set an error.

Signed-off-by: Luyao Huang 
---
v2:
- use vshCommandOptUInt to forbid negative number,
   and check if count is zero. (thanks Peter and Andrea)

  tools/virsh-domain.c | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)

Pushed after improving the commit message.


Thanks a lot for your quick review and help !


Cheers.



Luyao

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

Re: [libvirt] [PATCHv2 3/3] util: Produce friendlier error message to user

[lhuang]

On 10/21/2015 09:58 PM, John Ferlan wrote:


On 10/21/2015 12:13 AM, Luyao Huang wrote:

Commit 1c24cfe9 fix the problem in virNumaSetPagePoolSize,
this patch just like it and fix the issue in another function.

when user use freepages and specify a invalid node or page size,
will get error like this:

   # virsh freepages 0 1
   error: Failed to open file 
'/sys/devices/system/node/node0/hugepages/hugepages-1kB/free_hugepages':No such 
file or directory

Add two checks to catch this and therefore produce much more
friendlier error messages.

Signed-off-by: Luyao Huang 
---
  src/util/virnuma.c | 38 ++
  1 file changed, 38 insertions(+)


Now that I see things in their final form, one more suggestion which I
can take care of...   You've repeated the same "if (!virFileExists)"
check and error messages for all 3 callers to virNumaGetHugePageInfo.

So I think moving that check into virNumaGetHugePageInfo would now be
appropriate (see the attached patch which I'll squash)

I'll also fix up the commit messages and push some time later today


Oh, right, i should move the check in virNumaGetHugePageInfo to avoid 
code duplicated. Your patch looks good to me. And thanks a lot for your 
quick review and lots of help!



John


Luyao


diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 815cbc1..cd000f9 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -553,6 +553,25 @@ virNumaGetHugePageInfo(int node,
 page_size, "nr_hugepages") < 0)
  goto cleanup;
  
+if (!virFileExists(path)) {

+if (node != -1) {
+if (!virNumaNodeIsAvailable(node)) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("NUMA node %d is not available"),
+   node);
+} else {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("page size %u is not available on node 
%d"),
+   page_size, node);
+}
+} else {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("page size %u is not available"),
+   page_size);
+}
+goto cleanup;
+}
+
  if (virFileReadAll(path, 1024, ) < 0)
  goto cleanup;
  
@@ -572,6 +591,25 @@ virNumaGetHugePageInfo(int node,

 page_size, "free_hugepages") < 0)
  goto cleanup;
  
+if (!virFileExists(path)) {

+if (node != -1) {
+if (!virNumaNodeIsAvailable(node)) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("NUMA node %d is not available"),
+   node);
+} else {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("page size %u is not available on node 
%d"),
+   page_size, node);
+}
+} else {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("page size %u is not available"),
+   page_size);
+}
+goto cleanup;
+}
+
  if (virFileReadAll(path, 1024, ) < 0)
  goto cleanup;
  



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

Re: [libvirt] [PATCH] virsh: fix no error when pass a count <= 0 to setvcpus

[lhuang]

On 10/21/2015 05:28 PM, Andrea Bolognani wrote:

On Wed, 2015-10-21 at 17:01 +0800, lhuang wrote:

Hi peter and andrea,

I was fixing some issue in virsh client and find this old problem,
can i
post a new version of this patch ?

Right.

I guess you could use my version of the patch as a
starting point for nicer handling of negative values,
but make the error message when count == 0 more
specific as Peter suggested.


Ok, i will use the error peter suggested and the way you handling of 
negative values in a new version.


Thanks a lot for your quick reply.


Cheers.



Luyao

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

Re: [libvirt] [PATCH] conf: fix set the wrong fromconfig when specify addr in config

[lhuang]

On 10/20/2015 11:38 AM, Luyao Huang wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1238338#c6

If the user already specify address in xml, we will set the
wrong $fromConfig, which will make libvirt output a wrong error
message and make hot-plug fail when hot-plug a pci device (see
commit 1e15be1 and 9a12b6c).

Signed-off-by: Luyao Huang 
---
  src/conf/domain_addr.c | 11 ++-
  1 file changed, 10 insertions(+), 1 deletion(-)


Self NACK, although there is a small problem with the error message, but 
commit 1e15be1 and 9a12b6c not want skip the check during hot-plug a pci 
device to a not support hot-plug bus on a q35 machine, so this patch 
will introduce a another issue.


BTW, from the c033e210613b860bef4859a81e22088d0d2f0f29, we could know that
""
the same error message will be changed to indicate
either "internal" or "xml" error depending on whether the address came
from the config, or was automatically generated by libvirt
""

so in this place i hot-plug a device which specified the address in xml, 
i should get error VIR_ERR_XML_ERROR. however i got an internal error.



diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index ca5803e..49769f7 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -376,6 +376,14 @@ virDomainPCIAddressReserveSlot(virDomainPCIAddressSetPtr 
addrs,
  return virDomainPCIAddressReserveAddr(addrs, addr, flags, true, false);
  }
  
+static int

+virDomainPCIAddressReserveSlotFromConfig(virDomainPCIAddressSetPtr addrs,
+ virDevicePCIAddressPtr addr,
+ virDomainPCIConnectFlags flags)
+{
+return virDomainPCIAddressReserveAddr(addrs, addr, flags, true, true);
+}
+
  int
  virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs,
virDomainDeviceInfoPtr dev)
@@ -408,7 +416,8 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr 
addrs,
   addrStr, flags, true))
  goto cleanup;
  
-ret = virDomainPCIAddressReserveSlot(addrs, >addr.pci, flags);

+ret = virDomainPCIAddressReserveSlotFromConfig(addrs,
+   >addr.pci, flags);
  } else {
  ret = virDomainPCIAddressReserveNextSlot(addrs, dev, flags);
  }


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

Re: [libvirt] [PATCH] virsh: fix no error when pass a count <= 0 to setvcpus

[lhuang]

Hi peter and andrea,

I was fixing some issue in virsh client and find this old problem, can i 
post a new version of this patch ?


Thanks,
Luyao


On 07/30/2015 04:47 PM, Andrea Bolognani wrote:

On Thu, 2015-07-30 at 10:05 +0200, Peter Krempa wrote:

On Thu, Jul 30, 2015 at 09:51:15 +0200, Andrea Bolognani wrote:

On Thu, 2015-07-30 at 09:45 +0200, Peter Krempa wrote:

Since we know that here we are trying to set 0 cpus which is
wrong we
can also say that explicitly in the error message rather than
copying
the generic one.

Using the generic one means we don't have to add yet another
translatable string, though. Or did you mean to use a
completely different error message?

Completely different. Something along: "Can't set 0 processors for a
VM"

I see. I don't feel strongly either way, I just want to avoid
having a bunch of very similar but non-identical messages; if
you feel a custom error message is warranted in this situation
then go right ahead.

Cheers.



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

Re: [libvirt] [PATCH 2/2] util: Produce friendlier error message to user

[lhuang]

On 10/09/2015 10:27 PM, John Ferlan wrote:


On 10/08/2015 05:12 AM, lhuang wrote:


On 10/02/2015 11:54 PM, John Ferlan wrote:

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

Commit 1c24cfe9 fix the problem in virNumaSetPagePoolSize,
this patch just like it and fix the issue in another function.

when user use freepages and specify a invalid node or page size,
will get error like this:

   # virsh freepages 0 1
   error: Failed to open file
'/sys/devices/system/node/node0/hugepages/hugepages-1kB/free_hugepages':
No such file or directory

Add two checks to catch this and therefore produce much more
friendlier error messages.

Signed-off-by: Luyao Huang <lhu...@redhat.com>
---
   src/util/virnuma.c | 12 
   1 file changed, 12 insertions(+)

diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 8577bd8..c8beade 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -560,6 +560,12 @@ virNumaGetHugePageInfo(int node,
  page_size, "nr_hugepages") < 0)
   goto cleanup;
   +if (!virFileExists(path)) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("page size or NUMA node not available"));

duh - meant to add:

as perhaps a "patch 2" of the series (making this patch 3) - would it be
perhaps better to indicate "page size of "%u" or NUMA node "%d" not
available", using page_size, node as the arguments?

??

Good idea, and how about make the code like this:

diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index cb80972..8d85dc3 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -553,6 +553,19 @@ virNumaGetHugePageInfo(int node,
 page_size, "nr_hugepages") < 0)
  goto cleanup;

+if (!virFileExists(path)) {
+if (node != -1 && !virNumaNodeIsAvailable(node)) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("NUMA node %d is not available"),
+   node);
+} else {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("page size %u is not available"),
+   page_size);
+}
+goto cleanup;
+}
+
  if (virFileReadAll(path, 1024, ) < 0)
  goto cleanup;

@@ -572,6 +585,19 @@ virNumaGetHugePageInfo(int node,
 page_size, "free_hugepages") < 0)
  goto cleanup;

+if (!virFileExists(path)) {
+if (node != -1 && !virNumaNodeIsAvailable(node)) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("NUMA node %d is not available"),
+   node);
+} else {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("page size %u is not available"),
+   page_size);
+}
+goto cleanup;
+}
+
  if (virFileReadAll(path, 1024, ) < 0)
  goto cleanup;

@@ -836,19 +862,19 @@ virNumaSetPagePoolSize(int node,
  goto cleanup;
  }

-if (node != -1 && !virNumaNodeIsAvailable(node)) {
-virReportError(VIR_ERR_OPERATION_FAILED,
-   _("NUMA node %d is not available"),
-   node);
-goto cleanup;
-}
-
  if (virNumaGetHugePageInfoPath(_path, node, page_size,
"nr_hugepages") < 0)
  goto cleanup;

-if (!virFileExists(nr_path)) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-   _("page size or NUMA node not available"));
+if (!virFileExists(path)) {

s/path/nr_path/ ;-)


Thanks pointing out that, i wrote them too fast, so you know... :)




+if (node != -1 && !virNumaNodeIsAvailable(node)) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("NUMA node %d is not available"),
+   node);
+} else {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("page size %u is not available"),
+   page_size);

This part of the message could be displayed when node == -1 in the event
that !virNumaNodeIsAvailable is false. I think that's right based on
what virNumaGetHugePageInfoPath does, but I just want to make sure that
is what you'd expect...


Yes, this is expected, since if node == -1 we will check the 
/sys/kernel/mm/hugepages/* path, if we get failed, then means the 
hugepage size is not valid.



Conversely if page_size != -1, then would it make sense to add the
page_size or not?  That is, sure page_size 'x' is not ava

Re: [libvirt] [PATCH 2/2] util: Produce friendlier error message to user

[lhuang]

On 10/02/2015 11:49 PM, John Ferlan wrote:


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

Commit 1c24cfe9 fix the problem in virNumaSetPagePoolSize,
this patch just like it and fix the issue in another function.

when user use freepages and specify a invalid node or page size,
will get error like this:

  # virsh freepages 0 1
  error: Failed to open file 
'/sys/devices/system/node/node0/hugepages/hugepages-1kB/free_hugepages': No 
such file or directory

Add two checks to catch this and therefore produce much more
friendlier error messages.

Signed-off-by: Luyao Huang 
---
  src/util/virnuma.c | 12 
  1 file changed, 12 insertions(+)


Perhaps the more entertaining part of this patch is/was it possible to
use "freepages 0 0"?  Before patch 1, before patch 2, after patch 2.


Patch 1 + 2 can improve the error when use "freepages 0 0"


Based on the answer to the question below, I can adjust the commit
message (and code if necessary) a bit in order to remove the really long
line and make it flow better.


Thanks a lot for your help, i am not sure about if we need rework this 
code or add a patch 3 to improve the error message, please help to check 
the reply i will give in another mail.



diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 8577bd8..c8beade 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -560,6 +560,12 @@ virNumaGetHugePageInfo(int node,
 page_size, "nr_hugepages") < 0)
  goto cleanup;
  
+if (!virFileExists(path)) {

+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("page size or NUMA node not available"));
+goto cleanup;
+}
+

Why is there no "if (node != -1 && !virNumaNodeIsAvailable(node)) {"
check prior to the virNumaGetHugePageInfoPath call?




Hmm... i think this check is enough since the output error is "page size 
or NUMA node not available", the output message have already include the 
case which pass a invalid NUMA node.


I thought that add a check like "if (node != -1 && 
!virNumaNodeIsAvailable(node)) {" just output a different error in this 
place, however this (invalid nodeset) will be caught more early in 
another place (in nodeGetFreePages function):


if (startCell > lastCell) {
virReportError(VIR_ERR_INTERNAL_ERROR,
   _("start cell %d out of range (0-%d)"),
   startCell, lastCell);
goto cleanup;
}


I noticed another reply from you to this mail, and think we can make the 
error more clearly as your said in another reply. I will continue to 
reply in another mail.


Thanks a lot for your review and help !


John



Luyao


  if (virFileReadAll(path, 1024, ) < 0)
  goto cleanup;
  
@@ -579,6 +585,12 @@ virNumaGetHugePageInfo(int node,

 page_size, "free_hugepages") < 0)
  goto cleanup;
  
+if (!virFileExists(path)) {

+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("page size or NUMA node not available"));
+goto cleanup;
+}
+
  if (virFileReadAll(path, 1024, ) < 0)
  goto cleanup;
  



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

Re: [libvirt] [PATCH 2/2] util: Produce friendlier error message to user

[lhuang]

On 10/02/2015 11:54 PM, John Ferlan wrote:


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

Commit 1c24cfe9 fix the problem in virNumaSetPagePoolSize,
this patch just like it and fix the issue in another function.

when user use freepages and specify a invalid node or page size,
will get error like this:

  # virsh freepages 0 1
  error: Failed to open file 
'/sys/devices/system/node/node0/hugepages/hugepages-1kB/free_hugepages': No 
such file or directory

Add two checks to catch this and therefore produce much more
friendlier error messages.

Signed-off-by: Luyao Huang 
---
  src/util/virnuma.c | 12 
  1 file changed, 12 insertions(+)

diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 8577bd8..c8beade 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -560,6 +560,12 @@ virNumaGetHugePageInfo(int node,
 page_size, "nr_hugepages") < 0)
  goto cleanup;
  
+if (!virFileExists(path)) {

+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("page size or NUMA node not available"));

duh - meant to add:

as perhaps a "patch 2" of the series (making this patch 3) - would it be
perhaps better to indicate "page size of "%u" or NUMA node "%d" not
available", using page_size, node as the arguments?

??


Good idea, and how about make the code like this:

diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index cb80972..8d85dc3 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -553,6 +553,19 @@ virNumaGetHugePageInfo(int node,
page_size, "nr_hugepages") < 0)
 goto cleanup;

+if (!virFileExists(path)) {
+if (node != -1 && !virNumaNodeIsAvailable(node)) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("NUMA node %d is not available"),
+   node);
+} else {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("page size %u is not available"),
+   page_size);
+}
+goto cleanup;
+}
+
 if (virFileReadAll(path, 1024, ) < 0)
 goto cleanup;

@@ -572,6 +585,19 @@ virNumaGetHugePageInfo(int node,
page_size, "free_hugepages") < 0)
 goto cleanup;

+if (!virFileExists(path)) {
+if (node != -1 && !virNumaNodeIsAvailable(node)) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("NUMA node %d is not available"),
+   node);
+} else {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("page size %u is not available"),
+   page_size);
+}
+goto cleanup;
+}
+
 if (virFileReadAll(path, 1024, ) < 0)
 goto cleanup;

@@ -836,19 +862,19 @@ virNumaSetPagePoolSize(int node,
 goto cleanup;
 }

-if (node != -1 && !virNumaNodeIsAvailable(node)) {
-virReportError(VIR_ERR_OPERATION_FAILED,
-   _("NUMA node %d is not available"),
-   node);
-goto cleanup;
-}
-
 if (virNumaGetHugePageInfoPath(_path, node, page_size, 
"nr_hugepages") < 0)

 goto cleanup;

-if (!virFileExists(nr_path)) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-   _("page size or NUMA node not available"));
+if (!virFileExists(path)) {
+if (node != -1 && !virNumaNodeIsAvailable(node)) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("NUMA node %d is not available"),
+   node);
+} else {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("page size %u is not available"),
+   page_size);
+}
 goto cleanup;
 }



We need check if node == -1 to avoid output error include "NUMA node 
"-1" ", and instead of check the nodeset again and again, i think just 
check them when we cannot find the file (!virFileExists(nr_path)),

check the return from virNumaNodeIsAvailable() to output different error.

Thanks in advance for your reply and help.

Luyao


John

+goto cleanup;
+}
+
  if (virFileReadAll(path, 1024, ) < 0)
  goto cleanup;
  
@@ -579,6 +585,12 @@ virNumaGetHugePageInfo(int node,

 page_size, "free_hugepages") < 0)
  goto cleanup;
  
+if (!virFileExists(path)) {

+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("page size or NUMA node not available"));
+goto cleanup;
+}
+
  if (virFileReadAll(path, 1024, ) < 0)
  goto cleanup;
  



--

Re: [libvirt] [PATCH 1/2] util: split the virNumaGetHugePageInfoPath into separate function

[lhuang]

Sorry for the delay, i just came back from a long holiday.


On 10/02/2015 11:49 PM, John Ferlan wrote:


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

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

When pass 0 page_size to virNumaGetHugePageInfoPath function, we will
get fail like this:

  error : virFileReadAll:1358 : Failed to read file 
'/sys/devices/system/node/node0/hugepages/': Is a directory

Because when the page_size is 0 the virNumaGetHugePageInfoPath will
build the directory of system path, but we don't want that.

Introduce a new helper to build the dir path could avoid this issue.


This appears to be just a straight refactor - it took me a while to
figure out why the key was 'page_size == 0'. Looking at "just" this
context it would seem virNumaGetPages using 0 && suffix==NULL was the
only user/cause for this.  But the real abuser/issue shows up in patch 2
where if "other" callers use a page_size == 0, then we have issues.

So, how about the following for a commit message:

Refactor virNumaGetHugePageInfoPath to handle the passing a page_size
of 0 and suffix == NULL into a new function virNumaGetHugePageInfoDir.
Function virNumaGetPages expects to return a directory; whereas, other
callers that passed a page_size == 0 would not expect a directory
path in return. Each of those callers will use virFileReadAll which
fails if a directory path is returned as follows:

 error : virFileReadAll:1358 : Failed to read file
 '/sys/devices/system/node/node0/hugepages/': Is a directory




Looks good to me, thanks a lot for your help.


Signed-off-by: Luyao Huang 
---
  src/util/virnuma.c | 42 ++
  1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 1a62d62..8577bd8 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -493,45 +493,39 @@ virNumaGetHugePageInfoPath(char **path,
 unsigned int page_size,
 const char *suffix)
  {
-
-int ret = -1;
-
  if (node == -1) {
  /* We are aiming at overall system info */
-if (page_size) {
-/* And even on specific huge page size */
  if (virAsprintf(path,
  HUGEPAGES_SYSTEM_PREFIX HUGEPAGES_PREFIX 
"%ukB/%s",
  page_size, suffix ? suffix : "") < 0)

Now this has too many spaces shifted to the right (4 to be exact)


Indeed, i forgot this during wrote this patch.


-goto cleanup;
-} else {
-if (VIR_STRDUP(*path, HUGEPAGES_SYSTEM_PREFIX) < 0)
-goto cleanup;
-}
-
+return -1;
  } else {
  /* We are aiming on specific NUMA node */
-if (page_size) {
-/* And even on specific huge page size */
  if (virAsprintf(path,
  HUGEPAGES_NUMA_PREFIX "node%d/hugepages/"
  HUGEPAGES_PREFIX "%ukB/%s",
  node, page_size, suffix ? suffix : "") < 0)

same note here


-goto cleanup;
-} else {
-if (virAsprintf(path,
-HUGEPAGES_NUMA_PREFIX "node%d/hugepages/",
-node) < 0)
-goto cleanup;
-}
+return -1;
  }
  
-ret = 0;

- cleanup:
-return ret;
+return 0;
  }
  
+static int

+virNumaGetHugePageInfoDir(char **path, int node)
+{
+if (node == -1) {
+if (VIR_STRDUP(*path, HUGEPAGES_SYSTEM_PREFIX) < 0)
+return -1;
+} else {
+if (virAsprintf(path,
+HUGEPAGES_NUMA_PREFIX "node%d/hugepages/",
+node) < 0)
+return -1;

Even more optimization could be to just return VIR_STRDUP or virAsprintf
result.

Similarly the virNumaGetHugePageInfoPath can have the same change - that
is return directly from virAsprintf commands.


Right, i missed this, thanks for pointing out that.


I can adjust the commit message and nits - just let me know if the new
commit message reads better.


The new commit message is better, thanks a lot for your review and help !


John


+}
  
+return 0;

+}
  /**
   * virNumaGetHugePageInfo:
   * @node: NUMA node id
@@ -724,7 +718,7 @@ virNumaGetPages(int node,
   * is always shown as used memory. Here, however, we want to report
   * slightly different information. So we take the total memory on a node
   * and subtract memory taken by the huge pages. */
-if (virNumaGetHugePageInfoPath(, node, 0, NULL) < 0)
+if (virNumaGetHugePageInfoDir(, node) < 0)
  goto cleanup;
  
  if (!(dir = opendir(path))) {




Luyao

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

Re: [libvirt] [PATCH] conf: escape string for disk driver name attribute

[lhuang]

On 09/23/2015 05:54 PM, Martin Kletzander wrote:

On Tue, Sep 22, 2015 at 04:13:53PM +0800, Luyao Huang wrote:

Just like e92e5ba1, this attribute was missed.

Signed-off-by: Luyao Huang 
---
src/conf/domain_conf.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)



ACK && Pushed.


Thanks your review, Martin :)

Luyao

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

Re: [libvirt] [PATCH] Makefile: fix build fail when make rpm

[lhuang]

On 09/23/2015 02:44 PM, Ján Tomko wrote:

On Wed, Sep 23, 2015 at 10:09:47AM +0800, Luyao Huang wrote:

Build fail and error like this:

   CC   qemu/libvirt_driver_qemu_impl_la-qemu_command.lo
qemu/qemu_capabilities.c:46:27: fatal error: qemu_capspriv.h: No such file or 
directory
  #include "qemu_capspriv.h"

Add qemu_capspriv.h to source.

Signed-off-by: Luyao Huang 
---
  src/Makefile.am | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)


ACK and pushed.

Thanks for catching that.


You are welcome, thanks your quick review.


Jan


Luyao

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

Re: [libvirt] [libvirt-python][PATCH] generator: fix build fail with old xml lib

[lhuang]

On 09/21/2015 06:09 PM, Michal Privoznik wrote:

On 02.09.2015 07:58, Luyao Huang wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1222795#c6

if build libvirt-python with some old xml lib (python-pyxml),
build will fail and error like this:

   File "generator.py", line 139, in start
 if "string" in attrs:
   File "/usr/local/lib/python2.7/site-packages/_xmlplus/sax/xmlreader.py" \
 , line 316, in __getitem__
 return self._attrs[name]
 KeyError: 0

This is an old issue and have been mentioned in commit 3ae0a76d.
There is no __contains__ in class AttributesImpl, python will use
__getitem__ in this place, so we will get error.
Let's use 'YYY in XXX.keys()' to avoid this issue.

Signed-off-by: Luyao Huang 
---
  generator.py | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/generator.py b/generator.py
index 2fc838c..d9ae17e 100755
--- a/generator.py
+++ b/generator.py
@@ -136,7 +136,7 @@ class docParser(xml.sax.handler.ContentHandler):
  elif attrs['file'] == "libvirt-qemu":
  qemu_enum(attrs['type'],attrs['name'],attrs['value'])
  elif tag == "macro":
-if "string" in attrs:
+if "string" in attrs.keys():
  params.append((attrs['name'], attrs['string']))
  
  def end(self, tag):



ACKed and pushed.


Thanks your review, Michal :)


Michal


Luyao

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

Re: [libvirt] [PATCH] conf: fix crash when parse a disordered numa settings

[lhuang]

On 09/08/2015 04:40 PM, Michal Privoznik wrote:

On 08.09.2015 06:59, Luyao Huang wrote:

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

Introduced by 8fedbbdb, if we parse the disordered numa
cell, will get segfault. This is because we allow parse
the numa cell not in order and we alloc them before the
parse loop, the cpumask maybe NULL when parse each numa
cell.

Signed-off-by: Luyao Huang 
---
  src/conf/numa_conf.c   | 10 +---
  .../qemuxml2argv-cpu-numa-disordered.xml   | 26 +++
  .../qemuxml2xmlout-cpu-numa-disordered.xml | 29 ++
  tests/qemuxml2xmltest.c|  1 +
  4 files changed, 63 insertions(+), 3 deletions(-)
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-disordered.xml
  create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-cpu-numa-disordered.xml

I've reworded the commit message a bit, ACKed and pushed.


Thanks a lot for your quick review and help.


Michal


Luyao

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

Re: [libvirt] [python PATCH] examples: small fix for nodestats.py example

[lhuang]

On 08/27/2015 06:06 AM, John Ferlan wrote:


On 07/30/2015 10:19 PM, Luyao Huang wrote:

Add nodestats.py in MANIFEST.in and add a
small description for nodestats.py in README

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  MANIFEST.in | 1 +
  examples/README | 3 +++
  2 files changed, 4 insertions(+)


ACK and pushed.


Thanks a lot for your review.


John



Luyao

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

Re: [libvirt] [PATCHv2 0/2] qemu: fix the audit log is not correct for memory device

[lhuang]

On 08/27/2015 05:54 AM, John Ferlan wrote:


On 08/13/2015 10:15 AM, Luyao Huang wrote:

First review:
http://www.redhat.com/archives/libvir-list/2015-July/msg00982.html

Change in v2:
- split it to two patches
- fix some small mistake in hot-unplug part

I change the code in qemuDomainAttachMemory like what we do in other attach 
deivce
functions. And I have removed the jump in qemuDomainRemoveMemoryDevice, since i 
just
see only one place need jump to audit, so i refactor the code (thanks John's 
advise).

Luyao Huang (2):
   qemu: fix the audit log is not correct after hot-plug memory success
   qemu: fix the audit log is not correct after hot-unplug memory device

  src/qemu/qemu_hotplug.c | 35 +--
  1 file changed, 17 insertions(+), 18 deletions(-)


These look much cleaner code wise - had to clean up the commit messages,
but otherwise fine.  I have pushed the two patches.


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] qemu: Label correct per-VM path when starting

[lhuang]

On 08/25/2015 05:53 PM, Martin Kletzander wrote:

Commit f1f68ca33433825ce0deed2d96f1990200bc6618 overused mdir_name()
event though it was not needed in the latest version, hence labelling
directory one level up in the tree and not the one it should.

If anyone with SElinux managed to try run a domain with guest agent set
up, it's highly possible that they will need to run 'restorecon -F
/var/lib/libvirt/qemu/channel/target' to fix what was done.


I have test it and this patch can fix the problem which will make guest 
cannot start on my machine.


Luyao

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

Re: [libvirt] [PATCHv2] qemu: fix not update weight in def after success

[lhuang]

On 08/26/2015 04:22 AM, John Ferlan wrote:


On 08/18/2015 11:56 PM, Luyao Huang wrote:

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

Call virCgroupGetBlkioWeight to re-read blkio.weight right
after it are set in order to keep our internal structures
up-to-date.

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


Fixed some grammar in commit message and pushed.


Thanks a lot for your review and help.

Luyao


John


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

Re: [libvirt] [PATCH] qemu: add a check for nodeset in qemuDomainSetNumaParamsLive

[lhuang]

On 08/24/2015 08:52 PM, Martin Kletzander wrote:

On Fri, Aug 14, 2015 at 05:37:28PM +0800, Luyao Huang wrote:

We will try to set the node to cpuset.mems without check if
it is available, since we already have helper to check this.
Call virNumaNodesetIsAvailable to check if node is available,
then try to change it in the cgroup.

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



ACK, will push in a while.



Thanks a lot for your review.

Luyao


diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fa655b5..45dfca0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9977,6 +9977,9 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm,
goto cleanup;
}

+if (!virNumaNodesetIsAvailable(nodeset))
+goto cleanup;
+
/* Ensure the cpuset string is formatted before passing to cgroup */
if (!(nodeset_str = virBitmapFormat(nodeset)))
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] [libvirt-test-api][PATCH 3/3] Add a new case for getCPUStatus

[lhuang]

On 08/24/2015 11:41 AM, hongming wrote:


On 06/02/2015 07:57 AM, hongming wrote:

ACK and pushed
Don't forget to remove trailing whitespace next time.
Thanks

Hongming
On 05/18/2015 09:28 AM, Luyao Huang wrote:

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  cases/linux_domain.conf|   6 +++
  repos/domain/cpu_status.py | 113 
+

  2 files changed, 119 insertions(+)
  create mode 100644 repos/domain/cpu_status.py

diff --git a/cases/linux_domain.conf b/cases/linux_domain.conf
index 0a7d134..9f64226 100644
--- a/cases/linux_domain.conf
+++ b/cases/linux_domain.conf
@@ -269,6 +269,12 @@ virconn:connection_security_model
  guestname
  $defaultname
  +domain:virDomain_getCPUStats


The case name is wrong , I have fixed it directly

commit 5c433b7c934cd8ac3ad783939c5906b7cbc129a8
Author: Hongming Zhang honzh...@redhat.com
Date:   Mon Aug 24 11:25:04 2015 +0800

Fix a wrong case name in linux_domain.conf

Modify the virDomain_getCPUStats as cpu_status
modified:   cases/linux_domain.conf




Thanks, i must missed this place during change the name of this api.


Luyao



+guestname
+$defaultname
+conn
+qemu:///system
+
  domain:destroy
  guestname
  $defaultname
diff --git a/repos/domain/cpu_status.py b/repos/domain/cpu_status.py
new file mode 100644
index 000..6e511c0
--- /dev/null
+++ b/repos/domain/cpu_status.py
@@ -0,0 +1,113 @@
+#!/usr/bin/env python
+import libvirt
+from libvirt import libvirtError
+from  utils import utils
+
+required_params = ('guestname',)
+optional_params = {'conn': 'qemu:///system'}
+
+ONLINE_CPU = '/sys/devices/system/cpu/online'
+CGROUP_PERCPU = 
'/sys/fs/cgroup/cpuacct/machine.slice/machine-qemu\\x2d%s.scope/cpuacct.usage_percpu'
+CGROUP_PERVCPU = 
'/sys/fs/cgroup/cpuacct/machine.slice/machine-qemu\\x2d%s.scope/vcpu%d/cpuacct.usage_percpu'
+CGROUP_USAGE = 
'/sys/fs/cgroup/cpuacct/machine.slice/machine-qemu\\x2d%s.scope/cpuacct.usage'
+CGROUP_STAT = 
'/sys/fs/cgroup/cpuacct/machine.slice/machine-qemu\\x2d%s.scope/cpuacct.stat'

+
+def getcputime(a):
+return open(a[0]).read().split()[a[1]]
+
+def virtgetcputime(a):
+return a[0].getCPUStats(0)[a[1]][a[2]]
+
+def getvcputime(a):
+ret = 0
+for i in range(int(a[0])):
+ret += int(open(CGROUP_PERVCPU % (a[1], 
i)).read().split()[a[2]])

+
+return ret
+
+def virtgettotalcputime(a):
+return a[0].getCPUStats(1)[0][a[1]]
+
+def virtgettotalcputime2(a):
+return a[0].getCPUStats(1)[0][a[1]]/1000
+
+def cpu_status(params):
+
+   test API for getCPUStats in class virDomain
+
+logger = params['logger']
+fail=0
+
+cpu = utils.file_read(ONLINE_CPU)
+logger.info(host online cpulist is %s % cpu)
+
+cpu_tuple = utils.param_to_tuple_nolength(cpu)
+if not cpu_tuple:
+logger.info(error in function param_to_tuple_nolength)
+return 1
+
+try:
+conn = libvirt.open(params['conn'])
+
+logger.info(get connection to libvirtd)
+guest = params['guestname']
+vm = conn.lookupByName(guest)
+vcpus = vm.info()[3]
+for n in range(len(cpu_tuple)):
+if not cpu_tuple[n]:
+continue
+
+D = utils.get_standard_deviation(getcputime, 
virtgetcputime, \

+[CGROUP_PERCPU % guest, n], [vm,n,'cpu_time'])
+logger.info(Standard Deviation for host cpu %d cputime 
is %d % (n, D))

+
+ expectations 403423 is a average collected in a 
x86_64 low load machine

+if D  403423*5:
+fail=1
+logger.info(FAIL: Standard Deviation is too big \
+ (biger than %d) for host cpu %d % 
(403423*5, n))

+
+D = utils.get_standard_deviation(getvcputime, 
virtgetcputime, \

+[vcpus, guest, n], [vm,n,'vcpu_time'])
+logger.info(Standard Deviation for host cpu %d 
vcputime is %d % (n, D))

+
+ expectations 4034 is a average collected in a 
x86_64 low load machine

+if D  4034*5*vcpus:
+fail=1
+logger.info(FAIL: Standard Deviation is too big \
+ (biger than %d) for host cpu time %d 
% (4034*5*vcpus, n))

+
+D = utils.get_standard_deviation(getcputime, 
virtgettotalcputime, \

+[CGROUP_USAGE % guest, 0], [vm,'cpu_time'])
+logger.info(Standard Deviation for host cpu total cputime 
is %d % D)

+
+ expectations 313451 is a average collected in a x86_64 
low load machine

+if D  313451*5*len(cpu_tuple):
+fail=1
+logger.info(FAIL: Standard Deviation is too big \
+ (biger than %d) for host cpu time %d % 
(313451*5*len(cpu_tuple), n))

+
+D = utils.get_standard_deviation(getcputime, 
virtgettotalcputime2, \

+[CGROUP_STAT % guest, 3], [vm,'system_time'])
+logger.info(Standard 

Re: [libvirt] [PATCH] qemu: fix the error cover issue in qemuDomainAddCgroupForThread

[lhuang]

On 08/19/2015 01:40 AM, John Ferlan wrote:


On 08/14/2015 02:59 AM, Luyao Huang wrote:

Just like commit 704cf06, the error already will be set in
virCgroup* function, and virCgroupAddTask will return -1,
so We will always report error Operation not permitted
in this place.

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

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fa655b5..e0d7fa5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4606,9 +4606,6 @@ qemuDomainAddCgroupForThread(virCgroupPtr cgroup,
  /* Add pid/thread to the cgroup */
  rv = virCgroupAddTask(new_cgroup, pid);
  if (rv  0) {
-virReportSystemError(-rv,
- _(unable to add id %d task %d to cgroup),
- idx, pid);
  virCgroupRemove(new_cgroup);

Apparently virCgroupRemove can also overwrite a message, see
virCgroupNewMachineSystemd for an example of how to save the error
message and restore it..
Perhaps all the callers that fail would need a similar sequence


You are right, maybe we could introduce a macro maybe named 
virErrorAvoidRecover for these case.



John

I'm at KVM Forum this week so digging and finding out the answer myself
is a challenge with the flakiness of our network connection...


So lucky you are (of course i mean KVM Forum), i will fix them in 
another patches.


Thanks a lot for your review.

Luyao


  goto error;
  }



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

Re: [libvirt] [PATCH] qemu: fix the error cover issue in qemuDomainAddCgroupForThread

[lhuang]

On 08/19/2015 02:17 AM, Michal Privoznik wrote:

On 14.08.2015 08:59, Luyao Huang wrote:

Just like commit 704cf06, the error already will be set in
virCgroup* function, and virCgroupAddTask will return -1,
so We will always report error Operation not permitted
in this place.

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

Reworded the commit message a bit, ACKed and pushed.


Thanks a lot 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] [PATCH] qemu: fix not update weight in def after success

[lhuang]

On 08/19/2015 01:32 AM, John Ferlan wrote:


On 08/12/2015 09:53 PM, Luyao Huang wrote:

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

Update the weight in vm def to fix this.

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


Considering the series I recently ACK from Martin - shouldn't this to
make a virCgroupGetBlkioWeight type call to ensure you get/save whatever
the kernel saved?


Indeed, it will be better to check the value again (blkio.weigh and 
blkio.weight_device range is [10, 1000] right now, so seems these 2 
value won't hit the issue that was fixed by Martin), i will write a new 
version later.


Thanks a lot for your review.


John


Luyao


See:

http://www.redhat.com/archives/libvir-list/2015-August/msg00065.html

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7a984a8..0b984dc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9170,6 +9170,8 @@ qemuDomainSetBlkioParameters(virDomainPtr dom,
  if (STREQ(param-field, VIR_DOMAIN_BLKIO_WEIGHT)) {
  if (virCgroupSetBlkioWeight(priv-cgroup, param-value.ui)  
0)
  ret = -1;
+else
+def-blkio.weight = param-value.ui;
  } else if (STREQ(param-field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT) ||
 STREQ(param-field, VIR_DOMAIN_BLKIO_DEVICE_READ_IOPS) 
||
 STREQ(param-field, 
VIR_DOMAIN_BLKIO_DEVICE_WRITE_IOPS) ||



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

Re: [libvirt] [PATCH] virsh: fix output the incorrect error after try failed

[lhuang]

On 08/18/2015 08:56 PM, Erik Skultety wrote:


On 17/08/15 11:56, Luyao Huang wrote:

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

When we use some virsh cmd which need specify domain
name/id/uuid, if the command get failure we will get error
like this:

  # virsh domif-setlink 123 vnet1 up
  error: interface (target: vnet1) not found
  error: Domain not found: no domain with matching id 123

The second line should be reset after call virshLookupDomainInternal,
because after some tries we get domain pointer, so output error
during we tried will make user confuse.

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

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 173bb15..69c5562 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -98,6 +98,8 @@ virshLookupDomainInternal(vshControl *ctl,
  dom = virDomainLookupByName(priv-conn, name);
  }
  
+vshResetLibvirtError();

+
  if (!dom)
  vshError(ctl, _(failed to get domain '%s'), name);
  


ACK, I reworded the commit message and pushed.


Thanks a lot for your quick review and help.


Erik


Luyao

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

Re: [libvirt] [PATCH 0/2] 2 small fix for domrename

[lhuang]

On 08/18/2015 12:53 AM, John Ferlan wrote:


On 08/17/2015 01:21 AM, Luyao Huang wrote:

*** BLURB HERE ***

Luyao Huang (2):
   virsh: fix always return false in domrename
   libvirt-domain: forbid use virDomainRename in readonly connection

  src/libvirt-domain.c | 1 +
  tools/virsh-domain.c | 2 +-
  2 files changed, 2 insertions(+), 1 deletion(-)


ACK and push series,
Good catches,



Thanks a lot 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] lib: fix no zero arg check for iothread_id

[lhuang]

On 08/10/2015 05:23 PM, Peter Krempa wrote:

On Mon, Aug 10, 2015 at 17:06:31 +0800, Luyao Huang wrote:

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

We do not allow delete an iothread which id is 0 in
virDomainDelIOThread, but allow it in virDomainAddIOThread,
Also we will output an error when parse an iothread which id
is 0.

Add a check for iothread_id in virDomainAddIOThread to fix
it.

The limitation that iothread id shall not be 0 comes from the qemu
implementation so I think that we could possibly want to have iothread
id 0 in the future.

I think the check should be done in the qemu driver.


Make sense, i will move the check in qemu driver, also i will change the 
code for iothread pin/delete


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] virsh: fix domfsinfo wrong output in quiet mode

[lhuang]

On 08/05/2015 04:05 PM, Ján Tomko wrote:

On Wed, Aug 05, 2015 at 11:17:22AM +0800, Luyao Huang wrote:

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

When run domfsinfo in quiet mode, we cannot get any
useful information (just get \n), this is because
we didn't use vshPrint to print useful information.

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


ACK and pushed.


Thanks a lot for your quick review.



Jan


Luyao

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

Re: [libvirt] [PATCH] qemu: fix some api cannot work when disable cpuset in conf

[lhuang]

On 07/31/2015 08:33 PM, Martin Kletzander wrote:

On Mon, Jul 20, 2015 at 05:18:37PM +0800, Luyao Huang wrote:

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

If user disable cpuset in qemu.conf, we shouldn't
try to use it, also shouldn't make some command which
can work without cpuset cannot work.

Fix these case:
1. start guest with strict numa policy (we can use libnuma help us).
2. Hot add vcpu.
3. hot add iothread.

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


This looks fine, I'll just adjust the commit message before pushing
it.  ACK after release since we're in rc2 now and nobody really had a
problem with this.



Yes, this is a corner issue, thanks a lot for your review.


Martin


Luyao

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

Re: [libvirt] [PATCH 1/4] conf: introduce seclabels in shmem device element

[lhuang]

On 07/30/2015 06:28 PM, Daniel P. Berrange wrote:

On Thu, Jul 23, 2015 at 06:13:46PM +0800, Luyao Huang wrote:

Introduce a new element in shmem device element, this
could help users to change the shm label to a specified
label.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  docs/formatdomain.html.in |  7 ++
  docs/schemas/domaincommon.rng |  3 +++
  src/conf/domain_conf.c| 55 ++-
  src/conf/domain_conf.h|  5 
  4 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index d0c1741..e02c67c 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6098,6 +6098,13 @@ qemu-kvm -net nic,model=? /dev/null
vectors. The codeioeventd/code attribute enables/disables (values
on/off, respectively) ioeventfd.
  /dd
+dtcodeseclabel/code/dt
+dd
+  The  optional codeseclabel/code to override the way that labelling
+  is done on the shm object path or shm server path.  If this
+  element is not present, the a href=#seclabelsecurity label is 
inherited
+  from the per-domain setting/a.
+/dd
/dl
  
  h4a name=elementsMemoryMemory devices/a/h4

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 1120003..f58e8de 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3323,6 +3323,9 @@
  /optional
/element
  /optional
+zeroOrMore
+  ref name='devSeclabel'/
+/zeroOrMore
  optional
ref name=address/
  /optional

So in the disk XML we have an explicit element to indicate whether the
device is intended to be shared across multiple guests. shareable/

I think we need to have the same flag added to the shm device too, so
that we sanity check whether a particular shm was intended to be shared
or whether it is a mistake when multiple guests use it. This will also
allow us to integrate with the virtlockd to acquire exclusive locks
against the shm device to actively prevent admin mistakes starting
2 guests with the same shm. It will also let us automatically choose
the right default SELinux label ie svirt_image_t:s0:c214,c3242 for
exclusive access vs svirt_image_t:s0 for shared access



Good idea! i will introduce this new element in next version.

Thanks a lot for your advise.


Regards,
Daniel


Luyao

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

Re: [libvirt] [PATCH 4/4] qemu: call the helpers in virshm.c to manage shmem device

[lhuang]

On 07/30/2015 06:25 PM, Daniel P. Berrange wrote:

On Thu, Jul 23, 2015 at 06:13:49PM +0800, Luyao Huang wrote:

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_conf.h|   3 +
  src/qemu/qemu_driver.c  |   4 ++
  src/qemu/qemu_process.c | 158 
  3 files changed, 165 insertions(+)

+static int
+qemuPrepareShmemDevice(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   virDomainShmemDefPtr shmem)
+{
+int ret = -1;
+virShmObjectPtr tmp;
+virShmObjectListPtr list = driver-shmlist;
+bool othercreate = false;
+char *path = NULL;
+bool teardownlabel = false;
+bool teardownshm = false;
+int type, fd;
+
+virObjectLock(list);
+
+if ((tmp = virShmObjectFindByName(list, shmem-name))) {
+if (shmem-size  tmp-size) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _(Shmem object %s is already exists and 
+ size is smaller than require size),
+   tmp-name);
+goto cleanup;
+}
+
+if (virShmSetUsedDomain(tmp, QEMU_DRIVER_NAME, vm-def-name)  0)
+goto cleanup;
+
+if (virShmObjectSaveState(tmp, list-stateDir)  0)
+goto cleanup;
+
+virObjectUnlock(list);
+return 0;
+}
+
+if (!shmem-server.enabled) {
+if ((fd = virShmCreate(shmem-name, shmem-size, false, othercreate, 
0600))  0)
+goto cleanup;
+VIR_FORCE_CLOSE(fd);
+
+if ((ret = virShmBuildPath(shmem-name, path)) == -1) {
+ignore_value(virShmUnlink(shmem-name));
+goto cleanup;
+} else if (ret == -2  !othercreate) {
+ignore_value(virShmUnlink(shmem-name));

Why are you treating -1 differentl from -2 - in both cases we should
abort creation as that indicates the method either failed or is not
supported in this platform.


What i thought when i wrote this is : when ret = -2 this means we do not 
support virShmBuildPath in that platform (this could only happened on 
some other platform which support shm_open/shm_unlink ,just like 
solaris, freebsd) but we could use shm_open, on that platform the shm 
obj is not in /dev/shm or not exist in the file system, if we help to 
create that shm obj but cannot find a way to relabel it, qemu will 
cannot use the shm in that case, so unlink the shm and let qemu create 
it will make guest can start success, and we could unlink the qemu 
create shm obj if there is no guest use it.


I am not sure this is a good idea right now, since i am not sure this 
will work as except on different platform. Maybe i should remove it and 
make virShmBuildPath return -1 if not support on that platform.



Regards,
Daniel


Luyao

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

Re: [libvirt] [PATCH 4/4] qemu: call the helpers in virshm.c to manage shmem device

[lhuang]

Hi Marc-André

On 07/27/2015 11:52 PM, Marc-André Lureau wrote:

Hi

On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang lhu...@redhat.com wrote:

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_conf.h|   3 +
  src/qemu/qemu_driver.c  |   4 ++
  src/qemu/qemu_process.c | 158 
  3 files changed, 165 insertions(+)

diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 3f73929..61d3462 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -46,6 +46,7 @@
  # include virclosecallbacks.h
  # include virhostdev.h
  # include virfile.h
+# include virshm.h

  # ifdef CPU_SETSIZE /* Linux */
  #  define QEMUD_CPUMASK_LEN CPU_SETSIZE
@@ -235,6 +236,8 @@ struct _virQEMUDriver {
  /* Immutable pointer. Unsafe APIs. XXX */
  virHashTablePtr sharedDevices;

+virShmObjectListPtr shmlist;
+
  /* Immutable pointer, self-locking APIs */
  virPortAllocatorPtr remotePorts;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9dbe635..282ab45 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -778,6 +778,9 @@ qemuStateInitialize(bool privileged,
  if (qemuMigrationErrorInit(qemu_driver)  0)
  goto error;

+if (!(qemu_driver-shmlist = virShmObjectListGetDefault()))
+goto error;
+
  if (privileged) {
  char *channeldir;

@@ -1087,6 +1090,7 @@ qemuStateCleanup(void)
  virObjectUnref(qemu_driver-config);
  virObjectUnref(qemu_driver-hostdevMgr);
  virHashFree(qemu_driver-sharedDevices);
+virObjectUnref(qemu_driver-shmlist);
  virObjectUnref(qemu_driver-caps);
  virQEMUCapsCacheFree(qemu_driver-qemuCapsCache);

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1c0c734..84b3b5e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4321,6 +4321,154 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
  }


+static int
+qemuPrepareShmemDevice(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   virDomainShmemDefPtr shmem)
+{
+int ret = -1;
+virShmObjectPtr tmp;
+virShmObjectListPtr list = driver-shmlist;
+bool othercreate = false;
+char *path = NULL;
+bool teardownlabel = false;
+bool teardownshm = false;
+int type, fd;
+
+virObjectLock(list);
+
+if ((tmp = virShmObjectFindByName(list, shmem-name))) {
+if (shmem-size  tmp-size) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _(Shmem object %s is already exists and 
+ size is smaller than require size),
+   tmp-name);
+goto cleanup;
+}
+
+if (virShmSetUsedDomain(tmp, QEMU_DRIVER_NAME, vm-def-name)  0)
+goto cleanup;
+
+if (virShmObjectSaveState(tmp, list-stateDir)  0)
+goto cleanup;
+
+virObjectUnlock(list);
+return 0;
+}
+
+if (!shmem-server.enabled) {
+if ((fd = virShmCreate(shmem-name, shmem-size, false, othercreate, 
0600))  0)
+goto cleanup;
+VIR_FORCE_CLOSE(fd);
+
+if ((ret = virShmBuildPath(shmem-name, path)) == -1) {
+ignore_value(virShmUnlink(shmem-name));
+goto cleanup;
+} else if (ret == -2  !othercreate) {

What is ret == -2 ?


when ret = -2 this means we do not support virShmBuildPath in that 
platform (this could only happened on some other platform which support 
shm_open/shm_unlink ,just like solaris, freebsd), at that platform the 
shm obj is not in /dev/shm or not exist in the file system, if we help 
to create that shm obj but cannot find a way to relabel it, qemu will 
cannot use the shm in that case, so unlink the shm and let qemu create 
it will make guest can start success, and we could unlink the qemu 
create shm obj if there is no guest use it.





+ignore_value(virShmUnlink(shmem-name));
+}
+type = VIR_SHM_TYPE_SHM;
+} else {
+if (!virFileExists(shmem-server.chr.data.nix.path)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(Shmem device server socket is not exist));

is not / does not


+goto cleanup;
+} else {
+othercreate = true;
+}
+type = VIR_SHM_TYPE_SERVER;
+}
+teardownshm = true;
+
+if (virSecurityManagerSetShmemLabel(driver-securityManager,
+vm-def, shmem, path)  0)
+goto cleanup;

So each time a ivshmem device starts, it will potentially change the
labels. Not sure that's a good idea. Why not apply only when created?


Good question, we allow specify the label in the shmem device XML, and 
if the user create the shm with a label which only allow that guest 
access, then the other guest will cannot use it if we do not change the 
label, but if we change the label to a label which allow all libvirt 
guest can access, it will 

Re: [libvirt] [PATCH 4/4] qemu: call the helpers in virshm.c to manage shmem device

[lhuang]

On 07/30/2015 06:23 PM, Daniel P. Berrange wrote:

On Thu, Jul 23, 2015 at 06:13:49PM +0800, Luyao Huang wrote:

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_conf.h|   3 +
  src/qemu/qemu_driver.c  |   4 ++
  src/qemu/qemu_process.c | 158 
  3 files changed, 165 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1c0c734..84b3b5e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4321,6 +4321,154 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
  }
  
  
+static int

+qemuPrepareShmemDevice(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   virDomainShmemDefPtr shmem)
+{
+int ret = -1;
+virShmObjectPtr tmp;
+virShmObjectListPtr list = driver-shmlist;
+bool othercreate = false;
+char *path = NULL;
+bool teardownlabel = false;
+bool teardownshm = false;
+int type, fd;
+
+virObjectLock(list);
+
+if ((tmp = virShmObjectFindByName(list, shmem-name))) {
+if (shmem-size  tmp-size) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _(Shmem object %s is already exists and 
+ size is smaller than require size),
+   tmp-name);
+goto cleanup;
+}
+
+if (virShmSetUsedDomain(tmp, QEMU_DRIVER_NAME, vm-def-name)  0)
+goto cleanup;
+
+if (virShmObjectSaveState(tmp, list-stateDir)  0)
+goto cleanup;
+
+virObjectUnlock(list);
+return 0;
+}
+
+if (!shmem-server.enabled) {
+if ((fd = virShmCreate(shmem-name, shmem-size, false, othercreate, 
0600))  0)
+goto cleanup;
+VIR_FORCE_CLOSE(fd);
+
+if ((ret = virShmBuildPath(shmem-name, path)) == -1) {
+ignore_value(virShmUnlink(shmem-name));
+goto cleanup;
+} else if (ret == -2  !othercreate) {
+ignore_value(virShmUnlink(shmem-name));
+}
+type = VIR_SHM_TYPE_SHM;
+} else {
+if (!virFileExists(shmem-server.chr.data.nix.path)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(Shmem device server socket is not exist));
+goto cleanup;
+} else {
+othercreate = true;
+}
+type = VIR_SHM_TYPE_SERVER;
+}
+teardownshm = true;
+
+if (virSecurityManagerSetShmemLabel(driver-securityManager,
+vm-def, shmem, path)  0)
+goto cleanup;

You shouldn't be setting labelling at this point. That should be done
by the later call to virSecurityManagerSetAllLabel


Okay, i see, i will move it to virSecurityManagerSetAllLabel


+static int
+qemuCleanUpShmemDevice(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   virDomainShmemDefPtr shmem)
+{
+virShmObjectPtr tmp;
+virShmObjectListPtr list = driver-shmlist;
+int ret = -1;
+
+virObjectLock(list);
+
+if (!(tmp = virShmObjectFindByName(list, shmem-name))) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _(Cannot find share memory named '%s'),
+   shmem-name);
+goto cleanup;
+}
+if ((shmem-server.enabled  tmp-type != VIR_SHM_TYPE_SERVER) ||
+(!shmem-server.enabled  tmp-type != VIR_SHM_TYPE_SHM)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(Shmem object and shmem device type is not equal));
+goto cleanup;
+}
+
+if (virShmRemoveUsedDomain(tmp, QEMU_DRIVER_NAME, vm-def-name)  0)
+goto cleanup;
+
+if (tmp-ndomains == 0) {
+if (virSecurityManagerRestoreShmemLabel(driver-securityManager,
+vm-def, shmem, tmp-path)  0)
+VIR_WARN(Unable to restore shared memory device labelling);

Likewise this should be left to the main label restore code


Okay, Thanks a lot for your review and advise.


+
+if (!shmem-server.enabled) {
+if (!tmp-othercreate 
+virShmUnlink(tmp-name)  0)
+VIR_WARN(Unable to unlink shared memory object);
+}
+
+if (virShmObjectRemoveStateFile(list, tmp-name)  0)
+goto cleanup;
+virShmObjectListDel(list, tmp);
+virShmObjectFree(tmp);
+}
+
+ret = 0;
+ cleanup:
+virObjectUnlock(list);
+return ret;
+}

Regards,
Daniel


Luyao

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

Re: [libvirt] [PATCH 1/4] conf: introduce seclabels in shmem device element

[lhuang]

Hi Marc-André

On 07/27/2015 11:42 PM, Marc-André Lureau wrote:

Hi

On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang lhu...@redhat.com wrote:

Introduce a new element in shmem device element, this
could help users to change the shm label to a specified
label.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  docs/formatdomain.html.in |  7 ++
  docs/schemas/domaincommon.rng |  3 +++
  src/conf/domain_conf.c| 55 ++-
  src/conf/domain_conf.h|  5 
  4 files changed, 59 insertions(+), 11 deletions(-)


It would be better with a small test, checking parsing and format.


Oh, right, i forgot that, thanks for pointing out that, i will add them 
in next version.



diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index d0c1741..e02c67c 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6098,6 +6098,13 @@ qemu-kvm -net nic,model=? /dev/null
vectors. The codeioeventd/code attribute enables/disables (values
on/off, respectively) ioeventfd.
  /dd
+dtcodeseclabel/code/dt
+dd
+  The  optional codeseclabel/code to override the way that labelling

The element may contain an optional code...


Okay


+  is done on the shm object path or shm server path.  If this
+  element is not present, the a href=#seclabelsecurity label is 
inherited
+  from the per-domain setting/a.
+/dd
/dl

  h4a name=elementsMemoryMemory devices/a/h4
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 1120003..f58e8de 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3323,6 +3323,9 @@
  /optional
/element
  /optional
+zeroOrMore
+  ref name='devSeclabel'/
+/zeroOrMore
  optional
ref name=address/
  /optional
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 73ac537..cb3d72a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11261,6 +11261,8 @@ virDomainNVRAMDefParseXML(xmlNodePtr node,
  static virDomainShmemDefPtr
  virDomainShmemDefParseXML(xmlNodePtr node,
xmlXPathContextPtr ctxt,
+  virSecurityLabelDefPtr* vmSeclabels,
+  int nvmSeclabels,
unsigned int flags)
  {
  char *tmp = NULL;
@@ -11332,6 +11334,10 @@ virDomainShmemDefParseXML(xmlNodePtr node,
  if (virDomainDeviceInfoParseXML(node, NULL, def-info, flags)  0)
  goto cleanup;

+if (virSecurityDeviceLabelDefParseXML(def-seclabels, def-nseclabels,
+  vmSeclabels, nvmSeclabels,
+  ctxt, flags)  0)
+goto cleanup;

  ret = def;
  def = NULL;
@@ -12457,7 +12463,11 @@ virDomainDeviceDefParse(const char *xmlStr,
  goto error;
  break;
  case VIR_DOMAIN_DEVICE_SHMEM:
-if (!(dev-data.shmem = virDomainShmemDefParseXML(node, ctxt, flags)))
+if (!(dev-data.shmem = virDomainShmemDefParseXML(node,
+  ctxt,
+  def-seclabels,
+  def-nseclabels,
+  flags)))
  goto error;
  break;
  case VIR_DOMAIN_DEVICE_TPM:
@@ -16136,7 +16146,8 @@ virDomainDefParseXML(xmlDocPtr xml,
  for (i = 0; i  n; i++) {
  virDomainShmemDefPtr shmem;
  ctxt-node = nodes[i];
-shmem = virDomainShmemDefParseXML(nodes[i], ctxt, flags);
+shmem = virDomainShmemDefParseXML(nodes[i], ctxt, def-seclabels,
+  def-nseclabels, flags);
  if (!shmem)
  goto error;

@@ -20308,6 +20319,8 @@ virDomainShmemDefFormat(virBufferPtr buf,
  virDomainShmemDefPtr def,
  unsigned int flags)
  {
+size_t n;
+
  virBufferEscapeString(buf, shmem name='%s', def-name);

  if (!def-size 
@@ -20341,6 +20354,9 @@ virDomainShmemDefFormat(virBufferPtr buf,
  virBufferAddLit(buf, /\n);
  }

+for (n = 0; n  def-nseclabels; n++)
+virSecurityDeviceLabelDefFormat(buf, def-seclabels[n], flags);
+
  if (virDomainDeviceInfoFormat(buf, def-info, flags)  0)
  return -1;

@@ -23851,11 +23867,25 @@ virDomainObjListExport(virDomainObjListPtr domlist,
  }


+static virSecurityDeviceLabelDefPtr
+virDomainGetDeviceSecurityLabelDef(virSecurityDeviceLabelDefPtr *seclabels,
+   size_t nseclabels,
+   const char *model)
+{
+size_t i;
+
+for (i = 0; i  nseclabels; i++) {
+if (STREQ_NULLABLE(seclabels[i]-model, model))
+return seclabels[i];
+}
+return NULL;
+}
+
+
  

Re: [libvirt] [PATCH 1/4] conf: introduce seclabels in shmem device element

[lhuang]

On 07/30/2015 05:48 PM, Daniel P. Berrange wrote:

On Thu, Jul 23, 2015 at 06:13:46PM +0800, Luyao Huang wrote:

Introduce a new element in shmem device element, this
could help users to change the shm label to a specified
label.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  docs/formatdomain.html.in |  7 ++
  docs/schemas/domaincommon.rng |  3 +++
  src/conf/domain_conf.c| 55 ++-
  src/conf/domain_conf.h|  5 
  4 files changed, 59 insertions(+), 11 deletions(-)

As already mentioned, this must include additions to the qemu tests
suite for XML to XML conversion.


I must forgot this during wrote this patch, thanks for pointing out 
that, i will add a tests for the new XML element in next version.


Thanks a lot for your review.


Regards,
Daniel


Luyao

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

Re: [libvirt] [PATCH] virsh: fix no error when pass a count = 0 to setvcpus

[lhuang]

On 07/30/2015 03:29 PM, Andrea Bolognani wrote:

On Thu, 2015-07-30 at 10:57 +0800, Luyao Huang wrote:

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

When count = 0, the client exit without set an error.

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

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index f7edeeb..b6da684 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6744,8 +6744,12 @@ cmdSetvcpus(vshControl *ctl, const vshCmd
*cmd)
  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
  return false;
  
-if (vshCommandOptInt(ctl, cmd, count, count)  0 || count =

0)
+if (vshCommandOptInt(ctl, cmd, count, count)  0)
  goto cleanup;
+if (count = 0) {
+vshError(ctl, _(Invalid value '%d' for number of virtual
CPUs), count);
+goto cleanup;
+}
  
  /* none of the options were specified */

  if (!current  flags == 0) {

Nice catch, but I would go for the following diff instead:

-8-
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index a61656f..4b8ebbd 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6819,7 +6819,7 @@ static bool
  cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  {
  virDomainPtr dom;
-int count = 0;
+unsigned int count = 0;
  bool ret = false;
  bool maximum = vshCommandOptBool(cmd, maximum);
  bool config = vshCommandOptBool(cmd, config);
@@ -6846,8 +6846,15 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
  return false;
  
-if (vshCommandOptInt(ctl, cmd, count, count)  0 || count = 0)

+if (vshCommandOptUInt(ctl, cmd, count, count)  0)
+goto cleanup;
+
+if (count == 0) {
+vshError(ctl,
+ _(Numeric value '%s' for %s option is malformed or
out of range),
+ 0, count);
  goto cleanup;
+}
  
  /* none of the options were specified */

  if (!current  flags == 0) {
-8-

It's slightly more awkward, but his way we make sure the
error message is the same whether the value used for the
count option is foo, 0 or -20. vshCommandOptUInt()
already uses that error message internally.

Does it look good to you?


I think a clear error is works for me, i am not good at the error 
message for a long time ;)


Thanks a lot for your quick review.


Cheers.




Luyao

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

Re: [libvirt] [PATCH 0/4] manage the shmem device source

[lhuang]

On 07/28/2015 12:05 AM, Daniel P. Berrange wrote:

On Mon, Jul 27, 2015 at 03:40:36PM +0100, Daniel P. Berrange wrote:

On Mon, Jul 27, 2015 at 04:09:01PM +0200, Marc-André Lureau wrote:

Hi Luyao

On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huanglhu...@redhat.com  wrote:

But the problem is we cannot change the running shm-server process label,
We need wait ivshmem-server to be a part of qemu progrem, then setup the
ivshmem-server by libvirt. we cannot do nothing for the ivshmem-server right 
now.

ivshmem-server is going to be a separate server process, not part of
qemu process.

Is it enough if ivshmem-server is started by libvirt to solve the selinux issue?

What's missing to get started to support it with libvirt?

The complexity arises when multiple QEMUs want to connect to the same
memory region. Each QEMU has its own unique SELinux label (eg something
like svirt_t:s0:c352,c850 with random category values) So there is
no single SElinux label you can give to an ivshmem server process to
let it just work with multiple QEMUs, unless we chose to effectively
just let any QEMU connect whatsoever by running ivmshmem-server under
svirt_t:s0:c0.c1023 which removes all isolation between the guests.
This is label we use for disk images which must be shared between
QEMUs currently, but long term we're going to need to come up with
a way to allow concurrent access but kep separation. At that point
we'll likely need to implement the ivmshmem server as part of the
libvirt project itself, so we can deal with SELinux.

Until that point though, I think the simplest thing todo is to get
an addition to the SELinux policy. We want to have

   - ivshmem-server running under a 'qemu_shmemd_t' type
   - ivshmem-server UNIX domain socket labeled 'qemu_shmemd_sock_t'
   - svirt_t permitted to connect to any qemu_shmemd_sock_t

this doesn't require any code in libvirt or QEMU - should be possible
todo it entirely in selinux policy rules.

Just for clarification - this means I am NACK'ing all 4 patches
here, because I don't think any of this extra code is needed.


libvirt still need fix the issues when use shmem device without 
server(that is why i want manage the shmem resource ):


1. qemu won't unlink the share memory object after exit, and when we 
restart the guest which use the old
shmem device name but size is bigger than old shmem device, qemu will 
forbid guest start. then user need unlink the shm obj by themselves or 
give a new name to the shmem device (this will leak the shm obj).


2. if let qemu create shm obj the label will depend on qemu process 
label and like this:


-rwxrwxr-x. qemu   qemu   system_u:object_r:svirt_tmpfs_t:s0 my_shmem1

This label is not under libvirt control, and if the user want only one 
guest use this (for host-guest connect) , the label is not good enough.


3. it will be better if we offer a way to users to specify the label by 
themselves, because we cannot know how the user will use the shmem 
device, so the label maybe not correct in some case.



So i wrote some helper to manage the shmem device resource to solve 3 
issues, ivshmem server selinux label is a small part of them, i could 
remove that part in a new version, i think the first and second issue 
still need fix.


Thanks a lot for your review and advise.


Regards,
Daniel


Luyao

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

Re: [libvirt] [PATCH 0/4] manage the shmem device source

[lhuang]

Hi Marc-André

On 07/27/2015 10:09 PM, Marc-André Lureau wrote:

Hi Luyao

On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang lhu...@redhat.com wrote:

But the problem is we cannot change the running shm-server process label,
We need wait ivshmem-server to be a part of qemu progrem, then setup the
ivshmem-server by libvirt. we cannot do nothing for the ivshmem-server right 
now.

ivshmem-server is going to be a separate server process, not part of
qemu process.


OKay, thanks for clarification, i saw the patches in qemu-devel mail 
list, so i thought it will be a part of qemu project :)



Is it enough if ivshmem-server is started by libvirt to solve the selinux issue?


I think it is enough, also as Daniel's reply: it will be better if 
ivshmem-server could be a part of libvirt.



What's missing to get started to support it with libvirt?



I think is the ivshmem-server set up and audit part, and i think Martin 
will be the best people to give a answer.


Luyao

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

Re: [libvirt] [PATCH] qemu: fix the audit log is not correct after hot-plug memory success

[lhuang]

On 07/24/2015 08:14 PM, John Ferlan wrote:


On 07/17/2015 04:52 AM, Luyao Huang wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1226234#c3

After hot-plug a memory device success, the audit log show
that memory update failed:

type=VIRT_RESOURCE ... old-mem=1024000 new-mem=1548288 \
exe=/usr/sbin/libvirtd hostname=? addr=? terminal=pts/2 res=failed

This is because the ret is still -1 when we call audit function to help

Also we need audit when hot-plug/hot-unplug get failed in qemu side.
And i notice we use virDomainDefGetMemoryActual to get the newmem
, but when we failed to attach the memory device we the 
virDomainDefGetMemoryActual
will still output the oldmem size, so the audit log will not right
in that case.

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


Since it's two issues there should be two patches


OKay, i will split them in two patches, thanks your advise.


diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1ea397f..cf7ffa9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1745,6 +1745,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
  qemuDomainObjPrivatePtr priv = vm-privateData;
  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
  unsigned long long oldmem = virDomainDefGetMemoryActual(vm-def);
+unsigned long long newmem = oldmem + mem-size;
  char *devstr = NULL;
  char *objalias = NULL;
  const char *backendType;
@@ -1800,7 +1801,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,

Above this line, if there's a failure from virDomainMemoryInsert, the
code goes to cleanup.  Should that audit failure too?


Actually i still not know the audit rules of attach some device but get 
fail, and i notice the old function (attach disk,network): these 
functions will audit the failure when there is a failure when qemu get 
fail, and won't audit the failure before qemuDomainObjEnterMonitor(). So 
in my opinion if there's a failure from virDomainMemoryInsert, no need 
audit failure, because we still not really try to attach such device :)



  if (qemuDomainObjExitMonitor(driver, vm)  0) {
  /* we shouldn't touch mem now, as the def might be freed */
  mem = NULL;
-goto cleanup;
+goto audit;
  }
  
  event = virDomainEventDeviceAddedNewFromObj(vm, objalias);

@@ -1811,9 +1812,6 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
  if (fix_balloon)
  vm-def-mem.cur_balloon += mem-size;
  
-virDomainAuditMemory(vm, oldmem, virDomainDefGetMemoryActual(vm-def),

- update, ret == 0);
-

An alternative method would be to :


 if ((ret = qemuMonitorAddDevice(priv-mon, devstr))  0) {
...
 }

 if (qemuDomainObjExitMonitor(driver, vm)  0) {
  /* we shouldn't touch mem now, as the def might be freed */
  mem = NULL;
  ret = -1;
 }

 virDomainAuditMemory(vm, oldmem, newmem, update, ret == 0);
 if (ret  0)
 goto cleanup;

FWIW:
It seems many paths in the code will fail to audit if ExitMonitor
fails. There's no consensus as to the best mechanism. Although the
AuditNet's and most AuditChardev do at least try. The calls around
AuditHostdev are close, but the set ret = -1 and jump to cleanup before
audit which has a jump to cleanup if ret == -1.

Cleanup of those is something for another day it seems though...


Some of them is easy to fix but some for them is hard to fix (need 
refactor functions), we could fix them later ;)



  /* mem is consumed by vm-def */
  mem = NULL;
  
@@ -1823,6 +1821,8 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
  
  ret = 0;
  
+ audit:

+virDomainAuditMemory(vm, oldmem, newmem, update, ret == 0);
   cleanup:
  virObjectUnref(cfg);
  VIR_FREE(devstr);
@@ -1833,7 +1833,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
   removedef:

Rather than mess with the goto audit changes here, why not just :

 virDomainAuditMemory(vm, oldmem, newmem, update, false);


See qemuDomainChangeEjectableMedia for precedent.


Hmm...but then i need 3 virDomainAuditMemory() functions:

if ((ret = qemuMonitorAddDevice(priv-mon, devstr))  0) {
...
}

if (qemuDomainObjExitMonitor(driver, vm)  0) {
 /* we shouldn't touch mem now, as the def might be freed */
 mem = NULL;
 ret = -1;
}

virDomainAuditMemory(vm, oldmem, newmem, update, ret == 0);    one
if (ret  0)
goto cleanup;

...

 removedef:
if (qemuDomainObjExitMonitor(driver, vm)  0) {
mem = NULL;
virDomainAuditMemory(vm, oldmem, newmem, update, false); ---two
goto cleanup;
}

if ((id = virDomainMemoryFindByDef(vm-def, mem)) = 0)
mem = virDomainMemoryRemove(vm-def, id);
else
mem = NULL;

virDomainAuditMemory(vm, oldmem, newmem, update, false); three

Re: [libvirt] [PATCH 2/2] test: introduce a function in test driver to check get vcpupin info

[lhuang]

On 07/24/2015 06:52 PM, John Ferlan wrote:


On 07/14/2015 09:10 AM, Luyao Huang wrote:

As there is a regression in use vcpupin get info, introduce a new function
to test the virsh client.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/test/test_driver.c | 55 ++
  tests/vcpupin  | 34 +++
  2 files changed, 85 insertions(+), 4 deletions(-)


ACK and pushed the test (since 1/2 was fixed by a different commit)


Thanks a lot for your review.


John


Luyao

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

Re: [libvirt] [PATCH] qemu: fix the error cover issue in SetMemoryParameters

[lhuang]

On 07/22/2015 05:06 PM, Martin Kletzander wrote:

On Wed, Jul 22, 2015 at 03:35:14PM +0800, Luyao Huang wrote:

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

We won't return the errno after commit 0d7f45ae, and
the more clearly error will be set in the code in vircgroup*.
Also We will always report error Operation not permitted,
because the return is -1.



All called methods set the error appropriately, ACK  pushed.



Thanks a lot for your quick review.


Martin


Luyao

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

Re: [libvirt] [PATCH 1/2] virsh: really fix the error if vcpu number exceed the guest maxvcpu number

[lhuang]

On 07/15/2015 05:21 AM, John Ferlan wrote:


On 07/14/2015 09:10 AM, Luyao Huang wrote:

Commit '848ab68' left a issue: when we try to get a vcpupin info with no
no flags or --current flags, we still will get the wrong error. Because
VIR_DOMAIN_AFFECT_CURRENT is 0, so this check flags  VIR_DOMAIN_AFFECT_CURRENT
will always get false.

Signed-off-by: Luyao Huang lhu...@redhat.com
Reported by: Peter Krempa pkre...@redhat.com
---
  tools/virsh-domain.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)


hmmm ...  see Michal's patch on this (and my response):

http://www.redhat.com/archives/libvir-list/2015-July/msg00555.html

Of course when I merged your change when I made the original adjustment
based on review, I never ran it through my coverity checker either).


yes, Michal must notice the coverity either, so his patch will fix this 
issue.


Thanks a lot for your review.


John



Luyao

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

Re: [libvirt] [PATCH] RFC: audit: add shmem resource type

[lhuang]

Hi Marc-André:

On 07/13/2015 05:49 AM, Marc-André Lureau wrote:

On Sun, Jul 12, 2015 at 12:36 PM, Martin Kletzander mklet...@redhat.com wrote:

As said in previous attempt by Luyao to do this, the auditing should
be handled differently, there's also lot more info to audit.  Thanks
for the patch, but this must be done in another way.


Could you please give me a pointer?


Here : http://www.redhat.com/archives/libvir-list/2015-July/msg00316.html

As Martin's reply in this mail, the memory size is not enought, we need 
audit more information (just like shmobj name, the server socket path).


Thanks a lot for your patch.


thanks



Luyao

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

Re: [libvirt] [PATCH] qemu: fix no error settings if fail to find a disk match path

[lhuang]

On 07/09/2015 11:49 AM, Luyao Huang wrote:

When we use get blockjob info to a unexist disk path, we will
get a error like this:

  # virsh blockjob r7 vdc
  error: An error occurred, but the cause is unknown

This is because we do not set the error when jump to endjob.
As virDomainDiskByName won't set the error, we need set them
in the callers function.


Sorry for forget the bz:

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


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

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 900740e..f134248 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16414,8 +16414,11 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom,
  if (qemuDomainSupportsBlockJobs(vm, NULL)  0)
  goto endjob;
  
-if (!(disk = virDomainDiskByName(vm-def, path, true)))

+if (!(disk = virDomainDiskByName(vm-def, path, true))) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _(invalid path %s not assigned to domain), path);
  goto endjob;
+}
  
  qemuDomainObjEnterMonitor(driver, vm);

  ret = qemuMonitorGetBlockJobInfo(qemuDomainGetMonitor(vm),


Luyao

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

Re: [libvirt] [PATCH] qemu: fix no error settings if fail to find a disk match path

[lhuang]

On 07/09/2015 02:37 PM, Martin Kletzander wrote:

On Thu, Jul 09, 2015 at 11:49:15AM +0800, Luyao Huang wrote:

When we use get blockjob info to a unexist disk path, we will
get a error like this:

# virsh blockjob r7 vdc
error: An error occurred, but the cause is unknown

This is because we do not set the error when jump to endjob.
As virDomainDiskByName won't set the error, we need set them
in the callers function.

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

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 900740e..f134248 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16414,8 +16414,11 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom,
if (qemuDomainSupportsBlockJobs(vm, NULL)  0)
goto endjob;

-if (!(disk = virDomainDiskByName(vm-def, path, true)))
+if (!(disk = virDomainDiskByName(vm-def, path, true))) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _(invalid path %s not assigned to domain), 
path);


Since the 'path' parameter can be both path and a disk name
(e.g. 'vdc' in your example), I wouldn't use path here because it
looks weird when you get an error saying:

 invalid path vdc not assigned to domain

I'd change it to something more abstract like:

 Disk '%s' not found in the domain

or anything else but with such specification.  If you're OK with that
I'll push it with the modification (also fixing the commit message).



Okay, i think it will be ok for me as it sounds more friendly.

Thanks a lot for your quick review and help.

Luyao

goto endjob;
+}

qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetBlockJobInfo(qemuDomainGetMonitor(vm),
--
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 06/10] conf: Add helpers to insert/remove/find shmem devices in domain def

[lhuang]

On 07/09/2015 02:09 PM, Martin Kletzander wrote:

On Thu, Jul 09, 2015 at 11:44:30AM +0800, lhuang wrote:


On 07/08/2015 08:14 PM, Martin Kletzander wrote:

On Wed, Jun 17, 2015 at 11:56:17AM +0800, Luyao Huang wrote:

The helpers will be useful when implementing hotplug and coldplug of
shared memory devices.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
src/conf/domain_conf.c   | 61

src/conf/domain_conf.h   |  7 ++
src/libvirt_private.syms |  3 +++
3 files changed, 71 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 306b718..8a8e4f7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13343,6 +13343,67 @@ virDomainMemoryRemove(virDomainDefPtr def,
}


+int
+virDomainShmemInsert(virDomainDefPtr def,
+ virDomainShmemDefPtr shmem)
+{
+return VIR_APPEND_ELEMENT(def-shmems, def-nshmems, shmem);
+}
+
+
+ssize_t
+virDomainShmemFind(virDomainDefPtr def,
+   virDomainShmemDefPtr shmem)
+{
+size_t i;
+
+for (i = 0; i  def-nshmems; i++) {
+ virDomainShmemDefPtr tmpshmem = def-shmems[i];
+
+ if (STRNEQ_NULLABLE(shmem-name, tmpshmem-name))
+ continue;
+


I think that you shouldn't be able to have two shmem/ elements in
the same domain, and since the name is mandatory, STREQ() should be
enough to check whether you need to return current 'i'.



Okay, i agree use STREQ() instead of STRNEQ_NULLABLE. BTW I think one
guest could have more than one shmem/ element, maybe one is
non-server shmem and another is server shmem, it depends on how to use
it.



Maybe this function could have a bool parameter (something like a
'full_match') that would say whether everything must match or the name
is enough.  And it the bool is false, streq is enough, but if it's
true, you could abstract the internals of this for body into
virDomainShmemEquals() or something and that would be called instead.



Good idea, this way is okay to me.

Thanks a lot for your opinion.

Luyao


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

Re: [libvirt] [PATCH] qemu: move the guest status check before agent config and status check

[lhuang]

On 07/08/2015 04:57 PM, Michal Privoznik wrote:

On 03.07.2015 08:58, Luyao Huang wrote:

When use setvcpus command with --guest option to a offline vm,
we will get error:

  # virsh setvcpus test3 1 --guest
  error: Guest agent is not responding: QEMU guest agent is not connected

However guest is not running, agent status could not be connected.
In this case, report domain is not running will be better than agent is
not connected. Move the guest status check more early to output error to
point out guest status is not right.

Also from the logic, a running vm is a basic requirement to use
agent, we cannot use agent if vm is not running.

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

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f9bf32c..814fb2c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3084,6 +3084,13 @@ qemuDomainAgentAvailable(virDomainObjPtr vm,
  }
  return false;
  }
+if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) {
+if (reportError) {
+virReportError(VIR_ERR_OPERATION_INVALID, %s,
+   _(domain is not running));
+}
+return false;
+}
  if (!priv-agent) {
  if (qemuFindAgentConfig(vm-def)) {
  if (reportError) {
@@ -3099,13 +3106,6 @@ qemuDomainAgentAvailable(virDomainObjPtr vm,
  return false;
  }
  }
-if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) {
-if (reportError) {
-virReportError(VIR_ERR_OPERATION_INVALID, %s,
-   _(domain is not running));
-}
-return false;
-}
  return true;
  }
  


I think the check could have been moved even one block up. I mean, it
could be the very first check in the function.


Indeed


I've moved the check, ACKed and pushed.


Thanks a lot for your help and review.


Michal


Luyao

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

Re: [libvirt] [PATCH 01/10] qemu: auto assign pci address for shared memory device

[lhuang]

On 07/08/2015 05:37 PM, Martin Kletzander wrote:

On Fri, Jul 03, 2015 at 02:39:49PM +0200, Martin Kletzander wrote:

On Wed, Jun 17, 2015 at 11:56:12AM +0800, Luyao Huang wrote:

Shared memory device is base on PCI address, even we do not
pass the pci address to qemu, qemu will auto assign a pci
address for it.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
src/qemu/qemu_command.c| 11 +++
tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 16 
2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 89f775d..5ac43d8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2562,6 +2562,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
  flags)  0)
   goto error;
   }
+


Spurious change, ACK without that.



I also squashed in the following to make sure it works fine (which it
does):



Good idea ! Thanks a lot for your help.

diff --git i/tests/qemuxml2argvdata/qemuxml2argv-shmem.args 
w/tests/qemuxml2argvdata/qemuxml2argv-shmem.args

index 4c383db6985f..08cd5ac4588e 100644
--- i/tests/qemuxml2argvdata/qemuxml2argv-shmem.args
+++ w/tests/qemuxml2argvdata/qemuxml2argv-shmem.args
@@ -2,8 +2,8 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test 
LOGNAME=test QEMU_AUDIO_DRV=none \

/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults \
-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \
-device ivshmem,shm=shmem0,id=shmem0,bus=pci.0,addr=0x3 \
--device ivshmem,size=128m,shm=shmem1,id=shmem1,bus=pci.0,addr=0x4 \
--device ivshmem,size=256m,shm=shmem2,id=shmem2,bus=pci.0,addr=0x5 \
+-device ivshmem,size=128m,shm=shmem1,id=shmem1,bus=pci.0,addr=0x5 \
+-device ivshmem,size=256m,shm=shmem2,id=shmem2,bus=pci.0,addr=0x4 \
-device 
ivshmem,size=512m,chardev=charshmem3,id=shmem3,bus=pci.0,addr=0x6 \

-chardev socket,id=charshmem3,path=/var/lib/libvirt/shmem-shmem3-sock \
-device 
ivshmem,size=1024m,chardev=charshmem4,id=shmem4,bus=pci.0,addr=0x7 \
diff --git i/tests/qemuxml2argvdata/qemuxml2argv-shmem.xml 
w/tests/qemuxml2argvdata/qemuxml2argv-shmem.xml

index fd79c89c1a43..d4b38f91b050 100644
--- i/tests/qemuxml2argvdata/qemuxml2argv-shmem.xml
+++ w/tests/qemuxml2argvdata/qemuxml2argv-shmem.xml
@@ -23,6 +23,7 @@
/shmem
shmem name='shmem2'
  size unit='M'256/size
+  address type='pci' domain='0x' bus='0x00' slot='0x04' 
function='0x0'/

/shmem
shmem name='shmem3'
  size unit='M'512/size




Luyao

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

Re: [libvirt] [PATCH] qemu: remove deadcode in qemuDomain{HelperGetVcpus|GetIOThreadsLive}

[lhuang]

On 07/08/2015 04:27 PM, Michal Privoznik wrote:

On 03.07.2015 03:57, Luyao Huang wrote:

We set hostcpus but not use them.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_driver.c | 8 
  1 file changed, 8 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7a04e67..3f002b3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1418,13 +1418,9 @@ static int
  qemuDomainHelperGetVcpus(virDomainObjPtr vm, virVcpuInfoPtr info, int maxinfo,
   unsigned char *cpumaps, int maplen)
  {
-int hostcpus;
  size_t i, v;
  qemuDomainObjPrivatePtr priv = vm-privateData;
  
-if ((hostcpus = nodeGetCPUCount())  0)

-return -1;
-
  if (priv-vcpupids == NULL) {
  virReportError(VIR_ERR_OPERATION_INVALID,
 %s, _(cpu affinity is not supported));
@@ -5579,7 +5575,6 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
  qemuMonitorIOThreadInfoPtr *iothreads = NULL;
  virDomainIOThreadInfoPtr *info_ret = NULL;
  int niothreads = 0;
-int hostcpus;
  size_t i;
  int ret = -1;
  
@@ -5612,9 +5607,6 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,

  goto endjob;
  }
  
-if ((hostcpus = nodeGetCPUCount())  0)

-goto endjob;
-
  if (VIR_ALLOC_N(info_ret, niothreads)  0)
  goto endjob;
  


Tweaked the commit message a bit, ACKed and pushed.


Thanks a lot 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] [PATCH 05/10] conf:audit: introduce audit function for shared memory device

[lhuang]

On 07/08/2015 07:56 PM, Martin Kletzander wrote:

On Wed, Jun 17, 2015 at 11:56:16AM +0800, Luyao Huang wrote:

Signed-off-by: Luyao Huang lhu...@redhat.com
---
docs/auditlog.html.in| 16 
src/conf/domain_audit.c  | 16 
src/conf/domain_audit.h  |  6 ++
src/libvirt_private.syms |  1 +
4 files changed, 39 insertions(+)

diff --git a/docs/auditlog.html.in b/docs/auditlog.html.in
index 8a007ca..b168cbf 100644
--- a/docs/auditlog.html.in
+++ b/docs/auditlog.html.in
@@ -301,6 +301,22 @@
  ddUpdated path of the backing character device for given 
emulated device/dd

/dl

+h4a name=typeresourceivshmemShared memory device/a/h4
+p
+  The codemsg/code field will include the following sub-fields
+/p
+
+dl
+  dtreason/dt
+  ddThe reason which caused the resource to be assigned to 
happen/dd

+  dtresrc/dt
+  ddThe type of resource assigned. Set to codeshmem/code/dd
+  dtold-shmem/dt
+  ddOriginal memory size of share memory device in bytes, or 
0/dd

+  dtnew-shmem/dt
+  ddUpdated memory size of share memory device in bytes/dd


I don't think memory size is the thing audit cares about, it should be
the name/path mostly.  Even better if we could audit all of it (size,
name, path).


Okay, i agreed with you,

Thanks a lot for your review.

Luyao




+/dl
+
h4a name=typeresourcesmartcardsmartcard/a/h4
p
  The codemsg/code field will include the following sub-fields
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index 1900039..aa2b4b5 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -799,6 +799,19 @@ virDomainAuditIOThread(virDomainObjPtr vm,
  reason, success);
}

+
+void
+virDomainAuditShmem(virDomainObjPtr vm,
+virDomainShmemDefPtr oldDef, 
virDomainShmemDefPtr newDef,

+const char *reason, bool success)
+{
+return virDomainAuditResource(vm, shmem,
+  oldDef ? oldDef-size : 0,
+  newDef ? newDef-size : 0,
+  reason, success);
+}
+
+
static void
virDomainAuditLifecycle(virDomainObjPtr vm, const char *op,
const char *reason, bool success)
@@ -880,6 +893,9 @@ virDomainAuditStart(virDomainObjPtr vm, const 
char *reason, bool success)

for (i = 0; i  vm-def-nrngs; i++)
virDomainAuditRNG(vm, NULL, vm-def-rngs[i], start, true);

+for (i = 0; i  vm-def-nshmems; i++)
+virDomainAuditShmem(vm, NULL, vm-def-shmems[i], start, 
true);

+
if (vm-def-tpm)
virDomainAuditTPM(vm, vm-def-tpm, start, true);

diff --git a/src/conf/domain_audit.h b/src/conf/domain_audit.h
index 97dadca..081cbb1 100644
--- a/src/conf/domain_audit.h
+++ b/src/conf/domain_audit.h
@@ -129,6 +129,12 @@ void virDomainAuditRNG(virDomainObjPtr vm,
   const char *reason,
   bool success)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4);
+void virDomainAuditShmem(virDomainObjPtr vm,
+ virDomainShmemDefPtr oldDef,
+ virDomainShmemDefPtr newDef,
+ const char *reason,
+ bool success)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4);


#endif /* __VIR_DOMAIN_AUDIT_H__ */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index dc8a52d..3ceb4e3 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -134,6 +134,7 @@ virDomainAuditNetDevice;
virDomainAuditRedirdev;
virDomainAuditRNG;
virDomainAuditSecurityLabel;
+virDomainAuditShmem;
virDomainAuditStart;
virDomainAuditStop;
virDomainAuditVcpu;
--
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 10/10] qemu: report error when shmem have a invalid address

[lhuang]

On 07/08/2015 10:30 PM, Martin Kletzander wrote:

On Wed, Jul 08, 2015 at 02:22:36PM +0200, Martin Kletzander wrote:

On Wed, Jun 17, 2015 at 11:56:21AM +0800, Luyao Huang wrote:

If user pass a invalid address shared memory device
to qemu, qemu won't report the error, but will auto
assign a pci address to the shared memory device.

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



ACK



I also added a test case for this particular patch and pushed it along
with the other ACK'd ones.



Thanks a lot for your review and help.

Luyao


The test case diff squashed in:

diff --git c/tests/qemuxml2argvdata/qemuxml2argv-shmem-msi-only.xml 
i/tests/qemuxml2argvdata/qemuxml2argv-shmem-invalid-address.xml

similarity index 95%
copy from tests/qemuxml2argvdata/qemuxml2argv-shmem-msi-only.xml
copy to tests/qemuxml2argvdata/qemuxml2argv-shmem-invalid-address.xml
index d70279c21faa..8a4e56d5926a 100644
--- c/tests/qemuxml2argvdata/qemuxml2argv-shmem-msi-only.xml
+++ i/tests/qemuxml2argvdata/qemuxml2argv-shmem-invalid-address.xml
@@ -18,7 +18,7 @@
controller type='pci' index='0' model='pci-root'/
memballoon model='none'/
shmem name='shmem0'
-  msi/
+  address type='isa'/
/shmem
  /devices
/domain
diff --git c/tests/qemuxml2argvtest.c i/tests/qemuxml2argvtest.c
index bee66372767b..24c1f301e4b9 100644
--- c/tests/qemuxml2argvtest.c
+++ i/tests/qemuxml2argvtest.c
@@ -1614,6 +1614,8 @@ mymain(void)
DO_TEST_FAILURE(shmem, NONE);
DO_TEST_FAILURE(shmem-invalid-size, QEMU_CAPS_PCIDEVICE,
QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_IVSHMEM);
+DO_TEST_FAILURE(shmem-invalid-address, QEMU_CAPS_PCIDEVICE,
+QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_IVSHMEM);
DO_TEST_FAILURE(shmem-small-size, QEMU_CAPS_PCIDEVICE,
QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_IVSHMEM);
DO_TEST_PARSE_ERROR(shmem-msi-only, NONE);
--


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

Re: [libvirt] [PATCH 06/10] conf: Add helpers to insert/remove/find shmem devices in domain def

[lhuang]

On 07/08/2015 08:14 PM, Martin Kletzander wrote:

On Wed, Jun 17, 2015 at 11:56:17AM +0800, Luyao Huang wrote:

The helpers will be useful when implementing hotplug and coldplug of
shared memory devices.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
src/conf/domain_conf.c   | 61 


src/conf/domain_conf.h   |  7 ++
src/libvirt_private.syms |  3 +++
3 files changed, 71 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 306b718..8a8e4f7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13343,6 +13343,67 @@ virDomainMemoryRemove(virDomainDefPtr def,
}


+int
+virDomainShmemInsert(virDomainDefPtr def,
+ virDomainShmemDefPtr shmem)
+{
+return VIR_APPEND_ELEMENT(def-shmems, def-nshmems, shmem);
+}
+
+
+ssize_t
+virDomainShmemFind(virDomainDefPtr def,
+   virDomainShmemDefPtr shmem)
+{
+size_t i;
+
+for (i = 0; i  def-nshmems; i++) {
+ virDomainShmemDefPtr tmpshmem = def-shmems[i];
+
+ if (STRNEQ_NULLABLE(shmem-name, tmpshmem-name))
+ continue;
+


I think that you shouldn't be able to have two shmem/ elements in
the same domain, and since the name is mandatory, STREQ() should be
enough to check whether you need to return current 'i'.



Okay, i agree use STREQ() instead of STRNEQ_NULLABLE. BTW I think one 
guest could have more than one shmem/ element, maybe one is non-server 
shmem and another is server shmem, it depends on how to use it.



Well, depending on whether you are unplugging it (you want everything
that was specified to match) or plugging it in (if there's the same
name, just reject it).



Okay, we could just check the name if use the same shmem (both server 
and non-server) in the same guest is a invalid case.


Thanks a lot for your review.

Luyao


+ if (shmem-size != tmpshmem-size)
+ continue;
+
+ if (shmem-server.enabled != tmpshmem-server.enabled ||
+ (shmem-server.enabled 
+ STRNEQ_NULLABLE(shmem-server.chr.data.nix.path,
+ tmpshmem-server.chr.data.nix.path)))
+ continue;
+
+ if (shmem-msi.enabled != tmpshmem-msi.enabled ||
+ (shmem-msi.enabled 
+  (shmem-msi.vectors != tmpshmem-msi.vectors ||
+   shmem-msi.ioeventfd != tmpshmem-msi.ioeventfd)))
+ continue;
+
+if (shmem-info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE 
+ !virDomainDeviceInfoAddressIsEqual(shmem-info, tmpshmem-info))
+continue;
+
+break;
+}
+
+if (i  def-nshmems)
+return i;
+
+return -1;
+}
+
+
+virDomainShmemDefPtr
+virDomainShmemRemove(virDomainDefPtr def,
+ size_t idx)
+{
+virDomainShmemDefPtr ret = def-shmems[idx];
+
+VIR_DELETE_ELEMENT(def-shmems, idx, def-nshmems);
+
+return ret;
+}
+
+
char *
virDomainDefGetDefaultEmulator(virDomainDefPtr def,
   virCapsPtr caps)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a4b1bf3..39bc928 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2943,6 +2943,13 @@ int 
virDomainMemoryFindInactiveByDef(virDomainDefPtr def,

 virDomainMemoryDefPtr mem)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;

+int virDomainShmemInsert(virDomainDefPtr def, virDomainShmemDefPtr 
shmem)

+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+ssize_t virDomainShmemFind(virDomainDefPtr def, virDomainShmemDefPtr 
shmem)

+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+virDomainShmemDefPtr virDomainShmemRemove(virDomainDefPtr def, 
size_t idx)

+ATTRIBUTE_NONNULL(1);
+
VIR_ENUM_DECL(virDomainTaint)
VIR_ENUM_DECL(virDomainVirt)
VIR_ENUM_DECL(virDomainBoot)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3ceb4e3..6127f51 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -439,6 +439,9 @@ virDomainSaveStatus;
virDomainSaveXML;
virDomainSeclabelTypeFromString;
virDomainSeclabelTypeToString;
+virDomainShmemFind;
+virDomainShmemInsert;
+virDomainShmemRemove;
virDomainShutdownReasonTypeFromString;
virDomainShutdownReasonTypeToString;
virDomainShutoffReasonTypeFromString;
--
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 02/10] qemu: always build id when generate shared memory device CLI

[lhuang]

On 07/03/2015 08:41 PM, Martin Kletzander wrote:

On Wed, Jun 17, 2015 at 11:56:13AM +0800, Luyao Huang wrote:

When hot-unplug the device, qmp command device_del require a
device id.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
src/qemu/qemu_command.c|  4 ++--
tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 16 
2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 5ac43d8..636e040 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8470,9 +8470,9 @@ qemuBuildShmemDevCmd(virCommandPtr cmd,
}

if (!shmem-server.enabled) {
-virBufferAsprintf(buf, ,shm=%s, shmem-name);
+virBufferAsprintf(buf, ,shm=%s,id=%s, shmem-name, 
shmem-info.alias);

} else {
-virBufferAsprintf(buf, ,chardev=char%s, shmem-info.alias);
+virBufferAsprintf(buf, ,chardev=char%s,id=%s, 
shmem-info.alias, shmem-info.alias);

if (shmem-msi.enabled) {
virBufferAddLit(buf, ,msi=on);
if (shmem-msi.vectors)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args 
b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args

index d37879a..601167c 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args
@@ -1,16 +1,16 @@
LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test 
QEMU_AUDIO_DRV=none \

/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults \
-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \
--device ivshmem,shm=shmem0,bus=pci.0,addr=0x3 \
--device ivshmem,size=128m,shm=shmem1,bus=pci.0,addr=0x4 \
--device ivshmem,size=256m,shm=shmem2,bus=pci.0,addr=0x5 \
--device ivshmem,size=512m,chardev=charshmem3,bus=pci.0,addr=0x6 \
+-device ivshmem,shm=shmem0,id=shmem0,bus=pci.0,addr=0x3 \
+-device ivshmem,size=128m,shm=shmem1,id=shmem1,bus=pci.0,addr=0x4 \
+-device ivshmem,size=256m,shm=shmem2,id=shmem2,bus=pci.0,addr=0x5 \
+-device 
ivshmem,size=512m,chardev=charshmem3,id=shmem3,bus=pci.0,addr=0x6 \

-chardev socket,id=charshmem3,path=/var/lib/libvirt/shmem-shmem3-sock \
--device ivshmem,size=1024m,chardev=charshmem4,bus=pci.0,addr=0x7 \
+-device 
ivshmem,size=1024m,chardev=charshmem4,id=shmem4,bus=pci.0,addr=0x7 \

-chardev socket,id=charshmem4,path=/tmp/shmem4-sock \
--device 
ivshmem,size=2048m,chardev=charshmem5,msi=on,ioeventfd=off,bus=pci.0,addr=0x8 
\
+-device 
ivshmem,size=2048m,chardev=charshmem5,id=shmem5,msi=on,ioeventfd=off,bus=pci.0,addr=0x8 
\

-chardev socket,id=charshmem5,path=/tmp/shmem5-sock \
--device 
ivshmem,size=4096m,chardev=charshmem6,msi=on,vectors=16,bus=pci.0,addr=0x9 
\
+-device 
ivshmem,size=4096m,chardev=charshmem6,id=shmem6,msi=on,vectors=16,bus=pci.0,addr=0x9 
\

-chardev socket,id=charshmem6,path=/tmp/shmem6-sock \
--device 
ivshmem,size=8192m,chardev=charshmem7,msi=on,vectors=32,ioeventfd=on,bus=pci.0,addr=0xa 
\
+-device 
ivshmem,size=8192m,chardev=charshmem7,id=shmem7,msi=on,vectors=32,ioeventfd=on,bus=pci.0,addr=0xa 
\


make syntax-check complains about these, so they need to be split.  No
problem, though, ACK with that changed.



Got it, and i will split them to two lines in next version.

Thanks a lot for your review.

Luyao


-chardev socket,id=charshmem7,path=/tmp/shmem7-sock
--
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/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

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
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/10] qemu: auto assign pci address for shared memory device

[lhuang]

On 07/03/2015 08:39 PM, Martin Kletzander wrote:

On Wed, Jun 17, 2015 at 11:56:12AM +0800, Luyao Huang wrote:

Shared memory device is base on PCI address, even we do not
pass the pci address to qemu, qemu will auto assign a pci
address for it.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
src/qemu/qemu_command.c| 11 +++
tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 16 
2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 89f775d..5ac43d8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2562,6 +2562,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
   flags)  0)
goto error;
}
+


Spurious change, ACK without that.



Thanks a lot for your review, i will remove it in next version.

Luyao


/* Further non-primary video cards which have to be qxl type */
for (i = 1; i  def-nvideos; i++) {
if (def-videos[i]-type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
@@ -2575,6 +2576,16 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
   flags)  0)
goto error;
}
+
+/* Shared Memory */
+for (i = 0; i  def-nshmems; i++) {
+if (def-shmems[i]-info.type != 
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)

+continue;
+
+if (virDomainPCIAddressReserveNextSlot(addrs,
+ def-shmems[i]-info, flags)  0)
+goto error;
+}
for (i = 0; i  def-ninputs; i++) {
/* Nada - none are PCI based (yet) */
}
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args 
b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args

index a3d3cc8..d37879a 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args
@@ -1,16 +1,16 @@
LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test 
QEMU_AUDIO_DRV=none \

/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults \
-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \
--device ivshmem,shm=shmem0 \
--device ivshmem,size=128m,shm=shmem1 \
--device ivshmem,size=256m,shm=shmem2 \
--device ivshmem,size=512m,chardev=charshmem3 \
+-device ivshmem,shm=shmem0,bus=pci.0,addr=0x3 \
+-device ivshmem,size=128m,shm=shmem1,bus=pci.0,addr=0x4 \
+-device ivshmem,size=256m,shm=shmem2,bus=pci.0,addr=0x5 \
+-device ivshmem,size=512m,chardev=charshmem3,bus=pci.0,addr=0x6 \
-chardev socket,id=charshmem3,path=/var/lib/libvirt/shmem-shmem3-sock \
--device ivshmem,size=1024m,chardev=charshmem4 \
+-device ivshmem,size=1024m,chardev=charshmem4,bus=pci.0,addr=0x7 \
-chardev socket,id=charshmem4,path=/tmp/shmem4-sock \
--device ivshmem,size=2048m,chardev=charshmem5,msi=on,ioeventfd=off \
+-device 
ivshmem,size=2048m,chardev=charshmem5,msi=on,ioeventfd=off,bus=pci.0,addr=0x8 
\

-chardev socket,id=charshmem5,path=/tmp/shmem5-sock \
--device ivshmem,size=4096m,chardev=charshmem6,msi=on,vectors=16 \
+-device 
ivshmem,size=4096m,chardev=charshmem6,msi=on,vectors=16,bus=pci.0,addr=0x9 
\

-chardev socket,id=charshmem6,path=/tmp/shmem6-sock \
--device 
ivshmem,size=8192m,chardev=charshmem7,msi=on,vectors=32,ioeventfd=on \
+-device 
ivshmem,size=8192m,chardev=charshmem7,msi=on,vectors=32,ioeventfd=on,bus=pci.0,addr=0xa 
\

-chardev socket,id=charshmem7,path=/tmp/shmem7-sock
--
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] virsh: report error if vcpu number exceed the guest maxvcpu number

[lhuang]

On 07/02/2015 06:28 PM, John Ferlan wrote:


On 07/02/2015 05:46 AM, Pavel Hrdina wrote:

...


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,


This modification is much better and correspond to the error messages while
setting the vcpu pinning.


I just pushed this now - it'd need a bz for a backport (ahem) since
1.2.17 was cut before the push...


commit 848ab685f74afae102e265108518095942ecb293
Author: Luyao Huang lhu...@redhat.com
Date:   Mon Jun 29 10:10:15 2015 +0800

 virsh: report error if vcpu number exceed the guest maxvcpu number


Okay, i will help to find a bz for this patch.

thanks a lot for your help and review, Pavel and John :)


John


Luyao


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.


ACK to the patch than John updated and proposed.



--
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] 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

Re: [libvirt] [PATCH] util:qemu: fix IOThreadinfo always get failed when get running vm status

[lhuang]

On 06/29/2015 04:38 PM, Peter Krempa wrote:

On Mon, Jun 29, 2015 at 16:22:54 +0800, Luyao Huang wrote:

When we use iothreadinfo to get iothread information, we will get
error like this:

  # virsh iothreadinfo rhel7.0 --live
  error: Unable to get domain IOThreads information
  error: Unable to encode message payload

This is because virProcessGetAffinity() return a bitmap which map_len
is too big to send via rpc:

  (gdb) p *map
  $7 = {max_bit = 262144, map_len = 4096, map = 0x7f9b6c2c0a20}

To fix this issue add a new parameter maxcpu to virProcessGetAffinity()
to let callers specify the maxcpu (also in most machine, no need loop
262144 times to check if cpu is set), if set maxcpu to zero,
virProcessGetAffinity() will use default value (262144 or 1024) to
create bitmap. This issue was introduced in commit 825df8c.

Signed-off-by: Luyao Huanglhu...@redhat.com
---
  src/qemu/qemu_driver.c | 4 ++--
  src/util/virprocess.c  | 7 ---
  src/util/virprocess.h  | 2 +-
  3 files changed, 7 insertions(+), 6 deletions(-)


I think a better fix will be to make virBitmapToData smarter when
formatting a bitmap by finding the last set bit rather than formatting
the full bitmap.

This will avoid re-introducing the maxcpu argument.

The diff for such change is following:

diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index 9abc807..7234f7e 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -498,9 +498,12 @@ virBitmapPtr virBitmapNewData(void *data, int len)
   */
  int virBitmapToData(virBitmapPtr bitmap, unsigned char **data, int *dataLen)
  {
-int len;
+ssize_t len;

-len = (bitmap-max_bit + CHAR_BIT - 1) / CHAR_BIT;
+if ((len = virBitmapLastSetBit(bitmap))  0)
+len = 1;
+else
+len = (len + CHAR_BIT - 1) / CHAR_BIT;

  if (VIR_ALLOC_N(*data, len)  0)
  return -1;


Okay, i agree with you, and also we need remove the unused parameter in 
qemuDomainHelperGetVcpus() and qemuDomainGetIOThreadsLive()


diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2b530c8..6aa4c90 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1418,13 +1418,9 @@ static int
 qemuDomainHelperGetVcpus(virDomainObjPtr vm, virVcpuInfoPtr info, int 
maxinfo,

  unsigned char *cpumaps, int maplen)
 {
-int hostcpus;
 size_t i, v;
 qemuDomainObjPrivatePtr priv = vm-privateData;

-if ((hostcpus = nodeGetCPUCount())  0)
-return -1;
-
 if (priv-vcpupids == NULL) {
 virReportError(VIR_ERR_OPERATION_INVALID,
%s, _(cpu affinity is not supported));
@@ -5573,7 +5569,6 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
 qemuMonitorIOThreadInfoPtr *iothreads = NULL;
 virDomainIOThreadInfoPtr *info_ret = NULL;
 int niothreads = 0;
-int hostcpus;
 size_t i;
 int ret = -1;

@@ -5606,9 +5601,6 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
 goto endjob;
 }

-if ((hostcpus = nodeGetCPUCount())  0)
-goto endjob;
-
 if (VIR_ALLOC_N(info_ret, niothreads)  0)
 goto endjob;



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] 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: 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] [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] [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] 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: 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] conf: fix forget restore ctxt-node when parse memory device success

[lhuang]

On 05/21/2015 03:46 PM, Peter Krempa wrote:

On Thu, May 21, 2015 at 13:08:12 +0800, Luyao Huang wrote:

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

When set a memory device in the xml, sysinfo in xml will be lost.
Because we forgot restore ctxt-node to the oldnode after parse memory
device, this will make the parse function after virDomainMemoryDefParseXML
cannot find a node they need when parse a full xml(virDomainDefParseXML).

The commit message is really hard to read.


Okay, I will pay more attention about this. Maybe use the exist similar 
commit message as an example will be a good choice.





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

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bfdc94e..7ddc1ea 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11828,6 +11828,7 @@ virDomainMemoryDefParseXML(xmlNodePtr memdevNode,
  if (virDomainDeviceInfoParseXML(memdevNode, NULL, def-info, flags)  0)
  goto error;
  
+ctxt-node = save;

  return def;

A test case is missing. The bugzilla link contains one so I'll add it.


Oh, I see, I hesitated about if need introduce a unit test when i fix 
this issue.

Seems this is a rule about which case need a new test, isn't it? :)


ACK to the code. I'll rewrite the commit message and add a test case.


Thanks a lot for your quick review and help.



Peter


Luyao

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

Re: [libvirt] [PATCH] conf: fix forget restore ctxt-node when parse memory device success

[lhuang]

On 05/21/2015 06:14 PM, Daniel P. Berrange wrote:

On Thu, May 21, 2015 at 05:52:01PM +0800, lhuang wrote:

On 05/21/2015 03:46 PM, Peter Krempa wrote:

On Thu, May 21, 2015 at 13:08:12 +0800, Luyao Huang wrote:

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

When set a memory device in the xml, sysinfo in xml will be lost.
Because we forgot restore ctxt-node to the oldnode after parse memory
device, this will make the parse function after virDomainMemoryDefParseXML
cannot find a node they need when parse a full xml(virDomainDefParseXML).

The commit message is really hard to read.

Okay, I will pay more attention about this. Maybe use the exist similar
commit message as an example will be a good choice.


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

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bfdc94e..7ddc1ea 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11828,6 +11828,7 @@ virDomainMemoryDefParseXML(xmlNodePtr memdevNode,
  if (virDomainDeviceInfoParseXML(memdevNode, NULL, def-info, flags)  0)
  goto error;
+ctxt-node = save;
  return def;

A test case is missing. The bugzilla link contains one so I'll add it.

Oh, I see, I hesitated about if need introduce a unit test when i fix this
issue.
Seems this is a rule about which case need a new test, isn't it? :)

Our rule is that any addition to the XML parser needs to have a test case
added  - especially true if the addition is fixing a parsing bug :-)


Oh, get it, i will record it to some place as i have a bad memory :) , 
thank you Daniel




Regards,
Daniel


Luyao

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

Re: [libvirt] [PATCH] virsh: fix a typos issue cause error not right

[lhuang]

On 05/21/2015 08:41 PM, John Ferlan wrote:


On 05/20/2015 09:32 PM, Luyao Huang wrote:

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


While correct, this would also be fixed by patches on list for more
common virsh error message processing currently by Andrea Bolognani, see:

http://www.redhat.com/archives/libvir-list/2015-May/msg00686.html

Let's see where that series goes first...


Oh, you are right, this issue will be fixed after Andrea's patch is pushed.

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] 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