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

Allow binary addon to get multiple values for same property name #12780

Merged
merged 1 commit into from Sep 14, 2017

Conversation

@rbuehlma
Copy link
Contributor

commented Sep 7, 2017

This is an extension to #12737

There are some properties (probably only HTTP headers) which can have multiple values for the same key. (e.g. set-cookie can be sent multiple times by the server).
With GetFileProperties, it is possible to get a list of all these values.

@peak3d this function could replace File::GetProperty but as GetProperty is simpler to call and probably sufficient in most cases, I think its better to keep GetProperty. What do you think?

(sorry to bug again with "my cookies". but as far as I can see, this is the last remaining issue :-) )

Copy link
Member

left a comment

Please also add implementation of the new functionality for "new style" API.

@@ -478,6 +479,19 @@ namespace ADDON
}

/*!
* @brief Get multiple properties from an open file.
* @param file The file to the an properties for

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

Seems this needs rephrasing!

* @param file The file to the an properties for
* @param type type of the requested properties.
* @param name of the requested properties / can be null.
* @param numProperties number of properties returned.

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

Please make the first letter after the parameter name capital - applies to above three lines.

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

I don't see a reason to make such a big PR for an solution wich could be done in some lines of code in CCUrlFile. If multile header values are possible, we should simply iterate through them in CCurlFile::GetProperty() and attach them to the result string in the kodi convention (value&value&vaue) in this special case.

@rbuehlma

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2017

I have added the new style api and included the review outcome.

Yes, we could simply join the values somehow together into a single string but I do not consider that as a clean approach. What happens if the separator is used in the header value? We would need to encode and later decode the values.

I could replace the GetProperty functions with GetProperties in the Kodi internal code and on the interface simply return the first value of GetProperties if GetProperty is called. That would already reduce the code changes.

@Montellese

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

@peak3d: you're gonna have a hard time finding a character that you can use as a separator in the joined string that isn't allowed in the HTTP header value.

In our own webserver implementation I'm using an std::multimap to manage HTTP headers.

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

For passing such stuff (addon -> kodi and vice versa, see CCurlFile::GetCookies() we normally URLEncode the strings. Same for postdata, where we use b64 encoding.

Question is if we want to proceed with encoding, then its easy to select an separator (same as we do nowdays: &).

My approach without concatting would be pass an additional parameter through GetProperty (lastFetched, or a magic id) to tell GetProperty where to continue.

For me this PR feels like code duplicating, and tbh I mislike it.

@rbuehlma

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2017

This commit removes some code duplications.

* @param type type of the requested property.
* @param name of the requested property / can be null.
* @param type Type of the requested property.
* @param name Of the requested property / can be null.

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

This does not make sense. => "@param name Name of the ..."

* @brief Get multiple properties from an open file.
* @param file The file to get the properties for
* @param type Type of the requested properties.
* @param name Of the requested properties / can be null.

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

This does not make sense. => "@param name Name of the ..."

@ksooo

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

I think the concat and base64 encode approach @peak3d suggested makes sense and will actually work just fine.

I do not like the "paging" approach. More complexity, more calls, no real gain, imo.

@rbuehlma

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2017

I agree that concat and encode would work. But we would need to do that also for a single value. The addon would then need to decode.

The code duplication in kodi would probably remain as we do not want to encode/decode in code internally but only for the api, right?

Apart from that, why do you think it is nicer to pass a list of values as a concatenated and encoded string instead of using an array?

@ksooo

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

Apart from that, why do you think it is nicer to pass a list of values as a concatenated and encoded string instead of using an array?

I think passing the values in an array/vector is the cleanest approach. My point was that I dislike paging and that I think encode/decode should work. ;-)

@rbuehlma

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2017

I think passing the values in an array/vector is the cleanest approach. My point was that I dislike paging and that I think encode/decode should work. ;-)

Well, then I fully agree. Its ok for me to implement the base64 approach if required. Who can/will decide here which way we should go?

@ksooo

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

Who can/will decide here which way we should go?

No hard feelings here. I can live with both the encode and the array approach.

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

vector (/array) is fine for me, code samples how to pass vectors can be found in VFS::OpenDirectory

@ksooo

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

code samples how to pass vectors can be found in VFS::OpenDirectory

it's fine like @rbuehlma currently implemented it, imo.

@@ -110,6 +111,7 @@ class CFile
void Close();
int GetChunkSize();
const std::string GetProperty(XFILE::FileProperty type, const std::string &name = "") const;
const std::vector<std::string> GetProperties(XFILE::FileProperty type, const std::string &name = "") const;

This comment has been minimized.

Copy link
@Rechi

Rechi Sep 8, 2017

Member

... const override;

This comment has been minimized.

Copy link
@rbuehlma

rbuehlma Sep 8, 2017

Author Contributor

Its not an override here. The compiler complains if I add "override".

This comment has been minimized.

Copy link
@Rechi

Rechi Sep 9, 2017

Member

you are right, override is missing in a different file, which overrides this function

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

I'm unhappy with the name "GetProperties" as we only retrieve one property (regardless if it produces multiple values). @rbuehlma can you pls. remove my GetProperty() and rename your GetProperty()

If I'm alone with the naming of the function I'm ok with GetProperties. But it sounds not correct at least for me.

@ksooo

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

as we only retrieve one property (regardless if it produces multiple values)

If I'm alone with the naming of the function I'm ok with GetProperties.

If it's single property name - multiple values, then I'd suggest GetPropertyValues. ;-)

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

+1 @ksooo :-)

@rbuehlma rbuehlma force-pushed the rbuehlma:get_multiple_properties branch from 5db17b1 to dfa9354 Sep 8, 2017
@rbuehlma

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2017

Did the rename as proposed by @ksooo. Also did a squash.

vecReturn.push_back(res[i]);
::kodi::addon::CAddonBase::m_interface->toKodi->free_string(::kodi::addon::CAddonBase::m_interface->toKodi->kodiBase, res[i]);
}
::kodi::addon::CAddonBase::m_interface->toKodi->free_string(::kodi::addon::CAddonBase::m_interface->toKodi->kodiBase, (char*)res);

This comment has been minimized.

Copy link
@rbuehlma

rbuehlma Sep 8, 2017

Author Contributor

Is it ok the way I free the string-array here? The contained elements are already freed above.

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

Not sure. free_string is, as the name implies, for freeing strings (char*), not arbitrary heap allocated objects. @AlwinEsch ?

This comment has been minimized.

Copy link
@peak3d

peak3d Sep 8, 2017

Contributor

Question is if freestring is capable of freeing : char *ret = new char[*numValues];
I don't believe so, but just check freestring impl. and you know, but I think you have to use an malloc instead new[]

This comment has been minimized.

Copy link
@rbuehlma

rbuehlma Sep 8, 2017

Author Contributor
void CAddonDll::free_string(void* kodiBase, char* str)
{
  if (str)
    free(str);
}

I would say its technically ok.

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

No it's not. new[] and free do not fit together!

This comment has been minimized.

Copy link
@rbuehlma

rbuehlma Sep 8, 2017

Author Contributor

Oh, i see, free may only be used with malloc... Changed the allocation to malloc.

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

array vs non-array, c++ vs libc.

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

mallocis technically okay, then.

But my clear -1 for using free_stringto free the array - for semantical reasons. free_string's semantics is to free a single string not an array and the fact that it internally uses free which does technically "fit" is nothing more than an implementation detail that can change anytime.

Sorry, but imo we need another API functionfree_stringarray, which does the whole job, freeing the array elements and the array itself.

Copy link
Member

left a comment

not just nitpicking this time...

if (values.empty()) {
return nullptr;
}
strdup(values[0].c_str());

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

return missing ???

if (!helper)
return nullptr;

CFile* cfile = (CFile*)file;

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

Please use c++ style casts - here and everywhere in the code your PR introduces. Yes, we use c style casts in old code, but should use only c++ style casts in new code.

}
std::vector<std::string> values = cfile->GetPropertyValues(type, name);
*numValues = values.size();
char **ret = new char*[*numValues];

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

If you allocate the memory with new[]you definitely cannot free it with free_string(which internally uses free)!!! Do we need another C API function to free arrays? @AlwinEsch any opinion on this?

std::vector<std::string> values = cfile->GetPropertyValues(type, name);
*numValues = values.size();
char **ret = new char*[*numValues];
for (int i = 0; i < *numValues; i++)

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

I've learned that it is always good practice to use prefix operator++, not postfix - for performance reasons...

@@ -80,6 +80,7 @@ class CAddonCallbacksAddon
static bool FileExists(const void* addonData, const char *strFileName, bool bUseCache);
static int StatFile(const void* addonData, const char *strFileName, struct __stat64* buffer);
static char *GetFileProperty(const void* addonData, void* file, XFILE::FileProperty type, const char *name);

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

For consistency reasons, I would rename all existing single name - single value methods to GetFilePropertyValue.

@@ -110,6 +111,7 @@ class CFile
void Close();
int GetChunkSize();
const std::string GetProperty(XFILE::FileProperty type, const std::string &name = "") const;

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

GetPropertyValue

virtual const std::vector<std::string> GetPropertyValues(XFILE::FileProperty type, const std::string &name = "") const
{
std::vector<std::string> values;
return values;

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

No need for a variable here. return std::vector<std::string>(); is sufficient.

std::string value = GetProperty(type, name);
if (!value.empty())
{
values.push_back(value);

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

emplace_back

@@ -77,6 +77,7 @@ CAddonCallbacksAddon::CAddonCallbacksAddon(CAddon* addon)
m_callbacks->FileExists = FileExists;
m_callbacks->StatFile = StatFile;
m_callbacks->GetFileProperty = GetFileProperty;

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

GetFilePropertyValue (see my other comment re the naming)

This comment has been minimized.

Copy link
@peak3d

peak3d Sep 8, 2017

Contributor

GetFilePropertyValues (!!!)

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

No. We have two API functions now. One for single name - single value => GetFilePropertyValue
One for single name - multi values => GetFilePropertyValues

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

No. We have two API functions now. One for single name - single value, one for single name - multi values. Please look at the full diff and you will see what I mean.

@@ -18,6 +18,7 @@
*
*/

#include <vector>

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

We usually put a blank line between system headers and "own" headers.

@rbuehlma rbuehlma force-pushed the rbuehlma:get_multiple_properties branch from dfa9354 to 58038fd Sep 8, 2017
@peak3d

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

@rbuehlma this is business as usual, maybe it heads up: If I PR something in @ksooo 's area it's the same.
But, I have learned that his comments give a good feeling if its finally done.

@ksooo ksooo added this to the L 18.0-alpha1 milestone Sep 8, 2017
@ksooo

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

But, I have learned that his comments give a good feeling if its finally done.

:-)

Copy link
Member

left a comment

Did I already say "API bump" in this PR?

@@ -156,6 +156,7 @@ typedef struct CB_AddOn
bool (*FileExists)(const void* addonData, const char *strFileName, bool bUseCache);
int (*StatFile)(const void* addonData, const char *strFileName, struct __stat64* buffer);
char *(*GetFileProperty)(const void* addonData, void* file, XFILE::FileProperty type, const char *name);
char **(*GetFilePropertyValues)(const void* addonData, void* file, XFILE::FileProperty type, const char *name, int *numPorperties);

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 8, 2017

Member

Sorry, to mention this, but ofc this change (inserting a new function in the middle of the function table) makes each and every binary addon that still uses old-style API (like PVR addons) incompatible and therefore an API bump is required.

@ksooo ksooo dismissed their stale review Sep 8, 2017

did not (yet) want to approve

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

1.) IIRC we have agreed to only have 1 API function: GetFilePropertyValues() and wanted to remove GetFileProperty instead.
2.) Position of API method should not change anything, they are dyloaded function by function (and not VMT based)

@ksooo

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

1.) IIRC we have agreed to only have 1 API function: GetFilePropertyValues() and wanted to remove GetFileProperty instead.

Fine by me.

2.) Position of API method should not change anything, they are dyloaded function by function (and not VMT based)

I admit that I always get puzzled about this stuff. So, I shut up at this point and trust in your expertise.

@rbuehlma

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2017

@rbuehlma this is business as usual, maybe it heads up: If I PR something in @ksooo 's area it's the same.

I have no problem getting some feedback :) C/C++ is not really a language I'm used to. Hopefully I will improve.

I personally would prefer to keep GetProperty (aka GetPropertyValue) in the API as in over 90% of the time, only one value is expected and the call to it is much simpler.

I did implement the review outcome except the additional free function for arrays, shall I implement that as well?

I'm not sure about the API bump. Now that we rename GetProperty to GetPropertyValue, I guess a bump is required any way? How would I do that?

@rbuehlma rbuehlma force-pushed the rbuehlma:get_multiple_properties branch 2 times, most recently from 6d2f0cd to 471d94b Sep 9, 2017
@rbuehlma

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2017

