Re: [spice v2 2/2] utils: Remove the LL suffix from NSEC_PER_SEC

Uri Lublin <uril@xxxxxxxxxx> · Thu, 13 Jun 2019 15:18:00 +0300

On 6/13/19 2:47 PM, Frediano Ziglio wrote:

On 6/13/19 1:23 PM, Frediano Ziglio wrote:

This constant fits in a 32 bit signed integer so it does not need the
suffix. However some of the derived constants don't so use an uint64_t
cast to avoid the long vs long long confusion (such as in print
statements).
Also some of the expressions these constants are used in may overflow so
perform the appropriate casts in those locations. This makes it clearer
that these operations are 64 bit.

Hi,

Wouldn't it be simpler to replace the LL suffix with
a prefix of (uint64_t) ?

-#define NSEC_PER_SEC      1000000000LL
+#define NSEC_PER_SEC      (uint64_t)1000000000

That way there is no worry about overflows.

Uri.


You could also use the INT64_C macro for this, but Francois is trying to do
the opposite, not have the constant 64 bit in order to make the compiler able
to decide to use 32 bit arithmetic if it wants.

He tries "to avoid the long vs long long confusion"

I agree it's easier to printf an integer than an int64_t.


Note that "LL" is signed while uint64_t is unsigned, this could change some
math causing some nasty behaviour, I remember once I changed some code "easily"
like that and I got a nice buffer overflow for free.

Yes, you're right. Can be (int64) prefix.

And people say there is no such thing as a free nice buffer overflow :)

Uri.




Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
---

v2: Cast COMMON_CLIENT_TIMEOUT to an int64_t instead of an uint64_t.

I would personally use signed for all timeouts, specifically also
DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT, MJPEG_WARMUP_TIME and
RED_STREAM_INPUT_FPS_TIMEOUT.

Beside that patch is good.


v1:
https://lists.freedesktop.org/archives/spice-devel/2019-May/049193.html

   server/common-graphics-channel.h | 2 +-
   server/dcc.h                     | 2 +-
   server/gstreamer-encoder.c       | 2 +-
   server/mjpeg-encoder.c           | 2 +-
   server/utils.h                   | 4 ++--
   server/video-stream.c            | 4 ++--
   server/video-stream.h            | 2 +-
   7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/server/common-graphics-channel.h
b/server/common-graphics-channel.h
index d23f0c695..cccd24d44 100644
--- a/server/common-graphics-channel.h
+++ b/server/common-graphics-channel.h
@@ -27,7 +27,7 @@ G_BEGIN_DECLS
   
   bool common_channel_client_config_socket(RedChannelClient *rcc);
   
-#define COMMON_CLIENT_TIMEOUT (NSEC_PER_SEC * 30)
+#define COMMON_CLIENT_TIMEOUT (((int64_t)NSEC_PER_SEC) * 30)
   
   #define TYPE_COMMON_GRAPHICS_CHANNEL common_graphics_channel_get_type()
   
diff --git a/server/dcc.h b/server/dcc.h
index 76c078bf5..da8697762 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -66,7 +66,7 @@ GType display_channel_client_get_type(void)
G_GNUC_CONST;
   #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
   #define CLIENT_PALETTE_CACHE_SIZE 128
   
-#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (NSEC_PER_SEC * 10)
+#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (((uint64_t)NSEC_PER_SEC) *
10)
   #define DISPLAY_CLIENT_RETRY_INTERVAL 10000 //micro
   
   /* Each drawable can refer to at most 3 images: src, brush and mask */
diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 5f39ed877..91dd475ac 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -556,7 +556,7 @@ static void update_next_frame_mm_time(SpiceGstEncoder
*encoder)
       /* Figure out how many frames to drop to not exceed the current bit
       rate.
        * Use nanoseconds to avoid precision loss.
        */
-    uint64_t delay_ns = -encoder->vbuffer_free * 8 * NSEC_PER_SEC /
encoder->bit_rate;
+    uint64_t delay_ns = ((uint64_t)-encoder->vbuffer_free) * 8 *
NSEC_PER_SEC / encoder->bit_rate;
       uint32_t drops = (delay_ns + period_ns - 1) / period_ns; /* round up
       */
       spice_debug("drops=%u vbuffer %d/%d", drops, encoder->vbuffer_free,
                   encoder->vbuffer_size);
diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index 4a02e7c8b..d22e7d18b 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -65,7 +65,7 @@ static const int
mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
    * are not necessarily related to mis-estimation of the bit rate, and we
    would
    * like to wait till the stream stabilizes.
    */
-#define MJPEG_WARMUP_TIME (NSEC_PER_SEC * 3)
+#define MJPEG_WARMUP_TIME (((uint64_t)NSEC_PER_SEC) * 3)
   
   /* The compressed buffer initial size. */
   #define MJPEG_INITIAL_BUFFER_SIZE (32 * 1024)
diff --git a/server/utils.h b/server/utils.h
index 54bc9d490..4bbd45dff 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -52,7 +52,7 @@ static inline int test_bit(int index, uint32_t val)
   
   typedef int64_t red_time_t;
   
-#define NSEC_PER_SEC      1000000000LL
+#define NSEC_PER_SEC      1000000000
   #define NSEC_PER_MILLISEC 1000000
   #define NSEC_PER_MICROSEC 1000
   
@@ -62,7 +62,7 @@ static inline red_time_t
spice_get_monotonic_time_ns(void)
       struct timespec time;
   
       clock_gettime(CLOCK_MONOTONIC, &time);
-    return NSEC_PER_SEC * time.tv_sec + time.tv_nsec;
+    return ((uint64_t)NSEC_PER_SEC) * time.tv_sec + time.tv_nsec;
   }
   
   #define MSEC_PER_SEC 1000
diff --git a/server/video-stream.c b/server/video-stream.c
index 4ac25af8b..7a2dca7dd 100644
--- a/server/video-stream.c
+++ b/server/video-stream.c
@@ -415,8 +415,8 @@ static void
display_channel_create_stream(DisplayChannel
*display, Drawable *dra
        * the nearest integer, for instance 24 for 23.976.
        */
       uint64_t duration = drawable->creation_time -
       drawable->first_frame_time;
-    if (duration > NSEC_PER_SEC * drawable->frames_count / MAX_FPS) {
-        stream->input_fps = (NSEC_PER_SEC * drawable->frames_count +
duration / 2) / duration;
+    if (duration > ((uint64_t)NSEC_PER_SEC) * drawable->frames_count /
MAX_FPS) {
+        stream->input_fps = (((uint64_t)NSEC_PER_SEC) *
drawable->frames_count + duration / 2) / duration;
       } else {
           stream->input_fps = MAX_FPS;
       }
diff --git a/server/video-stream.h b/server/video-stream.h
index 46b076fd7..6c18d00f7 100644
--- a/server/video-stream.h
+++ b/server/video-stream.h
@@ -34,7 +34,7 @@
   #define RED_STREAM_GRADUAL_FRAMES_START_CONDITION 0.2
   #define RED_STREAM_FRAMES_RESET_CONDITION 100
   #define RED_STREAM_MIN_SIZE (96 * 96)
-#define RED_STREAM_INPUT_FPS_TIMEOUT (NSEC_PER_SEC * 5)
+#define RED_STREAM_INPUT_FPS_TIMEOUT (((uint64_t)NSEC_PER_SEC) * 5)
   #define RED_STREAM_CHANNEL_CAPACITY 0.8
   /* the client's stream report frequency is the minimum of the 2 values
   below
   */
   #define RED_STREAM_CLIENT_REPORT_WINDOW 5 // #frames

Frediano

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel







