linux-ntfs-dev — Development discussions and bug reports.

Re: [Linux-NTFS-Dev] how to fix bugs

[Andras E. <and...@in...>]

On Thu, 26 Feb 2004 13:13:01 +0000, Anton Altaparmakov <ai...@ca...> =
wrote:

>You will notice ...  We both agree.=20

sorry, was too hasty, misread it

>But
>alas, I wrote mkntfs before unistr.[hc] existed and never revisited
>mkntfs to fix it...

should i make that change?

br,
andras

[Linux-NTFS-Dev] Re: Prolbem with Data Runs - NTFS Win2K version

[Anton A. <ai...@ca...>]

On Thu, 2004-02-26 at 12:58, nag talekar wrote:
> i am doing a project on NTFS volume.

Cool. I am CC:-ing the ntfs list.

> Theres a problem.
> 
> In $INDEX_ALLOCATION (0xA0h) attribute of system32
> folder VCN range is mentioned as 0 to 45h
> 
> but when i count the no of clusters in  datarun
> entries it comes to around 117 (decimal).I think its
> ok as Windows can allocate extra clusters.
> 
> Problem arises when i access the Index records using
> data runs.
> 
> I mean first 5 data runs point to valid Index records.
> Then onwards dataruns point to certain clusters which
> do not contain valid Index records
> 
> I used the docs at the site
> 
> http://linux-ntfs.sourceforge.net/
> 
> could u please tell me how to locate dataruns that
> point to valid Index records??
> 
> Is there way to  map VCN to LCN values ??
> 
> even $BITMAP (0xB0) in that file reocrd for system32
> folder shows that first 46h Index entries (VCNs) are
> occupied 
> 
> ie 
> FF FF FF FF .... 3F (totals to 46h VCNs)
> 
> 
> i am not following B+ tree structure .I am just using
> dataruns to sequentially goto the Index records.
> 
> could please help me ??

Are you perhaps forgetting to apply the multi sector transfer fixups?

Also why do you not use the ntfs library for your accesses?

See http://linux-ntfs.sourceforge.net/downloads.html

Unpack it and look in include/*.h and libntfs/*.c for the library and
associated header files and in ntfsprogs/*.c for the utilities where you
can see how to call the library.

In the library you will see how to map VCNs to LCNs, too.

Have a look and feel free to come back with questions...

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ &
http://www-stu.christs.cam.ac.uk/~aia21/

Re: [Linux-NTFS-Dev] how to fix bugs

[Szakacsits S. <sz...@si...>]

On Thu, 26 Feb 2004, Anton Altaparmakov wrote:
> On Thu, 2004-02-26 at 11:21, Szakacsits Szabolcs wrote:
> > Adding NTFS 3.1 support would be also wonderful. There aren't many changes
> > compared to NTFS 1.2 and this could help _a lot_ to make (automatic)
> > testing and development easier. There wouldn't be need to get XP images,
> > we could create whenever we need.
> 
> Huh?  I never added NTFS 3.x support because any form of Windows 2k or
> later will just upgrade the volume on mount so it seems silly to have
> mkntfs more complicated.  

It's not silly. First of all I can not confirm you're right that all Win
versions, in all cases will upgrade from 1.2 to 3.x. Maybe you only tried
an NT4 -> W2K upgrade and in that case it make sense to upgrade NTFS but
not otherwise, as I've seen several times.

Secondly and most importantly, you _need_ Windows to do this. And that's
silly :-)

> Oh yes, the dest[i] at the bottom...  The 3rd solution is still not
> correct though.  

My point was the buffer overflow, not the technical detail how to handle
it. 

> > > >  * @param dst [out] points the buffer to receive the unicode string
> > > >  * @param src [in] points the char string to be converted
> > > >  * @param size [in] size of the @dst buffer in bytes
> > 
> > The [in|out]'s are _very_ important. I also always missed them.
> 
> Fair enough, we can add them but we can still please use this form:
> 
> @dst:	description [OUT]
> @src:	description2 [IN]
> @size: description3 [IN]

[IN|OUT] in the end is obfuscated and against the general conventions.
 
> Also is @dst really [OUT]?  To me it seems like @dst is [IN] but *@dst
> is [OUT]...

Modified directly, indirectly, struct members, etc. It's based on common
sense but maybe it's also written somewhere :)
 
> I hate this [IN] and [OUT] stuff but if you like them feel free to add
> them but _please_ at the end of the lines so I can just ignore them when
> reading...

That's very important who reads it. If only you then you're right. If
others also then their opinion might count as well if you're interested to
get some help ;)
 
	Szaka

Re: [Linux-NTFS-Dev] how to fix bugs

[Anton A. <ai...@ca...>]

On Thu, 2004-02-26 at 12:47, Andras Erdei wrote:
> is there any reason to list individuals beside the list address?
> should i do it, too?

No there isn't.  I just always click "Reply to All" because of work
emails where I cannot be sure that everyone in CC: is a member of the
relevant lists and often we use several different lists so you have to
reply to all of them...  Sorry if that means you get everything twice...

> On Thu, 26 Feb 2004 12:21:52 +0100 (MET), Szakacsits Szabolcs <sz...@si...> wrote:
> >> -       /* Need two bytes for null terminator. */
> >> -       maxlen -= 2;
> >> +       /* Convert maxlen from bytes to unicode characters. */
> >> +       maxlen /= sizeof(uchar_t);
> >> +       /* Need space for null terminator. */
> >> +       maxlen--;
> 
> >This is still wrong. If maxlen was <= 1 and src was bigger, then we have a
> >buffer overflow (currently it never should occure as Anton also pointed
> >out). Only Andras 3rd solution was correct.
> 
> the options:
> - an if()
> - an assert()
> - a comment
> 
> i assumed the if() was intentionally left out (efficiency :O),

No, it was forgotten.  )-:

> >Personally I prefer when by reading the code, it documents itself without
> >explicit /* */ or // comments. Real comments are for non-trivial things,
> >things that's thought but not in the code, function description, FIXME's,
> >etc.
> 
> i don't know what kind of audience are you targeting with this
> code; and you may be right, not using ((almost) trivial) comments 
> may actually be helpful as it raises the entry treshold and can
> prevent well-meaning newbies from poking around with the code and
> hurting themselves

