> 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.

> - > +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 ?

> +/* "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.

> + /* 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.

> + > + /* 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.

Suggested patch attached.

It seems a little bit perverse to have to put gettext around "%s: %s" but that's what make syntax-check wants.

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));

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));

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));

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));