[libvirt PATCH] qemu: reject readonly attribute for virtiofs

Ján Tomko posted 1 patch 1 week ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/18539fa2c8d6731dc3ab028fd2d264a26fa8694d.1589360327.git.jtomko@redhat.com
src/qemu/qemu_validate.c                      |  5 +++
.../vhost-user-fs-readonly.xml                | 44 +++++++++++++++++++
tests/qemuxml2argvtest.c                      |  1 +
3 files changed, 50 insertions(+)
create mode 100644 tests/qemuxml2argvdata/vhost-user-fs-readonly.xml

[libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Ján Tomko 1 week ago
This is not yet supported by virtiofsd.

Fixes #23

Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
 src/qemu/qemu_validate.c                      |  5 +++
 .../vhost-user-fs-readonly.xml                | 44 +++++++++++++++++++
 tests/qemuxml2argvtest.c                      |  1 +
 3 files changed, 50 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/vhost-user-fs-readonly.xml

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index fde1892d42..ac22270104 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3418,6 +3418,11 @@ qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
         return -1;
 
     case VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS:
+        if (fs->readonly) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("virtiofs does not yet supported read-only mode"));
+            return -1;
+        }
         if (!driver->privileged) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                            _("virtiofs is not yet supported in session mode"));
diff --git a/tests/qemuxml2argvdata/vhost-user-fs-readonly.xml b/tests/qemuxml2argvdata/vhost-user-fs-readonly.xml
new file mode 100644
index 0000000000..003ed41eb3
--- /dev/null
+++ b/tests/qemuxml2argvdata/vhost-user-fs-readonly.xml
@@ -0,0 +1,44 @@
+<domain type='kvm'>
+  <name>guest</name>
+  <uuid>126f2720-6f8e-45ab-a886-ec9277079a67</uuid>
+  <memory unit='KiB'>14680064</memory>
+  <currentMemory unit='KiB'>14680064</currentMemory>
+  <memoryBacking>
+    <source type='file'/>
+    <access mode='shared'/>
+  </memoryBacking>
+  <vcpu placement='static'>2</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <cpu mode='custom' match='exact' check='none'>
+    <model fallback='forbid'>qemu64</model>
+    <numa>
+      <cell id='0' cpus='0-1' memory='14680064' unit='KiB' memAccess='shared'/>
+    </numa>
+  </cpu>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' index='0' model='none'/>
+    <controller type='pci' index='0' model='pci-root'/>
+    <filesystem type='mount' accessmode='passthrough'>
+      <driver type='virtiofs' queue='1024'/>
+      <binary path='/usr/libexec/virtiofsd' xattr='on'>
+        <cache mode='always'/>
+        <lock posix='off' flock='off'/>
+      </binary>
+      <source dir='/path'/>
+      <target dir='mount_tag'/>
+      <readonly/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+    </filesystem>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 4ab664a846..8cbf3f8daf 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -3171,6 +3171,7 @@ mymain(void)
 
     DO_TEST_CAPS_LATEST("vhost-user-fs-fd-memory");
     DO_TEST_CAPS_LATEST("vhost-user-fs-hugepages");
+    DO_TEsT_CAPS_LATEST_PARSE_ERROR("vhost-user-fs-readonly");
 
     DO_TEST("riscv64-virt",
             QEMU_CAPS_DEVICE_VIRTIO_MMIO);
-- 
2.25.4

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Andrea Bolognani 1 week ago
On Wed, 2020-05-13 at 10:58 +0200, Ján Tomko wrote:
> This is not yet supported by virtiofsd.
> 
> Fixes #23

Please include the full URL here:

  https://gitlab.com/libvirt/libvirt/-/issues/23

>      case VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS:
> +        if (fs->readonly) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("virtiofs does not yet supported read-only mode"));

s/supported/support/

Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Daniel P. Berrangé 1 week ago
On Wed, May 13, 2020 at 11:34:05AM +0200, Andrea Bolognani wrote:
> On Wed, 2020-05-13 at 10:58 +0200, Ján Tomko wrote:
> > This is not yet supported by virtiofsd.
> > 
> > Fixes #23
> 
> Please include the full URL here:
> 
>   https://gitlab.com/libvirt/libvirt/-/issues/23

