search

intel / dleyna-server

dleyna-server is a library for implementing services that allow clients to discover, browse and manipulate Digital Media Servers. An implementation of such a service for linux is also included.
https://01.org/dleyna/
GNU Lesser General Public License v2.1
28 stars 28 forks source link

[BrowseObject]: Support Of ChildCount property #130

Closed lferrandis closed 11 years ago

lferrandis commented 11 years ago

Fix issue #122: https://github.com/01org/dleyna-server/issues/122

Signed-off-by: Ludovic Ferrandis ludovic.ferrandis@intel.com

lferrandis commented 11 years ago

Add 2 commits:

The first one is to fix up wit the original commit. It fix the ChildCount issue. The second commit is an optimization/fix already done in prv_get_all. Instead of using DLS_UPNP_MASK_ALL_PROPS, use the computed filter mask.

markdryan commented 11 years ago

This still isn't working quite right. The problem I think is that functions like prv_get_all assume that the dls_async_get_all_t structure is initialised to 0 before they execute. This is true for GetAll as the structure is initialised to 0 when the task object is created and is only used once. The problem with BrowseObjects is that the same object is used multiple times, i.e., once for each object being browsed. This causes problems. For example,

root.browse_objects(["/com/intel/dLeynaServer/server/0","/com/intel/dLeynaServer/server/0/2f686f6d652f6d61726b75732f444c4e41436f6e74656e742f566964656f2f4d5045475f50535f50414c2f422d4d503250535f502d322e6d7067"],['ChildCount'])
{
    "ChildCount": 2
}

{
    "ChildCount": 0
}

The second child is an item but dLeyna-server tries to retrieve the childcount for this object. I think this is because the need_child_count flag is set to true from the first object which is a container.

markdryan commented 11 years ago

I have to say, I'm a bit uneasy about re-using the dls_async_get_all_t and the get_all functions in this way. There might be more problems like this. Perhaps, it would be best to move all the common code out of prv_get_all and prv_get_object functions into some new functions that can be used by prv_get_all, prv_get_object and a new prv_browse_object function. This way browse has it's own methods and data structures and so there shouldn't be any other weird problems like this. In addition going forward it will mean that browse won't get broken by changes to get all.

lferrandis commented 11 years ago

I will reset the structure before each call.

On 25/09/2013 14:59, markdryan wrote:

This still isn't working quite right. The problem I think is that functions like prv_get_all assume that the dls_async_get_all_t structure is initialised to 0 before they execute. This is true for GetAll as the structure is initialised to 0 when the task object is created and is only used once. The problem with BrowseObjects is that the same object is used multiple times, i.e., once of each object. This causes problems. For example,

|root.browse_objects(["/com/intel/dLeynaServer/server/0","/com/intel/dLeynaServer/server/0/2f686f6d652f6d61726b75732f444c4e41436f6e74656e742f566964656f2f4d5045475f50535f50414c2f422d4d503250535f502d322e6d7067"],['ChildCount']) { "ChildCount": 2 }

{ "ChildCount": 0 } |

The second child is an item but dLeyna-server tries to retrieve the childcount for this object. I think this is because the need_child_count flag is set to true from the first object which is a container.

— Reply to this email directly or view it on GitHub https://github.com/01org/dleyna-server/pull/130#issuecomment-25083615.

Ludovic Ferrandis Open Source Technology Center Intel Corporation

lferrandis commented 11 years ago

On 25/09/2013 15:12, markdryan wrote:

I have to say, I'm a bit uneasy about re-using the dls_async_get_all_t and the get_all functions in this way. There might be more problems like this. Perhaps, it would be best to move all the common code out of prv_get_all and prv_get_object functions into some new functions that can be used by prv_get_all, prv_get_object and a new prv_browse_object function. This way browse has it's own methods and data structures and so there shouldn't be any other weird problems like this. In addition going forward it will mean that browse won't get broken by changes to get all.

— Reply to this email directly or view it on GitHub https://github.com/01org/dleyna-server/pull/130#issuecomment-25084460.

I have an upcomming patch for this. I have completely rewrite the get property feature. Get & GetAll and all others methode that need to build array of properties now use a single function. Currently we have 2 paths, one for Get, one for GetAll, and we reuse some of the functions in GetResources, Listxx.

So, in a near future, we should have a simplified way to manage properties.

Ludovic Ferrandis Open Source Technology Center Intel Corporation

lferrandis commented 11 years ago

Fixed. The 2 commit to fixup are for commit: [BrowseObject]: Support Of ChildCount property

markdryan commented 11 years ago

I have an upcomming patch for this. I have completely rewrite the get property feature.

Understood. So the plan is to get Browse working properly with the current, somewhat creaky implementation, and then clean it up in the next release. That's fine for me. I'll give it a good test now and if everything's okay, I'll submit.

markdryan commented 11 years ago

I'm getting a bug with browse objects but I don't think it's related to this commit. browse_objects is returning the wrong number of results if one of the objects being requested does not exist, e.g.,

root.browse_objects(["/com/intel/dLeynaServer/server/1/3732356665373461643964306463326536356236363830613133333330386364","/com/intel/dLeynaServer/server/1/6265303936386233376134633530393934336539386536613037353930656639","/com/intel/dLeynaServer/server/1/3","/com/intel/dLeynaServer/server/1/3133373234303164653836376230313163363161323862393161636433623763"],["DisplayName","ChildCount"])
{
    "ChildCount": 4, 
    "DisplayName": "WMVMED_PRO"
}

