Opened 8 years ago

Closed 8 years ago

#1528 closed enhancement (fixed)

Interface detection on Linux refactoring

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20120514
Component: libdhcp Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Interface detection code on Linux requires refactoring. In particular:

  • remove C structures (e.g. lists) and replace them with C++ (e.g. std::vector)
  • use more meaningful variable names
  • see other comments of ticket #1237

Subtickets

Change History (12)

comment:1 Changed 8 years ago by stephen

  • Milestone changed from DHCP 2012 to Sprint-DHCP-20120220

comment:2 Changed 8 years ago by tomek

  • Owner set to tomek
  • Status changed from new to accepted

comment:3 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen
  • Status changed from accepted to assigned

Code checked in. Ready for review.

comment:4 Changed 8 years ago by tomek

  • Owner changed from stephen to UnAssigned

comment:5 Changed 8 years ago by tomek

  • Status changed from assigned to reviewing

comment:6 follow-ups: Changed 8 years ago by stephen

  • Owner changed from UnAssigned to tomek

src/lib/dhcp/iface_mgr_linux.cc
Should update copyright to 2011-2012.

Really needs an introduction explaining the logic of Linux interfaces, e.g. what is netlink, what messages are being used, what needs to happen etc.

It is not clear why a vector of pointers is required to have many nlmsg structures pointing to the same header. Can this comment be expanded?

There is no need to use the keyword "struct" in the declaration of a vector of pointers to nlmsghdr objects.

"Holds information about interface or address attributes." This does not hold the information, it merely points to it. Also, what is the significance of "rt" in "rtaddtr", "RTattribs" and rtnl_* - Real-Time? (What I'm trying to get across is that the reader may not know much about the Netlink interface as it is not as widely-used as, say, the socket interface. Adding comments to guide them makes it much easier to follow the logic and to spot errors.)

Both NetlinkMessages and RTattribs objects hold pointers to other objects. This immediately raises the question of what is expected to free up the memory pointed to by them. This should be documented. (In fact, looking further on in the code I note that there is a special function that deletes the pointed-to objects. In this case, why not declare the vectors as vectors of scoped_ptrs? Then the objects will be automatically deleted when the vector is deleted.)

Typo: "addres info", should be "address info". Also "any code reuse" should be "any code reusable".

The assumption that IFA_MAX is shorter than IFLA_MAX can be explicitly checked during compilation using BOOST_STATIC_ASSERT.

No need to use "struct" in the declaration of rtnl_handle.

Should document why __u32 is used as a data type rather than uint32_t in rtnl_handle.

sndbuf/rcvbuf. These variables sound like send buffer and receive buffer, whereas in fact they are the size of the buffer. SNDBUF_SIZE and RCVBUF_SIZE (or xxx_MAX) would be better names for them.

rtnl_send_request
The members of "struct req" do not need to include the "struct" declaration in C++.

In rtnl, two structs are declared with another one. The enclosing struct is then sent via a sendto() call with sizeof(enclosing-struct) as the size. Are we sure that the compiler is not aligning the member structs on natural boundaries? In other words, in some circumstances could there be unused bytes between the two? Addition of an assertion to check that would be useful.

Spaces around the binary operator "|" are required.

A comment as to why handle.dump is set to ++handle.seq would be useful - that is non-obvious, as handle is just described as a "context structure". Also, I would explicitly put that assignment as a separate statement - it's easy to overlook.

In the call to sendto(), the casting should be done using C++ constructs.

rtnl_store_reply
"Appends nlmsg to a storage" should be "Appends a copy of a NetLink message to the message store".

Why the redundant parentheses in the call to "new"?

No need to check if copy == NULL as the "new" operation would throw an exception if this were true. (But if it didn't, the check should follow the memcpy() call, which would segfault if the destination were null.)

parse_rtattr
Reading this raised the following question: why is a boost array used to store pointers to the rtattr structure rather than a vector (as is the case for a NetlinkMessage pointer)? (As it is, if the number of rtattr structures is too many for the array, the excess are silently ignored.)

In the "if" block, braces are required even for single-line statements.

What are the RTA_OK and RTA_NEXT macros? Some comments are required here.

ipaddrs_get
This parses addr_info - but addr_info is described as a collection of parsed netlink messages. More information about what this function actually does is required.

Why is addr declared with a length of 16? A symbolic constant should be used.

struct ifaddrmsg *ifa" should be ifaddrmsg* ifa (note the absence of "struct" and the position of the "*".)

Should use C++ construct to cast to "ifaddrmsg*".

Redundant space after "(" in the "if" statement.

No need to use "struct rtattr*" in call to fill() - "rtattr*" should be sufficient.

The two "if" blocks checking ifa->family are identical, with the exception of the address family in the call to IOAddress::from_bytes and the size of the data in the memcpy() call. The common code should be abstracted into a separate function. (Additionally: (a) The "if" blocks in this code should include braces, (b) C++ casts should be used in the call to memcpy (and the second argument should be cast to const void*) and (c) symbolic constants should be used as the length of data to be copied in the call to memcpy.

rtnl_process_reply
It would be helpful if the header were to say what processing was going to be done. (And comments in the code to say that as well.)

Using "rth" as the designation for "netlink parameters" seems a bit obscure.

No need to use the "struct" keyword when declaring variables.

Declaration of "status" should be at first use. (The same goes for the declaration of "header".)

Does iov.iov_len vary between iterations of the loop? If not, why not set it outside?

The "if" statement checking for EINTR should have the body within braces.

If recvmsg() returns a value less than zero, it does not automatically mean that there is an overrun - it could be any one of a number of errors (determined by errno). Including errno in the exception thrown would be useful for diagnostic purposes.

I notice that here, if recvmsg fails due to an interrupted system call status, the call is retried. This does not happen in the recvmsg() calls in iface_mgr.cc. If this is an omission, I would create a separate function that calls recvmsg and handles the case of an interrupted system call.

Assignment of address of "buf" to "header" - use C++ casts.

The comment "Why we received this anyway?" does not give much information. From the code, better would be something like "Received a message not addressed to our process, or not with a sequence number we are expecting. Ignore, and look at the next one."

release_list
This would not be needed if the object were a vector of scoped_ptrs - a call to vector::clear() would handle the destruction of the pointed-to objects.

detectIfaces
The comment "link info" on a variable named "link_info" is not really helpful (same for addr_info).

Start comments with a capital letter.

"len" can be declared within the main loop, at the point of first use.

Within the main "for" loop, why not set "len" with a single statement. And what is "len" being set to?

The "valgrind" report has to be bogus - iface_name is scope-limited to the "for" loop and takes a copy of its argument. However, it is not clear what the string represents - a comment would be useful. And are we sure the string is non-null?

Looking at the declaration of iface:

  • The index is set in the constructor
  • The hardware type is set via a direct access to a member of Iface
  • The flags are set through a call to the method setFlagsx().

This is inconsistent.

The inconsistency is carried through to the setting of the mac_len_ and mac_ members: these should be zeroed in the the constructor. Similarly the copying of the MAC from the attribs_table should be handled by a single call to an Iface method, passing to it the address of the data and length of the data.

Note that if exceptions are thrown, the socket will not be closed. This suggests that rtnl_handle should contain a destructor that closes the socket. Also, since a socket is opened with rtnl_open_socket(), it would be symmetrical for there to be a rtnl_close_socket() call to avoid the need to expose handle.fd. In fact, going further, I would suggest that rtnl_handle should be a fully-fledged class, and that the all the rtnl_xxx functions should be class methods. This would give maximum information hiding.

IfaceMgr::setFlags
Why are some members of IfaceMgr public? In particular, why have the flags_xxx_ members, which effectively duplicate the contents of flags_?

src/lib/dhcp/tests/iface_mgr_unittest.cc
When processing ifconfig output, you may want to consider use of isc::util::str::tokens, which splits output into separate tokens, regardless of the number of intervening spaces. Processing tokens one at a time rather than searching through the string for the first space etc. may be easier. (This is a comment, no action required.)

parse_ifconfig: looking at the output of ifconfig on both FreeBSD (bikeshed) and Solaris (sol-10), it might be safer to regard the output for an interface as starting at the line where the first character is not whitespace.

Use spaces around binary operators (e.g. line.find("inet")+5).

src/lib/dhcp/tests/pkt4_unittest.cc
Test metaFields: other tests used boost::shared_ptr to store the newly-created Pkt4 object and to ensure destruction when the test exits, it would be logical for this to do so. (As an aside, use of shared_ptr in this context is an unnecessary overhead - if the pointer is not going to be shared, scoped_ptr would be better.)

comment:7 in reply to: ↑ 6 Changed 8 years ago by tomek

Replying to stephen:

src/lib/dhcp/iface_mgr_linux.cc
Should update copyright to 2011-2012.

Done.

Really needs an introduction explaining the logic of Linux interfaces, e.g. what is netlink, what messages are being used, what needs to happen etc.

Such introduction/explanation with links to netlink interface is included in ticket #1540.

It is not clear why a vector of pointers is required to have many nlmsg structures pointing to the same header. Can this comment be expanded?

Yes. See if this updated explanation is sufficient.

There is no need to use the keyword "struct" in the declaration of a vector of pointers to nlmsghdr objects.

Removed.

"Holds information about interface or address attributes." This does not hold the information, it merely points to it. Also, what is the significance of "rt" in "rtaddtr", "RTattribs" and rtnl_* - Real-Time? (What I'm trying to get across is that the reader may not know much about the Netlink interface as it is not as widely-used as, say, the socket interface. Adding comments to guide them makes it much easier to follow the logic and to spot errors.)

I'm not really sure. I'm just following convention here: rtnetlink.h,

The original interface used in somewhat similar to naming conventions used in ip tool from iproute2, which is used as a reference implementation for netlink. I assume that RT stands for routing, as netlink interface allows route modification as well. In fact, it is extremely robust, you can create/get/set/delete links, addresses, routes, queuing, manipulate traffic classes, manipulate neithborhood tables and even do something with address labels (whatever that is). I must admit that I don't fully comprehend all the details of the netlink interface. I can study it further if you want me to.

Both NetlinkMessages and RTattribs objects hold pointers to other objects. This immediately raises the question of what is expected to free up the memory pointed to by them. This should be documented.

Added extra explanation with more background about netlink, the way how normal byte buffer is read from netlink socket and then later is parsed as collection of netlink messages concatenated together.

Also added comments about releasing allocated memory (see comments around calls to rtnl_process_reply).

(In fact, looking further on in the code I note that there is a special function that deletes the pointed-to objects. In this case, why not declare the vectors as vectors of scoped_ptrs? Then the objects will be automatically deleted when the vector is deleted.)

Because this code is already convoluted enough. Have you ever tried to debug such horribly aliased interface? I'm willing to write those extra few lines of code for trivial memory release (I don't have to, as the code is already there).

Typo: "addres info", should be "address info". Also "any code reuse" should be "any code reusable".

The assumption that IFA_MAX is shorter than IFLA_MAX can be explicitly checked during compilation using BOOST_STATIC_ASSERT.

No need to use "struct" in the declaration of rtnl_handle.

Done.

Should document why __u32 is used as a data type rather than uint32_t in rtnl_handle.

sndbuf/rcvbuf. These variables sound like send buffer and receive buffer, whereas in fact they are the size of the buffer. SNDBUF_SIZE and RCVBUF_SIZE (or xxx_MAX) would be better names for them.

You mentioned both things in #1540. I will not duplicate it in #1528 to avoid merge problems.

rtnl_send_request
The members of "struct req" do not need to include the "struct" declaration in C++.

In rtnl, two structs are declared with another one. The enclosing struct is then sent via a sendto() call with sizeof(enclosing-struct) as the size. Are we sure that the compiler is not aligning the member structs on natural boundaries? In other words, in some circumstances could there be unused bytes between the two? Addition of an assertion to check that would be useful.

It will work as sizeof(nlmsghdr) = 8, so regardless of memory alignment, the next structure will start immediately after it. Nevertheless, it is a good idea to add assert to confirm that. I tried this approach:
BOOST_STATIC_ASSERT(sizeof(nlmsghdr) == offsetof(req,generic) );
But it does not work. Compiler complains about a comma not being allowed in macro. I think gcc is confused and thinks that two parameters are passed to BOOST_STATIC_ASSERT that takes only one.

Spaces around the binary operator "|" are required.

A comment as to why handle.dump is set to ++handle.seq would be useful - that is non-obvious, as handle is just described as a "context structure". Also, I would explicitly put that assignment as a separate statement - it's easy to overlook.

Done as requested. Also added extra comments for seq and dump fields.

In the call to sendto(), the casting should be done using C++ constructs.

Done.

rtnl_store_reply
"Appends nlmsg to a storage" should be "Appends a copy of a NetLink message to the message store".

Why the redundant parentheses in the call to "new"?

Copy and paste error. Fixed.

No need to check if copy == NULL as the "new" operation would throw an exception if this were true. (But if it didn't, the check should follow the memcpy() call, which would segfault if the destination were null.)

parse_rtattr
Reading this raised the following question: why is a boost array used to store pointers to the rtattr structure rather than a vector (as is the case for a NetlinkMessage pointer)? (As it is, if the number of rtattr structures is too many for the array, the excess are silently ignored.)

Because boost array offers the ability to fix size of the container. This saves up checking if the vector passed is of proper size.

In the "if" block, braces are required even for single-line statements.

What are the RTA_OK and RTA_NEXT macros? Some comments are required here.

These are macros defined in linux/rtnetlink.h for RTA manipulation. RTA_OK checks if pointed rta structure is sane. RTA_NEXT() returns pointer to the next rtattr message that immediately follows pointed rta. Added comment that explains that.

ipaddrs_get
This parses addr_info - but addr_info is described as a collection of parsed netlink messages. More information about what this function actually does is required.

Added more information.

Why is addr declared with a length of 16? A symbolic constant should be used.

Done.

struct ifaddrmsg *ifa" should be ifaddrmsg* ifa (note the absence of "struct" and the position of the "*".)

Done.

Should use C++ construct to cast to "ifaddrmsg*".

Done.

Redundant space after "(" in the "if" statement.

No need to use "struct rtattr*" in call to fill() - "rtattr*" should be sufficient.

The two "if" blocks checking ifa->family are identical, with the exception of the address family in the call to IOAddress::from_bytes and the size of the data in the memcpy() call. The common code should be abstracted into a separate function. (Additionally: (a) The "if" blocks in this code should include braces, (b) C++ casts should be used in the call to memcpy (and the second argument should be cast to const void*) and (c) symbolic constants should be used as the length of data to be copied in the call to memcpy.

Collapsed the code into a single block.


rtnl_process_reply
It would be helpful if the header were to say what processing was going to be done. (And comments in the code to say that as well.)

Added.

Using "rth" as the designation for "netlink parameters" seems a bit obscure.

Renamed to context.

No need to use the "struct" keyword when declaring variables.

Removed.

Declaration of "status" should be at first use. (The same goes for the declaration of "header".)

Done.

Does iov.iov_len vary between iterations of the loop? If not, why not set it outside?

Moved.

The "if" statement checking for EINTR should have the body within braces.

Done.

If recvmsg() returns a value less than zero, it does not automatically mean that there is an overrun - it could be any one of a number of errors (determined by errno). Including errno in the exception thrown would be useful for diagnostic purposes.

Updated.

I notice that here, if recvmsg fails due to an interrupted system call status, the call is retried. This does not happen in the recvmsg() calls in iface_mgr.cc. If this is an omission, I would create a separate function that calls recvmsg and handles the case of an interrupted system call.

The idea here is that there is no guarantee that messages received over netlink will come back in one piece. We have such guarantee for UDP packets, though. (For now we ignore the unlikely possibility of receiving fragmented UDP which is unheard of in DHCP world).

Assignment of address of "buf" to "header" - use C++ casts.

Done.

The comment "Why we received this anyway?" does not give much information. From the code, better would be something like "Received a message not addressed to our process, or not with a sequence number we are expecting. Ignore, and look at the next one."

Done.

release_list
This would not be needed if the object were a vector of scoped_ptrs - a call to vector::clear() would handle the destruction of the pointed-to objects.

See my comments above about trade-offs between ease of debugging and language fancy stuff. I had bad experience with trying to debug smart pointers in gdb, so I try to avoid smart pointers in code that deals with strange memory tricks. That certainly is true for netlink interface.

detectIfaces
The comment "link info" on a variable named "link_info" is not really helpful (same for addr_info).

Start comments with a capital letter.

"len" can be declared within the main loop, at the point of first use.

Within the main "for" loop, why not set "len" with a single statement. And what is "len" being set to?

Moved len into "for" loop.

The "valgrind" report has to be bogus - iface_name is scope-limited to the "for" loop and takes a copy of its argument. However, it is not clear what the string represents - a comment would be useful. And are we sure the string is non-null?

No, we are not sure. But we act on two assumptions:

  1. RFC3549 states that it is ASCII string. Null termination is not mentioned, but somewhat assumed.
  2. Practical experience shows that it is Null-terminated.

The same approach is used in ip route2. It is sort of reference implementation and was implemented by Alexey Kuznetzov, the same guy who wrote Linux kernel counter-part.

Looking at the declaration of iface:

  • The index is set in the constructor
  • The hardware type is set via a direct access to a member of Iface
  • The flags are set through a call to the method setFlagsx().

This is inconsistent.

To some degree it is. However, that was chosen for a reason. The absolute minimum of parameters that are required are interface name and ifindex. That is useful in some cases, where other information is not available, e.g. when interface information is read from a file rather than actually detected. There are many parameters that can possibly be available: name, index, MAC address, flags, assigned IPv4 and IPv6 addresses, possibly additional info. We have 3 choices here:
later if available.

  1. Create constructor with all parameters with most of the defaulting to 0 or NULL.
  2. Create multiple constructor combinations, depending on what information is available.
  3. Create simple constructor with just minimal list of parameters and set all other parameters

later, using dedicated methods.

First choice gives only a illusion of flexibility, as it is not possible to sort parameter from most to least likely to be available. Therefore any order proposed will not work in some cases.

Second option is not good due to code bloat as number of combinations are really large.

Therefore third option was chosen as the most flexible. Keep in mind that sooner or later, we will need to implement interface detection for systems like Solaris that provides only limited information about available APIs for interface detection.

The inconsistency is carried through to the setting of the mac_len_ and mac_ members: these should be zeroed in the the constructor. Similarly the copying of the MAC from the attribs_table should be handled by a single call to an Iface method, passing to it the address of the data and length of the data.

Implemented setMac(),getMac(), getMacLen(), setHWType() and getHWType().

Note that if exceptions are thrown, the socket will not be closed. This suggests that rtnl_handle should contain a destructor that closes the socket. Also, since a socket is

Agree. Added destructor that closes the socket if the descriptor is set.

opened with rtnl_open_socket(), it would be symmetrical for there to be a rtnl_close_socket() call to avoid the need to expose handle.fd. In fact, going further, I would suggest that
rtnl_handle should be a fully-fledged class, and that the all the rtnl_xxx functions should be class methods. This would give maximum information hiding.
And what benefit that would give us? I would rather work on something useful instead. During next meeting when asked about progress I would very much prefer to say "logging is implemented", "developer's documentation is ready" or "fixed some bugs", rather than "there's no real progress, but our interface detection code is now symmetric and nicely wrapped in a class".

Anyway, I did as you suggested.

Rest of the review comments will follow.

comment:8 in reply to: ↑ 6 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

Replying to stephen:

IfaceMgr::setFlags
Why are some members of IfaceMgr public? In particular, why have the flags_xxx_ members, which effectively duplicate the contents of flags_?

Because bit meaning is different on different systems. I chose several flags that I believe will be useful for us, theses are not all flags, hence the original flags_ field. Also, keep note that not only definition, but also number of flags available on various systems is different.

My idea was to hide those fields once we start supporting more than just Linux. Currently I do not have enough knowledge about different flavors of BSDs and Solaris to decide, which flags are common and which are not.

src/lib/dhcp/tests/iface_mgr_unittest.cc
When processing ifconfig output, you may want to consider use of isc::util::str::tokens, which splits output into separate tokens, regardless of the number of intervening spaces. Processing tokens one at a time rather than searching through the string for the first space etc. may be easier. (This is a comment, no action required.)

Added comment about isc::util::str::tokens.

parse_ifconfig: looking at the output of ifconfig on both FreeBSD (bikeshed) and Solaris (sol-10), it might be safer to regard the output for an interface as starting at the line where the first character is not whitespace.

Aha! This code is Linux specific and it will stay that way. This is really a chicken and egg problem. If there was one simple, unified way of detecting interfaces across all platforms, we would have used it in the code in the first place. If there were two such methods available, we would have used the second one in tests. Sadly, there is not even one.

Yes, parse_ifconfig has its limitations and it will stay that way. I was asked to implement tests for something that is not really fully testable. There are reports that parse_ifconfig breaks down even on Linux if there are interface aliases used. I'm sorry about that and offer my words of sympathy, but trying to make tests based on ifconfig working everywhere would be an exercise in futility. My recommendation hasn't changed: we must acknowledge that some code parts are better suited for testing than others.

Use spaces around binary operators (e.g. line.find("inet")+5).

I hope I spotted and fixed them all.

src/lib/dhcp/tests/pkt4_unittest.cc
Test metaFields: other tests used boost::shared_ptr to store the newly-created Pkt4 object and to ensure destruction when the test exits, it would be logical for this to do so. (As an aside, use of shared_ptr in this context is an unnecessary overhead - if the pointer is not going to be shared, scoped_ptr would be better.)

No, not at all. Consistency in tests is a fallacy. There is a value in trying to use tested code in different ways. In fact, as far as testing goes, the more variety the better. One may assume that the object should be destroyed in the same way, regardless if it is explicitly deleted using delete or destroyed by shared_ptr destructor. But again, tests should verify that rather than base on assumptions.

All improvements are checked in and awaiting next round of review.

comment:9 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

src/lib/dhcp/iface_mgr_linux.cc
I still had concern over the comments, so I've made some changes: only to the comments, although I've added/deleted some spaces. The changes have been pushed: please review.

...BOOST_STATIC_ASSERT(sizeof(nlmsghdr) == offsetof(req,generic) ); But it does not work. Compiler complains about a comma not being allowed in macro. I think gcc is confused and thinks that two parameters are passed to BOOST_STATIC_ASSERT that takes only one.

I suggest using a plain "assert()" instead. The run-time overhead is negligible but it is a useful additional check.

And what benefit that would give us? I would rather work on something useful instead.

The additional overhead of making it an explicit class is not much, and it does make it easier to test and re-use in a C++ context should the need arise.

src/lib/dhcp/iface_mgr_linux.cc - additional points
open_socket() - cast should be done using C++ constructs.

parse_rattr(): the check "<= table.size() - 1" could more easily be written as "< table.size()" (and saves the need to perform the subtraction before the test).

comment:10 in reply to: ↑ 9 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

Replying to stephen:

src/lib/dhcp/iface_mgr_linux.cc
I still had concern over the comments, so I've made some changes: only to the comments, although I've added/deleted some spaces. The changes have been pushed: please review.

Thanks. Changes looks ok.

...BOOST_STATIC_ASSERT(sizeof(nlmsghdr) == offsetof(req,generic) ); But it does not work. Compiler complains about a comma not being allowed in macro. I think gcc is confused and thinks that two parameters are passed to BOOST_STATIC_ASSERT that takes only one.

I suggest using a plain "assert()" instead. The run-time overhead is negligible but it is a useful additional check.

I managed to make it work with BOOST_STATIC_ASSERT. It seems that it required name of a type, not a name of an instance of that type.

And what benefit that would give us? I would rather work on something useful instead.

The additional overhead of making it an explicit class is not much, and it does make it easier to test and re-use in a C++ context should the need arise.

Netlink is now a class, so I assume that comment point is closed.

src/lib/dhcp/iface_mgr_linux.cc - additional points
open_socket() - cast should be done using C++ constructs.

Done.

parse_rattr(): the check "<= table.size() - 1" could more easily be written as "< table.size()" (and saves the need to perform the subtraction before the test).

Done.

I believe that addresses all comments raised. Unless there are new comments, I think the code is ready for merge.

comment:11 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

Checked commit 7eaf3a710ceb20f4822a636054aef7d8a6f7e6dd.

That looks OK, please merge.

Stephen

comment:12 Changed 8 years ago by tomek

  • Resolution set to fixed
  • Status changed from reviewing to closed

Code merged.

Note: See TracTickets for help on using tickets.