674478 - Implement rmdir rf without FTS on Solaris

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Ginn Chen]

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().

Comment 1

[Ginn Chen]

Attachment: patch

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 548721 [details] [diff] [review]
patch

Brian, please take this review so I can focus on the silent update reviews. Thanks

Comment 3

[Brian R. Bondy [:bbondy]]

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.

Comment 4

[Ginn Chen]

Attachment: patch

Comment 5

[Brian R. Bondy [:bbondy]]

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.

Comment 6

[Ginn Chen]

Attachment: patch

Comment 7

[Ginn Chen]

Attachment: patch

sorry, missed one file

Comment 8

[Brian R. Bondy [:bbondy]]

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.

Comment 9

[Ginn Chen]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1acd05deef4f

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1acd05deef4f