All users were logged out of Bugzilla on October 13th, 2018

If directory already exists, File.dirCreate returns -230

VERIFIED FIXED

Status

P3
minor
VERIFIED FIXED
19 years ago
3 years ago

People

(Reporter: depman1, Assigned: dprice)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

19 years ago
build 2000-06-13-09-M17. also see 41982.
1. Go to http://jimbob/trigger3.html
2. Select a_fileop_dircreate from the acceptance menu.
3. Press Trigger, then OK.
4. Trigger a_fileop_dircreate again.
Result: Get -230 error.
Notes: As expected, it does not remove directory contents. And unix and dos 
don't allow a directory to be recreated (we get an error msg). The issue raised 
in this bug is whether it's inconvenient for the script writer to have to check 
for the existence of the directory before creating it.
(Reporter)

Comment 1

19 years ago
changed QA contact to depstein
QA Contact: jimmylee → depstein
Yes, we could certainly live with the way things are currently designed (which 
mimics the platform) but it will make for *much* simpler scripts if script 
authors didn't have to check for directory existence first or reset error to 
ignore errors.

This is not one we'll get to in a while, but it would be nice I think. CC'ing 
Sean in case he has a different perspective as an experienced install author

Comment 3

19 years ago
I agree with Dan.  It is much easier if the script writer didn't have to check 
for the existance of the dir ahead of time.

This can actually be a one line fix in the NativeFileOpDirCreatePrepare() 
function to return nsInstall::SUCCESS regardless if the directory already exists 
or not, instead of the -230 error if it already exists.

Comment 4

17 years ago
Updating QA Contact.
QA Contact: depstein → jimmylee
(Assignee)

Comment 5

17 years ago
Created attachment 46841 [details] [diff] [review]
fix that always returns success
(Assignee)

Comment 6

17 years ago
I've made a patch that always returns sucess.  I notice we never pay attention
to the result of mTarget->Create()  Is that a bad thing?
Assignee: dveditz → dprice
You're right, expecting that Create() always succeeds is pretty lame. As long as
you're changing this code please fix it.

I think you'll find nsInstallFileOpItem is rife with such problems... feel free
to fix it up ;-)

In the future please give more context in your patches, the default -3 is not
adequate. "cvs diff -u10" would be a good minimum, some reviewers want -20 for
non-trivial changes.
(Assignee)

Comment 8

17 years ago
Created attachment 47937 [details] [diff] [review]
patch also checks return value of mTarget->Create()
Attachment #46841 - Attachment is obsolete: true
Comment on attachment 47937 [details] [diff] [review]
patch also checks return value of mTarget->Create()

>-  PRInt32 ret = nsInstall::ALREADY_EXISTS;
>+  PRInt32 ret;
>   PRBool  flagExists;
> 
>-  mAction = nsInstallFileOpItem::ACTION_FAILED;
>-
>   mTarget->Exists(&flagExists);
>   if (!flagExists)
>+    ret = mTarget->Create(1, 0755);
>+ 
>+  if (ret == NS_OK) 

Return value from an nsIFile is going to be an nsresult -- use the NS_SUCCEEDED/NS_FAILED macros.
And you really should use an nsresult for it rather than re-using a PRInt32.

You no longer have an initial value for ret, so what if the target *does* exist? What if it exists
and happens to be a file that's in the way? Now that already existing is OK, we have to make
sure it's the right kind of thing.

There's no check that mTarget->Exists() returns success, so you can't trust the potentially
undefined flagExists variable.
Attachment #47937 - Flags: needs-work+
(Assignee)

Comment 10

17 years ago
A quick clarification.  If mTarget exists, and it is a directory, then we're
okay to return nsInstall::SUCCESS  If mTarget is a file, then we want to fail
and return nsInstall::IS_FILE?
Sure, IS_FILE sounds appropriate.
(Assignee)

Comment 12

17 years ago
Created attachment 49681 [details] [diff] [review]
Patch v3.0
Comment on attachment 49681 [details] [diff] [review]
Patch v3.0

>+  else
>+  {

You could also get here if mTarget->Exists() fails, not just if it exists.
Presumably that would lead to mTarget->IsFile() failing as well so it's
not so bad to ignore that possibility, but

>+    rv = mTarget->IsFile(&flagIsFile);
>+    if (NS_SUCCEEDED(rv) && (flagIsFile))
>+    {
>+      mAction = nsInstallFileOpItem::ACTION_FAILED;
>+      ret = nsInstall::IS_FILE;
>+    }

you can't ignore the possibility that mTarget->IsFile() will fail. If it
does you will return nsInstall::SUCCESS and mAction will be set to
ACTION_SUCCESS

Since you're in nsInstallFileOpItem methods you don't need to
specify nsInstallFileOpItem:: to use ACTION_FAILED and ACTION_SUCCESS.
The rest of the file does, though, so it's up to you if you want to
strike a blow for readability or for consistency ;-)
Attachment #49681 - Flags: needs-work+
(Assignee)

Comment 14

17 years ago
Created attachment 51771 [details] [diff] [review]
newer patch
(Assignee)

Updated

17 years ago
Attachment #47937 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #49681 - Attachment is obsolete: true
(Assignee)

Comment 15

17 years ago
Thanks for the feedback Dan.  Here's a patch that should cover everything.  
Comment on attachment 51771 [details] [diff] [review]
newer patch

looks good, r=dveditz
Attachment #51771 - Flags: review+

Comment 17

17 years ago
Comment on attachment 51771 [details] [diff] [review]
newer patch

sr=mscott
Attachment #51771 - Flags: superreview+
(Assignee)

Comment 18

17 years ago
check in to the trunk
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 19

17 years ago
Build: 2001-10-31-03-trunk(WIN), 2001-10-31-08-trunk(MAC),
2001-10-31-08-trunk(LINUX)

Looks good on all platforms.  Marking Verified.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.