[libvirt] Storage segfaults after phyp patches
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
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 :|
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
2010/6/28 Eric Blake <eblake@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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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
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 :|
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (5)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake -
Justin Clift -
Matthias Bolte