| Message ID | pull.1537.v2.git.git.1701699574054.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] This PR enables a successful git build on z/OS. | expand |
On Mon, Dec 4, 2023 at 9:19 AM Haritha via GitGitGadget <gitgitgadget@gmail.com> wrote: > Rename functions like "release" and "fetch" > due to conflict in z/OS standard C libraries. > Also disables autoconversion facility on z/OS > and relies on iconv. > New files created in binary format are also > tagged as binary. > > Signed-off-by: Haritha D <harithamma.d@ibm.com> > --- > Enabling z/OS workflow for git > > z/OS is an IBM mainframe operating system, also known as OS/390. Our > team has been actively involved in porting Git to z/OS and we have made > significant modifications to facilitate this process. The patch below is > the initial configuration for z/OS. I also have few follow up changes > and I will send that after these changes are approved. Please let me > know if there are any concerns. It's fairly unlikely that this patch will be accepted as-is. Please see brian's[1] and Junio's[2] valuable review comments in response to your v1. They contain important suggestions which will give you a better chance of landing these changes. Generally speaking, the patch's commit message lacks sufficient detail to allow a reviewer (or future reader) to understand why the changes are being made. Moreover, this single patch seems to be addressing at least three separate issues, hence should be split into three or more patches, each standalone and tackling a single issue, and each easily digested by a reviewer. The commit message of each patch should fully explain and justify the changes made by the patch, keeping in mind that most reviewers probably aren't familiar with z/OS, thus will need extra hand-holding. More below... [1]: https://lore.kernel.org/git/ZVKrWSv7JguKTSYw@tapette.crustytoothpaste.net/ [2]: https://lore.kernel.org/git/xmqqcywd2m9i.fsf@gitster.g/ > diff --git a/builtin/archive.c b/builtin/archive.c > @@ -14,6 +14,15 @@ > static void create_output_file(const char *output_file) > { > int output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666); > +#ifdef __MVS__ > + /* > + * Since the data is in binary format, > + * we need to set the z/OS file tag > + * to binary to disable autoconversion > + */ > + if (__setfdbinary(output_fd)) > + die_errno(_("could not tag archive file '%s'"), output_file); > +#endif As mentioned in an earlier review, the project generally doesn't want #ifdef's littering the code and prefer that this sort of platform-specific specialization be wrapped up in its own "compat" file/function. For instance, perhaps you could create a platform-specific specialization of xopen() and then `#define xopen` to reference your specialized version. Your custom xopen() might first call the xopen() which Git defines and then perform whatever extra special work is needed for your platform. That way, you would not have to muck around either in the code which calls xopen() or in the Git-supplied xopen(). See, for example, how git-compat-util.h overriedes certain functions, such as stat(), fstat(), etc. using an #undefine/#define dance. > diff --git a/combine-diff.c b/combine-diff.c > @@ -1082,6 +1082,14 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent, > + #ifdef __MVS__ > + /* > + * Disable implicit autconversion on z/os, > + * rely on conversion from iconv > + */ > + __disableautocvt(fd); > + #endif > elem->mode = canon_mode(st.st_mode); Similar comment. Try to find an abstraction which allows you to perform this specialization in a way which does not require #ifdef's within the main source code if possible. > diff --git a/fetch-negotiator.h b/fetch-negotiator.h > @@ -47,7 +47,7 @@ struct fetch_negotiator { > - void (*release)(struct fetch_negotiator *); > + void (*release_negotiator)(struct fetch_negotiator *);> diff --git a/fetch-pack.c b/fetch-pack.c > @@ -1232,7 +1232,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args, > if (negotiator) > - negotiator->release(negotiator); > + negotiator->release_negotiator(negotiator); > return ref; > } > diff --git a/git-compat-util.h b/git-compat-util.h > @@ -223,7 +223,15 @@ struct strbuf; > +#ifdef __MVS__ > + #define release stdlib_release > + #define fetch stdlib_fetch > +#endif > #include <stdlib.h> > +#ifdef __MVS__ > + #undef fetch > + #undef release > +#endif So, the problem is that z/OS is polluting the C namespace or the preprocessor namespace with names "release" and "fetch"? When we've run across this problem on other platforms, we modify git-compat-util.h or some other files in compat/ to suppress the pollution introduced by the platform headers rather than "fixing" the Git source code. For instance, if "release" and "fetch" are macros on z/OS, then you may be able to simply #undef them after pulling in whichever z/OS header defines them. If they are actual system functions (rather than macros), you may be able to employ the #undef/#define dance to rename them to something else, such as "zos_release" and "zos_fetch" _before_ including the system header which declares those functions. > diff --git a/read-cache.c b/read-cache.c > @@ -205,6 +205,14 @@ static int ce_compare_data(struct index_state *istate, > int fd = git_open_cloexec(ce->name, O_RDONLY); > if (fd >= 0) { > + #ifdef __MVS__ > + /* > + * Since the data is in binary format, > + * we need to set the z/OS file tag to > + * binary to disable autoconversion > + */ > + __disableautocvt(fd); > + #endif Same comment as above about encapsulating this in a platform-specific specialization function in compat/ rather than polluting the code with #ifdef.
Am 04.12.23 um 22:46 schrieb Eric Sunshine: > On Mon, Dec 4, 2023 at 9:19 AM Haritha via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> diff --git a/fetch-negotiator.h b/fetch-negotiator.h >> @@ -47,7 +47,7 @@ struct fetch_negotiator { >> - void (*release)(struct fetch_negotiator *); >> + void (*release_negotiator)(struct fetch_negotiator *);> diff --git a/fetch-pack.c b/fetch-pack.c >> @@ -1232,7 +1232,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args, >> if (negotiator) >> - negotiator->release(negotiator); >> + negotiator->release_negotiator(negotiator); >> return ref; >> } >> diff --git a/git-compat-util.h b/git-compat-util.h >> @@ -223,7 +223,15 @@ struct strbuf; >> +#ifdef __MVS__ >> + #define release stdlib_release >> + #define fetch stdlib_fetch >> +#endif >> #include <stdlib.h> >> +#ifdef __MVS__ >> + #undef fetch >> + #undef release >> +#endif > > So, the problem is that z/OS is polluting the C namespace or the > preprocessor namespace with names "release" and "fetch"? When we've > run across this problem on other platforms, we modify > git-compat-util.h or some other files in compat/ to suppress the > pollution introduced by the platform headers rather than "fixing" the > Git source code. For instance, if "release" and "fetch" are macros on > z/OS, then you may be able to simply #undef them after pulling in > whichever z/OS header defines them. If they are actual system > functions (rather than macros), you may be able to employ the > #undef/#define dance to rename them to something else, such as > "zos_release" and "zos_fetch" _before_ including the system header > which declares those functions. I assume that [1] and [2] link to the documentation of these functions. Both pages include the following paragraph: "To avoid infringing on the user's name space, this nonstandard function has two names. One name is prefixed with two underscore characters, and one name is not. The name without the prefix underscore characters is exposed only when using the runtime library extensions." [3] defines "runtime library extensions" and mentions the macro __EXT and LANGLVL(EXTENDED). Do you need those extensions? If you don't then perhaps turning them off avoids the name collisions without needing to change the code? René [1] https://www.ibm.com/docs/en/zos/3.1.0?topic=functions-fetch-get-load-module [2] https://www.ibm.com/docs/en/zos/3.1.0?topic=functions-release-delete-load-module [3] https://www.ibm.com/docs/en/zos/3.1.0?topic=reference-zos-cc-compiler-feature#compiler_feature__ext_lib_func
diff --git a/builtin/archive.c b/builtin/archive.c index 90761fdfee0..3b1b258e383 100644 --- a/builtin/archive.c +++ b/builtin/archive.c @@ -14,6 +14,15 @@ static void create_output_file(const char *output_file) { int output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666); +#ifdef __MVS__ + /* + * Since the data is in binary format, + * we need to set the z/OS file tag + * to binary to disable autoconversion + */ + if (__setfdbinary(output_fd)) + die_errno(_("could not tag archive file '%s'"), output_file); +#endif if (output_fd != 1) { if (dup2(output_fd, 1) < 0) die_errno(_("could not redirect output")); diff --git a/builtin/hash-object.c b/builtin/hash-object.c index 5ffec99dcea..f43450db02d 100644 --- a/builtin/hash-object.c +++ b/builtin/hash-object.c @@ -62,6 +62,14 @@ static void hash_object(const char *path, const char *type, const char *vpath, { int fd; fd = xopen(path, O_RDONLY); +#ifdef __MVS__ + /* + * Since the data being read is in binary format, + * we need to disable autoconversion for z/OS + */ + if (__setfdbinary(fd)) + die_errno("Cannot set to binary '%s'", path); +#endif hash_fd(fd, type, vpath, flags, literally); } diff --git a/combine-diff.c b/combine-diff.c index f90f4424829..3230b660371 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -1082,6 +1082,14 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent, ssize_t done; int is_file, i; + #ifdef __MVS__ + /* + * Disable implicit autconversion on z/os, + * rely on conversion from iconv + */ + __disableautocvt(fd); + #endif + elem->mode = canon_mode(st.st_mode); /* if symlinks don't work, assume symlink if all parents * are symlinks diff --git a/copy.c b/copy.c index 23d84c6c1db..f5b9828b1c9 100644 --- a/copy.c +++ b/copy.c @@ -14,6 +14,13 @@ int copy_fd(int ifd, int ofd) if (write_in_full(ofd, buffer, len) < 0) return COPY_WRITE_ERROR; } + #ifdef __MVS__ + /* + * This is to ensure that file tags are copied + * from source to destination + */ + __copyfdccsid(ifd, ofd); + #endif return 0; } diff --git a/fetch-negotiator.h b/fetch-negotiator.h index e348905a1f0..71d44102fdc 100644 --- a/fetch-negotiator.h +++ b/fetch-negotiator.h @@ -14,7 +14,7 @@ struct repository; * Then, when "have" lines are required, call next(). Call ack() to report what * the server tells us. * - * Once negotiation is done, call release(). The negotiator then cannot be used + * Once negotiation is done, call release_negotiator(). The negotiator then cannot be used * (unless reinitialized with fetch_negotiator_init()). */ struct fetch_negotiator { @@ -47,7 +47,7 @@ struct fetch_negotiator { */ int (*ack)(struct fetch_negotiator *, struct commit *); - void (*release)(struct fetch_negotiator *); + void (*release_negotiator)(struct fetch_negotiator *); /* internal use */ void *data; diff --git a/fetch-pack.c b/fetch-pack.c index 26999e3b659..c1f2e714f8e 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1232,7 +1232,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args, all_done: if (negotiator) - negotiator->release(negotiator); + negotiator->release_negotiator(negotiator); return ref; } @@ -1853,7 +1853,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, die("fsck failed"); if (negotiator) - negotiator->release(negotiator); + negotiator->release_negotiator(negotiator); oidset_clear(&common); return ref; diff --git a/git-compat-util.h b/git-compat-util.h index 3e7a59b5ff1..be4516fa64e 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -223,7 +223,15 @@ struct strbuf; #include <sys/stat.h> #include <fcntl.h> #include <stddef.h> +#ifdef __MVS__ + #define release stdlib_release + #define fetch stdlib_fetch +#endif #include <stdlib.h> +#ifdef __MVS__ + #undef fetch + #undef release +#endif #include <stdarg.h> #include <string.h> #ifdef HAVE_STRINGS_H diff --git a/negotiator/default.c b/negotiator/default.c index 9a5b6963272..b1f9f153372 100644 --- a/negotiator/default.c +++ b/negotiator/default.c @@ -174,7 +174,7 @@ static int ack(struct fetch_negotiator *n, struct commit *c) return known_to_be_common; } -static void release(struct fetch_negotiator *n) +static void release_negotiator(struct fetch_negotiator *n) { clear_prio_queue(&((struct negotiation_state *)n->data)->rev_list); FREE_AND_NULL(n->data); @@ -187,7 +187,7 @@ void default_negotiator_init(struct fetch_negotiator *negotiator) negotiator->add_tip = add_tip; negotiator->next = next; negotiator->ack = ack; - negotiator->release = release; + negotiator->release_negotiator = release_negotiator; negotiator->data = CALLOC_ARRAY(ns, 1); ns->rev_list.compare = compare_commits_by_commit_date; diff --git a/negotiator/noop.c b/negotiator/noop.c index de39028ab7f..b2d555e0fec 100644 --- a/negotiator/noop.c +++ b/negotiator/noop.c @@ -30,7 +30,7 @@ static int ack(struct fetch_negotiator *n UNUSED, struct commit *c UNUSED) return 0; } -static void release(struct fetch_negotiator *n UNUSED) +static void release_negotiator (struct fetch_negotiator *n UNUSED) { /* nothing to release */ } @@ -41,6 +41,6 @@ void noop_negotiator_init(struct fetch_negotiator *negotiator) negotiator->add_tip = add_tip; negotiator->next = next; negotiator->ack = ack; - negotiator->release = release; + negotiator->release_negotiator = release_negotiator; negotiator->data = NULL; } diff --git a/negotiator/skipping.c b/negotiator/skipping.c index 5b91520430c..783b3f27e63 100644 --- a/negotiator/skipping.c +++ b/negotiator/skipping.c @@ -243,7 +243,7 @@ static int ack(struct fetch_negotiator *n, struct commit *c) return known_to_be_common; } -static void release(struct fetch_negotiator *n) +static void release_negotiator(struct fetch_negotiator *n) { clear_prio_queue(&((struct data *)n->data)->rev_list); FREE_AND_NULL(n->data); @@ -256,7 +256,7 @@ void skipping_negotiator_init(struct fetch_negotiator *negotiator) negotiator->add_tip = add_tip; negotiator->next = next; negotiator->ack = ack; - negotiator->release = release; + negotiator->release_negotiator = release_negotiator; negotiator->data = CALLOC_ARRAY(data, 1); data->rev_list.compare = compare; diff --git a/read-cache.c b/read-cache.c index 080bd39713b..b7189c58144 100644 --- a/read-cache.c +++ b/read-cache.c @@ -205,6 +205,14 @@ static int ce_compare_data(struct index_state *istate, int fd = git_open_cloexec(ce->name, O_RDONLY); if (fd >= 0) { + #ifdef __MVS__ + /* + * Since the data is in binary format, + * we need to set the z/OS file tag to + * binary to disable autoconversion + */ + __disableautocvt(fd); + #endif struct object_id oid; if (!index_fd(istate, &oid, fd, st, OBJ_BLOB, ce->name, 0)) match = !oideq(&oid, &ce->oid);