Added FreeStringArray

@@ -61,6 +61,11 @@ namespace XFILE

const std::string GetProperty(XFILE::FileProperty type, const std::string &name = "") const override;

virtual const std::vector<std::string> GetPropertyValues(XFILE::FileProperty type, const std::string &name = "") const

This comment has been minimized.

Copy link
@Rechi

Rechi Sep 9, 2017

Member

this is the function which needs the override specifier

@peak3d

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2017

@rbuehlma Ok, go ahead with keeping the previous one.
It#s still / espacially after this discussion / complexity of vector transport my deep opinion that a simple iterator logic would be much nicer / simpler / faster / transparent than this approach here.

@rbuehlma

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2017

Created PR against inputstream.adaptive:
peak3d/inputstream.adaptive#67

Let me know when I should squash the commits of this PR.

@Rechi

This comment has been minimized.

Copy link
Member

commented Sep 9, 2017

If this is now the final state, please squash the commits.

@ksooo

This comment has been minimized.

Copy link
Member

commented Sep 9, 2017

Now that we rename GetProperty to GetPropertyValue, I guess a bump is required any way? How would I do that?

https://github.com/xbmc/xbmc/blob/master/xbmc/addons/kodi-addon-dev-kit/include/kodi/versions.h#L45 => Increase micro version for both ADDON_GLOBAL_VERSION_MAIN and ADDON_GLOBAL_VERSION_MAIN_MIN

https://github.com/xbmc/xbmc/blob/master/xbmc/addons/kodi-addon-dev-kit/include/kodi/versions.h#L70 => Increase micro version for both ADDON_GLOBAL_VERSION_FILESYSTEM and ADDON_GLOBAL_VERSION_FILESYSTEM _MIN

@rbuehlma rbuehlma force-pushed the rbuehlma:get_multiple_properties branch from d60d750 to a7ae897 Sep 9, 2017
@rbuehlma

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2017

Squash and version bump done.

This change has been tested with pvr.zattoo, pvr.teleboy and inputstream.adaptive.

For pvr.teleboy to work, this PR is required as well: #12779 But I expect less iterations there :)

@ksooo

This comment has been minimized.

Copy link
Member

commented Sep 9, 2017

@MilhouseVH heads up! All binary addons need a rebuild if this PR gets merged.

@ksooo
ksooo approved these changes Sep 9, 2017
Copy link
Member

left a comment

Looks good. Thanks for your patience.

@cooley69

This comment has been minimized.

Copy link

commented Sep 9, 2017

@ksooo

This comment has been minimized.

Copy link
Member

commented Sep 9, 2017

What???

There might be multiple occurences of a http header (e.g. set-cookie)
@rbuehlma rbuehlma force-pushed the rbuehlma:get_multiple_properties branch from a7ae897 to 09fec8a Sep 10, 2017
@rbuehlma

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2017

Amend without changes to trigger jenkins rebuild.

@Rechi

This comment has been minimized.

Copy link
Member

commented Sep 11, 2017

jenkins build this please

@ksooo ksooo merged commit af97563 into xbmc:master Sep 14, 2017
1 check failed
1 check failed
default Sorry, building this PR failed. Please check the logs for the errors.
Details
@rbuehlma rbuehlma deleted the rbuehlma:get_multiple_properties branch Sep 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.