[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-swing-dev
Subject: Re: <Swing Dev> [13] RFR: JDK-8219914: Change the environment variable for Java Access Bridge loggin
From: Krishna Addepalli <krishna.addepalli () oracle ! com>
Date: 2019-04-01 6:35:51
Message-ID: EBBA6192-EAD4-438B-B1FF-0B74C839CDB3 () oracle ! com
[Download RAW message or body]
Hi Phil,
Thanks for the review. I have raised a CSR for the same.
Please review it. Here is the link: https://bugs.openjdk.java.net/browse/JDK-8221680 \
<https://bugs.openjdk.java.net/browse/JDK-8221680>
Thanks,
Krishna
> On 29-Mar-2019, at 2:27 AM, Philip Race <philip.race@oracle.com> wrote:
>
>
> Ok, fair enough
>
> +1
>
> -phil.
>
> On 3/27/19, 11:21 PM, Krishna Addepalli wrote:
> >
> > No it is not. When I'm adding "+1" and "+5", it simply adds that many number of \
> > characters to the size. The sizeof(char) can be more than 1, and this statement \
> > at line 51 (filePath = new char[filePathSize]), takes care of allocating \
> > sufficient number of bytes, which accommodate "filePathSize" number of \
> > characters. The problem is with memcpy and memset functions, which expect the \
> > size of the buffer to be provided in terms of bytes, which is why, we need the \
> > multiplier only in those functions.
> > Even I'm in favor of getting rid of them, but unfortunately I cannot std::string \
> > in this case, which would have vastly simplified the code, and avoided this \
> > manual calculation of sizes, but that results in compiler errors.
> > Thanks,
> > Krishna
> >
> > From: Phil Race
> > Sent: Thursday, March 28, 2019 3:11 AM
> > To: Krishna Addepalli <krishna.addepalli@oracle.com> \
> > <mailto:krishna.addepalli@oracle.com>; swing-dev@openjdk.java.net \
> > <mailto:swing-dev@openjdk.java.net>
> > Subject: Re: <Swing Dev> [13] RFR: JDK-8219914: Change the environment variable \
> > for Java Access Bridge logging to have a directory.
> > Hi,
> >
> > It is still inconsistent. the "+1" and "+5" logic relies on the answer to \
> > sizeof(char) being 1. So either get rid of all those calls or use sizeof('/') and \
> > sizeof(".logX"); instead of 1 and 5.
> > I'd get rid of them ....
> >
> > -phil
> >
> > On 3/27/19 1:41 AM, Krishna Addepalli wrote:
> > Hi Phil,
> >
> > Thanks for the review.
> > I have changed the variable to "JAVA_ACCESSBRIDGE_LOGDIR".
> >
> > Yes, ‘/' file separator character works on windows. I have used it in the past \
> > and have also currently tested it on my machine and it works.
> > I have added the multiplier "sizeof(char)" for all memcpy and memset lines in the \
> > code, to keep it consistent. Thanks for pointing that out.
> > Here is the link to the webrev: \
> > http://cr.openjdk.java.net/~kaddepalli/8219914/webrev01/ \
> > <http://cr.openjdk.java.net/%7Ekaddepalli/8219914/webrev01/>
> > Thanks,
> > Krishna
> >
> > From: Phil Race
> > Sent: Tuesday, March 26, 2019 10:42 PM
> > To: Krishna Addepalli <krishna.addepalli@oracle.com> \
> > <mailto:krishna.addepalli@oracle.com>; swing-dev@openjdk.java.net \
> > <mailto:swing-dev@openjdk.java.net>
> > Subject: Re: <Swing Dev> [13] RFR: JDK-8219914: Change the environment variable \
> > for Java Access Bridge logging to have a directory.
> > Can we just call it JAVA_ACCESSBRIDGE_LOGDIR ?
> >
> >
> > filePath[envFilePathLength] = '/';
> >
> > Is this right ? Does fopen on Windows expect this unix style separator ?
> >
> >
> > 53 memcpy(filePath, envfilePath, envFilePathLength*sizeof(char));
> >
> > 56 memcpy(filePath + envFilePathLength + 1 + fileNameLength, ".log", \
> > 4*sizeof(char));
> > Interesting that you feel it necessary to use sizeof(char) when clearly the whole \
> > logic, eg see :
> >
> > 50 auto filePathSize = envFilePathLength + 1 + fileNameLength + 5; //1 \
> > for "/", 5 for ".log" and 0;
> > assumes it is 1 ...
> >
> > PrintDebugString("couldnot open file %s", filePath);
> > couldnot -> could not
> >
> > -phil.
> >
> > On 3/26/19 2:45 AM, Krishna Addepalli wrote:
> > Hi Phil,
> >
> > Per our discussion, I have changed the JAVA_ACCESSBRIDGE_LOGFILE to \
> > JAVA_ACCESSBRIDGE_LOGDIRECTORY to reflect that it accepts only a directory value \
> > in the variable. I have also changed the code in AccessBridgeDebug.cpp \
> > appropriately. So, currently, the code will look for the environment variable, \
> > which should contain path to the directory, and two log files namely \
> > "java_access_bridge.log" and "windows_access_bridge.log" will be created.
> > Link to the JDK Issue: https://bugs.openjdk.java.net/browse/JDK-8219914 \
> > <https://bugs.openjdk.java.net/browse/JDK-8219914> Here is the webrev: \
> > http://cr.openjdk.java.net/~kaddepalli/8219914/webrev00/ \
> > <http://cr.openjdk.java.net/%7Ekaddepalli/8219914/webrev00/>
> >
> > Thanks,
> > Krishna
> >
> >
[Attachment #3 (unknown)]
<html><head><meta http-equiv="Content-Type" content="text/html; \
charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; \
line-break: after-white-space;" class="">Hi Phil,<div class=""><br \
class=""></div><div class="">Thanks for the review. I have raised a CSR for the \
same. </div><div class="">Please review it. Here is the link: <a \
href="https://bugs.openjdk.java.net/browse/JDK-8221680" \
class="">https://bugs.openjdk.java.net/browse/JDK-8221680</a></div><div class=""><br \
class=""></div><div class=""><br class=""></div><div class="">Thanks,</div><div \
class="">Krishna<br class=""><div><br class=""><blockquote type="cite" class=""><div \
class="">On 29-Mar-2019, at 2:27 AM, Philip Race <<a \
href="mailto:philip.race@oracle.com" class="">philip.race@oracle.com</a>> \
wrote:</div><br class="Apple-interchange-newline"><div class=""><br \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, \
255, 255); text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); \
font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: \
normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: \
0px; text-transform: none; white-space: normal; word-spacing: 0px; \
-webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); \
text-decoration: none; float: none; display: inline !important;" class="">Ok, fair \
enough</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: \
12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; \
background-color: rgb(255, 255, 255); text-decoration: none;" class=""><br \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, \
255, 255); text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); \
font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: \
normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: \
0px; text-transform: none; white-space: normal; word-spacing: 0px; \
-webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); \
text-decoration: none; float: none; display: inline !important;" \
class="">+1</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; \
font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; \
background-color: rgb(255, 255, 255); text-decoration: none;" class=""><br \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, \
255, 255); text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); \
font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: \
normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: \
0px; text-transform: none; white-space: normal; word-spacing: 0px; \
-webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); \
text-decoration: none; float: none; display: inline !important;" \
class="">-phil.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; \
font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; \
background-color: rgb(255, 255, 255); text-decoration: none;" class=""><br \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, \
255, 255); text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); \
font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: \
normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: \
0px; text-transform: none; white-space: normal; word-spacing: 0px; \
-webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); \
text-decoration: none; float: none; display: inline !important;" class="">On 3/27/19, \
11:21 PM, Krishna Addepalli wrote:</span><blockquote \
cite="mid:d0709935-fd27-4b42-945e-299f740c066f@default" type="cite" \
style="font-family: Helvetica; font-size: 12px; font-style: normal; \
font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: \
auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; \
widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; \
-webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); \
text-decoration: none;" class=""><div class="WordSection1" style="page: \
WordSection1;"><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: \
Calibri, sans-serif;" class=""><span style="color: rgb(31, 73, 125);" class="">No it \
is not. When I'm adding "+1" and "+5", it simply adds that many number of characters \
to the size.<o:p class=""></o:p></span></div><div style="margin: 0in 0in 0.0001pt; \
font-size: 11pt; font-family: Calibri, sans-serif;" class=""><span style="color: \
rgb(31, 73, 125);" class="">The sizeof(char) can be more than 1, and this statement \
at line 51 (filePath = new char[filePathSize]), takes care of allocating sufficient \
number of bytes, which accommodate "filePathSize" number of characters.<o:p \
class=""></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; \
font-family: Calibri, sans-serif;" class=""><span style="color: rgb(31, 73, 125);" \
class="">The problem is with memcpy and memset functions, which expect the size of \
the buffer to be provided in terms of bytes, which is why, we need the multiplier \
only in those functions.<o:p class=""></o:p></span></div><div style="margin: 0in 0in \
0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><span \
style="color: rgb(31, 73, 125);" class=""><o:p class=""> </o:p></span></div><div \
style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" \
class=""><span style="color: rgb(31, 73, 125);" class="">Even I'm in favor of getting \
rid of them, but unfortunately I cannot std::string in this case, which would have \
vastly simplified the code, and avoided this manual calculation of sizes, but that \
results in compiler errors.<o:p class=""></o:p></span></div><div style="margin: 0in \
0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><span \
style="color: rgb(31, 73, 125);" class=""><o:p class=""> </o:p></span></div><div \
style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" \
class=""><span style="color: rgb(31, 73, 125);" class="">Thanks,<o:p \
class=""></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; \
font-family: Calibri, sans-serif;" class=""><span style="color: rgb(31, 73, 125);" \
class="">Krishna<o:p class=""></o:p></span></div><div style="margin: 0in 0in \
0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><span \
style="color: rgb(31, 73, 125);" class=""><o:p class=""> </o:p></span></div><div \
class=""><div style="border-style: solid none none; border-top-width: 1pt; \
border-top-color: rgb(225, 225, 225); padding: 3pt 0in 0in;" class=""><div \
style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" \
class=""><b class=""><span style="color: windowtext;" class="">From:</span></b><span \
style="color: windowtext;" class=""><span \
class="Apple-converted-space"> </span>Phil Race<span \
class="Apple-converted-space"> </span><br class=""><b class="">Sent:</b><span \
class="Apple-converted-space"> </span>Thursday, March 28, 2019 3:11 AM<br \
class=""><b class="">To:</b><span class="Apple-converted-space"> </span>Krishna \
Addepalli<span class="Apple-converted-space"> </span><a \
class="moz-txt-link-rfc2396E" href="mailto:krishna.addepalli@oracle.com" \
style="color: rgb(149, 79, 114); text-decoration: \
underline;"><krishna.addepalli@oracle.com></a>;<span \
class="Apple-converted-space"> </span><a class="moz-txt-link-abbreviated" \
href="mailto:swing-dev@openjdk.java.net" style="color: rgb(149, 79, 114); \
text-decoration: underline;">swing-dev@openjdk.java.net</a><br class=""><b \
class="">Subject:</b><span class="Apple-converted-space"> </span>Re: <Swing \
Dev> [13] RFR: JDK-8219914: Change the environment variable for Java Access Bridge \
logging to have a directory.<o:p class=""></o:p></span></div></div></div><div \
style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" \
class=""><o:p class=""> </o:p></div><p class="MsoNormal" style="margin: 0in 0in \
12pt; font-size: 11pt; font-family: Calibri, sans-serif;">Hi,<br class=""><br \
class="">It is still inconsistent. the "+1" and "+5" logic relies on the answer to \
sizeof(char) being 1.<br class="">So either get rid of all those calls or use \
sizeof('/') and sizeof(".logX"); instead of 1 and 5.<br class=""><br class="">I'd get \
rid of them ....<br class=""><br class="">-phil<span style="font-size: 12pt;" \
class=""><o:p class=""></o:p></span></p><div class=""><div style="margin: 0in 0in \
0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">On 3/27/19 \
1:41 AM, Krishna Addepalli wrote:<o:p class=""></o:p></div></div><blockquote \
style="margin-top: 5pt; margin-bottom: 5pt;" class=""><div style="margin: 0in 0in \
0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><span \
style="color: rgb(31, 73, 125);" class="">Hi Phil,</span><o:p \
class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; \
font-family: Calibri, sans-serif;" class=""><span style="color: rgb(31, 73, 125);" \
class=""> </span><o:p class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; \
font-size: 11pt; font-family: Calibri, sans-serif;" class=""><span style="color: \
rgb(31, 73, 125);" class="">Thanks for the review.</span><o:p \
class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; \
font-family: Calibri, sans-serif;" class=""><span style="color: rgb(31, 73, 125);" \
class="">I have changed the variable to "JAVA_ACCESSBRIDGE_LOGDIR".</span><o:p \
class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; \
font-family: Calibri, sans-serif;" class=""><span style="color: rgb(31, 73, 125);" \
class=""> </span><o:p class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; \
font-size: 11pt; font-family: Calibri, sans-serif;" class=""><span style="color: \
rgb(31, 73, 125);" class="">Yes, ‘/' file separator character works on windows. I \
have used it in the past and have also currently tested it on my machine and it \
works.</span><o:p class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; \
font-size: 11pt; font-family: Calibri, sans-serif;" class=""><span style="color: \
rgb(31, 73, 125);" class=""> </span><o:p class=""></o:p></div><div \
style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" \
class=""><span style="color: rgb(31, 73, 125);" class="">I have added the multiplier \
"sizeof(char)" for all memcpy and memset lines in the code, to keep it consistent. \
Thanks for pointing that out.</span><o:p class=""></o:p></div><div style="margin: 0in \
0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><span \
style="color: rgb(31, 73, 125);" class=""> </span><o:p class=""></o:p></div><div \
style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" \
class=""><span style="color: rgb(31, 73, 125);" class="">Here is the link to the \
webrev:<span class="Apple-converted-space"> </span><a moz-do-not-send="true" \
href="http://cr.openjdk.java.net/%7Ekaddepalli/8219914/webrev01/" style="color: \
rgb(149, 79, 114); text-decoration: underline;" \
class="">http://cr.openjdk.java.net/~kaddepalli/8219914/webrev01/</a></span><o:p \
class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; \
font-family: Calibri, sans-serif;" class=""><span style="color: rgb(31, 73, 125);" \
class=""> </span><o:p class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; \
font-size: 11pt; font-family: Calibri, sans-serif;" class=""><span style="color: \
rgb(31, 73, 125);" class="">Thanks,</span><o:p class=""></o:p></div><div \
style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" \
class=""><span style="color: rgb(31, 73, 125);" class="">Krishna</span><o:p \
class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; \
font-family: Calibri, sans-serif;" class=""><span style="color: rgb(31, 73, 125);" \
class=""> </span><o:p class=""></o:p></div><div class=""><div \
style="border-style: solid none none; border-top-width: 1pt; border-top-color: \
rgb(225, 225, 225); padding: 3pt 0in 0in;" class=""><div style="margin: 0in 0in \
0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><b \
class=""><span style="color: windowtext;" class="">From:</span></b><span \
style="color: windowtext;" class=""><span \
class="Apple-converted-space"> </span>Phil Race<span \
class="Apple-converted-space"> </span><br class=""><b class="">Sent:</b><span \
class="Apple-converted-space"> </span>Tuesday, March 26, 2019 10:42 PM<br \
class=""><b class="">To:</b><span class="Apple-converted-space"> </span>Krishna \
Addepalli<span class="Apple-converted-space"> </span><a moz-do-not-send="true" \
href="mailto:krishna.addepalli@oracle.com" style="color: rgb(149, 79, 114); \
text-decoration: underline;" class=""><krishna.addepalli@oracle.com></a>;<span \
class="Apple-converted-space"> </span><a moz-do-not-send="true" \
href="mailto:swing-dev@openjdk.java.net" style="color: rgb(149, 79, 114); \
text-decoration: underline;" class="">swing-dev@openjdk.java.net</a><br class=""><b \
class="">Subject:</b><span class="Apple-converted-space"> </span>Re: <Swing \
Dev> [13] RFR: JDK-8219914: Change the environment variable for Java Access Bridge \
logging to have a directory.</span><o:p class=""></o:p></div></div></div><div \
style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" \
class=""> <o:p class=""></o:p></div><pre style="margin: 0in 0in 0.0001pt; \
font-size: 10pt; font-family: "Courier New";" class=""><span \
class="changed">Can we just call it JAVA_ACCESSBRIDGE_LOGDIR ?</span><o:p \
class=""></o:p></pre><pre style="margin: 0in 0in 0.0001pt; font-size: 10pt; \
font-family: "Courier New";" class=""><span \
class="changed"> </span><o:p class=""></o:p></pre><pre style="margin: 0in 0in \
0.0001pt; font-size: 10pt; font-family: "Courier New";" class=""><span \
class="changed"> </span><o:p class=""></o:p></pre><pre style="margin: 0in 0in \
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic