32 bit systems using 'time_t' will break in the year 2038, so we modify the code appropriately.
This patch removes the cast to 'time_t' in the assignment statement since we are eventually removing the time_t definition from the kernel as an effort to solve the y2038 problem.
This change impacts the layout of the structure retrieving profile data as it is being used in a vendor specific command that can get sent from user space and thus requires change in the ioctl interface.
Signed-off-by: Amitoj Kaur Chawla amitoj1606@gmail.com --- drivers/scsi/bfa/bfa_fcpim.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/bfa/bfa_fcpim.c b/drivers/scsi/bfa/bfa_fcpim.c index 6730340..e5c211f 100644 --- a/drivers/scsi/bfa/bfa_fcpim.c +++ b/drivers/scsi/bfa/bfa_fcpim.c @@ -1478,7 +1478,7 @@ bfa_itnim_get_ioprofile(struct bfa_itnim_s *itnim, return BFA_STATUS_IOPROFILE_OFF;
itnim->ioprofile.index = BFA_IOBUCKET_MAX; - itnim->ioprofile.io_profile_start_time = (u32)(time_t) + itnim->ioprofile.io_profile_start_time = (u32) bfa_io_profile_start_time(itnim->bfa); itnim->ioprofile.clock_res_mul = bfa_io_lat_clock_res_mul; itnim->ioprofile.clock_res_div = bfa_io_lat_clock_res_div;
On Thursday 12 November 2015 19:03:41 Amitoj Kaur Chawla wrote:
32 bit systems using 'time_t' will break in the year 2038, so we modify the code appropriately.
This patch removes the cast to 'time_t' in the assignment statement since we are eventually removing the time_t definition from the kernel as an effort to solve the y2038 problem.
This change impacts the layout of the structure retrieving profile data as it is being used in a vendor specific command that can get sent from user space and thus requires change in the ioctl interface.
Signed-off-by: Amitoj Kaur Chawla amitoj1606@gmail.com
I think in this case, while the change is not necessarily wrong (it has no effect on the generated binary), we really want a code comment at the point where the overflow happens.
Aside from that, the description seems misleading: the patch does not /solve/ impact the layout of the structure but instead hides the fact that the code is still broken, by avoiding the build error that we get for the incorrect code.
What the changelog should get across here is that the maintainers have to make a choice between two scenarios:
a) accept this patch, and live with the fact that the driver will silently break and return an incorrect value in io_profile_start_time at some point in the future (2038 or 2106, depending on whether user space interprets it as signed or unsigned). This may or may not be acceptable.
b) Not take this patch, and get a compile error at some point in the future for any configuration that intentionally removes the time_t definition in order to find broken code. They can then add a new command that has an extended time format to fix the issue properly.
diff --git a/drivers/scsi/bfa/bfa_fcpim.c b/drivers/scsi/bfa/bfa_fcpim.c index 6730340..e5c211f 100644 --- a/drivers/scsi/bfa/bfa_fcpim.c +++ b/drivers/scsi/bfa/bfa_fcpim.c @@ -1478,7 +1478,7 @@ bfa_itnim_get_ioprofile(struct bfa_itnim_s *itnim, return BFA_STATUS_IOPROFILE_OFF; itnim->ioprofile.index = BFA_IOBUCKET_MAX;
- itnim->ioprofile.io_profile_start_time = (u32)(time_t)
- itnim->ioprofile.io_profile_start_time = (u32) bfa_io_profile_start_time(itnim->bfa); itnim->ioprofile.clock_res_mul = bfa_io_lat_clock_res_mul; itnim->ioprofile.clock_res_div = bfa_io_lat_clock_res_div;
On Tue, Nov 17, 2015 at 2:28 AM, Arnd Bergmann arnd@arndb.de wrote:
I think in this case, while the change is not necessarily wrong (it has no effect on the generated binary), we really want a code comment at the point where the overflow happens.
Aside from that, the description seems misleading: the patch does not /solve/ impact the layout of the structure but instead hides the fact that the code is still broken, by avoiding the build error that we get for the incorrect code.
What the changelog should get across here is that the maintainers have to make a choice between two scenarios:
a) accept this patch, and live with the fact that the driver will silently break and return an incorrect value in io_profile_start_time at some point in the future (2038 or 2106, depending on whether user space interprets it as signed or unsigned). This may or may not be acceptable.
b) Not take this patch, and get a compile error at some point in the future for any configuration that intentionally removes the time_t definition in order to find broken code. They can then add a new command that has an extended time format to fix the issue properly.
diff --git a/drivers/scsi/bfa/bfa_fcpim.c b/drivers/scsi/bfa/bfa_fcpim.c index 6730340..e5c211f 100644 --- a/drivers/scsi/bfa/bfa_fcpim.c +++ b/drivers/scsi/bfa/bfa_fcpim.c @@ -1478,7 +1478,7 @@ bfa_itnim_get_ioprofile(struct bfa_itnim_s *itnim, return BFA_STATUS_IOPROFILE_OFF;
itnim->ioprofile.index = BFA_IOBUCKET_MAX;
itnim->ioprofile.io_profile_start_time = (u32)(time_t)
itnim->ioprofile.io_profile_start_time = (u32) bfa_io_profile_start_time(itnim->bfa); itnim->ioprofile.clock_res_mul = bfa_io_lat_clock_res_mul; itnim->ioprofile.clock_res_div = bfa_io_lat_clock_res_div;
So in addition to removing the cast I'm adding a comment where io_profile_start_time is declared that the variable would overflow in 2106 or 2038.
Also the changelog would look something like:
32 bit systems using 'time_t' will break in the year 2038, so we modify the code appropriately.
This patch removes the cast to 'time_t' in the assignment statement since we are eventually removing the time_t definition from the kernel as an effort to solve the y2038 problem. This change will avoid the build error but the code is still broken and requires a change in the ioctl interface.
Further, a comment has been added to the declaration of io_profile_start_time since the variable will break in 2038 or 2106 depending on user space interpreting it as signed or unsigned.
On Wednesday 18 November 2015 19:57:12 Amitoj Kaur Chawla wrote:
diff --git a/drivers/scsi/bfa/bfa_fcpim.c b/drivers/scsi/bfa/bfa_fcpim.c index 6730340..e5c211f 100644 --- a/drivers/scsi/bfa/bfa_fcpim.c +++ b/drivers/scsi/bfa/bfa_fcpim.c @@ -1478,7 +1478,7 @@ bfa_itnim_get_ioprofile(struct bfa_itnim_s *itnim, return BFA_STATUS_IOPROFILE_OFF;
itnim->ioprofile.index = BFA_IOBUCKET_MAX;
itnim->ioprofile.io_profile_start_time = (u32)(time_t)
itnim->ioprofile.io_profile_start_time = (u32) bfa_io_profile_start_time(itnim->bfa); itnim->ioprofile.clock_res_mul = bfa_io_lat_clock_res_mul; itnim->ioprofile.clock_res_div = bfa_io_lat_clock_res_div;So in addition to removing the cast I'm adding a comment where io_profile_start_time is declared that the variable would overflow in 2106 or 2038.
I was thinking of putting the comment inside of the bfa_itnim_get_ioprofile function, but you could also do both and make one of them a short reference to the other.
Also the changelog would look something like:
32 bit systems using 'time_t' will break in the year 2038, so we modify the code appropriately.
This patch removes the cast to 'time_t' in the assignment statement since we are eventually removing the time_t definition from the kernel as an effort to solve the y2038 problem. This change will avoid the build error but the code is still broken and requires a change in the ioctl interface.
Further, a comment has been added to the declaration of io_profile_start_time since the variable will break in 2038 or 2106 depending on user space interpreting it as signed or unsigned.
I would still add a sentence like this below the --- line:
Only apply this patch if it's seen as acceptable that the io_profile_start_time remains truncated to 32 bits in IOCMD_ITNIM_GET_IOPROFILE. If this is something that needs to be fixed by adding a replacement vendor command, leave the cast in place as a reminder.
Arnd
-
Amitoj Kaur Chawla -
Arnd Bergmann