Re: [patch@35055] vms.c rmsexpand refactor
On Jan 2, 2009, at 10:14 AM, John Malmberg wrote: Craig A. Berry wrote: Any manual attempt to sort out what was going on was pretty much sunk by having hundreds of lines consisting only of non-significant whitespace changes -- consider using diff -b. I made the whitespace changes in order to make the code easier to follow while I was refactoring it. How about I submit a patch that does the whitespace changes only as a first step with no other bug fixes? If you can do that, isn't it just as easy to submit a patch that doesn't mess with the existing indentation? And to make things consistent, what tab character usage policy do you want? No tabs, tabs on 8 character boundaries? I prefer the tabs on 8 character boundaries. A lot of people do, but since everyone puts their tab stops in different places, that generally leads to mayhem if anyone else wants to read your code in a different editor and/or with different settings. The standard described in perlstyle.pod is to use 4-column indents with spaces, not tabs, but preserving the existing indent style trumps that as long as what's already there is done consistently. The code you are working on uses 2-column indent with considerable consistency except where tabs have snuck in here and there from various patches. IMO whatever gain there might be from changing the indent style does not offset the loss from not being able to easily see what's changed. Craig A. Berry mailto:craigbe...@mac.com ... getting out of a sonnet is much more difficult than getting in. Brad Leithauser
Re: [patch@35055] vms.c rmsexpand refactor
Craig A. Berry wrote: On Dec 7, 2008, at 11:35 PM, John E. Malmberg wrote: Craig A. Berry wrote: It may be easier to apply the patches and then compare the results against the unpatched vms.c manually. Not really, and that's not something a committer should be asked to do anyway. I've just spent over an hour trying to get this series of six refactor patches to apply, but no luck. There were six separate patches for pathify_dirspec, unixify, unixpath, vmsify, fileify, and rmsexpand. Here and there a hunk succeeded but most did not. The hunks that did apply had hundreds of lines of offset, indicating something's missing. I do not know what happened there, I set up multiple reference directories so that each patch was incremental. The order was important because of name changes to the internal routines. I fiddled with the order in which I applied them, used -l, and spent some time trying to figure out what was what before giving up. Any manual attempt to sort out what was going on was pretty much sunk by having hundreds of lines consisting only of non-significant whitespace changes -- consider using diff -b. I made the whitespace changes in order to make the code easier to follow while I was refactoring it. How about I submit a patch that does the whitespace changes only as a first step with no other bug fixes? And to make things consistent, what tab character usage policy do you want? No tabs, tabs on 8 character boundaries? I prefer the tabs on 8 character boundaries. I look forward to a revised patch that applies cleanly and does not introduce new test failures. Since submitting them, I have found more bugs in VMS.c, some of which are in the patched routines. Before resubmitting I have one more test to get running in UNIX mode to get working. And the POSIX exit code setting still has some bugs in it. There is also a bug in the pipe exit handler, it is access violating if there is a pipe open to a child. I am only seeing this while debugging, and using the EXIT command in the debugger. -John wb8...@gmail.com -- The wb8...@qsl.net and my encompasserve.org e-mail addresses are out of service for the moment.
Re: [patch@35055] vms.c rmsexpand refactor
On Dec 7, 2008, at 11:35 PM, John E. Malmberg wrote: Craig A. Berry wrote: I will look at these, test them, and -- if all goes well -- apply them, but it's likely to take me a while to get to them. In general I agree with the principle that routines that can be called very early in start-up before thread context is initialized do need to be cautious about trying to access that context. I will just make a preliminary comment that it's very hard to see what's going on by reading the patches because they consist mostly of changes to non-significant whitespace. Thanks for looking at these. I'm glad to look at them and hope to get a version I can apply. It may be easier to apply the patches and then compare the results against the unpatched vms.c manually. Not really, and that's not something a committer should be asked to do anyway. I've just spent over an hour trying to get this series of six refactor patches to apply, but no luck. There were six separate patches for pathify_dirspec, unixify, unixpath, vmsify, fileify, and rmsexpand. Here and there a hunk succeeded but most did not. The hunks that did apply had hundreds of lines of offset, indicating something's missing. I fiddled with the order in which I applied them, used -l, and spent some time trying to figure out what was what before giving up. Any manual attempt to sort out what was going on was pretty much sunk by having hundreds of lines consisting only of non-significant whitespace changes -- consider using diff -b. I look forward to a revised patch that applies cleanly and does not introduce new test failures. Craig A. Berry mailto:craigbe...@mac.com ... getting out of a sonnet is much more difficult than getting in. Brad Leithauser
[patch@35055] vms.c rmsexpand refactor
This patch makes a version of rmsexpand available for calling with out
an implicit context, which is needed for threaded perl as several
routines do not have or need an implicit context.
This is the sixth of a multi-part conversion to remove access violations
when internal perl warning and error routines, and memory allocation
routines are called with a null pointer for the implicit context.
By removing the implicit context from these routines, it will cause a
compile time error on threaded perl should one of the affected routines
sneak back into this code.
Hopefully this is the last big patch for a while.
-John
[EMAIL PROTECTED]
Personal Opinion Only
--- /ref7_root/perl/vms/vms.c Sun Dec 7 17:20:31 2008
+++ vms/vms.c Sun Dec 7 17:31:53 2008
@@ -296,6 +296,10 @@
static char *mp_do_tounixspec(pTHX_ const char *, char *, int, int *);
static char *mp_do_pathify_dirspec(pTHX_ const char *dir,char *buf, int ts,
int *);
+static char * int_rmsexpand_vms(
+const char * filespec, char * outbuf, unsigned opts);
+static char * int_rmsexpand_tovms(
+const char * filespec, char * outbuf, unsigned opts);
static char *int_tovmsspec
(const char *path, char *buf, int dir_flag, int * utf8_flag);
static char * int_fileify_dirspec(const char *dir, char *buf, int *utf8_fl);
@@ -5364,384 +5368,478 @@
static char *mp_do_tounixspec(pTHX_ const char *, char *, int, int *);
static char *
-mp_do_rmsexpand
- (pTHX_ const char *filespec,
+int_rmsexpand
+ (const char *filespec,
char *outbuf,
-int ts,
const char *defspec,
unsigned opts,
int * fs_utf8,
int * dfs_utf8)
{
- static char __rmsexpand_retbuf[VMS_MAXRSS];
- char * vmsfspec, *tmpfspec;
- char * esa, *cp, *out = NULL;
- char * tbuf;
- char * esal = NULL;
- char * outbufl;
- struct FAB myfab = cc$rms_fab;
- rms_setup_nam(mynam);
- STRLEN speclen;
- unsigned long int retsts, trimver, trimtype, haslower = 0, isunix = 0;
- int sts;
+char * ret_spec;
+const char * in_spec;
+char * spec_buf;
+const char * def_spec;
+char * vmsfspec, *vmsdefspec;
+char * esa;
+char * esal = NULL;
+char * outbufl;
+struct FAB myfab = cc$rms_fab;
+rms_setup_nam(mynam);
+STRLEN speclen;
+unsigned long int retsts, trimver, trimtype, haslower = 0, isunix = 0;
+int sts;
- /* temp hack until UTF8 is actually implemented */
- if (fs_utf8 != NULL)
-*fs_utf8 = 0;
+/* temp hack until UTF8 is actually implemented */
+if (fs_utf8 != NULL)
+*fs_utf8 = 0;
- if (!filespec || !*filespec) {
-set_vaxc_errno(LIB$_INVARG); set_errno(EINVAL);
-return NULL;
- }
- if (!outbuf) {
-if (ts) out = Newx(outbuf,VMS_MAXRSS,char);
-elseoutbuf = __rmsexpand_retbuf;
- }
+if (!filespec || !*filespec) {
+set_vaxc_errno(LIB$_INVARG); set_errno(EINVAL);
+return NULL;
+}
- vmsfspec = NULL;
- tmpfspec = NULL;
- outbufl = NULL;
-
- isunix = 0;
- if ((opts PERL_RMSEXPAND_M_VMS_IN) == 0) {
-isunix = is_unix_filespec(filespec);
-if (isunix) {
- vmsfspec = PerlMem_malloc(VMS_MAXRSS);
- if (vmsfspec == NULL) _ckvmssts(SS$_INSFMEM);
- if (do_tovmsspec(filespec,vmsfspec,0,fs_utf8) == NULL) {
- PerlMem_free(vmsfspec);
- if (out)
- Safefree(out);
- return NULL;
- }
- filespec = vmsfspec;
+vmsfspec = NULL;
+vmsdefspec = NULL;
+outbufl = NULL;
+
+in_spec = filespec;
+isunix = 0;
+if ((opts PERL_RMSEXPAND_M_VMS_IN) == 0) {
+
+/* If this is a UNIX file spec, convert it to VMS */
+isunix = is_unix_filespec(filespec);
+if (isunix) {
+char * ret_spec;
+vmsfspec = PerlMem_malloc(VMS_MAXRSS);
+if (vmsfspec == NULL)
+_ckvmssts_noperl(SS$_INSFMEM);
- /* Unless we are forcing to VMS format, a UNIX input means
- * UNIX output, and that requires long names to be used
- */
+ret_spec = int_tovmsspec(filespec, vmsfspec, 0, fs_utf8);
+if (ret_spec == NULL) {
+PerlMem_free(vmsfspec);
+return NULL;
+}
+in_spec = (const char *)vmsfspec;
+
+/* Unless we are forcing to VMS format, a UNIX input means
+ * UNIX output, and that requires long names to be used
+ */
#if !defined(__VAX) defined(NAML$C_MAXRSS)
- if ((opts PERL_RMSEXPAND_M_VMS) == 0)
- opts |= PERL_RMSEXPAND_M_LONG;
- else
+if ((opts PERL_RMSEXPAND_M_VMS) == 0)
+opts |= PERL_RMSEXPAND_M_LONG;
+else
#endif
- isunix = 0;
- }
+isunix = 0;
+}
}
- rms_set_fna(myfab, mynam, (char *)filespec, strlen(filespec)); /* cast ok */
- rms_bind_fab_nam(myfab, mynam);
+rms_set_fna(myfab, mynam, (char *)in_spec, strlen(in_spec)); /* cast ok */
+rms_bind_fab_nam(myfab, mynam);
- if (defspec *defspec) {
-int t_isunix;
-t_isunix =
Re: [patch@35055] vms.c rmsexpand refactor
On Dec 7, 2008, at 6:44 PM, John E. Malmberg wrote: This patch makes a version of rmsexpand available for calling with out an implicit context, which is needed for threaded perl as several routines do not have or need an implicit context. This is the sixth of a multi-part conversion to remove access violations when internal perl warning and error routines, and memory allocation routines are called with a null pointer for the implicit context. I will look at these, test them, and -- if all goes well -- apply them, but it's likely to take me a while to get to them. In general I agree with the principle that routines that can be called very early in start-up before thread context is initialized do need to be cautious about trying to access that context. I will just make a preliminary comment that it's very hard to see what's going on by reading the patches because they consist mostly of changes to non- significant whitespace. Craig A. Berry mailto:[EMAIL PROTECTED] ... getting out of a sonnet is much more difficult than getting in. Brad Leithauser
