Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1452 closed task (complete)

add UDP socket passing to fd_X handling

Reported by: jelte Owned by: jinmei
Priority: low Milestone: Sprint-20120110
Component: ddns Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: DDNS
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 50.52 Internal?: no

Description (last modified by jelte)

Barring a general receptionist or receoptionist-like framework, we need to be able to pass UDP sockets similarly to how we pass TCP sockets now.

This is certainly needed for the DDNS module from ticket #1451 (though 1451 does not depend on it, #1454 does). But we also need this if we want to add UDP ixfr-out support.

Subtickets

Attachments (1)

makefile.diff (880 bytes) - added by jinmei 8 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 8 years ago by jelte

  • Description modified (diff)

comment:2 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 5

comment:3 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20111220

comment:4 Changed 8 years ago by jinmei

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

comment:5 follow-up: Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

trac1452 is (finally) ready for review.

Actually, this is only for the C++ implementation. Since even this
part is already quite big, I decided to defer the python wrapper
part to a separate implementation/review cycle (or even a separate
task).

Unfortunately the diff is pretty large. Basically, however, (I hope)
it only does some straightforward things. But the resulting code was
this big due to the need for considering various corner cases and
providing detailed tests. My suggestion for review is to first read
the overview doxygen documentation in util/io/socketsssion.h (in the
SocketSessionUtility page), then look at the ForwardTest::pushAndPop
test (in util/tests/socketsession_unittest.cc), which checks basic
expected behaviors, along with the corresponding implementation.

Other cases are generally for checking exceptional cases and error
handling, and it will be easier to understand them once you get the
overview of the design and implementation.

If it's still too big to review in a single cycle, we can discuss
whether/how to divide it into multiple chunks.

Some other notes:

  • My long-term plan is to eventually replace the existing "xfr client" with this new interface. But that will be a post ddns cleanup.
  • Likewise, I guess we might eventually hide fd_share details within the socket session implementation. That will also be a subject of a post ddns discussion.

comment:6 Changed 8 years ago by jinmei

Forgot to mention this: right now I don't plan to provide a changelog
entry for this change as it's too "internal".

comment:7 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:8 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

Unfortunately the diff is pretty large. Basically, however, (I hope)
it only does some straightforward things. But the resulting code was
this big due to the need for considering various corner cases and
providing detailed tests. My suggestion for review is to first read
the overview doxygen documentation in util/io/socketsssion.h (in the
SocketSessionUtility page), then look at the ForwardTest::pushAndPop
test (in util/tests/socketsession_unittest.cc), which checks basic
expected behaviors, along with the corresponding implementation.

It's quite OK to keep in mind, even when large code-wise.

But there are some problems with the branch.

First, the library now uses exceptions, so it must link to it. I pushed a makefile fix, so it now compiles for me.

The tests fail for me:

[ RUN      ] ForwardTest.pushTooFast
socketsession_unittest.cc:685: Failure
Expected: multiPush(forwarder_, *getSockAddr("192.0.2.1", "53").first, large_text_.c_str(), large_text_.length()) throws an exception of type SocketSessionError.
  Actual: it throws nothing.
[  FAILED  ] ForwardTest.pushTooFast (0 ms)

My guess for this is, linux takes the buffer size as some kind of hint only and makes its own mind about the proper size and 3 messages fit without a problem. If I increased it to 3000, the test passed, I didn't test when it started to pass and I'm not sure if it might be dependant on some system parameters (amount of memory, size of memory management pages, etc).

Also, is the „receptor“ name a good one? It reminds me with the little parts of cells that detect which chemicals are around and what temperature it is. Shouldn't it be a „recipient“, for the one who receives?

I'm not sure about how the blocking reads are used and such. For one, if there comes three messages to be forwarded at once and they are all handled by auth before a single context switch happens, the last one would get dropped, even if the system would be completely capable of handling the load. Also, I heard some system (linux is one of them) can cause a spurious wakeup on a select for a socket that has no data to read. I'm not sure if it can be with a local socket, or if it was a result of some packet that came for the connection but was corrupt, or something. Anyway, it is probably good enough for now, if we're considering to do it differently with the receptionist (I'd very like to have auth, xfrin, xfrout and ddns blocking then, using only single connection to the receptionist).

