Re: [libvirt] [PATCH] util: netdevbridge: fall back to ioctl from sysfs
On Tue, Nov 20, 2018 at 01:03:09PM +0100, Christian Ehrhardt wrote:
> On Tue, Nov 20, 2018 at 12:58 PM Daniel P. Berrangé
> wrote:
> >
> > On Tue, Nov 20, 2018 at 11:17:07AM +0100, Christian Ehrhardt wrote:
> > > There are certain cases e.g. containers where the sysfs path might
> > > exists, but might fail. Unfortunately the exact restrictions are only
> > > known to libvirt when trying to write to it so we need to try it.
> > >
> > > But in case it fails there is no need to fully abort, in those cases try
> > > to fall back to the older ioctl interface which can still work.
> > >
> > > That makes setting up a bridge in unprivileged LXD containers work.
> > >
> > > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906
> > >
> > > Signed-off-by: Christian Ehrhardt
> > > ---
> > > src/util/virnetdevbridge.c | 48 +-
> > > 1 file changed, 26 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> > > index 071ebb7b35..cbba3c9652 100644
> > > --- a/src/util/virnetdevbridge.c
> > > +++ b/src/util/virnetdevbridge.c
> > > @@ -113,6 +113,8 @@ static int virNetDevBridgeCmd(const char *brname,
> > > * or by ioctl on older kernels. Perhaps we could just use
> > > * ioctl for every kernel, but its not clear what the long
> > > * term lifespan of the ioctl interface is...
> > > + * Fall back to ioctl if sysfs interface is not available or
> > > + * failing (e.g. due to container isolation).
> > > */
> > > static int virNetDevBridgeSet(const char *brname,
> > >const char *paramname, /* sysfs param
> > > name */
> > > @@ -128,29 +130,31 @@ static int virNetDevBridgeSet(const char *brname,
> > > if (virFileExists(path)) {
> > > char valuestr[INT_BUFSIZE_BOUND(value)];
> > > snprintf(valuestr, sizeof(valuestr), "%lu", value);
> > > -if (virFileWriteStr(path, valuestr, 0) < 0) {
> > > -virReportSystemError(errno,
> > > - _("Unable to set bridge %s %s"),
> > > brname, paramname);
> > > -return -1;
> > > -}
> > > +if (virFileWriteStr(path, valuestr, 0) >= 0)
> > > +return 0;
> > > +virReportSystemError(errno,
> > > + _("Unable to set bridge %s %s via sysfs"),
> > > + brname, paramname);
> >
> > The virReportSystemError line should be dropped, since we're not
> > returning an error - we're moving onto the second code path instead.
>
> Yeah that sounds fine - would we want a VIR_WARN or VIR_INFO instead
> at this place.
> Any preferences?
I dont think it needs anything more than a VIR_DEBUG, since it is a normal
expected thing in the scenario you described.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: netdevbridge: fall back to ioctl from sysfs
On Tue, Nov 20, 2018 at 12:58 PM Daniel P. Berrangé wrote:
>
> On Tue, Nov 20, 2018 at 11:17:07AM +0100, Christian Ehrhardt wrote:
> > There are certain cases e.g. containers where the sysfs path might
> > exists, but might fail. Unfortunately the exact restrictions are only
> > known to libvirt when trying to write to it so we need to try it.
> >
> > But in case it fails there is no need to fully abort, in those cases try
> > to fall back to the older ioctl interface which can still work.
> >
> > That makes setting up a bridge in unprivileged LXD containers work.
> >
> > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906
> >
> > Signed-off-by: Christian Ehrhardt
> > ---
> > src/util/virnetdevbridge.c | 48 +-
> > 1 file changed, 26 insertions(+), 22 deletions(-)
> >
> > diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> > index 071ebb7b35..cbba3c9652 100644
> > --- a/src/util/virnetdevbridge.c
> > +++ b/src/util/virnetdevbridge.c
> > @@ -113,6 +113,8 @@ static int virNetDevBridgeCmd(const char *brname,
> > * or by ioctl on older kernels. Perhaps we could just use
> > * ioctl for every kernel, but its not clear what the long
> > * term lifespan of the ioctl interface is...
> > + * Fall back to ioctl if sysfs interface is not available or
> > + * failing (e.g. due to container isolation).
> > */
> > static int virNetDevBridgeSet(const char *brname,
> >const char *paramname, /* sysfs param name
> > */
> > @@ -128,29 +130,31 @@ static int virNetDevBridgeSet(const char *brname,
> > if (virFileExists(path)) {
> > char valuestr[INT_BUFSIZE_BOUND(value)];
> > snprintf(valuestr, sizeof(valuestr), "%lu", value);
> > -if (virFileWriteStr(path, valuestr, 0) < 0) {
> > -virReportSystemError(errno,
> > - _("Unable to set bridge %s %s"), brname,
> > paramname);
> > -return -1;
> > -}
> > +if (virFileWriteStr(path, valuestr, 0) >= 0)
> > +return 0;
> > +virReportSystemError(errno,
> > + _("Unable to set bridge %s %s via sysfs"),
> > + brname, paramname);
>
> The virReportSystemError line should be dropped, since we're not
> returning an error - we're moving onto the second code path instead.
Yeah that sounds fine - would we want a VIR_WARN or VIR_INFO instead
at this place.
Any preferences?
> > +}
> > +
> > +unsigned long paramid;
> > +if (STREQ(paramname, "stp_state")) {
> > +paramid = BRCTL_SET_BRIDGE_STP_STATE;
> > +} else if (STREQ(paramname, "forward_delay")) {
> > +paramid = BRCTL_SET_BRIDGE_FORWARD_DELAY;
> > } else {
> > -unsigned long paramid;
> > -if (STREQ(paramname, "stp_state")) {
> > -paramid = BRCTL_SET_BRIDGE_STP_STATE;
> > -} else if (STREQ(paramname, "forward_delay")) {
> > -paramid = BRCTL_SET_BRIDGE_FORWARD_DELAY;
> > -} else {
> > -virReportSystemError(EINVAL,
> > - _("Unable to set bridge %s %s"), brname,
> > paramname);
> > -return -1;
> > -}
> > -unsigned long args[] = { paramid, value, 0, 0 };
> > -ifr->ifr_data = (char*)
> > -if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
> > -virReportSystemError(errno,
> > - _("Unable to set bridge %s %s"), brname,
> > paramname);
> > -return -1;
> > -}
> > +virReportSystemError(EINVAL,
> > + _("Unable to set bridge %s %s via ioctl"),
> > + brname, paramname);
> > +return -1;
> > +}
> > +unsigned long args[] = { paramid, value, 0, 0 };
> > +ifr->ifr_data = (char*)
> > +if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
> > +virReportSystemError(errno,
> > + _("Failed to set bridge %s %s via ioctl"),
> > + brname, paramname);
> > +return -1;
> > }
> >
> > return 0;
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: netdevbridge: fall back to ioctl from sysfs
On Tue, Nov 20, 2018 at 11:17:07AM +0100, Christian Ehrhardt wrote:
> There are certain cases e.g. containers where the sysfs path might
> exists, but might fail. Unfortunately the exact restrictions are only
> known to libvirt when trying to write to it so we need to try it.
>
> But in case it fails there is no need to fully abort, in those cases try
> to fall back to the older ioctl interface which can still work.
>
> That makes setting up a bridge in unprivileged LXD containers work.
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/util/virnetdevbridge.c | 48 +-
> 1 file changed, 26 insertions(+), 22 deletions(-)
>
> diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> index 071ebb7b35..cbba3c9652 100644
> --- a/src/util/virnetdevbridge.c
> +++ b/src/util/virnetdevbridge.c
> @@ -113,6 +113,8 @@ static int virNetDevBridgeCmd(const char *brname,
> * or by ioctl on older kernels. Perhaps we could just use
> * ioctl for every kernel, but its not clear what the long
> * term lifespan of the ioctl interface is...
> + * Fall back to ioctl if sysfs interface is not available or
> + * failing (e.g. due to container isolation).
> */
> static int virNetDevBridgeSet(const char *brname,
>const char *paramname, /* sysfs param name */
> @@ -128,29 +130,31 @@ static int virNetDevBridgeSet(const char *brname,
> if (virFileExists(path)) {
> char valuestr[INT_BUFSIZE_BOUND(value)];
> snprintf(valuestr, sizeof(valuestr), "%lu", value);
> -if (virFileWriteStr(path, valuestr, 0) < 0) {
> -virReportSystemError(errno,
> - _("Unable to set bridge %s %s"), brname,
> paramname);
> -return -1;
> -}
> +if (virFileWriteStr(path, valuestr, 0) >= 0)
> +return 0;
> +virReportSystemError(errno,
> + _("Unable to set bridge %s %s via sysfs"),
> + brname, paramname);
The virReportSystemError line should be dropped, since we're not
returning an error - we're moving onto the second code path instead.
> +}
> +
> +unsigned long paramid;
> +if (STREQ(paramname, "stp_state")) {
> +paramid = BRCTL_SET_BRIDGE_STP_STATE;
> +} else if (STREQ(paramname, "forward_delay")) {
> +paramid = BRCTL_SET_BRIDGE_FORWARD_DELAY;
> } else {
> -unsigned long paramid;
> -if (STREQ(paramname, "stp_state")) {
> -paramid = BRCTL_SET_BRIDGE_STP_STATE;
> -} else if (STREQ(paramname, "forward_delay")) {
> -paramid = BRCTL_SET_BRIDGE_FORWARD_DELAY;
> -} else {
> -virReportSystemError(EINVAL,
> - _("Unable to set bridge %s %s"), brname,
> paramname);
> -return -1;
> -}
> -unsigned long args[] = { paramid, value, 0, 0 };
> -ifr->ifr_data = (char*)
> -if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
> -virReportSystemError(errno,
> - _("Unable to set bridge %s %s"), brname,
> paramname);
> -return -1;
> -}
> +virReportSystemError(EINVAL,
> + _("Unable to set bridge %s %s via ioctl"),
> + brname, paramname);
> +return -1;
> +}
> +unsigned long args[] = { paramid, value, 0, 0 };
> +ifr->ifr_data = (char*)
> +if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
> +virReportSystemError(errno,
> + _("Failed to set bridge %s %s via ioctl"),
> + brname, paramname);
> +return -1;
> }
>
> return 0;
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: netdevbridge: fall back to ioctl from sysfs
On Tue, Nov 20, 2018 at 11:17 AM Christian Ehrhardt wrote: > > There are certain cases e.g. containers where the sysfs path might > exists, but might fail. Unfortunately the exact restrictions are only > known to libvirt when trying to write to it so we need to try it. > > But in case it fails there is no need to fully abort, in those cases try > to fall back to the older ioctl interface which can still work. > > That makes setting up a bridge in unprivileged LXD containers work. > > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906 > > Signed-off-by: Christian Ehrhardt FYI: in the meantime I got the email of the bug-reporter. Eventually I'll add: Reported-by: Brian Candler But that alone was not worth a v2 submission. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
