Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rbd: Fix warning for uninitialized MirrorSnapshotNamespace::state #43396

Closed
wants to merge 2 commits into from
Closed

rbd: Fix warning for uninitialized MirrorSnapshotNamespace::state #43396

wants to merge 2 commits into from

Conversation

gbangera
Copy link

@gbangera gbangera commented Oct 1, 2021

Fixes: https://tracker.ceph.com/issues/52753
Signed-off-by: Geetika Bangera gbangera@cs.stonybrook.edu

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@gbangera
rbd: Fix warning for uninitialized MirrorSnapshotNamespace::state
a43f5b9 
Fixes: https://tracker.ceph.com/issues/52753
Signed-off-by: Geetika Bangera <gbangera@cs.stonybrook.edu>
@gbangera
librbd: Fix warnings found during build
c13687d 
Found following warning while fixing https://tracker.ceph.com/issues/52753:
[366/548] Building CXX object src/librbd/CMakeFiles/rbd_internal.dir/MirroringWatcher.cc.o
In file included from ../src/librbd/MirroringWatcher.h:12,
                 from ../src/librbd/MirroringWatcher.cc:4:
../src/librbd/mirroring_watcher/Types.h: In member function 'void librbd::MirroringWatcher<ImageCtxT>::handle_notify(uint64_t, uint64_t, uint64_t, ceph::bufferlist&) [with ImageCtxT = librbd::ImageCtx]':
../src/librbd/mirroring_watcher/Types.h:42:8: warning: '*((void*)(&<anonymous>)+8).librbd::mirroring_watcher::ImageUpdatedPayload::mirror_image_state' may be used uninitialized in this function [-Wmaybe-uninitialized]
 struct ImageUpdatedPayload {
        ^~~~~~~~~~~~~~~~~~~
Fixing this warning.

Signed-off-by: Geetika Bangera <gbangera@cs.stonybrook.edu>
@github-actions github-actions bot added the rbd label Oct 1, 2021
@ideepika
Copy link
Member

ideepika commented Oct 2, 2021

https://shaman.ceph.com/builds/ceph/wip-pr-43396/c13687d94b2c578166d80763d8556763e0717a35/default/283525/

@ideepika
Copy link
Member

ideepika commented Oct 2, 2021

https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos8,DIST=centos8,MACHINE_SIZE=gigantic/57777//consoleFull

@ideepika
Copy link
Member

ideepika commented Oct 3, 2021

jenkins test make check

@ideepika ideepika requested a review from trociny October 3, 2021 03:09
trociny
trociny reviewed Oct 3, 2021
View reviewed changes
Copy link
Contributor

@trociny trociny left a comment

Choose a reason for hiding this comment

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

I don't quite understand why the old version produced the warnings.

It is not that I mind much, but the old way seemed prefered in modern Ceph world.

@tchaikov Have you any thoughts about this?

idryomov
idryomov reviewed Oct 11, 2021
View reviewed changes
Copy link
Contributor

@idryomov idryomov left a comment

Choose a reason for hiding this comment

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

I don't quite understand why the old version produced the warnings.

It is not that I mind much, but the old way seemed prefered in modern Ceph world.

Yeah, it doesn't make sense to revert to the old style, especially in the case of MirrorSnapshotNamespace where state would then be initialized in the constructor and complete in the body. Maybe it's just CentOS 8's g++ version?

@stale
Copy link

stale bot commented Jan 9, 2022

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Jan 9, 2022
@ljflores
Copy link
Contributor

@gbangera @ideepika @idryomov @trociny is this PR still needed?

@stale stale bot removed the stale label Jan 20, 2022
@ideepika
Copy link
Member

@gbangera @ideepika @idryomov @trociny is this PR still needed?

I am not sure if we still see this warning or not, but since we'd like a different approach rather than using old style, this PR will need changes. I will see if we see this issue again or in latest gcc version or not

@djgalloway djgalloway changed the base branch from master to main July 1, 2022 00:00
@djgalloway djgalloway requested a review from a team as a code owner July 1, 2022 00:00
@idryomov
Copy link
Contributor

idryomov commented Jul 1, 2022

Looks like this warning is no longer there with a newer g++:

https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos8,DIST=centos8,MACHINE_SIZE=gigantic/63582/consoleText

@idryomov idryomov closed this Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idryomov idryomov idryomov left review comments

@trociny trociny trociny left review comments

Assignees
No one assigned
Labels
bug-fix needs-qa needs-review open-source-day rbd
Projects
None yet
Milestone
No milestone
6 participants
@gbangera @ideepika @ljflores @idryomov @trociny @varshar16