Also, there are several mostly minor issues:

  • Should the SA_TYPE in template be upper case? Don't we use it for constants and enum values? This is type, so it should be CamelCase:
    template <typename SA_TYPE>
    
  • Should we use cerrno instead of errno.h? The same applies for stdint.h and string.h.
  • Should the local includes be done first, before other headers, to ensure they don't use something from the system headers included above?
  • Should this use UNIX_MATH_PATH instead?
    (sizeof(impl.sock_un_.sun_path) - 1 < unix_file.length())
    
  • If you already check the size, why do you use the (more complicated) strncpy instead of strcpy? Is there a trick I'm forgetting?
        strncpy(impl.sock_un_.sun_path, unix_file.c_str(),
                sizeof(impl.sock_un_.sun_path));
    
  • I don't see why the „rest“ of the buffer with the path should be zeroed. The strncpy will zero only one byte after the end of string, and this might be far after it. I don't see anything zeroing it anywhere, so it might contain quite anything what was on the stack:
    assert(impl.sock_un_.sun_path[sizeof(impl.sock_un_.sun_path) - 1] == '\0');
    
  • Where does the magic number „2“ come from here?
    impl.sock_un_len_ = 2 + unix_file.length();
    
  • Should the right side be impl.sock_un_len_? Nobody anywhere assigned to sock_un_len_ and I don't see it anywhere.
    #ifdef HAVE_SA_LEN
        impl.sock_un_.sun_len = sock_un_len_;
    #endif
    
  • As described in the man page (The freeaddrinfo() function frees the memory that was allocated for the dynamically allocated linked list res.), the freeaddrinfo should be called only once on the whole result ‒ it frees the whole list, not on each of the nodes there. I guess there was no problem here, as it produces only single-element lists here, but it's needlessly complicated and, in principle, wrong, as it may create double-free errors.

With regards

comment:9 in reply to: ↑ 8 ; follow-up: Changed 8 years ago by jinmei

Replying to vorner:

Thanks for the review.

Replying to jinmei:

It's quite OK to keep in mind, even when large code-wise.

You're very good at reading/reviewing a large patch:-)

First, the library now uses exceptions, so it must link to it. I pushed a makefile fix, so it now compiles for me.

Ack, thanks.

The tests fail for me:

[ RUN      ] ForwardTest.pushTooFast
socketsession_unittest.cc:685: Failure
Expected: multiPush(forwarder_, *getSockAddr("192.0.2.1", "53").first, large_text_.c_str(), large_text_.length()) throws an exception of type SocketSessionError.
  Actual: it throws nothing.
[  FAILED  ] ForwardTest.pushTooFast (0 ms)

My guess for this is, linux takes the buffer size as some kind of hint only and makes its own mind about the proper size and 3 messages fit without a problem.

Ah, okay, google told me that Linux doubles the specified buffer size
by setsockopt(). So, increasing it to 4 (or perhaps 5) should be
enough. I've changed it to 10 to be a bit conservative (commit
8fda3f0). As commented in that commit, if it still makes it fail we
should take a more reliable approach, but for now I prefer the brevity.

Also, is the „receptor“ name a good one? It reminds me with the little parts of cells that detect which chemicals are around and what temperature it is. Shouldn't it be a „recipient“, for the one who receives?

Actually I was not sure if this naming is a good one. So I have no
problem with changing it. I've changed it to "Receiver" (not a strong
preference over "Recipient", but the former is a bit shorter and would
look more consistent compared to "Forwarder", in that they share the
suffix of "er").

I'm not sure about how the blocking reads are used and such.

For the moment I wanted to keep the interface simpler; asynchronous
read will make it more complicated while in many cases the
communication between these two processes is not expected to be
blocking (if it blocks, it's quite likely that the receiver side is
too busy and will have to drop some of the request anyway). Note also
that 2 full-DNS messages are quite large for the expected usage. In
many cases each session data should be much smaller.

That said, I don't deny that it could be improved in a future
version. For now I'd move forward with this style of operation and
see how practical it is.

Also, I heard some system (linux is one of them) can cause a spurious wakeup on a select for a socket that has no data to read.

If this happens on the UNIX domain socket we are using for the socket
session passing, that would indeed be a problem. In that case we
should consider a workaround (but note that blocking read on the
receiver side is not new in this implementation - we already do this
in the current recv_fd() interface. So we should've seen it if it
really happens at some noticeable level of frequency).

Also, there are several mostly minor issues:

  • Should the SA_TYPE in template be upper case? Don't we use it for constants and enum values? This is type, so it should be CamelCase:
    template <typename SA_TYPE>
    

Hmm, maybe. It was not a strong intent, but I kind of intended to
make it clear that it's a template parameter, not a real type name.
But that's not in the common guideline, and in any case it wouldn't
matter much for such a small function. So I changed it to SAType.

  • Should we use cerrno instead of errno.h? The same applies for stdint.h and string.h.

Hmm, perhaps. The header files provided with g++ don't make them so
different, but in terms of honor namespaces cerrno (etc) should be
better than errno.h (and in any case my first branch wasn't
consistent; I used both cassert and errno.h). I changed the code to
use cXXX versions as much as possible ("cstdint" doesn't seem to be a
standard or at least not so portable, so I didn't use it).

  • Should the local includes be done first, before other headers, to ensure they don't use something from the system headers included above?

Hmm, I don't know. Is that a common practice? I have no problem with
reordering them that way if it's more common, but in that case I think
we should rather make it in our project-wide guideline. For now I've
not make a change on this point.

  • Should this use UNIX_MATH_PATH instead?
    (sizeof(impl.sock_un_.sun_path) - 1 < unix_file.length())
    

You mean UNIX_MAX_PATH? I thought it was not portable.

  • If you already check the size, why do you use the (more complicated) strncpy instead of strcpy? Is there a trick I'm forgetting?
        strncpy(impl.sock_un_.sun_path, unix_file.c_str(),
                sizeof(impl.sock_un_.sun_path));
    

It was a kind of defense of depth (or being paranoid). Especially
because it's beyond the super trivial level (using a sizeof for a
structure member - see above why, using string::length(), etc), I
thought it could be possible that the length check may have a bug. I
wanted to make sure that even with such a case it at least doesn't
cause things like buffer overflow (this is also the reason for the use
of assert).

Maybe we should at least clarify the point? e.g.

    // the copy should be safe due to the above check, but we'd be rather
    // paranoid about making it 100% sure even if the check has a bug (with
    // triggering the assertion in the worse case)
    strncpy(impl.sock_un_.sun_path, unix_file.c_str(),
            sizeof(impl.sock_un_.sun_path));
    assert(impl.sock_un_.sun_path[sizeof(impl.sock_un_.sun_path) - 1] == '\0');
  • I don't see why the „rest“ of the buffer with the path should be zeroed.