Using the '#23' format is the recommended way. GitLab UI will
turn it into a hyperlink, and automatically close the mentioned
issue.

> 
> >      case VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS:
> > +        if (fs->readonly) {
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                           _("virtiofs does not yet supported read-only mode"));
> 
> s/supported/support/
> 
> Reviewed-by: Andrea Bolognani <abologna@redhat.com>
> 
> -- 
> Andrea Bolognani / Red Hat / Virtualization
> 

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 :|

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Peter Krempa 1 week ago
On Wed, May 13, 2020 at 10:41:23 +0100, Daniel Berrange wrote:
> On Wed, May 13, 2020 at 11:34:05AM +0200, Andrea Bolognani wrote:
> > On Wed, 2020-05-13 at 10:58 +0200, Ján Tomko wrote:
> > > This is not yet supported by virtiofsd.
> > > 
> > > Fixes #23
> > 
> > Please include the full URL here:
> > 
> >   https://gitlab.com/libvirt/libvirt/-/issues/23

While I definitely don't like gitlab's URIs ...

> Using the '#23' format is the recommended way. GitLab UI will
> turn it into a hyperlink, and automatically close the mentioned
> issue.

... just mentioning #23 is useless for users looking at the git repo
directly (not through the web-ui) or if we at some point decide to
abandon gitlab.

Even if gitlab can't handle the automation of closing the comment when
the URI is mentioned I don't see why we should make it harder for users
who want to stay away from the browser as much as possible.

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Daniel P. Berrangé 1 week ago
On Wed, May 13, 2020 at 11:51:50AM +0200, Peter Krempa wrote:
> On Wed, May 13, 2020 at 10:41:23 +0100, Daniel Berrange wrote:
> > On Wed, May 13, 2020 at 11:34:05AM +0200, Andrea Bolognani wrote:
> > > On Wed, 2020-05-13 at 10:58 +0200, Ján Tomko wrote:
> > > > This is not yet supported by virtiofsd.
> > > > 
> > > > Fixes #23
> > > 
> > > Please include the full URL here:
> > > 
> > >   https://gitlab.com/libvirt/libvirt/-/issues/23
> 
> While I definitely don't like gitlab's URIs ...
> 
> > Using the '#23' format is the recommended way. GitLab UI will
> > turn it into a hyperlink, and automatically close the mentioned
> > issue.
> 
> ... just mentioning #23 is useless for users looking at the git repo
> directly (not through the web-ui) or if we at some point decide to
> abandon gitlab.
> 
> Even if gitlab can't handle the automation of closing the comment when
> the URI is mentioned I don't see why we should make it harder for users
> who want to stay away from the browser as much as possible.

The point is for libvirt to follow normal practice from GitLab, so that
contributors don't have to know about libvirt specific rules for contributing
to the project. Telling users to change the normal issue syntax into a URL
whenever they send a patch is not something we should be doing.

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 :|

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Ján Tomko 1 week ago
On a Wednesday in 2020, Daniel P. Berrangé wrote:
>On Wed, May 13, 2020 at 11:51:50AM +0200, Peter Krempa wrote:
>> On Wed, May 13, 2020 at 10:41:23 +0100, Daniel Berrange wrote:
>> > On Wed, May 13, 2020 at 11:34:05AM +0200, Andrea Bolognani wrote:
>> > > On Wed, 2020-05-13 at 10:58 +0200, Ján Tomko wrote:
>> > > > This is not yet supported by virtiofsd.
>> > > >
>> > > > Fixes #23
>> > >
>> > > Please include the full URL here:
>> > >
>> > >   https://gitlab.com/libvirt/libvirt/-/issues/23
>>
>> While I definitely don't like gitlab's URIs ...
>>
>> > Using the '#23' format is the recommended way. GitLab UI will
>> > turn it into a hyperlink, and automatically close the mentioned
>> > issue.
>>
>> ... just mentioning #23 is useless for users looking at the git repo
>> directly (not through the web-ui) or if we at some point decide to
>> abandon gitlab.
>>
>> Even if gitlab can't handle the automation of closing the comment when
>> the URI is mentioned I don't see why we should make it harder for users
>> who want to stay away from the browser as much as possible.
>
>The point is for libvirt to follow normal practice from GitLab, so that
>contributors don't have to know about libvirt specific rules for contributing
>to the project. Telling users to change the normal issue syntax into a URL
>whenever they send a patch is not something we should be doing.
>

Okay, I'll roll a die and push it one of with:
1) no issue reference
2) #23
3) https://gitlab.com/libvirt/libvirt/-/issues/23
4) #23 and https://gitlab.com/libvirt/libvirt/-/issues/23
5) https://www.youtube.com/watch?v=dQw4w9WgXcQ
6) https://gitlab.com/libvirt/libvirt/-/issues/23 and #23

...

Jano

>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 :|
>

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Peter Krempa 1 week ago
On Wed, May 13, 2020 at 10:57:33 +0100, Daniel Berrange wrote:
> On Wed, May 13, 2020 at 11:51:50AM +0200, Peter Krempa wrote:
> > On Wed, May 13, 2020 at 10:41:23 +0100, Daniel Berrange wrote:
> > > On Wed, May 13, 2020 at 11:34:05AM +0200, Andrea Bolognani wrote:
> > > > On Wed, 2020-05-13 at 10:58 +0200, Ján Tomko wrote:
> > > > > This is not yet supported by virtiofsd.
> > > > > 
> > > > > Fixes #23
> > > > 
> > > > Please include the full URL here:
> > > > 
> > > >   https://gitlab.com/libvirt/libvirt/-/issues/23
> > 
> > While I definitely don't like gitlab's URIs ...
> > 
> > > Using the '#23' format is the recommended way. GitLab UI will
> > > turn it into a hyperlink, and automatically close the mentioned
> > > issue.
> > 
> > ... just mentioning #23 is useless for users looking at the git repo
> > directly (not through the web-ui) or if we at some point decide to
> > abandon gitlab.
> > 
> > Even if gitlab can't handle the automation of closing the comment when
> > the URI is mentioned I don't see why we should make it harder for users
> > who want to stay away from the browser as much as possible.
> 
> The point is for libvirt to follow normal practice from GitLab, so that
> contributors don't have to know about libvirt specific rules for contributing
> to the project. Telling users to change the normal issue syntax into a URL
> whenever they send a patch is not something we should be doing.

Even if they are flawed?! Just mentioning a random number in the commit
message may be nice for people looking at the code via web-ui. That is
nice but you can't do much there. But I and many other look at the code
in the local checkout and 'git' doesn't randomly decide to expand the
number to the gitlab uri and make it clickable.

Now if I'd like to look at the issue after some time I'll e.g. when
going through git-blame I'll have to open the browser, open gitlab, find
the project and find the issue rather than just click a link. That's
stupid. If gitlab can't deal with the link, it's frankly broken and we
should not give in to a broken approach just because everybody else
does.

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Daniel P. Berrangé 1 week ago
On Wed, May 13, 2020 at 12:05:44PM +0200, Peter Krempa wrote:
> On Wed, May 13, 2020 at 10:57:33 +0100, Daniel Berrange wrote:
> > On Wed, May 13, 2020 at 11:51:50AM +0200, Peter Krempa wrote:
> > > On Wed, May 13, 2020 at 10:41:23 +0100, Daniel Berrange wrote:
> > > > On Wed, May 13, 2020 at 11:34:05AM +0200, Andrea Bolognani wrote:
> > > > > On Wed, 2020-05-13 at 10:58 +0200, Ján Tomko wrote:
> > > > > > This is not yet supported by virtiofsd.
> > > > > > 
> > > > > > Fixes #23
> > > > > 
> > > > > Please include the full URL here:
> > > > > 
> > > > >   https://gitlab.com/libvirt/libvirt/-/issues/23
> > > 
> > > While I definitely don't like gitlab's URIs ...
> > > 
> > > > Using the '#23' format is the recommended way. GitLab UI will
> > > > turn it into a hyperlink, and automatically close the mentioned
> > > > issue.
> > > 
> > > ... just mentioning #23 is useless for users looking at the git repo
> > > directly (not through the web-ui) or if we at some point decide to
> > > abandon gitlab.
> > > 
> > > Even if gitlab can't handle the automation of closing the comment when
> > > the URI is mentioned I don't see why we should make it harder for users
> > > who want to stay away from the browser as much as possible.
> > 
> > The point is for libvirt to follow normal practice from GitLab, so that
> > contributors don't have to know about libvirt specific rules for contributing
> > to the project. Telling users to change the normal issue syntax into a URL
> > whenever they send a patch is not something we should be doing.
> 
> Even if they are flawed?! Just mentioning a random number in the commit
> message may be nice for people looking at the code via web-ui. That is
> nice but you can't do much there. But I and many other look at the code
> in the local checkout and 'git' doesn't randomly decide to expand the
> number to the gitlab uri and make it clickable.
> 
> Now if I'd like to look at the issue after some time I'll e.g. when
> going through git-blame I'll have to open the browser, open gitlab, find
> the project and find the issue rather than just click a link. That's
> stupid. If gitlab can't deal with the link, it's frankly broken and we
> should not give in to a broken approach just because everybody else
> does.

It isn't about giving in. Again the point is to not needlessly create
special rules for contributing to libvirt, because every special rule
we add is another thing for contributors to stumble over. Some rules
are worth it because they have meaningful benefits such as the use of
Signed-off-by/DCO. The mentioning of full URLs instead of the normal
issue reference syntax does not have a meaningful benefit that 
justifies a libvirt special rule for contributions.

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 :|

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Peter Krempa 1 week ago
On Wed, May 13, 2020 at 11:20:07 +0100, Daniel Berrange wrote:
> On Wed, May 13, 2020 at 12:05:44PM +0200, Peter Krempa wrote:
> > On Wed, May 13, 2020 at 10:57:33 +0100, Daniel Berrange wrote:
> > > On Wed, May 13, 2020 at 11:51:50AM +0200, Peter Krempa wrote:

[...]

> > > The point is for libvirt to follow normal practice from GitLab, so that
> > > contributors don't have to know about libvirt specific rules for contributing
> > > to the project. Telling users to change the normal issue syntax into a URL
> > > whenever they send a patch is not something we should be doing.
> > 
> > Even if they are flawed?! Just mentioning a random number in the commit
> > message may be nice for people looking at the code via web-ui. That is
> > nice but you can't do much there. But I and many other look at the code
> > in the local checkout and 'git' doesn't randomly decide to expand the
> > number to the gitlab uri and make it clickable.
> > 
> > Now if I'd like to look at the issue after some time I'll e.g. when
> > going through git-blame I'll have to open the browser, open gitlab, find
> > the project and find the issue rather than just click a link. That's
> > stupid. If gitlab can't deal with the link, it's frankly broken and we
> > should not give in to a broken approach just because everybody else
> > does.
> 
> It isn't about giving in. Again the point is to not needlessly create
> special rules for contributing to libvirt, because every special rule
> we add is another thing for contributors to stumble over. Some rules
> are worth it because they have meaningful benefits such as the use of
> Signed-off-by/DCO. The mentioning of full URLs instead of the normal
> issue reference syntax does not have a meaningful benefit that 
> justifies a libvirt special rule for contributions.

I gave an examples of two specific meaningful benefit above:

1) it provides a clickable link without second guessing where to go for
command line users
2) provides stable reference to the hosting of issues

Note that for example github uses exactly the same format for
referencing issues. That means that it's unclear what we are referring.

Just observe the following behaviours on the commit that was just pushed
and Jano helpfully for this demonstration included both versions:

1) gitlab: libvirt/libvirt

https://gitlab.com/libvirt/libvirt/-/commit/9c58b6eb0090b978f44299b22ff4f8d3be3a240f

Here I'd expect a clickable link but there isn't one. I have no idea
why.

You can see that gitlab doesn't recognize it. I'm not sure whether the
format Jano chose is bad, but this already enforces some kind of form we
need to conform to.

2) gitlab: pipo.sk/libvirt

https://gitlab.com/pipo.sk/libvirt/-/commit/9c58b6eb0090b978f44299b22ff4f8d3be3a240f

Unsurprisingly it doesn't work here, but the full link is shortened and
still points to the correct issue.

3) github: libvirt/libvirt

https://github.com/libvirt/libvirt/commit/9c58b6eb0090b978f44299b22ff4f8d3be3a240f

As expected #23 points to nothing, but the full link still points to the
issue even cross-hosting.

