Re: [Libvir] [PATCH] Another Report error in virsh.c code.
Would you give me a comment on my opinion ? If not, I make a patch that is suitable for my opinion. I forgot to attach a patch... I attach it. If not, please apply it. Shigeki Sakamoto. virsh.patch Description: Binary data -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] PATCH: Add SCSI HBA backend for storage driver
On Mon, Mar 24, 2008 at 11:15:47PM +, Daniel P. Berrange wrote:
And this time with the patch included...
[...]
PARTED_REQUIRED=1.8.0
+HAL_REQUIRED=0.5.0
With HAL being in Solaris now, maybe that could even be made non-linux
specific
[...]
@@ -584,6 +585,8 @@ AC_ARG_WITH(storage-iscsi,
[ --with-storage-iscsiwith iSCSI backend for the storage driver
(on)],[],[with_storage_iscsi=check])
AC_ARG_WITH(storage-disk,
[ --with-storage-disk with GPartd Disk backend for the storage
driver (on)],[],[with_storage_disk=check])
+AC_ARG_WITH(storage-scsi,
+[ --with-storage-scsi with HAL SCSI Disk backend for the storage
driver (on)],[],[with_storage_scsi=check])
if test $with_storage_fs = yes -o $with_storage_fs = check; then
AC_PATH_PROG(MOUNT, [mount], [], [$PATH:/sbin:/usr/sbin])
@@ -741,6 +744,30 @@ AC_SUBST(LIBPARTED_CFLAGS)
AC_SUBST(LIBPARTED_LIBS)
maybe a meta option --with-storage giving a default to the group of options
(that now 5 of them) could be helpful for reduced setups.
[...]
diff -N src/storage_backend_scsi.c
[...]
+#define LINUX_SYSFS_SCSI_HOST_PREFIX /sys/class/scsi_host/
+#define LINUX_SYSFS_SCSI_HOST_POSTFIX /device
hopefully we can add Solaris equivalent later
+if (strlen(absLink) = buflen) {
+virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ link too long for buffer);
+return -1;
+}
+strcpy(buf, absLink);
Even if just checked can we still use strncpy(), i assume at some point
we will add an automatic check against strcpy
[...]
+virStorageBackendSCSICreateVol(virConnectPtr conn,
+ virStoragePoolObjPtr pool,
+ const char *name,
+ const char *path,
+ const char *key,
+ unsigned long long size)
+{
+virStorageVolDefPtr vol;
+char *tmppath;
+
+if ((vol = calloc(1, sizeof(*vol))) == NULL) {
+virStorageReportError(conn, VIR_ERR_NO_MEMORY, %s, _(volume));
+goto cleanup;
+}
maybe check that size is greater than some absolute minimum like 1 MByte
(and maximum size ?) this could also allow to catch some conversion errors
or 0 being passed.
+tmppath = strdup(path);
[...]
+if ((vol-target.path = virStorageBackendStablePath(conn,
+pool,
+tmppath)) == NULL)
+goto cleanup;
+
+if (tmppath != vol-target.path)
+free(tmppath);
+tmppath = NULL;
it's a bit funky, you mean virStorageBackendStablePath could just grab the
parameter string or not and that need to be checked later to decide if it needs
freeing ? Even for an internal API it's a bit dangerous IMHO, what if
it is called with a constant string path, let's avoid the potential problem
get virStorageBackendStablePath to always strdup if it reuses the parameter,
no ?
[...]
+goto cleanup;
+}
+
+#define HAL_GET_PROPERTY(path, name, err, type, value) \
+(value) = libhal_device_get_property_ ## type(ctx, (path), (name),
(err)); \
+if (dbus_error_is_set((err))) { \
+virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, \
+ unable to lookup '%s' property on %s: %s, \
+ (name), (path), derr.message);\
+goto cleanup; \
+}
i'm not too fond of macros defined in the function code, move it just before
Also for token-pasting I though one needed to glue the # and the identifiers
like foo##bar , i'm surprized foo ## bar actually works
+/* These are props of the physical device */
+HAL_GET_PROPERTY(devname, scsi.host, derr, int, host);
+HAL_GET_PROPERTY(devname, scsi.bus, derr, int, bus);
+HAL_GET_PROPERTY(devname, scsi.target, derr, int, target);
+HAL_GET_PROPERTY(devname, scsi.lun, derr, int, lun);
+
+/* These are props of the logic device */
+HAL_GET_PROPERTY(strdevs[0], block.device, derr, string, dev);
+/*
+ * XXX storage.serial is not actually unique if they have
+ * multipath on the fibre channel adapter
+ */
+HAL_GET_PROPERTY(strdevs[0], storage.serial, derr, string, key);
+HAL_GET_PROPERTY(strdevs[0], storage.size, derr, uint64, size);
+
+#undef HAL_GET_PROPERTY
Looks good, and will make easier to test the simple storage management
on developpers machines, +1
thanks !
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
[EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list
Re: [Libvir] [PATCH] reindent __virErrorMsg with spaces instead of tabs
HI Daniel, On Mon, Mar 24, 2008 at 03:56:07AM -0400, Daniel Veillard wrote: Hum, could you send those kind of patches as attachments in the future ? Basically assuming spaces/tabs are correctly preserved in an email body is taking unecessary risks. Also having a name for the patch and being able to download them from on-line email archives is a good thing, O.k., fine. Now for the content of the patch, it seems to remove all tabs and replace them with 8 spaces. Is that really a better way, why use 8 characters when you can use one for the same purpose ? One could argue either way, maybe No, it uses 4 spaces for each indentation level, this is what the other files I checked (mostly qemu_*.c, xml.c) use and remote_internal.c even has: /* * vim: set tabstop=4: * vim: set shiftwidth=4: * vim: set expandtab: */ which is basically what I used for reindenting virterror.c. So the patch is just to make things consistent with the rest of the c code. -- Guido -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] [PATCH] Another Report error in virsh.c code.
On Tue, Mar 25, 2008 at 03:22:41PM +0900, S.Sakamoto wrote: Would you give me a comment on my opinion ? If not, I make a patch that is suitable for my opinion. I forgot to attach a patch... I attach it. If not, please apply it. Shigeki Sakamoto. OK, I've committed your final version. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] [PATCH] reindent __virErrorMsg with spaces instead of tabs
On Tue, Mar 25, 2008 at 11:06:02AM +0100, Guido Günther wrote: HI Daniel, Hi, [...] Now for the content of the patch, it seems to remove all tabs and replace them with 8 spaces. Is that really a better way, why use 8 characters when you can use one for the same purpose ? One could argue either way, maybe No, it uses 4 spaces for each indentation level, this is what the other files I checked (mostly qemu_*.c, xml.c) use and remote_internal.c even has: you just then use one tab for 2 indentation level, works just fine when I use vim on that file, really... /* * vim: set tabstop=4: * vim: set shiftwidth=4: * vim: set expandtab: */ which is basically what I used for reindenting virterror.c. So the patch is just to make things consistent with the rest of the c code. To me the patch I got replaced all tabs instead of using tabs for indenting of 2 levels. I also have tabstop=8 in my vim setup, that's rather classic. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] Patch for routed virtual networks
man, 24 03 2008 kl. 19:00 +, skrev Daniel P. Berrange: On Mon, Mar 24, 2008 at 10:52:41AM +0100, Mads Chr. Olesen wrote: Anything further I can do to help get this patch commited? I have been running with it, without problems across restarts, etc., for a couple of weeks now. I still don't see where the routing rules are defined / take place with this setup. There must be rules somewhere specifying the routing for the subnet 78.47.YYY.YYY/30, but its not being done in your patchset AFAICT Do you mean entries in the routing table, or? This is my routing table: $ route -n Kernel IP routing table Destination Gateway Genmask Flags Metric RefUse Iface 78.47.YYY.YYY 0.0.0.0 255.255.255.248 U 0 00 virsubnetbr0 85.10.XXX.0 0.0.0.0 255.255.255.224 U 0 00 eth0 0.0.0.0 85.10.XXX.1 0.0.0.0 UG10000 eth0 The entry for virsubnetbr0 is setup automatically, by the definition in the XML-file: ip address=78.47.YYY.YYY netmask=255.255.255.248 / -- Mads Chr. Olesen [EMAIL PROTECTED] shiyee.dk -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[Libvir] error : could not connect to xen:///; error: failed to connect to the hypervisor
I can't find the reason for the following error: # virsh libvir: error : could not connect to xen:/// error: failed to connect to the hypervisor # virsh -c ///var/lib/xend/xend-socket libvir: error : could not connect to ///var/lib/xend/xend-socket error: failed to connect to the hypervisor # lsof|grep xend-socket python 3655 root 18u unix 0x88001edce340 11774 /var/lib/xend/xend-socket system: Debian etch on amd64 Xen version 3.2.0 (compiled from sources) Linux 2.6.21 (patched Sources from redhat git) libvirtd and xend are running, `xm list` etc works fine # virsh -v 0.4.0 (installed from etch-backports) Output from `strace -s 1024 -f virsh list --all` is here: http://krohnjob.de/files/virsh.strace Can't see the reason for the above mentioned error in there... Kind regards, Felix Krohn -- Felix Krohn/ After silence, that which comes ] |- smtp, xmpp: [EMAIL PROTECTED] / nearest to expressing the inexpres-] |- gpg: 0x1C246E3B / sible is music. [Aldous Huxley] |- https://kro.hn / http://www.flickr.com/kro_royal ] -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] PATCH: Add SCSI HBA backend for storage driver
On Tue, Mar 25, 2008 at 03:24:48AM -0400, Daniel Veillard wrote:
On Mon, Mar 24, 2008 at 11:15:47PM +, Daniel P. Berrange wrote:
And this time with the patch included...
[...]
PARTED_REQUIRED=1.8.0
+HAL_REQUIRED=0.5.0
With HAL being in Solaris now, maybe that could even be made non-linux
specific
[...]
@@ -584,6 +585,8 @@ AC_ARG_WITH(storage-iscsi,
[ --with-storage-iscsiwith iSCSI backend for the storage driver
(on)],[],[with_storage_iscsi=check])
AC_ARG_WITH(storage-disk,
[ --with-storage-disk with GPartd Disk backend for the storage
driver (on)],[],[with_storage_disk=check])
+AC_ARG_WITH(storage-scsi,
+[ --with-storage-scsi with HAL SCSI Disk backend for the storage
driver (on)],[],[with_storage_scsi=check])
if test $with_storage_fs = yes -o $with_storage_fs = check; then
AC_PATH_PROG(MOUNT, [mount], [], [$PATH:/sbin:/usr/sbin])
@@ -741,6 +744,30 @@ AC_SUBST(LIBPARTED_CFLAGS)
AC_SUBST(LIBPARTED_LIBS)
maybe a meta option --with-storage giving a default to the group of options
(that now 5 of them) could be helpful for reduced setups.
Nah, you never need to use any of the --with-storage-XXX options in
normal circumstances. If you leave them out, it will automatically
probe and enable/disable as appropriate. You only need to use the
--with options if you want to force configure to abort if probing
fails.
[...]
diff -N src/storage_backend_scsi.c
[...]
+#define LINUX_SYSFS_SCSI_HOST_PREFIX /sys/class/scsi_host/
+#define LINUX_SYSFS_SCSI_HOST_POSTFIX /device
hopefully we can add Solaris equivalent later
Yeah, or change the way the Linux bit works so it sucks less. I'm
still undecided on this particular bit of code. I'm using sysfs
because I'll need to use it anyway when I add NPIV support. Then
again NPIV support may work out differently to how I expect - I'll
be getting access to some real NPIV hardware soon to verify...
+if (strlen(absLink) = buflen) {
+virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ link too long for buffer);
+return -1;
+}
+strcpy(buf, absLink);
Even if just checked can we still use strncpy(), i assume at some point
we will add an automatic check against strcpy
Sure, will do.
[...]
+virStorageBackendSCSICreateVol(virConnectPtr conn,
+ virStoragePoolObjPtr pool,
+ const char *name,
+ const char *path,
+ const char *key,
+ unsigned long long size)
+{
+virStorageVolDefPtr vol;
+char *tmppath;
+
+if ((vol = calloc(1, sizeof(*vol))) == NULL) {
+virStorageReportError(conn, VIR_ERR_NO_MEMORY, %s, _(volume));
+goto cleanup;
+}
maybe check that size is greater than some absolute minimum like 1 MByte
(and maximum size ?) this could also allow to catch some conversion errors
or 0 being passed.
This size value comes straight from HAL, as is the LUN size. The code
will re-probe this value a short while later when we open the actual
device node to lookup permissions/ownership, so I think its overkill
to check size here too.
+tmppath = strdup(path);
[...]
+if ((vol-target.path = virStorageBackendStablePath(conn,
+pool,
+tmppath)) == NULL)
+goto cleanup;
+
+if (tmppath != vol-target.path)
+free(tmppath);
+tmppath = NULL;
it's a bit funky, you mean virStorageBackendStablePath could just grab the
parameter string or not and that need to be checked later to decide if it
needs
freeing ? Even for an internal API it's a bit dangerous IMHO, what if
it is called with a constant string path, let's avoid the potential problem
get virStorageBackendStablePath to always strdup if it reuses the parameter,
no ?
Yeah, guess I should change the contract of that method. Its a little ugly.
All the other drivers work in this way though, so I'll do that cleanup as
a separate patch.
+goto cleanup;
+}
+
+#define HAL_GET_PROPERTY(path, name, err, type, value) \
+(value) = libhal_device_get_property_ ## type(ctx, (path), (name),
(err)); \
+if (dbus_error_is_set((err))) { \
+virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, \
+ unable to lookup '%s' property on %s: %s, \
+ (name), (path), derr.message);\
+goto cleanup; \
+}
i'm not too fond of macros defined in the function code, move it just before
Also for token-pasting I though one needed to glue the # and the identifiers
like foo##bar ,
