Skip to content

ADD: Write-enabled WEBDAV #1164

Merged
merged 0 commits into from Oct 8, 2012

5 participants

@koying
Team Kodi member
koying commented Jul 15, 2012

Adds delete/rename/write to webdav resources (files & directory)

@arnova arnova and 1 other commented on an outdated diff Jul 15, 2012
xbmc/filesystem/DAVCommon.cpp
+/*
+ * Search for <status> and return its content
+ */
+CStdString CDAVCommon::GetStatusTag(const TiXmlElement *pElement)
+{
+ const TiXmlElement *pChild;
+
+ for (pChild = pElement->FirstChild()->ToElement(); pChild != 0; pChild = pChild->NextSibling()->ToElement())
+ {
+ if (ValueWithoutNamespace(pChild, "status"))
+ {
+ return CStdString(pChild->GetText());
+ }
+ }
+
+ return CStdString("");
@arnova
Team Kodi member
arnova added a note Jul 15, 2012

No need to cast. Just return "";

@koying
Team Kodi member
koying added a note Jul 15, 2012

ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnova arnova commented on an outdated diff Jul 15, 2012
xbmc/filesystem/DAVFile.cpp
+
+ long response = m_state->Connect(m_bufferSize);
+ if( response < 0 || response >= 400)
+ return false;
+
+ SetCorrectHeaders(m_state);
+
+ // since we can't know the stream size up front if we're gzipped/deflated
+ // flag the stream with an unknown file size rather than the compressed
+ // file size.
+ if (m_contentencoding.size() > 0)
+ m_state->m_fileSize = 0;
+
+ m_multisession = false;
+ if(m_url.Left(5).Equals("http:") || m_url.Left(6).Equals("https:"))
+ {
@arnova
Team Kodi member
arnova added a note Jul 15, 2012

Isn't the result here static?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnova arnova commented on an outdated diff Jul 15, 2012
xbmc/filesystem/DAVFile.cpp
+ if(m_url.Left(5).Equals("http:") || m_url.Left(6).Equals("https:"))
+ {
+ m_multisession = true;
+ }
+
+ if(m_state->m_httpheader.GetValue("Transfer-Encoding").Equals("chunked"))
+ m_state->m_fileSize = 0;
+
+ m_seekable = false;
+ if(m_state->m_fileSize > 0)
+ {
+ m_seekable = true;
+
+ if(url2.GetProtocol().Equals("http")
+ || url2.GetProtocol().Equals("https"))
+ {
@arnova
Team Kodi member
arnova added a note Jul 15, 2012

Same

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnova arnova and 1 other commented on an outdated diff Jul 15, 2012
xbmc/filesystem/FileFactory.cpp
@@ -131,12 +132,11 @@ IFile* CFileFactory::CreateLoader(const CURL& url)
if( g_application.getNetwork().IsAvailable() )
{
- if (strProtocol == "http"
- || strProtocol == "https"
- || strProtocol == "dav"
- || strProtocol == "davs"
- || strProtocol == "ftp"
- || strProtocol == "ftps"
+ if (strProtocol == "dav" || strProtocol == "davs") return new CDAVFile();
+ else
+ if (strProtocol == "http" || strProtocol == "https"
+ || strProtocol == "ftp" || strProtocol == "ftps"
+// || strProtocol == "dav" || strProtocol == "davs"
@arnova
Team Kodi member
arnova added a note Jul 15, 2012

No need to comment this, just remove it.

@elupus
Team Kodi member
elupus added a note Jul 25, 2012

remove and add a new line for dav keeping old formatting. diff you should be 3 lines or so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@koying
Team Kodi member
koying commented Jul 15, 2012

Updated.

@arnova arnova and 1 other commented on an outdated diff Jul 17, 2012
xbmc/filesystem/DAVFile.cpp
+ m_state->m_fileSize = 0;
+
+ m_multisession = true;
+
+ if(m_state->m_httpheader.GetValue("Transfer-Encoding").Equals("chunked"))
+ m_state->m_fileSize = 0;
+
+ m_seekable = false;
+ if(m_state->m_fileSize > 0)
+ {
+ m_seekable = true;
+
+ // if server says explicitly it can't seek, respect that
+ if(m_state->m_httpheader.GetValue("Accept-Ranges").Equals("none"))
+ m_seekable = false;
+ }
@arnova
Team Kodi member
arnova added a note Jul 17, 2012

This looks a little backwards now: setting m_seekable to false, then true, then false again. I think you should just negate the if statement at line 148?

@koying
Team Kodi member
koying added a note Jul 17, 2012

ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnova arnova and 1 other commented on an outdated diff Jul 17, 2012
xbmc/filesystem/DAVFile.cpp
+
+ CLog::Log(LOGDEBUG, "CDAVFile::OpenForWrite(%p) %s", (void*)this, m_url.c_str());
+
+ ASSERT(m_state->m_easyHandle == NULL);
+ g_curlInterface.easy_aquire(url2.GetProtocol(), url2.GetHostName(), &m_state->m_easyHandle, NULL);
+
+ SetCommonOptions(m_state);
+ SetRequestHeaders(m_state);
+ g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_TIMEOUT, 5);
+ g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_NOBODY, 1);
+ g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_WRITEDATA, NULL); /* will cause write failure*/
+
+ CURLcode result = g_curlInterface.easy_perform(m_state->m_easyHandle);
+
+ if (result == CURLE_WRITE_ERROR || result == CURLE_OK)
+ if (!bOverWrite) {
@arnova
Team Kodi member
arnova added a note Jul 17, 2012

Please honor XBMC coding conventions: put {} on their own lines.

@koying
Team Kodi member
koying added a note Jul 17, 2012

ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnova arnova and 1 other commented on an outdated diff Jul 17, 2012
xbmc/filesystem/DAVFile.cpp
+int CDAVFile::Write(const void* lpBuf, int64_t uiBufSize)
+{
+ if (!m_opened)
+ return -1;
+
+ ASSERT(m_state->m_easyHandle);
+
+ SetCommonOptions(m_state);
+ SetRequestHeaders(m_state);
+ m_state->SetReadBuffer(lpBuf, uiBufSize);
+ g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_UPLOAD, 1);
+ g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_INFILESIZE_LARGE, uiBufSize);
+
+ CURLcode result = g_curlInterface.easy_perform(m_state->m_easyHandle);
+
+ if (result != CURLE_OK) {
@arnova
Team Kodi member
arnova added a note Jul 17, 2012

Same

@koying
Team Kodi member
koying added a note Jul 17, 2012

ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnova
Team Kodi member
arnova commented Jul 17, 2012

Nice work btw.

@koying
Team Kodi member
koying commented Jul 17, 2012

Updated

@arnova
Team Kodi member
arnova commented Jul 24, 2012

@elupus & @vdrfan: Mind ack-ing this?

@mkortstiege
Team Kodi member

Just had a brief look at the code and commented a few things. No functionality check as i do not have any DAV servers around at the moment.

@koying
Team Kodi member
koying commented Jul 25, 2012

Updated.Once this is merged, I propose to implement write support for all "standard" libcurl protocols (FTP, SFTP, ...)

@arnova
Team Kodi member
arnova commented Jul 25, 2012

Sure, if you would be so kind to provide additional patches for that ;-)

@koying
Team Kodi member
koying commented Jul 25, 2012

Would you prefer a big pull request with all, or this one first and another one for the other protocols?

@elupus
Team Kodi member
elupus commented Jul 25, 2012

separate pull for separate file systems

@elupus elupus and 1 other commented on an outdated diff Jul 25, 2012
xbmc/filesystem/DAVFile.cpp
+ SetRequestHeader("Content-Range", range.c_str());
+ SetRequestHeaders(m_state);
+
+ CURLcode result = g_curlInterface.easy_perform(m_state->m_easyHandle);
+
+ if (result != CURLE_OK)
+ {
+ long code;
+ if(g_curlInterface.easy_getinfo(m_state->m_easyHandle, CURLINFO_RESPONSE_CODE, &code) == CURLE_OK )
+ CLog::Log(LOGERROR, "%s - unable to write dav resource (%s) - %ld", __FUNCTION__, m_url.c_str(), code);
+ return -1;
+ }
+
+ m_writeOffset += uiBufSize;
+ return uiBufSize;
+}
@elupus
Team Kodi member
elupus added a note Jul 25, 2012

This looks insanely inefficient. One request per call. We can happen to write just a few bytes at a time.

Does dav/http need to know number of total bytes to write up front? If not, the mult_perform syntax like for read aught to be used and send data as we go along, closing connection when done.

@koying
Team Kodi member
koying added a note Jul 25, 2012

Actually, there is no provision in webdav for partial writes at all. The standard HTTP put is supposed to be used, i.e. all in one pass.
Doing a PUT with Content-Range is kind of a "de facto" standard.

Mult interface = asynchronous calls = buffering of the data
I thought about buffering, but that would hardly be feasible for large files, e.g. videos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elupus elupus commented on an outdated diff Jul 25, 2012
xbmc/filesystem/FileFactory.cpp
@@ -131,12 +132,10 @@ IFile* CFileFactory::CreateLoader(const CURL& url)
if( g_application.getNetwork().IsAvailable() )
{
- if (strProtocol == "http"
- || strProtocol == "https"
- || strProtocol == "dav"
- || strProtocol == "davs"
- || strProtocol == "ftp"
- || strProtocol == "ftps"
+ if (strProtocol == "dav" || strProtocol == "davs") return new CDAVFile();
+ else
+ if (strProtocol == "http" || strProtocol == "https"
+ || strProtocol == "ftp" || strProtocol == "ftps"
@elupus
Team Kodi member
elupus added a note Jul 25, 2012

Don't reformat here, remove dav(s) add a ifclause above or below. (mentioned on the old diff).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnova
Team Kodi member
arnova commented Aug 1, 2012

@elupus: Can this go in?

@elupus
Team Kodi member
elupus commented Aug 1, 2012
@koying
Team Kodi member
koying commented Aug 3, 2012

@elupus As far as I know, Write is used for exports and in the file manager.
In both case, my tests shows that writes are done in chunk of 128k.

I'd suggest to first find a case when one byte writes would be done rather than write code for no practical purpose. To cope both with 1 byte and 2Gb writes would be quite complicated...

Even if such a case exists, however inefficient, the code works, and it would maybe be best to fix the inefficient original write ;)

@arnova
Team Kodi member
arnova commented Aug 3, 2012

@elupus: I have to agree with koying. Perhaps merge this and see over time whether any problems exist with it?

@koying: Please rebase your PR

@koying
Team Kodi member
koying commented Aug 3, 2012

Rebased.

@elupus
Team Kodi member
elupus commented Aug 15, 2012

It's way duplication of code imho.. It should just have been put in FileCurl. Also, i just checked docs and the read_callback function can return a special PAUSE command to indicate there is no data available right now but will be later. So it should not be too complicated to do this by multi perform instead, having read_callback return pause when it has no data.

@koying
Team Kodi member
koying commented Aug 16, 2012

@elupus :
1) There might be code duplication, but as only DAV is now tested as write-enabled, and I have been asked to write-enable the other protocols in another patch, it doesn't belong to CurlFile, IMHO.
2) What exactly is the PAUSE supposed to achieve? As I understand it, that would just make the curl async interface synchronized to cope with the synchronized input. Are there specific benefits of the multi interface I'm not aware of?

@all:
Code reviews are fine and necessary, but has anyone actually done a functional test?
Over-engineering/optimizing for non-existing issues might be fun, but I'd rather receive feedback for actual functional problems...

@elupus
Team Kodi member
@koying
Team Kodi member
koying commented Aug 16, 2012

I understand that, but what's the point if the file adaptor still has to wait for input?
Or it has to buffer the writes, which are already sent in chunks of 128k... How large should the buffer be, considering at least part of the usage will be to copy video files of 2Gb+?
Are you certain libcurl doesn't have it's own buffering?

Bottom line: what is the benefit?

@elupus
Team Kodi member
@koying
Team Kodi member
koying commented Aug 16, 2012

Ok. Will do.

@NedScott
NedScott commented Oct 1, 2012

Candidate for Frodo maybes?

@elupus
Team Kodi member
elupus commented Oct 1, 2012
@koying
Team Kodi member
koying commented Oct 1, 2012

Well, I spent hours trying to make this multi interface work to no avail.
How hard I try, the read (for writing) callback is only called once (if even)

Help welcome...

A thing I learned, though, is that, even in easy mode, connections are cached, so I don't think you'll actually get 1 connection by chunk

@koying
Team Kodi member
koying commented Oct 5, 2012

@elupus Finally succeeded to have multi working ;)
I've also moved everything not DAV-specific to CurlFile.cpp

@elupus
Team Kodi member
elupus commented Oct 5, 2012
@arnova arnova and 1 other commented on an outdated diff Oct 6, 2012
xbmc/filesystem/File.cpp
@@ -518,6 +518,9 @@ void CFile::Close()
{
try
{
+ if (m_pFile)
+ m_pFile->Close();
+
@arnova
Team Kodi member
arnova added a note Oct 6, 2012

Where did this come from? Seems unrelated...

@koying
Team Kodi member
koying added a note Oct 6, 2012

1) In general, closing a file didn't close the embedded IFile descendant, which seems like a bug to me
2) It is needed for libcurl write, as closing the CurlFile writes a 0 bytes packet that is needed to "commit" the write

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@koying koying merged commit 64ef2fb into xbmc:master Oct 8, 2012
@koying
Team Kodi member
koying commented Oct 8, 2012

Sorry guys, I f... up with git and github automatically closed this PR, so I had to re-create it here: #1567

@tru tru added a commit to RasPlex/plex-home-theatre that referenced this pull request May 9, 2014
@tru tru Handle Photos in PlayAll/ShuffleAll commands.
Fixes #1164
8917013
@tru tru added a commit to RasPlex/plex-home-theatre that referenced this pull request May 9, 2014
@tru tru Handle Photos in PlayAll/ShuffleAll commands.
Fixes #1164
c0f6456
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.