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.

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

Update rc.initdiskless, fix error handling of remount_optional #1497

Closed
wants to merge 2 commits into from

Conversation

kevemueller
Copy link
Contributor

chkerr() ignores the exit code of a preceding mount command in case a file remount_optional exists. The check is performed and a subshell is launched to log the informational message and return. The return is executed in the context of the subshell, not the context of the chkerr() function, hence is a NOP. The remount_optional check is hence ineffective.

Change the code to if/then/fi, so the return is evaluated in the context of the chkerr function, to make the check effective.

Copy link
Member

@bsdimp bsdimp left a comment

Choose a reason for hiding this comment

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

Subtle but wrong before

chkerr() ignores the exit code of a preceding mount command in case a file ```remount_optional``` exists.
The check is performed and a subshell is launched to log the informational message and return.
The return is executed in the context of the subshell, not the context of the chkerr() function, hence is a NOP.
The remount_optional check is hence ineffective.

Change the code to if/then/fi, so the return is evaluated in the context of the  chkerr function, to make the check effective.

Reviewed by: imp, emaste
Pull Request: freebsd#1497
@bsdimp
Copy link
Member

bsdimp commented Dec 12, 2024

I staged this, but got the error

ERROR: Real email adress is needed
#2:
Author: Keve Müller <kevemueller@users.noreply.github.com>

Can you offer a better email?

Also, while you're at it, can you wrap the body of the commit message at 72 columns (+/-) please if you do this by updating the commit?

@bsdimp
Copy link
Member

bsdimp commented Dec 12, 2024

We need the email for this and #1452

@kevemueller kevemueller force-pushed the patch-1 branch 2 times, most recently from f6ec9d9 to 73c9f41 Compare December 13, 2024 15:17
@kevemueller
Copy link
Contributor Author

pushed with private email address.

This function is bugged since the beginning, but it never hit because
its variable doesn't allow.  However, since commit
a77e1f0 it happen now.

First, it assume that ds_len will always equal to real user requested
size.  So it being used for sector count calculation.  This is no longer
true, and will fail if attempt to read last few sectors.  Use
bp->bio_length instead.

Second, this being a loop is pointless because nsegs will never be > 1
as specified at bus_dma_tag_create() call.  And all it doing is to
repeat very same command again but with different ds_addr.  Since
bio_driver2 tag ident pointer are being reused, the result will be
discarded at ps3disk_intr().

Signed-off-by: Chattrapat Sangmanee <aomsin27@hotmail.co.th>
Reviewed by: imp,mav
Pull Request: freebsd#1414
@bsdimp
Copy link
Member

bsdimp commented Jan 24, 2025

merged, but couldn't push to show as merged

@bsdimp bsdimp closed this Jan 24, 2025
@bsdimp bsdimp added the merged label Jan 24, 2025
freebsd-git pushed a commit that referenced this pull request Jan 24, 2025
chkerr() ignores the exit code of a preceding mount command in case a
file ```remount_optional``` exists.  The check is performed and a
subshell is launched to log the informational message and return.  The
return is executed in the context of the subshell, not the context of
the chkerr() function, hence is a NOP.  The remount_optional check is
hence ineffective.

Change the code to if/then/fi, so the return is evaluated in the context
of the chkerr function, to make the check effective.

Reviewed by: imp, emaste
Pull Request: #1497
emaste added a commit to emaste/freebsd that referenced this pull request Jan 25, 2025
commit 298973d9eab33705133b6cd8081de88de8a89284
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Jan 24 21:48:18 2025 -0500

    openssh: regen ssh_namespace.h

commit 3107488b7a1f5606dbd3f725bfe67b467dd83755
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Jan 24 21:45:42 2025 -0500

    openssh: run freebsd-configure.sh

commit 90fe2b07724036b43b0e8978ef892b3a07c7fabc
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Jan 24 16:25:59 2025 -0500

    openssh: Bump VersionAddendum date

commit 2dcb19e247b7caf46624ea234bd3576b63f3a0d1
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Jan 24 16:15:22 2025 -0500

    openssh: Remove rebase leftovers

commit 61d26e562c5d4adf29dcfce93e59cafbd1e7172b
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Jan 24 16:13:46 2025 -0500

    openssh: Break style(9) to match upstream

commit 7cf86867661ed675fad6b445f6bc91817b123b40
Merge: ca36371 126e0f4
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Jan 24 16:10:09 2025 -0500

    Merge commit '126e0f4f7dcb4ed8d46dc373a4d00e022c457db4' into openssh-9.9p1-update

commit ca36371
Author: Ahmad Khalifa <ahmadkhalifa570@gmail.com>
Date:   Mon Jan 6 03:52:06 2025 +0200

    checkstyle9.pl: Remove unneeded checks

    Both the bad email check and the signoff check are handled in the
    "checklist" github workflow now.

    Reviewed by:	emaste, imp
    Pull request:	freebsd#1570

    Signed-off-by: Ahmad Khalifa <ahmadkhalifa570@gmail.com>

