Re: [PATCH 01/12] staging: usbip: userspace: migrate usbip_bind to libudev
From: Shuah Khan
Date: Thu Mar 06 2014 - 11:15:27 EST
On 03/04/2014 12:10 PM, Valentina Manea wrote:
This patch adds autoconf check for libudev and migrates
usbip_bind to the new library.
libsysfs will still be used until all userspace is modified.
Signed-off-by: Valentina Manea <valentina.manea.m@xxxxxxxxx>
---
drivers/staging/usbip/userspace/configure.ac | 6 +
.../staging/usbip/userspace/libsrc/usbip_common.h | 9 ++
drivers/staging/usbip/userspace/src/Makefile.am | 3 +-
drivers/staging/usbip/userspace/src/sysfs_utils.c | 36 +++++
drivers/staging/usbip/userspace/src/sysfs_utils.h | 8 ++
drivers/staging/usbip/userspace/src/usbip_bind.c | 149 +++++++++------------
drivers/staging/usbip/userspace/src/utils.c | 51 +++----
7 files changed, 136 insertions(+), 126 deletions(-)
create mode 100644 drivers/staging/usbip/userspace/src/sysfs_utils.c
create mode 100644 drivers/staging/usbip/userspace/src/sysfs_utils.h
diff --git a/drivers/staging/usbip/userspace/configure.ac b/drivers/staging/usbip/userspace/configure.ac
index 0ee5d92..a5193c6 100644
--- a/drivers/staging/usbip/userspace/configure.ac
+++ b/drivers/staging/usbip/userspace/configure.ac
@@ -50,6 +50,12 @@ AC_CHECK_HEADER([sysfs/libsysfs.h],
[AC_MSG_ERROR([Missing sysfs2 library!])])],
[AC_MSG_ERROR([Missing /usr/include/sysfs/libsysfs.h])])
+AC_CHECK_HEADER([libudev.h],
+ [AC_CHECK_LIB([udev], [udev_new],
+ [LIBS="$LIBS -ludev"],
+ [AC_MSG_ERROR([Missing udev library!])])],
+ [AC_MSG_ERROR([Missing /usr/include/libudev.h])])
+
# Checks for libwrap library.
AC_MSG_CHECKING([whether to use the libwrap (TCP wrappers) library])
AC_ARG_WITH([tcp-wrappers],
diff --git a/drivers/staging/usbip/userspace/libsrc/usbip_common.h b/drivers/staging/usbip/userspace/libsrc/usbip_common.h
index 2cb81b3..565ac78 100644
--- a/drivers/staging/usbip/userspace/libsrc/usbip_common.h
+++ b/drivers/staging/usbip/userspace/libsrc/usbip_common.h
@@ -29,6 +29,15 @@
#define USBIP_HOST_DRV_NAME "usbip-host"
#define USBIP_VHCI_DRV_NAME "vhci_hcd"
+/* sysfs constants */
+#define SYSFS_MNT_PATH "/sys"
+#define SYSFS_BUS_NAME "bus"
+#define SYSFS_BUS_TYPE "usb"
+#define SYSFS_DRIVERS_NAME "drivers"
+
+#define SYSFS_PATH_MAX 256
+#define SYSFS_BUS_ID_SIZE 32
I wish we have some global defines we could use.
+
extern int usbip_use_syslog;
extern int usbip_use_stderr;
extern int usbip_use_debug ;
diff --git a/drivers/staging/usbip/userspace/src/Makefile.am b/drivers/staging/usbip/userspace/src/Makefile.am
index b4f8c4b..6c91bcb 100644
--- a/drivers/staging/usbip/userspace/src/Makefile.am
+++ b/drivers/staging/usbip/userspace/src/Makefile.am
@@ -6,7 +6,8 @@ sbin_PROGRAMS := usbip usbipd
usbip_SOURCES := usbip.h utils.h usbip.c utils.c usbip_network.c \
usbip_attach.c usbip_detach.c usbip_list.c \
- usbip_bind.c usbip_unbind.c usbip_port.c
+ usbip_bind.c usbip_unbind.c usbip_port.c \
+ sysfs_utils.c
usbipd_SOURCES := usbip_network.h usbipd.c usbip_network.c
diff --git a/drivers/staging/usbip/userspace/src/sysfs_utils.c b/drivers/staging/usbip/userspace/src/sysfs_utils.c
new file mode 100644
index 0000000..2c362d1
--- /dev/null
+++ b/drivers/staging/usbip/userspace/src/sysfs_utils.c
@@ -0,0 +1,36 @@
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <errno.h>
+
+#include "sysfs_utils.h"
+#include "usbip_common.h"
+
+int write_sysfs_attribute(const char *attr_path, const char *new_value,
+ size_t len)
+{
+ int fd;
+ int length;
+
+ if (attr_path == NULL || new_value == NULL || len == 0) {
+ dbg("Invalid values provided for attribute %s.", attr_path);
+ errno = EINVAL;
+ return -1;
+ }
+
+ if ((fd = open(attr_path, O_WRONLY)) < 0) {
+ dbg("Error opening attribute %s.", attr_path);
+ return -1;
+ }
+
+ length = write(fd, new_value, len);
+ if (length < 0) {
+ dbg("Error writing to attribute %s.", attr_path);
+ close(fd);
+ return -1;
+ }
+
+ close(fd);
+
+ return 0;
+}
diff --git a/drivers/staging/usbip/userspace/src/sysfs_utils.h b/drivers/staging/usbip/userspace/src/sysfs_utils.h
new file mode 100644
index 0000000..32ac1d1
--- /dev/null
+++ b/drivers/staging/usbip/userspace/src/sysfs_utils.h
@@ -0,0 +1,8 @@
+
+#ifndef __SYSFS_UTILS_H
+#define __SYSFS_UTILS_H
+
+int write_sysfs_attribute(const char *attr_path, const char *new_value,
+ size_t len);
+
+#endif
diff --git a/drivers/staging/usbip/userspace/src/usbip_bind.c b/drivers/staging/usbip/userspace/src/usbip_bind.c
index 8cfd2db..d122089 100644
--- a/drivers/staging/usbip/userspace/src/usbip_bind.c
+++ b/drivers/staging/usbip/userspace/src/usbip_bind.c
@@ -16,7 +16,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#include <sysfs/libsysfs.h>
+#include <libudev.h>
#include <errno.h>
#include <stdio.h>
@@ -28,6 +28,7 @@
#include "usbip_common.h"
#include "utils.h"
#include "usbip.h"
+#include "sysfs_utils.h"
enum unbind_status {
UNBIND_ST_OK,
@@ -48,135 +49,94 @@ void usbip_bind_usage(void)
/* call at unbound state */
static int bind_usbip(char *busid)
{
- char bus_type[] = "usb";
char attr_name[] = "bind";
- char sysfs_mntpath[SYSFS_PATH_MAX];
char bind_attr_path[SYSFS_PATH_MAX];
- struct sysfs_attribute *bind_attr;
- int failed = 0;
- int rc, ret = -1;
-
- rc = sysfs_get_mnt_path(sysfs_mntpath, SYSFS_PATH_MAX);
- if (rc < 0) {
- err("sysfs must be mounted: %s", strerror(errno));
- return -1;
- }
+ int rc = -1;
snprintf(bind_attr_path, sizeof(bind_attr_path), "%s/%s/%s/%s/%s/%s",
- sysfs_mntpath, SYSFS_BUS_NAME, bus_type, SYSFS_DRIVERS_NAME,
- USBIP_HOST_DRV_NAME, attr_name);
+ SYSFS_MNT_PATH, SYSFS_BUS_NAME, SYSFS_BUS_TYPE,
+ SYSFS_DRIVERS_NAME, USBIP_HOST_DRV_NAME, attr_name);
+ dbg("bind attribute path: %s", bind_attr_path);
Is this needed since there are error messages in the error paths?
- bind_attr = sysfs_open_attribute(bind_attr_path);
- if (!bind_attr) {
- dbg("problem getting bind attribute: %s", strerror(errno));
- return -1;
- }
-
- rc = sysfs_write_attribute(bind_attr, busid, SYSFS_BUS_ID_SIZE);
+ rc = write_sysfs_attribute(bind_attr_path, busid, strlen(busid));
if (rc < 0) {
- dbg("bind driver at %s failed", busid);
- failed = 1;
+ dbg("Error binding device %s to driver: %s", busid,
+ strerror(errno));
I think it would make sense to have this as an error as opposed to debug
only message.
+ return -1;
}
- if (!failed)
- ret = 0;
-
- sysfs_close_attribute(bind_attr);
-
- return ret;
+ return 0;
}
/* buggy driver may cause dead lock */
static int unbind_other(char *busid)
{
- char bus_type[] = "usb";
- struct sysfs_device *busid_dev;
- struct sysfs_device *dev;
- struct sysfs_driver *drv;
- struct sysfs_attribute *unbind_attr;
- struct sysfs_attribute *bDevClass;
- int rc;
enum unbind_status status = UNBIND_ST_OK;
- busid_dev = sysfs_open_device(bus_type, busid);
- if (!busid_dev) {
- dbg("sysfs_open_device %s failed: %s", busid, strerror(errno));
- return -1;
- }
- dbg("busid path: %s", busid_dev->path);
+ char attr_name[] = "unbind";
+ char unbind_attr_path[SYSFS_PATH_MAX];
+ int rc = -1;
- bDevClass = sysfs_get_device_attr(busid_dev, "bDeviceClass");
- if (!bDevClass) {
- dbg("problem getting device attribute: %s",
- strerror(errno));
+ struct udev *udev;
+ struct udev_device *dev;
+ const char *driver;
+ const char *bDevClass;
+
+ /* Create libudev context. */
+ udev = udev_new();
+
+ /* Get the device. */
+ dev = udev_device_new_from_subsystem_sysname(udev, "usb", busid);
+ if (!dev) {
+ dbg("Unable to find device with bus ID %s.", busid);
goto err_close_busid_dev;
}
- if (!strncmp(bDevClass->value, "09", bDevClass->len)) {
- dbg("skip unbinding of hub");
+ /* Check what kind of device it is. */
+ bDevClass = udev_device_get_sysattr_value(dev, "bDeviceClass");
+ if (!bDevClass) {
+ dbg("Unable to get bDevClass device attribute.");
goto err_close_busid_dev;
}
- dev = sysfs_open_device(bus_type, busid);
- if (!dev) {
- dbg("could not open device: %s",
- strerror(errno));
+ if (!strncmp(bDevClass, "09", strlen(bDevClass))) {
+ dbg("Skip unbinding of hub.");
goto err_close_busid_dev;
}
- dbg("%s -> %s", dev->name, dev->driver_name);
-
- if (!strncmp("unknown", dev->driver_name, SYSFS_NAME_LEN)) {
- /* unbound interface */
- sysfs_close_device(dev);
+ /* Get the device driver. */
+ driver = udev_device_get_driver(dev);
+ if (!driver) {
+ /* No driver bound to this device. */
goto out;
}
- if (!strncmp(USBIP_HOST_DRV_NAME, dev->driver_name,
- SYSFS_NAME_LEN)) {
- /* already bound to usbip-host */
+ if (!strncmp(USBIP_HOST_DRV_NAME, driver,
+ strlen(USBIP_HOST_DRV_NAME))) {
+ /* Already bound to usbip-host. */
status = UNBIND_ST_USBIP_HOST;
- sysfs_close_device(dev);
goto out;
}
- /* unbinding */
- drv = sysfs_open_driver(bus_type, dev->driver_name);
- if (!drv) {
- dbg("could not open device driver on %s: %s",
- dev->name, strerror(errno));
- goto err_close_intf_dev;
- }
- dbg("device driver: %s", drv->path);
-
- unbind_attr = sysfs_get_driver_attr(drv, "unbind");
- if (!unbind_attr) {
- dbg("problem getting device driver attribute: %s",
- strerror(errno));
- goto err_close_intf_drv;
- }
+ /* Unbind device from driver. */
+ snprintf(unbind_attr_path, sizeof(unbind_attr_path), "%s/%s/%s/%s/%s/%s",
+ SYSFS_MNT_PATH, SYSFS_BUS_NAME, SYSFS_BUS_TYPE,
+ SYSFS_DRIVERS_NAME, driver, attr_name);
+ dbg("unbind attribute path: %s", unbind_attr_path);
I think this could be removed since there are error messages in failure
legs. I am finding usbip userspace is rather prolific with debug. So it
would help if err() is used in error legs as opposed to dbg() and use
dbg() only for debug messages.
- rc = sysfs_write_attribute(unbind_attr, dev->bus_id,
- SYSFS_BUS_ID_SIZE);
+ rc = write_sysfs_attribute(unbind_attr_path, busid, strlen(busid));
if (rc < 0) {
- /* NOTE: why keep unbinding other interfaces? */
- dbg("unbind driver at %s failed", dev->bus_id);
- status = UNBIND_ST_FAILED;
+ dbg("Error unbinding device %s from driver.", busid);
This could be err() instead of dbg()
+ goto err_close_busid_dev;
}
- sysfs_close_driver(drv);
- sysfs_close_device(dev);
-
goto out;
-err_close_intf_drv:
- sysfs_close_driver(drv);
-err_close_intf_dev:
- sysfs_close_device(dev);
err_close_busid_dev:
status = UNBIND_ST_FAILED;
out:
- sysfs_close_device(busid_dev);
+ udev_device_unref(dev);
+ udev_unref(udev);
return status;
}
@@ -184,6 +144,17 @@ out:
static int bind_device(char *busid)
{
int rc;
+ struct udev *udev;
+ struct udev_device *dev;
+
+ /* Check whether the device with this bus ID exists. */
+ udev = udev_new();
+ dev = udev_device_new_from_subsystem_sysname(udev, "usb", busid);
+ if (!dev) {
+ err("Device with the specified bus ID does not exist.");
+ return -1;
+ }
+ udev_unref(udev);
rc = unbind_other(busid);
if (rc == UNBIND_ST_FAILED) {
diff --git a/drivers/staging/usbip/userspace/src/utils.c b/drivers/staging/usbip/userspace/src/utils.c
index 2d4966e..c75acac 100644
--- a/drivers/staging/usbip/userspace/src/utils.c
+++ b/drivers/staging/usbip/userspace/src/utils.c
@@ -16,61 +16,40 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#include <sysfs/libsysfs.h>
-
#include <errno.h>
#include <stdio.h>
#include <string.h>
#include "usbip_common.h"
#include "utils.h"
+#include "sysfs_utils.h"
int modify_match_busid(char *busid, int add)
{
- char bus_type[] = "usb";
char attr_name[] = "match_busid";
- char buff[SYSFS_BUS_ID_SIZE + 4];
- char sysfs_mntpath[SYSFS_PATH_MAX];
+ char command[SYSFS_BUS_ID_SIZE + 4];
char match_busid_attr_path[SYSFS_PATH_MAX];
- struct sysfs_attribute *match_busid_attr;
- int rc, ret = 0;
-
- if (strnlen(busid, SYSFS_BUS_ID_SIZE) > SYSFS_BUS_ID_SIZE - 1) {
- dbg("busid is too long");
- return -1;
- }
-
- rc = sysfs_get_mnt_path(sysfs_mntpath, SYSFS_PATH_MAX);
- if (rc < 0) {
- err("sysfs must be mounted: %s", strerror(errno));
- return -1;
- }
+ int rc;
snprintf(match_busid_attr_path, sizeof(match_busid_attr_path),
- "%s/%s/%s/%s/%s/%s", sysfs_mntpath, SYSFS_BUS_NAME, bus_type,
- SYSFS_DRIVERS_NAME, USBIP_HOST_DRV_NAME, attr_name);
-
- match_busid_attr = sysfs_open_attribute(match_busid_attr_path);
- if (!match_busid_attr) {
- dbg("problem getting match_busid attribute: %s",
- strerror(errno));
- return -1;
- }
+ "%s/%s/%s/%s/%s/%s", SYSFS_MNT_PATH, SYSFS_BUS_NAME,
+ SYSFS_BUS_TYPE, SYSFS_DRIVERS_NAME, USBIP_HOST_DRV_NAME,
+ attr_name);
+ dbg("match_busid attribute path: %s", match_busid_attr_path);
if (add)
- snprintf(buff, SYSFS_BUS_ID_SIZE + 4, "add %s", busid);
+ snprintf(command, SYSFS_BUS_ID_SIZE + 4, "add %s", busid);
else
- snprintf(buff, SYSFS_BUS_ID_SIZE + 4, "del %s", busid);
+ snprintf(command, SYSFS_BUS_ID_SIZE + 4, "del %s", busid);
- dbg("write \"%s\" to %s", buff, match_busid_attr->path);
+ dbg("write \"%s\" to %s", command, match_busid_attr_path);
This messge could be removed.
- rc = sysfs_write_attribute(match_busid_attr, buff, sizeof(buff));
+ rc = write_sysfs_attribute(match_busid_attr_path, command,
+ sizeof(command));
if (rc < 0) {
- dbg("failed to write match_busid: %s", strerror(errno));
- ret = -1;
+ dbg("Failed to write match_busid: %s", strerror(errno));
+ return -1;
}
- sysfs_close_attribute(match_busid_attr);
-
- return ret;
+ return 0;
}
Good work. Thanks for doing this. You have my Reviewed-by after making
the recommended changes.
Reviewed-by: Shuah Khan <shuah.kh@xxxxxxxxxxx>
-- Shuah
--
Shuah Khan
Senior Linux Kernel Developer - Open Source Group
Samsung Research America(Silicon Valley)
shuah.kh@xxxxxxxxxxx | (970) 672-0658
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/