{
    "DisplayName": "O-WVM_P-01_PAL_CBR_6M_384_44_2.wmv"
}

{
    "Error": {
        "ID": 0, 
        "Message": "object path is badly formed."
    }
}

I'm expecting 4 elements in the returned array but I'm only getting 3. As far as I can tell this problem is not related to this PR. Do you agree?

lferrandis commented 11 years ago

I'm going to check it right now.

On 26/09/2013 10:03, markdryan wrote:

I'm getting a bug with browse objects but I don't think it's related to this commit. browse_objects is returning the wrong number of results if one of the objects being requested does not exist, e.g.,

|root.browse_objects(["/com/intel/dLeynaServer/server/1/3732356665373461643964306463326536356236363830613133333330386364","/com/intel/dLeynaServer/server/1/6265303936386233376134633530393934336539386536613037353930656639","/com/intel/dLeynaServer/server/1/3","/com/intel/dLeynaServer/server/1/3133373234303164653836376230313163363161323862393161636433623763"],["DisplayName","ChildCount"]) { "ChildCount": 4, "DisplayName": "WMVMED_PRO" }

{ "DisplayName": "O-WVM_P-01_PAL_CBR_6M_384_44_2.wmv" }

{ "Error": { "ID": 0, "Message": "object path is badly formed." } } |

I'm expecting 4 elements in the returned array but I'm only getting 3. As far as I can tell this problem is not related to this PR. Do you agree?

— Reply to this email directly or view it on GitHub https://github.com/01org/dleyna-server/pull/130#issuecomment-25150049.

Ludovic Ferrandis Open Source Technology Center Intel Corporation

lferrandis commented 11 years ago

On 26/09/2013 10:03, markdryan wrote:

I'm getting a bug with browse objects but I don't think it's related to this commit. browse_objects is returning the wrong number of results if one of the objects being requested does not exist, e.g.,

|root.browse_objects(["/com/intel/dLeynaServer/server/1/3732356665373461643964306463326536356236363830613133333330386364","/com/intel/dLeynaServer/server/1/6265303936386233376134633530393934336539386536613037353930656639","/com/intel/dLeynaServer/server/1/3","/com/intel/dLeynaServer/server/1/3133373234303164653836376230313163363161323862393161636433623763"],["DisplayName","ChildCount"]) { "ChildCount": 4, "DisplayName": "WMVMED_PRO" }

{ "DisplayName": "O-WVM_P-01_PAL_CBR_6M_384_44_2.wmv" }

{ "Error": { "ID": 0, "Message": "object path is badly formed." } } |

I'm expecting 4 elements in the returned array but I'm only getting 3. As far as I can tell this problem is not related to this PR. Do you agree?

— Reply to this email directly or view it on GitHub https://github.com/01org/dleyna-server/pull/130#issuecomment-25150049.

Ok. It's a bug introduced while implementing the get_children. Previously, we create a service task list pre-filled with all the task to execute. When an error occurs, the task processor simply start the next task in the queue.

With the new implementation, the service task contains only 1 task at a time, and we dynamically add new tasks, depending of the context. We add the new browse-object or the get_child_count. But in case of error we don't add a new task in the queue.

I'm going to fix it right now.

Ludovic Ferrandis Open Source Technology Center Intel Corporation

markdryan commented 11 years ago

Okay, so it is actually a problem with this PR. That explains it.

lferrandis commented 11 years ago

Fix pushed in Fixup 3

On 26/09/2013 10:30, markdryan wrote:

Okay, so it is actually a problem with this PR. That explains it.

— Reply to this email directly or view it on GitHub https://github.com/01org/dleyna-server/pull/130#issuecomment-25151351.

Ludovic Ferrandis Open Source Technology Center Intel Corporation

markdryan commented 11 years ago

The thing is, I think this is inconsistent with what happens with GetAll and list_children. As far as I can tell these functions return an error if the childcount cannot be retrieved.

lferrandis commented 11 years ago

I will keep the same behavior.

On 26/09/2013 11:27, markdryan wrote:

The thing is, I think this is inconsistent with what happens with GetAll and list_children. As far as I can tell these functions return an error if the childcount cannot be retrieved.

— Reply to this email directly or view it on GitHub https://github.com/01org/dleyna-server/pull/130#issuecomment-25154215.

Ludovic Ferrandis Open Source Technology Center Intel Corporation

lferrandis commented 11 years ago

Done. Now if get_child_count failed, we don't return partial result, only an error result.

On 26/09/2013 11:27, markdryan wrote:

The thing is, I think this is inconsistent with what happens with GetAll and list_children. As far as I can tell these functions return an error if the childcount cannot be retrieved.

— Reply to this email directly or view it on GitHub https://github.com/01org/dleyna-server/pull/130#issuecomment-25154215.

Ludovic Ferrandis Open Source Technology Center Intel Corporation

markdryan commented 11 years ago

Thanks. This is working well for me now. Do you mind if I squash all the commits into a single commit? It will be a little difficult to work out which individual commits to merge.

lferrandis commented 11 years ago

No, go ahead.

On 26/09/2013 12:46, markdryan wrote:

Thanks. This is working well for me now. Do you mind if I squash all the commits into a single commit? It will be a little difficult to work out which individual commits to merge.

— Reply to this email directly or view it on GitHub https://github.com/01org/dleyna-server/pull/130#issuecomment-25158345.

Ludovic Ferrandis Open Source Technology Center Intel Corporation

markdryan commented 11 years ago

Great. It's merged. Thanks.