| Message ID | 20250115170051.8947-1-smayhew@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [nfs-utils] nfsdctl: debug logging fixups | expand |
On Wed, 2025-01-15 at 12:00 -0500, Scott Mayhew wrote: > Move read_nfsd_conf() out of autostart_func() and into main(). Remove > hard-coded NFSD_FAMILY_NAME in the first error message in > netlink_msg_alloc() and make the error messages in netlink_msg_alloc() > more descriptive/unique. > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > --- > SteveD - this would go on top of Jeff's "nfsdctl: add support for new > lockd configuration interface" patches. > > utils/nfsdctl/nfsdctl.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c > index 003daba5..f81c78ae 100644 > --- a/utils/nfsdctl/nfsdctl.c > +++ b/utils/nfsdctl/nfsdctl.c > @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family > > id = genl_ctrl_resolve(sock, family); > if (id < 0) { > - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME); > + xlog(L_ERROR, "failed to resolve %s generic netlink family", family); > return NULL; > } > > @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family > } > > if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) { > - xlog(L_ERROR, "failed to allocate netlink message"); > + xlog(L_ERROR, "failed to add generic netlink headers to netlink message"); > nlmsg_free(msg); > return NULL; > } > @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv) > } > } > > - read_nfsd_conf(); > - > grace = conf_get_num("nfsd", "grace-time", 0); > ret = lockd_configure(sock, grace); > if (ret) { > @@ -1728,6 +1726,8 @@ int main(int argc, char **argv) > xlog_syslog(0); > xlog_stderr(1); > > + read_nfsd_conf(); > + > /* Parse the preliminary options */ > while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) { > switch (opt) { LGTM! Reviewed-by: Jeff Layton <jlayton@kernel.org>
On 1/15/25 12:00 PM, Scott Mayhew wrote: > Move read_nfsd_conf() out of autostart_func() and into main(). Remove > hard-coded NFSD_FAMILY_NAME in the first error message in > netlink_msg_alloc() and make the error messages in netlink_msg_alloc() > more descriptive/unique. > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > --- > SteveD - this would go on top of Jeff's "nfsdctl: add support for new > lockd configuration interface" patches. Got it... > > utils/nfsdctl/nfsdctl.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c > index 003daba5..f81c78ae 100644 > --- a/utils/nfsdctl/nfsdctl.c > +++ b/utils/nfsdctl/nfsdctl.c > @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family > > id = genl_ctrl_resolve(sock, family); > if (id < 0) { > - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME); > + xlog(L_ERROR, "failed to resolve %s generic netlink family", family); > return NULL; > } > > @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family > } > > if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) { > - xlog(L_ERROR, "failed to allocate netlink message"); > + xlog(L_ERROR, "failed to add generic netlink headers to netlink message"); > nlmsg_free(msg); > return NULL; > } > @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv) > } > } > > - read_nfsd_conf(); > - > grace = conf_get_num("nfsd", "grace-time", 0); > ret = lockd_configure(sock, grace); > if (ret) { > @@ -1728,6 +1726,8 @@ int main(int argc, char **argv) > xlog_syslog(0); > xlog_stderr(1); > > + read_nfsd_conf(); > + > /* Parse the preliminary options */ > while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) { > switch (opt) { Ok... at this point we a prettier error message $ nfsdctl nlm nfsdctl: failed to resolve lockd generic netlink family But the point of this argument is: Get information about NLM (lockd) settings in the current net namespace. This subcommand takes no arguments. How is that giving information from the running lockd? What am I missing?? steved.
On Wed, 2025-01-15 at 12:32 -0500, Steve Dickson wrote: > > On 1/15/25 12:00 PM, Scott Mayhew wrote: > > Move read_nfsd_conf() out of autostart_func() and into main(). Remove > > hard-coded NFSD_FAMILY_NAME in the first error message in > > netlink_msg_alloc() and make the error messages in netlink_msg_alloc() > > more descriptive/unique. > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > --- > > SteveD - this would go on top of Jeff's "nfsdctl: add support for new > > lockd configuration interface" patches. > Got it... > > > > > utils/nfsdctl/nfsdctl.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c > > index 003daba5..f81c78ae 100644 > > --- a/utils/nfsdctl/nfsdctl.c > > +++ b/utils/nfsdctl/nfsdctl.c > > @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family > > > > id = genl_ctrl_resolve(sock, family); > > if (id < 0) { > > - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME); > > + xlog(L_ERROR, "failed to resolve %s generic netlink family", family); > > return NULL; > > } > > > > @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family > > } > > > > if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) { > > - xlog(L_ERROR, "failed to allocate netlink message"); > > + xlog(L_ERROR, "failed to add generic netlink headers to netlink message"); > > nlmsg_free(msg); > > return NULL; > > } > > @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv) > > } > > } > > > > - read_nfsd_conf(); > > - > > grace = conf_get_num("nfsd", "grace-time", 0); > > ret = lockd_configure(sock, grace); > > if (ret) { > > @@ -1728,6 +1726,8 @@ int main(int argc, char **argv) > > xlog_syslog(0); > > xlog_stderr(1); > > > > + read_nfsd_conf(); > > + > > /* Parse the preliminary options */ > > while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) { > > switch (opt) { > Ok... at this point we a prettier error message > $ nfsdctl nlm > nfsdctl: failed to resolve lockd generic netlink family > > But the point of this argument is: > > Get information about NLM (lockd) settings in the current net > namespace. This subcommand takes no arguments. > > How is that giving information from the running lockd? > > What am I missing?? > You're missing a kernel that has the required netlink interface. To test this properly, you'll need to patch your kernel, until that patch makes it upstream.
On 1/15/25 12:35 PM, Jeff Layton wrote: > On Wed, 2025-01-15 at 12:32 -0500, Steve Dickson wrote: >> >> On 1/15/25 12:00 PM, Scott Mayhew wrote: >>> Move read_nfsd_conf() out of autostart_func() and into main(). Remove >>> hard-coded NFSD_FAMILY_NAME in the first error message in >>> netlink_msg_alloc() and make the error messages in netlink_msg_alloc() >>> more descriptive/unique. >>> >>> Signed-off-by: Scott Mayhew <smayhew@redhat.com> >>> --- >>> SteveD - this would go on top of Jeff's "nfsdctl: add support for new >>> lockd configuration interface" patches. >> Got it... >> >>> >>> utils/nfsdctl/nfsdctl.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c >>> index 003daba5..f81c78ae 100644 >>> --- a/utils/nfsdctl/nfsdctl.c >>> +++ b/utils/nfsdctl/nfsdctl.c >>> @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family >>> >>> id = genl_ctrl_resolve(sock, family); >>> if (id < 0) { >>> - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME); >>> + xlog(L_ERROR, "failed to resolve %s generic netlink family", family); >>> return NULL; >>> } >>> >>> @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family >>> } >>> >>> if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) { >>> - xlog(L_ERROR, "failed to allocate netlink message"); >>> + xlog(L_ERROR, "failed to add generic netlink headers to netlink message"); >>> nlmsg_free(msg); >>> return NULL; >>> } >>> @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv) >>> } >>> } >>> >>> - read_nfsd_conf(); >>> - >>> grace = conf_get_num("nfsd", "grace-time", 0); >>> ret = lockd_configure(sock, grace); >>> if (ret) { >>> @@ -1728,6 +1726,8 @@ int main(int argc, char **argv) >>> xlog_syslog(0); >>> xlog_stderr(1); >>> >>> + read_nfsd_conf(); >>> + >>> /* Parse the preliminary options */ >>> while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) { >>> switch (opt) { >> Ok... at this point we a prettier error message >> $ nfsdctl nlm >> nfsdctl: failed to resolve lockd generic netlink family >> >> But the point of this argument is: >> >> Get information about NLM (lockd) settings in the current net >> namespace. This subcommand takes no arguments. >> >> How is that giving information from the running lockd? >> >> What am I missing?? >> > > You're missing a kernel that has the required netlink interface. To > test this properly, you'll need to patch your kernel, until that patch > makes it upstream. Okay... I figured it was something like that. But doesn't make sense to wait until the patch is in upstream so the argument can be properly tested? Why add an argument that will always fail? steved.
On Wed, 2025-01-15 at 12:47 -0500, Steve Dickson wrote: > > On 1/15/25 12:35 PM, Jeff Layton wrote: > > On Wed, 2025-01-15 at 12:32 -0500, Steve Dickson wrote: > > > > > > On 1/15/25 12:00 PM, Scott Mayhew wrote: > > > > Move read_nfsd_conf() out of autostart_func() and into main(). Remove > > > > hard-coded NFSD_FAMILY_NAME in the first error message in > > > > netlink_msg_alloc() and make the error messages in netlink_msg_alloc() > > > > more descriptive/unique. > > > > > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > > > --- > > > > SteveD - this would go on top of Jeff's "nfsdctl: add support for new > > > > lockd configuration interface" patches. > > > Got it... > > > > > > > > > > > utils/nfsdctl/nfsdctl.c | 8 ++++---- > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c > > > > index 003daba5..f81c78ae 100644 > > > > --- a/utils/nfsdctl/nfsdctl.c > > > > +++ b/utils/nfsdctl/nfsdctl.c > > > > @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family > > > > > > > > id = genl_ctrl_resolve(sock, family); > > > > if (id < 0) { > > > > - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME); > > > > + xlog(L_ERROR, "failed to resolve %s generic netlink family", family); > > > > return NULL; > > > > } > > > > > > > > @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family > > > > } > > > > > > > > if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) { > > > > - xlog(L_ERROR, "failed to allocate netlink message"); > > > > + xlog(L_ERROR, "failed to add generic netlink headers to netlink message"); > > > > nlmsg_free(msg); > > > > return NULL; > > > > } > > > > @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv) > > > > } > > > > } > > > > > > > > - read_nfsd_conf(); > > > > - > > > > grace = conf_get_num("nfsd", "grace-time", 0); > > > > ret = lockd_configure(sock, grace); > > > > if (ret) { > > > > @@ -1728,6 +1726,8 @@ int main(int argc, char **argv) > > > > xlog_syslog(0); > > > > xlog_stderr(1); > > > > > > > > + read_nfsd_conf(); > > > > + > > > > /* Parse the preliminary options */ > > > > while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) { > > > > switch (opt) { > > > Ok... at this point we a prettier error message > > > $ nfsdctl nlm > > > nfsdctl: failed to resolve lockd generic netlink family > > > > > > But the point of this argument is: > > > > > > Get information about NLM (lockd) settings in the current net > > > namespace. This subcommand takes no arguments. > > > > > > How is that giving information from the running lockd? > > > > > > What am I missing?? > > > > > > > You're missing a kernel that has the required netlink interface. To > > test this properly, you'll need to patch your kernel, until that patch > > makes it upstream. > Okay... I figured it was something like that. But doesn't make sense to > wait until the patch is in upstream so the argument can be properly > tested? Why add an argument that will always fail? > Why can't it be properly tested? It's just a matter of running a more recent kernel that has the right interfaces. That should be in linux- next soon (if not already). I think the question is whether we want to wait until the kernel interfaces trickle out into downstream distro kernels before we ship any userland support in an upstream project (nfs-utils). If you want to wait until it hits Fedora Rawhide kernels, then you're looking at about 10-12 weeks from now. If you want to wait until it makes it into a stable Fedora release kernel then we're looking at about 6 months from now. I'll note that that it took 6 months to get the original nfsdctl patches merged because of the lag on kernel patches making it into distros, and I think that was way too long.
On 1/15/25 1:33 PM, Jeff Layton wrote: > On Wed, 2025-01-15 at 12:47 -0500, Steve Dickson wrote: >> >> On 1/15/25 12:35 PM, Jeff Layton wrote: >>> On Wed, 2025-01-15 at 12:32 -0500, Steve Dickson wrote: >>>> >>>> On 1/15/25 12:00 PM, Scott Mayhew wrote: >>>>> Move read_nfsd_conf() out of autostart_func() and into main(). Remove >>>>> hard-coded NFSD_FAMILY_NAME in the first error message in >>>>> netlink_msg_alloc() and make the error messages in netlink_msg_alloc() >>>>> more descriptive/unique. >>>>> >>>>> Signed-off-by: Scott Mayhew <smayhew@redhat.com> >>>>> --- >>>>> SteveD - this would go on top of Jeff's "nfsdctl: add support for new >>>>> lockd configuration interface" patches. >>>> Got it... >>>> >>>>> >>>>> utils/nfsdctl/nfsdctl.c | 8 ++++---- >>>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c >>>>> index 003daba5..f81c78ae 100644 >>>>> --- a/utils/nfsdctl/nfsdctl.c >>>>> +++ b/utils/nfsdctl/nfsdctl.c >>>>> @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family >>>>> >>>>> id = genl_ctrl_resolve(sock, family); >>>>> if (id < 0) { >>>>> - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME); >>>>> + xlog(L_ERROR, "failed to resolve %s generic netlink family", family); >>>>> return NULL; >>>>> } >>>>> >>>>> @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family >>>>> } >>>>> >>>>> if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) { >>>>> - xlog(L_ERROR, "failed to allocate netlink message"); >>>>> + xlog(L_ERROR, "failed to add generic netlink headers to netlink message"); >>>>> nlmsg_free(msg); >>>>> return NULL; >>>>> } >>>>> @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv) >>>>> } >>>>> } >>>>> >>>>> - read_nfsd_conf(); >>>>> - >>>>> grace = conf_get_num("nfsd", "grace-time", 0); >>>>> ret = lockd_configure(sock, grace); >>>>> if (ret) { >>>>> @@ -1728,6 +1726,8 @@ int main(int argc, char **argv) >>>>> xlog_syslog(0); >>>>> xlog_stderr(1); >>>>> >>>>> + read_nfsd_conf(); >>>>> + >>>>> /* Parse the preliminary options */ >>>>> while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) { >>>>> switch (opt) { >>>> Ok... at this point we a prettier error message >>>> $ nfsdctl nlm >>>> nfsdctl: failed to resolve lockd generic netlink family >>>> >>>> But the point of this argument is: >>>> >>>> Get information about NLM (lockd) settings in the current net >>>> namespace. This subcommand takes no arguments. >>>> >>>> How is that giving information from the running lockd? >>>> >>>> What am I missing?? >>>> >>> >>> You're missing a kernel that has the required netlink interface. To >>> test this properly, you'll need to patch your kernel, until that patch >>> makes it upstream. >> Okay... I figured it was something like that. But doesn't make sense to >> wait until the patch is in upstream so the argument can be properly >> tested? Why add an argument that will always fail? >> > > Why can't it be properly tested? It's just a matter of running a more > recent kernel that has the right interfaces. That should be in linux- > next soon (if not already). I'm doing my testing on a 6.13.0-0.rc6 which will soon be a 6.14 kernel... its my understanding the needed kernel patch will be in the 6.15 kernel... Please correct me if that is not true. > > I think the question is whether we want to wait until the kernel > interfaces trickle out into downstream distro kernels before we ship > any userland support in an upstream project (nfs-utils). Yes! As soon as the kernel support hits the upstream kernel, we will be good to go. I just don't want to put a feature in that will fail %100 of the time. > > If you want to wait until it hits Fedora Rawhide kernels, then you're > looking at about 10-12 weeks from now. If you want to wait until it > makes it into a stable Fedora release kernel then we're looking at > about 6 months from now. nfsdctl is in all current Fedora stable releases, which is the reason I'm pushing back. I do not want to put something in that will make it fail. That just does not make sense to me. > > I'll note that that it took 6 months to get the original nfsdctl > patches merged because of the lag on kernel patches making it into > distros, and I think that was way too long. It took that long because there were issues with the command. In which I was glad to help debug some of the issues... New technology takes time to develop... I just think this is one of those cases. steved.
diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c index 003daba5..f81c78ae 100644 --- a/utils/nfsdctl/nfsdctl.c +++ b/utils/nfsdctl/nfsdctl.c @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family id = genl_ctrl_resolve(sock, family); if (id < 0) { - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME); + xlog(L_ERROR, "failed to resolve %s generic netlink family", family); return NULL; } @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family } if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) { - xlog(L_ERROR, "failed to allocate netlink message"); + xlog(L_ERROR, "failed to add generic netlink headers to netlink message"); nlmsg_free(msg); return NULL; } @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv) } } - read_nfsd_conf(); - grace = conf_get_num("nfsd", "grace-time", 0); ret = lockd_configure(sock, grace); if (ret) { @@ -1728,6 +1726,8 @@ int main(int argc, char **argv) xlog_syslog(0); xlog_stderr(1); + read_nfsd_conf(); + /* Parse the preliminary options */ while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) { switch (opt) {
Move read_nfsd_conf() out of autostart_func() and into main(). Remove hard-coded NFSD_FAMILY_NAME in the first error message in netlink_msg_alloc() and make the error messages in netlink_msg_alloc() more descriptive/unique. Signed-off-by: Scott Mayhew <smayhew@redhat.com> --- SteveD - this would go on top of Jeff's "nfsdctl: add support for new lockd configuration interface" patches. utils/nfsdctl/nfsdctl.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)