Diskq: fix restart disk-queue #1886

gaborznagy · 2018-02-15T16:07:32Z

If a pop operation fails during reading from the disk-queue file the restart_corrupted method is called.
It has to close the current disk-queue file and rename it to ".corrupted" then it has to start a new file from scratch.

The function reuses the existing QDisk object after a deinit call, although it forgets to initialize the object properly before use and that causes a crash.

Questions:

Who creates the DiskQueueOptions object and where is it passed to the QDisk object?
Is it okay to get a reference of the options member?

Probably fixes crash in #1770 (but not the corruption of the disk-queue).

kira-syslogng · 2018-02-15T16:21:37Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

furiel

Looks good to me. Two minor comments.

furiel · 2018-02-16T14:51:28Z

modules/diskq/tests/test_diskq.c

+      disk_queue->restart_corrupted(disk_queue);
+
+      struct stat file_stat;
+      stat(filename, &file_stat);


Can you assert for the return value? It would support the Enable warnings PR.

furiel · 2018-02-16T14:53:36Z

modules/diskq/qdisk.c

@@ -965,6 +963,12 @@ qdisk_reset_file_if_possible(QDisk *self)
    }
 }

+DiskQueueOptions *
+qdisk_get_options_reference(QDisk *self)


From the name I would expect DiskQueueOptions is refcounted and this function increases the reference before returning. I suggest just calling qdisk_get_options.

I had problem naming this function. 😄
I am ok with qdisk_get_options.

+1 for qdisk_get_options.

Okay, after our discussion I agree.

MrAnno · 2018-02-22T15:45:23Z

Who creates the DiskQueueOptions object and where is it passed to the QDisk object?

DiskQueueOptions is owned by DiskQDestPlugin.
QDisk holds a reference that is passed via qdisk_init in the constructors of LogQueueDiskNonReliable and LogQueueDiskReliable.

Is it okay to get a reference of the options member?

I think it's OK. Otherwise, it wouldn't be easy to get the previous options instance in the middle of logqueue-disk.c.

MrAnno · 2018-02-22T15:48:15Z

modules/diskq/tests/test_diskq.c

-  LogQueue *q;
-  guint32 num;
-  guint32 single_msg_size;
-} feed_and_assert_t;


MrAnno · 2018-02-22T15:51:06Z

modules/diskq/tests/test_diskq.c

+    }
+}
+#undef TEST_DISK_BUF_SIZE
+#undef TEST_MEM_SIZE


I wouldn't unset these macros.

Why not simple use a const variable ?

TEST_MEM_SIZE is not even used for assertion as checking disk_buf_size is good enough.
I prefer macros as constants, but for here the const var is good too.
I have changed it to const var. and moved it inside the function.

MrAnno · 2018-02-22T15:53:50Z

modules/diskq/tests/test_diskq.c

+    {"test-diskq-restart.rqf", log_queue_disk_reliable_new, TRUE},
+  };
+
+  gint number_of_variants = sizeof(parameters)/sizeof(restart_test_parameters);


Please use gsize.

kira-syslogng · 2018-02-23T16:30:44Z

Build FAILURE, the tests were executed on test branch: master and test suite: functions

gaborznagy · 2018-02-26T08:28:54Z

@kira-syslogng retest this please

kira-syslogng · 2018-02-26T08:38:43Z

Build FAILURE, the tests were executed on test branch: master and test suite: functions

gaborznagy · 2018-02-26T10:27:31Z

@kira-syslogng retest this please

kira-syslogng · 2018-02-26T10:43:26Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

furiel · 2018-02-26T11:14:51Z

If style check is fixed, it is ready to merge.

Signed-off-by: Gabor Nagy <gabor.nagy@balabit.com>

As QDisk objects can be reused if a LogQueueDisk child-class creates a new disk-queue file (known as restart), QDisk object should be properly initialized before the QDisk object would be first used (in qdisk_start. i.e. init_instance functionality is missing. For this the `file_size` member is initialized in qdisk_init and qdisk_start. Signed-off-by: Gabor Nagy <gabor.nagy@balabit.com>

In restart_diskq QDisk object is reused without re-initialization. Signed-off-by: Gabor Nagy <gabor.nagy@balabit.com>

Signed-off-by: Gabor Nagy <gabor.nagy@balabit.com>

kira-syslogng · 2018-02-26T11:55:17Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

gaborznagy added the in progress label Feb 15, 2018

gaborznagy added needs-review and removed in progress labels Feb 16, 2018

furiel reviewed Feb 16, 2018

View reviewed changes

MrAnno reviewed Feb 22, 2018

View reviewed changes

modules/diskq/tests/test_diskq.c

LogQueue *q;

guint32 num;

guint32 single_msg_size;

} feed_and_assert_t;

Copy link

Collaborator

MrAnno Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. 👍

MrAnno reviewed Feb 22, 2018

View reviewed changes

gaborznagy force-pushed the diskq-fix-restart branch from 2275bf7 to e5ff74e Compare February 23, 2018 16:19

MrAnno approved these changes Feb 23, 2018

View reviewed changes

Gabor Nagy added 5 commits February 26, 2018 12:28

diskq: remove unused part from test

6b97565

Signed-off-by: Gabor Nagy <gabor.nagy@balabit.com>

diskq: fix restart functionality

8e9ec5c

In restart_diskq QDisk object is reused without re-initialization. Signed-off-by: Gabor Nagy <gabor.nagy@balabit.com>

diskq: remove obsolete object members

4de31cc

Signed-off-by: Gabor Nagy <gabor.nagy@balabit.com>

diskq: add unit test for restart

e4446ec

Signed-off-by: Gabor Nagy <gabor.nagy@balabit.com>

gaborznagy force-pushed the diskq-fix-restart branch from e5ff74e to e4446ec Compare February 26, 2018 11:28

furiel merged commit 8a7298a into syslog-ng:master Feb 28, 2018

lbudai removed the needs-review label Feb 28, 2018

gaborznagy mentioned this pull request Mar 22, 2018

Diskq: Fix crash when corrupt disk-queue is read (during move) #1924

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diskq: fix restart disk-queue #1886

Diskq: fix restart disk-queue #1886

gaborznagy commented Feb 15, 2018 •

edited

kira-syslogng commented Feb 15, 2018

furiel left a comment

furiel Feb 16, 2018

furiel Feb 16, 2018

gaborznagy Feb 16, 2018 •

edited

MrAnno Feb 22, 2018

gaborznagy Feb 23, 2018

MrAnno commented Feb 22, 2018

MrAnno Feb 22, 2018

MrAnno Feb 22, 2018

Kokan Feb 22, 2018

gaborznagy Feb 23, 2018

MrAnno Feb 22, 2018

kira-syslogng commented Feb 23, 2018

gaborznagy commented Feb 26, 2018

kira-syslogng commented Feb 26, 2018

gaborznagy commented Feb 26, 2018

kira-syslogng commented Feb 26, 2018

furiel commented Feb 26, 2018

kira-syslogng commented Feb 26, 2018

Diskq: fix restart disk-queue #1886

Diskq: fix restart disk-queue #1886

Conversation

gaborznagy commented Feb 15, 2018 • edited

kira-syslogng commented Feb 15, 2018

furiel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborznagy Feb 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrAnno commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kira-syslogng commented Feb 23, 2018

gaborznagy commented Feb 26, 2018

kira-syslogng commented Feb 26, 2018

gaborznagy commented Feb 26, 2018

kira-syslogng commented Feb 26, 2018

furiel commented Feb 26, 2018

kira-syslogng commented Feb 26, 2018

gaborznagy commented Feb 15, 2018 •

edited

gaborznagy Feb 16, 2018 •

edited