Hello,
I think it could be useful to store the corrupted memory cache before reset. We have very little info about what was really wrong in the cache when it was in inconsistent state. This way we could ask users to send us copy of the corrupted cache if they hit this issue. It could provide some more answers.
Patch is attached. It stores the corrupted cache in /var/lib/sss/mc/<cache_name>_corrupted.
Thanks Michal
On (12/08/13 17:47), Michal Židek wrote:
Hello,
I think it could be useful to store the corrupted memory cache before reset. We have very little info about what was really wrong in the cache when it was in inconsistent state. This way we could ask users to send us copy of the corrupted cache if they hit this issue. It could provide some more answers.
Patch is attached. It stores the corrupted cache in /var/lib/sss/mc/<cache_name>_corrupted.
Thanks Michal
Please rebase patch on top of patches from thread "[PATCH] mmap_cache: Check data->name value in client code"
It cannot be applied cleanly.
LS
On 08/14/2013 04:39 PM, Lukas Slebodnik wrote:
On (12/08/13 17:47), Michal Židek wrote:
Hello,
I think it could be useful to store the corrupted memory cache before reset. We have very little info about what was really wrong in the cache when it was in inconsistent state. This way we could ask users to send us copy of the corrupted cache if they hit this issue. It could provide some more answers.
Patch is attached. It stores the corrupted cache in /var/lib/sss/mc/<cache_name>_corrupted.
Thanks Michal
Please rebase patch on top of patches from thread "[PATCH] mmap_cache: Check data->name value in client code"
It cannot be applied cleanly.
LS
Ok. New patch is attached. (Rebased on top of the patches in thread [SSSD] [PATCH] mmap_cache: Check data->name value in client code)
Thanks Michal
On (14/08/13 18:50), Michal Židek wrote:
On 08/14/2013 04:39 PM, Lukas Slebodnik wrote:
On (12/08/13 17:47), Michal Židek wrote:
Hello,
I think it could be useful to store the corrupted memory cache before reset. We have very little info about what was really wrong in the cache when it was in inconsistent state. This way we could ask users to send us copy of the corrupted cache if they hit this issue. It could provide some more answers.
Patch is attached. It stores the corrupted cache in /var/lib/sss/mc/<cache_name>_corrupted.
Thanks Michal
Please rebase patch on top of patches from thread "[PATCH] mmap_cache: Check data->name value in client code"
It cannot be applied cleanly.
LS
Ok. New patch is attached. (Rebased on top of the patches in thread [SSSD] [PATCH] mmap_cache: Check data->name value in client code)
Thanks Michal
Backup of memory maped was created (gdb cheating)
ACK
LS
On 08/15/2013 11:01 AM, Lukas Slebodnik wrote:
On (14/08/13 18:50), Michal Židek wrote:
On 08/14/2013 04:39 PM, Lukas Slebodnik wrote:
On (12/08/13 17:47), Michal Židek wrote:
Hello,
I think it could be useful to store the corrupted memory cache before reset. We have very little info about what was really wrong in the cache when it was in inconsistent state. This way we could ask users to send us copy of the corrupted cache if they hit this issue. It could provide some more answers.
Patch is attached. It stores the corrupted cache in /var/lib/sss/mc/<cache_name>_corrupted.
Thanks Michal
Please rebase patch on top of patches from thread "[PATCH] mmap_cache: Check data->name value in client code"
It cannot be applied cleanly.
LS
Ok. New patch is attached. (Rebased on top of the patches in thread [SSSD] [PATCH] mmap_cache: Check data->name value in client code)
Thanks Michal
Backup of memory maped was created (gdb cheating)
ACK
LS
Just sending rebased version.
Michal
On Thu, Aug 15, 2013 at 06:42:53PM +0200, Michal Židek wrote:
On 08/15/2013 11:01 AM, Lukas Slebodnik wrote:
On (14/08/13 18:50), Michal Židek wrote:
On 08/14/2013 04:39 PM, Lukas Slebodnik wrote:
On (12/08/13 17:47), Michal Židek wrote:
Hello,
I think it could be useful to store the corrupted memory cache before reset. We have very little info about what was really wrong in the cache when it was in inconsistent state. This way we could ask users to send us copy of the corrupted cache if they hit this issue. It could provide some more answers.
Patch is attached. It stores the corrupted cache in /var/lib/sss/mc/<cache_name>_corrupted.
Thanks Michal
Please rebase patch on top of patches from thread "[PATCH] mmap_cache: Check data->name value in client code"
It cannot be applied cleanly.
LS
Ok. New patch is attached. (Rebased on top of the patches in thread [SSSD] [PATCH] mmap_cache: Check data->name value in client code)
Thanks Michal
Backup of memory maped was created (gdb cheating)
ACK
LS
Just sending rebased version.
Michal
The patch looks good to me and I was about to push it but I wonder if we should call sss_log() to explain to the admin what happened and what are these strange files sssd created?
On 08/19/2013 09:10 PM, Jakub Hrozek wrote:
On Thu, Aug 15, 2013 at 06:42:53PM +0200, Michal Židek wrote:
On 08/15/2013 11:01 AM, Lukas Slebodnik wrote:
On (14/08/13 18:50), Michal Židek wrote:
On 08/14/2013 04:39 PM, Lukas Slebodnik wrote:
On (12/08/13 17:47), Michal Židek wrote:
Hello,
I think it could be useful to store the corrupted memory cache before reset. We have very little info about what was really wrong in the cache when it was in inconsistent state. This way we could ask users to send us copy of the corrupted cache if they hit this issue. It could provide some more answers.
Patch is attached. It stores the corrupted cache in /var/lib/sss/mc/<cache_name>_corrupted.
Thanks Michal
Please rebase patch on top of patches from thread "[PATCH] mmap_cache: Check data->name value in client code"
It cannot be applied cleanly.
LS
Ok. New patch is attached. (Rebased on top of the patches in thread [SSSD] [PATCH] mmap_cache: Check data->name value in client code)
Thanks Michal
Backup of memory maped was created (gdb cheating)
ACK
LS
Just sending rebased version.
Michal
The patch looks good to me and I was about to push it but I wonder if we should call sss_log() to explain to the admin what happened and what are these strange files sssd created?
Ok. When the copy of memcache is successfully created a sss_log with SSS_LOG_NOTICE is called. We already do enough noise in sssd logs so I think NOTICE level in syslog is sufficient.
New patch attached. Michal
On 08/19/2013 09:51 PM, Michal Židek wrote:
On 08/19/2013 09:10 PM, Jakub Hrozek wrote:
On Thu, Aug 15, 2013 at 06:42:53PM +0200, Michal Židek wrote:
On 08/15/2013 11:01 AM, Lukas Slebodnik wrote:
On (14/08/13 18:50), Michal Židek wrote:
On 08/14/2013 04:39 PM, Lukas Slebodnik wrote:
On (12/08/13 17:47), Michal Židek wrote: > Hello, > > I think it could be useful to store the corrupted memory cache > before reset. We have very little info about what was really > wrong in the cache when it was in inconsistent state. This way we > could ask users to send us copy of the corrupted cache if they > hit this issue. It could provide some more answers. > > Patch is attached. It stores the corrupted cache in > /var/lib/sss/mc/<cache_name>_corrupted. > > Thanks > Michal
Please rebase patch on top of patches from thread "[PATCH] mmap_cache: Check data->name value in client code"
It cannot be applied cleanly.
LS
Ok. New patch is attached. (Rebased on top of the patches in thread [SSSD] [PATCH] mmap_cache: Check data->name value in client code)
Thanks Michal
Backup of memory maped was created (gdb cheating)
ACK
LS
Just sending rebased version.
Michal
The patch looks good to me and I was about to push it but I wonder if we should call sss_log() to explain to the admin what happened and what are these strange files sssd created?
Ok. When the copy of memcache is successfully created a sss_log with SSS_LOG_NOTICE is called. We already do enough noise in sssd logs so I think NOTICE level in syslog is sufficient.
New patch attached. Michal
I forgot to fixup one change.. will post new patch.
On 08/19/2013 09:52 PM, Michal Židek wrote:
On 08/19/2013 09:51 PM, Michal Židek wrote:
On 08/19/2013 09:10 PM, Jakub Hrozek wrote:
On Thu, Aug 15, 2013 at 06:42:53PM +0200, Michal Židek wrote:
On 08/15/2013 11:01 AM, Lukas Slebodnik wrote:
On (14/08/13 18:50), Michal Židek wrote:
On 08/14/2013 04:39 PM, Lukas Slebodnik wrote: > On (12/08/13 17:47), Michal Židek wrote: >> Hello, >> >> I think it could be useful to store the corrupted memory cache >> before reset. We have very little info about what was really >> wrong in the cache when it was in inconsistent state. This way we >> could ask users to send us copy of the corrupted cache if they >> hit this issue. It could provide some more answers. >> >> Patch is attached. It stores the corrupted cache in >> /var/lib/sss/mc/<cache_name>_corrupted. >> >> Thanks >> Michal > > Please rebase patch on top of patches from thread > "[PATCH] mmap_cache: Check data->name value in client code" > > It cannot be applied cleanly. > > LS >
Ok. New patch is attached. (Rebased on top of the patches in thread [SSSD] [PATCH] mmap_cache: Check data->name value in client code)
Thanks Michal
Backup of memory maped was created (gdb cheating)
ACK
LS
Just sending rebased version.
Michal
The patch looks good to me and I was about to push it but I wonder if we should call sss_log() to explain to the admin what happened and what are these strange files sssd created?
Ok. When the copy of memcache is successfully created a sss_log with SSS_LOG_NOTICE is called. We already do enough noise in sssd logs so I think NOTICE level in syslog is sufficient.
New patch attached. Michal
I forgot to fixup one change.. will post new patch.
This is the correct one.
Michal
On Mon, Aug 19, 2013 at 09:56:23PM +0200, Michal Židek wrote:
On 08/19/2013 09:52 PM, Michal Židek wrote:
On 08/19/2013 09:51 PM, Michal Židek wrote:
On 08/19/2013 09:10 PM, Jakub Hrozek wrote:
On Thu, Aug 15, 2013 at 06:42:53PM +0200, Michal Židek wrote:
On 08/15/2013 11:01 AM, Lukas Slebodnik wrote:
On (14/08/13 18:50), Michal Židek wrote: >On 08/14/2013 04:39 PM, Lukas Slebodnik wrote: >>On (12/08/13 17:47), Michal Židek wrote: >>>Hello, >>> >>>I think it could be useful to store the corrupted memory cache >>>before reset. We have very little info about what was really >>>wrong in the cache when it was in inconsistent state. This way we >>>could ask users to send us copy of the corrupted cache if they >>>hit this issue. It could provide some more answers. >>> >>>Patch is attached. It stores the corrupted cache in >>>/var/lib/sss/mc/<cache_name>_corrupted. >>> >>>Thanks >>>Michal >> >>Please rebase patch on top of patches from thread >>"[PATCH] mmap_cache: Check data->name value in client code" >> >>It cannot be applied cleanly. >> >>LS >> > >Ok. New patch is attached. (Rebased on top of the patches in thread >[SSSD] [PATCH] mmap_cache: Check data->name value in client code) > >Thanks >Michal > Backup of memory maped was created (gdb cheating)
ACK
LS
Just sending rebased version.
Michal
The patch looks good to me and I was about to push it but I wonder if we should call sss_log() to explain to the admin what happened and what are these strange files sssd created?
Ok. When the copy of memcache is successfully created a sss_log with SSS_LOG_NOTICE is called. We already do enough noise in sssd logs so I think NOTICE level in syslog is sufficient.
New patch attached. Michal
I forgot to fixup one change.. will post new patch.
This is the correct one.
Michal
That should let the admin know that something is up, thanks. ACK.
On Mon, Aug 19, 2013 at 10:14:07PM +0200, Jakub Hrozek wrote:
On Mon, Aug 19, 2013 at 09:56:23PM +0200, Michal Židek wrote:
On 08/19/2013 09:52 PM, Michal Židek wrote:
On 08/19/2013 09:51 PM, Michal Židek wrote:
On 08/19/2013 09:10 PM, Jakub Hrozek wrote:
On Thu, Aug 15, 2013 at 06:42:53PM +0200, Michal Židek wrote:
On 08/15/2013 11:01 AM, Lukas Slebodnik wrote: >On (14/08/13 18:50), Michal Židek wrote: >>On 08/14/2013 04:39 PM, Lukas Slebodnik wrote: >>>On (12/08/13 17:47), Michal Židek wrote: >>>>Hello, >>>> >>>>I think it could be useful to store the corrupted memory cache >>>>before reset. We have very little info about what was really >>>>wrong in the cache when it was in inconsistent state. This way we >>>>could ask users to send us copy of the corrupted cache if they >>>>hit this issue. It could provide some more answers. >>>> >>>>Patch is attached. It stores the corrupted cache in >>>>/var/lib/sss/mc/<cache_name>_corrupted. >>>> >>>>Thanks >>>>Michal >>> >>>Please rebase patch on top of patches from thread >>>"[PATCH] mmap_cache: Check data->name value in client code" >>> >>>It cannot be applied cleanly. >>> >>>LS >>> >> >>Ok. New patch is attached. (Rebased on top of the patches in thread >>[SSSD] [PATCH] mmap_cache: Check data->name value in client code) >> >>Thanks >>Michal >> >Backup of memory maped was created (gdb cheating) > >ACK > >LS
Just sending rebased version.
Michal
The patch looks good to me and I was about to push it but I wonder if we should call sss_log() to explain to the admin what happened and what are these strange files sssd created?
Ok. When the copy of memcache is successfully created a sss_log with SSS_LOG_NOTICE is called. We already do enough noise in sssd logs so I think NOTICE level in syslog is sufficient.
New patch attached. Michal
I forgot to fixup one change.. will post new patch.
This is the correct one.
Michal
Pushed to master sssd-1-10 and sssd-1-9
On 08/19/2013 09:56 PM, Michal Židek wrote:
+static void sss_mc_save_corrupted(struct sss_mc_ctx *mc_ctx) +{
- int err;
- int fd = -1;
- ssize_t written;
- char *file = NULL;
- TALLOC_CTX *tmp_ctx;
- if (mc_ctx == NULL) {
DEBUG(SSSDBG_TRACE_FUNC,("Cannot store uninitialized cache. Nothing to do.\n"));return;- }
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));return;- }
- file = talloc_asprintf(tmp_ctx, "%s_%s",
mc_ctx->file, "corrupted");- if (file == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));goto done;- }
- /* We will always store only the last problematic cache state */
- fd = creat(file, 0600);
- if (fd == -1) {
err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("Failed to open file '%s' [%d]: %s\n",file, err, strerror(err)));goto done;- }
- written = write(fd, mc_ctx->mmap_base, mc_ctx->mmap_size);
Shouldn't sss_atomic_write_s() be used?
- if (written != mc_ctx->mmap_size) {
if (written == -1) {err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("write() failed [%d]: %s\n", err, strerror(err)));} else {DEBUG(SSSDBG_CRIT_FAILURE,("write() returned %zd (expected (%zd))\n",written, mc_ctx->mmap_size));}goto done;- }
I think the file should be unlinked in case of error.
- sss_log(SSS_LOG_NOTICE,
"Stored copy of corrupted mmap cache in file '%s\n'", file);+done:
- if (fd != -1) {
close(fd);- }
- talloc_free(tmp_ctx);
+}
On (21/08/13 09:50), Pavel Březina wrote:
On 08/19/2013 09:56 PM, Michal Židek wrote:
+static void sss_mc_save_corrupted(struct sss_mc_ctx *mc_ctx) +{
- int err;
- int fd = -1;
- ssize_t written;
- char *file = NULL;
- TALLOC_CTX *tmp_ctx;
- if (mc_ctx == NULL) {
DEBUG(SSSDBG_TRACE_FUNC,("Cannot store uninitialized cache. Nothing to do.\n"));return;- }
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));return;- }
- file = talloc_asprintf(tmp_ctx, "%s_%s",
mc_ctx->file, "corrupted");- if (file == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));goto done;- }
- /* We will always store only the last problematic cache state */
- fd = creat(file, 0600);
- if (fd == -1) {
err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("Failed to open file '%s' [%d]: %s\n",file, err, strerror(err)));goto done;- }
- written = write(fd, mc_ctx->mmap_base, mc_ctx->mmap_size);
Shouldn't sss_atomic_write_s() be used?
- if (written != mc_ctx->mmap_size) {
if (written == -1) {err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("write() failed [%d]: %s\n", err, strerror(err)));} else {DEBUG(SSSDBG_CRIT_FAILURE,("write() returned %zd (expected (%zd))\n",written, mc_ctx->mmap_size));}goto done;- }
I think the file should be unlinked in case of error.
- sss_log(SSS_LOG_NOTICE,
"Stored copy of corrupted mmap cache in file '%s\n'", file);+done:
- if (fd != -1) {
close(fd);- }
- talloc_free(tmp_ctx);
+}
I think that function sss_mc_save_corrupted will never be called with patches from thread "[PATCHES] mmap_cache: Skip records which doesn't have same hash"
It is very likely that it is a dead code. I would prefer to ignore this nitpicks, because patches have already been pushed.
LS
On Wed, Aug 21, 2013 at 10:27:38AM +0200, Lukas Slebodnik wrote:
On (21/08/13 09:50), Pavel Březina wrote:
On 08/19/2013 09:56 PM, Michal Židek wrote:
+static void sss_mc_save_corrupted(struct sss_mc_ctx *mc_ctx) +{
- int err;
- int fd = -1;
- ssize_t written;
- char *file = NULL;
- TALLOC_CTX *tmp_ctx;
- if (mc_ctx == NULL) {
DEBUG(SSSDBG_TRACE_FUNC,("Cannot store uninitialized cache. Nothing to do.\n"));return;- }
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));return;- }
- file = talloc_asprintf(tmp_ctx, "%s_%s",
mc_ctx->file, "corrupted");- if (file == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));goto done;- }
- /* We will always store only the last problematic cache state */
- fd = creat(file, 0600);
- if (fd == -1) {
err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("Failed to open file '%s' [%d]: %s\n",file, err, strerror(err)));goto done;- }
- written = write(fd, mc_ctx->mmap_base, mc_ctx->mmap_size);
Shouldn't sss_atomic_write_s() be used?
- if (written != mc_ctx->mmap_size) {
if (written == -1) {err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("write() failed [%d]: %s\n", err, strerror(err)));} else {DEBUG(SSSDBG_CRIT_FAILURE,("write() returned %zd (expected (%zd))\n",written, mc_ctx->mmap_size));}goto done;- }
I think the file should be unlinked in case of error.
- sss_log(SSS_LOG_NOTICE,
"Stored copy of corrupted mmap cache in file '%s\n'", file);+done:
- if (fd != -1) {
close(fd);- }
- talloc_free(tmp_ctx);
+}
I think that function sss_mc_save_corrupted will never be called with patches from thread "[PATCHES] mmap_cache: Skip records which doesn't have same hash"
It is very likely that it is a dead code. I would prefer to ignore this nitpicks, because patches have already been pushed.
LS
No, sorry, I would prefer to always fix and improve code. At the very least, existing code acts as a reference when developing new code.
On (21/08/13 14:01), Jakub Hrozek wrote:
On Wed, Aug 21, 2013 at 10:27:38AM +0200, Lukas Slebodnik wrote:
On (21/08/13 09:50), Pavel Březina wrote:
On 08/19/2013 09:56 PM, Michal Židek wrote:
+static void sss_mc_save_corrupted(struct sss_mc_ctx *mc_ctx) +{
- int err;
- int fd = -1;
- ssize_t written;
- char *file = NULL;
- TALLOC_CTX *tmp_ctx;
- if (mc_ctx == NULL) {
DEBUG(SSSDBG_TRACE_FUNC,("Cannot store uninitialized cache. Nothing to do.\n"));return;- }
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));return;- }
- file = talloc_asprintf(tmp_ctx, "%s_%s",
mc_ctx->file, "corrupted");- if (file == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));goto done;- }
- /* We will always store only the last problematic cache state */
- fd = creat(file, 0600);
- if (fd == -1) {
err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("Failed to open file '%s' [%d]: %s\n",file, err, strerror(err)));goto done;- }
- written = write(fd, mc_ctx->mmap_base, mc_ctx->mmap_size);
Shouldn't sss_atomic_write_s() be used?
- if (written != mc_ctx->mmap_size) {
if (written == -1) {err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("write() failed [%d]: %s\n", err, strerror(err)));} else {DEBUG(SSSDBG_CRIT_FAILURE,("write() returned %zd (expected (%zd))\n",written, mc_ctx->mmap_size));}goto done;- }
I think the file should be unlinked in case of error.
- sss_log(SSS_LOG_NOTICE,
"Stored copy of corrupted mmap cache in file '%s\n'", file);+done:
- if (fd != -1) {
close(fd);- }
- talloc_free(tmp_ctx);
+}
I think that function sss_mc_save_corrupted will never be called with patches from thread "[PATCHES] mmap_cache: Skip records which doesn't have same hash"
It is very likely that it is a dead code. I would prefer to ignore this nitpicks, because patches have already been pushed.
LS
No, sorry, I would prefer to always fix and improve code. At the very least, existing code acts as a reference when developing new code.
sensible argument. In this case, I agree too.
LS
On 08/21/2013 02:01 PM, Jakub Hrozek wrote:
On Wed, Aug 21, 2013 at 10:27:38AM +0200, Lukas Slebodnik wrote:
On (21/08/13 09:50), Pavel Březina wrote:
On 08/19/2013 09:56 PM, Michal Židek wrote:
+static void sss_mc_save_corrupted(struct sss_mc_ctx *mc_ctx) +{
- int err;
- int fd = -1;
- ssize_t written;
- char *file = NULL;
- TALLOC_CTX *tmp_ctx;
- if (mc_ctx == NULL) {
DEBUG(SSSDBG_TRACE_FUNC,("Cannot store uninitialized cache. Nothing to do.\n"));return;- }
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));return;- }
- file = talloc_asprintf(tmp_ctx, "%s_%s",
mc_ctx->file, "corrupted");- if (file == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));goto done;- }
- /* We will always store only the last problematic cache state */
- fd = creat(file, 0600);
- if (fd == -1) {
err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("Failed to open file '%s' [%d]: %s\n",file, err, strerror(err)));goto done;- }
- written = write(fd, mc_ctx->mmap_base, mc_ctx->mmap_size);
Shouldn't sss_atomic_write_s() be used?
sss_atomic_write_s is probably better. I though that the function is only useful for non-blocking sockets, but after reading it's code I see it is good to use it here too.
- if (written != mc_ctx->mmap_size) {
if (written == -1) {err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("write() failed [%d]: %s\n", err, strerror(err)));} else {DEBUG(SSSDBG_CRIT_FAILURE,("write() returned %zd (expected (%zd))\n",written, mc_ctx->mmap_size));}goto done;- }
I think the file should be unlinked in case of error.
Ok. I added unlink() call for the case if write returns -1. If the file was only partially written (for whatever reason) it can still hold some useful information, so I would prefer to keep it.
- sss_log(SSS_LOG_NOTICE,
"Stored copy of corrupted mmap cache in file '%s\n'", file);+done:
- if (fd != -1) {
close(fd);- }
- talloc_free(tmp_ctx);
+}
I think that function sss_mc_save_corrupted will never be called with patches from thread "[PATCHES] mmap_cache: Skip records which doesn't have same hash"
It is very likely that it is a dead code. I would prefer to ignore this nitpicks, because patches have already been pushed.
LS
No, sorry, I would prefer to always fix and improve code. At the very least, existing code acts as a reference when developing new code.
Since the original patch was already pushed, this one just adds the requested changes.
Thanks Michal
On 08/21/2013 04:47 PM, Michal Židek wrote:
On 08/21/2013 02:01 PM, Jakub Hrozek wrote:
On Wed, Aug 21, 2013 at 10:27:38AM +0200, Lukas Slebodnik wrote:
On (21/08/13 09:50), Pavel Březina wrote:
On 08/19/2013 09:56 PM, Michal Židek wrote:
+static void sss_mc_save_corrupted(struct sss_mc_ctx *mc_ctx) +{
- int err;
- int fd = -1;
- ssize_t written;
- char *file = NULL;
- TALLOC_CTX *tmp_ctx;
- if (mc_ctx == NULL) {
DEBUG(SSSDBG_TRACE_FUNC,("Cannot store uninitialized cache. Nothing todo.\n"));
return;- }
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));return;- }
- file = talloc_asprintf(tmp_ctx, "%s_%s",
mc_ctx->file, "corrupted");- if (file == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));goto done;- }
- /* We will always store only the last problematic cache state */
- fd = creat(file, 0600);
- if (fd == -1) {
err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("Failed to open file '%s' [%d]: %s\n",file, err, strerror(err)));goto done;- }
- written = write(fd, mc_ctx->mmap_base, mc_ctx->mmap_size);
Shouldn't sss_atomic_write_s() be used?
sss_atomic_write_s is probably better. I though that the function is only useful for non-blocking sockets, but after reading it's code I see it is good to use it here too.
- if (written != mc_ctx->mmap_size) {
if (written == -1) {err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("write() failed [%d]: %s\n", err, strerror(err)));} else {DEBUG(SSSDBG_CRIT_FAILURE,("write() returned %zd (expected (%zd))\n",written, mc_ctx->mmap_size));}goto done;- }
I think the file should be unlinked in case of error.
Ok. I added unlink() call for the case if write returns -1. If the file was only partially written (for whatever reason) it can still hold some useful information, so I would prefer to keep it.
- sss_log(SSS_LOG_NOTICE,
"Stored copy of corrupted mmap cache in file '%s\n'",file); +done:
- if (fd != -1) {
close(fd);- }
- talloc_free(tmp_ctx);
+}
I think that function sss_mc_save_corrupted will never be called with patches from thread "[PATCHES] mmap_cache: Skip records which doesn't have same hash"
It is very likely that it is a dead code. I would prefer to ignore this nitpicks, because patches have already been pushed.
LS
No, sorry, I would prefer to always fix and improve code. At the very least, existing code acts as a reference when developing new code.
Since the original patch was already pushed, this one just adds the requested changes.
Thanks Michal
Since this was not yet pushed I did one more cosmetic change. I think calling the close() before unlink() is more readable. If the fd != -1 we already know that we want to close it, so the conditional check for unlink() just makes noise before it. And this order is probably more natural for most cases.
Michal
On 08/22/2013 01:20 AM, Michal Židek wrote:
On 08/21/2013 04:47 PM, Michal Židek wrote:
On 08/21/2013 02:01 PM, Jakub Hrozek wrote:
On Wed, Aug 21, 2013 at 10:27:38AM +0200, Lukas Slebodnik wrote:
On (21/08/13 09:50), Pavel Březina wrote:
On 08/19/2013 09:56 PM, Michal Židek wrote:
+static void sss_mc_save_corrupted(struct sss_mc_ctx *mc_ctx) +{
- int err;
- int fd = -1;
- ssize_t written;
- char *file = NULL;
- TALLOC_CTX *tmp_ctx;
- if (mc_ctx == NULL) {
DEBUG(SSSDBG_TRACE_FUNC,("Cannot store uninitialized cache. Nothing todo.\n"));
return;- }
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));return;- }
- file = talloc_asprintf(tmp_ctx, "%s_%s",
mc_ctx->file, "corrupted");- if (file == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n"));goto done;- }
- /* We will always store only the last problematic cache state */
- fd = creat(file, 0600);
- if (fd == -1) {
err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("Failed to open file '%s' [%d]: %s\n",file, err, strerror(err)));goto done;- }
- written = write(fd, mc_ctx->mmap_base, mc_ctx->mmap_size);
Shouldn't sss_atomic_write_s() be used?
sss_atomic_write_s is probably better. I though that the function is only useful for non-blocking sockets, but after reading it's code I see it is good to use it here too.
- if (written != mc_ctx->mmap_size) {
if (written == -1) {err = errno;DEBUG(SSSDBG_CRIT_FAILURE,("write() failed [%d]: %s\n", err,strerror(err)));
} else {DEBUG(SSSDBG_CRIT_FAILURE,("write() returned %zd (expected (%zd))\n",written, mc_ctx->mmap_size));}goto done;- }
I think the file should be unlinked in case of error.
Ok. I added unlink() call for the case if write returns -1. If the file was only partially written (for whatever reason) it can still hold some useful information, so I would prefer to keep it.
- sss_log(SSS_LOG_NOTICE,
"Stored copy of corrupted mmap cache in file '%s\n'",file); +done:
- if (fd != -1) {
close(fd);- }
- talloc_free(tmp_ctx);
+}
I think that function sss_mc_save_corrupted will never be called with patches from thread "[PATCHES] mmap_cache: Skip records which doesn't have same hash"
It is very likely that it is a dead code. I would prefer to ignore this nitpicks, because patches have already been pushed.
LS
No, sorry, I would prefer to always fix and improve code. At the very least, existing code acts as a reference when developing new code.
Since the original patch was already pushed, this one just adds the requested changes.
Thanks Michal
Since this was not yet pushed I did one more cosmetic change. I think calling the close() before unlink() is more readable. If the fd != -1 we already know that we want to close it, so the conditional check for unlink() just makes noise before it. And this order is probably more natural for most cases.
Michal
Ack.
On Thu, Aug 22, 2013 at 10:45:40AM +0200, Pavel Březina wrote:
On 08/22/2013 01:20 AM, Michal Židek wrote:
On 08/21/2013 04:47 PM, Michal Židek wrote:
On 08/21/2013 02:01 PM, Jakub Hrozek wrote:
On Wed, Aug 21, 2013 at 10:27:38AM +0200, Lukas Slebodnik wrote:
On (21/08/13 09:50), Pavel Březina wrote:
On 08/19/2013 09:56 PM, Michal Židek wrote: >+static void sss_mc_save_corrupted(struct sss_mc_ctx *mc_ctx) >+{ >+ int err; >+ int fd = -1; >+ ssize_t written; >+ char *file = NULL; >+ TALLOC_CTX *tmp_ctx; >+ >+ if (mc_ctx == NULL) { >+ DEBUG(SSSDBG_TRACE_FUNC, >+ ("Cannot store uninitialized cache. Nothing to >do.\n")); >+ return; >+ } >+ >+ tmp_ctx = talloc_new(NULL); >+ if (tmp_ctx == NULL) { >+ DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n")); >+ return; >+ } >+ >+ file = talloc_asprintf(tmp_ctx, "%s_%s", >+ mc_ctx->file, "corrupted"); >+ if (file == NULL) { >+ DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory.\n")); >+ goto done; >+ } >+ >+ /* We will always store only the last problematic cache state */ >+ fd = creat(file, 0600); >+ if (fd == -1) { >+ err = errno; >+ DEBUG(SSSDBG_CRIT_FAILURE, >+ ("Failed to open file '%s' [%d]: %s\n", >+ file, err, strerror(err))); >+ goto done; >+ } >+ >+ written = write(fd, mc_ctx->mmap_base, mc_ctx->mmap_size);
Shouldn't sss_atomic_write_s() be used?
sss_atomic_write_s is probably better. I though that the function is only useful for non-blocking sockets, but after reading it's code I see it is good to use it here too.
>+ if (written != mc_ctx->mmap_size) { >+ if (written == -1) { >+ err = errno; >+ DEBUG(SSSDBG_CRIT_FAILURE, >+ ("write() failed [%d]: %s\n", err, >strerror(err))); >+ } else { >+ DEBUG(SSSDBG_CRIT_FAILURE, >+ ("write() returned %zd (expected (%zd))\n", >+ written, mc_ctx->mmap_size)); >+ } >+ goto done; >+ }
I think the file should be unlinked in case of error.
Ok. I added unlink() call for the case if write returns -1. If the file was only partially written (for whatever reason) it can still hold some useful information, so I would prefer to keep it.
>+ >+ sss_log(SSS_LOG_NOTICE, >+ "Stored copy of corrupted mmap cache in file '%s\n'", >file); >+done: >+ if (fd != -1) { >+ close(fd); >+ } >+ talloc_free(tmp_ctx); >+}
I think that function sss_mc_save_corrupted will never be called with patches from thread "[PATCHES] mmap_cache: Skip records which doesn't have same hash"
It is very likely that it is a dead code. I would prefer to ignore this nitpicks, because patches have already been pushed.
LS
No, sorry, I would prefer to always fix and improve code. At the very least, existing code acts as a reference when developing new code.
Since the original patch was already pushed, this one just adds the requested changes.
Thanks Michal
Since this was not yet pushed I did one more cosmetic change. I think calling the close() before unlink() is more readable. If the fd != -1 we already know that we want to close it, so the conditional check for unlink() just makes noise before it. And this order is probably more natural for most cases.
Michal
Ack.
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek -
Pavel Březina