[PATCH V3] Add dumpCore tag support to memory
dumpCore tag in the <memory> is not supported by libvirt-cim and it will be dropped during updating any element in the xml definition of a domain. This patch keep the tag all the time. Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 28 +++++++++++++++++++++++++++- libxkutil/device_parsing.h | 3 +++ libxkutil/xmlgen.c | 9 +++++++++ 3 files changed, 39 insertions(+), 1 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 7900e06..96db532 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -605,8 +605,17 @@ static int parse_mem_device(xmlNode *node, struct virt_device **vdevs) if (XSTREQ(node->name, "currentMemory")) sscanf(content, "%" PRIu64, &mdev->size); - else if (XSTREQ(node->name, "memory")) + else if (XSTREQ(node->name, "memory")) { sscanf(content, "%" PRIu64, &mdev->maxsize); + content = get_attr_value(node, "dumpCore"); + if (XSTREQ(content, "on")) { + mdev->dumpCore = MEM_DUMP_CORE_ON; + } else if (XSTREQ(content, "off")) { + mdev->dumpCore = MEM_DUMP_CORE_OFF; + } else { + mdev->dumpCore = MEM_DUMP_CORE_NOT_SET; + } + } free(content); @@ -968,6 +977,7 @@ static int _get_mem_device(const char *xml, struct virt_device **list) struct virt_device *mdevs = NULL; struct virt_device *mdev = NULL; int ret; + bool mem_dump_core_set = false; ret = parse_devices(xml, &mdevs, CIM_RES_TYPE_MEM); if (ret <= 0) @@ -987,10 +997,26 @@ static int _get_mem_device(const char *xml, struct virt_device **list) mdevs[1].dev.mem.size); mdev->dev.mem.maxsize = MAX(mdevs[0].dev.mem.maxsize, mdevs[1].dev.mem.maxsize); + /* libvirt dumpCore tag always belong to memory xml node, but + * here we may have two mdev for memory node and currentMemory + * node. So pick up one value. + */ + if (mdevs[0].dev.mem.dumpCore != MEM_DUMP_CORE_NOT_SET) { + mdev->dev.mem.dumpCore = mdevs[0].dev.mem.dumpCore; + mem_dump_core_set = true; + } else if (mdevs[1].dev.mem.dumpCore != + MEM_DUMP_CORE_NOT_SET) { + if (mem_dump_core_set) { + CU_DEBUG("WARN: libvirt set memory core dump in" + "two nodes!"); + } + mdev->dev.mem.dumpCore = mdevs[1].dev.mem.dumpCore; + } } else { mdev->dev.mem.size = MAX(mdevs[0].dev.mem.size, mdevs[0].dev.mem.maxsize); mdev->dev.mem.maxsize = mdev->dev.mem.size; + mdev->dev.mem.dumpCore = mdevs[0].dev.mem.dumpCore; } mdev->type = CIM_RES_TYPE_MEM; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 2b6d3d1..979b792 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -75,6 +75,9 @@ struct net_device { struct mem_device { uint64_t size; uint64_t maxsize; + enum { MEM_DUMP_CORE_NOT_SET, + MEM_DUMP_CORE_ON, + MEM_DUMP_CORE_OFF } dumpCore; }; struct vcpu_device { diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 4287d42..30e9a5e 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -498,6 +498,15 @@ static const char *mem_xml(xmlNodePtr root, struct domain *dominfo) BAD_CAST string); free(string); + + if (tmp == NULL) + return XML_ERROR; + if (mem->dumpCore == MEM_DUMP_CORE_ON) { + xmlNewProp(tmp, BAD_CAST "dumpCore", BAD_CAST "on"); + } else if (mem->dumpCore == MEM_DUMP_CORE_OFF) { + xmlNewProp(tmp, BAD_CAST "dumpCore", BAD_CAST "off"); + } + out: if (tmp == NULL) return XML_ERROR; -- 1.7.1
On 08/19/2013 11:11 AM, Xu Wang wrote:
dumpCore tag in the <memory> is not supported by libvirt-cim and it will be dropped during updating any element in the xml definition of a domain. This patch keep the tag all the time.
Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 28 +++++++++++++++++++++++++++- libxkutil/device_parsing.h | 3 +++ libxkutil/xmlgen.c | 9 +++++++++ 3 files changed, 39 insertions(+), 1 deletions(-)
Hi, I am (again) writing to you only, as I am confused. With this patch you are obsoleting part of Thilo's patch to add support for the dumpCore tag. However, your version of the patch is not allowing to set the dumpCore, since it is lacking the RASD extension. How do you want to proceed, do you want us to rebase Thilo's patch on top of your version, once it gets accepted upstream? If so, that's fine with me and I would post a review comment on the libvirt-cim mailing list. If you plan to submit a patch for RASD support of dumpCore on your, please let me know, in which case we can spare us the effort of rebasing Thilo's patch. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
于 2013-08-19 19:34, Viktor Mihajlovski 写道:
On 08/19/2013 11:11 AM, Xu Wang wrote:
dumpCore tag in the <memory> is not supported by libvirt-cim and it will be dropped during updating any element in the xml definition of a domain. This patch keep the tag all the time.
Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 28 +++++++++++++++++++++++++++- libxkutil/device_parsing.h | 3 +++ libxkutil/xmlgen.c | 9 +++++++++ 3 files changed, 39 insertions(+), 1 deletions(-)
Hi,
I am (again) writing to you only, as I am confused. With this patch you are obsoleting part of Thilo's patch to add support for the dumpCore tag. However, your version of the patch is not allowing to set the dumpCore, since it is lacking the RASD extension. How do you want to proceed, do you want us to rebase Thilo's patch on top of your version, once it gets accepted upstream? If so, that's fine with me and I would post a review comment on the libvirt-cim mailing list. If you plan to submit a patch for RASD support of dumpCore on your, please let me know, in which case we can spare us the effort of rebasing Thilo's patch.
Hi, Ralf makes me submit this patch into mail list. But it may conflicts with some others. If you want the Thilo's patch merged into upstream please submit it ASAP. Because Ralf want that property supported by libvirt-cim IN THIS WEEK. If you merged that patch, please drop this one. Thanks, Xu Wang
于 2013-8-20 9:56, Xu Wang 写道:
于 2013-08-19 19:34, Viktor Mihajlovski 写道:
On 08/19/2013 11:11 AM, Xu Wang wrote:
dumpCore tag in the <memory> is not supported by libvirt-cim and it will be dropped during updating any element in the xml definition of a domain. This patch keep the tag all the time.
Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 28 +++++++++++++++++++++++++++- libxkutil/device_parsing.h | 3 +++ libxkutil/xmlgen.c | 9 +++++++++ 3 files changed, 39 insertions(+), 1 deletions(-)
Hi,
I am (again) writing to you only, as I am confused. With this patch you are obsoleting part of Thilo's patch to add support for the dumpCore tag. However, your version of the patch is not allowing to set the dumpCore, since it is lacking the RASD extension. How do you want to proceed, do you want us to rebase Thilo's patch on top of your version, once it gets accepted upstream? If so, that's fine with me and I would post a review comment on the libvirt-cim mailing list. If you plan to submit a patch for RASD support of dumpCore on your, please let me know, in which case we can spare us the effort of rebasing Thilo's patch.
Hi, Ralf makes me submit this patch into mail list. But it may conflicts with some others. If you want the Thilo's patch merged into upstream please submit it ASAP. Because Ralf want that property supported by libvirt-cim IN THIS WEEK. If you merged that patch, please drop this one.
Thanks, Xu Wang
Hi, we can discuss this kind of issue in internal email box. Let's use public email list for patch discuss, and hear from other developer's opinion.
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia
dumpCore tag in the <memory> is not supported by libvirt-cim and it will be dropped during updating any element in the xml definition of a domain. This patch keep the tag all the time.
Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 28 +++++++++++++++++++++++++++- libxkutil/device_parsing.h | 3 +++ libxkutil/xmlgen.c | 9 +++++++++ 3 files changed, 39 insertions(+), 1 deletions(-)
diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 7900e06..96db532 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -605,8 +605,17 @@ static int parse_mem_device(xmlNode *node, struct virt_device **vdevs)
if (XSTREQ(node->name, "currentMemory")) sscanf(content, "%" PRIu64, &mdev->size); - else if (XSTREQ(node->name, "memory")) + else if (XSTREQ(node->name, "memory")) { sscanf(content, "%" PRIu64, &mdev->maxsize); + content = get_attr_value(node, "dumpCore"); + if (XSTREQ(content, "on")) { + mdev->dumpCore = MEM_DUMP_CORE_ON; + } else if (XSTREQ(content, "off")) { + mdev->dumpCore = MEM_DUMP_CORE_OFF; + } else { + mdev->dumpCore = MEM_DUMP_CORE_NOT_SET; + } + }
free(content);
@@ -968,6 +977,7 @@ static int _get_mem_device(const char *xml, struct virt_device **list) struct virt_device *mdevs = NULL; struct virt_device *mdev = NULL; int ret; + bool mem_dump_core_set = false;
ret = parse_devices(xml, &mdevs, CIM_RES_TYPE_MEM); if (ret <= 0) @@ -987,10 +997,26 @@ static int _get_mem_device(const char *xml, struct virt_device **list) mdevs[1].dev.mem.size); mdev->dev.mem.maxsize = MAX(mdevs[0].dev.mem.maxsize, mdevs[1].dev.mem.maxsize); + /* libvirt dumpCore tag always belong to memory xml node, but + * here we may have two mdev for memory node and currentMemory + * node. So pick up one value. + */ + if (mdevs[0].dev.mem.dumpCore != MEM_DUMP_CORE_NOT_SET) { + mdev->dev.mem.dumpCore = mdevs[0].dev.mem.dumpCore; + mem_dump_core_set = true; + } else if (mdevs[1].dev.mem.dumpCore != + MEM_DUMP_CORE_NOT_SET) { + if (mem_dump_core_set) { + CU_DEBUG("WARN: libvirt set memory core dump in" + "two nodes!"); + } + mdev->dev.mem.dumpCore = mdevs[1].dev.mem.dumpCore; + } } else { mdev->dev.mem.size = MAX(mdevs[0].dev.mem.size, mdevs[0].dev.mem.maxsize); mdev->dev.mem.maxsize = mdev->dev.mem.size; + mdev->dev.mem.dumpCore = mdevs[0].dev.mem.dumpCore; }
mdev->type = CIM_RES_TYPE_MEM; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 2b6d3d1..979b792 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -75,6 +75,9 @@ struct net_device { struct mem_device { uint64_t size; uint64_t maxsize; + enum { MEM_DUMP_CORE_NOT_SET, + MEM_DUMP_CORE_ON, + MEM_DUMP_CORE_OFF } dumpCore; };
struct vcpu_device { diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 4287d42..30e9a5e 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -498,6 +498,15 @@ static const char *mem_xml(xmlNodePtr root, struct domain *dominfo) BAD_CAST string);
free(string); + + if (tmp == NULL) + return XML_ERROR; No checking with tmp is a bugfix, so better to fix it in another
于 2013-8-19 17:11, Xu Wang 写道: patch. And also, better to add documents in commit message that: "This patch just add mapping between libvirt xml and struct mem_device, it make sure this tag will not be changed in cim call". About the code, no other issues found. Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
+ if (mem->dumpCore == MEM_DUMP_CORE_ON) { + xmlNewProp(tmp, BAD_CAST "dumpCore", BAD_CAST "on"); + } else if (mem->dumpCore == MEM_DUMP_CORE_OFF) { + xmlNewProp(tmp, BAD_CAST "dumpCore", BAD_CAST "off"); + } + out: if (tmp == NULL) return XML_ERROR;
-- Best Regards Wenchao Xia
diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 4287d42..30e9a5e 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -498,6 +498,15 @@ static const char *mem_xml(xmlNodePtr root, struct domain *dominfo) BAD_CAST string);
free(string); + + if (tmp == NULL) + return XML_ERROR; No checking with tmp is a bugfix, so better to fix it in another patch. actually, this *is* related to the patch changes, because before the
On 08/20/2013 03:56 AM, Wenchao Xia wrote: [...] patch, tmp == NULL was handled in the out: section, without this check we would run into a problem doing the xmlNewProp calls. So you should keep this as is. BTW: the result of xmlNewProp isn't checked, but then the libxkutil code doesn't seem to be consistent in this matter... -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
于 2013-8-20 14:44, Viktor Mihajlovski 写道:
diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 4287d42..30e9a5e 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -498,6 +498,15 @@ static const char *mem_xml(xmlNodePtr root, struct domain *dominfo) BAD_CAST string);
free(string); + + if (tmp == NULL) + return XML_ERROR; No checking with tmp is a bugfix, so better to fix it in another patch. actually, this *is* related to the patch changes, because before the
On 08/20/2013 03:56 AM, Wenchao Xia wrote: [...] patch, tmp == NULL was handled in the out: section, without this check we would run into a problem doing the xmlNewProp calls. So you should keep this as is. BTW: the result of xmlNewProp isn't checked, but then the libxkutil code doesn't seem to be consistent in this matter...
You are right, I missed there are following lines, this check make sure no new bug introduced. -- Best Regards Wenchao Xia
于 2013-8-20 19:06, Wenchao Xia 写道:
于 2013-8-20 14:44, Viktor Mihajlovski 写道:
diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 4287d42..30e9a5e 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -498,6 +498,15 @@ static const char *mem_xml(xmlNodePtr root, struct domain *dominfo) BAD_CAST string);
free(string); + + if (tmp == NULL) + return XML_ERROR; No checking with tmp is a bugfix, so better to fix it in another patch. actually, this *is* related to the patch changes, because before the
On 08/20/2013 03:56 AM, Wenchao Xia wrote: [...] patch, tmp == NULL was handled in the out: section, without this check we would run into a problem doing the xmlNewProp calls. So you should keep this as is. BTW: the result of xmlNewProp isn't checked, but then the libxkutil code doesn't seem to be consistent in this matter...
You are right, I missed there are following lines, this check make sure no new bug introduced.
Hi, John The code of this patch seems OK to me, do you have a time to review it? If you found no new issue, can you push this patch upstream? -- Best Regards Wenchao Xia
On 08/19/2013 05:11 AM, Xu Wang wrote:
dumpCore tag in the <memory> is not supported by libvirt-cim and it will be dropped during updating any element in the xml definition of a domain. This patch keep the tag all the time.
I had put off reviewing/testing because I wasn't sure what the outcome of the initial responses on the list were going to be... Anyway, couple of thoughts 1. Is there test for this? That is - is there a way to ensure the right things are done based on the setting of this? Is there a specific cimtest that could be added? How would one know this setting has taken ahold? 2. Did you run cimtest with this applied? In an environment without dumpCore? Mine failed rather spectacularly today with just this applied to top of the tree. In fact "ComputerSystem" and "01_enum.py" returns: ComputerSystem - 01_enum.py: FAIL ERROR - Provider does not report system `f18', but virsh does ERROR - Provider does not report system `if18lcl', but virsh does ERROR - Provider does not report system `if18net', but virsh does ERROR - Provider does not report system `if18nopool', but virsh does ERROR - Provider does not report system `rh64', but virsh does ERROR - Provider does not report system `rh70-alpha3', but virsh does CIM_ERR_FAILED: Lost connection with cimprovagt "libvirt-cim". I did a bit of debugging though and believe I know the cause (see below).
Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 28 +++++++++++++++++++++++++++- libxkutil/device_parsing.h | 3 +++ libxkutil/xmlgen.c | 9 +++++++++ 3 files changed, 39 insertions(+), 1 deletions(-)
diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index 7900e06..96db532 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -605,8 +605,17 @@ static int parse_mem_device(xmlNode *node, struct virt_device **vdevs)
if (XSTREQ(node->name, "currentMemory")) sscanf(content, "%" PRIu64, &mdev->size); - else if (XSTREQ(node->name, "memory")) + else if (XSTREQ(node->name, "memory")) { sscanf(content, "%" PRIu64, &mdev->maxsize); + content = get_attr_value(node, "dumpCore");
if (content == NULL), then this fails rather spectacularly I squashed the following and pushed (after running cimtest successfully and checking for coverity issues) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index fc4338b..542e4e9 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -609,9 +609,9 @@ static int parse_mem_device(xmlNode *node, struct virt_devic else if (XSTREQ(node->name, "memory")) { sscanf(content, "%" PRIu64, &mdev->maxsize); content = get_attr_value(node, "dumpCore"); - if (XSTREQ(content, "on")) { + if (content && XSTREQ(content, "on")) { mdev->dumpCore = MEM_DUMP_CORE_ON; - } else if (XSTREQ(content, "off")) { + } else if (content && XSTREQ(content, "off")) { mdev->dumpCore = MEM_DUMP_CORE_OFF; } else { mdev->dumpCore = MEM_DUMP_CORE_NOT_SET; John
+ if (XSTREQ(content, "on")) { + mdev->dumpCore = MEM_DUMP_CORE_ON; + } else if (XSTREQ(content, "off")) { + mdev->dumpCore = MEM_DUMP_CORE_OFF; + } else { + mdev->dumpCore = MEM_DUMP_CORE_NOT_SET; + } + }
free(content);
@@ -968,6 +977,7 @@ static int _get_mem_device(const char *xml, struct virt_device **list) struct virt_device *mdevs = NULL; struct virt_device *mdev = NULL; int ret; + bool mem_dump_core_set = false;
ret = parse_devices(xml, &mdevs, CIM_RES_TYPE_MEM); if (ret <= 0) @@ -987,10 +997,26 @@ static int _get_mem_device(const char *xml, struct virt_device **list) mdevs[1].dev.mem.size); mdev->dev.mem.maxsize = MAX(mdevs[0].dev.mem.maxsize, mdevs[1].dev.mem.maxsize); + /* libvirt dumpCore tag always belong to memory xml node, but + * here we may have two mdev for memory node and currentMemory + * node. So pick up one value. + */ + if (mdevs[0].dev.mem.dumpCore != MEM_DUMP_CORE_NOT_SET) { + mdev->dev.mem.dumpCore = mdevs[0].dev.mem.dumpCore; + mem_dump_core_set = true; + } else if (mdevs[1].dev.mem.dumpCore != + MEM_DUMP_CORE_NOT_SET) { + if (mem_dump_core_set) { + CU_DEBUG("WARN: libvirt set memory core dump in" + "two nodes!"); + } + mdev->dev.mem.dumpCore = mdevs[1].dev.mem.dumpCore; + } } else { mdev->dev.mem.size = MAX(mdevs[0].dev.mem.size, mdevs[0].dev.mem.maxsize); mdev->dev.mem.maxsize = mdev->dev.mem.size; + mdev->dev.mem.dumpCore = mdevs[0].dev.mem.dumpCore; }
mdev->type = CIM_RES_TYPE_MEM; diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h index 2b6d3d1..979b792 100644 --- a/libxkutil/device_parsing.h +++ b/libxkutil/device_parsing.h @@ -75,6 +75,9 @@ struct net_device { struct mem_device { uint64_t size; uint64_t maxsize; + enum { MEM_DUMP_CORE_NOT_SET, + MEM_DUMP_CORE_ON, + MEM_DUMP_CORE_OFF } dumpCore; };
struct vcpu_device { diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 4287d42..30e9a5e 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -498,6 +498,15 @@ static const char *mem_xml(xmlNodePtr root, struct domain *dominfo) BAD_CAST string);
free(string); + + if (tmp == NULL) + return XML_ERROR; + if (mem->dumpCore == MEM_DUMP_CORE_ON) { + xmlNewProp(tmp, BAD_CAST "dumpCore", BAD_CAST "on"); + } else if (mem->dumpCore == MEM_DUMP_CORE_OFF) { + xmlNewProp(tmp, BAD_CAST "dumpCore", BAD_CAST "off"); + } + out: if (tmp == NULL) return XML_ERROR;
participants (4)
-
John Ferlan -
Viktor Mihajlovski -
Wenchao Xia -
Xu Wang