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.001.patch
        26 kB
        Colin Patrick McCabe
      2. HADOOP-10877-pnative.002.patch
        26 kB
        Colin Patrick McCabe

        Activity

        Colin Patrick McCabe created issue -
        Colin Patrick McCabe made changes -
        Field Original Value New Value
        Attachment HADOOP-10877-pnative.001.patch [ 12661287 ]
        Colin Patrick McCabe made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Colin Patrick McCabe made changes -
        Attachment HADOOP-10877-pnative.001.patch [ 12661287 ]
        Colin Patrick McCabe made changes -
        Attachment HADOOP-10877-pnative.001.patch [ 12661289 ]
        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.
        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 '/'.
        Colin Patrick McCabe made changes -
        Attachment HADOOP-10877-pnative.002.patch [ 12661876 ]
        Hide
        Colin Patrick McCabe added a comment -

        committed, thanks for the review

        Show
        Colin Patrick McCabe added a comment - committed, thanks for the review
        Colin Patrick McCabe made changes -
        Status In Progress [ 3 ] Resolved [ 5 ]
        Fix Version/s HADOOP-10388 [ 12326650 ]
        Resolution Fixed [ 1 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open In Progress In Progress
        21d 36m 1 Colin Patrick McCabe 12/Aug/14 20:26
        In Progress In Progress Resolved Resolved
        2d 1h 10m 1 Colin Patrick McCabe 14/Aug/14 21:37

          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