On Thu, Nov 5, 2020 at 2:13 PM Kyotaro Horiguchi <horikyota....@gmail.com> wrote: > > At Thu, 5 Nov 2020 11:48:24 +0530, Amit Kapila <amit.kapil...@gmail.com> > wrote in > > On Thu, Nov 5, 2020 at 9:44 AM Masahiko Sawada <sawada.m...@gmail.com> > > wrote: > > > > > > On Thu, Nov 5, 2020 at 11:18 AM Kyotaro Horiguchi > > > <horikyota....@gmail.com> wrote: > > > > As another issue, just replace memcpy with strlcpy makes compiler > > > > complain of type mismatch, as the first paramter to memcpy had an > > > > needless "&" operator. I removed it in this patch. > > > > > > > > (&msg.m_slotname is a "char (*)[NAMEDATALEN]", not a "char *".) > > > > > > > > > > The patch looks good to me. > > > > > > > LGTM as well but the proposed commit message seems to be a bit > > unclear. How about something like this: > > "Use strlcpy instead of memcpy for copying the slot name in pgstat.c. > > > > There is no outright bug here but it is better to be consistent with > > the usage at other places in the same file. In the passing, fix a wrong > > Assertion in pgstat_recv_replslot." > > Looks better, thanks. > > By the way, I noticed the following sequence. > > pgstat.c: 3204 > 3204> lbeentry.st_appname[0] = '\0'; > 3205> if (MyProcPort && MyProcPort->remote_hostname) > 3206> strlcpy(lbeentry.st_clienthostname, > MyProcPort->remote_hostname, > 3207> NAMEDATALEN); > 3208> else > 3209> lbeentry.st_clienthostname[0] = '\0'; > 3210> lbeentry.st_activity_raw[0] = '\0'; > 3211> /* Also make sure the last byte in each string area is always 0 */ > 3212> lbeentry.st_appname[NAMEDATALEN - 1] = '\0'; > 3213> lbeentry.st_clienthostname[NAMEDATALEN - 1] = '\0'; > 3214> lbeentry.st_activity_raw[pgstat_track_activity_query_size - 1] = '\0'; > > > The strlcpy at the line 3206 makes sure that st_clienthostname is > null-terminated so it's nonsense to do line 3213. st_appname and > st_activity_raw are set to zero-length string. > > Is there any point in setting terminating nul to them? >
I also don't see any reason for the same except being extra careful. This is not directly related to this patch so I think we can leave this or if you want you can discuss this in a separate thread. It seems to be introduced in commit 85ccb689. -- With Regards, Amit Kapila.
