| Message ID | 20170220135825.11430-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017.02.20 14:58:25 +0100, Takashi Iwai wrote: > send_display_send_uevent() sends two environment variable, and the > first one GVT_DISPLAY_READY is set including a new line at the end of > the string; that is obviously superfluous and wrong -- at least, it > *looks* so when you only read the code. > > However, it doesn't appear in the actual output by a (supposedly > unexpected) trick. The code uses snprintf() and truncates the string > in size 20 bytes. This makes the string as GVT_DISPLAY_READY=0 or > ...=1 including the trailing NUL-letter. That is, the '\n' found in > the format string is always cut off as a result. > > Although the code gives the correct result, it is confusing. This > patch addresses it, just removing the superfluous '\n' from the format > string for avoiding further confusion. If the argument "ready" were > not a bool, the size 20 should be corrected as well. But it's a > bool, so we can leave the magic number 20 as is for now. > > FWIW, the bug was spotted by a new GCC7 warning: > drivers/gpu/drm/i915/gvt/handlers.c: In function 'pvinfo_mmio_write': > drivers/gpu/drm/i915/gvt/handlers.c:1042:34: error: 'snprintf' output truncated before the last format character [-Werror=format-truncation=] > snprintf(display_ready_str, 20, "GVT_DISPLAY_READY=%d\n", ready); > ^~~~~~~~~~~~~~~~~~~~~~~~ > drivers/gpu/drm/i915/gvt/handlers.c:1042:2: note: 'snprintf' output 21 bytes into a destination of size 20 > snprintf(display_ready_str, 20, "GVT_DISPLAY_READY=%d\n", ready); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Fixes: 04d348ae3f0a ("drm/i915/gvt: vGPU display virtualization") > Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=1025903 > Reported-by: Richard Biener <rguenther@suse.com> > Cc: <stable@vger.kernel.org> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- Applied. Thanks! > drivers/gpu/drm/i915/gvt/handlers.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c > index 1d450627ff65..845aa1511cbf 100644 > --- a/drivers/gpu/drm/i915/gvt/handlers.c > +++ b/drivers/gpu/drm/i915/gvt/handlers.c > @@ -1039,7 +1039,7 @@ static int send_display_ready_uevent(struct intel_vgpu *vgpu, int ready) > char vmid_str[20]; > char display_ready_str[20]; > > - snprintf(display_ready_str, 20, "GVT_DISPLAY_READY=%d\n", ready); > + snprintf(display_ready_str, 20, "GVT_DISPLAY_READY=%d", ready); > env[0] = display_ready_str; > > snprintf(vmid_str, 20, "VMID=%d", vgpu->id); > -- > 2.11.1 > > _______________________________________________ > intel-gvt-dev mailing list > intel-gvt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c index 1d450627ff65..845aa1511cbf 100644 --- a/drivers/gpu/drm/i915/gvt/handlers.c +++ b/drivers/gpu/drm/i915/gvt/handlers.c @@ -1039,7 +1039,7 @@ static int send_display_ready_uevent(struct intel_vgpu *vgpu, int ready) char vmid_str[20]; char display_ready_str[20]; - snprintf(display_ready_str, 20, "GVT_DISPLAY_READY=%d\n", ready); + snprintf(display_ready_str, 20, "GVT_DISPLAY_READY=%d", ready); env[0] = display_ready_str; snprintf(vmid_str, 20, "VMID=%d", vgpu->id);
send_display_send_uevent() sends two environment variable, and the first one GVT_DISPLAY_READY is set including a new line at the end of the string; that is obviously superfluous and wrong -- at least, it *looks* so when you only read the code. However, it doesn't appear in the actual output by a (supposedly unexpected) trick. The code uses snprintf() and truncates the string in size 20 bytes. This makes the string as GVT_DISPLAY_READY=0 or ...=1 including the trailing NUL-letter. That is, the '\n' found in the format string is always cut off as a result. Although the code gives the correct result, it is confusing. This patch addresses it, just removing the superfluous '\n' from the format string for avoiding further confusion. If the argument "ready" were not a bool, the size 20 should be corrected as well. But it's a bool, so we can leave the magic number 20 as is for now. FWIW, the bug was spotted by a new GCC7 warning: drivers/gpu/drm/i915/gvt/handlers.c: In function 'pvinfo_mmio_write': drivers/gpu/drm/i915/gvt/handlers.c:1042:34: error: 'snprintf' output truncated before the last format character [-Werror=format-truncation=] snprintf(display_ready_str, 20, "GVT_DISPLAY_READY=%d\n", ready); ^~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gvt/handlers.c:1042:2: note: 'snprintf' output 21 bytes into a destination of size 20 snprintf(display_ready_str, 20, "GVT_DISPLAY_READY=%d\n", ready); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Fixes: 04d348ae3f0a ("drm/i915/gvt: vGPU display virtualization") Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=1025903 Reported-by: Richard Biener <rguenther@suse.com> Cc: <stable@vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- drivers/gpu/drm/i915/gvt/handlers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)