Good question about the audience.  I think I try to make the code
readable enough so that if I read it a year later I can easily
understand what it does and can fix bugs / make additions / etc...  And
even though maybe I write too many comments I have never so far seen any
of the libntfs or ntfs kernel code (new driver) that I couldn't
understand immediately so I think I will keep this style for myself.  It
seems to work for me so why change it.  (-;

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ &
http://www-stu.christs.cam.ac.uk/~aia21/

Re: [Linux-NTFS-Dev] how to fix bugs

[Anton A. <ai...@ca...>]

On Thu, 2004-02-26 at 12:31, Andras Erdei wrote:
> On Thu, 26 Feb 2004 09:57:00 +0000, Anton Altaparmakov <ai...@ca...> wrote:
> >I committed this fix instead:
> >-       /* Need two bytes for null terminator. */
> >-       maxlen -= 2;
> >+       /* Convert maxlen from bytes to unicode characters. */
> >+       maxlen /= sizeof(uchar_t);
> 
> just nitpicking, but maybe getting rid of the magic number 2
> and using sizeof(uchar_t), or even more sizeof(*dest) at both 
> places is a bit better

Of course. You will notice that the above is a unified diff and the
lines prefaced by '-' characters are the lines removed!  So yes, the
magic 2 has now disappeared.  We both agree.  (-;  I prefer
sizeof(uchar_t) in this case because we are talking about a file system
and hence the types cannot change as they are fixed by the NTFS
specifications and on-disk structures.  And sizeof(uchar_t) is a lot
more obvious than sizeof(*dest) where one has to check in the function
declaration for what type dest is.

A few comments I have been meaning to make and forgetting:  Note that
the functions in mkntfs were written for mkntfs only.  For example
stoucs() is mkntfs only.  In fact mkntfs ought to use the functions
provided by libntfs (unistr.[hc]::ntfs_mbstoucs() and friends.  But
alas, I wrote mkntfs before unistr.[hc] existed and never revisited
mkntfs to fix it...

The general goal is that any useful functionality from all the utilities
should one day be moved in a generalized form to the library itself and
the utilities modified to use the library instead of having their own
versions.  That will make the utilities smaller and even more
importantly, hopefully one day mkntfs() itself will just be a function
provided by libntfs and mkntfs.c would be just a wrapper calling the
library mkntfs() function (plus option parsing and error handling).

> >I don't like assert() very much because it is too easy to create
> >Heisenbugs by using things in inside the assert() that have side
> >effects...
> 
> bugs are easy to create with almost every language construct :O)

True.  But Szaka's point not to use assert() in a library is still quite
a valid one...  A library should _never_ abort, it should always return
an error code instead.

> ok, no asserts will be used

Thanks.  (-:

> do you feel that an if() is needed?
> or the comment added in my version of the fn description is enough?

For library functions() and kernel functions and any functions that
parse external data (from user input, another program, from the disk
partition, etc) you cannot rely on comments.  You need heavy sanity
checking otherwise the software will either crash or worse corrupt data
or even create security holes in the kernel or SUID applications so I
tend to try and catch all input parameter errors and return error with
errno set to EINVAL (or in kernel return -EINVAL;).

> >Please stick with DocBook style for parameters
> 
> can you give me a pointer to some DocBook intro ?

Um no, but since I use the same style as the kernel so here is the
except from the kernel docs:

The format of the block comment is like this:

/**
 * function_name(:)? (- short description)?
(* @parameterx: (description of parameter x)?)*
(* a blank line)?
 * (Description:)? (Description of function)?
 * (section header: (section description)? )*
(*)?*/

The short function description cannot be multiline, but the other
descriptions can be (and they can contain blank lines). Avoid putting a
spurious blank line after the function name, or else the description
will be repeated!

All descriptive text is further processed, scanning for the following
special patterns, which are highlighted appropriately.

'funcname()' - function
'$ENVVAR' - environment variable
'&struct_name' - name of a structure (up to two words including
'struct')
'@parameter' - name of a parameter
'%CONST' - name of a constant.

And I prefer:

/**
 * function_name - short description
(* @parameterx:	description of parameter x)*
 * a blank line
 * Description of function.
 * (section header: (section description)? )*
 */

> >Another comment about your style is can we please have C style comments:
> >	/* comment */
> >Instead of C++ style:
> >	//
> >
> >I only use C++ style for TODO or FIXME comments with the intention of
> >them going away when the TODO or FIXME has been done...
> 
> i've only seen that // was used extensively, but you are right,
> only for this specific purpose; if it's important i'll try to
> get back into the habit of using /* */ instead

That would be good as kernel hackers scream on sight of C++ comments and
as Szaka said code can be copied from the library to the kernel driver
so it would be nice if one didn't have to change all the comments in the
process...

> this is the only point where i feel i have to make a stand
> -- but if you'll insist, i'll do my best to optimize :O)
> 
> - i'd prefer maintainability over efficiency; if can be
>   understood, then it can be changed (fixed!), so it can 
>   be optimized; if it's uncomprehensible, it cannnot be 
>   fixed, updated or optimized, in the rare cases where 
>   that's needed, and imho this (or any other i/o-centric code) 
>   isn't that case

I would agree with this.  I would prefer readable code over optimized
code everywhere, except for performance critical functions.  An example
where performance is important is libntfs/attrib.c::ntfs_attr_lookup()
and friends which are called all the time before anything at all can
happen so if they are slow the whole of ntfs would be slowed down. 
Whether it actually makes any noticeable speed difference when compared
to the slowdowns imposed by needing to read something from disk I don't
know but in an ideal world intelligent read-ahead in the kernel will
have read all the data already so that we only deal with memory
accesses... (-;

> - today it's extremely hard to tell which code is faster,
>   even with asm code (consider pipelines, cache etc); with C 
>   basically the only way to tell is trial and error (or a lot 
>   of experience); for example your original (fixed) stoucs() 
>   and the last version generate the same code:

That is very true.  Nowadays noone knows what compilers get up to and
what different cpus get up to so the same code could run at different
speeds on different cpus, etc...

> should i still try to optimize?

You don't have to.  I just like to do it.  To my personal way of
thinking having to re-evaluate things repeatedly is bad epsecially in a
multithreaded / kernel environment as you are potentially using shared
variables which can be changed under you.  But in libntfs that is hardly
important but nevertheless IMHO it is cleaner to not re-evaluate.  But
the bottom line is that as long as code not performance critical and it
is readable I don't mind if you optimize or not.

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ &
http://www-stu.christs.cam.ac.uk/~aia21/

Re: [Linux-NTFS-Dev] how to fix bugs

[Andras E. <and...@in...>]

is there any reason to list individuals beside the list address?
should i do it, too?

On Thu, 26 Feb 2004 12:21:52 +0100 (MET), Szakacsits Szabolcs =
<sz...@si...> wrote:

>> -       /* Need two bytes for null terminator. */
>> -       maxlen -=3D 2;
>> +       /* Convert maxlen from bytes to unicode characters. */
>> +       maxlen /=3D sizeof(uchar_t);
>> +       /* Need space for null terminator. */
>> +       maxlen--;

>This is still wrong. If maxlen was <=3D 1 and src was bigger, then we =
have a
>buffer overflow (currently it never should occure as Anton also pointed
>out). Only Andras 3rd solution was correct.

the options:
- an if()
- an assert()
- a comment

i assumed the if() was intentionally left out (efficiency :O),
so i tried the other two approaches; as you don't seem to like
asserts(), do you think the comment

 * @par constraint: @dst must be able to hold at least the
 * terminating null [@size >=3D sizeof( uchar_t )]

was enough ?

>Personally I prefer when by reading the code, it documents itself =
without
>explicit /* */ or // comments. Real comments are for non-trivial things,
>things that's thought but not in the code, function description, =
FIXME's,
>etc.

i don't know what kind of audience are you targeting with this
code; and you may be right, not using ((almost) trivial) comments=20
may actually be helpful as it raises the entry treshold and can
prevent well-meaning newbies from poking around with the code and
hurting themselves

>Please note, the coding style is the
>
>	ntfsprogs/doc/CodingStyle
>	kernel/Documentation/CodingStyle

i've checked out the latter (it was referenced in the readme
or somewhere), now i'll read the former, too :O)

>Other reasons are=20
>
>	- if the code moved to the library then it would be pretty rude
>          to abort instead giving the chance for the caller to clean up

but then it would be pretty rude to leave it as it is (without the
assert) and let it abort with a involuntary core dump instead :O)

>	- important info on the failure context can get lost (e.g. to=20
>          debug user problems)

assert() is not for production code (users) -- they'll just get
the core dump :O)

but ok, ok, got the point, no assert()s, it's fine by me


br,
andras

[Linux-NTFS-Dev] more (1st style, minimal) patches for mkntfs.c (still untested!)

[Andras E. <and...@in...>]

mkntfs.c:
@@ -402,8 +402,6 @@
                if (!new_buf)
                        err_exit("Reallocating memory for bad blocks list=
 "
                                 "failed: %s\n", strerror(errno));
-               if (opts.bad_blocks !=3D new_buf)
-                       free(opts.bad_blocks);
                opts.bad_blocks =3D new_buf;
        }
        opts.bad_blocks[opts.nr_bad_blocks++] =3D block;

the original code free()s up a block on the heap that was
already free()-d by realloc()

mkntfs.c:
@@ -640,50 +640,44 @@
              }
              j =3D 0;
                 if (i & VOLUME_MODIFIED_BY_CHKDSK) {
-                  j =3D 1;
                         printf("VOLUME_MODIFIED_BY_CHKDSK");
+                  j =3D 1;
                 }
              if (i & VOLUME_REPAIR_OBJECT_ID) {
                     if (j)
                                 printf(" | ");
-                        else
-                          j =3D 0;
                         printf("VOLUME_REPAIR_OBJECT_ID");
+                    j =3D 1;
                 }
              if (i & VOLUME_DELETE_USN_UNDERWAY) {
                  if (j)
                                 printf(" | ");
-                        else
-                          j =3D 0;
                         printf("VOLUME_DELETE_USN_UNDERWAY");
+                 j =3D 1;
                 }
              if (i & VOLUME_MOUNTED_ON_NT4) {
                       if (j)
                                 printf(" | ");
-                        else
-                          j =3D 0;
                         printf("VOLUME_MOUNTED_ON_NT4");
+                      j =3D 1;
                 }
              if (i & VOLUME_UPGRADE_ON_MOUNT) {
                     if (j)
                                 printf(" | ");
-                        else
-                          j =3D 0;
                         printf("VOLUME_UPGRADE_ON_MOUNT");
+                    j =3D 1;
                 }
              if (i & VOLUME_RESIZE_LOG_FILE) {
                      if (j)
                                 printf(" | ");
-                        else
-                          j =3D 0;
                         printf("VOLUME_RESIZE_LOG_FILE");
+                     j =3D 1;
                 }
              if (i & VOLUME_IS_DIRTY) {
                     if (j)
                                 printf(" | ");
-                        else
-                          j =3D 0;
                         printf("VOLUME_IS_DIRTY");
+                    j =3D 1;
                 }
              printf("\n");
          return;

i assume the intention is to output "|"s between the flags
listed, but the original code will insert it only between
the first two flags

br,
andras

Re: [Linux-NTFS-Dev] how to fix bugs

[Andras E. <and...@in...>]

On Thu, 26 Feb 2004 09:57:00 +0000, Anton Altaparmakov <ai...@ca...> =
wrote:

>I committed this fix instead:
>-       /* Need two bytes for null terminator. */
>-       maxlen -=3D 2;
>+       /* Convert maxlen from bytes to unicode characters. */
>+       maxlen /=3D sizeof(uchar_t);

just nitpicking, but maybe getting rid of the magic number 2
and using sizeof(uchar_t), or even more sizeof(*dest) at both=20
places is a bit better

>why not uni_size =3D size / sizeof(uchar_t) - 1; ?
>then you don't need the -1 in the test

for some reason i thought it'll make easier for the casual=20
reader to detect that the - 1 is meant to reserve the
space for the terminator, but most likely you are right,
and it doesn't help; and if there is a comment, it doesn't
matter anyway

>I don't like assert() very much because it is too easy to create
>Heisenbugs by using things in inside the assert() that have side
>effects...

bugs are easy to create with almost every language construct :O)

ok, no asserts will be used
do you feel that an if() is needed?
or the comment added in my version of the fn description is enough?

>Please stick with DocBook style for parameters

can you give me a pointer to some DocBook intro ?

>Another comment about your style is can we please have C style comments:
>	/* comment */
>Instead of C++ style:
>	//
>
>I only use C++ style for TODO or FIXME comments with the intention of
>them going away when the TODO or FIXME has been done...

i've only seen that // was used extensively, but you are right,
only for this specific purpose; if it's important i'll try to
get back into the habit of using /* */ instead

>>   // while there's both input and output left
>>   while ( *src && --uni_size )
>>       *dst++ =3D cpu_to_le16( *src++ ) ; // ascii -> unicode
>
>Again, I don't trust gcc to do the right thing.  (-:  I prefer to cache
>*src in a temporary variable for reuse.  I guess gcc would do the right
>thing as you made src const...
>
>You might notice I (perhaps unfairly) don't trust gcc to do anything
>well so I tend to write what I consider optimized code instead of hoping
>that gcc would optimize it...  I know the performance of stoucs() in
>mkntfs isn't important at all but hey.  (-8
<also some previous comments on efficiency were deleted>

this is the only point where i feel i have to make a stand
-- but if you'll insist, i'll do my best to optimize :O)

- i'd prefer maintainability over efficiency; if can be
  understood, then it can be changed (fixed!), so it can=20
  be optimized; if it's uncomprehensible, it cannnot be=20
  fixed, updated or optimized, in the rare cases where=20
  that's needed, and imho this (or any other i/o-centric code)=20
  isn't that case

- today it's extremely hard to tell which code is faster,
  even with asm code (consider pipelines, cache etc); with C=20
  basically the only way to tell is trial and error (or a lot=20
  of experience); for example your original (fixed) stoucs()=20
  and the last version generate the same code:

gcc -O2 -Wall -fms-extensions -c -S o.c

; original
.L7:
* movzbl  (%ebx,%edi), %eax   ; al: c =3D src[i]
* testb %al, %al              ; !c ?
* je  .L3                     ; yes -> exit
* movsbl  %al,%eax            ; call cpu_to_le16( c )
* movl  %eax, (%esp)
* call  cpu_to_le16
  movl  8(%ebp), %edx         ; edx: dst
  movw  %ax, (%edx,%ebx,2)    ; dst[i] =3D result
* incl  %ebx                  ; ++i
  cmpl  %esi, %ebx            ; i < maxlen ?
  jl  .L7                     ; smaller -> repeat

; c-ish fix
.L14:
* movsbl  %al,%eax      ; eax =3D *src
* incl  %ebx            ; ++src
* movl  %eax, (%esp)    ; call cpu_to_le16( eax )
* call  cpu_to_le16
  movw  %ax, (%edi)     ; *dst =3D result
  addl  $2, %edi        ; ++dst
* movzbl  (%ebx), %eax  ; al =3D *src
* testb %al, %al        ; 0 =3D=3D *src ?
* je  .L11              ; yes -> exit
  decl  %esi            ; --uni_size
  jne .L14              ; nonzero -> repeat

  the only difference is the ordering of the instructions
  (* marks instructions that match in the two blocks), and=20
  three-three 1-cycle statements

  actually, on a 8086 the c-ish version would have been faster
  (all variables are stored in registers, and simpler addressing
  methods) than your optimized code, but on a pentium it's the same

should i still try to optimize?

br,
andras

"We should forget about small efficiencies, say about 97% of the time:=20
premature optimization is the root of all evil."
			Knuth (although some say it was originally Hoare)

Re: [Linux-NTFS-Dev] how to fix bugs

[Anton A. <ai...@ca...>]

On Thu, 2004-02-26 at 11:21, Szakacsits Szabolcs wrote:
> Adding NTFS 3.1 support would be also wonderful. There aren't many changes
> compared to NTFS 1.2 and this could help _a lot_ to make (automatic)
> testing and development easier. There wouldn't be need to get XP images,
> we could create whenever we need.

Huh?  I never added NTFS 3.x support because any form of Windows 2k or
later will just upgrade the volume on mount so it seems silly to have
mkntfs more complicated.  Also, even Windows actually creates NTFS 1
partitions which are then upgraded on mount...  (At least this used to
be the case for older windows no idea about the latest incarnations.)

> > so I committed this fix instead:
> > 
> > diff -Nru a/ntfsprogs/mkntfs.c b/ntfsprogs/mkntfs.c
> > --- a/ntfsprogs/mkntfs.c        Thu Feb 26 09:19:33 2004
> > +++ b/ntfsprogs/mkntfs.c        Thu Feb 26 09:19:33 2004
> > @@ -567,8 +567,10 @@
> >         char c;
> >         int i;
> >   
> > -       /* Need two bytes for null terminator. */
> > -       maxlen -= 2;
> > +       /* Convert maxlen from bytes to unicode characters. */
> > +       maxlen /= sizeof(uchar_t);
> > +       /* Need space for null terminator. */
> > +       maxlen--;
> >         for (i = 0; i < maxlen; i++) {
> 
> This is still wrong. If maxlen was <= 1 and src was bigger, then we have a
> buffer overflow (currently it never should occure as Anton also pointed
> out). Only Andras 3rd solution was correct.

Oh yes, the dest[i] at the bottom...  The 3rd solution is still not
correct though.  Have committed a fix to my fix:

if (maxlen < sizeof(uchar_t)) {
	errno = EINVAL;
	return -1;
}

and added the following to the function comment:

If @maxlen is less than the size of a single unicode character we cannot
write the terminating null unicode character and hence return -1 with
errno set to EINVAL.

Note that returning zero is wrong in that case because no terminating
NULL has been written.

> Please note, the coding style is the
> 
> 	ntfsprogs/doc/CodingStyle
> 	kernel/Documentation/CodingStyle

Oh yes, I forgot to point this out!  Thanks!

> Code can easily end up in the kernel in the future. The most often used

Indeed.

> > >  * @param dst [out] points the buffer to receive the unicode string
> > >  * @param src [in] points the char string to be converted
> > >  * @param size [in] size of the @dst buffer in bytes
> 
> The [in|out]'s are _very_ important. I also always missed them.

Fair enough, we can add them but we can still please use this form:

@dst:	description [OUT]
@src:	description2 [IN]
@size:	description3 [IN]

Also is @dst really [OUT]?  To me it seems like @dst is [IN] but *@dst
is [OUT]...

I hate this [IN] and [OUT] stuff but if you like them feel free to add
them but _please_ at the end of the lines so I can just ignore them when
reading...

> > In case you didn't know we keep the latest sources in BitKeeper on
> > linux-ntfs.bkbits.net.  
> 
> Sometimes there are source snapshots on
> 
> 	http://linux-ntfs.sourceforge.net/snapshots/
> 
> ntfsprogs-<date>       is the current ntfsprogs 1.8.x serial
> ntfsprogs-devel-<date> is the current ntfsprogs 1.9.x serial

Once again, I forgot about these.  Thanks!  I have also uploaded the
most recent code snapshots just now.

> > A small howto. Set parent to:  bk://linux-ntfs.bkbits.net/ntfsprogs
> > (or ntfsprogs-devel obviously), i.e. just "bk clone -q
> > bk://linux-ntfs.bbkits.net/ntfsprogs" which creates ntfsprogs directory
> > in your current working directory.  Preferably work on ntfsprogs not
> > ntfsprogs-devel, 
> 
> after "bk clone ..." you must do a "bk get" and/or "bk edit" otherwise you
> can't version control, edit the files.

You just need to do "bk -r edit" anywhere inside ntfsprogs or
ntfsprogs-devel repositories and all of your files in that repository
will be checked out and can be written to.  This was supposed to happen
automatically but it stopped working for some reason...  )-:  You also
need to do this after each "bk citool" or "bk ci" followed by "bk
commit".

> BitKeeper has short intros, guides at http://www.bitkeeper.com/
> 
> Of course you're not required/forced to use BitKeeper. E.g. I use my CVS
> and BitKeeper only when needed.

Yes of course.  Feel free to just use the snapshots and email normal GNU
diff -urNp patches.

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ &
http://www-stu.christs.cam.ac.uk/~aia21/

Re: [Linux-NTFS-Dev] how to fix bugs

[Szakacsits S. <sz...@si...>]

On Thu, 26 Feb 2004, Anton Altaparmakov wrote:
> On Wed, 2004-02-25 at 15:55, Andras Erdei wrote:
> > [sorry for the long post]

Long fixes or any help are always very well welcomed :-> 
 
> > i'm thinking about contributing some effort to your project;
> > a good first step can be getting familiar with already existing 
> > code, thus i picked mkntfs.c and started reading it

That's a very good idea! :) mkntfs is a very good start to understand
NTFS. Improving readability would be highly beneficial for potential
newcomers also -- and here I don't mean comments but code.

Adding NTFS 3.1 support would be also wonderful. There aren't many changes
compared to NTFS 1.2 and this could help _a lot_ to make (automatic)
testing and development easier. There wouldn't be need to get XP images,
we could create whenever we need.

> so I committed this fix instead:
> 
> diff -Nru a/ntfsprogs/mkntfs.c b/ntfsprogs/mkntfs.c
> --- a/ntfsprogs/mkntfs.c        Thu Feb 26 09:19:33 2004
> +++ b/ntfsprogs/mkntfs.c        Thu Feb 26 09:19:33 2004
> @@ -567,8 +567,10 @@
>         char c;
>         int i;
>   
> -       /* Need two bytes for null terminator. */
> -       maxlen -= 2;
> +       /* Convert maxlen from bytes to unicode characters. */
> +       maxlen /= sizeof(uchar_t);
> +       /* Need space for null terminator. */
> +       maxlen--;
>         for (i = 0; i < maxlen; i++) {

This is still wrong. If maxlen was <= 1 and src was bigger, then we have a
buffer overflow (currently it never should occure as Anton also pointed
out). Only Andras 3rd solution was correct.

> Note I didn't write maxlen = maxlen / 2 - 1; as both forms generate the
> same compiler output (gcc 3.3.1 from SuSE 9.0 Pro) but merging the
> comments makes the comment go multi line which I thought was uglier. 

Personally I prefer when by reading the code, it documents itself without
explicit /* */ or // comments. Real comments are for non-trivial things,
things that's thought but not in the code, function description, FIXME's,
etc.

> > int stoucs( uchar_t * dst, char const * src, int size )

Please note, the coding style is the

	ntfsprogs/doc/CodingStyle
	kernel/Documentation/CodingStyle

Code can easily end up in the kernel in the future. The most often used
way is this (consider spacing and 'const char' order):

	int stoucs(uchar_t *dst, const char *src, int size)

The exception usually is if one writes his own module/subsystem alone, he
maintains it alone and stands the permanent flames why his coding style is
different :-)

> > unsigned stoucs( uchar_t * dst, char const * src, unsigned const size )
> > {
> >   char const * const begin = src ;
> >   unsigned uni_size = size / sizeof( *dst ) ;
> > 
> >   assert( uni_size ) ;
> 
> I don't like assert() very much because it is too easy to create
> Heisenbugs by using things in inside the assert() that have side
> effects...

Other reasons are 

	- if the code moved to the library then it would be pretty rude
          to abort instead giving the chance for the caller to clean up

	- important info on the failure context can get lost (e.g. to 
          debug user problems)

Of course I don't mean this for this trivial case (here it's perfectly
useful and actually fixes the buffer overflow) but in general in
production code.
 
> >  * @param dst [out] points the buffer to receive the unicode string
> >  * @param src [in] points the char string to be converted
> >  * @param size [in] size of the @dst buffer in bytes

The [in|out]'s are _very_ important. I also always missed them.

> In case you didn't know we keep the latest sources in BitKeeper on
> linux-ntfs.bkbits.net.  

Sometimes there are source snapshots on

	http://linux-ntfs.sourceforge.net/snapshots/

ntfsprogs-<date>       is the current ntfsprogs 1.8.x serial
ntfsprogs-devel-<date> is the current ntfsprogs 1.9.x serial

> A small howto. Set parent to:  bk://linux-ntfs.bkbits.net/ntfsprogs
> (or ntfsprogs-devel obviously), i.e. just "bk clone -q
> bk://linux-ntfs.bbkits.net/ntfsprogs" which creates ntfsprogs directory
> in your current working directory.  Preferably work on ntfsprogs not
> ntfsprogs-devel, 