At least my man page states strncpy ensures that:

     The strncpy() function copies at most n characters from s2 into s1.  If s2
     is less than n characters long, the remainder of s1 is filled with `\0'
     characters.  Otherwise, s1 is not terminated.

Isn't it part of standard?

  • Where does the magic number „2“ come from here?
    impl.sock_un_len_ = 2 + unix_file.length();
    

Ah, okay, it should have (better) been sizeof(uint16_t). Changed.
(in case it's still not clear, it's for the "2-byte header length").

  • Should the right side be impl.sock_un_len_? Nobody anywhere assigned to sock_un_len_ and I don't see it anywhere.
    #ifdef HAVE_SA_LEN
        impl.sock_un_.sun_len = sock_un_len_;
    #endif
    

Good, catch, you're right. This made me notice that I should have
included config.h (which would have made the bug visible for me).
Fixed both points.

  • As described in the man page (The freeaddrinfo() function frees the memory that was allocated for the dynamically allocated linked list res.), the freeaddrinfo should be called only once on the whole result ‒ it frees the whole list, not on each of the nodes there. I guess there was no problem here, as it produces only single-element lists here, but it's needlessly complicated and, in principle, wrong, as it may create double-free errors.

I'm not sure what's wrong about this point. addrinfo_list_ should be
a list of the head pointer of the addrinfo chain. So this shouldn't
cause duplicate free:

        vector<struct addrinfo*>::const_iterator it;
        for (it = addrinfo_list_.begin(); it != addrinfo_list_.end(); ++it) {
            freeaddrinfo(*it);
        }

(if that's what you are talking about). Or am I missing something or
misunderstanding you?

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:11 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to vorner:

The tests fail for me:

[ RUN      ] ForwardTest.pushTooFast
socketsession_unittest.cc:685: Failure
Expected: multiPush(forwarder_, *getSockAddr("192.0.2.1", "53").first, large_text_.c_str(), large_text_.length()) throws an exception of type SocketSessionError.
  Actual: it throws nothing.
[  FAILED  ] ForwardTest.pushTooFast (0 ms)

My guess for this is, linux takes the buffer size as some kind of hint only and makes its own mind about the proper size and 3 messages fit without a problem.

Ah, okay, google told me that Linux doubles the specified buffer size
by setsockopt(). So, increasing it to 4 (or perhaps 5) should be
enough. I've changed it to 10 to be a bit conservative (commit
8fda3f0). As commented in that commit, if it still makes it fail we
should take a more reliable approach, but for now I prefer the brevity.

Yes, it passes now.

Also, is the „receptor“ name a good one? It reminds me with the little parts of cells that detect which chemicals are around and what temperature it is. Shouldn't it be a „recipient“, for the one who receives?

Actually I was not sure if this naming is a good one. So I have no
problem with changing it. I've changed it to "Receiver" (not a strong
preference over "Recipient", but the former is a bit shorter and would
look more consistent compared to "Forwarder", in that they share the
suffix of "er").

This one looks OK.

That said, I don't deny that it could be improved in a future
version. For now I'd move forward with this style of operation and
see how practical it is.

OK, agreed.

  • Should the local includes be done first, before other headers, to ensure they don't use something from the system headers included above?

Hmm, I don't know. Is that a common practice? I have no problem with
reordering them that way if it's more common, but in that case I think
we should rather make it in our project-wide guideline. For now I've
not make a change on this point.

I met several projects doing so, and I use it on unconscious level already. We probably don't have it in guidelines, but IMO it makes some sense, as it makes sure the headers are stand-alone. Should I bring it to -dev?

  • Should this use UNIX_MATH_PATH instead?
    (sizeof(impl.sock_un_.sun_path) - 1 < unix_file.length())
    

You mean UNIX_MAX_PATH? I thought it was not portable.

I'm not sure about portability. I found it in my man page.

Maybe we should at least clarify the point? e.g.

    // the copy should be safe due to the above check, but we'd be rather
    // paranoid about making it 100% sure even if the check has a bug (with
    // triggering the assertion in the worse case)
    strncpy(impl.sock_un_.sun_path, unix_file.c_str(),
            sizeof(impl.sock_un_.sun_path));
    assert(impl.sock_un_.sun_path[sizeof(impl.sock_un_.sun_path) - 1] == '\0');

Yes, maybe.

  • I don't see why the „rest“ of the buffer with the path should be zeroed.

At least my man page states strncpy ensures that:

     The strncpy() function copies at most n characters from s2 into s1.  If s2
     is less than n characters long, the remainder of s1 is filled with `\0'
     characters.  Otherwise, s1 is not terminated.

Isn't it part of standard?

Ah, I didn't know about that one. It is indeed in my man page as well.

  • Where does the magic number „2“ come from here?
    impl.sock_un_len_ = 2 + unix_file.length();
    

Ah, okay, it should have (better) been sizeof(uint16_t). Changed.
(in case it's still not clear, it's for the "2-byte header length").

Actually, I meant this 2:

    assert(impl.sock_un_.sun_path[sizeof(impl.sock_un_.sun_path) - 1] == '\0');
    impl.sock_un_len_ = 2 + unix_file.length();
