[libvirt] [PATCH] Support virDomainAttachDevice and virDomainDetachDevice for disks in UML
From: Soren Hansen so...@linux2go.dk
UML supports hot plugging and unplugging of various devices. This patch
exposes this functionality for disks.
Signed-off-by: Soren Hansen so...@linux2go.dk
---
src/uml/uml_driver.c | 223 +-
1 files changed, 221 insertions(+), 2 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 04493ba..a09bc60 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -1686,6 +1686,225 @@ cleanup:
}
+static inline void umlShrinkDisks(virDomainDefPtr def, size_t i)
+{
+if (def-ndisks 1) {
+memmove(def-disks + i,
+def-disks + i + 1,
+sizeof(*def-disks) *
+(def-ndisks - (i + 1)));
+def-ndisks--;
+if (VIR_REALLOC_N(def-disks, def-ndisks) 0) {
+/* ignore, harmless */
+}
+} else {
+VIR_FREE(def-disks);
+def-ndisks = 0;
+}
+}
+
+
+static int umlDomainAttachUmlDisk(struct uml_driver *driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
+{
+int i, ret;
+char *cmd = NULL;
+char *reply;
+
+for (i = 0 ; i vm-def-ndisks ; i++) {
+if (STREQ(vm-def-disks[i]-dst, disk-dst)) {
+umlReportError(VIR_ERR_OPERATION_FAILED,
+ _(target %s already exists), disk-dst);
+return -1;
+}
+}
+
+if (!disk-src) {
+umlReportError(VIR_ERR_INTERNAL_ERROR,
+ %s, _(disk source path is missing));
+goto error;
+}
+
+if (virAsprintf(cmd, config %s=%s, disk-dst, disk-src) 0) {
+virReportOOMError();
+return -1;
+}
+
+if (umlMonitorCommand(driver, vm, cmd, reply) 0)
+return -1;
+
+if (VIR_REALLOC_N(vm-def-disks, vm-def-ndisks+1) 0) {
+virReportOOMError();
+goto error;
+}
+
+if (ret 0)
+goto error;
+
+virDomainDiskInsertPreAlloced(vm-def, disk);
+
+VIR_FREE(cmd);
+
+return 0;
+
+error:
+
+VIR_FREE(cmd);
+
+return -1;
+}
+
+
+static int umlDomainAttachDevice(virDomainPtr dom, const char *xml)
+{
+struct uml_driver *driver = dom-conn-privateData;
+virDomainObjPtr vm;
+virDomainDeviceDefPtr dev = NULL;
+int ret = -1;
+
+umlDriverLock(driver);
+
+vm = virDomainFindByUUID(driver-domains, dom-uuid);
+if (!vm) {
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+virUUIDFormat(dom-uuid, uuidstr);
+umlReportError(VIR_ERR_NO_DOMAIN,
+ _(no domain with matching uuid '%s'), uuidstr);
+goto cleanup;
+}
+
+if (!virDomainObjIsActive(vm)) {
+umlReportError(VIR_ERR_OPERATION_INVALID,
+ %s, _(cannot attach device on inactive domain));
+goto cleanup;
+}
+
+dev = virDomainDeviceDefParse(driver-caps, vm-def, xml,
+ VIR_DOMAIN_XML_INACTIVE);
+
+if (dev == NULL)
+goto cleanup;
+
+if (dev-type == VIR_DOMAIN_DEVICE_DISK) {
+if (dev-data.disk-bus == VIR_DOMAIN_DISK_BUS_UML) {
+ret = umlDomainAttachUmlDisk(driver, vm, dev-data.disk);
+if (ret == 0)
+dev-data.disk = NULL;
+} else {
+umlReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _(disk bus '%s' cannot be hotplugged.),
+ virDomainDiskBusTypeToString(dev-data.disk-bus));
+}
+} else {
+umlReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _(device type '%s' cannot be attached),
+ virDomainDeviceTypeToString(dev-type));
+goto cleanup;
+}
+
+cleanup:
+
+virDomainDeviceDefFree(dev);
+if (vm)
+virDomainObjUnlock(vm);
+umlDriverUnlock(driver);
+return ret;
+}
+
+
+static int umlDomainDetachUmlDisk(struct uml_driver *driver,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev)
+{
+int i, ret = -1;
+virDomainDiskDefPtr detach = NULL;
+char *cmd;
+char *reply;
+
+for (i = 0 ; i vm-def-ndisks ; i++) {
+if (STREQ(vm-def-disks[i]-dst, dev-data.disk-dst)) {
+break;
+}
+}
+
+if (i == vm-def-ndisks) {
+umlReportError(VIR_ERR_OPERATION_FAILED,
+ _(disk %s not found), dev-data.disk-dst);
+return -1;
+}
+
+detach = vm-def-disks[i];
+
+if (virAsprintf(cmd, remove %s, detach-dst) 0) {
+virReportOOMError();
+return -1;
+}
+
+if (umlMonitorCommand(driver, vm, cmd, reply) 0)
+goto cleanup;
+
+umlShrinkDisks(vm-def, i);
+
+virDomainDiskDefFree(detach);
+
+ret = 0;
+
+cleanup:
+VIR_FREE(cmd);
+
+return ret;
+}
+
+
+static int umlDomainDetachDevice(virDomainPtr dom, const char *xml) {
+struct uml_driver *driver =
Re: [libvirt] [PATCH] Support virDomainAttachDevice and virDomainDetachDevice for disks in UML
On Thu, Aug 19, 2010 at 02:49:34PM +0200, so...@linux2go.dk wrote:
From: Soren Hansen so...@linux2go.dk
UML supports hot plugging and unplugging of various devices. This patch
exposes this functionality for disks.
Nice, I had no idea they supported that.
Signed-off-by: Soren Hansen so...@linux2go.dk
---
src/uml/uml_driver.c | 223
+-
1 files changed, 221 insertions(+), 2 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 04493ba..a09bc60 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -1686,6 +1686,225 @@ cleanup:
}
+static inline void umlShrinkDisks(virDomainDefPtr def, size_t i)
+{
+if (def-ndisks 1) {
+memmove(def-disks + i,
+def-disks + i + 1,
+sizeof(*def-disks) *
+(def-ndisks - (i + 1)));
+def-ndisks--;
+if (VIR_REALLOC_N(def-disks, def-ndisks) 0) {
+/* ignore, harmless */
+}
+} else {
+VIR_FREE(def-disks);
+def-ndisks = 0;
+}
+}
Since this code is already used in the QEMU driver, I think we
should just put it straight into src/conf/domain_conf.c so we
can share it. 'virDomainDiskRemove' is probably a more accurate
name than ShrinkDisks.
+
+
+static int umlDomainAttachUmlDisk(struct uml_driver *driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
+{
+int i, ret;
+char *cmd = NULL;
+char *reply;
+
+for (i = 0 ; i vm-def-ndisks ; i++) {
+if (STREQ(vm-def-disks[i]-dst, disk-dst)) {
+umlReportError(VIR_ERR_OPERATION_FAILED,
+ _(target %s already exists), disk-dst);
+return -1;
+}
+}
+
+if (!disk-src) {
+umlReportError(VIR_ERR_INTERNAL_ERROR,
+ %s, _(disk source path is missing));
+goto error;
+}
+
+if (virAsprintf(cmd, config %s=%s, disk-dst, disk-src) 0) {
+virReportOOMError();
+return -1;
+}
+
+if (umlMonitorCommand(driver, vm, cmd, reply) 0)
+return -1;
Needs to be a 'goto error' to avoid leaking 'cmd'
+
+if (VIR_REALLOC_N(vm-def-disks, vm-def-ndisks+1) 0) {
+virReportOOMError();
+goto error;
+}
+
+if (ret 0)
+goto error;
+
+virDomainDiskInsertPreAlloced(vm-def, disk);
+
+VIR_FREE(cmd);
+
+return 0;
+
+error:
+
+VIR_FREE(cmd);
+
+return -1;
+}
+dev = virDomainDeviceDefParse(driver-caps, vm-def, xml,
+ VIR_DOMAIN_XML_INACTIVE);
+if (dev == NULL)
+goto cleanup;
+
+if (dev-type == VIR_DOMAIN_DEVICE_DISK
+dev-data.disk-device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+if (dev-data.disk-bus == VIR_DOMAIN_DISK_BUS_UML)
+ret = umlDomainDetachUmlDisk(driver, vm, dev);
+else {
+umlReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+ _(This type of disk cannot be hot unplugged));
+}
+} else {
+umlReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ %s, _(This type of device cannot be hot
unplugged));
+}
+
+cleanup:
+virDomainDeviceDefFree(dev);
+if (vm)
+virDomainObjUnlock(vm);
+umlDriverUnlock(driver);
+return ret;
+}
+
static int umlDomainGetAutostart(virDomainPtr dom,
int *autostart) {
@@ -1900,9 +2119,9 @@ static virDriver umlDriver = {
umlDomainStartWithFlags, /* domainCreateWithFlags */
umlDomainDefine, /* domainDefineXML */
umlDomainUndefine, /* domainUndefine */
-NULL, /* domainAttachDevice */
+umlDomainAttachDevice, /* domainAttachDevice */
NULL, /* domainAttachDeviceFlags */
-NULL, /* domainDetachDevice */
+umlDomainDetachDevice, /* domainDetachDevice */
NULL, /* domainDetachDeviceFlags */
You should also implement the DeviceFlags variants at
the same time. You can just do a dumb wrapper like QEMU
driver does, for simplicity.
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] [PATCH 2/2] qemu: Update next usable PCI slot on domain reconnect
On 08/18/2010 05:26 AM, Daniel P. Berrange wrote: 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. At this point, I'm starting to think that we can just drop this 2/2 patch and not worry about nextslot being stable across libvirtd restarts. My original concern was for a windows vm created against an older qemu, with no hot-plugging support, then being updated to a newer libvirt: there, nextslot must be incremented in the same order when firing up the vm from XML so that windows won't complain. But that only affects start-up; once the guest is up and running, nextslot only matters for hotplugged slots, and based on my problem definition, this was for a VM defined in the days before hotplug support being ported to newer underlying tools. Even if nextslot is reset to 0 after a libvirtd restart, that should only have an impact on future hot-plugged devices, and not any of the original devices reserved by the xml. And if windows can already handle hot-plugging, then it shouldn't matter which device slot a hot-plug occurs on; even if it is a different slot after the libvirtd restart than it would have been if libvirtd had been constantly running. If anyone can prove us wrong with an actual bug report, we can deal with the issue then. But for now, let's just drop this as over-engineering a solution for a non-problem. -- 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] web page suggestion
On Mon, Aug 16, 2010 at 11:29:04AM -0600, Eric Blake wrote: My XSL skills are less than stellar, so I'm throwing this out to the list in case someone else can pick it up and come up with a decent patch in less time. Right now, http://libvirt.org/ChangeLog.html is worthless; it is linked from a couple of other pages, such as http://libvirt.org/news.html. A better place to link would be a live git page: http://libvirt.org/git/?p=libvirt.git;a=log I don't know whether it is easier to update news.html.in and sitemap.html.in to point directly to the new link, or if we should keep ChangeLog.xsl but have it revamped to point to the new link. Just delete ChangeLog.xsl/.html completely. These were needed when we had CVS because there was no online history browser, but we don't need it in the GITWEB enabled world 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] [PATCH] Allow chardev of type 'file' for UML domains.
On Wed, Aug 18, 2010 at 01:35:29PM +0200, so...@linux2go.dk wrote:
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.
Yep, this looks fine.
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 */
Any reason not to let the code deal with PIPE too ? It seems
like the code should work equally well for both PIPE FILE.
(well drop the O_CREATE|O_APPEND - assume the user has got
the pipe pre-created with 'mkfifo'.
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);
I think this leaks file descriptors in libvirtd. There's no existing
code which ever closes the FDs in keepfd after spawning UML later
on in the function.
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
Re: [libvirt] Xen string2sexpr and sexpr2string lose quotes?
On Tue, Aug 17, 2010 at 08:26:09PM -0700, Thomas Graves wrote:
Hello all,
I am running xen on rhel5 and using libvirt0.7.2 (I also tried 0.7.7) and it
looks like the routines string2sexpr and sexpr2string seem to lose the
quotes around the image args in the configuration.
Has anyone seen this and have a patch for this?
I have the following libvirt config:
os
typelinux/type
kernel/usr/lib/xen/boot/pv-grub-x86_64.gz/kernel
cmdline(hd0,0)/grub/menu.lst/cmdline
/os
It generates the xm config info:
(image
(linux
(kernel /usr/lib/xen/boot/pv-grub-x86_64.gz)
(args '(hd0,0)/grub/menu.lst')
(device_model /usr/lib64/xen/bin/qemu-dm)
)
)
I call virDomainSetAutostart on the domain and traced it through and saw
that it gets the string quoted (args '(hd0,0)/grub/menu.lst') from xen then
ends up calling string2sexpr, changes the xend_on_start, and then
sexpr2string, and it ends up without quotes (args (hd0,0)/grub/menu.lst) and
that is what it sends back to xen. Xen then seems to chop it off to (args
('hd0,0'))
Try adding this patch to sexpr2string
index 7e370db..df7057e 100644
--- a/src/xen/sexpr.c
+++ b/src/xen/sexpr.c
@@ -244,7 +244,9 @@ sexpr2string(const struct sexpr * sexpr, char *buffer,
size_t n_buffer)
ret += tmp;
break;
case SEXPR_VALUE:
-if (strchr(sexpr-u.value, ' '))
+if (strchr(sexpr-u.value, ' ') ||
+strchr(sexpr-u.value, ')') ||
+strchr(sexpr-u.value, '('))
tmp = snprintf(buffer + ret, n_buffer - ret, '%s',
sexpr-u.value);
else
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] qemu: Fix JSON migrate_set_downtime command
---
src/qemu/qemu_monitor_json.c |9 ++---
1 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e8609aa..8a586bc 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1481,17 +1481,12 @@ int qemuMonitorJSONSetMigrationDowntime(qemuMonitorPtr
mon,
unsigned long long downtime)
{
int ret;
-char *downtimestr;
virJSONValuePtr cmd;
virJSONValuePtr reply = NULL;
-if (virAsprintf(downtimestr, %llums, downtime) 0) {
-virReportOOMError();
-return -1;
-}
+
cmd = qemuMonitorJSONMakeCommand(migrate_set_downtime,
- s:value, downtimestr,
+ d:value, downtime / 1000.0,
NULL);
-VIR_FREE(downtimestr);
if (!cmd)
return -1;
--
1.7.2
--
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
There's no requirement to plug devices in ascending slot order - we can have gaps at will with any ordering. At this point, I'm starting to think that we can just drop this 2/2 patch and not worry about nextslot being stable across libvirtd restarts. Which means we don't even need most of 1/2 since the reason for changing the hash payload to be a structure instead of a string was this second patch. So what do you think, should I push it as is or make a smaller patch which would just fix OOM checking when PCI addresses are converted to strings? Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Support virDomainAttachDevice and virDomainDetachDevice for disks in UML
On 19-08-2010 15:22, Daniel P. Berrange wrote:
+static inline void umlShrinkDisks(virDomainDefPtr def, size_t i)
+{
+if (def-ndisks 1) {
+memmove(def-disks + i,
+def-disks + i + 1,
+sizeof(*def-disks) *
+(def-ndisks - (i + 1)));
+def-ndisks--;
+if (VIR_REALLOC_N(def-disks, def-ndisks) 0) {
+/* ignore, harmless */
+}
+} else {
+VIR_FREE(def-disks);
+def-ndisks = 0;
+}
+}
Since this code is already used in the QEMU driver, I think we
should just put it straight into src/conf/domain_conf.c so we
can share it. 'virDomainDiskRemove' is probably a more accurate
name than ShrinkDisks.
Sounds good to me. I wasn't sure where to stick it, so I just left it
here and figured you'd probably tell me something like this :) I'll move it.
+
+
+static int umlDomainAttachUmlDisk(struct uml_driver *driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
+{
+int i, ret;
+char *cmd = NULL;
+char *reply;
+
+for (i = 0 ; i vm-def-ndisks ; i++) {
+if (STREQ(vm-def-disks[i]-dst, disk-dst)) {
+umlReportError(VIR_ERR_OPERATION_FAILED,
+ _(target %s already exists), disk-dst);
+return -1;
+}
+}
+
+if (!disk-src) {
+umlReportError(VIR_ERR_INTERNAL_ERROR,
+ %s, _(disk source path is missing));
+goto error;
+}
+
+if (virAsprintf(cmd, config %s=%s, disk-dst, disk-src) 0) {
+virReportOOMError();
+return -1;
+}
+
+if (umlMonitorCommand(driver, vm, cmd, reply) 0)
+return -1;
Needs to be a 'goto error' to avoid leaking 'cmd'
Ah, yes, good catch. I also need to free the reply. :)
@@ -1900,9 +2119,9 @@ static virDriver umlDriver = {
umlDomainStartWithFlags, /* domainCreateWithFlags */
umlDomainDefine, /* domainDefineXML */
umlDomainUndefine, /* domainUndefine */
-NULL, /* domainAttachDevice */
+umlDomainAttachDevice, /* domainAttachDevice */
NULL, /* domainAttachDeviceFlags */
-NULL, /* domainDetachDevice */
+umlDomainDetachDevice, /* domainDetachDevice */
NULL, /* domainDetachDeviceFlags */
You should also implement the DeviceFlags variants at
the same time. You can just do a dumb wrapper like QEMU
driver does, for simplicity.
Right you are. I misunderstood what those were for.
--
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
On Thu, Aug 19, 2010 at 04:57:17PM +0200, Jiri Denemark wrote: There's no requirement to plug devices in ascending slot order - we can have gaps at will with any ordering. At this point, I'm starting to think that we can just drop this 2/2 patch and not worry about nextslot being stable across libvirtd restarts. Which means we don't even need most of 1/2 since the reason for changing the hash payload to be a structure instead of a string was this second patch. So what do you think, should I push it as is or make a smaller patch which would just fix OOM checking when PCI addresses are converted to strings? Yep, lets do a simpler patch 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 19-08-2010 15:41, Daniel P. Berrange wrote: + case VIR_DOMAIN_CHR_TYPE_PIPE: +/* XXX could open the pipe just pass the FDs */ Any reason not to let the code deal with PIPE too ? It seems like the code should work equally well for both PIPE FILE. (well drop the O_CREATE|O_APPEND - assume the user has got the pipe pre-created with 'mkfifo'. Not in particular. I was just in a hurry, wanted to share the patch sooner rather than later and then attack the pipes later on. I'll fix it up, but perhaps I won't get to it today. I think this leaks file descriptors in libvirtd. There's no existing code which ever closes the FDs in keepfd after spawning UML later on in the function. True. I somehow thought virExecWhatever took care of that, but I see now that it doesn't, and I guess that would be troublesome. I'll handle this in the uml driver. -- 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] using sync_manager with libvirt
On Wed, Aug 18, 2010 at 07:44:18PM -0400, Perry Myers wrote: 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. I'm only aware of one goal, and the current plan is to implement it correctly and completely. That goal is to lock vm images so if the vm happens to run on two hosts, only one instance can access the image. It seems unlikely that any other purposes for sync_manager would change how we're planning to protect vm images. Dave -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] New save/restore api proposal
Hello all, I'd like to add support for save and restore to the OpenVZ and VirtualBox drivers because I have to support these operations in the application I'm working on. However, the save/restore API in its current state doesn't fit well to our needs. The main problem is that the domain definition is included inside the save file. This is problematic because between the save and the restore operations, the names of the network interfaces on the host side are likely to have changed and we can't modify them before restoring the domain. To summarize, what we would like to be able to do is: - save a domain and undefine it - update the domain definition to use the new names of host side interfaces - restore the domain This is why I would like to add two new functions with the following signatures (better names are probably needed): int virDomainSaveState(virDomainPtr domain, const char *to); int virDomainRestoreState(virDomainPtr domain, const char *from); The first one would do the same as virDomainSave but without prepending the domain's definition to the save file. The other function would be able to restore a domain saved by the first one. What do you think ? Regards, Jean-Baptiste -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Fix JSON migrate_set_downtime command
On 08/19/2010 08:47 AM, Jiri Denemark wrote:
---
src/qemu/qemu_monitor_json.c |9 ++---
1 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e8609aa..8a586bc 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1481,17 +1481,12 @@ int
qemuMonitorJSONSetMigrationDowntime(qemuMonitorPtr mon,
unsigned long long downtime)
{
int ret;
-char *downtimestr;
virJSONValuePtr cmd;
virJSONValuePtr reply = NULL;
-if (virAsprintf(downtimestr, %llums, downtime) 0) {
-virReportOOMError();
-return -1;
-}
+
cmd = qemuMonitorJSONMakeCommand(migrate_set_downtime,
- s:value, downtimestr,
+ d:value, downtime / 1000.0,
Does d:value correctly handle an unsigned long long argument passed
through varargs? I'm thinking you either need a modifier in
qemuMonitorJSONMakeCommand that knows how to receive long long
arguments, or you need a cast here to pass the correct integer type.
--
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] [PATCH] qemu: Fix JSON migrate_set_downtime command
On 08/19/2010 10:33 AM, Eric Blake wrote:
On 08/19/2010 08:47 AM, Jiri Denemark wrote:
---
src/qemu/qemu_monitor_json.c |9 ++---
1 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e8609aa..8a586bc 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1481,17 +1481,12 @@ int
qemuMonitorJSONSetMigrationDowntime(qemuMonitorPtr mon,
unsigned long long downtime)
{
int ret;
-char *downtimestr;
virJSONValuePtr cmd;
virJSONValuePtr reply = NULL;
-if (virAsprintf(downtimestr, %llums, downtime) 0) {
-virReportOOMError();
-return -1;
-}
+
cmd = qemuMonitorJSONMakeCommand(migrate_set_downtime,
- s:value, downtimestr,
+ d:value, downtime / 1000.0,
Does d:value correctly handle an unsigned long long argument passed
through varargs? I'm thinking you either need a modifier in
qemuMonitorJSONMakeCommand that knows how to receive long long
arguments, or you need a cast here to pass the correct integer type.
Never mind. I was thinking too much of printf's %d. But with
qemuMonitorJSONMakeCommand, d: is a double, and your division by 1000.0
does indeed create a value that passes just fine through varargs.
ACK.
--
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] using sync_manager with libvirt
On Thu, Aug 19, 2010 at 11:12:25AM -0400, David Teigland wrote: I'm only aware of one goal, and the current plan is to implement it correctly and completely. That goal is to lock vm images so if the vm happens to run on two hosts, only one instance can access the image. (That's slightly misleading; technically, the lock prevents a second qemu process from even being started.) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/10] nwfilter: use consistent OOM reporting
libvir-list-boun...@redhat.com wrote on 08/18/2010 07:45:05 PM:
libvir-list-boun...@redhat.com
* 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);
ACK.
Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/10] nwfilter: use consistent OOM reporting
On 08/19/2010 12:41 PM, Stefan Berger wrote: libvir-list-boun...@redhat.com wrote on 08/18/2010 07:45:05 PM: libvir-list-boun...@redhat.com * src/nwfilter/nwfilter_driver.c (nwfilterLog): Delete. (nwfilterDriverStartup): Use virReportOOMError instead. ACK. Thanks; pushed. -- 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] [PATCH 1/7] Change calling conventions in remote driver client internals
On 08/17/2010 11:02 AM, Daniel P. Berrange wrote:
The remoteIO() method has wierd calling conventions, where
it is passed a pre-allocated 'struct remote_call *' but
then free()s it itself, instead of letting the caller free().
This fixes those wierd semantics
s/wierd/weird/g
* src/remote/remote_driver.c: Santize semantics of remoteIO
s/Santize/Sanitize/
method wrt to memory release
---
src/remote/remote_driver.c | 25 +
1 files changed, 13 insertions(+), 12 deletions(-)
@@ -10021,6 +10019,7 @@ call (virConnectPtr conn, struct private_data *priv,
xdrproc_t ret_filter, char *ret)
{
struct remote_thread_call *thiscall;
+int rv;
Any reason you used 'ret' in some hunks, but 'rv' in this one?
$ git grep 'return rv;' | wc
167 5017084
$ git grep 'return res;' | wc
12 36 518
$ git grep 'return ret;' | wc
12503838 49749
Consistency argues for naming it 'ret'.
ACK with that nit fixed.
--
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] [PATCH 2/7] Support callbacks on virStream APIs in remote driver client
On 08/17/2010 11:02 AM, Daniel P. Berrange wrote:
The current remote driver code for streams only supports
blocking I/O mode. This is fine for the usage with migration
but is a problem for more general use cases, in particular
bi-directional streams.
This adds supported for the stream callbacks and non-blocking
I/O. with the minor caveat is that it doesn't actually do
non-blocking I/O for sending stream data, only receiving it.
A future patch will try todo non-blocking sends, but this is
s/todo/to do/
quite tricky to get right.
* src/remote/remote_driver.c: Allow non-blocking I/O for
streams and support callbacks
---
src/remote/remote_driver.c | 188
1 files changed, 172 insertions(+), 16 deletions(-)
+static void
+remoteStreamEventTimer(int timer ATTRIBUTE_UNUSED, void *opaque)
+{
+virStreamPtr st = opaque;
+struct private_data *priv = st-conn-privateData;
+struct private_stream_data *privst = st-privateData;
+
+remoteDriverLock(priv);
+if (privst-cb
+(privst-cbEvents VIR_STREAM_EVENT_READABLE)
+privst-incomingOffset) {
+virStreamEventCallback cb = privst-cb;
+void *cbOpaque = privst-cbOpaque;
+virFreeCallback cbFree = privst-cbFree;
+
+privst-cbDispatch = 1;
+remoteDriverUnlock(priv);
+(cb)(st, VIR_STREAM_EVENT_READABLE, cbOpaque);
Do we have/want a return value from this callback? If so, what would we
do about a non-zero return value?
+remoteDriverLock(priv);
+privst-cbDispatch = 0;
+
+if (!privst-cb cbFree)
Can never be true - privst-cb had to be non-NULL 12 lines earlier to
get to this point. I think you meant just 'if (cbFree)'.
+(cbFree)(cbOpaque);
Any reason that cp is called while the driver is unlocked, but cbFree is
called while the lock is still held? It seems like if we are worried
that the callbacks might deadlock if we still hold the driver lock, then
we should treat both callbacks in the same manner.
Also, any reason you use (cb)() instead of the simpler cp() calling
convention?
static int
-remoteStreamEventRemoveCallback(virStreamPtr stream ATTRIBUTE_UNUSED)
+remoteStreamEventRemoveCallback(virStreamPtr st)
{
-return -1;
+struct private_data *priv = st-conn-privateData;
+struct private_stream_data *privst = st-privateData;
+int ret = -1;
+
+remoteDriverLock(priv);
+
+if (!privst-cb) {
+remoteError(VIR_ERR_INTERNAL_ERROR,
+_(no stream callback registered));
+goto cleanup;
+}
+
+if (!privst-cbDispatch
+privst-cbFree)
+(privst-cbFree)(privst-cbOpaque);
Depending on whether you feel the callback should be run without the
driver lock, this might need changing.
--
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] [PATCH 3/7] Introduce a virDomainOpenConsole API
On 08/17/2010 11:02 AM, Daniel P. Berrange wrote:
To enable virsh console (or equivalent) to be used remotely
it is neccessary to provide remote access to the /dev/pts/XXX
s/neccessary/necessary
psuedo-TTY associated with the console/serial/parallel device
s/psuedo/pseudo/
in the guest. The virStream API provide a bi-directional I/O
stream capability that can be used for this purpose. This
patch thus introduces a virDomainOpenConsole API that uses
the stream APIs.
* src/libvirt.c, src/libvirt_public.syms,
include/libvirt/libvirt.h.in, src/driver.h: Define the
new virDomainOpenConsole API
* src/esx/esx_driver.c, src/lxc/lxc_driver.c,
src/opennebula/one_driver.c, src/openvz/openvz_driver.c,
src/phyp/phyp_driver.c, src/qemu/qemu_driver.c,
src/remote/remote_driver.c, src/test/test_driver.c,
src/uml/uml_driver.c, src/vbox/vbox_tmpl.c,
src/xen/xen_driver.c, src/xenapi/xenapi_driver.c: Stub
API entry point
index 4fb357b..648e4fe 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -4318,6 +4318,7 @@ static virDriver esxDriver = {
esxDomainRevertToSnapshot, /* domainRevertToSnapshot */
esxDomainSnapshotDelete, /* domainSnapshotDelete */
NULL,/* qemuDomainMonitorCommand */
+NULL, /* domainOpenConsole */
Consistent indentation of the comment. (It's a shame that we aren't
consistent between the various device drivers, so that you can't just
copy and paste a new NULL line across all of them.)
+/**
+ * virDomainOpenConsole:
+ * @dom: the domain whose console to open
Reads awkwardly. How about:
the domain where a console will be opened
+++ b/src/libvirt_public.syms
@@ -405,4 +405,9 @@ LIBVIRT_0.8.2 {
virDomainCreateWithFlags;
} LIBVIRT_0.8.1;
+LIBVIRT_0.8.3 {
+global:
+virDomainOpenConsole;
+} LIBVIRT_0.8.2;
0.8.3 is already out; this needs bumping to 0.8.4.
ACK with those nits fixed.
--
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] [PATCH 4/7] Remote driver client and server for virDomainOpenConsole
On 08/17/2010 11:02 AM, Daniel P. Berrange wrote:
This provides an implementation of the virDomainOpenConsole
API for the remote driver client and server.
* daemon/remote.c: Server side impl
* src/remote/remote_driver.c: Client impl
* src/remote/remote_protocol.x: Wire definition
---
daemon/remote.c | 52 +
daemon/remote_dispatch_args.h |1 +
daemon/remote_dispatch_prototypes.h |8 +++
daemon/remote_dispatch_table.h |5 ++
src/remote/remote_driver.c | 108 --
src/remote/remote_protocol.c| 13
src/remote/remote_protocol.h| 10 +++
src/remote/remote_protocol.x|8 ++-
8 files changed, 172 insertions(+), 33 deletions(-)
No change to src/remote_protocol-structs? Install the dwarves package;
this will double-check that you aren't breaking any existing APIs, but
it will flag that this new call is an API addition worthy of an update
to src/remote_protocol-structs.
@@ -9665,8 +9709,8 @@ processCallDispatchStream(virConnectPtr conn
ATTRIBUTE_UNUSED,
privst = privst-next;
if (!privst) {
-VIR_WARN(No registered stream matching serial=%d, proc=%d,
- hdr-serial, hdr-proc);
+VIR_DEBUG(No registered stream matching serial=%d, proc=%d,
+ hdr-serial, hdr-proc);
Quite a few conversions from VIR_WARN to VIR_DEBUG in this patch.
Should they be split into a separate patch, since they are independent
of the new command plumbing?
--
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] [PATCH 5/7] Support virDomainOpenConsole with QEMU
On 08/17/2010 11:02 AM, Daniel P. Berrange wrote:
This provides an implementation of the virDomainOpenConsole
API with the QEMU driver. For the streams code, this reuses
most of the code previously added for the tunnelled migration
streams since it is generic.
* src/qemu/qemu_driver.c: Support virDomainOpenConsole
---
src/qemu/qemu_driver.c | 267 +--
1 files changed, 210 insertions(+), 57 deletions(-)
+static int qemuStreamFDRead(virStreamPtr st, char *bytes, size_t nbytes)
+{
...
+} else {
+ret = -1;
+virReportSystemError(errno, %s,
+ _(cannot write to stream));
s/write to/read from/
+
+static int
+qemuDomainOpenConsole(virDomainPtr dom,
+ const char *devname,
+ virStreamPtr st,
+ unsigned int flags ATTRIBUTE_UNUSED)
Drop the attribute...
+{
+struct qemud_driver *driver = dom-conn-privateData;
+virDomainObjPtr vm = NULL;
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+int ret = -1;
+int i;
+virDomainChrDefPtr chr = NULL;
+struct qemuStreamFD *qemust;
...and add virCheckFlags(0, -1) here.
+if (devname) {
+if (vm-def-console
+STREQ(devname, vm-def-console-info.alias))
+chr = vm-def-console;
+for (i = 0 ; !chr i vm-def-nserials ; i++) {
+if (STREQ(devname, vm-def-serials[i]-info.alias))
+chr = vm-def-serials[i];
+}
+for (i = 0 ; !chr i vm-def-nparallels ; i++) {
+if (STREQ(devname, vm-def-parallels[i]-info.alias))
+chr = vm-def-parallels[i];
+}
+} else {
+if (vm-def-console)
+chr = vm-def-console;
+else if (vm-def-nserials)
+chr = vm-def-serials[0];
+}
Missing the check for vm-dev-nparallels when devname is NULL and there
is no console or serial devices?
ACK with those nits fixed.
--
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] [PATCH 01/10] uml: fix logic bug in checking reply length
2010/8/19 Eric Blake ebl...@redhat.com:
* 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)) {
You could reverse the order to
nbytes offsetof(struct monitor_request, data) + res.length
to be in line with the layout of the data, but that's just me nit-picking here.
ACK.
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/10] build: delete dead comment
2010/8/19 Eric Blake ebl...@redhat.com:
* 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;
This line even got a typo fix 2 years ago :)
ACK.
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/7] Re-write virsh console to use streams
On 08/17/2010 11:02 AM, Daniel P. Berrange wrote:
This re-writes the 'virsh console' command so that it uses
the new streams API. This lets it run remotely and/or as a
non-root user. This requires that virsh be linked against
the simple event loop from libvirtd in daemon/event.c
As an added bonus, it can now connect to any console device,
not just the first one.
* tools/Makefile.am: Link to event.c
* tools/console.c, tools/console.h: Rewrite to use the
virDomainOpenConsole() APIs with streams
* tools/virsh.c: Support choosing the console name
via --devname $NAME
---
tools/Makefile.am |1 +
tools/console.c | 330
-
tools/console.h |2 +-
tools/virsh.c | 76 -
4 files changed, 274 insertions(+), 135 deletions(-)
+
+struct virConsoleBuffer {
+size_t length;
+size_t offset;
s/size_t/off_t/ for offset? (but see below)
+char *data;
+};
+
+typedef struct virConsole virConsole;
+typedef virConsole *virConsolePtr;
+struct virConsole {
+virStreamPtr st;
+int quit;
s/int/bool/, and adjust clients to use false/true instead of 0/1
+
+int stdinWatch;
+int stdoutWatch;
+
+struct virConsoleBuffer streamToTerminal;
+struct virConsoleBuffer terminalToStream;
+};
+
static int got_signal = 0;
static void do_signal(int sig ATTRIBUTE_UNUSED) {
got_signal = 1;
@@ -61,22 +86,192 @@ cfmakeraw (struct termios *attr)
}
# endif /* !HAVE_CFMAKERAW */
-int vshRunConsole(const char *tty) {
-int ttyfd, ret = -1;
+static void
+virConsoleEventOnStream(virStreamPtr st,
+int events, void *opaque)
+{
+virConsolePtr con = opaque;
+
+if (events VIR_STREAM_EVENT_READABLE) {
+size_t avail = con-streamToTerminal.length -
+con-streamToTerminal.offset;
Oh, never mind. size_t for offset is probably just fine after all.
+int got;
+
+if (avail 1024) {
+if (VIR_REALLOC_N(con-streamToTerminal.data,
+ con-streamToTerminal.length + 1024) 0) {
/me Note to self - another VIR_EXPAND_N or VIR_RESIZE_N candidate.
+virReportOOMError();
+con-quit = 1;
+return;
+}
+con-streamToTerminal.length += 1024;
+avail += 1024;
+}
+
+got = virStreamRecv(st,
+con-streamToTerminal.data +
+con-streamToTerminal.offset,
+avail);
+if (got == -2)
+return; /* blocking */
+if (got = 0) {
+con-quit = 1;
+return;
+}
+con-streamToTerminal.offset += got;
+if (con-streamToTerminal.offset)
+virEventUpdateHandleImpl(con-stdoutWatch,
+ VIR_EVENT_HANDLE_WRITABLE);
+}
+
+if (events VIR_STREAM_EVENT_WRITABLE
+con-terminalToStream.offset) {
+ssize_t done;
+size_t avail;
+done = virStreamSend(con-st,
+ con-terminalToStream.data,
+ con-terminalToStream.offset);
+if (done == -2)
+return; /* blocking */
+if (done 0) {
+virErrorPtr err = virGetLastError();
+con-quit = 1;
+return;
+}
+memmove(con-terminalToStream.data,
+con-terminalToStream.data + done,
+con-terminalToStream.offset - done);
+con-terminalToStream.offset -= done;
+
+avail = con-terminalToStream.length - con-terminalToStream.offset;
+if (avail 1024) {
+if (VIR_REALLOC_N(con-terminalToStream.data,
+ con-terminalToStream.offset + 1024) 0)
+{}
/me Note to self - a VIR_SHRINK_N candidate.
+con-terminalToStream.length = con-terminalToStream.offset +
1024;
+}
+}
+if (!con-terminalToStream.offset)
+virStreamEventUpdateCallback(con-st,
+ VIR_STREAM_EVENT_READABLE);
+
+if (events VIR_STREAM_EVENT_ERROR ||
+events VIR_STREAM_EVENT_HANGUP) {
+con-quit = 1;
+}
+}
+
+static void
+virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
+ int fd ATTRIBUTE_UNUSED,
+ int events,
+ void *opaque)
+{
+virConsolePtr con = opaque;
+
+if (events VIR_EVENT_HANDLE_READABLE) {
+size_t avail = con-terminalToStream.length -
+con-terminalToStream.offset;
+int got;
+
+if (avail 1024) {
+if (VIR_REALLOC_N(con-terminalToStream.data,
+ con-terminalToStream.length + 1024) 0) {
+virReportOOMError();
+con-quit =
Re: [libvirt] [PATCH 05/10] storage: avoid s[n]printf
2010/8/19 Eric Blake ebl...@redhat.com: * 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. Nice one, before it would just have executed parted with a random string in the argumentlist in that case. ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/10] squash to dead comment
2010/8/19 Eric Blake ebl...@redhat.com:
* 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
Disabled two times :)
ACK.
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/10] xenapi: avoid sprintf
2010/8/19 Eric Blake ebl...@redhat.com: * 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. ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/7] Fix busy-wait loop on closed file descriptor
On 08/17/2010 11:02 AM, Daniel P. Berrange wrote: When closing open streams after a client quits, the event callback was not removed. This mean that poll() was using a closed FD and returning POLLNVAL in a busy-wait loop. * daemon/stream.c: Disconnect stream callbacks --- daemon/stream.c |7 ++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/daemon/stream.c b/daemon/stream.c index d64fe73..cac54ea 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -108,6 +108,7 @@ remoteStreamEvent(virStreamPtr st, int events, void *opaque) remote_error rerr; memset(rerr, 0, sizeof rerr); stream-closed = 1; +virStreamEventRemoveCallback(stream-st); virStreamAbort(stream-st); ACK. -- 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] [PATCH 08/10] vbox: add location used in rpmfusion release
2010/8/19 Eric Blake ebl...@redhat.com: * configure.ac (vbox_xpcomc_dir): Add another potential dir. --- To test that my vbox changes would still compile, I had to install vbox. But VirtualBox-OSE-3.2.6-2.fc13.x86_64 from rpmfusion-free installs to a new location. configure.ac | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/configure.ac b/configure.ac index 3968617..d42d9e6 100644 --- a/configure.ac +++ b/configure.ac @@ -336,6 +336,7 @@ if test x$with_vbox = xyes || test x$with_vbox = xcheck; then for vbox in \ /usr/lib/virtualbox/VBoxXPCOMC.so \ + /usr/lib64/virtualbox/VBoxXPCOMC.so \ /usr/lib/VirtualBox/VBoxXPCOMC.so \ /opt/virtualbox/VBoxXPCOMC.so \ /opt/VirtualBox/VBoxXPCOMC.so \ You could have tricked configure instead of actually installing VirtualBox, but this way had an additional benefit :) ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/10] vbox: factor a large function
2010/8/19 Eric Blake ebl...@redhat.com: * 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. Looks fine and I can still start a VirtualBox guest after applying this patch. vboxDomainDefineXML is even worse, and contains the remaining sprintf instances in this file; I'll probably try to factor that one down as well. Yes, unfortunately the driver code has many indentation levels and large functions in several places. ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Add support for Vendor and Model in Storage Pool XML
On 08/17/2010 11:44 AM, Patrick Dignan wrote:
+if (src-product != NULL) {
+virBufferVSprintf(buf,product name='%s'/\n, src-product);
+}
+
virBufferAddLit(buf,/source\n);
The only change is that these should use virBufferEscapeString
since vendor/product strings might contain, or characters
which would make the XML unhappy
Regards,
Daniel
Ok, so I've updated that. Hopefully all is well with this patch!
Almost. The .rng file had some whitespace issues. Also, the
sourcefmtfs reference only occurs as part of the sourcefs element, so
only the latter needed the optional sourceinfovendor. And 'make
syntax-check' didn't like your use of the the in a comment nor your
trailing spaces.
Meanwhile, you missed documentation in docs/formatstorage.html.in. I'm
getting more serious about blocking patches to docs/schemas without the
corresponding changes to docs/format*html.in, because even if it's hard
to do up front, it's much harder to do long after the fact; but until we
have a strong precedence of that action in the git history, I can see
how you overlooked it.
But those were minor enough to have my ACK; I didn't mind touching this
up since it was your first submission, but hopefully don't have to spend
as much cleanup work on future patches from you. Here's what I squashed
in before pushing (along with an AUTHORS update not listed here).
diff --git i/docs/formatstorage.html.in w/docs/formatstorage.html.in
index 5c1d36c..91f70a3 100644
--- i/docs/formatstorage.html.in
+++ w/docs/formatstorage.html.in
@@ -70,6 +70,8 @@
lt;sourcegt;
lt;host name=iscsi.example.com/gt;
lt;device path=demo-target/gt;
+ lt;vendor name=Acme/gt;
+ lt;product name=model/t;
lt;/sourcegt;
.../pre
@@ -108,6 +110,16 @@
type, or network filesystem type, or partition table type, or
LVM metadata type. All drivers are required to have a default
value for this, so it is optional. span class=sinceSince
0.4.1/span/dd
+
+ dtcodevendor/code/dt
+ ddProvides optional information about the vendor of the
+storage device. This contains a single
+attribute codename/code whose value is backend
+specific. span class=sinceSince 0.8.4/span/dd
+ dtcodeproduct/code/dt
+ ddProvides an optional product name of the storage device.
+This contains a single attribute codename/code whose value
+is backend specific. span class=sinceSince 0.8.4/span/dd
/dl
h3a name=StoragePoolTargetTarget elements/a/h3
diff --git i/docs/schemas/storagepool.rng w/docs/schemas/storagepool.rng
index a8a3f36..54eb802 100644
--- i/docs/schemas/storagepool.rng
+++ w/docs/schemas/storagepool.rng
@@ -289,9 +289,6 @@
valueocfs2/value
/choice
/attribute
-optional
- ref name='sourceinfovendor'/
-/optional
/element
/optional
/define
@@ -371,7 +368,7 @@
ref name='sourceinfodev'/
ref name='sourcefmtfs'/
optional
- ref name='sourceinfovendor'/
+ref name='sourceinfovendor'/
/optional
/element
/define
@@ -382,7 +379,7 @@
ref name='sourceinfodir'/
ref name='sourcefmtnetfs'/
optional
- ref name='sourceinfovendor'/
+ref name='sourceinfovendor'/
/optional
/element
/define
@@ -399,7 +396,7 @@
/oneOrMore
ref name='sourcefmtlogical'/
optional
- ref name='sourceinfovendor'/
+ref name='sourceinfovendor'/
/optional
/element
/define
@@ -408,8 +405,8 @@
element name='source'
ref name='sourceinfodev'/
ref name='sourcefmtdisk'/
- optional
- ref name='sourceinfovendor'/
+ optional
+ref name='sourceinfovendor'/
/optional
/element
/define
@@ -425,7 +422,7 @@
ref name='sourceinfoauth'/
/optional
optional
- ref name='sourceinfovendor'/
+ref name='sourceinfovendor'/
/optional
/element
/define
@@ -434,7 +431,7 @@
element name='source'
ref name='sourceinfoadapter'/
optional
- ref name='sourceinfovendor'/
+ref name='sourceinfovendor'/
/optional
/element
diff --git i/src/conf/storage_conf.h w/src/conf/storage_conf.h
index 5f17b5a..fef0a46 100644
--- i/src/conf/storage_conf.h
+++ w/src/conf/storage_conf.h
@@ -237,7 +237,7 @@ struct _virStoragePoolSource {
virStoragePoolAuthChap chap;
} auth;
-/* Vendor of the the source */
+/* Vendor of the source */
char *vendor;
/* Product name of the source*/
diff --git i/src/conf/storage_conf.c w/src/conf/storage_conf.c
index cf6271e..168243f 100644
--- i/src/conf/storage_conf.c
+++ w/src/conf/storage_conf.c
@@ -844,14 +844,14 @@ virStoragePoolSourceFormat(virBufferPtr buf,
src-auth.chap.login,
Re: [libvirt] [PATCH 10/10] vbox: avoid sprintf
2010/8/19 Eric Blake ebl...@redhat.com:
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();
Why don't we abort the try to start the guest when hitting OOM?
Starting the guest will probably fail because of OOM anyway.
Also why switch to virAsprintf when snprintf would work too?
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/10] maint: whitespace cleanups
On 08/19/2010 02:59 PM, Matthias Bolte wrote:
2010/8/19 Eric Blake ebl...@redhat.com:
* 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) {
While touching this code add a space between if and (.
ACK.
Done for several instance in that file, and pushed.
--
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] [PATCH 05/10] storage: avoid s[n]printf
On 08/19/2010 03:10 PM, Matthias Bolte wrote: 2010/8/19 Eric Blake ebl...@redhat.com: * 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. Nice one, before it would just have executed parted with a random string in the argumentlist in that case. ACK. Thanks; pushed. -- 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] [PATCH 07/10] xenapi: avoid sprintf
On 08/19/2010 03:14 PM, Matthias Bolte wrote: 2010/8/19 Eric Blake ebl...@redhat.com: * 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. ACK. Thanks; pushed. -- 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] [PATCH 08/10] vbox: add location used in rpmfusion release
On 08/19/2010 03:17 PM, Matthias Bolte wrote: 2010/8/19 Eric Blake ebl...@redhat.com: * configure.ac (vbox_xpcomc_dir): Add another potential dir. --- To test that my vbox changes would still compile, I had to install vbox. But VirtualBox-OSE-3.2.6-2.fc13.x86_64 from rpmfusion-free installs to a new location. configure.ac |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/configure.ac b/configure.ac index 3968617..d42d9e6 100644 --- a/configure.ac +++ b/configure.ac @@ -336,6 +336,7 @@ if test x$with_vbox = xyes || test x$with_vbox = xcheck; then for vbox in \ /usr/lib/virtualbox/VBoxXPCOMC.so \ +/usr/lib64/virtualbox/VBoxXPCOMC.so \ /usr/lib/VirtualBox/VBoxXPCOMC.so \ /opt/virtualbox/VBoxXPCOMC.so \ /opt/VirtualBox/VBoxXPCOMC.so \ You could have tricked configure instead of actually installing VirtualBox, but this way had an additional benefit :) ACK. Thanks; pushed. -- 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] [PATCH 09/10] vbox: factor a large function
On 08/19/2010 03:44 PM, Matthias Bolte wrote: 2010/8/19 Eric Blake ebl...@redhat.com: * 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. Looks fine and I can still start a VirtualBox guest after applying this patch. vboxDomainDefineXML is even worse, and contains the remaining sprintf instances in this file; I'll probably try to factor that one down as well. Yes, unfortunately the driver code has many indentation levels and large functions in several places. ACK. Thanks; pushed. I'm working on splitting vboxDomainDefineXML before tackling the sprintf uses in this file. -- 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] [PATCH 10/10] vbox: avoid sprintf
On 08/19/2010 04:14 PM, Matthias Bolte wrote:
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();
Why don't we abort the try to start the guest when hitting OOM?
Starting the guest will probably fail because of OOM anyway.
The existing code didn't worry about it, but you are probably right that
we might as well give up harder on the first OOM.
Also why switch to virAsprintf when snprintf would work too?
Even this sprintf is currently safe (the %.24s fits into the length
allocated for guiDisplay), but fragile and arbitrarily limited. That
is, the s[n]printf solution locks you into a particular length, and has
to be revisited if someone ever has a reason why a 24-character display
string is too short (and I can totally see someone complaining that
their 25-byte FQDN is a reason to support a longer DISPLAY). At least
snprintf only has one place to touch (the array declaration length)
instead of two with sprintf (the array declaration and the %.24s in the
format string), but virAsprintf gets rid of the length limitation once
and for all.
But I'll be reposting the patch after refactoring the other three
instances in the file, so you can wait for a v2 of this patch.
--
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
