libftdi Archives

Subject: Re: [PATCH] Match close with open in ftdi_usb_get_strings

From: Fahrzin Hemmati <fahhem@xxxxxxxxxx>
To: Thomas Jarosch <thomas.jarosch@xxxxxxxxxxxxx>
Cc: libftdi@xxxxxxxxxxxxxxxxxxxxxxx
Date: Sat, 9 Apr 2016 01:45:42 -0700
It seems libftdi has a lot of ...2() functions, so a major API revision may make sense to just wipe those compat layers away. In any case, I went with ftdi_usb_get_strings2() with a short compat wrapper that opens the device first so ftdi_usb_get_strings2() won't open nor close it, then closes it itself. This matches the expected behavior.

Here's the patch:

From 97369e6300c293728e78f2755f1723f49db92274 Mon Sep 17 00:00:00 2001
From: Fahrzin Hemmati <fahhem@xxxxxxxxx>
Date: Mon, 25 Jan 2016 23:23:42 -0800
Subject: [PATCH] Match close with open in ftdi_usb_get_strings

---
 src/ftdi.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/src/ftdi.c b/src/ftdi.c
index aa4b4ec..bccc56e 100644
--- a/src/ftdi.c
+++ b/src/ftdi.c
@@ -415,7 +415,56 @@ int ftdi_usb_get_strings(struct ftdi_context * ftdi, struct libusb_device * dev,
         return -1;
 
     if (ftdi->usb_dev == NULL && libusb_open(dev, &ftdi->usb_dev) < 0)
-            ftdi_error_return(-4, "libusb_open() failed");
+        ftdi_error_return(-4, "libusb_open() failed");
+
+    // ftdi->usb_dev will not be NULL when entering ftdi_usb_get_strings2(), so
+    // it won't be closed either. This allows us to close it whether we actually
+    // called libusb_open() up above or not. This matches the expected behavior
+    // (and note) for ftdi_usb_get_strings().
+    ret = ftdi_usb_get_strings2(ftdi, dev, manufacturer, mnf_len, description, desc_len, serial, serial_len);
+
+    // Only close it if it was successful, as all other return codes close
+    // before returning already.
+    if (ret == 0) {
+      ftdi_usb_close_internal(ftdi);
+    }
+    return ret;
+}
+
+/**
+    Return device ID strings from the usb device.
+
+    The parameters manufacturer, description and serial may be NULL
+    or pointer to buffers to store the fetched strings.
+
+    \param ftdi pointer to ftdi_context
+    \param dev libusb usb_dev to use
+    \param manufacturer Store manufacturer string here if not NULL
+    \param mnf_len Buffer size of manufacturer string
+    \param description Store product description string here if not NULL
+    \param desc_len Buffer size of product description string
+    \param serial Store serial string here if not NULL
+    \param serial_len Buffer size of serial string
+
+    \retval   0: all fine
+    \retval  -1: wrong arguments
+    \retval  -4: unable to open device
+    \retval  -7: get product manufacturer failed
+    \retval  -8: get product description failed
+    \retval  -9: get serial number failed
+    \retval -11: libusb_get_device_descriptor() failed
+*/
+int ftdi_usb_get_strings2(struct ftdi_context * ftdi, struct libusb_device * dev,
+                          char * manufacturer, int mnf_len, char * description, int desc_len, char * serial, int serial_len)
+{
+    struct libusb_device_descriptor desc;
+
+    if ((ftdi==NULL) || (dev==NULL))
+        return -1;
+
+    int need_open = ftdi->usb_dev == NULL;
+    if (need_open && libusb_open(dev, &ftdi->usb_dev) < 0)
+        ftdi_error_return(-4, "libusb_open() failed");
 
     if (libusb_get_device_descriptor(dev, &desc) < 0)
         ftdi_error_return(-11, "libusb_get_device_descriptor() failed");
@@ -447,7 +496,8 @@ int ftdi_usb_get_strings(struct ftdi_context * ftdi, struct libusb_device * dev,
         }
     }
 
-    ftdi_usb_close_internal (ftdi);
+    if (need_open)
+        ftdi_usb_close_internal (ftdi);
 
     return 0;
 }
-- 
2.8.0.rc3.226.g39d4020

Let me know if you need anything else!

On Sat, Mar 26, 2016 at 4:44 AM, Thomas Jarosch <thomas.jarosch@xxxxxxxxxxxxx> wrote:
Hi Fahrzin,

Am 26.01.2016 um 09:12 schrieb Fahrzin Hemmati:
> In 1.2, ftdi_usb_get_strings stops opening a device if it's already
> open. But it still closes the device, even if it didn't open it. So if I
> have an open device, there's no way to just get the string info without
> having to reopen it.
>
> The following simple patch changes the behavior to only close the
> usb_dev if it was opened, so now the open and close functions are
> matched internally.

thanks for the patch. I certainly see your point with the change.

There's one issue: Existing code might rely on the "bad"
behavior of libftdi. If we change they code now, the "client code"
might stop working as it expects libftdi to close the device.
-> leaked device pointer etc.

I'm not sure how we can fix this in a backward compatible way.

Any idea?

We could add a flag to ftdi_context, but that API design
would be even more bad :o) Unfortunately we don't have function
overloading in C or optional parameters with default arguments.

What we *could* do is a ftdi_usb_get_strings2() function and
add a compat wrapper for the old one. Once we do a major API revision
(nothing planned though), we could remove the old version.

Cheers,
Thomas




libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to libftdi+unsubscribe@xxxxxxxxxxxxxxxxxxxxxxx


Current Thread