[libvirt] [PATCH Java] Remove non-thread-safe error reporting

2010-09-03 Thread Matthias Bolte
virConnCopyLastError is not thread-safe, don't use it.

Reported by Ravi Pawar.
---
 src/main/java/org/libvirt/Connect.java  |2 +-
 src/main/java/org/libvirt/ErrorHandler.java |   17 -
 2 files changed, 1 insertions(+), 18 deletions(-)

diff --git a/src/main/java/org/libvirt/Connect.java 
b/src/main/java/org/libvirt/Connect.java
index fb8ea89..7761c1f 100644
--- a/src/main/java/org/libvirt/Connect.java
+++ b/src/main/java/org/libvirt/Connect.java
@@ -1319,7 +1319,7 @@ public class Connect {
  * @throws LibvirtException
  */
 protected void processError() throws LibvirtException {
-ErrorHandler.processError(libvirt, VCP);
+ErrorHandler.processError(libvirt);
 }
 
 /**
diff --git a/src/main/java/org/libvirt/ErrorHandler.java 
b/src/main/java/org/libvirt/ErrorHandler.java
index 7b723bb..e30291b 100644
--- a/src/main/java/org/libvirt/ErrorHandler.java
+++ b/src/main/java/org/libvirt/ErrorHandler.java
@@ -28,21 +28,4 @@ public class ErrorHandler {
 throw new LibvirtException(error);
 }
 }
-
-/**
- * Look for the latest error from libvirt tied to a connection
- * 
- * @param libvirt
- *the active connection
- * @throws LibvirtException
- */
-public static void processError(Libvirt libvirt, ConnectionPointer conn) 
throws LibvirtException {
-virError vError = new virError();
-int errorCode = libvirt.virConnCopyLastError(conn, vError);
-if (errorCode  0) {
-Error error = new Error(vError);
-libvirt.virConnResetLastError(conn);
-throw new LibvirtException(error);
-}
-}
 }
-- 
1.7.0.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH Java] Remove non-thread-safe error reporting

2010-09-03 Thread Daniel P. Berrange
On Fri, Sep 03, 2010 at 01:33:26PM +0200, Matthias Bolte wrote:
 virConnCopyLastError is not thread-safe, don't use it.
 
 Reported by Ravi Pawar.
 ---
  src/main/java/org/libvirt/Connect.java  |2 +-
  src/main/java/org/libvirt/ErrorHandler.java |   17 -
  2 files changed, 1 insertions(+), 18 deletions(-)

ACK


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 Java] Remove non-thread-safe error reporting

2010-09-03 Thread Matthias Bolte
2010/9/3 Daniel P. Berrange berra...@redhat.com:
 On Fri, Sep 03, 2010 at 01:33:26PM +0200, Matthias Bolte wrote:
 virConnCopyLastError is not thread-safe, don't use it.

 Reported by Ravi Pawar.
 ---
  src/main/java/org/libvirt/Connect.java      |    2 +-
  src/main/java/org/libvirt/ErrorHandler.java |   17 -
  2 files changed, 1 insertions(+), 18 deletions(-)

 ACK


Thanks, pushed.

Matthias

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] Default to qemu:///system if accessible

2010-09-03 Thread Soren Hansen
If no uri is passed to one of the virConnectOpen-ish calls, libvirt
attempts to autoprobe a sensible URI. Part of the current logic checks
for getuid()  0, and if that check is succesful, a libvirtd daemon is
spawned. This patch replaces this check with a call to
access(LIBVIRTD_PRIV_UNIX_SOCKET, W_OK) so that users with access to the
qemu:///system UNIX socket connect to that one by default instead of
spawning a new libvirtd daemon.

Signed-off-by: Soren Hansen so...@linux2go.dk
---
 src/remote/remote_driver.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index a945710..17e6ead 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -1086,7 +1086,7 @@ remoteOpen (virConnectPtr conn,
 if (!conn-uri) {
 DEBUG0(Auto-probe remote URI);
 #ifndef __sun
-if (getuid()  0) {
+if (access(LIBVIRTD_PRIV_UNIX_SOCKET, W_OK)) {
 DEBUG0(Auto-spawn user daemon instance);
 rflags |= VIR_DRV_OPEN_REMOTE_USER;
 if (!autostart ||
-- 
1.7.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Default to qemu:///system if accessible

2010-09-03 Thread Daniel P. Berrange
On Fri, Sep 03, 2010 at 03:28:58PM +0200, Soren Hansen wrote:
 If no uri is passed to one of the virConnectOpen-ish calls, libvirt
 attempts to autoprobe a sensible URI. Part of the current logic checks
 for getuid()  0, and if that check is succesful, a libvirtd daemon is
 spawned. This patch replaces this check with a call to
 access(LIBVIRTD_PRIV_UNIX_SOCKET, W_OK) so that users with access to the
 qemu:///system UNIX socket connect to that one by default instead of
 spawning a new libvirtd daemon.

NACK, I don't think we should be changing this. If the user
is unprivileged, it should always default to the unprivileged
libvirtd, regardless of whether they are also authorized to
connect to the privileged libvirtd (via socket permissions or
policykit, or kerberos). If the unprivileged user still wants
the privileged libvirtd, they should given an explicit URI.

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


[libvirt] [PATCH] build: avoid unused variable warning

2010-09-03 Thread Eric Blake
* src/vbox/vbox_tmpl.c (vboxAttachDrives): Capture return value.
---

Pushing under the build-breaker rule.  Gcc didn't catch this under
'-g -O0', but with -Werror and -O2, it dies with a warning that rc
is used uninitialized.  I suspect that clang would have also
reported this, although I haven't tested that suspicion.

This was a pre-existing bug; my refactoring exposed it (before the
refactoring, rc contained stale contents, unrelated to the actual
message being printed; after the refactoring, rc was indeed used
without initialization).

 src/vbox/vbox_tmpl.c |   34 ++
 1 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 3e8ff23..b5cd2e4 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -3788,40 +3788,42 @@ vboxAttachDrives(virDomainDefPtr def, vboxGlobalData 
*data, IMachine *machine)
 VBOX_UTF16_FREE(mediumFileUtf16);
 continue;
 }

 if (!medium) {
 PRUnichar *mediumEmpty = NULL;

 VBOX_UTF8_TO_UTF16(, mediumEmpty);

 if (def-disks[i]-device == VIR_DOMAIN_DISK_DEVICE_DISK) {
-data-vboxObj-vtbl-OpenHardDisk(data-vboxObj,
-  mediumFileUtf16,
-  AccessMode_ReadWrite,
-  false,
-  mediumEmpty,
-  false,
-  mediumEmpty,
-  medium);
+rc = data-vboxObj-vtbl-OpenHardDisk(data-vboxObj,
+   mediumFileUtf16,
+   
AccessMode_ReadWrite,
+   false,
+   mediumEmpty,
+   false,
+   mediumEmpty,
+   medium);
 } else if (def-disks[i]-device ==
VIR_DOMAIN_DISK_DEVICE_CDROM) {
-data-vboxObj-vtbl-OpenDVDImage(data-vboxObj,
-  mediumFileUtf16,
-  mediumEmpty,
-  medium);
+rc = data-vboxObj-vtbl-OpenDVDImage(data-vboxObj,
+   mediumFileUtf16,
+   mediumEmpty,
+   medium);
 } else if (def-disks[i]-device ==
VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
-data-vboxObj-vtbl-OpenFloppyImage(data-vboxObj,
- mediumFileUtf16,
- mediumEmpty,
- medium);
+rc = data-vboxObj-vtbl-OpenFloppyImage(data-vboxObj,
+  mediumFileUtf16,
+  mediumEmpty,
+  medium);
+} else {
+rc = 0;
 }

 VBOX_UTF16_FREE(mediumEmpty);
 }

 if (!medium) {
 vboxError(VIR_ERR_INTERNAL_ERROR,
   _(Failed to attach the following disk/dvd/floppy 
 to the machine: %s, rc=%08x),
   def-disks[i]-src, (unsigned)rc);
-- 
1.7.2.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


[libvirt] libvirt-guests and ./configure --without-libvirtd

2010-09-03 Thread Eric Blake
On mingw, libvirt is typically configured --without-libvirtd, because 
the mingw build is designed to target other hosts, but cannot natively 
run as a hypervisor.  Additionally, mingw doesn't really have a notion 
of /etc/sysconf, so installing /etc/sysconfig/libvirt-guests doesn't 
work too well.


Should tools/Makefile.am make the build and installation of 
libvirt-guests dependent on the already-existing WITH_LIBVIRTD Makefile 
conditional, or would it ever make sense to build libvirt-guests but not 
libvirtd?  I guess the answer to that boils down to whether it ever 
makes sense to use libvirt-guests on a system that is not running 
libvirtd, in which case it would only be able to control remote URIs. 
But the whole point of the script is to cleanly save state for local 
VMs, so I don't see using libvirt-guests as a management tool for remote 
URIs.


Unless anyone speaks up with another opinion, I'll prepare a patch that 
makes libvirt-guests depend on WITH_LIBVIRTD.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] libvirt-guests and ./configure --without-libvirtd

2010-09-03 Thread Jiri Denemark
 Unless anyone speaks up with another opinion, I'll prepare a patch that 
 makes libvirt-guests depend on WITH_LIBVIRTD.

The init script is part of libvirt-client package because it doesn't require
libvirtd. On hosts with VirtualBox hypervisor, libvirtd is not needed and
libvirt-guests script can still handle guests on host shutdown/startup.

The installation of the init script and corresponding sysconfig file depends
on LIBVIRT_INIT_SCRIPT_RED_HAT, which feels like it doesn't make sense to
enable it on mingw. Isn't it sufficient?

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] libvirt-guests and ./configure --without-libvirtd

2010-09-03 Thread Eric Blake

On 09/03/2010 12:34 PM, Jiri Denemark wrote:

Unless anyone speaks up with another opinion, I'll prepare a patch that
makes libvirt-guests depend on WITH_LIBVIRTD.


The init script is part of libvirt-client package because it doesn't require
libvirtd. On hosts with VirtualBox hypervisor, libvirtd is not needed and
libvirt-guests script can still handle guests on host shutdown/startup.

The installation of the init script and corresponding sysconfig file depends
on LIBVIRT_INIT_SCRIPT_RED_HAT, which feels like it doesn't make sense to
enable it on mingw. Isn't it sufficient?


So the real bug, then, is that when cross-compiling from fedora-mingw, 
that LIBVIRT_INIT_SCRIPT_RED_HAT is being defined even though it 
shouldn't be.  Thanks; I think I can figure out something to fix that, then.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] OpenVZ: add ethernet interface type support

2010-09-03 Thread Eric Blake

On 08/18/2010 09:05 AM, Jean-Baptiste Rouault wrote:

Hi all,

This patch adds support for ethernet interface type to OpenVZ domains
as stated in this previous message: http://www.redhat.com/archives/libvir-
list/2010-July/msg00658.html

Regards,

Jean-Baptiste


Seems reasonable to me; sorry for the delayed review.

ACK and pushed.

--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


[libvirt] [PATCH] mingw: match recent changes in spec file

2010-09-03 Thread Eric Blake
These changes allow './autobuild.sh' to complete again, when a full
mingw cross-compilation is available on Fedora.

* libvirt.spec.in (%file): List new installed files.
* configure.ac (with_init_script): Assume default of none when
cross-compiling.
---
 configure.ac|6 +++---
 mingw32-libvirt.spec.in |5 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index d42d9e6..6c0fc69 100644
--- a/configure.ac
+++ b/configure.ac
@@ -276,10 +276,10 @@ AC_ARG_WITH([init-script],
 [AC_HELP_STRING([--with-init-script=@:@redhat|auto|none@:@],
 [Style of init script to install @:@default=auto@:@])])
 if test x$with_init_script = x || test x$with_init_script = xauto; then
-if test -f /etc/redhat-release ; then
-with_init_script=redhat
-else
+if test $cross_compiling = yes || test ! -f /etc/redhat-release; then
 with_init_script=none
+else
+with_init_script=redhat
 fi
 fi
 AM_CONDITIONAL([LIBVIRT_INIT_SCRIPT_RED_HAT], test x$with_init_script = 
xredhat)
diff --git a/mingw32-libvirt.spec.in b/mingw32-libvirt.spec.in
index 6b2b5c6..4bbbc3b 100644
--- a/mingw32-libvirt.spec.in
+++ b/mingw32-libvirt.spec.in
@@ -71,6 +71,7 @@ rm -rf $RPM_BUILD_ROOT%{_mingw32_datadir}/doc/*
 rm -rf $RPM_BUILD_ROOT%{_mingw32_datadir}/gtk-doc/*

 rm $RPM_BUILD_ROOT%{_mingw32_libdir}/libvirt.a
+rm $RPM_BUILD_ROOT%{_mingw32_libdir}/libvirt-qemu.a


 %clean
@@ -83,10 +84,13 @@ rm -rf $RPM_BUILD_ROOT
 %{_mingw32_bindir}/virsh.exe
 %{_mingw32_bindir}/virt-xml-validate
 %{_mingw32_bindir}/virt-pki-validate
+%{_mingw32_bindir}/libvirt-qemu-0.dll

 %{_mingw32_libdir}/libvirt.dll.a
 %{_mingw32_libdir}/libvirt.la
 %{_mingw32_libdir}/pkgconfig/libvirt.pc
+%{_mingw32_libdir}/libvirt-qemu.dll.a
+%{_mingw32_libdir}/libvirt-qemu.la

 %dir %{_mingw32_datadir}/libvirt/
 %dir %{_mingw32_datadir}/libvirt/schemas/
@@ -109,6 +113,7 @@ rm -rf $RPM_BUILD_ROOT
 %dir %{_mingw32_includedir}/libvirt
 %{_mingw32_includedir}/libvirt/libvirt.h
 %{_mingw32_includedir}/libvirt/virterror.h
+%{_mingw32_includedir}/libvirt/libvirt-qemu.h

 %{_mingw32_mandir}/man1/virsh.1*
 %{_mingw32_mandir}/man1/virt-xml-validate.1*
-- 
1.7.2.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Default to qemu:///system if accessible

2010-09-03 Thread Soren Hansen
On 03-09-2010 16:08, Daniel P. Berrange wrote:
 If no uri is passed to one of the virConnectOpen-ish calls, libvirt
 attempts to autoprobe a sensible URI. Part of the current logic checks
 for getuid()  0, and if that check is succesful, a libvirtd daemon is
 spawned. This patch replaces this check with a call to
 access(LIBVIRTD_PRIV_UNIX_SOCKET, W_OK) so that users with access to the
 qemu:///system UNIX socket connect to that one by default instead of
 spawning a new libvirtd daemon.
 NACK, I don't think we should be changing this. If the user
 is unprivileged, it should always default to the unprivileged
 libvirtd, regardless of whether they are also authorized to
 connect to the privileged libvirtd (via socket permissions or
 policykit, or kerberos). If the unprivileged user still wants
 the privileged libvirtd, they should given an explicit URI.

Hm... I didn't think this was going to be controversial :)

I firmly believe that if a user has access to the privileged libvirt
daemon, that's the one he'll want to interact with in all but
extraordinary cases. I consider it very analogous to how virt-manager
doesn't even offer usermode networking if you're connected to
qemu:///system, since if you aren't stuck with usermode networking,
there's no reason to use it (other than to prove this statement wrong).

Ubuntu has carried patches for this for virsh, virt-manager, and
virt-viewer for a while now (at least a year and a half). I'm not sure
if they've been submitted here (or to et-mgmt-tools) before (and if not,
why not), but that's the lay of the land, and I've had nothing but good
feedback on it. Now I just wanted to fix it properly (directly in
libvirt) and get it upstream. It's certainly saved me a lot of typing.

-- 
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] Default to qemu:///system if accessible

2010-09-03 Thread Eric Blake

On 09/03/2010 02:38 PM, Soren Hansen wrote:

NACK, I don't think we should be changing this. If the user
is unprivileged, it should always default to the unprivileged
libvirtd, regardless of whether they are also authorized to
connect to the privileged libvirtd (via socket permissions or
policykit, or kerberos). If the unprivileged user still wants
the privileged libvirtd, they should given an explicit URI.


Hm... I didn't think this was going to be controversial :)


Maybe a less-controversial patch would be changing configure.ac to add a 
configure option for the default URI string for non-privileged users? 
Right now, the default is hard-coded to qemu:///session, but by letting 
it be a configure choice, then it would be up to the end user (or 
distro) whether to risk the default of qemu:///system as well as 
exposing the socket as writable.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


Re: [libvirt] [PATCH] Default to qemu:///system if accessible

2010-09-03 Thread Soren Hansen
On 03-09-2010 15:59, Daniel Veillard wrote:
 diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
 index a945710..17e6ead 100644
 --- a/src/remote/remote_driver.c
 +++ b/src/remote/remote_driver.c
 @@ -1086,7 +1086,7 @@ remoteOpen (virConnectPtr conn,
  if (!conn-uri) {
  DEBUG0(Auto-probe remote URI);
  #ifndef __sun
 -if (getuid()  0) {
 +if (access(LIBVIRTD_PRIV_UNIX_SOCKET, W_OK)) {
  DEBUG0(Auto-spawn user daemon instance);
  rflags |= VIR_DRV_OPEN_REMOTE_USER;
  if (!autostart ||
 
 Hum, that sounds like a change of semantic.

Depends. I think of the (getuid()  0) as a check for whether you can
access the privileged UNIX socket that simply doesn't take into account
that users other than root may have been given this privilege by way of
membership of the group owning the socket.

 Before as non root you would span a local daemon, now you use the system
 one. I'm not saying it's a bad thing in most cases but it's a serious
 change, and we try to avoid those in general.

As I suggested in another e-mail a short while ago in this thread, I'm
making this change to maintain the status quo in Ubuntu. We already do
this in virsh and virt-viewer and something very, very similar in
virt-manager. Someone recently dropped the virt-viewer patch by mistake,
and I decided to make the change directly in libvirt to not change how
libvirt has behaved for us in the past couple of years, so I can
certainly relate to your desire to not change the behaviour.

Personally, at least, I find this behaviour much more pleasant. On all
my servers, I just grant whoever needs to be able to deal with the VM's
on it access to the socket (by adding them to the libvirtd group), and
that's it. I don't need to instruct them to set per-application
environment variables or whatever.

Generally, I use libvirt on two classes of machine: Workstations/laptops
(where I'm functionally the only user), or on servers whose job is to
run virtual machines and nothing else.

In the former case, obviously I want to use the systemwide libvirtd to
get access to all the fancy networking things. I have absolutely no
motivation to use qemu:///session on there anymore.

In the latter case, only people who are supposed to manage the virtual
machines will have access to the server, and IMO they have no business
running stuff under qemu:///session on that box.

The only situation where I actually need qemu:///session is if I for
some reason want to run a VM on a machine that isn't mine (neither in
terms of ownership, nor responsibility). So far, that's never happened.
It's cool that it's possible(!), but I've never needed it.

-- 
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] esx: Add .vmdk storage volume creation

2010-09-03 Thread Matthias Bolte
2010/8/30 Eric Blake ebl...@redhat.com:
 On 08/28/2010 01:53 PM, Matthias Bolte wrote:

 +    /* Validate config */
 +    tmp1 = strchr(def-name, '/');
 +    tmp2 = strrchr(def-name, '/');
 +
 +    if (tmp1 == NULL || tmp1 == def-name ||
 +        tmp2 == NULL || tmp2[1] == '\0') {

 tmp2 cannot be NULL if tmp1 is not NULL, since if the string contains a / in
 a forward search, it will also contain one in a reverse search. Also,
 checking that tmp1 != def-name can be done with a single-byte probe.  What
 about using a single temporary for a faster test:

 tmp = strrchr(def-name, '/');
 if (tmp == NULL || *def-name == '/' || tmp[1] == '\0') {

Yes, that's fine too. I'll fold that in.

 +        /*
 +         * FIXME: The adapter type is a required parameter, but there is
 no
 +         * way to let the user specifiy it in the volume XML config.
 Therefore,

 s/specifiy/specify/

 +         * default to 'busLogic' here.
 +         */

 Sounds like a good followup patch, but doesn't impact whether this one is
 okay as-is.

 +        virtualDiskSpec-adapterType = (char *)busLogic;
 +
 +        virtualDiskSpec-capacityKb-value = def-capacity / 1024; /*
 Scale from byte to kilobyte */

 Do you need to round up?  That is, should  def-capacity of 1 result in 0 or
 1024 bytes?

I truncate here on purpose. The rest of the ESX driver does it this
way, also the rest of the libvirt codebase truncates in situations
like this. Therefore, I'll leave this as it is.

 I think those points are minor enough to not need a v2, so:

 ACK with those points addressed.


Thanks, pushed.

Matthias

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] esx: Use the VirtualDisk UUID as storage volume key

2010-09-03 Thread Matthias Bolte
2010/8/30 Eric Blake ebl...@redhat.com:
 On 08/29/2010 05:00 PM, Matthias Bolte wrote:

 VirtualDisks are .vmdk file based. Other files in a datastore
 like .iso or .flp files don't have a UUID attached, fall back
 to the path as key for them.
 ---
  src/esx/esx_storage_driver.c   |  190
 
  src/esx/esx_util.c             |   19 
  src/esx/esx_util.h             |    2 +
  src/esx/esx_vi.c               |   53 +++
  src/esx/esx_vi.h               |    4 +
  src/esx/esx_vi_generator.input |    7 ++
  6 files changed, 256 insertions(+), 19 deletions(-)

 ACK.


Thanks, pushed.

Matthias

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] esx: Fall back to path as key when QueryVirtualDiskUuid isn't available

2010-09-03 Thread Matthias Bolte
QueryVirtualDiskUuid is only available on an ESX(i) server. vCenter
returns an NotImplemented fault and a GSX server is missing the
VirtualDiskManager completely. Therefore only use QueryVirtualDiskUuid
with an ESX(i) server and fall back to path as storage volume key for
vCenter and GSX server.
---
 src/esx/esx_storage_driver.c |   36 +++--
 src/esx/esx_vi.c |   50 +++--
 src/esx/esx_vi.h |1 +
 3 files changed, 62 insertions(+), 25 deletions(-)

diff --git a/src/esx/esx_storage_driver.c b/src/esx/esx_storage_driver.c
index 76f8769..329020b 100644
--- a/src/esx/esx_storage_driver.c
+++ b/src/esx/esx_storage_driver.c
@@ -783,6 +783,13 @@ esxStorageVolumeLookupByKey(virConnectPtr conn, const char 
*key)
 return esxStorageVolumeLookupByPath(conn, key);
 }
 
+if (!priv-primary-hasQueryVirtualDiskUuid) {
+ESX_ERROR(VIR_ERR_INTERNAL_ERROR, %s,
+  _(QueryVirtualDiskUuid not avialable, cannot lookup storage 

+volume by UUID));
+return NULL;
+}
+
 if (esxVI_EnsureSession(priv-primary)  0) {
 return NULL;
 }
@@ -916,7 +923,7 @@ esxStorageVolumeCreateXML(virStoragePoolPtr pool, const 
char *xmldesc,
 esxVI_ManagedObjectReference *task = NULL;
 esxVI_TaskInfoState taskInfoState;
 char *uuid_string = NULL;
-char key[VIR_UUID_STRING_BUFLEN] = ;
+char *key = NULL;
 
 virCheckFlags(0, NULL);
 
@@ -1068,14 +1075,26 @@ esxStorageVolumeCreateXML(virStoragePoolPtr pool, const 
char *xmldesc,
 goto cleanup;
 }
 
-if (esxVI_QueryVirtualDiskUuid(priv-primary, datastorePath,
-   priv-primary-datacenter-_reference,
-   uuid_string)  0) {
-goto cleanup;
-}
+if (priv-primary-hasQueryVirtualDiskUuid) {
+if (VIR_ALLOC_N(key, VIR_UUID_STRING_BUFLEN)  0) {
+virReportOOMError();
+goto cleanup;
+}
 
-if (esxUtil_ReformatUuid(uuid_string, key)  0) {
-goto cleanup;
+if (esxVI_QueryVirtualDiskUuid(priv-primary, datastorePath,
+   
priv-primary-datacenter-_reference,
+   uuid_string)  0) {
+goto cleanup;
+}
+
+if (esxUtil_ReformatUuid(uuid_string, key)  0) {
+goto cleanup;
+}
+} else {
+/* Fall back to the path as key */
+if (esxVI_String_DeepCopyValue(key, datastorePath)  0) {
+goto cleanup;
+}
 }
 } else {
 ESX_ERROR(VIR_ERR_INTERNAL_ERROR,
@@ -1103,6 +1122,7 @@ esxStorageVolumeCreateXML(virStoragePoolPtr pool, const 
char *xmldesc,
 esxVI_FileBackedVirtualDiskSpec_Free(virtualDiskSpec);
 esxVI_ManagedObjectReference_Free(task);
 VIR_FREE(uuid_string);
+VIR_FREE(key);
 
 return volume;
 }
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index 00e15f0..78cfdfd 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -453,6 +453,17 @@ esxVI_Context_Connect(esxVI_Context *ctx, const char *url,
 return -1;
 }
 
+if (ctx-productVersion  esxVI_ProductVersion_ESX) {
+/*
+ * FIXME: Actually this should be detected by really calling
+ * QueryVirtualDiskUuid and checking if a NotImplemented fault is
+ * returned. But currently we don't deserialized the details of a
+ * possbile fault and therefore we don't know if the fault was a
+ * NotImplemented fault or not.
+ */
+ctx-hasQueryVirtualDiskUuid = true;
+}
+
 if (esxVI_Login(ctx, username, password, NULL, ctx-session)  0 ||
 esxVI_BuildSelectSetCollection(ctx)  0) {
 return -1;
@@ -3267,28 +3278,33 @@ 
esxVI_LookupStorageVolumeKeyByDatastorePath(esxVI_Context *ctx,
 return -1;
 }
 
-if (esxVI_LookupFileInfoByDatastorePath(ctx, datastorePath, false, 
fileInfo,
-esxVI_Occurrence_RequiredItem)  
0) {
-goto cleanup;
-}
-
-if (esxVI_VmDiskFileInfo_DynamicCast(fileInfo) != NULL) {
-/* VirtualDisks have a UUID, use it as key */
-if (esxVI_QueryVirtualDiskUuid(ctx, datastorePath,
-   ctx-datacenter-_reference,
-   uuid_string)  0) {
+if (ctx-hasQueryVirtualDiskUuid) {
+if (esxVI_LookupFileInfoByDatastorePath
+  (ctx, datastorePath, false, fileInfo,
+   esxVI_Occurrence_RequiredItem)  0) {
 goto cleanup;
 }
 
-if (VIR_ALLOC_N(*key, VIR_UUID_STRING_BUFLEN)  0) {
-virReportOOMError();
-goto cleanup;
-}
+if (esxVI_VmDiskFileInfo_DynamicCast(fileInfo) != NULL) {
+/* VirtualDisks have