[Libvir] [PATCH] increase storage domains of a definition file
Hi, When I install by virt-intall with long command (e.g."# /usr/sbin/virt-install --name testvm --ram 350 --vcpus 2 --file /root/test.img --file-size 1 --file /root/tmp02.img -- file-size 1 --file /root/tmp03.img --file-size 1 --file /root/tmp04.img --file-size 1 --file /root/tmp05.img --file-size 1 --file /root/tmp06.img --file-size 1 --file /root/tmp07.img --file-size 1 --file /root/tmp08.img --file-size 1 --file /root/tmp09.img --file-size 1 --file /root/tmp10.img --file-size 1 --file /root/tmp11.img --file-size 1 --file /root/tmp12.img --file-size 1 --file /root/tmp13.img --file-size 1 --file /root/tmp14.img --file-size 1 --file /root/tmp15.img --file-size 1 --file /root/tmp16.img --file-size 1 --vnc --paravirt --location ftp://xx.xx.xx.xx/rhel5ga_x86 --noautoconsole -- debug"), put out "abort". Because a definition file is long, it overflows from buffer. So, this patch increase buffer size. Signed-off-by: Shigeki Sakamoto <fj0588di@aa.jp.fujitsu.com> Thanks, Shigeki Sakamoto. Index: src/internal.h =================================================================== RCS file: /data/cvs/libvirt/src/internal.h,v retrieving revision 1.37 diff -u -p -r1.37 internal.h --- src/internal.h 4 Apr 2007 14:19:49 -0000 1.37 +++ src/internal.h 19 Apr 2007 11:01:12 -0000 @@ -106,6 +106,11 @@ extern "C" { #define VIR_CONNECT_RO 1 /** + * buffer size for definition file + */ +#define VIR_XML_STRING_BUFLEN (1024 + PATH_MAX * 16 + FILENAME_MAX * 16) + +/** * _virConnect: * * Internal structure associated to a connection Index: src/xend_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xend_internal.c,v retrieving revision 1.109 diff -u -p -r1.109 xend_internal.c --- src/xend_internal.c 13 Apr 2007 14:08:38 -0000 1.109 +++ src/xend_internal.c 19 Apr 2007 11:01:16 -0000 @@ -587,7 +587,7 @@ static int xend_op_ext2(virConnectPtr xend, const char *path, char *error, size_t n_error, const char *key, va_list ap) { - char ops[1024]; + char ops[VIR_XML_STRING_BUFLEN]; const char *k = key, *v; int offset = 0; Index: src/xml.c =================================================================== RCS file: /data/cvs/libvirt/src/xml.c,v retrieving revision 1.72 diff -u -p -r1.72 xml.c --- src/xml.c 13 Apr 2007 00:43:57 -0000 1.72 +++ src/xml.c 19 Apr 2007 11:01:20 -0000 @@ -1165,7 +1165,7 @@ virDomainParseXMLDesc(virConnectPtr conn { xmlDocPtr xml = NULL; xmlNodePtr node; - char *ret = NULL, *nam = NULL; + char *nam = NULL; virBuffer buf; xmlChar *prop; xmlParserCtxtPtr pctxt; @@ -1182,10 +1182,9 @@ virDomainParseXMLDesc(virConnectPtr conn if (name != NULL) *name = NULL; - ret = malloc(1000); - if (ret == NULL) + buf.content = malloc(1000); + if (buf.content == NULL) return (NULL); - buf.content = ret; buf.size = 1000; buf.use = 0; @@ -1376,7 +1375,7 @@ virDomainParseXMLDesc(virConnectPtr conn else free(nam); - return (ret); + return (buf.content); error: if (nam != NULL) @@ -1389,8 +1388,8 @@ virDomainParseXMLDesc(virConnectPtr conn xmlFreeDoc(xml); if (pctxt != NULL) xmlFreeParserCtxt(pctxt); - if (ret != NULL) - free(ret); + if (buf.content != NULL) + free(buf.content); return (NULL); }
On Fri, Apr 20, 2007 at 02:43:31PM +0900, S.Sakamoto wrote:
Hi,
When I install by virt-intall with long command [...] put out "abort". Because a definition file is long, it overflows from buffer.
So, this patch increase buffer size.
Oh right, the second bug is rerious, when we grow the buffer there is no garantee that realloc will provide the same base pointer, that was serious ! Applied and commited, thanks a lot ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Fri, Apr 20, 2007 at 02:43:31PM +0900, S.Sakamoto wrote:
Hi,
When I install by virt-intall with long command (e.g."# /usr/sbin/virt-install --name testvm --ram 350 --vcpus 2 --file /root/test.img --file-size 1 --file /root/tmp02.img -- file-size 1 --file /root/tmp03.img --file-size 1 --file /root/tmp04.img --file-size 1 --file /root/tmp05.img --file-size 1 --file /root/tmp06.img --file-size 1 --file /root/tmp07.img --file-size 1 --file /root/tmp08.img --file-size 1 --file /root/tmp09.img --file-size 1 --file /root/tmp10.img --file-size 1 --file /root/tmp11.img --file-size 1 --file /root/tmp12.img --file-size 1 --file /root/tmp13.img --file-size 1 --file /root/tmp14.img --file-size 1 --file /root/tmp15.img --file-size 1 --file /root/tmp16.img --file-size 1 --vnc --paravirt --location ftp://xx.xx.xx.xx/rhel5ga_x86 --noautoconsole -- debug"), put out "abort". Because a definition file is long, it overflows from buffer.
So, this patch increase buffer size.
Thanks, Shigeki Sakamoto.
Index: src/internal.h =================================================================== RCS file: /data/cvs/libvirt/src/internal.h,v retrieving revision 1.37 diff -u -p -r1.37 internal.h --- src/internal.h 4 Apr 2007 14:19:49 -0000 1.37 +++ src/internal.h 19 Apr 2007 11:01:12 -0000 @@ -106,6 +106,11 @@ extern "C" { #define VIR_CONNECT_RO 1
/** + * buffer size for definition file + */ +#define VIR_XML_STRING_BUFLEN (1024 + PATH_MAX * 16 + FILENAME_MAX * 16)
Considering we were only rarely overflowing the 1024 byte buffer, I think increasing the default buffer size to 132096 - is little bit of overkill. I'd rather see all buffers dynamically allocated and then this constant wouldn't be needed.
+ +/** * _virConnect: * * Internal structure associated to a connection Index: src/xend_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xend_internal.c,v retrieving revision 1.109 diff -u -p -r1.109 xend_internal.c --- src/xend_internal.c 13 Apr 2007 14:08:38 -0000 1.109 +++ src/xend_internal.c 19 Apr 2007 11:01:16 -0000 @@ -587,7 +587,7 @@ static int xend_op_ext2(virConnectPtr xend, const char *path, char *error, size_t n_error, const char *key, va_list ap) { - char ops[1024]; + char ops[VIR_XML_STRING_BUFLEN];
This gives a 130 KB static string. I can't help thinking this method would be better off using the dyn allocated virBuffer* routines instead of a static string & snprintf. NB, this patch doesn't modify the proxy code at all which has a 4096 byte field defined for this, on the wire. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Hi, Dan Thank you for comment. Shigeki does not repond this issue at this moment. I will comment on this instead. He should respond this issue quickly! In his survey, Many area is allocated as constant data(usually about 1KB size) in libvirt. He plan to fix this issue(dynamical allocation(which you suggested) as next stage (Fedora 8 or later.) As a Fedora 7 phase solution, He choses constant data size for solving this issue. I also think 130KB is too big. Thanks Atsushi SAKAI "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Fri, Apr 20, 2007 at 02:43:31PM +0900, S.Sakamoto wrote:
Hi,
When I install by virt-intall with long command (e.g."# /usr/sbin/virt-install --name testvm --ram 350 --vcpus 2 --file /root/test.img --file-size 1 --file /root/tmp02.img -- file-size 1 --file /root/tmp03.img --file-size 1 --file /root/tmp04.img --file-size 1 --file /root/tmp05.img --file-size 1 --file /root/tmp06.img --file-size 1 --file /root/tmp07.img --file-size 1 --file /root/tmp08.img --file-size 1 --file /root/tmp09.img --file-size 1 --file /root/tmp10.img --file-size 1 --file /root/tmp11.img --file-size 1 --file /root/tmp12.img --file-size 1 --file /root/tmp13.img --file-size 1 --file /root/tmp14.img --file-size 1 --file /root/tmp15.img --file-size 1 --file /root/tmp16.img --file-size 1 --vnc --paravirt --location ftp://xx.xx.xx.xx/rhel5ga_x86 --noautoconsole -- debug"), put out "abort". Because a definition file is long, it overflows from buffer.
So, this patch increase buffer size.
Thanks, Shigeki Sakamoto.
Index: src/internal.h =================================================================== RCS file: /data/cvs/libvirt/src/internal.h,v retrieving revision 1.37 diff -u -p -r1.37 internal.h --- src/internal.h 4 Apr 2007 14:19:49 -0000 1.37 +++ src/internal.h 19 Apr 2007 11:01:12 -0000 @@ -106,6 +106,11 @@ extern "C" { #define VIR_CONNECT_RO 1
/** + * buffer size for definition file + */ +#define VIR_XML_STRING_BUFLEN (1024 + PATH_MAX * 16 + FILENAME_MAX * 16)
Considering we were only rarely overflowing the 1024 byte buffer, I think increasing the default buffer size to 132096 - is little bit of overkill. I'd rather see all buffers dynamically allocated and then this constant wouldn't be needed.
+ +/** * _virConnect: * * Internal structure associated to a connection Index: src/xend_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xend_internal.c,v retrieving revision 1.109 diff -u -p -r1.109 xend_internal.c --- src/xend_internal.c 13 Apr 2007 14:08:38 -0000 1.109 +++ src/xend_internal.c 19 Apr 2007 11:01:16 -0000 @@ -587,7 +587,7 @@ static int xend_op_ext2(virConnectPtr xend, const char *path, char *error, size_t n_error, const char *key, va_list ap) { - char ops[1024]; + char ops[VIR_XML_STRING_BUFLEN];
This gives a 130 KB static string. I can't help thinking this method would be better off using the dyn allocated virBuffer* routines instead of a static string & snprintf.
NB, this patch doesn't modify the proxy code at all which has a 4096 byte field defined for this, on the wire.
Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Apr 24, 2007 at 08:36:01PM +0900, Atsushi SAKAI wrote:
Hi, Dan
Thank you for comment. Shigeki does not repond this issue at this moment. I will comment on this instead. He should respond this issue quickly!
In his survey, Many area is allocated as constant data(usually about 1KB size) in libvirt. He plan to fix this issue(dynamical allocation(which you suggested) as next stage (Fedora 8 or later.)
As a Fedora 7 phase solution, He choses constant data size for solving this issue. I also think 130KB is too big.
I just want to point out that 0.2.2 is the version released last week and which should end up in Fedora 7, but further release will be pushed as updates (as I submitted 0.2.2 for FC6 testing on Monday). Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Hi, Daniel Thank you for you information It would be helpful. Thanks Atsushi SAKAI Daniel Veillard <veillard@redhat.com> wrote:
On Tue, Apr 24, 2007 at 08:36:01PM +0900, Atsushi SAKAI wrote:
Hi, Dan
Thank you for comment. Shigeki does not repond this issue at this moment. I will comment on this instead. He should respond this issue quickly!
In his survey, Many area is allocated as constant data(usually about 1KB size) in libvirt. He plan to fix this issue(dynamical allocation(which you suggested) as next stage (Fedora 8 or later.)
As a Fedora 7 phase solution, He choses constant data size for solving this issue. I also think 130KB is too big.
I just want to point out that 0.2.2 is the version released last week and which should end up in Fedora 7, but further release will be pushed as updates (as I submitted 0.2.2 for FC6 testing on Monday).
Daniel
-- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Hi, Daniel
I can't help thinking this method would be better off using the dyn allocated virBuffer* routines instead of a static string & snprintf. Oops, sorry, virBuffer* routines had entirely slipped my mind. Buffer is allocated dynamically when I use this.
therefore, I make the patch which virBuffer* routines is in. Thanks, Shigeki Sakamoto. Index: src/internal.h =================================================================== RCS file: /data/cvs/libvirt/src/internal.h,v retrieving revision 1.38 diff -u -p -r1.38 internal.h --- src/internal.h 23 Apr 2007 07:41:23 -0000 1.38 +++ src/internal.h 24 Apr 2007 11:00:50 -0000 @@ -106,11 +106,6 @@ extern "C" { #define VIR_CONNECT_RO 1 /** - * buffer size for definition file - */ -#define VIR_XML_STRING_BUFLEN (1024 + PATH_MAX * 16 + FILENAME_MAX * 16) - -/** * _virConnect: * * Internal structure associated to a connection Index: src/xend_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xend_internal.c,v retrieving revision 1.110 diff -u -p -r1.110 xend_internal.c --- src/xend_internal.c 23 Apr 2007 07:41:23 -0000 1.110 +++ src/xend_internal.c 24 Apr 2007 11:00:55 -0000 @@ -587,24 +587,33 @@ static int xend_op_ext2(virConnectPtr xend, const char *path, char *error, size_t n_error, const char *key, va_list ap) { - char ops[VIR_XML_STRING_BUFLEN]; const char *k = key, *v; - int offset = 0; + virBuffer buf; + int ret; + + buf.content = malloc(1000); + if (buf.content == NULL) + return -1; + buf.size = 1000; + buf.use = 0; while (k) { v = va_arg(ap, const char *); - offset += snprintf(ops + offset, sizeof(ops) - offset, "%s", k); - offset += snprintf(ops + offset, sizeof(ops) - offset, "%s", "="); - offset += snprintf(ops + offset, sizeof(ops) - offset, "%s", v); + virBufferVSprintf(&buf, "%s", k); + virBufferVSprintf(&buf, "%s", "="); + virBufferVSprintf(&buf, "%s", v); k = va_arg(ap, const char *); if (k) - offset += snprintf(ops + offset, - sizeof(ops) - offset, "%s", "&"); + virBufferVSprintf(&buf, "%s", "&"); } - return http2unix(xend, xend_post(xend, path, ops, error, n_error)); + ret = http2unix(xend, xend_post(xend, path, buf.content, error, n_error)); + if (buf.content != NULL) + free(buf.content); + + return ret; }
On Tue, Apr 24, 2007 at 09:48:09PM +0900, S.Sakamoto wrote:
Hi, Daniel
I can't help thinking this method would be better off using the dyn allocated virBuffer* routines instead of a static string & snprintf. Oops, sorry, virBuffer* routines had entirely slipped my mind. Buffer is allocated dynamically when I use this.
therefore, I make the patch which virBuffer* routines is in.
Cool, way better :-) , and honnestly the buffer based code is more readable. Applied and commited,
+ buf.content = malloc(1000); + if (buf.content == NULL) + return -1;
Except here where I changed it to raise an error: if (buf.content == NULL) { virXendError(xend, VIR_ERR_NO_MEMORY, _("allocate new buffer")); return -1; } (did the same for urlencode a few lines down too) Thanks a lot ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Tue, Apr 24, 2007 at 09:48:09PM +0900, S.Sakamoto wrote:
diff -u -p -r1.110 xend_internal.c --- src/xend_internal.c 23 Apr 2007 07:41:23 -0000 1.110 +++ src/xend_internal.c 24 Apr 2007 11:00:55 -0000 @@ -587,24 +587,33 @@ static int xend_op_ext2(virConnectPtr xend, const char *path, char *error, size_t n_error, const char *key, va_list ap) { - char ops[VIR_XML_STRING_BUFLEN]; const char *k = key, *v; - int offset = 0; + virBuffer buf; + int ret; + + buf.content = malloc(1000); + if (buf.content == NULL) + return -1; + buf.size = 1000; + buf.use = 0;
Why not virBufferNew(1000) ? Is it really advantage that you save one extra malloc() for the buf pointer? I don't think so -- especially if you need to manually set internal virBuffer stuff.
while (k) { v = va_arg(ap, const char *);
- offset += snprintf(ops + offset, sizeof(ops) - offset, "%s", k); - offset += snprintf(ops + offset, sizeof(ops) - offset, "%s", "="); - offset += snprintf(ops + offset, sizeof(ops) - offset, "%s", v); + virBufferVSprintf(&buf, "%s", k); + virBufferVSprintf(&buf, "%s", "="); + virBufferVSprintf(&buf, "%s", v);
Please, use virBufferStrcat or virBufferAdd if you needn't a string formatting. It's cheaper and faster. virBufferStrcat(&buf, k, "=", v, NULL);
- offset += snprintf(ops + offset, - sizeof(ops) - offset, "%s", "&"); + virBufferVSprintf(&buf, "%s", "&");
virBufferAdd(&buf, "&", 1); Pedantic Karel :-) -- Karel Zak <kzak@redhat.com> Red Hat Czech s.r.o. Purkynova 99/71, 612 45 Brno, Czech Republic Reg.id: CZ27690016
On Mon, Apr 30, 2007 at 10:02:36AM +0200, Karel Zak wrote:
On Tue, Apr 24, 2007 at 09:48:09PM +0900, S.Sakamoto wrote:
diff -u -p -r1.110 xend_internal.c --- src/xend_internal.c 23 Apr 2007 07:41:23 -0000 1.110 +++ src/xend_internal.c 24 Apr 2007 11:00:55 -0000 @@ -587,24 +587,33 @@ static int xend_op_ext2(virConnectPtr xend, const char *path, char *error, size_t n_error, const char *key, va_list ap) { - char ops[VIR_XML_STRING_BUFLEN]; const char *k = key, *v; - int offset = 0; + virBuffer buf; + int ret; + + buf.content = malloc(1000); + if (buf.content == NULL) + return -1; + buf.size = 1000; + buf.use = 0;
Why not virBufferNew(1000) ?
Is it really advantage that you save one extra malloc() for the buf pointer? I don't think so -- especially if you need to manually set internal virBuffer stuff.
Hum, compared to Xend latency we probably should not care :-)
while (k) { v = va_arg(ap, const char *);
- offset += snprintf(ops + offset, sizeof(ops) - offset, "%s", k); - offset += snprintf(ops + offset, sizeof(ops) - offset, "%s", "="); - offset += snprintf(ops + offset, sizeof(ops) - offset, "%s", v); + virBufferVSprintf(&buf, "%s", k); + virBufferVSprintf(&buf, "%s", "="); + virBufferVSprintf(&buf, "%s", v);
Please, use virBufferStrcat or virBufferAdd if you needn't a string formatting. It's cheaper and faster.
virBufferStrcat(&buf, k, "=", v, NULL);
- offset += snprintf(ops + offset, - sizeof(ops) - offset, "%s", "&"); + virBufferVSprintf(&buf, "%s", "&");
virBufferAdd(&buf, "&", 1);
Pedantic Karel :-)
Care enough to send a patch from SVN head ;-) ? Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (5)
-
Atsushi SAKAI -
Daniel P. Berrange -
Daniel Veillard -
Karel Zak -
S.Sakamoto