|
From: Yura P. <cha...@us...> - 2005-07-29 14:44:32
|
Changes by: cha0smaster Update of /cvsroot/linux-ntfs/ntfsprogs/libntfs In directory sc8-pr-cvs1.sourceforge.net:/tmp/cvs-serv12046/libntfs Modified Files: volume.c Log Message: Change errno and print more verbose message in case if logfile check failed. Index: volume.c =================================================================== RCS file: /cvsroot/linux-ntfs/ntfsprogs/libntfs/volume.c,v retrieving revision 1.47 retrieving revision 1.48 diff -u -p -r1.47 -r1.48 --- volume.c 18 Jul 2005 02:09:43 -0000 1.47 +++ volume.c 29 Jul 2005 14:44:24 -0000 1.48 @@ -562,7 +562,7 @@ static int ntfs_volume_check_logfile(ntf { ntfs_inode *ni; ntfs_attr *na = NULL; - int ret, err = 0; + int err = 0; if ((ni = ntfs_inode_open(vol, FILE_LogFile)) == NULL) { Dprintf("Failed to open inode FILE_LogFile.\n"); @@ -571,23 +571,20 @@ static int ntfs_volume_check_logfile(ntf } if ((na = ntfs_attr_open(ni, AT_DATA, AT_UNNAMED, 0)) == NULL) { Dprintf("Failed to open $FILE_LogFile/$DATA\n"); - ret = -1; err = EIO; goto exit; } - if (ntfs_check_logfile(na) && ntfs_is_logfile_clean(na)) - ret = 0; - else { - ret = -1; - err = EIO; - } + if (!ntfs_check_logfile(na) || !ntfs_is_logfile_clean(na)) + err = EOPNOTSUPP; exit: if (na) ntfs_attr_close(na); ntfs_inode_close(ni); - if (ret) + if (err) { errno = err; - return ret; + return -1; + } + return 0; } /** |
|
From: Szakacsits S. <sz...@si...> - 2005-07-29 14:58:37
|
On Fri, 29 Jul 2005, Yura Pakhuchiy wrote:
> - if (ntfs_check_logfile(na) && ntfs_is_logfile_clean(na))
> - ret = 0;
> - else {
> - ret = -1;
> - err = EIO;
> - }
> + if (!ntfs_check_logfile(na) || !ntfs_is_logfile_clean(na))
> + err = EOPNOTSUPP;
I don't like this very much. Now users would get the message that mounting
NTFS is not supported and that's pretty bogus. EIO covered the issue much
more better.
The change also "breaks" ntfsresize. Currently users get this message for
EIO:
"Apparently you have a corrupted NTFS. Please run the filesystem checker\n"
"on Windows by invoking chkdsk /f. Don't forget the /f (force) parameter,\n"
"it's important! You probably also need to reboot Windows to take effect.\n"
"Then you can try ntfsresize again. No modification was made to your NTFS.\n";
This always helped in the past. Now they would get only the "not supported
message". It would be too confusing.
Szaka
|
|
From: Yura P. <pak...@ip...> - 2005-07-29 15:11:22
|
On Fri, 2005-07-29 at 16:58 +0200, Szakacsits Szabolcs wrote:
> On Fri, 29 Jul 2005, Yura Pakhuchiy wrote:
>
> > - if (ntfs_check_logfile(na) && ntfs_is_logfile_clean(na))
> > - ret = 0;
> > - else {
> > - ret = -1;
> > - err = EIO;
> > - }
> > + if (!ntfs_check_logfile(na) || !ntfs_is_logfile_clean(na))
> > + err = EOPNOTSUPP;
>
> I don't like this very much. Now users would get the message that mounting
> NTFS is not supported and that's pretty bogus. EIO covered the issue much
> more better.
I don't think so. We should return EIO on real errors (eg. damaged
volume), but unclean logfile it's not damaged volume, it's simply case
that we don't support yet. So I think that EOPNOTSUPP is absolutely
correct here.
> The change also "breaks" ntfsresize. Currently users get this message for
> EIO:
>
> "Apparently you have a corrupted NTFS. Please run the filesystem checker\n"
> "on Windows by invoking chkdsk /f. Don't forget the /f (force) parameter,\n"
> "it's important! You probably also need to reboot Windows to take effect.\n"
> "Then you can try ntfsresize again. No modification was made to your NTFS.\n";
>
> This always helped in the past. Now they would get only the "not supported
> message". It would be too confusing.
BTW, mount fails with EOPNOTSUPP only in case unclean logfile, so you
can print above message in this case too. All other utils will print
correct message due to changes in volume.c.
--
Best regards,
Yura
|
|
From: Yura P. <pak...@ip...> - 2005-07-29 15:21:44
|
On Fri, 2005-07-29 at 18:11 +0300, Yura Pakhuchiy wrote:
> BTW, mount fails with EOPNOTSUPP only in case unclean logfile, so you
> can print above message in this case too. All other utils will print
> correct message due to changes in volume.c.
I meant utils.c.
--
Best regards,
Yura
|
|
From: Szakacsits S. <sz...@si...> - 2005-07-29 15:31:35
|
On Fri, 29 Jul 2005, Yura Pakhuchiy wrote:
> On Fri, 2005-07-29 at 16:58 +0200, Szakacsits Szabolcs wrote:
> > On Fri, 29 Jul 2005, Yura Pakhuchiy wrote:
> >
> > > - if (ntfs_check_logfile(na) && ntfs_is_logfile_clean(na))
> > > - ret = 0;
> > > - else {
> > > - ret = -1;
> > > - err = EIO;
> > > - }
> > > + if (!ntfs_check_logfile(na) || !ntfs_is_logfile_clean(na))
> > > + err = EOPNOTSUPP;
> >
> > I don't like this very much. Now users would get the message that mounting
> > NTFS is not supported and that's pretty bogus. EIO covered the issue much
> > more better.
>
> I don't think so. We should return EIO on real errors (eg. damaged
> volume), but unclean logfile it's not damaged volume, it's simply case
> that we don't support yet. So I think that EOPNOTSUPP is absolutely
> correct here.
You converted all ntfs_check_logfile() and ntfs_is_logfile_clean() error
cases to EOPNOTSUPP. Damaged volumes too.
Szaka
|
|
From: Yura P. <pak...@ip...> - 2005-07-29 15:40:00
|
On Fri, 2005-07-29 at 17:31 +0200, Szakacsits Szabolcs wrote:
> On Fri, 29 Jul 2005, Yura Pakhuchiy wrote:
> > On Fri, 2005-07-29 at 16:58 +0200, Szakacsits Szabolcs wrote:
> > > On Fri, 29 Jul 2005, Yura Pakhuchiy wrote:
> > >
> > > > - if (ntfs_check_logfile(na) && ntfs_is_logfile_clean(na))
> > > > - ret = 0;
> > > > - else {
> > > > - ret = -1;
> > > > - err = EIO;
> > > > - }
> > > > + if (!ntfs_check_logfile(na) || !ntfs_is_logfile_clean(na))
> > > > + err = EOPNOTSUPP;
> > >
> > > I don't like this very much. Now users would get the message that mounting
> > > NTFS is not supported and that's pretty bogus. EIO covered the issue much
> > > more better.
> >
> > I don't think so. We should return EIO on real errors (eg. damaged
> > volume), but unclean logfile it's not damaged volume, it's simply case
> > that we don't support yet. So I think that EOPNOTSUPP is absolutely
> > correct here.
>
> You converted all ntfs_check_logfile() and ntfs_is_logfile_clean() error
> cases to EOPNOTSUPP. Damaged volumes too.
Yes, because this functions check consistency of logfile content, even
if it's absolutely invalid it's not a damaged volume. On damaged volumes
we will receive error much earlier, during previous steps of mount,
ntfs_inode_open for logfile or ntfs_attr_open.
--
Best regards,
Yura
|
|
From: Szakacsits S. <sz...@si...> - 2005-08-01 20:37:14
|
Hi Yura, On Fri, 29 Jul 2005, Yura Pakhuchiy wrote: > On Fri, 2005-07-29 at 17:31 +0200, Szakacsits Szabolcs wrote: > > On Fri, 29 Jul 2005, Yura Pakhuchiy wrote: > > > > > > I don't think so. We should return EIO on real errors (eg. damaged > > > volume), but unclean logfile it's not damaged volume, it's simply case > > > that we don't support yet. So I think that EOPNOTSUPP is absolutely > > > correct here. Please also note that EOPNOTSUPP == ENOTSUP, which is used in quite many places in libntfs already. > > You converted all ntfs_check_logfile() and ntfs_is_logfile_clean() error > > cases to EOPNOTSUPP. Damaged volumes too. > > Yes, because this functions check consistency of logfile content, even > if it's absolutely invalid it's not a damaged volume. On damaged volumes > we will receive error much earlier, during previous steps of mount, > ntfs_inode_open for logfile or ntfs_attr_open. Well, anyway, maybe it's still better because I've never met ENOTSUP from ntfs_mount so we could now distinguish immediately this error case from all the others. Szaka |
|
From: Yura P. <pak...@ip...> - 2005-08-01 20:40:32
|
On Mon, 2005-08-01 at 22:37 +0200, Szakacsits Szabolcs wrote:
> Hi Yura,
>
> On Fri, 29 Jul 2005, Yura Pakhuchiy wrote:
> > On Fri, 2005-07-29 at 17:31 +0200, Szakacsits Szabolcs wrote:
> > > On Fri, 29 Jul 2005, Yura Pakhuchiy wrote:
> > > >
> > > > I don't think so. We should return EIO on real errors (eg. damaged
> > > > volume), but unclean logfile it's not damaged volume, it's simply case
> > > > that we don't support yet. So I think that EOPNOTSUPP is absolutely
> > > > correct here.
>
> Please also note that EOPNOTSUPP == ENOTSUP, which is used in quite many
> places in libntfs already.
Yes, I know, but prefer it because of portability, kernel doesn't have
ENOTSUP, only EOPNOTSUPP.
> > > You converted all ntfs_check_logfile() and ntfs_is_logfile_clean() error
> > > cases to EOPNOTSUPP. Damaged volumes too.
> >
> > Yes, because this functions check consistency of logfile content, even
> > if it's absolutely invalid it's not a damaged volume. On damaged volumes
> > we will receive error much earlier, during previous steps of mount,
> > ntfs_inode_open for logfile or ntfs_attr_open.
>
> Well, anyway, maybe it's still better because I've never met ENOTSUP from
> ntfs_mount so we could now distinguish immediately this error case from
> all the others.
Great! Glad that you agreed. :-)
--
Best regards,
Yura
|
|
From: Szakacsits S. <sz...@si...> - 2005-08-01 21:03:16
|
On Mon, 1 Aug 2005, Yura Pakhuchiy wrote: > On Mon, 2005-08-01 at 22:37 +0200, Szakacsits Szabolcs wrote: >> >> Please also note that EOPNOTSUPP == ENOTSUP, which is used in quite many >> places in libntfs already. > > Yes, I know, but prefer it because of portability, kernel doesn't have > ENOTSUP, only EOPNOTSUPP. How about replacing all ENOTSUP to EOPNOTSUPP? Szaka |
|
From: Yura P. <pak...@ip...> - 2005-08-01 21:06:49
|
On Tue, 2005-08-02 at 00:02 +0300, Szakacsits Szabolcs wrote:
> On Mon, 1 Aug 2005, Yura Pakhuchiy wrote:
> > On Mon, 2005-08-01 at 22:37 +0200, Szakacsits Szabolcs wrote:
> >>
> >> Please also note that EOPNOTSUPP == ENOTSUP, which is used in quite many
> >> places in libntfs already.
> >
> > Yes, I know, but prefer it because of portability, kernel doesn't have
> > ENOTSUP, only EOPNOTSUPP.
>
> How about replacing all ENOTSUP to EOPNOTSUPP?
I think it's good idea. I use EOPNOTSUPP in all my new code, but never
had time for mass-replace in the library.
--
Best regards,
Yura
|
|
From: Szakacsits S. <sz...@si...> - 2005-08-01 21:08:49
|
On Tue, 2 Aug 2005, Yura Pakhuchiy wrote: > On Tue, 2005-08-02 at 00:02 +0300, Szakacsits Szabolcs wrote: >> On Mon, 1 Aug 2005, Yura Pakhuchiy wrote: >>> On Mon, 2005-08-01 at 22:37 +0200, Szakacsits Szabolcs wrote: >>>> >>>> Please also note that EOPNOTSUPP == ENOTSUP, which is used in quite many >>>> places in libntfs already. >>> >>> Yes, I know, but prefer it because of portability, kernel doesn't have >>> ENOTSUP, only EOPNOTSUPP. >> >> How about replacing all ENOTSUP to EOPNOTSUPP? > > I think it's good idea. I use EOPNOTSUPP in all my new code, but never > had time for mass-replace in the library. I can do it in about 10 seconds, if it's also OK for Anton. Szaka |
|
From: Anton A. <ai...@ca...> - 2005-08-01 21:28:14
|
On Tue, 2 Aug 2005, Szakacsits Szabolcs wrote: > On Tue, 2 Aug 2005, Yura Pakhuchiy wrote: > > On Tue, 2005-08-02 at 00:02 +0300, Szakacsits Szabolcs wrote: > > > On Mon, 1 Aug 2005, Yura Pakhuchiy wrote: > > > > On Mon, 2005-08-01 at 22:37 +0200, Szakacsits Szabolcs wrote: > > > > > > > > > > Please also note that EOPNOTSUPP == ENOTSUP, which is used in quite > > > > > many > > > > > places in libntfs already. > > > > > > > > Yes, I know, but prefer it because of portability, kernel doesn't have > > > > ENOTSUP, only EOPNOTSUPP. > > > > > > How about replacing all ENOTSUP to EOPNOTSUPP? > > > > I think it's good idea. I use EOPNOTSUPP in all my new code, but never > > had time for mass-replace in the library. > > I can do it in about 10 seconds, if it's also OK for Anton. Sure, I don't really care. As long as they both exist on all OS we support that is fine. And at least according to the man page for errno it says that both are listed by POSIX.1 so we should be fine. 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/ |
