Re: [Sugar-devel] [PATCH 3/7] metadatastore: store/change files on disk defensively #2317
On Mon, Oct 1, 2012 at 12:59 PM, Martin Langhoff wrote: >> From that section I understand that we are encoding the data into >> utf-8 if value is "unicode", but if it is just a "basestring", why we >> are doing "str()"? > > That's actually not changed code.. only changes indentation. I don't > like it, I very seriously don't like it, but fixing the problem is > outside the scope of this patchset. Why do we do str(), I don't know. > I believe it's a noop in this case. Actually, I know why. AIUI, data types coming from dbus can be ints and floats. And that var is coming from dbus. So we cannot write directly to a file without a cast. See http://dbus.freedesktop.org/doc/dbus-python/doc/tutorial.html#data-types > The problem is that we are dealing with some raw binary data (ie: > thumbnails) as if it were a string. You can get away with that as long > as your string ops are 8-bit clean. Once we move to utf-8, they won't > be. That table also indicates, in the 'notes' column, that all string data must be valid utf8. I don't know how we are getting the thumbnails through, and it seems likely that at some point it'll break, perhaps due to changes in dbus. We must change the thumbnail passing to be in a file, not too far in the future. cheers, m -- mar...@laptop.org -- Software Architect - OLPC - ask interesting questions - don't get distracted with shiny stuff - working code first - http://wiki.laptop.org/go/User:Martinlanghoff ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [PATCH 3/7] metadatastore: store/change files on disk defensively #2317
On Mon, Oct 1, 2012 at 12:46 PM, Manuel Kaufmann wrote: > Sorry for the Top-Posting, maybe you didn't see the section that I > included in my last email. Ah, yeah, didn't catch that. > From that section I understand that we are encoding the data into > utf-8 if value is "unicode", but if it is just a "basestring", why we > are doing "str()"? That's actually not changed code.. only changes indentation. I don't like it, I very seriously don't like it, but fixing the problem is outside the scope of this patchset. Why do we do str(), I don't know. I believe it's a noop in this case. The problem is that we are dealing with some raw binary data (ie: thumbnails) as if it were a string. You can get away with that as long as your string ops are 8-bit clean. Once we move to utf-8, they won't be. > The FIXME comment refers to a problem we will have with Python 3 in > the future, right? In the future in general. Yes, we'll hit it with Python 3, perhaps even sooner. m -- mar...@laptop.org -- Software Architect - OLPC - ask interesting questions - don't get distracted with shiny stuff - working code first - http://wiki.laptop.org/go/User:Martinlanghoff ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [PATCH 3/7] metadatastore: store/change files on disk defensively #2317
On Sun, Sep 30, 2012 at 3:10 PM, Manuel Kaufmann wrote:
> The only thing that I do not understand well is this comment /
> section. Can you explain to me this a bit more? Thanks
>
>
> On Fri, Sep 21, 2012 at 12:55 AM, Martin Langhoff wrote:
>> +# FIXME: this codepath handles raw image data
>> +# str() is 8-bit clean right now, but
>> +# this won't last. We will need more explicit
>> +# handling of strings, int/floats vs raw data
>> +if isinstance(value, unicode):
>> +value = value.encode('utf-8')
>> +elif not isinstance(value, basestring):
>> +value = str(value)
Sorry for the Top-Posting, maybe you didn't see the section that I
included in my last email.
>From that section I understand that we are encoding the data into
utf-8 if value is "unicode", but if it is just a "basestring", why we
are doing "str()"?
The FIXME comment refers to a problem we will have with Python 3 in
the future, right?
--
Kaufmann Manuel
Blog: http://humitos.wordpress.com/
Porfolio: http://fotos.mkaufmann.com.ar/
PyAr: http://www.python.com.ar/
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [PATCH 3/7] metadatastore: store/change files on disk defensively #2317
On Sun, Sep 30, 2012 at 2:10 PM, Manuel Kaufmann wrote: > The only thing that I do not understand well is this comment / > section. Can you explain to me this a bit more? Thanks What is unclear? The only thing I could add is a pre-amble: Previous code deleted all metadata entries for a given DS entry and then recreated them, on every call to update(). This caused immediate loss of metadata in the case of ENOSPC or any other I/O problem. With this patch: - only delete metadata files for keys that are being removed - only write files when the data changes - write/replace metadata files atomically, to avoid corrupting existing data in case of an error With this patch, we no longer corrupt metadata when trying to edit/update a ds entry with the system hitting ENOSPC. Signed-off-by: Martin Langhoff -- now, if you read carefully, it is the same thing I say in the last paragraph :-) m -- mar...@laptop.org -- Software Architect - OLPC - ask interesting questions - don't get distracted with shiny stuff - working code first - http://wiki.laptop.org/go/User:Martinlanghoff ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [PATCH 3/7] metadatastore: store/change files on disk defensively #2317
Hi,
I reviewed this patch and it seems to be OK for me.
The only thing that I do not understand well is this comment /
section. Can you explain to me this a bit more? Thanks
On Fri, Sep 21, 2012 at 12:55 AM, Martin Langhoff wrote:
> +# FIXME: this codepath handles raw image data
> +# str() is 8-bit clean right now, but
> +# this won't last. We will need more explicit
> +# handling of strings, int/floats vs raw data
> +if isinstance(value, unicode):
> +value = value.encode('utf-8')
> +elif not isinstance(value, basestring):
> +value = str(value)
--
Kaufmann Manuel
Blog: http://humitos.wordpress.com/
Porfolio: http://fotos.mkaufmann.com.ar/
PyAr: http://www.python.com.ar/
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [PATCH 3/7] metadatastore: store/change files on disk defensively #2317
On Thu, Sep 20, 2012 at 11:55 PM, Martin Langhoff wrote:
> -f = open(os.path.join(metadata_path, key), 'w')
> -try:
> -if isinstance(value, unicode):
> -value = value.encode('utf-8')
> -elif not isinstance(value, basestring):
> -value = str(value)
> -f.write(value)
> -finally:
> -f.close()
BTW, I simplified the structure of
f = open(path, 'w')
try:
f.write(value)
finally:
f.close()
because in practice, f.write() writes into a buffer, so it never
fails. It is open() and close() that throw exceptions. In fact, at
ENOSPC, close() was failing.
And the use of finally is misguided. In my code, which wraps
open/write/close in a single try, if f.write() does throw an
exception, Python will close the FH when it goes out of scope.
Simpler, saner.
Python won't leak FHs on you. (IOWs, it takes some work to leak FHs on
any modern scripting language).
cheers,
m
--
martin.langh...@gmail.com
mar...@laptop.org -- Software Architect - OLPC
- ask interesting questions
- don't get distracted with shiny stuff - working code first
- http://wiki.laptop.org/go/User:Martinlanghoff
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel
[Sugar-devel] [PATCH 3/7] metadatastore: store/change files on disk defensively #2317
- only delete metadata files for keys that are being removed
- only write files when the data changes
- write/replace metadata files atomically, to avoid corrupting
existing data in case of an error
With this patch, we no longer corrupt metadata when trying
to edit/update a ds entry with the system hitting ENOSPC.
Signed-off-by: Martin Langhoff
---
src/carquinyol/metadatastore.py | 51 ++-
1 file changed, 34 insertions(+), 17 deletions(-)
diff --git a/src/carquinyol/metadatastore.py b/src/carquinyol/metadatastore.py
index 5967017..52cc10f 100644
--- a/src/carquinyol/metadatastore.py
+++ b/src/carquinyol/metadatastore.py
@@ -14,27 +14,46 @@ class MetadataStore(object):
if not os.path.exists(metadata_path):
os.makedirs(metadata_path)
else:
+received_keys = metadata.keys()
for key in os.listdir(metadata_path):
-if key not in _INTERNAL_KEYS:
+if key not in _INTERNAL_KEYS and key not in received_keys:
os.remove(os.path.join(metadata_path, key))
metadata['uid'] = uid
for key, value in metadata.items():
+self._set_property(uid, key, value, md_path=metadata_path)
-# Hack to support activities that still pass properties named as
+def _set_property(self, uid, key, value, md_path=False):
+if not md_path:
+md_path = layoutmanager.get_instance().get_metadata_path(uid)
+# Hack to support activities that still pass properties named as
# for example title:text.
-if ':' in key:
-key = key.split(':', 1)[0]
-
-f = open(os.path.join(metadata_path, key), 'w')
-try:
-if isinstance(value, unicode):
-value = value.encode('utf-8')
-elif not isinstance(value, basestring):
-value = str(value)
-f.write(value)
-finally:
-f.close()
+if ':' in key:
+key = key.split(':', 1)[0]
+
+changed = True
+fpath = os.path.join(md_path, key)
+tpath = os.path.join(md_path, '.' + key)
+# FIXME: this codepath handles raw image data
+# str() is 8-bit clean right now, but
+# this won't last. We will need more explicit
+# handling of strings, int/floats vs raw data
+if isinstance(value, unicode):
+value = value.encode('utf-8')
+elif not isinstance(value, basestring):
+value = str(value)
+
+# avoid pointless writes; replace atomically
+if os.path.exists(fpath):
+stored_val = open(fpath, 'r').read()
+
+if stored_val == value:
+changed = False
+if changed:
+f = open(tpath, 'w')
+f.write(value)
+f.close()
+os.rename(tpath, fpath)
def retrieve(self, uid, properties=None):
metadata_path = layoutmanager.get_instance().get_metadata_path(uid)
@@ -55,6 +74,4 @@ class MetadataStore(object):
return None
def set_property(self, uid, key, value):
-metadata_path = layoutmanager.get_instance().get_metadata_path(uid)
-property_path = os.path.join(metadata_path, key)
-open(property_path, 'w').write(value)
+self._set_property(uid, key, value)
--
1.7.10.4
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel
