[PATCH] Match close with open in ftdi_usb_get_strings

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] Match close with open in ftdi_usb_get_strings

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.

I don't know whether or where to mention this change. Should I add it to a changelog anywhere?

Best regards,
Fahrzin Hemmati

From 171c4747dd295d3b6085f09fa56e2c766d0f6cd9 Mon Sep 17 00:00:00 2001
From: Fahrzin Hemmati <[hidden email]>
Date: Mon, 25 Jan 2016 23:23:42 -0800
Subject: [PATCH] Match close with open in ftdi_usb_get_strings
                                                                                                                                      
---
 src/ftdi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
                                                                                                                                      
diff --git a/src/ftdi.c b/src/ftdi.c
index aa4b4ec..8ebddbd 100644
--- a/src/ftdi.c
+++ b/src/ftdi.c
@@ -414,7 +414,8 @@ int ftdi_usb_get_strings(struct ftdi_context * ftdi, struct libusb_device * dev,
     if ((ftdi==NULL) || (dev==NULL))
         return -1;
                                                                                                                                      
- if (ftdi->usb_dev == NULL && libusb_open(dev, &ftdi->usb_dev) < 0)
+ 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)
@@ -447,7 +448,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.7.0.rc3.207.g0ac5344 


libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to [hidden email]


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Match close with open in ftdi_usb_get_strings

Thomas Jarosch
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 [hidden email]  

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Match close with open in ftdi_usb_get_strings

Fahrzin Hemmati
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 <[hidden email]>
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 <[hidden email]> 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 [hidden email]


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Match close with open in ftdi_usb_get_strings

Thomas Jarosch
Hi Fahrzin,

Am 09.04.2016 um 10:45 schrieb Fahrzin Hemmati:
> 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:

thanks, applied!

I had to manually copy the patch hunks over since the HTML email mangled
the diff lines. Patches are much better sent as text only messages ;)
Please double check I didn't break anything in the process.

I've massaged the changelog a bit, added the function to ftdi.h
and to the python wrapper.


libftdi has a few xxx2() functions to ensure backward compatibility.
If we ever change the API to be opaque like it is for the EEPROM
(get and set functions), then a major API revision might make sense.
I just don't want to break flashrom, openocd and a ton of other
programs for no real reason.

Cheers,
Thomas


--
libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to [hidden email]  

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Match close with open in ftdi_usb_get_strings

Fahrzin Hemmati
Ah, sorry about that. gmail doesn't make 'text' vs 'html' emails an option, I'll look for something that does next time.

The EEPROM being opaque actually bothered me in an application (I needed to replace some bits not available otherwise), but thankfully (accidentally?) the Python bindings grab the struct directly so I was able to do what I needed. I wouldn't recommend going opaque unless you've made all the fields accessible.

Thanks!


On Mon, Apr 18, 2016 2:06 PM, Thomas Jarosch [hidden email] wrote:
Hi Fahrzin, Am 09.04.2016 um 10:45 schrieb Fahrzin Hemmati: > 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: thanks, applied! I had to manually copy the patch hunks over since the HTML email mangled the diff lines. Patches are much better sent as text only messages ;) Please double check I didn't break anything in the process. I've massaged the changelog a bit, added the function to ftdi.h and to the python wrapper. libftdi has a few xxx2() functions to ensure backward compatibility. If we ever change the API to be opaque like it is for the EEPROM (get and set functions), then a major API revision might make sense. I just don't want to break flashrom, openocd and a ton of other programs for no real reason. Cheers, Thomas -- libftdi - see http://www.intra2net.com/en/developer/libftdi for details. To unsubscribe send a mail to [hidden email]


libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to [hidden email]


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Match close with open in ftdi_usb_get_strings

Thomas Jarosch
Am 18.04.2016 um 23:14 schrieb Fahrzin Hemmati:
> The EEPROM being opaque actually bothered me in an application (I needed
> to replace some bits not available otherwise), but thankfully
> (accidentally?) the Python bindings grab the struct directly so I was
> able to do what I needed.

probably it was available accidentally ;)

> I wouldn't recommend going opaque unless
> you've made all the fields accessible.

no plans from my side to go opaque for now.

Thomas


--
libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to [hidden email]  

Loading...