4) github: pipo/libvirt (plus I've created 23 test issues in my fork)

https://gitlab.com/pipo.sk/libvirt/-/commit/9c58b6eb0090b978f44299b22ff4f8d3be3a240f

Now this is fun. '#23' points to the local issue #23 in my fork while
the full link still points to the issue.


The shortened issue names are ambiguous and the hosting has no way in
figuring out where to point to. Providing full URL is not something
which should be described as "no meaningful benefit" but it actively
disambiguates the links regardless of where it's hosted or refered from.

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Daniel P. Berrangé 1 week ago
On Wed, May 13, 2020 at 01:06:58PM +0200, Peter Krempa wrote:
> On Wed, May 13, 2020 at 11:20:07 +0100, Daniel Berrange wrote:
> > On Wed, May 13, 2020 at 12:05:44PM +0200, Peter Krempa wrote:
> > > On Wed, May 13, 2020 at 10:57:33 +0100, Daniel Berrange wrote:
> > > > On Wed, May 13, 2020 at 11:51:50AM +0200, Peter Krempa wrote:
> 
> [...]
> 
> > > > The point is for libvirt to follow normal practice from GitLab, so that
> > > > contributors don't have to know about libvirt specific rules for contributing
> > > > to the project. Telling users to change the normal issue syntax into a URL
> > > > whenever they send a patch is not something we should be doing.
> > > 
> > > Even if they are flawed?! Just mentioning a random number in the commit
> > > message may be nice for people looking at the code via web-ui. That is
> > > nice but you can't do much there. But I and many other look at the code
> > > in the local checkout and 'git' doesn't randomly decide to expand the
> > > number to the gitlab uri and make it clickable.
> > > 
> > > Now if I'd like to look at the issue after some time I'll e.g. when
> > > going through git-blame I'll have to open the browser, open gitlab, find
> > > the project and find the issue rather than just click a link. That's
> > > stupid. If gitlab can't deal with the link, it's frankly broken and we
> > > should not give in to a broken approach just because everybody else
> > > does.
> > 
> > It isn't about giving in. Again the point is to not needlessly create
> > special rules for contributing to libvirt, because every special rule
> > we add is another thing for contributors to stumble over. Some rules
> > are worth it because they have meaningful benefits such as the use of
> > Signed-off-by/DCO. The mentioning of full URLs instead of the normal
> > issue reference syntax does not have a meaningful benefit that 
> > justifies a libvirt special rule for contributions.
> 
> I gave an examples of two specific meaningful benefit above:
> 
> 1) it provides a clickable link without second guessing where to go for
> command line users
> 2) provides stable reference to the hosting of issues
> 
> Note that for example github uses exactly the same format for
> referencing issues. That means that it's unclear what we are referring.
> 
> Just observe the following behaviours on the commit that was just pushed
> and Jano helpfully for this demonstration included both versions:
> 
> 1) gitlab: libvirt/libvirt
> 
> https://gitlab.com/libvirt/libvirt/-/commit/9c58b6eb0090b978f44299b22ff4f8d3be3a240f
> 
> Here I'd expect a clickable link but there isn't one. I have no idea
> why.
> 
> You can see that gitlab doesn't recognize it. I'm not sure whether the
> format Jano chose is bad, but this already enforces some kind of form we
> need to conform to.
> 
> 2) gitlab: pipo.sk/libvirt
> 
> https://gitlab.com/pipo.sk/libvirt/-/commit/9c58b6eb0090b978f44299b22ff4f8d3be3a240f
> 
> Unsurprisingly it doesn't work here, but the full link is shortened and
> still points to the correct issue.
> 
> 3) github: libvirt/libvirt
> 
> https://github.com/libvirt/libvirt/commit/9c58b6eb0090b978f44299b22ff4f8d3be3a240f
> 
> As expected #23 points to nothing, but the full link still points to the
> issue even cross-hosting.
> 
> 4) github: pipo/libvirt (plus I've created 23 test issues in my fork)
> 
> https://gitlab.com/pipo.sk/libvirt/-/commit/9c58b6eb0090b978f44299b22ff4f8d3be3a240f
> 
> Now this is fun. '#23' points to the local issue #23 in my fork while
> the full link still points to the issue.
> 
> 
> The shortened issue names are ambiguous and the hosting has no way in
> figuring out where to point to. Providing full URL is not something
> which should be described as "no meaningful benefit" but it actively
> disambiguates the links regardless of where it's hosted or refered from.

Ok, I'll withdraw my objection to use of full URLs.

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 :|

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Peter Krempa 1 week ago
On Wed, May 13, 2020 at 13:06:58 +0200, Peter Krempa wrote:
> On Wed, May 13, 2020 at 11:20:07 +0100, Daniel Berrange wrote:
> > On Wed, May 13, 2020 at 12:05:44PM +0200, Peter Krempa wrote:
> > > On Wed, May 13, 2020 at 10:57:33 +0100, Daniel Berrange wrote:
> > > > On Wed, May 13, 2020 at 11:51:50AM +0200, Peter Krempa wrote:
> 
> [...]
> 
> > > > The point is for libvirt to follow normal practice from GitLab, so that
> > > > contributors don't have to know about libvirt specific rules for contributing
> > > > to the project. Telling users to change the normal issue syntax into a URL
> > > > whenever they send a patch is not something we should be doing.
> > > 
> > > Even if they are flawed?! Just mentioning a random number in the commit
> > > message may be nice for people looking at the code via web-ui. That is
> > > nice but you can't do much there. But I and many other look at the code
> > > in the local checkout and 'git' doesn't randomly decide to expand the
> > > number to the gitlab uri and make it clickable.
> > > 
> > > Now if I'd like to look at the issue after some time I'll e.g. when
> > > going through git-blame I'll have to open the browser, open gitlab, find
> > > the project and find the issue rather than just click a link. That's
> > > stupid. If gitlab can't deal with the link, it's frankly broken and we
> > > should not give in to a broken approach just because everybody else
> > > does.
> > 
> > It isn't about giving in. Again the point is to not needlessly create
> > special rules for contributing to libvirt, because every special rule
> > we add is another thing for contributors to stumble over. Some rules
> > are worth it because they have meaningful benefits such as the use of
> > Signed-off-by/DCO. The mentioning of full URLs instead of the normal
> > issue reference syntax does not have a meaningful benefit that 
> > justifies a libvirt special rule for contributions.
> 
> I gave an examples of two specific meaningful benefit above:
> 
> 1) it provides a clickable link without second guessing where to go for
> command line users
> 2) provides stable reference to the hosting of issues
> 
> Note that for example github uses exactly the same format for
> referencing issues. That means that it's unclear what we are referring.
> 
> Just observe the following behaviours on the commit that was just pushed
> and Jano helpfully for this demonstration included both versions:
> 
> 1) gitlab: libvirt/libvirt
> 
> https://gitlab.com/libvirt/libvirt/-/commit/9c58b6eb0090b978f44299b22ff4f8d3be3a240f
> 
> Here I'd expect a clickable link but there isn't one. I have no idea
> why.
> 
> You can see that gitlab doesn't recognize it. I'm not sure whether the
> format Jano chose is bad, but this already enforces some kind of form we
> need to conform to.
> 
> 2) gitlab: pipo.sk/libvirt
> 
> https://gitlab.com/pipo.sk/libvirt/-/commit/9c58b6eb0090b978f44299b22ff4f8d3be3a240f
> 
> Unsurprisingly it doesn't work here, but the full link is shortened and
> still points to the correct issue.
> 
> 3) github: libvirt/libvirt
> 
> https://github.com/libvirt/libvirt/commit/9c58b6eb0090b978f44299b22ff4f8d3be3a240f
> 
> As expected #23 points to nothing, but the full link still points to the
> issue even cross-hosting.
> 
> 4) github: pipo/libvirt (plus I've created 23 test issues in my fork)
> 
> https://gitlab.com/pipo.sk/libvirt/-/commit/9c58b6eb0090b978f44299b22ff4f8d3be3a240f

Oops, messed up the most interesting one:

https://github.com/pipo/libvirt/commit/9c58b6eb0090b978f44299b22ff4f8d3be3a240f

> 
> Now this is fun. '#23' points to the local issue #23 in my fork while
> the full link still points to the issue.
> 
> 
> The shortened issue names are ambiguous and the hosting has no way in
> figuring out where to point to. Providing full URL is not something
> which should be described as "no meaningful benefit" but it actively
> disambiguates the links regardless of where it's hosted or refered from.
> 

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Andrea Bolognani 1 week ago
On Wed, 2020-05-13 at 13:06 +0200, Peter Krempa wrote:
> On Wed, May 13, 2020 at 11:20:07 +0100, Daniel Berrange wrote:
> > It isn't about giving in. Again the point is to not needlessly create
> > special rules for contributing to libvirt, because every special rule
> > we add is another thing for contributors to stumble over. Some rules
> > are worth it because they have meaningful benefits such as the use of
> > Signed-off-by/DCO. The mentioning of full URLs instead of the normal
> > issue reference syntax does not have a meaningful benefit that 
> > justifies a libvirt special rule for contributions.
> 
> I gave an examples of two specific meaningful benefit above:
> 
> 1) it provides a clickable link without second guessing where to go for
> command line users
> 2) provides stable reference to the hosting of issues
> 
> Note that for example github uses exactly the same format for
> referencing issues. That means that it's unclear what we are referring.
> 
[...]
> 
> The shortened issue names are ambiguous and the hosting has no way in
> figuring out where to point to. Providing full URL is not something
> which should be described as "no meaningful benefit" but it actively
> disambiguates the links regardless of where it's hosted or refered from.