after "bk clone ..." you must do a "bk get" and/or "bk edit" otherwise you
can't version control, edit the files.

BitKeeper has short intros, guides at http://www.bitkeeper.com/

Of course you're not required/forced to use BitKeeper. E.g. I use my CVS
and BitKeeper only when needed.

> And after all that, welcome to the Linux-NTFS project! 

Welcome :))

	Szaka

Re: [Linux-NTFS-Dev] how to fix bugs

[Anton A. <ai...@ca...>]

Hi,

On Wed, 2004-02-25 at 15:55, Andras Erdei wrote:
> [sorry for the long post]

No problem.

> i'm thinking about contributing some effort to your project;
> a good first step can be getting familiar with already existing 
> code, thus i picked mkntfs.c and started reading it

Great!

> there seem to be a few bugs in the code, so while reading it
> i might as well try to fix them, but i don't know what kind

Good idea.  (-:

> of fixes would you prefer, so i selected a trivial function
> with a trivial bug, and written three fixes -- please let me
> know which one would you prefer, and flame me a bit, so that
> my future fixes can get through more easily (if you want them
> at all :O)

Ok, see below...

> the function in question is stoucs(); it gets the size of
> the output buffer in bytes (as maxlen), but in fact it can
> write (maxlen-2)*2+2 bytes into it (e.g. into an output
> buffer of 10 bytes it writes up to 18 bytes):

Eeek!  You are quite right.  It is somewhat broken...  Luckily maxlen is
always made big enough by the callers of stoucs() which allocate dest =
malloc((strlen(src) + 1) * 2);  So we have never seen a problem...

> /**
>  * stoucs - convert ASCII string to unicode-character string
>  * @dest: points to buffer to receive the converted string
>  * @src:  points to string to convert
>  * @maxlen: size of @dest buffer in bytes
>  *
>  * Return the number of characters written to @dest, not including the
>  * terminating null unicode character.
>  */
> int stoucs(uchar_t *dest, const char *src, int maxlen)
> {
>   char c;
>   int i;
> 
>   /* Need two bytes for null terminator. */
>   maxlen -= 2;
>   for (i = 0; i < maxlen; i++) {
>     c = src[i];
>     if (!c)
>       break;
>     dest[i] = cpu_to_le16(c);
>   }
>   dest[i] = cpu_to_le16('\0');
>   return i;
> }
> 
> the first fix is the minimalistic one:
> 
> --- mkntfs.c.orig       2004-02-25 15:29:28.000000000 +0100
> +++ mkntfs.c    2004-02-25 16:23:38.000000000 +0100
> @@ -569,7 +569,7 @@
> 
>         /* Need two bytes for null terminator. */
>         maxlen -= 2;
> -       for (i = 0; i < maxlen; i++) {
> +  for (i = 0; i < maxlen/2; i++) {
>                 c = src[i];
>                 if (!c)
>                         break;

Yes that would be my preferred fix, except that I wouldn't trust gcc to
optimize away the maxlen/2.  I assume it would just re-evaluate it on
every i < maxlen/2 test which is silly so I committed this fix instead:

diff -Nru a/ntfsprogs/mkntfs.c b/ntfsprogs/mkntfs.c
--- a/ntfsprogs/mkntfs.c        Thu Feb 26 09:19:33 2004
+++ b/ntfsprogs/mkntfs.c        Thu Feb 26 09:19:33 2004
@@ -567,8 +567,10 @@
        char c;
        int i;
  
-       /* Need two bytes for null terminator. */
-       maxlen -= 2;
+       /* Convert maxlen from bytes to unicode characters. */
+       maxlen /= sizeof(uchar_t);
+       /* Need space for null terminator. */
+       maxlen--;
        for (i = 0; i < maxlen; i++) {
                c = src[i];
                if (!c)

Note I didn't write maxlen = maxlen / 2 - 1; as both forms generate the
same compiler output (gcc 3.3.1 from SuSE 9.0 Pro) but merging the
comments makes the comment go multi line which I thought was uglier. 
(-:

> the second fix preserves the original method, with a
> little cleanup:
> 
> int stoucs( uchar_t * dst, char const * src, int size )
> {
>   int const uni_size = size / sizeof( *dst ) ;

why not uni_size = size / sizeof(uchar_t) - 1; ?

>   int cp ;
> 
>   for ( cp = 0 ; src[ cp ] && cp < uni_size - 1 ; ++cp )

then you don't need the -1 in the test although I guess gcc ought to be
able to optimize this away as you declared uni_size as const so it
shouldn't make much difference.

>       dst[ cp ] = cpu_to_le16( src[ cp ] ) ;
> 
>   dst[ cp ] = cpu_to_le16( '0' ) ;
> 
>   return cp ;
> }
> 
> the third fix is an imho more c-ish complete rewrite
> (+ assert + comments):
> 
> #include <assert.h>
> 
> unsigned stoucs( uchar_t * dst, char const * src, unsigned const size )
> {
>   char const * const begin = src ;
>   unsigned uni_size = size / sizeof( *dst ) ;
> 
>   assert( uni_size ) ;

I don't like assert() very much because it is too easy to create
Heisenbugs by using things in inside the assert() that have side
effects...

>   // while there's both input and output left
>   while ( *src && --uni_size )
>       *dst++ = cpu_to_le16( *src++ ) ; // ascii -> unicode

Again, I don't trust gcc to do the right thing.  (-:  I prefer to cache
*src in a temporary variable for reuse.  I guess gcc would do the right
thing as you made src const...

You might notice I (perhaps unfairly) don't trust gcc to do anything
well so I tend to write what I consider optimized code instead of hoping
that gcc would optimize it...  I know the performance of stoucs() in
mkntfs isn't important at all but hey.  (-8

> /**
>  * stoucs - convert ASCIIZ string to unicode string
>  *
>  * @param dst [out] points the buffer to receive the unicode string
>  * @param src [in] points the char string to be converted
>  * @param size [in] size of the @dst buffer in bytes

Please stick with DocBook style for parameters, i.e.

@paramname:	description
@paramname2:	description2
etc...

If nothing else I find the style we currently use a hell of a lot more
readable than what you used...  My eyes are automatically drawn to the @
character and I want to then find the parameter name there rather than
having to scan to the next work.  Also having the descriptions aligned
looks neater and again helps readability IMHO.

Another comment about your style is can we please have C style comments:
	/* comment */
Instead of C++ style:
	//

I only use C++ style for TODO or FIXME comments with the intention of
them going away when the TODO or FIXME has been done...

In case you didn't know we keep the latest sources in BitKeeper on
linux-ntfs.bkbits.net.  Here is what I wrote (somewhat modified) in a
previous email on how to use BK with ntfsprogs.  You might find it
useful...

A small howto. Set parent to:  bk://linux-ntfs.bkbits.net/ntfsprogs
(or ntfsprogs-devel obviously), i.e. just "bk clone -q
bk://linux-ntfs.bbkits.net/ntfsprogs" which creates ntfsprogs directory
in your current working directory.  Preferably work on ntfsprogs not
ntfsprogs-devel, then "bk citool" (need X running!) then, generate patch
with bk export -tpatch -r1.865 (1.865 is the changeset number you can
get that by looking typing "bk changes". If  there is only one changeset
commit you can use -r+ instead of the number).  And email the patch to
lin...@li....  To get updated code you just go
into your ntfsprogs/ntfsprogs-devel repositories and type "bk pull".  To
look at changes use "bk revtool" and "bk csettool" (csettool is invoked
automatically bu revtool when needed) and of course "bk helptool" is the
generic BK help system...

And after all that, welcome to the Linux-NTFS project!  I hope I haven't
flamed you too much.  (-:

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ &
http://www-stu.christs.cam.ac.uk/~aia21/

Re: [Linux-NTFS-Dev] Re: Efficient use of ntfsclone unreliable?

[Per O. <pe...@ds...>]

On Wed, Feb 25, 2004 at 21:09 +0100, Szakacsits Szabolcs wrote:
> 
> On Wed, 25 Feb 2004, Per Olofsson wrote:
> 
> > I see. The reason I never sent anything to you is that my utilities
> > are just a hack 
> 
> That's ok :)
> 
> > and don't really work as they are now. 
> 
> That's not so ok :)

If you're interested, you can get the source from
<http://dsv.su.se/~pelle/ntfsimage/>. It worked when I tried it on a
filesystem that I had created with mkntfs, but it didn't work on a
real Windows partition. I don't know if it actually compiles with the
latest libntfs either. The format of the sparse images are documented
in comments in the source code. The biggest problem with this code is
that it doesn't do enough error checking.

> I think that was also what I suggested. ntfsimage and ntfsrestore? Or just
> ntfsimage?

The point of not calling the restoration utility ntfsrestore is that
it isn't really specific to NTFS - it could work with any
filesystem. Only the image creator (ntfsimage) is specific to NTFS.

> I'm also quite sure when we would be ready Ian will also arrive saying
> "here is my ntfsimage I promised a year ago!" :)

Surely :)

> Merging the two using the name ntfsimage would solve my "ntfsclone syntax
> problem" also :)

Yes :)

I'll have to look more at the ntfsclone source code and see if I can
work out a patch, but I can't promise anything. My ntfsimage utility
also has the feature of skipping the page file and the bad clusters
file, which perhaps also could be added to ntfsclone as a command line
option.

-- 
Pelle

Re: [Linux-NTFS-Dev] Re: Efficient use of ntfsclone unreliable?

[Szakacsits S. <sz...@si...>]

On Wed, 25 Feb 2004, Per Olofsson wrote:

> I see. The reason I never sent anything to you is that my utilities
> are just a hack 

That's ok :)

> and don't really work as they are now. 

That's not so ok :)

> Are ntfsimage and restoreimage available somewhere?

No, AFAIK. The only ntfsimage reference google finds is here:

http://packages.debian.org/changelogs/pool/main/c/chiark-utils/chiark-utils_4.0.0/changelog

> ntfsclone and ntfsimage do essentially the same thing, they skip over
> unused blocks. The output format is merely a detail and could be
> chosen with a command-line option. I think it would be better if they
> were one utility, in order to reduce duplicated efforts.

I think that was also what I suggested. ntfsimage and ntfsrestore? Or just
ntfsimage? 

I'm also quite sure when we would be ready Ian will also arrive saying
"here is my ntfsimage I promised a year ago!" :)

Merging the two using the name ntfsimage would solve my "ntfsclone syntax
problem" also :)

	Szaka

[Linux-NTFS-Dev] cp --sparse might corrupt DEST

[Szakacsits S. <sz...@si...>]

On Wed, 25 Feb 2004, Szakacsits Szabolcs wrote:
> On Wed, 25 Feb 2004, Per Olofsson wrote:
> 
> > This is what I thought would be unreliable:
> > 
> > cp --sparse=always sparse-image-file.img /dev/hda1
> > 
> > But since you're saying that cp won't allow that, this apparently
> > isn't an issue.
> 
> You were right. Unfortunately this is possible and nobody should ever use
> it. I must definitely mention this in the ntfsclone manual ... thank you!

I had some time to think about the issue, not just quickly answer email ;)

Actually it's a bug in cp. What's cp job? To copy files reliable. Can it
do that when used in the above case (block device as the destination)? No.

Sure it's a bug. If cp detects sparseness in the source file it will skip
on the destination file and leave the skipped data unchanged. Source and
destination won't be the same. It wasn't a copy operation. cp should fill
those skipped areas with zeros, well actually never skip in these cases.

cp should check the file type of the destination and if it's block device
then fallback to --sparse=never.

% cp --version
cp (coreutils) 5.0

	Szaka

Re: [Linux-NTFS-Dev] Re: Efficient use of ntfsclone unreliable?

[Per O. <pe...@ds...>]

On Wed, Feb 25, 2004 at 17:50 +0100, Szakacsits Szabolcs wrote:
> > Still, there is one drawback with ntfsclone. It's not possible, AFAIK,
> > to restore images efficiently from a pipe. If you want to put an image
> > on a web server, or more importantly, if you want to multicast an
> > image to many computers, you need to have some kind of temporary
> > storage on the computer(s) you're writing the image to.
> 
> The 'temporary storage' isn't true but the 'efficiently' is true. E.g.
> 
>   ntfsclone -o - dev | gzip -2c | ssh -C host 'gunzip -c | dd of=dev'
> 
> No temporary storage but it also doesn't have optimal network and disk
> usage.

What I meant was that, as you say, you can't get efficiency without
temporary storage. Of course, you don't need temporary storage if you
use the inefficient approach.

> About one year ago, after lot of discussions with Ian Jackson, we agreed
> to make three different utilities:
> 
> 	- ntfsimage and restoreimage
> 	- ntfsclone
> 
> He wrote the first two, exactly what you wrote but never sent to us.

I see. The reason I never sent anything to you is that my utilities
are just a hack and don't really work as they are now. Are ntfsimage
and restoreimage available somewhere?

> So only ntfsclone left. I needed it badly for development to debug user
> issues (see --metadata option) and to do things as fast and efficient as
> possible (no much free time to play with NTFS). Full backup functionality
> came mostly as a side-effect. 
...
> Yes, there is definitely need for it. I also mentioned this earlier. I'll
> think about it again how it would be the best to progress :) Suggestions
> are of course also welcomed :)

