Re: [libvirt] [PATCH 5/5]: Rewrite findLuns function
Chris Lalancette [EMAIL PROTECTED] wrote:
This rather large patch rewrites the virStorageBackendISCSIFindLUNs() function
to only rely on sysfs for finding LUNs, given a session number. Along the
way,
it also fixes the bug where we wouldn't find LUNs for older kernels (with the
block:sda format), and also (possibly) fixes a race condition where we could
try
to find the LUN before udev has finished connecting it. I say it possibly
fixes it because I haven't been able to hit it so far, but I definitely need
more testing to try and confirm.
Changes since last time:
1) Don't blindly ignore the 0'th LUN (thanks Stefan). Instead, look if the
LUNs are Direct-Access devices (as defined by the kernel); if so, they are
LUNs
we can use.
2) Fix up whitespace damage.
3) Check the return value of the scsidev strdup as pointed out by Jim.
4) Change all ISCSIADM commands to be const char *const as pointed out by
Jim.
diff -urp libvirt.sendtarget/src/storage_backend_iscsi.c
libvirt.findLun/src/storage_backend_iscsi.c
--- libvirt.sendtarget/src/storage_backend_iscsi.c2008-06-16
14:35:34.0 +0200
+++ libvirt.findLun/src/storage_backend_iscsi.c 2008-06-16
14:52:31.0 +0200
@@ -119,7 +119,7 @@ virStorageBackendISCSISession(virConnect
...
+while ((sys_dirent = readdir(sysdir))) {
+/* double-negative, so we can use the same function for scandir
below */
+if (!notdotdir(sys_dirent))
+continue;
+
+if (STREQLEN(sys_dirent-d_name, target, 6)) {
+if (sscanf(sys_dirent-d_name, target%u:%u:%u,
+ host, bus, target) != 3) {
+virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ _(Failed to parse target in sysfs
path %s: %s),
+ sysfs_path, strerror(errno));
You can remove the : %s suffix and the strerror(errno) argument,
since errno isn't relevant here. Or maybe better: leave the suffix
and use sys_dirent-d_name as the argument, so the diagnostic shows
what couldn't be parsed.
+closedir(sysdir);
+return -1;
+
The above line has just three TABs.
Best to delete it.
The rest, including patches 1-4, looks fine,
so, pending whatever tests you deem adequate,
ACK.
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5]: Rewrite findLuns function
Reposting the 5/5 patch with Jim's minor comments fixed up. Compile tested only
with his changes.
This rather large patch rewrites the virStorageBackendISCSIFindLUNs() function
to only rely on sysfs for finding LUNs, given a session number. Along the way,
it also fixes the bug where we wouldn't find LUNs for older kernels (with the
block:sda format), and also (possibly) fixes a race condition where we could try
to find the LUN before udev has finished connecting it. I say it possibly
fixes it because I haven't been able to hit it so far, but I definitely need
more testing to try and confirm.
Changes since last time:
1) Don't blindly ignore the 0'th LUN (thanks Stefan). Instead, look if the
LUNs are Direct-Access devices (as defined by the kernel); if so, they are LUNs
we can use.
2) Fix up whitespace damage.
3) Check the return value of the scsidev strdup as pointed out by Jim.
4) Change all ISCSIADM commands to be const char *const as pointed out by Jim.
Signed-off-by: Chris Lalancette [EMAIL PROTECTED]
diff -urp libvirt.1to4/src/storage_backend_iscsi.c libvirt.5/src/storage_backend_iscsi.c
--- libvirt.1to4/src/storage_backend_iscsi.c 2008-06-17 12:52:44.0 +0200
+++ libvirt.5/src/storage_backend_iscsi.c 2008-06-17 12:54:21.0 +0200
@@ -119,7 +119,7 @@ virStorageBackendISCSISession(virConnect
int vars[] = {
2,
};
-const char *prog[] = {
+const char *const prog[] = {
ISCSIADM, --mode, session, NULL
};
char *session = NULL;
@@ -153,7 +153,7 @@ virStorageBackendISCSIConnection(virConn
const char *portal,
const char *action)
{
-const char *cmdargv[] = {
+const char *const cmdargv[] = {
ISCSIADM, --mode, node, --portal, portal,
--targetname, pool-def-source.devices[0].path, action, NULL
};
@@ -164,110 +164,30 @@ virStorageBackendISCSIConnection(virConn
return 0;
}
-
static int
-virStorageBackendISCSIMakeLUN(virConnectPtr conn,
- virStoragePoolObjPtr pool,
- char **const groups,
- void *data)
+virStorageBackendISCSINewLun(virConnectPtr conn, virStoragePoolObjPtr pool,
+ unsigned int lun, const char *dev)
{
virStorageVolDefPtr vol;
int fd = -1;
-unsigned int target, channel, id, lun;
-char lunid[100];
-int opentries = 0;
char *devpath = NULL;
-char *session = data;
-char sysfs_path[PATH_MAX];
-char *dev = NULL;
-DIR *sysdir;
-struct dirent *block_dirent;
-struct stat sbuf;
-int len;
-
-if ((virStrToLong_ui(groups[0], NULL, 10, target) 0) ||
-(virStrToLong_ui(groups[1], NULL, 10, channel) 0) ||
-(virStrToLong_ui(groups[2], NULL, 10, id) 0) ||
-(virStrToLong_ui(groups[3], NULL, 10, lun) 0)) {
-virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, %s,
- _(Failed parsing iscsiadm commands));
-return -1;
-}
-
-if (lun == 0) {
-/* the 0'th LUN isn't a real LUN, it's just a control LUN; skip it */
-return 0;
-}
-
-snprintf(sysfs_path, PATH_MAX,
- /sys/class/iscsi_session/session%s/device/
- target%d:%d:%d/%d:%d:%d:%d/block,
- session, target, channel, id, target, channel, id, lun);
-
-if (stat(sysfs_path, sbuf) 0) {
-/* block path in subdir didn't exist; this is unexpected, so fail */
-virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
- _(Failed to find the sysfs path for %d:%d:%d:%d: %s),
- target, channel, id, lun, strerror(errno));
-return -1;
-}
-
-sysdir = opendir(sysfs_path);
-if (sysdir == NULL) {
-/* we failed for some reason; return an error */
-virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
- _(Failed to opendir sysfs path %s: %s),
- sysfs_path, strerror(errno));
-return -1;
-}
-
-while ((block_dirent = readdir(sysdir)) != NULL) {
-len = strlen(block_dirent-d_name);
-if ((len == 1 block_dirent-d_name[0] == '.') ||
-(len == 2 block_dirent-d_name[0] == '.' block_dirent-d_name[1] == '.')) {
-/* the . and .. directories; just skip them */
-continue;
-}
-
-/* OK, not . or ..; let's see if it is a SCSI device */
-if (len 2
-block_dirent-d_name[0] == 's'
-block_dirent-d_name[1] == 'd') {
-/* looks like a scsi device, smells like scsi device; it must be
- a scsi device */
-dev = strdup(block_dirent-d_name);
-break;
-}
-}
-closedir(sysdir);
-
-if (dev == NULL) {
-/* we didn't find the sd? device we were looking for; fail */
-virStorageReportError(conn,
Re: [libvirt] [PATCH 5/5]: Rewrite findLuns function
On Tue, Jun 17, 2008 at 01:02:30PM +0200, Chris Lalancette wrote: Reposting the 5/5 patch with Jim's minor comments fixed up. Compile tested only with his changes. This rather large patch rewrites the virStorageBackendISCSIFindLUNs() function to only rely on sysfs for finding LUNs, given a session number. Along the way, it also fixes the bug where we wouldn't find LUNs for older kernels (with the block:sda format), and also (possibly) fixes a race condition where we could try to find the LUN before udev has finished connecting it. I say it possibly fixes it because I haven't been able to hit it so far, but I definitely need more testing to try and confirm. I guess it got sufficient review and feedback, +1 Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5]: Rewrite findLuns function
Chris Lalancette wrote: Reposting the 5/5 patch with Jim's minor comments fixed up. Compile tested only with his changes. This rather large patch rewrites the virStorageBackendISCSIFindLUNs() function to only rely on sysfs for finding LUNs, given a session number. Along the way, it also fixes the bug where we wouldn't find LUNs for older kernels (with the block:sda format), and also (possibly) fixes a race condition where we could try to find the LUN before udev has finished connecting it. I say it possibly fixes it because I haven't been able to hit it so far, but I definitely need more testing to try and confirm. Changes since last time: 1) Don't blindly ignore the 0'th LUN (thanks Stefan). Instead, look if the LUNs are Direct-Access devices (as defined by the kernel); if so, they are LUNs we can use. 2) Fix up whitespace damage. 3) Check the return value of the scsidev strdup as pointed out by Jim. 4) Change all ISCSIADM commands to be const char *const as pointed out by Jim. Signed-off-by: Chris Lalancette [EMAIL PROTECTED] Committed Chris Lalancette -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5]: Rewrite findLuns function
This rather large patch rewrites the virStorageBackendISCSIFindLUNs() function
to only rely on sysfs for finding LUNs, given a session number. Along the way,
it also fixes the bug where we wouldn't find LUNs for older kernels (with the
block:sda format), and also (possibly) fixes a race condition where we could try
to find the LUN before udev has finished connecting it. I say it possibly
fixes it because I haven't been able to hit it so far, but I definitely need
more testing to try and confirm.
Changes since last time:
1) Don't blindly ignore the 0'th LUN (thanks Stefan). Instead, look if the
LUNs are Direct-Access devices (as defined by the kernel); if so, they are LUNs
we can use.
2) Fix up whitespace damage.
3) Check the return value of the scsidev strdup as pointed out by Jim.
4) Change all ISCSIADM commands to be const char *const as pointed out by Jim.
Signed-off-by: Chris Lalancette [EMAIL PROTECTED]
diff -urp libvirt.sendtarget/src/storage_backend_iscsi.c libvirt.findLun/src/storage_backend_iscsi.c
--- libvirt.sendtarget/src/storage_backend_iscsi.c 2008-06-16 14:35:34.0 +0200
+++ libvirt.findLun/src/storage_backend_iscsi.c 2008-06-16 14:52:31.0 +0200
@@ -119,7 +119,7 @@ virStorageBackendISCSISession(virConnect
int vars[] = {
2,
};
-const char *prog[] = {
+const char *const prog[] = {
ISCSIADM, --mode, session, NULL
};
char *session = NULL;
@@ -153,7 +153,7 @@ virStorageBackendISCSIConnection(virConn
const char *portal,
const char *action)
{
-const char *cmdargv[] = {
+const char *const cmdargv[] = {
ISCSIADM, --mode, node, --portal, portal,
--targetname, pool-def-source.devices[0].path, action, NULL
};
@@ -164,110 +164,30 @@ virStorageBackendISCSIConnection(virConn
return 0;
}
-
static int
-virStorageBackendISCSIMakeLUN(virConnectPtr conn,
- virStoragePoolObjPtr pool,
- char **const groups,
- void *data)
+virStorageBackendISCSINewLun(virConnectPtr conn, virStoragePoolObjPtr pool,
+ unsigned int lun, const char *dev)
{
virStorageVolDefPtr vol;
int fd = -1;
-unsigned int target, channel, id, lun;
-char lunid[100];
-int opentries = 0;
char *devpath = NULL;
-char *session = data;
-char sysfs_path[PATH_MAX];
-char *dev = NULL;
-DIR *sysdir;
-struct dirent *block_dirent;
-struct stat sbuf;
-int len;
-
-if ((virStrToLong_ui(groups[0], NULL, 10, target) 0) ||
-(virStrToLong_ui(groups[1], NULL, 10, channel) 0) ||
-(virStrToLong_ui(groups[2], NULL, 10, id) 0) ||
-(virStrToLong_ui(groups[3], NULL, 10, lun) 0)) {
-virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, %s,
- _(Failed parsing iscsiadm commands));
-return -1;
-}
-
-if (lun == 0) {
-/* the 0'th LUN isn't a real LUN, it's just a control LUN; skip it */
-return 0;
-}
-
-snprintf(sysfs_path, PATH_MAX,
- /sys/class/iscsi_session/session%s/device/
- target%d:%d:%d/%d:%d:%d:%d/block,
- session, target, channel, id, target, channel, id, lun);
-
-if (stat(sysfs_path, sbuf) 0) {
-/* block path in subdir didn't exist; this is unexpected, so fail */
-virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
- _(Failed to find the sysfs path for %d:%d:%d:%d: %s),
- target, channel, id, lun, strerror(errno));
-return -1;
-}
-
-sysdir = opendir(sysfs_path);
-if (sysdir == NULL) {
-/* we failed for some reason; return an error */
-virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
- _(Failed to opendir sysfs path %s: %s),
- sysfs_path, strerror(errno));
-return -1;
-}
-
-while ((block_dirent = readdir(sysdir)) != NULL) {
-len = strlen(block_dirent-d_name);
-if ((len == 1 block_dirent-d_name[0] == '.') ||
-(len == 2 block_dirent-d_name[0] == '.' block_dirent-d_name[1] == '.')) {
-/* the . and .. directories; just skip them */
-continue;
-}
-
-/* OK, not . or ..; let's see if it is a SCSI device */
-if (len 2
-block_dirent-d_name[0] == 's'
-block_dirent-d_name[1] == 'd') {
-/* looks like a scsi device, smells like scsi device; it must be
- a scsi device */
-dev = strdup(block_dirent-d_name);
-break;
-}
-}
-closedir(sysdir);
-
-if (dev == NULL) {
-/* we didn't find the sd? device we were looking for; fail */
-virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
- _(Failed to find