I completely agree, #nnn is too ambiguous to be useful.

It seems that GitLab happily accepts full URLs instead of shortened
identifiers:

  https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#default-closing-pattern

We can have a simple prebuild check, similar to the one we already
use for DCO checking, which catches uses of

  Fixes #nnn

and similar and tells contributors to use a full URL instead.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Daniel P. Berrangé 1 week ago
On Wed, May 13, 2020 at 01:19:35PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-05-13 at 13:06 +0200, Peter Krempa wrote:
> > On Wed, May 13, 2020 at 11:20:07 +0100, Daniel Berrange wrote:
> > > It isn't about giving in. Again the point is to not needlessly create
> > > special rules for contributing to libvirt, because every special rule
> > > we add is another thing for contributors to stumble over. Some rules
> > > are worth it because they have meaningful benefits such as the use of
> > > Signed-off-by/DCO. The mentioning of full URLs instead of the normal
> > > issue reference syntax does not have a meaningful benefit that 
> > > justifies a libvirt special rule for contributions.
> > 
> > I gave an examples of two specific meaningful benefit above:
> > 
> > 1) it provides a clickable link without second guessing where to go for
> > command line users
> > 2) provides stable reference to the hosting of issues
> > 
> > Note that for example github uses exactly the same format for
> > referencing issues. That means that it's unclear what we are referring.
> > 
> [...]
> > 
> > The shortened issue names are ambiguous and the hosting has no way in
> > figuring out where to point to. Providing full URL is not something
> > which should be described as "no meaningful benefit" but it actively
> > disambiguates the links regardless of where it's hosted or refered from.
> 
> I completely agree, #nnn is too ambiguous to be useful.

