Re: [libvirt] [PATCH 2/3] Managed save compression flags.
On Tue, Aug 17, 2010 at 03:58:22PM -0400, Chris Lalancette wrote: On 08/13/10 - 04:06:24PM, Daniel P. Berrange wrote: On Fri, Aug 13, 2010 at 10:53:48AM -0400, Chris Lalancette wrote: Add in the ability to specify compression flags from the managed save API. We map these to the supported QEMU compression flags internally. I'm not really convinced we need todo this. I think the host level setting in /etc/libvirt/qemu.conf is sufficient already. Don't get me wrong, I don't think this is a killer feature. But when I initially implemented compression, I definitely would have done it through the API if we had had a flags parameter available for virDomainSave(). Now that we have a ManagedSave that does have a flags parameter, I figured we do it the right way. I think it's a little cleaner, and more intuitive, to do it through the API, and it makes the feature available to hypervisors other than qemu. I still can't imagine anyone needing the ability to specify a different compression method per-guest VM. Perhaps a VIR_SAVE_COMPRESS to toggle compression on / off, with actual type of compression determined in the host config, but anything more seems rather overkill. Daniel -- |: 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 2/2] qemu: Update next usable PCI slot on domain reconnect
On Tue, Aug 17, 2010 at 11:37:34PM +0200, Jiri Denemark wrote: The hash iterator visits in an unpredictable order. It shouldn't matter because for this usage, all that is important is that 'nextslot' eventually ends up with the largest slot ID + 1. We don't want the first unused slot, rather the last slot reservation that was in place before the reconnect (often the first unused slot if you haven't done a lot of hot-unplugging, but not necessarily the case). The only way I can see to get at this is to revisit the reservations in the same order as they were reserved, rather than in the random order Actually, thinking about it you are right that setting nextslot to the largest slot used + 1 is not always correct. Even when no PCI devices are added/removed after a guest is started, nextslot doesn't have to look like that. E.g., if there is a device with explicit PCI address 0:0:31 in the XML. Although in normal configurations it would work. However, if we start thinking about hot(un)plugging PCI devices, even your suggestion of revisiting reservations in the original order wouldn't help much since the last device which influenced nextslot value might have been already unplugged from the guest. The only solution which really updates nextslot to exactly the same value it had before restarting libvirt is storing it in the runtime XML inside domstatus, from where it can be just read when the domain is being reconnected. Why do we need it to be exactly the same value ? nextslot is just an efficiency optimization isn't it. ie, so instead of starting from slot 0 and iterating over 'N' already used slots till we find a free slot, we can get the next free slot in 1 step. As such do we really need to worry about restoring it to the same value after restarting libvirtd. Worst case, the first PCI hotplug needs to do an O(N) loop over PCI devices to find an unused one, thereafter all the hotplug ops will be back to O(1) because nextslot will be set again. Daniel -- |: 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] how to print size_t in LGPLv2+ program
[removed bug-gnulib from CC] Eric Blake wrote: printf (% PRIuPTR, (uintptr_t) size_t_value); But still needs the use of need-formatstring-macros, and gettext 0.16.1 or newer (whereas libvirt is stuck on 0.14.1 at the moment). The uses of these formatstring macros require - at package preparation time: the AM_GNU_GETTEXT macro from gettext 0.16.1 or newer, - at build time and run time: a glibc newer than 2004-01-14 [1]. So, it requires gettext = 0.16.1 to be installed ONLY on the machines which are used to develop and release libvirt. Bruno [1] http://sourceware.org/git/?p=glibc.git;a=commit;h=083dc54a01d3b65b9d95599e4015256c9ee25342 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] how to print size_t in LGPLv2+ program
Hi Eric, My understanding is that on 64-bit windows, sizeof(long)==4 but sizeof(void*)==8; and ... sizeof(size_t) is also 8. Yes, correct. Which means you _can't_ use %lu,(unsigned long)size_t_val. You _can_ use this. It will work as long as each of your program's data structures is less than 4 GB large. Remember that size_t values get larger than 4 GB only if you have a memory object (array) that is larger than 4 GB. What a shame that POSIX omitted an inttypes.h PRIu* for size_t. You can define it by yourself: Basically you define #if @BITSIZEOF_SIZE_T@ == 32 # define PRIuSIZE PRIu32 #endif #if @BITSIZEOF_SIZE_T@ == 64 # define PRIuSIZE PRIu64 #endif This will work with mingw's and msvc's native printf, because gnulib's inttypes.h replacement defines PRIu64 to I64u, and the native printf supports %I64u directives. Note that this will not work inside gettext() arguments, though, because PRIuSIZE is not standard. For internationalized messages, you will need the workaround described in the second half of http://www.gnu.org/software/gettext/manual/html_node/Preparing-Strings.html. I hope one of these two alternatives works for you, so that we can avoid an LGPLv2+ cascade. Bruno -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] detecting uses of %zu
Eric Blake wrote: it still requires auditing code and forbidding %zu in favor of %PRIuSIZE (or whatever other name we settle on). You could hack GCC, clang, or even xgettext to produce a warning when it sees a 'z' size modifier in a format string. Bruno -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] gettext and need-formatstring-macros
[This question ought to have been sent to bug-gettext, not bug-gnulib.] Hi Eric, Use of PRIuMAX in a translated string produces a .pot file that refers to the special string PRIuMAX, and which requires the need-formatstring-macros option passed to AM_GNU_GETTEXT in configure.ac to be universally supported. If I'm reading gettext.git history correctly, need-formatstring-macros was added in gettext 0.11.4 (commit 8b45c5df), yes. but had bugs until 0.17 (commit 4e34b2ac); It had bugs until 0.16, fixed in versions = 0.16.1 (released on 2006-11-27). am I missing any details about how to properly use the gettext formatstring-macros option You invoke AM_GNU_GETTEXT([external], [need-formatstring-macros]) and, on glibc systems, use a glibc newer than 2002. which gettext versions are supported Versions 0.16.1, 0.17, 0.18.1.1 support need-formatstring-macros. Bruno -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] how to print size_t in LGPLv2+ program
Eric Blake ebl...@redhat.com writes: In a quick google search, I found this use of PRIdS (which is geared more for ssize_t, but the analog PRIuS would apply to size_t): http://bytes.com/topic/c/answers/506972-64-bit-portability-size_t-printf-format-strings However, S is awfully short; I'd prefer something a bit longer like PRIuSIZE. To my eye, PRIuSIZE follows the pattern set by PRIdPTR, PRIuLEASTN, etc. I like it. -- Ben Pfaff http://benpfaff.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] detecting uses of %zu
On Tue, Aug 17, 2010 at 6:41 PM, Bruno Haible wrote: Eric Blake wrote: it still requires auditing code and forbidding %zu in favor of %PRIuSIZE (or whatever other name we settle on). You could hack GCC, clang, or even xgettext to produce a warning when it sees a 'z' size modifier in a format string. or build it with a mingw toolchain and watch the warnings flood in ;) -mike -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] PRIuSIZE
Eric Blake wrote: Here's where a cross-project change to GNU Coding Standards could be helpful - if we all agree that gnulib should add the macro and gettext should add the support for it at the same time, then it would be much easier for all remaining GNU projects to take advantage of a standardized name Yes. And it will need either a POSIX change or a common GNU extension, because you will also need it implemented in glibc http://sourceware.org/git/?p=glibc.git;a=blob;f=intl/loadmsgcat.c. That's certainly a thing you could propose to the Austin group. Bruno -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] how to print size_t in LGPLv2+ program
What a shame that POSIX omitted an inttypes.h PRIu* for size_t. You can define it by yourself ... Or use uintptr_t instead of size_t. By the definition of these types, you can be sure that uintptr_t is at least as wide as size_t. Then use printf (% PRIuPTR, (uintptr_t) size_t_value); - Works fine with gnulib's inttypes.h. - Works fine with gettext. - Doesn't need gettext or glibc versions that don't exist yet. Bruno -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] build: avoid %zu in translated strings
On Tue, Aug 17, 2010 at 04:05:03PM -0600, Eric Blake wrote:
Mingw64 lacks %zu, and has the unfortunate setup where
sizeof(long)==4 but sizeof(size_t)==8. Since gnulib's
printf-posix module is not LGPLv2+, the best we can do
is manually cast to the only portable int type known to
hold size_t, and rely on gnulib's inttypes.h.
* src/remote/remote_driver.c (remoteStreamPacket): Rewrite size_t
formatting.
* src/storage/storage_driver.c (storageWipeExtent): Likewise.
---
Here's the latter option; there are many more uses of %zu, but
only in DEBUG statements. I suppose it would also be easy enough
to teach cfg.mk how to recognize and reject %z inside translated
strings, as part of 'make syntax-check'.
src/remote/remote_driver.c |7 +--
src/storage/storage_driver.c |7 ---
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index cb0d8e1..d9115c8 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -34,6 +34,7 @@
#include fcntl.h
#include arpa/inet.h
#include sys/wait.h
+#include inttypes.h
/* Windows socket compatibility functions. */
#include errno.h
@@ -8024,8 +8025,10 @@ remoteStreamPacket(virStreamPtr st,
if (status == REMOTE_CONTINUE) {
if (((4 + REMOTE_MESSAGE_MAX) - thiscall-bufferLength) nbytes) {
-remoteError(VIR_ERR_RPC, _(data size %zu too large for payload
%d),
-nbytes, ((4 + REMOTE_MESSAGE_MAX) -
thiscall-bufferLength));
+remoteError(VIR_ERR_RPC,
+_(data size % PRIuMAX too large for payload %d),
+(uintmax_t) nbytes,
+((4 + REMOTE_MESSAGE_MAX) - thiscall-bufferLength));
I find the PRI* stuff rather fugly. Can't we just use %llu and
cast to (unsigned long long)
The question of printf-posix license doesn't appear relevant since
remoteError friends all use asprintf() which is LGPLv2+ already.
Regards,
Daniel
--
|: 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
[libvirt] [PATCH] Allow chardev of type 'file' for UML domains.
From: Soren Hansen so...@linux2go.dk
Like that comment suggested, we just open the file and pass the file
descriptor to uml. The input stream is set to null, since I couldn't
find any useful way to actually use a file for input for a chardev and
this also mimics what e.g. QEmu does internally.
Signed-off-by: Soren Hansen so...@linux2go.dk
---
src/uml/uml_conf.c | 30 +-
src/uml/uml_conf.h |1 +
src/uml/uml_driver.c |6 +-
3 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
index bc8cbce..659f881 100644
--- a/src/uml/uml_conf.c
+++ b/src/uml/uml_conf.c
@@ -297,7 +297,8 @@ error:
static char *
umlBuildCommandLineChr(virDomainChrDefPtr def,
- const char *dev)
+ const char *dev,
+ fd_set *keepfd)
{
char *ret = NULL;
@@ -346,8 +347,26 @@ umlBuildCommandLineChr(virDomainChrDefPtr def,
break;
case VIR_DOMAIN_CHR_TYPE_FILE:
-case VIR_DOMAIN_CHR_TYPE_PIPE:
-/* XXX could open the file/pipe just pass the FDs */
+ {
+int fd_out;
+
+if ((fd_out = open(def-data.file.path,
+ O_WRONLY | O_APPEND | O_CREAT, 0660)) 0) {
+virReportSystemError(errno,
+ _(failed to open chardev file: %s),
+ def-data.file.path);
+return NULL;
+}
+if (virAsprintf(ret, %s%d=null,fd:%d, dev, def-target.port,
fd_out) 0) {
+virReportOOMError();
+close(fd_out);
+return NULL;
+}
+FD_SET(fd_out, keepfd);
+}
+break;
+ case VIR_DOMAIN_CHR_TYPE_PIPE:
+/* XXX could open the pipe just pass the FDs */
case VIR_DOMAIN_CHR_TYPE_VC:
case VIR_DOMAIN_CHR_TYPE_UDP:
@@ -393,6 +412,7 @@ static char *umlNextArg(char *args)
int umlBuildCommandLine(virConnectPtr conn,
struct uml_driver *driver ATTRIBUTE_UNUSED,
virDomainObjPtr vm,
+fd_set *keepfd,
const char ***retargv,
const char ***retenv)
{
@@ -515,7 +535,7 @@ int umlBuildCommandLine(virConnectPtr conn,
for (i = 0 ; i UML_MAX_CHAR_DEVICE ; i++) {
char *ret;
if (i == 0 vm-def-console)
-ret = umlBuildCommandLineChr(vm-def-console, con);
+ret = umlBuildCommandLineChr(vm-def-console, con, keepfd);
else
if (virAsprintf(ret, con%d=none, i) 0)
goto no_memory;
@@ -529,7 +549,7 @@ int umlBuildCommandLine(virConnectPtr conn,
if (vm-def-serials[j]-target.port == i)
chr = vm-def-serials[j];
if (chr)
-ret = umlBuildCommandLineChr(chr, ssl);
+ret = umlBuildCommandLineChr(chr, ssl, keepfd);
else
if (virAsprintf(ret, ssl%d=none, i) 0)
goto no_memory;
diff --git a/src/uml/uml_conf.h b/src/uml/uml_conf.h
index b33acc8..d8b2349 100644
--- a/src/uml/uml_conf.h
+++ b/src/uml/uml_conf.h
@@ -71,6 +71,7 @@ virCapsPtr umlCapsInit (void);
int umlBuildCommandLine (virConnectPtr conn,
struct uml_driver *driver,
virDomainObjPtr dom,
+ fd_set *keepfd,
const char ***retargv,
const char ***retenv);
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 04493ba..5f73ce2 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -737,10 +737,6 @@ static int umlMonitorCommand(const struct uml_driver
*driver,
virReportSystemError(errno, _(cannot read reply %s), cmd);
goto error;
}
-if (nbytes sizeof res) {
-virReportSystemError(0, _(incomplete reply %s), cmd);
-goto error;
-}
if (sizeof res.data res.length) {
virReportSystemError(0, _(invalid length in reply %s), cmd);
goto error;
@@ -871,7 +867,7 @@ static int umlStartVMDaemon(virConnectPtr conn,
return -1;
}
-if (umlBuildCommandLine(conn, driver, vm,
+if (umlBuildCommandLine(conn, driver, vm, keepfd,
argv, progenv) 0) {
close(logfd);
umlCleanupTapDevices(conn, vm);
--
1.7.0.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Add new API for accessing remote guest text console
On 17-08-2010 19:02, Daniel P. Berrange wrote: The 'virsh console' command has been an oddity that only works when run locally, as the same UID as the QEMU instance. This is because it directly opens /dev/pty/XXX. This introduces a formal API for accessing consoles that uses the virStreamPtr APIs. Now any app can open consoles anywhere it can connect to libvirt I don't (right now, at least) have any comments on the patches themselves, but I can't help but wonder what other wonderful improvements you've got in your pipeline. I spent at least a couple of hours on something like this a couple of weeks ago, but had I known that you were already doing it, I wouldn't have wasted my time. So, in an effort to not duplicate efforts, perhaps everyone who's working on something reasonably big (certainly stuff like this, but also smaller change sets) could put a list up somewhere for all to see? Perhaps such a list already exists and I just don't know about it? -- Soren Hansen Ubuntu Developer http://www.ubuntu.com/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Update next usable PCI slot on domain reconnect
Why do we need it to be exactly the same value ? nextslot is just an efficiency optimization isn't it. ie, so instead of starting from slot 0 and iterating over 'N' already used slots till we find a free slot, we can get the next free slot in 1 step. As such do we really need to worry about restoring it to the same value after restarting libvirtd. That was my understanding too. But Eric was concerned (in an older thread) about hotplugging PCI devices in a nonmonotonic way. He thinks it could upset Windows guests. Of course, if nextslot ever wraps from QEMU_PCI_ADDRESS_LAST_SLOT back to zero, such guests would be doomed anyway so we are only a bit nicer to them. I don't know if this is a real issue or not since I haven't met a Windows guest which I'd like to hotplug a PCI device in. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Update next usable PCI slot on domain reconnect
On Wed, Aug 18, 2010 at 01:15:55PM +0200, Jiri Denemark wrote: Why do we need it to be exactly the same value ? nextslot is just an efficiency optimization isn't it. ie, so instead of starting from slot 0 and iterating over 'N' already used slots till we find a free slot, we can get the next free slot in 1 step. As such do we really need to worry about restoring it to the same value after restarting libvirtd. That was my understanding too. But Eric was concerned (in an older thread) about hotplugging PCI devices in a nonmonotonic way. He thinks it could upset Windows guests. Of course, if nextslot ever wraps from QEMU_PCI_ADDRESS_LAST_SLOT back to zero, such guests would be doomed anyway so we are only a bit nicer to them. I don't know if this is a real issue or not since I haven't met a Windows guest which I'd like to hotplug a PCI device in. There's no requirement to plug devices in ascending slot order - we can have gaps at will with any ordering. Daniel -- |: 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] Allow chardev of type 'file' for UML domains.
On 18-08-2010 12:45, so...@linux2go.dk wrote:
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 04493ba..5f73ce2 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -737,10 +737,6 @@ static int umlMonitorCommand(const struct uml_driver
*driver,
virReportSystemError(errno, _(cannot read reply %s), cmd);
goto error;
}
-if (nbytes sizeof res) {
-virReportSystemError(0, _(incomplete reply %s), cmd);
-goto error;
-}
if (sizeof res.data res.length) {
virReportSystemError(0, _(invalid length in reply %s), cmd);
goto error;
Whoops, this bit wasn't meant to be included. /me reposts without it.
--
Soren Hansen
Ubuntu Developer
http://www.ubuntu.com/
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Allow chardev of type 'file' for UML domains.
From: Soren Hansen so...@linux2go.dk
Like that comment suggested, we just open the file and pass the file
descriptor to uml. The input stream is set to null, since I couldn't
find any useful way to actually use a file for input for a chardev and
this also mimics what e.g. QEmu does internally.
Signed-off-by: Soren Hansen so...@linux2go.dk
---
src/uml/uml_conf.c | 30 +-
src/uml/uml_conf.h |1 +
src/uml/uml_driver.c |2 +-
3 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
index bc8cbce..659f881 100644
--- a/src/uml/uml_conf.c
+++ b/src/uml/uml_conf.c
@@ -297,7 +297,8 @@ error:
static char *
umlBuildCommandLineChr(virDomainChrDefPtr def,
- const char *dev)
+ const char *dev,
+ fd_set *keepfd)
{
char *ret = NULL;
@@ -346,8 +347,26 @@ umlBuildCommandLineChr(virDomainChrDefPtr def,
break;
case VIR_DOMAIN_CHR_TYPE_FILE:
-case VIR_DOMAIN_CHR_TYPE_PIPE:
-/* XXX could open the file/pipe just pass the FDs */
+ {
+int fd_out;
+
+if ((fd_out = open(def-data.file.path,
+ O_WRONLY | O_APPEND | O_CREAT, 0660)) 0) {
+virReportSystemError(errno,
+ _(failed to open chardev file: %s),
+ def-data.file.path);
+return NULL;
+}
+if (virAsprintf(ret, %s%d=null,fd:%d, dev, def-target.port,
fd_out) 0) {
+virReportOOMError();
+close(fd_out);
+return NULL;
+}
+FD_SET(fd_out, keepfd);
+}
+break;
+ case VIR_DOMAIN_CHR_TYPE_PIPE:
+/* XXX could open the pipe just pass the FDs */
case VIR_DOMAIN_CHR_TYPE_VC:
case VIR_DOMAIN_CHR_TYPE_UDP:
@@ -393,6 +412,7 @@ static char *umlNextArg(char *args)
int umlBuildCommandLine(virConnectPtr conn,
struct uml_driver *driver ATTRIBUTE_UNUSED,
virDomainObjPtr vm,
+fd_set *keepfd,
const char ***retargv,
const char ***retenv)
{
@@ -515,7 +535,7 @@ int umlBuildCommandLine(virConnectPtr conn,
for (i = 0 ; i UML_MAX_CHAR_DEVICE ; i++) {
char *ret;
if (i == 0 vm-def-console)
-ret = umlBuildCommandLineChr(vm-def-console, con);
+ret = umlBuildCommandLineChr(vm-def-console, con, keepfd);
else
if (virAsprintf(ret, con%d=none, i) 0)
goto no_memory;
@@ -529,7 +549,7 @@ int umlBuildCommandLine(virConnectPtr conn,
if (vm-def-serials[j]-target.port == i)
chr = vm-def-serials[j];
if (chr)
-ret = umlBuildCommandLineChr(chr, ssl);
+ret = umlBuildCommandLineChr(chr, ssl, keepfd);
else
if (virAsprintf(ret, ssl%d=none, i) 0)
goto no_memory;
diff --git a/src/uml/uml_conf.h b/src/uml/uml_conf.h
index b33acc8..d8b2349 100644
--- a/src/uml/uml_conf.h
+++ b/src/uml/uml_conf.h
@@ -71,6 +71,7 @@ virCapsPtr umlCapsInit (void);
int umlBuildCommandLine (virConnectPtr conn,
struct uml_driver *driver,
virDomainObjPtr dom,
+ fd_set *keepfd,
const char ***retargv,
const char ***retenv);
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 04493ba..73f77f8 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -871,7 +871,7 @@ static int umlStartVMDaemon(virConnectPtr conn,
return -1;
}
-if (umlBuildCommandLine(conn, driver, vm,
+if (umlBuildCommandLine(conn, driver, vm, keepfd,
argv, progenv) 0) {
close(logfd);
umlCleanupTapDevices(conn, vm);
--
1.7.0.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] Managed save compression flags.
On 08/18/2010 02:32 AM, Daniel P. Berrange wrote: Don't get me wrong, I don't think this is a killer feature. But when I initially implemented compression, I definitely would have done it through the API if we had had a flags parameter available for virDomainSave(). Now that we have a ManagedSave that does have a flags parameter, I figured we do it the right way. I think it's a little cleaner, and more intuitive, to do it through the API, and it makes the feature available to hypervisors other than qemu. I still can't imagine anyone needing the ability to specify a different compression method per-guest VM. Perhaps a VIR_SAVE_COMPRESS to toggle compression on / off, with actual type of compression determined in the host config, but anything more seems rather overkill. But where is the host config for hypervisors other than qemu? Having only a compress/no-compress toggle makes sense as a reasonable compromise to me, if we know how to let the user specify the preferred compression type for all hypervisors. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] build: avoid %zu in translated strings
On 08/18/2010 03:04 AM, Daniel P. Berrange wrote: I find the PRI* stuff rather fugly. Can't we just use %llu and cast to (unsigned long long) Unfortunately, %llu is equally non-portable to mingw. And yes, we also have some %llu encoded into translated strings, which would also need help. The question of printf-posix license doesn't appear relevant since remoteError friends all use asprintf() which is LGPLv2+ already. We use the 'vasprintf' module, which is indeed LGPLv2+, but it does not guarantee the existence of %llu nor %zu -- it only guarantees that you have the [v]asprintf wrappers around your current system's (non-)compliant printf family, so it inherits the same bugs regarding unsupported specifiers. We would have to use the vasprintf-posix module to get %zu, but that module is LGPLv3+. One other potential solution: most (all?) of our translated strings involve error messages, and are therefore already funneled through our virterror.c implementation. As long as a string containing %zu or %llu is never directly handed to printf, but is guaranteed to go through virterror.c, then it would be possible to have virterror.c do some #ifdef magic such that on all sane platforms, the %zu and %llu are used unchanged (no extra overhead required); but on broken platforms (aka mingw), %zu and %llu are translated at runtime to %lu or %I64u (the %zu translation depends on whether you are 32-bit/64-bit mingw, and the 64-bit translation relies on microsoft's non-standard format specifier). This solution would then confine the ugliness to one file, such that the rest of libvirt can use %zu and %llu at will. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] build: avoid %zu in translated strings
On Wed, Aug 18, 2010 at 07:41:16AM -0600, Eric Blake wrote: On 08/18/2010 03:04 AM, Daniel P. Berrange wrote: I find the PRI* stuff rather fugly. Can't we just use %llu and cast to (unsigned long long) Unfortunately, %llu is equally non-portable to mingw. And yes, we also have some %llu encoded into translated strings, which would also need help. The question of printf-posix license doesn't appear relevant since remoteError friends all use asprintf() which is LGPLv2+ already. We use the 'vasprintf' module, which is indeed LGPLv2+, but it does not guarantee the existence of %llu nor %zu -- it only guarantees that you have the [v]asprintf wrappers around your current system's (non-)compliant printf family, so it inherits the same bugs regarding unsupported specifiers. We would have to use the vasprintf-posix module to get %zu, but that module is LGPLv3+. Hmm, that's odd, because support for %llu in printf was one of the primary reasons we started using GNULIB in the first place. We have been relying on %llu working, throughout the code. Daniel -- |: 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] [RFC PATCH] build: avoid %zu in translated strings
[re-adding bug-gnulib for another question] On 08/18/2010 07:51 AM, Daniel P. Berrange wrote: On Wed, Aug 18, 2010 at 07:41:16AM -0600, Eric Blake wrote: On 08/18/2010 03:04 AM, Daniel P. Berrange wrote: I find the PRI* stuff rather fugly. Can't we just use %llu and cast to (unsigned long long) Unfortunately, %llu is equally non-portable to mingw. And yes, we also have some %llu encoded into translated strings, which would also need help. The question of printf-posix license doesn't appear relevant since remoteError friends all use asprintf() which is LGPLv2+ already. We use the 'vasprintf' module, which is indeed LGPLv2+, but it does not guarantee the existence of %llu nor %zu -- it only guarantees that you have the [v]asprintf wrappers around your current system's (non-)compliant printf family, so it inherits the same bugs regarding unsupported specifiers. We would have to use the vasprintf-posix module to get %zu, but that module is LGPLv3+. Hmm, that's odd, because support for %llu in printf was one of the primary reasons we started using GNULIB in the first place. We have been relying on %llu working, throughout the code. Bruno, is my understanding of the differences between vasprintf and vasprintf-posix correct? If so, it means that gnulib is already in the situation that the bulk of the source code to support %zu and %llu is available via the vasprintf module, but hidden behind #defines that are not supplied unless you use the LGPL vasprint-posix module. But the vasprintf-module only adds .m4 files on top of the existing mix of vasprintf files, and there is nothing about the .m4 files that must be LGPLv3+ only (since they already have a more permissive license). On the surface, it seems like I could technically copy the contents of the vasprintf-posix .m4 files (since they are already a more permissive license) and couple that with the existing LGPLv2+ vasprintf module, all without violating any licensing. Looking a bit closer, though, vasprintf-posix drags in some new LGPLv3+ dependencies, like isnand-libm and printf-frexp; but even then, if I'm willing to link with -lm on mingw and avoid %a and %Lg, it seems like I can avoid those extra dependencies. Still, I'm reluctant to bite the bullet and go with the LGPLv2+ cascade on vasprintf-posix. So maybe the solution is an intermediate module: LGPLv2+ vasprintf - bare bones, guarantees a wrapper around system printf, so %zu and %llu are unsafe because of mingw LGPLv2+ vasprintf-sizes - guarantees %zu, %llu, %ju, %tu; but not %Lg (which means splitting gl_PRINTF_SIZES_C99 into two) or %'d LGPLV3+ vasprintf-posix - guarantees full contingency of POSIX specifiers If this three-level proposal makes sense, then I can start on the work of extracting the simpler portions of vasprintf-posix into the new vasprintf-sizes. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] build: avoid %zu in translated strings
On Wed, Aug 18, 2010 at 07:41:16AM -0600, Eric Blake wrote: On 08/18/2010 03:04 AM, Daniel P. Berrange wrote: I find the PRI* stuff rather fugly. Can't we just use %llu and cast to (unsigned long long) Unfortunately, %llu is equally non-portable to mingw. And yes, we also have some %llu encoded into translated strings, which would also need help. The question of printf-posix license doesn't appear relevant since remoteError friends all use asprintf() which is LGPLv2+ already. We use the 'vasprintf' module, which is indeed LGPLv2+, but it does not guarantee the existence of %llu nor %zu -- it only guarantees that you have the [v]asprintf wrappers around your current system's (non-)compliant printf family, so it inherits the same bugs regarding unsupported specifiers. We would have to use the vasprintf-posix module to get %zu, but that module is LGPLv3+. I don't think this is correct. The 'vasprintf' module was added in GNULIB in 87b04f998fd3e668027074b5b5d37205d3cdfec3. This commit includes a full re-implementation of format parsing that appears independent of the host system printf() impl. $ git show 87b04f998fd3e668027074b5b5d37205d3cdfec3 | diffstat | grep lib lib/ChangeLog | 13 lib/asnprintf.c| 38 ++ lib/asprintf.c | 38 ++ lib/printf-args.c | 119 lib/printf-args.h | 134 + lib/printf-parse.c | 477 lib/printf-parse.h | 72 lib/vasnprintf.c | 767 + lib/vasnprintf.h | 61 lib/vasprintf.c| 38 ++ lib/vasprintf.h| 64 These printf-args/parse files appear to handle long long int %llu combinations correctly. Since mingw32 lacks any vasprintf() at all, we will be using this gnulib replacement. The vasprintf-posix seems to only be used where vasprintf() exists but is broken, thus not on mingw32 I find it strange that vasprintf is a more liberal license than the vasprintf-posix, since the former is where all the really cool code is - the latter just seems to be a few m4 macros that anyone could reimplement with ease. Regards, Daniel -- |: 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] [RFC PATCH] build: avoid %zu in translated strings
On 08/18/2010 08:35 AM, Daniel P. Berrange wrote: These printf-args/parse files appear to handle long long int %llu combinations correctly. Since mingw32 lacks any vasprintf() at all, we will be using this gnulib replacement. The vasprintf-posix seems to only be used where vasprintf() exists but is broken, thus not on mingw32 The vasprintf module only uses the full printf parsing when told to do so by #ifdef; it prefers the much smaller approach of using snprintf to do all the work when there is no other compelling reason to use full parsing. I find it strange that vasprintf is a more liberal license than the vasprintf-posix, since the former is where all the really cool code is - the latter just seems to be a few m4 macros that anyone could reimplement with ease. vasprintf-posix is more than just m4 macros; it also drags in LGPLv3+ module dependencies that implement floating point parsing (such as printf-ldexp and isnand-nolibm), then does the additional checks for which #defines to enable. Among other things, it is these additional #defines that then force the compelling reason for vasnprintf.c to use full-blown parsing rather than the smaller footprint of wrapping snprintf. So our dilemma is that %zu and %llu need to be compelling reasons to turn on a subset of the full-blown parser, since mingw's snprintf lacks those, but without also dragging in the LGPLv3+ floating-point handling. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] OpenVZ: add ethernet interface type support
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
From 140ecba1ee0ed19df8eba5538c0cd7f1fd167ac2 Mon Sep 17 00:00:00 2001
From: Jean-Baptiste Rouault jean-baptiste.roua...@diateam.net
Date: Wed, 18 Aug 2010 16:59:59 +0200
Subject: [PATCH] OpenVZ: add ethernet interface type support
---
src/openvz/openvz_driver.c | 47 +++
1 files changed, 25 insertions(+), 22 deletions(-)
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index d2f91c6..a8dacec 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -741,53 +741,56 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid,
virCapabilitiesGenerateMac(driver-caps, host_mac);
virFormatMacAddr(host_mac, host_macaddr);
-if (net-type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
+if (net-type == VIR_DOMAIN_NET_TYPE_BRIDGE ||
+(net-type == VIR_DOMAIN_NET_TYPE_ETHERNET
+ net-data.ethernet.ipaddr == NULL)) {
virBuffer buf = VIR_BUFFER_INITIALIZER;
-char *dev_name_ve;
int veid = openvzGetVEID(vpsid);
//--netif_add ifname[,mac,host_ifname,host_mac]
ADD_ARG_LIT(--netif_add) ;
-/* generate interface name in ve and copy it to options */
-dev_name_ve = openvzGenerateContainerVethName(veid);
-if (dev_name_ve == NULL) {
- openvzError(VIR_ERR_INTERNAL_ERROR, %s,
- _(Could not generate eth name for container));
- rc = -1;
- goto exit;
+/* if user doesn't specify guest interface name,
+ * then we need to generate it */
+if (net-data.ethernet.dev == NULL) {
+net-data.ethernet.dev = openvzGenerateContainerVethName(veid);
+if (net-data.ethernet.dev == NULL) {
+ openvzError(VIR_ERR_INTERNAL_ERROR, %s,
+ _(Could not generate eth name for container));
+ rc = -1;
+ goto exit;
+}
}
/* if user doesn't specified host interface name,
* than we need to generate it */
if (net-ifname == NULL) {
-net-ifname = openvzGenerateVethName(veid, dev_name_ve);
+net-ifname = openvzGenerateVethName(veid, net-data.ethernet.dev);
if (net-ifname == NULL) {
openvzError(VIR_ERR_INTERNAL_ERROR, %s,
_(Could not generate veth name));
rc = -1;
- VIR_FREE(dev_name_ve);
goto exit;
}
}
-virBufferAdd(buf, dev_name_ve, -1); /* Guest dev */
+virBufferAdd(buf, net-data.ethernet.dev, -1); /* Guest dev */
virBufferVSprintf(buf, ,%s, macaddr); /* Guest dev mac */
virBufferVSprintf(buf, ,%s, net-ifname); /* Host dev */
virBufferVSprintf(buf, ,%s, host_macaddr); /* Host dev mac */
-if (driver-version = VZCTL_BRIDGE_MIN_VERSION) {
-virBufferVSprintf(buf, ,%s, net-data.bridge.brname); /* Host bridge */
-} else {
-virBufferVSprintf(configBuf, ifname=%s, dev_name_ve);
-virBufferVSprintf(configBuf, ,mac=%s, macaddr); /* Guest dev mac */
-virBufferVSprintf(configBuf, ,host_ifname=%s, net-ifname); /* Host dev */
-virBufferVSprintf(configBuf, ,host_mac=%s, host_macaddr); /* Host dev mac */
-virBufferVSprintf(configBuf, ,bridge=%s, net-data.bridge.brname); /* Host bridge */
+if (net-type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
+if (driver-version = VZCTL_BRIDGE_MIN_VERSION) {
+virBufferVSprintf(buf, ,%s, net-data.bridge.brname); /* Host bridge */
+} else {
+virBufferVSprintf(configBuf, ifname=%s, net-data.ethernet.dev);
+virBufferVSprintf(configBuf, ,mac=%s, macaddr); /* Guest dev mac */
+virBufferVSprintf(configBuf, ,host_ifname=%s, net-ifname); /* Host dev */
+virBufferVSprintf(configBuf, ,host_mac=%s, host_macaddr); /* Host dev mac */
+virBufferVSprintf(configBuf, ,bridge=%s, net-data.bridge.brname); /* Host bridge */
+}
}
-VIR_FREE(dev_name_ve);
-
if (!(opt = virBufferContentAndReset(buf)))
goto no_memory;
--
1.7.0.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] nodedev: Fix sysfs paths for vport operations
+if (stat(operation_path, st) != 0) {
+VIR_FREE(operation_path);
+if (virAsprintf(operation_path,
+%shost%d%s,
+LINUX_SYSFS_SCSI_HOST_PREFIX,
+parent_host,
+operation_file) 0) {
It's slightly more efficient to write:
virAsprintf(operation_path, LINUX_SYSFS_SCSI_HOST_PREFIX host%d%s,
parent_host, operation_file)
Yeah, it is but the original version is used on several places in node_device
so guess it's better to be consistent with the rest of the code. And although
it's slightly more efficient, I don't think it's a good idea to rewrite
existing code since it could lead into some ugly long lines, such as
LINUX_SYSFS_FC_HOST_PREFIX host%d LINUX_SYSFS_VPORT_CREATE_POSTFIX
:-)
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/5] Salvage old fixes
The following fixes did never make it to upstream libvirt Daniel Berrange (2): remote: Fix incorrect use of private data field xen: Fix device count on detach Daniel Veillard (1): xen: Fix scheduler setting problems Dave Allan (1): nodedev: Fix sysfs paths for vport operations Jiri Denemark (1): nodedev: Free the right pointers when getting WWNs fails ACK to series, although see comment on patch 3/5 for a nit. Thanks, I pushed all of them. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] build: avoid %zu in translated strings
On 08/18/2010 08:30 AM, Eric Blake wrote: Still, I'm reluctant to bite the bullet and go with the LGPLv2+ cascade on vasprintf-posix. So maybe the solution is an intermediate module: LGPLv2+ vasprintf - bare bones, guarantees a wrapper around system printf, so %zu and %llu are unsafe because of mingw LGPLv2+ vasprintf-sizes - guarantees %zu, %llu, %ju, %tu; but not %Lg (which means splitting gl_PRINTF_SIZES_C99 into two) or %'d LGPLV3+ vasprintf-posix - guarantees full contingency of POSIX specifiers If this three-level proposal makes sense, then I can start on the work of extracting the simpler portions of vasprintf-posix into the new vasprintf-sizes. Actually, after looking into this deeper, it (happily) appears that I may have been mistaken. It looks like the gnulib vasprintf module _already_ performs printf parsing on mingw; and that as a virtue of that printf parsing, %zu and %llu are already rewritten into modifiers understood by mingw. I tested this by modifying test-vasprintf.c to try %zu and %llu on a mingw compilation, and the test still passed. The gnulib module snprintf will likewise support %zu and %llu, but only if it is in use (right now, libvirt is not using the snprintf module). However, there is no counterpart printf or sprintf, but libvirt uses those functions as well. So there is still some work to be done in libvirt to make sure all *printf() family calls eventually end up going through gnulib wrappers, to take advantage of the fact that gnulib's vasprintf _is_ sufficient to support %zu and %llu on mingw. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] build: avoid %zu in translated strings
On Wed, Aug 18, 2010 at 09:54:51AM -0600, Eric Blake wrote: On 08/18/2010 08:30 AM, Eric Blake wrote: Still, I'm reluctant to bite the bullet and go with the LGPLv2+ cascade on vasprintf-posix. So maybe the solution is an intermediate module: LGPLv2+ vasprintf - bare bones, guarantees a wrapper around system printf, so %zu and %llu are unsafe because of mingw LGPLv2+ vasprintf-sizes - guarantees %zu, %llu, %ju, %tu; but not %Lg (which means splitting gl_PRINTF_SIZES_C99 into two) or %'d LGPLV3+ vasprintf-posix - guarantees full contingency of POSIX specifiers If this three-level proposal makes sense, then I can start on the work of extracting the simpler portions of vasprintf-posix into the new vasprintf-sizes. Actually, after looking into this deeper, it (happily) appears that I may have been mistaken. It looks like the gnulib vasprintf module _already_ performs printf parsing on mingw; and that as a virtue of that printf parsing, %zu and %llu are already rewritten into modifiers understood by mingw. I tested this by modifying test-vasprintf.c to try %zu and %llu on a mingw compilation, and the test still passed. The gnulib module snprintf will likewise support %zu and %llu, but only if it is in use (right now, libvirt is not using the snprintf module). However, there is no counterpart printf or sprintf, but libvirt uses those functions as well. So there is still some work to be done in libvirt to make sure all *printf() family calls eventually end up going through gnulib wrappers, to take advantage of the fact that gnulib's vasprintf _is_ sufficient to support %zu and %llu on mingw. Do we actually have any places where printf/sprintf hurts ? I see nothing using %zu # find -name '*.c' | xargs grep -i printf | grep -i -v asprintf | grep -v virBuffer | grep -v gnulib | grep '%z' And just a handful of things using %ll $ find -name '*.c' | xargs grep -i printf | grep -i -v asprintf | grep -v virBuffer | grep -v gnulib | grep '%ll' ./src/storage/storage_backend.c:snprintf(size, sizeof(size), %lluK, vol-capacity/1024); ./src/storage/storage_backend.c:snprintf(size, sizeof(size), %llu, vol-capacity/1024/1024); ./src/storage/parthelper.c: printf(%s%s%d%c%s%c%s%c%llu%c%llu%c%llu%c, ./src/storage/parthelper.c:printf(%s%c%s%c%s%c%llu%c%llu%c%llu%c, ./src/storage/storage_backend_disk.c:snprintf(start, sizeof(start)-1, %lluB, startOffset); ./src/storage/storage_backend_disk.c:snprintf(end, sizeof(end)-1, %lluB, endOffset); ./src/storage/storage_backend_logical.c:snprintf(size, sizeof(size)-1, %lluK, vol-capacity/1024); ./tests/qemuhelptest.c:fprintf(stderr, Computed flags do not match: got 0x%llx, expected 0x%llx\n, ./examples/domain-events/events-c/event-test.c:printf(%s EVENT: Domain %s(%d) rtc change %lld\n, __func__, virDomainGetName(dom), All these are code modules which are Linux specific, though arguably we should kill the snprintf() in favour of virAsprintf() for sanity sake. Everything else ends up routed via vasprintf so, I think it is sufficient for libvirt to just use vasprintf and not worry about the printf/snprintf issue (at least not urgently) Regards, Daniel -- |: 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
[libvirt] virsh attach-disk using files and KVM
Currently virsh attach-disk interface only accepts 'file' or 'tap' for driver type when attaching files as disks. One can succesfully attach a file as disk with: virsh attach-disk vm file drive --driver file --type disk which generates the following xml which is passed to libvirt: disk type='file' device='disk' driver name='file' type='raw'/ source file='/images/test02.img'/ target dev='vdc' bus='virtio'/ alias name='virtio-disk2'/ address type='pci' domain='0x' bus='0x00' slot='0x07' function='0x0'/ /disk Now, if you shutdown the guest and restart, libvirt complains that the driver type 'file' isn't supported. This is from src/qemu/qemu_conf.c:4146 where if the driver name isn't 'qemu' it rejects the configuration. How best to resolve this? Update qemu_conf.c to accept 'file' type? update virsh to allow specifying 'qemu' as a driver type for files? -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ry...@us.ibm.com -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH] build: avoid %zu in translated strings
On 08/18/2010 10:07 AM, Daniel P. Berrange wrote:
Do we actually have any places where printf/sprintf hurts ?
[v]as[n]printf are already safe, thanks to the vasprintf module.
snprintf is safe, but only indirectly, due to the getaddrinfo module
dragging it in (if getaddrinfo is changed to not rely on snprintf, then
we would lose the indirect support), but I agree that we should be using
virAsprintf in that case, anyways.
vsnprintf is not safe, but can easily be made safe at the same time as
snprintf.
[v][f]printf and [v]sprintf are not safe, with nothing in gnulib to
protect them while still staying at LGPLv2+; but I agree that we can
probably avoid the issues with these by converting sprintf to
virAsprintf, and just being careful with [f]printf.
And just a handful of things using %ll
$ find -name '*.c' | xargs grep -i printf | grep -i -v asprintf | grep -v
virBuffer | grep -v gnulib | grep '%ll'
./src/storage/storage_backend.c:snprintf(size, sizeof(size), %lluK,
vol-capacity/1024);
./src/storage/storage_backend.c:snprintf(size, sizeof(size), %llu,
vol-capacity/1024/1024);
./src/storage/parthelper.c:
printf(%s%s%d%c%s%c%s%c%llu%c%llu%c%llu%c,
./src/storage/parthelper.c:
printf(%s%c%s%c%s%c%llu%c%llu%c%llu%c,
./src/storage/storage_backend_disk.c:snprintf(start, sizeof(start)-1,
%lluB, startOffset);
./src/storage/storage_backend_disk.c:snprintf(end, sizeof(end)-1,
%lluB, endOffset);
./src/storage/storage_backend_logical.c:snprintf(size, sizeof(size)-1,
%lluK, vol-capacity/1024);
./tests/qemuhelptest.c:fprintf(stderr, Computed flags do not match:
got 0x%llx, expected 0x%llx\n,
./examples/domain-events/events-c/event-test.c:printf(%s EVENT: Domain
%s(%d) rtc change %lld\n, __func__, virDomainGetName(dom),
For a more complete list of all potential problems, I used:
$ git grep '\bv\?s\?f\?printf \?(' \
daemon/ tools/ src/ include/ proxy/ tests/ | wc -l
236
And except for the few you already listed above, none of them had %z or
%ll issues. So fixing those few, plus converting snprintf to
virAsprintf, seems like a manageable task; I'm now working on it.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] virsh attach-disk using files and KVM
On Wed, Aug 18, 2010 at 12:22:27PM -0500, Ryan Harper wrote: Currently virsh attach-disk interface only accepts 'file' or 'tap' for driver type when attaching files as disks. One can succesfully attach a file as disk with: virsh attach-disk vm file drive --driver file --type disk which generates the following xml which is passed to libvirt: disk type='file' device='disk' driver name='file' type='raw'/ source file='/images/test02.img'/ target dev='vdc' bus='virtio'/ alias name='virtio-disk2'/ address type='pci' domain='0x' bus='0x00' slot='0x07' function='0x0'/ /disk Now, if you shutdown the guest and restart, libvirt complains that the driver type 'file' isn't supported. This is from src/qemu/qemu_conf.c:4146 where if the driver name isn't 'qemu' it rejects the configuration. How best to resolve this? Update qemu_conf.c to accept 'file' type? update virsh to allow specifying 'qemu' as a driver type for files? No, 'file' is not the correct type for QEMU. virsh is broken - it has no business doing any validation checks for these parameters. It must be left upto the driver itself to validate Daniel -- |: 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] virsh attach-disk using files and KVM
* Daniel P. Berrange berra...@redhat.com [2010-08-18 12:35]: On Wed, Aug 18, 2010 at 12:22:27PM -0500, Ryan Harper wrote: Currently virsh attach-disk interface only accepts 'file' or 'tap' for driver type when attaching files as disks. One can succesfully attach a file as disk with: virsh attach-disk vm file drive --driver file --type disk which generates the following xml which is passed to libvirt: disk type='file' device='disk' driver name='file' type='raw'/ source file='/images/test02.img'/ target dev='vdc' bus='virtio'/ alias name='virtio-disk2'/ address type='pci' domain='0x' bus='0x00' slot='0x07' function='0x0'/ /disk Now, if you shutdown the guest and restart, libvirt complains that the driver type 'file' isn't supported. This is from src/qemu/qemu_conf.c:4146 where if the driver name isn't 'qemu' it rejects the configuration. How best to resolve this? Update qemu_conf.c to accept 'file' type? update virsh to allow specifying 'qemu' as a driver type for files? No, 'file' is not the correct type for QEMU. virsh is broken - it has no business doing any validation checks for these parameters. It must be left upto the driver itself to validate ok, so rip out all of the driver and mode check and just let it make the xml from the inputs and let libvirt validate? Daniel -- |: 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 :| -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ry...@us.ibm.com -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] build: fix compiler warning
node_device/node_device_driver.c: In function 'nodeDeviceVportCreateDelete': node_device/node_device_driver.c:423: error: implicit declaration of function 'stat' [-Wimplicit-function-declaration] * src/node_device/node_device_driver.c (includes): Add sys/stat.h. --- Pushing as obvious under the build-breaker rule. src/node_device/node_device_driver.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index a6ac80b..448cfd3 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1,6 +1,7 @@ /* * node_device.c: node device enumeration * + * Copyright (C) 2010 Red Hat, Inc. * Copyright (C) 2008 Virtual Iron Software, Inc. * Copyright (C) 2008 David F. Lively * @@ -27,6 +28,7 @@ #include errno.h #include fcntl.h #include time.h +#include sys/stat.h #include virterror_internal.h #include datatypes.h -- 1.7.2.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] using sync_manager with libvirt
On 08/11/2010 05:27 PM, Daniel P. Berrange wrote: On Wed, Aug 11, 2010 at 03:37:12PM -0400, David Teigland wrote: On Wed, Aug 11, 2010 at 05:59:55PM +0100, Daniel P. Berrange wrote: On Tue, Aug 10, 2010 at 12:44:06PM -0400, David Teigland wrote: ... There's two different, but related problems here: - Preventing 2 different VMs using the same disk - Preventing the same VM running on 2 hosts at once The first requires that there is a lease per configured disk (since a guest can have multiple disks). The latter requires a lease per VM and can ignore specifices of what disks are configured. IIUC, sync-manager is aiming for the latter. If we only aim for the latter, then there is no protection mechanism to prevent two sysadmins using the same storage from independently creating two vms that use the same backend disk accidentally. On the other hand, we also need to be able to support the concept of a single block device shared among multiple guests intentionally (i.e. clustered filesystems, or applications that know how to properly use shared storage) So in addition to per-disk exclusive-write leases, do we also need per-disk shared-write leases? Or do we just say that disks that are marked as 'shared' just don't get leases at all? The present integration effort is aiming for the latter. sync_manager itself aims to be agnostic about what it's managing. Ok, it makes a bit of a difference to how we integrate with it in libvirt. If we want to ever let sync-manager do per-disk leases then we'd want to pass more info to the SM callbacks so it knows what disks QEMU has, instead of just its name I dunno, but if the end goal is the latter, then why not do it correct from the start rather than integrating halfway and then having a second round of integration to move from per-vm leasing to per-disk leasing. Perry -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 02/10] nwfilter: use consistent OOM reporting
* src/nwfilter/nwfilter_driver.c (nwfilterLog): Delete.
(nwfilterDriverStartup): Use virReportOOMError instead.
---
No point making printf uses harder to audit by hiding them in a macro,
especially when this file already uses virReportOOMError elsewhere.
src/nwfilter/nwfilter_driver.c |6 +-
1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 0e8241e..bda50f9 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -42,9 +42,6 @@
#define VIR_FROM_THIS VIR_FROM_NWFILTER
-#define nwfilterLog(msg...) fprintf(stderr, msg)
-
-
static virNWFilterDriverStatePtr driverState;
static int nwfilterDriverShutdown(void);
@@ -95,7 +92,6 @@ nwfilterDriverStartup(int privileged) {
goto error;
if (virAsprintf(base, %s/.libvirt, userdir) == -1) {
-nwfilterLog(out of memory in virAsprintf);
VIR_FREE(userdir);
goto out_of_memory;
}
@@ -118,7 +114,7 @@ nwfilterDriverStartup(int privileged) {
return 0;
out_of_memory:
-nwfilterLog(virNWFilterStartup: out of memory);
+virReportOOMError();
error:
VIR_FREE(base);
--
1.7.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 00/10] start tackling *printf issues
Based on the long thread about whether %zu and %llu are safe to use, here's a series of patches to start tackling the problems. I've got more work to do, but had enough ready that it was worth posting this much for review, and can push in pieces as individual patches are ACK'd. Eric Blake (10): uml: fix logic bug in checking reply length nwfilter: use consistent OOM reporting build: delete dead comment maint: whitespace cleanups storage: avoid s[n]printf squash to dead comment xenapi: avoid sprintf vbox: add location used in rpmfusion release vbox: factor a large function vbox: avoid sprintf configure.ac |1 + src/nwfilter/nwfilter_driver.c |6 +- src/qemu/qemu_driver.c |1 - src/storage/storage_backend.c | 16 ++- src/storage/storage_backend_disk.c | 124 +- src/uml/uml_driver.c |8 +- src/vbox/vbox_tmpl.c | 327 +++- src/xen/sexpr.c|8 - src/xenapi/xenapi_utils.c | 15 +- src/xenapi/xenapi_utils.h |4 - 10 files changed, 277 insertions(+), 233 deletions(-) -- 1.7.2.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 01/10] uml: fix logic bug in checking reply length
* src/uml/uml_driver.c (umlMonitorCommand): Validate that enough
bytes were read to dereference both res.length, and that many
bytes from res.data.
Reported by Soren Hansen.
---
Whoops; this is a resend of an unrelated issue, but it is still
sitting on my tree, and the original email has no review yet,
perhaps because it was in a reply to a longish thread.
src/uml/uml_driver.c |7 ++-
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 04493ba..37ddc39 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -737,14 +737,11 @@ static int umlMonitorCommand(const struct uml_driver
*driver,
virReportSystemError(errno, _(cannot read reply %s), cmd);
goto error;
}
-if (nbytes sizeof res) {
+if (nbytes offsetof(struct monitor_request, data) ||
+nbytes res.length + offsetof(struct monitor_request, data)) {
virReportSystemError(0, _(incomplete reply %s), cmd);
goto error;
}
-if (sizeof res.data res.length) {
-virReportSystemError(0, _(invalid length in reply %s), cmd);
-goto error;
-}
if (VIR_REALLOC_N(retdata, retlen + res.length) 0) {
virReportOOMError();
--
1.7.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 07/10] xenapi: avoid sprintf
* src/xenapi/xenapi_utils.h (createVifNetwork): Delete prototype.
* src/xenapi/xenapi_utils.c (createVifNetwork): Change signature,
and use virAsprintf. Detect allocation failure.
(createVMRecordFromXml): Adjust caller.
---
I couldn't find any other uses of createVifNetwork, so changing
its prototype and marking it static seemed best.
src/xenapi/xenapi_utils.c | 15 +--
src/xenapi/xenapi_utils.h |4
2 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c
index 4eb17fa..23d3fef 100644
--- a/src/xenapi/xenapi_utils.c
+++ b/src/xenapi/xenapi_utils.c
@@ -387,8 +387,8 @@ xenapiSessionErrorHandle(virConnectPtr conn, virErrorNumber
errNum,
}
/* creates network intereface for VM */
-int
-createVifNetwork (virConnectPtr conn, xen_vm vm, char *device,
+static int
+createVifNetwork (virConnectPtr conn, xen_vm vm, int device,
char *bridge, char *mac)
{
xen_session *session = ((struct _xenapiPrivate
*)(conn-privateData))-session;
@@ -432,7 +432,8 @@ createVifNetwork (virConnectPtr conn, xen_vm vm, char
*device,
vif_record-other_config = xen_string_string_map_alloc(0);
vif_record-runtime_properties = xen_string_string_map_alloc(0);
vif_record-qos_algorithm_params = xen_string_string_map_alloc(0);
-vif_record-device = strdup(device);
+if (virAsprintf(vif_record-device, %d, device) 0)
+return -1;
xen_vif_create(session, vif, vif_record);
if (!vif) {
xen_vif_free(vif);
@@ -553,9 +554,11 @@ createVMRecordFromXml (virConnectPtr conn, virDomainDefPtr
def,
}
}
if (mac != NULL bridge != NULL) {
-char device[NETWORK_DEVID_SIZE] = \0;
-sprintf(device, %d, device_number);
-createVifNetwork(conn, *vm, device, bridge, mac);
+if (createVifNetwork(conn, *vm, device_number, bridge,
+ mac) 0) {
+VIR_FREE(bridge);
+goto error_cleanup;
+}
VIR_FREE(bridge);
device_number++;
}
diff --git a/src/xenapi/xenapi_utils.h b/src/xenapi/xenapi_utils.h
index c062a1e..2140105 100644
--- a/src/xenapi/xenapi_utils.h
+++ b/src/xenapi/xenapi_utils.h
@@ -78,8 +78,4 @@ createVMRecordFromXml (virConnectPtr conn, virDomainDefPtr
defPtr,
int
allocStringMap (xen_string_string_map **strings, char *key, char *val);
-int
-createVifNetwork(virConnectPtr conn, xen_vm vm, char *device,
- char *bridge, char *mac);
-
#endif /* __VIR_XENAPI_UTILS__ */
--
1.7.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 03/10] build: delete dead comment
* src/qemu/qemu_driver.c (qemudGetProcessInfo): Clean up.
---
src/qemu/qemu_driver.c |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3d61ccd..656a1e4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4316,7 +4316,6 @@ static int qemudGetProcessInfo(unsigned long long
*cpuTime, int *lastCpu, int pi
}
if (!(pidinfo = fopen(proc, r))) {
-/*printf(cannot read pid info);*/
/* VM probably shut down, so fake 0 */
if (cpuTime)
*cpuTime = 0;
--
1.7.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 04/10] maint: whitespace cleanups
* src/storage/storage_backend_disk.c
(virStorageBackendDiskPartFormat): Fix spacing.
---
Should be cosmetic only.
src/storage/storage_backend_disk.c | 66 +++-
1 files changed, 35 insertions(+), 31 deletions(-)
diff --git a/src/storage/storage_backend_disk.c
b/src/storage/storage_backend_disk.c
index 7188386..4038093 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -385,20 +385,22 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool,
{
int i;
if (pool-def-source.format == VIR_STORAGE_POOL_DISK_DOS) {
-const char *partedFormat =
virStoragePartedFsTypeTypeToString(vol-target.format);
+const char *partedFormat;
+partedFormat = virStoragePartedFsTypeTypeToString(vol-target.format);
if(partedFormat == NULL) {
- virStorageReportError(VIR_ERR_INTERNAL_ERROR,
- %s, _(Invalid partition type));
- return -1;
+virStorageReportError(VIR_ERR_INTERNAL_ERROR,
+ %s, _(Invalid partition type));
+return -1;
}
if (vol-target.format == VIR_STORAGE_VOL_DISK_EXTENDED) {
/* make sure we don't have a extended partition already */
for (i = 0; i pool-volumes.count; i++) {
- if (pool-volumes.objs[i]-target.format ==
VIR_STORAGE_VOL_DISK_EXTENDED) {
- virStorageReportError(VIR_ERR_INTERNAL_ERROR,
- %s, _(extended partition already
exists));
- return -1;
- }
+if (pool-volumes.objs[i]-target.format ==
+VIR_STORAGE_VOL_DISK_EXTENDED) {
+virStorageReportError(VIR_ERR_INTERNAL_ERROR, %s,
+ _(extended partition already
exists));
+return -1;
+}
}
sprintf(partFormat, %s, partedFormat);
} else {
@@ -407,25 +409,26 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool,
to create logical partitions. */
/* XXX Only support one extended partition */
switch (virStorageBackendDiskPartTypeToCreate(pool)) {
-case VIR_STORAGE_VOL_DISK_TYPE_PRIMARY:
- sprintf(partFormat, primary %s, partedFormat);
- break;
-case VIR_STORAGE_VOL_DISK_TYPE_LOGICAL:
- /* make sure we have a extended partition */
- for (i = 0; i pool-volumes.count; i++) {
- if (pool-volumes.objs[i]-target.format ==
VIR_STORAGE_VOL_DISK_EXTENDED) {
- sprintf(partFormat, logical %s,
partedFormat);
- break;
- }
- }
- if (i == pool-volumes.count) {
- virStorageReportError(VIR_ERR_INTERNAL_ERROR,
- %s, _(no extended
partition found and no primary partition available));
- return -1;
- }
- break;
-default:
- break;
+case VIR_STORAGE_VOL_DISK_TYPE_PRIMARY:
+sprintf(partFormat, primary %s, partedFormat);
+break;
+case VIR_STORAGE_VOL_DISK_TYPE_LOGICAL:
+/* make sure we have a extended partition */
+for (i = 0; i pool-volumes.count; i++) {
+if (pool-volumes.objs[i]-target.format ==
+VIR_STORAGE_VOL_DISK_EXTENDED) {
+sprintf(partFormat, logical %s, partedFormat);
+break;
+}
+}
+if (i == pool-volumes.count) {
+virStorageReportError(VIR_ERR_INTERNAL_ERROR,
+ %s, _(no extended partition found
and no primary partition available));
+return -1;
+}
+break;
+default:
+break;
}
}
} else {
@@ -436,7 +439,7 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool,
/**
* Aligns a new partition to nearest cylinder boundry
- * when haveing a msdos partition table type
+ * when having a msdos partition table type
* to avoid any problem with all ready existing
* partitions
*/
@@ -455,7 +458,8 @@ virStorageBackendDiskPartBoundries(virStoragePoolObjPtr
pool,
unsigned long long cylinderSize = dev-geometry.heads *
dev-geometry.sectors * SECTOR_SIZE;
-DEBUG(find free area: allocation %llu, cyl size %llu, allocation,
cylinderSize);
+DEBUG(find free
[libvirt] [PATCH 06/10] squash to dead comment
* src/uml/uml_driver.c (umlGetProcessInfo): Likewise.
* src/xen/sexpr.c (_string2sexpr): Likewise.
---
Oops - I hit 'git send-email' before rebasing one last time.
This one will be squashed into 3/10 before I push anything.
src/uml/uml_driver.c |1 -
src/xen/sexpr.c |8
2 files changed, 0 insertions(+), 9 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 37ddc39..2940978 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -1069,7 +1069,6 @@ static int umlGetProcessInfo(unsigned long long *cpuTime,
int pid) {
}
if (!(pidinfo = fopen(proc, r))) {
-/*printf(cannot read pid info);*/
/* VM probably shut down, so fake 0 */
*cpuTime = 0;
return 0;
diff --git a/src/xen/sexpr.c b/src/xen/sexpr.c
index 7e370db..2184060 100644
--- a/src/xen/sexpr.c
+++ b/src/xen/sexpr.c
@@ -320,14 +320,6 @@ _string2sexpr(const char *buffer, size_t * end)
sexpr_free(tmp);
goto error;
}
-#if 0
-if (0) {
-char buf[4096];
-
-sexpr2string(ret, buf, sizeof(buf));
-printf(%s\n, buffer);
-}
-#endif
ptr = trim(ptr + tmp_len);
}
--
1.7.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 05/10] storage: avoid s[n]printf
* src/storage/storage_backend.c (virStorageBackendCreateQemuImg)
(virStorageBackendCreateQcowCreate): Use virAsprintf instead.
* src/storage/storage_backend_disk.c
(virStorageBackendDiskCreateVol, virStorageBackendDiskPartFormat):
Likewise.
---
Things to look out for:
virStorageBackendDiskPartFormat can now fail where it used to
do nothing to the passed-in partFormat variable, if the switch
statement hits the default.
src/storage/storage_backend.c | 16 ++--
src/storage/storage_backend_disk.c | 66 +--
2 files changed, 59 insertions(+), 23 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 1fe7ba6..580d859 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -636,7 +636,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
unsigned int flags ATTRIBUTE_UNUSED)
{
int ret = -1;
-char size[100];
+char *size = NULL;
char *create_tool;
const char *type = virStorageFileFormatTypeToString(vol-target.format);
@@ -726,7 +726,10 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
}
/* Size in KB */
-snprintf(size, sizeof(size), %lluK, vol-capacity/1024);
+if (virAsprintf(size, %lluK, vol-capacity / 1024) 0) {
+virReportOOMError();
+goto cleanup;
+}
/* KVM is usually ahead of qemu on features, so try that first */
create_tool = virFindFileInPath(kvm-img);
@@ -821,6 +824,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
}
cleanup:
+VIR_FREE(size);
VIR_FREE(create_tool);
return ret;
@@ -838,7 +842,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn
ATTRIBUTE_UNUSED,
unsigned int flags ATTRIBUTE_UNUSED)
{
int ret;
-char size[100];
+char *size;
const char *imgargv[4];
if (inputvol) {
@@ -867,7 +871,10 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn
ATTRIBUTE_UNUSED,
}
/* Size in MB - yes different units to qemu-img :-( */
-snprintf(size, sizeof(size), %llu, vol-capacity/1024/1024);
+if (virAsprintf(size, %llu, vol-capacity / 1024 / 1024) 0) {
+virReportOOMError();
+return -1;
+}
imgargv[0] = virFindFileInPath(qcow-create);
imgargv[1] = size;
@@ -876,6 +883,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn
ATTRIBUTE_UNUSED,
ret = virStorageBackendCreateExecCommand(pool, vol, imgargv);
VIR_FREE(imgargv[0]);
+VIR_FREE(size);
return ret;
}
diff --git a/src/storage/storage_backend_disk.c
b/src/storage/storage_backend_disk.c
index 4038093..587b1a6 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -381,7 +381,7 @@ virStorageBackendDiskPartTypeToCreate(virStoragePoolObjPtr
pool)
static int
virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
-char* partFormat)
+char** partFormat)
{
int i;
if (pool-def-source.format == VIR_STORAGE_POOL_DISK_DOS) {
@@ -402,7 +402,10 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool,
return -1;
}
}
-sprintf(partFormat, %s, partedFormat);
+if ((*partFormat = strdup(partedFormat)) == NULL) {
+virReportOOMError();
+return -1;
+}
} else {
/* create primary partition as long as it is possible
and after that check if an extended partition exists
@@ -410,14 +413,21 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool,
/* XXX Only support one extended partition */
switch (virStorageBackendDiskPartTypeToCreate(pool)) {
case VIR_STORAGE_VOL_DISK_TYPE_PRIMARY:
-sprintf(partFormat, primary %s, partedFormat);
+if (virAsprintf(partFormat, primary %s, partedFormat) 0) {
+virReportOOMError();
+return -1;
+}
break;
case VIR_STORAGE_VOL_DISK_TYPE_LOGICAL:
/* make sure we have a extended partition */
for (i = 0; i pool-volumes.count; i++) {
if (pool-volumes.objs[i]-target.format ==
VIR_STORAGE_VOL_DISK_EXTENDED) {
-sprintf(partFormat, logical %s, partedFormat);
+if (virAsprintf(partFormat, logical %s,
+partedFormat) 0) {
+virReportOOMError();
+return -1;
+}
break;
}
}
@@ -428,11 +438,16 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool,
}
[libvirt] [PATCH 10/10] vbox: avoid sprintf
Work in progress:
This gets 2 of the 5 suspect uses; I ran out of time today.
* src/vbox/vbox_tmpl.c (vboxStartMachine): Use virAsprintf instead.
---
I could check this in as is (after tweaking the commit comment)
and do the other three sprintf uses in another patch, but I'd
rather merge all sprintf changes in this file to a single commit.
At any rate, reviewing this portion for any memory leaks now will
be helpful.
src/vbox/vbox_tmpl.c | 19 ++-
1 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index c28981c..6b9de6c 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -3162,7 +3162,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;
@@ -3242,8 +3241,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);
}
@@ -3252,8 +3256,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);
}
--
1.7.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 09/10] vbox: factor a large function
* src/vbox/vbox_tmpl.c (vboxDomainCreateWithFlags): Split...
(vboxStartMachine): ...into new helper.
---
This function was just too unbearable with that much nested indentation.
This should be a no-op refactoring.
vboxDomainDefineXML is even worse, and contains the remaining sprintf
instances in this file; I'll probably try to factor that one down as well.
src/vbox/vbox_tmpl.c | 318 ++
1 files changed, 165 insertions(+), 153 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 31fec67..c28981c 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -3144,14 +3144,174 @@ cleanup:
return ret;
}
+
+static int
+vboxStartMachine(virDomainPtr dom, int i, IMachine *machine, vboxIID *iid
+ )
+{
+VBOX_OBJECT_CHECK(dom-conn, int, -1);
+int vrdpPresent = 0;
+int sdlPresent = 0;
+int guiPresent = 0;
+char *guiDisplay = NULL;
+char *sdlDisplay = NULL;
+PRUnichar *keyTypeUtf16 = NULL;
+PRUnichar *valueTypeUtf16= NULL;
+char *valueTypeUtf8 = NULL;
+PRUnichar *keyDislpayUtf16 = NULL;
+PRUnichar *valueDisplayUtf16 = NULL;
+char *valueDisplayUtf8 = NULL;
+IProgress *progress = NULL;
+char displayutf8[32] = {0};
+PRUnichar *env = NULL;
+PRUnichar *sessionType = NULL;
+nsresult rc;
+
+VBOX_UTF8_TO_UTF16(FRONTEND/Type, keyTypeUtf16);
+machine-vtbl-GetExtraData(machine, keyTypeUtf16, valueTypeUtf16);
+VBOX_UTF16_FREE(keyTypeUtf16);
+
+if (valueTypeUtf16) {
+VBOX_UTF16_TO_UTF8(valueTypeUtf16, valueTypeUtf8);
+VBOX_UTF16_FREE(valueTypeUtf16);
+
+if ( STREQ(valueTypeUtf8, sdl) || STREQ(valueTypeUtf8, gui) ) {
+
+VBOX_UTF8_TO_UTF16(FRONTEND/Display, keyDislpayUtf16);
+machine-vtbl-GetExtraData(machine, keyDislpayUtf16,
+valueDisplayUtf16);
+VBOX_UTF16_FREE(keyDislpayUtf16);
+
+if (valueDisplayUtf16) {
+VBOX_UTF16_TO_UTF8(valueDisplayUtf16, valueDisplayUtf8);
+VBOX_UTF16_FREE(valueDisplayUtf16);
+
+if (strlen(valueDisplayUtf8) = 0) {
+VBOX_UTF8_FREE(valueDisplayUtf8);
+valueDisplayUtf8 = NULL;
+}
+}
+
+if (STREQ(valueTypeUtf8, sdl)) {
+sdlPresent = 1;
+if (valueDisplayUtf8) {
+sdlDisplay = strdup(valueDisplayUtf8);
+if (sdlDisplay == NULL) {
+virReportOOMError();
+/* just don't go to cleanup yet as it is ok to have
+ * sdlDisplay as NULL and we check it below if it
+ * exist and then only use it there
+ */
+}
+}
+}
+
+if (STREQ(valueTypeUtf8, gui)) {
+guiPresent = 1;
+if (valueDisplayUtf8) {
+guiDisplay = strdup(valueDisplayUtf8);
+if (guiDisplay == NULL) {
+virReportOOMError();
+/* just don't go to cleanup yet as it is ok to have
+ * guiDisplay as NULL and we check it below if it
+ * exist and then only use it there
+ */
+}
+}
+}
+}
+
+if (STREQ(valueTypeUtf8, vrdp)) {
+vrdpPresent = 1;
+}
+
+if (!vrdpPresent !sdlPresent !guiPresent) {
+/* if nothing is selected it means either the machine xml
+ * file is really old or some values are missing so fallback
+ */
+guiPresent = 1;
+}
+
+VBOX_UTF8_FREE(valueTypeUtf8);
+
+} else {
+guiPresent = 1;
+}
+if (valueDisplayUtf8)
+VBOX_UTF8_FREE(valueDisplayUtf8);
+
+if (guiPresent) {
+if (guiDisplay) {
+sprintf(displayutf8, DISPLAY=%.24s, guiDisplay);
+VBOX_UTF8_TO_UTF16(displayutf8, env);
+VIR_FREE(guiDisplay);
+}
+
+VBOX_UTF8_TO_UTF16(gui, sessionType);
+}
+
+if (sdlPresent) {
+if (sdlDisplay) {
+sprintf(displayutf8, DISPLAY=%.24s, sdlDisplay);
+VBOX_UTF8_TO_UTF16(displayutf8, env);
+VIR_FREE(sdlDisplay);
+}
+
+VBOX_UTF8_TO_UTF16(sdl, sessionType);
+}
+
+if (vrdpPresent) {
+VBOX_UTF8_TO_UTF16(vrdp, sessionType);
+}
+
+rc = data-vboxObj-vtbl-OpenRemoteSession(data-vboxObj,
+data-vboxSession,
+iid,
+sessionType,
+