ntfsclone and ntfsimage do essentially the same thing, they skip over
unused blocks. The output format is merely a detail and could be
chosen with a command-line option. I think it would be better if they
were one utility, in order to reduce duplicated efforts.

-- 
Pelle

Re: [Linux-NTFS-Dev] Re: Efficient use of ntfsclone unreliable?

[Szakacsits S. <sz...@si...>]

On Wed, 25 Feb 2004, Per Olofsson wrote:

> > Well, there is (FIBMAP ioctl) but it's not used because it's very
> > inefficient. Only XFS supports this efficiently. I wrote about these in
> > the ntfsclone manual. Did you check it out?
> 
> Yes, I did, but you only talked about efficiency. 

True, maybe I should move those parts to the end of the manual or just
delete it completely, if it confuses/scares people.

> > What about cp and tar? They are unable to restore an _image_ to a block
> > device (partition), they will give an error when one tries to open the
> > device.
> 
> Okay, I thought that they would. In principle, they should be able to
> write to a block device, but I assume that they won't because of some
> built-in sanity checks then?

AFAIR somebody wanted to or sent a patch for this to tar developers but 
I don't know the status. As for cp, I remembered it doesn't work.
Now I tried again your example below on a lopback device and it worked.
That's bad. One could misuse cp and indeed lose data, as you wrote.

Well, I think I did only minimum one million times in my life
'cp floppy.img /dev/floppy'. Dumb me ...
 
> This is what I thought would be unreliable:
> 
> cp --sparse=always sparse-image-file.img /dev/hda1
> 
> But since you're saying that cp won't allow that, this apparently
> isn't an issue.

You were right. Unfortunately this is possible and nobody should ever use
it. I must definitely mention this in the manual ... thank you!
 
> Note that I was referring only to this specific potential use of
> ntfsclone, not ntfsclone in itself. 

I wanted it to be the simplest to use and make it work easily with
standard tools. The end result seems to be a bit confusing and potential
misuse.

I also don't like the syntax

	ntfsclone --[output|overwrite] [destination|-] source

this would have been better

	ntfsclone [--overwrite] source [destination]

But I can't change since this would be fatal:

	old version: ntfsclone --overwrite destination source
	new version: ntfsclone --overwrite source destination