The widespread usage by any other project using GitLab/GitHub proves
otherwise and libvirt isn't special in this regard. We're all 
smart enough to understand this.

> We can have a simple prebuild check, similar to the one we already
> use for DCO checking, which catches uses of
> 
>   Fixes #nnn
> 
> and similar and tells contributors to use a full URL instead.

This is needless extra work for contributors.

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 :|

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Jiri Denemark 1 week ago
On Wed, May 13, 2020 at 12:30:48 +0100, Daniel P. Berrangé wrote:
> On Wed, May 13, 2020 at 01:19:35PM +0200, Andrea Bolognani wrote:
...
> > I completely agree, #nnn is too ambiguous to be useful.
> 
> The widespread usage by any other project using GitLab/GitHub proves
> otherwise and libvirt isn't special in this regard.

This just means that either everyone uses the reference only as a simple
way of closing the associated issue or they use the web UI to look at
commits.

> We're all smart enough to understand this.

It's not really about understanding how to get to the issue. It's about
the effort needed to find the issue. I guess for most commits with the
short reference looking at the original issue would not be worth the
effort of finding it. Which is fine, we can safe time by not looking at
unimportant links.

> > We can have a simple prebuild check, similar to the one we already
> > use for DCO checking, which catches uses of
> > 
> >   Fixes #nnn
> > 
> > and similar and tells contributors to use a full URL instead.
> 
> This is needless extra work for contributors.

I guess I'm a bit strange, but to me pasting a complete link to an issue
is (if I even know such issue exists, of course) less work than
selecting just the number and prefixing it with '#'.

