Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when reading a forwarded State attribute #550 #552

Open
wants to merge 2 commits into
base: tango-9-lts
from

Conversation

Projects
None yet
2 participants
@bourtemb
Copy link
Member

commented May 14, 2019

  • Add a test to read a State forwarded attribute
  • Fix issue
@bourtemb

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

Travis CI should have failed for the first commit (Add a test to read State forwarded attribute).
It looks like it didn't fail in this PR because of the following reasons:
When the PR was created with only the first commit, Travis could not build it immediately because some other jobs were already running and the job was put in the waiting queue.
I pushed the commit with the fix only a few minutes after creating the Pull Request.
It looks like Travis is always getting the HEAD of the branch when testing/building a pull request (quite confusing! See travis-ci/travis-ci#8577) so it looks like when it finally could build for the first commit, the fix commit was already there. So it passed...

So I think we should automatically cancel older not already built Travis jobs when a new commit is added to a PR because the old ones become meaningless and bring confusion.

If you look at my fork, you will see Travis failed for the first commit and succeeded for the second one:
https://github.com/bourtemb/cppTango/commits/fix-550

datum.resize(1);
datum[0] = d_state;
return true;
}

This comment has been minimized.

Copy link
@mliszcz

mliszcz May 22, 2019

Collaborator

I've just checked equivalent code that extracts single DevState (instead of a vector).

  1. For some reason, they set d_state_filled to false after reading the state. Do you know why this is done? Do we need to reset it also here?
  2. Checking for d_state_filled is before the attempt to extract data from StateSeq. Consider doing the same for consistency and to avoid nesting if in else.

if (d_state_filled == true)
{
datum = d_state;
d_state_filled = false;
return ret;
}
if (StateSeq.operator->() != NULL)

This comment has been minimized.

Copy link
@bourtemb

bourtemb May 22, 2019

Author Member

It is not clear to me why d_state_filled is reset in the example you provided.
It is probably done to simulate the consumption of the data.
If you try to extract twice from the same DeviceAttribute object, at the second invocation check_for_data() will return false because is_empty() will return true, because of this d_state_filled reset to false.
To be consistent, it would probably good to add d_state_filled = false in the methods I modified indeed.

// read attributes
TS_ASSERT_THROWS_NOTHING(state_attr = fwd_device->read_attribute("fwd_state"));
DevState the_state;
if(state_attr >> the_state)

This comment has been minimized.

Copy link
@mliszcz

mliszcz May 22, 2019

Collaborator

Shouldn't you extract to a std::vector<DevState>?
I think this one is called now: bool DeviceAttribute::operator >> (DevState &datum)
And there were no (direct) changes in above operator.

This comment has been minimized.

Copy link
@bourtemb

bourtemb May 22, 2019

Author Member

This is what we really want to test: what happens when we read a forwarded DevState attribute.
The change was indeed in DeviceAttribute::operator >> (DevState &datum) which is used internally when the user tries to read a DevState forwarded attribute.
It is actually used internally to read the orginal DevState attribute.

This comment has been minimized.

Copy link
@mliszcz

mliszcz May 22, 2019

Collaborator

My point was that the change was done in:

bool DeviceAttribute::operator >> (vector<DevState>& datum)

and not in:

bool DeviceAttribute::operator >> (DevState &datum)

But maybe the control goes internally through the vector overload anyway.

This comment has been minimized.

Copy link
@bourtemb

bourtemb May 22, 2019

Author Member

But maybe the control goes internally through the vector overload anyway.

I understand why you had some doubts.
There was a change in bool DeviceAttribute::operator >> (vector<DevState>& datum) and no test was added to test that.
The control goes internally to bool DeviceAttribute::operator >> (DevVarStateArray* &datum) (which was changed in this PR too) when reading a DevState forwarded attribute.
It is called from template<typename T>void FwdAttribute::set_local_attribute(DeviceAttribute &da, T *&seq_ptr), which is called by void FwdAttr::read(DeviceImpl *dev,Attribute &attr) when reading a DevState forwarded attribute.

bool DeviceAttribute::operator >> (vector<DevState>& datum) was also changed in this PR for consistency reasons.
A test could be added to test it.

This comment has been minimized.

Copy link
@mliszcz

mliszcz May 22, 2019

Collaborator

Thanks! It's clear now.

@@ -1457,7 +1457,7 @@ void Attribute::set_value(Tango::DevState *p_data,long x,long y,bool release)
*tmp_ptr = *p_data;
value.state_seq = new Tango::DevVarStateArray(data_size,data_size,tmp_ptr,release);
if (is_fwd_att() == true)
delete p_data;
delete [] p_data;
else
delete p_data;

This comment has been minimized.

Copy link
@mliszcz

mliszcz May 22, 2019

Collaborator

consider using SAFE_DELETE(p_data) from attribute.h. There will be redundant check for release == true but you will not have duplicate code.

This comment has been minimized.

Copy link
@bourtemb

bourtemb May 22, 2019

Author Member

Macros are evil and not easy to debug and there was a redundant check. This is indeed why I didn't use the macro in this case.
SAFE_DELETE macro is convenient but it is a nightmare to debug issues in this macro, like #512 for instance.

This comment has been minimized.

Copy link
@mliszcz

mliszcz May 22, 2019

Collaborator

Maybe we should remove the macro and add something like

  • void safe_delete(T* pointer) and/or
  • void safe_delete_if_needed(T* pointer, bool needed)

Also, shall we define a new item for removing all macros? We may start from server/attribute.h file.

This comment has been minimized.

Copy link
@bourtemb

bourtemb May 22, 2019

Author Member

Maybe we should remove the macro and add something like

* `void safe_delete(T* pointer)` and/or

* `void safe_delete_if_needed(T* pointer, bool needed)`

I would be in favour of such a change.

Also, shall we define a new item for removing all macros? We may start from server/attribute.h file.

A new issue could be created indeed in order to replace existing macros when possible.

This comment has been minimized.

Copy link
@mliszcz

mliszcz May 22, 2019

Collaborator

I've opened #553 to track this effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.