There is a synonym for 'overwrite'? I thought about using 'restore'
but it;s not always that. It's really used as 'overwrite'.

> Still, there is one drawback with ntfsclone. It's not possible, AFAIK,
> to restore images efficiently from a pipe. If you want to put an image
> on a web server, or more importantly, if you want to multicast an
> image to many computers, you need to have some kind of temporary
> storage on the computer(s) you're writing the image to.

The 'temporary storage' isn't true but the 'efficiently' is true. E.g.

  ntfsclone -o - dev | gzip -2c | ssh -C host 'gunzip -c | dd of=dev'

No temporary storage but it also doesn't have optimal network and disk
usage.

> I have written a utility quite similar to ntfsclone which instead of
> sparse files uses "sparse images" to store the filesystem. The images
> are stored in a special format which explicitly marks the segments of
> the filesystem which are unused, and leaves those out the file. With
> this, you can do something like:
> 
> On server:
> ntfsimage /dev/hda1 >image.simg
> 
> On client:
> wget -O- http://server/image.simg | simgrestore /dev/hda1
> 
> And no unused blocks need to be transferred. 

About one year ago, after lot of discussions with Ian Jackson, we agreed
to make three different utilities:

	- ntfsimage and restoreimage
	- ntfsclone

He wrote the first two, exactly what you wrote but never sent to us. 

So only ntfsclone left. I needed it badly for development to debug user
issues (see --metadata option) and to do things as fast and efficient as
possible (no much free time to play with NTFS). Full backup functionality
came mostly as a side-effect. 

You can read about these discussions in the archive, just search for
ntfsimage.

	http://marc.theaimsgroup.com/?l=linux-ntfs-dev&w=2

> Unfortunately I haven't spent much time on this utility so it's still
> buggy, although it works on simple test cases. It would be interesting
> to try to integrate this functionality into ntfsclone.

Yes, there is definitely need for it. I also mentioned this earlier. I'll
think about it again how it would be the best to progress :) Suggestions
are of course also welcomed :)

	Szaka

[Linux-NTFS-Dev] how to fix bugs

[Andras E. <cc...@fr...>]

[sorry for the long post]

i'm thinking about contributing some effort to your project;
a good first step can be getting familiar with already existing=20
code, thus i picked mkntfs.c and started reading it

there seem to be a few bugs in the code, so while reading it
i might as well try to fix them, but i don't know what kind
of fixes would you prefer, so i selected a trivial function
with a trivial bug, and written three fixes -- please let me
know which one would you prefer, and flame me a bit, so that
my future fixes can get through more easily (if you want them
at all :O)

the function in question is stoucs(); it gets the size of
the output buffer in bytes (as maxlen), but in fact it can
write (maxlen-2)*2+2 bytes into it (e.g. into an output
buffer of 10 bytes it writes up to 18 bytes):

/**
 * stoucs - convert ASCII string to unicode-character string
 * @dest: points to buffer to receive the converted string
 * @src:  points to string to convert
 * @maxlen: size of @dest buffer in bytes
 *
 * Return the number of characters written to @dest, not including the
 * terminating null unicode character.
 */
int stoucs(uchar_t *dest, const char *src, int maxlen)
{
  char c;
  int i;

  /* Need two bytes for null terminator. */
  maxlen -=3D 2;
  for (i =3D 0; i < maxlen; i++) {
    c =3D src[i];
    if (!c)
      break;
    dest[i] =3D cpu_to_le16(c);
  }
  dest[i] =3D cpu_to_le16('\0');
  return i;
}

the first fix is the minimalistic one:

--- mkntfs.c.orig       2004-02-25 15:29:28.000000000 +0100
+++ mkntfs.c    2004-02-25 16:23:38.000000000 +0100
@@ -569,7 +569,7 @@

        /* Need two bytes for null terminator. */
        maxlen -=3D 2;
-       for (i =3D 0; i < maxlen; i++) {
+  for (i =3D 0; i < maxlen/2; i++) {
                c =3D src[i];
                if (!c)
                        break;

the second fix preserves the original method, with a
little cleanup:

int stoucs( uchar_t * dst, char const * src, int size )
{
  int const uni_size =3D size / sizeof( *dst ) ;
  int cp ;

  for ( cp =3D 0 ; src[ cp ] && cp < uni_size - 1 ; ++cp )
      dst[ cp ] =3D cpu_to_le16( src[ cp ] ) ;

  dst[ cp ] =3D cpu_to_le16( '0' ) ;

  return cp ;
}

the third fix is an imho more c-ish complete rewrite
(+ assert + comments):

#include <assert.h>

unsigned stoucs( uchar_t * dst, char const * src, unsigned const size )
{
  char const * const begin =3D src ;
  unsigned uni_size =3D size / sizeof( *dst ) ;

  assert( uni_size ) ;

  // while there's both input and output left
  while ( *src && --uni_size )
      *dst++ =3D cpu_to_le16( *src++ ) ; // ascii -> unicode

  // make sure the output is terminated even when there wasn't
  // enough space to convert the whole input
  *dst =3D cpu_to_le16( '0' ) ;

  return src - begin ;
}

i've also created a revised version of the comment describing
the function:

/**
 * stoucs - convert ASCIIZ string to unicode string
 *
 * @param dst [out] points the buffer to receive the unicode string
 * @param src [in] points the char string to be converted
 * @param size [in] size of the @dst buffer in bytes
 *
 * @returns the number of chars converted, excluding the
 * terminating null unicode character.
 *
 * @par constraint: @dst must be able to hold at least the
 * terminating null [@size >=3D sizeof( uchar_t )]
 * @see ucstos
 */


br,
andras

ps: i've compiled the above changes, but haven't tested
them, so please don't use them yet

[Linux-NTFS-Dev] Re: Efficient use of ntfsclone unreliable?

[Per O. <pe...@ds...>]

On Wed, Feb 25, 2004 at 13:09 +0100, Szakacsits Szabolcs wrote:
> Right. Here is the simplest scenario:
> 
>   backup:   ntfsclone -o mybackup.img /dev/hda1
>   restore:  ntfsclone -O /dev/hda1 mybackup.img
> 
> The restoration will not write the unused blocks back to the disk.

Oh, sorry, I overlooked this one for some reason. It indeed writes a
sparse file image reliably.

> Well, there is (FIBMAP ioctl) but it's not used because it's very
> inefficient. Only XFS supports this efficiently. I wrote about these in
> the ntfsclone manual. Did you check it out?

Yes, I did, but you only talked about efficiency. I was more
interested in whether it was possible to _reliably_ detect where holes
in a sparse file are located. The method above which uses ntfsclone to
restore the image doesn't look at where the holes are, it reads the
actual NTFS filesystem to determine which parts to write onto the
target device.

> What about cp and tar? They are unable to restore an _image_ to a block
> device (partition), they will give an error when one tries to open the
> device.

Okay, I thought that they would. In principle, they should be able to
write to a block device, but I assume that they won't because of some
built-in sanity checks then?

> And now comes the interesting part. What if you want to copy/create an
> image with cp or tar with the -S option to a _file_? Can they mess up
> data? The answer is again no. Both will create a new file and if they
> detect there is somewhere only zeros then altough they skip that part in
> the target but the filesystem will set them 0. They will be also sparse
> files, no messed up data.
> 
> I hope I could explain the issue, if not please give a specific example
> when you think the process would be unreliable.

This is what I thought would be unreliable:

cp --sparse=always sparse-image-file.img /dev/hda1

But since you're saying that cp won't allow that, this apparently
isn't an issue.

> > And therefore, this use of ntfsclone is not reliable. Is this correct?
> 
> It's not correct. I don't know many NTFS imaging softwares but I've heard
> several times from several sources that e.g. Ghost can trash NTFS.
> Partimage, Mondo (based on Partimage) is unreliable. I wrote about it here
> longer:

Note that I was referring only to this specific potential use of
ntfsclone, not ntfsclone in itself. My intent was not to say that
ntfsclone is an unreliable program as such.

Still, there is one drawback with ntfsclone. It's not possible, AFAIK,
to restore images efficiently from a pipe. If you want to put an image
on a web server, or more importantly, if you want to multicast an
image to many computers, you need to have some kind of temporary
storage on the computer(s) you're writing the image to.

I have written a utility quite similar to ntfsclone which instead of
sparse files uses "sparse images" to store the filesystem. The images
are stored in a special format which explicitly marks the segments of
the filesystem which are unused, and leaves those out the file. With
this, you can do something like:

On server:
ntfsimage /dev/hda1 >image.simg

On client:
wget -O- http://server/image.simg | simgrestore /dev/hda1

And no unused blocks need to be transferred. Unfortunately I haven't
spent much time on this utility so it's still buggy, although it works
on simple test cases. It would be interesting to try to integrate this
functionality into ntfsclone.

-- 
Pelle

[Linux-NTFS-Dev] Re: Efficient use of ntfsclone unreliable?

[Szakacsits S. <sz...@si...>]

Hello Pelle,

I CC: linux-ntfs-dev because you made an interesting point.

On Wed, 25 Feb 2004, Per Olofsson wrote:

> I have a question regarding ntfsclone:

Please feel free to ask anytime. I'm very interested what bothers users,
what they think, what they find confusing, and so on.
 
> Since ntfsclone uses sparse files, where holes represent unused blocks
> (clusters) of the cloned partition, I thought it would be possible to
> use this functionality to avoid writing unused blocks when restoring
> an image to a hard disc partition. 

Right. Here is the simplest scenario:

  backup:   ntfsclone -o mybackup.img /dev/hda1
  restore:  ntfsclone -O /dev/hda1 mybackup.img

The restoration will not write the unused blocks back to the disk. Both
action should take about the same time and this time is limited only by
the maximal speed of the disk and how good the kernel is scheduling the
disk IO. Disk seeks are minimalised by ntfsclone.

> But as far as I can tell, sparse files are just a filesystem level
> optimisation, 

Right.

> and there is no way for programs to tell where the holes in a sparse
> file actually are. 

Well, there is (FIBMAP ioctl) but it's not used because it's very
inefficient. Only XFS supports this efficiently. I wrote about these in
the ntfsclone manual. Did you check it out?

	http://linux-ntfs.sourceforge.net/man/ntfsclone.html

> It seems like tar and cp just scans the file for large chunks of
> zeroes and then assumes that they represent holes.

Unfortunately true. It's also mentioned in the ntfsclone manual.

> If this method is used to restore an image to a partition, I assume
> that if there actually *is* files on the image containing longer
> streams of zeroes, these will simply not get written to the target
> partition, thus being replaced by whatever garbage was there
> before. 

This is not true. There are only two ways to restore an image to a
partition:

     1) ntfsclone - it does the job correctly and saves the time
                    writting back unused blocks.

     2) dd        - it writes _everything_. It will also write the zeros,
                    thus it's slower.

What about cp and tar? They are unable to restore an _image_ to a block
device (partition), they will give an error when one tries to open the
device.

And now comes the interesting part. What if you want to copy/create an
image with cp or tar with the -S option to a _file_? Can they mess up
data? The answer is again no. Both will create a new file and if they
detect there is somewhere only zeros then altough they skip that part in
the target but the filesystem will set them 0. They will be also sparse
files, no messed up data.

I hope I could explain the issue, if not please give a specific example
when you think the process would be unreliable.

> And therefore, this use of ntfsclone is not reliable. Is this correct?

It's not correct. I don't know many NTFS imaging softwares but I've heard
several times from several sources that e.g. Ghost can trash NTFS.
Partimage, Mondo (based on Partimage) is unreliable. I wrote about it here
longer:

	http://www.partimage.org/forums/viewtopic.php?t=5

So the only tools I would trust at present to backup/restore NTFS is dd
and ntfsclone. 

Well, actually I do use it for almost a year _very_ intensively because
it's needed for development (backups and automatic testing all the time)
and I've never had any problem. Others didn't report problems either.

	Szaka

Re: [Linux-NTFS-Dev] linux crash down...!

[Szakacsits S. <sz...@si...>]

> > I couldn't see the linux. so can u help me to recover the linux.

Sometimes (always?) when you install Windows after Linux, Windows rewrites
the MBR thus Linux can't be booted anymore (the LILO or GRUB boot manager
gets lost). 

Most distros has some rescue options on the install CD that recovers the
lost MBR and make all OS's bootable. E.g. Mandrake knows this for sure.

	Szaka

Re: [Linux-NTFS-Dev] linux crash down...!

[Richard R. <nt...@fl...>]

Hi Ameen,

> I installed  windowsXP and then linux. both were working well

Great

> then I installed 2003 sever in another partision.then my linux
> corrupted,now it doesn't work

Tough luck.  That's Windows for you.

> I couldn't see the linux. so can u help me to recover the linux.

Sorry, no.  Even if we had the time, it would be extremely difficult to
find out what went wrong, without physical access to the machine.

The 2003 install could have done anything in the range from, deleting
the partition, to stamping all over it.  We could spend lots of time
helping one person, just to find that nothing is recoverable.

Cheers,
  FlatCap (Rich)
  nt...@fl...

[Linux-NTFS-Dev] linux crash down...!

[Mohamed A. <MA...@vi...>]

Sir,
my machine have 3 OS. which r XP(2003 billgate edition) , windows server =
2003 & linux ver-8.0.first I installed  windowsXPand then linux. both =
were working well.then I installed 2003 sever in another partision.then =
my linux corrupted,now it doesn't work,even I couldn't see the linux.so =
can u help me to recover the linux.

Regards,
Ameen
am...@ho... =20

Re: [Linux-NTFS-Dev] BUG: Found compressed file with data_size not equal to initialized_size

[Szakacsits S. <sz...@si...>]

On Fri, 20 Feb 2004, Oezguer Demir wrote:

> > Feb 20 00:16:06 nostromo kernel: NTFS-fs warning (device 03:01):
> ntfs_read_locked_inode(): BUG: Found compressed file with data_size
> not equal to initialized_size. This will probably cause problems when
> trying to access the file. Please notify lin...@li...
> that you saw this message. Thanks!
> 
> your welcome. need more info?

Thank you but no more info needed. Anton took care about this (harmless)
issue a year ago.

However I'd recommend you an upgrade because there were important fixes in
the driver since your current driver was released.

Cheers,
	Szaka

[Linux-NTFS-Dev] BUG: Found compressed file with data_size not equal to initialized_size

[Oezguer D. <oe...@gm...>]

> Feb 20 00:16:06 nostromo kernel: NTFS-fs warning (device 03:01): ntfs_read_locked_inode(): BUG: Found compressed file with data_size not equal to initialized_size. This will probably cause problems when trying to access the file. Please notify lin...@li... that you saw this message. Thanks!

your welcome. need more info?

oeze

Re: [Linux-NTFS-Dev] ntfs_device_size_get: 2 TB support on 32 bits platforms

[Anton A. <ai...@ca...>]

On Tue, 17 Feb 2004, Szakacsits Szabolcs wrote:
> I committed the above fix in response to a bug report. The limit was only
> 1 TB due to a bug by misusing the BLKGETSIZE ioctl. I checked e2fsprogs
> from where the relevant code was adapted and it also uses the correct
> 'unsigned long' for the device size now.

Cool.  I went a step further and added detection and use of BLKGETSIZE64
which uses a 64 bit operand and thus allows for 2^63-1 bytes for the
device size.  (-:

I also updated ntfsclone to use this.

Szaka, well done on the resize stuff!

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/