commit a9a6a51
Author: Ahmad Khalifa <ahmadkhalifa570@gmail.com>
Date:   Sun Jan 5 07:01:07 2025 +0200

    github: Add new checklist workflow

    Add a new 'checklist' workflow that checks the commit messages on pull
    requests. Currently, the workflow creates a comment on the pull request
    if any of these conditions are hit:
      - Missing Signed-off-by
      - Malformed Signed-off-by
      - Bad email (i.e *noreply*)

    Reviewed by:	emaste, imp
    Pull request:	freebsd#1570

    Signed-off-by: Ahmad Khalifa <ahmadkhalifa570@gmail.com>

commit c814172
Author: CismonX <admin@cismon.net>
Date:   Fri Jan 3 18:52:34 2025 +0800

    open.2: update description for O_PATH

    - Add fstatfs(), fchdir(), fchroot(), extattr_*_fd(), cap_*_get(),
      cap_*_limit() to the list of syscalls that can take an O_PATH fd.
    - Remove readlinkat() from the list, since it is already discussed
      in the first few lines of the paragraph.  It was originally added
      to the list when readlinkat() adds support for non-dir fd with
      an empty relative path (as if with AT_EMPTY_PATH), however,
      such use case is also discussed in the next paragraph.
    - Add funlinkat() to the list, since it accepts an extra fd
      (of the file to be unlinked), which is worth extra mentioning.
    - Fix a syntax issue which causes a bogus space to be rendered
      before a closing parentheses.

    Signed-off-by: CismonX <admin@cismon.net>

    Reviewed by:	markj, jhb
    MFC after:	2 weeks
    Pull Request:	freebsd#1564

commit 813f244
Author: Chattrapat Sangmanee <aomsin27@hotmail.co.th>
Date:   Wed Oct 16 21:49:22 2024 +0700

    ps3disk.c: Rewrite ps3disk_transfer

    This function is bugged since the beginning, but it never hit because
    its variable doesn't allow.  However, since commit
    a77e1f0 it happen now.

    First, it assume that ds_len will always equal to real user requested
    size.  So it being used for sector count calculation.  This is no longer
    true, and will fail if attempt to read last few sectors.  Use
    bp->bio_length instead.

    Second, this being a loop is pointless because nsegs will never be > 1
    as specified at bus_dma_tag_create() call.  And all it doing is to
    repeat very same command again but with different ds_addr.  Since
    bio_driver2 tag ident pointer are being reused, the result will be
    discarded at ps3disk_intr().

    Signed-off-by: Chattrapat Sangmanee <aomsin27@hotmail.co.th>
    Reviewed by: imp,mav
    Pull Request: freebsd#1414

commit 3c61bbe
Author: Keve Müller <kevemueller@users.noreply.github.com>
Date:   Sun Oct 27 14:09:24 2024 +0100

    Update rc.initdiskless, fix error handling of remount_optional

    chkerr() ignores the exit code of a preceding mount command in case a
    file ```remount_optional``` exists.  The check is performed and a
    subshell is launched to log the informational message and return.  The
    return is executed in the context of the subshell, not the context of
    the chkerr() function, hence is a NOP.  The remount_optional check is
    hence ineffective.

    Change the code to if/then/fi, so the return is evaluated in the context
    of the chkerr function, to make the check effective.

    Reviewed by: imp, emaste
    Pull Request: freebsd#1497

commit d726bc2
Author: Franco Fichtner <franco@opnsense.org>
Date:   Thu Jan 23 12:27:09 2025 +0100

    bsdinstall: hook up help line and prompt for ZFS disk selection

    Previously we were passing the wrong variable names for the prompt and
    help line, so the intended action wasn't clear to the user.

    Reviewed by:	jhb, markj
    MFC after:	3 days
    See also:	opnsense/installer#22
    Pull Request:	freebsd#1579

commit dc27305
Author: FUKAUMI Naoki <naoki@radxa.com>
Date:   Thu Jan 23 10:43:59 2025 +0900

    ure(4): Add support for ELECOM EDC-QUA3C

    ELECOM EDC-QUA3C is a USB3.1 Gen1 Type-A/C 2.5GBASE-T network adapter.
    This also works as a cdce(4) device by:

    usbconfig -d X.Y set_config 1
      or
    usbconfig -d X.Y set_config 2

    Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>

    MFC after:	2 weeks
    Pull Request:	freebsd#1578

commit 126e0f4
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Sep 20 05:24:23 2024 -0400

    Vendor import of OpenSSH 9.9p1

    Sponsored by:	The FreeBSD Foundation

commit d565364
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Mon Jul 1 10:01:36 2024 -0400

    Vendor import of OpenSSH 9.8p1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants