[libvirt] [PATCH] virsh: fixed handling of targetless disks (e.g. empty CDROM) in 'domblkinfo' cmd
virDomainGetBlockInfo() returns error if called on a disk with no target (a targetless disk might be a removable media drive with no media in it, for instance an empty CDROM drive). So far this caused the virsh domblkinfo --all command to abort and ignore any remaining (not yet displayed) disk devices. This patch fixes it by ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives, similar to how it's done for network drives. https://bugzilla.redhat.com/show_bug.cgi?id=1619625 Signed-off-by: Pavel Mores <pmores@redhat.com> --- tools/virsh-domain-monitor.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 0e2c4191d7..0f495c1a3f 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) char *cap = NULL; char *alloc = NULL; char *phy = NULL; + char *device_type = NULL; vshTablePtr table = NULL; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) @@ -510,6 +511,8 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) rc = virDomainGetBlockInfo(dom, target, &info, 0); if (rc < 0) { + device_type = virXPathString("string(./@device)", ctxt); + /* If protocol is present that's an indication of a networked * storage device which cannot provide statistics, so generate * 0 based data and get the next disk. */ @@ -518,9 +521,16 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) virGetLastErrorDomain() == VIR_FROM_STORAGE) { memset(&info, 0, sizeof(info)); vshResetLibvirtError(); + } else if (device_type != NULL && + (STRCASEEQ(device_type, "cdrom") || + STRCASEEQ(device_type, "floppy"))) { + memset(&info, 0, sizeof(info)); + vshResetLibvirtError(); } else { goto cleanup; } + + VIR_FREE(device_type); } if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human)) @@ -556,6 +566,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) VIR_FREE(target); VIR_FREE(protocol); VIR_FREE(disks); + VIR_FREE(device_type); xmlXPathFreeContext(ctxt); xmlFreeDoc(xmldoc); return ret; -- 2.21.0
Real estate in the commit message summary is precious, so the e.g. part belongs in the commit message. Also, (unless some strange usage sneaked into our XML), <source> deals with the host-side where <target> says how the device appears in the guest, so 'targetless' is wrong here. On Tue, Oct 01, 2019 at 04:45:02PM +0200, Pavel Mores wrote:
virDomainGetBlockInfo() returns error if called on a disk with no target
s/target/source/
(a targetless disk might be a removable media drive with no media in it, for instance an empty CDROM drive).
So far this caused the virsh domblkinfo --all command to abort and ignore
Most of virsh commands correspond 1:1 to a libvirt API. It would be nicer if that was the case here, since now we have to guess what happened in the daemon, because we try to treat --all as --all-that-are-sensible-and-supported
any remaining (not yet displayed) disk devices. This patch fixes it by ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives, similar to how it's done for network drives.
I don't think that's an approach that should be copied, see below
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
Signed-off-by: Pavel Mores <pmores@redhat.com> --- tools/virsh-domain-monitor.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 0e2c4191d7..0f495c1a3f 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) char *cap = NULL; char *alloc = NULL; char *phy = NULL; + char *device_type = NULL; vshTablePtr table = NULL;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) @@ -510,6 +511,8 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) rc = virDomainGetBlockInfo(dom, target, &info, 0);
if (rc < 0) { + device_type = virXPathString("string(./@device)", ctxt); + /* If protocol is present that's an indication of a networked * storage device which cannot provide statistics, so generate * 0 based data and get the next disk. */
This comment is misleading, we should be capable of getting statistics from gluster even for inactive domains. Which is probably the reason we only look for the presence of protocol if the API failed.
@@ -518,9 +521,16 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) virGetLastErrorDomain() == VIR_FROM_STORAGE) { memset(&info, 0, sizeof(info)); vshResetLibvirtError(); + } else if (device_type != NULL && + (STRCASEEQ(device_type, "cdrom") || + STRCASEEQ(device_type, "floppy"))) { + memset(&info, 0, sizeof(info)); + vshResetLibvirtError();
What if the cdrom is not empty and the error was something else? To preserve that, doing a virXPathBoolean query on the source element should say whether it makes sense to invoke the API or not. (Maybe fill in dashes instead of zeroes as proposed in the discussion last time [0]) Alternatively, we can resurrect that patch [1] that optimistically ignored all errors and save ourselves the trouble of having to add another exception here later. Jano
} else { goto cleanup; } + + VIR_FREE(device_type); }
if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human))
[0] https://www.redhat.com/archives/libvir-list/2018-August/msg01332.html [1] https://www.redhat.com/archives/libvir-list/2018-August/msg01300.html
@@ -556,6 +566,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) VIR_FREE(target); VIR_FREE(protocol); VIR_FREE(disks); + VIR_FREE(device_type); xmlXPathFreeContext(ctxt); xmlFreeDoc(xmldoc); return ret; -- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Oct 02, 2019 at 01:15:12PM +0200, Ján Tomko wrote:
Real estate in the commit message summary is precious, so the e.g. part belongs in the commit message.
Right.
Also, (unless some strange usage sneaked into our XML), <source> deals with the host-side where <target> says how the device appears in the guest, so 'targetless' is wrong here.
On Tue, Oct 01, 2019 at 04:45:02PM +0200, Pavel Mores wrote:
virDomainGetBlockInfo() returns error if called on a disk with no target
s/target/source/
Yes, sorry for the mixup.
(a targetless disk might be a removable media drive with no media in it, for instance an empty CDROM drive).
So far this caused the virsh domblkinfo --all command to abort and ignore
Most of virsh commands correspond 1:1 to a libvirt API. It would be nicer if that was the case here, since now we have to guess what happened in the daemon, because we try to treat --all as --all-that-are-sensible-and-supported
Could you please elaborate? I don't have enough context yet, I'm not sure what in particular the above means for this patch.
any remaining (not yet displayed) disk devices. This patch fixes it by ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives, similar to how it's done for network drives.
I don't think that's an approach that should be copied, see below
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
Signed-off-by: Pavel Mores <pmores@redhat.com> --- tools/virsh-domain-monitor.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 0e2c4191d7..0f495c1a3f 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) char *cap = NULL; char *alloc = NULL; char *phy = NULL; + char *device_type = NULL; vshTablePtr table = NULL;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) @@ -510,6 +511,8 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) rc = virDomainGetBlockInfo(dom, target, &info, 0);
if (rc < 0) { + device_type = virXPathString("string(./@device)", ctxt); + /* If protocol is present that's an indication of a networked * storage device which cannot provide statistics, so generate * 0 based data and get the next disk. */
This comment is misleading, we should be capable of getting statistics from gluster even for inactive domains. Which is probably the reason we only look for the presence of protocol if the API failed.
@@ -518,9 +521,16 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) virGetLastErrorDomain() == VIR_FROM_STORAGE) { memset(&info, 0, sizeof(info)); vshResetLibvirtError(); + } else if (device_type != NULL && + (STRCASEEQ(device_type, "cdrom") || + STRCASEEQ(device_type, "floppy"))) { + memset(&info, 0, sizeof(info)); + vshResetLibvirtError();
What if the cdrom is not empty and the error was something else?
Whatever error occurs, you get dashes instead of actual numbers.
To preserve that, doing a virXPathBoolean query on the source element should say whether it makes sense to invoke the API or not. (Maybe fill in dashes instead of zeroes as proposed in the discussion last time [0])
Don't know if that's what you're talking about but I already get a dashed output. For instance: virsh # domblkinfo --all archlinux Target Capacity Allocation Physical -------------------------------------------------- vda 16106127360 6850265088 16108814336 sda - - - fda - - - If querying <source> beforehand is the right way I'm happy to fix the patch.
Alternatively, we can resurrect that patch [1] that optimistically ignored all errors and save ourselves the trouble of having to add another exception here later.
Jano
} else { goto cleanup; } + + VIR_FREE(device_type); }
if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human))
[0] https://www.redhat.com/archives/libvir-list/2018-August/msg01332.html [1] https://www.redhat.com/archives/libvir-list/2018-August/msg01300.html
@@ -556,6 +566,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) VIR_FREE(target); VIR_FREE(protocol); VIR_FREE(disks); + VIR_FREE(device_type); xmlXPathFreeContext(ctxt); xmlFreeDoc(xmldoc); return ret; -- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Oct 02, 2019 at 02:11:39PM +0200, Pavel Mores wrote:
On Wed, Oct 02, 2019 at 01:15:12PM +0200, Ján Tomko wrote:
Real estate in the commit message summary is precious, so the e.g. part belongs in the commit message.
Right.
Also, (unless some strange usage sneaked into our XML), <source> deals with the host-side where <target> says how the device appears in the guest, so 'targetless' is wrong here.
On Tue, Oct 01, 2019 at 04:45:02PM +0200, Pavel Mores wrote:
virDomainGetBlockInfo() returns error if called on a disk with no target
s/target/source/
Yes, sorry for the mixup.
(a targetless disk might be a removable media drive with no media in it, for instance an empty CDROM drive).
So far this caused the virsh domblkinfo --all command to abort and ignore
Most of virsh commands correspond 1:1 to a libvirt API. It would be nicer if that was the case here, since now we have to guess what happened in the daemon, because we try to treat --all as --all-that-are-sensible-and-supported
Could you please elaborate? I don't have enough context yet, I'm not sure what in particular the above means for this patch.
I mean that I do not consider the answer clear-cut. For a single device invocation a failure in the libvirt API makes the virsh command fail. For multi-device, we have to decide which failures are OK or not.
any remaining (not yet displayed) disk devices. This patch fixes it by ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives, similar to how it's done for network drives.
I don't think that's an approach that should be copied, see below
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
Signed-off-by: Pavel Mores <pmores@redhat.com> --- tools/virsh-domain-monitor.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 0e2c4191d7..0f495c1a3f 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) char *cap = NULL; char *alloc = NULL; char *phy = NULL; + char *device_type = NULL; vshTablePtr table = NULL;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) @@ -510,6 +511,8 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) rc = virDomainGetBlockInfo(dom, target, &info, 0);
if (rc < 0) { + device_type = virXPathString("string(./@device)", ctxt); + /* If protocol is present that's an indication of a networked * storage device which cannot provide statistics, so generate * 0 based data and get the next disk. */
This comment is misleading, we should be capable of getting statistics from gluster even for inactive domains. Which is probably the reason we only look for the presence of protocol if the API failed.
@@ -518,9 +521,16 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) virGetLastErrorDomain() == VIR_FROM_STORAGE) { memset(&info, 0, sizeof(info)); vshResetLibvirtError(); + } else if (device_type != NULL && + (STRCASEEQ(device_type, "cdrom") || + STRCASEEQ(device_type, "floppy"))) { + memset(&info, 0, sizeof(info)); + vshResetLibvirtError();
What if the cdrom is not empty and the error was something else?
Whatever error occurs, you get dashes instead of actual numbers.
Yes, the question is whether an error for something else than an empty source should be reported.
To preserve that, doing a virXPathBoolean query on the source element should say whether it makes sense to invoke the API or not. (Maybe fill in dashes instead of zeroes as proposed in the discussion last time [0])
Don't know if that's what you're talking about but I already get a dashed output. For instance:
virsh # domblkinfo --all archlinux Target Capacity Allocation Physical -------------------------------------------------- vda 16106127360 6850265088 16108814336 sda - - - fda - - -
If querying <source> beforehand is the right way I'm happy to fix the patch.
a) if we care about preserving the individual errors, please check for source and skip the API call instead b) if we do not, we can ignore all errors and simplify the code Either way looks nicer to me than introducing a specific 'ResetError()' call. Jano
Alternatively, we can resurrect that patch [1] that optimistically ignored all errors and save ourselves the trouble of having to add another exception here later.
Jano
On 10/1/19 11:45 AM, Pavel Mores wrote:
virDomainGetBlockInfo() returns error if called on a disk with no target (a targetless disk might be a removable media drive with no media in it, for instance an empty CDROM drive).
So far this caused the virsh domblkinfo --all command to abort and ignore any remaining (not yet displayed) disk devices. This patch fixes it by ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives, similar to how it's done for network drives.
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
Signed-off-by: Pavel Mores <pmores@redhat.com> --- tools/virsh-domain-monitor.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 0e2c4191d7..0f495c1a3f 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) char *cap = NULL; char *alloc = NULL; char *phy = NULL; + char *device_type = NULL;
I believe you need to use VIR_AUTO(char *) here. Even considering that the macro will be deprecated in the near future for glib stuff, by using VIR_AUTO here: - you'll make it easier to introduce the glib replacement, since s/VIR_AUTO/<g_lib_macro> is a trivial change; - you won't be adding more VIR_FREE() on top of existing cases that will need to be addressed in the future. Thanks, DHB
vshTablePtr table = NULL;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) @@ -510,6 +511,8 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) rc = virDomainGetBlockInfo(dom, target, &info, 0);
if (rc < 0) { + device_type = virXPathString("string(./@device)", ctxt); + /* If protocol is present that's an indication of a networked * storage device which cannot provide statistics, so generate * 0 based data and get the next disk. */ @@ -518,9 +521,16 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) virGetLastErrorDomain() == VIR_FROM_STORAGE) { memset(&info, 0, sizeof(info)); vshResetLibvirtError(); + } else if (device_type != NULL && + (STRCASEEQ(device_type, "cdrom") || + STRCASEEQ(device_type, "floppy"))) { + memset(&info, 0, sizeof(info)); + vshResetLibvirtError(); } else { goto cleanup; } + + VIR_FREE(device_type); }
if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human)) @@ -556,6 +566,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) VIR_FREE(target); VIR_FREE(protocol); VIR_FREE(disks); + VIR_FREE(device_type); xmlXPathFreeContext(ctxt); xmlFreeDoc(xmldoc); return ret;
On Wed, Oct 02, 2019 at 08:34:10AM -0300, Daniel Henrique Barboza wrote:
On 10/1/19 11:45 AM, Pavel Mores wrote:
virDomainGetBlockInfo() returns error if called on a disk with no target (a targetless disk might be a removable media drive with no media in it, for instance an empty CDROM drive).
So far this caused the virsh domblkinfo --all command to abort and ignore any remaining (not yet displayed) disk devices. This patch fixes it by ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives, similar to how it's done for network drives.
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
Signed-off-by: Pavel Mores <pmores@redhat.com> --- tools/virsh-domain-monitor.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 0e2c4191d7..0f495c1a3f 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) char *cap = NULL; char *alloc = NULL; char *phy = NULL; + char *device_type = NULL;
I believe you need to use VIR_AUTO(char *) here. Even considering that the macro will be deprecated in the near future for glib stuff, by using VIR_AUTO here:
- you'll make it easier to introduce the glib replacement, since s/VIR_AUTO/<g_lib_macro> is a trivial change;
- you won't be adding more VIR_FREE() on top of existing cases that will need to be addressed in the future.
Was that the consensus from the migration debate? If so I'm happy to fix my patch. pvl
Thanks,
DHB
vshTablePtr table = NULL; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) @@ -510,6 +511,8 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) rc = virDomainGetBlockInfo(dom, target, &info, 0); if (rc < 0) { + device_type = virXPathString("string(./@device)", ctxt); + /* If protocol is present that's an indication of a networked * storage device which cannot provide statistics, so generate * 0 based data and get the next disk. */ @@ -518,9 +521,16 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) virGetLastErrorDomain() == VIR_FROM_STORAGE) { memset(&info, 0, sizeof(info)); vshResetLibvirtError(); + } else if (device_type != NULL && + (STRCASEEQ(device_type, "cdrom") || + STRCASEEQ(device_type, "floppy"))) { + memset(&info, 0, sizeof(info)); + vshResetLibvirtError(); } else { goto cleanup; } + + VIR_FREE(device_type); } if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human)) @@ -556,6 +566,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) VIR_FREE(target); VIR_FREE(protocol); VIR_FREE(disks); + VIR_FREE(device_type); xmlXPathFreeContext(ctxt); xmlFreeDoc(xmldoc); return ret;
On Wed, Oct 02, 2019 at 04:21:52PM +0200, Pavel Mores wrote:
On Wed, Oct 02, 2019 at 08:34:10AM -0300, Daniel Henrique Barboza wrote:
On 10/1/19 11:45 AM, Pavel Mores wrote:
virDomainGetBlockInfo() returns error if called on a disk with no target (a targetless disk might be a removable media drive with no media in it, for instance an empty CDROM drive).
So far this caused the virsh domblkinfo --all command to abort and ignore any remaining (not yet displayed) disk devices. This patch fixes it by ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives, similar to how it's done for network drives.
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
Signed-off-by: Pavel Mores <pmores@redhat.com> --- tools/virsh-domain-monitor.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 0e2c4191d7..0f495c1a3f 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) char *cap = NULL; char *alloc = NULL; char *phy = NULL; + char *device_type = NULL;
I believe you need to use VIR_AUTO(char *) here. Even considering that the macro will be deprecated in the near future for glib stuff, by using VIR_AUTO here:
- you'll make it easier to introduce the glib replacement, since s/VIR_AUTO/<g_lib_macro> is a trivial change;
- you won't be adding more VIR_FREE() on top of existing cases that will need to be addressed in the future.
Was that the consensus from the migration debate? If so I'm happy to fix my patch.
IIUC the consensus was that we will switch to g_auto eventually, but it's a simple string replace that will be dealt with by whoever pushes the patch. Also, if you go with any of the two approaches I suggested (ignoring all errors or using the XPathBoolean), you don't need to introduce a new allocated variable Jano
pvl
On Thu, Oct 03, 2019 at 11:54:44AM +0200, Ján Tomko wrote:
On Wed, Oct 02, 2019 at 04:21:52PM +0200, Pavel Mores wrote:
On Wed, Oct 02, 2019 at 08:34:10AM -0300, Daniel Henrique Barboza wrote:
On 10/1/19 11:45 AM, Pavel Mores wrote:
virDomainGetBlockInfo() returns error if called on a disk with no target (a targetless disk might be a removable media drive with no media in it, for instance an empty CDROM drive).
So far this caused the virsh domblkinfo --all command to abort and ignore any remaining (not yet displayed) disk devices. This patch fixes it by ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives, similar to how it's done for network drives.
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
Signed-off-by: Pavel Mores <pmores@redhat.com> --- tools/virsh-domain-monitor.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 0e2c4191d7..0f495c1a3f 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) char *cap = NULL; char *alloc = NULL; char *phy = NULL; + char *device_type = NULL;
I believe you need to use VIR_AUTO(char *) here. Even considering that the macro will be deprecated in the near future for glib stuff, by using VIR_AUTO here:
- you'll make it easier to introduce the glib replacement, since s/VIR_AUTO/<g_lib_macro> is a trivial change;
- you won't be adding more VIR_FREE() on top of existing cases that will need to be addressed in the future.
Was that the consensus from the migration debate? If so I'm happy to fix my patch.
IIUC the consensus was that we will switch to g_auto eventually, but it's a simple string replace that will be dealt with by whoever pushes the patch.
Also, if you go with any of the two approaches I suggested (ignoring all errors or using the XPathBoolean), you don't need to introduce a new allocated variable
Cool, so if there aren't any objections, I'll implement the XPathBoolean variant and submit v2. pvl
On Thu, Oct 03, 2019 at 01:09:55PM +0200, Pavel Mores wrote:
On Thu, Oct 03, 2019 at 11:54:44AM +0200, Ján Tomko wrote:
On Wed, Oct 02, 2019 at 04:21:52PM +0200, Pavel Mores wrote:
On Wed, Oct 02, 2019 at 08:34:10AM -0300, Daniel Henrique Barboza wrote:
On 10/1/19 11:45 AM, Pavel Mores wrote:
virDomainGetBlockInfo() returns error if called on a disk with no target (a targetless disk might be a removable media drive with no media in it, for instance an empty CDROM drive).
So far this caused the virsh domblkinfo --all command to abort and ignore any remaining (not yet displayed) disk devices. This patch fixes it by ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives, similar to how it's done for network drives.
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
Signed-off-by: Pavel Mores <pmores@redhat.com> --- tools/virsh-domain-monitor.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 0e2c4191d7..0f495c1a3f 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) char *cap = NULL; char *alloc = NULL; char *phy = NULL; + char *device_type = NULL;
I believe you need to use VIR_AUTO(char *) here. Even considering that the macro will be deprecated in the near future for glib stuff, by using VIR_AUTO here:
- you'll make it easier to introduce the glib replacement, since s/VIR_AUTO/<g_lib_macro> is a trivial change;
- you won't be adding more VIR_FREE() on top of existing cases that will need to be addressed in the future.
Was that the consensus from the migration debate? If so I'm happy to fix my patch.
IIUC the consensus was that we will switch to g_auto eventually, but it's a simple string replace that will be dealt with by whoever pushes the patch.
Also, if you go with any of the two approaches I suggested (ignoring all errors or using the XPathBoolean), you don't need to introduce a new allocated variable
Cool, so if there aren't any objections, I'll implement the XPathBoolean variant and submit v2.
As it turns out, the thing with the virDomainGetBlockInfo() call is that even if it fails, it does initialise the virDomainBlockInfo structure enough to be displayed in the table (even if just as a target name and dashes). It follows apparently that if we go the virXPathBoolean() way and skip the virDomainGetBlockInfo() call, we can't show the sourceless device at all, we can't even indicate that it exists. Is that acceptable? Or is there another way to fill in virDomainBlockInfo? Thanks, pvl
On Thu, Oct 03, 2019 at 02:45:36PM +0200, Pavel Mores wrote:
On Thu, Oct 03, 2019 at 01:09:55PM +0200, Pavel Mores wrote:
On Thu, Oct 03, 2019 at 11:54:44AM +0200, Ján Tomko wrote:
On Wed, Oct 02, 2019 at 04:21:52PM +0200, Pavel Mores wrote:
On Wed, Oct 02, 2019 at 08:34:10AM -0300, Daniel Henrique Barboza wrote:
On 10/1/19 11:45 AM, Pavel Mores wrote:
virDomainGetBlockInfo() returns error if called on a disk with no target (a targetless disk might be a removable media drive with no media in it, for instance an empty CDROM drive).
So far this caused the virsh domblkinfo --all command to abort and ignore any remaining (not yet displayed) disk devices. This patch fixes it by ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives, similar to how it's done for network drives.
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
Signed-off-by: Pavel Mores <pmores@redhat.com> --- tools/virsh-domain-monitor.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 0e2c4191d7..0f495c1a3f 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) char *cap = NULL; char *alloc = NULL; char *phy = NULL; + char *device_type = NULL;
I believe you need to use VIR_AUTO(char *) here. Even considering that the macro will be deprecated in the near future for glib stuff, by using VIR_AUTO here:
- you'll make it easier to introduce the glib replacement, since s/VIR_AUTO/<g_lib_macro> is a trivial change;
- you won't be adding more VIR_FREE() on top of existing cases that will need to be addressed in the future.
Was that the consensus from the migration debate? If so I'm happy to fix my patch.
IIUC the consensus was that we will switch to g_auto eventually, but it's a simple string replace that will be dealt with by whoever pushes the patch.
Also, if you go with any of the two approaches I suggested (ignoring all errors or using the XPathBoolean), you don't need to introduce a new allocated variable
Cool, so if there aren't any objections, I'll implement the XPathBoolean variant and submit v2.
As it turns out, the thing with the virDomainGetBlockInfo() call is that even if it fails, it does initialise the virDomainBlockInfo structure enough to be displayed in the table (even if just as a target name and dashes).
The https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainBlockInfo only contains numbers, not the target name. The target name is output by the 'vshTableRowAppend' command.
It follows apparently that if we go the virXPathBoolean() way and skip the virDomainGetBlockInfo() call, we can't show the sourceless device at all, we can't even indicate that it exists.
Is that acceptable? Or is there another way to fill in virDomainBlockInfo?
cmdDomblkinfoGet happily handles a virDomainBlockInfo with all zeroes, so as long as info is reset to zeroes on every loop iteration, it's okay to execute the rest of the loop even if we did not call virDomainGetBlockInfo Jano
Thanks,
pvl
On Thu, Oct 03, 2019 at 06:25:40PM +0200, Ján Tomko wrote:
On Thu, Oct 03, 2019 at 02:45:36PM +0200, Pavel Mores wrote:
On Thu, Oct 03, 2019 at 01:09:55PM +0200, Pavel Mores wrote:
On Thu, Oct 03, 2019 at 11:54:44AM +0200, Ján Tomko wrote:
On Wed, Oct 02, 2019 at 04:21:52PM +0200, Pavel Mores wrote:
On Wed, Oct 02, 2019 at 08:34:10AM -0300, Daniel Henrique Barboza wrote:
On 10/1/19 11:45 AM, Pavel Mores wrote: > virDomainGetBlockInfo() returns error if called on a disk with no target > (a targetless disk might be a removable media drive with no media in it, > for instance an empty CDROM drive). > > So far this caused the virsh domblkinfo --all command to abort and ignore > any remaining (not yet displayed) disk devices. This patch fixes it by > ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives, > similar to how it's done for network drives. > > https://bugzilla.redhat.com/show_bug.cgi?id=1619625 > > Signed-off-by: Pavel Mores <pmores@redhat.com> > --- > tools/virsh-domain-monitor.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c > index 0e2c4191d7..0f495c1a3f 100644 > --- a/tools/virsh-domain-monitor.c > +++ b/tools/virsh-domain-monitor.c > @@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) > char *cap = NULL; > char *alloc = NULL; > char *phy = NULL; > + char *device_type = NULL;
I believe you need to use VIR_AUTO(char *) here. Even considering that the macro will be deprecated in the near future for glib stuff, by using VIR_AUTO here:
- you'll make it easier to introduce the glib replacement, since s/VIR_AUTO/<g_lib_macro> is a trivial change;
- you won't be adding more VIR_FREE() on top of existing cases that will need to be addressed in the future.
Was that the consensus from the migration debate? If so I'm happy to fix my patch.
IIUC the consensus was that we will switch to g_auto eventually, but it's a simple string replace that will be dealt with by whoever pushes the patch.
Also, if you go with any of the two approaches I suggested (ignoring all errors or using the XPathBoolean), you don't need to introduce a new allocated variable
Cool, so if there aren't any objections, I'll implement the XPathBoolean variant and submit v2.
As it turns out, the thing with the virDomainGetBlockInfo() call is that even if it fails, it does initialise the virDomainBlockInfo structure enough to be displayed in the table (even if just as a target name and dashes).
The https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainBlockInfo only contains numbers, not the target name.
The target name is output by the 'vshTableRowAppend' command.
It follows apparently that if we go the virXPathBoolean() way and skip the virDomainGetBlockInfo() call, we can't show the sourceless device at all, we can't even indicate that it exists.
Is that acceptable? Or is there another way to fill in virDomainBlockInfo?
cmdDomblkinfoGet happily handles a virDomainBlockInfo with all zeroes, so as long as info is reset to zeroes on every loop iteration, it's okay to execute the rest of the loop even if we did not call virDomainGetBlockInfo
You're right, the problem wasn't actually caused by the virDomainBlockInfo instance. I'll post a v2 shortly. Thanks! pvl
participants (3)
-
Daniel Henrique Barboza -
Ján Tomko -
Pavel Mores