Decide what to do with the XrdCl::OpenFlags::Append flag

[murrayc3]

The XrdCl::OpenFlags::Append flag is part of the XRootD client API. The flag is mapped to the kXR_open_apnd constant of the XRootD protocol. However the flag is not mapped and passed to the XrdSfsFile::open() method of the XRootD server API.

Please could the XrdCl::OpenFlags::Append flag either be completely removed from the XRootD client API or could the XRootD server map kXR_open_apnd to a dedicated server side flag and pass it through to the XrdSfsFile::open() method?

This issue is being created due to the following incident which uncovered the fact that the XRootD server does not pass kXR_open_apnd through to XrdSfsFile::open(). A client contacted an XRootD server with an "open for append" request using the XrdCl::OpenFlags::Append flag. This request should have been considered an "open for write" however the server incorrectly interpreted it as an "open for read" because "#define SFS_O_RDONLY 0 // open read/only".

[murrayc3]

To summarize the XRootD server currently has a bug because it converts an "open for append" request to an "open for read" request.

[abh3]

Well, it depends on how you define "bug" :-) Historically, flags have been defined for future features to make sure other developers don't interfere by defining conflicting flags. Such is the distributed nature of the collaboration. The append flag falls into this category. The reaction of the server when you specify this flag is, in some sense, correct in that you will completely fail should you try to append. I suppose that may be confusing but at least you won't pollute any data. Asking that the server return "not supported" would only address XRootD-based implementation but not, for instance, dCache-based servers. I suppose, we could remove the flag from the client interface "hh" file but leave it defined in the private headers.

[murrayc3]

Hi Andy, the problem is SFS_O_RDONLY = 0. When a "open for write" flag such as kXR_open_apnd is sent to an XRootD server it is translated to SFS_O_RDONLY. Here at CERN we are marking files within our tape backed directories as "NOT updateable". This is an EOS specific ACL. Such files should be read-only once they have been created. An "open for write" on such files including the use of the kXR_open_apnd flag should fail either with a permissions error or with a "not supported error". In order for us to detect kXR_open_apnd it must be passed through to XrdSfsFile::open() in some way. Ignoring it and passing SFS_O_RDONLY is not correct. Removing the kXR_open_apnd flag all together is another solution.

[abh3]

Well, I think we have a philosophical difference. If we remove the flag definition from the client interface that should suffice, as it cannot be specified unless you subvert the system. Having the unsupported append converted to a read is certainly a safe option though you won't get what you wanted, then again, he server doesn't support append. Frankly, other that semantics here I don't see what the real problem is as you shouldn't be speciying append if it isn't supported and if you do it will back down to something that is prefectly safe.

…

On Thu, 30 Jan 2020, murrayc3 wrote: Hi Andy, the problem is SFS_O_RDONLY = 0. When a "open for write" flag such as kXR_open_apnd is sent to an XRootD server it is translated to SFS_O_RDONLY. Here at CERN we are marking files within our tape backed directories as "NOT updateable". This is an EOS specific ACL. Such files should be read-only once they have been created. An "open for write" on such files including the use of the kXR_open_apnd flag should fail either with a permissions error or with a "not supported error". In order for us to detect kXR_open_apnd it must be passed through to XrdSfsFile::open() in some way. Ignoring it and passing SFS_O_RDONLY is not correct. Removing the kXR_open_apnd flag all together is another solution. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #1126 (comment)

[murrayc3]

Maybe rephrasing the problem to "The XRootD server is stripping out the kXR_open_apnd flag sent by the client so that the XrdSfsFile::open() never gets to see it".

[murrayc3]

Looking at the following XRootD protocol code I can see that there is a protocol flag for each desired type of open including read:

[itctabuild02] xrdcl_tests (master) > vi /usr/include/xrootd/XProtocol/XProtocol.hh
...
enum XOpenRequestOption {
kXR_compress = 1, // also locate (return unique hosts)
kXR_delete = 2,
kXR_force = 4,
kXR_new = 8,
kXR_open_read= 16,
kXR_open_updt= 32,
kXR_async = 64,
kXR_refresh = 128, // also locate
kXR_mkpath = 256,
kXR_prefname = 256, // only locate
kXR_open_apnd= 512,
kXR_retstat = 1024,
kXR_replica = 2048,
kXR_posc = 4096,
kXR_nowait = 8192, // also locate
kXR_seqio =16384,
kXR_open_wrto=32768
};

In other words read on the wire is not represented by a 0 and from what I can tell 0 means nothing/none. Would you not be able to modify the XRootD server code to return an error to the client when there are leftover flags that its translation logic has not translated/consumed?

[abh3]

The protocol spec does not, in many isntance, require that a server implement the corresponding action and leaves it unspecified what the server does when an unsupported flag is set. As I said, these flags are defined so that should a server wish to support the feature that is the flag that must be used. I see tat we both have dug in our heels on this one. You say if it's defined it has to do something and I say f it's defined but not supported it should not do anythng eveil but it need not do anything useful either. The mistake here is exposing it in the client API which, arguably, would seem to imply it actually does something. I suppose we could make the server check but then we would need to do this in many other places as we have defined flags that are not (yet) supported. The current approach has been like this for some 20 years with no complaints. So, why all of a sudden is this a real issue?

…

On Thu, 30 Jan 2020, murrayc3 wrote: Looking at the following XRootD protocol code I can see that there is a protocol flag for each desired type of open including read: [itctabuild02] xrdcl_tests (master) > vi /usr/include/xrootd/XProtocol/XProtocol.hh ... enum XOpenRequestOption { kXR_compress = 1, // also locate (return unique hosts) kXR_delete = 2, kXR_force = 4, kXR_new = 8, kXR_open_read= 16, kXR_open_updt= 32, kXR_async = 64, kXR_refresh = 128, // also locate kXR_mkpath = 256, kXR_prefname = 256, // only locate kXR_open_apnd= 512, kXR_retstat = 1024, kXR_replica = 2048, kXR_posc = 4096, kXR_nowait = 8192, // also locate kXR_seqio =16384, kXR_open_wrto=32768 }; In other words read on the wire is not represented by a 0 and from what I can tell 0 means nothing/none. Would you not be able to modify the XRootD server code to return an error to the client when there are leftover flags that its translation logic has not translated/consumed? -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #1126 (comment)

[abh3]

The decision was to remove options that were not implemented.