[libvirt] command line chroot in a VM
Manao ahoana, Hello, Bonjour,
From the host, I would like to perform a batch operation on the guests.
For example:
# virsh list
1 001
2 002
3 003
All guests are KVM, Debian Lenny
I want to
for GUEST in $(virsh list | awk '{blah blah}')
do
? ${GUEST} apt-get update
? ${GUEST} apt-get upgrade
done
And then I just have to *read* and answer yes or no to the apt
questions.
The host is a Debian Lenny, but I can backport some packages if needed.
What would be your suggestions?
- libguestfs? (I see an unstable debian package existing)
- ...?
Misaotra, Thanks, Merci.
--
Architecte Informatique chez Blueline/Gulfsat:
Administration Systeme, Recherche Developpement
+261 34 56 000 19
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [virt-tools-list] command line chroot in a VM
On Wed, Sep 01, 2010 at 12:08:27PM +0300, Mihamina Rakotomandimby wrote:
Manao ahoana, Hello, Bonjour,
From the host, I would like to perform a batch operation on the guests.
For example:
# virsh list
1 001
2 002
3 003
All guests are KVM, Debian Lenny
I want to
for GUEST in $(virsh list | awk '{blah blah}')
do
? ${GUEST} apt-get update
? ${GUEST} apt-get upgrade
done
And then I just have to *read* and answer yes or no to the apt
questions.
The host is a Debian Lenny, but I can backport some packages if needed.
What would be your suggestions?
- libguestfs? (I see an unstable debian package existing)
- ...?
This is basically a job for Func[1] or a proper configuration management
tool like Puppet[2]
Daniel
[1] https://fedorahosted.org/func/
[2] http://www.puppetlabs.com/
--
|: Red Hat, Engineering, London-o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org-o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- 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
Re: [libvirt] [PATCH] esx: Fix generator for string return values
2010/8/30 Eric Blake ebl...@redhat.com:
On 08/29/2010 05:00 PM, Matthias Bolte wrote:
Distinguish between strings as parameters (const char *)
and strings as return values (char **).
Here, you mention char**,
if self.type == String and \
self.occurrence not in [OCCURRENCE__REQUIRED_LIST,
OCCURRENCE__OPTIONAL_LIST]:
- return const char *
+ if self.is_return_value:
+ return char *
But here, it is only char*. Which is it?
That's a bit tricky. In case Parameter.get_type_string() is called
from Parameter.generate_return() then
string += %s*%s)%s % (self.get_type_string(), self.name, end_of_line)
in line 104 adds the second *.
I attached v2 where this is simplified.
Matthias
From d2c9dcd37b0bf7effc165b92919af41ccf725a81 Mon Sep 17 00:00:00 2001
From: Matthias Bolte matthias.bo...@googlemail.com
Date: Sun, 29 Aug 2010 19:17:10 +0200
Subject: [PATCH] esx: Fix generator for string return values
Distinguish between strings as parameters (const char *)
and strings as return values (char **).
---
src/esx/esx_vi_generator.py | 28
src/esx/esx_vi_methods.c| 22 --
2 files changed, 32 insertions(+), 18 deletions(-)
diff --git a/src/esx/esx_vi_generator.py b/src/esx/esx_vi_generator.py
index 411fd80..4e91c4d 100755
--- a/src/esx/esx_vi_generator.py
+++ b/src/esx/esx_vi_generator.py
@@ -94,11 +94,11 @@ class Parameter:
def generate_return(self, offset = 0, end_of_line = ;):
if self.occurrence == OCCURRENCE__IGNORED:
-raise ValueError(invalid function parameteroccurrence value '%s' % self.occurrence)
+raise ValueError(invalid function parameter occurrence value '%s' % self.occurrence)
else:
string =
string += * offset
-string += %s*%s)%s % (self.get_type_string(), self.name, end_of_line)
+string += %s%s)%s % (self.get_type_string(True), self.name, end_of_line)
while len(string) 59:
string +=
@@ -124,15 +124,25 @@ class Parameter:
return ESX_VI__METHOD__PARAMETER__SERIALIZE(%s, %s)\n % (self.type, self.name)
-def get_type_string(self):
+def get_type_string(self, as_return_value = False):
+string =
+
if self.type == String and \
self.occurrence not in [OCCURRENCE__REQUIRED_LIST,
OCCURRENCE__OPTIONAL_LIST]:
-return const char *
+if as_return_value:
+string += char *
+else:
+string += const char *
elif self.is_enum():
-return esxVI_%s % self.type
+string += esxVI_%s % self.type
else:
-return esxVI_%s * % self.type
+string += esxVI_%s * % self.type
+
+if as_return_value:
+string += *
+
+return string
def get_occurrence_comment(self):
@@ -225,9 +235,11 @@ class Method:
source += ),\n
if self.returns is None:
-source +=void, None,\n
+source +=void, /* nothing */, None,\n
+elif self.returns.type == String:
+source +=String, Value, %s,\n % self.returns.get_occurrence_short_enum()
else:
-source +=%s, %s,\n % (self.returns.type, self.returns.get_occurrence_short_enum())
+source +=%s, /* nothing */, %s,\n % (self.returns.type, self.returns.get_occurrence_short_enum())
source += {\n
diff --git a/src/esx/esx_vi_methods.c b/src/esx/esx_vi_methods.c
index 56d2e58..cba0424 100644
--- a/src/esx/esx_vi_methods.c
+++ b/src/esx/esx_vi_methods.c
@@ -67,34 +67,34 @@
-#define ESX_VI__METHOD__DESERIALIZE_OUTPUT__None(_type) \
+#define ESX_VI__METHOD__DESERIALIZE_OUTPUT__None(_type, _suffix) \
/* nothing */
-#define ESX_VI__METHOD__DESERIALIZE_OUTPUT__RequiredItem(_type) \
-if (esxVI_##_type##_Deserialize(response-node, output) 0) {\
+#define ESX_VI__METHOD__DESERIALIZE_OUTPUT__RequiredItem(_type, _suffix) \
+if (esxVI_##_type##_Deserialize##_suffix(response-node, output) 0) { \
goto cleanup; \
}
-#define ESX_VI__METHOD__DESERIALIZE_OUTPUT__RequiredList(_type) \
+#define ESX_VI__METHOD__DESERIALIZE_OUTPUT__RequiredList(_type, _suffix) \
if (esxVI_##_type##_DeserializeList(response-node, output) 0) {\
goto cleanup; \
}
-#define ESX_VI__METHOD__DESERIALIZE_OUTPUT__OptionalItem(_type) \
+#define ESX_VI__METHOD__DESERIALIZE_OUTPUT__OptionalItem(_type, _suffix) \
if (response-node !=
[libvirt] Next release 0.8.4 proposed schedule
So it's already September and while back from vacations I didn't yet fully scan all the list for potential patches. Still I'm tempted to try to get a release by the 10 which mean we would enter the freeze this week-end. I guess it would be 0.8.4 since there is no huge feature or change commited since 0.8.3. Is there anything important shich should land before the next release and that I may have overlooked ? There is also a number of patches which were sent but not ACK'ed or pushed but since I didn't finished the full list, I don't know yet if they got reposted, I will try to finish the scan tomorrow. But if you have patches blocked missing review please raise them ! thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] esx: Rework datastore path parsing and handling
On 08/26/2010 04:37 PM, Matthias Bolte wrote:
Instead of splitting the path part of a datastore path into
directory and file name, keep this in one piece. An example:
[datastore] directory/file
was split into this before:
datastoreName = datastore
directoryName = directory
fileName = file
Now it's split into this:
datastoreName = datastore
directoryName = directory
directoryAndFileName = directory/file
Seems reasonable.
This simplifies code using esxUtil_ParseDatastorePath, because
directoryAndFileName is used more often than fileName. Also the
old approach expected the datastore path to reference a actual
s/ a / an /
@@ -52,8 +52,7 @@ typedef struct _esxVMX_Data esxVMX_Data;
struct _esxVMX_Data {
esxVI_Context *ctx;
-const char *datastoreName;
-const char *directoryName;
+char *datastorePathWithoutFileName;
I guess losing the const here is okay.
@@ -2612,8 +2608,9 @@ esxDomainDumpXML(virDomainPtr domain, int flags)
esxVI_ObjectContent_Free(virtualMachine);
VIR_FREE(datastoreName);
VIR_FREE(directoryName);
-VIR_FREE(fileName);
+VIR_FREE(directoryAndFileName);
VIR_FREE(url);
+VIR_FREE(data.datastorePathWithoutFileName);
VIR_FREE(vmx);
virDomainDefFree(def);
@@ -2640,8 +2637,7 @@ esxDomainXMLFromNative(virConnectPtr conn, const char
*nativeFormat,
}
data.ctx = priv-primary;
-data.datastoreName = ?;
-data.directoryName = ?;
+data.datastorePathWithoutFileName = (char *)[?] ?;
Are you missing a strdup() here? I'm worried that the
VIR_FREE(data.datastorePathWithoutFileName) in esxDomainDumpXML will now
try to free static storage.
diff --git a/src/esx/esx_storage_driver.c b/src/esx/esx_storage_driver.c
index af8876a..d2d8f22 100644
--- a/src/esx/esx_storage_driver.c
+++ b/src/esx/esx_storage_driver.c
@@ -611,10 +611,8 @@ esxStoragePoolListStorageVolumes(virStoragePoolPtr pool,
char **const names,
esxVI_HostDatastoreBrowserSearchResults *searchResultsList = NULL;
esxVI_HostDatastoreBrowserSearchResults *searchResults = NULL;
esxVI_FileInfo *fileInfo = NULL;
-char *datastoreName = NULL;
-char *directoryName = NULL;
-char *fileName = NULL;
-char *prefix = NULL;
+char *directoryAndFileName = NULL;
+int length;
s/int/size_t/
+++ b/src/esx/esx_vi.c
@@ -2946,7 +2946,9 @@ esxVI_LookupFileInfoByDatastorePath(esxVI_Context *ctx,
int result = -1;
char *datastoreName = NULL;
char *directoryName = NULL;
+char *directoryAndFileName = NULL;
char *fileName = NULL;
+int length;
s/int/size_t/
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] esx: Fix generator for string return values
On 09/01/2010 09:32 AM, Matthias Bolte wrote: That's a bit tricky. In case Parameter.get_type_string() is called from Parameter.generate_return() then string += %s*%s)%s % (self.get_type_string(), self.name, end_of_line) in line 104 adds the second *. I attached v2 where this is simplified. Yes, it's much easier to see in v2. ACK, if you trust my minimal python reviewing skills. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] buf: Fix possible infinite loop in EscapeString, VSnprintf
The current code will go into an infinite loop if the printf generated
string is = 1000, AND exactly 1 character smaller than the amount of free
space in the buffer. When this happens, we are dropped into the loop body,
but nothing will actually change, because count == (buf-size - buf-use - 1),
and virBufferGrow returns unchanged if count (buf-size - buf-use)
Fix this by removing the '- 1' bit from 'size'. The *nprintf functions handle
the NULL byte for us anyways, so we shouldn't need to manually accomodate
for it.
Here's a bug where we are actually hitting this issue:
https://bugzilla.redhat.com/show_bug.cgi?id=602772
Signed-off-by: Cole Robinson crobi...@redhat.com
---
src/util/buf.c | 14 ++
1 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/util/buf.c b/src/util/buf.c
index fc1217b..734b8f6 100644
--- a/src/util/buf.c
+++ b/src/util/buf.c
@@ -236,11 +236,11 @@ virBufferVSprintf(const virBufferPtr buf, const char
*format, ...)
virBufferGrow(buf, 100) 0)
return;
-size = buf-size - buf-use - 1;
+size = buf-size - buf-use;
va_start(argptr, format);
va_copy(locarg, argptr);
while (((count = vsnprintf(buf-content[buf-use], size, format,
- locarg)) 0) || (count = size - 1)) {
+ locarg)) 0) || (count = size)) {
buf-content[buf-use] = 0;
va_end(locarg);
@@ -250,13 +250,12 @@ virBufferVSprintf(const virBufferPtr buf, const char
*format, ...)
return;
}
-size = buf-size - buf-use - 1;
+size = buf-size - buf-use;
va_copy(locarg, argptr);
}
va_end(argptr);
va_end(locarg);
buf-use += count;
-buf-content[buf-use] = '\0';
}
/**
@@ -340,19 +339,18 @@ virBufferEscapeString(const virBufferPtr buf, const char
*format, const char *st
return;
}
-size = buf-size - buf-use - 1;
+size = buf-size - buf-use;
while (((count = snprintf(buf-content[buf-use], size, format,
- (char *)escaped)) 0) || (count = size - 1)) {
+ (char *)escaped)) 0) || (count = size)) {
buf-content[buf-use] = 0;
grow_size = (count 1000) ? count : 1000;
if (virBufferGrow(buf, grow_size) 0) {
VIR_FREE(escaped);
return;
}
-size = buf-size - buf-use - 1;
+size = buf-size - buf-use;
}
buf-use += count;
-buf-content[buf-use] = '\0';
VIR_FREE(escaped);
}
--
1.7.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/7] round 2 of sprintf cleanups
I don't think all snprintf uses are bad - those that format a number into an appropriately-sized maximum-possible array should be fine. For example, there are some fixed-size buffers whose size is determined by the kernel, where snprintf was the appropriate course of action in nwfilter_ebiptables_driver.c. But sprintf doesn't have quite the coverage as snprintf, so this continuation of *printf cleanups converts some of these. I still have more patches to go, but this has taken me long enough to get some more review rather than slamming a 20-patch series all at the end. More comments within individual patches. Eric Blake (7): build: add some modules vbox: factor a large function vbox: avoid problematic uses of sprintf network: use virAsprintf when appropriate lxc: avoid large stacks with veth creation openvz: formatting cleanups openvz: use virAsprintf to avoid large stacks bootstrap.conf |2 + src/conf/network_conf.c| 15 +- src/lxc/lxc_driver.c | 32 +- src/lxc/veth.c | 84 ++- src/lxc/veth.h |6 +- src/openvz/openvz_conf.c | 142 ++-- src/openvz/openvz_driver.c | 23 +- src/vbox/vbox_tmpl.c | 2108 +++- 8 files changed, 1269 insertions(+), 1143 deletions(-) -- 1.7.2.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/7] build: add some modules
snprintf is currently implicitly picked up by getaddrinfo, but we might as well make it explicit so that mingw doesn't break if getaddrinfo changes to drop the dependency. func doesn't matter for gcc compilation, but may help other compilers cope with our use of __func__. * bootstrap.conf (gnulib_modules): Add snprintf and func. --- Shouldn't impact a GNU/Linux build. bootstrap.conf |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index ca31a6e..8a85b91 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -29,6 +29,7 @@ count-one-bits crypto/md5 dirname-lgpl fcntl-h +func getaddrinfo gethostname getpass @@ -53,6 +54,7 @@ random_r sched send setsockopt +snprintf socket stpcpy strchrnul -- 1.7.2.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 6/7] openvz: formatting cleanups
* src/openvz/openvz_conf.c: Whitespace fixes.
* src/openvz/openvz_driver.c: Likewise.
---
Should just be formatting, no content change.
openvz has other problems, like its use of popen (totally unsafe);
so I'll be fixing it some more when I get to the virCommand patch
series.
src/openvz/openvz_conf.c | 70 ++--
src/openvz/openvz_driver.c | 23 +++---
2 files changed, 47 insertions(+), 46 deletions(-)
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index b52f4ac..356c7f0 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -64,7 +64,7 @@ strtoI(const char *str)
int val;
if (virStrToLong_i(str, NULL, 10, val) 0)
-return 0 ;
+return 0;
return val;
}
@@ -338,7 +338,7 @@ openvz_replace(const char* str,
from_len = strlen(from);
to_len = strlen(to);
-while((offset = strstr(str_start, from)))
+while ((offset = strstr(str_start, from)))
{
virBufferAdd(buf, str_start, offset-str_start);
virBufferAdd(buf, to, to_len);
@@ -447,7 +447,7 @@ int openvzLoadDomains(struct openvz_driver *driver) {
return -1;
}
-while(!feof(fp)) {
+while (!feof(fp)) {
if (fscanf(fp, %d %s\n, veid, status) != 2) {
if (feof(fp))
break;
@@ -556,7 +556,7 @@ openvzWriteConfigParam(const char * conf_file, const char
*param, const char *va
{
char * temp_file = NULL;
int fd = -1, temp_fd = -1;
-char line[PATH_MAX] ;
+char line[PATH_MAX];
if (virAsprintf(temp_file, %s.tmp, conf_file)0) {
virReportOOMError();
@@ -572,7 +572,7 @@ openvzWriteConfigParam(const char * conf_file, const char
*param, const char *va
goto error;
}
-while(1) {
+while (1) {
if (openvz_readline(fd, line, sizeof(line)) = 0)
break;
@@ -606,7 +606,7 @@ error:
close(fd);
if (temp_fd != -1)
close(temp_fd);
-if(temp_file)
+if (temp_file)
unlink(temp_file);
VIR_FREE(temp_file);
return -1;
@@ -626,9 +626,9 @@ openvzWriteVPSConfigParam(int vpsid, const char *param,
const char *value)
static int
openvzReadConfigParam(const char * conf_file ,const char * param, char *value,
int maxlen)
{
-char line[PATH_MAX] ;
+char line[PATH_MAX];
int ret, found = 0;
-int fd ;
+int fd;
char * sf, * token;
char *saveptr = NULL;
@@ -638,16 +638,16 @@ openvzReadConfigParam(const char * conf_file ,const char
* param, char *value, i
if (fd == -1)
return -1;
-while(1) {
+while (1) {
ret = openvz_readline(fd, line, sizeof(line));
-if(ret = 0)
+if (ret = 0)
break;
saveptr = NULL;
if (STREQLEN(line, param, strlen(param))) {
sf = line;
sf += strlen(param);
if (sf[0] == '=' sf[1] != '\0' ) {
-sf ++;
+sf++;
if ((token = strtok_r(sf,\\t\n, saveptr)) != NULL) {
if (virStrcpy(value, token, maxlen) == NULL) {
ret = -1;
@@ -663,7 +663,7 @@ openvzReadConfigParam(const char * conf_file ,const char *
param, char *value, i
if (ret == 0 found)
ret = 1;
-return ret ;
+return ret;
}
/*
@@ -676,7 +676,7 @@ openvzReadConfigParam(const char * conf_file ,const char *
param, char *value, i
int
openvzReadVPSConfigParam(int vpsid ,const char * param, char *value, int
maxlen)
{
-char conf_file[PATH_MAX] ;
+char conf_file[PATH_MAX];
if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, conf)0)
return -1;
@@ -700,7 +700,7 @@ openvz_copyfile(char* from_path, char* to_path)
return -1;
}
-while(1) {
+while (1) {
if (openvz_readline(fd, line, sizeof(line)) = 0)
break;
@@ -739,7 +739,8 @@ openvzCopyDefaultConfig(int vpsid)
char conf_file[PATH_MAX];
int ret = -1;
-if(openvzReadConfigParam(VZ_CONF_FILE, CONFIGFILE, configfile_value,
PATH_MAX) 0)
+if (openvzReadConfigParam(VZ_CONF_FILE, CONFIGFILE, configfile_value,
+ PATH_MAX) 0)
goto cleanup;
confdir = openvzLocateConfDir();
@@ -792,10 +793,10 @@ static char
const char *conf_dir_list[] = {/etc/vz/conf, /usr/local/etc/conf,
NULL};
int i=0;
-while(conf_dir_list[i]) {
-if(!access(conf_dir_list[i], F_OK))
+while (conf_dir_list[i]) {
+if (!access(conf_dir_list[i], F_OK))
return strdup(conf_dir_list[i]);
-i ++;
+i++;
}
return NULL;
@@ -808,14 +809,13 @@ openvz_readline(int fd, char *ptr, int maxlen)
int n, rc;
char c;
-for(n = 1; n maxlen; n ++) {
-if( (rc = read(fd, c, 1)) == 1) {
+for (n = 1; n maxlen; n++) {
+if ( (rc = read(fd, c, 1)) == 1) {
*ptr++ = c;
-if(c == '\n')
+if (c ==
[libvirt] [PATCH 3/7] vbox: avoid problematic uses of sprintf
* src/vbox/vbox_tmpl.c (vboxStartMachine, vboxAttachUSB): Use
virAsprintf instead.
---
This removes all use of sprintf in vbox. The first 3 use virAsprintf
(DISPLAY may be arbitrarily long, and while we are unlikely to hit
devices, it's better to be safe than to risk silent buffer
overflow); the remaining two are sized appropriately (actually, they
are sized too large, the real boundary size would be sizeof(int)*2+1
rather than 40); I felt better using snprintf rather than sprintf.
This doesn't address the fact that vbox doesn't really have very good
OOM handling (ie. it keeps on trying, although after the first OOM,
it will likely get another one); but that is an independent issue.
src/vbox/vbox_tmpl.c | 39 +++
1 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 3e8ff23..f50a12e 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -3161,7 +3161,6 @@ vboxStartMachine(virDomainPtr dom, int i, IMachine
*machine, vboxIID *iid)
PRUnichar *valueDisplayUtf16 = NULL;
char *valueDisplayUtf8 = NULL;
IProgress *progress = NULL;
-char displayutf8[32] = {0};
PRUnichar *env = NULL;
PRUnichar *sessionType = NULL;
nsresult rc;
@@ -3241,8 +3240,13 @@ vboxStartMachine(virDomainPtr dom, int i, IMachine
*machine, vboxIID *iid)
if (guiPresent) {
if (guiDisplay) {
-sprintf(displayutf8, DISPLAY=%.24s, guiDisplay);
-VBOX_UTF8_TO_UTF16(displayutf8, env);
+char *displayutf8;
+if (virAsprintf(displayutf8, DISPLAY=%s, guiDisplay) 0)
+virReportOOMError();
+else {
+VBOX_UTF8_TO_UTF16(displayutf8, env);
+VIR_FREE(displayutf8);
+}
VIR_FREE(guiDisplay);
}
@@ -3251,8 +3255,13 @@ vboxStartMachine(virDomainPtr dom, int i, IMachine
*machine, vboxIID *iid)
if (sdlPresent) {
if (sdlDisplay) {
-sprintf(displayutf8, DISPLAY=%.24s, sdlDisplay);
-VBOX_UTF8_TO_UTF16(displayutf8, env);
+char *displayutf8;
+if (virAsprintf(displayutf8, DISPLAY=%s, sdlDisplay) 0)
+virReportOOMError();
+else {
+VBOX_UTF8_TO_UTF16(displayutf8, env);
+VIR_FREE(displayutf8);
+}
VIR_FREE(sdlDisplay);
}
@@ -4457,15 +4466,19 @@ vboxAttachUSB(virDomainDefPtr def, vboxGlobalData
*data, IMachine *machine)
if (def-hostdevs[i]-source.subsys.type ==
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
-char filtername[11]= {0};
+char *filtername = NULL;
PRUnichar *filternameUtf16 = NULL;
IUSBDeviceFilter *filter = NULL;
-/* Assuming can't have more then devices so
- * restricting to %04d
+/* Zero pad for nice alignment when fewer than
+ * devices.
*/
-sprintf(filtername, filter%04d, i);
-VBOX_UTF8_TO_UTF16(filtername, filternameUtf16);
+if (virAsprintf(filtername, filter%04d, i) 0) {
+virReportOOMError();
+} else {
+VBOX_UTF8_TO_UTF16(filtername, filternameUtf16);
+VIR_FREE(filtername);
+}
USBController-vtbl-CreateDeviceFilter(USBController,
filternameUtf16,
@@ -4482,13 +4495,15 @@ vboxAttachUSB(virDomainDefPtr def, vboxGlobalData
*data, IMachine *machine)
char productId[40]= {0};
if (def-hostdevs[i]-source.subsys.u.usb.vendor) {
-sprintf(vendorId, %x,
def-hostdevs[i]-source.subsys.u.usb.vendor);
+snprintf(vendorId, sizeof(vendorId), %x,
+
def-hostdevs[i]-source.subsys.u.usb.vendor);
VBOX_UTF8_TO_UTF16(vendorId, vendorIdUtf16);
filter-vtbl-SetVendorId(filter,
vendorIdUtf16);
VBOX_UTF16_FREE(vendorIdUtf16);
}
if (def-hostdevs[i]-source.subsys.u.usb.product)
{
-sprintf(productId, %x,
def-hostdevs[i]-source.subsys.u.usb.product);
+snprintf(productId, sizeof(productId), %x,
+
def-hostdevs[i]-source.subsys.u.usb.product);
[libvirt] [PATCH 5/7] lxc: avoid large stacks with veth creation
* src/lxc/veth.h (vethCreate): Change prototype.
* src/lxc/veth.c (vethCreate): Always malloc veth2, and allocate
veth1 if needed.
(getFreeVethName): Adjust signature, and use virAsprintf.
* src/lxc/lxc_driver.c (lxcSetupInterfaces): Adjust caller.
---
This issue crossed file boundaries. It was a bit tricky, since
vethCreate was used in two patterns - one where the parent name was
already known, and another where the parent name is picked as the
first available option. But the end result avoids strdup'ing a
fixed-width buffer, and I think I correctly avoided any leaks (in
lxcSetupInterfaces, once a string is transferred to *veths, then
returning failure will cause that string to be freed in the caller).
It also gets rid of a PATH_MAX stack over-allocation.
src/lxc/lxc_driver.c | 32 ---
src/lxc/veth.c | 84 ++---
src/lxc/veth.h |6 ++--
3 files changed, 67 insertions(+), 55 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 6fe20b1..326fee6 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -812,14 +812,16 @@ static int lxcSetupInterfaces(virConnectPtr conn,
return -1;
for (i = 0 ; i def-nnets ; i++) {
-char parentVeth[PATH_MAX] = ;
-char containerVeth[PATH_MAX] = ;
+char *parentVeth;
+char *containerVeth = NULL;
switch (def-nets[i]-type) {
case VIR_DOMAIN_NET_TYPE_NETWORK:
{
-virNetworkPtr network = virNetworkLookupByName(conn,
-
def-nets[i]-data.network.name);
+virNetworkPtr network;
+
+network = virNetworkLookupByName(conn,
+ def-nets[i]-data.network.name);
if (!network) {
goto error_exit;
}
@@ -852,31 +854,23 @@ static int lxcSetupInterfaces(virConnectPtr conn,
}
DEBUG0(calling vethCreate());
-if (NULL != def-nets[i]-ifname) {
-strcpy(parentVeth, def-nets[i]-ifname);
-}
-DEBUG(parentVeth: %s, containerVeth: %s, parentVeth, containerVeth);
-if (vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX) 0)
+parentVeth = def-nets[i]-ifname;
+if (vethCreate(parentVeth, containerVeth) 0)
goto error_exit;
+DEBUG(parentVeth: %s, containerVeth: %s, parentVeth, containerVeth);
if (NULL == def-nets[i]-ifname) {
-def-nets[i]-ifname = strdup(parentVeth);
+def-nets[i]-ifname = parentVeth;
}
+
if (VIR_REALLOC_N(*veths, (*nveths)+1) 0) {
virReportOOMError();
+VIR_FREE(containerVeth);
goto error_exit;
}
-if (((*veths)[(*nveths)] = strdup(containerVeth)) == NULL) {
-virReportOOMError();
-goto error_exit;
-}
+(*veths)[(*nveths)] = containerVeth;
(*nveths)++;
-if (NULL == def-nets[i]-ifname) {
-virReportOOMError();
-goto error_exit;
-}
-
{
char macaddr[VIR_MAC_STRING_BUFLEN];
virFormatMacAddr(def-nets[i]-mac, macaddr);
diff --git a/src/lxc/veth.c b/src/lxc/veth.c
index 5f038d6..14cfaa2 100644
--- a/src/lxc/veth.c
+++ b/src/lxc/veth.c
@@ -13,6 +13,7 @@
#include config.h
#include string.h
+#include stdbool.h
#include stdio.h
#include sys/types.h
#include sys/wait.h
@@ -33,42 +34,46 @@
/* Functions */
/**
* getFreeVethName:
- * @veth: name for veth device (NULL to find first open)
- * @maxLen: max length of veth name
+ * @veth: pointer to store returned name for veth device
* @startDev: device number to start at (x in vethx)
*
* Looks in /sys/class/net/ to find the first available veth device
* name.
*
- * Returns 0 on success or -1 in case of error
+ * Returns non-negative device number on success or -1 in case of error
*/
-static int getFreeVethName(char *veth, int maxLen, int startDev)
+static int getFreeVethName(char **veth, int startDev)
{
-int rc = -1;
int devNum = startDev-1;
-char path[PATH_MAX];
+char *path = NULL;
do {
+VIR_FREE(path);
++devNum;
-snprintf(path, PATH_MAX, /sys/class/net/veth%d/, devNum);
+if (virAsprintf(path, /sys/class/net/veth%d/, devNum) 0) {
+virReportOOMError();
+return -1;
+}
} while (virFileExists(path));
+VIR_FREE(path);
-snprintf(veth, maxLen, veth%d, devNum);
-
-rc = devNum;
-
-return rc;
+if (virAsprintf(veth, veth%d, devNum) 0) {
+virReportOOMError();
+return -1;
+}
+return devNum;
}
/**
* vethCreate:
- * @veth1: name for one end of veth pair
- * @veth1MaxLen: max length of veth1 name
- * @veth2: name for one end of veth pair
- * @veth2MaxLen: max length of veth1 name
+ * @veth1: pointer to name for parent end of veth
[libvirt] [PATCH 4/7] network: use virAsprintf when appropriate
* src/conf/network_conf.c (virNetworkAllocateBridge): Avoid
limited buffer from snprintf.
---
Why print to a fixed-width buffer to then just strdup it later,
when we can print to a malloc'd buffer in the first place. Besides,
I couldn't easily guarantee if the buffer was large enough or if
it would contain arbitrary user input.
src/conf/network_conf.c | 15 ++-
1 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 347fc0b..4c0248c 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -891,17 +891,14 @@ char *virNetworkAllocateBridge(const virNetworkObjListPtr
nets,
template = virbr%d;
do {
-char try[50];
-
-snprintf(try, sizeof(try), template, id);
-
-if (!virNetworkBridgeInUse(nets, try, NULL)) {
-if (!(newname = strdup(try))) {
-virReportOOMError();
-return NULL;
-}
+if (virAsprintf(newname, template, id) 0) {
+virReportOOMError();
+return NULL;
+}
+if (!virNetworkBridgeInUse(nets, newname, NULL)) {
return newname;
}
+VIR_FREE(newname);
id++;
} while (id = MAX_BRIDGE_ID);
--
1.7.2.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 7/7] openvz: use virAsprintf to avoid large stacks
* src/openvz/openvz_conf.c (openvzLocateConfFile): Alter
signature.
(openvzGetVPSUUID, openvzSetDefinedUUID)
(openvzWriteVPSConfigParam, openvzReadVPSConfigParam)
(openvzCopyDefaultConfig): Adjust callers.
---
Nuke a few more PATH_MAX stack allocations.
src/openvz/openvz_conf.c | 78 -
1 files changed, 48 insertions(+), 30 deletions(-)
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index 356c7f0..f00f2f4 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -55,7 +55,7 @@
static char *openvzLocateConfDir(void);
static int openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len);
-static int openvzLocateConfFile(int vpsid, char *conffile, int maxlen, const
char *ext);
+static int openvzLocateConfFile(int vpsid, char **conffile, const char *ext);
static int openvzAssignUUIDs(void);
int
@@ -615,12 +615,15 @@ error:
int
openvzWriteVPSConfigParam(int vpsid, const char *param, const char *value)
{
-char conf_file[PATH_MAX];
+char *conf_file;
+int ret;
-if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, conf)0)
+if (openvzLocateConfFile(vpsid, conf_file, conf) 0)
return -1;
-return openvzWriteConfigParam(conf_file, param, value);
+ret = openvzWriteConfigParam(conf_file, param, value);
+VIR_FREE(conf_file);
+return ret;
}
static int
@@ -676,12 +679,15 @@ openvzReadConfigParam(const char * conf_file ,const char
* param, char *value, i
int
openvzReadVPSConfigParam(int vpsid ,const char * param, char *value, int
maxlen)
{
-char conf_file[PATH_MAX];
+char *conf_file;
+int ret;
-if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, conf)0)
+if (openvzLocateConfFile(vpsid, conf_file, conf) 0)
return -1;
-return openvzReadConfigParam(conf_file, param, value, maxlen);
+ret = openvzReadConfigParam(conf_file, param, value, maxlen);
+VIR_FREE(conf_file);
+return ret;
}
static int
@@ -736,7 +742,7 @@ openvzCopyDefaultConfig(int vpsid)
char * confdir = NULL;
char * default_conf_file = NULL;
char configfile_value[PATH_MAX];
-char conf_file[PATH_MAX];
+char *conf_file = NULL;
int ret = -1;
if (openvzReadConfigParam(VZ_CONF_FILE, CONFIGFILE, configfile_value,
@@ -747,12 +753,13 @@ openvzCopyDefaultConfig(int vpsid)
if (confdir == NULL)
goto cleanup;
-if (virAsprintf(default_conf_file, %s/ve-%s.conf-sample, confdir,
configfile_value) 0) {
+if (virAsprintf(default_conf_file, %s/ve-%s.conf-sample, confdir,
+configfile_value) 0) {
virReportOOMError();
goto cleanup;
}
-if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, conf)0)
+if (openvzLocateConfFile(vpsid, conf_file, conf) 0)
goto cleanup;
if (openvz_copyfile(default_conf_file, conf_file)0)
@@ -762,6 +769,7 @@ openvzCopyDefaultConfig(int vpsid)
cleanup:
VIR_FREE(confdir);
VIR_FREE(default_conf_file);
+VIR_FREE(conf_file);
return ret;
}
@@ -770,7 +778,7 @@ cleanup:
* 0 - OK
*/
static int
-openvzLocateConfFile(int vpsid, char *conffile, int maxlen, const char *ext)
+openvzLocateConfFile(int vpsid, char **conffile, const char *ext)
{
char * confdir;
int ret = 0;
@@ -779,9 +787,11 @@ openvzLocateConfFile(int vpsid, char *conffile, int
maxlen, const char *ext)
if (confdir == NULL)
return -1;
-if (snprintf(conffile, maxlen, %s/%d.%s,
- confdir, vpsid, ext ? ext : conf) = maxlen)
+if (virAsprintf(conffile, %s/%d.%s, confdir, vpsid,
+ext ? ext : conf) 0) {
+virReportOOMError();
ret = -1;
+}
VIR_FREE(confdir);
return ret;
@@ -830,27 +840,25 @@ openvz_readline(int fd, char *ptr, int maxlen)
static int
openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len)
{
-char conf_file[PATH_MAX];
+char *conf_file;
char line[1024];
char *saveptr = NULL;
char *uuidbuf;
char *iden;
int fd, ret;
-int retval = 0;
+int retval = -1;
-if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, conf)0)
+if (openvzLocateConfFile(vpsid, conf_file, conf) 0)
return -1;
fd = open(conf_file, O_RDONLY);
if (fd == -1)
-return -1;
+goto cleanup;
while (1) {
ret = openvz_readline(fd, line, sizeof(line));
-if (ret == -1) {
-close(fd);
-return -1;
-}
+if (ret == -1)
+goto cleanup;
if (ret == 0) { /* EoF, UUID was not found */
uuidstr[0] = 0;
@@ -861,12 +869,18 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len)
uuidbuf = strtok_r(NULL, \n, saveptr);
if (iden != NULL uuidbuf != NULL STREQ(iden, #UUID:)) {
-if (virStrcpy(uuidstr, uuidbuf, len) == NULL)
-retval = -1;
+if (virStrcpy(uuidstr, uuidbuf, len) == NULL) {
+
Re: [libvirt] [PATCH] buf: Fix possible infinite loop in EscapeString, VSnprintf
On 09/01/2010 01:43 PM, Cole Robinson wrote:
The current code will go into an infinite loop if the printf generated
string is= 1000, AND exactly 1 character smaller than the amount of free
space in the buffer. When this happens, we are dropped into the loop body,
but nothing will actually change, because count == (buf-size - buf-use - 1),
and virBufferGrow returns unchanged if count (buf-size - buf-use)
Fix this by removing the '- 1' bit from 'size'. The *nprintf functions handle
the NULL byte for us anyways, so we shouldn't need to manually accomodate
for it.
Per 'man vsnprintf',
The functions snprintf() and vsnprintf() do not write more than size
bytes (including the trailing '\0'). If the output was truncated due
to this limit then the return value is the number of characters (not
including the trailing '\0') which would have been written to the final
string if enough space had been available. Thus, a return value of
size or more means that the output was truncated. (See also below
under NOTES.)
So, if the result is == size, then we MUST re-alloc and try again, to
avoid truncation.
-size = buf-size - buf-use - 1;
+size = buf-size - buf-use;
va_start(argptr, format);
va_copy(locarg, argptr);
while (((count = vsnprintf(buf-content[buf-use], size, format,
This makes sense - we might as well tell vsnprintf exactly how many
bytes it has to play with.
- locarg)) 0) || (count= size - 1)) {
+ locarg)) 0) || (count= size)) {
However, I think this still has issues. vsnprintf returns -1 ONLY when
there is a non-recoverable error (EILSEQ, ENOMEM), and NOT when the
string was truncated. So if it returns -1 once, it will ALWAYS return
-1 no matter how much larger we make the buffer. (At least, since we
use the gnulib module, it will always obey POSIX. If it weren't for
gnulib, then yes, I could see how you could worry about older glibc that
didn't obey POSIX and returned -1 instead of the potential print size).
So why not write it as an if() instead of a while(), since we really
only need one truncated vsnprintf attempt before guaranteeing that the
buffer is large enough for the second attempt.
[Side note: I'm really starting to get annoyed by the Thunderbird bug
that corrupts spacing of quoted text that contains or . Sorry to
everyone else that has to put up with my review comments that have been
mangled.]
@@ -250,13 +250,12 @@ virBufferVSprintf(const virBufferPtr buf, const char
*format, ...)
return;
}
You're missing a step. It's okay that size is 1 larger, but you also
need to make the virBufferGrow(buf, grow_size) guarantee that it
allocates at least count+1 bytes, to avoid a needless second iteration
through the loop. Back to that comment of an if() being better than a
while() loop.
-size = buf-size - buf-use - 1;
+size = buf-size - buf-use;
va_copy(locarg, argptr);
}
va_end(argptr);
va_end(locarg);
buf-use += count;
-buf-content[buf-use] = '\0';
Agree with this part.
}
/**
@@ -340,19 +339,18 @@ virBufferEscapeString(const virBufferPtr buf, const char
*format, const char *st
return;
}
-size = buf-size - buf-use - 1;
+size = buf-size - buf-use;
while (((count = snprintf(buf-content[buf-use], size, format,
- (char *)escaped)) 0) || (count= size - 1)) {
+ (char *)escaped)) 0) || (count= size)) {
buf-content[buf-use] = 0;
grow_size = (count 1000) ? count : 1000;
if (virBufferGrow(buf, grow_size) 0) {
Same problem on while() vs. if(), and not allocating a large enough buffer.
NACK as written, but this is indeed a serious bug, so looking forward to
v2 (I can help you write it, if needed).
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/7] build: add some modules
2010/9/1 Eric Blake ebl...@redhat.com: snprintf is currently implicitly picked up by getaddrinfo, but we might as well make it explicit so that mingw doesn't break if getaddrinfo changes to drop the dependency. func doesn't matter for gcc compilation, but may help other compilers cope with our use of __func__. * bootstrap.conf (gnulib_modules): Add snprintf and func. --- Shouldn't impact a GNU/Linux build. ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/7] network: use virAsprintf when appropriate
2010/9/1 Eric Blake ebl...@redhat.com:
* src/conf/network_conf.c (virNetworkAllocateBridge): Avoid
limited buffer from snprintf.
---
Why print to a fixed-width buffer to then just strdup it later,
when we can print to a malloc'd buffer in the first place. Besides,
I couldn't easily guarantee if the buffer was large enough or if
it would contain arbitrary user input.
src/conf/network_conf.c | 15 ++-
1 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 347fc0b..4c0248c 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -891,17 +891,14 @@ char *virNetworkAllocateBridge(const
virNetworkObjListPtr nets,
template = virbr%d;
do {
- char try[50];
-
- snprintf(try, sizeof(try), template, id);
-
- if (!virNetworkBridgeInUse(nets, try, NULL)) {
- if (!(newname = strdup(try))) {
- virReportOOMError();
- return NULL;
- }
+ if (virAsprintf(newname, template, id) 0) {
+ virReportOOMError();
+ return NULL;
+ }
+ if (!virNetworkBridgeInUse(nets, newname, NULL)) {
return newname;
}
+ VIR_FREE(newname);
id++;
} while (id = MAX_BRIDGE_ID);
--
1.7.2.2
ACK.
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/7] openvz: formatting cleanups
2010/9/1 Eric Blake ebl...@redhat.com: * src/openvz/openvz_conf.c: Whitespace fixes. * src/openvz/openvz_driver.c: Likewise. --- Should just be formatting, no content change. ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] OpenVZ: add ethernet interface type support
Hi, Raising this patch to get a review for a potential inclusion in 0.8.4. Regards, Jean-Baptiste Hi all, This patch adds support for ethernet interface type to OpenVZ domains as stated in this previous message: http://www.redhat.com/archives/libvir- list/2010-July/msg00658.html Regards, Jean-Baptiste -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/7] openvz: use virAsprintf to avoid large stacks
2010/9/1 Eric Blake ebl...@redhat.com:
* src/openvz/openvz_conf.c (openvzLocateConfFile): Alter
signature.
(openvzGetVPSUUID, openvzSetDefinedUUID)
(openvzWriteVPSConfigParam, openvzReadVPSConfigParam)
(openvzCopyDefaultConfig): Adjust callers.
---
Nuke a few more PATH_MAX stack allocations.
src/openvz/openvz_conf.c | 78 -
1 files changed, 48 insertions(+), 30 deletions(-)
@@ -861,12 +869,18 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len)
uuidbuf = strtok_r(NULL, \n, saveptr);
if (iden != NULL uuidbuf != NULL STREQ(iden, #UUID:)) {
- if (virStrcpy(uuidstr, uuidbuf, len) == NULL)
- retval = -1;
+ if (virStrcpy(uuidstr, uuidbuf, len) == NULL) {
+ virReportOOMError();
+ goto cleanup;
+ }
virStrcpy cannot fail because of OOM, it doesn't do an allocation.
When it returns NULL, this means that one tried to copy too much data
to the given destination buffer. The typical error message in such a
case looks like this:
if (virStrcpy(sa_qemu.sun_path, unixfile,
sizeof(sa_qemu.sun_path)) == NULL) {
qemuReportError(VIR_ERR_INTERNAL_ERROR,
_(Unix socket '%s' too big for destination),
unixfile);
goto cleanup;
}
break;
}
}
- close(fd);
+ retval = 0;
+cleanup:
+ if (0 = fd)
if (fd = 0) reads nicer here.
+ close(fd);
+ VIR_FREE(conf_file);
return retval;
}
ACK, with these two comments addressed.
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] buf: Fix possible infinite loop in EscapeString, VSnprintf
The current code will go into an infinite loop if the printf generated
string is = 1000, AND exactly 1 character smaller than the amount of free
space in the buffer. When this happens, we are dropped into the loop body,
but nothing will actually change, because count == (buf-size - buf-use - 1),
and virBufferGrow returns unchanged if count (buf-size - buf-use)
Fix this by removing the '- 1' bit from 'size'. The *nprintf functions handle
the NULL byte for us anyways, so we shouldn't need to manually accomodate
for it.
Here's a bug where we are actually hitting this issue:
https://bugzilla.redhat.com/show_bug.cgi?id=602772
v2: Eric's improvements: while - if (), remove extra va_list variable,
make sure we report buffer error if snprintf fails
Signed-off-by: Cole Robinson crobi...@redhat.com
---
src/util/buf.c | 70 ---
1 files changed, 46 insertions(+), 24 deletions(-)
diff --git a/src/util/buf.c b/src/util/buf.c
index fc1217b..553e2a0 100644
--- a/src/util/buf.c
+++ b/src/util/buf.c
@@ -224,7 +224,7 @@ void
virBufferVSprintf(const virBufferPtr buf, const char *format, ...)
{
int size, count, grow_size;
-va_list locarg, argptr;
+va_list argptr;
if ((format == NULL) || (buf == NULL))
return;
@@ -236,27 +236,38 @@ virBufferVSprintf(const virBufferPtr buf, const char
*format, ...)
virBufferGrow(buf, 100) 0)
return;
-size = buf-size - buf-use - 1;
va_start(argptr, format);
-va_copy(locarg, argptr);
-while (((count = vsnprintf(buf-content[buf-use], size, format,
- locarg)) 0) || (count = size - 1)) {
+
+size = buf-size - buf-use;
+if ((count = vsnprintf(buf-content[buf-use],
+ size, format, argptr)) 0) {
+buf-error = 1;
+goto err;
+}
+
+/* Grow buffer if necessary and retry */
+if (count = size) {
buf-content[buf-use] = 0;
-va_end(locarg);
+va_end(argptr);
+va_start(argptr, format);
-grow_size = (count 1000) ? count : 1000;
+grow_size = (count + 1 1000) ? count + 1 : 1000;
if (virBufferGrow(buf, grow_size) 0) {
-va_end(argptr);
-return;
+goto err;
}
-size = buf-size - buf-use - 1;
-va_copy(locarg, argptr);
+size = buf-size - buf-use;
+if ((count = vsnprintf(buf-content[buf-use],
+ size, format, argptr)) 0) {
+buf-error = 1;
+goto err;
+}
}
-va_end(argptr);
-va_end(locarg);
buf-use += count;
-buf-content[buf-use] = '\0';
+
+err:
+va_end(argptr);
+return;
}
/**
@@ -336,23 +347,34 @@ virBufferEscapeString(const virBufferPtr buf, const char
*format, const char *st
if ((buf-use = buf-size)
virBufferGrow(buf, 100) 0) {
-VIR_FREE(escaped);
-return;
+goto err;
+}
+
+size = buf-size - buf-use;
+if ((count = snprintf(buf-content[buf-use], size,
+ format, (char *)escaped)) 0) {
+buf-error = 1;
+goto err;
}
-size = buf-size - buf-use - 1;
-while (((count = snprintf(buf-content[buf-use], size, format,
- (char *)escaped)) 0) || (count = size - 1)) {
+/* Grow buffer if necessary and retry */
+if (count = size) {
buf-content[buf-use] = 0;
-grow_size = (count 1000) ? count : 1000;
+grow_size = (count + 1 1000) ? count + 1 : 1000;
if (virBufferGrow(buf, grow_size) 0) {
-VIR_FREE(escaped);
-return;
+goto err;
+}
+size = buf-size - buf-use;
+
+if ((count = snprintf(buf-content[buf-use], size,
+ format, (char *)escaped)) 0) {
+buf-error = 1;
+goto err;
}
-size = buf-size - buf-use - 1;
}
buf-use += count;
-buf-content[buf-use] = '\0';
+
+err:
VIR_FREE(escaped);
}
--
1.7.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] buf: Fix possible infinite loop in EscapeString, VSnprintf
On 09/01/2010 04:38 PM, Eric Blake wrote:
On 09/01/2010 01:43 PM, Cole Robinson wrote:
The current code will go into an infinite loop if the printf generated
string is= 1000, AND exactly 1 character smaller than the amount of free
space in the buffer. When this happens, we are dropped into the loop body,
but nothing will actually change, because count == (buf-size - buf-use -
1),
and virBufferGrow returns unchanged if count (buf-size - buf-use)
Fix this by removing the '- 1' bit from 'size'. The *nprintf functions handle
the NULL byte for us anyways, so we shouldn't need to manually accomodate
for it.
Per 'man vsnprintf',
The functions snprintf() and vsnprintf() do not write more than size
bytes (including the trailing '\0'). If the output was truncated
due
to this limit then the return value is the number of characters
(not
including the trailing '\0') which would have been written to the
final
string if enough space had been available. Thus, a return value
of
size or more means that the output was truncated. (See also
below
under NOTES.)
So, if the result is == size, then we MUST re-alloc and try again, to
avoid truncation.
-size = buf-size - buf-use - 1;
+size = buf-size - buf-use;
va_start(argptr, format);
va_copy(locarg, argptr);
while (((count = vsnprintf(buf-content[buf-use], size, format,
This makes sense - we might as well tell vsnprintf exactly how many
bytes it has to play with.
- locarg)) 0) || (count= size - 1)) {
+ locarg)) 0) || (count= size)) {
However, I think this still has issues. vsnprintf returns -1 ONLY when
there is a non-recoverable error (EILSEQ, ENOMEM), and NOT when the
string was truncated. So if it returns -1 once, it will ALWAYS return
-1 no matter how much larger we make the buffer. (At least, since we
use the gnulib module, it will always obey POSIX. If it weren't for
gnulib, then yes, I could see how you could worry about older glibc that
didn't obey POSIX and returned -1 instead of the potential print size).
So why not write it as an if() instead of a while(), since we really
only need one truncated vsnprintf attempt before guaranteeing that the
buffer is large enough for the second attempt.
[Side note: I'm really starting to get annoyed by the Thunderbird bug
that corrupts spacing of quoted text that contains or . Sorry to
everyone else that has to put up with my review comments that have been
mangled.]
@@ -250,13 +250,12 @@ virBufferVSprintf(const virBufferPtr buf, const char
*format, ...)
return;
}
You're missing a step. It's okay that size is 1 larger, but you also
need to make the virBufferGrow(buf, grow_size) guarantee that it
allocates at least count+1 bytes, to avoid a needless second iteration
through the loop. Back to that comment of an if() being better than a
while() loop.
-size = buf-size - buf-use - 1;
+size = buf-size - buf-use;
va_copy(locarg, argptr);
}
va_end(argptr);
va_end(locarg);
buf-use += count;
-buf-content[buf-use] = '\0';
Agree with this part.
}
/**
@@ -340,19 +339,18 @@ virBufferEscapeString(const virBufferPtr buf, const
char *format, const char *st
return;
}
-size = buf-size - buf-use - 1;
+size = buf-size - buf-use;
while (((count = snprintf(buf-content[buf-use], size, format,
- (char *)escaped)) 0) || (count= size - 1))
{
+ (char *)escaped)) 0) || (count= size)) {
buf-content[buf-use] = 0;
grow_size = (count 1000) ? count : 1000;
if (virBufferGrow(buf, grow_size) 0) {
Same problem on while() vs. if(), and not allocating a large enough buffer.
NACK as written, but this is indeed a serious bug, so looking forward to
v2 (I can help you write it, if needed).
THanks for the review (online and offline). I've sent an updated patch
with your recommended improvements.
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] buf: Fix possible infinite loop in EscapeString, VSnprintf
On 09/01/2010 03:41 PM, Cole Robinson wrote: The current code will go into an infinite loop if the printf generated string is= 1000, AND exactly 1 character smaller than the amount of free space in the buffer. When this happens, we are dropped into the loop body, but nothing will actually change, because count == (buf-size - buf-use - 1), and virBufferGrow returns unchanged if count (buf-size - buf-use) Fix this by removing the '- 1' bit from 'size'. The *nprintf functions handle the NULL byte for us anyways, so we shouldn't need to manually accomodate s/accomodate/accommodate/ for it. Here's a bug where we are actually hitting this issue: https://bugzilla.redhat.com/show_bug.cgi?id=602772 v2: Eric's improvements: while - if (), remove extra va_list variable, make sure we report buffer error if snprintf fails ACK, with one spelling nit in the commit message. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] buf: Fix possible infinite loop in EscapeString, VSnprintf
On 09/01/2010 03:41 PM, Cole Robinson wrote:
+size = buf-size - buf-use;
+if ((count = vsnprintf(buf-content[buf-use],
+ size, format, argptr)) 0) {
+buf-error = 1;
+goto err;
+}
Hmm, thinking about this a bit more, most callers blindly assume that if
virBufferContentAndReset returns NULL, then they should report OOM. But
in this case, OOM is a bit misleading if the real cause is that the
format string was invalid and snprintf died with EINVAL (OOM implies
good code but insufficient runtime resources; while EINVAL implies a
programming error that should immediately be reported as a bug and fixed).
Don't make any changes to your patch (it's fine as is: it fixes a real
bug, and we are unlikely to hit the misleading OOM message since we
shouldn't be passing bad format strings in the first place). But I'm
wondering if a future patch should change buf-error to track the actual
errno value of the first failure encountered (ENOMEM for the common
case, but EILSEQ, EINVAL, EOVERFLOW for snprintf failures), and add a
function like virBufferReportError() which calls the appropriate error
message (virReportOOMError() vs. xxx(VIR_ERR_INTERNAL_ERROR, errno,
unexpected failure). However, this would mean an audit of all
existing virBuffer clients to call virBufferReportError() instead of
virReportOOMError() on failure. Or indeed, if _all_ callers already
(should) print errors, maybe it makes sense to inline the OOM error
printing into virBufferContentAndReset in the first place.
[I thought of this because the pending virCommand patch series has the
same issue - not all failures are ENOMEM related, and it would be nice
for virCommandRun to distinguish between the failures. But with the
current state of those patches, the contract is that virCommandRun
prints the error, rather than leaving it up to the caller.]
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/7] build: add some modules
On 09/01/2010 03:04 PM, Matthias Bolte wrote: 2010/9/1 Eric Blakeebl...@redhat.com: snprintf is currently implicitly picked up by getaddrinfo, but we might as well make it explicit so that mingw doesn't break if getaddrinfo changes to drop the dependency. func doesn't matter for gcc compilation, but may help other compilers cope with our use of __func__. * bootstrap.conf (gnulib_modules): Add snprintf and func. --- Shouldn't impact a GNU/Linux build. ACK. Thanks; pushed. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/7] openvz: use virAsprintf to avoid large stacks
On 09/01/2010 03:41 PM, Matthias Bolte wrote:
2010/9/1 Eric Blakeebl...@redhat.com:
* src/openvz/openvz_conf.c (openvzLocateConfFile): Alter
signature.
(openvzGetVPSUUID, openvzSetDefinedUUID)
(openvzWriteVPSConfigParam, openvzReadVPSConfigParam)
(openvzCopyDefaultConfig): Adjust callers.
---
Nuke a few more PATH_MAX stack allocations.
src/openvz/openvz_conf.c | 78 -
1 files changed, 48 insertions(+), 30 deletions(-)
@@ -861,12 +869,18 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len)
uuidbuf = strtok_r(NULL, \n,saveptr);
if (iden != NULL uuidbuf != NULL STREQ(iden, #UUID:)) {
-if (virStrcpy(uuidstr, uuidbuf, len) == NULL)
-retval = -1;
+if (virStrcpy(uuidstr, uuidbuf, len) == NULL) {
+virReportOOMError();
+goto cleanup;
+}
virStrcpy cannot fail because of OOM, it doesn't do an allocation.
/me slaps forehead. Of course not; but at the same time, silently
returning -1 is wrong.
qemuReportError(VIR_ERR_INTERNAL_ERROR,
_(Unix socket '%s' too big for destination),
unixfile);
Unrelated, but the example you copied from reads a bit awkwardly (maybe
Unix socket too small to contain '%s' would have sounded better?), but
that's a separate patch.
break;
}
}
-close(fd);
+retval = 0;
+cleanup:
+if (0= fd)
if (fd= 0) reads nicer here.
That's coreutils HACKING policy rearing it's head (where Jim always
prefers over ); I've adjusted accordingly.
ACK, with these two comments addressed.
Thanks for the review; I squashed in the diff below, then pushed.
That leaves 2, 3, and 5 of the series still awaiting review.
diff --git i/src/openvz/openvz_conf.c w/src/openvz/openvz_conf.c
index f00f2f4..ec11bbc 100644
--- i/src/openvz/openvz_conf.c
+++ w/src/openvz/openvz_conf.c
@@ -870,7 +870,8 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len)
if (iden != NULL uuidbuf != NULL STREQ(iden, #UUID:)) {
if (virStrcpy(uuidstr, uuidbuf, len) == NULL) {
-virReportOOMError();
+openvzError(VIR_ERR_INTERNAL_ERROR,
+_(invalid uuid %s), uuidbuf);
goto cleanup;
}
break;
@@ -878,7 +879,7 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len)
}
retval = 0;
cleanup:
-if (0 = fd)
+if (fd = 0)
close(fd);
VIR_FREE(conf_file);
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/7] network: use virAsprintf when appropriate
On 09/01/2010 03:20 PM, Matthias Bolte wrote: 2010/9/1 Eric Blakeebl...@redhat.com: * src/conf/network_conf.c (virNetworkAllocateBridge): Avoid limited buffer from snprintf. --- Why print to a fixed-width buffer to then just strdup it later, when we can print to a malloc'd buffer in the first place. Besides, I couldn't easily guarantee if the buffer was large enough or if it would contain arbitrary user input. ACK. Thanks; pushed. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/7] openvz: formatting cleanups
On 09/01/2010 03:24 PM, Matthias Bolte wrote: 2010/9/1 Eric Blakeebl...@redhat.com: * src/openvz/openvz_conf.c: Whitespace fixes. * src/openvz/openvz_driver.c: Likewise. --- Should just be formatting, no content change. ACK. Thanks; pushed. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