#ifdef HAVE_SA_LEN
  • As described in the man page (The freeaddrinfo() function frees the memory that was allocated for the dynamically allocated linked list res.), the freeaddrinfo should be called only once on the whole result ‒ it frees the whole list, not on each of the nodes there. I guess there was no problem here, as it produces only single-element lists here, but it's needlessly complicated and, in principle, wrong, as it may create double-free errors.

I'm not sure what's wrong about this point. addrinfo_list_ should be
a list of the head pointer of the addrinfo chain. So this shouldn't
cause duplicate free:

        vector<struct addrinfo*>::const_iterator it;
        for (it = addrinfo_list_.begin(); it != addrinfo_list_.end(); ++it) {
            freeaddrinfo(*it);
        }

(if that's what you are talking about). Or am I missing something or
misunderstanding you?

No, sorry, I misread the code. I thought the vector contained all the nodes.

comment:12 in reply to: ↑ 11 Changed 8 years ago by jinmei

Replying to vorner:

  • Should the local includes be done first, before other headers, to ensure they don't use something from the system headers included above?

Hmm, I don't know. Is that a common practice? I have no problem with
reordering them that way if it's more common, but in that case I think
we should rather make it in our project-wide guideline. For now I've
not make a change on this point.

I met several projects doing so, and I use it on unconscious level already. We probably don't have it in guidelines, but IMO it makes some sense, as it makes sure the headers are stand-alone. Should I bring it to -dev?

Yes, please. For now I've not touched the ordering. If we agree on
the ordering there will be a cleanup ticket that will cover this one,
too.

You mean UNIX_MAX_PATH? I thought it was not portable.

I'm not sure about portability. I found it in my man page.

At least they are not defined under /usr/include in FreeBSD or MacOS

  1. So I still keep avoiding it.

Maybe we should at least clarify the point? e.g.

    // the copy should be safe due to the above check, but we'd be rather
    // paranoid about making it 100% sure even if the check has a bug (with
    // triggering the assertion in the worse case)
    strncpy(impl.sock_un_.sun_path, unix_file.c_str(),
            sizeof(impl.sock_un_.sun_path));
    assert(impl.sock_un_.sun_path[sizeof(impl.sock_un_.sun_path) - 1] == '\0');

Yes, maybe.

Added the comment.

  • Where does the magic number „2“ come from here?

Actually, I meant this 2:

    assert(impl.sock_un_.sun_path[sizeof(impl.sock_un_.sun_path) - 1] == '\0');
    impl.sock_un_len_ = 2 + unix_file.length();
#ifdef HAVE_SA_LEN

Oh, okay. I meant this:

    impl.sock_un_len_ = offsetof(struct sockaddr_un, sun_path) +
        unix_file.length();

I didn't know offsetof was defined in cstddef and might not be very
portable...now I see it's part of cstddef, so I've updated the code
using offsetof.

comment:13 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:14 follow-up: Changed 8 years ago by jinmei

As I worked on the python wrapper part of this task I noticed I
overlooked a few more corner cases in this branch. So I revised the
branch to handle these cases. Could you check these changes, too?

Thanks,

comment:15 Changed 8 years ago by jinmei

The python wrapper part is also ready for review at branch trac1452b.

The first few commits were for importing the C++ part (trac1452) and
should be ignored. The effective branch point is commit 9d39266.

If it's better to create a separate ticket for the review of the
python part please let me know.

comment:16 Changed 8 years ago by jinmei

(Forgot to mention this) commit 9d39266 is not really relevant to the
subject of this task, but I thought it made sense to update the
template at some point, and this task is a nice opportunity to do it.
It's a completely separate diff and isn't even necessary to compile
the rest of the branch, so the review of this diff can be deferred,
and if the main changes are deemed to be too big we can completely defer
9d39266 to a separate (new) task.

comment:17 in reply to: ↑ 14 ; follow-up: Changed 8 years ago by vorner

Hello

Replying to jinmei:

As I worked on the python wrapper part of this task I noticed I
overlooked a few more corner cases in this branch. So I revised the
branch to handle these cases. Could you check these changes, too?

These look OK.

Anyway, it started to fail to compile for me. It doesn't seem anything relevant changed here, though.

cc1plus: warnings being treated as errors
../../../../../src/lib/util/io/fd_share.cc: In function ‘int isc::util::io::recv_fd(int)’:
../../../../../src/lib/util/io/fd_share.cc:106:47: error: dereferencing type-punned pointer will break strict-aliasing rules
../../../../../src/lib/util/io/fd_share.cc: In function ‘int isc::util::io::send_fd(int, int)’:
../../../../../src/lib/util/io/fd_share.cc:135:32: error: dereferencing type-punned pointer will break strict-aliasing rules
make[6]: *** [fd_share.lo] Error 1

Did you do anything regarding it? I guess we should try to be aliasing save, and if not, disable the strict aliasing uptimisations for the given objects.

I'll look into the python part now.

Thanks

comment:18 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

About the python part, I noticed just one thing. In the SocketSessionReceiver_init function, what would happen in case the object didn't have a fileno method at all or if the call raised an exception? It doesn't seem to be explicitly handled, maybe it is handled implicitly somehow. But in such case, it would deserve a comment at last I think.

Thank you

comment:19 Changed 8 years ago by jelte

  • Priority changed from major to minor

comment:20 in reply to: ↑ 17 Changed 8 years ago by jinmei

Replying to vorner:

As I worked on the python wrapper part of this task I noticed I
overlooked a few more corner cases in this branch. So I revised the
branch to handle these cases. Could you check these changes, too?

These look OK.

Anyway, it started to fail to compile for me. It doesn't seem anything relevant changed here, though.

cc1plus: warnings being treated as errors
../../../../../src/lib/util/io/fd_share.cc: In function ‘int isc::util::io::recv_fd(int)’:
../../../../../src/lib/util/io/fd_share.cc:106:47: error: dereferencing type-punned pointer will break strict-aliasing rules

[...]

}}}