Either way I think fighting about such insignificant point is not the
best use of our time. Let's save it for more important battles we're
going to face, such as the switch to merge requests.

Jirka

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Andrea Bolognani 1 week ago
On Wed, 2020-05-13 at 15:00 +0200, Jiri Denemark wrote:
> On Wed, May 13, 2020 at 12:30:48 +0100, Daniel P. Berrangé wrote:
> > On Wed, May 13, 2020 at 01:19:35PM +0200, Andrea Bolognani wrote:
> > > I completely agree, #nnn is too ambiguous to be useful.
> > 
> > The widespread usage by any other project using GitLab/GitHub proves
> > otherwise and libvirt isn't special in this regard.
> 
> This just means that either everyone uses the reference only as a simple
> way of closing the associated issue or they use the web UI to look at
> commits.

Let's just hope they don't use the "wrong" Web UI, for example by
looking at the GitHub mirror, because otherwise they might be sent
to a completely wrong issue, as Peter just demonstrated.

> > > We can have a simple prebuild check, similar to the one we already
> > > use for DCO checking, which catches uses of
> > > 
> > >   Fixes #nnn
> > > 
> > > and similar and tells contributors to use a full URL instead.
> > 
> > This is needless extra work for contributors.
> 
> I guess I'm a bit strange, but to me pasting a complete link to an issue
> is (if I even know such issue exists, of course) less work than
> selecting just the number and prefixing it with '#'.

If that's the case, then at least know that you're not the only one
who's strange around here :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Ján Tomko 1 week ago
On a Wednesday in 2020, Andrea Bolognani wrote:
>On Wed, 2020-05-13 at 13:06 +0200, Peter Krempa wrote:
>> On Wed, May 13, 2020 at 11:20:07 +0100, Daniel Berrange wrote:
>> > It isn't about giving in. Again the point is to not needlessly create
>> > special rules for contributing to libvirt, because every special rule
>> > we add is another thing for contributors to stumble over. Some rules
>> > are worth it because they have meaningful benefits such as the use of
>> > Signed-off-by/DCO. The mentioning of full URLs instead of the normal
>> > issue reference syntax does not have a meaningful benefit that
>> > justifies a libvirt special rule for contributions.
>>
>> I gave an examples of two specific meaningful benefit above:
>>
>> 1) it provides a clickable link without second guessing where to go for
>> command line users
>> 2) provides stable reference to the hosting of issues
>>
>> Note that for example github uses exactly the same format for
>> referencing issues. That means that it's unclear what we are referring.
>>
>[...]
>>
>> The shortened issue names are ambiguous and the hosting has no way in
>> figuring out where to point to. Providing full URL is not something
>> which should be described as "no meaningful benefit" but it actively
>> disambiguates the links regardless of where it's hosted or refered from.
>
>I completely agree, #nnn is too ambiguous to be useful.
>
>It seems that GitLab happily accepts full URLs instead of shortened
>identifiers:
>
>  https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#default-closing-pattern
>
>We can have a simple prebuild check, similar to the one we already
>use for DCO checking, which catches uses of
>
>  Fixes #nnn
>
>and similar and tells contributors to use a full URL instead.
>

For DCO, the idea is that you specifically need the user to act on it.

If we switch to merge requests, this coudl be handled by the same bot
that applies the R-b tags, right? :)

Jano

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Pino Toscano 1 week ago
On Wednesday, 13 May 2020 10:58:51 CEST Ján Tomko wrote:
> +        if (fs->readonly) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("virtiofs does not yet supported read-only mode"));

s/supported/support/

-- 
Pino Toscano

Re: [libvirt PATCH] qemu: reject readonly attribute for virtiofs

Posted by Daniel P. Berrangé 1 week ago
On Wed, May 13, 2020 at 10:58:51AM +0200, Ján Tomko wrote:
> This is not yet supported by virtiofsd.
> 
> Fixes #23
> 
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> ---
>  src/qemu/qemu_validate.c                      |  5 +++
>  .../vhost-user-fs-readonly.xml                | 44 +++++++++++++++++++
>  tests/qemuxml2argvtest.c                      |  1 +
>  3 files changed, 50 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/vhost-user-fs-readonly.xml

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|