YottaDB / YDB

Mirrored from https://gitlab.com/YottaDB/DB/YDB
Other
76 stars 37 forks source link

[#264] Zero pad the hexadecimal pid in the backup temporary file name to keep its length at 4-bytes even if the process_id is less than 2**24 #269

Closed nars1 closed 6 years ago

ksbhaskar commented 6 years ago

On 06/01/2018 09:58 AM, Steven E. Estes wrote:

@estess commented on this pull request.


In sr_port/mupip_backup.c https://github.com/YottaDB/YottaDB/pull/269#discussion_r192404524:

@@ -610,8 +610,12 @@ void mupip_backup(void) mubclnup(rptr, need_to_del_tempfile); mupip_exit(ustatus); }

  • / Creating temp filename here. Check if we have the required space. /
  • nbytes = SNPRINTF(tempfilename, SIZEOF(tempfilename), "%.s/%.s_%4x_XXXXXX",
  • /* Creating temp filename here. Check if we have the required space.
    • Use %04x (instead of %x) for process_id to ensure we have 4 bytes always even if process_id
    • is a small number (< 2**24 etc.) and is 0-filled on the left if needed. Makes the length
    • deterministic instead of being dependent on the process_id.
  • */
  • nbytes = SNPRINTF(tempfilename, SIZEOF(tempfilename), "%.s/%.s_%04x_XXXXXX",

Does it matter that we might be truncating a pid here? PIDs are /usually/ < 32K but the pid_t itself is an int and I thought there was a configuration option on the Linux kernel to widen the used PID field once storage passed a certain point or some such thing - can't remember. Anyway, should this be 8x instead of 4x to properly represent a 32 bit value?

[KSB] From https://elixir.bootlin.com/linux/v4.16.13/source/include/linux/threads.h

/*

– Bhaskar

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/YottaDB/YottaDB/pull/269#pullrequestreview-125178349, or mute the thread https://github.com/notifications/unsubscribe-auth/AAiqehblgUoupBd4CpEyNyAH-ygX3szRks5t4UiXgaJpZM4UWvQ-.

-- YottaDB - Rock solid. Lightning fast. Secure. Pick any three.

nars1 commented 6 years ago

For the record. Even though this pull request identifies the issue # as #264, it actually fixes #267.