Did you do anything regarding it? I guess we should try to be aliasing save, and if not, disable the strict aliasing uptimisations for the given objects.

Ah, sorry, yes, I removed this from Makefile.am

libutil_io_la_CXXFLAGS = $(AM_CXXFLAGS) -fno-strict-aliasing

as I thought it was because we previously compiled this with
dependency on asio (which requires -fno-strict-aliasing) and we now
didn't need it (and in fact it compiled for me without the flag). It
was apparently too aggressive.

Since it's not directly related to the subject of this branch I could
revert to the original setting. But I also think it's a good
opportunity to make it cleaner. I believe the attached diff
(makefile.diff) should solve the compilation problem for you.
(Assuming so) I'm okay either with applying this cleanup or with
reverting to the previous Makefile.am

Changed 8 years ago by jinmei

comment:21 in reply to: ↑ 18 Changed 8 years ago by jinmei

Replying to vorner:

About the python part, I noticed just one thing. In the SocketSessionReceiver_init function, what would happen in case the object didn't have a fileno method at all or if the call raised an exception? It doesn't seem to be explicitly handled, maybe it is handled implicitly somehow. But in such case, it would deserve a comment at last I think.

I thought I clarified that here:

    } catch (const PyCPPWrapperException& ex) {
        // This could happen due to memory allocation failure, but it's more
        // likely that the object doesn't have the "fileno()" method or it
        // returns an unexpected type of value.  So we adjust the error
        // message accordingly.
        PyErr_SetString(PyExc_TypeError, "Failed to parse parameter, "
                        "probably not a valid socket object");

but apparently it was not sufficient or very clear. I tried to
provide some more comments. How about that?

comment:22 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:23 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei
  • Total Hours changed from 0 to 6

Hello

The patch helps, please apply it.

As for the explanation, when pointed to it, it seems clear (and yet better when updated). I didn't notice it, as I looked into the code, not to the exceptions.

Please merge (both parts).

Thank you

comment:24 in reply to: ↑ 23 Changed 8 years ago by jinmei

Merge done (with the proposed patch), closing ticket.

Thanks for the review.

comment:25 Changed 8 years ago by jinmei

  • Total Hours changed from 6 to 49.42

comment:26 Changed 8 years ago by jinmei

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

comment:27 Changed 8 years ago by jinmei

  • Total Hours changed from 49.42 to 50.52
Note: See TracTickets for help on using tickets.