Attached is an updated patch. The changes are: - updated to latest CVS - run make check / syntax-check - remove virsh subcommand (as per Dan's suggestion - see below) - some more things that Dan pointed out - see below I would like to add this to CVS because it is quite a pain tracking CVS changes. I know there's no remote support at the moment, but I can add that later. On Thu, Apr 24, 2008 at 03:02:11PM +0100, Daniel P. Berrange wrote:

...

Index: configure.in =================================================================== RCS file: /data/cvs/libvirt/configure.in,v retrieving revision 1.139 diff -u -r1.139 configure.in --- configure.in 8 Apr 2008 16:45:57 -0000 1.139 +++ configure.in 16 Apr 2008 15:18:07 -0000 @@ -60,6 +60,10 @@

LIBVIRT_COMPILE_WARNINGS(maximum)

+dnl Support large files / 64 bit seek offsets. +dnl Use --disable-largefile if you don't want this. +AC_SYS_LARGEFILE +

IIRC, this is redundant now - we already added it elsewhere in the configure script when we did the storage patches.

In the patch I removed the second AC_SYS_LARGEFILE and left the useful comment.

...

- +int virDomainBlockPeek (virDomainPtr dom, + const char *path, + long long offset, + size_t size, + void *buffer);

Should probably make offset be an 'unsigned long long' unless we have some semantics which want -ve numbers ? Is 'char *' better or worse than 'void *' for the buffer arg ?

I changed the offset to unsigned long long. Left the buffer as void *.

...

+/* "domblkpeek" command + */ +static vshCmdInfo info_domblkpeek[] = { + {"syntax", "domblkpeek <domain> <path> <offset> <size>"}, + {"help", gettext_noop("peek at a domain block device")}, + {"desc", gettext_noop("Peek into a domain block device.")}, + {NULL,NULL} +}; + +static vshCmdOptDef opts_domblkpeek[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, + {"path", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("block device path")}, + {"offset", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("start offset")}, + {"size", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("size in bytes")}, + {NULL, 0, 0, NULL} +};

I'm wondering if this is perhaps one of the few APIs we should /not/ include in virsh ? The data is pretty useless on its own - I figure any app needing to access this is almost certainly not going to be shell scripting, and thus using one of the language bindings directly. Having the virsh command will probably just encourage people to use this as a really dumb file copy command.

This virsh changed removed.

...

+ /* The path is correct, now try to open it and get its size. */ + fd = open (path, O_RDONLY); + if (fd == -1 || fstat (fd, &statbuf) == -1) { + virXendError (domain->conn, VIR_ERR_SYSTEM_ERROR, strerror (errno)); + goto done; + } + + /* XXX The following test fails for LVM devices for a couple + * of reasons: (1) They are commonly symlinks to /dev/mapper/foo + * and despite the man page for fstat, fstat stats the link not + * the file. (2) Stat even on the block device returns st_size==0. + * + * Anyhow, it should be safe to ignore this test since we are + * in O_RDONLY mode. + */ +#if 0 + /* NB we know offset > 0, size >= 0 */ + if (offset + size > statbuf.st_size) { + virXendError (domain->conn, VIR_ERR_INVALID_ARG, "offset"); + goto done; + } +#endif

Actually the core problem is that fstat() does not return block device capacity. The best way to determine capacity of a block device is to lseek() to the end of it, and grab the return value of lseek.

There's also a Linux speciifc ioctl() to get device capacity, but the lseek approach is portable.

I just removed this code. It's not needed.

...

+ + /* Seek and read. */ + /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should + * be 64 bits on all platforms. + */ + if (lseek (fd, offset, SEEK_SET) == (off_t) -1 || + saferead (fd, buffer, size) == (ssize_t) -1) { + virXendError (domain->conn, VIR_ERR_SYSTEM_ERROR, strerror (errno)); + goto done; + } + + ret = 0; + done: + if (fd >= 0) close (fd); + return ret; +} + #endif /* ! PROXY */ #endif /* WITH_XEN */ Index: src/xend_internal.h =================================================================== RCS file: /data/cvs/libvirt/src/xend_internal.h,v retrieving revision 1.40 diff -u -r1.40 xend_internal.h --- src/xend_internal.h 10 Apr 2008 16:54:54 -0000 1.40 +++ src/xend_internal.h 16 Apr 2008 15:18:26 -0000 @@ -228,6 +228,8 @@ int xenDaemonDomainMigratePrepare (virConnectPtr dconn, char **cookie, int *cookielen, const char *uri_in, char **uri_out, unsigned long flags, const char *dname, unsigned long resource); int xenDaemonDomainMigratePerform (virDomainPtr domain, const char *cookie, int cookielen, const char *uri, unsigned long flags, const char *dname, unsigned long resource);

