Implement rmdir rf without FTS on Solaris

RESOLVED FIXED in mozilla13

Status

()

RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

Tracking

unspecified
mozilla13
x86
Solaris
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
Solaris doesn't have FTS library.
So I need to implement it without FTS.

My patch is mostly based on Windows version of add_dir_entries().
(Assignee)

Comment 1

8 years ago
Created attachment 548721 [details] [diff] [review]
patch
Assignee: nobody → ginn.chen
Status: NEW → ASSIGNED
Attachment #548721 - Flags: review?(robert.bugzilla)
Comment on attachment 548721 [details] [diff] [review]
patch

Brian, please take this review so I can focus on the silent update reviews. Thanks
Attachment #548721 - Flags: review?(robert.bugzilla) → review?(netzen)
Comment on attachment 548721 [details] [diff] [review]
patch

Review of attachment 548721 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay in reviewing this patch, could this be rebased to mozilla-central tip? I will review it within 24 hours after that.

::: toolkit/mozapps/update/updater/updater.cpp
@@ +137,1 @@
>  

This no longer applies, please rebase the patch for this part.
Attachment #548721 - Flags: review?(netzen)
(Assignee)

Comment 4

7 years ago
Created attachment 598097 [details] [diff] [review]
patch
Attachment #548721 - Attachment is obsolete: true
Attachment #598097 - Flags: review?(netzen)
Comment on attachment 598097 [details] [diff] [review]
patch

Review of attachment 598097 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch and sorry again for the long review time. Please remark me with r? with the following minor things and I'll r+ it.

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2330,5 @@
> +  // Remove the trailing slash so the paths don't contain double slashes. The
> +  // existence of the slash has already been checked in DoUpdate.
> +  searchpath[NS_tstrlen(searchpath) - 1] = NS_T('\0');
> +
> +  DIR* dir = opendir(searchpath);

Please do a check here for a NULL return from opendir and return early from the function.

@@ +2361,5 @@
> +
> +      Action *action = new RemoveFile();
> +      rv = action->Parse(quotedpath);
> +      if (rv) {
> +        LOG(("add_dir_entries Parse error on recurse: " LOG_S ", err: %d\n", quotedpath, rv));

nit: Line length should be at most 80 chars.

@@ +2378,5 @@
> +
> +  Action *action = new RemoveDir();
> +  rv = action->Parse(quotedpath);
> +  if (rv)
> +    LOG(("add_dir_entries Parse error on close: " LOG_S ", err: %d\n", quotedpath, rv));

nit: ditto line length.
Attachment #598097 - Flags: review?(netzen)
(Assignee)

Comment 6

7 years ago
Created attachment 598876 [details] [diff] [review]
patch
Attachment #598097 - Attachment is obsolete: true
Attachment #598876 - Flags: review?(netzen)
(Assignee)

Comment 7

7 years ago
Created attachment 598877 [details] [diff] [review]
patch

sorry, missed one file
Attachment #598876 - Attachment is obsolete: true
Attachment #598876 - Flags: review?(netzen)
Attachment #598877 - Flags: review?(netzen)
Comment on attachment 598877 [details] [diff] [review]
patch

Review of attachment 598877 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and thanks for the patch!
Please carry forward r+ and r=me with nits addressed.

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2347,5 @@
> +                 NS_T("%s%s"), dirpath, ent->d_name);
> +    struct stat64 st_buf;
> +    int test = stat64(foundpath, &st_buf);
> +    if (test) {
> +      return UNEXPECTED_ERROR;

nit: closedir(dir); before this return.

@@ +2356,5 @@
> +      // Recurse into the directory.
> +      rv = add_dir_entries(foundpath, list);
> +      if (rv) {
> +        LOG(("add_dir_entries error: " LOG_S ", err: %d\n", foundpath, rv));
> +        return rv;

nit: closedir(dir); before this return.

@@ +2362,5 @@
> +    } else {
> +      // Add the file to be removed to the ActionList.
> +      NS_tchar *quotedpath = get_quoted_path(foundpath);
> +      if (!quotedpath)
> +        return PARSE_ERROR;

nit: closedir(dir); before this return.

@@ +2369,5 @@
> +      rv = action->Parse(quotedpath);
> +      if (rv) {
> +        LOG(("add_dir_entries Parse error on recurse: " LOG_S ", err: %d\n",
> +             quotedpath, rv));
> +        return rv;

nit: closedir(dir); before this return.

@@ +2386,5 @@
> +  Action *action = new RemoveDir();
> +  rv = action->Parse(quotedpath);
> +  if (rv)
> +    LOG(("add_dir_entries Parse error on close: " LOG_S ", err: %d\n",
> +         quotedpath, rv));

nit: I prefer to have braces around anything that spans multiple lines even if it is 1 statement.

@@ +2388,5 @@
> +  if (rv)
> +    LOG(("add_dir_entries Parse error on close: " LOG_S ", err: %d\n",
> +         quotedpath, rv));
> +  else
> +    list->Append(action);

nit: and since the other block will have braces this one too.
Attachment #598877 - Flags: review?(netzen) → review+
https://hg.mozilla.org/mozilla-central/rev/1acd05deef4f
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.