[libvirt] [PATCH v4] Add support for fd: protocol
by Corey Bryant
sVirt provides SELinux MAC isolation for Qemu guest processes and their
corresponding resources (image files). sVirt provides this support
by labeling guests and resources with security labels that are stored
in file system extended attributes. Some file systems, such as NFS, do
not support the extended attribute security namespace, which is needed
for image file isolation when using the sVirt SELinux security driver
in libvirt.
The proposed solution entails a combination of Qemu, libvirt, and
SELinux patches that work together to isolate multiple guests' images
when they're stored in the same NFS mount. This results in an
environment where sVirt isolation and NFS image file isolation can both
be provided.
Currently, Qemu opens an image file in addition to performing the
necessary read and write operations. The proposed solution will move
the open out of Qemu and into libvirt. Once libvirt opens an image
file for the guest, it will pass the file descriptor to Qemu via a
new fd: protocol.
If the image file resides in an NFS mount, the following SELinux policy
changes will provide image isolation:
- A new SELinux boolean is created (e.g. virt_read_write_nfs) to
allow Qemu (svirt_t) to only have SELinux read and write
permissions on nfs_t files
- Qemu (svirt_t) also gets SELinux use permissions on libvirt
(virtd_t) file descriptors
Following is a sample invocation of Qemu using the fd: protocol on
the command line:
qemu -drive file=fd:4,format=qcow2
The fd: protocol is also supported by the drive_add monitor command.
This requires that the specified file descriptor is passed to the
monitor alongside a prior getfd monitor command.
This patch also supports the following features for the fd: protocol:
- -snapshot command line option
- savevm monitor command
This patch does not contain support for the following features, all
of which are planned to be supported in the future:
- Copy-on-write backing files
- snapshot_blkdev monitor command
- -cdrom command line option
- -drive command line option with media=cdrom
- change monitor command
v2:
- Add drive_add monitor command support
- Fence off unsupported features that re-open image file
v3:
- Fence off cdrom and change monitor command support
v4:
- Removed checks that fenced off features for fd: protocol
- Enabled -snapshot and savevm support
Signed-off-by: Corey Bryant <coreyb(a)linux.vnet.ibm.com>
---
block.c | 9 ++---
block/raw-posix.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++----
migration-fd.c | 2 +-
monitor.c | 39 ++++++++++++--------
monitor.h | 3 +-
net.c | 2 +-
qemu-options.hx | 8 +++--
qemu-tool.c | 5 +++
8 files changed, 131 insertions(+), 36 deletions(-)
diff --git a/block.c b/block.c
index 785c88e..e9e5613 100644
--- a/block.c
+++ b/block.c
@@ -555,7 +555,6 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
if (flags & BDRV_O_SNAPSHOT) {
BlockDriverState *bs1;
int64_t total_size;
- int is_protocol = 0;
BlockDriver *bdrv_qcow2;
QEMUOptionParameter *options;
char tmp_filename[PATH_MAX];
@@ -573,19 +572,17 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
}
total_size = bdrv_getlength(bs1) & BDRV_SECTOR_MASK;
- if (bs1->drv && bs1->drv->protocol_name)
- is_protocol = 1;
-
bdrv_delete(bs1);
get_tmp_filename(tmp_filename, sizeof(tmp_filename));
/* Real path is meaningless for protocols */
- if (is_protocol)
+ if (path_has_protocol(filename)) {
snprintf(backing_filename, sizeof(backing_filename),
"%s", filename);
- else if (!realpath(filename, backing_filename))
+ } else if (!realpath(filename, backing_filename)) {
return -errno;
+ }
bdrv_qcow2 = bdrv_find_format("qcow2");
options = parse_option_parameters("", bdrv_qcow2->create_options, NULL);
diff --git a/block/raw-posix.c b/block/raw-posix.c
index c5c9944..42ae2f4 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -28,6 +28,7 @@
#include "block_int.h"
#include "module.h"
#include "block/raw-posix-aio.h"
+#include "monitor.h"
#ifdef CONFIG_COCOA
#include <paths.h>
@@ -185,7 +186,8 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
int bdrv_flags, int open_flags)
{
BDRVRawState *s = bs->opaque;
- int fd, ret;
+ int fd = -1;
+ int ret;
ret = raw_normalize_devicepath(&filename);
if (ret != 0) {
@@ -207,13 +209,21 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
if (!(bdrv_flags & BDRV_O_CACHE_WB))
s->open_flags |= O_DSYNC;
- s->fd = -1;
- fd = qemu_open(filename, s->open_flags, 0644);
- if (fd < 0) {
- ret = -errno;
- if (ret == -EROFS)
- ret = -EACCES;
- return ret;
+ if (s->fd == -1) {
+ fd = qemu_open(filename, s->open_flags, 0644);
+ if (fd < 0) {
+ ret = -errno;
+ if (ret == -EROFS) {
+ ret = -EACCES;
+ }
+ return ret;
+ }
+ } else {
+ fd = dup(s->fd);
+ if (fd < 0) {
+ ret = -errno;
+ return ret;
+ }
}
s->fd = fd;
s->aligned_buf = NULL;
@@ -271,6 +281,7 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
{
BDRVRawState *s = bs->opaque;
+ s->fd = -1;
s->type = FTYPE_FILE;
return raw_open_common(bs, filename, flags, 0);
}
@@ -904,6 +915,74 @@ static BlockDriver bdrv_file = {
.create_options = raw_create_options,
};
+static int raw_open_fd(BlockDriverState *bs, const char *filename, int flags)
+{
+ BDRVRawState *s = bs->opaque;
+ const char *fd_str;
+ int fd;
+
+ /* extract the file descriptor - fail if it's not fd: */
+ if (!strstart(filename, "fd:", &fd_str)) {
+ return -EINVAL;
+ }
+
+ if (!qemu_isdigit(fd_str[0])) {
+ /* get fd from monitor, but don't remove from monitor's fd list */
+ fd = qemu_get_fd(fd_str, 0);
+ if (fd == -1) {
+ return -EBADF;
+ }
+ } else {
+ char *endptr = NULL;
+
+ fd = strtol(fd_str, &endptr, 10);
+ if (*endptr || (fd == 0 && fd_str == endptr)) {
+ return -EBADF;
+ }
+ }
+
+ s->fd = fd;
+ s->type = FTYPE_FILE;
+
+ return raw_open_common(bs, filename, flags, 0);
+}
+
+static void raw_close_fd(BlockDriverState *bs)
+{
+ const char *fd_str;
+
+ /* if file descriptor is an fdname, remove it from monitor's fd list */
+ if (strstart(bs->filename, "fd:", &fd_str)) {
+ if (!qemu_isdigit(fd_str[0])) {
+ qemu_get_fd(fd_str, 1);
+ }
+ }
+
+ raw_close(bs);
+}
+
+static BlockDriver bdrv_file_fd = {
+ .format_name = "file",
+ .protocol_name = "fd",
+ .instance_size = sizeof(BDRVRawState),
+ .bdrv_probe = NULL, /* no probe for protocols */
+ .bdrv_file_open = raw_open_fd,
+ .bdrv_read = raw_read,
+ .bdrv_write = raw_write,
+ .bdrv_close = raw_close_fd,
+ .bdrv_flush = raw_flush,
+ .bdrv_discard = raw_discard,
+
+ .bdrv_aio_readv = raw_aio_readv,
+ .bdrv_aio_writev = raw_aio_writev,
+ .bdrv_aio_flush = raw_aio_flush,
+
+ .bdrv_truncate = raw_truncate,
+ .bdrv_getlength = raw_getlength,
+
+ .create_options = raw_create_options,
+};
+
/***********************************************/
/* host device */
@@ -1012,6 +1091,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
}
#endif
+ s->fd = -1;
s->type = FTYPE_FILE;
#if defined(__linux__)
{
@@ -1184,6 +1264,7 @@ static int floppy_open(BlockDriverState *bs, const char *filename, int flags)
BDRVRawState *s = bs->opaque;
int ret;
+ s->fd = -1;
s->type = FTYPE_FD;
/* open will not fail even if no floppy is inserted, so add O_NONBLOCK */
@@ -1302,6 +1383,7 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
{
BDRVRawState *s = bs->opaque;
+ s->fd = -1;
s->type = FTYPE_CD;
/* open will not fail even if no CD is inserted, so add O_NONBLOCK */
@@ -1527,6 +1609,7 @@ static void bdrv_file_init(void)
* Register all the drivers. Note that order is important, the driver
* registered last will get probed first.
*/
+ bdrv_register(&bdrv_file_fd);
bdrv_register(&bdrv_file);
bdrv_register(&bdrv_host_device);
#ifdef __linux__
diff --git a/migration-fd.c b/migration-fd.c
index aee690a..b1ba625 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -61,7 +61,7 @@ MigrationState *fd_start_outgoing_migration(Monitor *mon,
s = g_malloc0(sizeof(*s));
- s->fd = monitor_get_fd(mon, fdname);
+ s->fd = monitor_get_fd(mon, fdname, 1);
if (s->fd == -1) {
DPRINTF("fd_migration: invalid file descriptor identifier\n");
goto err_after_alloc;
diff --git a/monitor.c b/monitor.c
index 39791dc..d03e762 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1198,21 +1198,21 @@ static int add_graphics_client(Monitor *mon, const QDict *qdict, QObject **ret_d
qerror_report(QERR_DEVICE_NOT_ACTIVE, "spice");
return -1;
}
- qerror_report(QERR_ADD_CLIENT_FAILED);
- return -1;
+ qerror_report(QERR_ADD_CLIENT_FAILED);
+ return -1;
#ifdef CONFIG_VNC
} else if (strcmp(protocol, "vnc") == 0) {
- int fd = monitor_get_fd(mon, fdname);
- vnc_display_add_client(NULL, fd, skipauth);
- return 0;
+ int fd = monitor_get_fd(mon, fdname, 1);
+ vnc_display_add_client(NULL, fd, skipauth);
+ return 0;
#endif
} else if ((s = qemu_chr_find(protocol)) != NULL) {
- int fd = monitor_get_fd(mon, fdname);
- if (qemu_chr_add_client(s, fd) < 0) {
- qerror_report(QERR_ADD_CLIENT_FAILED);
- return -1;
- }
- return 0;
+ int fd = monitor_get_fd(mon, fdname, 1);
+ if (qemu_chr_add_client(s, fd) < 0) {
+ qerror_report(QERR_ADD_CLIENT_FAILED);
+ return -1;
+ }
+ return 0;
}
qerror_report(QERR_INVALID_PARAMETER, "protocol");
@@ -2833,7 +2833,7 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
}
}
-int monitor_get_fd(Monitor *mon, const char *fdname)
+int monitor_get_fd(Monitor *mon, const char *fdname, unsigned char remove)
{
mon_fd_t *monfd;
@@ -2846,10 +2846,12 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
fd = monfd->fd;
- /* caller takes ownership of fd */
- QLIST_REMOVE(monfd, next);
- g_free(monfd->name);
- g_free(monfd);
+ if (remove) {
+ /* caller takes ownership of fd */
+ QLIST_REMOVE(monfd, next);
+ g_free(monfd->name);
+ g_free(monfd);
+ }
return fd;
}
@@ -2857,6 +2859,11 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
return -1;
}
+int qemu_get_fd(const char *fdname, unsigned char remove)
+{
+ return cur_mon ? monitor_get_fd(cur_mon, fdname, remove) : -1;
+}
+
static const mon_cmd_t mon_cmds[] = {
#include "hmp-commands.h"
{ NULL, NULL, },
diff --git a/monitor.h b/monitor.h
index 4f2d328..f18de19 100644
--- a/monitor.h
+++ b/monitor.h
@@ -50,7 +50,8 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
BlockDriverCompletionFunc *completion_cb,
void *opaque);
-int monitor_get_fd(Monitor *mon, const char *fdname);
+int monitor_get_fd(Monitor *mon, const char *fdname, unsigned char remove);
+int qemu_get_fd(const char *fdname, unsigned char remove);
void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
GCC_FMT_ATTR(2, 0);
diff --git a/net.c b/net.c
index d05930c..3482598 100644
--- a/net.c
+++ b/net.c
@@ -727,7 +727,7 @@ int net_handle_fd_param(Monitor *mon, const char *param)
if (!qemu_isdigit(param[0]) && mon) {
- fd = monitor_get_fd(mon, param);
+ fd = monitor_get_fd(mon, param, 1);
if (fd == -1) {
error_report("No file descriptor named %s found", param);
return -1;
diff --git a/qemu-options.hx b/qemu-options.hx
index d86815d..869320b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -131,7 +131,7 @@ using @file{/dev/cdrom} as filename (@pxref{host_drives}).
ETEXI
DEF("drive", HAS_ARG, QEMU_OPTION_drive,
- "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
+ "-drive [file=[fd:]file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
" [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
" [,cache=writethrough|writeback|none|unsafe][,format=f]\n"
" [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
@@ -144,10 +144,12 @@ STEXI
Define a new drive. Valid options are:
@table @option
-@item file=@var{file}
+@item file=[fd:]@var{file}
This option defines which disk image (@pxref{disk_images}) to use with
this drive. If the filename contains comma, you must double it
-(for instance, "file=my,,file" to use file "my,file").
+(for instance, "file=my,,file" to use file "my,file"). @option{fd:}@var{file}
+specifies the file descriptor of an already open disk image.
+@option{format=}@var{format} is required by @option{fd:}@var{file}.
@item if=@var{interface}
This option defines on which type on interface the drive is connected.
Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio.
diff --git a/qemu-tool.c b/qemu-tool.c
index eb89fe0..a2a0129 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -96,3 +96,8 @@ int64_t qemu_get_clock_ns(QEMUClock *clock)
{
return 0;
}
+
+int qemu_get_fd(const char *fdname, unsigned char remove)
+{
+ return -1;
+}
--
1.7.3.4
13 years, 4 months
[libvirt] [PATCH] virsh: Return false if connection or domain is not available
by Osier Yang
Instead of goto cleanup lable, it will work fine if go to
cleanup lable, but it's somewhat waste. And some functions
don't check if the "domain == NULL" before trying to free
it with virDomainFree(dom), as a result, there is additional
error in the log says:
error: invalid domain pointer in virDomainFree
Which is useless.
---
tools/virsh.c | 60 ++++++++++++++++++++++++++++----------------------------
1 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 7d849ec..ffb4ced 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -5201,10 +5201,10 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
int ret = -1;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto out;
+ return -1;
if (!(dom = vshCommandOptDomain(ctl, cmd, &name)))
- goto out;
+ return -1;
if (vshCommandOptString(cmd, "path", &path) < 0)
goto out;
@@ -10519,10 +10519,10 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
char *xml;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
- goto cleanup;
+ return false;
if (vshCommandOptString(cmd, "type", &type) <= 0)
goto cleanup;
@@ -10633,10 +10633,10 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
unsigned int flags;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
- goto cleanup;
+ return false;
if (vshCommandOptString(cmd, "type", &type) <= 0)
goto cleanup;
@@ -10922,10 +10922,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
char *xml;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
- goto cleanup;
+ return false;
if (vshCommandOptString(cmd, "source", &source) <= 0)
goto cleanup;
@@ -11105,10 +11105,10 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd)
unsigned int flags;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
- goto cleanup;
+ return false;
if (vshCommandOptString(cmd, "target", &target) <= 0)
goto cleanup;
@@ -11655,11 +11655,11 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd)
int flags = VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
dom = vshCommandOptDomain (ctl, cmd, NULL);
if (dom == NULL)
- goto cleanup;
+ return false;
/* Get the XML configuration of the domain. */
doc = virDomainGetXMLDesc (dom, flags);
@@ -11806,11 +11806,11 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd)
char *name = NULL;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
dom = vshCommandOptDomain(ctl, cmd, NULL);
if (dom == NULL)
- goto cleanup;
+ return false;
if (vshCommandOptString(cmd, "xmlfile", &from) <= 0)
buffer = vshStrdup(ctl, "<domainsnapshot/>");
@@ -11902,11 +11902,11 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
virBuffer buf = VIR_BUFFER_INITIALIZER;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
dom = vshCommandOptDomain(ctl, cmd, NULL);
if (dom == NULL)
- goto cleanup;
+ return false;
if (vshCommandOptString(cmd, "name", &name) < 0 ||
vshCommandOptString(cmd, "description", &desc) < 0) {
@@ -11995,11 +11995,11 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd)
char *xml = NULL;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
dom = vshCommandOptDomain(ctl, cmd, NULL);
if (dom == NULL)
- goto cleanup;
+ return false;
current = virDomainHasCurrentSnapshot(dom, 0);
if (current < 0)
@@ -12079,11 +12079,11 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
struct tm time_info;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
dom = vshCommandOptDomain(ctl, cmd, NULL);
if (dom == NULL)
- goto cleanup;
+ return false;
numsnaps = virDomainSnapshotNum(dom, 0);
@@ -12186,11 +12186,11 @@ cmdSnapshotDumpXML(vshControl *ctl, const vshCmd *cmd)
char *xml = NULL;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
dom = vshCommandOptDomain(ctl, cmd, NULL);
if (dom == NULL)
- goto cleanup;
+ return false;
if (vshCommandOptString(cmd, "snapshotname", &name) <= 0)
goto cleanup;
@@ -12245,11 +12245,11 @@ cmdSnapshotParent(vshControl *ctl, const vshCmd *cmd)
xmlXPathContextPtr ctxt = NULL;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
dom = vshCommandOptDomain(ctl, cmd, NULL);
if (dom == NULL)
- goto cleanup;
+ return false;
if (vshCommandOptString(cmd, "snapshotname", &name) <= 0)
goto cleanup;
@@ -12311,11 +12311,11 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd)
virDomainSnapshotPtr snapshot = NULL;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
dom = vshCommandOptDomain(ctl, cmd, NULL);
if (dom == NULL)
- goto cleanup;
+ return false;
if (vshCommandOptString(cmd, "snapshotname", &name) <= 0)
goto cleanup;
@@ -12364,11 +12364,11 @@ cmdSnapshotDelete(vshControl *ctl, const vshCmd *cmd)
unsigned int flags = 0;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
dom = vshCommandOptDomain(ctl, cmd, NULL);
if (dom == NULL)
- goto cleanup;
+ return false;
if (vshCommandOptString(cmd, "snapshotname", &name) <= 0)
goto cleanup;
@@ -12427,11 +12427,11 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd)
bool pad = false;
if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup;
+ return false;
dom = vshCommandOptDomain(ctl, cmd, NULL);
if (dom == NULL)
- goto cleanup;
+ return false;
while ((opt = vshCommandOptArgv(cmd, opt))) {
if (pad)
--
1.7.6
13 years, 4 months
[libvirt] ignore vs. error when inappropriate or unrecognized attributes/elements are present in XML
by Laine Stump
This is related to: https://bugzilla.redhat.com/show_bug.cgi?id=638633#c14
I had started to reply to it in the comments of the bug, but my reply
became too long, and expanded into an issue wider than that single bug,
so I figured I'd better discuss it here instead.
A synopsis: It's possible to specify a script to be executed when
bringing up a guest interface device. This is done by adding a <script
path='xxx'/> element to the <interface> definition.
The bug report was that <script path='blah'/> was added to an <interface
type='bridge'> definition in the config for a qemu domain, and nobody
complained that 'blah' didn't exist. This eventually led to the
realization that when interface type='bridge' and driver=qemu, <script>
isn't used, so it's ignored. More exactly, the script element is only
recognized by the parser when interface type == 'bridge' or 'ethernet',
and is only used by the hypervisor driver when:
1) the interface type='bridge' and the xen driver is being used.
or
2) the interface type='ethernet' and the qemu driver is being used.
After this all came to light, in comment 14, Dave says:
> That's being the case, we need to explicitly reject the attempt to
specify a
> script.
(implied: when interface type='bridge' and the hypervisor is qemu).
On the surface that sounds like a good idea, but it's opening a pretty
big can of worms.
Specifically for this issue, this rejection must take place in the
hypervisor because different hypervisors support scripts for different
types of interfaces - a script for type='bridge' is perfectly acceptable
for xen, but not for qemu. That can be handled with an error log in the
qemu driver in the case of type='bridge'. But if someone defines
<interface type='network'> and specifies a script, the parser ignores
the <script> element (because normally it's stored either in a union
that's only valid when type='bridge' or a different union only valid
when type='ethernet'), so qemu has no way of knowing a script was
specified and therefore can't log an error.
That's not a terribly difficult problem, though. Just a bit more code
than a simple "else { log an error; }" somewhere in the code - the
parser needs to be changed to move the script attribute out of the
unions and into the main struct, and always populate it, then the
drivers need to be taught to log an error when appropriate).
But if we're doing that for the <script> subelement of <interface>,
we're creating a prerequisite for what's expected in the case of all the
*other* attributes/elements that are currently ignored under similar
circumstances by libvirt's parsers. Essentially anything that's in a
union contained in any of the virXxxDef structs is probably treated the
same way. As a simple first example, look at "keymap", which is an
attribute of a domain's devices/graphics element, but only valid if the
type is spice or vnc. The parser ignores it in all other cases, and
since it's stored in data.spice or data.vnc, there's not even a
possibility of qemu (or other hypervisor) indicating any kind of error
when someone specifies e.g. <graphics type='sdl' 'keymap='blah' .../>.
Similarly, if someone were to add "tlsPort='5910' to a <graphics> of any
type other than spice. Of course, from the point of view of all the code
associated with <graphics type='vnc' ... />, this is no different than
if someone had added "frozzle='fib'" - it's totally unexpected (in this
case by any type), so it's ignored.
Actually, I can see now there are several different classes of this
problem. Here are the first few that come to mind:
1) an attribute/element is completely unknown/unexpected in all cases
(e.g. "frozzle='fib'" anywhere, or more insidious, something that
*looks* correct, but isn't, e.g. "<script name='/path/to/script'/>"*)
2) an attribute/element is useful/expected only when some other
attribute is set to a particular value (usually one called "type", but
could be something else), for example keymap='blah' is only expected in
a <graphics> element when type='spice' or type='vnc'.
3) an attribute/element is useful/expected only for certain combinations
of the value of some other attribute and which driver is using the
element, e.g. the subject of this bug - script='blah' is only expected
when type='bridge' and it's used by the Xen driver, or type='ethernet'
and it's used by the qemu driver.
So what are the rules of engagement for these various cases? When do we
ignore, when do we log an error during parsing, and when do we log an
error in the code that's using the parsed data?
(*Another example of (1) from recent real-life - during testing of
https://bugzilla.redhat.com/show_bug.cgi?id=727982, a QA person claimed
that the bugt hadn't been fixed because the hosts file wasn't being
updated; it turned out that they were adding the new <host> element at
the toplevel under <network> rather than inside the <dhcp> subelement,
so the parser was ignoring it (since it's unrecognized at that level)
rather than logging an error).
13 years, 4 months
[libvirt] Draft document about source control for type of pools
by Lei Li
Hi Daniel,
The last version of my patch fixed bug #611823 prohibit pools with duplicate storage,
you gave some comments that should do check by source info instead of target path I used.
These days I view the code and existing documents about storage pool, and tried to
code based on your comments, but looks like it is turning out to be more complex.
First, I think it is possible to make clear what it means for a pool to be a duplicate.
Below is the detailed source info be used for each type of pool according to the existing
documents and the main item to control.
For details of each type of pool:
VIR_STORAGE_POOL_DIR, /* For dir type, use source->dir only(But in the code and
example XML file,it use target path).*/
A pool with a type of dir provides the means to manage files within a directory.
VIR_STORAGE_POOL_FS, /* For fs type, use source->device->path.*/
This is a variant of the directory pool. Instead of creating a directory on an
existing mounted filesystem though, it expects a source block device to be named.
This block device will be mounted and files managed in the directory of its mount point.
VIR_STORAGE_POOL_NETFS, /* For netfs, use host name and source->dir */
This is a variant of the filesystem pool. Instead of requiring a local block device as
the source, it requires the name of a host and path of an exported directory. It will
mount this network filesystem and manage files within the directory of its mount point.
VIR_STORAGE_POOL_LOGICAL, /* For logical, use source->device->path. */
This provides a pool based on an LVM volume group. For a pre-defined LVM volume group,
simply providing the group name is sufficient,while to build a new group requires
providing a list of source devices to serve as physical volumes.
VIR_STORAGE_POOL_DISK, /* For disk type, use source->device->path.*/
This provides a pool based on a physical disk. Volumes are created by adding
partitions to the disk.
VIR_STORAGE_POOL_ISCSI, /* For ISCSI type, use source->device->path. */
This provides a pool based on an iSCSI target.
VIR_STORAGE_POOL_SCSI, /* For SCSI type, use source->adapter name. */
This provides a pool based on a SCSI HBA.
VIR_STORAGE_POOL_MPATH,
This provides a pool that contains all the multipath devices on the host.
Volume creating is not supported via the libvirt APIs, so we will just
ignore this type.
After a preliminary investigation, for source element,
source->device->path,
-Pool type: fs, logical, disk, ISCSI.
-May only occur once.
source->dir,
-Pool type: dir, netfs.
-May only occur once.
source->adapter
-Pool type: SCSI.
-May only occur once.
are the main required location info, and we can control the duplicate storage
based on these three item above for related type of pool.
Second, I have tried to do check by source info for a test, when I debug, I found
that pools->objs[]->def->source.* which come from virConnectPtr where all existing
pools info be keeped == NULL, but the current target pool virStoragePoolDefPtr def
can get source field value, so I think libvirt didn't get source field info in the
pools list where we do check at all today.
Do you think we should do check based on this? Or any comments for question one and
two. After you confirmed then I will work on code. Thank you.
--
Lei
13 years, 4 months
[libvirt] [PATCH] qemu: Substitute VIR_ERR_NO_SUPPORT with VIR_ERR_OPERATION_INVALID
by Osier Yang
* src/qemu/qemu_monitor_text.c: Error like "this function is not
supported by the connection driver" is confused obviously.
---
src/qemu/qemu_monitor_text.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index d296675..4a7c242 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -713,7 +713,7 @@ int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon,
* to detect if qemu supports the command.
*/
if (strstr(info, "\ninfo ")) {
- qemuReportError(VIR_ERR_NO_SUPPORT,
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
"%s",
_("'info blockstats' not supported by this qemu"));
goto cleanup;
@@ -1388,7 +1388,7 @@ int qemuMonitorTextMigrate(qemuMonitorPtr mon,
/* If the command isn't supported then qemu prints:
* unknown command: migrate" */
if (strstr(info, "unknown command:")) {
- qemuReportError(VIR_ERR_NO_SUPPORT,
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
_("migration to '%s' not supported by this qemu: %s"), dest, info);
goto cleanup;
}
@@ -1652,7 +1652,7 @@ int qemuMonitorTextAddPCIHostDevice(qemuMonitorPtr mon,
}
if (strstr(reply, "invalid type: host")) {
- qemuReportError(VIR_ERR_NO_SUPPORT, "%s",
+ qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("PCI device assignment is not supported by this version of qemu"));
goto cleanup;
}
--
1.7.6
13 years, 4 months
[libvirt] [PATCH] virsh: Print error if specified bandwidth is invalid for blockjob
by Osier Yang
It's strange that the command fails but without any error if one
specifies as not a number.
---
tools/virsh.c | 21 ++++++++++++++++-----
1 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index ffb4ced..97512b5 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -5209,17 +5209,28 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
if (vshCommandOptString(cmd, "path", &path) < 0)
goto out;
- if (vshCommandOptUL(cmd, "bandwidth", &bandwidth) < 0)
+ if (vshCommandOptUL(cmd, "bandwidth", &bandwidth) < 0) {
+ vshError(ctl, "%s", _("bandwidth must be a number"));
goto out;
+ }
- if (mode == VSH_CMD_BLOCK_JOB_ABORT)
+ if (mode == VSH_CMD_BLOCK_JOB_ABORT) {
+ vshPrint(ctl, "abort");
ret = virDomainBlockJobAbort(dom, path, 0);
- else if (mode == VSH_CMD_BLOCK_JOB_INFO)
+ }
+ else if (mode == VSH_CMD_BLOCK_JOB_INFO) {
+ vshPrint(ctl, "info\n");
ret = virDomainGetBlockJobInfo(dom, path, info, 0);
- else if (mode == VSH_CMD_BLOCK_JOB_SPEED)
+ vshPrint(ctl, "ret = %d\n", ret);
+ }
+ else if (mode == VSH_CMD_BLOCK_JOB_SPEED) {
+ vshPrint(ctl, "speed");
ret = virDomainBlockJobSetSpeed(dom, path, bandwidth, 0);
- else if (mode == VSH_CMD_BLOCK_JOB_PULL)
+ }
+ else if (mode == VSH_CMD_BLOCK_JOB_PULL) {
+ vshPrint(ctl, "pull");
ret = virDomainBlockPull(dom, path, bandwidth, 0);
+ }
out:
virDomainFree(dom);
--
1.7.6
13 years, 4 months
[libvirt] Adding migration speed to domainXML
by Jim Fehlig
I finally have some time to investigate Eric's suggestion [1] to add
virDomainMigrateGetMaxSpeed API and track current value in domainXML.
WRT the XML representation, it seems this best fits under the existing
lifecycle controls, e.g. a new "on_migrate" element. I'm not sure if
this should be kept simple and rather flat such as
<on_migrate max-speed='1000000000' />
or a little more expressive and extensible with something like
<on_migrate>
<bandwidth peak='1000000000' />
</on_migrate>
Any comments on these representations or perhaps alternative suggestions?
Regards,
Jim
[1] https://www.redhat.com/archives/libvir-list/2011-August/msg00224.html
13 years, 4 months
[libvirt] [PATCH] qemu: Report error if qemu monitor command not found for BlockJob
by Osier Yang
* src/qemu/qemu_monitor_json.c: Handle error "CommandNotFound" and
report the error.
* src/qemu/qemu_monitor_text.c: If a sub info command is not found,
it prints the output of "help info", for other commands,
"unknown command" is printed.
Without this patch, libvirt always report:
An error occurred, but the cause is unknown
---
src/qemu/qemu_monitor_json.c | 34 ++++++++++++++++++-----------
src/qemu/qemu_monitor_text.c | 47 +++++++++++++++++++++++++++++++++---------
2 files changed, 58 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 7adfb26..715b26e 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2909,20 +2909,25 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
int ret = -1;
virJSONValuePtr cmd = NULL;
virJSONValuePtr reply = NULL;
-
- if (mode == BLOCK_JOB_ABORT)
- cmd = qemuMonitorJSONMakeCommand("block_job_cancel",
- "s:device", device, NULL);
- else if (mode == BLOCK_JOB_INFO)
- cmd = qemuMonitorJSONMakeCommand("query-block-jobs", NULL);
- else if (mode == BLOCK_JOB_SPEED)
- cmd = qemuMonitorJSONMakeCommand("block_job_set_speed",
- "s:device", device,
- "U:value", bandwidth * 1024ULL * 1024ULL,
+ const char *cmd_name = NULL;
+
+ if (mode == BLOCK_JOB_ABORT) {
+ cmd_name = "block_job_cancel";
+ cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, NULL);
+ } else if (mode == BLOCK_JOB_INFO) {
+ cmd_name = "query-block-jobs";
+ cmd = qemuMonitorJSONMakeCommand(cmd_name, NULL);
+ } else if (mode == BLOCK_JOB_SPEED) {
+ cmd_name = "block_job_set_speed";
+ cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device",
+ device, "U:value",
+ bandwidth * 1024ULL * 1024ULL,
NULL);
- else if (mode == BLOCK_JOB_PULL)
- cmd = qemuMonitorJSONMakeCommand("block_stream",
- "s:device", device, NULL);
+ } else if (mode == BLOCK_JOB_PULL) {
+ cmd_name = "block_stream";
+ cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device",
+ device, NULL);
+ }
if (!cmd)
return -1;
@@ -2939,6 +2944,9 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
else if (qemuMonitorJSONHasError(reply, "NotSupported"))
qemuReportError(VIR_ERR_OPERATION_INVALID,
_("Operation is not supported for device: %s"), device);
+ else if (qemuMonitorJSONHasError(reply, "CommandNotFound"))
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ _("Command '%s' is not found"), cmd_name);
else
qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Unexpected error"));
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 52d924a..d296675 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -3031,18 +3031,24 @@ int qemuMonitorTextBlockJob(qemuMonitorPtr mon,
char *cmd = NULL;
char *reply = NULL;
int ret;
-
- if (mode == BLOCK_JOB_ABORT)
- ret = virAsprintf(&cmd, "block_job_cancel %s", device);
- else if (mode == BLOCK_JOB_INFO)
- ret = virAsprintf(&cmd, "info block-jobs");
- else if (mode == BLOCK_JOB_SPEED)
- ret = virAsprintf(&cmd, "block_job_set_speed %s %llu", device,
+ const char *cmd_name = NULL;
+
+ if (mode == BLOCK_JOB_ABORT) {
+ cmd_name = "block_job_cancel";
+ ret = virAsprintf(&cmd, "%s %s", cmd_name, device);
+ } else if (mode == BLOCK_JOB_INFO) {
+ cmd_name = "info block-jobs";
+ ret = virAsprintf(&cmd, "%s", cmd_name);
+ } else if (mode == BLOCK_JOB_SPEED) {
+ cmd_name = "block_job_set_speed";
+ ret = virAsprintf(&cmd, "%s %s %llu", cmd_name, device,
bandwidth * 1024ULL * 1024ULL);
- else if (mode == BLOCK_JOB_PULL)
- ret = virAsprintf(&cmd, "block_stream %s", device);
- else
+ } else if (mode == BLOCK_JOB_PULL) {
+ cmd_name = "block_stream";
+ ret = virAsprintf(&cmd, "%s %s", cmd_name, device);
+ } else {
return -1;
+ }
if (ret < 0) {
virReportOOMError();
@@ -3056,6 +3062,27 @@ int qemuMonitorTextBlockJob(qemuMonitorPtr mon,
goto cleanup;
}
+ /* Check error: Command not found, for "info block-jobs" command.
+ * If qemu monitor command "info" doesn't support one sub-command,
+ * it will print the output of "help info" instead, so simply check
+ * if "info version" is in the output.
+ */
+ if (mode == BLOCK_JOB_INFO) {
+ if (strstr(reply, "info version")) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ _("Command '%s' is not found"), cmd_name);
+ ret = -1;
+ goto cleanup;
+ }
+ } else {
+ if (strstr(reply, "unknown command")) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ _("Command '%s' is not found"), cmd_name);
+ ret = -1;
+ goto cleanup;
+ }
+ }
+
ret = qemuMonitorTextParseBlockJob(reply, device, info);
cleanup:
--
1.7.6
13 years, 4 months