+int xenDaemonDomainBlockPeek (virDomainPtr domain, const char *path, long long offset, size_t size, void *buffer); + #ifdef __cplusplus } #endif Index: src/xm_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xm_internal.c,v retrieving revision 1.70 diff -u -r1.70 xm_internal.c --- src/xm_internal.c 10 Apr 2008 16:54:54 -0000 1.70 +++ src/xm_internal.c 16 Apr 2008 15:18:29 -0000 @@ -3160,4 +3160,15 @@ return (ret); }

+int +xenXMDomainBlockPeek (virDomainPtr dom, + const char *path ATTRIBUTE_UNUSED, + long long offset ATTRIBUTE_UNUSED, + size_t size ATTRIBUTE_UNUSED, + void *buffer ATTRIBUTE_UNUSED) +{ + xenXMError (dom->conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + return -1; +}

Hmm, is there no way to share the code here with the main Xen driver ? THe XenD driver impl parses the SEXPR to get the device info. Perhaps we could parse the XML format instead, then the only difference would be the API you call to get the XML doc in each driver. For that matter, if we worked off the XML format, this driver impl would be trivially sharable to QEMU and LXC, etc too.

I left this as it was because I'm not sure what you are suggesting. Do you mean to do a dumpxml operation inside the driver? Note that we're already dumping the XML outside the driver in order to get the paths to pass in. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v

"Richard W.M. Jones" <rjones@redhat.com> wrote:

Suggested patch attached.

Looks fine. Now that you've added the format string argument, we can give diagnostics that are more useful not just to users, but also to those who end up debugging things.

It seems a little bit perverse to have to put gettext around "%s: %s" but that's what make syntax-check wants. ... On second though, you can think of this as encouragement to avoid format strings with no translatable text.

I've found that bare "filename: strerror (errno)" diagnostics pose enough of a problem from the superficial "where did that come from" perspective that I try hard now always to provide a little more information (and thus something that is translatable). Of course, the minimal diagnostics are often hard to understand, too. I suspect that users don't mind getting that little bit of added info...

Index: src/xend_internal.c =================================================================== ... @@ -3925,12 +3937,14 @@ xenDaemonDomainMigratePrepare (virConnec if (uri_in == NULL) { r = gethostname (hostname, HOST_NAME_MAX+1); if (r == -1) { - virXendError (dconn, VIR_ERR_SYSTEM_ERROR, strerror (errno)); + virXendError (dconn, VIR_ERR_SYSTEM_ERROR, + "%s", strerror (errno));

maybe give more info: "gethostname failed: %s", strerror (errno));

return -1; } *uri_out = strdup (hostname); if (*uri_out == NULL) { - virXendError (dconn, VIR_ERR_SYSTEM_ERROR, strerror (errno)); + virXendError (dconn, VIR_ERR_SYSTEM_ERROR, + "%s", strerror (errno));

likewise, (but I don't like this one, but maybe better than nothing): "hostname strdup failed: %s", strerror (errno));

return -1; } } @@ -4575,14 +4589,15 @@ xenDaemonDomainBlockPeek (virDomainPtr d

if (!data.ok) { virXendError (domain->conn, VIR_ERR_INVALID_ARG, - _("invalid path")); + _("%s: invalid path"), path); return -1; }

/* The path is correct, now try to open it and get its size. */ fd = open (path, O_RDONLY); if (fd == -1) { - virXendError (domain->conn, VIR_ERR_SYSTEM_ERROR, strerror (errno)); + virXendError (domain->conn, VIR_ERR_SYSTEM_ERROR, + _("%s: %s"), path, strerror (errno));

For the above, how about _("%s: failed to open for reading: %s"), path, strerror (errno)); and for the one below: _("%s: lseek failed: %s"), path, strerror (errno));

goto done; }

@@ -4592,7 +4607,8 @@ xenDaemonDomainBlockPeek (virDomainPtr d */ if (lseek (fd, offset, SEEK_SET) == (off_t) -1 || saferead (fd, buffer, size) == (ssize_t) -1) { - virXendError (domain->conn, VIR_ERR_SYSTEM_ERROR, strerror (errno)); + virXendError (domain->conn, VIR_ERR_SYSTEM_ERROR, + _("%s: %s"), path, strerror (errno));