Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: HADOOP-10388
    • Fix Version/s: HADOOP-10388
    • Component/s: native
    • Labels:
      None

      Description

      In the pure native client, we need to implement hdfsMove and hdfsCopy. These are basically recursive copy functions (in the Java code, move is copy with a delete at the end).

      1. HADOOP-10877-pnative.002.patch
        26 kB
        Colin Patrick McCabe
      2. HADOOP-10877-pnative.001.patch
        26 kB
        Colin Patrick McCabe

        Activity

        Hide
        Colin Patrick McCabe added a comment -

        committed, thanks for the review

        Show
        Colin Patrick McCabe added a comment - committed, thanks for the review
        Hide
        Colin Patrick McCabe added a comment -

        Nit: Maybe leave out the notion of copying since this is a utility function?

        Well, move is implemented as copy + delete. This is the same as the java code. I think it's probably fine to expose the fact that we're doing a copy in our log messages... this is useful information to have when debugging when something fails. We could try to log something different for copy versus delete, but the fact is, both operations start with a copy, so the log message is just accurate in either case.

        Nit: strncmp(src->path, dst->path, src_path_len) == 0 implies equality of 'n' characters. In this case, strlen(src->path) <= strlen(dst->path). If src->path == dst->path up to 'n' characters, then dst->path will never be smaller in size than src->path.

        I guess the issue here was that we don't want to declare "subdir violation" when copying /a to /ab. Even though /a is a substring of /ab, it's not a subdirectory because there's no slash after the /a part. But we also need to handle the case where some doofus is trying to copy /a to /a, in which case there would be nothing after the /a part.

        But you're right that the second strlen isn't really needed, since we know that the length of src->path is at least the length of dst->path. I'll change it to just check for '\0' (null terminator) or '/'.

        Show
        Colin Patrick McCabe added a comment - Nit: Maybe leave out the notion of copying since this is a utility function? Well, move is implemented as copy + delete. This is the same as the java code. I think it's probably fine to expose the fact that we're doing a copy in our log messages... this is useful information to have when debugging when something fails. We could try to log something different for copy versus delete, but the fact is, both operations start with a copy, so the log message is just accurate in either case. Nit: strncmp(src->path, dst->path, src_path_len) == 0 implies equality of 'n' characters. In this case, strlen(src->path) <= strlen(dst->path). If src->path == dst->path up to 'n' characters, then dst->path will never be smaller in size than src->path. I guess the issue here was that we don't want to declare "subdir violation" when copying /a to /ab. Even though /a is a substring of /ab, it's not a subdirectory because there's no slash after the /a part. But we also need to handle the case where some doofus is trying to copy /a to /a, in which case there would be nothing after the /a part. But you're right that the second strlen isn't really needed, since we know that the length of src->path is at least the length of dst->path. I'll change it to just check for '\0' (null terminator) or '/'.
        Hide
        Abraham Elmahrek added a comment -
        • Nit: Maybe leave out the notion of copying since this is a utility function?
          +        return hadoop_lerr_alloc(EINVAL, "subdir_check: can't copy %s into "
          +                "%s, because the latter is a subdirectory of the former.",
          +                src->path, dst->path);
          
        • Nit: strncmp(src->path, dst->path, src_path_len) == 0 implies equality of 'n' characters. In this case, strlen(src->path) <= strlen(dst->path). If src->path == dst->path up to 'n' characters, then dst->path will never be smaller in size than src->path.
          +    src_path_len = strlen(src->path);
          +    if ((strncmp(src->path, dst->path, src_path_len) == 0) &&
          +            ((strlen(dst->path) <= src_path_len) ||
          +            (dst->path[src_path_len] == '/'))) {
          

        +1 though... good stuff.

        Show
        Abraham Elmahrek added a comment - Nit: Maybe leave out the notion of copying since this is a utility function? + return hadoop_lerr_alloc(EINVAL, "subdir_check: can't copy %s into " + "%s, because the latter is a subdirectory of the former." , + src->path, dst->path); Nit: strncmp(src->path, dst->path, src_path_len) == 0 implies equality of 'n' characters. In this case, strlen(src->path) <= strlen(dst->path). If src->path == dst->path up to 'n' characters, then dst->path will never be smaller in size than src->path. + src_path_len = strlen(src->path); + if ((strncmp(src->path, dst->path, src_path_len) == 0) && + ((strlen(dst->path) <= src_path_len) || + (dst->path[src_path_len] == '/'))) { +1 though... good stuff.

          People

          • Assignee:
            Colin Patrick McCabe
            Reporter:
            Colin Patrick McCabe
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development