[haiku-commits] haiku: hrev46820 - src/add-ons/kernel/file_systems/exfat
- From: John Scipione <jscipione@xxxxxxxxx>
- To: "haiku-commits@xxxxxxxxxxxxx" <haiku-commits@xxxxxxxxxxxxx>
- Date: Wed, 5 Feb 2014 05:29:22 -0500
On Wednesday, February 5, 2014, Stephan Aßmus <superstippi@xxxxxx> wrote:
>
> On 05.02.2014 02:49, jscipione@xxxxxxxxx wrote:
>
>> 762b846: exfat: Set the mountpoint to same as volume name
>>
>> Fixes #10501
>>
>> * update copyright headers, attribute to Haiku, Inc. add authors
>>
>
> Just a quick question, did you ask every affected person permission before
> doing this? Otherwise you can't just reattribute copyright, I think.
Hi Stephan,
There is no reattribution of copyright, I added my changes copyright Haiku,
Inc. the rest of the copyright statements are kept intact.
On Wednesday, February 5, 2014, Axel Dörfler <axeld@xxxxxxxxxxxxxxxx> wrote:
> > On February 5, 2014 at 2:49 AM jscipione@xxxxxxxxx wrote:
> > c79381d: exfat: limit backup volume name to 11 characters
>
> This is quite messy...
Hi Axel,
I guess I should have gotten some peer review on this one...
> +++ b/src/add-ons/kernel/file_systems/exfat/Volume.cpp
> > @@ -377,8 +377,8 @@ Volume::Mount(const char* deviceName, uint32 flags)
> > double size = double((10 * diskSize + divisor - 1) /
> divisor);
> > // %g in the kernel does not support precision...
> >
> > - snprintf(fName, sizeof(fName), "%g %cB ExFAT Volume",
> > - size / 10, unit);
> > + snprintf(fName, 11 + 1, "%g %cB ExFAT", size / 10, unit);
> > + // exfat volume names can only contain 11
> characters
>
> This is just wrong, please revert. fName is 34 bytes long, and snprintf()
> operates on bytes.
> Please just leave the sizeof(fName) in there; there is no reason to
> cripple the
> name to 11 bytes here.
I may need the result to fit back into 11 utf-16 characters to store it
back into the volume label. It's a limitation of exfat. Since this is ASCII
text that means 11 bytes here. 34 bytes is the widest posible if your name
contains all 3 byte UTF8 chars plus a \0.
> +++ b/src/add-ons/kernel/file_systems/exfat/Utility.cpp
> [...]
> > + * Axel Dörfler, axeld@xxxxxxxxxxxxxxxx
>
> I wasn't involved here AFAICT.
I copied your name from the header, I'll remove it from this file only.
Thanks
> +status_t
> > +volume_name(struct exfat_entry* entry, char* _name)
> > +{
> > + if (entry == NULL || _name == NULL)
> > + return B_BAD_VALUE;
> > +
> > + if (entry->type == EXFAT_ENTRY_TYPE_NOT_IN_USE) {
> > + const char* untitled = "Untitled";
>
> Isn't that where the size of the volume should be used instead? (ie. the
> code
> above?)
Maybe... You are allowed to have a labeless volume, it's up to us to decide
what to do in this case. The fallback name is only used for the volume
name, not the mount point currently but I suppose I could change that.
> > + size_t length = strlen(untitled);
> > + strncpy(_name, untitled, length);
>
> Why use strncpy()? It doesn't make any sense.
If you pass a buffer too small I could buffer overrun with strcpy here and
crash so I'm being a bit cautious.
> + if (strlen(_name) < length)
> > + return B_NAME_TOO_LONG;
>
> How should that happen?
Just being cautious checking to make sure the buffer you passed in
was large enough to fit "Untitled"
> + } else if (entry->type == EXFAT_ENTRY_TYPE_LABEL) {
> > + // UCS-2 can encode codepoints in the range U+0000 to
> U+FFFF
> > + // UTF-8 needs at most 3 bytes to encode values in this
> range
> > + size_t utf8NameLength = entry->label.length * 3;
>
> volume_name() should get the buffer length; the name that is returned is
> pretty
> much independent from exFAT (like "Untitled").
The way I wrote it you are responsible for providing a buffer that's large
enough or you'll get B_NAME_TOO_LONG. The calculation is for the conversion
from Unicode to UTF8. I suppose I could have you pass in a buffer size.
> Also, we don't prefix buffers to be filled, only variables that are
> overwritten.
> Ie. only for char** _name.
> Also, the name should be get_volume_name(), as it doesn't return the name,
> but a
> status.
Ok... I was not aware of either of these rules. Sorry. I thought the _
prefix was for any out parameter.
> > + memset(_name, 0, utf8NameLength + 1);
> > + // zero out the character array
>
> Why? This doesn't seem to happen at other invocations of unicode_to_utf8().
Perhaps this can be omitted, I'll have to doublecheck.
> +/*! Reads the volume name from an exfat entry and writes it to \a _name
> > + as a UTF-8 char array. Writes "Untitled" The volume name is not
> set.
>
> "The" -> "if the".
>
> > - fVolume->SetName(utfName);
> > + char name[34];
>
> Isn't there some constant you can use instead?
>
> > - char fName[32];
> > + char fName[34];
> > + // Max number of bytes needed is (11 * 3)
> + 1
> > for the \0,
> > + // that is 11 UCS-2 characters converted to
> > UTF-8.
>
> Same here. Well, just introduce one, if it's not there already.
Okay will do. No constant need to add one.
> struct identify_cookie {
> > exfat_super_block super_block;
> > + char name[34];
>
> And here as well.
>
> > memcpy(&cookie->super_block, &superBlock,
> sizeof(exfat_super_block));
> > + memset(cookie->name, 0, 34);
> > + // zero out volume name
>
> Ouch. Ever heard of sizeof()?
What's wrong with 34? That's the size, I know it is since I set it a few
lines up. With a magic constant this might read better but I think it's
valid to specify a constant size here.
> -exfat_scan_partition(int fd, partition_data *partition, void *_cookie)
> > +exfat_scan_partition(int fd, partition_data* _partition, void* _cookie)
>
> Again, do not prefix buffers or structures that are filled.
>
Are you sure about that? The only reason I changed it is because I was
confused about whether partition was suppose to be filled out already or
not. The underscore helps me to know that I am expected to fill it with
data here.
Other related posts:
