9:34 a.m.
Basically:
$ gdb virsh
...
(gdb) run --connect qemu:///session pool-list --all
Starting program: /usr/bin/virsh --connect qemu:///session pool-list --all
[Thread debugging using libthread_db enabled]
Detaching after fork from child process 29579.
Program received signal SIGSEGV, Segmentation fault.
...
(gdb) bt
#0 _libssh2_channel_open (session=0x0, channel_type=0x7ffff7b82737 "session",
channel_type_len=7, window_size=65536, packet_size=32768, message=0x0,
message_len=0) at channel.c:139
#1 0x00000030cb008a7c in libssh2_channel_open_ex (session=0x0,
type=0x7ffff7b82737 "session", type_len=7, window_size=65536,
packet_size=32768, msg=<value optimized out>, msg_len=0) at channel.c:338
#2 0x00007ffff7ac9edb in phypExec (session=0x0,
cmd=0x63ba50 "viosvrcmd -m (null) --id 0 -c 'lsvg'|grep -c
'^.*$'",
exit_status=0x7fffffffdbcc, conn=<value optimized out>)
at phyp/phyp_driver.c:124
#3 0x00007ffff7acae1c in phypNumOfStoragePools (conn=0x6341b0)
at phyp/phyp_driver.c:3032
#4 0x00007ffff7a771be in virConnectNumOfStoragePools (conn=0x6341b0)
at libvirt.c:7223
#5 0x0000000000415f9e in cmdPoolList (ctl=0x7fffffffdf80, cmd=0x1)
at virsh.c:4911
#6 0x0000000000409dcc in vshCommandRun (ctl=0x7fffffffdf80, cmd=0x62d500)
at virsh.c:9647
#7 0x00000000004186ed in main (argc=5, argv=<value optimized out>)
at virsh.c:10656
Looks like the phyp storage driver is being used instead of the libvirtd
storage driver (and then segfaulting). Looked briefly for a fix, but it
wasn't obvious to me.
Thanks,
Cole
9:45 a.m.
On Mon, Jun 28, 2010 at 10:34:34AM -0400, Cole Robinson wrote:
Basically:
> $ gdb virsh
...
> (gdb) run --connect qemu:///session pool-list --all
> Starting program: /usr/bin/virsh --connect qemu:///session pool-list --all
> [Thread debugging using libthread_db enabled]
> Detaching after fork from child process 29579.
>
> Program received signal SIGSEGV, Segmentation fault.
...
> (gdb) bt
> #0 _libssh2_channel_open (session=0x0, channel_type=0x7ffff7b82737
"session",
> channel_type_len=7, window_size=65536, packet_size=32768, message=0x0,
> message_len=0) at channel.c:139
> #1 0x00000030cb008a7c in libssh2_channel_open_ex (session=0x0,
> type=0x7ffff7b82737 "session", type_len=7, window_size=65536,
> packet_size=32768, msg=<value optimized out>, msg_len=0) at channel.c:338
> #2 0x00007ffff7ac9edb in phypExec (session=0x0,
> cmd=0x63ba50 "viosvrcmd -m (null) --id 0 -c 'lsvg'|grep -c
'^.*$'",
> exit_status=0x7fffffffdbcc, conn=<value optimized out>)
> at phyp/phyp_driver.c:124
> #3 0x00007ffff7acae1c in phypNumOfStoragePools (conn=0x6341b0)
> at phyp/phyp_driver.c:3032
> #4 0x00007ffff7a771be in virConnectNumOfStoragePools (conn=0x6341b0)
> at libvirt.c:7223
> #5 0x0000000000415f9e in cmdPoolList (ctl=0x7fffffffdf80, cmd=0x1)
> at virsh.c:4911
> #6 0x0000000000409dcc in vshCommandRun (ctl=0x7fffffffdf80, cmd=0x62d500)
> at virsh.c:9647
> #7 0x00000000004186ed in main (argc=5, argv=<value optimized out>)
> at virsh.c:10656
Looks like the phyp storage driver is being used instead of the libvirtd
storage driver (and then segfaulting). Looked briefly for a fix, but it
wasn't obvious to me.
The phypStorageOpen() method is totally bogus. It is ignoring the conn
object and accepting all connections.
It must only return VIR_DRV_OPEN_SUCCESS if conn->driver->name == VIR_DRV_PHYP
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 :|
12:40 p.m.
New subject: [libvirt] [PATCH] phyp: don't steal storage management from other drivers
Fix regression introduced in commit a4a287242 - basically, the
phyp storage driver should only accept the same URIs that the
main phyp driver is willing to accept. Blindly accepting all
URIs meant that the phyp storage driver was being consulted for
'virsh -c qemu:///session pool-list --all', rather than the
qemu storage driver, then since the URI was not for phyp, attempts
to then use the phyp driver crahsed because it was not initialized.
* src/phyp/phyp_driver.c (phypStorageOpen): Copy phypOpen on which
connections we are willing to accept.
---
> Looks like the phyp storage driver is being used instead of the
libvirtd
> storage driver (and then segfaulting). Looked briefly for a fix, but it
> wasn't obvious to me.
The phypStorageOpen() method is totally bogus. It is ignoring the conn
object and accepting all connections.
Sorry for not realizing that during my review; this is my first
time dealing with a storage driver patch.
It must only return VIR_DRV_OPEN_SUCCESS if conn->driver->name
== VIR_DRV_PHYP
Agreed. I think this patch does the right thing.
src/phyp/phyp_driver.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 126 insertions(+), 1 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index 3692f2c..62dfbcf 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -3875,13 +3875,138 @@ phypDomainSetCPU(virDomainPtr dom, unsigned int nvcpus)
}
static virDrvOpenStatus
-phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED,
+phypStorageOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
int flags)
{
+ LIBSSH2_SESSION *session = NULL;
+ ConnectionData *connection_data = NULL;
+ char *string = NULL;
+ size_t len = 0;
+ int internal_socket;
+ uuid_tablePtr uuid_table = NULL;
+ phyp_driverPtr phyp_driver = NULL;
+ char *char_ptr;
+ char *managed_system = NULL;
+
virCheckFlags(0, VIR_DRV_OPEN_ERROR);
+ if (!conn || !conn->uri)
+ return VIR_DRV_OPEN_DECLINED;
+
+ if (conn->uri->scheme == NULL || STRNEQ(conn->uri->scheme,
"phyp"))
+ return VIR_DRV_OPEN_DECLINED;
+
+ if (conn->uri->server == NULL) {
+ PHYP_ERROR(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Missing server name in phyp:// URI"));
+ return VIR_DRV_OPEN_ERROR;
+ }
+
+ if (VIR_ALLOC(phyp_driver) < 0) {
+ virReportOOMError();
+ goto failure;
+ }
+
+ if (VIR_ALLOC(uuid_table) < 0) {
+ virReportOOMError();
+ goto failure;
+ }
+
+ if (VIR_ALLOC(connection_data) < 0) {
+ virReportOOMError();
+ goto failure;
+ }
+
+ if (conn->uri->path) {
+ len = strlen(conn->uri->path) + 1;
+
+ if (VIR_ALLOC_N(string, len) < 0) {
+ virReportOOMError();
+ goto failure;
+ }
+
+ /* need to shift one byte in order to remove the first "/" of URI
component */
+ if (conn->uri->path[0] == '/')
+ managed_system = strdup(conn->uri->path + 1);
+ else
+ managed_system = strdup(conn->uri->path);
+
+ if (!managed_system) {
+ virReportOOMError();
+ goto failure;
+ }
+
+ /* here we are handling only the first component of the path,
+ * so skipping the second:
+ * */
+ char_ptr = strchr(managed_system, '/');
+
+ if (char_ptr)
+ *char_ptr = '\0';
+
+ if (escape_specialcharacters(conn->uri->path, string, len) == -1) {
+ PHYP_ERROR(VIR_ERR_INTERNAL_ERROR,
+ "%s",
+ _("Error parsing 'path'. Invalid
characters."));
+ goto failure;
+ }
+ }
+
+ if ((session = openSSHSession(conn, auth, &internal_socket)) == NULL) {
+ PHYP_ERROR(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Error while opening SSH session."));
+ goto failure;
+ }
+
+ connection_data->session = session;
+
+ uuid_table->nlpars = 0;
+ uuid_table->lpars = NULL;
+
+ if (conn->uri->path)
+ phyp_driver->managed_system = managed_system;
+
+ phyp_driver->uuid_table = uuid_table;
+ if ((phyp_driver->caps = phypCapsInit()) == NULL) {
+ virReportOOMError();
+ goto failure;
+ }
+
+ conn->privateData = phyp_driver;
+ conn->networkPrivateData = connection_data;
+
+ if ((phyp_driver->system_type = phypGetSystemType(conn)) == -1)
+ goto failure;
+
+ if (phypUUIDTable_Init(conn) == -1)
+ goto failure;
+
+ if (phyp_driver->system_type == HMC) {
+ if ((phyp_driver->vios_id = phypGetVIOSPartitionID(conn)) == -1)
+ goto failure;
+ }
+
return VIR_DRV_OPEN_SUCCESS;
+
+ failure:
+ if (phyp_driver != NULL) {
+ virCapabilitiesFree(phyp_driver->caps);
+ VIR_FREE(phyp_driver->managed_system);
+ VIR_FREE(phyp_driver);
+ }
+
+ phypUUIDTable_Free(uuid_table);
+
+ if (session != NULL) {
+ libssh2_session_disconnect(session, "Disconnecting...");
+ libssh2_session_free(session);
+ }
+
+ VIR_FREE(connection_data);
+ VIR_FREE(string);
+
+ return VIR_DRV_OPEN_ERROR;
}
static int
--
1.7.0.1
1:06 p.m.
New subject: [libvirt] [PATCH] phyp: don't steal storage management from other drivers
2010/6/28 Eric Blake <eblake(a)redhat.com>:
Fix regression introduced in commit a4a287242 - basically, the
phyp storage driver should only accept the same URIs that the
main phyp driver is willing to accept. Blindly accepting all
URIs meant that the phyp storage driver was being consulted for
'virsh -c qemu:///session pool-list --all', rather than the
qemu storage driver, then since the URI was not for phyp, attempts
to then use the phyp driver crahsed because it was not initialized.
* src/phyp/phyp_driver.c (phypStorageOpen): Copy phypOpen on which
connections we are willing to accept.
---
> > Looks like the phyp storage driver is being used instead of the libvirtd
> > storage driver (and then segfaulting). Looked briefly for a fix, but it
> > wasn't obvious to me.
> The phypStorageOpen() method is totally bogus. It is ignoring the conn
> object and accepting all connections.
Sorry for not realizing that during my review; this is my first
time dealing with a storage driver patch.
> It must only return VIR_DRV_OPEN_SUCCESS if conn->driver->name ==
VIR_DRV_PHYP
Agreed. I think this patch does the right thing.
The intention is correct, but the implementation is not. Why do you
duplicate the whole phypOpen code in phypStorageOpen?
Besides of being unnecessary this also overwrites the already
initialized private driver data of the virConnectPtr.
Something like I did for the ESX storage driver (and Daniel suggested)
should be sufficient here too.
static virDrvOpenStatus
phypStorageOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
int flags)
{
virCheckFlags(0, VIR_DRV_OPEN_ERROR);
if (STRNEQ(conn->driver->name, "PHYP")) {
return VIR_DRV_OPEN_DECLINED;
}
return VIR_DRV_OPEN_SUCCESS;
}
Probably comparing the conn->driver->no would be even better.
static virDrvOpenStatus
phypStorageOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
int flags)
{
virCheckFlags(0, VIR_DRV_OPEN_ERROR);
if (conn->driver->no != VIR_DRV_PHYP) {
return VIR_DRV_OPEN_DECLINED;
}
return VIR_DRV_OPEN_SUCCESS;
}
When libvirt tries to open the secondary drivers (like a storage
driver) we already have a main hypervisor driver successfully opened.
so we don't need to check the URI again. We just need to make sure
that the PHYP storage driver only accepts an open call iff the open
main driver is the PHYP driver.
Matthias
1:29 p.m.
New subject: [libvirt] [PATCH] phyp: don't steal storage management from other drivers
On 06/28/2010 12:06 PM, Matthias Bolte wrote:
> Sorry for not realizing that during my review; this is my first
> time dealing with a storage driver patch.
>
>> It must only return VIR_DRV_OPEN_SUCCESS if conn->driver->name ==
VIR_DRV_PHYP
>
> Agreed. I think this patch does the right thing.
>
The intention is correct, but the implementation is not. Why do you
duplicate the whole phypOpen code in phypStorageOpen?
Besides of being unnecessary this also overwrites the already
initialized private driver data of the virConnectPtr.
Like I said, this is my first time dealing with a storage driver ;)
Something like I did for the ESX storage driver (and Daniel suggested)
should be sufficient here too.
Probably comparing the conn->driver->no would be even better.
static virDrvOpenStatus
phypStorageOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
int flags)
{
virCheckFlags(0, VIR_DRV_OPEN_ERROR);
if (conn->driver->no != VIR_DRV_PHYP) {
Are we guaranteed that conn and conn->driver will be non-null by all
callers? Then again, I finally looked at esx_storage_driver.c for
inspiration, and see that you assumed they are valid.
return VIR_DRV_OPEN_DECLINED;
}
return VIR_DRV_OPEN_SUCCESS;
}
I'll resubmit v2 accordingly.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:21 p.m.
New subject: [libvirt] [PATCHv2] phyp: don't steal storage management from other drivers
Fix regression introduced in commit a4a287242 - basically, the
phyp storage driver should only accept the same URIs that the
main phyp driver is willing to accept. Blindly accepting all
URIs meant that the phyp storage driver was being consulted for
'virsh -c qemu:///session pool-list --all', rather than the
qemu storage driver, then since the URI was not for phyp, attempts
to then use the phyp driver crahsed because it was not initialized.
* src/phyp/phyp_driver.c (phypStorageOpen): Only accept connections
already open to a phyp driver.
---
diff from v1 - assume that for the secondary storage driver, the
master driver has already been initialized and we don't have to
reparse the URI.
Definitely much shorter, once I realized I should be copying from
esx_storage_driver.c as a prior example.
src/phyp/phyp_driver.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index 3692f2c..ee1e21b 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -3875,12 +3875,15 @@ phypDomainSetCPU(virDomainPtr dom, unsigned int nvcpus)
}
static virDrvOpenStatus
-phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED,
+phypStorageOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
int flags)
{
virCheckFlags(0, VIR_DRV_OPEN_ERROR);
+ if (conn->driver->no != VIR_DRV_PHYP)
+ return VIR_DRV_OPEN_DECLINED;
+
return VIR_DRV_OPEN_SUCCESS;
}
--
1.7.0.1
4:53 p.m.
New subject: [libvirt] [PATCHv2] phyp: don't steal storage management from other drivers
On 06/29/2010 06:21 AM, Eric Blake wrote:
Fix regression introduced in commit a4a287242 - basically, the
<snip>
diff from v1 - assume that for the secondary storage driver, the
master driver has already been initialized and we don't have to
reparse the URI.
Good work, this one also works. No segfault, things seem decent through
virsh. (I don't know the code enough to safely ACK though)
Regards and best wishes,
Justin Clift
--
Salasaga - Open Source eLearning IDE
http://www.salasaga.org
3:34 a.m.
New subject: [libvirt] [PATCHv2] phyp: don't steal storage management from other drivers
On Mon, Jun 28, 2010 at 02:21:46PM -0600, Eric Blake wrote:
Fix regression introduced in commit a4a287242 - basically, the
phyp storage driver should only accept the same URIs that the
main phyp driver is willing to accept. Blindly accepting all
URIs meant that the phyp storage driver was being consulted for
'virsh -c qemu:///session pool-list --all', rather than the
qemu storage driver, then since the URI was not for phyp, attempts
to then use the phyp driver crahsed because it was not initialized.
* src/phyp/phyp_driver.c (phypStorageOpen): Only accept connections
already open to a phyp driver.
---
diff from v1 - assume that for the secondary storage driver, the
master driver has already been initialized and we don't have to
reparse the URI.
Definitely much shorter, once I realized I should be copying from
esx_storage_driver.c as a prior example.
src/phyp/phyp_driver.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index 3692f2c..ee1e21b 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -3875,12 +3875,15 @@ phypDomainSetCPU(virDomainPtr dom, unsigned int nvcpus)
}
static virDrvOpenStatus
-phypStorageOpen(virConnectPtr conn ATTRIBUTE_UNUSED,
+phypStorageOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
int flags)
{
virCheckFlags(0, VIR_DRV_OPEN_ERROR);
+ if (conn->driver->no != VIR_DRV_PHYP)
+ return VIR_DRV_OPEN_DECLINED;
+
return VIR_DRV_OPEN_SUCCESS;
}
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 :|
8:24 a.m.
New subject: [libvirt] [PATCHv2] phyp: don't steal storage management from other drivers
On 06/29/2010 02:34 AM, Daniel P. Berrange wrote:
On Mon, Jun 28, 2010 at 02:21:46PM -0600, Eric Blake wrote:
> Fix regression introduced in commit a4a287242 - basically, the
> phyp storage driver should only accept the same URIs that the
> main phyp driver is willing to accept. Blindly accepting all
> URIs meant that the phyp storage driver was being consulted for
> 'virsh -c qemu:///session pool-list --all', rather than the
> qemu storage driver, then since the URI was not for phyp, attempts
> to then use the phyp driver crahsed because it was not initialized.
>
> * src/phyp/phyp_driver.c (phypStorageOpen): Only accept connections
> already open to a phyp driver.
ACK
Thanks; applied.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5261
days inactive
5262
days old
8 comments
5 participants
participants (5)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake -
Justin Clift -
Matthias Bolte