[Libvir] Reopening the old discussion about virDomainBlockPeek
Daniel P. Berrange
berrange at redhat.com
Thu Apr 24 14:02:11 UTC 2008
On Wed, Apr 16, 2008 at 04:18:31PM +0100, Richard W.M. Jones wrote:
>
> Virt-df[1] has now gained the ability to fully parse LVM2 partitions,
> thus:
>
> # virt-df -c qemu:///system -h
> Filesystem Size Used Available Type
> rhel51x32kvm:hda1 96.8 MiB 14.6 MiB 82.2 MiB Linux ext2/3
> rhel51x32kvm:VolGroup00/LogVol00 6.4 GiB 3.6 GiB 2.8 GiB Linux ext2/3
> rhel51x32kvm:VolGroup00/LogVol01 992.0 MiB Linux swap
>
> However it still has to do it by opening the local partitions / files,
> which means it isn't using a "proper" part of libvirt and more
> importantly it cannot run remotely.
>
> I'd like to find out whether the time has come for us to look again at
> a virDomainBlockPeek call for libvirt. Here is the original thread
> plus patch from 7 months ago:
>
> http://www.redhat.com/archives/libvir-list/2007-October/thread.html#00089
>
> (I've attached an updated patch against current CVS).
>
> I appreciate that some cases might not be simple (iSCSI?), but the
> same argument applies to block device statistics too, and we make
> those available where possible. I think a best-effort call allowing
> callers to peek into the block devices of guests would be very useful.
While I don't particularly like the idea of adding a general API for reading
data from guest disks, I think given that clear & valid use case from the
virt-df, we should go ahead and add the virDomainBlockPeek API.
It will be rather fun for virt-df to deal with non-raw devices like qcow
but that's not really a concern for libvirt...
> 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.
Regards,
Daniel.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
More information about the libvir-list
mailing list